Re: [PATCH v4] usb: uhci: Add clk support to uhci-platform

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello!

On 1/16/2018 12:53 AM, Benjamin Herrenschmidt wrote:

The Aspeed SoCs use uhci-platform. With the new dynamic clock
control framework, the corresponding IP block clock must be
properly enabled.

This is a simplified variant of what ehci-platform does, it
looks for *one* clock attached to the device, and if it's
there, enables it.

Signed-off-by: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>
Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
---

v4. Remove unnecessary NULL checks

v3. Fix tab vs. spaces and probe error path

v2. Don't forget to git add latest changes :-) This adds the
      part where we turn the clock off when removing the driver.
---
   drivers/usb/host/uhci-hcd.h      |  3 +++
   drivers/usb/host/uhci-platform.c | 20 ++++++++++++++++++--
   2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h
index f1cc47292a59..7f9f33c8c232 100644
--- a/drivers/usb/host/uhci-hcd.h
+++ b/drivers/usb/host/uhci-hcd.h
@@ -4,6 +4,7 @@
#include <linux/list.h>
   #include <linux/usb.h>
+#include <linux/clk.h>
#define usb_packetid(pipe) (usb_pipein(pipe) ? USB_PID_IN : USB_PID_OUT)
   #define PIPE_DEVEP_MASK		0x0007ff00
@@ -447,6 +448,8 @@ struct uhci_hcd {
   	int total_load;				/* Sum of array values */
   	short load[MAX_PHASE];			/* Periodic allocations */
+ struct clk *clk; /* (optional) clock source */
+
   	/* Reset host controller */
   	void	(*reset_hc) (struct uhci_hcd *uhci);
   	int	(*check_and_reset_hc) (struct uhci_hcd *uhci);
diff --git a/drivers/usb/host/uhci-platform.c b/drivers/usb/host/uhci-platform.c
index 6cb16d4b2257..55c2d99bf7ec 100644
--- a/drivers/usb/host/uhci-platform.c
+++ b/drivers/usb/host/uhci-platform.c
@@ -89,6 +89,8 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
   	if (!hcd)
   		return -ENOMEM;
+ uhci = hcd_to_uhci(hcd);
+
   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   	hcd->regs = devm_ioremap_resource(&pdev->dev, res);
   	if (IS_ERR(hcd->regs)) {
@@ -98,8 +100,6 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
   	hcd->rsrc_start = res->start;
   	hcd->rsrc_len = resource_size(res);
- uhci = hcd_to_uhci(hcd);
-
   	uhci->regs = hcd->regs;
/* Grab some things from the device-tree */
@@ -119,6 +119,19 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
   				 "Enabled Aspeed implementation workarounds\n");
   		}
   	}
+
+	/* Get and enable clock if any specified */
+	uhci->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(uhci->clk)) {
+		ret = PTR_ERR(uhci->clk);
+		goto err_rmr;
+	}
+	ret = clk_prepare_enable(uhci->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "Error couldn't enable clock (%d)\n", ret);
+		goto err_rmr;
+	}
+
   	ret = usb_add_hcd(hcd, pdev->resource[1].start, IRQF_SHARED);
   	if (ret)
   		goto err_rmr;
@@ -127,6 +140,7 @@ static int uhci_hcd_platform_probe(struct platform_device *pdev)
   	return 0;
err_rmr:
+	clk_disable_unprepare(uhci->clk);

      Really? You haven't enabled the clock before either *goto* above, why
disable it here?

If clk is NULL or an error, disable will do nothing. The only case

The rule of thumb was that you don't pass the error pointers to APIs but indeed I'm now seeing the NULL check extended to the error pointers too...

would be if enable at failed but then disable will do nothing in that
case.

Are you sure there'll be no WARN_ON() triggered by clk_disable() with the enable count == 0?

The above change was done at the suggestion of Alan. If you really

   I'd probably missed his mail.

prefer I could add a clk_put and clearing of "clk" in the error path of
clk_prepare_enable, but that's about it.

BTW. DO we really need to bike shed such a simple patch to death like
that ?

   It was not about bike shedding, just about the correctness...

[...]

MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux