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

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

[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

 .../devicetree/bindings/net/dsa/microchip,ksz.yaml    | 19 +++++++++++++++++++
 drivers/net/dsa/microchip/ksz8795.c                   |  5 +++++
 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, 31 insertions(+)

-- 
2.11.0

[PATCH net-next v2 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. If property is not set, reference
clock needs to be provided externally.

Signed-off-by: Ante Knezic <ante.knezic@helmholz.de>
---
 drivers/net/dsa/microchip/ksz8795.c     | 5 +++++
 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, 12 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 91aba470fb2f..78f3a668aa99 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -1434,6 +1434,11 @@ 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))
+		ksz_cfg(dev, KSZ88X3_REG_FVID_AND_HOST_MODE,
+			KSZ88X3_PORT3_RMII_CLK_INTERNAL,
+			dev->rmii_clk_internal);
+
 	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 3c9dae53e4d8..beca974e0171 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 KSZ88X3_REG_FVID_AND_HOST_MODE  0xC6
+#define KSZ88X3_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 b800ace40ce1..0a0a53ce5b1b 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -4160,6 +4160,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 8842efca0871..e5b0445fe2ca 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -163,6 +163,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

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

[Vladimir Oltean]

Commit message: "net: dsa: microchip: ".

On Thu, Oct 12, 2023 at 12:55:55PM +0200, Ante Knezic wrote:
> 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. If property is not set, reference
> clock needs to be provided externally.
> 
> Signed-off-by: Ante Knezic <ante.knezic@helmholz.de>
> ---
>  drivers/net/dsa/microchip/ksz8795.c     | 5 +++++
>  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, 12 insertions(+)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> index 91aba470fb2f..78f3a668aa99 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -1434,6 +1434,11 @@ 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))
> +		ksz_cfg(dev, KSZ88X3_REG_FVID_AND_HOST_MODE,
> +			KSZ88X3_PORT3_RMII_CLK_INTERNAL,
> +			dev->rmii_clk_internal);
> +

Can this be done in dev->dev_ops->phylink_mac_config() (which so far has no implementation)
for port 3 of ksz88x3?

>  	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 3c9dae53e4d8..beca974e0171 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 KSZ88X3_REG_FVID_AND_HOST_MODE  0xC6
> +#define KSZ88X3_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 b800ace40ce1..0a0a53ce5b1b 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -4160,6 +4160,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");

Port property.

> +
>  	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 8842efca0871..e5b0445fe2ca 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -163,6 +163,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 v2 1/2] net:dsa:microchip: add property to select internal RMII reference clock

[Ante Knezic]

On Mon, 16 Oct 2023 13:37:08 +0300, Vladimir Oltean wrote:
> > diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> > index 91aba470fb2f..78f3a668aa99 100644
> > --- a/drivers/net/dsa/microchip/ksz8795.c
> > +++ b/drivers/net/dsa/microchip/ksz8795.c
> > @@ -1434,6 +1434,11 @@ 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))
> > +	        ksz_cfg(dev, KSZ88X3_REG_FVID_AND_HOST_MODE,
> > +	                KSZ88X3_PORT3_RMII_CLK_INTERNAL,
> > +	                dev->rmii_clk_internal);
> > +
> 
> Can this be done in dev->dev_ops->phylink_mac_config() (which so far has no implementation)
> for port 3 of ksz88x3?

> >  	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 3c9dae53e4d8..beca974e0171 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 KSZ88X3_REG_FVID_AND_HOST_MODE  0xC6
> > +#define KSZ88X3_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 b800ace40ce1..0a0a53ce5b1b 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.c
> > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > @@ -4160,6 +4160,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");
> 
> Port property.

Yes, I guess we can do it in phylink_mac_config. Or perhaps it would be better
to put everything in ksz8_port_setup if you suggest this is a port specific 
property and not global?
Something like:

@@ -1312,8 +1314,15 @@ void ksz8_port_setup(struct ksz_device *dev, int port, bool cpu_port)
        ksz_port_cfg(dev, port, P_PRIO_CTRL, PORT_802_1P_ENABLE, true);

        if (cpu_port) {
-               if (!ksz_is_ksz88x3(dev))
+               if (!ksz_is_ksz88x3(dev)) {
                        ksz8795_cpu_interface_select(dev, port);
+               } else {
+                       dev->rmii_clk_internal = of_property_read_bool(dev->dev->of_node,
+                                                                      "microchip,rmii-clk-internal");
+                       ksz_cfg(dev, KSZ88X3_REG_FVID_AND_HOST_MODE,
+                               KSZ88X3_PORT3_RMII_CLK_INTERNAL,
+                               dev->rmii_clk_internal);
+               }

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

[Ante Knezic]

> > +  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.
> > +
> > +if:
> > +  not:
> > +    properties:
> > +      compatible:
> > +        enum:
> > +          - microchip,ksz8863
> > +          - microchip,ksz8873
> > +then:
> > +  not:
> > +    required:
> > +      - microchip,rmii-clk-internal
> 
> I think that what you want to express is that microchip,rmii-clk-internal
> is only defined for microchip,ksz8863 and microchip,ksz8873.
> Can't you describe that as "if: properties: compatible: (...) then:
> properties: microchip,rmii-clk-internal"?

If I understood you correctly you are refering to a solution 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.

This was already suggested in v1, but was not a satisfactory solution
according to Mr. Conor Dooley:

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

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

[Conor Dooley]

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

On Tue, Oct 17, 2023 at 09:35:48AM +0200, Ante Knezic wrote:
> > > +  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.
> > > +
> > > +if:
> > > +  not:
> > > +    properties:
> > > +      compatible:
> > > +        enum:
> > > +          - microchip,ksz8863
> > > +          - microchip,ksz8873
> > > +then:
> > > +  not:
> > > +    required:
> > > +      - microchip,rmii-clk-internal

I think this bit can become the slightly simpler
	then:
	  properties:
	     microchip,rmii-clk-internal: false

> > I think that what you want to express is that microchip,rmii-clk-internal
> > is only defined for microchip,ksz8863 and microchip,ksz8873.
> > Can't you describe that as "if: properties: compatible: (...) then:
> > properties: microchip,rmii-clk-internal"?
> 
> If I understood you correctly you are refering to a solution 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.
> 
> This was already suggested in v1, but was not a satisfactory solution
> according to Mr. Conor Dooley:

Yeah, we prefer not to have the property definitions inside the
conditionals, but rather constrain or allow/disallow them there.

Cheers,
Conor.

> 
> >> 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 v2 2/2] dt-bindings: net: microchip,ksz: document microchip,rmii-clk-internal

[Vladimir Oltean]

On Tue, Oct 17, 2023 at 08:48:27AM +0100, Conor Dooley wrote:
> On Tue, Oct 17, 2023 at 09:35:48AM +0200, Ante Knezic wrote:
> > > > +  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.
> > > > +
> > > > +if:
> > > > +  not:
> > > > +    properties:
> > > > +      compatible:
> > > > +        enum:
> > > > +          - microchip,ksz8863
> > > > +          - microchip,ksz8873
> > > > +then:
> > > > +  not:
> > > > +    required:
> > > > +      - microchip,rmii-clk-internal
> 
> I think this bit can become the slightly simpler
> 	then:
> 	  properties:
> 	     microchip,rmii-clk-internal: false

This looks better. I don't understand how the original formulation worked
("not: required:" when the property was never "required" in the first
place - does that do anything?), but I understand how this one does.

> > > I think that what you want to express is that microchip,rmii-clk-internal
> > > is only defined for microchip,ksz8863 and microchip,ksz8873.
> > > Can't you describe that as "if: properties: compatible: (...) then:
> > > properties: microchip,rmii-clk-internal"?
> > 
> > If I understood you correctly you are refering to a solution 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.
> > 
> > This was already suggested in v1, but was not a satisfactory solution
> > according to Mr. Conor Dooley:
> 
> Yeah, we prefer not to have the property definitions inside the
> conditionals, but rather constrain or allow/disallow them there.
> 
> Cheers,
> Conor.

Ok, now you know I didn't open the discussion on v1 :)

[PATCH net-next v2 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>
---
 .../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.
+
+if:
+  not:
+    properties:
+      compatible:
+        enum:
+          - microchip,ksz8863
+          - microchip,ksz8873
+then:
+  not:
+    required:
+      - microchip,rmii-clk-internal
+
+
 required:
   - compatible
   - reg
-- 
2.11.0

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

[Conor Dooley]

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

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.

> +
> +if:
> +  not:
> +    properties:
> +      compatible:
> +        enum:
> +          - microchip,ksz8863
> +          - microchip,ksz8873
> +then:
> +  not:
> +    required:
> +      - microchip,rmii-clk-internal
> +
> +
>  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 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?

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 v2 2/2] dt-bindings: net: microchip,ksz: document microchip,rmii-clk-internal

[Ante Knezic]

On Tue, 17 Oct 2023 09:00:15 +0100, Conor Dooley wrote:

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

Thats a good question... Other KSZ chips don't have the ability to route clock
internally, and these two (ksz8863 and ksz8873) are actually by default the
expecting to route it externally. Why this is so is a matter of HW design. 
The KSZ88x3 does not have to provide the reference clock, it can be provided 
externally, by some other device, for example the uC. 
BUT in case when it is provided by the switch we have the option to route the 
internal block that is generating the clock to the clock input externally 
(as a copper track) or internally.
To quote the manual:
"When EN_REFCLKO_3 is high, KSZ8863RLL outputs a 50 MHz in REFCLKO_3. 
Register 198 bit [3] is used to select the internal or external reference clock.
Internal reference clock means that the clock for the RMII of KSZ8863RLL is
provided by KSZ8863RLL internally and the REFCLKI_3 pin is unconnected. For the 
external reference clock, the clock provides to KSZ8863RLL via REFCLKI_3.
If KSZ8863RLL does not provide the reference clock, this 50 MHz reference clock
with divide-by-2 (25 MHz) has to be used in X1 pin instead of the 25 MHz 
crystal, since the clock skew of these two clock sources impacts the RMII timing
before Rev A3 part. The Rev A3 part can connect the external 50 MHz reference 
clock to X1 pin and SMTXC3/REFCLKI_3 pins directly with strap pins of pin 17 
SMTXD33/EN_REFCLKO_3 and pin 18 SMTXD32 to be pulled down."

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

There are actually 5 RMII clock configuration modes depending on the
setting of the EN_REFCLKO_3, register 0xC6 and SMTXD32 pin (which affects
the expected clock, 25 or 50 Mhz), but the patch covers the case in which
the switch is generating the reference clock (outputing it to REFCLKO_3), 
because EN_REFCLK0_3 is pulled up. By clearing/setting the 0xC6 bit 3 we can 
choose whether to connect the REFCLKO to REFCLKI externally, or internally.

If reference clock is being provided to the ksz88x3 by some other device,
then there is no point in setting the 0xC6 bit 3 because other device is 
providing the clock to REFCLKI_3 and REFCLKO should not be connected and 
should not generate the clock (as EN_REFCLKI is pulled down).

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

In case when ksz88x3 is generating the reference clock the devices rmii 
interface should be configured to external clock usage, for example something
like rmii-clock-ext for TI cpsw. This should be true regardles of whether
the ksz88x3 node has "microchip,rmii-clk-internal" set or not.

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: 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 v2 2/2] dt-bindings: net: microchip,ksz: document microchip,rmii-clk-internal

[Vladimir Oltean]

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.
> +
> +if:
> +  not:
> +    properties:
> +      compatible:
> +        enum:
> +          - microchip,ksz8863
> +          - microchip,ksz8873
> +then:
> +  not:
> +    required:
> +      - microchip,rmii-clk-internal

I think that what you want to express is that microchip,rmii-clk-internal
is only defined for microchip,ksz8863 and microchip,ksz8873.
Can't you describe that as "if: properties: compatible: (...) then:
properties: microchip,rmii-clk-internal"?

Also, this is a port interface property, so I would like to see it in
the xMII port and not global to the switch. The KSZ8873/KSZ8863 is
somewhat special in that it only contains a single xMII port, but that
doesn't mean that RMII settings are switch-wide.

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

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

[Vladimir Oltean]

On Thu, Oct 12, 2023 at 12:55:54PM +0200, Ante Knezic wrote:
> 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

The dt-bindings patch should be placed before the kernel patch.