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

[Petr Kulhavy <petr-Qh/3xLP0EvwAvxtiuMwx3w@public.gmane.org>]

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