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

[Li Yang <leoli@freescale.com>]

Hi Anton,

On Wed, 2008-08-06 at 16:07 +0400, Anton Vorontsov wrote:
> 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.

Ok.  I didn't notice your addition to the binding.  I will try to update
it for device mode.

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

As there will be two drivers chosen by compatible, I'm now inclined to
put this information in compatible.

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

Actually I am more apt to leaving full hardware access to kernel than
firmware, especially for devices that are not used in firmware.  The
reason why I made the pin-configuration flexible is that for development
boards the role of pins are often changeable.

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

I just saw this.  Will try to reuse it.

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

Pio config is board and board configuration specific.  It's better to
make it configurable by device tree.

- Leo