| Message ID | 1496316319-5404-1-git-send-email-daniel.meng@rock-chips.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
On 1 June 2017 at 05:25, Meng Dongyang <daniel.meng@rock-chips.com> wrote: > Fix macro error of dwc2 driver, add macro definition to config force mode > and HNP/SRP capability. > > Signed-off-by: Meng Dongyang <daniel.meng@rock-chips.com> > --- > > Changes in v2: > - Splited from patch [07/08] of v1 > > drivers/usb/host/dwc2.c | 9 ++++++++- > drivers/usb/host/dwc2.h | 10 ++++++---- > 2 files changed, 14 insertions(+), 5 deletions(-) > Reviewed-by: Simon Glass <sjg@chromium.org>
On 06/01/2017 01:25 PM, Meng Dongyang wrote: > Fix macro error of dwc2 driver, add macro definition to config force mode > and HNP/SRP capability. Can you please elaborate some more on the purpose of this patch ? It seems wrong, since the driver works on systems using DWC2 as is and this patch changes constants used in the driver. > Signed-off-by: Meng Dongyang <daniel.meng@rock-chips.com> > --- > > Changes in v2: > - Splited from patch [07/08] of v1 > > drivers/usb/host/dwc2.c | 9 ++++++++- > drivers/usb/host/dwc2.h | 10 ++++++---- > 2 files changed, 14 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c > index 0e5df15..51619d6 100644 > --- a/drivers/usb/host/dwc2.c > +++ b/drivers/usb/host/dwc2.c > @@ -370,7 +370,7 @@ static void dwc_otg_core_init(struct dwc2_priv *priv) > usbcfg &= ~DWC2_GUSBCFG_DDRSEL; > #endif > } else { /* UTMI+ interface */ > -#if (CONFIG_DWC2_UTMI_PHY_WIDTH == 16) > +#if (CONFIG_DWC2_UTMI_WIDTH == 16) > usbcfg |= DWC2_GUSBCFG_PHYIF; > #endif > } > @@ -394,6 +394,11 @@ static void dwc_otg_core_init(struct dwc2_priv *priv) > usbcfg |= DWC2_GUSBCFG_ULPI_CLK_SUS_M; > } > #endif > + > +#ifdef CONFIG_DWC2_FORCE_HOST_MODE > + usbcfg |= DWC2_GUSBCFG_FORCEHOSTMODE; > +#endif > + > writel(usbcfg, ®s->gusbcfg); > > /* Program the GAHBCFG Register. */ > @@ -422,8 +427,10 @@ static void dwc_otg_core_init(struct dwc2_priv *priv) > > writel(ahbcfg, ®s->gahbcfg); > > +#ifdef CONFIG_DWC2_HNP_SRP_CAP > /* Program the GUSBCFG register for HNP/SRP. */ > setbits_le32(®s->gusbcfg, DWC2_GUSBCFG_HNPCAP | DWC2_GUSBCFG_SRPCAP); > +#endif > > #ifdef CONFIG_DWC2_IC_USB_CAP > setbits_le32(®s->gusbcfg, DWC2_GUSBCFG_IC_USB_CAP); > diff --git a/drivers/usb/host/dwc2.h b/drivers/usb/host/dwc2.h > index 4482dc6..3c62e9b 100644 > --- a/drivers/usb/host/dwc2.h > +++ b/drivers/usb/host/dwc2.h > @@ -771,11 +771,11 @@ struct dwc2_core_regs { > #define CONFIG_DWC2_MAX_TRANSFER_SIZE 65535 > #define CONFIG_DWC2_MAX_PACKET_COUNT 511 > > -#define DWC2_PHY_TYPE_FS 0 > -#define DWC2_PHY_TYPE_UTMI 1 > -#define DWC2_PHY_TYPE_ULPI 2 > +#define DWC2_PHY_TYPE_UTMI 0 > +#define DWC2_PHY_TYPE_ULPI 1 > +#define DWC2_PHY_TYPE_FS 2 > #define CONFIG_DWC2_PHY_TYPE DWC2_PHY_TYPE_UTMI /* PHY type */ > -#define CONFIG_DWC2_UTMI_WIDTH 8 /* UTMI bus width (8/16) */ > +#define CONFIG_DWC2_UTMI_WIDTH 16 /* UTMI bus width (8/16) */ So basically you now force 16bit UTMI bus width instead of 8 ? This breaks every single platform in U-Boot using the DWC2. > #undef CONFIG_DWC2_PHY_ULPI_DDR /* ULPI PHY uses DDR mode */ > #define CONFIG_DWC2_PHY_ULPI_EXT_VBUS /* ULPI PHY controls VBUS */ > @@ -785,5 +785,7 @@ struct dwc2_core_regs { > #undef CONFIG_DWC2_THR_CTL /* Threshold control */ > #define CONFIG_DWC2_TX_THR_LENGTH 64 > #undef CONFIG_DWC2_IC_USB_CAP /* IC Cap */ > +#define CONFIG_DWC2_FORCE_HOST_MODE /* Force host mode */ > +#undef CONFIG_DWC2_HNP_SRP_CAP /* HNP/SRP Cap */ > > #endif /* __DWC2_H__ */ >
On 2017/6/3 18:23, Marek Vasut wrote: > On 06/01/2017 01:25 PM, Meng Dongyang wrote: >> Fix macro error of dwc2 driver, add macro definition to config force mode >> and HNP/SRP capability. > Can you please elaborate some more on the purpose of this patch ? It > seems wrong, since the driver works on systems using DWC2 as is and this > patch changes constants used in the driver. OK, I will think about making it works as before. But I found that some config need to be change on some platform. > >> Signed-off-by: Meng Dongyang <daniel.meng@rock-chips.com> >> --- >> >> Changes in v2: >> - Splited from patch [07/08] of v1 >> >> drivers/usb/host/dwc2.c | 9 ++++++++- >> drivers/usb/host/dwc2.h | 10 ++++++---- >> 2 files changed, 14 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c >> index 0e5df15..51619d6 100644 >> --- a/drivers/usb/host/dwc2.c >> +++ b/drivers/usb/host/dwc2.c >> @@ -370,7 +370,7 @@ static void dwc_otg_core_init(struct dwc2_priv *priv) >> usbcfg &= ~DWC2_GUSBCFG_DDRSEL; >> #endif >> } else { /* UTMI+ interface */ >> -#if (CONFIG_DWC2_UTMI_PHY_WIDTH == 16) >> +#if (CONFIG_DWC2_UTMI_WIDTH == 16) The macro definition of UTMI width is CONFIG_DWC2_UTMI_WIDTH, yet here is CONFIG_DWC2_UTMI_PHY_WIDTH. So maybe it's better to change them to the same one. >> usbcfg |= DWC2_GUSBCFG_PHYIF; >> #endif >> } >> @@ -394,6 +394,11 @@ static void dwc_otg_core_init(struct dwc2_priv *priv) >> usbcfg |= DWC2_GUSBCFG_ULPI_CLK_SUS_M; >> } >> #endif >> + >> +#ifdef CONFIG_DWC2_FORCE_HOST_MODE >> + usbcfg |= DWC2_GUSBCFG_FORCEHOSTMODE; >> +#endif >> + I found that on the platform not support HNP/SRP capability, the dr_mode will still be otg after init and the host mode can't work. So may be a config of force_mode is necessary. >> writel(usbcfg, ®s->gusbcfg); >> >> /* Program the GAHBCFG Register. */ >> @@ -422,8 +427,10 @@ static void dwc_otg_core_init(struct dwc2_priv *priv) >> >> writel(ahbcfg, ®s->gahbcfg); >> >> +#ifdef CONFIG_DWC2_HNP_SRP_CAP >> /* Program the GUSBCFG register for HNP/SRP. */ >> setbits_le32(®s->gusbcfg, DWC2_GUSBCFG_HNPCAP | DWC2_GUSBCFG_SRPCAP); >> +#endif >> >> #ifdef CONFIG_DWC2_IC_USB_CAP >> setbits_le32(®s->gusbcfg, DWC2_GUSBCFG_IC_USB_CAP); >> diff --git a/drivers/usb/host/dwc2.h b/drivers/usb/host/dwc2.h >> index 4482dc6..3c62e9b 100644 >> --- a/drivers/usb/host/dwc2.h >> +++ b/drivers/usb/host/dwc2.h >> @@ -771,11 +771,11 @@ struct dwc2_core_regs { >> #define CONFIG_DWC2_MAX_TRANSFER_SIZE 65535 >> #define CONFIG_DWC2_MAX_PACKET_COUNT 511 >> >> -#define DWC2_PHY_TYPE_FS 0 >> -#define DWC2_PHY_TYPE_UTMI 1 >> -#define DWC2_PHY_TYPE_ULPI 2 >> +#define DWC2_PHY_TYPE_UTMI 0 >> +#define DWC2_PHY_TYPE_ULPI 1 >> +#define DWC2_PHY_TYPE_FS 2 According to table 5-10 of "DesignWare Cores USB 2.0 Hi-Speed On-The-Go(OTG)" 3.10a databook, ULPI or UTMI+ Select (ULPI_UTMI_Sel) The application uses this bit to select either a UTMI+ interface or ULPI Interface. ■ 1'b0: UTMI+ Interface ■ 1'b1: ULPI Interface But the macor definition in current code would make a opposite config on ULPI_UTMI_Sel bit, (in the function "dwc_otg_core_init") 1: UTMI+ Interface 0: ULPI Interface It seems wrong. >> #define CONFIG_DWC2_PHY_TYPE DWC2_PHY_TYPE_UTMI /* PHY type */ >> -#define CONFIG_DWC2_UTMI_WIDTH 8 /* UTMI bus width (8/16) */ >> +#define CONFIG_DWC2_UTMI_WIDTH 16 /* UTMI bus width (8/16) */ > So basically you now force 16bit UTMI bus width instead of 8 ? Three changes basically 1. force 16bit UTMI bus width instead of 8 2. config force mode on the platform without HNP/SRP capability. 3. change macro definition about PHY type (ULPI_UTMI_Sel). > This breaks every single platform in U-Boot using the DWC2. > >> #undef CONFIG_DWC2_PHY_ULPI_DDR /* ULPI PHY uses DDR mode */ >> #define CONFIG_DWC2_PHY_ULPI_EXT_VBUS /* ULPI PHY controls VBUS */ >> @@ -785,5 +785,7 @@ struct dwc2_core_regs { >> #undef CONFIG_DWC2_THR_CTL /* Threshold control */ >> #define CONFIG_DWC2_TX_THR_LENGTH 64 >> #undef CONFIG_DWC2_IC_USB_CAP /* IC Cap */ >> +#define CONFIG_DWC2_FORCE_HOST_MODE /* Force host mode */ >> +#undef CONFIG_DWC2_HNP_SRP_CAP /* HNP/SRP Cap */ >> >> #endif /* __DWC2_H__ */ >> >
On 06/05/2017 06:09 AM, rock-chips(daniel.meng) wrote: > > > On 2017/6/3 18:23, Marek Vasut wrote: >> On 06/01/2017 01:25 PM, Meng Dongyang wrote: >>> Fix macro error of dwc2 driver, add macro definition to config force mode >>> and HNP/SRP capability. >> Can you please elaborate some more on the purpose of this patch ? It >> seems wrong, since the driver works on systems using DWC2 as is and this >> patch changes constants used in the driver. > OK, I will think about making it works as before. But I found that some > config need to be > change on some platform. If you have platform specific tweaks, they need to be incorporated such that they don't break other platforms ! >> >>> Signed-off-by: Meng Dongyang <daniel.meng@rock-chips.com> >>> --- >>> >>> Changes in v2: >>> - Splited from patch [07/08] of v1 >>> >>> drivers/usb/host/dwc2.c | 9 ++++++++- >>> drivers/usb/host/dwc2.h | 10 ++++++---- >>> 2 files changed, 14 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c >>> index 0e5df15..51619d6 100644 >>> --- a/drivers/usb/host/dwc2.c >>> +++ b/drivers/usb/host/dwc2.c >>> @@ -370,7 +370,7 @@ static void dwc_otg_core_init(struct dwc2_priv *priv) >>> usbcfg &= ~DWC2_GUSBCFG_DDRSEL; >>> #endif >>> } else { /* UTMI+ interface */ >>> -#if (CONFIG_DWC2_UTMI_PHY_WIDTH == 16) >>> +#if (CONFIG_DWC2_UTMI_WIDTH == 16) > The macro definition of UTMI width is CONFIG_DWC2_UTMI_WIDTH, yet here > is CONFIG_DWC2_UTMI_PHY_WIDTH. So maybe it's better to change them to > the same one. >>> usbcfg |= DWC2_GUSBCFG_PHYIF; >>> #endif >>> } >>> @@ -394,6 +394,11 @@ static void dwc_otg_core_init(struct dwc2_priv *priv) >>> usbcfg |= DWC2_GUSBCFG_ULPI_CLK_SUS_M; >>> } >>> #endif >>> + >>> +#ifdef CONFIG_DWC2_FORCE_HOST_MODE >>> + usbcfg |= DWC2_GUSBCFG_FORCEHOSTMODE; >>> +#endif >>> + > I found that on the platform not support HNP/SRP capability, the dr_mode > will still be otg after > init and the host mode can't work. > So may be a config of force_mode is necessary. Can we use DT props instead of ad-hoc config options ? >>> writel(usbcfg, ®s->gusbcfg); >>> >>> /* Program the GAHBCFG Register. */ >>> @@ -422,8 +427,10 @@ static void dwc_otg_core_init(struct dwc2_priv *priv) >>> >>> writel(ahbcfg, ®s->gahbcfg); >>> >>> +#ifdef CONFIG_DWC2_HNP_SRP_CAP >>> /* Program the GUSBCFG register for HNP/SRP. */ >>> setbits_le32(®s->gusbcfg, DWC2_GUSBCFG_HNPCAP | DWC2_GUSBCFG_SRPCAP); >>> +#endif >>> >>> #ifdef CONFIG_DWC2_IC_USB_CAP >>> setbits_le32(®s->gusbcfg, DWC2_GUSBCFG_IC_USB_CAP); >>> diff --git a/drivers/usb/host/dwc2.h b/drivers/usb/host/dwc2.h >>> index 4482dc6..3c62e9b 100644 >>> --- a/drivers/usb/host/dwc2.h >>> +++ b/drivers/usb/host/dwc2.h >>> @@ -771,11 +771,11 @@ struct dwc2_core_regs { >>> #define CONFIG_DWC2_MAX_TRANSFER_SIZE 65535 >>> #define CONFIG_DWC2_MAX_PACKET_COUNT 511 >>> >>> -#define DWC2_PHY_TYPE_FS 0 >>> -#define DWC2_PHY_TYPE_UTMI 1 >>> -#define DWC2_PHY_TYPE_ULPI 2 >>> +#define DWC2_PHY_TYPE_UTMI 0 >>> +#define DWC2_PHY_TYPE_ULPI 1 >>> +#define DWC2_PHY_TYPE_FS 2 > According to table 5-10 of "DesignWare Cores USB 2.0 Hi-Speed > On-The-Go(OTG)" 3.10a databook, > > ULPI or UTMI+ Select (ULPI_UTMI_Sel) > The application uses this bit to select either a UTMI+ interface or ULPI > Interface. > ■ 1'b0: UTMI+ Interface > ■ 1'b1: ULPI Interface > > But the macor definition in current code would make a opposite config on > ULPI_UTMI_Sel bit, > (in the function "dwc_otg_core_init") > 1: UTMI+ Interface > 0: ULPI Interface > > It seems wrong. Please check at least socfpga, exynos and possibly other DWC2 users and ev. fix them up. Changing what seems wrong but works might cause quite some breakage. >>> #define CONFIG_DWC2_PHY_TYPE DWC2_PHY_TYPE_UTMI /* PHY type */ >>> -#define CONFIG_DWC2_UTMI_WIDTH 8 /* UTMI bus width (8/16) */ >>> +#define CONFIG_DWC2_UTMI_WIDTH 16 /* UTMI bus width (8/16) */ >> So basically you now force 16bit UTMI bus width instead of 8 ? > Three changes basically Three patches then. > 1. force 16bit UTMI bus width instead of 8 > 2. config force mode on the platform without HNP/SRP capability. > 3. change macro definition about PHY type (ULPI_UTMI_Sel). >> This breaks every single platform in U-Boot using the DWC2. >> >>> #undef CONFIG_DWC2_PHY_ULPI_DDR /* ULPI PHY uses DDR mode */ >>> #define CONFIG_DWC2_PHY_ULPI_EXT_VBUS /* ULPI PHY controls VBUS */ >>> @@ -785,5 +785,7 @@ struct dwc2_core_regs { >>> #undef CONFIG_DWC2_THR_CTL /* Threshold control */ >>> #define CONFIG_DWC2_TX_THR_LENGTH 64 >>> #undef CONFIG_DWC2_IC_USB_CAP /* IC Cap */ >>> +#define CONFIG_DWC2_FORCE_HOST_MODE /* Force host mode */ >>> +#undef CONFIG_DWC2_HNP_SRP_CAP /* HNP/SRP Cap */ >>> >>> #endif /* __DWC2_H__ */ >>> >> >
diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index 0e5df15..51619d6 100644 --- a/drivers/usb/host/dwc2.c +++ b/drivers/usb/host/dwc2.c @@ -370,7 +370,7 @@ static void dwc_otg_core_init(struct dwc2_priv *priv) usbcfg &= ~DWC2_GUSBCFG_DDRSEL; #endif } else { /* UTMI+ interface */ -#if (CONFIG_DWC2_UTMI_PHY_WIDTH == 16) +#if (CONFIG_DWC2_UTMI_WIDTH == 16) usbcfg |= DWC2_GUSBCFG_PHYIF; #endif } @@ -394,6 +394,11 @@ static void dwc_otg_core_init(struct dwc2_priv *priv) usbcfg |= DWC2_GUSBCFG_ULPI_CLK_SUS_M; } #endif + +#ifdef CONFIG_DWC2_FORCE_HOST_MODE + usbcfg |= DWC2_GUSBCFG_FORCEHOSTMODE; +#endif + writel(usbcfg, ®s->gusbcfg); /* Program the GAHBCFG Register. */ @@ -422,8 +427,10 @@ static void dwc_otg_core_init(struct dwc2_priv *priv) writel(ahbcfg, ®s->gahbcfg); +#ifdef CONFIG_DWC2_HNP_SRP_CAP /* Program the GUSBCFG register for HNP/SRP. */ setbits_le32(®s->gusbcfg, DWC2_GUSBCFG_HNPCAP | DWC2_GUSBCFG_SRPCAP); +#endif #ifdef CONFIG_DWC2_IC_USB_CAP setbits_le32(®s->gusbcfg, DWC2_GUSBCFG_IC_USB_CAP); diff --git a/drivers/usb/host/dwc2.h b/drivers/usb/host/dwc2.h index 4482dc6..3c62e9b 100644 --- a/drivers/usb/host/dwc2.h +++ b/drivers/usb/host/dwc2.h @@ -771,11 +771,11 @@ struct dwc2_core_regs { #define CONFIG_DWC2_MAX_TRANSFER_SIZE 65535 #define CONFIG_DWC2_MAX_PACKET_COUNT 511 -#define DWC2_PHY_TYPE_FS 0 -#define DWC2_PHY_TYPE_UTMI 1 -#define DWC2_PHY_TYPE_ULPI 2 +#define DWC2_PHY_TYPE_UTMI 0 +#define DWC2_PHY_TYPE_ULPI 1 +#define DWC2_PHY_TYPE_FS 2 #define CONFIG_DWC2_PHY_TYPE DWC2_PHY_TYPE_UTMI /* PHY type */ -#define CONFIG_DWC2_UTMI_WIDTH 8 /* UTMI bus width (8/16) */ +#define CONFIG_DWC2_UTMI_WIDTH 16 /* UTMI bus width (8/16) */ #undef CONFIG_DWC2_PHY_ULPI_DDR /* ULPI PHY uses DDR mode */ #define CONFIG_DWC2_PHY_ULPI_EXT_VBUS /* ULPI PHY controls VBUS */ @@ -785,5 +785,7 @@ struct dwc2_core_regs { #undef CONFIG_DWC2_THR_CTL /* Threshold control */ #define CONFIG_DWC2_TX_THR_LENGTH 64 #undef CONFIG_DWC2_IC_USB_CAP /* IC Cap */ +#define CONFIG_DWC2_FORCE_HOST_MODE /* Force host mode */ +#undef CONFIG_DWC2_HNP_SRP_CAP /* HNP/SRP Cap */ #endif /* __DWC2_H__ */
Fix macro error of dwc2 driver, add macro definition to config force mode and HNP/SRP capability. Signed-off-by: Meng Dongyang <daniel.meng@rock-chips.com> --- Changes in v2: - Splited from patch [07/08] of v1 drivers/usb/host/dwc2.c | 9 ++++++++- drivers/usb/host/dwc2.h | 10 ++++++---- 2 files changed, 14 insertions(+), 5 deletions(-)