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

[Sergei Shtylyov <sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.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 <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)...

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