From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sergei Shtylyov Subject: Re: [PATCH 1/1] Drivers: USB: DA8xx MUSB: added DT support Date: Sun, 7 Feb 2016 01:27:29 +0300 Message-ID: <56B67351.1030604@cogentembedded.com> References: <1454693676-20211-1-git-send-email-petr@barix.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1454693676-20211-1-git-send-email-petr-Qh/3xLP0EvwAvxtiuMwx3w@public.gmane.org> Sender: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Petr Kulhavy , Felipe Balbi , robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: devicetree@vger.kernel.org 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 > --- > .../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 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