From: Colin Foster <colin.foster@in-advantage.com>
To: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: Andrew Lunn <andrew@lunn.ch>, Rob Herring <robh+dt@kernel.org>,
Claudiu Manoil <claudiu.manoil@nxp.com>,
Alexandre Belloni <alexandre.belloni@bootlin.com>,
"supporter:OCELOT ETHERNET SWITCH DRIVER"
<UNGLinuxDriver@microchip.com>,
Vivien Didelot <vivien.didelot@gmail.com>,
Florian Fainelli <f.fainelli@gmail.com>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
Russell King <linux@armlinux.org.uk>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@vger.kernel.org>,
open list <linux-kernel@vger.kernel.org>,
"open list:OCELOT ETHERNET SWITCH DRIVER"
<netdev@vger.kernel.org>
Subject: Re: [RFC PATCH vN net-next 2/2] net: mscc: ocelot: add support for VSC75XX SPI control
Date: Wed, 5 May 2021 05:35:32 -0700 [thread overview]
Message-ID: <20210505123532.GA1738454@euler> (raw)
In-Reply-To: <20210504125942.nx5b6j2cy34qyyhm@skbuf>
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@in-advantage.com>
> > > ---
> > > 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.
next prev parent reply other threads:[~2021-05-05 12:35 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-04 5:11 [RFC PATCH vN net-next 1/2] net: mscc: ocelot: add support for non-mmio regmaps Colin Foster
2021-05-04 5:11 ` [RFC PATCH vN net-next 2/2] net: mscc: ocelot: add support for VSC75XX SPI control Colin Foster
2021-05-04 12:31 ` Andrew Lunn
2021-05-04 12:59 ` Vladimir Oltean
2021-05-04 13:36 ` Andrew Lunn
2021-05-04 13:55 ` Alexandre Belloni
2021-05-04 14:36 ` Vladimir Oltean
2021-05-04 15:08 ` Alexandre Belloni
2021-05-05 13:20 ` Colin Foster
2021-05-05 12:35 ` Colin Foster [this message]
2021-05-06 10:22 ` Vladimir Oltean
2021-05-06 18:34 ` Colin Foster
2021-05-07 8:47 ` Vladimir Oltean
2021-05-08 22:26 ` Colin Foster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210505123532.GA1738454@euler \
--to=colin.foster@in-advantage.com \
--cc=UNGLinuxDriver@microchip.com \
--cc=alexandre.belloni@bootlin.com \
--cc=andrew@lunn.ch \
--cc=claudiu.manoil@nxp.com \
--cc=davem@davemloft.net \
--cc=devicetree@vger.kernel.org \
--cc=f.fainelli@gmail.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=netdev@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=vivien.didelot@gmail.com \
--cc=vladimir.oltean@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).