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