usb: usb5303: add support for reference clock specified in device tree

Message ID 1400586822-3837-1-git-send-email-m.szyprowski@samsung.com
State New
Headers show

Commit Message

Marek Szyprowski May 20, 2014, 11:53 a.m.
USB3503 chip supports 8 values of reference clock. The value is
specified by REF_SEL[1:0] pins and INT_N line. This patch add support
for getting 'refclk' clock, enabling it and setting INT_N line according
to the value of the gathered clock. If no clock has been specified,
driver defaults to the old behaviour (assuming that clock has been
specified by REF_SEL pins from primary reference clock frequencies
table).

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/usb/misc/usb3503.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

Hello,

This extension to USB3503 driver is needed to add support for OdroidU3
board, which uses 24MHz reference clock, sourced directly from CLKOUT
line from Exynos4412 SoC.

Best regards
Marek Szyprowski
Samsung R&D Institute Poland

Comments

Mark Brown May 20, 2014, 11:57 a.m. | #1
On Tue, May 20, 2014 at 01:53:42PM +0200, Marek Szyprowski wrote:

> +		hub->clk = devm_clk_get(dev, "refclk");
> +		if (!IS_ERR(hub->clk)) {

This won't handle deferred probe - the driver should error out if it
gets -EPROBE_DEFER since it means the clock exists and might be provided
later on.

> +			unsigned long rate;
> +
> +			clk_prepare_enable(hub->clk);
> +			rate = clk_get_rate(hub->clk);

No error checking here.

> +
> +			if (rate == 38400000 || rate == 26000000 ||
> +			    rate == 19200000 || rate == 12000000)
> +				hub->secondary_ref_clk = 0;
> +			else if (rate == 24000000 || rate == 27000000 ||
> +			    rate == 25000000 || rate == 50000000)
> +				hub->secondary_ref_clk = 1;
> +			else
> +				dev_err(dev,
> +					"unsupported reference clock rate (%d)\n",
> +					rate);

This looks like a switch statement.  Should the driver not try to set
the clock to a supported rate if it's not already at one rather than
error out - it seems like a more constructive thing to do?
Marek Szyprowski May 20, 2014, 12:18 p.m. | #2
Hello,

On 2014-05-20 13:57, Mark Brown wrote:
> On Tue, May 20, 2014 at 01:53:42PM +0200, Marek Szyprowski wrote:
>
> > +		hub->clk = devm_clk_get(dev, "refclk");
> > +		if (!IS_ERR(hub->clk)) {
>
> This won't handle deferred probe - the driver should error out if it
> gets -EPROBE_DEFER since it means the clock exists and might be provided
> later on.

Ok, I will add such case here.

>
> > +			unsigned long rate;
> > +
> > +			clk_prepare_enable(hub->clk);
> > +			rate = clk_get_rate(hub->clk);
>
> No error checking here.
> > +
> > +			if (rate == 38400000 || rate == 26000000 ||
> > +			    rate == 19200000 || rate == 12000000)
> > +				hub->secondary_ref_clk = 0;
> > +			else if (rate == 24000000 || rate == 27000000 ||
> > +			    rate == 25000000 || rate == 50000000)
> > +				hub->secondary_ref_clk = 1;
> > +			else
> > +				dev_err(dev,
> > +					"unsupported reference clock rate (%d)\n",
> > +					rate);
>
> This looks like a switch statement.  Should the driver not try to set
> the clock to a supported rate if it's not already at one rather than
> error out - it seems like a more constructive thing to do?

Hmm, the problem here is that you cannot determine the right rate. The 
rate is
encoded on REF_SEL[1:0] pins (usually soldered to some resistors) and 
cannot be
read via i2c commands. To set add support for rate setting, I would need 
to add
one more property with correct ref clock rate. Is this okay?

Best regards
Mark Brown May 20, 2014, 9:52 p.m. | #3
On Tue, May 20, 2014 at 02:18:30PM +0200, Marek Szyprowski wrote:
> On 2014-05-20 13:57, Mark Brown wrote:

> >> +			else
> >> +				dev_err(dev,
> >> +					"unsupported reference clock rate (%d)\n",
> >> +					rate);

> >This looks like a switch statement.  Should the driver not try to set
> >the clock to a supported rate if it's not already at one rather than
> >error out - it seems like a more constructive thing to do?

> Hmm, the problem here is that you cannot determine the right rate. The rate
> is
> encoded on REF_SEL[1:0] pins (usually soldered to some resistors) and cannot
> be
> read via i2c commands. To set add support for rate setting, I would need to
> add
> one more property with correct ref clock rate. Is this okay?

Sounds reasonable to me, yes.

Patch

diff --git a/drivers/usb/misc/usb3503.c b/drivers/usb/misc/usb3503.c
index a31641e18d19..52cb7549b775 100644
--- a/drivers/usb/misc/usb3503.c
+++ b/drivers/usb/misc/usb3503.c
@@ -18,6 +18,7 @@ 
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
+#include <linux/clk.h>
 #include <linux/i2c.h>
 #include <linux/gpio.h>
 #include <linux/delay.h>
@@ -57,10 +58,12 @@  struct usb3503 {
 	enum usb3503_mode	mode;
 	struct regmap		*regmap;
 	struct device		*dev;
+	struct clk		*clk;
 	u8	port_off_mask;
 	int	gpio_intn;
 	int	gpio_reset;
 	int	gpio_connect;
+	bool	secondary_ref_clk;
 };
 
 static int usb3503_reset(struct usb3503 *hub, int state)
@@ -186,6 +189,25 @@  static int usb3503_probe(struct usb3503 *hub)
 	} else if (np) {
 		hub->port_off_mask = 0;
 
+		hub->clk = devm_clk_get(dev, "refclk");
+		if (!IS_ERR(hub->clk)) {
+			unsigned long rate;
+
+			clk_prepare_enable(hub->clk);
+			rate = clk_get_rate(hub->clk);
+
+			if (rate == 38400000 || rate == 26000000 ||
+			    rate == 19200000 || rate == 12000000)
+				hub->secondary_ref_clk = 0;
+			else if (rate == 24000000 || rate == 27000000 ||
+			    rate == 25000000 || rate == 50000000)
+				hub->secondary_ref_clk = 1;
+			else
+				dev_err(dev,
+					"unsupported reference clock rate (%d)\n",
+					rate);
+		}
+
 		property = of_get_property(np, "disabled-ports", &len);
 		if (property && (len / sizeof(u32)) > 0) {
 			int i;
@@ -213,8 +235,10 @@  static int usb3503_probe(struct usb3503 *hub)
 		dev_err(dev, "Ports disabled with no control interface\n");
 
 	if (gpio_is_valid(hub->gpio_intn)) {
-		err = devm_gpio_request_one(dev, hub->gpio_intn,
-				GPIOF_OUT_INIT_HIGH, "usb3503 intn");
+		int val = hub->secondary_ref_clk ? GPIOF_OUT_INIT_LOW :
+						   GPIOF_OUT_INIT_HIGH;
+		err = devm_gpio_request_one(dev, hub->gpio_intn, val,
+					    "usb3503 intn");
 		if (err) {
 			dev_err(dev,
 				"unable to request GPIO %d as connect pin (%d)\n",