Hi Vladimir and Andrew,
On Tue, May 04, 2021 at 12:59:43PM +0000, Vladimir Oltean wrote:
> On Tue, May 04, 2021 at 02:31:34PM +0200, Andrew Lunn wrote:
> > On Mon, May 03, 2021 at 10:11:27PM -0700, Colin Foster wrote:
> > > Add support for control for VSC75XX chips over SPI control. Starting with the
> > > VSC9959 code, this will utilize a spi bus instead of PCIe or memory-mapped IO to
> > > control the chip.
> >
> > Hi Colin
> >
> > Please fix your subject line for the next version. vN should of been
> > v1. The number is important so we can tell revisions apart.
>
> Yes, it was my indication to use --subject-prefix="[PATCH vN net-next]",
> I was expecting Colin to replace N with 1, 2, 3 etc but I didn't make
> that clear enough :)
>
Ha. Yes, I suppose I took that too literally. I'll fix it in vO :)
> > >
> > > Signed-off-by: Colin Foster <colin.foster@xxxxxxxxxxxxxxxx>
> > > ---
> > > arch/arm/boot/dts/rpi-vsc7512-spi-overlay.dts | 124 ++
> > > drivers/net/dsa/ocelot/Kconfig | 11 +
> > > drivers/net/dsa/ocelot/Makefile | 5 +
> > > drivers/net/dsa/ocelot/felix_vsc7512_spi.c | 1214 +++++++++++++++++
> > > include/soc/mscc/ocelot.h | 15 +
> >
> > Please split this patch up. The DT overlay will probably be merged via
> > ARM SOC, not netdev. You also need to document the device tree
> > binding, as a separate patch.
I will take this out of the patch, though the feedback is helpful. I
suspect that the end result will be an example in
Documentation/devicetree/bindings/net/dsa/ocelot.txt because there isn't
any commercial hardware available with this functionality, as far as I
know. (If there is I'd love to get my hands on it!)
> >
> > > + fragment@3 {
> > > + target = <&spi0>;
> > > + __overlay__ {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > + cs-gpios = <&gpio 8 1>;
> > > + status = "okay";
> > > +
> > > + vsc7512: vsc7512@0{
> > > + compatible = "mscc,vsc7512";
> > > + spi-max-frequency = <250000>;
> > > + reg = <0>;
> > > +
> > > + ports {
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + port@0 {
> > > + reg = <0>;
> > > + ethernet = <ðernet>;
> > > + phy-mode = "internal";
>
> Additionally, being a completely off-chip switch, are you sure that the
> phy-mode is "internal"?
No, I'm not sure. I don't remember my justification but I had come
across something that made me believe that there needed to be at least
one "internal phy-mode" for DSA to work. This might actually make sense,
however, since it would be the port internal to the on-chip processor.
My hope was that I would've been able to test this with actual hardware
a couple weeks ago and see everything in action. Unfortunately there
seems to be a hardware issue on my setup I'll need EE support to
troubleshoot.
When the hardware is finally communicating, I plan to do this type of
functional verification. I've been in charge of writing the interface
layer of this chip family in the past, but I have coworkers who are
familiar with the actual operation who's advice I'll seek.
>
> > > + fixed-link {
> > > + speed = <1000>;
> > > + full-duplex;
> > > + };
> > > + };
> > > +
> > > + port@1 {
> > > + reg = <1>;
> > > + label = "swp1";
> > > + status = "disabled";
> > > + };
> > > +
> > > + port@2 {
> > > + reg = <2>;
> > > + label = "swp2";
> > > + status = "disabled";
> > > + };
> > > +static void vsc7512_phylink_validate(struct ocelot *ocelot, int port,
> > > + unsigned long *supported,
> > > + struct phylink_link_state *state)
> > > +{
> > > + struct ocelot_port *ocelot_port = ocelot->ports[port];
> > > + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = {
> > > + 0,
> > > + };
> >
> > This function seems out of place. Why would SPI access change what the
> > ports are capable of doing? Please split this up into more
> > patches. Keep the focus of this patch as being adding SPI support.
>
> What is going on is that this is just the way in which the drivers are
> structured. Colin is not really "adding SPI support" to any of the
> existing DSA switches that are supported (VSC9953, VSC9959) as much as
> "adding support for a new switch which happens to be controlled over
> SPI" (VSC7512).
> The layering is as follows:
> - drivers/net/dsa/ocelot/felix_vsc7512_spi.c: deals with the most
> hardware specific SoC support. The regmap is defined here, so are the
> port capabilities.
> - drivers/net/dsa/ocelot/felix.c: common integration with DSA
> - drivers/net/ethernet/mscc/ocelot*.c: the SoC-independent hardware
> support.
>
> I'm not actually sure that splitting the port PHY mode support in a
> separate patch is possible while keeping functional intermediate
> results. But I do agree about the rest, splitting the device tree
> changes, etc.
