[PATCH 1/2] usb: misc: onboard_usb_hub: Add support for clock input

[PATCH 1/2] usb: misc: onboard_usb_hub: Add support for clock input

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

Most onboard USB hubs have a dedicated crystal oscillator but on some
boards the clock signal for the hub is provided by the SoC.

In order to support this, we add the possibility of specifying a
clock in the devicetree that gets enabled/disabled when the hub
is powered up/down.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/usb/misc/onboard_usb_hub.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
index a341b2fbb7b44..e710e3c82ba9b 100644
--- a/drivers/usb/misc/onboard_usb_hub.c
+++ b/drivers/usb/misc/onboard_usb_hub.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2022, Google LLC
  */
 
+#include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/export.h>
 #include <linux/gpio/consumer.h>
@@ -60,12 +61,19 @@ struct onboard_hub {
 	bool going_away;
 	struct list_head udev_list;
 	struct mutex lock;
+	struct clk *clk;
 };
 
 static int onboard_hub_power_on(struct onboard_hub *hub)
 {
 	int err;
 
+	err = clk_prepare_enable(hub->clk);
+	if (err) {
+		dev_err(hub->dev, "failed to enable clock: %d\n", err);
+		return err;
+	}
+
 	err = regulator_bulk_enable(hub->pdata->num_supplies, hub->supplies);
 	if (err) {
 		dev_err(hub->dev, "failed to enable supplies: %d\n", err);
@@ -92,6 +100,8 @@ static int onboard_hub_power_off(struct onboard_hub *hub)
 		return err;
 	}
 
+	clk_disable_unprepare(hub->clk);
+
 	hub->is_powered_on = false;
 
 	return 0;
@@ -266,6 +276,10 @@ static int onboard_hub_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	hub->clk = devm_clk_get_optional(dev, NULL);
+	if (IS_ERR(hub->clk))
+		return dev_err_probe(dev, PTR_ERR(hub->clk), "failed to get clock\n");
+
 	hub->reset_gpio = devm_gpiod_get_optional(dev, "reset",
 						  GPIOD_OUT_HIGH);
 	if (IS_ERR(hub->reset_gpio))
-- 
2.42.1

[PATCH 2/2] usb: misc: onboard_usb_hub: Add support for Cypress CY7C6563x

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

The Cypress CY7C6563x is a 2/4-port USB 2.0 hub. Add support for
this hub in the driver in order to bring up reset, supply or clock
dependencies.

There is no reset pulse width given in the datasheet so we expect
a minimal value of 1us to be enough. This hasn't been tested though
due to lack of hardware which has the reset connected to a GPIO.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/usb/misc/onboard_usb_hub.c | 1 +
 drivers/usb/misc/onboard_usb_hub.h | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
index e710e3c82ba9b..e2e011036d359 100644
--- a/drivers/usb/misc/onboard_usb_hub.c
+++ b/drivers/usb/misc/onboard_usb_hub.c
@@ -440,6 +440,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_CYPRESS, 0x6504) }, /* CYUSB33{0,1,2}x/CYUSB230x 3.0 */
 	{ USB_DEVICE(VENDOR_ID_CYPRESS, 0x6506) }, /* CYUSB33{0,1,2}x/CYUSB230x 2.0 */
+	{ USB_DEVICE(VENDOR_ID_CYPRESS, 0x6570) }, /* CY7C6563x 2.0 */
 	{ USB_DEVICE(VENDOR_ID_GENESYS, 0x0608) }, /* Genesys Logic GL850G USB 2.0 */
 	{ USB_DEVICE(VENDOR_ID_GENESYS, 0x0610) }, /* Genesys Logic GL852G USB 2.0 */
 	{ USB_DEVICE(VENDOR_ID_GENESYS, 0x0620) }, /* Genesys Logic GL3523 USB 3.1 */
diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h
index c4e24a7b92904..67b2cc1e15e67 100644
--- a/drivers/usb/misc/onboard_usb_hub.h
+++ b/drivers/usb/misc/onboard_usb_hub.h
@@ -31,6 +31,11 @@ static const struct onboard_hub_pdata cypress_hx3_data = {
 	.num_supplies = 2,
 };
 
+static const struct onboard_hub_pdata cypress_hx2vl_data = {
+	.reset_us = 1,
+	.num_supplies = 1,
+};
+
 static const struct onboard_hub_pdata genesys_gl850g_data = {
 	.reset_us = 3,
 	.num_supplies = 1,
@@ -54,6 +59,7 @@ static const struct of_device_id onboard_hub_match[] = {
 	{ .compatible = "usb451,8142", .data = &ti_tusb8041_data, },
 	{ .compatible = "usb4b4,6504", .data = &cypress_hx3_data, },
 	{ .compatible = "usb4b4,6506", .data = &cypress_hx3_data, },
+	{ .compatible = "usb4b4,6570", .data = &cypress_hx2vl_data, },
 	{ .compatible = "usb5e3,608", .data = &genesys_gl850g_data, },
 	{ .compatible = "usb5e3,610", .data = &genesys_gl852g_data, },
 	{ .compatible = "usb5e3,620", .data = &genesys_gl852g_data, },
-- 
2.42.1

Re: [PATCH 1/2] usb: misc: onboard_usb_hub: Add support for clock input

[Greg Kroah-Hartman]

On Thu, Nov 23, 2023 at 02:47:20PM +0100, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> Most onboard USB hubs have a dedicated crystal oscillator but on some
> boards the clock signal for the hub is provided by the SoC.
> 
> In order to support this, we add the possibility of specifying a
> clock in the devicetree that gets enabled/disabled when the hub
> is powered up/down.
> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
>  drivers/usb/misc/onboard_usb_hub.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
> index a341b2fbb7b44..e710e3c82ba9b 100644
> --- a/drivers/usb/misc/onboard_usb_hub.c
> +++ b/drivers/usb/misc/onboard_usb_hub.c
> @@ -5,6 +5,7 @@
>   * Copyright (c) 2022, Google LLC
>   */
>  
> +#include <linux/clk.h>
>  #include <linux/device.h>
>  #include <linux/export.h>
>  #include <linux/gpio/consumer.h>
> @@ -60,12 +61,19 @@ struct onboard_hub {
>  	bool going_away;
>  	struct list_head udev_list;
>  	struct mutex lock;
> +	struct clk *clk;
>  };
>  
>  static int onboard_hub_power_on(struct onboard_hub *hub)
>  {
>  	int err;
>  
> +	err = clk_prepare_enable(hub->clk);
> +	if (err) {
> +		dev_err(hub->dev, "failed to enable clock: %d\n", err);
> +		return err;
> +	}

But what happens if clk is not set here?

And doesn't clk_prepare_enable() print out a message if it fails?

thanks,

greg k-h

Re: [PATCH 1/2] usb: misc: onboard_usb_hub: Add support for clock input

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 769 bytes --]

Hello,

On Thu, Nov 23, 2023 at 01:55:57PM +0000, Greg Kroah-Hartman wrote:
> On Thu, Nov 23, 2023 at 02:47:20PM +0100, Frieder Schrempf wrote:
> > +	err = clk_prepare_enable(hub->clk);
> > +	if (err) {
> > +		dev_err(hub->dev, "failed to enable clock: %d\n", err);
> > +		return err;

I suggest to use %pe (and ERR_PTR(err)) here.

> > +	}
> 
> But what happens if clk is not set here?

clk_prepare_enable() just does "return 0" if the clk argument is NULL.

> And doesn't clk_prepare_enable() print out a message if it fails?

clk_prepare_enable is silent on errors.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] usb: misc: onboard_usb_hub: Add support for clock input

[Frieder Schrempf]

Hi Greg, hi Uwe,

thanks for reviewing!

On 23.11.23 18:36, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Nov 23, 2023 at 01:55:57PM +0000, Greg Kroah-Hartman wrote:
>> On Thu, Nov 23, 2023 at 02:47:20PM +0100, Frieder Schrempf wrote:
>>> +	err = clk_prepare_enable(hub->clk);
>>> +	if (err) {
>>> +		dev_err(hub->dev, "failed to enable clock: %d\n", err);
>>> +		return err;
> 
> I suggest to use %pe (and ERR_PTR(err)) here.

Ok, I added this in v2. I also added a patch to convert the other error
logs to be consistent within the driver.

> 
>>> +	}
>>
>> But what happens if clk is not set here?
> 
> clk_prepare_enable() just does "return 0" if the clk argument is NULL.

Exactly!

> 
>> And doesn't clk_prepare_enable() print out a message if it fails?
> 
> clk_prepare_enable is silent on errors.

Right!

Thanks
Frieder