[PATCH net-next 0/3] net: xilinx: emaclite: Adopt clock support

[PATCH net-next 0/3] net: xilinx: emaclite: Adopt clock support

[Radhey Shyam Pandey]

This patchset adds emaclite clock support. AXI Ethernet Lite IP can also
be used on SoC platforms like Zynq UltraScale+ MPSoC which combines
powerful processing system (PS) and user-programmable logic (PL) into
the same device. On these platforms it is mandatory to explicitly enable
IP clocks for proper functionality.

Abin Joseph (3):
  dt-bindings: net: emaclite: Add clock support
  net: emaclite: Replace alloc_etherdev() with devm_alloc_etherdev()
  net: emaclite: Adopt clock support

 .../bindings/net/xlnx,emaclite.yaml           |  3 +++
 drivers/net/ethernet/xilinx/xilinx_emaclite.c | 22 ++++++++++---------
 2 files changed, 15 insertions(+), 10 deletions(-)


base-commit: c824deb1a89755f70156b5cdaf569fca80698719
-- 
2.34.1

[PATCH net-next 1/3] dt-bindings: net: emaclite: Add clock support

[Radhey Shyam Pandey]

From: Abin Joseph <abin.joseph@amd.com>

Add s_axi_aclk AXI4 clock support and make clk optional to keep DTB
backward compatibility. Define max supported clock constraints.

Signed-off-by: Abin Joseph <abin.joseph@amd.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
 Documentation/devicetree/bindings/net/xlnx,emaclite.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/xlnx,emaclite.yaml b/Documentation/devicetree/bindings/net/xlnx,emaclite.yaml
index 92d8ade988f6..8fcf0732d713 100644
--- a/Documentation/devicetree/bindings/net/xlnx,emaclite.yaml
+++ b/Documentation/devicetree/bindings/net/xlnx,emaclite.yaml
@@ -29,6 +29,9 @@ properties:
   interrupts:
     maxItems: 1
 
+  clocks:
+    maxItems: 1
+
   phy-handle: true
 
   local-mac-address: true
-- 
2.34.1

Re: [PATCH net-next 1/3] dt-bindings: net: emaclite: Add clock support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]

On Tue, Oct 01, 2024 at 01:25:36AM +0530, Radhey Shyam Pandey wrote:
> From: Abin Joseph <abin.joseph@amd.com>
> 
> Add s_axi_aclk AXI4 clock support and make clk optional to keep DTB
> backward compatibility. Define max supported clock constraints.

Why was the clock not provided before, but is now?
Was it automatically enabled by firmware and that is no longer done?
I'm suspicious of the clock being made optional, but the driver doing
nothing other than enable it. That reeks of actually being required to
me.

> 
> Signed-off-by: Abin Joseph <abin.joseph@amd.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> ---
>  Documentation/devicetree/bindings/net/xlnx,emaclite.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/xlnx,emaclite.yaml b/Documentation/devicetree/bindings/net/xlnx,emaclite.yaml
> index 92d8ade988f6..8fcf0732d713 100644
> --- a/Documentation/devicetree/bindings/net/xlnx,emaclite.yaml
> +++ b/Documentation/devicetree/bindings/net/xlnx,emaclite.yaml
> @@ -29,6 +29,9 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  clocks:
> +    maxItems: 1
> +
>    phy-handle: true
>  
>    local-mac-address: true
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH net-next 1/3] dt-bindings: net: emaclite: Add clock support

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Conor Dooley <conor@kernel.org>
> Sent: Tuesday, October 1, 2024 10:22 PM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org;
> Simek, Michal <michal.simek@amd.com>; Joseph, Abin <Abin.Joseph@amd.com>;
> u.kleine-koenig@pengutronix.de; elfring@users.sourceforge.net; Katakam, Harini
> <harini.katakam@amd.com>; netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; git (AMD-Xilinx)
> <git@amd.com>
> Subject: Re: [PATCH net-next 1/3] dt-bindings: net: emaclite: Add clock support
> 
> On Tue, Oct 01, 2024 at 01:25:36AM +0530, Radhey Shyam Pandey wrote:
> > From: Abin Joseph <abin.joseph@amd.com>
> >
> > Add s_axi_aclk AXI4 clock support and make clk optional to keep DTB
> > backward compatibility. Define max supported clock constraints.
> 
> Why was the clock not provided before, but is now?
> Was it automatically enabled by firmware and that is no longer done?
> I'm suspicious of the clock being made optional, but the driver doing nothing other
> than enable it. That reeks of actually being required to me.

Traditionally these IP were used on microblaze platforms which had fixed
clocks enabled all the time. Since AXI Ethernet Lite is a PL IP, it can also
be used on SoC platforms like Zynq UltraScale+ MPSoC which combines 
processing system (PS) and user-programmable logic (PL) into the same 
device. On these platforms instead of fixed enabled clocks it is mandatory
to explicitly enable IP clocks for proper functionality. 

It gets more interesting when the PL clock is shared between two IPs 
and one of the drivers is clock adopted and disable the clocks after use 
and clock framework does not know about other clock users (emaclite 
IP using clock) and it will turn off the clocks which would lead to 
hang on emaclite reg access. So, it is needed to correctly model the
clock consumers.

While browsing i found a similar usecase for GMII to RGMII PL IP.
Similar to dt-bindings: net: xilinx_gmii2rgmii: Add clock support[1]
[1]: https://lore.kernel.org/all/4ae4d926-73f0-4f30-9d83-908a92046829@kernel.org/

In this series - I noticed that Krzysztof suggested to:
Nope, just write the description as items in clocks, instead of
maxItems. And drop clock names, are not needed and are kind of obvious.

So something like the below would be fine?

+  clocks:
+    items:
+      - description: AXI4 clock.

> 
> >
> > Signed-off-by: Abin Joseph <abin.joseph@amd.com>
> > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> > ---
> >  Documentation/devicetree/bindings/net/xlnx,emaclite.yaml | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/xlnx,emaclite.yaml
> > b/Documentation/devicetree/bindings/net/xlnx,emaclite.yaml
> > index 92d8ade988f6..8fcf0732d713 100644
> > --- a/Documentation/devicetree/bindings/net/xlnx,emaclite.yaml
> > +++ b/Documentation/devicetree/bindings/net/xlnx,emaclite.yaml
> > @@ -29,6 +29,9 @@ properties:
> >    interrupts:
> >      maxItems: 1
> >
> > +  clocks:
> > +    maxItems: 1
> > +
> >    phy-handle: true
> >
> >    local-mac-address: true
> > --
> > 2.34.1
> >

Re: [PATCH net-next 1/3] dt-bindings: net: emaclite: Add clock support

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4289 bytes --]

On Tue, Oct 01, 2024 at 07:06:12PM +0000, Pandey, Radhey Shyam wrote:
> > -----Original Message-----
> > From: Conor Dooley <conor@kernel.org>
> > Sent: Tuesday, October 1, 2024 10:22 PM
> > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> > pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org;
> > Simek, Michal <michal.simek@amd.com>; Joseph, Abin <Abin.Joseph@amd.com>;
> > u.kleine-koenig@pengutronix.de; elfring@users.sourceforge.net; Katakam, Harini
> > <harini.katakam@amd.com>; netdev@vger.kernel.org; devicetree@vger.kernel.org;
> > linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; git (AMD-Xilinx)
> > <git@amd.com>
> > Subject: Re: [PATCH net-next 1/3] dt-bindings: net: emaclite: Add clock support
> > 
> > On Tue, Oct 01, 2024 at 01:25:36AM +0530, Radhey Shyam Pandey wrote:
> > > From: Abin Joseph <abin.joseph@amd.com>
> > >
> > > Add s_axi_aclk AXI4 clock support and make clk optional to keep DTB
> > > backward compatibility. Define max supported clock constraints.
> > 
> > Why was the clock not provided before, but is now?
> > Was it automatically enabled by firmware and that is no longer done?
> > I'm suspicious of the clock being made optional, but the driver doing nothing other
> > than enable it. That reeks of actually being required to me.
> 
> Traditionally these IP were used on microblaze platforms which had fixed
> clocks enabled all the time. Since AXI Ethernet Lite is a PL IP, it can also
> be used on SoC platforms like Zynq UltraScale+ MPSoC which combines 
> processing system (PS) and user-programmable logic (PL) into the same 
> device. On these platforms instead of fixed enabled clocks it is mandatory
> to explicitly enable IP clocks for proper functionality. 
> 
> It gets more interesting when the PL clock is shared between two IPs 
> and one of the drivers is clock adopted and disable the clocks after use 
> and clock framework does not know about other clock users (emaclite 
> IP using clock) and it will turn off the clocks which would lead to 
> hang on emaclite reg access. So, it is needed to correctly model the
> clock consumers.

That means the clock _is_ required, and should be added as such in the
binding. The older platforms having a fixed clock doesn't impact whether
or not the emaclite IP itself requires the clock to function or not.
Mark it required in the binding, although of course the driver cannot
require it for backwards compatibility reasons.

> While browsing i found a similar usecase for GMII to RGMII PL IP.
> Similar to dt-bindings: net: xilinx_gmii2rgmii: Add clock support[1]
> [1]: https://lore.kernel.org/all/4ae4d926-73f0-4f30-9d83-908a92046829@kernel.org/
> 
> In this series - I noticed that Krzysztof suggested to:
> Nope, just write the description as items in clocks, instead of
> maxItems. And drop clock names, are not needed and are kind of obvious.
> 
> So something like the below would be fine?
> 
> +  clocks:
> +    items:
> +      - description: AXI4 clock.

This would be fine, but your patch is not the same as the one you linked
to. It was using clock-names to provide information on the clock, yours
does not do that. If there's only one clock, there's usually little
point in having clock-names, which is why Krzysztof made that
suggestion.

Cheers,
Conor.

> > >
> > > Signed-off-by: Abin Joseph <abin.joseph@amd.com>
> > > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> > > ---
> > >  Documentation/devicetree/bindings/net/xlnx,emaclite.yaml | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/net/xlnx,emaclite.yaml
> > > b/Documentation/devicetree/bindings/net/xlnx,emaclite.yaml
> > > index 92d8ade988f6..8fcf0732d713 100644
> > > --- a/Documentation/devicetree/bindings/net/xlnx,emaclite.yaml
> > > +++ b/Documentation/devicetree/bindings/net/xlnx,emaclite.yaml
> > > @@ -29,6 +29,9 @@ properties:
> > >    interrupts:
> > >      maxItems: 1
> > >
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> > >    phy-handle: true
> > >
> > >    local-mac-address: true
> > > --
> > > 2.34.1
> > >

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH net-next 2/3] net: emaclite: Replace alloc_etherdev() with devm_alloc_etherdev()

[Radhey Shyam Pandey]

From: Abin Joseph <abin.joseph@amd.com>

Use device managed ethernet device allocation to simplify the error
handling logic. No functional change.

Signed-off-by: Abin Joseph <abin.joseph@amd.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
 drivers/net/ethernet/xilinx/xilinx_emaclite.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_emaclite.c b/drivers/net/ethernet/xilinx/xilinx_emaclite.c
index 940452d0a4d2..13ac619df273 100644
--- a/drivers/net/ethernet/xilinx/xilinx_emaclite.c
+++ b/drivers/net/ethernet/xilinx/xilinx_emaclite.c
@@ -1097,7 +1097,7 @@ static int xemaclite_of_probe(struct platform_device *ofdev)
 	dev_info(dev, "Device Tree Probing\n");
 
 	/* Create an ethernet device instance */
-	ndev = alloc_etherdev(sizeof(struct net_local));
+	ndev = devm_alloc_etherdev(dev, sizeof(struct net_local));
 	if (!ndev)
 		return -ENOMEM;
 
@@ -1110,15 +1110,13 @@ static int xemaclite_of_probe(struct platform_device *ofdev)
 	/* Get IRQ for the device */
 	rc = platform_get_irq(ofdev, 0);
 	if (rc < 0)
-		goto error;
+		return rc;
 
 	ndev->irq = rc;
 
 	lp->base_addr = devm_platform_get_and_ioremap_resource(ofdev, 0, &res);
-	if (IS_ERR(lp->base_addr)) {
-		rc = PTR_ERR(lp->base_addr);
-		goto error;
-	}
+	if (IS_ERR(lp->base_addr))
+		return PTR_ERR(lp->base_addr);
 
 	ndev->mem_start = res->start;
 	ndev->mem_end = res->end;
@@ -1167,8 +1165,6 @@ static int xemaclite_of_probe(struct platform_device *ofdev)
 
 put_node:
 	of_node_put(lp->phy_node);
-error:
-	free_netdev(ndev);
 	return rc;
 }
 
@@ -1197,8 +1193,6 @@ static void xemaclite_of_remove(struct platform_device *of_dev)
 
 	of_node_put(lp->phy_node);
 	lp->phy_node = NULL;
-
-	free_netdev(ndev);
 }
 
 #ifdef CONFIG_NET_POLL_CONTROLLER
-- 
2.34.1

[PATCH net-next 3/3] net: emaclite: Adopt clock support

[Radhey Shyam Pandey]

From: Abin Joseph <abin.joseph@amd.com>

Adapt to use the clock framework. Add s_axi_aclk clock from the processor
bus clock domain and make clk optional to keep DTB backward compatibility.

Signed-off-by: Abin Joseph <abin.joseph@amd.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
 drivers/net/ethernet/xilinx/xilinx_emaclite.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/xilinx/xilinx_emaclite.c b/drivers/net/ethernet/xilinx/xilinx_emaclite.c
index 13ac619df273..4f469eaffa07 100644
--- a/drivers/net/ethernet/xilinx/xilinx_emaclite.c
+++ b/drivers/net/ethernet/xilinx/xilinx_emaclite.c
@@ -7,6 +7,7 @@
  * Copyright (c) 2007 - 2013 Xilinx, Inc.
  */
 
+#include <linux/clk.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/uaccess.h>
@@ -1091,6 +1092,7 @@ static int xemaclite_of_probe(struct platform_device *ofdev)
 	struct net_device *ndev = NULL;
 	struct net_local *lp = NULL;
 	struct device *dev = &ofdev->dev;
+	struct clk *clkin;
 
 	int rc = 0;
 
@@ -1127,6 +1129,12 @@ static int xemaclite_of_probe(struct platform_device *ofdev)
 	lp->tx_ping_pong = get_bool(ofdev, "xlnx,tx-ping-pong");
 	lp->rx_ping_pong = get_bool(ofdev, "xlnx,rx-ping-pong");
 
+	clkin = devm_clk_get_optional_enabled(&ofdev->dev, NULL);
+	if (IS_ERR(clkin)) {
+		return dev_err_probe(&ofdev->dev, PTR_ERR(clkin),
+				"Failed to get and enable clock from Device Tree\n");
+	}
+
 	rc = of_get_ethdev_address(ofdev->dev.of_node, ndev);
 	if (rc) {
 		dev_warn(dev, "No MAC address found, using random\n");
-- 
2.34.1