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 <[EMAIL PROTECTED]>
> > ---
> > 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: [EMAIL PROTECTED] {
> > + 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 @@
> > };
> >
> > [EMAIL PROTECTED] {
> > - 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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
