diff mbox series

[v1,06/11] usb: misc: onboard_usb_hub: add Genesys Logic GL3523-QFN76 hub support

Message ID 20221228100321.15949-7-linux.amoon@gmail.com
State New
Headers show
Series None | expand

Commit Message

Anand Moon Dec. 28, 2022, 10:03 a.m. UTC
Genesys Logic GL3523-QFN76 is a 4-port USB 3.1 hub that has a reset pin to
toggle and a 5.0V core supply exported though an integrated LDO is
available for powering it.

Add the support for this hub, for controlling the reset pin and the core
power supply.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 drivers/usb/misc/onboard_usb_hub.c | 1 +
 drivers/usb/misc/onboard_usb_hub.h | 1 +
 2 files changed, 2 insertions(+)

Comments

Matthias Kaehlcke Jan. 4, 2023, 8:43 p.m. UTC | #1
Hi Anand,

On Wed, Dec 28, 2022 at 10:03:15AM +0000, Anand Moon wrote:
> Genesys Logic GL3523-QFN76 is a 4-port USB 3.1 hub that has a reset pin to
> toggle and a 5.0V core supply exported though an integrated LDO is
> available for powering it.
> 
> Add the support for this hub, for controlling the reset pin and the core
> power supply.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  drivers/usb/misc/onboard_usb_hub.c | 1 +
>  drivers/usb/misc/onboard_usb_hub.h | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
> index c0e8e6f4ec0a..699050eb3f17 100644
> --- a/drivers/usb/misc/onboard_usb_hub.c
> +++ b/drivers/usb/misc/onboard_usb_hub.c
> @@ -410,6 +410,7 @@ static void onboard_hub_usbdev_disconnect(struct usb_device *udev)
>  static const struct usb_device_id onboard_hub_id_table[] = {
>  	{ USB_DEVICE(VENDOR_ID_GENESYS, 0x0608) }, /* Genesys Logic GL850G USB 2.0 */
>  	{ USB_DEVICE(VENDOR_ID_GENESYS, 0x0610) }, /* Genesys Logic GL852G-OHG USB 2.0 */
> +	{ USB_DEVICE(VENDOR_ID_GENESYS, 0x0620) }, /* Genesys Logic GL3523-QFN76 USB 3.1 */

Please drop the '-QFN76' suffix. The GL3523 comes in different packages, 'QFN76'
is one of them, I'd expect the other packages to use the same product id.

The GL3523 is a single IC, however like the TI USB8041 or the RTS5414 it
provides both a USB 3.1 and a USB 2.0 hub. You should also add an entry for
the USB 2.0 hub here.

>  	{ USB_DEVICE(VENDOR_ID_MICROCHIP, 0x2514) }, /* USB2514B USB 2.0 */
>  	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x0411) }, /* RTS5411 USB 3.1 */
>  	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5411) }, /* RTS5411 USB 2.1 */
> diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h
> index 2ee1b0032d23..b32fad3a70f9 100644
> --- a/drivers/usb/misc/onboard_usb_hub.h
> +++ b/drivers/usb/misc/onboard_usb_hub.h
> @@ -32,6 +32,7 @@ static const struct of_device_id onboard_hub_match[] = {
>  	{ .compatible = "usb451,8142", .data = &ti_tusb8041_data, },
>  	{ .compatible = "usb5e3,608", .data = &genesys_gl850g_data, },
>  	{ .compatible = "genesys,usb5e3,610", .data = &genesys_gl850g_data, },
> +	{ .compatible = "genesys,usb5e3,620", .data = &genesys_gl850g_data, },

s/genesys,//

This reuses the settings of the GL850G hub, which doesn't seem correct in
this case. For the GL850G a (minimum) reset time of 3us is configured. The
data sheet of the GL3523 says:

  "The (internal) reset will be released after approximately 40 μS after
   power good.

   To fully control the reset process of GL3523, we suggest the reset time
   applied in the external reset circuit should longer than that of the
   internal reset circuit."

Since it is 'approximately 40 μS' I'd say make the external reset 50 μS
to be on the safe side, it's a very short time in any case.

Please also add an entry for the USB 2.0 part of the IC.

>  	{ .compatible = "usbbda,411", .data = &realtek_rts5411_data, },
>  	{ .compatible = "usbbda,5411", .data = &realtek_rts5411_data, },
>  	{ .compatible = "usbbda,414", .data = &realtek_rts5411_data, },
> -- 
> 2.38.1
>
Anand Moon Jan. 7, 2023, 2:58 p.m. UTC | #2
Hi Matthias,

Thanks for your review comments,

On Thu, 5 Jan 2023 at 02:13, Matthias Kaehlcke <mka@chromium.org> wrote:
>
> Hi Anand,
>
> On Wed, Dec 28, 2022 at 10:03:15AM +0000, Anand Moon wrote:
> > Genesys Logic GL3523-QFN76 is a 4-port USB 3.1 hub that has a reset pin to
> > toggle and a 5.0V core supply exported though an integrated LDO is
> > available for powering it.
> >
> > Add the support for this hub, for controlling the reset pin and the core
> > power supply.
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> >  drivers/usb/misc/onboard_usb_hub.c | 1 +
> >  drivers/usb/misc/onboard_usb_hub.h | 1 +
> >  2 files changed, 2 insertions(+)
> >
> > diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
> > index c0e8e6f4ec0a..699050eb3f17 100644
> > --- a/drivers/usb/misc/onboard_usb_hub.c
> > +++ b/drivers/usb/misc/onboard_usb_hub.c
> > @@ -410,6 +410,7 @@ static void onboard_hub_usbdev_disconnect(struct usb_device *udev)
> >  static const struct usb_device_id onboard_hub_id_table[] = {
> >       { USB_DEVICE(VENDOR_ID_GENESYS, 0x0608) }, /* Genesys Logic GL850G USB 2.0 */
> >       { USB_DEVICE(VENDOR_ID_GENESYS, 0x0610) }, /* Genesys Logic GL852G-OHG USB 2.0 */
> > +     { USB_DEVICE(VENDOR_ID_GENESYS, 0x0620) }, /* Genesys Logic GL3523-QFN76 USB 3.1 */
>
> Please drop the '-QFN76' suffix. The GL3523 comes in different packages, 'QFN76'
> is one of them, I'd expect the other packages to use the same product id.
>
> The GL3523 is a single IC, however like the TI USB8041 or the RTS5414 it
> provides both a USB 3.1 and a USB 2.0 hub. You should also add an entry for
> the USB 2.0 hub here.
>

Ok,

> >       { USB_DEVICE(VENDOR_ID_MICROCHIP, 0x2514) }, /* USB2514B USB 2.0 */
> >       { USB_DEVICE(VENDOR_ID_REALTEK, 0x0411) }, /* RTS5411 USB 3.1 */
> >       { USB_DEVICE(VENDOR_ID_REALTEK, 0x5411) }, /* RTS5411 USB 2.1 */
> > diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h
> > index 2ee1b0032d23..b32fad3a70f9 100644
> > --- a/drivers/usb/misc/onboard_usb_hub.h
> > +++ b/drivers/usb/misc/onboard_usb_hub.h
> > @@ -32,6 +32,7 @@ static const struct of_device_id onboard_hub_match[] = {
> >       { .compatible = "usb451,8142", .data = &ti_tusb8041_data, },
> >       { .compatible = "usb5e3,608", .data = &genesys_gl850g_data, },
> >       { .compatible = "genesys,usb5e3,610", .data = &genesys_gl850g_data, },
> > +     { .compatible = "genesys,usb5e3,620", .data = &genesys_gl850g_data, },
>
> s/genesys,//
>
> This reuses the settings of the GL850G hub, which doesn't seem correct in
> this case. For the GL850G a (minimum) reset time of 3us is configured. The
> data sheet of the GL3523 says:
>
>   "The (internal) reset will be released after approximately 40 μS after
>    power good.
>
>    To fully control the reset process of GL3523, we suggest the reset time
>    applied in the external reset circuit should longer than that of the
>    internal reset circuit."
>
> Since it is 'approximately 40 μS' I'd say make the external reset 50 μS
> to be on the safe side, it's a very short time in any case.
>

Thanks for this input will update this in the next version.

> Please also add an entry for the USB 2.0 part of the IC.

alarm@odroid-n2:~$ lsusb -tv
/:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M
    ID 1d6b:0003 Linux Foundation 3.0 root hub
    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 5000M
        ID 05e3:0620 Genesys Logic, Inc. GL3523 Hub
/:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/2p, 480M
    ID 1d6b:0002 Linux Foundation 2.0 root hub
    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
        ID 05e3:0610 Genesys Logic, Inc. Hub

So earlier patch adds support for this device ID.
>
> >       { .compatible = "usbbda,411", .data = &realtek_rts5411_data, },
> >       { .compatible = "usbbda,5411", .data = &realtek_rts5411_data, },
> >       { .compatible = "usbbda,414", .data = &realtek_rts5411_data, },
> > --
> > 2.38.1
> >
diff mbox series

Patch

diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
index c0e8e6f4ec0a..699050eb3f17 100644
--- a/drivers/usb/misc/onboard_usb_hub.c
+++ b/drivers/usb/misc/onboard_usb_hub.c
@@ -410,6 +410,7 @@  static void onboard_hub_usbdev_disconnect(struct usb_device *udev)
 static const struct usb_device_id onboard_hub_id_table[] = {
 	{ USB_DEVICE(VENDOR_ID_GENESYS, 0x0608) }, /* Genesys Logic GL850G USB 2.0 */
 	{ USB_DEVICE(VENDOR_ID_GENESYS, 0x0610) }, /* Genesys Logic GL852G-OHG USB 2.0 */
+	{ USB_DEVICE(VENDOR_ID_GENESYS, 0x0620) }, /* Genesys Logic GL3523-QFN76 USB 3.1 */
 	{ USB_DEVICE(VENDOR_ID_MICROCHIP, 0x2514) }, /* USB2514B USB 2.0 */
 	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x0411) }, /* RTS5411 USB 3.1 */
 	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5411) }, /* RTS5411 USB 2.1 */
diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h
index 2ee1b0032d23..b32fad3a70f9 100644
--- a/drivers/usb/misc/onboard_usb_hub.h
+++ b/drivers/usb/misc/onboard_usb_hub.h
@@ -32,6 +32,7 @@  static const struct of_device_id onboard_hub_match[] = {
 	{ .compatible = "usb451,8142", .data = &ti_tusb8041_data, },
 	{ .compatible = "usb5e3,608", .data = &genesys_gl850g_data, },
 	{ .compatible = "genesys,usb5e3,610", .data = &genesys_gl850g_data, },
+	{ .compatible = "genesys,usb5e3,620", .data = &genesys_gl850g_data, },
 	{ .compatible = "usbbda,411", .data = &realtek_rts5411_data, },
 	{ .compatible = "usbbda,5411", .data = &realtek_rts5411_data, },
 	{ .compatible = "usbbda,414", .data = &realtek_rts5411_data, },