| Message ID | 20210510141509.929120-3-l.stach@pengutronix.de |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | None | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 53 lines checked |
On Mon, May 10, 2021 at 04:15:05PM +0200, Lucas Stach wrote: > We don't need to have a phandle of the PHY, as we know the compatible > of the node we are looking for. This will make it easier to put add > more PHY handling for new generations later on, where the > "fsl,imx7d-pcie-phy" phandle would be a misnomer. > > Also we can use a helper function to get the resource for us, > simplifying out driver code a bit. Better yes, but really all the phy handling should be split out to its own driver even in the older h/w with shared phy registers. Soon as there's a chip with 2 PCI hosts, you're going to need the phy binding. > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > --- > .../devicetree/bindings/pci/fsl,imx6q-pcie.txt | 5 ++--- > drivers/pci/controller/dwc/pci-imx6.c | 17 +++++------------ > 2 files changed, 7 insertions(+), 15 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > index de4b2baf91e8..308540df99ef 100644 > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > @@ -54,7 +54,6 @@ Additional required properties for imx7d-pcie and imx8mq-pcie: > - "pciephy" > - "apps" > - "turnoff" > -- fsl,imx7d-pcie-phy: A phandle to an fsl,imx7d-pcie-phy node. > > Additional required properties for imx8mq-pcie: > - clock-names: Must include the following additional entries: > @@ -88,8 +87,8 @@ Example: > > * Freescale i.MX7d PCIe PHY > > -This is the PHY associated with the IMX7d PCIe controller. It's used by the > -PCI-e controller via the fsl,imx7d-pcie-phy phandle. > +This is the PHY associated with the IMX7d PCIe controller. It's looked up by > +the PCI-e controller via the fsl,imx7d-pcie-phy compatible. > > Required properties: > - compatible: > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 922c14361cd3..5e13758222e8 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -555,7 +555,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) > writel(PCIE_PHY_CMN_REG26_ATT_MODE, > imx6_pcie->phy_base + PCIE_PHY_CMN_REG26); > } else { > - dev_warn(dev, "Unable to apply ERR010728 workaround. DT missing fsl,imx7d-pcie-phy phandle ?\n"); > + dev_warn(dev, "Unable to apply ERR010728 workaround. DT missing fsl,imx7d-pcie-phy node?\n"); > } > > imx7d_pcie_wait_for_phy_pll_lock(imx6_pcie); > @@ -970,7 +970,7 @@ static int imx6_pcie_probe(struct platform_device *pdev) > struct device *dev = &pdev->dev; > struct dw_pcie *pci; > struct imx6_pcie *imx6_pcie; > - struct device_node *np; > + struct device_node *np = NULL; > struct resource *dbi_base; > struct device_node *node = dev->of_node; > int ret; > @@ -991,17 +991,10 @@ static int imx6_pcie_probe(struct platform_device *pdev) > imx6_pcie->pci = pci; > imx6_pcie->drvdata = of_device_get_match_data(dev); > > - /* Find the PHY if one is defined, only imx7d uses it */ > - np = of_parse_phandle(node, "fsl,imx7d-pcie-phy", 0); > + /* Find the PHY if one is present in DT, only imx7d uses it */ > + np = of_find_compatible_node(NULL, NULL, "fsl,imx7d-pcie-phy"); > if (np) { > - struct resource res; > - > - ret = of_address_to_resource(np, 0, &res); > - if (ret) { > - dev_err(dev, "Unable to map PCIe PHY\n"); > - return ret; > - } > - imx6_pcie->phy_base = devm_ioremap_resource(dev, &res); > + imx6_pcie->phy_base = devm_of_iomap(dev, np, 0, NULL); > if (IS_ERR(imx6_pcie->phy_base)) { > dev_err(dev, "Unable to map PCIe PHY\n"); > return PTR_ERR(imx6_pcie->phy_base); > -- > 2.29.2 >
Am Montag, dem 10.05.2021 um 12:05 -0500 schrieb Rob Herring: > On Mon, May 10, 2021 at 04:15:05PM +0200, Lucas Stach wrote: > > We don't need to have a phandle of the PHY, as we know the compatible > > of the node we are looking for. This will make it easier to put add > > more PHY handling for new generations later on, where the > > "fsl,imx7d-pcie-phy" phandle would be a misnomer. > > > > Also we can use a helper function to get the resource for us, > > simplifying out driver code a bit. > > Better yes, but really all the phy handling should be split out to > its own driver even in the older h/w with shared phy registers. > That would be a quite massive DT binding changing break, possibly even a separate driver. Maybe it's time to do this for i.MX8MM, as the current driver just kept piling on special cases for "almost the same" hardware that by now looks quite different to the original i.MX6 PCIe integration this driver was supposed to handle. > Soon as there's a chip with 2 PCI hosts, you're going to need the phy > binding. Uh, there even is a chip like that already (i.MX8MQ), it just happened to not require any handling of the PHY registers. Don't know why I didn't think of this. :/ Regards, Lucas > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de> > > --- > > .../devicetree/bindings/pci/fsl,imx6q-pcie.txt | 5 ++--- > > drivers/pci/controller/dwc/pci-imx6.c | 17 +++++------------ > > 2 files changed, 7 insertions(+), 15 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > > index de4b2baf91e8..308540df99ef 100644 > > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > > @@ -54,7 +54,6 @@ Additional required properties for imx7d-pcie and imx8mq-pcie: > > - "pciephy" > > - "apps" > > - "turnoff" > > -- fsl,imx7d-pcie-phy: A phandle to an fsl,imx7d-pcie-phy node. > > > > Additional required properties for imx8mq-pcie: > > - clock-names: Must include the following additional entries: > > @@ -88,8 +87,8 @@ Example: > > > > * Freescale i.MX7d PCIe PHY > > > > -This is the PHY associated with the IMX7d PCIe controller. It's used by the > > -PCI-e controller via the fsl,imx7d-pcie-phy phandle. > > +This is the PHY associated with the IMX7d PCIe controller. It's looked up by > > +the PCI-e controller via the fsl,imx7d-pcie-phy compatible. > > > > Required properties: > > - compatible: > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > > index 922c14361cd3..5e13758222e8 100644 > > --- a/drivers/pci/controller/dwc/pci-imx6.c > > +++ b/drivers/pci/controller/dwc/pci-imx6.c > > @@ -555,7 +555,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) > > writel(PCIE_PHY_CMN_REG26_ATT_MODE, > > imx6_pcie->phy_base + PCIE_PHY_CMN_REG26); > > } else { > > - dev_warn(dev, "Unable to apply ERR010728 workaround. DT missing fsl,imx7d-pcie-phy phandle ?\n"); > > + dev_warn(dev, "Unable to apply ERR010728 workaround. DT missing fsl,imx7d-pcie-phy node?\n"); > > } > > > > imx7d_pcie_wait_for_phy_pll_lock(imx6_pcie); > > @@ -970,7 +970,7 @@ static int imx6_pcie_probe(struct platform_device *pdev) > > struct device *dev = &pdev->dev; > > struct dw_pcie *pci; > > struct imx6_pcie *imx6_pcie; > > - struct device_node *np; > > + struct device_node *np = NULL; > > struct resource *dbi_base; > > struct device_node *node = dev->of_node; > > int ret; > > @@ -991,17 +991,10 @@ static int imx6_pcie_probe(struct platform_device *pdev) > > imx6_pcie->pci = pci; > > imx6_pcie->drvdata = of_device_get_match_data(dev); > > > > - /* Find the PHY if one is defined, only imx7d uses it */ > > - np = of_parse_phandle(node, "fsl,imx7d-pcie-phy", 0); > > + /* Find the PHY if one is present in DT, only imx7d uses it */ > > + np = of_find_compatible_node(NULL, NULL, "fsl,imx7d-pcie-phy"); > > if (np) { > > - struct resource res; > > - > > - ret = of_address_to_resource(np, 0, &res); > > - if (ret) { > > - dev_err(dev, "Unable to map PCIe PHY\n"); > > - return ret; > > - } > > - imx6_pcie->phy_base = devm_ioremap_resource(dev, &res); > > + imx6_pcie->phy_base = devm_of_iomap(dev, np, 0, NULL); > > if (IS_ERR(imx6_pcie->phy_base)) { > > dev_err(dev, "Unable to map PCIe PHY\n"); > > return PTR_ERR(imx6_pcie->phy_base); > > -- > > 2.29.2 > >
On Tue, May 11, 2021 at 3:11 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Montag, dem 10.05.2021 um 12:05 -0500 schrieb Rob Herring: > > On Mon, May 10, 2021 at 04:15:05PM +0200, Lucas Stach wrote: > > > We don't need to have a phandle of the PHY, as we know the compatible > > > of the node we are looking for. This will make it easier to put add > > > more PHY handling for new generations later on, where the > > > "fsl,imx7d-pcie-phy" phandle would be a misnomer. > > > > > > Also we can use a helper function to get the resource for us, > > > simplifying out driver code a bit. > > > > Better yes, but really all the phy handling should be split out to > > its own driver even in the older h/w with shared phy registers. > > > That would be a quite massive DT binding changing break, possibly even > a separate driver. Maybe it's time to do this for i.MX8MM, as the > current driver just kept piling on special cases for "almost the same" > hardware that by now looks quite different to the original i.MX6 PCIe > integration this driver was supposed to handle. No, you don't need to change DT, and a DT change adding a phy node wouldn't even be correct modeling of the h/w IMO. For the i.MX6 phy, a separate PHY driver would have to create its own platform device in its initcall (if the iMX6 PCI compatible is found). Then the PCI driver would need to use a non-DT based phy_get() lookup. For the cases with a phandle to the phy, I'd assume a phy driver could be instantiated for that node. You'll again need a non-DT phy_get() if not using the phy binding. Rob
Am Dienstag, dem 11.05.2021 um 09:21 -0500 schrieb Rob Herring: > On Tue, May 11, 2021 at 3:11 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > Am Montag, dem 10.05.2021 um 12:05 -0500 schrieb Rob Herring: > > > On Mon, May 10, 2021 at 04:15:05PM +0200, Lucas Stach wrote: > > > > We don't need to have a phandle of the PHY, as we know the compatible > > > > of the node we are looking for. This will make it easier to put add > > > > more PHY handling for new generations later on, where the > > > > "fsl,imx7d-pcie-phy" phandle would be a misnomer. > > > > > > > > Also we can use a helper function to get the resource for us, > > > > simplifying out driver code a bit. > > > > > > Better yes, but really all the phy handling should be split out to > > > its own driver even in the older h/w with shared phy registers. > > > > > That would be a quite massive DT binding changing break, possibly even > > a separate driver. Maybe it's time to do this for i.MX8MM, as the > > current driver just kept piling on special cases for "almost the same" > > hardware that by now looks quite different to the original i.MX6 PCIe > > integration this driver was supposed to handle. > > No, you don't need to change DT, and a DT change adding a phy node > wouldn't even be correct modeling of the h/w IMO. For the i.MX6 phy, a > separate PHY driver would have to create its own platform device in > its initcall (if the iMX6 PCI compatible is found). Then the PCI > driver would need to use a non-DT based phy_get() lookup. For the > cases with a phandle to the phy, I'd assume a phy driver could be > instantiated for that node. You'll again need a non-DT phy_get() if > not using the phy binding. The original i.MX6 PCIe with the internal PHY is the easy case, as you laid out above. What I'm more concerned about is the i.MX7 and i.MX8MQ, where we have a MMIO mapped PHY and quite a bit of the clocks/reset/GPR handling would need to move from the controller to the PHY driver. Without a binding change I fear that we end up in a worst of both worlds situation, where we have lots of code in the driver to separate resources that are currently all attached to the PCIe controller node in the DT, without a real gain in making the driver any simpler or easier to maintain. But right now that's all speculation. Maybe I need to type something up and see where it falls on the shiny/horrible scale. Regards, Lucas
On Tue, May 11, 2021 at 9:54 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Dienstag, dem 11.05.2021 um 09:21 -0500 schrieb Rob Herring: > > On Tue, May 11, 2021 at 3:11 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > Am Montag, dem 10.05.2021 um 12:05 -0500 schrieb Rob Herring: > > > > On Mon, May 10, 2021 at 04:15:05PM +0200, Lucas Stach wrote: > > > > > We don't need to have a phandle of the PHY, as we know the compatible > > > > > of the node we are looking for. This will make it easier to put add > > > > > more PHY handling for new generations later on, where the > > > > > "fsl,imx7d-pcie-phy" phandle would be a misnomer. > > > > > > > > > > Also we can use a helper function to get the resource for us, > > > > > simplifying out driver code a bit. > > > > > > > > Better yes, but really all the phy handling should be split out to > > > > its own driver even in the older h/w with shared phy registers. > > > > > > > That would be a quite massive DT binding changing break, possibly even > > > a separate driver. Maybe it's time to do this for i.MX8MM, as the > > > current driver just kept piling on special cases for "almost the same" > > > hardware that by now looks quite different to the original i.MX6 PCIe > > > integration this driver was supposed to handle. > > > > No, you don't need to change DT, and a DT change adding a phy node > > wouldn't even be correct modeling of the h/w IMO. For the i.MX6 phy, a > > separate PHY driver would have to create its own platform device in > > its initcall (if the iMX6 PCI compatible is found). Then the PCI > > driver would need to use a non-DT based phy_get() lookup. For the > > cases with a phandle to the phy, I'd assume a phy driver could be > > instantiated for that node. You'll again need a non-DT phy_get() if > > not using the phy binding. > > The original i.MX6 PCIe with the internal PHY is the easy case, as you > laid out above. > > What I'm more concerned about is the i.MX7 and i.MX8MQ, where we have a > MMIO mapped PHY and quite a bit of the clocks/reset/GPR handling would > need to move from the controller to the PHY driver. Without a binding > change I fear that we end up in a worst of both worlds situation, where > we have lots of code in the driver to separate resources that are > currently all attached to the PCIe controller node in the DT, without a > real gain in making the driver any simpler or easier to maintain. One option for handling compatibility is making an overlay for old DTs instead of coding the old DT handling. There's an example of this for rcar-du in drivers/gpu/drm/rcar-du/rcar_du_of_lvds_*.dts. Rob
On Tue, May 11, 2021 at 04:54:08PM +0200, Lucas Stach wrote: > Am Dienstag, dem 11.05.2021 um 09:21 -0500 schrieb Rob Herring: > > On Tue, May 11, 2021 at 3:11 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > Am Montag, dem 10.05.2021 um 12:05 -0500 schrieb Rob Herring: > > > > On Mon, May 10, 2021 at 04:15:05PM +0200, Lucas Stach wrote: > > > > > We don't need to have a phandle of the PHY, as we know the compatible > > > > > of the node we are looking for. This will make it easier to put add > > > > > more PHY handling for new generations later on, where the > > > > > "fsl,imx7d-pcie-phy" phandle would be a misnomer. > > > > > > > > > > Also we can use a helper function to get the resource for us, > > > > > simplifying out driver code a bit. > > > > > > > > Better yes, but really all the phy handling should be split out to > > > > its own driver even in the older h/w with shared phy registers. > > > > > > > That would be a quite massive DT binding changing break, possibly even > > > a separate driver. Maybe it's time to do this for i.MX8MM, as the > > > current driver just kept piling on special cases for "almost the same" > > > hardware that by now looks quite different to the original i.MX6 PCIe > > > integration this driver was supposed to handle. > > > > No, you don't need to change DT, and a DT change adding a phy node > > wouldn't even be correct modeling of the h/w IMO. For the i.MX6 phy, a > > separate PHY driver would have to create its own platform device in > > its initcall (if the iMX6 PCI compatible is found). Then the PCI > > driver would need to use a non-DT based phy_get() lookup. For the > > cases with a phandle to the phy, I'd assume a phy driver could be > > instantiated for that node. You'll again need a non-DT phy_get() if > > not using the phy binding. > > The original i.MX6 PCIe with the internal PHY is the easy case, as you > laid out above. > > What I'm more concerned about is the i.MX7 and i.MX8MQ, where we have a > MMIO mapped PHY and quite a bit of the clocks/reset/GPR handling would > need to move from the controller to the PHY driver. Without a binding > change I fear that we end up in a worst of both worlds situation, where > we have lots of code in the driver to separate resources that are > currently all attached to the PCIe controller node in the DT, without a > real gain in making the driver any simpler or easier to maintain. > > But right now that's all speculation. Maybe I need to type something up > and see where it falls on the shiny/horrible scale. Hi Lucas, given the feedback I will mark this series as "Changes requested" waiting with what follows, please let me know if that's what you expected. Thanks, Lorenzo
diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt index de4b2baf91e8..308540df99ef 100644 --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt @@ -54,7 +54,6 @@ Additional required properties for imx7d-pcie and imx8mq-pcie: - "pciephy" - "apps" - "turnoff" -- fsl,imx7d-pcie-phy: A phandle to an fsl,imx7d-pcie-phy node. Additional required properties for imx8mq-pcie: - clock-names: Must include the following additional entries: @@ -88,8 +87,8 @@ Example: * Freescale i.MX7d PCIe PHY -This is the PHY associated with the IMX7d PCIe controller. It's used by the -PCI-e controller via the fsl,imx7d-pcie-phy phandle. +This is the PHY associated with the IMX7d PCIe controller. It's looked up by +the PCI-e controller via the fsl,imx7d-pcie-phy compatible. Required properties: - compatible: diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c index 922c14361cd3..5e13758222e8 100644 --- a/drivers/pci/controller/dwc/pci-imx6.c +++ b/drivers/pci/controller/dwc/pci-imx6.c @@ -555,7 +555,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) writel(PCIE_PHY_CMN_REG26_ATT_MODE, imx6_pcie->phy_base + PCIE_PHY_CMN_REG26); } else { - dev_warn(dev, "Unable to apply ERR010728 workaround. DT missing fsl,imx7d-pcie-phy phandle ?\n"); + dev_warn(dev, "Unable to apply ERR010728 workaround. DT missing fsl,imx7d-pcie-phy node?\n"); } imx7d_pcie_wait_for_phy_pll_lock(imx6_pcie); @@ -970,7 +970,7 @@ static int imx6_pcie_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct dw_pcie *pci; struct imx6_pcie *imx6_pcie; - struct device_node *np; + struct device_node *np = NULL; struct resource *dbi_base; struct device_node *node = dev->of_node; int ret; @@ -991,17 +991,10 @@ static int imx6_pcie_probe(struct platform_device *pdev) imx6_pcie->pci = pci; imx6_pcie->drvdata = of_device_get_match_data(dev); - /* Find the PHY if one is defined, only imx7d uses it */ - np = of_parse_phandle(node, "fsl,imx7d-pcie-phy", 0); + /* Find the PHY if one is present in DT, only imx7d uses it */ + np = of_find_compatible_node(NULL, NULL, "fsl,imx7d-pcie-phy"); if (np) { - struct resource res; - - ret = of_address_to_resource(np, 0, &res); - if (ret) { - dev_err(dev, "Unable to map PCIe PHY\n"); - return ret; - } - imx6_pcie->phy_base = devm_ioremap_resource(dev, &res); + imx6_pcie->phy_base = devm_of_iomap(dev, np, 0, NULL); if (IS_ERR(imx6_pcie->phy_base)) { dev_err(dev, "Unable to map PCIe PHY\n"); return PTR_ERR(imx6_pcie->phy_base);
We don't need to have a phandle of the PHY, as we know the compatible of the node we are looking for. This will make it easier to put add more PHY handling for new generations later on, where the "fsl,imx7d-pcie-phy" phandle would be a misnomer. Also we can use a helper function to get the resource for us, simplifying out driver code a bit. Signed-off-by: Lucas Stach <l.stach@pengutronix.de> --- .../devicetree/bindings/pci/fsl,imx6q-pcie.txt | 5 ++--- drivers/pci/controller/dwc/pci-imx6.c | 17 +++++------------ 2 files changed, 7 insertions(+), 15 deletions(-)