| Message ID | 20200319135952.16258-2-tobias@waldekranz.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v2,1/2] dt-bindings: net: add marvell smi2usb bindings | expand |
On Thu, Mar 19, 2020 at 02:59:52PM +0100, Tobias Waldekranz wrote: > An MDIO controller present on development boards for Marvell switches > from the Link Street (88E6xxx) family. > > Using this module, you can use the following setup as a development > platform for switchdev and DSA related work. > > .-------. .-----------------. > | USB----USB | > | SoC | | 88E6390X-DB ETH1-10 > | ETH----ETH0 | > '-------' '-----------------' > > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > --- > > v1->v2: > - Reverse christmas tree ordering of local variables. > > --- > MAINTAINERS | 1 + > drivers/net/phy/Kconfig | 7 ++ > drivers/net/phy/Makefile | 1 + > drivers/net/phy/mdio-smi2usb.c | 137 +++++++++++++++++++++++++++++++++ > 4 files changed, 146 insertions(+) > create mode 100644 drivers/net/phy/mdio-smi2usb.c Hi Tobias Where does the name mii2usb come from? To me, it seems to be the wrong way around, it is USB to MII. I suppose the Marvell Switch team could of given it this name, for them the switch is the centre of their world, and things connect to it? I'm just wondering if we should actually ignore Marvell and call it usb2mii? I also think there should be a marvell prefix in the name, since were could be other implementations of USB/MII. mvusb2mii? Do you know how this is implemented? Is it a product you can purchase? Or a microcontroller on the board which implements this? It would be an interesting product, especially on x86 machines which generally end up doing bit-banging because of the lack of drivers using kernel MDIO. > +static int smi2usb_probe(struct usb_interface *interface, > + const struct usb_device_id *id) > +{ > + struct device *dev = &interface->dev; > + struct mii_bus *mdio; > + struct smi2usb *smi; > + int err = -ENOMEM; > + > + mdio = devm_mdiobus_alloc_size(dev, sizeof(*smi)); > + if (!mdio) > + goto err; > + ... > +static void smi2usb_disconnect(struct usb_interface *interface) > +{ > + struct smi2usb *smi; > + > + smi = usb_get_intfdata(interface); > + mdiobus_unregister(smi->mdio); > + usb_set_intfdata(interface, NULL); > + > + usb_put_intf(interface); > + usb_put_dev(interface_to_usbdev(interface)); > +} I don't know enough about USB. Does disconnect have the same semantics remove()? You used devm_mdiobus_alloc_size() to allocate the bus structure. Will it get freed after disconnect? I've had USB devices connected via flaky USB hubs and they have repeatedly disappeared and reappeared. I wonder if in that case you are leaking memory if disconnect does not release the memory? > + usb_put_intf(interface); > + usb_put_dev(interface_to_usbdev(interface)); > +} Another USB novice question. Is this safe? Could the put of interface cause it to be destroyed? Then interface_to_usbdev() is called on invalid memory? Maybe this should be cross posted to a USB mailing list, so we can get the USB aspects reviewed. The MDIO bits seem good to me. Andrew
On Thu, Mar 19, 2020 at 04:49:37PM +0100, Andrew Lunn wrote: > On Thu, Mar 19, 2020 at 02:59:52PM +0100, Tobias Waldekranz wrote: > > An MDIO controller present on development boards for Marvell switches > > from the Link Street (88E6xxx) family. > > > > Using this module, you can use the following setup as a development > > platform for switchdev and DSA related work. > > > > .-------. .-----------------. > > | USB----USB | > > | SoC | | 88E6390X-DB ETH1-10 > > | ETH----ETH0 | > > '-------' '-----------------' > > > > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > > --- > > > > v1->v2: > > - Reverse christmas tree ordering of local variables. > > > > --- > > MAINTAINERS | 1 + > > drivers/net/phy/Kconfig | 7 ++ > > drivers/net/phy/Makefile | 1 + > > drivers/net/phy/mdio-smi2usb.c | 137 +++++++++++++++++++++++++++++++++ > > 4 files changed, 146 insertions(+) > > create mode 100644 drivers/net/phy/mdio-smi2usb.c > > Hi Tobias > > Where does the name mii2usb come from? To me, it seems to be the wrong > way around, it is USB to MII. I suppose the Marvell Switch team could > of given it this name, for them the switch is the centre of their > world, and things connect to it? The name is indeed coming from Marvell. They use the term SMI over MDIO in most of their software and documentation. I had the same reaction to the name regarding the ordering of the terms, but felt it was best to go with the vendor's choice. > I'm just wondering if we should actually ignore Marvell and call it > usb2mii? > > I also think there should be a marvell prefix in the name, since were > could be other implementations of USB/MII. mvusb2mii? You're absolutely right that there should be an mv prefix in there. Calling it usb2mii seems like a misnomer though. At least for me, MII relates more to the data interface between a MAC and a PHY, whereas MDIO or SMI refers to the control interface (MDC/MDIO). How about just mdio-mvusb? > Do you know how this is implemented? Is it a product you can purchase? > Or a microcontroller on the board which implements this? It would be > an interesting product, especially on x86 machines which generally end > up doing bit-banging because of the lack of drivers using kernel MDIO. On the 88E6390X-DB, I know that there is a chip by the USB port that is probably either an MCU or a small FPGA. I can have a closer look at it when I'm at the office tomorrow if you'd like. I also remember seeing some docs from Marvell which seemed to indicate that they have a standalone product providing only the USB-to-MDIO functionality. The x86 use-case is interesting. It would be even more so if there was some way of loading a DSA DT fragment so that you could hook it up to your machine's Ethernet port. > > +static int smi2usb_probe(struct usb_interface *interface, > > + const struct usb_device_id *id) > > +{ > > + struct device *dev = &interface->dev; > > + struct mii_bus *mdio; > > + struct smi2usb *smi; > > + int err = -ENOMEM; > > + > > + mdio = devm_mdiobus_alloc_size(dev, sizeof(*smi)); > > + if (!mdio) > > + goto err; > > + > > ... > > > > +static void smi2usb_disconnect(struct usb_interface *interface) > > +{ > > + struct smi2usb *smi; > > + > > + smi = usb_get_intfdata(interface); > > + mdiobus_unregister(smi->mdio); > > + usb_set_intfdata(interface, NULL); > > + > > + usb_put_intf(interface); > > + usb_put_dev(interface_to_usbdev(interface)); > > +} > > I don't know enough about USB. Does disconnect have the same semantics > remove()? You used devm_mdiobus_alloc_size() to allocate the bus > structure. Will it get freed after disconnect? I've had USB devices > connected via flaky USB hubs and they have repeatedly disappeared and > reappeared. I wonder if in that case you are leaking memory if > disconnect does not release the memory? Disclaimer: This is my first ever USB driver. I assumed that since we're removing 'interface', 'interface->dev' will be removed as well and thus calling all devm hooks. > > + usb_put_intf(interface); > > + usb_put_dev(interface_to_usbdev(interface)); > > +} > > Another USB novice question. Is this safe? Could the put of interface > cause it to be destroyed? Then interface_to_usbdev() is called on > invalid memory? That does indeed look scary. I inverted the order of the calls to the _get_ functions, which I got from the USB skeleton driver. I'll try to review some other drivers to see if I can figure this out. > Maybe this should be cross posted to a USB mailing list, so we can get > the USB aspects reviewed. The MDIO bits seem good to me. Good idea. Any chance you can help an LKML rookie out? How does one go about that? Do I simply reply to this thread and add the USB list, or do I post the patches again as a new series? Any special tags? Is there any documentation available? > Andrew
Hi Tobias > How about just mdio-mvusb? Yes, i like that. > On the 88E6390X-DB, I know that there is a chip by the USB port that > is probably either an MCU or a small FPGA. I can have a closer look at > it when I'm at the office tomorrow if you'd like. I also remember > seeing some docs from Marvell which seemed to indicate that they have > a standalone product providing only the USB-to-MDIO functionality. I would be interested in knowing more. > The x86 use-case is interesting. It would be even more so if there was > some way of loading a DSA DT fragment so that you could hook it up to > your machine's Ethernet port. We don't have that at the moment. But so long as you only need internal copper PHYs, it is possible to use a platform device and it all just works. > > > +static int smi2usb_probe(struct usb_interface *interface, > > > + const struct usb_device_id *id) > > > +{ > > > + struct device *dev = &interface->dev; > > > + struct mii_bus *mdio; > > > + struct smi2usb *smi; > > > + int err = -ENOMEM; > > > + > > > + mdio = devm_mdiobus_alloc_size(dev, sizeof(*smi)); > > > + if (!mdio) > > > + goto err; > > > + > > > > ... > > > > > > > +static void smi2usb_disconnect(struct usb_interface *interface) > > > +{ > > > + struct smi2usb *smi; > > > + > > > + smi = usb_get_intfdata(interface); > > > + mdiobus_unregister(smi->mdio); > > > + usb_set_intfdata(interface, NULL); > > > + > > > + usb_put_intf(interface); > > > + usb_put_dev(interface_to_usbdev(interface)); > > > +} > > > > I don't know enough about USB. Does disconnect have the same semantics > > remove()? You used devm_mdiobus_alloc_size() to allocate the bus > > structure. Will it get freed after disconnect? I've had USB devices > > connected via flaky USB hubs and they have repeatedly disappeared and > > reappeared. I wonder if in that case you are leaking memory if > > disconnect does not release the memory? > > Disclaimer: This is my first ever USB driver. And i've only ever written one which has been merged. > I assumed that since we're removing 'interface', 'interface->dev' will > be removed as well and thus calling all devm hooks. > > > > + usb_put_intf(interface); > > > + usb_put_dev(interface_to_usbdev(interface)); > > > +} > > > > Another USB novice question. Is this safe? Could the put of interface > > cause it to be destroyed? Then interface_to_usbdev() is called on > > invalid memory? > > That does indeed look scary. I inverted the order of the calls to the > _get_ functions, which I got from the USB skeleton driver. I'll try to > review some other drivers to see if I can figure this out. > > > Maybe this should be cross posted to a USB mailing list, so we can get > > the USB aspects reviewed. The MDIO bits seem good to me. > > Good idea. Any chance you can help an LKML rookie out? How does one go > about that? Do I simply reply to this thread and add the USB list, or > do I post the patches again as a new series? Any special tags? Is > there any documentation available? I would fixup the naming and repost. You can put whatever comments you want under the --- marker. So say this driver should be merged via netdev, but you would appreciate reviews of the USB parts from USB maintainers. linux-usb@vger.kernel.org would be the correct list to add. Andrew
Hi Andrew, > > How about just mdio-mvusb? > > > Yes, i like that. ACK. > > On the 88E6390X-DB, I know that there is a chip by the USB port that > > is probably either an MCU or a small FPGA. I can have a closer look at > > it when I'm at the office tomorrow if you'd like. I also remember > > seeing some docs from Marvell which seemed to indicate that they have > > a standalone product providing only the USB-to-MDIO functionality. > > > I would be interested in knowing more. It seems like they are using the Cypress FX2 controller (CY7C68013). I've used it before on USB device projects. If I remember correctly it has an 8052 core, a USB2 controller and some low-speed I/O blocks. Couldn't locate the slide deck about a standalone device unfortunately. > I would fixup the naming and repost. You can put whatever comments you > want under the --- marker. So say this driver should be merged via > netdev, but you would appreciate reviews of the USB parts from USB > maintainers. linux-usb@vger.kernel.org would be the correct list to > add. Great. Just to make sure I've understood: I'll send v3 with _both_ netdev and linux-usb in "To:"? Thanks, wkz
On Fri, Mar 20, 2020 at 03:44:34PM +0100, Tobias Waldekranz wrote: > Hi Andrew, > > > > How about just mdio-mvusb? > > > > > > Yes, i like that. > > ACK. > > > > On the 88E6390X-DB, I know that there is a chip by the USB port that > > > is probably either an MCU or a small FPGA. I can have a closer look at > > > it when I'm at the office tomorrow if you'd like. I also remember > > > seeing some docs from Marvell which seemed to indicate that they have > > > a standalone product providing only the USB-to-MDIO functionality. > > > > > > I would be interested in knowing more. > > It seems like they are using the Cypress FX2 controller > (CY7C68013). I've used it before on USB device projects. If I remember > correctly it has an 8052 core, a USB2 controller and some low-speed > I/O blocks. Couldn't locate the slide deck about a standalone device > unfortunately. > > > I would fixup the naming and repost. You can put whatever comments you > > want under the --- marker. So say this driver should be merged via > > netdev, but you would appreciate reviews of the USB parts from USB > > maintainers. linux-usb@vger.kernel.org would be the correct list to > > add. > > Great. Just to make sure I've understood: I'll send v3 with _both_ > netdev and linux-usb in "To:"? Hi Tobias I normally use To: for the maintainer i expect to merge the patch, i.e. DaveM, and Cc: for lists and other maintainers who should review it. Andrew
diff --git a/MAINTAINERS b/MAINTAINERS index 83bb7ce3e23e..a7771e577832 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -10100,6 +10100,7 @@ MARVELL SMI2USB MDIO CONTROLLER DRIVER M: Tobias Waldekranz <tobias@waldekranz.com> L: netdev@vger.kernel.org S: Maintained +F: drivers/net/phy/mdio-smi2usb.c F: Documentation/devicetree/bindings/net/marvell,smi2usb.yaml MARVELL SOC MMC/SD/SDIO CONTROLLER DRIVER diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index cc7f1df855da..ddde79c6f354 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -189,6 +189,13 @@ config MDIO_OCTEON buses. It is required by the Octeon and ThunderX ethernet device drivers on some systems. +config MDIO_SMI2USB + tristate "Marvell SMI2USB" + depends on OF_MDIO && USB + help + A USB to MDIO converter present on development boards for + Marvell's Link Street family of Ethernet switches. + config MDIO_SUN4I tristate "Allwinner sun4i MDIO interface support" depends on ARCH_SUNXI || COMPILE_TEST diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index 70774ab474e6..fcbe4bd26747 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -41,6 +41,7 @@ obj-$(CONFIG_MDIO_IPQ8064) += mdio-ipq8064.o obj-$(CONFIG_MDIO_MOXART) += mdio-moxart.o obj-$(CONFIG_MDIO_MSCC_MIIM) += mdio-mscc-miim.o obj-$(CONFIG_MDIO_OCTEON) += mdio-octeon.o +obj-$(CONFIG_MDIO_SMI2USB) += mdio-smi2usb.o obj-$(CONFIG_MDIO_SUN4I) += mdio-sun4i.o obj-$(CONFIG_MDIO_THUNDER) += mdio-thunder.o obj-$(CONFIG_MDIO_XGENE) += mdio-xgene.o diff --git a/drivers/net/phy/mdio-smi2usb.c b/drivers/net/phy/mdio-smi2usb.c new file mode 100644 index 000000000000..c4f7f555a091 --- /dev/null +++ b/drivers/net/phy/mdio-smi2usb.c @@ -0,0 +1,137 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of_mdio.h> +#include <linux/phy.h> +#include <linux/usb.h> + +#define USB_MARVELL_VID 0x1286 + +static const struct usb_device_id smi2usb_table[] = { + { USB_DEVICE(USB_MARVELL_VID, 0x1fa4) }, + + {} +}; +MODULE_DEVICE_TABLE(usb, smi2usb_table); + +enum { + SMI_CMD_PREAMBLE0, + SMI_CMD_PREAMBLE1, + SMI_CMD_ADDR, + SMI_CMD_VAL, +}; + +struct smi2usb { + struct usb_device *udev; + struct usb_interface *intf; + + struct mii_bus *mdio; + + __le16 buf[4]; +}; + +static int smi2usb_mdio_read(struct mii_bus *mdio, int dev, int reg) +{ + struct smi2usb *smi = mdio->priv; + int err, alen; + + if (dev & MII_ADDR_C45) + return -EOPNOTSUPP; + + smi->buf[SMI_CMD_ADDR] = cpu_to_le16(0xa400 | (dev << 5) | reg); + + err = usb_bulk_msg(smi->udev, usb_sndbulkpipe(smi->udev, 2), + smi->buf, 6, &alen, 100); + if (err) + return err; + + err = usb_bulk_msg(smi->udev, usb_rcvbulkpipe(smi->udev, 6), + &smi->buf[SMI_CMD_VAL], 2, &alen, 100); + if (err) + return err; + + return le16_to_cpu(smi->buf[SMI_CMD_VAL]); +} + +static int smi2usb_mdio_write(struct mii_bus *mdio, int dev, int reg, u16 val) +{ + struct smi2usb *smi = mdio->priv; + int alen; + + if (dev & MII_ADDR_C45) + return -EOPNOTSUPP; + + smi->buf[SMI_CMD_ADDR] = cpu_to_le16(0x8000 | (dev << 5) | reg); + smi->buf[SMI_CMD_VAL] = cpu_to_le16(val); + + return usb_bulk_msg(smi->udev, usb_sndbulkpipe(smi->udev, 2), + smi->buf, 8, &alen, 100); +} + +static int smi2usb_probe(struct usb_interface *interface, + const struct usb_device_id *id) +{ + struct device *dev = &interface->dev; + struct mii_bus *mdio; + struct smi2usb *smi; + int err = -ENOMEM; + + mdio = devm_mdiobus_alloc_size(dev, sizeof(*smi)); + if (!mdio) + goto err; + + smi = mdio->priv; + smi->mdio = mdio; + smi->udev = usb_get_dev(interface_to_usbdev(interface)); + smi->intf = usb_get_intf(interface); + + /* Reversed from USB PCAPs, no idea what these mean. */ + smi->buf[SMI_CMD_PREAMBLE0] = cpu_to_le16(0xe800); + smi->buf[SMI_CMD_PREAMBLE1] = cpu_to_le16(0x0001); + + usb_set_intfdata(interface, smi); + + snprintf(mdio->id, MII_BUS_ID_SIZE, "smi2usb-%s", dev_name(dev)); + mdio->name = mdio->id; + mdio->parent = dev; + mdio->read = smi2usb_mdio_read; + mdio->write = smi2usb_mdio_write; + + err = of_mdiobus_register(mdio, dev->of_node); + if (err) + goto err_put; + + return 0; + +err_put: + usb_put_intf(interface); + usb_put_dev(interface_to_usbdev(interface)); +err: + return err; +} + +static void smi2usb_disconnect(struct usb_interface *interface) +{ + struct smi2usb *smi; + + smi = usb_get_intfdata(interface); + mdiobus_unregister(smi->mdio); + usb_set_intfdata(interface, NULL); + + usb_put_intf(interface); + usb_put_dev(interface_to_usbdev(interface)); +} + +static struct usb_driver smi2usb_driver = { + .name = "smi2usb", + .id_table = smi2usb_table, + .probe = smi2usb_probe, + .disconnect = smi2usb_disconnect, +}; + +module_usb_driver(smi2usb_driver); + +MODULE_AUTHOR("Tobias Waldekranz <tobias@waldekranz.com>"); +MODULE_DESCRIPTION("Marvell SMI2USB Adapter"); +MODULE_LICENSE("GPL");
An MDIO controller present on development boards for Marvell switches from the Link Street (88E6xxx) family. Using this module, you can use the following setup as a development platform for switchdev and DSA related work. .-------. .-----------------. | USB----USB | | SoC | | 88E6390X-DB ETH1-10 | ETH----ETH0 | '-------' '-----------------' Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> --- v1->v2: - Reverse christmas tree ordering of local variables. --- MAINTAINERS | 1 + drivers/net/phy/Kconfig | 7 ++ drivers/net/phy/Makefile | 1 + drivers/net/phy/mdio-smi2usb.c | 137 +++++++++++++++++++++++++++++++++ 4 files changed, 146 insertions(+) create mode 100644 drivers/net/phy/mdio-smi2usb.c