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

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

On Thu, Aug 07, 2008 at 05:31:44PM +0800, Li Yang wrote:
[...]
> > 
> > >  			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.

Please don't. I deliberately wrote bindings w/o specifing "udc" or
"host" in the compatible entry.

"udc"/"host" is the modes of an USB controller, but the controller
itself is the same: "fsl,mpc8323-qe-usb" (the first chip with QE USB).

So the driver should always match this device, but it can return
-ENODEV if mode is unspecified or !peripheral.

> > > +			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.
[...]
> Pio config is board and board configuration specific.  It's better to
> make it configurable by device tree.

Device tree isn't configuration file. The bad thing about pio-map is that
it is passing raw values instead of actually describing the hardware.

For example,

pio-map = <1  6  2  0  1  0>;

The thing describes bankB/pin6.. but you'll can't tell what exactly
this pin supposed to do. :-/

Basically "pio-map" is expanded version of this:

fsl,cpodr-reg = <0x...>;
fsl,cppar1-reg = <0x...>;
fsl,cppar2-reg = <0x...>;
...


Instead, it would be great to have something like this:

usb@... {
	/*
	 *      gpio/pinmux    pin
	 *      controller
	 */
	pio-map = <&pinmuxA     1 /* bindings says first pin is clk */
		   &pinmuxB    14 /* bindings says second pin is usboe */
		   ...>;
};

ucc@... {
	pio-map = <&pinmuxA     2 /* bindings says first pin is clk */
		   &pinmuxB    24 /* bindings says second pin is rxd0 */
		   &pinmuxB    21 /* bindings says second pin is rxd1 */
		   ...>;
};

Then drivers would call something like this in probe():

clk = qe_get_clock(node, "fsl,fullspeed-clock");
qe_set_pinmux(pin0, QE_PIN_FUNC_CLK(clk));
qe_set_pinmux(pin1, QE_PIN_FUNC_USB_OE);
...or ucc ethernet...
qe_set_pinmux(pin[rx_n], QE_PIN_FUNC_UCC_RXD(ucc_num, rx_n));

Obviously, this is quite hard to implement (and expensive, too), since
each SoC implementation has its own function<->pin<->regvalue mapping..

Thus nobody even think to bother with this.


Anyway, I'm not that opposed to the current pio-maps, but it
would be great if we could avoid them where possible.

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