Re: [PATCH v12 2/6] PCI: mediatek: Add new method to get shared pcie-cfg base address

[Chuanjia Liu]

On Fri, 2021-08-27 at 11:46 -0500, Bjorn Helgaas wrote:
> On Mon, Aug 23, 2021 at 11:27:56AM +0800, Chuanjia Liu wrote:
> > For the new dts format, add a new method to get
> > shared pcie-cfg base address and use it to configure
> > the PCIECFG controller
> 
> Rewrap this to fill 75 columns.
> 
> > Signed-off-by: Chuanjia Liu <chuanjia.liu@xxxxxxxxxxxx>
> > Acked-by: Ryder Lee <ryder.lee@xxxxxxxxxxxx>
> > ---
> >  drivers/pci/controller/pcie-mediatek.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek.c
> > b/drivers/pci/controller/pcie-mediatek.c
> > index 25bee693834f..4296d9e04240 100644
> > --- a/drivers/pci/controller/pcie-mediatek.c
> > +++ b/drivers/pci/controller/pcie-mediatek.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/irqchip/chained_irq.h>
> >  #include <linux/irqdomain.h>
> >  #include <linux/kernel.h>
> > +#include <linux/mfd/syscon.h>
> >  #include <linux/msi.h>
> >  #include <linux/module.h>
> >  #include <linux/of_address.h>
> > @@ -23,6 +24,7 @@
> >  #include <linux/phy/phy.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> >  #include <linux/reset.h>
> >  
> >  #include "../pci.h"
> > @@ -207,6 +209,7 @@ struct mtk_pcie_port {
> >   * struct mtk_pcie - PCIe host information
> >   * @dev: pointer to PCIe device
> >   * @base: IO mapped register base
> > + * @cfg: IO mapped register map for PCIe config
> >   * @free_ck: free-run reference clock
> >   * @mem: non-prefetchable memory resource
> >   * @ports: pointer to PCIe port information
> > @@ -215,6 +218,7 @@ struct mtk_pcie_port {
> >  struct mtk_pcie {
> >  	struct device *dev;
> >  	void __iomem *base;
> > +	struct regmap *cfg;
> >  	struct clk *free_ck;
> >  
> >  	struct list_head ports;
> > @@ -682,6 +686,10 @@ static int mtk_pcie_startup_port_v2(struct
> > mtk_pcie_port *port)
> >  		val |= PCIE_CSR_LTSSM_EN(port->slot) |
> >  		       PCIE_CSR_ASPM_L1_EN(port->slot);
> >  		writel(val, pcie->base + PCIE_SYS_CFG_V2);
> > +	} else if (pcie->cfg) {
> > +		val = PCIE_CSR_LTSSM_EN(port->slot) |
> > +		      PCIE_CSR_ASPM_L1_EN(port->slot);
> > +		regmap_update_bits(pcie->cfg, PCIE_SYS_CFG_V2, val,
> > val);
> >  	}
> >  
> >  	/* Assert all reset signals */
> > @@ -985,6 +993,7 @@ static int mtk_pcie_subsys_powerup(struct
> > mtk_pcie *pcie)
> >  	struct device *dev = pcie->dev;
> >  	struct platform_device *pdev = to_platform_device(dev);
> >  	struct resource *regs;
> > +	struct device_node *cfg_node;
> >  	int err;
> >  
> >  	/* get shared registers, which are optional */
> > @@ -995,6 +1004,14 @@ static int mtk_pcie_subsys_powerup(struct
> > mtk_pcie *pcie)
> >  			return PTR_ERR(pcie->base);
> >  	}
> >  
> > +	cfg_node = of_find_compatible_node(NULL, NULL,
> > +					   "mediatek,generic-pciecfg");
> 
> This looks wrong to me.  IIUC, since we start at NULL, this searches
> the entire device tree for any node with
> 
>   compatible = "mediatek,generic-pciecfg"
> 
> but we should only care about the specific device/node this driver
> claimed.
> 
> Should this be part of the match data, i.e., struct mtk_pcie_soc?
Hi Bjorn,

Thanks for your review.

Many drivers in the drivers/ folder use compatible to search directly.
 
I guess that when different devices need to search for different nodes,
it is necessary to use match data to determine the current node 
information that needs to be searched.

But in the devices supported by this driver, I guess it no need 
through match data to confirm the compatible information，because it 
always search same compatible "mediatek, generic-pciecfg"

> 
> > +	if (cfg_node) {
> > +		pcie->cfg = syscon_node_to_regmap(cfg_node);
> 
> Other drivers in drivers/pci/controller/ use
> syscon_regmap_lookup_by_phandle() (j721e, dra7xx, keystone,
> layerscape, artpec6) or syscon_regmap_lookup_by_compatible() (imx6,
> kirin, v3-semi).
> 
> You should do it the same way unless there's a need to be different.
I have used phandle, but Rob suggested to search for the node by 
compatible. The reason why syscon_regmap_lookup_by_compatible() is not 
used here is that the pciecfg node is optional, and there is no need to
return error when the node is not searched.

> It's also nice if you can use the same struct member name
> ("mtk_pcie.cfg") as other drivers.  They're not all consistent, but I
> don't see any other "cfg".
Other drivers struct member name are also different.
For example, scfg(layerscape)，map(v3-semi), crgctrl(kirin),
regmap(artpec6).Or just use local variables, named syscon or regmap.
So I couldn't be consistent with other drivers.

Thanks
Chuanjia
> 
> > +		if (IS_ERR(pcie->cfg))
> > +			return PTR_ERR(pcie->cfg);
> > +	}
> > +
> >  	pcie->free_ck = devm_clk_get(dev, "free_ck");
> >  	if (IS_ERR(pcie->free_ck)) {
> >  		if (PTR_ERR(pcie->free_ck) == -EPROBE_DEFER)
> > -- 
> > 2.18.0
> >