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

Vladimir Oltean
2022-03-08T14:40:49Z

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.