| Message ID | 20230721-ti-mdio-pinmux-v2-1-4bb443e09ac0@kernel.org |
|---|---|
| State | Superseded |
| Delegated to: | Ramon Fried |
| Headers | show |
| Series | net: ti: am65-cpsw-nuss: Fix DT binding handling of pinctrl | expand |
On 21/07/2023 16:07, Maxime Ripard wrote: > The binding represents the MDIO controller as a child device tree > node of the MAC device tree node. > > The U-Boot driver mostly ignores that child device tree node and just > hardcodes the resources it uses to support both the MAC and MDIO in a > single driver. > > However, some resources like pinctrl muxing states are thus ignored. > This has been a problem with some device trees that will put some > pinctrl states on the MDIO device tree node, like the SK-AM62 Device > Tree does. You don't clearly explain the problem. I think you need to mention that there wash a hackish solution in place that was duplicating MDIO pinctrl node in the CPSW node. And this causes problems in Linux (if booted with u-boot's device tree) due to 2 drivers (CPSW and MDIO) racing for the same MDIO pinctrl resource. Then mention how you are fixing it. By deleting the duplicate MDIO pinctrl entry from CPSW node. > > Let's rework the driver a bit to create a dummy MDIO driver that we will > then get during our initialization to force the core to select the right > muxing. > > Signed-off-by: Maxime Ripard <mripard@kernel.org> > --- > drivers/net/ti/Kconfig | 1 + > drivers/net/ti/am65-cpsw-nuss.c | 67 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 68 insertions(+) > > diff --git a/drivers/net/ti/Kconfig b/drivers/net/ti/Kconfig > index e13dbc940182..08c81f79adf9 100644 > --- a/drivers/net/ti/Kconfig > +++ b/drivers/net/ti/Kconfig > @@ -41,6 +41,7 @@ endchoice > config TI_AM65_CPSW_NUSS > bool "TI K3 AM65x MCU CPSW Nuss Ethernet controller driver" > depends on ARCH_K3 > + imply DM_MDIO > imply MISC_INIT_R > imply MISC > select PHYLIB > diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c > index 3069550d53c2..ac7907e7ef70 100644 > --- a/drivers/net/ti/am65-cpsw-nuss.c > +++ b/drivers/net/ti/am65-cpsw-nuss.c > @@ -15,6 +15,7 @@ > #include <dm.h> > #include <dm/device_compat.h> > #include <dm/lists.h> > +#include <dm/pinctrl.h> > #include <dma-uclass.h> > #include <dm/of_access.h> > #include <miiphy.h> > @@ -580,14 +581,69 @@ static const struct soc_attr k3_mdio_soc_data[] = { > { /* sentinel */ }, > }; > > +static ofnode am65_cpsw_find_mdio(ofnode parent) > +{ > + ofnode node; > + > + ofnode_for_each_subnode(node, parent) > + if (ofnode_device_is_compatible(node, "ti,cpsw-mdio")) > + return node; > + > + return ofnode_null(); > +} > + > +static int am65_cpsw_mdio_setup(struct udevice *dev) > +{ > + struct am65_cpsw_priv *priv = dev_get_priv(dev); > + struct am65_cpsw_common *cpsw_common = priv->cpsw_common; > + struct udevice *mdio_dev; > + ofnode mdio; > + int ret; > + > + mdio = am65_cpsw_find_mdio(dev_ofnode(cpsw_common->dev)); > + if (!ofnode_valid(mdio)) > + return -ENODEV; Do we really want to treat this as an error? As per Linux/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml mdio node is not a required property/child. > + > + /* > + * The MDIO controller is represented in the DT binding by a > + * subnode of the MAC controller. > + * > + * Our driver largely ignores that and supports MDIO by > + * hardcoding the register offsets. > + * > + * However, some resources (clocks, pinctrl) might be tied to > + * the MDIO node, and thus ignored. > + * > + * Clocks are not a concern at the moment since it's shared > + * between the MAC and MDIO, so the driver handles both at the > + * same time. I think you can drop the above 3 paras and instead say "We don't yet have a DM device driver for the MDIO device and so its pinctrl setting will be ignored." > + * > + * However, we do need to make sure the pins states tied to the > + * MDIO node are configured properly. Fortunately, the core DM > + * does that for use when we get a device, so we can work around > + * that whole issue by just requesting a dummy MDIO driver to > + * probe, and our pins will get muxed. > + */ > + ret = uclass_get_device_by_ofnode(UCLASS_MDIO, mdio, &mdio_dev); > + if (ret) > + return ret; > + > + return 0; > +} > + > static int am65_cpsw_mdio_init(struct udevice *dev) > { > struct am65_cpsw_priv *priv = dev_get_priv(dev); > struct am65_cpsw_common *cpsw_common = priv->cpsw_common; > + int ret; > > if (!priv->has_phy || cpsw_common->bus) > return 0; > > + ret = am65_cpsw_mdio_setup(dev); > + if (ret) > + return ret; > + > cpsw_common->bus = cpsw_mdio_init(dev->name, > cpsw_common->mdio_base, > cpsw_common->bus_freq, > @@ -854,3 +910,14 @@ U_BOOT_DRIVER(am65_cpsw_nuss_port) = { > .plat_auto = sizeof(struct eth_pdata), > .flags = DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_OS_PREPARE, > }; > + > +static const struct udevice_id am65_cpsw_mdio_ids[] = { > + { .compatible = "ti,cpsw-mdio" }, > + { } > +}; > + > +U_BOOT_DRIVER(am65_cpsw_mdio) = { > + .name = "am65_cpsw_mdio", > + .id = UCLASS_MDIO, > + .of_match = am65_cpsw_mdio_ids, > +}; >
On Sat, Jul 22, 2023 at 11:25:50AM +0300, Roger Quadros wrote: > On 21/07/2023 16:07, Maxime Ripard wrote: > > The binding represents the MDIO controller as a child device tree > > node of the MAC device tree node. > > > > The U-Boot driver mostly ignores that child device tree node and just > > hardcodes the resources it uses to support both the MAC and MDIO in a > > single driver. > > > > However, some resources like pinctrl muxing states are thus ignored. > > This has been a problem with some device trees that will put some > > pinctrl states on the MDIO device tree node, like the SK-AM62 Device > > Tree does. > > You don't clearly explain the problem. I'm sorry to hear that. > I think you need to mention that there wash a hackish solution in > place that was duplicating MDIO pinctrl node in the CPSW node. And > this causes problems in Linux (if booted with u-boot's device tree) > due to 2 drivers (CPSW and MDIO) racing for the same MDIO pinctrl > resource. I mean, ultimately, this hack was due to nothing but a workaround around the fact that U-Boot was ignoring the MDIO child node resources. This is the root cause. And once fixed, that hack can go away. > Then mention how you are fixing it. I'm fixing it by "rework[ing] the driver a bit to create a dummy MDIO driver that we will then get during our initialization to force the core to select the right muxing." If that's still not a clear explanation, please provide a better one so that we can move forward. > By deleting the duplicate MDIO pinctrl entry from CPSW node. Not really, no. If I was only deleting the duplitate pinctrl entry, U-Boot would still be broken. > > Let's rework the driver a bit to create a dummy MDIO driver that we will > > then get during our initialization to force the core to select the right > > muxing. > > > > Signed-off-by: Maxime Ripard <mripard@kernel.org> > > --- > > drivers/net/ti/Kconfig | 1 + > > drivers/net/ti/am65-cpsw-nuss.c | 67 +++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 68 insertions(+) > > > > diff --git a/drivers/net/ti/Kconfig b/drivers/net/ti/Kconfig > > index e13dbc940182..08c81f79adf9 100644 > > --- a/drivers/net/ti/Kconfig > > +++ b/drivers/net/ti/Kconfig > > @@ -41,6 +41,7 @@ endchoice > > config TI_AM65_CPSW_NUSS > > bool "TI K3 AM65x MCU CPSW Nuss Ethernet controller driver" > > depends on ARCH_K3 > > + imply DM_MDIO > > imply MISC_INIT_R > > imply MISC > > select PHYLIB > > diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c > > index 3069550d53c2..ac7907e7ef70 100644 > > --- a/drivers/net/ti/am65-cpsw-nuss.c > > +++ b/drivers/net/ti/am65-cpsw-nuss.c > > @@ -15,6 +15,7 @@ > > #include <dm.h> > > #include <dm/device_compat.h> > > #include <dm/lists.h> > > +#include <dm/pinctrl.h> > > #include <dma-uclass.h> > > #include <dm/of_access.h> > > #include <miiphy.h> > > @@ -580,14 +581,69 @@ static const struct soc_attr k3_mdio_soc_data[] = { > > { /* sentinel */ }, > > }; > > > > +static ofnode am65_cpsw_find_mdio(ofnode parent) > > +{ > > + ofnode node; > > + > > + ofnode_for_each_subnode(node, parent) > > + if (ofnode_device_is_compatible(node, "ti,cpsw-mdio")) > > + return node; > > + > > + return ofnode_null(); > > +} > > + > > +static int am65_cpsw_mdio_setup(struct udevice *dev) > > +{ > > + struct am65_cpsw_priv *priv = dev_get_priv(dev); > > + struct am65_cpsw_common *cpsw_common = priv->cpsw_common; > > + struct udevice *mdio_dev; > > + ofnode mdio; > > + int ret; > > + > > + mdio = am65_cpsw_find_mdio(dev_ofnode(cpsw_common->dev)); > > + if (!ofnode_valid(mdio)) > > + return -ENODEV; > > Do we really want to treat this as an error? > > As per Linux/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml > mdio node is not a required property/child. Ack, I'll change it. > > + > > + /* > > + * The MDIO controller is represented in the DT binding by a > > + * subnode of the MAC controller. > > + * > > + * Our driver largely ignores that and supports MDIO by > > + * hardcoding the register offsets. > > + * > > + * However, some resources (clocks, pinctrl) might be tied to > > + * the MDIO node, and thus ignored. > > + * > > + * Clocks are not a concern at the moment since it's shared > > + * between the MAC and MDIO, so the driver handles both at the > > + * same time. > > I think you can drop the above 3 paras and instead say > "We don't yet have a DM device driver for the MDIO device and so its > pinctrl setting will be ignored." Ok. Maxime
On 24/07/2023 11:39, Maxime Ripard wrote: > On Sat, Jul 22, 2023 at 11:25:50AM +0300, Roger Quadros wrote: >> On 21/07/2023 16:07, Maxime Ripard wrote: >>> The binding represents the MDIO controller as a child device tree >>> node of the MAC device tree node. >>> >>> The U-Boot driver mostly ignores that child device tree node and just >>> hardcodes the resources it uses to support both the MAC and MDIO in a >>> single driver. >>> >>> However, some resources like pinctrl muxing states are thus ignored. >>> This has been a problem with some device trees that will put some >>> pinctrl states on the MDIO device tree node, like the SK-AM62 Device >>> Tree does. >> >> You don't clearly explain the problem. > > I'm sorry to hear that. > >> I think you need to mention that there wash a hackish solution in >> place that was duplicating MDIO pinctrl node in the CPSW node. And >> this causes problems in Linux (if booted with u-boot's device tree) >> due to 2 drivers (CPSW and MDIO) racing for the same MDIO pinctrl >> resource. > > I mean, ultimately, this hack was due to nothing but a workaround around > the fact that U-Boot was ignoring the MDIO child node resources. This is > the root cause. And once fixed, that hack can go away. > >> Then mention how you are fixing it. > > I'm fixing it by "rework[ing] the driver a bit to create a dummy MDIO > driver that we will then get during our initialization to force the core > to select the right muxing." > > If that's still not a clear explanation, please provide a better one so > that we can move forward. This is fine thanks. > >> By deleting the duplicate MDIO pinctrl entry from CPSW node. > > Not really, no. If I was only deleting the duplitate pinctrl entry, > U-Boot would still be broken. > >>> Let's rework the driver a bit to create a dummy MDIO driver that we will >>> then get during our initialization to force the core to select the right >>> muxing. >>> >>> Signed-off-by: Maxime Ripard <mripard@kernel.org> >>> --- >>> drivers/net/ti/Kconfig | 1 + >>> drivers/net/ti/am65-cpsw-nuss.c | 67 +++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 68 insertions(+) >>> >>> diff --git a/drivers/net/ti/Kconfig b/drivers/net/ti/Kconfig >>> index e13dbc940182..08c81f79adf9 100644 >>> --- a/drivers/net/ti/Kconfig >>> +++ b/drivers/net/ti/Kconfig >>> @@ -41,6 +41,7 @@ endchoice >>> config TI_AM65_CPSW_NUSS >>> bool "TI K3 AM65x MCU CPSW Nuss Ethernet controller driver" >>> depends on ARCH_K3 >>> + imply DM_MDIO >>> imply MISC_INIT_R >>> imply MISC >>> select PHYLIB >>> diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c >>> index 3069550d53c2..ac7907e7ef70 100644 >>> --- a/drivers/net/ti/am65-cpsw-nuss.c >>> +++ b/drivers/net/ti/am65-cpsw-nuss.c >>> @@ -15,6 +15,7 @@ >>> #include <dm.h> >>> #include <dm/device_compat.h> >>> #include <dm/lists.h> >>> +#include <dm/pinctrl.h> >>> #include <dma-uclass.h> >>> #include <dm/of_access.h> >>> #include <miiphy.h> >>> @@ -580,14 +581,69 @@ static const struct soc_attr k3_mdio_soc_data[] = { >>> { /* sentinel */ }, >>> }; >>> >>> +static ofnode am65_cpsw_find_mdio(ofnode parent) >>> +{ >>> + ofnode node; >>> + >>> + ofnode_for_each_subnode(node, parent) >>> + if (ofnode_device_is_compatible(node, "ti,cpsw-mdio")) >>> + return node; >>> + >>> + return ofnode_null(); >>> +} >>> + >>> +static int am65_cpsw_mdio_setup(struct udevice *dev) >>> +{ >>> + struct am65_cpsw_priv *priv = dev_get_priv(dev); >>> + struct am65_cpsw_common *cpsw_common = priv->cpsw_common; >>> + struct udevice *mdio_dev; >>> + ofnode mdio; >>> + int ret; >>> + >>> + mdio = am65_cpsw_find_mdio(dev_ofnode(cpsw_common->dev)); >>> + if (!ofnode_valid(mdio)) >>> + return -ENODEV; >> >> Do we really want to treat this as an error? >> >> As per Linux/Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml >> mdio node is not a required property/child. > > Ack, I'll change it. > >>> + >>> + /* >>> + * The MDIO controller is represented in the DT binding by a >>> + * subnode of the MAC controller. >>> + * >>> + * Our driver largely ignores that and supports MDIO by >>> + * hardcoding the register offsets. >>> + * >>> + * However, some resources (clocks, pinctrl) might be tied to >>> + * the MDIO node, and thus ignored. >>> + * >>> + * Clocks are not a concern at the moment since it's shared >>> + * between the MAC and MDIO, so the driver handles both at the >>> + * same time. >> >> I think you can drop the above 3 paras and instead say >> "We don't yet have a DM device driver for the MDIO device and so its >> pinctrl setting will be ignored." > > Ok. > > Maxime
diff --git a/drivers/net/ti/Kconfig b/drivers/net/ti/Kconfig index e13dbc940182..08c81f79adf9 100644 --- a/drivers/net/ti/Kconfig +++ b/drivers/net/ti/Kconfig @@ -41,6 +41,7 @@ endchoice config TI_AM65_CPSW_NUSS bool "TI K3 AM65x MCU CPSW Nuss Ethernet controller driver" depends on ARCH_K3 + imply DM_MDIO imply MISC_INIT_R imply MISC select PHYLIB diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c index 3069550d53c2..ac7907e7ef70 100644 --- a/drivers/net/ti/am65-cpsw-nuss.c +++ b/drivers/net/ti/am65-cpsw-nuss.c @@ -15,6 +15,7 @@ #include <dm.h> #include <dm/device_compat.h> #include <dm/lists.h> +#include <dm/pinctrl.h> #include <dma-uclass.h> #include <dm/of_access.h> #include <miiphy.h> @@ -580,14 +581,69 @@ static const struct soc_attr k3_mdio_soc_data[] = { { /* sentinel */ }, }; +static ofnode am65_cpsw_find_mdio(ofnode parent) +{ + ofnode node; + + ofnode_for_each_subnode(node, parent) + if (ofnode_device_is_compatible(node, "ti,cpsw-mdio")) + return node; + + return ofnode_null(); +} + +static int am65_cpsw_mdio_setup(struct udevice *dev) +{ + struct am65_cpsw_priv *priv = dev_get_priv(dev); + struct am65_cpsw_common *cpsw_common = priv->cpsw_common; + struct udevice *mdio_dev; + ofnode mdio; + int ret; + + mdio = am65_cpsw_find_mdio(dev_ofnode(cpsw_common->dev)); + if (!ofnode_valid(mdio)) + return -ENODEV; + + /* + * The MDIO controller is represented in the DT binding by a + * subnode of the MAC controller. + * + * Our driver largely ignores that and supports MDIO by + * hardcoding the register offsets. + * + * However, some resources (clocks, pinctrl) might be tied to + * the MDIO node, and thus ignored. + * + * Clocks are not a concern at the moment since it's shared + * between the MAC and MDIO, so the driver handles both at the + * same time. + * + * However, we do need to make sure the pins states tied to the + * MDIO node are configured properly. Fortunately, the core DM + * does that for use when we get a device, so we can work around + * that whole issue by just requesting a dummy MDIO driver to + * probe, and our pins will get muxed. + */ + ret = uclass_get_device_by_ofnode(UCLASS_MDIO, mdio, &mdio_dev); + if (ret) + return ret; + + return 0; +} + static int am65_cpsw_mdio_init(struct udevice *dev) { struct am65_cpsw_priv *priv = dev_get_priv(dev); struct am65_cpsw_common *cpsw_common = priv->cpsw_common; + int ret; if (!priv->has_phy || cpsw_common->bus) return 0; + ret = am65_cpsw_mdio_setup(dev); + if (ret) + return ret; + cpsw_common->bus = cpsw_mdio_init(dev->name, cpsw_common->mdio_base, cpsw_common->bus_freq, @@ -854,3 +910,14 @@ U_BOOT_DRIVER(am65_cpsw_nuss_port) = { .plat_auto = sizeof(struct eth_pdata), .flags = DM_FLAG_ALLOC_PRIV_DMA | DM_FLAG_OS_PREPARE, }; + +static const struct udevice_id am65_cpsw_mdio_ids[] = { + { .compatible = "ti,cpsw-mdio" }, + { } +}; + +U_BOOT_DRIVER(am65_cpsw_mdio) = { + .name = "am65_cpsw_mdio", + .id = UCLASS_MDIO, + .of_match = am65_cpsw_mdio_ids, +};
The binding represents the MDIO controller as a child device tree node of the MAC device tree node. The U-Boot driver mostly ignores that child device tree node and just hardcodes the resources it uses to support both the MAC and MDIO in a single driver. However, some resources like pinctrl muxing states are thus ignored. This has been a problem with some device trees that will put some pinctrl states on the MDIO device tree node, like the SK-AM62 Device Tree does. Let's rework the driver a bit to create a dummy MDIO driver that we will then get during our initialization to force the core to select the right muxing. Signed-off-by: Maxime Ripard <mripard@kernel.org> --- drivers/net/ti/Kconfig | 1 + drivers/net/ti/am65-cpsw-nuss.c | 67 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+)