[U-Boot] [PATCH v4 16/20] tegra: usb: Add support for Tegra USB peripheral

[Simon Glass]

Hi Stephen,

On Wed, Jan 18, 2012 at 4:43 PM, Stephen Warren <swarren at nvidia.com> wrote:
> On 01/11/2012 09:33 PM, Simon Glass wrote:
>> This adds basic support for the Tegra2 USB controller. Board files should
>> call board_usb_init() to set things up.
>
>> diff --git a/arch/arm/cpu/armv7/tegra2/usb.c b/arch/arm/cpu/armv7/tegra2/usb.c
>
>> +/* Record which controller can switch from host to device mode */
>> +static struct fdt_usb *host_dev_ctlr;
>
> As you'll note from my comments on the DT bindings, it doesn't make
> sense to have a single global variable for that; both USB1 and USB3 can
> switch at run-time apparently, and USB2 at initialization time.
>
>> +void usbf_reset_controller(enum periph_id id, struct usb_ctlr *usbctlr)
> ...
>> +       /*
>> +        * Set USB1_NO_LEGACY_MODE to 1, Registers are accessible under
>> +        * base address
>> +        */
>> +       if (id == PERIPH_ID_USBD)
>> +               setbits_le32(&usbctlr->usb1_legacy_ctrl, USB1_NO_LEGACY_MODE);
> ...
>> +       /* Set USB3 to use UTMIP PHY */
>> +       if (id == PERIPH_ID_USB3)
>> +               setbits_le32(&usbctlr->susp_ctrl, UTMIP_PHY_ENB);
> ...
>> +static void init_usb_controller(enum periph_id id, struct usb_ctlr *usbctlr,
>> +               const u32 timing[])
> ...
>> +       /*
>> +        * To Use the A Session Valid for cable detection logic, VBUS_WAKEUP
>> +        * mux must be switched to actually use a_sess_vld threshold.
>> +        */
>> +       if (id == PERIPH_ID_USBD) {
>> +               clrsetbits_le32(&usbctlr->usb1_legacy_ctrl,
>> +                       VBUS_SENSE_CTL_MASK, VBUS_SENSE_CTL_A_SESS_VLD);
>> +       }
>
> Uggh. The driver shouldn't really be altering its behaviour based on
> knowledge of the instance number. Can those conditions be modified to
> test something else instead?
>
> For the first (USBD) condition, it seems like if the registers are
> different, we should really have either different compatible values, or
> a flag in the DT to indicate this.
>
> For the second (USB3) condition, shouldn't this be testing the phy type
> field, not the instance ID?
>
> For the third (USBD) condition, can it test whether there's a VBUS GPIO
> defined, or whether an appropriate combination of my proposed
> enable-host-mode/enable-device-mode properties are present?

Yes I'm pretty sure I can clean this up to make it more fdt-dependent.
Will have a look.

Regards,
Simon

>
> --
> nvpublic