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

[Sergei Shtylyov <sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>]

On 2/8/2016 12:19 PM, Petr Kulhavy 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.

    He has submitted a patch to do that.

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

    And the patch is against 3.17? You should only submit patches against the 
recent kernel. In this case, against the -next branch of Felipe's repo on 
kernel.org.

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

    The CFGCHIP2 register controls the PHY in practice. The code manipulating 
it should be moved to a PHY driver.

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

    OK, just thought I'd let you know.

>>> 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
>>> +~~~~~~~~~~~~~
>>> +
>>> +Rquired 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)...

    So I'd suggest "ti,da830-musb".

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

    Wildcards are not allowed in the "compatible" prop. People here told you 
the same.

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

    Even twice, in omap2430.c and musb_dsps.c. omap2430.c even parses 
non-prefixed props, ew...

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

    I'd prefer doing it that way, sure.

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

    Refering me to the AM18xx TRM when I asked for just correctly naming the 
SI unit is strange. :-)

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

    Define this function in musb_core.c.

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

    musb_core.c again.

>>
>> [...]
>>> +        /* 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.

    Why are you not using CFGCHIP2 macro in this file as the rest of the code 
does?

> Regards
> Petr

MBR, Sergei

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