Re: [PATCH 5/5 v8] usb: musb: da8xx: Add DT support for the DA8xx driver

2016-03-08 Thread Bin Liu 
On Tue, Mar 08, 2016 at 05:48:07PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 3/8/2016 11:35 AM, Petr Kulhavy wrote:
> 
> >@@ -544,17 +643,25 @@ static int da8xx_probe(struct platform_device 
> >*pdev)
> >  pinfo.data = pdata;
> >  pinfo.size_data = sizeof(*pdata);
> >
> >+ret = regulator_enable(glue->vbus_supply);
> 
>    What does this achieve with a regulator that can't be explicitly
> controlled?
> >>>
> >>>If this is a _real_ regulator, then it needs to be enabled by somebody,
> >>>doesn't it?
> >>
> >>   Well, it's possible that Vbus is controlled via GPIO like on DaVinci
> >>DM644x. But so far we haven't seen this on DA8xx...
> >>
> >>>No, as you can see just a few lines above in the patch.
> >>
> >>   I'm seeing exactly opposite -- you're enabling the regulator right before
> >>registering the platform device, so after registering generic PHY. It's
> >>therefore not at all clear why you've exchanged the calls above. I can't ACK
> >>this patch.
> >>
> >You are right. I'm wondering that this might be anyway incorrect, since the
> >Vbus should be provided in host mode only.
> 
>And there's musb_platform_ops::set_vbus() method for that. The
> regulator should be enabled/disabled there (if at all).
> 
> >We're back at the issue of the missing PHY.
> 
>Missing PHY driver, you mean?
> 
> >And on DA8xx the Vbus is (supposed to be) controlled via the DRVVBUS pin. So
> >it's probably better to not to touch the regulator at all, what do you think?
> 
>Yes, enabling it in the probe doesn't make much sense indeed.

Well, first of all, I never used regulator framework, and haven't looked
how to use it in a driver. So my following comments might be wrong.

The problem we are trying to solve is how pass the previous 'mentor,power' value
to musb driver, so how to use the regulator to control vbus is irrelevant
and unnecessary. So if we could set the 500mA value to a property of a
regulator in DT, then read it back in musb driver at the same place
currently reading 'mentor,power', it is done.

Regards,
-Bin.

> 
> [...]
> >@@ -582,11 +690,20 @@ static int da8xx_remove(struct platform_device 
> >*pdev)
> >  return 0;
> >  }
> >
> >+static const struct of_device_id da8xx_id_table[] = {
> >+{
> >+.compatible = "ti,da830-musb",
> >+},
> >+{},
> >+};
> >+MODULE_DEVICE_TABLE(of, da8xx_id_table);
> >+
> >  static struct platform_driver da8xx_driver = {
> >  .probe= da8xx_probe,
> >  .remove= da8xx_remove,
> >  .driver= {
> >  .name= "musb-da8xx",
> >+.of_match_table = of_match_ptr(da8xx_id_table),
> 
>    Doesn't this cause a warning about in non-DT case?
> 
> >>>No, the of_match_table is defined independent of the CONFIG_OF.
> >>
> >>   But of_match_ptr() depends on it, IIRC.
> >
> >In include/linux/of.h I do see:
> >
> >#ifdef (CONFIG_OF)
> >[...]
> >
> >#defineof_match_ptr
> >(_ptr)  (_ptr)
> >
> >[...]
> >#else
> >[...]
> >
> >#defineof_match_ptr
> >(_ptr)NULL
> >
> >[...]
> >#endif
> 
>Yes, I was wondering if the warning about da8xx_id_table[] being
> unused was generated when CONFIG_OF=n.
> 
> >Regards
> >Petr
> 
> MBR, Sergei
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5 v8] usb: musb: da8xx: Add DT support for the DA8xx driver

2016-03-08 Thread Sergei Shtylyov 

Hello.

On 3/8/2016 11:35 AM, Petr Kulhavy wrote:


@@ -544,17 +643,25 @@ static int da8xx_probe(struct platform_device *pdev)
  pinfo.data = pdata;
  pinfo.size_data = sizeof(*pdata);

+ret = regulator_enable(glue->vbus_supply);


   What does this achieve with a regulator that can't be explicitly
controlled?


If this is a _real_ regulator, then it needs to be enabled by somebody,
doesn't it?


   Well, it's possible that Vbus is controlled via GPIO like on DaVinci
DM644x. But so far we haven't seen this on DA8xx...


No, as you can see just a few lines above in the patch.


   I'm seeing exactly opposite -- you're enabling the regulator right before
registering the platform device, so after registering generic PHY. It's
therefore not at all clear why you've exchanged the calls above. I can't ACK
this patch.


You are right. I'm wondering that this might be anyway incorrect, since the
Vbus should be provided in host mode only.


   And there's musb_platform_ops::set_vbus() method for that. The regulator 
should be enabled/disabled there (if at all).



We're back at the issue of the missing PHY.


   Missing PHY driver, you mean?


And on DA8xx the Vbus is (supposed to be) controlled via the DRVVBUS pin. So
it's probably better to not to touch the regulator at all, what do you think?


   Yes, enabling it in the probe doesn't make much sense indeed.


[...]

@@ -582,11 +690,20 @@ static int da8xx_remove(struct platform_device *pdev)
  return 0;
  }

+static const struct of_device_id da8xx_id_table[] = {
+{
+.compatible = "ti,da830-musb",
+},
+{},
+};
+MODULE_DEVICE_TABLE(of, da8xx_id_table);
+
  static struct platform_driver da8xx_driver = {
  .probe= da8xx_probe,
  .remove= da8xx_remove,
  .driver= {
  .name= "musb-da8xx",
+.of_match_table = of_match_ptr(da8xx_id_table),


   Doesn't this cause a warning about in non-DT case?


No, the of_match_table is defined independent of the CONFIG_OF.


   But of_match_ptr() depends on it, IIRC.


In include/linux/of.h I do see:

#ifdef (CONFIG_OF)
[...]

#defineof_match_ptr
(_ptr)  (_ptr)

[...]
#else
[...]

#defineof_match_ptr
(_ptr)NULL

[...]
#endif


   Yes, I was wondering if the warning about da8xx_id_table[] being unused 
was generated when CONFIG_OF=n.



Regards
Petr


MBR, Sergei

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

Re: [PATCH 5/5 v8] usb: musb: da8xx: Add DT support for the DA8xx driver

2016-03-07 Thread Sergei Shtylyov 

On 03/07/2016 07:08 PM, Petr Kulhavy wrote:

Hello

On 07.03.2016 14:29, Sergei Shtylyov wrote:

Hello.

On 3/7/2016 2:20 PM, Petr Kulhavy wrote:


@@ -544,17 +643,25 @@ static int da8xx_probe(struct platform_device *pdev)
  pinfo.data = pdata;
  pinfo.size_data = sizeof(*pdata);

+ret = regulator_enable(glue->vbus_supply);


   What does this achieve with a regulator that can't be explicitly controlled?



If this is a _real_ regulator, then it needs to be enabled by somebody,
doesn't it?



   Well, it's possible that Vbus is controlled via GPIO like on DaVinci 
DM644x. But so far we haven't seen this on DA8xx...



+if (ret) {
+dev_err(>dev, "failed to enable power: %d\n", ret);
+goto err6;
+}
+
  glue->musb = musb = platform_device_register_full();
  if (IS_ERR(musb)) {
  ret = PTR_ERR(musb);
  dev_err(>dev, "failed to register musb device: %d\n", ret);
-goto err6;
+goto err7;
  }

  return 0;
+err7:
+usb_phy_generic_unregister(glue->phy);

  err6:
-usb_phy_generic_unregister(glue->phy);
+regulator_disable(glue->vbus_supply);


   How's that? Aren't you envaling Vbus regulator *after* registering PHY?


No, as you can see just a few lines above in the patch.


   I'm seeing exactly opposite -- you're enabling the regulator right before 
registering the platform device, so after registering generic PHY. It's 
therefore not at all clear why you've exchanged the calls above. I can't ACK 
this patch.



[...]

@@ -582,11 +690,20 @@ static int da8xx_remove(struct platform_device *pdev)
  return 0;
  }

+static const struct of_device_id da8xx_id_table[] = {
+{
+.compatible = "ti,da830-musb",
+},
+{},
+};
+MODULE_DEVICE_TABLE(of, da8xx_id_table);
+
  static struct platform_driver da8xx_driver = {
  .probe= da8xx_probe,
  .remove= da8xx_remove,
  .driver= {
  .name= "musb-da8xx",
+.of_match_table = of_match_ptr(da8xx_id_table),


   Doesn't this cause a warning about in non-DT case?


No, the of_match_table is defined independent of the CONFIG_OF.


   But of_match_ptr() depends on it, IIRC.


Regards
Petr


MBR, Sergei

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

Re: [PATCH 5/5 v8] usb: musb: da8xx: Add DT support for the DA8xx driver

2016-03-07 Thread Petr Kulhavy 

Hello

On 07.03.2016 14:29, Sergei Shtylyov wrote:

Hello.

On 3/7/2016 2:20 PM, Petr Kulhavy wrote:

@@ -544,17 +643,25 @@ static int da8xx_probe(struct platform_device 
*pdev)

  pinfo.data = pdata;
  pinfo.size_data = sizeof(*pdata);

+ret = regulator_enable(glue->vbus_supply);


   What does this achieve with a regulator that can't be explicitly 
controlled?




If this is a _real_ regulator, then it needs to be enabled by somebody, 
doesn't it?



+if (ret) {
+dev_err(>dev, "failed to enable power: %d\n", ret);
+goto err6;
+}
+
  glue->musb = musb = platform_device_register_full();
  if (IS_ERR(musb)) {
  ret = PTR_ERR(musb);
  dev_err(>dev, "failed to register musb device: %d\n", 
ret);

-goto err6;
+goto err7;
  }

  return 0;
+err7:
+usb_phy_generic_unregister(glue->phy);

  err6:
-usb_phy_generic_unregister(glue->phy);
+regulator_disable(glue->vbus_supply);


   How's that? Aren't you envaling Vbus regulator *after* registering 
PHY?



No, as you can see just a few lines above in the patch.


[...]
@@ -582,11 +690,20 @@ static int da8xx_remove(struct platform_device 
*pdev)

  return 0;
  }

+static const struct of_device_id da8xx_id_table[] = {
+{
+.compatible = "ti,da830-musb",
+},
+{},
+};
+MODULE_DEVICE_TABLE(of, da8xx_id_table);
+
  static struct platform_driver da8xx_driver = {
  .probe= da8xx_probe,
  .remove= da8xx_remove,
  .driver= {
  .name= "musb-da8xx",
+.of_match_table = of_match_ptr(da8xx_id_table),


   Doesn't this cause a warning about in non-DT case?


No, the of_match_table is defined independent of the CONFIG_OF.

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

Re: [PATCH 5/5 v8] usb: musb: da8xx: Add DT support for the DA8xx driver

2016-03-07 Thread Sergei Shtylyov 

Hello.

On 3/7/2016 2:20 PM, Petr Kulhavy wrote:


This adds DT support for TI DA8xx/OMAP-L1x/AM17xx/AM18xx MUSB driver

Signed-off-by: Petr Kulhavy 
Tested-by: Petr Kulhavy 
Acked-by: Sergei Shtylyov 

[...]


diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
index b03d3b8..bbd8cac 100644
--- a/drivers/usb/musb/da8xx.c
+++ b/drivers/usb/musb/da8xx.c

[...]

@@ -544,17 +643,25 @@ static int da8xx_probe(struct platform_device *pdev)
pinfo.data = pdata;
pinfo.size_data = sizeof(*pdata);

+   ret = regulator_enable(glue->vbus_supply);


   What does this achieve with a regulator that can't be explicitly controlled?


+   if (ret) {
+   dev_err(>dev, "failed to enable power: %d\n", ret);
+   goto err6;
+   }
+
glue->musb = musb = platform_device_register_full();
if (IS_ERR(musb)) {
ret = PTR_ERR(musb);
dev_err(>dev, "failed to register musb device: %d\n", 
ret);
-   goto err6;
+   goto err7;
}

return 0;
+err7:
+   usb_phy_generic_unregister(glue->phy);

  err6:
-   usb_phy_generic_unregister(glue->phy);
+   regulator_disable(glue->vbus_supply);


   How's that? Aren't you envaling Vbus regulator *after* registering PHY?

[...]

@@ -582,11 +690,20 @@ static int da8xx_remove(struct platform_device *pdev)
return 0;
  }

+static const struct of_device_id da8xx_id_table[] = {
+   {
+   .compatible = "ti,da830-musb",
+   },
+   {},
+};
+MODULE_DEVICE_TABLE(of, da8xx_id_table);
+
  static struct platform_driver da8xx_driver = {
.probe  = da8xx_probe,
.remove = da8xx_remove,
.driver = {
.name   = "musb-da8xx",
+   .of_match_table = of_match_ptr(da8xx_id_table),


   Doesn't this cause a warning about in non-DT case?

[...]

MBR, Sergei

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

[PATCH 5/5 v8] usb: musb: da8xx: Add DT support for the DA8xx driver

2016-03-07 Thread Petr Kulhavy 
This adds DT support for TI DA8xx/OMAP-L1x/AM17xx/AM18xx MUSB driver

Signed-off-by: Petr Kulhavy 
Tested-by: Petr Kulhavy 
Acked-by: Sergei Shtylyov 
---
v1: 

v2:
 - using standard MUSB properties "dr_mode", "mentor,power", "mentor,num-eps", 
"mentor,multipoint", "mentor,ram-bits"
 - using "ti," prefix instead of "da8xx," for specific property names
 - no wilcards in compatibility string
 - using CFGCHIP2_USB2PHYCLKMUX_SHIFT instead of CFGCHIP2_USB2PHYCLKMUX_OFFSET

v3:
 - DMA mask initialization corrected
 - removed extra #ifdef CONFIG_OF

v4:
 - compatibility string set to "ti,da830-musb"
 - "mentor,num-eps", "mentor,multipoint", "mentor,ram-bits" properties removed 
and hardcoded
 - "ti,phy20-clkmux-cfg" renamed to "ti,phy20-clkmux-pll" and changed to boolean
 - removed use of the DA8XX_SYSCFG0_VIRT macro

v5:
 - using CFGCHIP2_REFFREQ_ in get_phy_refclk_cfg()
 - simplified initialization of the hard coded config parameters
 - optimization CFGCHIP2 register update

v6:
 - using "ti,usb2-phy-" prefix instead of "ti,phy20-" for the specific 
properties
 - optimization CFGCHIP2 register update
 
v7:
 - pdata::power hard coded to 500mA

v8:
 - USB maximum power modelled via a regulator "vbus-supply"
 - "ti,usb2-phy-refclock-frequency" renamed to "ti,usb2-phy-refclock-hz"
 - "ti,usb2-phy-clkmux-pll" changed to "ti,usb2-phy-clkmux-refclkin" and the 
boolean meaning inverted to reflect the more common case

 drivers/usb/musb/da8xx.c | 121 ++-
 1 file changed, 119 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
index b03d3b8..bbd8cac 100644
--- a/drivers/usb/musb/da8xx.c
+++ b/drivers/usb/musb/da8xx.c
@@ -6,6 +6,9 @@
  * Based on the DaVinci "glue layer" code.
  * Copyright (C) 2005-2006 by Texas Instruments
  *
+ * DT support
+ * Copyright (c) 2016 Petr Kulhavy, Barix AG 
+ *
  * This file is part of the Inventra Controller Driver for Linux.
  *
  * The Inventra Controller Driver for Linux is free software; you
@@ -33,9 +36,11 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
+#include 
 
 #include "musb_core.h"
 
@@ -87,6 +92,7 @@ struct da8xx_glue {
struct platform_device  *musb;
struct platform_device  *phy;
struct clk  *clk;
+   struct regulator*vbus_supply;
 };
 
 /*
@@ -134,6 +140,48 @@ static inline void phy_off(void)
__raw_writel(cfgchip2, CFGCHIP2);
 }
 
+static inline int get_phy_refclk_cfg(struct device_node *np)
+{
+   u32 freq;
+
+   if (of_property_read_u32(np, "ti,usb2-phy-refclock-hz", ))
+   return -EINVAL;
+
+   switch (freq) {
+   case 1200:
+   return CFGCHIP2_REFFREQ_12MHZ;
+   case 1300:
+   return CFGCHIP2_REFFREQ_13MHZ;
+   case 1920:
+   return CFGCHIP2_REFFREQ_19_2MHZ;
+   case 2000:
+   return CFGCHIP2_REFFREQ_20MHZ;
+   case 2400:
+   return CFGCHIP2_REFFREQ_24MHZ;
+   case 2600:
+   return CFGCHIP2_REFFREQ_26MHZ;
+   case 3840:
+   return CFGCHIP2_REFFREQ_38_4MHZ;
+   case 4000:
+   return CFGCHIP2_REFFREQ_40MHZ;
+   case 4800:
+   return CFGCHIP2_REFFREQ_48MHZ;
+   default:
+   return -EINVAL;
+   }
+}
+
+static inline u8 get_vbus_power(struct regulator *reg)
+{
+   int current_uA;
+
+   current_uA = regulator_get_current_limit(reg);
+   if (current_uA <= 0 || current_uA > 51)
+   return 255;
+
+   return current_uA / 1000;
+}
+
 /*
  * Because we don't set CTRL.UINT, it's "important" to:
  * - not read/write INTRUSB/INTRUSBE (except during
@@ -482,6 +530,12 @@ static const struct platform_device_info da8xx_dev_info = {
.dma_mask   = DMA_BIT_MASK(32),
 };
 
+static const struct musb_hdrc_config da8xx_config = {
+   .ram_bits = 10,
+   .num_eps = 5,
+   .multipoint = 1,
+};
+
 static int da8xx_probe(struct platform_device *pdev)
 {
struct resource musb_resources[2];
@@ -490,6 +544,7 @@ static int da8xx_probe(struct platform_device *pdev)
struct da8xx_glue   *glue;
struct platform_device_info pinfo;
struct clk  *clk;
+   struct device_node  *np = pdev->dev.of_node;
 
int ret = -ENOMEM;
 
@@ -515,6 +570,50 @@ static int da8xx_probe(struct platform_device *pdev)
glue->dev   = >dev;
glue->clk   = clk;
 
+   if (IS_ENABLED(CONFIG_OF) && np) {
+   int refclk_cfg;
+   u32 cfgchip2;
+
+   pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
+   if (!pdata) {
+   ret = -ENOMEM;
+   goto err5;
+