| Message ID | 20230720-ti-mdio-pinmux-v1-2-0bd3bd1cf759@kernel.org |
|---|---|
| State | Superseded |
| Delegated to: | Ramon Fried |
| Headers | show |
| Series | net: ti: am65-cpsw-nuss: Fix DT binding handling of pinctrl | expand |
On 20/07/2023 12:55, 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 device trees since Linux 6.5-rc1 which will > put some pinctrl states on the MDIO device tree node. > > Let's rework the driver a bit to fetch the pinctrl state from the MDIO > node and apply it. > > Signed-off-by: Maxime Ripard <mripard@kernel.org> > --- > drivers/net/ti/am65-cpsw-nuss.c | 49 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c > index f674b0baa359..2d579bf4af2c 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> > @@ -696,6 +697,50 @@ out: > return ret; > } > > +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_setup_mdio(struct udevice *dev) > +{ > + ofnode mdio; > + int ret; > + > + mdio = am65_cpsw_find_mdio(dev_ofnode(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. > + */ Please mention this as a HACK: that needs to go away when davinci_mdio and this driver properly supports MDIO infrastructure. > + ret = pinctrl_select_state_by_ofnode(dev, mdio, "default"); Instead of modifying the API, can we have a dummy driver for the "ti,davinci_mdio" device that registers as class UCLASS_MDIO but does nothing else? Then you can drop patch 1 and instead do ret = uclass_get_device_by_ofnode(UCLASS_MDIO, node, &mdio_dev); if (!ret) pinctrl_select_state(mdio_dev, "default"); > + if (ret) > + return ret; > + > + return 0; > +} > + > static int am65_cpsw_probe_nuss(struct udevice *dev) > { > struct am65_cpsw_common *cpsw_common = dev_get_priv(dev); > @@ -728,6 +773,10 @@ static int am65_cpsw_probe_nuss(struct udevice *dev) > AM65_CPSW_CPSW_NU_ALE_BASE; > cpsw_common->mdio_base = cpsw_common->ss_base + AM65_CPSW_MDIO_BASE; > > + ret = am65_cpsw_setup_mdio(dev); > + if (ret) > + goto out; > + > ports_np = dev_read_subnode(dev, "ethernet-ports"); > if (!ofnode_valid(ports_np)) { > ret = -ENOENT; >
On Thu, Jul 20, 2023 at 06:47:43PM +0300, Roger Quadros wrote: > On 20/07/2023 12:55, 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 device trees since Linux 6.5-rc1 which will > > put some pinctrl states on the MDIO device tree node. > > > > Let's rework the driver a bit to fetch the pinctrl state from the MDIO > > node and apply it. > > > > Signed-off-by: Maxime Ripard <mripard@kernel.org> > > --- > > drivers/net/ti/am65-cpsw-nuss.c | 49 +++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 49 insertions(+) > > > > diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c > > index f674b0baa359..2d579bf4af2c 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> > > @@ -696,6 +697,50 @@ out: > > return ret; > > } > > > > +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_setup_mdio(struct udevice *dev) > > +{ > > + ofnode mdio; > > + int ret; > > + > > + mdio = am65_cpsw_find_mdio(dev_ofnode(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. > > + */ > > Please mention this as a HACK: that needs to go away when davinci_mdio > and this driver properly supports MDIO infrastructure. Will do > > + ret = pinctrl_select_state_by_ofnode(dev, mdio, "default"); > > Instead of modifying the API, can we have a dummy driver for the "ti,davinci_mdio" > device that registers as class UCLASS_MDIO but does nothing else? > > Then you can drop patch 1 and instead do > > ret = uclass_get_device_by_ofnode(UCLASS_MDIO, node, &mdio_dev); > if (!ret) > pinctrl_select_state(mdio_dev, "default"); Good idea, thanks :) Maxime
diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c index f674b0baa359..2d579bf4af2c 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> @@ -696,6 +697,50 @@ out: return ret; } +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_setup_mdio(struct udevice *dev) +{ + ofnode mdio; + int ret; + + mdio = am65_cpsw_find_mdio(dev_ofnode(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. + */ + ret = pinctrl_select_state_by_ofnode(dev, mdio, "default"); + if (ret) + return ret; + + return 0; +} + static int am65_cpsw_probe_nuss(struct udevice *dev) { struct am65_cpsw_common *cpsw_common = dev_get_priv(dev); @@ -728,6 +773,10 @@ static int am65_cpsw_probe_nuss(struct udevice *dev) AM65_CPSW_CPSW_NU_ALE_BASE; cpsw_common->mdio_base = cpsw_common->ss_base + AM65_CPSW_MDIO_BASE; + ret = am65_cpsw_setup_mdio(dev); + if (ret) + goto out; + ports_np = dev_read_subnode(dev, "ethernet-ports"); if (!ofnode_valid(ports_np)) { ret = -ENOENT;
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 device trees since Linux 6.5-rc1 which will put some pinctrl states on the MDIO device tree node. Let's rework the driver a bit to fetch the pinctrl state from the MDIO node and apply it. Signed-off-by: Maxime Ripard <mripard@kernel.org> --- drivers/net/ti/am65-cpsw-nuss.c | 49 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)