| Message ID | 20210814025003.2449143-1-colin.foster@in-advantage.com |
|---|---|
| Headers | show |
| Series | add support for VSC75XX control over SPI | expand |
On Fri, Aug 13, 2021 at 07:49:54PM -0700, Colin Foster wrote: > The pci_bar variables for the switch and imdio don't make sense for the > generic felix driver. Moving them to felix_vsc9959 to limit scope and > simplify the felix_info struct. > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > --- I distinctly remember giving a Reviewed-by tag for this patch in the previous series: https://patchwork.kernel.org/project/netdevbpf/patch/20210710192602.2186370-2-colin.foster@in-advantage.com/ It would be nice if you could carry them along from one series to the next so we don't have to chase you. If you use git b4 when you start working on a new version, the extra tags in the comments are downloaded and appended automatically. That is if you are not ok with manually copy-pasting them into your commit message.
On Fri, Aug 13, 2021 at 07:49:59PM -0700, Colin Foster wrote: > Moving these to a separate file will allow them to be shared to other > drivers. > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > --- What about the VCAP bit fields?
On Fri, Aug 13, 2021 at 07:50:02PM -0700, Colin Foster wrote: > +/* Code taken from ocelot_adjust_link. Since we don't have a phydev, and > + * therefore a phydev->link associated with the NPI port, it needs to be enabled > + * blindly. > + */ This makes no sense. You do have a phylink associated with the NPI port, see for yourself, all of felix_phylink_mac_link_up, felix_phylink_mac_config, felix_phylink_validate get called for the NPI port. The trouble, really, is that what is done in felix_phylink_mac_link_up is not sufficient for your hardware. The felix_vsc9959 and seville_vsc9953 drivers are Microchip switches integrated with NXP PCS, and the NXP PCS has a dedicated driver in drivers/net/pcs/pcs-lynx.c. So you won't see any of the PCS1G writes in the common driver, because NXP integrations of these switches don't have that block. This is not the proper way to do things. You are "fixing" SGMII for the NPI/CPU port by pretending it's an NPI port issue, but in reality all the other ports that use SGMII need the same treatment. What we might need is a dedicated PCS driver for the VSC7512 switch, and a way for the felix driver to interchangeably work with either struct lynx_pcs or struct ocelot_pcs (or whatever it's going to be called). The issue is that the registers for the PCS1G block look nothing like the MDIO clause 22 layout, so anything that tries to map the struct ocelot_pcs over a struct mdio_device is going to look like a horrible shoehorn. For that we might need Russell's assistance. The documentation is at: http://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10489.pdf search for "Information about the registers for this product is available in the attached file." and then open the PDF embedded within the PDF.
On Sat, Aug 14, 2021 at 02:43:29PM +0300, Vladimir Oltean wrote: > The issue is that the registers for the PCS1G block look nothing like > the MDIO clause 22 layout, so anything that tries to map the struct > ocelot_pcs over a struct mdio_device is going to look like a horrible > shoehorn. > > For that we might need Russell's assistance. > > The documentation is at: > http://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10489.pdf > search for "Information about the registers for this product is available in the attached file." > and then open the PDF embedded within the PDF. In fact I do notice now that as long as you don't use any of the optional phylink_mii_c22_pcs_* helpers in your PCS driver, then struct phylink_pcs has pretty much zero dependency on struct mdio_device, which means that I'm wrong and it should be completely within reach to write a dedicated PCS driver for this hardware. As to how to make the common felix.c work with different implementations of struct phylink_pcs, one thing that certainly has to change is that struct felix should hold a struct phylink_pcs **pcs and not a struct lynx_pcs **pcs. Does this mean that we should refactor lynx_pcs_create() to return a struct phylink_pcs * instead of struct lynx_pcs *, and lynx_pcs_destroy() to receive the struct phylink_pcs *, use container_of() and free the larger struct lynx_pcs *? Yes, probably. If you feel uncomfortable with this, I can try to refactor lynx_pcs to make it easier to accomodate a different PCS driver in felix.
On Sat, Aug 14, 2021 at 02:07:05PM +0300, Vladimir Oltean wrote: > On Fri, Aug 13, 2021 at 07:49:54PM -0700, Colin Foster wrote: > > The pci_bar variables for the switch and imdio don't make sense for the > > generic felix driver. Moving them to felix_vsc9959 to limit scope and > > simplify the felix_info struct. > > > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > > --- > > I distinctly remember giving a Reviewed-by tag for this patch in the > previous series: > > https://patchwork.kernel.org/project/netdevbpf/patch/20210710192602.2186370-2-colin.foster@in-advantage.com/ > > It would be nice if you could carry them along from one series to the > next so we don't have to chase you. > > If you use git b4 when you start working on a new version, the extra > tags in the comments are downloaded and appended automatically. That is > if you are not ok with manually copy-pasting them into your commit > message. Yes, you did. I'll do that next time. Forgive me, for this entire process is very much a learning experience for me.
On Sat, Aug 14, 2021 at 03:02:11PM +0300, Vladimir Oltean wrote: > In fact I do notice now that as long as you don't use any of the > optional phylink_mii_c22_pcs_* helpers in your PCS driver, then > struct phylink_pcs has pretty much zero dependency on struct mdio_device, > which means that I'm wrong and it should be completely within reach to > write a dedicated PCS driver for this hardware. Yes, this was one of the design goals when I created phylink_pcs, as I have exactly this situation with my hardware - PCS that do not have a MDIO interface and do not conform to MDIO register layouts. So, I explicitly ensured that phylink_pcs, just like the rest of phylink, is not tied to any particular model of how hardware should look like. Glad to see that this design decision is coming in handy for other people now. :)
On Mon, Aug 16, 2021 at 12:14:54AM +0100, Russell King (Oracle) wrote: > > diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c > > index a84129d18007..d0b3f6be360f 100644 > > --- a/drivers/net/dsa/ocelot/felix_vsc9959.c > > +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c > > @@ -1046,7 +1046,7 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot) > > int rc; > > > > felix->pcs = devm_kcalloc(dev, felix->info->num_ports, > > - sizeof(struct lynx_pcs *), > > + sizeof(struct phylink_pcs *), > > GFP_KERNEL); > > if (!felix->pcs) { > > dev_err(dev, "failed to allocate array for PCS PHYs\n"); > > @@ -1095,8 +1095,8 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot) > > > > for (port = 0; port < felix->info->num_ports; port++) { > > struct ocelot_port *ocelot_port = ocelot->ports[port]; > > + struct phylink_pcs *phylink; > > struct mdio_device *pcs; > > - struct lynx_pcs *lynx; > > Normally, "phylink" is used to refer to the main phylink data > structure, so I'm not too thrilled to see it getting re-used for the > PCS. However, as you have a variable called "pcs" already, I suppose > you don't have much choice. > > That said, it would be nice to have consistent naming through at > least a single file, and you do have "pcs" below to refer to this > same thing. > > Maybe using plpcs or ppcs would suffice? Or maybe use the "long name" > of phylink_pcs ? I noticed this as well. It seems to me like the mdio_device variable name of pcs is misleading, and perhaps should be "mdio" and phylink_pcs should be pcs, or any of the alternatives you suggested. > > > > > if (dsa_is_unused_port(felix->ds, port)) > > continue; > > @@ -1108,13 +1108,13 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot) > > if (IS_ERR(pcs)) > > continue; > > > > - lynx = lynx_pcs_create(pcs); > > + phylink = lynx_pcs_create(pcs); > > if (!lynx) { > > I think you want to change this test. Yes, I caught these shortly after submitting it. Fixed.
On Sun, Aug 15, 2021 at 04:27:53PM -0700, Colin Foster wrote: > On Mon, Aug 16, 2021 at 12:14:54AM +0100, Russell King (Oracle) wrote: > > > diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c > > > index a84129d18007..d0b3f6be360f 100644 > > > --- a/drivers/net/dsa/ocelot/felix_vsc9959.c > > > +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c > > > @@ -1046,7 +1046,7 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot) > > > int rc; > > > > > > felix->pcs = devm_kcalloc(dev, felix->info->num_ports, > > > - sizeof(struct lynx_pcs *), > > > + sizeof(struct phylink_pcs *), > > > GFP_KERNEL); > > > if (!felix->pcs) { > > > dev_err(dev, "failed to allocate array for PCS PHYs\n"); > > > @@ -1095,8 +1095,8 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot) > > > > > > for (port = 0; port < felix->info->num_ports; port++) { > > > struct ocelot_port *ocelot_port = ocelot->ports[port]; > > > + struct phylink_pcs *phylink; > > > struct mdio_device *pcs; > > > - struct lynx_pcs *lynx; > > > > Normally, "phylink" is used to refer to the main phylink data > > structure, so I'm not too thrilled to see it getting re-used for the > > PCS. However, as you have a variable called "pcs" already, I suppose > > you don't have much choice. > > > > That said, it would be nice to have consistent naming through at > > least a single file, and you do have "pcs" below to refer to this > > same thing. > > > > Maybe using plpcs or ppcs would suffice? Or maybe use the "long name" > > of phylink_pcs ? > > I noticed this as well. It seems to me like the mdio_device variable > name of pcs is misleading, and perhaps should be "mdio" and phylink_pcs > should be pcs, or any of the alternatives you suggested. Yes, we could alternatively could use mdiodev for mdio devices, which would free up "pcs" for use with struct phylink_pcs.
On Sun, Aug 15, 2021 at 01:41:49PM -0700, Colin Foster wrote: > I also came across some curious code in Seville where it is callocing a > struct phy_device * array instead of struct lynx_pcs *. I'm not sure if > that's technically a bug or if the thought is "a pointer array is a > pointer array." git blame will show you that it is a harmless leftover of commit 588d05504d2d ("net: dsa: ocelot: use the Lynx PCS helpers in Felix and Seville"). Before that patch, the pcs was a struct phy_device. > @@ -1062,12 +1062,12 @@ static void vsc9953_mdio_bus_free(struct ocelot *ocelot) > int port; > > for (port = 0; port < ocelot->num_phys_ports; port++) { > - struct lynx_pcs *pcs = felix->pcs[port]; > + struct phylink_pcs *pcs = felix->pcs[port]; > > if (!pcs) > continue; > > - mdio_device_free(pcs->mdio); > + mdio_device_free(lynx_pcs_get_mdio(pcs)); Don't really have a better suggestion than lynx_pcs_get_mdio. > lynx_pcs_destroy(pcs); > } > felix_mdio_bus_free(ocelot); > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > index ccaf7e35abeb..484f0d4efefe 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c > @@ -270,10 +270,11 @@ static int dpaa2_pcs_create(struct dpaa2_mac *mac, > > static void dpaa2_pcs_destroy(struct dpaa2_mac *mac) > { > - struct lynx_pcs *pcs = mac->pcs; > + struct phylink_pcs *pcs = mac->pcs; > > if (pcs) { > - struct device *dev = &pcs->mdio->dev; > + struct mdio_device *mdio = lynx_get_mdio_device(pcs); > + struct device *dev = &mdio->dev; > lynx_pcs_destroy(pcs); > put_device(dev); Ideally dpaa2 would call mdio_device_free too, just like the others. > mac->pcs = NULL; > @@ -336,7 +337,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac) > mac->phylink = phylink; > > if (mac->pcs) > - phylink_set_pcs(mac->phylink, &mac->pcs->pcs); > + phylink_set_pcs(mac->phylink, mac->pcs); > > err = phylink_of_phy_connect(mac->phylink, dpmac_node, 0); > if (err) { > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.c b/drivers/net/ethernet/freescale/enetc/enetc_pf.c > index 31274325159a..cc2ca51ac984 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.c > @@ -823,7 +823,7 @@ static int enetc_imdio_create(struct enetc_pf *pf) > { > struct device *dev = &pf->si->pdev->dev; > struct enetc_mdio_priv *mdio_priv; > - struct lynx_pcs *pcs_lynx; > + struct phylink_pcs *pcs_phylink; > struct mdio_device *pcs; Agree with Russell's suggestion to replace "pcs" with "mdiodev" wherever it refers to a struct mdio_device. Likely as a separate patch. > struct mii_bus *bus; > int err; > @@ -341,13 +355,13 @@ struct lynx_pcs *lynx_pcs_create(struct mdio_device *mdio) > lynx_pcs->pcs.ops = &lynx_pcs_phylink_ops; > lynx_pcs->pcs.poll = true; > > - return lynx_pcs; > + return lynx_to_phylink_pcs(lynx_pcs); I would probably write another patch to convert all occurrences of "struct lynx_pcs" variables to the same naming scheme. Currently we have "lynx", "pcs", "lynx_pcs" only within the pcs-lynx.c file itself. "lynx" seems to be the predominant name so all others could be replaced with that too. > } > EXPORT_SYMBOL(lynx_pcs_create); > > -void lynx_pcs_destroy(struct lynx_pcs *pcs) > +void lynx_pcs_destroy(struct phylink_pcs *pcs) > { > - kfree(pcs); > + kfree(phylink_pcs_to_lynx(pcs)); I would perhaps do this in two stages struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs); kfree(lynx); > } > EXPORT_SYMBOL(lynx_pcs_destroy); > > diff --git a/include/linux/pcs-lynx.h b/include/linux/pcs-lynx.h > index a6440d6ebe95..5712cc2ce775 100644 > --- a/include/linux/pcs-lynx.h > +++ b/include/linux/pcs-lynx.h > @@ -9,13 +9,10 @@ > #include <linux/mdio.h> > #include <linux/phylink.h> > > -struct lynx_pcs { > - struct phylink_pcs pcs; > - struct mdio_device *mdio; > -}; Good that this structure is no longer exposed. > +struct mdio_device *lynx_get_mdio_device(struct phylink_pcs *pcs); > > -struct lynx_pcs *lynx_pcs_create(struct mdio_device *mdio); > +struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio); > > -void lynx_pcs_destroy(struct lynx_pcs *pcs); > +void lynx_pcs_destroy(struct phylink_pcs *pcs); We don't want the few phylink_pcs drivers going in different directions, so we should modify pcs-xpcs.c too such that it no longer exposes struct dw_xpcs to the outside world. I think I hid most of that away already, and grepping for "xpcs->" in drivers/net/dsa and drivers/net/ethernet, I only see xpcs->mdiodev and xpcs->pcs being accessed, so converting khat should be a walk in the park. Anyway, I would focus for now on getting the ocelot hardware to work and writing the phylink_pcs driver for that. That is one part where I can't help a lot with.
Add support for configuration and control of the VSC7511, VSC7512, VSC7513, and VSC7514 chips over a SPI interface. The intent is to control these chips from an external CPU. The expectation is to have most of the features of the net/ethernet/mscc/ocelot_vsc7514 driver. 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 Colin Foster (10): net: dsa: ocelot: remove unnecessary pci_bar variables net: mdio: mscc-miim: convert to a regmap implementation net: dsa: ocelot: felix: switch to mdio-mscc-miim driver for indirect mdio access net: dsa: ocelot: felix: Remove requirement for PCS in felix devices net: dsa: ocelot: felix: add interface for custom regmaps net: mscc: ocelot: split register definitions to a separate file net: mscc: ocelot: expose ocelot wm functions net: mscc: ocelot: felix: add ability to enable a CPU / NPI port net: dsa: ocelot: felix: add support for VSC75XX control over SPI docs: devicetree: add documentation for the VSC7512 SPI device .../devicetree/bindings/net/dsa/ocelot.txt | 92 ++ drivers/net/dsa/ocelot/Kconfig | 14 + drivers/net/dsa/ocelot/Makefile | 7 + drivers/net/dsa/ocelot/felix.c | 11 +- drivers/net/dsa/ocelot/felix.h | 5 +- drivers/net/dsa/ocelot/felix_mdio.c | 52 + drivers/net/dsa/ocelot/felix_mdio.h | 12 + drivers/net/dsa/ocelot/felix_vsc9959.c | 11 +- drivers/net/dsa/ocelot/ocelot_vsc7512_spi.c | 1133 +++++++++++++++++ drivers/net/dsa/ocelot/seville_vsc9953.c | 109 +- drivers/net/ethernet/mscc/Makefile | 2 + drivers/net/ethernet/mscc/ocelot.c | 8 + drivers/net/ethernet/mscc/ocelot_regs.c | 309 +++++ drivers/net/ethernet/mscc/ocelot_vsc7514.c | 323 +---- drivers/net/ethernet/mscc/ocelot_wm.c | 39 + drivers/net/mdio/mdio-mscc-miim.c | 137 +- include/linux/mdio/mdio-mscc-miim.h | 19 + include/soc/mscc/ocelot.h | 24 + include/soc/mscc/ocelot_regs.h | 20 + 19 files changed, 1857 insertions(+), 470 deletions(-) create mode 100644 drivers/net/dsa/ocelot/felix_mdio.c create mode 100644 drivers/net/dsa/ocelot/felix_mdio.h create mode 100644 drivers/net/dsa/ocelot/ocelot_vsc7512_spi.c create mode 100644 drivers/net/ethernet/mscc/ocelot_regs.c create mode 100644 drivers/net/ethernet/mscc/ocelot_wm.c create mode 100644 include/linux/mdio/mdio-mscc-miim.h create mode 100644 include/soc/mscc/ocelot_regs.h -- 2.25.1