[U-Boot] [PATCH v2 1/4] ehci-omap: Clean up added ehci-omap.c
Govindraj
govindraj.ti at gmail.com
Fri Feb 3 13:50:24 CET 2012
Hi Igor,
Sorry for late reply, Was really busy with some other tasks.
On Sun, Jan 29, 2012 at 3:12 PM, Igor Grinberg <grinberg at compulab.co.il> wrote:
> Hi Govindraj,
>
> Put Remy on Cc.
>
Yes Sure,
> Note: my new mail client for some reason, messes with white spaces...
> (Thunderbird (9) just gets better and better...)
> So, please ignore those.
> BTW, is there anyone who has those issues fixed and can save me a wasted
> hour?
>
[...]
>
> Ok, I think you should split this patch to:
> 1) modify the ulpi framework... (this one extends the ulpi functionality
> and fixes current users)
> 2) add omap ulpi support (this one makes use of the ulpi framework
> on omap platforms and IMO, should amended to you original
> 1/4 patch: ehci-omap: Clean up added ehci-omap.c)
Yes Correct, I have done that will be posting the complete
series in short while.
>
>
>>
>> Tested on beagle-xm and Panda.
>> Compile tested on efikamx platform.
>>
>> This patch is based on earlier posted patch series[1].
>
>
> Shouldn't this be [2]?
yes correct my mistake.
>
>
>>
>> Prior to this patch ulpi reset was done directly now
>> it is done using ulpi_reset.
>>
>> Thanks to Igor Grinberg<grinberg at compulab.co.il> for reviewing the
>> omap-ehci patches and suggesting this approach.
>>
>> This patch along with the patch series [1] rebased on latest u-boot
>
>
> Probably same here, or am I mistaken?
yes you are right, it should be [2]
>
>
>> is available here.
>> git://gitorious.org/denx_u-boot/denx_uboot_omap.git v2_ehci_omap4
>>
>> [1]: http://www.mail-archive.com/u-boot@lists.denx.de/msg76076.html
>> [2]: "[PATCH v2 0/4] Clean up ehci-omap and extend support for omap3/4
>> socs"
>>
>> Signed-off-by: Govindraj.R<govindraj.raja at ti.com>
>> ---
>> board/efikamx/efikamx-usb.c | 21 ++++---
>> drivers/usb/host/ehci-omap.c | 21 ++-----
>> drivers/usb/ulpi/Makefile | 1 +
>> drivers/usb/ulpi/omap-ulpi-viewport.c | 110
>> +++++++++++++++++++++++++++++++++
>> drivers/usb/ulpi/ulpi-viewport.c | 30 +++++-----
>> drivers/usb/ulpi/ulpi.c | 54 ++++++++--------
>> include/configs/omap3_beagle.h | 3 +
>> include/configs/omap4_panda.h | 2 +
>> include/usb/ulpi.h | 37 ++++++++----
>
>
> You should also, update the README file with the new
> CONFIG_USB_ULPI_VIEWPORT_OMAP option.
> (Also may be some OMAP specific doc files..)
>
yes fine, I am adding a new doc
README.omap-ulpi-viewport
and adding some minimal info on the omap-ulpi-viewport
usage and usecase.
>
>
>> 9 files changed, 202 insertions(+), 77 deletions(-)
>> create mode 100644 drivers/usb/ulpi/omap-ulpi-viewport.c
>>
>> diff --git a/board/efikamx/efikamx-usb.c b/board/efikamx/efikamx-usb.c
>> index 840bd9a..bb1398b 100644
>> --- a/board/efikamx/efikamx-usb.c
>> +++ b/board/efikamx/efikamx-usb.c
>> @@ -120,6 +120,7 @@ static void efika_ehci_init(struct usb_ehci *ehci,
>> uint32_t stp_gpio,
>> {
>> int ret;
>> struct ulpi_regs *ulpi = (struct ulpi_regs *)0;
>> + struct ulpi_viewport ulpi_vp;
>>
>> mxc_request_iomux(stp_gpio, alt0);
>> mxc_iomux_set_pad(stp_gpio, PAD_CTL_DRV_HIGH |
>> @@ -133,23 +134,25 @@ static void efika_ehci_init(struct usb_ehci
>> *ehci, uint32_t stp_gpio,
>> mxc_iomux_set_pad(stp_gpio, USB_PAD_CONFIG);
>> udelay(10000);
>>
>> - ret = ulpi_init((u32)&ehci->ulpi_viewpoint);
>> + ulpi_vp.viewport_addr = (u32)&ehci->ulpi_viewpoint;
>
>
> here, you should also:
>
> + ulpi_vp.port_num = 0;
okay.
>
> because the ulpi_vp structure is not initialized and can have garbage in it.
>
>> +
>> + ret = ulpi_init(&ulpi_vp);
>> if (ret) {
>> printf("Efika USB ULPI initialization failed\n");
>> return;
>> }
>>
>> /* ULPI set flags */
>> - ulpi_write((u32)&ehci->ulpi_viewpoint,&ulpi->otg_ctrl,
>> + ulpi_write(&ulpi_vp,&ulpi->otg_ctrl,
>>
>> ULPI_OTG_DP_PULLDOWN | ULPI_OTG_DM_PULLDOWN |
>> ULPI_OTG_EXTVBUSIND);
>> - ulpi_write((u32)&ehci->ulpi_viewpoint,&ulpi->function_ctrl,
>> + ulpi_write(&ulpi_vp,&ulpi->function_ctrl,
>>
>> ULPI_FC_FULL_SPEED | ULPI_FC_OPMODE_NORMAL |
>> ULPI_FC_SUSPENDM);
>> - ulpi_write((u32)&ehci->ulpi_viewpoint,&ulpi->iface_ctrl, 0);
>> + ulpi_write(&ulpi_vp,&ulpi->iface_ctrl, 0);
>>
>>
>> /* Set VBus */
>> - ulpi_write((u32)&ehci->ulpi_viewpoint,&ulpi->otg_ctrl_set,
>> + ulpi_write(&ulpi_vp,&ulpi->otg_ctrl_set,
>>
>> ULPI_OTG_DRVVBUS | ULPI_OTG_DRVVBUS_EXT);
>>
>> /*
>> @@ -158,7 +161,7 @@ static void efika_ehci_init(struct usb_ehci *ehci,
>> uint32_t stp_gpio,
>> * NOTE: This violates USB specification, but otherwise, USB on
>> Efika
>> * doesn't work.
>> */
>> - ulpi_write((u32)&ehci->ulpi_viewpoint,&ulpi->otg_ctrl_set,
>> + ulpi_write(&ulpi_vp,&ulpi->otg_ctrl_set,
>> ULPI_OTG_CHRGVBUS);
>
>
> This fits 80 characters, right? Can this now be on the same line?
sure.
>
>> }
>>
>> @@ -177,9 +180,11 @@ void ehci_powerup_fixup(uint32_t *status_reg,
>> uint32_t *reg)
>> uint32_t port = OTG_BASE_ADDR + (0x200 * CONFIG_MXC_USB_PORT);
>> struct usb_ehci *ehci = (struct usb_ehci *)port;
>> struct ulpi_regs *ulpi = (struct ulpi_regs *)0;
>> + struct ulpi_viewport ulpi_vp;
>>
>> - ulpi_write((u32)&ehci->ulpi_viewpoint,&ulpi->otg_ctrl_set,
>>
>> - ULPI_OTG_CHRGVBUS);
>> + ulpi_vp.viewport_addr = (u32)&ehci->ulpi_viewpoint;
>
>
> same here:
>
> + ulpi_vp.port_num = 0;
>
>> +
>> + ulpi_write(&ulpi_vp,&ulpi->otg_ctrl_set, ULPI_OTG_CHRGVBUS);
>>
>> wait_ms(50);
>>
>
> [...]
>
>
>> diff --git a/drivers/usb/ulpi/Makefile b/drivers/usb/ulpi/Makefile
>> index d43b229..67d5e5e 100644
>> --- a/drivers/usb/ulpi/Makefile
>> +++ b/drivers/usb/ulpi/Makefile
>> @@ -24,6 +24,7 @@ LIB := $(obj)libusb_ulpi.o
>>
>> COBJS-$(CONFIG_USB_ULPI) += ulpi.o
>> COBJS-$(CONFIG_USB_ULPI_VIEWPORT) += ulpi-viewport.o
>> +COBJS-$(CONFIG_USB_OMAP_ULPI_VIEWPORT) += omap-ulpi-viewport.o
>
>
> IMO, CONFIG_USB_ULPI_VIEWPORT_OMAP will look better here, but
> you can leave it as it is - it does not really meter.
>
yes fine corrected.
>
>>
>> COBJS := $(COBJS-y)
>> SRCS := $(COBJS:.o=.c)
>> diff --git a/drivers/usb/ulpi/omap-ulpi-viewport.c
>> b/drivers/usb/ulpi/omap-ulpi-viewport.c
>> new file mode 100644
>> index 0000000..1718788
>> --- /dev/null
>> +++ b/drivers/usb/ulpi/omap-ulpi-viewport.c
>> @@ -0,0 +1,110 @@
>
>
> [...]
>
>> +/* ULPI viewport control bits */
>> +#define OMAP_ULPI_PORT_SHIFT 24
>> +
>> +#define OMAP_ULPI_WR_OPSEL (3<< 21)
>> +#define OMAP_ULPI_ACCESS (1<< 31)
>> +
>> +/*
>> + * Wait for the ULPI Access to complete
>> + */
>> +static int ulpi_wait(struct ulpi_viewport *ulpi_vp, u32 mask)
>> +{
>> + int timeout = CONFIG_USB_ULPI_TIMEOUT;
>> +
>> + while (--timeout) {
>> + if ((readl(ulpi_vp->viewport_addr)& mask))
>>
>> + return 0;
>> +
>> + udelay(1);
>> + }
>> +
>> + return ULPI_ERROR;
>> +}
>> +
>> +/*
>> + * Wake the ULPI PHY up for communication
>> + *
>> + * returns 0 on success.
>> + */
>> +static int ulpi_wakeup(struct ulpi_viewport *ulpi_vp)
>> +{
>> + int err;
>> +
>> + if (readl(ulpi_vp->viewport_addr)& OMAP_ULPI_ACCESS)
>>
>> + return 0; /* already awake */
>> +
>> + writel(OMAP_ULPI_ACCESS, ulpi_vp->viewport_addr);
>> +
>> + err = ulpi_wait(ulpi_vp, OMAP_ULPI_ACCESS);
>> + if (err)
>> + debug("ULPI wakeup timed out\n");
>> +
>> + return err;
>> +}
>
>
> This function looks really weird...
> You write the OMAP_ULPI_ACCESS bit, but you don't specify the port...
> Now, if you will specify the port, what access type (read/write) should it
> be?
> So, I don't really know what whould be the consequence of that access...
>
> TRM does not talk about waking the ULPI PHY at all or have I missed it...
> I think this function is unneeded on OMAP - it looks like OMAP wakes
> the ULPI PHY automatically as soon as you access it for read/write,
> but the TRM does not talk too much on that...
>
> Have you tried without this function?
> Just calling ulpi_wait() to verify that the previous access has completed,
> should do the job, no? Can you, please, check that?
>
Actually we have to start the ulpi access first, then we can write
port number and mode to be used and after writing we can check for access
done bit. (checking read/write op was successful can be polled for
access done bit)
without ulpi_wake access done was not happening and timing out.
>
>> +
>> +/*
>> + * Issue a ULPI read/write request
>> + */
>> +static int ulpi_request(struct ulpi_viewport *ulpi_vp, u32 value)
>> +{
>> + int err;
>> +
>> + err = ulpi_wakeup(ulpi_vp);
>> + if (err)
>> + return err;
>> +
>> + writel(value, ulpi_vp->viewport_addr);
>> +
>> + err = ulpi_wait(ulpi_vp, OMAP_ULPI_ACCESS);
>> + if (err)
>> + debug("ULPI request timed out\n");
>> +
>> + return err;
>> +}
>> +
>> +int ulpi_write(struct ulpi_viewport *ulpi_vp, u8 *reg, u32 value)
>> +{
>> + u32 val = (ulpi_vp->port_num<< OMAP_ULPI_PORT_SHIFT) |
>> + OMAP_ULPI_WR_OPSEL |
>> + ((u32)reg<< 16) | (value& 0xff);
>
>
> If you use OMAP_ULPI_PORT_SHIFT,
> then probably 16 should be also defined in the same way?
> Or... I would just drop the OMAP_ULPI_PORT_SHIFT, because
> it is clear that 24 is the offset of port number...
>
okay, retaining as just 24
> Also, you should mask the port_num:
> ulpi_vp->port_num & 0xf
> as for this field is 4 bits wide.
>
corrected.
>
>> +
>> + return ulpi_request(ulpi_vp, val);
>> +}
>> +
>> +u32 ulpi_read(struct ulpi_viewport *ulpi_vp, u8 *reg)
>> +{
>> + int err;
>> + u32 val = (ulpi_vp->port_num<< OMAP_ULPI_PORT_SHIFT) |
>> + OMAP_ULPI_WR_OPSEL |
>> + ((u32)reg<< 16);
>
>
> same here
corrected.
>
>> +
>> + err = ulpi_request(ulpi_vp, val);
>> + if (err)
>> + return err;
>> +
>> + return readl(ulpi_vp->viewport_addr)& 0xff;
>>
>> +}
>> diff --git a/drivers/usb/ulpi/ulpi-viewport.c
>> b/drivers/usb/ulpi/ulpi-viewport.c
>> index 490fb0e..6f03f08 100644
>> --- a/drivers/usb/ulpi/ulpi-viewport.c
>> +++ b/drivers/usb/ulpi/ulpi-viewport.c
>> @@ -40,13 +40,13 @@
>
>
> [...]
>
>
>> -int ulpi_write(u32 ulpi_viewport, u8 *reg, u32 value)
>> +int ulpi_write(struct ulpi_viewport *ulpi_vp, u8 *reg, u32 value)
>> {
>> u32 val = ULPI_RWRUN | ULPI_RWCTRL | ((u32)reg<< 16) | (value&
>> 0xff);
>
>
> You have extended the ULPI framework - now it has support for port number...
> here, you should also:
> val |= (ulpi_vp->port_num & 0x7) << 24;
>
> (yes, 24 is the correct offset and it is 3 bits wide according to the iMX35
> RM).
>
okay, corrected this.
>
>>
>> - return ulpi_request(ulpi_viewport, val);
>> + return ulpi_request(ulpi_vp, val);
>> }
>>
>> -u32 ulpi_read(u32 ulpi_viewport, u8 *reg)
>> +u32 ulpi_read(struct ulpi_viewport *ulpi_vp, u8 *reg)
>> {
>> int err;
>> u32 val = ULPI_RWRUN | ((u32)reg<< 16);
>
>
> same here
>
>>
>> - err = ulpi_request(ulpi_viewport, val);
>> + err = ulpi_request(ulpi_vp, val);
>> if (err)
>> return err;
>>
>> - return (readl(ulpi_viewport)>> 8)& 0xff;
>> + return (readl(ulpi_vp->viewport_addr)>> 8)& 0xff;
>>
>> }
>> diff --git a/drivers/usb/ulpi/ulpi.c b/drivers/usb/ulpi/ulpi.c
>> index 6202227..dde2585 100644
>> --- a/drivers/usb/ulpi/ulpi.c
>> +++ b/drivers/usb/ulpi/ulpi.c
>> @@ -37,18 +37,18 @@
>
>
> [...]
>
>
>> -int ulpi_reset(u32 ulpi_viewport)
>> +int ulpi_reset(struct ulpi_viewport *ulpi_vp)
>> {
>> int err;
>>
>> - err = ulpi_write(ulpi_viewport,
>> + err = ulpi_write(ulpi_vp,
>> &ulpi->function_ctrl_set, ULPI_FC_RESET);
>
>
> This fits 80 characters now. Please, make it on the same line.
sure.
>
>
>> if (err) {
>> printf("ULPI: %s: failed writing reset bit\n", __func__);
>> return err;
>> }
>>
>> - return ulpi_reset_wait(ulpi_viewport);
>> + return ulpi_reset_wait(ulpi_vp);
>> }
>> diff --git a/include/configs/omap3_beagle.h
>> b/include/configs/omap3_beagle.h
>> index b4d6443..2183ea6 100644
>> --- a/include/configs/omap3_beagle.h
>> +++ b/include/configs/omap3_beagle.h
>> @@ -130,6 +130,9 @@
>> #define CONFIG_CMD_USB
>> #define CONFIG_USB_EHCI
>> #define CONFIG_USB_EHCI_OMAP
>> +#define CONFIG_USB_ULPI
>> +#define CONFIG_USB_OMAP_ULPI_VIEWPORT
>> +
>> /*#define CONFIG_EHCI_DCACHE*/ /* leave it disabled for now */
>> #define CONFIG_OMAP_EHCI_PHY1_RESET_GPIO 147
>> #define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS 3
>> diff --git a/include/configs/omap4_panda.h b/include/configs/omap4_panda.h
>> index 23c0230..0228a66 100644
>> --- a/include/configs/omap4_panda.h
>> +++ b/include/configs/omap4_panda.h
>> @@ -38,6 +38,8 @@
>> #define CONFIG_USB_HOST
>> #define CONFIG_USB_EHCI
>> #define CONFIG_USB_EHCI_OMAP
>> +#define CONFIG_USB_ULPI
>> +#define CONFIG_USB_OMAP_ULPI_VIEWPORT
>> #define CONFIG_USB_STORAGE
>> #define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS 3
>
>
> This also should apply for TAM3517 board.
> Stefano, can you test this on TAM3517?
>
> Probably Ilya is also interested in this for HTKW mcx board.
> Adding both to Cc.
>
>
>>
>> diff --git a/include/usb/ulpi.h b/include/usb/ulpi.h
>> index 802f077..1da43ae 100644
>> --- a/include/usb/ulpi.h
>> +++ b/include/usb/ulpi.h
>> @@ -28,12 +28,24 @@
>> #endif
>>
>> /*
>> + * ulpi view port address and
>> + * Port_number that can be passed.
>> + * Any additional data to be passed can
>> + * be extended from this structure
>> + */
>> +struct ulpi_viewport {
>> + u32 viewport_addr;
>> + u8 port_num;
>
>
> You shift this by 24 bits...
> Can this be u32 instead, to remove any confusion?
> It can also spare some alignment bugs with some compilers/linkers...
>
yes fine.
>
>> +};
>> +
>> +/*
>> * Initialize the ULPI transciever and check the interface integrity.
>> - * @ulpi_viewport - the address of the ULPI viewport register.
>> + * @ulpi_viewport - structure containing the address of the ULPI
>> viewport
>> + register and port number to access.
>
>
> Please, do not describe here the internal fields of the structure,
> it can be done better along with structure definition.
> IMO, simply:
> 'structure containing ULPI viewport data'
> should be enough.
>
Okay fine.
--
Thanks,
Govindraj.R
More information about the U-Boot
mailing list