Message-ID: <20220510014350.GI895@COLIN-DESKTOP1.localdomain>
Date:   Mon, 9 May 2022 18:43:50 -0700
From:   Colin Foster <colin.foster@...advantage.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Terry Bowman <terry.bowman@....com>,
        Wolfram Sang <wsa@...nel.org>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Steen Hegelund <Steen.Hegelund@...rochip.com>,
        Lars Povlsen <lars.povlsen@...rochip.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Russell King <linux@...linux.org.uk>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Lee Jones <lee.jones@...aro.org>
Subject: Re: [RFC v8 net-next 00/16] add support for VSC7512 control over SPI

On Mon, May 09, 2022 at 05:13:05PM +0000, Vladimir Oltean wrote:
> Hi Colin,
> 
> On Sun, May 08, 2022 at 11:52:57AM -0700, Colin Foster wrote:
> > The patch set in general is to add support for the VSC7512, and
> > eventually the VSC7511, VSC7513 and VSC7514 devices controlled over
> > SPI. The driver is believed to be fully functional for the internal
> > phy ports (0-3)  on the VSC7512. It is not yet functional for SGMII,
> > QSGMII, and SerDes ports.
> > 
> > I have mentioned previously:
> > The hardware setup I'm using for development is a beaglebone black, with
> > jumpers from SPI0 to the microchip VSC7512 dev board. The microchip dev
> > board has been modified to not boot from flash, but wait for SPI. An
> > ethernet cable is connected from the beaglebone ethernet to port 0 of
> > the dev board.
> > 
> > The relevant sections of the device tree I'm using for the VSC7512 is
> > below. Notably the SGPIO LEDs follow link status and speed from network
> > triggers.
> > 
> > In order to make this work, I have modified the cpsw driver, and now the
> > cpsw_new driver, to allow for frames over 1500 bytes. Otherwise the
> > tagging protocol will not work between the beaglebone and the VSC7512. I
> > plan to eventually try to get those changes in mainline, but I don't
> > want to get distracted from my initial goal. I also had to change
> > bonecommon.dtsi to avoid using VLAN 0.
> 
> This ti,dual-emac-pvid thing is a really odd thing to put in the device
> tree. But what's the problem with VLAN 0 anyway?

Ahh, I see that was an exchange between me and Grygorii Strashko that
wasn't public. Looking now, it might be VLAN 1...

I'd see "failed to initialize vlan filtering" when I ran "ip link set
dev swp1 master br0" because the default bridge vlan conflicted with
slave_data->dual_emac_res_vlan = port_id;
(drivers/net/ethernet/ti/cpsw_new.c, around line 1325)

My initial attempt was to just change cpsw_port1
ti,dual-emac-pvid=<12>; but that didn't change the behavior. Maybe if I
went back to it again, seeing as I'm much older and wiser than I was
before, I could find the correct device tree solution... Ideally I think
I should have the ability to not enable cpsw_port1 and be good.

But I think the magic was really just to set
slave_data->dual_emac_res_vlan = 10 + port_id; to avoid conflicts.


This became an issue at 5.15, when cpsw_new was rolled in to the .dtsis
I've been using.

> 
> > 
> > I believe much of the MFD sections are very near feature-complete,
> > whereas the switch section will require ongoing work to enable
> > additional ports / features. This could lead to a couple potential
> > scenarios:
> > 
> > The first being patches 1-8 being split into a separate patch set, while
> > patches 9-16 remain in the RFC state. This would offer the pinctrl /
> > sgpio / mdio controller functionality, but no switch control until it is
> > ready.
> > 
> > The second would assume the current state of the switch driver is
> > acceptable (or at least very near so) and the current patch set gets an
> > official PATCH set (with minor changes as necessary - e.g. squashing
> > patch 16 into 14). That might be ambitious.
> > 
> > The third would be to keep this patch set in RFC until switch
> > functionality is more complete. I'd understand if this was the desired
> > path... but it would mean me having to bug more reviewers.
> 
> Considering that the merge window is approaching, I'd say get the
> non-DSA stuff accepted until then, then repost the DSA stuff in ~3 weeks
> from now as non-RFC, once v5.18 is cut and the development for v5.20
> (or whatever the number will be) begins.

That's the approach I'd prefer as well.

> 
> > / {
> > 	vscleds {
> > 		compatible = "gpio-leds";
> > 		vscled@0 {
> > 			label = "port0led";
> > 			gpios = <&sgpio_out1 0 0 GPIO_ACTIVE_LOW>;
> > 			default-state = "off";
> > 			linux,default-trigger = "ocelot-miim0.2.auto-mii:00:link";
> > 		};
> > 		vscled@1 {
> > 			label = "port0led1";
> > 			gpios = <&sgpio_out1 0 1 GPIO_ACTIVE_LOW>;
> > 			default-state = "off";
> > 			linux,default-trigger = "ocelot-miim0.2.auto-mii:00:1Gbps";
> > 		};
> > [ ... ]
> > 	};
> > };
> > 
> > &spi0 {
> > 	#address-cells = <1>;
> > 	#size-cells = <0>;
> > 	status = "okay";
> > 
> > 	ocelot-chip@0 {
> > 		compatible = "mscc,vsc7512_mfd_spi";
> 
> Can you use hyphens instead of underscores in this compatible string?
> 
> > 		spi-max-frequency = <2500000>;
> > 		reg = <0>;
> > 
> > 		ethernet-switch@0 {
> 
> I don't think the switch node should have any address?
> 
> > 			compatible = "mscc,vsc7512-ext-switch";
> > 			ports {
> > 				#address-cells = <1>;
> > 				#size-cells = <0>;
> > 
> > 				port@0 {
> > 					reg = <0>;
> > 					label = "cpu";
> > 					status = "okay";
> > 					ethernet = <&mac_sw>;
> > 					phy-handle = <&sw_phy0>;
> > 					phy-mode = "internal";
> > 				};
> > 
> > 				port@1 {
> > 					reg = <1>;
> > 					label = "swp1";
> > 					status = "okay";
> > 					phy-handle = <&sw_phy1>;
> > 					phy-mode = "internal";
> > 				};
> > 			};
> > 		};
> > 
> > 		mdio0: mdio0@0 {
> 
> This is going to be interesting. Some drivers with multiple MDIO buses
> create an "mdios" container with #address-cells = <1> and put the MDIO
> bus nodes under that. Others create an "mdio" node and an "mdio0" node
> (and no address for either of them).
> 
> The problem with the latter approach is that
> Documentation/devicetree/bindings/net/mdio.yaml does not accept the
> "mdio0"/"mdio1" node name for an MDIO bus.

Hmm... That'll be interesting indeed. The 7514
(arch/mips/boot/dts/mscc/ocelot.dtsi) is where I undoubtedly started.
Is there an issue with the 7514, or is it just an issue with my
implementation, which should be:

mdio0: mdio@0 {

instead of mdio0@0?

> 
> > 			compatible = "mscc,ocelot-miim";
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 
> > 			sw_phy0: ethernet-phy@0 {
> > 				reg = <0x0>;
> > 			};
> > 
> > 			sw_phy1: ethernet-phy@1 {
> > 				reg = <0x1>;
> > 			};
> > 
> > 			sw_phy2: ethernet-phy@2 {
> > 				reg = <0x2>;
> > 			};
> > 
> > 			sw_phy3: ethernet-phy@3 {
> > 				reg = <0x3>;
> > 			};
> > 		};
> > 
> > 		mdio1: mdio1@1 {
> > 			compatible = "mscc,ocelot-miim";
> > 			pinctrl-names = "default";
> > 			pinctrl-0 = <&miim1>;
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 
> > 			sw_phy4: ethernet-phy@4 {
> > 				reg = <0x4>;
> > 			};
> > 
> > 			sw_phy5: ethernet-phy@5 {
> > 				reg = <0x5>;
> > 			};
> > 
> > 			sw_phy6: ethernet-phy@6 {
> > 				reg = <0x6>;
> > 			};
> > 
> > 			sw_phy7: ethernet-phy@7 {
> > 				reg = <0x7>;
> > 			};
> > 		};
> > 
> > 		gpio: pinctrl@0 {
> 
> Similar thing with the address. All these @0 addresses actually conflict
> with each other.
> 
> > 			compatible = "mscc,ocelot-pinctrl";
> > 			gpio-controller;
> > 			#gpio_cells = <2>;
> > 			gpio-ranges = <&gpio 0 0 22>;
> > 
> > 			led_shift_reg_pins: led-shift-reg-pins {
> > 				pins = "GPIO_0", "GPIO_1", "GPIO_2", "GPIO_3";
> > 				function = "sg0";
> > 			};
> > 
> > 			miim1: miim1 {
> > 				pins = "GPIO_14", "GPIO_15";
> > 				function = "miim";
> > 			};
> > 		};
> > 
> > 		sgpio: sgpio {
> 
> And mixing nodes with addresses with nodes without addresses is broken too.
> 
> > 			compatible = "mscc,ocelot-sgpio";
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 			bus-frequency=<12500000>;
> > 			clocks = <&ocelot_clock>;
> > 			microchip,sgpio-port-ranges = <0 15>;
> > 			pinctrl-names = "default";
> > 			pinctrl-0 = <&led_shift_reg_pins>;
> > 
> > 			sgpio_in0: sgpio@0 {
> > 				compatible = "microchip,sparx5-sgpio-bank";
> > 				reg = <0>;
> > 				gpio-controller;
> > 				#gpio-cells = <3>;
> > 				ngpios = <64>;
> > 			};
> > 
> > 			sgpio_out1: sgpio@1 {
> > 				compatible = "microchip,sparx5-sgpio-bank";
> > 				reg = <1>;
> > 				gpio-controller;
> > 				#gpio-cells = <3>;
> > 				ngpios = <64>;
> > 			};
> > 		};
> > 	};
> > };
> > 
> > And I'll include the relevant dmesg prints - I don't love the "invalid
> > resource" prints, as they seem to be misleading. They're a byproduct of
> > looking for IO resources before falling back to REG.
> > 
> > [    0.000000] Booting Linux on physical CPU 0x0
> > [    0.000000] Linux version 5.18.0-rc5-01295-g47053e327c52 (X@X) (arm-linux-gnueabi-gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0, GNU ld (GNU Binutils for Ubuntu) 2.34) #630 SMP PREEMPT Sun May 8 10:56:51 PDT 2022
> > ...
> > [    2.829319] pinctrl-ocelot ocelot-pinctrl.0.auto: DMA mask not set
> 
> Why does this get printed, if you put a dump_stack() in of_dma_configure_id()?

I'll run that tonight.

> 
> > [    2.835718] pinctrl-ocelot ocelot-pinctrl.0.auto: invalid resource
> > [    2.842717] gpiochip_find_base: found new base at 2026
> > [    2.842774] gpio gpiochip4: (ocelot-gpio): created GPIO range 0->21 ==> ocelot-pinctrl.0.auto PIN 0->21
> > [    2.845693] gpio gpiochip4: (ocelot-gpio): added GPIO chardev (254:4)
> > [    2.845828] gpio gpiochip4: registered GPIOs 2026 to 2047 on ocelot-gpio
> > [    2.845855] pinctrl-ocelot ocelot-pinctrl.0.auto: driver registered
> > [    2.855925] pinctrl-microchip-sgpio ocelot-sgpio.1.auto: DMA mask not set
> > [    2.863089] pinctrl-microchip-sgpio ocelot-sgpio.1.auto: invalid resource
> > [    2.870801] gpiochip_find_base: found new base at 1962
> > [    2.871528] gpio_stub_drv gpiochip5: (ocelot-sgpio.1.auto-input): added GPIO chardev (254:5)
> > [    2.871666] gpio_stub_drv gpiochip5: registered GPIOs 1962 to 2025 on ocelot-sgpio.1.auto-input
> > [    2.872364] gpiochip_find_base: found new base at 1898
> > [    2.873244] gpio_stub_drv gpiochip6: (ocelot-sgpio.1.auto-output): added GPIO chardev (254:6)
> > [    2.873354] gpio_stub_drv gpiochip6: registered GPIOs 1898 to 1961 on ocelot-sgpio.1.auto-output
> > [    2.881148] mscc-miim ocelot-miim0.2.auto: DMA mask not set
> > [    2.886929] mscc-miim ocelot-miim0.2.auto: invalid resource
> > [    2.893738] mdio_bus ocelot-miim0.2.auto-mii: GPIO lookup for consumer reset
> > [    2.893769] mdio_bus ocelot-miim0.2.auto-mii: using device tree for GPIO lookup
> > [    2.893802] of_get_named_gpiod_flags: can't parse 'reset-gpios' property of node '/ocp/interconnect@...00000/segment@...arget-module@...00/spi@...celot-chip@...dio0[0]'
> > [    2.893898] of_get_named_gpiod_flags: can't parse 'reset-gpio' property of node '/ocp/interconnect@...00000/segment@...arget-module@...00/spi@...celot-chip@...dio0[0]'
> > [    2.893996] mdio_bus ocelot-miim0.2.auto-mii: using lookup tables for GPIO lookup
> > [    2.894012] mdio_bus ocelot-miim0.2.auto-mii: No GPIO consumer reset found
> > [    3.395738] mdio_bus ocelot-miim0.2.auto-mii:00: GPIO lookup for consumer reset
> > [    3.395777] mdio_bus ocelot-miim0.2.auto-mii:00: using device tree for GPIO lookup
> > [    3.395840] of_get_named_gpiod_flags: can't parse 'reset-gpios' property of node '/ocp/interconnect@...00000/segment@...arget-module@...00/spi@...celot-chip@...dio0/ethernet-phy@0[0]'
> > [    3.395959] of_get_named_gpiod_flags: can't parse 'reset-gpio' property of node '/ocp/interconnect@...00000/segment@...arget-module@...00/spi@...celot-chip@...dio0/ethernet-phy@0[0]'
> > [    3.396069] mdio_bus ocelot-miim0.2.auto-mii:00: using lookup tables for GPIO lookup
> > [    3.396086] mdio_bus ocelot-miim0.2.auto-mii:00: No GPIO consumer reset found
> > ...
> > [    3.449187] ocelot-ext-switch ocelot-ext-switch.4.auto: DMA mask not set
> > [    5.336880] ocelot-ext-switch ocelot-ext-switch.4.auto: PHY [ocelot-miim0.2.auto-mii:00] driver [Generic PHY] (irq=POLL)
> > [    5.349087] ocelot-ext-switch ocelot-ext-switch.4.auto: configuring for phy/internal link mode
> > [    5.363619] ocelot-ext-switch ocelot-ext-switch.4.auto swp1 (uninitialized): PHY [ocelot-miim0.2.auto-mii:01] driver [Generic PHY] (irq=POLL)
> > [    5.381396] ocelot-ext-switch ocelot-ext-switch.4.auto swp2 (uninitialized): PHY [ocelot-miim0.2.auto-mii:02] driver [Generic PHY] (irq=POLL)
> > [    5.398525] ocelot-ext-switch ocelot-ext-switch.4.auto swp3 (uninitialized): PHY [ocelot-miim0.2.auto-mii:03] driver [Generic PHY] (irq=POLL)
> 
> Do the PHYs not have a specific driver?

When I have the other four ports defined, those correctly find the
vsc85xx driver, perform serdes calibration, etc. (assuming I have that
phy support compiled in) The internal phys I believe have always just
been using a generic driver.

> 
> > [    5.422048] device eth0 entered promiscuous mode
> > [    5.426785] DSA: tree 0 setup
> > ...
> > [    7.450067] ocelot-ext-switch ocelot-ext-switch.4.auto: Link is Up - 100Mbps/Full - flow control off
> > [   21.556395] cpsw-switch 4a100000.switch: starting ndev. mode: dual_mac
> > [   21.648564] SMSC LAN8710/LAN8720 4a101000.mdio:00: attached PHY driver (mii_bus:phy_addr=4a101000.mdio:00, irq=POLL)
> > [   21.667970] 8021q: adding VLAN 0 to HW filter on device eth0
> > [   21.705360] ocelot-ext-switch ocelot-ext-switch.4.auto swp1: configuring for phy/internal link mode
> > [   22.018230] ocelot-ext-switch ocelot-ext-switch.4.auto: Link is Down
> > [   23.771740] cpsw-switch 4a100000.switch eth0: Link is Up - 100Mbps/Full - flow control off
> > [   24.090929] ocelot-ext-switch ocelot-ext-switch.4.auto: Link is Up - 100Mbps/Full - flow control off
> > [   25.853021] ocelot-ext-switch ocelot-ext-switch.4.auto swp1: Link is Up - 1Gbps/Full - flow control rx/tx
> > 
> > 
> > RFC history:
> > v1 (accidentally named vN)
> > 	* Initial architecture. Not functional
> > 	* General concepts laid out
> > 
> > v2
> > 	* Near functional. No CPU port communication, but control over all
> > 	external ports
> > 	* Cleaned up regmap implementation from v1
> > 
> > v3
> > 	* Functional
> > 	* Shared MDIO transactions routed through mdio-mscc-miim
> > 	* CPU / NPI port enabled by way of vsc7512_enable_npi_port /
> > 	felix->info->enable_npi_port
> > 	* NPI port tagging functional - Requires a CPU port driver that supports
> > 	frames of 1520 bytes. Verified with a patch to the cpsw driver
> > 
> > v4
> >     * Functional
> >     * Device tree fixes
> >     * Add hooks for pinctrl-ocelot - some functionality by way of sysfs
> >     * Add hooks for pinctrl-microsemi-sgpio - not yet fully functional
> >     * Remove lynx_pcs interface for a generic phylink_pcs. The goal here
> >     is to have an ocelot_pcs that will work for each configuration of
> >     every port.
> > 
> > v5
> >     * Restructured to MFD
> >     * Several commits were split out, submitted, and accepted
> >     * pinctrl-ocelot believed to be fully functional (requires commits
> >     from the linux-pinctrl tree)
> >     * External MDIO bus believed to be fully functional
> > 
> > v6
> >     * Applied several suggestions from the last RFC from Lee Jones. I
> >       hope I didn't miss anything.
> >     * Clean up MFD core - SPI interaction. They no longer use callbacks.
> >     * regmaps get registered to the child device, and don't attempt to
> >       get shared. It seems if a regmap is to be shared, that should be
> >       solved with syscon, not dev or mfd.
> > 
> > v7
> >     * Applied as much as I could from Lee and Vladimir's suggestions. As
> >       always, the feedback is greatly appreciated!
> >     * Remove "ocelot_spi" container complication
> >     * Move internal MDIO bus from ocelot_ext to MFD, with a devicetree
> >       change to match
> >     * Add initial HSIO support
> >     * Switch to IORESOURCE_REG for resource definitions
> > 
> > v8
> >     * Applied another round of suggestions from Lee and Vladimir
> >     * Utilize regmap bus reads, which speeds bulk transfers up by an
> 
> bus -> bulk?

Either is probably valid. Here I'm referencing struct regmap_bus, 
so _regmap_bus_read allows the utilization of bulk transfers for stats.

> 
> >       order of magnitude
> >     * Add two additional patches to utilize phylink_generic_validate
> >     * Changed GPL V2 to GPL in licenses where applicable (checkpatch)
> >     * Remove initial hsio/serdes changes from the RFC
> > 
> > 
> > Colin Foster (16):
> >   pinctrl: ocelot: allow pinctrl-ocelot to be loaded as a module
> >   pinctrl: microchip-sgpio: allow sgpio driver to be used as a module
> >   net: ocelot: add interface to get regmaps when exernally controlled
> >   net: mdio: mscc-miim: add ability to be used in a non-mmio
> >     configuration
> >   pinctrl: ocelot: add ability to be used in a non-mmio configuration
> >   pinctrl: microchip-sgpio: add ability to be used in a non-mmio
> >     configuration
> >   resource: add define macro for register address resources
> >   mfd: ocelot: add support for the vsc7512 chip via spi
> >   net: mscc: ocelot: expose ocelot wm functions
> >   net: dsa: felix: add configurable device quirks
> >   net: mscc: ocelot: expose regfield definition to be used by other
> >     drivers
> >   net: mscc: ocelot: expose stats layout definition to be used by other
> >     drivers
> >   net: mscc: ocelot: expose vcap_props structure
> >   net: dsa: ocelot: add external ocelot switch control
> >   net: dsa: felix: add phylink_get_caps capability
> >   net: dsa: ocelot: utilize phylink_generic_validate
> > 
> >  drivers/mfd/Kconfig                        |  18 +
> >  drivers/mfd/Makefile                       |   2 +
> >  drivers/mfd/ocelot-core.c                  | 138 ++++++++
> >  drivers/mfd/ocelot-spi.c                   | 311 +++++++++++++++++
> >  drivers/mfd/ocelot.h                       |  34 ++
> >  drivers/net/dsa/ocelot/Kconfig             |  14 +
> >  drivers/net/dsa/ocelot/Makefile            |   5 +
> >  drivers/net/dsa/ocelot/felix.c             |  29 +-
> >  drivers/net/dsa/ocelot/felix.h             |   3 +
> >  drivers/net/dsa/ocelot/felix_vsc9959.c     |   1 +
> >  drivers/net/dsa/ocelot/ocelot_ext.c        | 366 +++++++++++++++++++++
> >  drivers/net/dsa/ocelot/seville_vsc9953.c   |   1 +
> >  drivers/net/ethernet/mscc/ocelot_devlink.c |  31 ++
> >  drivers/net/ethernet/mscc/ocelot_vsc7514.c | 230 +------------
> >  drivers/net/ethernet/mscc/vsc7514_regs.c   | 200 +++++++++++
> >  drivers/net/mdio/mdio-mscc-miim.c          |  31 +-
> >  drivers/pinctrl/Kconfig                    |   4 +-
> >  drivers/pinctrl/pinctrl-microchip-sgpio.c  |  26 +-
> >  drivers/pinctrl/pinctrl-ocelot.c           |  35 +-
> >  include/linux/ioport.h                     |   5 +
> >  include/soc/mscc/ocelot.h                  |  19 ++
> >  include/soc/mscc/vsc7514_regs.h            |   6 +
> >  22 files changed, 1251 insertions(+), 258 deletions(-)
> >  create mode 100644 drivers/mfd/ocelot-core.c
> >  create mode 100644 drivers/mfd/ocelot-spi.c
> >  create mode 100644 drivers/mfd/ocelot.h
> >  create mode 100644 drivers/net/dsa/ocelot/ocelot_ext.c
> > 
> > -- 
> > 2.25.1
> >

Powered by blists - more mailing lists