[PATCH v3 1/2] dt-bindings: net: add marvell usb to mdio bindings

[PATCH v3 1/2] dt-bindings: net: add marvell usb to mdio bindings

[Tobias Waldekranz]

Describe how the USB to MDIO controller can optionally use device tree
bindings to reference attached devices such as switches.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 .../bindings/net/marvell,mvusb.yaml           | 65 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/marvell,mvusb.yaml

diff --git a/Documentation/devicetree/bindings/net/marvell,mvusb.yaml b/Documentation/devicetree/bindings/net/marvell,mvusb.yaml
new file mode 100644
index 000000000000..9458f6659be1
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/marvell,mvusb.yaml
@@ -0,0 +1,65 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/marvell,mvusb.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Marvell USB to MDIO Controller
+
+maintainers:
+  - Tobias Waldekranz <tobias@waldekranz.com>
+
+description: |+
+  This controller is mounted on development boards for Marvell's Link Street
+  family of Ethernet switches. It allows you to configure the switch's registers
+  using the standard MDIO interface.
+
+  Since the device is connected over USB, there is no strict requirement of
+  having a device tree representation of the device. But in order to use it with
+  the mv88e6xxx driver, you need a device tree node in which to place the switch
+  definition.
+
+allOf:
+  - $ref: "mdio.yaml#"
+
+properties:
+  compatible:
+    const: usb1286,1fa4
+  reg:
+    maxItems: 1
+    description: The USB port number on the host controller
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+
+examples:
+  - |
+    /* USB host controller */
+    &usb1 {
+            mvusb: mdio@1 {
+                    compatible = "usb1286,1fa4";
+                    reg = <1>;
+                    #address-cells = <1>;
+                    #size-cells = <0>;
+            };
+    };
+
+    /* MV88E6390X devboard */
+    &mvusb {
+            switch@0 {
+                    compatible = "marvell,mv88e6190";
+                    status = "ok";
+                    reg = <0x0>;
+
+                    ports {
+                            /* Port definitions */
+                    };
+
+                    mdio {
+                            /* PHY definitions */
+                    };
+            };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 97dce264bc7c..ff35669f8712 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10101,6 +10101,12 @@ M:	Nicolas Pitre <nico@fluxnic.net>
 S:	Odd Fixes
 F:	drivers/mmc/host/mvsdio.*
 
+MARVELL USB MDIO CONTROLLER DRIVER
+M:	Tobias Waldekranz <tobias@waldekranz.com>
+L:	netdev@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/net/marvell,mvusb.yaml
+
 MARVELL XENON MMC/SD/SDIO HOST CONTROLLER DRIVER
 M:	Hu Ziji <huziji@marvell.com>
 L:	linux-mmc@vger.kernel.org
-- 
2.17.1

[PATCH v3 2/2] net: phy: add marvell usb to mdio controller

[Tobias Waldekranz]

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>
---

Hi linux-usb,

This is my first ever USB driver, therefore I would really appreciate
it if someone could have a look at it from a USB perspective before it
is (hopefully) pulled into net-next.

Thanks

v2->v3:
- Rename driver	smi2usb	-> mvusb.
- Clean	up unused USB references.

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-mvusb.c | 120 +++++++++++++++++++++++++++++++++++
 4 files changed, 129 insertions(+)
 create mode 100644 drivers/net/phy/mdio-mvusb.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ff35669f8712..f36023d4ee44 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10105,6 +10105,7 @@ MARVELL USB MDIO CONTROLLER DRIVER
 M:	Tobias Waldekranz <tobias@waldekranz.com>
 L:	netdev@vger.kernel.org
 S:	Maintained
+F:	drivers/net/phy/mdio-mvusb.c
 F:	Documentation/devicetree/bindings/net/marvell,mvusb.yaml
 
 MARVELL XENON MMC/SD/SDIO HOST CONTROLLER DRIVER
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index cc7f1df855da..be9ae8620cd1 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -179,6 +179,13 @@ config MDIO_MSCC_MIIM
 	  This driver supports the MIIM (MDIO) interface found in the network
 	  switches of the Microsemi SoCs
 
+config MDIO_MVUSB
+	tristate "Marvell USB to MDIO Adapter"
+	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_OCTEON
 	tristate "Octeon and some ThunderX SOCs MDIO buses"
 	depends on (64BIT && OF_MDIO) || COMPILE_TEST
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 70774ab474e6..2f5c7093a65b 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_MDIO_I2C)		+= mdio-i2c.o
 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_MVUSB)	+= mdio-mvusb.o
 obj-$(CONFIG_MDIO_OCTEON)	+= mdio-octeon.o
 obj-$(CONFIG_MDIO_SUN4I)	+= mdio-sun4i.o
 obj-$(CONFIG_MDIO_THUNDER)	+= mdio-thunder.o
diff --git a/drivers/net/phy/mdio-mvusb.c b/drivers/net/phy/mdio-mvusb.c
new file mode 100644
index 000000000000..d5eabddfdf51
--- /dev/null
+++ b/drivers/net/phy/mdio-mvusb.c
@@ -0,0 +1,120 @@
+// 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 mvusb_mdio_table[] = {
+	{ USB_DEVICE(USB_MARVELL_VID, 0x1fa4) },
+
+	{}
+};
+MODULE_DEVICE_TABLE(usb, mvusb_mdio_table);
+
+enum {
+	MVUSB_CMD_PREAMBLE0,
+	MVUSB_CMD_PREAMBLE1,
+	MVUSB_CMD_ADDR,
+	MVUSB_CMD_VAL,
+};
+
+struct mvusb_mdio {
+	struct usb_device *udev;
+	struct mii_bus *mdio;
+
+	__le16 buf[4];
+};
+
+static int mvusb_mdio_read(struct mii_bus *mdio, int dev, int reg)
+{
+	struct mvusb_mdio *mvusb = mdio->priv;
+	int err, alen;
+
+	if (dev & MII_ADDR_C45)
+		return -EOPNOTSUPP;
+
+	mvusb->buf[MVUSB_CMD_ADDR] = cpu_to_le16(0xa400 | (dev << 5) | reg);
+
+	err = usb_bulk_msg(mvusb->udev, usb_sndbulkpipe(mvusb->udev, 2),
+			   mvusb->buf, 6, &alen, 100);
+	if (err)
+		return err;
+
+	err = usb_bulk_msg(mvusb->udev, usb_rcvbulkpipe(mvusb->udev, 6),
+			   &mvusb->buf[MVUSB_CMD_VAL], 2, &alen, 100);
+	if (err)
+		return err;
+
+	return le16_to_cpu(mvusb->buf[MVUSB_CMD_VAL]);
+}
+
+static int mvusb_mdio_write(struct mii_bus *mdio, int dev, int reg, u16 val)
+{
+	struct mvusb_mdio *mvusb = mdio->priv;
+	int alen;
+
+	if (dev & MII_ADDR_C45)
+		return -EOPNOTSUPP;
+
+	mvusb->buf[MVUSB_CMD_ADDR] = cpu_to_le16(0x8000 | (dev << 5) | reg);
+	mvusb->buf[MVUSB_CMD_VAL]  = cpu_to_le16(val);
+
+	return usb_bulk_msg(mvusb->udev, usb_sndbulkpipe(mvusb->udev, 2),
+			    mvusb->buf, 8, &alen, 100);
+}
+
+static int mvusb_mdio_probe(struct usb_interface *interface,
+			    const struct usb_device_id *id)
+{
+	struct device *dev = &interface->dev;
+	struct mvusb_mdio *mvusb;
+	struct mii_bus *mdio;
+
+	mdio = devm_mdiobus_alloc_size(dev, sizeof(*mvusb));
+	if (!mdio)
+		return -ENOMEM;
+
+	mvusb = mdio->priv;
+	mvusb->mdio = mdio;
+	mvusb->udev = usb_get_dev(interface_to_usbdev(interface));
+
+	/* Reversed from USB PCAPs, no idea what these mean. */
+	mvusb->buf[MVUSB_CMD_PREAMBLE0] = cpu_to_le16(0xe800);
+	mvusb->buf[MVUSB_CMD_PREAMBLE1] = cpu_to_le16(0x0001);
+
+	snprintf(mdio->id, MII_BUS_ID_SIZE, "mvusb-%s", dev_name(dev));
+	mdio->name = mdio->id;
+	mdio->parent = dev;
+	mdio->read = mvusb_mdio_read;
+	mdio->write = mvusb_mdio_write;
+
+	usb_set_intfdata(interface, mvusb);
+	return of_mdiobus_register(mdio, dev->of_node);
+}
+
+static void mvusb_mdio_disconnect(struct usb_interface *interface)
+{
+	struct mvusb_mdio *mvusb = usb_get_intfdata(interface);
+	struct usb_device *udev = mvusb->udev;
+
+	mdiobus_unregister(mvusb->mdio);
+	usb_set_intfdata(interface, NULL);
+	usb_put_dev(udev);
+}
+
+static struct usb_driver mvusb_mdio_driver = {
+	.name       = "mvusb_mdio",
+	.id_table   = mvusb_mdio_table,
+	.probe      = mvusb_mdio_probe,
+	.disconnect = mvusb_mdio_disconnect,
+};
+
+module_usb_driver(mvusb_mdio_driver);
+
+MODULE_AUTHOR("Tobias Waldekranz <tobias@waldekranz.com>");
+MODULE_DESCRIPTION("Marvell USB MDIO Adapter");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCH v3 2/2] net: phy: add marvell usb to mdio controller

[Greg KH]

On Sat, Mar 21, 2020 at 09:24:43PM +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>
> ---
> 
> Hi linux-usb,
> 
> This is my first ever USB driver, therefore I would really appreciate
> it if someone could have a look at it from a USB perspective before it
> is (hopefully) pulled into net-next.

From a USB point of view, it looks sane, only one question:

> +static int mvusb_mdio_probe(struct usb_interface *interface,
> +			    const struct usb_device_id *id)
> +{
> +	struct device *dev = &interface->dev;
> +	struct mvusb_mdio *mvusb;
> +	struct mii_bus *mdio;
> +
> +	mdio = devm_mdiobus_alloc_size(dev, sizeof(*mvusb));

You allocate a bigger buffer here than the original pointer thinks it is
pointing to?

> +	if (!mdio)
> +		return -ENOMEM;
> +
> +	mvusb = mdio->priv;

And then you set this pointer here?

If that's the way this is supposed to work, that's fine, just feels like
the math is wrong somewhere...

thanks,

greg k-h

Re: [PATCH v3 2/2] net: phy: add marvell usb to mdio controller

[Tobias Waldekranz]

On Sun Mar 22, 2020 at 8:40 AM, Greg KH wrote:
> From a USB point of view, it looks sane, only one question:

Great, thanks for the review.

> > +static int mvusb_mdio_probe(struct usb_interface *interface,
> > +			    const struct usb_device_id *id)
> > +{
> > +	struct device *dev = &interface->dev;
> > +	struct mvusb_mdio *mvusb;
> > +	struct mii_bus *mdio;
> > +
> > +	mdio = devm_mdiobus_alloc_size(dev, sizeof(*mvusb));
>
> 
> You allocate a bigger buffer here than the original pointer thinks it is
> pointing to?

Yes. I've seen this pattern in a couple of places in the kernel,
e.g. alloc_netdev also does this. The object is extended with the
requested size, and the offset is stored somewhere for later use by
the driver.

> > +	if (!mdio)
> > +		return -ENOMEM;
> > +
> > +	mvusb = mdio->priv;
>
> 
> And then you set this pointer here?

...in this case in the priv member.

https://code.woboq.org/linux/linux/drivers/net/phy/mdio_bus.c.html#143

Thanks

Re: [PATCH v3 2/2] net: phy: add marvell usb to mdio controller

[Greg KH]

On Sun, Mar 22, 2020 at 10:09:28AM +0100, Tobias Waldekranz wrote:
> On Sun Mar 22, 2020 at 8:40 AM, Greg KH wrote:
> > From a USB point of view, it looks sane, only one question:
> 
> Great, thanks for the review.
> 
> > > +static int mvusb_mdio_probe(struct usb_interface *interface,
> > > +			    const struct usb_device_id *id)
> > > +{
> > > +	struct device *dev = &interface->dev;
> > > +	struct mvusb_mdio *mvusb;
> > > +	struct mii_bus *mdio;
> > > +
> > > +	mdio = devm_mdiobus_alloc_size(dev, sizeof(*mvusb));
> >
> > 
> > You allocate a bigger buffer here than the original pointer thinks it is
> > pointing to?
> 
> Yes. I've seen this pattern in a couple of places in the kernel,
> e.g. alloc_netdev also does this. The object is extended with the
> requested size, and the offset is stored somewhere for later use by
> the driver.
> 
> > > +	if (!mdio)
> > > +		return -ENOMEM;
> > > +
> > > +	mvusb = mdio->priv;
> >
> > 
> > And then you set this pointer here?
> 
> ...in this case in the priv member.
> 
> https://code.woboq.org/linux/linux/drivers/net/phy/mdio_bus.c.html#143

Ok, just wanted to make sure :)

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v3 2/2] net: phy: add marvell usb to mdio controller

[Andrew Lunn]

On Sat, Mar 21, 2020 at 09:24:43PM +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>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v3 2/2] net: phy: add marvell usb to mdio controller

[Florian Fainelli]

Hi Tobias,

On 3/21/2020 1:24 PM, 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>
> ---

[snip]


> +config MDIO_MVUSB
> +	tristate "Marvell USB to MDIO Adapter"
> +	depends on OF_MDIO && USB

You could drop OF_MDIO here since of_mdiobus_register() does default to
mdiobus_register() when CONFIG_OF is disabled. Other than that, LGTM!

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH v3 1/2] dt-bindings: net: add marvell usb to mdio bindings

[Andrew Lunn]

On Sat, Mar 21, 2020 at 09:24:42PM +0100, Tobias Waldekranz wrote:
> Describe how the USB to MDIO controller can optionally use device tree
> bindings to reference attached devices such as switches.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v3 1/2] dt-bindings: net: add marvell usb to mdio bindings

[Andrew Lunn]

On Sat, Mar 21, 2020 at 09:24:42PM +0100, Tobias Waldekranz wrote:
> Describe how the USB to MDIO controller can optionally use device tree
> bindings to reference attached devices such as switches.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Hi Tobias

A few process things.

The subject line should be "[PATCH net-next] ..." to indicate which
tree this is for.

For a patch set, please always include a cover note. git format-patch
will generate the template and then include text about the patch
series as a whole.

https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt

	Andrew

Re: [PATCH v3 1/2] dt-bindings: net: add marvell usb to mdio bindings

[Tobias Waldekranz]

On Sun Mar 22, 2020 at 2:35 PM, Andrew Lunn wrote:
> The subject line should be "[PATCH net-next] ..." to indicate which
> tree this is for.

Right, my bad.

> For a patch set, please always include a cover note. git format-patch
> will generate the template and then include text about the patch
> series as a whole.

Roger that. Do you want a v4 with these changes, or just things to
keep in mind for the future?

Thank you for your patience

Re: [PATCH v3 1/2] dt-bindings: net: add marvell usb to mdio bindings

[Florian Fainelli]

On 3/21/2020 1:24 PM, Tobias Waldekranz wrote:
> Describe how the USB to MDIO controller can optionally use device tree
> bindings to reference attached devices such as switches.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH v3 1/2] dt-bindings: net: add marvell usb to mdio bindings

[Rob Herring]

On Sat, Mar 21, 2020 at 09:24:42PM +0100, Tobias Waldekranz wrote:
> Describe how the USB to MDIO controller can optionally use device tree
> bindings to reference attached devices such as switches.

Looks like this is in linux-next now. First I'm seeing it because the DT 
list was not Cc'ed.

This is breaking 'make dt_binding_check'. Revert or fix before this goes 
to Linus please.

> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---
>  .../bindings/net/marvell,mvusb.yaml           | 65 +++++++++++++++++++
>  MAINTAINERS                                   |  6 ++
>  2 files changed, 71 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/marvell,mvusb.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/marvell,mvusb.yaml b/Documentation/devicetree/bindings/net/marvell,mvusb.yaml
> new file mode 100644
> index 000000000000..9458f6659be1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/marvell,mvusb.yaml
> @@ -0,0 +1,65 @@
> +# SPDX-License-Identifier: GPL-2.0

New bindings should be:

(GPL-2.0-only OR BSD-2-Clause)

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/marvell,mvusb.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell USB to MDIO Controller
> +
> +maintainers:
> +  - Tobias Waldekranz <tobias@waldekranz.com>
> +
> +description: |+
> +  This controller is mounted on development boards for Marvell's Link Street
> +  family of Ethernet switches. It allows you to configure the switch's registers
> +  using the standard MDIO interface.
> +
> +  Since the device is connected over USB, there is no strict requirement of
> +  having a device tree representation of the device. But in order to use it with
> +  the mv88e6xxx driver, you need a device tree node in which to place the switch
> +  definition.
> +
> +allOf:
> +  - $ref: "mdio.yaml#"
> +
> +properties:
> +  compatible:
> +    const: usb1286,1fa4
> +  reg:
> +    maxItems: 1
> +    description: The USB port number on the host controller

Really, it's the port on the hub which could be a root hub.

> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +
> +examples:
> +  - |
> +    /* USB host controller */
> +    &usb1 {

This won't compile because there's no 'usb1' to reference.

> +            mvusb: mdio@1 {
> +                    compatible = "usb1286,1fa4";
> +                    reg = <1>;
> +                    #address-cells = <1>;
> +                    #size-cells = <0>;
> +            };
> +    };
> +
> +    /* MV88E6390X devboard */
> +    &mvusb {

Move this into the above node.

> +            switch@0 {
> +                    compatible = "marvell,mv88e6190";
> +                    status = "ok";

Don't show status in examples.

> +                    reg = <0x0>;
> +
> +                    ports {
> +                            /* Port definitions */

Incomplete examples will eventually fail as when complete schema are 
present.

> +                    };
> +
> +                    mdio {
> +                            /* PHY definitions */
> +                    };
> +            };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 97dce264bc7c..ff35669f8712 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10101,6 +10101,12 @@ M:	Nicolas Pitre <nico@fluxnic.net>
>  S:	Odd Fixes
>  F:	drivers/mmc/host/mvsdio.*
>  
> +MARVELL USB MDIO CONTROLLER DRIVER
> +M:	Tobias Waldekranz <tobias@waldekranz.com>
> +L:	netdev@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/net/marvell,mvusb.yaml
> +
>  MARVELL XENON MMC/SD/SDIO HOST CONTROLLER DRIVER
>  M:	Hu Ziji <huziji@marvell.com>
>  L:	linux-mmc@vger.kernel.org
> -- 
> 2.17.1
>