| Message ID | 1406684102-18313-1-git-send-email-kever.yang@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Kever, On Tue, Jul 29, 2014 at 6:35 PM, Kever Yang <kever.yang@rock-chips.com> wrote: > This patch add compatible data for dwc2 controller found on > rk3066, rk3188 and rk3288 processors from rockchip. > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > --- > drivers/usb/dwc2/platform.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) I'm nowhere an expert here, but... > diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c > index a10e7a3..cc5983c 100644 > --- a/drivers/usb/dwc2/platform.c > +++ b/drivers/usb/dwc2/platform.c > @@ -75,6 +75,34 @@ static const struct dwc2_core_params params_bcm2835 = { > .uframe_sched = 0, > }; > > +static const struct dwc2_core_params params_rk3066 = { > + .otg_cap = 2, /* no HNP/SRP capable */ Are you sure HNP/SRP is not available? Do things break if you leave this at 0? > + .otg_ver = 0, /* 1.3 */ > + .dma_enable = 1, > + .dma_desc_enable = 0, > + .speed = 0, /* High Speed */ > + .enable_dynamic_fifo = 1, > + .en_multiple_tx_fifo = 1, > + .host_rx_fifo_size = 520, /* 520 DWORDs */ > + .host_nperio_tx_fifo_size = 128, /* 128 DWORDs */ > + .host_perio_tx_fifo_size = 256, /* 256 DWORDs */ > + .max_transfer_size = 65536, > + .max_packet_count = 512, Header file says max_packet_count should be max of 511. > + .host_channels = 9, > + .phy_type = 1, /* UTMI */ > + .phy_utmi_width = 16, /* 8 bits */ Either comment or value is wrong since 16 != 8 bits. > + .phy_ulpi_ddr = 0, /* Single */ > + .phy_ulpi_ext_vbus = 0, > + .i2c_enable = 0, > + .ulpi_fs_ls = 0, > + .host_support_fs_ls_low_power = 0, > + .host_ls_low_power_phy_clk = 0, /* 48 MHz */ > + .ts_dline = 0, > + .reload_ctl = 1, > + .ahbcfg = 0x17, /* dma enable & INCR16 */ > + .uframe_sched = 1, > +}; Many of these values could just be -1 to autodetect / use default. Should we consider doing that? -Doug
Hi Doug: On 07/29/2014 09:21 PM, Doug Anderson wrote: > Kever, > > On Tue, Jul 29, 2014 at 6:35 PM, Kever Yang <kever.yang@rock-chips.com> wrote: >> This patch add compatible data for dwc2 controller found on >> rk3066, rk3188 and rk3288 processors from rockchip. >> >> Signed-off-by: Kever Yang <kever.yang@rock-chips.com> >> --- >> drivers/usb/dwc2/platform.c | 29 +++++++++++++++++++++++++++++ >> 1 file changed, 29 insertions(+) > I'm nowhere an expert here, but... > > >> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c >> index a10e7a3..cc5983c 100644 >> --- a/drivers/usb/dwc2/platform.c >> +++ b/drivers/usb/dwc2/platform.c >> @@ -75,6 +75,34 @@ static const struct dwc2_core_params params_bcm2835 = { >> .uframe_sched = 0, >> }; >> >> +static const struct dwc2_core_params params_rk3066 = { >> + .otg_cap = 2, /* no HNP/SRP capable */ > Are you sure HNP/SRP is not available? Do things break if you leave this at 0? 1. HNP/SRP is not need right now, I didn't see it used in mobile devices. 2. If HNP/SRP is enabled, the controller will monitor the otg_vbus about 5V, overcurrent change will happen if vbus not detect after switch to host for a period of time. If not, the controller will monitor b_valid signal about 3V instead of 5V vbus, which match the hardware reference design from Rockchip. > > >> + .otg_ver = 0, /* 1.3 */ >> + .dma_enable = 1, >> + .dma_desc_enable = 0, >> + .speed = 0, /* High Speed */ >> + .enable_dynamic_fifo = 1, >> + .en_multiple_tx_fifo = 1, >> + .host_rx_fifo_size = 520, /* 520 DWORDs */ >> + .host_nperio_tx_fifo_size = 128, /* 128 DWORDs */ >> + .host_perio_tx_fifo_size = 256, /* 256 DWORDs */ >> + .max_transfer_size = 65536, >> + .max_packet_count = 512, > Header file says max_packet_count should be max of 511. Yeap, you are right, I will fix this. > > >> + .host_channels = 9, >> + .phy_type = 1, /* UTMI */ >> + .phy_utmi_width = 16, /* 8 bits */ > Either comment or value is wrong since 16 != 8 bits. I will change this to auto detect. > > >> + .phy_ulpi_ddr = 0, /* Single */ >> + .phy_ulpi_ext_vbus = 0, >> + .i2c_enable = 0, >> + .ulpi_fs_ls = 0, >> + .host_support_fs_ls_low_power = 0, >> + .host_ls_low_power_phy_clk = 0, /* 48 MHz */ >> + .ts_dline = 0, >> + .reload_ctl = 1, >> + .ahbcfg = 0x17, /* dma enable & INCR16 */ >> + .uframe_sched = 1, >> +}; > Many of these values could just be -1 to autodetect / use default. > Should we consider doing that? Most of the parameter can be auto-detect right except some have more than one choice, so I will leave the parameters to driver if it can do it right. > > -Doug > > >
Kever, On Tue, Jul 29, 2014 at 11:40 PM, Kever Yang <kever.yang@rock-chips.com> wrote: >> Are you sure HNP/SRP is not available? Do things break if you leave this >> at 0? > > 1. HNP/SRP is not need right now, I didn't see it used in mobile devices. > 2. If HNP/SRP is enabled, the controller will monitor the otg_vbus about 5V, > overcurrent change will happen if vbus not detect after switch to host > for a period of time. > If not, the controller will monitor b_valid signal about 3V instead of > 5V vbus, > which match the hardware reference design from Rockchip. OK. USB is pretty far away from my realm of expertise and I knew even less about OTG/gadget. It does sound like there's no fundamental issue with HNP/SRP on the SoC itself but that we're working around an issue with the board, though. If that's true, then perhaps we need another device tree property? -Doug
> From: Kever Yang [mailto:kever.yang@rock-chips.com] > Sent: Tuesday, July 29, 2014 6:35 PM > > This patch add compatible data for dwc2 controller found on > rk3066, rk3188 and rk3288 processors from rockchip. > > Signed-off-by: Kever Yang <kever.yang@rock-chips.com> > --- > drivers/usb/dwc2/platform.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c > index a10e7a3..cc5983c 100644 > --- a/drivers/usb/dwc2/platform.c > +++ b/drivers/usb/dwc2/platform.c > @@ -75,6 +75,34 @@ static const struct dwc2_core_params params_bcm2835 = { > .uframe_sched = 0, > }; > > +static const struct dwc2_core_params params_rk3066 = { > + .otg_cap = 2, /* no HNP/SRP capable */ > + .otg_ver = 0, /* 1.3 */ > + .dma_enable = 1, > + .dma_desc_enable = 0, > + .speed = 0, /* High Speed */ > + .enable_dynamic_fifo = 1, > + .en_multiple_tx_fifo = 1, > + .host_rx_fifo_size = 520, /* 520 DWORDs */ > + .host_nperio_tx_fifo_size = 128, /* 128 DWORDs */ > + .host_perio_tx_fifo_size = 256, /* 256 DWORDs */ > + .max_transfer_size = 65536, > + .max_packet_count = 512, > + .host_channels = 9, > + .phy_type = 1, /* UTMI */ > + .phy_utmi_width = 16, /* 8 bits */ The comment doesn't match the value. > + .phy_ulpi_ddr = 0, /* Single */ > + .phy_ulpi_ext_vbus = 0, > + .i2c_enable = 0, > + .ulpi_fs_ls = 0, > + .host_support_fs_ls_low_power = 0, > + .host_ls_low_power_phy_clk = 0, /* 48 MHz */ > + .ts_dline = 0, > + .reload_ctl = 1, > + .ahbcfg = 0x17, /* dma enable & INCR16 */ Don't set the dma enable bit here, the driver will set that bit according to the '.dma_enable' member above.
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index a10e7a3..cc5983c 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -75,6 +75,34 @@ static const struct dwc2_core_params params_bcm2835 = { .uframe_sched = 0, }; +static const struct dwc2_core_params params_rk3066 = { + .otg_cap = 2, /* no HNP/SRP capable */ + .otg_ver = 0, /* 1.3 */ + .dma_enable = 1, + .dma_desc_enable = 0, + .speed = 0, /* High Speed */ + .enable_dynamic_fifo = 1, + .en_multiple_tx_fifo = 1, + .host_rx_fifo_size = 520, /* 520 DWORDs */ + .host_nperio_tx_fifo_size = 128, /* 128 DWORDs */ + .host_perio_tx_fifo_size = 256, /* 256 DWORDs */ + .max_transfer_size = 65536, + .max_packet_count = 512, + .host_channels = 9, + .phy_type = 1, /* UTMI */ + .phy_utmi_width = 16, /* 8 bits */ + .phy_ulpi_ddr = 0, /* Single */ + .phy_ulpi_ext_vbus = 0, + .i2c_enable = 0, + .ulpi_fs_ls = 0, + .host_support_fs_ls_low_power = 0, + .host_ls_low_power_phy_clk = 0, /* 48 MHz */ + .ts_dline = 0, + .reload_ctl = 1, + .ahbcfg = 0x17, /* dma enable & INCR16 */ + .uframe_sched = 1, +}; + /** * dwc2_driver_remove() - Called when the DWC_otg core is unregistered with the * DWC_otg driver @@ -97,6 +125,7 @@ static int dwc2_driver_remove(struct platform_device *dev) static const struct of_device_id dwc2_of_match_table[] = { { .compatible = "brcm,bcm2835-usb", .data = ¶ms_bcm2835 }, + { .compatible = "rockchip,rk3066-usb", .data = ¶ms_rk3066 }, { .compatible = "snps,dwc2", .data = NULL }, {}, };
This patch add compatible data for dwc2 controller found on rk3066, rk3188 and rk3288 processors from rockchip. Signed-off-by: Kever Yang <kever.yang@rock-chips.com> --- drivers/usb/dwc2/platform.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)