| Message ID | 1454693676-20211-1-git-send-email-petr@barix.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Friday 05 February 2016 18:34:36 Petr Kulhavy wrote: > TI DA8xx MUSB driver equipped with DeviceTree support. > Tested with AM1808 board and USB2.0 (OTG) in host mode. > > Signed-off-by: Petr Kulhavy <petr@barix.com> > --- > .../devicetree/bindings/usb/da8xx-usb.txt | 63 ++++++++ > drivers/usb/musb/da8xx.c | 162 +++++++++++++++++++++ > include/linux/platform_data/usb-davinci.h | 3 +- The changes look good, thanks. One more thing: > 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" > + Please make this Should be one of "ti,da850-musb" or "ti,da830-musb" We don't use wildcards in compatible strings, so better use specific model names in case we later find something that is different betweent the models. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5 February 2016 at 23:22, Arnd Bergmann <arnd@arndb.de> wrote: > One more thing: > >> 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" >> + > > Please make this > > Should be one of "ti,da850-musb" or "ti,da830-musb" > > We don't use wildcards in compatible strings, so better use specific model > names in case we later find something that is different betweent the models. OK, I will update the binding. But is the MODULE_DEVICE_TABLE() in the code able to handle wildcards? Or do I need to create 2 entries in the code? I mean something like this: static const struct of_device_id da8xx_id_table[] = { { .compatible = "ti,da850-musb" }, { .compatible = "ti,da830-musb" }, {}, }; MODULE_DEVICE_TABLE(of, da8xx_id_table); Thanks Petr -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 02/06/2016 02:55 AM, Petr Kulhavy wrote: >> One more thing: >> >>> 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" >>> + >> >> Please make this >> >> Should be one of "ti,da850-musb" or "ti,da830-musb" >> >> We don't use wildcards in compatible strings, so better use specific model >> names in case we later find something that is different betweent the models. > OK, I will update the binding. But is the MODULE_DEVICE_TABLE() in the > code able to handle wildcards? No! > Or do I need to create 2 entries in the code? I mean something like this: You don't -- unless you'll find something different between DMA8x0/AM1x0x. If not, "ti,da830-musb" will do for all SoCs. > static const struct of_device_id da8xx_id_table[] = { > { > .compatible = "ti,da850-musb" > }, > { > .compatible = "ti,da830-musb" > }, > {}, > }; > > MODULE_DEVICE_TABLE(of, da8xx_id_table); > > Thanks > Petr MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello.
[Changed Felipe's address to the kernel.org one, so thta he can read this
too. :-)]
On 02/05/2016 08:34 PM, Petr Kulhavy wrote:
> 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? Felipe wants the PHY
specific parts moved to a PHY driver...
> Signed-off-by: Petr Kulhavy <petr@barix.com>
> ---
> .../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)...
> +
> + - reg: offset and length of the usbss register sets
Sets?! How many?! (If more than one, you better have "reg-names" as well.)
> +
> + - 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...
> +
> + - mentor,multipoint : Valid only in host mode. Enables addressing of USB hubs,
> + which is normally something you want and therefore should be set to "1".
> + If set to "0" only point-to-point communication is enabled, i.e. only a single
> + device can be attached.
Same here.
> +
> + - mentor,ram-bits : This 2-base logarithm value defines the RAM FIFO size of the controller.
> + The FIFO size is calculated in 32-bit words. E.g. if your controller has 4kiB of RAM FIFO
> + this value should be set to "10": 2^10 = 1024 words of 32-bits, i.e. 4096 bytes.
> + For the DA8xx controller this value should be set to 10.
And here.
> +
> +
> +Optional properties:
> +
> + - ti,phy20-clkmux-cfg: Integer. Defines the USB 2.0 PHY reference clock source.
> + Supported values: "0" for external pin, "1" for internal PLL.
Boolean prop looks more promising in this case.
> +
> + - 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...
> +
> +
> +Example:
> +
> + usb20: usb@1e00000 {
> + compatible = "ti,da8xx-musb";
> + ti,hwmods = "usb_otg_hs";
We need this on AM1808 too now?
[...]
> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
> index 4e13fe2..281d503 100644
> --- a/drivers/usb/musb/da8xx.c
> +++ b/drivers/usb/musb/da8xx.c
[...]
> @@ -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?
[...]
> @@ -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.
[...]
> @@ -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...
[...]
> + /* 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/.
> + /* optional parameter reference clock frequency */
> + if (!of_property_read_u32(np, "ti,phy20-refclock-frequency",
> + &phy20_refclock_freq)) {
> + u32 cfgchip2;
> + int phy20_refclk_cfg;
> +
> + phy20_refclk_cfg = phy_refclk_cfg(phy20_refclock_freq);
> + if (phy20_refclk_cfg < 0) {
> + dev_err(&pdev->dev,
> + "invalid PHY clock frequency %u Hz\n",
> + phy20_refclock_freq);
> + ret = -EINVAL;
> + goto err5;
> + }
> +
> + /*
> + * Set up USB clock/mode in the CFGCHIP2 register.
> + */
> + cfgchip2 = __raw_readl(
> + DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
> +
> + /* USB2.0 PHY reference clock is 24 MHz */
You've just read this freq. out of DT, why this comment?
> + cfgchip2 &= ~CFGCHIP2_REFFREQ;
> + cfgchip2 |= phy20_refclk_cfg;
> +
> + __raw_writel(cfgchip2,
> + DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
> + }
> + }
> +
> pdata->platform_ops = &da8xx_ops;
>
> glue->phy = usb_phy_generic_register();
[...]
MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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@barix.com> >> --- >> .../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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@barix.com> >>> --- >>> .../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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2/5/2016 8:34 PM, Petr Kulhavy wrote: > TI DA8xx MUSB driver equipped with DeviceTree support. > Tested with AM1808 board and USB2.0 (OTG) in host mode. > > Signed-off-by: Petr Kulhavy <petr@barix.com> > --- > .../devicetree/bindings/usb/da8xx-usb.txt | 63 ++++++++ BTW Documentation/devicetree/bindings/submitting-patches.txt tells to submit the bindings in a separate patch. > 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 [...] MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Sergei, On 08.02.2016 12:20, Sergei Shtylyov wrote: > 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. > Could you give me the full branch name please. > >> 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. > I'm not sure if I can do that at the moment. Would you accept the patch using the current situation, i.e. the CFGCHIP2 being manipulated in the da8xx.c? >>> >>> 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". Thanks. I've just checked the da830 doc and I don't see any difference in the USB register sets. So I will update the name as you suggest. >> 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. OK. So I will move the num_eps and ram_bits into the "compatible" structure. And leave the mode, power and multipoint still configurable via DT as they depend on the hardware wiring. I believe someone still might want to set multipoint to 0 if he has a single device (not a hub) hard-connected directly to the controller. Even if I don't see a benefit of doing so. Why does this parameter exist at all? [...] >>> [...] >>>> @@ -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. > I will do. Why doesn't the musb core use directly the USB_DR_MODE_xxx literals instead? I don't see any benefit of defining them again and translating. > >>> >>> [...] >>>> + /* 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? I've just noticed that too. I will use the CFGCHIP2 macro. Regards Petr -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2/8/2016 2:48 PM, Petr Kulhavy wrote: >> 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. > > Could you give me the full branch name please. See the MAINTAINERS entry for drivers/usb/musb/. The branch is named just 'next'. >>> 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. >> > I'm not sure if I can do that at the moment. Would you accept the patch using > the current situation, i.e. the CFGCHIP2 being manipulated in the da8xx.c? Felipe accepts MUSB changes, not me. I'll ACK the patch once it's in good shape, w/o the PHY driver. [...] >>> 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. > OK. So I will move the num_eps and ram_bits into the "compatible" structure. > And leave the mode, power and multipoint still configurable via DT as they > depend on the hardware wiring. As for the mode and power, I agree -- they are part of the board-specific platform data (there's also power-on-to-power-good delay BTW which you missed). > I believe someone still might want to set multipoint to 0 if he has a single > device (not a hub) hard-connected directly to the controller. Even if I don't > see a benefit of doing so. > Why does this parameter exist at all? No, multipoint is a part of the 'struct musb_hdrc_config' and IIRC just indicates whether the target endpoint control registers are present. >>>>> @@ -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. >> > I will do. Why doesn't the musb core use directly the USB_DR_MODE_xxx literals > instead? MUSB driver predates these (DT specific?) definitions IIRC... [...] > Regards > Petr MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08.02.2016 13:25, Sergei Shtylyov wrote: > > As for the mode and power, I agree -- they are part of the > board-specific platform data (there's also power-on-to-power-good > delay BTW which you missed). > I've also noticed the potpgt it in the platform driver. However the musb doesn't seem to use it, it's used by the ohci-da8xx driver.I will do. Why doesn't the musb core use directly the USB_DR_MODE_xxx literals >> instead? > > MUSB driver predates these (DT specific?) definitions IIRC... Would it make sense to replace it with the newer USB_DR_MODE_xxx ? Petr -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Arndt, On 05.02.2016 23:22, Arnd Bergmann wrote: > Please make this > > Should be one of "ti,da850-musb" or "ti,da830-musb" > > We don't use wildcards in compatible strings, I'm seeing something different though: $ grep -r "compatible.*xx" Documentation/devicetree/bindings/ |wc -l 44 > so better use specific model > names in case we later find something that is different betweent the models. OK, thanks. Regards Petr -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 08 February 2016 14:04:22 Petr Kulhavy wrote: > Hello Arndt, > > On 05.02.2016 23:22, Arnd Bergmann wrote: > > Please make this > > > > Should be one of "ti,da850-musb" or "ti,da830-musb" > > > > We don't use wildcards in compatible strings, > > I'm seeing something different though: > > $ grep -r "compatible.*xx" Documentation/devicetree/bindings/ |wc -l > 44 They are all either really old, or were never reviewed properly. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/08/2016 03:49 PM, Petr Kulhavy wrote: >> As for the mode and power, I agree -- they are part of the board-specific >> platform data (there's also power-on-to-power-good delay BTW which you missed). >> > I've also noticed the potpgt it in the platform driver. However the musb > doesn't seem to use it, it's used by the ohci-da8xx driver.I will do. Indeed, it's not used and that probably should be fixed... >>> Why doesn't the musb core use directly the USB_DR_MODE_xxx literals >>> instead? >> >> MUSB driver predates these (DT specific?) definitions IIRC... >> > Would it make sense to replace it with the newer USB_DR_MODE_xxx ? Well, if you want to take on this task, ask Felipe, he's the maintainer... > Petr MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Sergei, I have prepared the patch as per you comments. Unfortunately I cannot use the 4.5 kernel from Felipe's repo as the of_usb_get_dr_mode() has been replaced with of_usb_get_dr_mode_by_phy() which requires a PHY node, which does not exist for DA8xx as we discussed before. What shall we do? Regards Petr On 08.02.2016 13:25, Sergei Shtylyov wrote: > On 2/8/2016 2:48 PM, Petr Kulhavy wrote: > >>> 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. >> >> Could you give me the full branch name please. > > See the MAINTAINERS entry for drivers/usb/musb/. The branch is > named just 'next'. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/08/2016 06:32 PM, Petr Kulhavy wrote: > I have prepared the patch as per you comments. > Unfortunately I cannot use the 4.5 kernel from Felipe's repo as the > of_usb_get_dr_mode() has been replaced with > of_usb_get_dr_mode_by_phy() which requires a PHY node, which does not exist > for DA8xx as we discussed before. > > What shall we do? Doesn't usb_get_dr_mode() work for you? > Regards > Petr MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 05, 2016 at 06:34:36PM +0100, Petr Kulhavy wrote: > TI DA8xx MUSB driver equipped with DeviceTree support. > Tested with AM1808 board and USB2.0 (OTG) in host mode. > > Signed-off-by: Petr Kulhavy <petr@barix.com> > --- > .../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" > + > + - reg: offset and length of the usbss register sets > + > + - interrupts: USB interrupt number > + > + - interrupt-names: should be set to "mc" Kind of pointless with only one. > + - 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". If DA8xx is the only thing you support, then why is this needed? > + - mentor,multipoint : Valid only in host mode. Enables addressing of USB hubs, > + which is normally something you want and therefore should be set to "1". > + If set to "0" only point-to-point communication is enabled, i.e. only a single > + device can be attached. Given 1 is a superset of 0, why ever use 0? > + - mentor,ram-bits : This 2-base logarithm value defines the RAM FIFO size of the controller. > + The FIFO size is calculated in 32-bit words. E.g. if your controller has 4kiB of RAM FIFO > + this value should be set to "10": 2^10 = 1024 words of 32-bits, i.e. 4096 bytes. > + For the DA8xx controller this value should be set to 10. Again, if fixed for the compatible string, then don't put into the DT. > + > + > +Optional properties: > + > + - ti,phy20-clkmux-cfg: Integer. Defines the USB 2.0 PHY reference clock source. > + Supported values: "0" for external pin, "1" for internal PLL. > + > + - 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 Add units suffix (-hz). -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/08/2016 10:49 PM, Rob Herring wrote: >> TI DA8xx MUSB driver equipped with DeviceTree support. >> Tested with AM1808 board and USB2.0 (OTG) in host mode. >> >> Signed-off-by: Petr Kulhavy <petr@barix.com> >> --- >> .../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" >> + >> + - reg: offset and length of the usbss register sets >> + >> + - interrupts: USB interrupt number >> + >> + - interrupt-names: should be set to "mc" > > Kind of pointless with only one. Required by the driver. >> + - 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". > > If DA8xx is the only thing you support, then why is this needed? We already agreed that it should be deduced from the "compatible" prop. Unfortunately, the author had bad examples in omap2430.c and musb_dsps.c... :-( >> + - mentor,multipoint : Valid only in host mode. Enables addressing of USB hubs, >> + which is normally something you want and therefore should be set to "1". >> + If set to "0" only point-to-point communication is enabled, i.e. only a single >> + device can be attached. This is author's fantasies, not the real meaning. :-) > > Given 1 is a superset of 0, why ever use 0? Already this way in the exsiting MUSB bindings. Actually 0/1 indicates that the target endpoint control regs are absent/present. >> + - mentor,ram-bits : This 2-base logarithm value defines the RAM FIFO size of the controller. >> + The FIFO size is calculated in 32-bit words. E.g. if your controller has 4kiB of RAM FIFO >> + this value should be set to "10": 2^10 = 1024 words of 32-bits, i.e. 4096 bytes. >> + For the DA8xx controller this value should be set to 10. > > Again, if fixed for the compatible string, then don't put into the DT. Fully agreed. Bad examples exist, though... >> + >> + >> +Optional properties: >> + >> + - ti,phy20-clkmux-cfg: Integer. Defines the USB 2.0 PHY reference clock source. >> + Supported values: "0" for external pin, "1" for internal PLL. >> + >> + - 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 > > Add units suffix (-hz). Doesn't "frequency" alteady tell that? MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08.02.2016 18:55, Sergei Shtylyov wrote: > On 02/08/2016 06:32 PM, Petr Kulhavy wrote: > >> I have prepared the patch as per you comments. >> Unfortunately I cannot use the 4.5 kernel from Felipe's repo as the >> of_usb_get_dr_mode() has been replaced with >> of_usb_get_dr_mode_by_phy() which requires a PHY node, which does not >> exist >> for DA8xx as we discussed before. >> >> What shall we do? > > Doesn't usb_get_dr_mode() work for you? I can't really say because I can't test it. Is this the replacement for of_usb_get_dr_mode() ? Thanks Petr -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2/9/2016 11:30 AM, Petr Kulhavy wrote: >>> I have prepared the patch as per you comments. >>> Unfortunately I cannot use the 4.5 kernel from Felipe's repo as the >>> of_usb_get_dr_mode() has been replaced with >>> of_usb_get_dr_mode_by_phy() which requires a PHY node, which does not exist >>> for DA8xx as we discussed before. >>> >>> What shall we do? >> >> Doesn't usb_get_dr_mode() work for you? > > I can't really say because I can't test it. Is this the replacement for > of_usb_get_dr_mode() ? Seems so. The call in musb_dsps.c was just replaced. > Thanks > Petr MNR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09.02.2016 11:50, Sergei Shtylyov wrote: > >>> Doesn't usb_get_dr_mode() work for you? >> >> I can't really say because I can't test it. Is this the replacement for >> of_usb_get_dr_mode() ? > > Seems so. The call in musb_dsps.c was just replaced. Thanks, Sergei. Yesterday I submitted the (hopefully) final version version of the patch based on Felipe's "next" branch and incorporating all you guys' feedback. Also the binding was submitted separately. Please let me know if anything else needs to be amended. Thanks Petr -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2/10/2016 2:26 PM, Petr Kulhavy wrote: >>>> Doesn't usb_get_dr_mode() work for you? >>> >>> I can't really say because I can't test it. Is this the replacement for >>> of_usb_get_dr_mode() ? >> >> Seems so. The call in musb_dsps.c was just replaced. > > Thanks, Sergei. Yesterday I submitted the (hopefully) final version version Hope dies last. :-) There's too much cut&pasted code in your patch still... > of the patch based > on Felipe's "next" branch and incorporating all you guys' feedback. Also the > binding was submitted separately. I'll go find it, haven't seen yet. > Please let me know if anything else needs to be amended. I've just reviewed the glue patch. > Thanks > Petr MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2/10/2016 5:07 PM, Sergei Shtylyov wrote: [...] >> Thanks, Sergei. Yesterday I submitted the (hopefully) final version version > > Hope dies last. :-) > There's too much cut&pasted code in your patch still... >> of the patch based >> on Felipe's "next" branch and incorporating all you guys' feedback. Also the >> binding was submitted separately. > > I'll go find it, haven't seen yet. I just haven't received it. Please make sure to CC at least linux-usb next time you post it. >> Thanks >> Petr MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10.02.2016 15:12, Sergei Shtylyov wrote: > of the patch based >>> on Felipe's "next" branch and incorporating all you guys' feedback. >>> Also the >>> binding was submitted separately. >> >> I'll go find it, haven't seen yet. > > I just haven't received it. Please make sure to CC at least > linux-usb next time you post it. > I've just followed the instructions in Documentation/devicetree/bindings/submitting-patches.txt as you hinted me last time. It doesn't say the subsystem mailing list should be CCed as well. Neither does the get_maintainer.pl print the linux-usb list. I'm sorry! :-7 But sure, next time I'll copy the list as well. Petr -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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" + + - reg: offset and length of the usbss register sets + + - 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". + + - mentor,multipoint : Valid only in host mode. Enables addressing of USB hubs, + which is normally something you want and therefore should be set to "1". + If set to "0" only point-to-point communication is enabled, i.e. only a single + device can be attached. + + - mentor,ram-bits : This 2-base logarithm value defines the RAM FIFO size of the controller. + The FIFO size is calculated in 32-bit words. E.g. if your controller has 4kiB of RAM FIFO + this value should be set to "10": 2^10 = 1024 words of 32-bits, i.e. 4096 bytes. + For the DA8xx controller this value should be set to 10. + + +Optional properties: + + - ti,phy20-clkmux-cfg: Integer. Defines the USB 2.0 PHY reference clock source. + Supported values: "0" for external pin, "1" for internal PLL. + + - 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 + + +Example: + + usb20: usb@1e00000 { + compatible = "ti,da8xx-musb"; + ti,hwmods = "usb_otg_hs"; + reg = <0x00200000 0x10000>; + interrupt-parent = <&intc>; + interrupts = <58>; + interrupt-names = "mc"; + + dr_mode = "host"; + mentor,power = <500>; + mentor,multipoint = <1>; + mentor,num-eps = <5>; + mentor,ram-bits = <10>; + + ti,phy20-clkmux-cfg = <1>; /* PHY clock internally generated from the PLL */ + ti,phy20-refclock-frequency = <24000000>; + + status = "okay"; + }; diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c index 4e13fe2..281d503 100644 --- a/drivers/usb/musb/da8xx.c +++ b/drivers/usb/musb/da8xx.c @@ -1,6 +1,9 @@ /* * Texas Instruments DA8xx/OMAP-L1x "glue layer" * + * DT support + * Copyright (c) 2015-2016 Petr Kulhavy, Barix AG <petr@barix.com> + * * Copyright (c) 2008-2009 MontaVista Software, Inc. <source@mvista.com> * * Based on the DaVinci "glue layer" code. @@ -36,6 +39,8 @@ #include <mach/da8xx.h> #include <linux/platform_data/usb-davinci.h> +#include <linux/of_platform.h> +#include <linux/usb/of.h> #include "musb_core.h" @@ -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 + * on unsupported frequency returns -1 + */ +static inline int phy_refclk_cfg(int frequency) +{ + switch (frequency) { + case 12000000: + return 0x01; + case 13000000: + return 0x06; + case 19200000: + return 0x05; + case 20000000: + return 0x08; + case 24000000: + return 0x02; + case 26000000: + return 0x07; + case 38400000: + return 0x05; + case 40000000: + return 0x09; + case 48000000: + return 0x03; + default: + return -1; + } +} + /* * Because we don't set CTRL.UINT, it's "important" to: * - not read/write INTRUSB/INTRUSBE (except during @@ -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; + } +} + +static int get_int_prop(struct device_node *dn, const char *s) +{ + int ret; + u32 val; + + ret = of_property_read_u32(dn, s, &val); + return ret ? 0 : val; +} + static int da8xx_probe(struct platform_device *pdev) { struct resource musb_resources[2]; @@ -535,6 +598,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; @@ -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 = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); + if (!pdata) { + ret = -ENOMEM; + goto err5; + } + + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); + if (!data) { + ret = -ENOMEM; + goto err5; + } + + config = devm_kzalloc(&pdev->dev, sizeof(*config), GFP_KERNEL); + if (!config) { + ret = -ENOMEM; + goto err5; + } + + 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"); + + pdata->board_data = data; + pdata->config = config; + + /* 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)); + + cfgchip2 &= ~CFGCHIP2_USB2PHYCLKMUX; + cfgchip2 |= (phy20_clkmux_cfg & 1) << + CFGCHIP2_USB2PHYCLKMUX_SHIFT; + + __raw_writel(cfgchip2, + DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG)); + + } + + /* optional parameter reference clock frequency */ + if (!of_property_read_u32(np, "ti,phy20-refclock-frequency", + &phy20_refclock_freq)) { + u32 cfgchip2; + int phy20_refclk_cfg; + + phy20_refclk_cfg = phy_refclk_cfg(phy20_refclock_freq); + if (phy20_refclk_cfg < 0) { + dev_err(&pdev->dev, + "invalid PHY clock frequency %u Hz\n", + phy20_refclock_freq); + ret = -EINVAL; + goto err5; + } + + /* + * Set up USB clock/mode in the CFGCHIP2 register. + */ + cfgchip2 = __raw_readl( + DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG)); + + /* USB2.0 PHY reference clock is 24 MHz */ + cfgchip2 &= ~CFGCHIP2_REFFREQ; + cfgchip2 |= phy20_refclk_cfg; + + __raw_writel(cfgchip2, + DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG)); + } + } + pdata->platform_ops = &da8xx_ops; glue->phy = usb_phy_generic_register(); @@ -627,11 +780,20 @@ static int da8xx_remove(struct platform_device *pdev) return 0; } +static const struct of_device_id da8xx_id_table[] = { + { + .compatible = "ti,da8xx-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), }, }; diff --git a/include/linux/platform_data/usb-davinci.h b/include/linux/platform_data/usb-davinci.h index e0bc4ab..0948ce5 100644 --- a/include/linux/platform_data/usb-davinci.h +++ b/include/linux/platform_data/usb-davinci.h @@ -21,7 +21,8 @@ #define CFGCHIP2_FORCE_DEVICE (2 << 13) #define CFGCHIP2_FORCE_HOST_VBUS_LOW (3 << 13) #define CFGCHIP2_USB1PHYCLKMUX (1 << 12) -#define CFGCHIP2_USB2PHYCLKMUX (1 << 11) +#define CFGCHIP2_USB2PHYCLKMUX_SHIFT (11) +#define CFGCHIP2_USB2PHYCLKMUX (1 << CFGCHIP2_USB2PHYCLKMUX_SHIFT) #define CFGCHIP2_PHYPWRDN (1 << 10) #define CFGCHIP2_OTGPWRDN (1 << 9) #define CFGCHIP2_DATPOL (1 << 8)
TI DA8xx MUSB driver equipped with DeviceTree support. Tested with AM1808 board and USB2.0 (OTG) in host mode. Signed-off-by: Petr Kulhavy <petr@barix.com> --- .../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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html