mbox series

[RFC,v2,linux-next,00/14] Add NXP SJA1110 support to the sja1105 DSA driver

Message ID 20210526135535.2515123-1-vladimir.oltean@nxp.com
Headers show
Series Add NXP SJA1110 support to the sja1105 DSA driver | expand

Message

Vladimir Oltean May 26, 2021, 1:55 p.m. UTC
This patch series is sent as RFC and based on linux-next because it
depends on some changes which are in "net" but not in "net-next".

Changes in v2:
- converted nxp,sja1105 DT bindings to YAML
- registered the PCS MDIO bus and forced auto-probing off for all PHY
  addresses for this bus
- changed the container node name for the 2 MDIO buses from "mdio" to
  "mdios" to avoid matching on the mdio.yaml schema (it's just a
  container node, not an MDIO bus)
- fixed an uninitialized "offset" variable usage in
  sja1110_pcs_mdio_{read,write}
- using the mdiobus_c45_addr macro instead of open-coding that operation

Reason for reposting so early:
Would like some feedback on the DT bindings for the internal MDIO buses.

Feedback from v1 not addressed:
(Q) Can the Synopsys PCS initialization code be moved into
    drivers/net/pcs/xpcs.c?
(A) Yes and no. Initializing the PCS is not sufficient for proper
    SGMII/2500base-x operation, one also needs to initialize the
    non-Synopsys PMA/PMD, which is accessible through the same
    struct mdio_device as the PCS itself.
(Q) No interrupts for the internal PHYs?
(A) In a later patch series (this one is already large), and only in a
    reduced functionality mode, where the switch driver registers an
    irqchip but it busy polls the interrupt status register. The board I
    am working on does not have the switch interrupt pin wired.

Previous cover letter:

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.

Cc: Russell King <linux@armlinux.org.uk>
Cc: Rob Herring <robh@kernel.org>

Vladimir Oltean (14):
  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: convert to YAML schema
  dt-bindings: net: dsa: sja1105: add SJA1110 bindings
  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

 .../bindings/net/dsa/nxp,sja1105.yaml         | 172 ++++++
 .../devicetree/bindings/net/dsa/sja1105.txt   | 156 -----
 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        | 539 ++++++++++++++++++
 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 +++-
 13 files changed, 2619 insertions(+), 324 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
 delete mode 100644 Documentation/devicetree/bindings/net/dsa/sja1105.txt
 create mode 100644 drivers/net/dsa/sja1105/sja1105_mdio.c

Comments

Rob Herring (Arm) May 26, 2021, 2:19 p.m. UTC | #1
On Wed, May 26, 2021 at 8:56 AM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> There are 4 variations of the SJA1110 switch which have a different set
> of MII protocols supported per port. Document the compatible strings.
>
> Also, the SJA1110 optionally supports 2 internal MDIO buses for 2
> different types of Ethernet PHYs. Document a container node called
> "mdios" which has 2 subnodes "mdio@0" and "mdio@1", identifiable via
> compatible string, under which the driver finds the internal PHYs.
>
> Cc: Rob Herring <robh@kernel.org>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> Changes in v2:
> Patch is new.
>
>  .../bindings/net/dsa/nxp,sja1105.yaml         | 43 +++++++++++++++++++
>  1 file changed, 43 insertions(+)

Please use get_maintainers.pl and resend to the right lists.
Specifically, the DT list in this case.

> diff --git a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
> index c1f18849a54a..640da65b0f59 100644
> --- a/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/nxp,sja1105.yaml
> @@ -28,10 +28,53 @@ properties:
>            - nxp,sja1105q
>            - nxp,sja1105r
>            - nxp,sja1105s
> +          - nxp,sja1110a
> +          - nxp,sja1110b
> +          - nxp,sja1110c
> +          - nxp,sja1110d
>
>    reg:
>      maxItems: 1
>
> +  # Optional container node for the 2 internal MDIO buses of the SJA1110
> +  # (one for the internal 100base-T1 PHYs and the other for the single
> +  # 100base-TX PHY). The "reg" property does not have physical significance.
> +  # The PHY addresses to port correspondence is as follows: for 100base-T1,
> +  # port 5 has PHY 1, port 6 has PHY 2 etc, while for 100base-TX, port 1 has
> +  # PHY 1.
> +  mdios:
> +    type: object
> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +      '#size-cells':
> +        const: 0
> +
> +    patternProperties:
> +      "^mdio@[0-1]$":
> +        type: object
> +
> +        allOf:
> +          - $ref: "http://devicetree.org/schemas/net/mdio.yaml#"
> +
> +        properties:
> +          compatible:
> +            oneOf:
> +              - enum:

Don't need oneOf when there is only 1 entry.


> +                  - nxp,sja1110-base-t1-mdio
> +                  - nxp,sja1110-base-tx-mdio
> +
> +          reg:
> +            oneOf:
> +              - enum:
> +                - 0
> +                - 1
> +
> +        required:
> +          - compatible
> +          - reg
> +
>  patternProperties:
>    "^(ethernet-)?ports$":
>      type: object
> --
> 2.25.1
>
Russell King (Oracle) May 26, 2021, 3:24 p.m. UTC | #2
On Wed, May 26, 2021 at 04:55:24PM +0300, Vladimir Oltean wrote:
> -	const struct sja1105_regs *regs = priv->info->regs;
> +	u64 addr = (mmd << 16) | pcs_reg;

What is the reason for using "u64" here. pcs_reg is 16-bits, and mmd is
five bits, which is well below 32 bits. So, why not u32?
Russell King (Oracle) May 26, 2021, 3:42 p.m. UTC | #3
On Wed, May 26, 2021 at 06:34:47PM +0300, Vladimir Oltean wrote:
> Hi Russell,
> 
> On Wed, May 26, 2021 at 04:24:54PM +0100, Russell King (Oracle) wrote:
> > On Wed, May 26, 2021 at 04:55:24PM +0300, Vladimir Oltean wrote:
> > > -	const struct sja1105_regs *regs = priv->info->regs;
> > > +	u64 addr = (mmd << 16) | pcs_reg;
> > 
> > What is the reason for using "u64" here. pcs_reg is 16-bits, and mmd is
> > five bits, which is well below 32 bits. So, why not u32?
> 
> The "addr" variable holds a SPI address, and in the sja1105 driver, the
> SPI addresses are universally held in u64 variables, mainly because of
> the packing() API (Documentation/core-api/packing.rst).

As you are passing it into a function, the argument of which is a u64,
the compiler will promote the u32 to a u64 by itself. I guess it
doesn't actually matter, but the current code just looks really weird.