Subject: Re: [PATCH 08/11] powerpc/mpc5121: add USB host support

From: Anatolij Gustschin <[email-masked]>
Date: Mon, 25 Jan 2010 18:00:24 +0100

On Thu, 21 Jan 2010 10:43:34 -0700
Grant Likely <[email-masked]> wrote:

> > diff --git a/Documentation/powerpc/dts-bindings/fsl/usb.txt b/Documentation/powerpc/dts-bindings/fsl/usb.txt
> > index b001524..9050154 100644
> > --- a/Documentation/powerpc/dts-bindings/fsl/usb.txt
> > +++ b/Documentation/powerpc/dts-bindings/fsl/usb.txt
> > @@ -33,6 +33,14 @@ Recommended properties :
> >  - interrupt-parent : the phandle for the interrupt controller that
> >    services interrupts for this device.
> >
> > +Optional properties :
> 
> > + - big-endian-regs : boolean; if defined, indicates the USB host
> > +   controller registers format is big endian.
> 
> Rather than testing for this explicitly, add fsl,mpc5121-usb2-dr to
> the match table and use the .data pointer for setting device specific
> quirks.

There is no match table. fsl_usb_of_init() is an arch_initcall and
tests other properties using the same approach.

> > + - invert-drvvbus : boolean; for MPC5121 only. Indicates the port
> > +   power polarity of internal PHY signal DRVVBUS is inverted.
> > + - invert-pwr-fault : boolean; for MPC5121 only. Indicates the
> > +   PWR_FAULT signal polarity is inverted.
> 
> These are also characteristics of the chip, not the board, right?  If
> so then these also can be determined implicitly by the compatible
> value.

No, these are characteristics of the board. The internal USB PHY doesn't
provide supply voltage. Some boards use MIC2025 switches which require active
high DRVVBUS and active low PWR_FAULT. Some boards could use MIC2026 or
MAX1838 which require other polarities.

> Finally, these are all freescale specific properties.  If you still
> need them, then prefix the property names with 'fsl,'

OK.

> > ...
> > +
> > +config USB_FSL_BIG_ENDIAN_MMIO
> > +       bool
> 
> What's this for?

This is currently unused (will be used later), I will remove it for now.

> > ...
> > @@ -77,14 +77,13 @@ static int usb_hcd_fsl_probe(const struct hc_driver *driver,
> >                return -ENODEV;
> >        }
> >
> > -       res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > -       if (!res) {
> > +       irq = platform_get_irq(pdev, 0);
> > +       if (irq < 0) {
> >                dev_err(&pdev->dev,
> >                        "Found HC with no IRQ. Check %s setup!\n",
> >                        dev_name(&pdev->dev));
> >                return -ENODEV;
> >        }
> > -       irq = res->start;
> 
> Put this hunk in a separate patch.

OK.

> > ...
> > +       if (pdata->have_sysif_regs) {
> >  #ifdef CONFIG_PPC_85xx
> > -       out_be32(non_ehci + FSL_SOC_USB_PRICTRL, 0x00000008);
> > -       out_be32(non_ehci + FSL_SOC_USB_AGECNTTHRSH, 0x00000080);
> > +               out_be32(non_ehci + FSL_SOC_USB_PRICTRL, 0x00000008);
> > +               out_be32(non_ehci + FSL_SOC_USB_AGECNTTHRSH, 0x00000080);
> >  #else
> > -       out_be32(non_ehci + FSL_SOC_USB_PRICTRL, 0x0000000c);
> > -       out_be32(non_ehci + FSL_SOC_USB_AGECNTTHRSH, 0x00000040);
> > +               out_be32(non_ehci + FSL_SOC_USB_PRICTRL, 0x0000000c);
> > +               out_be32(non_ehci + FSL_SOC_USB_AGECNTTHRSH, 0x00000040);
> >  #endif
> > -       out_be32(non_ehci + FSL_SOC_USB_SICTRL, 0x00000001);
> > +               out_be32(non_ehci + FSL_SOC_USB_SICTRL, 0x00000001);
> > +       }
> >  }
> 
> Unrelated whitespace changes.  Put in separate patch.

OK.

Thanks,

Anatolij