Re: [PATCH 2/2] USB: isp1760: Support board-specific hardware configurations

2008-05-22 Thread Olof Johansson 
On Wed, May 21, 2008 at 04:28:51PM -0500, Nate Case wrote:
 This adds support for hardware configurations that don't match the
 chip default register settings (e.g., 16-bit data bus, DACK and
 DREQ pulled down instead of up, analog overcurrent mode).
 
 These settings are passed in via the OF device tree.  The PCI
 interface still assumes the same default values.

Nice! I'll give this a spin on Electra as well, it should make it work
there.

A couple of comments on the OF interface:

 @@ -55,8 +57,34 @@ static int of_isp1760_probe(struct of_device *dev,
   virq = irq_create_of_mapping(oirq.controller, oirq.specifier,
   oirq.size);
  
 + prop = of_get_property(dp, port1-disable, NULL);
 + if (prop  *prop != 0)
 + devflags |= ISP1760_FLAG_PORT1_DIS;

It should be sufficient to add the property without a value, and just
check for the presence of it. The cpu nodes have properties like this if
you want something to compare with.

 +
 + /* Some systems wire up only 16 of the 32 data lines */
 + prop = of_get_property(dp, bus-width, NULL);
 + if (prop  *prop == 16)
 + devflags |= ISP1760_FLAG_BUS_WIDTH_16;

This is the obvious exception, here you'll have to check the value.

I'm not sure if the DT police will complain of the overloaded bus-width
property name and would prefer a custom one.


-Olof
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH 2/2] USB: isp1760: Support board-specific hardware configurations

2008-05-22 Thread Sebastian Siewior 
* Nate Case | 2008-05-21 16:28:51 [-0500]:

This adds support for hardware configurations that don't match the
chip default register settings (e.g., 16-bit data bus, DACK and
DREQ pulled down instead of up, analog overcurrent mode).
Nice patch. A few comments inline.

These settings are passed in via the OF device tree.  The PCI
interface still assumes the same default values.
The default values should be okay for PCI . It should only be required
for the eval kit from NXP. There was an ISP1761 behind a PLB bridge. I
had all three ports functional there (no OTG) in my first shot after
enabling or disabling some resistors (don't remember the details
anymore). Than I moved to my powerpc and returned the eval kit.

Signed-off-by: Nate Case [EMAIL PROTECTED]
---
 drivers/usb/host/isp1760-hcd.c |   68 +++
 drivers/usb/host/isp1760-hcd.h |   19 ++-
 drivers/usb/host/isp1760-if.c  |   34 +++-
 3 files changed, 103 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/host/isp1760-hcd.c b/drivers/usb/host/isp1760-hcd.c
index 65aa5ec..679b8df 100644
--- a/drivers/usb/host/isp1760-hcd.c
+++ b/drivers/usb/host/isp1760-hcd.c
@@ -38,6 +38,7 @@ struct isp1760_hcd {
   unsignedi_thresh;
   unsigned long   reset_done;
   unsigned long   next_statechange;
+  unsigned intdevflags;
 };
 
 static inline struct isp1760_hcd *hcd_to_priv(struct usb_hcd *hcd)
@@ -378,9 +379,31 @@ static int isp1760_hc_setup(struct usb_hcd *hcd)
 {
   struct isp1760_hcd *priv = hcd_to_priv(hcd);
   int result;
-  u32 scratch;
+  u32 scratch, hwmode;
+
+  /* Setup HW Mode Control: This assumes a level active-low interrupt */
+  hwmode = HW_DATA_BUS_32BIT;
+
+  if (priv-devflags  ISP1760_FLAG_BUS_WIDTH_16)
+  hwmode = ~HW_DATA_BUS_32BIT;

If you don't mind, I would prefer having ISP1760_FLAG_BUS_WIDTH_32 and
or conditionally HW_DATA_BUS_32BIT.

+  if (priv-devflags  ISP1760_FLAG_ANALOG_OC)
+  hwmode |= HW_ANA_DIGI_OC;
+  if (priv-devflags  ISP1760_FLAG_DACK_POL_HIGH)
+  hwmode |= HW_DACK_POL_HIGH;
+  if (priv-devflags  ISP1760_FLAG_DREQ_POL_HIGH)
+  hwmode |= HW_DREQ_POL_HIGH;
+
+  /*
+   * We have to set this first in case we're in 16-bit mode.
+   * Write it twice to ensure correct upper bits if switching
+   * to 16-bit mode.
+   */
+  isp1760_writel(hwmode, hcd-regs + HC_HW_MODE_CTRL);
+  isp1760_writel(hwmode, hcd-regs + HC_HW_MODE_CTRL);
 
   isp1760_writel(0xdeadbabe, hcd-regs + HC_SCRATCH_REG);
+  /* Change bus pattern */
+  scratch = isp1760_readl(hcd-regs + HC_CHIP_ID_REG);
   scratch = isp1760_readl(hcd-regs + HC_SCRATCH_REG);
Why do you have to read the chip id here? Is this a dummy read to ensure
something?

   if (scratch != 0xdeadbabe) {
   printk(KERN_ERR ISP1760: Scratch test failed.\n);
@@ -403,17 +426,30 @@ static int isp1760_hc_setup(struct usb_hcd *hcd)
 
   /* Step 11 passed */
 
-  isp1760_writel(INTERRUPT_ENABLE_MASK, hcd-regs + HC_INTERRUPT_REG);
-  isp1760_writel(INTERRUPT_ENABLE_MASK, hcd-regs + HC_INTERRUPT_ENABLE);
+  isp1760_info(priv, bus width: %d, oc: %s\n,
+ (priv-devflags  ISP1760_FLAG_BUS_WIDTH_16) ?
+ 16 : 32, (priv-devflags  ISP1760_FLAG_ANALOG_OC) ?
+ analog : digital);
 
   /* ATL reset */
-  scratch = isp1760_readl(hcd-regs + HC_HW_MODE_CTRL);
-  isp1760_writel(scratch | ALL_ATX_RESET, hcd-regs + HC_HW_MODE_CTRL);
+  isp1760_writel(hwmode | ALL_ATX_RESET, hcd-regs + HC_HW_MODE_CTRL);
   mdelay(10);
-  isp1760_writel(scratch, hcd-regs + HC_HW_MODE_CTRL);
+  isp1760_writel(hwmode, hcd-regs + HC_HW_MODE_CTRL);
 
-  isp1760_writel(PORT1_POWER | PORT1_INIT2, hcd-regs + HC_PORT1_CTRL);
-  mdelay(10);
+  isp1760_writel(INTERRUPT_ENABLE_MASK, hcd-regs + HC_INTERRUPT_REG);
+  isp1760_writel(INTERRUPT_ENABLE_MASK, hcd-regs + HC_INTERRUPT_ENABLE);
+
+  /*
+   * PORT 1 Control register of the ISP1760 is the OTG control
+   * register on ISP1761.
+   */
+  scratch = isp1760_readl(hcd-regs + HC_CHIP_ID_REG);
+  if (((scratch  0x) == 0x1760) 
+  !(priv-devflags  ISP1760_FLAG_PORT1_DIS)) {
+  isp1760_writel(PORT1_POWER | PORT1_INIT2,
+ hcd-regs + HC_PORT1_CTRL);
+  mdelay(10);
+  }
I'm sorry, you can't solve this way. My ISP1760 claims to be an ISP1761
this way :) So I end up with one functional port... The only way to
distinguish between 1760  1761 would be at the probing level.

Sebastian
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH 2/2] USB: isp1760: Support board-specific hardware configurations

2008-05-22 Thread Nathaniel Case 
On Thu, 2008-05-22 at 17:35 +0200, Sebastian Siewior wrote:
 If you don't mind, I would prefer having ISP1760_FLAG_BUS_WIDTH_32 and
 or conditionally HW_DATA_BUS_32BIT.

It doesn't matter much to me -- I just reasoned that a 32-bit bus width
was the norm since it's the hardware default and so having a flags=0
would be desirable for that case.  Did you have something else in mind?

 Why do you have to read the chip id here? Is this a dummy read to ensure
 something?

The idea is to force the data bus lines to change states to protect
against a false success.  E.g., you read back 0xdeadbabe but it was due
to the line states from the previous write rather than coming from the
chip itself (I've seen this when you get the bus chip select or timing
configuration wrong). 

 I'm sorry, you can't solve this way. My ISP1760 claims to be an ISP1761
 this way :) So I end up with one functional port... The only way to
 distinguish between 1760  1761 would be at the probing level.

Yikes -- you're right.  I would have never guessed that the 1760 would
have returned 0x1761, but sure enough that's what the datasheet says.

Thanks for the feedback.

--
Nate Case [EMAIL PROTECTED]

___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev