| Message ID | 20210719073456.28666-3-chuanjia.liu@mediatek.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: mediatek: Spilt PCIe node to comply with hardware design | expand |
On Mon, 2021-07-19 at 15:34 +0800, Chuanjia Liu wrote: > For the new dts format, add a new method to get > shared pcie-cfg base address and parse node. > > Signed-off-by: Chuanjia Liu <chuanjia.liu@mediatek.com> > Acked-by: Ryder Lee <ryder.lee@mediatek.com> > --- > drivers/pci/controller/pcie-mediatek.c | 52 +++++++++++++++++++------- > 1 file changed, 39 insertions(+), 13 deletions(-) > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > index 25bee693834f..928e0983a900 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; > @@ -650,7 +654,11 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port, > return err; > } > > - port->irq = platform_get_irq(pdev, port->slot); > + if (of_find_property(dev->of_node, "interrupt-names", NULL)) > + port->irq = platform_get_irq_byname(pdev, "pcie_irq"); > + else > + port->irq = platform_get_irq(pdev, port->slot); > + > if (port->irq < 0) > return port->irq; > > @@ -682,6 +690,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 +997,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 +1008,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) > @@ -1027,22 +1048,27 @@ static int mtk_pcie_setup(struct mtk_pcie *pcie) > struct device *dev = pcie->dev; > struct device_node *node = dev->of_node, *child; > struct mtk_pcie_port *port, *tmp; > - int err; > + int err, slot; > + > + slot = of_get_pci_domain_nr(dev->of_node); > + if (slot < 0) { > + for_each_available_child_of_node(node, child) { > + err = of_pci_get_devfn(child); > + if (err < 0) { > + dev_err(dev, "failed to get devfn: %d\n", err); > + goto error_put_node; > + } > > - for_each_available_child_of_node(node, child) { > - int slot; > + slot = PCI_SLOT(err); > > - err = of_pci_get_devfn(child); > - if (err < 0) { > - dev_err(dev, "failed to parse devfn: %d\n", err); > - goto error_put_node; > + err = mtk_pcie_parse_port(pcie, child, slot); > + if (err) > + goto error_put_node; > } > - > - slot = PCI_SLOT(err); > - > - err = mtk_pcie_parse_port(pcie, child, slot); > + } else { > + err = mtk_pcie_parse_port(pcie, node, slot); > if (err) > - goto error_put_node; > + return err; Hi，Rob I changed this in the v9 version: When the new dts format is used, of_node_get() is not called. So when mtk_pcie_parse_port fails, of_node_put don't need to be called. if you still ok for this, I will add R-b in next version. Best Regards > } > > err = mtk_pcie_subsys_powerup(pcie);
On Mon, 2021-07-19 at 15:34 +0800, Chuanjia Liu wrote: Gently ping... > For the new dts format, add a new method to get > shared pcie-cfg base address and parse node. > > Signed-off-by: Chuanjia Liu <chuanjia.liu@mediatek.com> > Acked-by: Ryder Lee <ryder.lee@mediatek.com> > --- > drivers/pci/controller/pcie-mediatek.c | 52 +++++++++++++++++++------- > 1 file changed, 39 insertions(+), 13 deletions(-) > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > index 25bee693834f..928e0983a900 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; > @@ -650,7 +654,11 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port, > return err; > } > > - port->irq = platform_get_irq(pdev, port->slot); > + if (of_find_property(dev->of_node, "interrupt-names", NULL)) > + port->irq = platform_get_irq_byname(pdev, "pcie_irq"); > + else > + port->irq = platform_get_irq(pdev, port->slot); > + > if (port->irq < 0) > return port->irq; > > @@ -682,6 +690,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 +997,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 +1008,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) > @@ -1027,22 +1048,27 @@ static int mtk_pcie_setup(struct mtk_pcie *pcie) > struct device *dev = pcie->dev; > struct device_node *node = dev->of_node, *child; > struct mtk_pcie_port *port, *tmp; > - int err; > + int err, slot; > + > + slot = of_get_pci_domain_nr(dev->of_node); > + if (slot < 0) { > + for_each_available_child_of_node(node, child) { > + err = of_pci_get_devfn(child); > + if (err < 0) { > + dev_err(dev, "failed to get devfn: %d\n", err); > + goto error_put_node; > + } > > - for_each_available_child_of_node(node, child) { > - int slot; > + slot = PCI_SLOT(err); > > - err = of_pci_get_devfn(child); > - if (err < 0) { > - dev_err(dev, "failed to parse devfn: %d\n", err); > - goto error_put_node; > + err = mtk_pcie_parse_port(pcie, child, slot); > + if (err) > + goto error_put_node; > } > - > - slot = PCI_SLOT(err); > - > - err = mtk_pcie_parse_port(pcie, child, slot); > + } else { > + err = mtk_pcie_parse_port(pcie, node, slot); > if (err) > - goto error_put_node; > + return err; > } > > err = mtk_pcie_subsys_powerup(pcie);
On Mon, Jul 19, 2021 at 8:59 PM Chuanjia Liu <chuanjia.liu@mediatek.com> wrote: > > On Mon, 2021-07-19 at 15:34 +0800, Chuanjia Liu wrote: > > For the new dts format, add a new method to get > > shared pcie-cfg base address and parse node. > > > > Signed-off-by: Chuanjia Liu <chuanjia.liu@mediatek.com> > > Acked-by: Ryder Lee <ryder.lee@mediatek.com> > > --- > > drivers/pci/controller/pcie-mediatek.c | 52 +++++++++++++++++++------- > > 1 file changed, 39 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > > index 25bee693834f..928e0983a900 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; > > @@ -650,7 +654,11 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port, > > return err; > > } > > > > - port->irq = platform_get_irq(pdev, port->slot); > > + if (of_find_property(dev->of_node, "interrupt-names", NULL)) > > + port->irq = platform_get_irq_byname(pdev, "pcie_irq"); > > + else > > + port->irq = platform_get_irq(pdev, port->slot); > > + > > if (port->irq < 0) > > return port->irq; > > > > @@ -682,6 +690,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 +997,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 +1008,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) > > @@ -1027,22 +1048,27 @@ static int mtk_pcie_setup(struct mtk_pcie *pcie) > > struct device *dev = pcie->dev; > > struct device_node *node = dev->of_node, *child; > > struct mtk_pcie_port *port, *tmp; > > - int err; > > + int err, slot; > > + > > + slot = of_get_pci_domain_nr(dev->of_node); > > + if (slot < 0) { > > + for_each_available_child_of_node(node, child) { > > + err = of_pci_get_devfn(child); > > + if (err < 0) { > > + dev_err(dev, "failed to get devfn: %d\n", err); > > + goto error_put_node; > > + } > > > > - for_each_available_child_of_node(node, child) { > > - int slot; > > + slot = PCI_SLOT(err); > > > > - err = of_pci_get_devfn(child); > > - if (err < 0) { > > - dev_err(dev, "failed to parse devfn: %d\n", err); > > - goto error_put_node; > > + err = mtk_pcie_parse_port(pcie, child, slot); > > + if (err) > > + goto error_put_node; > > } > > - > > - slot = PCI_SLOT(err); > > - > > - err = mtk_pcie_parse_port(pcie, child, slot); > > + } else { > > + err = mtk_pcie_parse_port(pcie, node, slot); > > if (err) > > - goto error_put_node; > > + return err; > > Hi，Rob > I changed this in the v9 version: > When the new dts format is used, of_node_get() is not called. > So when mtk_pcie_parse_port fails, of_node_put don't need to be called. > if you still ok for this, I will add R-b in next version. Yes, and that's small enough change to keep my R-b. Rob
On Tue, 2021-08-03 at 16:18 -0600, Rob Herring wrote: > On Mon, Jul 19, 2021 at 8:59 PM Chuanjia Liu < > chuanjia.liu@mediatek.com> wrote: > > > > On Mon, 2021-07-19 at 15:34 +0800, Chuanjia Liu wrote: > > > For the new dts format, add a new method to get > > > shared pcie-cfg base address and parse node. > > > > > > Signed-off-by: Chuanjia Liu <chuanjia.liu@mediatek.com> > > > Acked-by: Ryder Lee <ryder.lee@mediatek.com> > > > --- > > > drivers/pci/controller/pcie-mediatek.c | 52 +++++++++++++++++++- > > > ------ > > > 1 file changed, 39 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/pci/controller/pcie-mediatek.c > > > b/drivers/pci/controller/pcie-mediatek.c > > > index 25bee693834f..928e0983a900 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; > > > @@ -650,7 +654,11 @@ static int mtk_pcie_setup_irq(struct > > > mtk_pcie_port *port, > > > return err; > > > } > > > > > > - port->irq = platform_get_irq(pdev, port->slot); > > > + if (of_find_property(dev->of_node, "interrupt-names", > > > NULL)) > > > + port->irq = platform_get_irq_byname(pdev, > > > "pcie_irq"); > > > + else > > > + port->irq = platform_get_irq(pdev, port->slot); > > > + > > > if (port->irq < 0) > > > return port->irq; > > > > > > @@ -682,6 +690,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 +997,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 +1008,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) > > > @@ -1027,22 +1048,27 @@ static int mtk_pcie_setup(struct mtk_pcie > > > *pcie) > > > struct device *dev = pcie->dev; > > > struct device_node *node = dev->of_node, *child; > > > struct mtk_pcie_port *port, *tmp; > > > - int err; > > > + int err, slot; > > > + > > > + slot = of_get_pci_domain_nr(dev->of_node); > > > + if (slot < 0) { > > > + for_each_available_child_of_node(node, child) { > > > + err = of_pci_get_devfn(child); > > > + if (err < 0) { > > > + dev_err(dev, "failed to get devfn: > > > %d\n", err); > > > + goto error_put_node; > > > + } > > > > > > - for_each_available_child_of_node(node, child) { > > > - int slot; > > > + slot = PCI_SLOT(err); > > > > > > - err = of_pci_get_devfn(child); > > > - if (err < 0) { > > > - dev_err(dev, "failed to parse devfn: %d\n", > > > err); > > > - goto error_put_node; > > > + err = mtk_pcie_parse_port(pcie, child, > > > slot); > > > + if (err) > > > + goto error_put_node; > > > } > > > - > > > - slot = PCI_SLOT(err); > > > - > > > - err = mtk_pcie_parse_port(pcie, child, slot); > > > + } else { > > > + err = mtk_pcie_parse_port(pcie, node, slot); > > > if (err) > > > - goto error_put_node; > > > + return err; > > > > Hi，Rob > > I changed this in the v9 version: > > When the new dts format is used, of_node_get() is not called. > > So when mtk_pcie_parse_port fails, of_node_put don't need to be > > called. > > if you still ok for this, I will add R-b in next version. > > Yes, and that's small enough change to keep my R-b. > > Rob Hi, Rob Thanks for your review. Hi lorenzo If there are no more comments, can this series be merged? Best Regards
On Mon, Jul 19, 2021 at 03:34:54PM +0800, Chuanjia Liu wrote: > For the new dts format, add a new method to get > shared pcie-cfg base address and parse node. This commit log doesn't seem to really cover what's going on here. It looks like: - You added a check for "mediatek,generic-pciecfg" (I guess this is the "shared pcie-cfg base address" part). Probably could have been its own patch. - You added checks for "interrupt-names" and "pcie_irq". Not explained in commit log; probably could have been its own patch, too. - You now look for "linux,pci-domain" (via of_get_pci_domain_nr()). If present, you parse only one port instead of looking for all the children of the node. That's sort of weird behavior -- why should the presence of "linux,pci-domain" determine whether the node can have children? Is that really what you intend? Should be explained in the commit log and could have been its own patch, too. > Signed-off-by: Chuanjia Liu <chuanjia.liu@mediatek.com> > Acked-by: Ryder Lee <ryder.lee@mediatek.com> > --- > drivers/pci/controller/pcie-mediatek.c | 52 +++++++++++++++++++------- > 1 file changed, 39 insertions(+), 13 deletions(-) > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > index 25bee693834f..928e0983a900 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; > @@ -650,7 +654,11 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port, > return err; > } > > - port->irq = platform_get_irq(pdev, port->slot); > + if (of_find_property(dev->of_node, "interrupt-names", NULL)) > + port->irq = platform_get_irq_byname(pdev, "pcie_irq"); > + else > + port->irq = platform_get_irq(pdev, port->slot); > + > if (port->irq < 0) > return port->irq; > > @@ -682,6 +690,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 +997,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 +1008,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) > @@ -1027,22 +1048,27 @@ static int mtk_pcie_setup(struct mtk_pcie *pcie) > struct device *dev = pcie->dev; > struct device_node *node = dev->of_node, *child; > struct mtk_pcie_port *port, *tmp; > - int err; > + int err, slot; > + > + slot = of_get_pci_domain_nr(dev->of_node); > + if (slot < 0) { > + for_each_available_child_of_node(node, child) { > + err = of_pci_get_devfn(child); > + if (err < 0) { > + dev_err(dev, "failed to get devfn: %d\n", err); > + goto error_put_node; > + } > > - for_each_available_child_of_node(node, child) { > - int slot; > + slot = PCI_SLOT(err); > > - err = of_pci_get_devfn(child); > - if (err < 0) { > - dev_err(dev, "failed to parse devfn: %d\n", err); > - goto error_put_node; > + err = mtk_pcie_parse_port(pcie, child, slot); > + if (err) > + goto error_put_node; > } > - > - slot = PCI_SLOT(err); > - > - err = mtk_pcie_parse_port(pcie, child, slot); > + } else { > + err = mtk_pcie_parse_port(pcie, node, slot); > if (err) > - goto error_put_node; > + return err; > } > > err = mtk_pcie_subsys_powerup(pcie); > -- > 2.18.0 >
On Tue, Aug 10, 2021 at 02:42:50PM -0500, Bjorn Helgaas wrote: > On Mon, Jul 19, 2021 at 03:34:54PM +0800, Chuanjia Liu wrote: > > For the new dts format, add a new method to get > > shared pcie-cfg base address and parse node. > > This commit log doesn't seem to really cover what's going on here. It > looks like: > > - You added a check for "mediatek,generic-pciecfg" (I guess this is > the "shared pcie-cfg base address" part). Probably could have > been its own patch. > > - You added checks for "interrupt-names" and "pcie_irq". Not > explained in commit log; probably could have been its own patch, > too. > > - You now look for "linux,pci-domain" (via of_get_pci_domain_nr()). > If present, you parse only one port instead of looking for all the > children of the node. > > That's sort of weird behavior -- why should the presence of > "linux,pci-domain" determine whether the node can have children? > Is that really what you intend? > > Should be explained in the commit log and could have been its own > patch, too. I agree with Bjorn, this patch should be split (and commit logs rewritten). I will drop it from my tree, waiting for a v12. Lorenzo > > Signed-off-by: Chuanjia Liu <chuanjia.liu@mediatek.com> > > Acked-by: Ryder Lee <ryder.lee@mediatek.com> > > --- > > drivers/pci/controller/pcie-mediatek.c | 52 +++++++++++++++++++------- > > 1 file changed, 39 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > > index 25bee693834f..928e0983a900 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; > > @@ -650,7 +654,11 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port, > > return err; > > } > > > > - port->irq = platform_get_irq(pdev, port->slot); > > + if (of_find_property(dev->of_node, "interrupt-names", NULL)) > > + port->irq = platform_get_irq_byname(pdev, "pcie_irq"); > > + else > > + port->irq = platform_get_irq(pdev, port->slot); > > + > > if (port->irq < 0) > > return port->irq; > > > > @@ -682,6 +690,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 +997,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 +1008,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) > > @@ -1027,22 +1048,27 @@ static int mtk_pcie_setup(struct mtk_pcie *pcie) > > struct device *dev = pcie->dev; > > struct device_node *node = dev->of_node, *child; > > struct mtk_pcie_port *port, *tmp; > > - int err; > > + int err, slot; > > + > > + slot = of_get_pci_domain_nr(dev->of_node); > > + if (slot < 0) { > > + for_each_available_child_of_node(node, child) { > > + err = of_pci_get_devfn(child); > > + if (err < 0) { > > + dev_err(dev, "failed to get devfn: %d\n", err); > > + goto error_put_node; > > + } > > > > - for_each_available_child_of_node(node, child) { > > - int slot; > > + slot = PCI_SLOT(err); > > > > - err = of_pci_get_devfn(child); > > - if (err < 0) { > > - dev_err(dev, "failed to parse devfn: %d\n", err); > > - goto error_put_node; > > + err = mtk_pcie_parse_port(pcie, child, slot); > > + if (err) > > + goto error_put_node; > > } > > - > > - slot = PCI_SLOT(err); > > - > > - err = mtk_pcie_parse_port(pcie, child, slot); > > + } else { > > + err = mtk_pcie_parse_port(pcie, node, slot); > > if (err) > > - goto error_put_node; > > + return err; > > } > > > > err = mtk_pcie_subsys_powerup(pcie); > > -- > > 2.18.0 > >
On Fri, 2021-08-13 at 16:22 +0100, Lorenzo Pieralisi wrote: > On Tue, Aug 10, 2021 at 02:42:50PM -0500, Bjorn Helgaas wrote: > > On Mon, Jul 19, 2021 at 03:34:54PM +0800, Chuanjia Liu wrote: > > > For the new dts format, add a new method to get > > > shared pcie-cfg base address and parse node. > > > > This commit log doesn't seem to really cover what's going on > > here. It > > looks like: > > > > - You added a check for "mediatek,generic-pciecfg" (I guess this > > is > > the "shared pcie-cfg base address" part). Probably could have > > been its own patch. > > > > - You added checks for "interrupt-names" and "pcie_irq". Not > > explained in commit log; probably could have been its own > > patch, > > too. > > > > - You now look for "linux,pci-domain" (via > > of_get_pci_domain_nr()). > > If present, you parse only one port instead of looking for all > > the > > children of the node. > > > > That's sort of weird behavior -- why should the presence of > > "linux,pci-domain" determine whether the node can have > > children? > > Is that really what you intend? > > > > Should be explained in the commit log and could have been its > > own > > patch, too. hi, Bjorn, Yes, this is my intention,"Linux,pci-domain" has two purposes 1)Distinguish the old and new dts formats, then parse them 2)Determine the current pcie slot num in new dts format, because the offset of some regs needs to be based on slot num I will split this patch into three patches, and then explain them in commit log, thanks for your comment. > > I agree with Bjorn, this patch should be split (and commit logs > rewritten). I will drop it from my tree, waiting for a v12. > > Lorenzo hi, Lorenzo Thanks for your confirmation, I will send the V12 version as soon as possible. Chuanjia > > > > Signed-off-by: Chuanjia Liu <chuanjia.liu@mediatek.com> > > > Acked-by: Ryder Lee <ryder.lee@mediatek.com> > > > --- > > > drivers/pci/controller/pcie-mediatek.c | 52 +++++++++++++++++++- > > > ------ > > > 1 file changed, 39 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/pci/controller/pcie-mediatek.c > > > b/drivers/pci/controller/pcie-mediatek.c > > > index 25bee693834f..928e0983a900 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; > > > @@ -650,7 +654,11 @@ static int mtk_pcie_setup_irq(struct > > > mtk_pcie_port *port, > > > return err; > > > } > > > > > > - port->irq = platform_get_irq(pdev, port->slot); > > > + if (of_find_property(dev->of_node, "interrupt-names", NULL)) > > > + port->irq = platform_get_irq_byname(pdev, "pcie_irq"); > > > + else > > > + port->irq = platform_get_irq(pdev, port->slot); > > > + > > > if (port->irq < 0) > > > return port->irq; > > > > > > @@ -682,6 +690,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 +997,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 +1008,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) > > > @@ -1027,22 +1048,27 @@ static int mtk_pcie_setup(struct mtk_pcie > > > *pcie) > > > struct device *dev = pcie->dev; > > > struct device_node *node = dev->of_node, *child; > > > struct mtk_pcie_port *port, *tmp; > > > - int err; > > > + int err, slot; > > > + > > > + slot = of_get_pci_domain_nr(dev->of_node); > > > + if (slot < 0) { > > > + for_each_available_child_of_node(node, child) { > > > + err = of_pci_get_devfn(child); > > > + if (err < 0) { > > > + dev_err(dev, "failed to get devfn: > > > %d\n", err); > > > + goto error_put_node; > > > + } > > > > > > - for_each_available_child_of_node(node, child) { > > > - int slot; > > > + slot = PCI_SLOT(err); > > > > > > - err = of_pci_get_devfn(child); > > > - if (err < 0) { > > > - dev_err(dev, "failed to parse devfn: %d\n", > > > err); > > > - goto error_put_node; > > > + err = mtk_pcie_parse_port(pcie, child, slot); > > > + if (err) > > > + goto error_put_node; > > > } > > > - > > > - slot = PCI_SLOT(err); > > > - > > > - err = mtk_pcie_parse_port(pcie, child, slot); > > > + } else { > > > + err = mtk_pcie_parse_port(pcie, node, slot); > > > if (err) > > > - goto error_put_node; > > > + return err; > > > } > > > > > > err = mtk_pcie_subsys_powerup(pcie); > > > -- > > > 2.18.0 > > >
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c index 25bee693834f..928e0983a900 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; @@ -650,7 +654,11 @@ static int mtk_pcie_setup_irq(struct mtk_pcie_port *port, return err; } - port->irq = platform_get_irq(pdev, port->slot); + if (of_find_property(dev->of_node, "interrupt-names", NULL)) + port->irq = platform_get_irq_byname(pdev, "pcie_irq"); + else + port->irq = platform_get_irq(pdev, port->slot); + if (port->irq < 0) return port->irq; @@ -682,6 +690,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 +997,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 +1008,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) @@ -1027,22 +1048,27 @@ static int mtk_pcie_setup(struct mtk_pcie *pcie) struct device *dev = pcie->dev; struct device_node *node = dev->of_node, *child; struct mtk_pcie_port *port, *tmp; - int err; + int err, slot; + + slot = of_get_pci_domain_nr(dev->of_node); + if (slot < 0) { + for_each_available_child_of_node(node, child) { + err = of_pci_get_devfn(child); + if (err < 0) { + dev_err(dev, "failed to get devfn: %d\n", err); + goto error_put_node; + } - for_each_available_child_of_node(node, child) { - int slot; + slot = PCI_SLOT(err); - err = of_pci_get_devfn(child); - if (err < 0) { - dev_err(dev, "failed to parse devfn: %d\n", err); - goto error_put_node; + err = mtk_pcie_parse_port(pcie, child, slot); + if (err) + goto error_put_node; } - - slot = PCI_SLOT(err); - - err = mtk_pcie_parse_port(pcie, child, slot); + } else { + err = mtk_pcie_parse_port(pcie, node, slot); if (err) - goto error_put_node; + return err; } err = mtk_pcie_subsys_powerup(pcie);