usb: dwc2: Add support for v3 snpsid value
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/6 usb: dwc2: Add support for v3 snpsid value
Quoted 15 lines · Peter Griffin
This has been tested to the extent that I can enumerate a asix usb networking adapter and boot a kernel over usb on the 96boards hikey u-boot port I'm currently doing.
Signed-off-by Peter Griffin <[email protected]>--- drivers/usb/host/dwc2.c | 3 ++- drivers/usb/host/dwc2.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)drivers/usb/host/dwc2.c +0
-0
drivers/usb/host/dwc2.c int usb_lowlevel_init(int index, enum usb_init_type · line 1019
Quoted 6 lines
drivers/usb/host/dwc2.c +2
-1@@ -1015,7 +1015,8 @@ int usb_lowlevel_init(int index, enum usb_init_type init, void **controller) snpsid = readl(®s->gsnpsid);printf ("Core Release: %x.%03x\n" , snpsid >>12 &0xf , snpsid &0xfff ); -if ((snpsid &DWC2_SNPSID_DEVID_MASK ) != DWC2_SNPSID_DEVID_VER_2xx) { +if ((snpsid &DWC2_SNPSID_DEVID_MASK ) != DWC2_SNPSID_DEVID_VER_2xx && + (snpsid &DWC2_SNPSID_DEVID_MASK ) != DWC2_SNPSID_DEVID_VER_3xx) {
Isn't this then a DWC3 controller instead ?
Best regards,
Marek Vasut
PATCH 1/6 usb: dwc2: Add support for v3 snpsid value
Hi Marek,
Quoted 19 lines · Marek Vasut
Quoted 18 lines · Peter Griffin
This has been tested to the extent that I can enumerate a asix usb networking adapter and boot a kernel over usb on the 96boards hikey u-boot port I'm currently doing.
Signed-off-by Peter Griffin <[email protected]>--- drivers/usb/host/dwc2.c | 3 ++- drivers/usb/host/dwc2.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)drivers/usb/host/dwc2.c +0
-0@@ -1015,7 +1015,8 @@ int usb_lowlevel_init(int index, enum usb_init_type init, void **controller) snpsid = readl(®s->gsnpsid);printf ("Core Release: %x.%03x\n" , snpsid >>12 &0xf , snpsid &0xfff );
Quoted 5 lines
drivers/usb/host/dwc2.c int usb_lowlevel_init(int index, enum usb_init_type · line 1019
Quoted 3 lines
drivers/usb/host/dwc2.c +2
-1-if ((snpsid &DWC2_SNPSID_DEVID_MASK ) != DWC2_SNPSID_DEVID_VER_2xx) { +if ((snpsid &DWC2_SNPSID_DEVID_MASK ) != DWC2_SNPSID_DEVID_VER_2xx && + (snpsid &DWC2_SNPSID_DEVID_MASK ) != DWC2_SNPSID_DEVID_VER_3xx) {Isn't this then a DWC3 controller instead ?
No I don't believe so. dwc3 is a usb3 controller with different register set AFAIK.
I believe this piece of code is replicating what the Linux driver does here: -
http://lxr.free-electrons.com/source/drivers/usb/dwc2/core.c#L2664
Also see here for the various different sub core revisions within 2.xx and 3.xx which are supported by the dwc2 Linux driver
http://lxr.free-electrons.com/source/drivers/usb/dwc2/core.h#L625.
This also matches up with the available datasheet here
https://github.com/96boards/documentation/blob/master/hikey/
Hi6220V100_Multi-Mode_Application_Processor_Function_Description.pdf on page 219 which states it is a USB2 OTG controller, but near the bottom of the page also
tates: -
"The USB core used in Hi6220 is build from Synopsys IP(3.00a).".
I just checked through the DWC_otg_databook.odf, and these revision numbers also match up with the "Revision History" changes in the document. So it all ties together.
It is worth highlighting at this point that not bailing here (along with setting up the appropriate clocks to the IP) was enough for me to get a DWC2 core rev 3.00a enumerating a ASIX networking adapter and tftp a kernel into DDR. However there maybe some other changes required to "fully" support the 3.00a hardware version. There is of course the problem I mentioned in the cover letter regarding enumerating mass storage devices which may or may not be related to this.
Although a quick grep of the Linux driver only gets three hits
git grep DWC2_CORE_REV_3_00a
core.h:#define DWC2_CORE_REV_3_00a 0x4f54300a
core_intr.c: if (hsotg->hw_params.snpsid >= DWC2_CORE_REV_3_00a)
hcd.c: if (hsotg->hw_params.snpsid >= DWC2_CORE_REV_3_00a) {Neither of which seem relevant to the mass storage device problem (one adds a delay in the irq handler, and the other sets a bit when freeing the hcd.
kind regards,
Peter.
PATCH 1/6 usb: dwc2: Add support for v3 snpsid value
Quoted 65 lines · Peter Griffin
Hi Marek,
Quoted 19 lines · Marek Vasut
Quoted 18 lines · Peter Griffin
This has been tested to the extent that I can enumerate a asix usb networking adapter and boot a kernel over usb on the 96boards hikey u-boot port I'm currently doing.
Signed-off-by Peter Griffin <[email protected]>---
drivers/usb/host/dwc2.c | 3 ++- drivers/usb/host/dwc2.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)drivers/usb/host/dwc2.c +0
-0@@ -1015,7 +1015,8 @@ int usb_lowlevel_init(int index, enum usb_init_type init, void **controller) snpsid = readl(®s->gsnpsid);Quoted 8 lines
drivers/usb/host/dwc2.c int usb_lowlevel_init(int index, enum · line 1021
Quoted 6 lines
drivers/usb/host/dwc2.c +2
-1printf ("Core Release: %x.%03x\n" , snpsid >>12 &0xf , snpsid &0xfff ); -if ((snpsid &DWC2_SNPSID_DEVID_MASK ) != DWC2_SNPSID_DEVID_VER_2xx) { +if ((snpsid &DWC2_SNPSID_DEVID_MASK ) != DWC2_SNPSID_DEVID_VER_2xx && + (snpsid &DWC2_SNPSID_DEVID_MASK ) != DWC2_SNPSID_DEVID_VER_3xx) {Isn't this then a DWC3 controller instead ?
No I don't believe so. dwc3 is a usb3 controller with different register set AFAIK.
I believe this piece of code is replicating what the Linux driver does here: -
http://lxr.free-electrons.com/source/drivers/usb/dwc2/core.c#L2664
Also see here for the various different sub core revisions within 2.xx and 3.xx which are supported by the dwc2 Linux driver
http://lxr.free-electrons.com/source/drivers/usb/dwc2/core.h#L625.This also matches up with the available datasheet here
https://github.com/96boards/documentation/blob/master/hikey/
Hi6220V100_Multi-Mode_Application_Processor_Function_Description.pdf on page 219 which states it is a USB2 OTG controller, but near the bottom of the page also tates: -"The USB core used in Hi6220 is build from Synopsys IP(3.00a).".
I just checked through the DWC_otg_databook.odf, and these revision numbers also match up with the "Revision History" changes in the document. So it all ties together.
It is worth highlighting at this point that not bailing here (along with setting up the appropriate clocks to the IP) was enough for me to get a DWC2 core rev 3.00a enumerating a ASIX networking adapter and tftp a kernel into DDR. However there maybe some other changes required to "fully" support the 3.00a hardware version. There is of course the problem I mentioned in the cover letter regarding enumerating mass storage devices which may or may not be related to this.
Although a quick grep of the Linux driver only gets three hits
Quoted 7 lines
git grep
DWC2_CORE_REV_3_00a
core.h:#defineDWC2_CORE_REV_3_00a0x4f54300acore_intr.c: if (hsotg->hw_params.snpsid >= DWC2_CORE_REV_3_00a) hcd.c: if (hsotg->hw_params.snpsid >= DWC2_CORE_REV_3_00a) {Neither of which seem relevant to the mass storage device problem (one adds a delay in the irq handler, and the other sets a bit when freeing the hcd.
Ooh, I see. Thank you for the extensive explanation!
I picked this patch for u-boot-usb, so feel free to submit any improvements for the 3.xx core as you cook them. Thanks!
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 2/6 dm: gpio: hi6220: Add a hi6220 GPIO driver model driver.
On Tuesday, May 12, 2015 at 03:38:28 PM, Peter Griffin wrote:
Hi!
Quoted 19 lines
This patch adds support for the GPIO perif found on hi6220 SoC.
Signed-off-by Peter Griffin <[email protected]>--- arch/arm/include/asm/arch-armv8/gpio.h | 47 +++++++++++++++++ drivers/gpio/Makefile | 2 + drivers/gpio/hi6220_gpio.c | 95 ++++++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+) create mode 100644 arch/arm/include/asm/arch-armv8/gpio.h create mode 100644 drivers/gpio/hi6220_gpio.c(quoted diff) +0
-0
b/arch/arm/include/asm/arch-armv8/gpio.hnew file mode 100644 index 0000000..162c2d9arch/arm/include/asm/arch-armv8/gpio.h new file +1
-0@@ -0,0 +1,47 @@ +/*
arch/arm/include/asm/arch-armv8/gpio.h line 11
Quoted 10 lines
arch/arm/include/asm/arch-armv8/gpio.h new file +10
-0+* Copyright (C) 2015 Linaro +* Peter Griffin <[email protected]> +* +* SPDX-License-Identifier: GPL-2.0+ +*/ + +#ifndef _HI6220_GPIO_H_ +#define _HI6220_GPIO_H_ + +#define HI6220_GPIO0_BASE (void *)0xf8011000
You should drop the explicit cast, that's nasty.
Also, why don't you define this as a HI6220_GPIO_BASE(bank) instead? That'd trim down the number of macros and from what I see, it should be rather easy to do ...
#define HI6220_GPIO_BASE(bank) (((bank < 4) ? 0xf8012000 : (0xf7020000 - 0x4000)) + (0x1000 * bank))
Quoted 9 lines
arch/arm/include/asm/arch-armv8/gpio.h new file +9
-0+#define HI6220_GPIO1_BASE (void *)0xf8012000 +#define HI6220_GPIO2_BASE (void *)0xf8013000 +#define HI6220_GPIO3_BASE (void *)0xf8014000 +#define HI6220_GPIO4_BASE (void *)0xf7020000 +#define HI6220_GPIO5_BASE (void *)0xf7021000 +#define HI6220_GPIO6_BASE (void *)0xf7022000 +#define HI6220_GPIO7_BASE (void *)0xf7023000 +#define HI6220_GPIO8_BASE (void *)0xf7024000 +#define HI6220_GPIO9_BASE (void *)0xf7025000
arch/arm/include/asm/arch-armv8/gpio.h line 30
Quoted 10 lines
arch/arm/include/asm/arch-armv8/gpio.h new file +10
-0+#define HI6220_GPIO10_BASE (void *)0xf7026000 +#define HI6220_GPIO11_BASE (void *)0xf7027000 +#define HI6220_GPIO12_BASE (void *)0xf7028000 +#define HI6220_GPIO13_BASE (void *)0xf7029000 +#define HI6220_GPIO14_BASE (void *)0xf702a000 +#define HI6220_GPIO15_BASE (void *)0xf702b000 +#define HI6220_GPIO16_BASE (void *)0xf702c000 +#define HI6220_GPIO17_BASE (void *)0xf702d000 +#define HI6220_GPIO18_BASE (void *)0xf702e000 +#define HI6220_GPIO19_BASE (void *)0xf702f000
But are these even used in the driver anywhere ?
arch/arm/include/asm/arch-armv8/gpio.h line 31
Quoted 1 line
arch/arm/include/asm/arch-armv8/gpio.h new file +1
-0+#define BIT (x) (1 << (x))
This macro should be placed into common header files.
arch/arm/include/asm/arch-armv8/gpio.h line 36
Quoted 5 lines
arch/arm/include/asm/arch-armv8/gpio.h new file +5
-0+#define HI6220_GPIO_PER_BANK 8 +#define HI6220_GPIO_DIR 0x400 + +struct gpio_bank { +u8 *base;/* address of registers in physical memory */
Should be void __iomem *, no ?
Quoted 9 lines
arch/arm/include/asm/arch-armv8/gpio.h new file +9
-0+}; + +/* Information about a GPIO bank */ +struct hikey_gpio_platdata { +int bank_index; +void *base;/* address of registers in physical memory */ +}; + +#endif /* _HI6220_GPIO_H_ */
[...]
PATCH 2/6 dm: gpio: hi6220: Add a hi6220 GPIO driver model driver.
Quoted 1 line · Marek Vasut
On Tuesday, May 12, 2015 at 03:38:28 PM, Peter Griffin wrote:
[snip]
Quoted 3 lines
Quoted 1 line
+#define BIT(x) (1 << (x))
This macro should be placed into common header files.
We'll fix that once Jagan's series comes in which gives everyone BIT().
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150512/cc8ecd94/attachment.sig>
PATCH 2/6 dm: gpio: hi6220: Add a hi6220 GPIO driver model driver.
In reply to Marek VasutHi Marek,
Thanks for reviewing.
Quoted 24 lines
Quoted 24 lines
This patch adds support for the GPIO perif found on hi6220 SoC.
Signed-off-by Peter Griffin <[email protected]>--- arch/arm/include/asm/arch-armv8/gpio.h | 47 +++++++++++++++++ drivers/gpio/Makefile | 2 + drivers/gpio/hi6220_gpio.c | 95 ++++++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+) create mode 100644 arch/arm/include/asm/arch-armv8/gpio.h create mode 100644 drivers/gpio/hi6220_gpio.c(quoted diff) +0
-0
b/arch/arm/include/asm/arch-armv8/gpio.hnew file mode 100644 index 0000000..162c2d9arch/arm/include/asm/arch-armv8/gpio.h new file +5
-0@@ -0,0 +1,47 @@ +/* +* Copyright (C) 2015 Linaro +* Peter Griffin <[email protected]> +* +* SPDX-License-Identifier: GPL-2.0+
Quoted 8 lines
arch/arm/include/asm/arch-armv8/gpio.h line 11
Quoted 6 lines
arch/arm/include/asm/arch-armv8/gpio.h new file +6
-0+*/ + +#ifndef _HI6220_GPIO_H_ +#define _HI6220_GPIO_H_ + +#define HI6220_GPIO0_BASE (void *)0xf8011000You should drop the explicit cast, that's nasty.
If I drop the cast I get lots of
include/asm/arch/gpio.h:11:32: warning: initialization makes pointer from integer without a cast
compiler warnings.
Quoted 6 lines
Also, why don't you define this as a
HI6220_GPIO_BASE(bank)instead? That'd trim down the number of macros and from what I see, it should be rather easy to do ...#define HI6220_GPIO_BASE(bank) (((bank < 4) ? 0xf8012000 : (0xf7020000 - 0x4000)) + (0x1000 * bank))
Yes good idea, I will do it like you suggest in V2. Currently I have
#define HI6220_GPIO_BASE(bank) (void *)(((bank < 4) ? 0xf8011000 : \ 0xf7020000 - 0x4000) + (0x1000 * bank))
To avoid the warnings mentioned above.
<snip>
Quoted 5 lines
arch/arm/include/asm/arch-armv8/gpio.h line 14
Quoted 3 lines
arch/arm/include/asm/arch-armv8/gpio.h new file +3
-0+#define HI6220_GPIO17_BASE (void *)0xf702d000 +#define HI6220_GPIO18_BASE (void *)0xf702e000 +#define HI6220_GPIO19_BASE (void *)0xf702f000But are these even used in the driver anywhere ?
They are currently used in the hikey.c board file which defines the hikey_gpio_platdata structure.
Although thinking about it some more this should be moved from the hikey board file to the driver as it is SoC specific rather than board specific.
Quoted 5 lines
arch/arm/include/asm/arch-armv8/gpio.h line 15
Quoted 2 lines
arch/arm/include/asm/arch-armv8/gpio.h new file +1
-0+#define BIT (x) (1 << (x))This macro should be placed into common header files.
Quoted 7 lines
arch/arm/include/asm/arch-armv8/gpio.h line 20
Quoted 5 lines
arch/arm/include/asm/arch-armv8/gpio.h new file +5
-0+#define HI6220_GPIO_PER_BANK 8 +#define HI6220_GPIO_DIR 0x400 + +struct gpio_bank { +u8 *base;/* address of registers in physical memory */ Should be void
__iomem*, no ?
Will fix in v2.
regards,
Peter.
PATCH 2/6 dm: gpio: hi6220: Add a hi6220 GPIO driver model driver.
Quoted 3 lines · Peter Griffin
Hi Marek,
Thanks for reviewing.
Hi!
Quoted 7 lines
Quoted 6 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+#ifndef _HI6220_GPIO_H_ +#define _HI6220_GPIO_H_ + +#define HI6220_GPIO0_BASE (void *)0xf8011000You should drop the explicit cast, that's nasty.
Quoted 6 lines
If I drop the cast I get lots of
include/asm/arch/gpio.h:11:32: warning: initialization makes pointer from integer without a castcompiler warnings.
But this is gpio.h header for your architecture, so maybe fix it there too?
Quoted 6 lines
Quoted 6 lines
Also, why don't you define this as a
HI6220_GPIO_BASE(bank)instead? That'd trim down the number of macros and from what I see, it should be rather easy to do ...#define
HI6220_GPIO_BASE(bank)
Quoted 8 lines
Quoted 1 line
(((bank < 4) ? 0xf8012000 : (0xf7020000 - 0x4000)) + (0x1000 * bank))Yes good idea, I will do it like you suggest in V2. Currently I have
#define HI6220_GPIO_BASE(bank) (void *)(((bank < 4) ? 0xf8011000 : \ 0xf7020000 - 0x4000) + (0x1000 * bank))To avoid the warnings mentioned above.
Yup, or maybe even make it an inline function so you get proper typechecking?
Quoted 8 lines
<snip>
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+#define HI6220_GPIO17_BASE (void *)0xf702d000 +#define HI6220_GPIO18_BASE (void *)0xf702e000 +#define HI6220_GPIO19_BASE (void *)0xf702f000But are these even used in the driver anywhere ?
Quoted 5 lines
They are currently used in the
hikey.cboard file which defines thehikey_gpio_platdatastructure.Although thinking about it some more this should be moved from the hikey board file to the driver as it is SoC specific rather than board specific.
This is specific to the CPU, so this should be in arch-<something> .
Quoted 12 lines
Quoted 11 lines
Quoted 1 line
+#define BIT(x) (1 << (x))
This macro should be placed into common header files.
Review
Quoted 5 lines
(quoted diff) +5
-0+#define HI6220_GPIO_PER_BANK 8 +#define HI6220_GPIO_DIR 0x400 + +struct gpio_bank { + u8 *base; /* address of registers in physical memory */Should be void
__iomem*, no ?
Quoted 1 line
Will fix in v2.
Thanks! Also, please wait a bit for other reviews before sending V2 :)
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 5/6 mmc: hi6220_dw_mmc: Add hi6220 glue code for dw_mmc controller.
Quoted 5 lines · Peter Griffin
This patch adds the glue code for hi6220 SoC which has 2x synopsis
dw_mmccontrollers. This will be used by the hikey board support in subsequent patches.Signed-off-by Peter Griffin <[email protected]>
[...]
Quoted 14 lines
drivers/mmc/hi6220_dw_mmc.c new file +8
-0@@ -0,0 +1,63 @@ +/* +* (C) Copyright 2015 Linaro +* peter.griffin <[email protected]> +* +* SPDX-License-Identifier: GPL-2.0+ +*/ + +#include <common.h>
drivers/mmc/hi6220_dw_mmc.c line 18
Quoted 10 lines
drivers/mmc/hi6220_dw_mmc.c new file +10
-0+#include <dwmmc.h> +#include <malloc.h> +#include <asm-generic/errno.h> + +#define DWMMC_MAX_CH_NUM 4 + +/* +#define DWMMC_MAX_FREQ 52000000 +#define DWMMC_MIN_FREQ 400000 +*/
Please zap these dead macros.
Quoted 7 lines
drivers/mmc/hi6220_dw_mmc.c new file +7
-0+/*TODO we should probably use the frequencies above, but ATF uses +the ones below so stick with that for the moment */ +#define DWMMC_MAX_FREQ 50000000 +#define DWMMC_MIN_FREQ 378000 + +/* Source clock is configured to 100Mhz by ATF bl1*/ +#define MMC0_DEFAULT_FREQ 100000000
[...]
drivers/mmc/hi6220_dw_mmc.c
Quoted 5 lines
drivers/mmc/hi6220_dw_mmc.c new file +5
-0+int hi6220_dwmci_add_port (int index,u32 regbase,int bus_width) +{ +struct dwmci_host *host =NULL ; + + host =malloc (sizeof (struct dwmci_host ));
calloc(1, sizeof(...)) so the data are inited/zero'd out please. [...]
PATCH 5/6 mmc: hi6220_dw_mmc: Add hi6220 glue code for dw_mmc controller.
Hi Marek,
Quoted 27 lines · Marek Vasut
Quoted 5 lines · Peter Griffin
This patch adds the glue code for hi6220 SoC which has 2x synopsis
dw_mmccontrollers. This will be used by the hikey board support in subsequent patches.Signed-off-by Peter Griffin <[email protected]>[...]
Quoted 18 lines
drivers/mmc/hi6220_dw_mmc.c new file +12
-0@@ -0,0 +1,63 @@ +/* +* (C) Copyright 2015 Linaro +* peter.griffin <[email protected]> +* +* SPDX-License-Identifier: GPL-2.0+ +*/ + +#include <common.h> +#include <dwmmc.h> +#include <malloc.h> +#include <asm-generic/errno.h> +
Quoted 8 lines
drivers/mmc/hi6220_dw_mmc.c line 18
Quoted 6 lines
drivers/mmc/hi6220_dw_mmc.c new file +6
-0+#define DWMMC_MAX_CH_NUM 4 + +/* +#define DWMMC_MAX_FREQ 52000000 +#define DWMMC_MIN_FREQ 400000 +*/ Please zap these dead macros.
Ok will fix in v2.
Quoted 11 lines
Quoted 8 lines
drivers/mmc/hi6220_dw_mmc.c new file +7
-0+/*TODO we should probably use the frequencies above, but ATF uses +the ones below so stick with that for the moment */ +#define DWMMC_MAX_FREQ 50000000 +#define DWMMC_MIN_FREQ 378000 + +/* Source clock is configured to 100Mhz by ATF bl1*/ +#define MMC0_DEFAULT_FREQ 100000000[...]
Quoted 8 lines
drivers/mmc/hi6220_dw_mmc.c
Quoted 5 lines
drivers/mmc/hi6220_dw_mmc.c new file +5
-0+int hi6220_dwmci_add_port (int index,u32 regbase,int bus_width) +{ +struct dwmci_host *host =NULL ; + + host =malloc (sizeof (struct dwmci_host ));
calloc(1, sizeof(...))so the data are inited/zero'd out please. [...]
Ok will fix in v2
regards,
Peter.