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

[PATCH net-next v4 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.

---
V4:
  - remove rmii_clk_internal from ksz_device, as its not needed any more
  - move rmii clk config as well as ksz8795_cpu_interface_select to 
    ksz8_config_cpu_port
V3: 
  - move ksz_cfg from global switch config to port config as suggested by Vladimir
    Oltean
  - reverse patch order as suggested by Vladimir Oltean
  - adapt dt schema as suggested by Conor Dooley
V2: 
  - don't rely on default register settings - enforce set/clear property as
    suggested by Andrew Lunn
  - enforce dt schema as suggested by Conor Dooley

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

 .../devicetree/bindings/net/dsa/microchip,ksz.yaml | 17 +++++++++++++
 drivers/net/dsa/microchip/ksz8795.c                | 28 +++++++++++++++++-----
 drivers/net/dsa/microchip/ksz8795_reg.h            |  3 +++
 3 files changed, 42 insertions(+), 6 deletions(-)

-- 
2.11.0

[PATCH net-next v4 1/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      | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
index 41014f5c01c4..624feb1bb9be 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
@@ -72,6 +72,23 @@ 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:
+  properties:
+    microchip,rmii-clk-internal: false
+
 required:
   - compatible
   - reg
-- 
2.11.0

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

[Conor Dooley]

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

On Fri, Oct 20, 2023 at 04:25:03PM +0200, Ante Knezic wrote:
> Add documentation for selecting reference rmii clock on KSZ88X3 devices
> 
> Signed-off-by: Ante Knezic <ante.knezic@helmholz.de>

Something in the commit message explaining why a property is needed
rather than using the regular old clocks property would be good.
Please do that if you re-spin. Otherwise,

Acked-by: Conor Dooley <conor.dooley@microchip.com>

(and despite the domain, I have nothing to do with switches...)

Cheers,
Conor.

> ---
>  .../devicetree/bindings/net/dsa/microchip,ksz.yaml      | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> index 41014f5c01c4..624feb1bb9be 100644
> --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> @@ -72,6 +72,23 @@ 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:
> +  properties:
> +    microchip,rmii-clk-internal: false
> +
>  required:
>    - compatible
>    - reg
> -- 
> 2.11.0
> 

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

[PATCH net-next v4 2/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.

While at it, move the ksz8795_cpu_interface_select() to
ksz8_config_cpu_port() to get a cleaner call path for cpu port.

Signed-off-by: Ante Knezic <ante.knezic@helmholz.de>
---
 drivers/net/dsa/microchip/ksz8795.c     | 28 ++++++++++++++++++++++------
 drivers/net/dsa/microchip/ksz8795_reg.h |  3 +++
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 4bf4d67557dc..b0a305b7c37e 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -1358,6 +1358,9 @@ static void ksz8795_cpu_interface_select(struct ksz_device *dev, int port)
 {
 	struct ksz_port *p = &dev->ports[port];
 
+	if (!ksz_is_ksz87xx(dev))
+		return;
+
 	if (!p->interface && dev->compat_interface) {
 		dev_warn(dev->dev,
 			 "Using legacy switch \"phy-mode\" property, because it is missing on port %d node. "
@@ -1391,18 +1394,28 @@ void ksz8_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 	/* enable 802.1p priority */
 	ksz_port_cfg(dev, port, P_PRIO_CTRL, PORT_802_1P_ENABLE, true);
 
-	if (cpu_port) {
-		if (!ksz_is_ksz88x3(dev))
-			ksz8795_cpu_interface_select(dev, port);
-
+	if (cpu_port)
 		member = dsa_user_ports(ds);
-	} else {
+	else
 		member = BIT(dsa_upstream_port(ds, port));
-	}
 
 	ksz8_cfg_port_member(dev, port, member);
 }
 
+static void ksz88x3_config_rmii_clk(struct ksz_device *dev)
+{
+	bool rmii_clk_internal;
+
+	if (!ksz_is_ksz88x3(dev))
+		return;
+
+	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, rmii_clk_internal);
+}
+
 void ksz8_config_cpu_port(struct dsa_switch *ds)
 {
 	struct ksz_device *dev = ds->priv;
@@ -1419,6 +1432,9 @@ void ksz8_config_cpu_port(struct dsa_switch *ds)
 
 	ksz8_port_setup(dev, dev->cpu_port, true);
 
+	ksz8795_cpu_interface_select(dev, dev->cpu_port);
+	ksz88x3_config_rmii_clk(dev);
+
 	for (i = 0; i < dev->phy_port_cnt; i++) {
 		ksz_port_stp_state_set(ds, i, BR_STATE_DISABLED);
 	}
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)
-- 
2.11.0

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

[Vladimir Oltean]

On Fri, Oct 20, 2023 at 04:25:04PM +0200, Ante Knezic wrote:
> +static void ksz88x3_config_rmii_clk(struct ksz_device *dev)
> +{
> +	bool rmii_clk_internal;
> +
> +	if (!ksz_is_ksz88x3(dev))
> +		return;
> +
> +	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, rmii_clk_internal);
> +}

Sorry, I didn't realize on v3 that you didn't completely apply my
feedback on v2. Can "microchip,rmii-clk-internal" be a port device tree
property? You have indeed moved its parsing to port code, but it is
still located directly under the switch node in the device tree.

I'm thinking that if this property was also applicable to other switches
with multiple RMII ports, the setting would be per port rather than global.

Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select

[Ante Knezic]

On Fri, 20 Oct 2023 17:37:59 +0300, Vladimir Oltean wrote:

> Sorry, I didn't realize on v3 that you didn't completely apply my
> feedback on v2. Can "microchip,rmii-clk-internal" be a port device tree
> property? You have indeed moved its parsing to port code, but it is
> still located directly under the switch node in the device tree.
> 
> I'm thinking that if this property was also applicable to other switches
> with multiple RMII ports, the setting would be per port rather than global.

As far as I am aware only the KSZ8863 and KSZ8873 have this property available,
but the biggger issue might be in scaling this to port property as the register
"Forward Invalid VID Frame and Host Mode" where the setting is applied is
located under "Advanced Control Registers" section which is actually global at
least looking from the switch point of view. Usually port properties are more
applicable when registers in question are located under "Port Registers" section.
This is somewhat similar to for example enabling the tail tag mode which is 
again used only by the port 3 interface and is control from "Global Control 1"
register.
With this in mind - if you still believe we should move this to port dt 
property, then should we forbid setting the property for any other port other 
than port 3, and can/should this be enforced by the dt schema?

Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select

[Oleksij Rempel]

On Mon, Oct 23, 2023 at 09:27:00AM +0200, Ante Knezic wrote:
> On Fri, 20 Oct 2023 17:37:59 +0300, Vladimir Oltean wrote:
> 
> > Sorry, I didn't realize on v3 that you didn't completely apply my
> > feedback on v2. Can "microchip,rmii-clk-internal" be a port device tree
> > property? You have indeed moved its parsing to port code, but it is
> > still located directly under the switch node in the device tree.
> > 
> > I'm thinking that if this property was also applicable to other switches
> > with multiple RMII ports, the setting would be per port rather than global.
> 
> As far as I am aware only the KSZ8863 and KSZ8873 have this property available,
> but the biggger issue might be in scaling this to port property as the register
> "Forward Invalid VID Frame and Host Mode" where the setting is applied is
> located under "Advanced Control Registers" section which is actually global at
> least looking from the switch point of view. Usually port properties are more
> applicable when registers in question are located under "Port Registers" section.
> This is somewhat similar to for example enabling the tail tag mode which is 
> again used only by the port 3 interface and is control from "Global Control 1"
> register.
> With this in mind - if you still believe we should move this to port dt 
> property, then should we forbid setting the property for any other port other 
> than port 3, and can/should this be enforced by the dt schema?
> 

If I see it correctly, KSZ9897R supports RMII on two ports (6 and 7)
with configurable clock direction. See page 124 "5.2.3.2 XMII Port Control 1
Register"
http://ww1.microchip.com/downloads/en/DeviceDoc/00002330B.pdf

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select

[Ante Knezic]

On Mon, 23 Oct 2023 09:58:48 +0200, Oleksij Rempel wrote:

> If I see it correctly, KSZ9897R supports RMII on two ports (6 and 7)
> with configurable clock direction. See page 124 "5.2.3.2 XMII Port Control 1
> Register"
> http://ww1.microchip.com/downloads/en/DeviceDoc/00002330B.pdf

Clock direction is possible I guess with other devices as well, but I don't see
this specific property (routing REFCLKO to REFCLKI internally when switch is
used as clock provider) for any other, including KSZ9897? 
I am no expert on micrel switches, but this to me looks like something specific
only to KSZ88X3 devices as the clocking seems a bit different on KSZ9897 and
alike. KSZ88X3 may generate clock to REFCLKO but still needs this clock fed
back to REFCLKI (or will be routed internally with the "microchip-rmii-internal"
property). This is managed differently on KSZ9897?

Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select

[Oleksij Rempel]

On Mon, Oct 23, 2023 at 10:22:30AM +0200, Ante Knezic wrote:
> On Mon, 23 Oct 2023 09:58:48 +0200, Oleksij Rempel wrote:
> 
> > If I see it correctly, KSZ9897R supports RMII on two ports (6 and 7)
> > with configurable clock direction. See page 124 "5.2.3.2 XMII Port Control 1
> > Register"
> > http://ww1.microchip.com/downloads/en/DeviceDoc/00002330B.pdf
> 
> Clock direction is possible I guess with other devices as well, but I don't see
> this specific property (routing REFCLKO to REFCLKI internally when switch is
> used as clock provider) for any other, including KSZ9897?
>
> I am no expert on micrel switches, but this to me looks like something specific
> only to KSZ88X3 devices as the clocking seems a bit different on KSZ9897 and
> alike. KSZ88X3 may generate clock to REFCLKO but still needs this clock fed
> back to REFCLKI (or will be routed internally with the "microchip-rmii-internal"
> property). This is managed differently on KSZ9897?

Here is KSZ8873 as initial reference:
https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/00002348A.pdf
3.3.9 RMII INTERFACE OPERATION:
"When EN_REFCLKO_3 is high, KSZ8873RLL will output a 50 MHz in REFCLKO_3.
Register 198 bit[3] is used to select internal or external reference
clock. Internal reference clock means that the clock for the RMII of
KSZ8873RLL will be provided by the KSZ8873RLL internally and the
REFCLKI_3 pin is unconnected. For the external reference clock, the
clock will provide to KSZ8873RLL via REFCLKI_3."

KSZ9897:
http://ww1.microchip.com/downloads/en/DeviceDoc/00002330B.pdf
4.11.2 REDUCED MEDIA INDEPENDENT INTERFACE (RMII)

"The user selects one of the two RMII clocking modes by setting the
appropriate strapping option. The clocking mode is selected separately
for ports 6 and 7.

While in RMII Normal Mode, the port will require an external 50MHz
signal to be input to TX_CLKx/REFCLKIx from an external source. This
mode is selected by strapping the appropriate pin (RXD6_1 for port 6;
RXD7_1 for port 7) high during reset.

While in RMII Clock Mode, the port will output a 50MHz clock on
RX_CLKx/REFCLKOx, which is derived from the 25MHz crystal or oscillator
attached to the XI clock input. The TX_CLKx/REFCLKIx input is unused in
this mode. This mode is selected by strapping the appropriate pin
(RXD6_1 for port 6; RXD7_1 for port 7) low during reset.
"

Information about corresponding bits I linked in previous email.

I do not see much differences.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select

[Ante Knezic]

On Mon, 23 Oct 2023 10:41:50 +0200, Oleksij Rempel wrote:

> Here is KSZ8873 as initial reference:
> https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/00002348A.pdf
> 3.3.9 RMII INTERFACE OPERATION:
> "When EN_REFCLKO_3 is high, KSZ8873RLL will output a 50 MHz in REFCLKO_3.
> Register 198 bit[3] is used to select internal or external reference
> clock. Internal reference clock means that the clock for the RMII of
> KSZ8873RLL will be provided by the KSZ8873RLL internally and the
> REFCLKI_3 pin is unconnected. For the external reference clock, the
> clock will provide to KSZ8873RLL via REFCLKI_3."
> 
> KSZ9897:
> http://ww1.microchip.com/downloads/en/DeviceDoc/00002330B.pdf
> 4.11.2 REDUCED MEDIA INDEPENDENT INTERFACE (RMII)

The upper paragraph refers to the case when switch is acting as a clock
provider (regardless whether its set as internal or external reference
clock). You can see this if you look at the next paragraph:
"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 ..."
So rmii-clk-internal property does not select whether switch is acting
as a clock provider or clock consumer which is what you are refering to
I believe? The clock provider/consumer is set via strapping pins.

Real case scenario: I have a board where switch is acting as a clock
provider, generating output to REFCLKO pin and feeding it to uC. 
This board does not have externally routed copper track from REFCLKO 
to REFCLKI, thus making the RMII interface not operable, unless the 
rmii-clk-internal bit is set.
If this bit is not set, only way to make it running is to solder a
jumper wire from REFCLKO to REFCLKI.

Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select

[Oleksij Rempel]

On Mon, Oct 23, 2023 at 10:57:50AM +0200, Ante Knezic wrote:
> On Mon, 23 Oct 2023 10:41:50 +0200, Oleksij Rempel wrote:
> 
> > Here is KSZ8873 as initial reference:
> > https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/00002348A.pdf
> > 3.3.9 RMII INTERFACE OPERATION:
> > "When EN_REFCLKO_3 is high, KSZ8873RLL will output a 50 MHz in REFCLKO_3.
> > Register 198 bit[3] is used to select internal or external reference
> > clock. Internal reference clock means that the clock for the RMII of
> > KSZ8873RLL will be provided by the KSZ8873RLL internally and the
> > REFCLKI_3 pin is unconnected. For the external reference clock, the
> > clock will provide to KSZ8873RLL via REFCLKI_3."
> > 
> > KSZ9897:
> > http://ww1.microchip.com/downloads/en/DeviceDoc/00002330B.pdf
> > 4.11.2 REDUCED MEDIA INDEPENDENT INTERFACE (RMII)
> 
> The upper paragraph refers to the case when switch is acting as a clock
> provider (regardless whether its set as internal or external reference
> clock). You can see this if you look at the next paragraph:
> "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 ..."
> So rmii-clk-internal property does not select whether switch is acting
> as a clock provider or clock consumer which is what you are refering to
> I believe? The clock provider/consumer is set via strapping pins.
> 
> Real case scenario: I have a board where switch is acting as a clock
> provider, generating output to REFCLKO pin and feeding it to uC. 
> This board does not have externally routed copper track from REFCLKO 
> to REFCLKI, thus making the RMII interface not operable, unless the 
> rmii-clk-internal bit is set.
> If this bit is not set, only way to make it running is to solder a
> jumper wire from REFCLKO to REFCLKI.

In case of KSZ8873 we seems to have something like:

Switch MAC<-.
            |
  PLL -> clk sel -> REFCLKO
            \-----< REFCLKI

Clock select in this case is controlled by Register 198 (0xC6).

In case of KSZ9897 we probably have something like:

Switch MAC<-.
            |
  PLL -> clk sel -> REFCLKO
            \--x--< REFCLKI
	       |
            Gate REFCLKI if REFCLKO is used.

In both cases:
- KSZ8873, Setting bit3 in Register 198 (0xC6) will control use of clk
  select
- KSZ9897, setting bit2 in Register 0xN301, will controll use of clk
  select and probably gate REFCLKI.

So far, it looks very similar to me and it is usually handled by
phy-mode rmii vs revrmii. Correct?

So, the main question is still, do we need this kind of configuration
per port or it is enough to have it per switch?

For some reasons KSZ8863MLL datasheet provides RMII clock select
configuration for two ports (port 1 and 3)
https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/KSZ8863MLL-FLL-RLL-Data-Sheet-00002335C.pdf
May be there are variants with two RMIIs?

Something similar but with multiple RMII interfaces seems to be
supported by KSZ8864CNX:
https://eu.mouser.com/datasheet/2/268/00002229A-1082534.pdf

And all KSZ9xxx series seems to handle it per port as well. 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select

[Ante Knezic]

On Mon, 23 Oct 2023 13:49:16 +0200, Oleksij Rempel wrote:

> In case of KSZ8873 we seems to have something like:
> 
> Switch MAC<-.
>             |
>   PLL -> clk sel -> REFCLKO
>             \-----< REFCLKI
> 
> Clock select in this case is controlled by Register 198 (0xC6).
> 
> In case of KSZ9897 we probably have something like:
> 
> Switch MAC<-.
>             |
>   PLL -> clk sel -> REFCLKO
>             \--x--< REFCLKI
>                |
>             Gate REFCLKI if REFCLKO is used.
> 
> In both cases:
> - KSZ8873, Setting bit3 in Register 198 (0xC6) will control use of clk
>   select
> - KSZ9897, setting bit2 in Register 0xN301, will controll use of clk
>   select and probably gate REFCLKI.
> 
> So far, it looks very similar to me and it is usually handled by
> phy-mode rmii vs revrmii. Correct?

Thats correct I guess with one important point: default setting for KSZ88X3 is
not to gate back to REFCLKI, while KSZ9897 will (correct me if I am wrong) 
automatically gate to REFCLKI and does not have the ability to gate/not-to-gate
REFCLKO to REFCLKI with any register setting. Thats more-less what this patch
is all about. Something that is automatically active (and can not be changed
during run-time?) on KSZ9897 needs to be manually configured on KSZ88X3.

> So, the main question is still, do we need this kind of configuration
> per port or it is enough to have it per switch?

Thats something on which more experienced developers/maintainers may have more
to say, my impression is that its somewhat specific to KSZ88X3 to have this
option available and the location of the register itself also makes a point on
its own. I may be wrong about this of course.

> For some reasons KSZ8863MLL datasheet provides RMII clock select
> configuration for two ports (port 1 and 3)
> https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/KSZ8863MLL-FLL-RLL-Data-Sheet-00002335C.pdf
> May be there are variants with two RMIIs?

As you noticed KSZ8863 provides configuration for port1 as well, but I was not
able to find any reference to what it may actually select. The product
identification system for ksz8863 does not mention devices with 2 RMII
interfaces. The KSZ8873 has this bit set to "Reserved" so I can't really
tell whats going on there...

> Something similar but with multiple RMII interfaces seems to be
> supported by KSZ8864CNX:
> https://eu.mouser.com/datasheet/2/268/00002229A-1082534.pdf
> 

As well as these datasheets are sometimes muddled and difficult to get a grasp of
by looking at the KSZ8873 datasheet it seems to me that it can select between
"clock" (default) and "normal" mode, which is something similar to the 
rmii-clk-internal property of KSZ88X3 devices, however this mode of operation 
seems to be selectable only at boot time by strap pins (see description of 
Register 12 (Global Control 10).

> And all KSZ9xxx series seems to handle it per port as well.

I guess you are refering to register 87 (RMII Management Control Register)?
But this only selects whether to enable clock output on RXC pin?

Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select

[Oleksij Rempel]

On Mon, Oct 23, 2023 at 02:41:30PM +0200, Ante Knezic wrote:
> On Mon, 23 Oct 2023 13:49:16 +0200, Oleksij Rempel wrote:
> 
> > In case of KSZ8873 we seems to have something like:
> > 
> > Switch MAC<-.
> >             |
> >   PLL -> clk sel -> REFCLKO
> >             \-----< REFCLKI
> > 
> > Clock select in this case is controlled by Register 198 (0xC6).
> > 
> > In case of KSZ9897 we probably have something like:
> > 
> > Switch MAC<-.
> >             |
> >   PLL -> clk sel -> REFCLKO
> >             \--x--< REFCLKI
> >                |
> >             Gate REFCLKI if REFCLKO is used.
> > 
> > In both cases:
> > - KSZ8873, Setting bit3 in Register 198 (0xC6) will control use of clk
> >   select
> > - KSZ9897, setting bit2 in Register 0xN301, will controll use of clk
> >   select and probably gate REFCLKI.
> > 
> > So far, it looks very similar to me and it is usually handled by
> > phy-mode rmii vs revrmii. Correct?
> 
> Thats correct I guess with one important point: default setting for KSZ88X3 is
> not to gate back to REFCLKI, while KSZ9897 will (correct me if I am wrong) 
> automatically gate to REFCLKI and does not have the ability to gate/not-to-gate
> REFCLKO to REFCLKI with any register setting. Thats more-less what this patch
> is all about. Something that is automatically active (and can not be changed
> during run-time?) on KSZ9897 needs to be manually configured on KSZ88X3.

If I see it correctly, in both cases there is only one bit to configure
direction. The code need to support two interface modes:
- PHY_INTERFACE_MODE_RMII (MAC mode) PLL is the clock provider. REFCLKO
  is used.
- PHY_INTERFACE_MODE_REVRMII (PHY mode) PLL is not used, REFCLKI is the
  clock provider.

Linkmodes for KSZ9xxx series are implemented in the ksz_set_xmii()
function.

I already did some work to configure CPU interface, where which can be at least
partially reused for your work:
https://lore.kernel.org/all/20230517121034.3801640-2-o.rempel@pengutronix.de/
(Looks I forgot to complete mainlining for this patch)

If implanted as described, no new devicetree properties will be needed.

> > So, the main question is still, do we need this kind of configuration
> > per port or it is enough to have it per switch?
> 
> Thats something on which more experienced developers/maintainers may have more
> to say, my impression is that its somewhat specific to KSZ88X3 to have this
> option available and the location of the register itself also makes a point on
> its own. I may be wrong about this of course.
> 
> > For some reasons KSZ8863MLL datasheet provides RMII clock select
> > configuration for two ports (port 1 and 3)
> > https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/KSZ8863MLL-FLL-RLL-Data-Sheet-00002335C.pdf
> > May be there are variants with two RMIIs?
> 
> As you noticed KSZ8863 provides configuration for port1 as well, but I was not
> able to find any reference to what it may actually select. The product
> identification system for ksz8863 does not mention devices with 2 RMII
> interfaces. The KSZ8873 has this bit set to "Reserved" so I can't really
> tell whats going on there...
> 
> > Something similar but with multiple RMII interfaces seems to be
> > supported by KSZ8864CNX:
> > https://eu.mouser.com/datasheet/2/268/00002229A-1082534.pdf
> > 
> 
> As well as these datasheets are sometimes muddled and difficult to get a grasp of
> by looking at the KSZ8873 datasheet it seems to me that it can select between
> "clock" (default) and "normal" mode, which is something similar to the 
> rmii-clk-internal property of KSZ88X3 devices, however this mode of operation 
> seems to be selectable only at boot time by strap pins (see description of 
> Register 12 (Global Control 10).

Ack. No need to worry about this one for now, it was only as example of a ksz8
style device with multiple RMII ports.

> > And all KSZ9xxx series seems to handle it per port as well.
> 
> I guess you are refering to register 87 (RMII Management Control Register)?
> But this only selects whether to enable clock output on RXC pin?

RXC seems to be not correct description or may be just internal signal
within the IP core. This register points to the strap pin affecting value of
this bit. Reading strap description in KSZ9477S or KSZ9897S give better
idea about meaning of this bit:

0: MII: PHY Mode (Default)
   RMII: Clock Mode. RMII 50MHz reference clock is output on REFCLKO6. (Default)
   RGMII: No effect
1: MII: MAC Mode
   RMII: Normal Mode. RMII 50MHz reference clock is input on REFCLKI6.
   RGMII: No effect

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select

[Ante Knezic]

On Mon, 23 Oct 2023 16:36:35 +0200, Oleksij Rempel wrote:
> If I see it correctly, in both cases there is only one bit to configure
> direction. The code need to support two interface modes:
> - PHY_INTERFACE_MODE_RMII (MAC mode) PLL is the clock provider. REFCLKO
>  is used.
> - PHY_INTERFACE_MODE_REVRMII (PHY mode) PLL is not used, REFCLKI is the
   clock provider.

As you suggested, it looks like KSZ9897 clocking mode depends on RMII interface
mode (with strapping pins), but I don't see this for KSZ8863. The PHY/MAC mode
is selected with Register 0x35 bit 7 and the clocking mode is selected via
strapping pins EN_REFCLKO and SMTXD32 (and additional register 0xC6 bit 3).
I guess its possible for the KSZ8863 to be the clock provider/consumer
regardless of PHY/MAC mode?

Table 3-5: RMII CLOCK SETTING of KSZ8863 datasheet describes the available 
clocking modes. If we try to create a relation between KSZ9897 and KSZ8863:

KSZ9897 "Normal Mode" is equivalent to KSZ8863 mode described in first column
of table 3-5: 
- EN_REFCLKO = 0, 0xC6(3) = 0 -> external 50Mhz OSC input to REFCLKI and X1 
  pin directly

KSZ9897 "Clock Mode" is equivalent to KSZ8863 mode described in fourth/fifth 
column (difference is only clock frequency) of table 3-5:
- EN_REFCLKO = 1, 0xC6(3) = 1 -> 50/25Mhz on X1 pin, 50/25Mhz RMII clock goes
  to REFCLKI internally. REFCLKI can be pulled down by resistor.

That leaves us with additional columns 2 and 3 of table 3-5 for KSZ8863, that
are similar to KSZ9897 Clock mode, but REFCLKI needs to be fed externally from
REFCLKO.

> I already did some work to configure CPU interface, where which can be at least
> partially reused for your work:
> https://lore.kernel.org/all/20230517121034.3801640-2-o.rempel@pengutronix.de/
> (Looks I forgot to complete mainlining for this patch)
> 
> If implanted as described, no new devicetree properties will be needed.

I don't quite get how the proposed patch might effect this topic? By setting
PHY/MAC mode? As noted, I dont see the same relation between clock and
MII mode for KSZ8863 as for KSZ9897? 

Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select

[Oleksij Rempel]

On Tue, Oct 24, 2023 at 09:56:43AM +0200, Ante Knezic wrote:
> On Mon, 23 Oct 2023 16:36:35 +0200, Oleksij Rempel wrote:
> > If I see it correctly, in both cases there is only one bit to configure
> > direction. The code need to support two interface modes:
> > - PHY_INTERFACE_MODE_RMII (MAC mode) PLL is the clock provider. REFCLKO
> >  is used.
> > - PHY_INTERFACE_MODE_REVRMII (PHY mode) PLL is not used, REFCLKI is the
>    clock provider.
> 
> As you suggested, it looks like KSZ9897 clocking mode depends on RMII interface
> mode (with strapping pins), but I don't see this for KSZ8863. The PHY/MAC mode
> is selected with Register 0x35 bit 7 and the clocking mode is selected via
> strapping pins EN_REFCLKO and SMTXD32 (and additional register 0xC6 bit 3).
> I guess its possible for the KSZ8863 to be the clock provider/consumer
> regardless of PHY/MAC mode?

Register 0x35 bit 7 is for MII mode
Register 0xC6 bit 3 is for RMII mode

MII != RMII

> Table 3-5: RMII CLOCK SETTING of KSZ8863 datasheet describes the available 
> clocking modes. If we try to create a relation between KSZ9897 and KSZ8863:
> 
> KSZ9897 "Normal Mode" is equivalent to KSZ8863 mode described in first column
> of table 3-5: 
> - EN_REFCLKO = 0, 0xC6(3) = 0 -> external 50Mhz OSC input to REFCLKI and X1 
>   pin directly
> 
> KSZ9897 "Clock Mode" is equivalent to KSZ8863 mode described in fourth/fifth 
> column (difference is only clock frequency) of table 3-5:
> - EN_REFCLKO = 1, 0xC6(3) = 1 -> 50/25Mhz on X1 pin, 50/25Mhz RMII clock goes
>   to REFCLKI internally. REFCLKI can be pulled down by resistor.
> 
> That leaves us with additional columns 2 and 3 of table 3-5 for KSZ8863, that
> are similar to KSZ9897 Clock mode, but REFCLKI needs to be fed externally from
> REFCLKO.

All of 5 variants described in "Table 3-5: RMII CLOCK SETTING of KSZ8863"
can be boiled down to two main configurations:

REFCLKI is used as clock source for internal MAC == Normal Mode or
RevRMII mode.
REFCLKI is not used as clock source for internal MAC == Clock Mode or
RMII mode.

Variants 1, 2, 3 describe only how can we feed REFCLKI from outside of
the chip. Even variant 2 and 3 make the switch to be an actually
physical clock provider, we still need to use REFCLKI and wire it
outside of the chip which make it practically a Normal Mode or RevRMII mode.

> > I already did some work to configure CPU interface, where which can be at least
> > partially reused for your work:
> > https://lore.kernel.org/all/20230517121034.3801640-2-o.rempel@pengutronix.de/
> > (Looks I forgot to complete mainlining for this patch)
> > 
> > If implanted as described, no new devicetree properties will be needed.
> 
> I don't quite get how the proposed patch might effect this topic?

You will need to add ksz8_phylink_mac_link_up() as this patch already
dose.

> By setting PHY/MAC mode? As noted, I dont see the same relation between clock and
> MII mode for KSZ8863 as for KSZ9897? 

I hope current mail will clear it.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select

[Ante Knezic]

On Tue, 24 Oct 2023 12:09:15 +0200, Oleksij Rampel wrote:

> > As you suggested, it looks like KSZ9897 clocking mode depends on RMII interface
> > mode (with strapping pins), but I don't see this for KSZ8863. The PHY/MAC mode
> > is selected with Register 0x35 bit 7 and the clocking mode is selected via
> > strapping pins EN_REFCLKO and SMTXD32 (and additional register 0xC6 bit 3).
> > I guess its possible for the KSZ8863 to be the clock provider/consumer
> > regardless of PHY/MAC mode?
>
>Register 0x35 bit 7 is for MII mode
>Register 0xC6 bit 3 is for RMII mode
>
>MII != RMII

Yes, right you are. Looks like I got lost in the datasheets...

> > Table 3-5: RMII CLOCK SETTING of KSZ8863 datasheet describes the available
> > clocking modes. If we try to create a relation between KSZ9897 and KSZ8863:
> >
> > KSZ9897 "Normal Mode" is equivalent to KSZ8863 mode described in first column
> > of table 3-5:
> > - EN_REFCLKO = 0, 0xC6(3) = 0 -> external 50Mhz OSC input to REFCLKI and X1
> >   pin directly
> >
> > KSZ9897 "Clock Mode" is equivalent to KSZ8863 mode described in fourth/fifth
> > column (difference is only clock frequency) of table 3-5:
> > - EN_REFCLKO = 1, 0xC6(3) = 1 -> 50/25Mhz on X1 pin, 50/25Mhz RMII clock goes
> >   to REFCLKI internally. REFCLKI can be pulled down by resistor.
> >
> > That leaves us with additional columns 2 and 3 of table 3-5 for KSZ8863, that
> > are similar to KSZ9897 Clock mode, but REFCLKI needs to be fed externally from
> > REFCLKO.
> 
> All of 5 variants described in "Table 3-5: RMII CLOCK SETTING of KSZ8863"
> can be boiled down to two main configurations:
> 
> REFCLKI is used as clock source for internal MAC == Normal Mode or
> RevRMII mode.
> REFCLKI is not used as clock source for internal MAC == Clock Mode or
> RMII mode.
> 
> Variants 1, 2, 3 describe only how can we feed REFCLKI from outside of
> the chip. Even variant 2 and 3 make the switch to be an actually
> physical clock provider, we still need to use REFCLKI and wire it
> outside of the chip which make it practically a Normal Mode or RevRMII mode.

That is correct, I guess its a matter of nomenclature, but how do you 
"tell" the switch whether it has REFCLKI routed externally or not if not by 
setting the 0xC6 bit 3? Is there another way to achieve this?

> > > I already did some work to configure CPU interface, where which can be at least
> > > partially reused for your work:
> > > https://lore.kernel.org/all/20230517121034.3801640-2-o.rempel@pengutronix.de/
> > > (Looks I forgot to complete mainlining for this patch)
> > >
> > > If implanted as described, no new devicetree properties will be needed.
> >
> > I don't quite get how the proposed patch might effect this topic?
> 
> You will need to add ksz8_phylink_mac_link_up() as this patch already
> dose.
> 
> > By setting PHY/MAC mode? As noted, I dont see the same relation between clock and
> > MII mode for KSZ8863 as for KSZ9897?
> 
> I hope current mail will clear it.

I tried your patch but it does not do it for me. As stated, my hw platform does
not have REFCLKI routed externally so a state at column 4/5 is expected.

Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select

[Oleksij Rempel]

On Tue, Oct 24, 2023 at 03:08:32PM +0200, Ante Knezic wrote:
> On Tue, 24 Oct 2023 12:09:15 +0200, Oleksij Rampel wrote:
> 
> > > As you suggested, it looks like KSZ9897 clocking mode depends on RMII interface
> > > mode (with strapping pins), but I don't see this for KSZ8863. The PHY/MAC mode
> > > is selected with Register 0x35 bit 7 and the clocking mode is selected via
> > > strapping pins EN_REFCLKO and SMTXD32 (and additional register 0xC6 bit 3).
> > > I guess its possible for the KSZ8863 to be the clock provider/consumer
> > > regardless of PHY/MAC mode?
> >
> >Register 0x35 bit 7 is for MII mode
> >Register 0xC6 bit 3 is for RMII mode
> >
> >MII != RMII
> 
> Yes, right you are. Looks like I got lost in the datasheets...
> 
> > > Table 3-5: RMII CLOCK SETTING of KSZ8863 datasheet describes the available
> > > clocking modes. If we try to create a relation between KSZ9897 and KSZ8863:
> > >
> > > KSZ9897 "Normal Mode" is equivalent to KSZ8863 mode described in first column
> > > of table 3-5:
> > > - EN_REFCLKO = 0, 0xC6(3) = 0 -> external 50Mhz OSC input to REFCLKI and X1
> > >   pin directly
> > >
> > > KSZ9897 "Clock Mode" is equivalent to KSZ8863 mode described in fourth/fifth
> > > column (difference is only clock frequency) of table 3-5:
> > > - EN_REFCLKO = 1, 0xC6(3) = 1 -> 50/25Mhz on X1 pin, 50/25Mhz RMII clock goes
> > >   to REFCLKI internally. REFCLKI can be pulled down by resistor.
> > >
> > > That leaves us with additional columns 2 and 3 of table 3-5 for KSZ8863, that
> > > are similar to KSZ9897 Clock mode, but REFCLKI needs to be fed externally from
> > > REFCLKO.
> > 
> > All of 5 variants described in "Table 3-5: RMII CLOCK SETTING of KSZ8863"
> > can be boiled down to two main configurations:
> > 
> > REFCLKI is used as clock source for internal MAC == Normal Mode or
> > RevRMII mode.
> > REFCLKI is not used as clock source for internal MAC == Clock Mode or
> > RMII mode.
> > 
> > Variants 1, 2, 3 describe only how can we feed REFCLKI from outside of
> > the chip. Even variant 2 and 3 make the switch to be an actually
> > physical clock provider, we still need to use REFCLKI and wire it
> > outside of the chip which make it practically a Normal Mode or RevRMII mode.
> 
> That is correct, I guess its a matter of nomenclature, but how do you 
> "tell" the switch whether it has REFCLKI routed externally or not if not by 
> setting the 0xC6 bit 3? Is there another way to achieve this?

I do not see any other way to "tell" it. The only thing to change in you
patches is a different way to tell it to the kernel.
Instead of introducing a new devicetree property, you need to reuse
phy-mode property.

> > > > I already did some work to configure CPU interface, where which can be at least
> > > > partially reused for your work:
> > > > https://lore.kernel.org/all/20230517121034.3801640-2-o.rempel@pengutronix.de/
> > > > (Looks I forgot to complete mainlining for this patch)
> > > >
> > > > If implanted as described, no new devicetree properties will be needed.
> > >
> > > I don't quite get how the proposed patch might effect this topic?
> > 
> > You will need to add ksz8_phylink_mac_link_up() as this patch already
> > dose.
> > 
> > > By setting PHY/MAC mode? As noted, I dont see the same relation between clock and
> > > MII mode for KSZ8863 as for KSZ9897?
> > 
> > I hope current mail will clear it.
> 
> I tried your patch but it does not do it for me. As stated, my hw platform does
> not have REFCLKI routed externally so a state at column 4/5 is expected.

My patches do not address your problem. It is just example which already creates
ksz8_phylink_mac_link_up() and do some CPU port configuration, which you
will need do too, but for other registers. Or just for get it to avoid
confusion.

What you need is to set 0xC6 bit 3 for PHY_INTERFACE_MODE_REVRMII and 
clear it for PHY_INTERFACE_MODE_RMII.

Since phy-mode for RMII was never set correctly, it will most probably
break every single devicetree using KSZ switches. It is the price of fixing
things :/

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select

[Ante Knezic]

On Tue, 24 Oct 2023 16:24:26 +0200, Oleksij Rampel wrote:

> > That is correct, I guess its a matter of nomenclature, but how do you 
> > "tell" the switch whether it has REFCLKI routed externally or not if not by 
> > setting the 0xC6 bit 3? Is there another way to achieve this?
> 
> I do not see any other way to "tell" it. The only thing to change in you
> patches is a different way to tell it to the kernel.
> Instead of introducing a new devicetree property, you need to reuse
> phy-mode property.

> ...

> Since phy-mode for RMII was never set correctly, it will most probably
> break every single devicetree using KSZ switches. It is the price of fixing
> things :/

To Vladimir Oltean: What are your thoughts on this?

Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select

[Vladimir Oltean]

On Fri, Oct 27, 2023 at 08:37:43AM +0200, Ante Knezic wrote:
> On Tue, 24 Oct 2023 16:24:26 +0200, Oleksij Rampel wrote:
> 
> > > That is correct, I guess its a matter of nomenclature, but how do you 
> > > "tell" the switch whether it has REFCLKI routed externally or not if not by 
> > > setting the 0xC6 bit 3? Is there another way to achieve this?
> > 
> > I do not see any other way to "tell" it. The only thing to change in you
> > patches is a different way to tell it to the kernel.
> > Instead of introducing a new devicetree property, you need to reuse
> > phy-mode property.
> 
> > ...
> 
> > Since phy-mode for RMII was never set correctly, it will most probably
> > break every single devicetree using KSZ switches. It is the price of fixing
> > things :/
> 
> To Vladimir Oltean: What are your thoughts on this?
> 

Sorry for the delayed response.

It's complicated. In the Google-searchable RMII spec, the REF_CLK is an
input from the perspective of the PHY, and an input or output from the
perspective of the MAC.

Additionally, some RMII PHYs like NXP TJA1110 provide "extensions" to
the RMII spec, where the PHY itself can provide the REF_CLK based on an
internal 25 MHz crystal (see "nxp,rmii-refclk-in").

My understanding is that some other PHYs, like KSZ8061RNB, go even
further and make the REF_CLK be always an output from the PHY's
perspective (this is not configurable).

It is noteworthy that on Wikipedia, it says directly that REF_CLK can
also be driven from the PHY to the MAC, which itself represents an
extension to what the spec says.

Additionally, some MACs like the NXP SJA1105 switch ports are not as
flexible as the standard would suggest. In RMII MAC mode, this switch
always wants to drive the REF_CLK pin itself. And in RMII PHY
(self-called REV-MII) mode, the SJA1105 always wants the REF_CLK to be
driven externally (from the PHY or external oscillator). So, for example
the SJA1105 in RMII MAC mode cannot be connected to the KSZ8061RNB, as
both would attempt to drive the REF_CLK signal.

In addition to all of that, the MAC/PHY roles are not just about the
direction of the REF_CLK, but also about the /J/ /K/ codewords that are
placed by the PHY in the inter packet gap on RXD[1:0]. A MAC doesn't do
this, and if it did, the PHY wouldn't expect it, and AFAIK, would
blindly propagate those code words onto the BASE-T wire, which is
undesirable.

So, my opinion is that although what Oleksij would like to see is
admirable, I don't think that the REF_CLK direction is a matter of RMII
MAC vs PHY role, and thus, we wouldn't need to change "rmii" to "rev-rmii"
and cause breakage everywhere. It's just that - a matter of REF_CLK
direction. It's true, though, that this is a generic problem and that
the generic bindings for RMII that we currently have are under-specified.
We could try to devise an extended RMII binding which makes it clear for
both the MAC and the PHY who is responsible to drive this signal. You
are not attempting that, you are just coming up with yet another
vendor-specific MAC property which solves a generic problem. I can't say
I am completely opposed to that, either, which is why I haven't really
spoken out against it. The PHY maintainers would also have to weigh in,
and not all of them are CCed here.

Now as to Conor's previous question about describing the REF_CLK as a
CCF clock, which is a bit related, honestly I haven't really analyzed
the merits of doing that. We know from the RMII standard that has a
known expected frequency of 50 MHz, it's just a matter of who provides
it.

Just one small thing to note is that I have heard of RMII PHYs which,
when REF_CLK is an input from their perspective, don't even have their
registers accessible until that REF_CLK is turned on. This may be
problematic, because the attached MAC driver may also own the MDIO bus
on which said PHY is located, and it practically needs to turn on the
RMII REF_CLK before the PHY on the MDIO bus could even be probed.
Currently, IIUC, the way in which that works is that RMII MAC drivers
are simply coded up to do just that, and I guess that worked so far.
With CCF, I guess you would describe this by making the MAC be a
"clocks" provider for the PHY. But it could also be the other way
around, and in that case, the MAC would be the parent of the PHY, but
it would also depend on a component that the PHY provides. I hope
fw_devlink doesn't mind reverse dependencies like that...

I am afraid that creating a CCF style binding for REF_CLK will be an
enormous hammer for a very small nail and will see very limited adoption
to other drivers, but I might as well be wrong about it. Compatibility
between RMII MACs and PHYs which may or may not be CCF-ready might also
be a concern.

Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select

[Andrew Lunn]

> So, my opinion is that although what Oleksij would like to see is
> admirable, I don't think that the REF_CLK direction is a matter of RMII
> MAC vs PHY role, and thus, we wouldn't need to change "rmii" to "rev-rmii"
> and cause breakage everywhere. It's just that - a matter of REF_CLK
> direction. It's true, though, that this is a generic problem and that
> the generic bindings for RMII that we currently have are under-specified.
> We could try to devise an extended RMII binding which makes it clear for
> both the MAC and the PHY who is responsible to drive this signal. You
> are not attempting that, you are just coming up with yet another
> vendor-specific MAC property which solves a generic problem. I can't say
> I am completely opposed to that, either, which is why I haven't really
> spoken out against it. The PHY maintainers would also have to weigh in,
> and not all of them are CCed here.

I would recommend looking around other PHYs and find a property which
does what you want, and copy it.

We do have all sorts of properties. There are some to enable the
REF_CLK out of the PHY. Some to disable the REF_CLK out, some to
disable it when the link is down, some to indicate what frequency it
should tick at, etc.

If you want to go the extra mile, maybe you can make a summary of all
these properties, and maybe we can produce a guide line for what we
want the properties to be called going forward.

> I am afraid that creating a CCF style binding for REF_CLK will be an
> enormous hammer for a very small nail and will see very limited adoption
> to other drivers, but I might as well be wrong about it. Compatibility
> between RMII MACs and PHYs which may or may not be CCF-ready might also
> be a concern.

I also don't think using the CCF makes too much sense, except for
where the SoC provides the lock, and already has a CCF covering it.

I would also be hesitant to add more dependencies between the MAC and
the PHY. The DT often has circular dependencies and we have had issues
with probing being deferred because the core does not always
understand these circular dependencies.

	   Andrew

Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select

[Oleksij Rempel]

On Tue, Oct 31, 2023 at 02:00:05AM +0100, Andrew Lunn wrote:
> > So, my opinion is that although what Oleksij would like to see is
> > admirable, I don't think that the REF_CLK direction is a matter of RMII
> > MAC vs PHY role, and thus, we wouldn't need to change "rmii" to "rev-rmii"
> > and cause breakage everywhere. It's just that - a matter of REF_CLK
> > direction. It's true, though, that this is a generic problem and that
> > the generic bindings for RMII that we currently have are under-specified.
> > We could try to devise an extended RMII binding which makes it clear for
> > both the MAC and the PHY who is responsible to drive this signal. You
> > are not attempting that, you are just coming up with yet another
> > vendor-specific MAC property which solves a generic problem. I can't say
> > I am completely opposed to that, either, which is why I haven't really
> > spoken out against it. The PHY maintainers would also have to weigh in,
> > and not all of them are CCed here.
> 
> I would recommend looking around other PHYs and find a property which
> does what you want, and copy it.
> 
> We do have all sorts of properties. There are some to enable the
> REF_CLK out of the PHY. Some to disable the REF_CLK out, some to
> disable it when the link is down, some to indicate what frequency it
> should tick at, etc.
> 
> If you want to go the extra mile, maybe you can make a summary of all
> these properties, and maybe we can produce a guide line for what we
> want the properties to be called going forward.
> 
> > I am afraid that creating a CCF style binding for REF_CLK will be an
> > enormous hammer for a very small nail and will see very limited adoption
> > to other drivers, but I might as well be wrong about it. Compatibility
> > between RMII MACs and PHYs which may or may not be CCF-ready might also
> > be a concern.
> 
> I also don't think using the CCF makes too much sense, except for
> where the SoC provides the lock, and already has a CCF covering it.
> 
> I would also be hesitant to add more dependencies between the MAC and
> the PHY. The DT often has circular dependencies and we have had issues
> with probing being deferred because the core does not always
> understand these circular dependencies.

Heh, this are unsolved problems making me pain in different projects.

Here are some real life examples, which are unsolved in one or another project
and till now didn't went mainline:

1. In scenarios where PHYs require an RMII clock from the MAC, initialization
becomes complex. This is often resolved through bootloader and kernel
modifications. Right now it kind of works and postponed until it will make
real pain :)

2. Complexity increases in designs with multiple PHYs used by different MACs
but connected to one MDIO bus. Same is here, there was already some
regressions but the pain is still not enough for making things right.

3. For some MACs like STMMAC, configuration is challenging without an external
clock from the PHY. For example, VLAN configuration isn't possible with EEE
enabled unless deep power saving states are disabled during register access.
If I remember it correctly, there was floating discussions and patches trying
to address similar issues.

Transferring these issues to KSZ8863, we might face difficulties configuring
STMMAC if KSZ8863, acting as the clock provider, isn't enabled early before MAC
driver probing, a tricky scenario in the DSA framework.

Working on deep sleep states for the KSZ switch driver, I find that dynamic
clock control, potentially offered by CCF, could be quite handy.

Please do not see this answer as a request to Ante for complex rework. It's
more of a red flag notifying that the clocking issue is still unsolved, and
someone (may be me), sooner or later, will have enough motivation to jump into
this wasp nest :)

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select

[Vladimir Oltean]

On Tue, Oct 31, 2023 at 08:28:47AM +0100, Oleksij Rempel wrote:
> Transferring these issues to KSZ8863, we might face difficulties configuring
> STMMAC if KSZ8863, acting as the clock provider, isn't enabled early before MAC
> driver probing, a tricky scenario in the DSA framework.

So for each driver, there are 2 components to using CCF for MAC/PHY
interface clocks, one is providing what you have, and the other is
requesting what you need.

To avoid circular dependencies, we should make the clock provider per
DSA port be independent of the DSA driver itself. That is because, as
you point out in your example, the conduit interface (stmmac) may depend
on a clock provided by the switch, but the switch also depends on the
conduit being fully probed.

Stronger separation / more granular dependencies was one reason why I
wanted for more DSA drivers to follow Colin Foster's suit, but I stopped
working on that too:
https://lore.kernel.org/lkml/20221222134844.lbzyx5hz7z5n763n@skbuf/

but in the case of interface clocks, the separation is not so clear.
I would expect that for most if not all switches, the interface clock is
implicitly provided by enabling and configuring the respective MAC, the
same MAC that is intrinsically controlled through phylink by the main
DSA (switching IP) driver. So we have 2 APIs for controlling the same
piece of hardware, I'm not sure how conflicting requests are supposed to
be resolved.

This piece of the puzzle is quite complicated to fit into the larger
architecture in a coherent way, although I'm not arguing that there will
also be benefits.

Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select

[Oleksij Rempel]

On Mon, Oct 30, 2023 at 07:42:25PM +0200, Vladimir Oltean wrote:
> On Fri, Oct 27, 2023 at 08:37:43AM +0200, Ante Knezic wrote:
> > On Tue, 24 Oct 2023 16:24:26 +0200, Oleksij Rampel wrote:
> > 
> > > > That is correct, I guess its a matter of nomenclature, but how do you 
> > > > "tell" the switch whether it has REFCLKI routed externally or not if not by 
> > > > setting the 0xC6 bit 3? Is there another way to achieve this?
> > > 
> > > I do not see any other way to "tell" it. The only thing to change in you
> > > patches is a different way to tell it to the kernel.
> > > Instead of introducing a new devicetree property, you need to reuse
> > > phy-mode property.
> > 
> > > ...
> > 
> > > Since phy-mode for RMII was never set correctly, it will most probably
> > > break every single devicetree using KSZ switches. It is the price of fixing
> > > things :/
> > 
> > To Vladimir Oltean: What are your thoughts on this?
> > 
> 
> In addition to all of that, the MAC/PHY roles are not just about the
> direction of the REF_CLK, but also about the /J/ /K/ codewords that are
> placed by the PHY in the inter packet gap on RXD[1:0]. A MAC doesn't do
> this, and if it did, the PHY wouldn't expect it, and AFAIK, would
> blindly propagate those code words onto the BASE-T wire, which is
> undesirable.

Interesting detail. I didn't knew it, it would be good to document
it somewhere near to revrmii binding :)

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select

[Vladimir Oltean]

On Mon, Oct 23, 2023 at 09:27:00AM +0200, Ante Knezic wrote:
> As far as I am aware only the KSZ8863 and KSZ8873 have this property available,
> but the biggger issue might be in scaling this to port property as the register
> "Forward Invalid VID Frame and Host Mode" where the setting is applied is
> located under "Advanced Control Registers" section which is actually global at
> least looking from the switch point of view. Usually port properties are more
> applicable when registers in question are located under "Port Registers" section.
> This is somewhat similar to for example enabling the tail tag mode which is 
> again used only by the port 3 interface and is control from "Global Control 1"
> register.
> With this in mind - if you still believe we should move this to port dt 
> property, then should we forbid setting the property for any other port other 
> than port 3, and can/should this be enforced by the dt schema?

I have no doubt that RMII settings are port settings. Scaling up the implementation
to multiple ports on other switches doesn't mean that the DT binding shouldn't be
per port.

Anyway, the per-port access to a global switch setting is indeed a common theme
with the old Micrel switches. I once tried to introduce the concept of "wacky"
regmap regfields for that:
https://patchwork.kernel.org/project/netdevbpf/patch/20230316161250.3286055-3-vladimir.oltean@nxp.com/

but I don't have hardware to test and nobody who does picked up upon the regfield
idea, it seems.

Re: [PATCH net-next v4 2/2] net:dsa:microchip: add property to select

[Ante Knezic]

On  Mon, 23 Oct 2023 15:19:24 +0300, Vladimir Oltean wrote:

> I have no doubt that RMII settings are port settings. Scaling up the implementation
> to multiple ports on other switches doesn't mean that the DT binding shouldn't be
> per port.
> 
> Anyway, the per-port access to a global switch setting is indeed a common theme
> with the old Micrel switches. I once tried to introduce the concept of "wacky"
> regmap regfields for that:
> https://patchwork.kernel.org/project/netdevbpf/patch/20230316161250.3286055-3-vladimir.oltean@nxp.com/
> 
> but I don't have hardware to test and nobody who does picked up upon the regfield
> idea, it seems.

Ok so I see about moving this to port property.