| Message ID | 20210823032800.1660-3-chuanjia.liu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI: mediatek: Spilt PCIe node to comply with hardware design | expand |
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@mediatek.com> > Acked-by: Ryder Lee <ryder.lee@mediatek.com> > --- > 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? > + 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. 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". > + 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 >
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@mediatek.com> > > Acked-by: Ryder Lee <ryder.lee@mediatek.com> > > --- > > 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 > >
On Mon, Aug 30, 2021 at 03:09:44PM +0800, Chuanjia Liu wrote: > 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: > > > @@ -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"); > > > + 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. How about this? regmap = syscon_regmap_lookup_by_compatible("mediatek,generic-pciecfg"); if (!IS_ERR(regmap)) pcie->cfg = regmap;
On Mon, 2021-08-30 at 16:43 -0500, Bjorn Helgaas wrote: > On Mon, Aug 30, 2021 at 03:09:44PM +0800, Chuanjia Liu wrote: > > 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: > > > > @@ -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"); > > > > + 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. > > How about this? > > regmap = syscon_regmap_lookup_by_compatible("mediatek,generic- > pciecfg"); > if (!IS_ERR(regmap)) > pcie->cfg = regmap; Hi Bjorn, We need to deal with three situations 1) No error 2) The error of the node not found, don't do anything 3) Other errors, return errors I guess you mean regmap = syscon_regmap_lookup_by_compatible("mediatek,generic- pciecfg"); if (!IS_ERR(regmap)) pcie->cfg = regmap; else if (IS_ERR(regmap) && PTR_ERR(regmap) != -ENODEV) return PTR_ERR(regmap); I'm not sure if we need this, it seems a little weird and there are many drivers in other subsystems that use syscon_node_to_regmap(). Thanks Chuanjia
On Fri, Aug 27, 2021 at 11:46 AM Bjorn Helgaas <helgaas@kernel.org> 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@mediatek.com> > > Acked-by: Ryder Lee <ryder.lee@mediatek.com> > > --- > > 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? What would you put in match data exactly? The other way to do this is to have a DT property with the phandle which people like to do (have everything in the node 'for their driver'). If there's only 1 possible node (which is almost always the case), then there is little benefit to having another property. It's just redundant data. A phandle lookup might be a bit faster with the caching we do, but on a miss it would still walk all nodes. The other thing with these 'extra register bits to twiddle' is that they tend to be SoC specific and change from chip to chip, so either way is not very portable. The real question to ask is should there be a standard interface used or created. > > > + 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). There's no phandle to use in this case. As above, I'm trying to break people of this habit. Rob
On Mon, Aug 30, 2021 at 10:31 PM Chuanjia Liu <chuanjia.liu@mediatek.com> wrote: > > On Mon, 2021-08-30 at 16:43 -0500, Bjorn Helgaas wrote: > > On Mon, Aug 30, 2021 at 03:09:44PM +0800, Chuanjia Liu wrote: > > > 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: > > > > > @@ -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"); > > > > > + 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. > > > > How about this? > > > > regmap = syscon_regmap_lookup_by_compatible("mediatek,generic- > > pciecfg"); > > if (!IS_ERR(regmap)) > > pcie->cfg = regmap; +1 > > Hi Bjorn, > > We need to deal with three situations > 1) No error > 2) The error of the node not found, don't do anything > 3) Other errors, return errors > > I guess you mean > > regmap = syscon_regmap_lookup_by_compatible("mediatek,generic- > pciecfg"); > if (!IS_ERR(regmap)) > pcie->cfg = regmap; > else if (IS_ERR(regmap) && PTR_ERR(regmap) != -ENODEV) You already know IS_ERR is true here. > return PTR_ERR(regmap); syscon_regmap_lookup_by_compatible_optional is the function you are looking for. The _optional flavor doesn't exist, so create it. There is one for the phandle lookup. > > I'm not sure if we need this, it seems a little weird and there are > many drivers in other subsystems that use syscon_node_to_regmap(). You are implementing the exact same sequence that syscon_regmap_lookup_by_compatible() does, so clearly you should be using it. The one difference is you forgot the of_node_put(). Rob
On Tue, Aug 31, 2021 at 10:04:40AM -0500, Rob Herring wrote: > On Fri, Aug 27, 2021 at 11:46 AM Bjorn Helgaas <helgaas@kernel.org> 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@mediatek.com> > > > Acked-by: Ryder Lee <ryder.lee@mediatek.com> > > > --- > > > 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? > > What would you put in match data exactly? > > The other way to do this is to have a DT property with the phandle > which people like to do (have everything in the node 'for their > driver'). If there's only 1 possible node (which is almost always the > case), then there is little benefit to having another property. It's > just redundant data. A phandle lookup might be a bit faster with the > caching we do, but on a miss it would still walk all nodes. > > The other thing with these 'extra register bits to twiddle' is that > they tend to be SoC specific and change from chip to chip, so either > way is not very portable. The real question to ask is should there be > a standard interface used or created. > > > > + 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). > > There's no phandle to use in this case. As above, I'm trying to break > people of this habit. Thanks! I was mistaken in lots of ways here. I first assumed "mediatek,generic-pciecfg" was local to the pcie node, but that's not true. Then I thought there might be an ownership issue because the regmap is not local to the device, and several drivers look up and use the same regmap. But it looks like regmap provides some internal locking which mitigates most or all of that concern. Just to check -- you prefer syscon_regmap_lookup_by_compatible() over syscon_regmap_lookup_by_phandle()?
On Tue, Aug 31, 2021 at 10:47 AM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Tue, Aug 31, 2021 at 10:04:40AM -0500, Rob Herring wrote: > > On Fri, Aug 27, 2021 at 11:46 AM Bjorn Helgaas <helgaas@kernel.org> 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@mediatek.com> > > > > Acked-by: Ryder Lee <ryder.lee@mediatek.com> > > > > --- > > > > 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? > > > > What would you put in match data exactly? > > > > The other way to do this is to have a DT property with the phandle > > which people like to do (have everything in the node 'for their > > driver'). If there's only 1 possible node (which is almost always the > > case), then there is little benefit to having another property. It's > > just redundant data. A phandle lookup might be a bit faster with the > > caching we do, but on a miss it would still walk all nodes. > > > > The other thing with these 'extra register bits to twiddle' is that > > they tend to be SoC specific and change from chip to chip, so either > > way is not very portable. The real question to ask is should there be > > a standard interface used or created. > > > > > > + 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). > > > > There's no phandle to use in this case. As above, I'm trying to break > > people of this habit. > > Thanks! I was mistaken in lots of ways here. I first assumed > "mediatek,generic-pciecfg" was local to the pcie node, but that's not > true. Then I thought there might be an ownership issue because the > regmap is not local to the device, and several drivers look up and use > the same regmap. But it looks like regmap provides some internal > locking which mitigates most or all of that concern. > > Just to check -- you prefer syscon_regmap_lookup_by_compatible() over > syscon_regmap_lookup_by_phandle()? Yes, unless there is either more than 1 instance of the syscon or some additional data specific to the user (e.g. a register offset). Rob
On Tue, 2021-08-31 at 10:17 -0500, Rob Herring wrote: > On Mon, Aug 30, 2021 at 10:31 PM Chuanjia Liu < > chuanjia.liu@mediatek.com> wrote: > > > > On Mon, 2021-08-30 at 16:43 -0500, Bjorn Helgaas wrote: > > > On Mon, Aug 30, 2021 at 03:09:44PM +0800, Chuanjia Liu wrote: > > > > 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: > > > > > > @@ -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,generi > > > > > > c- > > > > > > 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. > > > > > > How about this? > > > > > > regmap = syscon_regmap_lookup_by_compatible("mediatek,generic- > > > pciecfg"); > > > if (!IS_ERR(regmap)) > > > pcie->cfg = regmap; > > +1 > > > > > Hi Bjorn, > > > > We need to deal with three situations > > 1) No error > > 2) The error of the node not found, don't do anything > > 3) Other errors, return errors > > > > I guess you mean > > > > regmap = syscon_regmap_lookup_by_compatible("mediatek,generic- > > pciecfg"); > > if (!IS_ERR(regmap)) > > pcie->cfg = regmap; > > else if (IS_ERR(regmap) && PTR_ERR(regmap) != -ENODEV) > > You already know IS_ERR is true here. > > > return PTR_ERR(regmap); > > syscon_regmap_lookup_by_compatible_optional is the function you are > looking for. The _optional flavor doesn't exist, so create it. There > is one for the phandle lookup. > > > > > I'm not sure if we need this, it seems a little weird and there are > > many drivers in other subsystems that use syscon_node_to_regmap(). > > You are implementing the exact same sequence that > syscon_regmap_lookup_by_compatible() does, so clearly you should be > using it. The one difference is you forgot the of_node_put(). Hi,Rob Thanks for your explanation. You're right. Now I understand use syscon_regmap_lookup_by_compatible() is a better way. I'll follow your advice regmap = syscon_regmap_lookup_by_compatible("mediatek,generic- pciecfg"); if (!IS_ERR(regmap)) pcie->cfg = regmap; else if (PTR_ERR(regmap) != -ENODEV) return PTR_ERR(regmap); Best regards Chuanjia > > Rob > > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek
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"); + if (cfg_node) { + pcie->cfg = syscon_node_to_regmap(cfg_node); + 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)