diff mbox series

[v2,3/3] usb: misc: onboard_usb_hub: Add TI USB8041 hub support

Message ID 20220715073300.868087-3-alexander.stein@ew.tq-group.com (mailing list archive)
State Superseded
Headers show
Series [v2,1/3] dt-bindings: usb: Add binding for TI USB8041 hub controller | expand

Commit Message

Alexander Stein July 15, 2022, 7:33 a.m. UTC
This is a 4-port 3.0 USB hub.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes in v2:
* Add devtype data containing waiting times

 drivers/usb/misc/onboard_usb_hub.c | 3 +++
 drivers/usb/misc/onboard_usb_hub.h | 7 +++++++
 2 files changed, 10 insertions(+)

Comments

Matthias Kaehlcke July 15, 2022, 7:50 p.m. UTC | #1
On Fri, Jul 15, 2022 at 09:33:00AM +0200, Alexander Stein wrote:
> This is a 4-port 3.0 USB hub.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> Changes in v2:
> * Add devtype data containing waiting times
> 
>  drivers/usb/misc/onboard_usb_hub.c | 3 +++
>  drivers/usb/misc/onboard_usb_hub.h | 7 +++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
> index 1dd7f4767def..319b4b1748fb 100644
> --- a/drivers/usb/misc/onboard_usb_hub.c
> +++ b/drivers/usb/misc/onboard_usb_hub.c
> @@ -339,6 +339,7 @@ static struct platform_driver onboard_hub_driver = {
>  /************************** USB driver **************************/
>  
>  #define VENDOR_ID_REALTEK	0x0bda
> +#define VENDOR_ID_TI		0x0451
>  
>  /*
>   * Returns the onboard_hub platform device that is associated with the USB
> @@ -416,6 +417,8 @@ static const struct usb_device_id onboard_hub_id_table[] = {
>  	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5411) }, /* RTS5411 USB 2.1 */
>  	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x0414) }, /* RTS5414 USB 3.2 */
>  	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5414) }, /* RTS5414 USB 2.1 */
> +	{ USB_DEVICE(VENDOR_ID_TI, 0x8140) }, /* TI USB8041 3.0 */
> +	{ USB_DEVICE(VENDOR_ID_TI, 0x8142) }, /* TI USB8041 2.0 */
>  	{}
>  };
>  MODULE_DEVICE_TABLE(usb, onboard_hub_id_table);
> diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h
> index 7e743f4c8aaa..fcb6a9024bbd 100644
> --- a/drivers/usb/misc/onboard_usb_hub.h
> +++ b/drivers/usb/misc/onboard_usb_hub.h
> @@ -11,7 +11,14 @@ struct onboard_hub_devtype_data {
>  	unsigned long reset_duration;		/* reset pulse width in us */
>  };
>  
> +static const struct onboard_hub_devtype_data ti_tusb8041_data = {
> +	.power_stable_time = 3000,
> +	.reset_duration = 3000,

Aren't these two values actually the same thing, i.e. the minimum
duration of the reset?

From the data sheet:

  A minimum reset duration of 3 ms is required

  td2: VDD and VDD33 stable before de-assertion of GRSTz (>= 3ms)

My understanding is that td2 is just another expression of the first
requirement.

> +};
> +
>  static const struct of_device_id onboard_hub_match[] = {
> +	{ .compatible = "usb451,8140", .data = &ti_tusb8041_data, },
> +	{ .compatible = "usb451,8142", .data = &ti_tusb8041_data, },
>  	{ .compatible = "usbbda,411" },
>  	{ .compatible = "usbbda,5411" },
>  	{ .compatible = "usbbda,414" },

Ah, now I see why the struct is defined in the .h file, never mind my comment
on the other patch.
Alexander Stein July 20, 2022, 7:18 a.m. UTC | #2
Am Freitag, 15. Juli 2022, 21:50:41 CEST schrieb Matthias Kaehlcke:
> On Fri, Jul 15, 2022 at 09:33:00AM +0200, Alexander Stein wrote:
> > This is a 4-port 3.0 USB hub.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > Changes in v2:
> > * Add devtype data containing waiting times
> > 
> >  drivers/usb/misc/onboard_usb_hub.c | 3 +++
> >  drivers/usb/misc/onboard_usb_hub.h | 7 +++++++
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/drivers/usb/misc/onboard_usb_hub.c
> > b/drivers/usb/misc/onboard_usb_hub.c index 1dd7f4767def..319b4b1748fb
> > 100644
> > --- a/drivers/usb/misc/onboard_usb_hub.c
> > +++ b/drivers/usb/misc/onboard_usb_hub.c
> > @@ -339,6 +339,7 @@ static struct platform_driver onboard_hub_driver = {
> > 
> >  /************************** USB driver **************************/
> >  
> >  #define VENDOR_ID_REALTEK	0x0bda
> > 
> > +#define VENDOR_ID_TI		0x0451
> > 
> >  /*
> >  
> >   * Returns the onboard_hub platform device that is associated with the
> >   USB
> > 
> > @@ -416,6 +417,8 @@ static const struct usb_device_id
> > onboard_hub_id_table[] = {> 
> >  	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5411) }, /* RTS5411 USB 2.1 */
> >  	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x0414) }, /* RTS5414 USB 3.2 */
> >  	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5414) }, /* RTS5414 USB 2.1 */
> > 
> > +	{ USB_DEVICE(VENDOR_ID_TI, 0x8140) }, /* TI USB8041 3.0 */
> > +	{ USB_DEVICE(VENDOR_ID_TI, 0x8142) }, /* TI USB8041 2.0 */
> > 
> >  	{}
> >  
> >  };
> >  MODULE_DEVICE_TABLE(usb, onboard_hub_id_table);
> > 
> > diff --git a/drivers/usb/misc/onboard_usb_hub.h
> > b/drivers/usb/misc/onboard_usb_hub.h index 7e743f4c8aaa..fcb6a9024bbd
> > 100644
> > --- a/drivers/usb/misc/onboard_usb_hub.h
> > +++ b/drivers/usb/misc/onboard_usb_hub.h
> > @@ -11,7 +11,14 @@ struct onboard_hub_devtype_data {
> > 
> >  	unsigned long reset_duration;		/* reset pulse width in 
us */
> >  
> >  };
> > 
> > +static const struct onboard_hub_devtype_data ti_tusb8041_data = {
> > +	.power_stable_time = 3000,
> > +	.reset_duration = 3000,
> 
> Aren't these two values actually the same thing, i.e. the minimum
> duration of the reset?
> 
> From the data sheet:
> 
>   A minimum reset duration of 3 ms is required
> 
>   td2: VDD and VDD33 stable before de-assertion of GRSTz (>= 3ms)
> 
> My understanding is that td2 is just another expression of the first
> requirement.

They may have the same values, but IMHO they are applied differently.
'power_stable_time' (or power_stable_us now) applies after supplying power, 
while 'reset_duration' (or reset_us now) applies when you deassert the reset 
line. 
For that reason I would like to keep them separated.

Thanks and best regards,
Alexander
Matthias Kaehlcke July 21, 2022, 8:38 p.m. UTC | #3
On Wed, Jul 20, 2022 at 09:18:50AM +0200, Alexander Stein wrote:
> Am Freitag, 15. Juli 2022, 21:50:41 CEST schrieb Matthias Kaehlcke:
> > On Fri, Jul 15, 2022 at 09:33:00AM +0200, Alexander Stein wrote:
> > > This is a 4-port 3.0 USB hub.
> > > 
> > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > ---
> > > Changes in v2:
> > > * Add devtype data containing waiting times
> > > 
> > >  drivers/usb/misc/onboard_usb_hub.c | 3 +++
> > >  drivers/usb/misc/onboard_usb_hub.h | 7 +++++++
> > >  2 files changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/usb/misc/onboard_usb_hub.c
> > > b/drivers/usb/misc/onboard_usb_hub.c index 1dd7f4767def..319b4b1748fb
> > > 100644
> > > --- a/drivers/usb/misc/onboard_usb_hub.c
> > > +++ b/drivers/usb/misc/onboard_usb_hub.c
> > > @@ -339,6 +339,7 @@ static struct platform_driver onboard_hub_driver = {
> > > 
> > >  /************************** USB driver **************************/
> > >  
> > >  #define VENDOR_ID_REALTEK	0x0bda
> > > 
> > > +#define VENDOR_ID_TI		0x0451
> > > 
> > >  /*
> > >  
> > >   * Returns the onboard_hub platform device that is associated with the
> > >   USB
> > > 
> > > @@ -416,6 +417,8 @@ static const struct usb_device_id
> > > onboard_hub_id_table[] = {> 
> > >  	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5411) }, /* RTS5411 USB 2.1 */
> > >  	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x0414) }, /* RTS5414 USB 3.2 */
> > >  	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5414) }, /* RTS5414 USB 2.1 */
> > > 
> > > +	{ USB_DEVICE(VENDOR_ID_TI, 0x8140) }, /* TI USB8041 3.0 */
> > > +	{ USB_DEVICE(VENDOR_ID_TI, 0x8142) }, /* TI USB8041 2.0 */
> > > 
> > >  	{}
> > >  
> > >  };
> > >  MODULE_DEVICE_TABLE(usb, onboard_hub_id_table);
> > > 
> > > diff --git a/drivers/usb/misc/onboard_usb_hub.h
> > > b/drivers/usb/misc/onboard_usb_hub.h index 7e743f4c8aaa..fcb6a9024bbd
> > > 100644
> > > --- a/drivers/usb/misc/onboard_usb_hub.h
> > > +++ b/drivers/usb/misc/onboard_usb_hub.h
> > > @@ -11,7 +11,14 @@ struct onboard_hub_devtype_data {
> > > 
> > >  	unsigned long reset_duration;		/* reset pulse width in 
> us */
> > >  
> > >  };
> > > 
> > > +static const struct onboard_hub_devtype_data ti_tusb8041_data = {
> > > +	.power_stable_time = 3000,
> > > +	.reset_duration = 3000,
> > 
> > Aren't these two values actually the same thing, i.e. the minimum
> > duration of the reset?
> > 
> > From the data sheet:
> > 
> >   A minimum reset duration of 3 ms is required
> > 
> >   td2: VDD and VDD33 stable before de-assertion of GRSTz (>= 3ms)
> > 
> > My understanding is that td2 is just another expression of the first
> > requirement.
> 
> They may have the same values, but IMHO they are applied differently.
> 'power_stable_time' (or power_stable_us now) applies after supplying power, 
> while 'reset_duration' (or reset_us now) applies when you deassert the reset 
> line. 
> For that reason I would like to keep them separated.

In both cases the datasheet talks about the reset duration of 3 ms in
relation with the power supplies:

  7.6 Timing Requirements, Power-Up

  td2: VDD and VDD33 stable before de-assertion of GRSTz


  8.3.7 Power-Up and Reset

  A minimum reset duration of 3 ms is required. This is defined as the time when
  the power supplies are in the recommended operating range to the de-assertion
  of GRSTz.

At this point I see no good reason for having two different values. If it turns
out later that it is really needed for other hubs another fiels like
'power_stable_us' can still be added.
diff mbox series

Patch

diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
index 1dd7f4767def..319b4b1748fb 100644
--- a/drivers/usb/misc/onboard_usb_hub.c
+++ b/drivers/usb/misc/onboard_usb_hub.c
@@ -339,6 +339,7 @@  static struct platform_driver onboard_hub_driver = {
 /************************** USB driver **************************/
 
 #define VENDOR_ID_REALTEK	0x0bda
+#define VENDOR_ID_TI		0x0451
 
 /*
  * Returns the onboard_hub platform device that is associated with the USB
@@ -416,6 +417,8 @@  static const struct usb_device_id onboard_hub_id_table[] = {
 	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5411) }, /* RTS5411 USB 2.1 */
 	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x0414) }, /* RTS5414 USB 3.2 */
 	{ USB_DEVICE(VENDOR_ID_REALTEK, 0x5414) }, /* RTS5414 USB 2.1 */
+	{ USB_DEVICE(VENDOR_ID_TI, 0x8140) }, /* TI USB8041 3.0 */
+	{ USB_DEVICE(VENDOR_ID_TI, 0x8142) }, /* TI USB8041 2.0 */
 	{}
 };
 MODULE_DEVICE_TABLE(usb, onboard_hub_id_table);
diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h
index 7e743f4c8aaa..fcb6a9024bbd 100644
--- a/drivers/usb/misc/onboard_usb_hub.h
+++ b/drivers/usb/misc/onboard_usb_hub.h
@@ -11,7 +11,14 @@  struct onboard_hub_devtype_data {
 	unsigned long reset_duration;		/* reset pulse width in us */
 };
 
+static const struct onboard_hub_devtype_data ti_tusb8041_data = {
+	.power_stable_time = 3000,
+	.reset_duration = 3000,
+};
+
 static const struct of_device_id onboard_hub_match[] = {
+	{ .compatible = "usb451,8140", .data = &ti_tusb8041_data, },
+	{ .compatible = "usb451,8142", .data = &ti_tusb8041_data, },
 	{ .compatible = "usbbda,411" },
 	{ .compatible = "usbbda,5411" },
 	{ .compatible = "usbbda,414" },