[U-Boot] [PATCH] ulpi: add generic ULPI functionality

[Igor Grinberg]

Hi Jana,

On 11/12/11 03:09, Jana Rapava wrote:
> 
> 
> 2011/11/8 Igor Grinberg <grinberg at compulab.co.il <mailto:grinberg at compulab.co.il>>
> 
> 
>     > +/*
>     > + * This is a family of wrapper functions which sets bits in ULPI registers.
>     > + * Access mode could be WRITE, SET or CLEAR.
> 
>     What about READ?
>     I know it can be done from any of those registers, but it is confusing.
> 
>     > + * For further informations see ULPI 1.1 specification.
>     > + */
>     > +void ulpi_otg_ctrl_flags
>     > +     (struct usb_ehci *ehci, struct ulpi_regs *ulpi, int access_mode, u32 flags)
>     > +{
>     > +     switch (access_mode) {
>     > +     case WRITE:
>     > +             ulpi_write(ehci, (u32)&ulpi->otg_ctrl_write, flags);
>     > +             break;
>     > +     case SET:
>     > +             ulpi_write(ehci, (u32)&ulpi->otg_ctrl_set, flags);
>     > +             break;
>     > +     case CLEAR:
>     > +             ulpi_write(ehci, (u32)&ulpi->otg_ctrl_clear, flags);
>     > +             break;
>     > +     }
>     > +}
>     > +
>     > +void ulpi_function_ctrl_flags
>     > +     (struct usb_ehci *ehci, struct ulpi_regs *ulpi, int access_mode, u32 flags)
>     > +{
>     > +     switch (access_mode) {
>     > +     case WRITE:
>     > +             ulpi_write(ehci, (u32)&ulpi->function_ctrl_write, flags);
>     > +             break;
>     > +     case SET:
>     > +             ulpi_write(ehci, (u32)&ulpi->function_ctrl_set, flags);
>     > +             break;
>     > +     case CLEAR:
>     > +             ulpi_write(ehci, (u32)&ulpi->function_ctrl_clear, flags);
>     > +             break;
>     > +     }
>     > +}
>     > +
>     > +void ulpi_iface_ctrl_flags
>     > +     (struct usb_ehci *ehci, struct ulpi_regs *ulpi, int access_mode, u32 flags)
>     > +{
>     > +     switch (access_mode) {
>     > +     case WRITE:
>     > +             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_write, flags);
>     > +             break;
>     > +     case SET:
>     > +             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_set, flags);
>     > +             break;
>     > +     case CLEAR:
>     > +             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_clear, flags);
>     > +             break;
>     > +     }
>     > +
>     > +}
> 
>     All the above functions are just making users hard to understand
>     what they should do and what information they should know and
>     supply to the driver.
>     More function oriented API would be much more useful here.
> 
> 
> Please, could you be more specific? I would be thankful, because I know only Linux kernel ULPI API, where driver has to fill otg_transciever structure with bits it needs to set (if I understand it well) and I would like to use something more lightweight here.

Please, don't write long long lines, they are extremely hard to read.

Now regarding the API:
User needs to use the *functionality* without having the need to
look into the ULPI spec to figure out the bits that are needed
for that functionality (I'm not sure it is possible, but we should try).
What I suggest is to look into your use case with mx5,
identify the functionality that is needed for it to function properly
and implement it.

Currently, in your use case, I can identify following functionalities:
1) selecting the transceiver type
2) selecting vbus indicator type
3) Dp/Dm pull downs enable/disable
4) selecting operation mode
5) suspend/resume - can be useful for usb start/stop commands
6) reset
7) drive vbus internal/external
8) ...

You are writing a generic driver, right?
Then you should also go through the ULPI spec and see what
functionalities it provides.
Of course you don't have to implement all the functionalities
described by the spec (although it would be very nice of you),
but at least those that your board uses and in a generic way.

> 
>     > +
>     > +void ulpi_init(struct usb_ehci *ehci, struct ulpi_regs *ulpi)
>     > +{
>     > +     u32 tmp = 0;
>     > +     int reg;
>     > +
>     > +     /* Assemble ID from four ULPI ID registers (8 bits each). */
>     > +     for (reg = ULPI_ID_REGS_COUNT - 1; reg >= 0; reg--)
>     > +             tmp |= ulpi_read(ehci, reg) << (reg * 8);
>     > +
>     > +     /* Split ID into vendor and product ID. */
>     > +     debug("Found ULPI TX, ID %04x:%04x\n", tmp >> 16, tmp & 0xffff);
> 
>     What is TX? Is it transceiver or xceiver or both in one?
> 
> 
> According to ULPI 1.1 specification, it is transceiver.

then can it be transceiver instead of TX?


-- 
Regards,
Igor.