[PATCH 1/2] net: dsa: qca8k: Add additional PORT0_PAD_CTRL options

[PATCH 1/2] net: dsa: qca8k: Add additional PORT0_PAD_CTRL options

[Matthew Hagan]

A number of devices require additional PORT0_PAD configuration that cannot
otherwise be inferred. This patch is based on John Crispin's "net: dsa:
qca8k: allow swapping of mac0 and mac6", adding the ability to swap mac0
and mac6, as well as to set the transmit and receive clock phase to falling
edge.

To keep things tidy, a function has been added to handle these device tree
properties. However this handling could be moved to the
qca8k_phylink_mac_config function if preferred.

Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
---
 drivers/net/dsa/qca8k.c | 42 +++++++++++++++++++++++++++++++++++------
 drivers/net/dsa/qca8k.h |  3 +++
 2 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 4acad5fa0c84..1443f97dd348 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -587,6 +587,28 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv)
 	return 0;
 }
 
+static void
+qca8k_setup_port_pad_ctrl_reg(struct qca8k_priv *priv)
+{
+	u32 val = qca8k_read(priv, QCA8K_REG_PORT0_PAD_CTRL);
+
+	/* Swap MAC0-MAC6 */
+	if (of_property_read_bool(priv->dev->of_node,
+				  "qca,exchange-mac0-mac6"))
+		val |= QCA8K_PORT0_PAD_CTRL_MAC06_EXCHG;
+
+	/* SGMII Clock phase configuration */
+	if (of_property_read_bool(priv->dev->of_node,
+				  "qca,sgmii-rxclk-falling-edge"))
+		val |= QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE;
+
+	if (of_property_read_bool(priv->dev->of_node,
+				  "qca,sgmii-txclk-falling-edge"))
+		val |= QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE;
+
+	qca8k_write(priv, QCA8K_REG_PORT0_PAD_CTRL, val);
+}
+
 static int
 qca8k_setup(struct dsa_switch *ds)
 {
@@ -611,6 +633,9 @@ qca8k_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
+	/* Configure additional port pad properties */
+	qca8k_setup_port_pad_ctrl_reg(priv);
+
 	/* Enable CPU Port */
 	qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
 		      QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
@@ -722,27 +747,32 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		return;
 	}
 
+	/* Read port pad ctrl reg */
+	val = qca8k_read(priv, reg);
+
 	switch (state->interface) {
 	case PHY_INTERFACE_MODE_RGMII:
 		/* RGMII mode means no delay so don't enable the delay */
-		qca8k_write(priv, reg, QCA8K_PORT_PAD_RGMII_EN);
+		val |= QCA8K_PORT_PAD_RGMII_EN;
+		qca8k_write(priv, reg, val);
 		break;
 	case PHY_INTERFACE_MODE_RGMII_ID:
 		/* RGMII_ID needs internal delay. This is enabled through
 		 * PORT5_PAD_CTRL for all ports, rather than individual port
 		 * registers
 		 */
-		qca8k_write(priv, reg,
-			    QCA8K_PORT_PAD_RGMII_EN |
-			    QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
-			    QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY));
+		val |= QCA8K_PORT_PAD_RGMII_EN |
+		       QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
+		       QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY);
+		qca8k_write(priv, reg, val);
 		qca8k_write(priv, QCA8K_REG_PORT5_PAD_CTRL,
 			    QCA8K_PORT_PAD_RGMII_RX_DELAY_EN);
 		break;
 	case PHY_INTERFACE_MODE_SGMII:
 	case PHY_INTERFACE_MODE_1000BASEX:
 		/* Enable SGMII on the port */
-		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
+		val |= QCA8K_PORT_PAD_SGMII_EN;
+		qca8k_write(priv, reg, val);
 
 		/* Enable/disable SerDes auto-negotiation as necessary */
 		val = qca8k_read(priv, QCA8K_REG_PWS);
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 10ef2bca2cde..4e115557e571 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -26,6 +26,9 @@
 #define   QCA8K_MASK_CTRL_ID_M				0xff
 #define   QCA8K_MASK_CTRL_ID_S				8
 #define QCA8K_REG_PORT0_PAD_CTRL			0x004
+#define   QCA8K_PORT0_PAD_CTRL_MAC06_EXCHG		BIT(31)
+#define   QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE	BIT(19)
+#define   QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE	BIT(18)
 #define QCA8K_REG_PORT5_PAD_CTRL			0x008
 #define QCA8K_REG_PORT6_PAD_CTRL			0x00c
 #define   QCA8K_PORT_PAD_RGMII_EN			BIT(26)
-- 
2.25.4

[PATCH 2/2] dt-bindings: net: dsa: qca8k: Add PORT0_PAD_CTRL properties

[Matthew Hagan]

Add names and decriptions of additional PORT0_PAD_CTRL properties.

Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
---
 Documentation/devicetree/bindings/net/dsa/qca8k.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
index ccbc6d89325d..3d34c4f2e891 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
@@ -13,6 +13,14 @@ Optional properties:
 
 - reset-gpios: GPIO to be used to reset the whole device
 
+Optional MAC configuration properties:
+
+- qca,exchange-mac0-mac6:	If present, internally swaps MAC0 and MAC6.
+- qca,sgmii-rxclk-falling-edge:	If present, sets receive clock phase to
+				falling edge.
+- qca,sgmii-txclk-falling-edge:	If present, sets transmit clock phase to
+				falling edge.
+
 Subnodes:
 
 The integrated switch subnode should be specified according to the binding
-- 
2.25.4

Re: [PATCH 2/2] dt-bindings: net: dsa: qca8k: Add PORT0_PAD_CTRL properties

[Jakub Kicinski]

On Mon, 13 Jul 2020 21:50:26 +0100 Matthew Hagan wrote:
> Add names and decriptions of additional PORT0_PAD_CTRL properties.
> 
> Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
> ---
>  Documentation/devicetree/bindings/net/dsa/qca8k.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> index ccbc6d89325d..3d34c4f2e891 100644
> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> @@ -13,6 +13,14 @@ Optional properties:
>  
>  - reset-gpios: GPIO to be used to reset the whole device
>  
> +Optional MAC configuration properties:
> +
> +- qca,exchange-mac0-mac6:	If present, internally swaps MAC0 and MAC6.

Perhaps we can say a little more here?

> +- qca,sgmii-rxclk-falling-edge:	If present, sets receive clock phase to
> +				falling edge.
> +- qca,sgmii-txclk-falling-edge:	If present, sets transmit clock phase to
> +				falling edge.

These are not something that other vendors may implement and therefore
something we may want to make generic? Andrew?

>  Subnodes:
>  
>  The integrated switch subnode should be specified according to the binding

Re: [PATCH 2/2] dt-bindings: net: dsa: qca8k: Add PORT0_PAD_CTRL properties

[Andrew Lunn]

On Thu, Jul 16, 2020 at 03:09:25PM -0700, Jakub Kicinski wrote:
> On Mon, 13 Jul 2020 21:50:26 +0100 Matthew Hagan wrote:
> > Add names and decriptions of additional PORT0_PAD_CTRL properties.
> > 
> > Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/net/dsa/qca8k.txt | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> > index ccbc6d89325d..3d34c4f2e891 100644
> > --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> > +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> > @@ -13,6 +13,14 @@ Optional properties:
> >  
> >  - reset-gpios: GPIO to be used to reset the whole device
> >  
> > +Optional MAC configuration properties:
> > +
> > +- qca,exchange-mac0-mac6:	If present, internally swaps MAC0 and MAC6.
> 
> Perhaps we can say a little more here?
> 
> > +- qca,sgmii-rxclk-falling-edge:	If present, sets receive clock phase to
> > +				falling edge.
> > +- qca,sgmii-txclk-falling-edge:	If present, sets transmit clock phase to
> > +				falling edge.
> 
> These are not something that other vendors may implement and therefore
> something we may want to make generic? Andrew?

I've never seen any other vendor implement this. Which to me makes me
think this is a vendor extension, to Ciscos vendor extension of
1000BaseX.

Matthew, do you have a real use cases of these? I don't see a DT patch
making use of them. And if you do, what is the PHY on the other end
which also allows you to invert the clocks?

       Andrew

Re: [PATCH 2/2] dt-bindings: net: dsa: qca8k: Add PORT0_PAD_CTRL properties

[Matthew Hagan]

On 16/07/2020 23:32, Andrew Lunn wrote:
> On Thu, Jul 16, 2020 at 03:09:25PM -0700, Jakub Kicinski wrote:
>> On Mon, 13 Jul 2020 21:50:26 +0100 Matthew Hagan wrote:
>>> Add names and decriptions of additional PORT0_PAD_CTRL properties.
>>>
>>> Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
>>> ---
>>>  Documentation/devicetree/bindings/net/dsa/qca8k.txt | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
>>> index ccbc6d89325d..3d34c4f2e891 100644
>>> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
>>> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
>>> @@ -13,6 +13,14 @@ Optional properties:
>>>  
>>>  - reset-gpios: GPIO to be used to reset the whole device
>>>  
>>> +Optional MAC configuration properties:
>>> +
>>> +- qca,exchange-mac0-mac6:	If present, internally swaps MAC0 and MAC6.
>>
>> Perhaps we can say a little more here?
>>
>>> +- qca,sgmii-rxclk-falling-edge:	If present, sets receive clock phase to
>>> +				falling edge.
>>> +- qca,sgmii-txclk-falling-edge:	If present, sets transmit clock phase to
>>> +				falling edge.
>>
>> These are not something that other vendors may implement and therefore
>> something we may want to make generic? Andrew?
> 
> I've never seen any other vendor implement this. Which to me makes me
> think this is a vendor extension, to Ciscos vendor extension of
> 1000BaseX.
> 
> Matthew, do you have a real use cases of these? I don't see a DT patch
> making use of them. And if you do, what is the PHY on the other end
> which also allows you to invert the clocks?
> 
The use case I am working on is the Cisco Meraki MX65 which requires bit
18 set (qca,sgmii-txclk-falling-edge). On the other side is a BCM58625
SRAB with ports 4 and 5 in SGMII mode. There is no special polarity
configuration set on this side though I do have very limited info on
what is available. The settings I have replicate the vendor
configuration extracted from the device.

The qca,sgmii-rxclk-falling-edge option (bit 19) is commonly used
according to the device trees found in the OpenWrt, which is still using
the ar8216 driver. With a count through the ar8327-initvals I see bit 19
set on 18 of 22 devices using SGMII on MAC0.
>        Andrew
> 

Matthew

Re: [PATCH 2/2] dt-bindings: net: dsa: qca8k: Add PORT0_PAD_CTRL properties

[Florian Fainelli]

On 7/17/2020 12:26 PM, Matthew Hagan wrote:
> 
> 
> On 16/07/2020 23:32, Andrew Lunn wrote:
>> On Thu, Jul 16, 2020 at 03:09:25PM -0700, Jakub Kicinski wrote:
>>> On Mon, 13 Jul 2020 21:50:26 +0100 Matthew Hagan wrote:
>>>> Add names and decriptions of additional PORT0_PAD_CTRL properties.
>>>>
>>>> Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/net/dsa/qca8k.txt | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
>>>> index ccbc6d89325d..3d34c4f2e891 100644
>>>> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
>>>> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
>>>> @@ -13,6 +13,14 @@ Optional properties:
>>>>  
>>>>  - reset-gpios: GPIO to be used to reset the whole device
>>>>  
>>>> +Optional MAC configuration properties:
>>>> +
>>>> +- qca,exchange-mac0-mac6:	If present, internally swaps MAC0 and MAC6.
>>>
>>> Perhaps we can say a little more here?
>>>
>>>> +- qca,sgmii-rxclk-falling-edge:	If present, sets receive clock phase to
>>>> +				falling edge.
>>>> +- qca,sgmii-txclk-falling-edge:	If present, sets transmit clock phase to
>>>> +				falling edge.
>>>
>>> These are not something that other vendors may implement and therefore
>>> something we may want to make generic? Andrew?
>>
>> I've never seen any other vendor implement this. Which to me makes me
>> think this is a vendor extension, to Ciscos vendor extension of
>> 1000BaseX.
>>
>> Matthew, do you have a real use cases of these? I don't see a DT patch
>> making use of them. And if you do, what is the PHY on the other end
>> which also allows you to invert the clocks?
>>
> The use case I am working on is the Cisco Meraki MX65 which requires bit
> 18 set (qca,sgmii-txclk-falling-edge). On the other side is a BCM58625
> SRAB with ports 4 and 5 in SGMII mode. There is no special polarity
> configuration set on this side though I do have very limited info on
> what is available. The settings I have replicate the vendor
> configuration extracted from the device.

The only polarity change that I am aware of on the BCM58625 side is to
allow for the TXDP/TXDN to be swapped, this is achieved by setting bit 5
in the TX_ACONTROL0 register (block address is 0x8060), that does look
different than what this is controlling though.
-- 
Florian

Re: [PATCH 2/2] dt-bindings: net: dsa: qca8k: Add PORT0_PAD_CTRL properties

[Vladimir Oltean]

On Fri, Jul 17, 2020 at 08:26:02PM +0100, Matthew Hagan wrote:
> 
> 
> On 16/07/2020 23:32, Andrew Lunn wrote:
> > On Thu, Jul 16, 2020 at 03:09:25PM -0700, Jakub Kicinski wrote:
> >> On Mon, 13 Jul 2020 21:50:26 +0100 Matthew Hagan wrote:
> >>> Add names and decriptions of additional PORT0_PAD_CTRL properties.
> >>>
> >>> Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
> >>> ---
> >>>  Documentation/devicetree/bindings/net/dsa/qca8k.txt | 8 ++++++++
> >>>  1 file changed, 8 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> >>> index ccbc6d89325d..3d34c4f2e891 100644
> >>> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> >>> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> >>> @@ -13,6 +13,14 @@ Optional properties:
> >>>  
> >>>  - reset-gpios: GPIO to be used to reset the whole device
> >>>  
> >>> +Optional MAC configuration properties:
> >>> +
> >>> +- qca,exchange-mac0-mac6:	If present, internally swaps MAC0 and MAC6.
> >>
> >> Perhaps we can say a little more here?
> >>
> >>> +- qca,sgmii-rxclk-falling-edge:	If present, sets receive clock phase to
> >>> +				falling edge.
> >>> +- qca,sgmii-txclk-falling-edge:	If present, sets transmit clock phase to
> >>> +				falling edge.
> >>
> >> These are not something that other vendors may implement and therefore
> >> something we may want to make generic? Andrew?
> > 
> > I've never seen any other vendor implement this. Which to me makes me
> > think this is a vendor extension, to Ciscos vendor extension of
> > 1000BaseX.
> > 
> > Matthew, do you have a real use cases of these? I don't see a DT patch
> > making use of them. And if you do, what is the PHY on the other end
> > which also allows you to invert the clocks?
> > 
> The use case I am working on is the Cisco Meraki MX65 which requires bit
> 18 set (qca,sgmii-txclk-falling-edge). On the other side is a BCM58625
> SRAB with ports 4 and 5 in SGMII mode. There is no special polarity
> configuration set on this side though I do have very limited info on
> what is available. The settings I have replicate the vendor
> configuration extracted from the device.
> 
> The qca,sgmii-rxclk-falling-edge option (bit 19) is commonly used
> according to the device trees found in the OpenWrt, which is still using
> the ar8216 driver. With a count through the ar8327-initvals I see bit 19
> set on 18 of 22 devices using SGMII on MAC0.
> >        Andrew
> > 
> 
> Matthew

Let's say I'm a user. When would I need to set
qca,sgmii-txclk-falling-edge and/or qca,sgmii-rxclk-falling-edge, and
wwhen would I need not to?

Thanks,
-Vladimir

Re: [PATCH 2/2] dt-bindings: net: dsa: qca8k: Add PORT0_PAD_CTRL properties

[Rob Herring]

On Fri, Jul 17, 2020 at 08:26:02PM +0100, Matthew Hagan wrote:
> 
> 
> On 16/07/2020 23:32, Andrew Lunn wrote:
> > On Thu, Jul 16, 2020 at 03:09:25PM -0700, Jakub Kicinski wrote:
> >> On Mon, 13 Jul 2020 21:50:26 +0100 Matthew Hagan wrote:
> >>> Add names and decriptions of additional PORT0_PAD_CTRL properties.
> >>>
> >>> Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
> >>> ---
> >>>  Documentation/devicetree/bindings/net/dsa/qca8k.txt | 8 ++++++++
> >>>  1 file changed, 8 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> >>> index ccbc6d89325d..3d34c4f2e891 100644
> >>> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> >>> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> >>> @@ -13,6 +13,14 @@ Optional properties:
> >>>  
> >>>  - reset-gpios: GPIO to be used to reset the whole device
> >>>  
> >>> +Optional MAC configuration properties:
> >>> +
> >>> +- qca,exchange-mac0-mac6:	If present, internally swaps MAC0 and MAC6.
> >>
> >> Perhaps we can say a little more here?
> >>
> >>> +- qca,sgmii-rxclk-falling-edge:	If present, sets receive clock phase to
> >>> +				falling edge.
> >>> +- qca,sgmii-txclk-falling-edge:	If present, sets transmit clock phase to
> >>> +				falling edge.
> >>
> >> These are not something that other vendors may implement and therefore
> >> something we may want to make generic? Andrew?
> > 
> > I've never seen any other vendor implement this. Which to me makes me
> > think this is a vendor extension, to Ciscos vendor extension of
> > 1000BaseX.
> > 
> > Matthew, do you have a real use cases of these? I don't see a DT patch
> > making use of them. And if you do, what is the PHY on the other end
> > which also allows you to invert the clocks?
> > 
> The use case I am working on is the Cisco Meraki MX65 which requires bit
> 18 set (qca,sgmii-txclk-falling-edge). On the other side is a BCM58625
> SRAB with ports 4 and 5 in SGMII mode. There is no special polarity
> configuration set on this side though I do have very limited info on
> what is available. The settings I have replicate the vendor
> configuration extracted from the device.
> 
> The qca,sgmii-rxclk-falling-edge option (bit 19) is commonly used
> according to the device trees found in the OpenWrt, which is still using
> the ar8216 driver. With a count through the ar8327-initvals I see bit 19
> set on 18 of 22 devices using SGMII on MAC0.

Can't you identify the device and configure the setting based on that? 
After all, MDIO devices are discoverable. Or there's no MDIO here?

Rob

Re: [PATCH 2/2] dt-bindings: net: dsa: qca8k: Add PORT0_PAD_CTRL properties

[Vladimir Oltean]

On Thu, Jul 16, 2020 at 03:09:25PM -0700, Jakub Kicinski wrote:
> On Mon, 13 Jul 2020 21:50:26 +0100 Matthew Hagan wrote:
> > Add names and decriptions of additional PORT0_PAD_CTRL properties.
> > 
> > Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/net/dsa/qca8k.txt | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> > index ccbc6d89325d..3d34c4f2e891 100644
> > --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> > +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> > @@ -13,6 +13,14 @@ Optional properties:
> >  
> >  - reset-gpios: GPIO to be used to reset the whole device
> >  
> > +Optional MAC configuration properties:
> > +
> > +- qca,exchange-mac0-mac6:	If present, internally swaps MAC0 and MAC6.
> 
> Perhaps we can say a little more here?
> 
> > +- qca,sgmii-rxclk-falling-edge:	If present, sets receive clock phase to
> > +				falling edge.
> > +- qca,sgmii-txclk-falling-edge:	If present, sets transmit clock phase to
> > +				falling edge.
> 
> These are not something that other vendors may implement and therefore
> something we may want to make generic? Andrew?
> 

It was asked before whether this device uses source-synchronous clock
for SGMII or if it recovers the clock from the data stream. Just "pass"
was given for a response.

https://patchwork.ozlabs.org/project/netdev/patch/8ddd76e484e1bedd12c87ea0810826b60e004a65.1591380105.git.noodles@earth.li/

One can, in principle, tell easily by examining schematics. If the SGMII
is only connected via RX_P, RX_N, TX_P, TX_N (and optionally there might
be external reference clocks for the SERDES lanes, but these are not
part of the data connection itself), then the clock is recovered from
the serial data stream, and we have no idea what "SGMII delays" are.

If the schematic shows 2 extra clock signals, one in each transmit
direction, then this is, in Russell King's words, "a new world of RGMII
delay pain but for SGMII". In principle I would fully expect clock skews
to be necessary for any high-speed protocol with source-synchronous
clocking. The problem, really, is that we aren't ready to deal with this
properly. We aren't distinguishing "SGMII with clock" from "SGMII
without clock" in any way. We have no idea who else is using such a
thing. Depending on the magnitude of this new world, it may be wise to
let these bindings go in as-is, or do something more kernel-wide...

One simple question to ask Matthew is what are you connecting to these
SGMII lanes, and if you need any special configuration on the other end
of those lanes too (and what is the configuration you are using on the
qca8k: enable the "SGMII delays" in both directions?).

> >  Subnodes:
> >  
> >  The integrated switch subnode should be specified according to the binding
> 

Thanks,
-Vladimir

Re: [PATCH 2/2] dt-bindings: net: dsa: qca8k: Add PORT0_PAD_CTRL properties

[Jonathan McDowell]

On Fri, Jul 17, 2020 at 01:38:22AM +0300, Vladimir Oltean wrote:
> On Thu, Jul 16, 2020 at 03:09:25PM -0700, Jakub Kicinski wrote:
> > On Mon, 13 Jul 2020 21:50:26 +0100 Matthew Hagan wrote:
> > > +- qca,sgmii-rxclk-falling-edge:	If present, sets receive clock phase to
> > > +				falling edge.
> > > +- qca,sgmii-txclk-falling-edge:	If present, sets transmit clock phase to
> > > +				falling edge.
> > 
> > These are not something that other vendors may implement and therefore
> > something we may want to make generic? Andrew?
> > 
> 
> It was asked before whether this device uses source-synchronous clock
> for SGMII or if it recovers the clock from the data stream. Just "pass"
> was given for a response.
> 
> https://patchwork.ozlabs.org/project/netdev/patch/8ddd76e484e1bedd12c87ea0810826b60e004a65.1591380105.git.noodles@earth.li/
> 
> One can, in principle, tell easily by examining schematics. If the SGMII
> is only connected via RX_P, RX_N, TX_P, TX_N (and optionally there might
> be external reference clocks for the SERDES lanes, but these are not
> part of the data connection itself), then the clock is recovered from
> the serial data stream, and we have no idea what "SGMII delays" are.
> 
> If the schematic shows 2 extra clock signals, one in each transmit
> direction, then this is, in Russell King's words, "a new world of RGMII
> delay pain but for SGMII". In principle I would fully expect clock skews
> to be necessary for any high-speed protocol with source-synchronous
> clocking. The problem, really, is that we aren't ready to deal with this
> properly. We aren't distinguishing "SGMII with clock" from "SGMII
> without clock" in any way. We have no idea who else is using such a
> thing. Depending on the magnitude of this new world, it may be wise to
> let these bindings go in as-is, or do something more kernel-wide...

I don't have the schematic for the device I've been working with, but
the switch data sheet just shows 2 differential pairs (input/output) for
the SerDes Interface (whereas the RGMII interfaces *are* listed with
their clocks).

J.

-- 
I just Fedexed my soul to hell. I'm *real* clever.

Re: [PATCH 2/2] dt-bindings: net: dsa: qca8k: Add PORT0_PAD_CTRL properties

[Matthew Hagan]

On 16/07/2020 23:09, Jakub Kicinski wrote:
> On Mon, 13 Jul 2020 21:50:26 +0100 Matthew Hagan wrote:
>> Add names and decriptions of additional PORT0_PAD_CTRL properties.
>>
>> Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
>> ---
>>  Documentation/devicetree/bindings/net/dsa/qca8k.txt | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
>> index ccbc6d89325d..3d34c4f2e891 100644
>> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
>> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
>> @@ -13,6 +13,14 @@ Optional properties:
>>  
>>  - reset-gpios: GPIO to be used to reset the whole device
>>  
>> +Optional MAC configuration properties:
>> +
>> +- qca,exchange-mac0-mac6:	If present, internally swaps MAC0 and MAC6.
> 
> Perhaps we can say a little more here?
> 
From John's patch:
"The switch allows us to swap the internal wirering of the two cpu ports.
For the HW offloading to work the ethernet MAC conencting to the LAN
ports must be wired to cpu port 0. There is HW in the wild that does not
fulfill this requirement. On these boards we need to swap the cpu ports."

This option is somewhat linked to instances where both MAC0 and MAC6 are
used as CPU ports. I may omit this for now since support for this hasn't
been added and MAC0 is hard-coded as the CPU port. The initial intention
here was to cover options commonly set by OpenWrt devices, based upon
their ar8327-initvals, to allow migration to qca8k.


>> +- qca,sgmii-rxclk-falling-edge:	If present, sets receive clock phase to
>> +				falling edge.
>> +- qca,sgmii-txclk-falling-edge:	If present, sets transmit clock phase to
>> +				falling edge.
> 
> These are not something that other vendors may implement and therefore
> something we may want to make generic? Andrew?
> 
>>  Subnodes:
>>  
>>  The integrated switch subnode should be specified according to the binding
> 
Matthew

Re: [PATCH 2/2] dt-bindings: net: dsa: qca8k: Add PORT0_PAD_CTRL properties

[Florian Fainelli]

On 7/17/2020 1:29 PM, Matthew Hagan wrote:
> 
> 
> On 16/07/2020 23:09, Jakub Kicinski wrote:
>> On Mon, 13 Jul 2020 21:50:26 +0100 Matthew Hagan wrote:
>>> Add names and decriptions of additional PORT0_PAD_CTRL properties.
>>>
>>> Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
>>> ---
>>>  Documentation/devicetree/bindings/net/dsa/qca8k.txt | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
>>> index ccbc6d89325d..3d34c4f2e891 100644
>>> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
>>> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
>>> @@ -13,6 +13,14 @@ Optional properties:
>>>  
>>>  - reset-gpios: GPIO to be used to reset the whole device
>>>  
>>> +Optional MAC configuration properties:
>>> +
>>> +- qca,exchange-mac0-mac6:	If present, internally swaps MAC0 and MAC6.
>>
>> Perhaps we can say a little more here?
>>
> From John's patch:
> "The switch allows us to swap the internal wirering of the two cpu ports.
> For the HW offloading to work the ethernet MAC conencting to the LAN
> ports must be wired to cpu port 0. There is HW in the wild that does not
> fulfill this requirement. On these boards we need to swap the cpu ports."
> 
> This option is somewhat linked to instances where both MAC0 and MAC6 are
> used as CPU ports. I may omit this for now since support for this hasn't
> been added and MAC0 is hard-coded as the CPU port. The initial intention
> here was to cover options commonly set by OpenWrt devices, based upon
> their ar8327-initvals, to allow migration to qca8k.

If you update the description of the property, I do not see a reason why
this should not be supported as of today, sooner or later you will need
it to convert more devices to qca8k as you say.
-- 
Florian

Re: [PATCH 2/2] dt-bindings: net: dsa: qca8k: Add PORT0_PAD_CTRL properties

[John Crispin]

On 17.07.20 22:39, Florian Fainelli wrote:
>
> On 7/17/2020 1:29 PM, Matthew Hagan wrote:
>>
>> On 16/07/2020 23:09, Jakub Kicinski wrote:
>>> On Mon, 13 Jul 2020 21:50:26 +0100 Matthew Hagan wrote:
>>>> Add names and decriptions of additional PORT0_PAD_CTRL properties.
>>>>
>>>> Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
>>>> ---
>>>>   Documentation/devicetree/bindings/net/dsa/qca8k.txt | 8 ++++++++
>>>>   1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
>>>> index ccbc6d89325d..3d34c4f2e891 100644
>>>> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
>>>> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
>>>> @@ -13,6 +13,14 @@ Optional properties:
>>>>   
>>>>   - reset-gpios: GPIO to be used to reset the whole device
>>>>   
>>>> +Optional MAC configuration properties:
>>>> +
>>>> +- qca,exchange-mac0-mac6:	If present, internally swaps MAC0 and MAC6.
>>> Perhaps we can say a little more here?
>>>
>>  From John's patch:
>> "The switch allows us to swap the internal wirering of the two cpu ports.
>> For the HW offloading to work the ethernet MAC conencting to the LAN
>> ports must be wired to cpu port 0. There is HW in the wild that does not
>> fulfill this requirement. On these boards we need to swap the cpu ports."
>>
>> This option is somewhat linked to instances where both MAC0 and MAC6 are
>> used as CPU ports. I may omit this for now since support for this hasn't
>> been added and MAC0 is hard-coded as the CPU port. The initial intention
>> here was to cover options commonly set by OpenWrt devices, based upon
>> their ar8327-initvals, to allow migration to qca8k.
> If you update the description of the property, I do not see a reason why
> this should not be supported as of today, sooner or later you will need
> it to convert more devices to qca8k as you say.

correct, there will be patches soonish to make qcom dakota and 
hawkeye/cypress use qca8k as their switch fabric is 95% identical.  we 
already started working on it. it is mmio based rather than mdio based, 
so the patch is quite a large rework right now.

     John

Re: [PATCH 2/2] dt-bindings: net: dsa: qca8k: Add PORT0_PAD_CTRL properties

[John Crispin]

On 17.07.20 22:29, Matthew Hagan wrote:
>
> On 16/07/2020 23:09, Jakub Kicinski wrote:
>> On Mon, 13 Jul 2020 21:50:26 +0100 Matthew Hagan wrote:
>>> Add names and decriptions of additional PORT0_PAD_CTRL properties.
>>>
>>> Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
>>> ---
>>>   Documentation/devicetree/bindings/net/dsa/qca8k.txt | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
>>> index ccbc6d89325d..3d34c4f2e891 100644
>>> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
>>> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
>>> @@ -13,6 +13,14 @@ Optional properties:
>>>   
>>>   - reset-gpios: GPIO to be used to reset the whole device
>>>   
>>> +Optional MAC configuration properties:
>>> +
>>> +- qca,exchange-mac0-mac6:	If present, internally swaps MAC0 and MAC6.
>> Perhaps we can say a little more here?
>>
>  From John's patch:
> "The switch allows us to swap the internal wirering of the two cpu ports.
> For the HW offloading to work the ethernet MAC conencting to the LAN
> ports must be wired to cpu port 0. There is HW in the wild that does not
> fulfill this requirement. On these boards we need to swap the cpu ports."
>
> This option is somewhat linked to instances where both MAC0 and MAC6 are
> used as CPU ports. I may omit this for now since support for this hasn't
> been added and MAC0 is hard-coded as the CPU port. The initial intention
> here was to cover options commonly set by OpenWrt devices, based upon
> their ar8327-initvals, to allow migration to qca8k.
>
>
correct, specifically quantenna designs do this, also saw ciscos swap 
mac0/6 for cpu port, that part of the patch is definitely safe to go. I 
stumbled across this while making qca8k work for g-fiber on a quantenna SoC.

in regards to the sgmii clk skew. I never understood the electrics fully 
I am afraid, but without the patch it simply does not work. my eletcric 
foo is unfortunately is not sufficient to understand the "whys" I am afraid.

     John

Re: [PATCH 2/2] dt-bindings: net: dsa: qca8k: Add PORT0_PAD_CTRL properties

[Russell King - ARM Linux admin]

On Fri, Jul 17, 2020 at 10:44:19PM +0200, John Crispin wrote:
> in regards to the sgmii clk skew. I never understood the electrics fully I
> am afraid, but without the patch it simply does not work. my eletcric foo is
> unfortunately is not sufficient to understand the "whys" I am afraid.

Do you happen to know what frequency the clock is?  Is it 1.25GHz or
625MHz?  It sounds like it may be 1.25GHz if the edge is important.

If the clock is 1.25GHz, the "why" is because of hazards (it has
nothing to do with delays in RGMII being propagated to SGMII).

Quite simply, a flip-flop suffers from metastability if the clock and
data inputs change at about the same time.  Amongst the parametrics of
flip-flops will be a data setup time, and a data hold time, referenced
to the clock signal.

If the data changes within the setup and hold times of the clock
changing, then the output of the flip-flop is unpredictable - it can
latch a logic 1 or a logic 0, or oscillate between the two until
settling on one state.

So, if data is clocked out on the rising edge of a clock signal, and
clocked in on the rising edge of a clock signal - and the data and
clock edges arrive within the setup and hold times at the flip-flop
that is clocking the data in, there is a metastability hazard, and
the data bit that is latched is unpredictable.

One way to solve this is to clock data out on one edge, and clock data
in on the opposite edge - this is used on buses such as SPI.  Other
buses such as I2C define minimum separation between transitions between
the SDA and SCL signals.

These solutions don't work with RGMII - the RGMII TXC clocks data on
both edges.  The only solution there is to ensure a delay is introduced
between the data and clock changes seen at the receiver - which can be
done by introducing delays at the transmitter or at the receiver, or by
serpentine routing of the traces to induce delays to separate the clock
and data transitions sufficiently to avoid metastability.

If the clock is 625MHz (as with some Marvell devices for SGMII) then
both clock edges are used, and both edges are used just like RGMII.
Therefore, the same considerations as RGMII apply there to ensure that
the data setup and hold times are not violated.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/2] dt-bindings: net: dsa: qca8k: Add PORT0_PAD_CTRL properties

[Andrew Lunn]

On Sat, Jul 18, 2020 at 02:20:11PM +0100, Russell King - ARM Linux admin wrote:
> On Fri, Jul 17, 2020 at 10:44:19PM +0200, John Crispin wrote:
> > in regards to the sgmii clk skew. I never understood the electrics fully I
> > am afraid, but without the patch it simply does not work. my eletcric foo is
> > unfortunately is not sufficient to understand the "whys" I am afraid.
> 
> Do you happen to know what frequency the clock is?  Is it 1.25GHz or
> 625MHz?  It sounds like it may be 1.25GHz if the edge is important.

I'm also a bit clueless when it comes to these systems.

I thought the clock was embedded into the SERDES signal? You recover
it from the signal?

Florian, does the switch have a separate clock input/output?

   Andrew

Re: [PATCH 2/2] dt-bindings: net: dsa: qca8k: Add PORT0_PAD_CTRL properties

[Russell King - ARM Linux admin]

On Sat, Jul 18, 2020 at 04:44:35PM +0200, Andrew Lunn wrote:
> On Sat, Jul 18, 2020 at 02:20:11PM +0100, Russell King - ARM Linux admin wrote:
> > On Fri, Jul 17, 2020 at 10:44:19PM +0200, John Crispin wrote:
> > > in regards to the sgmii clk skew. I never understood the electrics fully I
> > > am afraid, but without the patch it simply does not work. my eletcric foo is
> > > unfortunately is not sufficient to understand the "whys" I am afraid.
> > 
> > Do you happen to know what frequency the clock is?  Is it 1.25GHz or
> > 625MHz?  It sounds like it may be 1.25GHz if the edge is important.
> 
> I'm also a bit clueless when it comes to these systems.
> 
> I thought the clock was embedded into the SERDES signal? You recover
> it from the signal?

Indeed it is.  An external clock can be used to avoid needing clock
recovery in the SERDES receiver.

For example, with the 88E1111, it only accepts the datastream from
the MAC with no provision for a separate clock, but it can be
configured to generate a 625MHz clock for the data stream to the MAC
if required.

Being 625MHz (half the data rate), both edges of the clock are used,
with a delay to help avoid the metastability hazard I previously
described at the receiver.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/2] dt-bindings: net: dsa: qca8k: Add PORT0_PAD_CTRL properties

[Matthew Hagan]

On 18/07/2020 14:20, Russell King - ARM Linux admin wrote:
> On Fri, Jul 17, 2020 at 10:44:19PM +0200, John Crispin wrote:
>> in regards to the sgmii clk skew. I never understood the electrics fully I
>> am afraid, but without the patch it simply does not work. my eletcric foo is
>> unfortunately is not sufficient to understand the "whys" I am afraid.
> 
> Do you happen to know what frequency the clock is?  Is it 1.25GHz or
> 625MHz?  It sounds like it may be 1.25GHz if the edge is important.
> 
> If the clock is 1.25GHz, the "why" is because of hazards (it has
> nothing to do with delays in RGMII being propagated to SGMII).
> 
> Quite simply, a flip-flop suffers from metastability if the clock and
> data inputs change at about the same time.  Amongst the parametrics of
> flip-flops will be a data setup time, and a data hold time, referenced
> to the clock signal.
> 
> If the data changes within the setup and hold times of the clock
> changing, then the output of the flip-flop is unpredictable - it can
> latch a logic 1 or a logic 0, or oscillate between the two until
> settling on one state.
> 
> So, if data is clocked out on the rising edge of a clock signal, and
> clocked in on the rising edge of a clock signal - and the data and
> clock edges arrive within the setup and hold times at the flip-flop
> that is clocking the data in, there is a metastability hazard, and
> the data bit that is latched is unpredictable.
>
With default settings, in my case, the device will work at first, though
eventually problems arise with loss of connectivity, but constant
activity on the individual port led.

> One way to solve this is to clock data out on one edge, and clock data
> in on the opposite edge - this is used on buses such as SPI.  Other
> buses such as I2C define minimum separation between transitions between
> the SDA and SCL signals.
> 
Is there any case where it would matter which way round the clocks are,
or is it only relevant that they are on opposite edges? Why not do this
by default for qca8k devices?

> These solutions don't work with RGMII - the RGMII TXC clocks data on
> both edges.  The only solution there is to ensure a delay is introduced
> between the data and clock changes seen at the receiver - which can be
> done by introducing delays at the transmitter or at the receiver, or by
> serpentine routing of the traces to induce delays to separate the clock
> and data transitions sufficiently to avoid metastability.
> 
> If the clock is 625MHz (as with some Marvell devices for SGMII) then
> both clock edges are used, and both edges are used just like RGMII.
> Therefore, the same considerations as RGMII apply there to ensure that
> the data setup and hold times are not violated.
> 
By default, both tx and rx are set to rising edge.

Matthew

Re: [PATCH 2/2] dt-bindings: net: dsa: qca8k: Add PORT0_PAD_CTRL properties

[Florian Fainelli]

On 7/13/2020 1:50 PM, Matthew Hagan wrote:
> Add names and decriptions of additional PORT0_PAD_CTRL properties.
> 
> Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
> ---
>  Documentation/devicetree/bindings/net/dsa/qca8k.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> index ccbc6d89325d..3d34c4f2e891 100644
> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> @@ -13,6 +13,14 @@ Optional properties:
>  
>  - reset-gpios: GPIO to be used to reset the whole device
>  
> +Optional MAC configuration properties:
> +
> +- qca,exchange-mac0-mac6:	If present, internally swaps MAC0 and MAC6.
> +- qca,sgmii-rxclk-falling-edge:	If present, sets receive clock phase to
> +				falling edge.
> +- qca,sgmii-txclk-falling-edge:	If present, sets transmit clock phase to
> +				falling edge.

Are not these two mutually exclusive, that is the presence of one
implies the absence of the other?
-- 
Florian

Re: [PATCH 1/2] net: dsa: qca8k: Add additional PORT0_PAD_CTRL options

[Jakub Kicinski]

On Mon, 13 Jul 2020 21:50:25 +0100 Matthew Hagan wrote:
> +	u32 val = qca8k_read(priv, QCA8K_REG_PORT0_PAD_CTRL);

> +		val |= QCA8K_PORT0_PAD_CTRL_MAC06_EXCHG;

> +		val |= QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE;

> +		val |= QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE;

> +	qca8k_write(priv, QCA8K_REG_PORT0_PAD_CTRL, val);

> +	val = qca8k_read(priv, reg);

> +		val |= QCA8K_PORT_PAD_RGMII_EN;
> +		qca8k_write(priv, reg, val);

> +		val |= QCA8K_PORT_PAD_RGMII_EN |
> +		       QCA8K_PORT_PAD_RGMII_TX_DELAY(QCA8K_MAX_DELAY) |
> +		       QCA8K_PORT_PAD_RGMII_RX_DELAY(QCA8K_MAX_DELAY);
> +		qca8k_write(priv, reg, val);


> +		val |= QCA8K_PORT_PAD_SGMII_EN;
> +		qca8k_write(priv, reg, val);

Since throughout the patch you're only setting bits perhaps
qca8k_reg_set() would be a better choice than manually reading 
and then writing?