Re: [PATCH 1/1] Drivers: USB: DA8xx MUSB: added DT support

Hello,

On 06.02.2016 23:27, Sergei Shtylyov wrote:

   [Changed Felipe's address to the kernel.org one, so thta he can 
read this too. :-)] 

He should also update the MAINTAINERS file with his new address.

quoted

TI DA8xx MUSB driver equipped with DeviceTree support.
Tested with AM1808 board and USB2.0 (OTG) in host mode.

   Have you notices "depends on BROKEN" in Kconfig, BTW? 

Honestly, I don't see any "depends on BROKEN". However I do build with 
the 3.17 kernel.

Felipe wants the PHY specific parts moved to a PHY driver...

I was wondering about a PHY driver too. However the AM1808 has no 
standalone PHY module like e.g. the AM335x does. The PHY together with 
the USB core are integrated into a single block and there is only a 
little control through the SYSCFG registers.
And that change has nothing to do with DT support - it's a structural 
change of the da8xx USB and platform drivers.
That's why I didn't go for that.

quoted

Signed-off-by: Petr Kulhavy <petr-Qh/3xLP0EvwAvxtiuMwx3w@public.gmane.org>
---
  .../devicetree/bindings/usb/da8xx-usb.txt          |  63 ++++++++
  drivers/usb/musb/da8xx.c                           | 162 
+++++++++++++++++++++
  include/linux/platform_data/usb-davinci.h          |   3 +-
  3 files changed, 227 insertions(+), 1 deletion(-)
  create mode 100644 Documentation/devicetree/bindings/usb/da8xx-usb.txt

diff --git a/Documentation/devicetree/bindings/usb/da8xx-usb.txt 

b/Documentation/devicetree/bindings/usb/da8xx-usb.txt
new file mode 100644
index 0000000..69c0961

--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/da8xx-usb.txt

@@ -0,0 +1,63 @@
+TI DA8xx MUSB
+~~~~~~~~~~~~~
+
+Required properties:
+~~~~~~~~~~~~~~~~~~~~
+ - compatible : Should be "ti,da8xx-musb"

   No way. You'l probably need to select a lowest common denominator 
model. I don't remember offhand but I don't think DA850 (AM1808) is 
different from DA830 (AM170x)...

I just did what the guys here on the mailing list told me. I'm getting a 
bit confused now what is the right approach then...

quoted

+
+ - interrupts: USB interrupt number
+
+ - interrupt-names: should be set to "mc"
+
+ - dr_mode: The USB operation mode. Should be one of "host", 
"peripheral" or "otg".
+
+ - mentor,power : Specifies the maximum current in milli-ampers the 
controller can
+     supply in host mode. The maximum configurable value is 510mA.
+
+ - mentor,num-eps : Valid only in host mode. Specifies the number of 
target endpoints
+     supported by the controller. For DA8xx it is "5".

   That number should actually be deducible from the "compatible" 
prop. I'm not sure it's a good idea to specify this in DT... although 
TI have done this once already, for OMAPs...

All the other MUSBs specify these parameters in the DT. Do you want to 
make an exception?

quoted

+
+ - ti,phy20-refclock-frequency : Integer. Defines the USB 2.0 PHY 
reference clock input
+     frequency in Hz in case the clock is generated by the internal 
PLL.
+     Supported values are 12MHz, 13MHz, 19.2MHz, 20MHz, 24MHz, 
26MHz, 38.4MHz, 40MHz, 48MHz

   Ugh. At least these both are something board specific indeed...

See above.

quoted

@@ -134,6 +139,35 @@ static inline void phy_off(void)
      __raw_writel(cfgchip2, CFGCHIP2);
  }

+/* converts PHY refclk frequency in HZ into USB0REF_FREQ config value

   It's Hz, no?

Nope, see the spruh82a.pdf

[...]

quoted

@@ -527,6 +561,35 @@ static const struct platform_device_info 

da8xx_dev_info = {
      .dma_mask    = DMA_BIT_MASK(32),
  };

+static int get_musb_port_mode(struct device_node *np)
+{
+    enum usb_dr_mode mode;
+
+    mode = of_usb_get_dr_mode(np);
+    switch (mode) {
+    case USB_DR_MODE_HOST:
+        return MUSB_HOST;
+
+    case USB_DR_MODE_PERIPHERAL:
+        return MUSB_PERIPHERAL;
+
+    case USB_DR_MODE_OTG:
+        return MUSB_OTG;
+
+    default:
+        return MUSB_UNDEFINED;
+    }
+}

   This is clearly MUSB generic code.

So what does it mean?

[...]

quoted

@@ -560,6 +624,95 @@ static int da8xx_probe(struct platform_device 

*pdev)
      glue->dev            = &pdev->dev;
      glue->clk            = clk;

+    if (IS_ENABLED(CONFIG_OF) && np) {
+        struct musb_hdrc_config *config;
+        struct musb_hdrc_platform_data *data;
+        u32 phy20_refclock_freq, phy20_clkmux_cfg;

[...]

quoted

+        pdata->mode = get_musb_port_mode(np);
+        config->num_eps = get_int_prop(np, "mentor,num-eps");
+        config->ram_bits = get_int_prop(np, "mentor,ram-bits");
+        /* the "mentor,power" value is in milli-amps, whereas
+         * the mentor configuration is in 2mA units
+         */
+        pdata->power = get_int_prop(np, "mentor,power") / 2;
+        config->multipoint = of_property_read_bool(np,
+            "mentor,multipoint");

   These props are MUSB generic. Parsing them in each glue separately 
doesn't make much sense...

What do you suggest then?

[...]

quoted

+        /* optional parameter reference clock frequency */
+        if (!of_property_read_u32(np, "ti,phy20-clkmux-cfg",
+            &phy20_clkmux_cfg)) {
+            u32 cfgchip2;
+
+            /*
+             * Select internal reference clock for USB 2.0 PHY
+             * and use it as a clock source for USB 1.1 PHY
+             * (this is the default setting anyway).
+             */
+
+            cfgchip2 = __raw_readl(
+                DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));

   That's why a PHY driver is needed. DA8XX_SYSCFG2_VIRT() shouldn't 
be used outside arch/arm/mach-davinci/.

See above.

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