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

[Roger Quadros]

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 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.

> +
> +	/*
> +	 * 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,
> +};
> 

-- 
cheers,
-roger