[U-Boot] [PATCH v2 1/4] ehci-omap: Clean up added ehci-omap.c
Igor Grinberg
grinberg at compulab.co.il
Sun Jan 29 10:42:56 CET 2012
Hi Govindraj,
Put Remy on Cc.
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?
On 01/27/12 15:29, Govindraj wrote:
> On Wed, Jan 25, 2012 at 7:10 PM, Igor Grinberg<grinberg at compulab.co.il> wrote:
[...]
>>
>> My suggestion for the change is:
>> 1) introduce some kind of
>> struct ulpi_viewport {
>> u32 viewport_addr;
>> uint portnum;
>> }
>>
>> 2) use the above struct _instead_ of the "u32 ulpi_viewport" parameter
>>
>> Another way, would be instead of uint portnum, use void *private_data,
>> but I think it will just complicate things too much and there will be no
>> real benefit (and also will add, otherwise needless, castings).
>> If the above structure will not be enough for some platform,
>> it can be extended easily and without changing the API anymore.
>>
>
> Thanks for the suggestions.
>
> Here [1] is the first attempt to implement the same.
>
> sorry for late reply, as I was preempted with some other work.
Thanks for the work you've done!
Overall it looks very good, though several tiny comments below.
>
> --
> Thanks,
> Govindraj.R
>
> [1]:
>
>> From 2bc311c26572aff3ebaac035106434b444692f68 Mon Sep 17 00:00:00 2001
> From: "Govindraj.R"<govindraj.raja at ti.com>
> Date: Fri, 27 Jan 2012 18:34:11 +0530
> Subject: [PATCH] ulpi: Modify the ulpi framework to accept port number
>
> Based on discussion in the thread [1] this patch
> attempts to modify the ulpi framework to accept the
> port number along with view port address, adding the omap-ulpi
> view port that helps us in using the generic ulpi
> framework to configure ulpi phy using the INSNREG05_ULPI viewport
> reg available on omap platform.
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)
>
> 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]?
>
> 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?
> 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..)
> 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;
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?
> }
>
> @@ -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.
>
> 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?
> +
> +/*
> + * 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...
Also, you should mask the port_num:
ulpi_vp->port_num & 0xf
as for this field is 4 bits wide.
> +
> + 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
> +
> + 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).
>
> - 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.
> 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...
> +};
> +
> +/*
> * 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.
[...]
--
Regards,
Igor.
More information about the U-Boot
mailing list