Re: [PATCH 5/6] powerpc: add USB peripheral support to MPC836xMDS

[Anton Vorontsov <avorontsov@ru.mvista.com>]

Hello Li,

On Wed, Aug 06, 2008 at 03:04:44PM +0800, Li Yang wrote:
> Signed-off-by: Li Yang <leoli@freescale.com>
> ---
>  arch/powerpc/boot/dts/mpc836x_mds.dts     |   15 ++++++-
>  arch/powerpc/platforms/83xx/mpc836x_mds.c |   19 ++++++++-
>  arch/powerpc/platforms/83xx/mpc83xx.h     |    1 +
>  arch/powerpc/platforms/83xx/usb.c         |   67 +++++++++++++++++++++++++++++
>  4 files changed, 100 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/boot/dts/mpc836x_mds.dts b/arch/powerpc/boot/dts/mpc836x_mds.dts
> index a3b76a7..596377b 100644
> --- a/arch/powerpc/boot/dts/mpc836x_mds.dts
> +++ b/arch/powerpc/boot/dts/mpc836x_mds.dts
> @@ -235,6 +235,17 @@
>  					0  2  1  0  1  0>; /* MDC */
>  			};
>  
> +			pio_usb: usb_pin@01 {
> +				pio-map = <
> +			/* port  pin  dir  open_drain  assignment  has_irq */
> +					1  2  1  0  3  0        /* USBOE  */
> +					1  3  1  0  3  0        /* USBTP  */
> +					1  8  1  0  1  0        /* USBTN  */
> +					1 10  2  0  3  0        /* USBRXD */
> +					1  9  2  1  3  0        /* USBRP  */
> +					1 11  2  1  3  0>;      /* USBRN  */
> +			};
> +
>  		};
>  	};
>  
> @@ -280,11 +291,13 @@
>  		};
>  
>  		usb@6c0 {
> -			compatible = "qe_udc";
> +			compatible = "fsl,qe_udc";

You might want to reuse existing bindings as described in
Documentation/powerpc/dts-bindings/fsl/cpm_qe/qe/usb.txt.

>  			reg = <0x6c0 0x40 0x8b00 0x100>;
>  			interrupts = <11>;
>  			interrupt-parent = <&qeic>;
>  			mode = "slave";

I'd suggest to rename this to "peripheral" as we use for fsl dual-role
usb controller.

> +			usb-clock = <21>;
> +			pio-handle = <&pio_usb>;

Can we not introduce new pio maps? The pio setup should be done
by the firmware, or at least fixed up via the board file, as in
arch/powerpc/platforms/83xx/mpc832x_rdb.c.

[...]
> +#ifdef CONFIG_QUICC_ENGINE
> +/* QE USB_CLOCK configure functions */
> +int qe_usb_clock_set(struct device_node *np)

We already have this function, in arch/powerpc/sysdev/qe_lib/usb.c

It does not parse any of properties though, driver should do this.

> +{
> +	u32 tmpreg = 0;
> +	struct qe_mux *qemux = NULL;
> +	const int *clock;
> +
> +	qemux = &qe_immr->qmx;
> +
> +	clock = of_get_property(np, "usb-clock", NULL);
> +	if (!clock)
> +		return -EINVAL;
> +
> +	/* CLK21 -> USBCLK on MPC8360-PB*/
> +	tmpreg = in_be32(&qemux->cmxgcr) & ~QE_CMXGCR_USBCS;
> +	switch (*clock) {
> +	case 21:
> +		tmpreg |= 0x8;
> +		out_be32(&qemux->cmxgcr, tmpreg);
> +		par_io_config_pin(2, 20, 2, 0, 1, 0);  /* PC20 for CLK21 */

No, pio config is very board-specific. This should be done by the
firmware (ideally) or by the board file.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2