Re: Device tree for c67x00

Re: Device tree for c67x00

[Grant Likely]

(added devicetree-discuss mailing list to cc: list since some of the
content is relevant to current discussions about device tree syntax.)

On Fri, Sep 26, 2008 at 04:09:01PM -0700, Stephen Neuendorffer wrote:
> 
> Below is an fdt patch for the cypress USB.  It generates fragments like:
> 
>                 ext_usb: xps-epc@85000000 {
>                         #address-cells = <1>;
>                         #size-cells = <1>;
>                         compatible = "xlnx,compound";
>                         ranges = < 0x85000000 0x85000000 0x10000 >;

Aside: Considering that this is a 1:1 mapping, you could just specify
"ranges;" here instead...  Of course, since the epc devices really are
bridges, it probably is appropriate for ranges to be explicit.

However, since the epc devices have multiple chip selects, it might be
better to use #address-cells = <2> and encode the chip select in the
first address cell.  Then ranges would look something like this:
	ranges = < 0 0 0x85000000 0x10000 >; (CS0, offset 0)

Additional chip selects would add additional tuples to the ranges
property:
	ranges = < 0 0 0x85000000 0x10000    (CS0, offset 0)
	           1 0 0x86000000 0x10000 >; (CS1, offset 0)

and the reg property for the c67x00 would be:
	reg = <0 0 0x10000>;

This scheme would be an accurate representation of what the FPGA design
is actually doing.

>                         usb@85000000 {
>                                 compatible = "cypress,c67x00";
>                                 reg = < 0x85000000 0x10000 >;
>                         } ;
>                 } ;
> 
> Assuming, of course, that we connect through the epc core.
> There are two pieces of trickiness:
> 
> 1) How do we know that the epc connects to a cypress chip outside of the
> FPGA.  This needs to be encoded in the EDK project somehow..  We have
> the same problem with FLASH.

This is a major use-case for the new syntax being discussed to add to dtc.
Specifically being able to include one file into another.  It would be
desirable for EDK to generate a base layout that describes the FPGA
design, and then have a board file that includes the generated file and
adds the nodes/properties that are board, not FPGA, specific.

I'll try to write up some specific use cases that would help in
evaluating new dts syntax.  (Jon, David; I can't help much with the dtc
code, but I can at least provide my impressions from the user side of
the equation).

> 2) There is also an external interrupt line, but the interrupt line
> doesn't connect through the core.  Presumably, we need someway of
> representing this association in the EDK project as well..

Ditto for this.

> In both cases, this information could be neatly stored in a hierarchical
> design, that described what was inside the FPGA *AND* what was outside,
> but today, we'll just have to hack something in...  Any suggestions?

RE: Device tree for c67x00

[Stephen Neuendorffer]

> -----Original Message-----
> From: Grant Likely [mailto:glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org] On Behalf Of Grant
Likely
> Sent: Saturday, October 04, 2008 10:58 PM
> To: Stephen Neuendorffer
> Cc: git-dev; Walter Tomkoski; Michal Simek; Jim Hwang;
devicetree-discuss-mnsaURCQ41sdnm+yROfE0A@public.gmane.org
> Subject: Re: Device tree for c67x00
> 
> (added devicetree-discuss mailing list to cc: list since some of the
> content is relevant to current discussions about device tree syntax.)
> 
> On Fri, Sep 26, 2008 at 04:09:01PM -0700, Stephen Neuendorffer wrote:
> >
> > Below is an fdt patch for the cypress USB.  It generates fragments
like:
> >
> >                 ext_usb: xps-epc@85000000 {
> >                         #address-cells = <1>;
> >                         #size-cells = <1>;
> >                         compatible = "xlnx,compound";
> >                         ranges = < 0x85000000 0x85000000 0x10000 >;
> 
> Aside: Considering that this is a 1:1 mapping, you could just specify
> "ranges;" here instead...  Of course, since the epc devices really are
> bridges, it probably is appropriate for ranges to be explicit.
> 
> However, since the epc devices have multiple chip selects, it might be
> better to use #address-cells = <2> and encode the chip select in the
> first address cell.  Then ranges would look something like this:
> 	ranges = < 0 0 0x85000000 0x10000 >; (CS0, offset 0)
> 
> Additional chip selects would add additional tuples to the ranges
> property:
> 	ranges = < 0 0 0x85000000 0x10000    (CS0, offset 0)
> 	           1 0 0x86000000 0x10000 >; (CS1, offset 0)
> 
> and the reg property for the c67x00 would be:
> 	reg = <0 0 0x10000>;

Hmm.. interesting idea.

> This scheme would be an accurate representation of what the FPGA
design
> is actually doing.
> 
> >                         usb@85000000 {
> >                                 compatible = "cypress,c67x00";
> >                                 reg = < 0x85000000 0x10000 >;
> >                         } ;
> >                 } ;
> >
> > Assuming, of course, that we connect through the epc core.
> > There are two pieces of trickiness:
> >
> > 1) How do we know that the epc connects to a cypress chip outside of
the
> > FPGA.  This needs to be encoded in the EDK project somehow..  We
have
> > the same problem with FLASH.
> 
> This is a major use-case for the new syntax being discussed to add to
dtc.
> Specifically being able to include one file into another.  It would be
> desirable for EDK to generate a base layout that describes the FPGA
> design, and then have a board file that includes the generated file
and
> adds the nodes/properties that are board, not FPGA, specific.
> 
> I'll try to write up some specific use cases that would help in
> evaluating new dts syntax.  (Jon, David; I can't help much with the
dtc
> code, but I can at least provide my impressions from the user side of
> the equation).

I completely agree..  At the moment I'm more concerned about how that
might be encoded in the EDK project...  Although maybe it's not as much
of an issue (although if the device tree information for the board is
completely disconnected from EDK, then it would force a board designer
to replicate information that might be in Base System Builder).

Steve

This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Re: Device tree for c67x00

[Peter Korsgaard]

>>>>> "Grant" == Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> writes:

Hi,

 >>     usb@85000000 {
 >>                  compatible = "cypress,c67x00";
 >>                  reg = < 0x85000000 0x10000 >;
 >>                  } ;

This is the first I see of this, but that weren't we supposed to use
the stock ticker (E.G. cy)? It would also be good to list the exact
cy67x00 variant:

usb@85000000 {
             compatible = "cy,c67300" "cy,c67x00";
             ...

-- 
Bye, Peter Korsgaard

Re: Device tree for c67x00

[Grant Likely]

On Mon, Oct 06, 2008 at 09:22:58AM +0200, Peter Korsgaard wrote:
> >>>>> "Grant" == Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> writes:
> 
> Hi,
> 
>  >>     usb@85000000 {
>  >>                  compatible = "cypress,c67x00";
>  >>                  reg = < 0x85000000 0x10000 >;
>  >>                  } ;
> 
> This is the first I see of this, but that weren't we supposed to use
> the stock ticker (E.G. cy)? It would also be good to list the exact
> cy67x00 variant:
> 
> usb@85000000 {
>              compatible = "cy,c67300" "cy,c67x00";
>              ...

Heh, I got caught up in the other discussion and I never thought to
review the binding.  Oops.

Yes, you're absolutely right.  The compatible property should be
specific and the binding needs to be documented and reviewed before
committing to it.  In particular, there needs to be some properties to
describe the mode of each SIE on the c67300.

g.

Device tree for c67x00

[Jorge Sánchez de Nova]

[-- Attachment #1: Type: text/plain, Size: 1936 bytes --]

Hi,

 I am trying to load the c67x00 driver in my Xilinx ML403 (PowerPC) based
platform. I am running a merged tree between DENX+Adeos/Xenomai and Xilinx
(2.6.29.4). I have the XPS_EPC core interfaced to the Cypress CY7C67300.
Before, in PPC I used to have something like this in my virtex_devices.c

/*
 * Cypress USB C67x00 shortcut macro for single instance
 */
#define XPAR_C67x00_USB(num) { \
 .name = "c67x00", \
 .id = num, \
 .num_resources = 2, \
 .resource = (struct resource[]) { \
         { \
         .start = XPAR_C67X00_USB_PRH##num##_BASEADDR, \
         .end = XPAR_C67X00_USB_PRH##num##_BASEADDR + 0xf, \
         .flags = IORESOURCE_MEM, \
         }, \
         { \
         .start  = XPAR_OPB_INTC_0_SYSTEM_USB_HPI_INT_PIN_INTR, \
         .end    = XPAR_OPB_INTC_0_SYSTEM_USB_HPI_INT_PIN_INTR, \
         .flags  = IORESOURCE_IRQ, \
          }, \
  }, \
 .dev.platform_data = &(struct c67x00_platform_data) { \
 .sie_config = C67X00_SIE1_HOST | C67X00_SIE2_PERIPHERAL_A, \
 .hpi_regstep = 0x02, /* A0 not connected on 16bit bus */ \
 }, \
}

Together with the xparameters.h #defines. And even if I never managed to get
it working, the kernel was loading it. Now I am trying to have the
equivalent information in the device-tree .dts with no luck. Something like
this:

                xps_epc_0: usb@80800000 {
          compatible = "cy,c67300 cy,c67x00";
                  interrupt-parent = <&xps_intc_0>;
          interrupts = < 0 2 >;
                  reg = < 0x80800000 0x10000 >;
            xlnx,family = "virtex4";
                  } ;

(inspired by another mail in this list).

It doesn't work at all since it doesn't load anything. I have looked at the
driver and there is apparently no openfirmware support for it, so maybe the
dts info won't work without it. Am I wrong? Does this means that the c67x00
needs OF support to work in this configuration? How can I make it otherwise?


Thanks,
Jorge

[-- Attachment #2: Type: text/html, Size: 2160 bytes --]

Re: Device tree for c67x00

[Peter Korsgaard]

>>>>> "Jorge" =3D=3D Jorge S=C3=A1nchez de Nova <j.s.denova@gmail.com> writ=
es:

 Jorge> Hi,

 Jorge> It doesn't work at all since it doesn't load anything. I have
 Jorge> looked at the driver and there is apparently no openfirmware
 Jorge> support for it, so maybe the dts info won't work without
 Jorge> it. Am I wrong? Does this means that the c67x00 needs OF
 Jorge> support to work in this configuration? How can I make it
 Jorge> otherwise?

Yes, the c67x00 driver doesn't currently have any OF bindings. Either
you can add it, or simply manually create the struct platform_device
in your board file (or scan the DT in your board file and fill in the
correct base address / interrupt number from it).

Remember that arch/powerpc uses virtual interrupt numbers if you're
going to fill in the platform_device by hand.

--=20
Bye, Peter Korsgaard

Re: Device tree for c67x00

[Jorge Sánchez de Nova]

[-- Attachment #1.1: Type: text/plain, Size: 7904 bytes --]

Hi,

 ok, so I have managed to give it some OF support in a preliminary way(see
patch). But now, I am facing serious problems which I am finding difficult
to tackle. I understand that maybe these problems have to do partially or
entirely to the Xilinx ML403. Let me explain myself: First of all, I don't
know of anyone else who has used it for the ML403 with the XPS_EPC , so
there might be some particularities I should specifically take care of.

 First of all, I see a resetting problem. When I load the kernel with my
board from a powered off state to a powered on state it behaves differently
that if I just to try to load the kernel again with the board powered on
(see ERROR 1 below). I have seen that the reset line in the c67300 is
connected to the fpga and for sure, to take care of it one would need to do
it through a third gpio line outside. This would be to make a hard reset.
Looking at this old specific driver that I have found for kernel 2.4.20 (
http://www.sysf.physto.se/~attila/ATLAS/Digitizer/Testbench/System_ISE_SoftMAC/linux/software/uClinux-2.4.x/drivers/usb/cy7c67300/<http://www.sysf.physto.se/%7Eattila/ATLAS/Digitizer/Testbench/System_ISE_SoftMAC/linux/software/uClinux-2.4.x/drivers/usb/cy7c67300/>)
I see that the hard resets are common using the ugly gpio method.

 With the board on, if I reload the fpga programming and kernel image into
memory and set the PC to run properly, it gives different errors. Sometimes
ERROR2 and some other times ERROR3. When ERROR2, the processor halts when
calling WARN_ON(!res) and in the second, the interrupt handler keeps writing
to the output endlessly.

 I have added some printks to trace the problem and the OF support is simply
a rewriting of the normal platform_driver registration. Any ideas?

Jorge

-- ERROR 1 (just after power on--
Generic platform RAM MTD, (c) 2004 Simtec Electronics
usbmon: debugfs is not available
C67X00_DBG:c67x00_of_probe() - Request memory region
C67X00_DBG:c67x00_of_probe() - Allocating data structs
C67X00_DBG:c67x00_of_probe() - Configuring c67x00 device
C67X00_DBG:c67x00_of_probe() - Configure platform data based on the
device-tree data
C67X00_DBG:c67x00_of_probe() - hpi.regstep: 2
C67X00_DBG:c67x00_of_probe() - SIE_config: 1
C67X00_DBG:c67x00_of_probe() - SIE_config: 21
C67X00_DBG:c67x00_of_probe() - Low-level initizalization
C67X00_DBG:c67x00_ll_hpi_reg_init(): Reg:324 written=0x00 ; read: 0
C67X00_DBG:c67x00_ll_hpi_reg_init(): Reg:328 written=0x00 ; read: 0
C67X00_DBG:c67x00_of_probe() - Registering IRQ
C67X00_DBG:c67x00_of_probe() - Trying to register IRQ:17 @ 17
C67X00_DBG:c67x00_irq() - Handling IRQ number 57005
�����;�L�=?� : Not all interrupts handled! status = 0xdead
C67X00_DBG:c67x00_irq() - Handling IRQ number 57005
C67X00_DBG:c67x00_of_probe() - Low-level reset
C67X00_DBG:c67x00_ll_reset() - Send mbox
C67X00_DBG:c67x00_ll_reset() - recv_msg
C67X00_DBG:ll_recv_msg() -  calling wait_for_completion_timeout
C67X00_DBG:ll_recv_msg() res=5000
C67X00_DBG:c67x00_ll_reset() - done recv_msg
C67X00_DBG:c67x00_of_probe() - Configuring SIEs
C67X00_DBG:ll_recv_msg() -  calling wait_for_completion_timeout
C67X00_DBG:ll_recv_msg() res=5000
����� : SIE 0 not set to host mode
����� : Cypress C67X00 Host Controller
Unable to handle kernel paging request for data at address 0x00000002
Faulting instruction address: 0xc0012654
Oops: Kernel access of bad area, sig: 11 [#1]
PREEMPT Xilinx Virtex
Modules linked in:
NIP: c0012654 LR: c01c8ca0 CTR: c01fa0e4
REGS: c381db90 TRAP: 0300   Not tainted  (2.6.29.4)
MSR: 00029030 <EE,ME,CE,IR,DR>  CR: 35039055  XER: a000004b
DEAR: 00000002, ESR: 00000000
TASK = c381a000[1] 'swapper' THREAD: c381c000
GPR00: 00000002 c381dc40 c381a000 00000002 00000001 00000000 fffffffe
ffffffff
GPR08: 00000000 00000008 00000003 c380a0b1 35039059 ffffffff ffffffff
ffffffff
GPR16: ffffffff ffffffff ffffffff ffffffff ffffffff c398fee0 c035bdc4
c0360acc
GPR24: 00000000 00000000 c380a000 c03cac98 00000001 c385a84c 000000d0
c385a84c
NIP [c0012654] strlen+0x4/0x18
LR [c01c8ca0] kobject_get_path+0x34/0xe0
Call Trace:
[c381dc40] [c01c91fc] add_uevent_var+0x74/0xf4 (unreliable)
[c381dc60] [c01fa190] dev_uevent+0xac/0x210
[c381dc80] [c01c94a8] kobject_uevent_env+0x22c/0x454
[c381dcd0] [c01fb164] device_add+0x39c/0x594
[c381dd20] [c01fb418] device_create_vargs+0x8c/0xd0
[c381dd50] [c01fb49c] device_create+0x40/0x50
[c381dd80] [c0219fc0] usb_add_hcd+0x140/0x6d4
[c381ddb0] [c022daac] c67x00_hcd_probe+0x120/0x1f0
[c381ddd0] [c022c88c] c67x00_probe_sie+0xcc/0xe0
[c381dde0] [c02f43a4] c67x00_of_probe+0x3fc/0x458
[c381de50] [c0258a88] of_platform_device_probe+0x5c/0x84
[c381de70] [c01fdb8c] driver_probe_device+0xbc/0x1f4
[c381de90] [c01fdd68] __driver_attach+0xa4/0xa8
[c381deb0] [c01fce78] bus_for_each_dev+0x5c/0x98
[c381dee0] [c01fd990] driver_attach+0x24/0x34
[c381def0] [c01fd798] bus_add_driver+0x1d0/0x250
[c381df20] [c01fe0b4] driver_register+0x5c/0x150
[c381df40] [c0258950] of_register_driver+0x54/0x70
[c381df50] [c03aaa8c] c67x00_init+0x34/0x8c
[c381df60] [c000236c] do_one_initcall+0x34/0x1a4
[c381dfd0] [c0393170] kernel_init+0x90/0xfc
[c381dff0] [c000fa24] kernel_thread+0x4c/0x68
Instruction dump:
4d820020 7ca903a6 38a3ffff 3884ffff 8c650001 2c830000 8c040001 7c601851
4d860020 4102ffec 4e800020 3883ffff <8c040001> 2c000000 4082fff8 7c632050
---[ end trace f8500dd73d54b5fd ]---
Kernel panic - not syncing: Attempted to kill init!
Rebooting in 180 seconds..

-- ERROR 2 (with the board already on, just reprogramming fpga, kernel and
booting again)--

Generic platform RAM MTD, (c) 2004 Simtec Electronics
usbmon: debugfs is not available
C67X00_DBG:c67x00_of_probe() - Request memory region
C67X00_DBG:c67x00_of_probe() - Allocating data structs
C67X00_DBG:c67x00_of_probe() - Configuring c67x00 device
C67X00_DBG:c67x00_of_probe() - Configure platform data based on the
device-tree data
C67X00_DBG:c67x00_of_probe() - hpi.regstep: 2
C67X00_DBG:c67x00_of_probe() - SIE_config: 1
C67X00_DBG:c67x00_of_probe() - SIE_config: 21
C67X00_DBG:c67x00_of_probe() - Low-level initizalization
C67X00_DBG:c67x00_ll_hpi_reg_init(): Reg:324 written=0x00 ; read: 0
C67X00_DBG:c67x00_ll_hpi_reg_init(): Reg:328 written=0x00 ; read: 0
C67X00_DBG:c67x00_of_probe() - Registering IRQ
C67X00_DBG:c67x00_of_probe() - Trying to register IRQ:17 @ 17
C67X00_DBG:c67x00_of_probe() - Low-level reset
C67X00_DBG:c67x00_ll_reset() - Send mbox
C67X00_DBG:c67x00_ll_reset() - recv_msg
C67X00_DBG:ll_recv_msg() -  calling wait_for_completion_timeout
C67X00_DBG:ll_recv_msg() res=0
--> Here the processor halts when in call to WARN_ON(!res);

-- ERROR 3: gets stuck in the interrupt handler --
�����;�L�=?� : Not all interrupts handled! status = 0x0148
C67X00_DBG:c67x00_irq() - Handling IRQ number 328

2009/6/29 Peter Korsgaard <jacmet@sunsite.dk>

> >>>>> "Jorge" == Jorge Sánchez de Nova <j.s.denova@gmail.com> writes:
>
>  Jorge> Hi,
>
>  Jorge> It doesn't work at all since it doesn't load anything. I have
>  Jorge> looked at the driver and there is apparently no openfirmware
>  Jorge> support for it, so maybe the dts info won't work without
>  Jorge> it. Am I wrong? Does this means that the c67x00 needs OF
>  Jorge> support to work in this configuration? How can I make it
>  Jorge> otherwise?
>
> Yes, the c67x00 driver doesn't currently have any OF bindings. Either
> you can add it, or simply manually create the struct platform_device
> in your board file (or scan the DT in your board file and fill in the
> correct base address / interrupt number from it).
>
> Remember that arch/powerpc uses virtual interrupt numbers if you're
> going to fill in the platform_device by hand.
>
> --
> Bye, Peter Korsgaard
>

[-- Attachment #1.2: Type: text/html, Size: 8731 bytes --]

[-- Attachment #2: c67x00_of.patch --]
[-- Type: text/x-patch, Size: 8957 bytes --]

diff --git a/drivers/usb/c67x00/c67x00-drv.c b/drivers/usb/c67x00/c67x00-drv.c
index 5633bc5..699cbef 100644
--- a/drivers/usb/c67x00/c67x00-drv.c
+++ b/drivers/usb/c67x00/c67x00-drv.c
@@ -39,6 +39,10 @@
 #include <linux/list.h>
 #include <linux/usb.h>
 #include <linux/usb/c67x00.h>
+#if defined(CONFIG_OF)
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
+#endif
 
 #include "c67x00.h"
 #include "c67x00-hcd.h"
@@ -87,8 +91,9 @@ static irqreturn_t c67x00_irq(int irq, void *__dev)
 	struct c67x00_sie *sie;
 	u16 msg, int_status;
 	int i, count = 8;
-
+//	printk("Handling IRQ!\n");
 	int_status = c67x00_ll_hpi_status(c67x00);
+	printk("C67X00_DBG:c67x00_irq() - Handling IRQ number %d\n",int_status);
 	if (!int_status)
 		return IRQ_NONE;
 
@@ -225,14 +230,241 @@ static struct platform_driver c67x00_driver = {
 };
 MODULE_ALIAS("platform:c67x00");
 
+/* ---------------------------------------------------------------------
+ * OF_Platform Bus Support
+ */
+
+#if defined(CONFIG_OF)
+static int __devinit c67x00_of_probe(struct of_device *dev, 
+				     const struct of_device_id *match)
+{
+ 	struct c67x00_device *c67x00;
+ 	struct c67x00_platform_data *pdata;
+	struct resource r_irq_struct;
+	struct resource *res;
+	struct resource memory;
+	struct device_node *dp = dev->node;
+	struct resource* r_irq = &r_irq_struct;
+	u32* iprop;
+	int len, ret, i;
+
+	/* Request memory region */
+	printk("C67X00_DBG:c67x00_of_probe() - Request memory region\n");
+	ret = of_address_to_resource(dp, 0, &memory);
+	if (ret)
+		return -ENXIO;
+
+	res = request_mem_region(memory.start, memory.end - memory.start + 1,
+			dev_name(&dev->dev));
+
+	if (!res){
+		dev_err(&dev->dev, "Memory region busy\n"); 
+		return -EBUSY;		
+	}
+
+	/* Allocate device and platform_data structs */
+	printk("C67X00_DBG:c67x00_of_probe() - Allocating data structs\n");
+	c67x00 = kzalloc(sizeof(*c67x00), GFP_KERNEL);
+	if (!c67x00)	
+	    return -ENOMEM;
+
+	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+	if (!pdata){
+		ret = -ENOMEM;
+	        goto request_mem_failed;
+	}
+
+	/* Configure c67x00 device */
+	printk("C67X00_DBG:c67x00_of_probe() - Configuring c67x00 device\n");
+	c67x00->hpi.base = ioremap(memory.start, memory.end - memory.start + 1);
+
+	if (!c67x00->hpi.base) {
+		dev_err(&dev->dev, "Unable to map HPI registers\n");
+		ret = -EIO;
+		goto map_failed;
+	}
+
+	/* Configure platform data based on the device-tree data */	
+	printk("C67X00_DBG:c67x00_of_probe() - Configure platform data based on the device-tree data\n");
+	spin_lock_init(&c67x00->hpi.lock);
+	//TODO: Check that we are using the ML403
+
+	iprop = of_get_property(dev->node, "xlnx,hpi_regstep", &len);
+	if (!iprop || len < sizeof(*iprop)) {
+		dev_err(&dev->dev, "No 'xlnx,hpi_regstep property'\n");
+		ret = -EIO;
+		goto map_failed; //TODO
+	}
+	c67x00->hpi.regstep = *iprop;
+	printk("C67X00_DBG:c67x00_of_probe() - hpi.regstep: %x\n", c67x00->hpi.regstep);	
+
+	iprop = of_get_property(dev->node, "xlnx,sie1_config", &len);
+	if (!iprop || len < sizeof(*iprop)) {
+		dev_err(&dev->dev, "No 'xlnx,sie1_config'\n");
+		ret = -EIO;
+		goto map_failed; //TODO
+	}
+	pdata->sie_config =  *iprop;
+	printk("C67X00_DBG:c67x00_of_probe() - SIE_config: %x\n", pdata->sie_config);	
+
+	iprop = of_get_property(dev->node, "xlnx,sie2_config", &len);
+	if (!iprop || len < sizeof(*iprop)) {
+		dev_err(&dev->dev, "No 'xlnx,sie2_config'\n");
+		ret = -EIO;
+		goto map_failed; //TODO
+	}
+	pdata->sie_config |= *iprop << 4;
+	printk("C67X00_DBG:c67x00_of_probe() - SIE_config: %x\n", pdata->sie_config);
+
+	c67x00->pdata = pdata;
+	c67x00->pdev = dev;
+
+	/* Low-level initizalization */
+	printk("C67X00_DBG:c67x00_of_probe() - Low-level initizalization \n");
+	c67x00_ll_init(c67x00);
+	c67x00_ll_hpi_reg_init(c67x00);
+
+	/* Request IRQ */	
+	printk("C67X00_DBG:c67x00_of_probe() - Registering IRQ\n");
+	dev_dbg(&dev->dev, "Registering IRQ\n");
+	ret = of_irq_to_resource(dev->node, 0, r_irq);
+
+	if (ret == NO_IRQ) {
+		dev_err(&dev->dev, "Couldn't find IRQ in device-tree\n");
+		goto request_irq_failed;
+	}
+	printk("C67X00_DBG:c67x00_of_probe() - Trying to register IRQ:%d @ %d\n",ret,r_irq->start);
+	ret = request_irq(r_irq->start, c67x00_irq, 0, dev_name(&dev->dev), c67x00);
+	if (ret) {
+		dev_err(&dev->dev, "Cannot claim IRQ\n");
+		goto request_irq_failed;
+	}
+
+	/* Reset c67x00 as part of the initizalization*/
+	printk("C67X00_DBG:c67x00_of_probe() - Low-level reset\n");
+	ret = c67x00_ll_reset(c67x00);
+	if (ret) {
+		printk("Device reset failed\n");
+		dev_err(&dev->dev, "Device reset failed\n");
+		goto reset_failed;
+	}
+
+	/* Probe each of the SIEs */
+	printk("C67X00_DBG:c67x00_of_probe() - Configuring SIEs\n");
+	dev_dbg(&dev->dev, "Configuring SIEs\n");
+	for (i = 0; i < C67X00_SIES; i++)
+		c67x00_probe_sie(&c67x00->sie[i], c67x00, i);
+	printk("C67X00_DBG:c67x00_of_probe() - Saving driver data\n");
+	dev_set_drvdata(&dev->dev, c67x00);
+	return 0;
+
+ reset_failed:
+	free_irq(r_irq->start, c67x00);
+ request_irq_failed:
+	iounmap(c67x00->hpi.base);
+ map_failed:
+	release_mem_region(memory.start, memory.end - memory.start + 1);
+	kfree(pdata);
+ request_mem_failed:
+	kfree(c67x00);
+    
+	return ret;
+}
+
+static int __devexit c67x00_of_remove(struct of_device *dev)
+{
+	struct c67x00_device *c67x00 = dev_get_drvdata(dev);
+	struct resource *res;
+	int i;
+
+	dev_dbg(dev, "c67x00_free(%p)\n", dev);
+
+	for (i = 0; i < C67X00_SIES; i++)
+		c67x00_remove_sie(&c67x00->sie[i]);
+
+	c67x00_ll_release(c67x00);
+
+	//TODO: release IRQ?
+// 	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+// 	if (res)
+// 		free_irq(res->start, c67x00);
+
+	iounmap(c67x00->hpi.base);
+
+
+// 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+// 	if (res)
+// 		release_mem_region(res->start, res->end - res->start + 1);
+
+	kfree(c67x00);
+
+	return 0;
+}
+
+/* Match table for of_platform binding */
+static struct of_device_id c67x00_of_match[] __devinitdata = {	
+	{ .compatible = "cy,c67x00", },
+	{ .compatible = "cy,c67300", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, c67x00_of_match);
+
+static struct of_platform_driver c67x00_of_driver = {
+	.owner = THIS_MODULE,
+	.name = "c67x00",
+	.match_table = c67x00_of_match,
+	.probe = c67x00_of_probe,
+	.remove = __devexit_p(c67x00_of_remove),
+	.driver = {
+		.name = "c67x00",
+		.owner = THIS_MODULE,
+	},
+};
+
+/* Registration helpers to keep the number of #ifdefs to a minimum */
+static inline int __init c67x00_of_register(void)
+{
+	pr_debug("c67x00: registering OF binding\n");
+	return of_register_platform_driver(&c67x00_of_driver);
+}
+
+static inline void __exit c67x00_of_unregister(void)
+{
+	of_unregister_platform_driver(&c67x00_of_driver);
+}
+#else /* CONFIG_OF */
+/* CONFIG_OF not enabled; do nothing helpers */
+static inline int __init c67x00_of_register(void) { return 0; }
+static inline void __exit c67x00_of_unregister(void) { }
+#endif /* CONFIG_OF */
+
 static int __init c67x00_init(void)
 {
-	return platform_driver_register(&c67x00_driver);
+	int rc;
+
+	rc = c67x00_of_register();
+	if (rc)
+		goto err_of;
+
+	pr_debug("c67x00: registering platform binding\n");
+        rc = platform_driver_register(&c67x00_driver);
+	if (rc)
+		goto err_plat;
+
+	return 0; 
+
+err_of:
+	c67x00_of_unregister();
+err_plat: 
+	printk("c67x00: registration failed; err:%i\n",rc);
+	return rc; 	
 }
 
 static void __exit c67x00_exit(void)
 {
+	pr_debug("Unregistering c67x00\n"); 
 	platform_driver_unregister(&c67x00_driver);
+        c67x00_of_unregister();
 }
 
 module_init(c67x00_init);
diff --git a/drivers/usb/c67x00/c67x00-ll-hpi.c b/drivers/usb/c67x00/c67x00-ll-hpi.c
index a9636f4..2e7061a 100644
--- a/drivers/usb/c67x00/c67x00-ll-hpi.c
+++ b/drivers/usb/c67x00/c67x00-ll-hpi.c
@@ -219,6 +219,7 @@ void c67x00_ll_hpi_reg_init(struct c67x00_device *dev)
 	for (i = 0; i < C67X00_SIES; i++) {
 		hpi_write_word(dev, SIEMSG_REG(i), 0);
 		hpi_read_word(dev, SIEMSG_REG(i));
+		printk("C67X00_DBG:c67x00_ll_hpi_reg_init(): Reg:%d written=0x00 ; read: %d\n",SIEMSG_REG(i));
 	}
 }
 
@@ -240,8 +241,9 @@ void c67x00_ll_hpi_disable_sofeop(struct c67x00_sie *sie)
 static inline u16 ll_recv_msg(struct c67x00_device *dev)
 {
 	u16 res;
-
+	printk("C67X00_DBG:ll_recv_msg() -  calling wait_for_completion_timeout \n");
 	res = wait_for_completion_timeout(&dev->hpi.lcp.msg_received, 5 * HZ);
+	printk("C67X00_DBG:ll_recv_msg() res=%d\n", res);
 	WARN_ON(!res);
 
 	return (res == 0) ? -EIO : 0;
@@ -388,10 +390,13 @@ void c67x00_ll_irq(struct c67x00_device *dev, u16 int_status)
 int c67x00_ll_reset(struct c67x00_device *dev)
 {
 	int rc;
-
+	
 	mutex_lock(&dev->hpi.lcp.mutex);
+	printk("C67X00_DBG:c67x00_ll_reset() - Send mbox\n");
 	hpi_send_mbox(dev, COMM_RESET);
+	printk("C67X00_DBG:c67x00_ll_reset() - recv_msg\n");
 	rc = ll_recv_msg(dev);
+	printk("C67X00_DBG:c67x00_ll_reset() - done recv_msg\n");
 	mutex_unlock(&dev->hpi.lcp.mutex);
 
 	return rc;