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

[Roger Quadros]

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

-- 
cheers,
-roger