Re: [RFC v7 net-next 00/13] add support for VSC7512 control over SPI

[Vladimir Oltean]

On Sun, Mar 06, 2022 at 06:11:55PM -0800, 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.
> 
> 
> Of note: The Felix driver had the ability to register the internal MDIO
> bus. I am no longer using that in the switch driver, it is now an
> additional sub-device under the MFD.
> 
> I also made use of IORESOURCE_REG, which removed the "device_is_mfd"
> requirement.
> 
> 
> / {
> 	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";
> 		};
> [ ... ]
> 		vscled@71 {
> 			label = "port7led1";
> 			gpios = <&sgpio_out1 7 1 GPIO_ACTIVE_LOW>;
> 			default-state = "off";
> 			linux,default-trigger = "ocelot-miim1-mii:07:1Gbps";
> 		};
> 	};
> };
> 
> &spi0 {
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 	status = "okay";
> 
> 	ocelot-chip@0 {
> 		compatible = "mscc,vsc7512_mfd_spi";
> 		spi-max-frequency = <2500000>;
> 		reg = <0>;
> 
> 		ethernet-switch@0 {

I'm not exactly clear on what exactly does the bus address (@0)
represent here and in other (but not all) sub-nodes.
dtc probably warns that there shouldn't be any unit address, since
#address-cells and #size-cells are both 0 for ocelot-chip@0.

> 			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 {
> 			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 {
> 			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 {
> 			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>;
> 			};
> 		};
> 
> 		hsio: syscon {
> 			compatible = "mscc,ocelot-hsio", "syscon", "simple-mfd";
> 
> 			serdes: serdes {
> 				compatible = "mscc,vsc7514-serdes";
> 				#phy-cells = <2>;
> 			};
> 		};
> 	};
> };

The switch-related portion of this patch set looks good enough to me.
I'll let somebody else with more knowledge provide feedback on the
mfd/pinctrl/gpio/phylink/led integration aspects.