From mboxrd@z Thu Jan 1 00:00:00 1970 From: kever.yang@rock-chips.com (Kever Yang) Date: Tue, 29 Jul 2014 23:40:14 -0700 Subject: [PATCH 3/4] usb: dwc2: add compatible data for rockchip soc In-Reply-To: References: <1406683873-18194-1-git-send-email-kever.yang@rock-chips.com> <1406684102-18313-1-git-send-email-kever.yang@rock-chips.com> Message-ID: <53D8934E.9040807@rock-chips.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Doug: On 07/29/2014 09:21 PM, Doug Anderson wrote: > Kever, > > On Tue, Jul 29, 2014 at 6:35 PM, Kever Yang wrote: >> This patch add compatible data for dwc2 controller found on >> rk3066, rk3188 and rk3288 processors from rockchip. >> >> Signed-off-by: Kever Yang >> --- >> 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 > > > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kever Yang Subject: Re: [PATCH 3/4] usb: dwc2: add compatible data for rockchip soc Date: Tue, 29 Jul 2014 23:40:14 -0700 Message-ID: <53D8934E.9040807@rock-chips.com> References: <1406683873-18194-1-git-send-email-kever.yang@rock-chips.com> <1406684102-18313-1-git-send-email-kever.yang@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Doug Anderson Cc: Mark Rutland , Randy Dunlap , =?UTF-8?B?SGVpa28gU3TDvGJuZXI=?= , "linux-doc@vger.kernel.org" , Jianqun Xu , Russell King , Stephen Warren , Kishon Vijay Abraham I , lyz@rock-chips.com, "devicetree@vger.kernel.org" , Addy Ke , Pawel Moll , Ian Campbell , Matt Porter , Olof Johansson , Rob Herring , wulf@rock-chips.com, Sonny Rao , "linux-arm-kernel@lists.infradead.org" , Greg Kroah-Hartman , "linux-usb@vger.kernel.org" , linux-kernel List-Id: devicetree@vger.kernel.org Hi Doug: On 07/29/2014 09:21 PM, Doug Anderson wrote: > Kever, > > On Tue, Jul 29, 2014 at 6:35 PM, Kever Yang wrote: >> This patch add compatible data for dwc2 controller found on >> rk3066, rk3188 and rk3288 processors from rockchip. >> >> Signed-off-by: Kever Yang >> --- >> 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 > > >