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