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

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:

quoted

Signed-off-by: Li Yang <redacted>
---
 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.

quoted

 			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.

quoted

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

[...]

quoted

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

quoted

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