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

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

 




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.

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.

Signed-off-by: Petr Kulhavy <petr@xxxxxxxxx>
---
  .../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...
+
+ - 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?

+
+ - 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.

@@ -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

[...]
@@ -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?

[...]
@@ -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;
[...]
+        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?


[...]
+        /* 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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux