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