[U-Boot] [PATCH v4 0/3] AM335x: Add USB support in u-boot.

These patches add USB support in u-boot for AM335x. The support for host or device is selected depending on the config selected from boards.cfg file. Host mode is selected for USB1 and device mode is selected for USB0. Base addresses are selected accordingly.
Gene Zarkhin (1): AM335x : Add USB support for AM335x in u-boot
Harman Sohanpal (2): AM335x : Configs to add USB host support. musb_udc : Fix compile warning.
drivers/usb/musb/Makefile | 1 + drivers/usb/musb/am335x.c | 121 ++++++++++++++++++++++++++++++++++++++++++ drivers/usb/musb/am335x.h | 113 +++++++++++++++++++++++++++++++++++++++ drivers/usb/musb/musb_core.h | 2 + drivers/usb/musb/musb_hcd.h | 3 - include/configs/am335x_evm.h | 41 ++++++++++++++ include/usb.h | 3 +- 7 files changed, 280 insertions(+), 4 deletions(-) create mode 100644 drivers/usb/musb/am335x.c create mode 100644 drivers/usb/musb/am335x.h

From: Gene Zarkhin gene_zarkhin@bose.com
Adds USB support in uboot for AM335x. The support for USB0 and USB1 can be added dynamically by changing the boards.cfg file for USB0 or USB1. This patch adds support for USB0. USB 1 has a full size connector so acts in host mode and USB 0 has a mini connector so used in device mode.
Signed-off-by: Gene Zarkhin gene_zarkhin@bose.com Signed-off-by: Harman Sohanpal harman_sohanpal@ti.com --- Changes for v2: - none Changes for v3: - Changed commit message to specify why USB 1 has been enabled by default. Changes for v4: - Selected USB module from board.cfg. - Cleaned up indentation. - Base address of USB selected according to config selected in boards.cfg.
boards.cfg | 2 +- drivers/usb/musb/Makefile | 1 + drivers/usb/musb/am335x.c | 121 ++++++++++++++++++++++++++++++++++++++++++++ drivers/usb/musb/am335x.h | 122 +++++++++++++++++++++++++++++++++++++++++++++ include/usb.h | 3 +- 5 files changed, 247 insertions(+), 2 deletions(-) create mode 100644 drivers/usb/musb/am335x.c create mode 100644 drivers/usb/musb/am335x.h
diff --git a/boards.cfg b/boards.cfg index 9ef903a..35aec79 100644 --- a/boards.cfg +++ b/boards.cfg @@ -183,7 +183,7 @@ versatileqemu arm arm926ejs versatile armltd integratorap_cm946es arm arm946es integrator armltd - integratorap:CM946ES integratorcp_cm946es arm arm946es integrator armltd - integratorcp:CM946ES ca9x4_ct_vxp arm armv7 vexpress armltd -am335x_evm arm armv7 am335x ti am33xx +am335x_evm arm armv7 am335x ti am33xx am335x_evm:AM335X_USB0 highbank arm armv7 highbank - highbank efikamx arm armv7 efikamx - mx5 efikamx:MACH_TYPE=MACH_TYPE_MX51_EFIKAMX,IMX_CONFIG=board/efikamx/imximage_mx.cfg efikasb arm armv7 efikamx - mx5 efikamx:MACH_TYPE=MACH_TYPE_MX51_EFIKASB,IMX_CONFIG=board/efikamx/imximage_sb.cfg diff --git a/drivers/usb/musb/Makefile b/drivers/usb/musb/Makefile index 20b5503..d00ec40 100644 --- a/drivers/usb/musb/Makefile +++ b/drivers/usb/musb/Makefile @@ -32,6 +32,7 @@ COBJS-$(CONFIG_USB_DAVINCI) += davinci.o COBJS-$(CONFIG_USB_OMAP3) += omap3.o COBJS-$(CONFIG_USB_DA8XX) += da8xx.o COBJS-$(CONFIG_USB_AM35X) += am35x.o +COBJS-$(CONFIG_USB_AM335X) += am335x.o
COBJS := $(COBJS-y) SRCS := $(COBJS:.o=.c) diff --git a/drivers/usb/musb/am335x.c b/drivers/usb/musb/am335x.c new file mode 100644 index 0000000..4b59769 --- /dev/null +++ b/drivers/usb/musb/am335x.c @@ -0,0 +1,121 @@ +/* + * am335x.c - TI's AM335x platform specific usb wrapper functions. + * + * Author: gene Zarkhin gene_zarkhin@bose.com + * Modified by: Harman Sohanpal harman_sohanpal@ti.com + * + * Based on drivers/usb/musb/da8xx.c + * + * Copyright (c) 2012 Texas Instruments Incorporated + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + */ +#include <common.h> +#include "am335x.h" + +/* MUSB platform configuration */ +struct musb_config musb_cfg = { + .regs = (struct musb_regs *)(AM335X_USB_OTG_CORE_BASE), + .timeout = AM335X_USB_OTG_TIMEOUT, + .musb_speed = 0, +}; + +/* + * Enable the USB phy + */ +static u8 phy_on(void) +{ + u32 timeout; + u32 regAddr = CM_REGISTERS + USB_CTRL0_REG_OFFSET; + u32 usb_ctrl_reg; + + usb_ctrl_reg = readl(regAddr); + usb_ctrl_reg &= ~(CM_PHY_PWRDN | CM_PHY_OTG_PWRDN); + usb_ctrl_reg |= (OTGVDET_EN | OTGSESSENDEN); + writel(usb_ctrl_reg, regAddr); + + timeout = musb_cfg.timeout; + writel(0x1, &am335x_usb_regs->ctrl); + udelay(6000); + while (timeout--) { + if ((readl(&am335x_usb_regs->ctrl) & SOFT_RESET_BIT) == 0) + return 1; + } + /* USB phy was not turned on */ + return 0; +} + +/* + * Disable the USB phy + */ +static void phy_off(void) +{ + u32 regAddr = CM_REGISTERS + USB_CTRL0_REG_OFFSET; + u32 usb_ctrl_reg; + + usb_ctrl_reg = readl(regAddr); + usb_ctrl_reg |= (CM_PHY_PWRDN | CM_PHY_OTG_PWRDN); + writel(usb_ctrl_reg, regAddr); + + /* Disable the USB module */ + writel(PRCM_MODULE_DSBL, CM_PER_USB0_CLKCTRL); +} + +/* + * This function performs platform specific initialization for usb0. + */ +int musb_platform_init(void) +{ + u32 timeout; + u32 revision; + + /* USB */ + /* PLL Gate set up */ + writel(DPLL_CLKDCOLDO_GATE_CTRL, CM_CLKDCOLDO_DPLL_PER); + + /* CLOCK */ + writel(PRCM_MOD_EN, CM_PER_USB0_CLKCTRL); + timeout = musb_cfg.timeout; + while (timeout--) { + if (readl(CM_PER_USB0_CLKCTRL) != PRCM_MOD_EN) + continue; + else + break; + } + if (timeout == 0) { + printf("\nUSB module not enabled\nAborting"); + return -1; + } + + /* USB module fully functional */ + /* start the on-chip usb phy and its pll */ + if (phy_on() == 0) + return -1; + /* Returns zero if e.g. not clocked */ + revision = readl(&am335x_usb_regs->revision); + if (revision == 0) + return -1; + + return 0; +} + +/* + * This function performs platform specific deinitialization for usb0. + */ +void musb_platform_deinit(void) +{ + /* Turn off the phy */ + phy_off(); +} diff --git a/drivers/usb/musb/am335x.h b/drivers/usb/musb/am335x.h new file mode 100644 index 0000000..b772460 --- /dev/null +++ b/drivers/usb/musb/am335x.h @@ -0,0 +1,122 @@ +/* + * am335x.h - TI's AM335x platform specific usb wrapper definitions. + * + * Author: Gene Zarkhin gene_zarkhin@bose.com + * Modified by: Harman Sohanpal harman_sohanpal@ti.com + * + * Based on drivers/usb/musb/da8xx.h + * + * Copyright (c) 2012 Texas Instruments Incorporated + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. + */ + +#ifndef __AM335X_USB_H__ +#define __AM335X_USB_H__ + +#include "musb_core.h" +/* PRCM Definitions */ +#define CM_CLKDCOLDO_DPLL_PER (CM_WKUP + 0x7c) +#define CM_PER_USB0_CLKCTRL (CM_PER + 0x1c) +#define PRCM_MOD_EN 0x2 +#define PRCM_MODULE_DSBL 0x0 + + +/* Control Module Registers */ +#define CM_REGISTERS CTRL_BASE + +#define PRCM_IDLEST 0x30000 +#define DPLL_CLKDCOLDO_GATE_CTRL 0x300 + +/* Base address of musb wrapper */ +#define AM335X_USB0_OTG_BASE 0x47401000 +#define AM335X_USB1_OTG_BASE 0x47401800 + +/* Selecting between USB0 and USB1 */ + +#ifdef CONFIG_AM335X_USB0 +#define AM335X_USB_OTG_BASE AM335X_USB0_OTG_BASE +#define USB_CTRL0_REG_OFFSET 0x620 +#elif defined(CONFIG_AM335X_USB1) +#define AM335X_USB_OTG_BASE AM335X_USB1_OTG_BASE +#define USB_CTRL0_REG_OFFSET 0x628 +#endif + +/* Base address of musb core */ +#define AM335X_USB_OTG_CORE_BASE (AM335X_USB_OTG_BASE + 0x400) + +/* Timeout for AM35x usb module */ +#define AM335X_USB_OTG_TIMEOUT 0x3FFFFFF + +/* + * AM335x platform USB wrapper register overlay. + */ +struct am335x_usb_regs { + u32 revision; /* 0x00 */ + u32 reserved0[4]; + u32 ctrl; /* 0x14 */ + u32 status; /* 0x18 */ + u32 reserved1[1]; + u32 irqmstat; /* 0x20 */ + u32 irqeoi; /* 0x24 */ + u32 irqstatraw0; /* 0x28 */ + u32 irqstatraw1; /* 0x2c */ + u32 irqstat0; /* 0x30 */ + u32 irqstat1; /* 0x34 */ + u32 irqenableset0; /* 0x38 */ + u32 irqenableset1; /* 0x3c */ + u32 irqenableclr0; /* 0x40 */ + u32 irqenableclr1; /* 0x44 */ + u32 reserved2[10]; + u32 txmode; /* 0x70 */ + u32 rxmode; /* 0x74 */ + u32 reserved3[2]; + u32 genrndisep1; /* 0x80 */ + u32 genrndisep2; /* 0x84 */ + u32 genrndisep3; /* 0x88 */ + u32 genrndisep4; /* 0x8c */ + u32 genrndisep5; /* 0x90 */ + u32 genrndisep6; /* 0x94 */ + u32 genrndisep7; /* 0x98 */ + u32 genrndisep8; /* 0x9c */ + u32 genrndisep9; /* 0xa0 */ + u32 genrndisep10; /* 0xa4 */ + u32 genrndisep11; /* 0xa8 */ + u32 genrndisep12; /* 0xac */ + u32 genrndisep13; /* 0xb0 */ + u32 genrndisep14; /* 0xb4 */ + u32 genrndisep15; /* 0xb8 */ + u32 reserved4[5]; + u32 autoreq; /* 0xd0 */ + u32 srpfixtime; /* 0xd4 */ + u32 tdown; /* 0xd8 */ + u32 reserved5[1]; + u32 utmi; /* 0xe0 */ + u32 utmilb; /* 0xe4 */ + u32 mode; /* 0xe8 */ +}; + +#define am335x_usb_regs ((struct am335x_usb_regs *)AM335X_USB_OTG_BASE) + +/* USB 2.0 PHY Control */ +#define CM_PHY_PWRDN (1 << 0) +#define CM_PHY_OTG_PWRDN (1 << 1) +#define OTGVDET_EN (1 << 19) +#define OTGSESSENDEN (1 << 20) + +/* USB CTRL REG FIELDS */ +#define SOFT_RESET_BIT (1 << 0) + +#endif /* __AM335X_USB_H__ */ diff --git a/include/usb.h b/include/usb.h index 6da91e7..13f5434 100644 --- a/include/usb.h +++ b/include/usb.h @@ -141,7 +141,8 @@ struct usb_device { defined(CONFIG_USB_SL811HS) || defined(CONFIG_USB_ISP116X_HCD) || \ defined(CONFIG_USB_R8A66597_HCD) || defined(CONFIG_USB_DAVINCI) || \ defined(CONFIG_USB_OMAP3) || defined(CONFIG_USB_DA8XX) || \ - defined(CONFIG_USB_BLACKFIN) || defined(CONFIG_USB_AM35X) + defined(CONFIG_USB_BLACKFIN) || defined(CONFIG_USB_AM35X) || \ + defined(CONFIG_USB_AM335X)
int usb_lowlevel_init(void); int usb_lowlevel_stop(void);

Adds required configs in config file for am335x_evm to add support for usb. Host mode or device mode selected according to the USB module selected from boards.cfg file. Host for USB1 and device for USB0. By default USB0 is selected.
Signed-off-by: Harman Sohanpal harman_sohanpal@ti.com --- Changes for v2: - #define cleanups Changes for v3: - remove configs for usb as keyboard in host mode. - removed extra test of CONFIG_USB_AM335X - changed commit message. Changes for v4: -Selecting USB module according to config selected from boards.cfg file. - Selecting host mode if USB1 is selected. - Selecting device mode if USB0 is selected.
include/configs/am335x_evm.h | 36 ++++++++++++++++++++++++++++++++++++ 1 files changed, 36 insertions(+), 0 deletions(-)
diff --git a/include/configs/am335x_evm.h b/include/configs/am335x_evm.h index d0fbc88..2a3ae0c 100644 --- a/include/configs/am335x_evm.h +++ b/include/configs/am335x_evm.h @@ -165,6 +165,42 @@ #define CONFIG_SKIP_LOWLEVEL_INIT #endif
+ +/* + * USB configuration + * Enables CONFIG_MUSB_HCD for Host functionalities MSC for USB1 + * Enables CONFIG_MUSB_UDC for Device functionalities for USB0 + * Selected according to the USB module selected from boards.cfg. + */ + +#define CONFIG_USB_AM335X +#ifdef CONFIG_AM335X_USB0 +#define CONFIG_MUSB_UDC +#elif defined(CONFIG_AM335X_USB1) +#define CONFIG_MUSB_HCD +#endif + +#ifdef CONFIG_MUSB_HCD +#define CONFIG_CMD_USB +#define CONFIG_USB_STORAGE +#define CONGIG_CMD_STORAGE +#define CONFIG_CMD_FAT +#endif /* CONFIG_MUSB_HCD */ + +#ifdef CONFIG_MUSB_UDC +/* USB device configuration */ +#ifndef CONFIG_SPL_BUILD +#define CONFIG_USB_DEVICE +#define CONFIG_USB_TTY +#define CONFIG_SYS_CONSOLE_IS_IN_ENV +#endif /* CONFIG_SPL_BUILD */ +/* Change these to suit your needs */ +#define CONFIG_USBD_VENDORID 0x0451 +#define CONFIG_USBD_PRODUCTID 0x5678 +#define CONFIG_USBD_MANUFACTURER "Texas Instruments" +#define CONFIG_USBD_PRODUCT_NAME "AM335xEVM" +#endif /* CONFIG_MUSB_UDC */ + /* Unsupported features */ #undef CONFIG_USE_IRQ

Fix the compile warning : implicit declaration of musb_platform_init when CONFIG_MUSB_UDC is defined. The extern musb_platform_init was declared in musb_hcd.h but no such extern function was declared for musb_udc. So a common function has been declared in musb_core.h which can be used for both host mode and device mode.
Signed-off-by: Harman Sohanpal harman_sohanpal@ti.com --- Changes for v2: - none Changes for v3: - none Changes for v4: - none
drivers/usb/musb/musb_core.h | 2 ++ drivers/usb/musb/musb_hcd.h | 3 --- 2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h index a8adcce..14253f0 100644 --- a/drivers/usb/musb/musb_core.h +++ b/drivers/usb/musb/musb_core.h @@ -360,6 +360,8 @@ extern void musb_start(void); extern void musb_configure_ep(const struct musb_epinfo *epinfo, u8 cnt); extern void write_fifo(u8 ep, u32 length, void *fifo_data); extern void read_fifo(u8 ep, u32 length, void *fifo_data); +extern int musb_platform_init(void); +extern void musb_platform_deinit(void);
#if defined(CONFIG_USB_BLACKFIN) /* Every USB register is accessed as a 16-bit even if the value itself diff --git a/drivers/usb/musb/musb_hcd.h b/drivers/usb/musb/musb_hcd.h index dde7d37..5621f7e 100644 --- a/drivers/usb/musb/musb_hcd.h +++ b/drivers/usb/musb/musb_hcd.h @@ -105,8 +105,5 @@ extern unsigned char new[]; #define RH_REQ_ERR -1 #define RH_NACK 0x00
-/* extern functions */ -extern int musb_platform_init(void); -extern void musb_platform_deinit(void);
#endif /* __MUSB_HCD_H__ */

Dear Harman Sohanpal,
Fix the compile warning : implicit declaration of musb_platform_init when CONFIG_MUSB_UDC is defined. The extern musb_platform_init was declared in musb_hcd.h but no such extern function was declared for musb_udc. So a common function has been declared in musb_core.h which can be used for both host mode and device mode.
Signed-off-by: Harman Sohanpal harman_sohanpal@ti.com
[...]
Tom, what's the status? I dont see the warning.
Best regards, Marek Vasut

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 10/15/12 22:43, Marek Vasut wrote:
Lets just drop this patch. The whole series if still in patchwork should be superseded by the work Ilya is doing for MUSB.
- -- Tom

Dear Harman Sohanpal,
These patches add USB support in u-boot for AM335x. The support for host or device is selected depending on the config selected from boards.cfg file. Host mode is selected for USB1 and device mode is selected for USB0. Base addresses are selected accordingly.
I'll go through these once I get some sleep ... aka in 12 hours or so
Best regards, Marek Vasut

Dear Harman Sohanpal,
These patches add USB support in u-boot for AM335x. The support for host or device is selected depending on the config selected from boards.cfg file. Host mode is selected for USB1 and device mode is selected for USB0. Base addresses are selected accordingly.
Gene Zarkhin (1): AM335x : Add USB support for AM335x in u-boot
Harman Sohanpal (2): AM335x : Configs to add USB host support. musb_udc : Fix compile warning.
Dumb question ... but, can this not be made part of am35x USB ?
drivers/usb/musb/Makefile | 1 + drivers/usb/musb/am335x.c | 121 ++++++++++++++++++++++++++++++++++++++++++ drivers/usb/musb/am335x.h | 113 +++++++++++++++++++++++++++++++++++++++ drivers/usb/musb/musb_core.h | 2 + drivers/usb/musb/musb_hcd.h | 3 - include/configs/am335x_evm.h | 41 ++++++++++++++ include/usb.h | 3 +- 7 files changed, 280 insertions(+), 4 deletions(-) create mode 100644 drivers/usb/musb/am335x.c create mode 100644 drivers/usb/musb/am335x.h
Best regards, Marek Vasut

On Sat, Jun 30, 2012 at 6:15 AM, Marek Vasut marex@denx.de wrote:
Dear Harman Sohanpal,
These patches add USB support in u-boot for AM335x. The support for host or device is selected depending on the config selected from boards.cfg file. Host mode is selected for USB1 and device mode is selected for USB0. Base addresses are selected accordingly.
Gene Zarkhin (1): AM335x : Add USB support for AM335x in u-boot
Harman Sohanpal (2): AM335x : Configs to add USB host support. musb_udc : Fix compile warning.
Dumb question ... but, can this not be made part of am35x USB ?
Hi Marek, Well this can always be made part of am35x.c. But there would be a lot of changes required in the file. And also I believe it would not make much sense. It would require ifdefs at a lot of places. Best example I can give to support what i said is that the control register is at an offset of 4 in am35x and 14 in am335x. I am sure adding an ifdef at that place would not seem good to you to change address from 4 to 14 acc to platform. Is there much pain to add these 2 files? In my opinion we must need to have a separate file for this. This is as per my understanding. It could also cause confusions to some due to name. maybe :) Kindly give your thoughts. In case still some changes are required, we can think upon it :) Thanks, Harman
drivers/usb/musb/Makefile | 1 + drivers/usb/musb/am335x.c | 121 ++++++++++++++++++++++++++++++++++++++++++ drivers/usb/musb/am335x.h | 113 +++++++++++++++++++++++++++++++++++++++ drivers/usb/musb/musb_core.h | 2 + drivers/usb/musb/musb_hcd.h | 3 - include/configs/am335x_evm.h | 41 ++++++++++++++ include/usb.h | 3 +- 7 files changed, 280 insertions(+), 4 deletions(-) create mode 100644 drivers/usb/musb/am335x.c create mode 100644 drivers/usb/musb/am335x.h
Best regards, Marek Vasut

Dear Harman Sohanpal,
On Sat, Jun 30, 2012 at 6:15 AM, Marek Vasut marex@denx.de wrote:
Dear Harman Sohanpal,
These patches add USB support in u-boot for AM335x. The support for host or device is selected depending on the config selected from boards.cfg file. Host mode is selected for USB1 and device mode is selected for USB0. Base addresses are selected accordingly.
Gene Zarkhin (1): AM335x : Add USB support for AM335x in u-boot
Harman Sohanpal (2): AM335x : Configs to add USB host support. musb_udc : Fix compile warning.
Dumb question ... but, can this not be made part of am35x USB ?
Hi Marek, Well this can always be made part of am35x.c. But there would be a lot of changes required in the file.
Why? They use different IP block or something?
And also I believe it would not make much sense. It would require ifdefs at a lot of places. Best example I can give to support what i said is that the control register is at an offset of 4 in am35x and 14 in am335x.
So what, define two sets of register structures and pass them according to CPU ID.
I am sure adding an ifdef at that place would not seem good to you to change address from 4 to 14 acc to platform.
well ... struct regs_a { uint32_t padding[?]; uint32_t reg; ... };
struct regs_b { uint32_t reg; ... };
Create IO accessors ... like ... my_usb_writel() and my_usb_readl() to read and write the registers. And those accessors will cover the differences. Or is there more?
Is there much pain to add these 2 files?
Yes, duplication of code is always bad.
In my opinion we must need to have a separate file for this.
Why? If it's only about the registers, won't the approach above work?
This is as per my understanding. It could also cause confusions to some due to name. maybe :)
I'm no omap guru, Tom is. Tom?
Kindly give your thoughts.
Oh my brain is spinning from this :-)
In case still some changes are required, we can think upon it :)
I'm really glad to hear that, let's do our best to find the best possible solution :-)
Thanks, Harman
Best regards, Marek Vasut

On Sat, Jun 30, 2012 at 9:28 AM, Marek Vasut marex@denx.de wrote:
Dear Harman Sohanpal,
On Sat, Jun 30, 2012 at 6:15 AM, Marek Vasut marex@denx.de wrote:
Dear Harman Sohanpal,
These patches add USB support in u-boot for AM335x. The support for host or device is selected depending on the config selected from boards.cfg file. Host mode is selected for USB1 and device mode is selected for USB0. Base addresses are selected accordingly.
Gene Zarkhin (1): AM335x : Add USB support for AM335x in u-boot
Harman Sohanpal (2): AM335x : Configs to add USB host support. musb_udc : Fix compile warning.
Dumb question ... but, can this not be made part of am35x USB ?
Hi Marek, Well this can always be made part of am35x.c. But there would be a lot of changes required in the file.
Why? They use different IP block or something?
And also I believe it would not make much sense. It would require ifdefs at a lot of places. Best example I can give to support what i said is that the control register is at an offset of 4 in am35x and 14 in am335x.
So what, define two sets of register structures and pass them according to CPU ID.
I am sure adding an ifdef at that place would not seem good to you to change address from 4 to 14 acc to platform.
well ... struct regs_a { uint32_t padding[?]; uint32_t reg; ... };
struct regs_b { uint32_t reg; ... };
Create IO accessors ... like ... my_usb_writel() and my_usb_readl() to read and write the registers. And those accessors will cover the differences. Or is there more?
Is there much pain to add these 2 files?
Yes, duplication of code is always bad.
In my opinion we must need to have a separate file for this.
Why? If it's only about the registers, won't the approach above work?
This is as per my understanding. It could also cause confusions to some due to name. maybe :)
I'm no omap guru, Tom is. Tom?
Kindly give your thoughts.
Oh my brain is spinning from this :-)
In case still some changes are required, we can think upon it :)
I'm really glad to hear that, let's do our best to find the best possible solution :-)
Thanks, Harman
Best regards, Marek Vasut
Also we are adding the USB module to be selected dynamically from boards.cfg as suggested by Tom. If such is the case I believe the same should be done for am35x. Also from the previous mail of Tom, he seemed to be fine with separate files. The best approach would be to wait for Tom's reply. Thanks for the review :) Harman

Dear Harman Sohanpal,
On Sat, Jun 30, 2012 at 9:28 AM, Marek Vasut marex@denx.de wrote:
Dear Harman Sohanpal,
On Sat, Jun 30, 2012 at 6:15 AM, Marek Vasut marex@denx.de wrote:
Dear Harman Sohanpal,
These patches add USB support in u-boot for AM335x. The support for host or device is selected depending on the config selected from boards.cfg file. Host mode is selected for USB1 and device mode is selected for USB0. Base addresses are selected accordingly.
Gene Zarkhin (1): AM335x : Add USB support for AM335x in u-boot
Harman Sohanpal (2): AM335x : Configs to add USB host support. musb_udc : Fix compile warning.
Dumb question ... but, can this not be made part of am35x USB ?
Hi Marek, Well this can always be made part of am35x.c. But there would be a lot of changes required in the file.
Why? They use different IP block or something?
And also I believe it would not make much sense. It would require ifdefs at a lot of places. Best example I can give to support what i said is that the control register is at an offset of 4 in am35x and 14 in am335x.
So what, define two sets of register structures and pass them according to CPU ID.
I am sure adding an ifdef at that place would not seem good to you to change address from 4 to 14 acc to platform.
well ... struct regs_a { uint32_t padding[?]; uint32_t reg; ... };
struct regs_b { uint32_t reg; ... };
Create IO accessors ... like ... my_usb_writel() and my_usb_readl() to read and write the registers. And those accessors will cover the differences. Or is there more?
Is there much pain to add these 2 files?
Yes, duplication of code is always bad.
In my opinion we must need to have a separate file for this.
Why? If it's only about the registers, won't the approach above work?
This is as per my understanding. It could also cause confusions to some due to name. maybe :)
I'm no omap guru, Tom is. Tom?
Kindly give your thoughts.
Oh my brain is spinning from this :-)
In case still some changes are required, we can think upon it :)
I'm really glad to hear that, let's do our best to find the best possible solution :-)
Thanks, Harman
Best regards, Marek Vasut
Also we are adding the USB module to be selected dynamically from boards.cfg as suggested by Tom. If such is the case I believe the same should be done for am35x. Also from the previous mail of Tom, he seemed to be fine with separate files. The best approach would be to wait for Tom's reply. Thanks for the review :)
Hm, previous review? Was I not CCed on it maybe ? I'll check it.
Harman
Best regards, Marek Vasut

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 06/29/2012 08:58 PM, Marek Vasut wrote:
Dear Harman Sohanpal,
On Sat, Jun 30, 2012 at 6:15 AM, Marek Vasut marex@denx.de wrote:
Dear Harman Sohanpal,
These patches add USB support in u-boot for AM335x. The support for host or device is selected depending on the config selected from boards.cfg file. Host mode is selected for USB1 and device mode is selected for USB0. Base addresses are selected accordingly.
Gene Zarkhin (1): AM335x : Add USB support for AM335x in u-boot
Harman Sohanpal (2): AM335x : Configs to add USB host support. musb_udc : Fix compile warning.
Dumb question ... but, can this not be made part of am35x USB ?
Hi Marek, Well this can always be made part of am35x.c. But there would be a lot of changes required in the file.
Why? They use different IP block or something?
It's not an identical block so register maps have changed slightly. More so if we get a later conversion of the other similar parts (da850.c, davinci.c, omap3.c).
And also I believe it would not make much sense. It would require ifdefs at a lot of places. Best example I can give to support what i said is that the control register is at an offset of 4 in am35x and 14 in am335x.
So what, define two sets of register structures and pass them according to CPU ID.
I am sure adding an ifdef at that place would not seem good to you to change address from 4 to 14 acc to platform.
well ... struct regs_a { uint32_t padding[?]; uint32_t reg; ... };
struct regs_b { uint32_t reg; ... };
Create IO accessors ... like ... my_usb_writel() and my_usb_readl() to read and write the registers. And those accessors will cover the differences. Or is there more?
Is there much pain to add these 2 files?
Yes, duplication of code is always bad.
In my opinion we must need to have a separate file for this.
Why? If it's only about the registers, won't the approach above work?
This is as per my understanding. It could also cause confusions to some due to name. maybe :)
I'm no omap guru, Tom is. Tom?
I think what we need to do is take a shot at converting am35x.c and am335x.c into a 'ti_musb.[ch]' and per-family header files that give the register layout, etc, etc. We need to see how maintainable or not such a setup will be.
- -- Tom

Dear Tom Rini,
[...]
This is as per my understanding. It could also cause confusions to some due to name. maybe :)
I'm no omap guru, Tom is. Tom?
I think what we need to do is take a shot at converting am35x.c and am335x.c into a 'ti_musb.[ch]' and per-family header files that give the register layout, etc, etc. We need to see how maintainable or not such a setup will be.
Good, will you please poke into it, guys?
Best regards, Marek Vasut

On 07/04/2012 05:29 PM, Marek Vasut wrote:
Dear Tom Rini,
[...]
This is as per my understanding. It could also cause confusions to some due to name. maybe :)
I'm no omap guru, Tom is. Tom?
I think what we need to do is take a shot at converting am35x.c and am335x.c into a 'ti_musb.[ch]' and per-family header files that give the register layout, etc, etc. We need to see how maintainable or not such a setup will be.
Good, will you please poke into it, guys?
If Harman doesn't, Ilya will.
participants (4)
-
Harman Sohanpal
-
Harman Sohanpal
-
Marek Vasut
-
Tom Rini