mbox series

[net-next,00/13] Add NXP SJA1110 support to the sja1105 DSA driver

Message ID 20210524232214.1378937-1-olteanv@gmail.com
Headers show
Series Add NXP SJA1110 support to the sja1105 DSA driver | expand

Message

Vladimir Oltean May 24, 2021, 11:22 p.m. UTC
From: Vladimir Oltean <vladimir.oltean@nxp.com>

The NXP SJA1110 is an automotive Ethernet switch with an embedded Arm
Cortex-M7 microcontroller. The switch has 11 ports (10 external + one
for the DSA-style connection to the microcontroller).
The microcontroller can be disabled and the switch can be controlled
over SPI, a la SJA1105 - this is how this driver handles things.

There are some integrated NXP PHYs (100base-T1 and 100base-TX). Their
initialization is handled by their own PHY drivers, the switch is only
concerned with enabling register accesses to them, by registering two
MDIO buses.

PHY interrupts might be possible, however I believe that the board I am
working on does not have them wired, which makes things a bit more
difficult to test.

Cc: Russell King <linux@armlinux.org.uk>

Vladimir Oltean (13):
  net: dsa: sja1105: be compatible with "ethernet-ports" OF node name
  net: dsa: sja1105: allow SGMII PCS configuration to be per port
  net: dsa: sja1105: the 0x1F0000 SGMII "base address" is actually
    MDIO_MMD_VEND2
  net: dsa: sja1105: cache the phy-mode port property
  net: dsa: sja1105: add a PHY interface type compatibility matrix
  net: dsa: sja1105: add a translation table for port speeds
  net: dsa: sja1105: always keep RGMII ports in the MAC role
  net: dsa: sja1105: some table entries are always present when read
    dynamically
  dt-bindings: net: dsa: sja1105: add compatible strings for SJA1110
  net: dsa: sja1105: add support for the SJA1110 switch family
  net: dsa: sja1105: register the MDIO buses for 100base-T1 and
    100base-TX
  net: dsa: sja1105: expose the SGMII PCS as an mdio_device
  net: dsa: sja1105: add support for the SJA1110 SGMII/2500base-x PCS

 .../devicetree/bindings/net/dsa/sja1105.txt   |   4 +
 drivers/net/dsa/sja1105/Makefile              |   1 +
 drivers/net/dsa/sja1105/sja1105.h             |  88 ++-
 drivers/net/dsa/sja1105/sja1105_clocking.c    | 120 +++-
 .../net/dsa/sja1105/sja1105_dynamic_config.c  | 336 ++++++++++-
 .../net/dsa/sja1105/sja1105_dynamic_config.h  |   1 +
 drivers/net/dsa/sja1105/sja1105_main.c        | 518 +++++++++++++----
 drivers/net/dsa/sja1105/sja1105_mdio.c        | 530 ++++++++++++++++++
 drivers/net/dsa/sja1105/sja1105_sgmii.h       |  63 ++-
 drivers/net/dsa/sja1105/sja1105_spi.c         | 368 +++++++++++-
 .../net/dsa/sja1105/sja1105_static_config.c   | 483 ++++++++++++++++
 .../net/dsa/sja1105/sja1105_static_config.h   |  98 +++-
 12 files changed, 2442 insertions(+), 168 deletions(-)
 create mode 100644 drivers/net/dsa/sja1105/sja1105_mdio.c

Comments

Andrew Lunn May 25, 2021, 2:03 a.m. UTC | #1
> There are some integrated NXP PHYs (100base-T1 and 100base-TX). Their

> initialization is handled by their own PHY drivers, the switch is only

> concerned with enabling register accesses to them, by registering two

> MDIO buses.

> 

> PHY interrupts might be possible, however I believe that the board I am

> working on does not have them wired, which makes things a bit more

> difficult to test.


In general, internal PHYs have an internal interrupt controller, often
in the switch register space. There then might be one interrupt from
the switch to the host. It could be this one interrupt is missing on
your board. But this is also quite common with mv88e6xxx boards. So i
added code to poll the interrupt bit, i think 10 times per
second. Polling one bit 10 times a second is more efficient than
having phylib poll each PHY every second when it needs to read a
number of registers. And the latency is better.

       Andrew
Florian Fainelli May 25, 2021, 2:13 a.m. UTC | #2
On 5/24/2021 4:22 PM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>

> 

> Since commit f2f3e09396be ("net: dsa: sja1105: be compatible with

> "ethernet-ports" OF node name"), DSA supports the "ethernet-ports" name

> for the container node of the ports, but the sja1105 driver doesn't,

> because it handles some device tree parsing of its own.

> 

> Add the second node name as a fallback.

> 

> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>


Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

-- 
Florian
Florian Fainelli May 25, 2021, 2:16 a.m. UTC | #3
On 5/24/2021 4:22 PM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>

> 

> The SJA1105 R and S switches have 1 SGMII port (port 4). Because there

> is only one such port, there is no "port" parameter in the configuration

> code for the SGMII PCS.

> 

> However, the SJA1110 can have up to 4 SGMII ports, each with its own

> SGMII register map. So we need to generalize the logic.

> 

> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

> ---

[snip]
>  

> -	if (sja1105_supports_sgmii(priv, SJA1105_SGMII_PORT)) {

> -		bool an_enabled = !!(bmcr & BMCR_ANENABLE);

> +		if (!sja1105_supports_sgmii(priv, i))

> +			continue;

> +

> +		an_enabled = !!(bmcr[i] & BMCR_ANENABLE);


Nit you could have a temporary bmcr variable here in the loop which
aliases bmcr[i] just for the sake of reducing the diff to review.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

-- 
Florian
Andrew Lunn May 25, 2021, 2:18 a.m. UTC | #4
On Tue, May 25, 2021 at 02:22:12AM +0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>

> 

> The SJA1110 contains two types of integrated PHYs: one 100base-TX PHY

> and multiple 100base-T1 PHYs.

> 

> The access procedure for the 100base-T1 PHYs is also different than it

> is for the 100base-TX one. So we register 2 MDIO buses, one for the

> base-TX and the other for the base-T1. Each bus has an OF node which is

> a child of the "mdio" subnode of the switch, and they are recognized by

> compatible string.


The mv88e6xxx also can have two MDIO busses. It is however an internal
bus for the internal PHYs and an external bus. The code however
evolved, since earlier devices only had one MDIO i ended up with a
binding like this:

       mdio {
                #address-cells = <1>;
                #size-cells = <0>;
                interrupt-parent = <&gpio0>;
                interrupts = <27 IRQ_TYPE_LEVEL_LOW>;
                interrupt-controller;
                #interrupt-cells = <2>;

                switch0: switch@0 {
                        compatible = "marvell,mv88e6390";
                        reg = <0>;
                        reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>;

                        mdio {
                                #address-cells = <1>;
                                #size-cells = <0>;
                                switch1phy0: switch1phy0@0 {
                                        reg = <0>;
                                        interrupt-parent = <&switch0>;
                                        interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
                                };
                        };

                        mdio1 {
                                compatible = "marvell,mv88e6xxx-mdio-external";
                                #address-cells = <1>;
                                #size-cells = <0>;
                                switch1phy9: switch1phy0@9 {
                                        reg = <9>;
                                };
                        };
                };
        };

It however sounds like you have the two busses one level deeper?

It would be good if you document this as part of the binding.

   Andrew
Florian Fainelli May 25, 2021, 2:19 a.m. UTC | #5
On 5/24/2021 4:22 PM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>

> 

> Looking at the SGMII PCS from SJA1110, which is accessed indirectly

> through a different base address as can be seen in the next patch, it

> appears odd that the address accessed through indirection still

> references the base address from the SJA1105S register map (first MDIO

> register is at 0x1f0000), when it could index the SGMII registers

> starting from zero.

> 

> Except that the 0x1f0000 is not a base address at all, it seems. It is

> 0x1f << 16 | 0x0000, and 0x1f is coding for the vendor-specific MMD2.

> So, it turns out, the Synopsys PCS implements all its registers inside

> the vendor-specific MMDs 1 and 2 (0x1e and 0x1f). This explains why the

> PCS has no overlaps (for the other MMDs) with other register regions of

> the switch (because no other MMDs are implemented).

> 

> Change the code to remove the SGMII "base address" and explicitly encode

> the MMD for reads/writes. This will become necessary for SJA1110 support.

> 

> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

> ---


[snip]
>  

> @@ -1905,7 +1904,9 @@ int sja1105_static_config_reload(struct sja1105_private *priv,

>  		mac[i].speed = SJA1105_SPEED_AUTO;

>  

>  		if (sja1105_supports_sgmii(priv, i))

> -			bmcr[i] = sja1105_sgmii_read(priv, i, MII_BMCR);

> +			bmcr[i] = sja1105_sgmii_read(priv, i,

> +						     MDIO_MMD_VEND2,

> +						     MDIO_CTRL1);


This appears different from what you had before?
-- 
Florian
Florian Fainelli May 25, 2021, 2:23 a.m. UTC | #6
On 5/24/2021 4:22 PM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>

> 

> On the SJA1105, all ports support the parallel "xMII" protocols (MII,

> RMII, RGMII) except for port 4 on SJA1105R/S which supports only SGMII.

> This was relatively easy to model, by special-casing the SGMII port.

> 

> On the SJA1110, certain ports can be pinmuxed between SGMII and xMII, or

> between SGMII and an internal 100base-TX PHY. This creates problems,

> because the driver's assumption so far was that if a port supports

> SGMII, it uses SGMII.

> 

> We allow the device tree to tell us how the port pinmuxing is done, and

> check that against a PHY interface type compatibility matrix for

> plausibility.

> 

> The other big change is that instead of doing SGMII configuration based

> on what the port supports, we do it based on what is the configured

> phy_mode of the port.

> 

> The 2500base-x support added in this patch is not complete.

> 

> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

> ---

>  drivers/net/dsa/sja1105/sja1105.h      |  5 +++

>  drivers/net/dsa/sja1105/sja1105_main.c | 59 +++++++++++++-------------

>  drivers/net/dsa/sja1105/sja1105_spi.c  | 20 +++++++++

>  3 files changed, 55 insertions(+), 29 deletions(-)

> 

> diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h

> index d5c0217b1f65..a27841642693 100644

> --- a/drivers/net/dsa/sja1105/sja1105.h

> +++ b/drivers/net/dsa/sja1105/sja1105.h

> @@ -111,6 +111,11 @@ struct sja1105_info {

>  				enum packing_op op);

>  	int (*clocking_setup)(struct sja1105_private *priv);

>  	const char *name;

> +	bool supports_mii[SJA1105_MAX_NUM_PORTS];

> +	bool supports_rmii[SJA1105_MAX_NUM_PORTS];

> +	bool supports_rgmii[SJA1105_MAX_NUM_PORTS];

> +	bool supports_sgmii[SJA1105_MAX_NUM_PORTS];

> +	bool supports_2500basex[SJA1105_MAX_NUM_PORTS];


If you used a bitmap you may be able to play some nice tricks with
ordering them in PHY_INTERFACE_MODE_* order and just increment a pointer
to the bitmap.

Since it looks like all of the chips support MII, RMII, and RGMII on all
ports, maybe you can specify only those that don't?

Still:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

-- 
Florian
Florian Fainelli May 25, 2021, 2:24 a.m. UTC | #7
On 5/24/2021 4:22 PM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>

> 

> In SJA1105, the xMII Mode Parameters Table field called PHY_MAC denotes

> the 'role' of the port, be it a PHY or a MAC. This makes a difference in

> the MII and RMII protocols, but RGMII is symmetric, so either PHY or MAC

> settings result in the same hardware behavior.

> 

> The SJA1110 is different, and the RGMII ports only work when configured

> in MAC mode, so keep the port roles in MAC mode unconditionally.

> 

> Why we had an RGMII port in the PHY role in the first place was because

> we wanted to have a way in the driver to denote whether RGMII delays

> should be applied based on the phy-mode property or not. This is already

> done in sja1105_parse_rgmii_delays() based on an intermediary

> struct sja1105_dt_port (which contains the port role). So it is a

> logical fallacy to use the hardware configuration as a scratchpad for

> driver data, it isn't necessary.

> 

> We can also remove the gating condition for applying RGMII delays only

> for ports in the PHY role. The .setup_rgmii_delay() method looks at

> the priv->rgmii_rx_delay[port] and priv->rgmii_tx_delay[port] properties

> which are already populated properly (in the case of a port in the MAC

> role they are false). Removing this condition generates a few more SPI

> writes for these ports (clearing the RGMII delays) which are perhaps

> useless for SJA1105P/Q/R/S, where we know that the delays are disabled

> by default. But for SJA1110, the firmware on the embedded microcontroller

> might have done something funny, so it's always a good idea to clear the

> RGMII delays if that's what Linux expects.

> 

> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>


Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

-- 
Florian
Florian Fainelli May 25, 2021, 2:27 a.m. UTC | #8
On 5/24/2021 4:22 PM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>

> 

> The SJA1110 contains two types of integrated PHYs: one 100base-TX PHY

> and multiple 100base-T1 PHYs.

> 

> The access procedure for the 100base-T1 PHYs is also different than it

> is for the 100base-TX one. So we register 2 MDIO buses, one for the

> base-TX and the other for the base-T1. Each bus has an OF node which is

> a child of the "mdio" subnode of the switch, and they are recognized by

> compatible string.

> 

> Cc: Russell King <linux@armlinux.org.uk>

> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>


Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

-- 
Florian
Andrew Lunn May 25, 2021, 2:40 a.m. UTC | #9
> Except that the 0x1f0000 is not a base address at all, it seems. It is

> 0x1f << 16 | 0x0000, and 0x1f is coding for the vendor-specific MMD2.

> So, it turns out, the Synopsys PCS implements all its registers inside

> the vendor-specific MMDs 1 and 2 (0x1e and 0x1f).


Any idea if this is specific to this device, or how the Synopsys PCS
does this in general? I was wondering if the code could be pulled out
and placed in driver/net/pcs, if it could be shared with other devices
using this IP?

      Andrew
Vladimir Oltean May 25, 2021, 9:12 a.m. UTC | #10
On Mon, May 24, 2021 at 07:19:17PM -0700, Florian Fainelli wrote:
> 

> 

> On 5/24/2021 4:22 PM, Vladimir Oltean wrote:

> > From: Vladimir Oltean <vladimir.oltean@nxp.com>

> > 

> > Looking at the SGMII PCS from SJA1110, which is accessed indirectly

> > through a different base address as can be seen in the next patch, it

> > appears odd that the address accessed through indirection still

> > references the base address from the SJA1105S register map (first MDIO

> > register is at 0x1f0000), when it could index the SGMII registers

> > starting from zero.

> > 

> > Except that the 0x1f0000 is not a base address at all, it seems. It is

> > 0x1f << 16 | 0x0000, and 0x1f is coding for the vendor-specific MMD2.

> > So, it turns out, the Synopsys PCS implements all its registers inside

> > the vendor-specific MMDs 1 and 2 (0x1e and 0x1f). This explains why the

> > PCS has no overlaps (for the other MMDs) with other register regions of

> > the switch (because no other MMDs are implemented).

> > 

> > Change the code to remove the SGMII "base address" and explicitly encode

> > the MMD for reads/writes. This will become necessary for SJA1110 support.

> > 

> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

> > ---

> 

> [snip]

> >  

> > @@ -1905,7 +1904,9 @@ int sja1105_static_config_reload(struct sja1105_private *priv,

> >  		mac[i].speed = SJA1105_SPEED_AUTO;

> >  

> >  		if (sja1105_supports_sgmii(priv, i))

> > -			bmcr[i] = sja1105_sgmii_read(priv, i, MII_BMCR);

> > +			bmcr[i] = sja1105_sgmii_read(priv, i,

> > +						     MDIO_MMD_VEND2,

> > +						     MDIO_CTRL1);

> 

> This appears different from what you had before?


MDIO_CTRL1 is the clause 45 alias of MII_BMCR, all in all it is still a
cosmetic change in line with the patch's idea of expressing accesses as
clause 45. I didn't replace the "bmcr" variable names because that would
have introduced more noise than I would have liked.
Vladimir Oltean May 25, 2021, 11:54 a.m. UTC | #11
On Tue, May 25, 2021 at 04:18:24AM +0200, Andrew Lunn wrote:
> On Tue, May 25, 2021 at 02:22:12AM +0300, Vladimir Oltean wrote:

> > From: Vladimir Oltean <vladimir.oltean@nxp.com>

> > 

> > The SJA1110 contains two types of integrated PHYs: one 100base-TX PHY

> > and multiple 100base-T1 PHYs.

> > 

> > The access procedure for the 100base-T1 PHYs is also different than it

> > is for the 100base-TX one. So we register 2 MDIO buses, one for the

> > base-TX and the other for the base-T1. Each bus has an OF node which is

> > a child of the "mdio" subnode of the switch, and they are recognized by

> > compatible string.

> 

> The mv88e6xxx also can have two MDIO busses. It is however an internal

> bus for the internal PHYs and an external bus. The code however

> evolved, since earlier devices only had one MDIO i ended up with a

> binding like this:

> 

>        mdio {

>                 #address-cells = <1>;

>                 #size-cells = <0>;

>                 interrupt-parent = <&gpio0>;

>                 interrupts = <27 IRQ_TYPE_LEVEL_LOW>;

>                 interrupt-controller;

>                 #interrupt-cells = <2>;

> 

>                 switch0: switch@0 {

>                         compatible = "marvell,mv88e6390";

>                         reg = <0>;

>                         reset-gpios = <&gpio5 1 GPIO_ACTIVE_LOW>;

> 

>                         mdio {

>                                 #address-cells = <1>;

>                                 #size-cells = <0>;

>                                 switch1phy0: switch1phy0@0 {

>                                         reg = <0>;

>                                         interrupt-parent = <&switch0>;

>                                         interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;

>                                 };

>                         };

> 

>                         mdio1 {

>                                 compatible = "marvell,mv88e6xxx-mdio-external";

>                                 #address-cells = <1>;

>                                 #size-cells = <0>;

>                                 switch1phy9: switch1phy0@9 {

>                                         reg = <9>;

>                                 };

>                         };

>                 };

>         };

> 

> It however sounds like you have the two busses one level deeper?

> 

> It would be good if you document this as part of the binding.


Yes, it looks like this:

	ethernet-switch@2 {
		compatible = "nxp,sja1110a";

		ethernet-ports {
			...
		};

		mdio {
			#address-cells = <1>;
			#size-cells = <0>;

			mdio@0 {
				reg = <0>;
				compatible = "nxp,sja1110-base-t1-mdio";
				#address-cells = <1>;
				#size-cells = <0>;

				sw2_port5_base_t1_phy: ethernet-phy@1 {
					compatible = "ethernet-phy-ieee802.3-c45";
					reg = <0x1>;
				};

				...
			};

			mdio@1 {
				reg = <1>;
				compatible = "nxp,sja1110-base-tx-mdio";
				#address-cells = <1>;
				#size-cells = <0>;

				sw2_port1_base_tx_phy: ethernet-phy@1 {
					reg = <0x1>;
				};
			};
		};
	};

I will try to document it.
Andrew Lunn May 25, 2021, 1:16 p.m. UTC | #12
> > It however sounds like you have the two busses one level deeper?

> > 

> > It would be good if you document this as part of the binding.

> 

> Yes, it looks like this:

> 

> 	ethernet-switch@2 {

> 		compatible = "nxp,sja1110a";

> 

> 		ethernet-ports {

> 			...

> 		};

> 

> 		mdio {

> 			#address-cells = <1>;

> 			#size-cells = <0>;

> 

> 			mdio@0 {

> 				reg = <0>;

> 				compatible = "nxp,sja1110-base-t1-mdio";

> 				#address-cells = <1>;

> 				#size-cells = <0>;

> 

> 				sw2_port5_base_t1_phy: ethernet-phy@1 {

> 					compatible = "ethernet-phy-ieee802.3-c45";

> 					reg = <0x1>;

> 				};

> 

> 				...

> 			};

> 


We should run this by Rob.

That is probably not the intention of
Documentation/devicetree/bindings/net/mdio.yaml, it works because of
additionalProperties: true

What meaning does reg have, in mdio@0?

     Andrew
Vladimir Oltean May 25, 2021, 1:21 p.m. UTC | #13
On Tue, May 25, 2021 at 03:16:52PM +0200, Andrew Lunn wrote:
> > > It however sounds like you have the two busses one level deeper?

> > > 

> > > It would be good if you document this as part of the binding.

> > 

> > Yes, it looks like this:

> > 

> > 	ethernet-switch@2 {

> > 		compatible = "nxp,sja1110a";

> > 

> > 		ethernet-ports {

> > 			...

> > 		};

> > 

> > 		mdio {

> > 			#address-cells = <1>;

> > 			#size-cells = <0>;

> > 

> > 			mdio@0 {

> > 				reg = <0>;

> > 				compatible = "nxp,sja1110-base-t1-mdio";

> > 				#address-cells = <1>;

> > 				#size-cells = <0>;

> > 

> > 				sw2_port5_base_t1_phy: ethernet-phy@1 {

> > 					compatible = "ethernet-phy-ieee802.3-c45";

> > 					reg = <0x1>;

> > 				};

> > 

> > 				...

> > 			};

> > 

> 

> We should run this by Rob.

> 

> That is probably not the intention of

> Documentation/devicetree/bindings/net/mdio.yaml, it works because of

> additionalProperties: true

> 

> What meaning does reg have, in mdio@0?


None apart from "not the other MDIO bus".
I haven't run this through the DT validator yet, I am doing that right
now and will copy Rob to the next patch series.
Just to be clear, what is your suggestion for this? I am not a great fan
of 2 internal MDIO buses, but as mentioned, the PHY access procedure is
different for the 100base-TX and the 100base-T1 PHYs.
Andrew Lunn May 25, 2021, 1:43 p.m. UTC | #14
> Just to be clear, what is your suggestion for this? I am not a great fan

> of 2 internal MDIO buses, but as mentioned, the PHY access procedure is

> different for the 100base-TX and the 100base-T1 PHYs.


Do what the mv88e6xxx driver does. Have two busses, but do not put
them into a container node. You then avoid issues with the yaml
validater and not need the reg values etc.

	  Andrew
Vladimir Oltean May 26, 2021, 11:52 a.m. UTC | #15
On Tue, May 25, 2021 at 03:43:13PM +0200, Andrew Lunn wrote:
> > Just to be clear, what is your suggestion for this? I am not a great fan

> > of 2 internal MDIO buses, but as mentioned, the PHY access procedure is

> > different for the 100base-TX and the 100base-T1 PHYs.

> 

> Do what the mv88e6xxx driver does. Have two busses, but do not put

> them into a container node. You then avoid issues with the yaml

> validater and not need the reg values etc.


It's funny, because marvell,mv88e6xxx-mdio-external is not covered by
any yaml schema, and even if it was, the "mdio1" node name would not
match the pattern "^mdio(@.*)?" required by mdio.yaml (which reads
'must be "mdio" or "mdio@some number", and "mdio1" is neither of those).
Vladimir Oltean May 26, 2021, 12:37 p.m. UTC | #16
On Mon, May 24, 2021 at 07:23:09PM -0700, Florian Fainelli wrote:
> 

> 

> On 5/24/2021 4:22 PM, Vladimir Oltean wrote:

> > From: Vladimir Oltean <vladimir.oltean@nxp.com>

> > 

> > On the SJA1105, all ports support the parallel "xMII" protocols (MII,

> > RMII, RGMII) except for port 4 on SJA1105R/S which supports only SGMII.

> > This was relatively easy to model, by special-casing the SGMII port.

> > 

> > On the SJA1110, certain ports can be pinmuxed between SGMII and xMII, or

> > between SGMII and an internal 100base-TX PHY. This creates problems,

> > because the driver's assumption so far was that if a port supports

> > SGMII, it uses SGMII.

> > 

> > We allow the device tree to tell us how the port pinmuxing is done, and

> > check that against a PHY interface type compatibility matrix for

> > plausibility.

> > 

> > The other big change is that instead of doing SGMII configuration based

> > on what the port supports, we do it based on what is the configured

> > phy_mode of the port.

> > 

> > The 2500base-x support added in this patch is not complete.

> > 

> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

> > ---

> >  drivers/net/dsa/sja1105/sja1105.h      |  5 +++

> >  drivers/net/dsa/sja1105/sja1105_main.c | 59 +++++++++++++-------------

> >  drivers/net/dsa/sja1105/sja1105_spi.c  | 20 +++++++++

> >  3 files changed, 55 insertions(+), 29 deletions(-)

> > 

> > diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h

> > index d5c0217b1f65..a27841642693 100644

> > --- a/drivers/net/dsa/sja1105/sja1105.h

> > +++ b/drivers/net/dsa/sja1105/sja1105.h

> > @@ -111,6 +111,11 @@ struct sja1105_info {

> >  				enum packing_op op);

> >  	int (*clocking_setup)(struct sja1105_private *priv);

> >  	const char *name;

> > +	bool supports_mii[SJA1105_MAX_NUM_PORTS];

> > +	bool supports_rmii[SJA1105_MAX_NUM_PORTS];

> > +	bool supports_rgmii[SJA1105_MAX_NUM_PORTS];

> > +	bool supports_sgmii[SJA1105_MAX_NUM_PORTS];

> > +	bool supports_2500basex[SJA1105_MAX_NUM_PORTS];

> 

> If you used a bitmap you may be able to play some nice tricks with

> ordering them in PHY_INTERFACE_MODE_* order and just increment a pointer

> to the bitmap.

> 

> Since it looks like all of the chips support MII, RMII, and RGMII on all

> ports, maybe you can specify only those that don't?

> 

> Still:

> 

> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>


Because the SJA1110 doesn't have public documentation, I am making a bit
of a compromise for SJA1105 in order to be very clear about which SJA1110
port supports which PHY interface type. With pointers to the beginning
and end of a phy_interface_t array for each switch type and port number,
it wouldn't be quite as clear, in fact it might end up quite a bit
messy.
Vladimir Oltean May 26, 2021, 12:39 p.m. UTC | #17
On Mon, May 24, 2021 at 07:16:20PM -0700, Florian Fainelli wrote:
> 

> 

> On 5/24/2021 4:22 PM, Vladimir Oltean wrote:

> > From: Vladimir Oltean <vladimir.oltean@nxp.com>

> > 

> > The SJA1105 R and S switches have 1 SGMII port (port 4). Because there

> > is only one such port, there is no "port" parameter in the configuration

> > code for the SGMII PCS.

> > 

> > However, the SJA1110 can have up to 4 SGMII ports, each with its own

> > SGMII register map. So we need to generalize the logic.

> > 

> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

> > ---

> [snip]

> >  

> > -	if (sja1105_supports_sgmii(priv, SJA1105_SGMII_PORT)) {

> > -		bool an_enabled = !!(bmcr & BMCR_ANENABLE);

> > +		if (!sja1105_supports_sgmii(priv, i))

> > +			continue;

> > +

> > +		an_enabled = !!(bmcr[i] & BMCR_ANENABLE);

> 

> Nit you could have a temporary bmcr variable here in the loop which

> aliases bmcr[i] just for the sake of reducing the diff to review.

> 

> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>


I could have, but I don't have a good name for the bmcr array, and
"bmcr = bmcr_arr[i]" looks worse to me than just working with bmcr[i].
Vladimir Oltean May 26, 2021, 12:51 p.m. UTC | #18
On Tue, May 25, 2021 at 04:03:47AM +0200, Andrew Lunn wrote:
> > There are some integrated NXP PHYs (100base-T1 and 100base-TX). Their

> > initialization is handled by their own PHY drivers, the switch is only

> > concerned with enabling register accesses to them, by registering two

> > MDIO buses.

> > 

> > PHY interrupts might be possible, however I believe that the board I am

> > working on does not have them wired, which makes things a bit more

> > difficult to test.

> 

> In general, internal PHYs have an internal interrupt controller, often

> in the switch register space. There then might be one interrupt from

> the switch to the host. It could be this one interrupt is missing on

> your board. But this is also quite common with mv88e6xxx boards. So i

> added code to poll the interrupt bit, i think 10 times per

> second. Polling one bit 10 times a second is more efficient than

> having phylib poll each PHY every second when it needs to read a

> number of registers. And the latency is better.


That is a good suggestion and probably what I'll end up doing, but not
in this patch series since it is already on the heavy side, and getting
access to the interrupt status registers isn't easy-peasy.