[PATCH net-next 0/2] net: dsa: microchip: enable setting rmii reference

[PATCH net-next 0/2] net: dsa: microchip: enable setting rmii reference

[Ante Knezic]

KSZ88X3 devices can select between internal and external RMII reference clock.
This patch series introduces new device tree property for setting reference
clock to internal.

Ante Knezic (2):
  net:dsa:microchip add property to select internal RMII reference clock
  dt-bindings: net: microchip,ksz: document microchip,rmii-clk-internal

 Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml | 6 ++++++
 drivers/net/dsa/microchip/ksz8795.c                          | 4 ++++
 drivers/net/dsa/microchip/ksz8795_reg.h                      | 3 +++
 drivers/net/dsa/microchip/ksz_common.c                       | 3 +++
 drivers/net/dsa/microchip/ksz_common.h                       | 1 +
 5 files changed, 17 insertions(+)

-- 
2.11.0

[PATCH net-next 1/2] net:dsa:microchip: add property to select internal RMII reference clock

[Ante Knezic]

Microchip KSZ8863/KSZ8873 have the ability to select between internal
and external RMII reference clock. By default, reference clock
needs to be provided via REFCLKI_3 pin. If required, device can be
setup to provide RMII clock internally so that REFCLKI_3 pin can be
left unconnected.
Add a new "microchip,rmii-clk-internal" property which will set
RMII clock reference to internal.

Signed-off-by: Ante Knezic <ante.knezic@helmholz.de>
---
 drivers/net/dsa/microchip/ksz8795.c     | 4 ++++
 drivers/net/dsa/microchip/ksz8795_reg.h | 3 +++
 drivers/net/dsa/microchip/ksz_common.c  | 3 +++
 drivers/net/dsa/microchip/ksz_common.h  | 1 +
 4 files changed, 11 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 91aba470fb2f..c5df571153ab 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -1434,6 +1434,10 @@ int ksz8_setup(struct dsa_switch *ds)
 	for (i = 0; i < (dev->info->num_vlans / 4); i++)
 		ksz8_r_vlan_entries(dev, i);
 
+	if (ksz_is_ksz88x3(dev) && dev->rmii_clk_internal)
+		ksz_cfg(dev, KSZ8863_REG_FVID_AND_HOST_MODE,
+			KSZ8863_PORT3_RMII_CLK_INTERNAL, true);
+
 	return ksz8_handle_global_errata(ds);
 }
 
diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h b/drivers/net/dsa/microchip/ksz8795_reg.h
index 7a57c6088f80..0a3fff7d6d32 100644
--- a/drivers/net/dsa/microchip/ksz8795_reg.h
+++ b/drivers/net/dsa/microchip/ksz8795_reg.h
@@ -22,6 +22,9 @@
 #define KSZ8863_GLOBAL_SOFTWARE_RESET	BIT(4)
 #define KSZ8863_PCS_RESET		BIT(0)
 
+#define KSZ8863_REG_FVID_AND_HOST_MODE  0xC6
+#define KSZ8863_PORT3_RMII_CLK_INTERNAL BIT(3)
+
 #define REG_SW_CTRL_0			0x02
 
 #define SW_NEW_BACKOFF			BIT(7)
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 6673122266b7..dc8cf59c6dd9 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -3631,6 +3631,9 @@ int ksz_switch_register(struct ksz_device *dev)
 		}
 	}
 
+	dev->rmii_clk_internal = of_property_read_bool(dev->dev->of_node,
+						       "microchip,rmii-clk-internal");
+
 	ret = dsa_register_switch(dev->ds);
 	if (ret) {
 		dev->dev_ops->exit(dev);
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index a4de58847dea..54589736398a 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -157,6 +157,7 @@ struct ksz_device {
 	phy_interface_t compat_interface;
 	bool synclko_125;
 	bool synclko_disable;
+	bool rmii_clk_internal;
 
 	struct vlan_table *vlan_cache;
 
-- 
2.11.0

[PATCH net-next 2/2] dt-bindings: net: microchip,ksz: document microchip,rmii-clk-internal

[Ante Knezic]

Add documentation for selecting reference rmii clock on KSZ88X3 devices

Signed-off-by: Ante Knezic <ante.knezic@helmholz.de>
---
 Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
index e51be1ac0362..3df5d2e72dba 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
@@ -49,6 +49,12 @@ properties:
       Set if the output SYNCLKO clock should be disabled. Do not mix with
       microchip,synclko-125.
 
+  microchip,rmii-clk-internal:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Set if the RMII reference clock should be provided internally. Applies only
+      to KSZ88X3 devices.
+
 required:
   - compatible
   - reg
-- 
2.11.0

Re: [PATCH net-next 2/2] dt-bindings: net: microchip,ksz: document microchip,rmii-clk-internal

[Andrew Lunn]

> +  microchip,rmii-clk-internal:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Set if the RMII reference clock should be provided internally. Applies only
> +      to KSZ88X3 devices.

It would be good to define what happens when
microchip,rmii-clk-internal is not present. Looking at the code, you
leave it unchanged. Is that what we want, or do we want to force it to
external?

	Andrew

[PATCH net-next 2/2] dt-bindings: net: microchip,ksz: document microchip,rmii-clk-internal

[Ante Knezic]

On Tue, 10 Oct 2023 15:25:44 +0200, Andrew Lunn wrote:
>> +  microchip,rmii-clk-internal:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description:
>> +      Set if the RMII reference clock should be provided internally. Applies only
>> +      to KSZ88X3 devices.
>
>It would be good to define what happens when
>microchip,rmii-clk-internal is not present. Looking at the code, you
>leave it unchanged. Is that what we want, or do we want to force it to
>external?
>
>	Andrew

Default register setting is to use external RMII clock (which is btw only 
available option for other KSZ devices - as far as I am aware) so I guess 
theres no need to force it to external clock?

Re: [PATCH net-next 2/2] dt-bindings: net: microchip,ksz: document microchip,rmii-clk-internal

[Andrew Lunn]

On Tue, Oct 10, 2023 at 03:41:39PM +0200, Ante Knezic wrote:
> On Tue, 10 Oct 2023 15:25:44 +0200, Andrew Lunn wrote:
> >> +  microchip,rmii-clk-internal:
> >> +    $ref: /schemas/types.yaml#/definitions/flag
> >> +    description:
> >> +      Set if the RMII reference clock should be provided internally. Applies only
> >> +      to KSZ88X3 devices.
> >
> >It would be good to define what happens when
> >microchip,rmii-clk-internal is not present. Looking at the code, you
> >leave it unchanged. Is that what we want, or do we want to force it to
> >external?
> >
> >	Andrew
> 
> Default register setting is to use external RMII clock (which is btw only 
> available option for other KSZ devices - as far as I am aware) so I guess 
> theres no need to force it to external clock?

We just need to watch out for a bootloader setting it. Or is it really
guaranteed to be false, because the DSA driver always does a device reset,
removing all existing configuration?

I prefer it is unambiguously documented what not having the property
means.

	Andrew

Re: [PATCH net-next 2/2] dt-bindings: net: microchip,ksz: document microchip,rmii-clk-internal

[Conor Dooley]

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

On Tue, Oct 10, 2023 at 03:18:54PM +0200, Ante Knezic wrote:
> Add documentation for selecting reference rmii clock on KSZ88X3 devices
> 
> Signed-off-by: Ante Knezic <ante.knezic@helmholz.de>
> ---
>  Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> index e51be1ac0362..3df5d2e72dba 100644
> --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> @@ -49,6 +49,12 @@ properties:
>        Set if the output SYNCLKO clock should be disabled. Do not mix with
>        microchip,synclko-125.
>  
> +  microchip,rmii-clk-internal:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Set if the RMII reference clock should be provided internally.

> Applies only
> +      to KSZ88X3 devices.

This should be enforced by the schema, the example schema in the docs
should show you how to do this.

Thanks,
Conor.

> +
>  required:
>    - compatible
>    - reg
> -- 
> 2.11.0
> 
> 

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

[PATCH net-next 2/2] dt-bindings: net: microchip,ksz: document microchip,rmii-clk-internal

[Ante Knezic]

On Tue, 10 Oct 2023 16:25:55 +0100, Conor Dooley wrote:
> On Tue, Oct 10, 2023 at 03:18:54PM +0200, Ante Knezic wrote:
> > Add documentation for selecting reference rmii clock on KSZ88X3 devices
> > 
> > Signed-off-by: Ante Knezic <ante.knezic@helmholz.de>
> > ---
> >  Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > index e51be1ac0362..3df5d2e72dba 100644
> > --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > @@ -49,6 +49,12 @@ properties:
> >        Set if the output SYNCLKO clock should be disabled. Do not mix with
> >        microchip,synclko-125.
> >  
> > +  microchip,rmii-clk-internal:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description:
> > +      Set if the RMII reference clock should be provided internally.
> 
> > Applies only
> > +      to KSZ88X3 devices.
> 
> This should be enforced by the schema, the example schema in the docs
> should show you how to do this.

I am guessing you are refering to limiting the property to ksz88x3 devices?
Something like:

if:
  properties:
    compatible:
      enum:
        - microchip,ksz8863
        - microchip,ksz8873
then:
  properties:
    microchip,rmii-clk-internal:
      $ref: /schemas/types.yaml#/definitions/flag
      description:
        Set if the RMII reference clock is provided internally. Otherwise
        reference clock should be provided externally.

Thanks,
Ante

[PATCH net-next 2/2] dt-bindings: net: microchip,ksz: document microchip,rmii-clk-internal

[Ante Knezic]

On Tue, 10 Oct 2023 15:57:34 +0200 Anrew Lunn wrote:
>On Tue, Oct 10, 2023 at 03:41:39PM +0200, Ante Knezic wrote:
>> On Tue, 10 Oct 2023 15:25:44 +0200, Andrew Lunn wrote:
>> >> +  microchip,rmii-clk-internal:
>> >> +    $ref: /schemas/types.yaml#/definitions/flag
>> >> +    description:
>> >> +      Set if the RMII reference clock should be provided internally. Applies only
>> >> +      to KSZ88X3 devices.
>> >
>> >It would be good to define what happens when
>> >microchip,rmii-clk-internal is not present. Looking at the code, you
>> >leave it unchanged. Is that what we want, or do we want to force it to
>> >external?
>> >
>> >	Andrew
>> 
>> Default register setting is to use external RMII clock (which is btw only 
>> available option for other KSZ devices - as far as I am aware) so I guess 
>> theres no need to force it to external clock?
>
>We just need to watch out for a bootloader setting it. Or is it really
>guaranteed to be false, because the DSA driver always does a device reset,
>removing all existing configuration?
>
>I prefer it is unambiguously documented what not having the property
>means.
>
>	Andrew

The bootloader case might be a issue if the reset-gpio property is not defined
for the switch. In this case we should probably enforce the value either way.
I will do the changes and repost.

Thanks for feedback,
Ante

Re: [PATCH net-next 2/2] dt-bindings: net: microchip,ksz: document microchip,rmii-clk-internal

[Conor Dooley]

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

On Wed, Oct 11, 2023 at 03:26:00PM +0200, Ante Knezic wrote:
> On Tue, 10 Oct 2023 16:25:55 +0100, Conor Dooley wrote:
> > On Tue, Oct 10, 2023 at 03:18:54PM +0200, Ante Knezic wrote:
> > > Add documentation for selecting reference rmii clock on KSZ88X3 devices
> > > 
> > > Signed-off-by: Ante Knezic <ante.knezic@helmholz.de>
> > > ---
> > >  Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > > index e51be1ac0362..3df5d2e72dba 100644
> > > --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > > @@ -49,6 +49,12 @@ properties:
> > >        Set if the output SYNCLKO clock should be disabled. Do not mix with
> > >        microchip,synclko-125.
> > >  
> > > +  microchip,rmii-clk-internal:
> > > +    $ref: /schemas/types.yaml#/definitions/flag
> > > +    description:
> > > +      Set if the RMII reference clock should be provided internally.
> > 
> > > Applies only
> > > +      to KSZ88X3 devices.
> > 
> > This should be enforced by the schema, the example schema in the docs
> > should show you how to do this.
> 
> I am guessing you are refering to limiting the property to ksz88x3 devices?
> Something like:
> 
> if:
>   properties:
>     compatible:
>       enum:
>         - microchip,ksz8863
>         - microchip,ksz8873
> then:
>   properties:
>     microchip,rmii-clk-internal:
>       $ref: /schemas/types.yaml#/definitions/flag
>       description:
>         Set if the RMII reference clock is provided internally. Otherwise
>         reference clock should be provided externally.

Not quite. The definition of the property should be outside the if/then,
but one should be used to allow/disallow the property.

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

Re: [PATCH net-next 2/2] dt-bindings: net: microchip,ksz: document microchip,rmii-clk-internal

[Conor Dooley]

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

On Tue, Oct 17, 2023 at 02:59:46PM +0200, Andrew Lunn wrote:
> > The switch always provides it's own external reference, wut? Why would
> > anyone actually bother doing this instead of just using the internal
> > reference?
> 
> I think you are getting provider and consumer mixed up.

The comment suggested that it was acting as both, via an external
loopback:
> > > In both cases (external and internal), the KSZ88X3 is actually providing the
> > > RMII reference clock. Difference is only will the clock be routed as external
> > > copper track (pin REFCLKO -> pin REFCLKI), or will it be routed internally.

If there's another interpretation for that, it's lost on me.
A later mail goes on to say:
> > > The KSZ88x3 does not have to provide the reference clock, it can be provided 
> > > externally, by some other device, for example the uC.

So I think I was just picking up on a mistaken explanation.

> Lets simplify to just a MAC and a PHY. There needs to be a shared
> clock between these two. Sometimes the PHY is the provider and the MAC
> is the consumer, sometimes the MAC is the provider, and the PHY is the
> consumer. Sometimes the hardware gives you no choices, sometimes it
> does. Sometimes a third party provides the clock, and both are
> consumers.
> 
> With the KSZ, we are talking about a switch, so there are multiple
> MACs and PHYs. They can all share the same clock, so long as you have
> one provider, and the rest are consumers. Or each pair can figure out
> its provider/consumer etc.

Thanks for the explanation. I'm still not really sure why someone would
want to employ external loopback, instead of the internal one though.

> How this is described in DT has evolved over time. We don't have clean
> clock provider/consumer relationships. The PHYs and MACs are generally
> not CCF consumers/providers. They just have a property to enable the
> to output a clock, or maybe a property to disable the clock output in
> order to save power. There are a few exceptions, but that tends to be
> where the clock provider is already CCF clock, e.g. a SoC clock.

Yeah, I did acknowledge that at the end of my mail (although I managed
to typo "that ship has sailed").
Doing ccf stuff doesn't seem viable given there's currently no required
clocks in the binding despite there likely being some in use.

I'm fine acking the binding with the change I suggested, I was just
looking to understand why a clocks property could not be used and I
think I have my answer to that now :)

Cheers,
Conor.

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

Re: [PATCH net-next 2/2] dt-bindings: net: microchip,ksz: document microchip,rmii-clk-internal

[Andrew Lunn]

> The switch always provides it's own external reference, wut? Why would
> anyone actually bother doing this instead of just using the internal
> reference?

I think you are getting provider and consumer mixed up.

Lets simplify to just a MAC and a PHY. There needs to be a shared
clock between these two. Sometimes the PHY is the provider and the MAC
is the consumer, sometimes the MAC is the provider, and the PHY is the
consumer. Sometimes the hardware gives you no choices, sometimes it
does. Sometimes a third party provides the clock, and both are
consumers.

With the KSZ, we are talking about a switch, so there are multiple
MACs and PHYs. They can all share the same clock, so long as you have
one provider, and the rest are consumers. Or each pair can figure out
its provider/consumer etc.

How this is described in DT has evolved over time. We don't have clean
clock provider/consumer relationships. The PHYs and MACs are generally
not CCF consumers/providers. They just have a property to enable the
to output a clock, or maybe a property to disable the clock output in
order to save power. There are a few exceptions, but that tends to be
where the clock provider is already CCF clock, e.g. a SoC clock.

      Andrew

Re: [PATCH net-next 2/2] dt-bindings: net: microchip,ksz: document microchip,rmii-clk-internal

[Conor Dooley]

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

On Mon, Oct 16, 2023 at 09:53:49AM +0200, Ante Knezic wrote:
> On Thu, 12 Oct 2023 16:18:09 +0100, Conor Dooley wrote:
> > On Thu, Oct 12, 2023 at 12:55:56PM +0200, Ante Knezic wrote:
> > > Add documentation for selecting reference rmii clock on KSZ88X3 devices
> > > 
> > > Signed-off-by: Ante Knezic <ante.knezic@helmholz.de>
> > > ---
> > >  .../devicetree/bindings/net/dsa/microchip,ksz.yaml    | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > > index 41014f5c01c4..eaa347b04db1 100644
> > > --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > > @@ -72,6 +72,25 @@ properties:
> > >    interrupts:
> > >      maxItems: 1
> > >  
> > > +  microchip,rmii-clk-internal:
> > > +    $ref: /schemas/types.yaml#/definitions/flag
> > > +    description:
> > > +      Set if the RMII reference clock is provided internally. Otherwise
> > > +      reference clock should be provided externally.
> > 
> > I regret not asking this on the previous iteration - how come you need a
> > custom property? In the externally provided case would there not be a
> > clocks property pointing to the RMII reference clock, that would be
> > absent when provided by the itnernal reference?

> In both cases (external and internal), the KSZ88X3 is actually providing the
> RMII reference clock.
> Difference is only will the clock be routed as external
> copper track (pin REFCLKO -> pin REFCLKI), or will it be routed internally.

The switch always provides it's own external reference, wut? Why would
anyone actually bother doing this instead of just using the internal
reference?

> So, this should not affect the clock relation between the uC and the switch
> device?

> This property has no effect if KSZ88X3 is not providing the reference clock.

This appears to contradict with the above, unless I am misunderstanding
something.

> Maybe I should provide more info in the commit message of both patches as well?

What I would have expected to see is that when the reference clock is
provided externally that there would be a clocks property in the DT
node, pointing at that external clock & when there was not, then
no property. Likely that ship has already said, as I don't see clocks
present in the current binding. How does the driver get the frequency of
the RMII reference clock when an external reference is provided?

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

[PATCH net-next 2/2] dt-bindings: net: microchip,ksz: document microchip,rmii-clk-internal

[Ante Knezic]

On Thu, 12 Oct 2023 16:18:09 +0100, Conor Dooley wrote:
> On Thu, Oct 12, 2023 at 12:55:56PM +0200, Ante Knezic wrote:
> > Add documentation for selecting reference rmii clock on KSZ88X3 devices
> > 
> > Signed-off-by: Ante Knezic <ante.knezic@helmholz.de>
> > ---
> >  .../devicetree/bindings/net/dsa/microchip,ksz.yaml    | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > index 41014f5c01c4..eaa347b04db1 100644
> > --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > @@ -72,6 +72,25 @@ properties:
> >    interrupts:
> >      maxItems: 1
> >  
> > +  microchip,rmii-clk-internal:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description:
> > +      Set if the RMII reference clock is provided internally. Otherwise
> > +      reference clock should be provided externally.
> 
> I regret not asking this on the previous iteration - how come you need a
> custom property? In the externally provided case would there not be a
> clocks property pointing to the RMII reference clock, that would be
> absent when provided by the itnernal reference?
> 
> Cheers,
> Conor.

In both cases (external and internal), the KSZ88X3 is actually providing the
RMII reference clock. Difference is only will the clock be routed as external
copper track (pin REFCLKO -> pin REFCLKI), or will it be routed internally.
So, this should not affect the clock relation between the uC and the switch
device? 
This property has no effect if KSZ88X3 is not providing the reference clock.
Maybe I should provide more info in the commit message of both patches as well?