[PATCH v2 1/3] net: ti: am65-cpsw-nuss: Enforce pinctrl state on the MDIO child node

[Maxime Ripard]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20230724/ac052682/attachment.sig>