| Message ID | 20221228100321.15949-7-linux.amoon@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,01/11] dt-bindings: usb: Add device id for Genesys Logic hub controller | expand |
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 >
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 > >
On Sat, Jan 07, 2023 at 08:28:11PM +0530, Anand Moon wrote: > 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. Do I understand correctly that 0x0610 is the product id of both the GL852G [1] and the USB 2 part of the GL3523 (the above 'lsusb' output)? [1] https://patchwork.kernel.org/project/linux-usb/patch/20221228100321.15949-2-linux.amoon@gmail.com/
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, },
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(+)