| Message ID | 1467860066-15142-4-git-send-email-william.wu@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi William, Am Donnerstag, 7. Juli 2016, 10:54:24 schrieb William Wu: > Add a quirk to configure the core to support the > UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY > interface is hardware property, and it's platform > dependent. Normall, the PHYIf can be configured > during coreconsultant. But for some specific usb > cores(e.g. rk3399 soc dwc3), the default PHYIf > configuration value is fault, so we need to > reconfigure it by software. > > And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM > must be set to the corresponding value according to > the UTMI+ PHY interface. > > Signed-off-by: William Wu <william.wu@rock-chips.com> > --- [...] > diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt > b/Documentation/devicetree/bindings/usb/dwc3.txt index 020b0e9..8d7317d > 100644 > --- a/Documentation/devicetree/bindings/usb/dwc3.txt > +++ b/Documentation/devicetree/bindings/usb/dwc3.txt > @@ -42,6 +42,10 @@ Optional properties: > - snps,dis-u2-freeclk-exists-quirk: when set, clear the > u2_freeclk_exists in GUSB2PHYCFG, specify that USB2 PHY doesn't provide > a free-running PHY clock. > + - snps,phyif-utmi-quirk: when set core will set phyif UTMI+ interface. > + - snps,phyif-utmi: the value to configure the core to support a UTMI+ > PHY + with an 8- or 16-bit interface. Value 0 select 8-bit > + interface, value 1 select 16-bit interface. maybe snps,phyif-utmi-width = <8> or <16>; devicetree is about describing the hardware, not the things that get written to registers :-) . The conversion from the described width to the register value can easily be done in the driver. Also I don't think you need two properties for this. If the snps,phyif-utmi property is specified it indicates that you want to manually set the width and if it is absent you want to use the IC default. All functions reading property-values indicate if the property is missing. But it looks like there is already a precendence in snps,tx_de_emphasis(_quirk), so maybe Felipe has a different opinion here? > - snps,is-utmi-l1-suspend: true when DWC3 asserts output signal > utmi_l1_suspend_n, false when asserts utmi_sleep_n > - snps,hird-threshold: HIRD threshold > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 0b7bfd2..94036b1 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -408,6 +408,7 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc) > static int dwc3_phy_setup(struct dwc3 *dwc) > { > u32 reg; > + u32 usbtrdtim; > int ret; > > reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); > @@ -503,6 +504,15 @@ static int dwc3_phy_setup(struct dwc3 *dwc) > if (dwc->dis_u2_freeclk_exists_quirk) > reg &= ~DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS; > > + if (dwc->phyif_utmi_quirk) { > + reg &= ~(DWC3_GUSB2PHYCFG_PHYIF_MASK | > + DWC3_GUSB2PHYCFG_USBTRDTIM_MASK); > + usbtrdtim = dwc->phyif_utmi ? USBTRDTIM_UTMI_16_BIT : > + USBTRDTIM_UTMI_8_BIT; > + reg |= DWC3_GUSB2PHYCFG_PHYIF(dwc->phyif_utmi) | > + DWC3_GUSB2PHYCFG_USBTRDTIM(usbtrdtim); > + } > + > dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > > return 0; > @@ -834,6 +844,7 @@ static int dwc3_probe(struct platform_device *pdev) > struct resource *res; > struct dwc3 *dwc; > u8 lpm_nyet_threshold; > + u8 phyif_utmi; > u8 tx_de_emphasis; > u8 hird_threshold; > > @@ -880,6 +891,9 @@ static int dwc3_probe(struct platform_device *pdev) > /* default to highest possible threshold */ > lpm_nyet_threshold = 0xff; > > + /* default to UTMI+ 8-bit interface */ > + phyif_utmi = 0; > + > /* default to -3.5dB de-emphasis */ > tx_de_emphasis = 1; > > @@ -929,6 +943,10 @@ static int dwc3_probe(struct platform_device *pdev) > "snps,dis_rxdet_inp3_quirk"); > dwc->dis_u2_freeclk_exists_quirk = device_property_read_bool(dev, > "snps,dis-u2-freeclk-exists-quirk"); > + dwc->phyif_utmi_quirk = device_property_read_bool(dev, > + "snps,phyif-utmi-quirk"); > + device_property_read_u8(dev, "snps,phyif-utmi", > + &phyif_utmi); As described above device_property_read_u8 will return an error if the property is not present, so you could fill your dwc->phyif_utmi_quirk from that: ret = device_property_read_u8(dev, "snps,phyif-utmi", &phyif_utmi); dwc->phyif_utmi_quirk = (ret == 0) ? true : false; Heiko
Hi, Heiko Stuebner <heiko@sntech.de> writes: > Am Donnerstag, 7. Juli 2016, 10:54:24 schrieb William Wu: >> Add a quirk to configure the core to support the >> UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY >> interface is hardware property, and it's platform >> dependent. Normall, the PHYIf can be configured >> during coreconsultant. But for some specific usb >> cores(e.g. rk3399 soc dwc3), the default PHYIf >> configuration value is fault, so we need to >> reconfigure it by software. >> >> And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM >> must be set to the corresponding value according to >> the UTMI+ PHY interface. >> >> Signed-off-by: William Wu <william.wu@rock-chips.com> >> --- > [...] >> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt >> b/Documentation/devicetree/bindings/usb/dwc3.txt index 020b0e9..8d7317d >> 100644 >> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >> @@ -42,6 +42,10 @@ Optional properties: >> - snps,dis-u2-freeclk-exists-quirk: when set, clear the >> u2_freeclk_exists in GUSB2PHYCFG, specify that USB2 PHY doesn't provide >> a free-running PHY clock. >> + - snps,phyif-utmi-quirk: when set core will set phyif UTMI+ interface. >> + - snps,phyif-utmi: the value to configure the core to support a UTMI+ >> PHY + with an 8- or 16-bit interface. Value 0 select 8-bit >> + interface, value 1 select 16-bit interface. > > maybe > snps,phyif-utmi-width = <8> or <16>; > > devicetree is about describing the hardware, not the things that get written > to registers :-) . The conversion from the described width to the register > value can easily be done in the driver. > > > Also I don't think you need two properties for this. If the snps,phyif-utmi > property is specified it indicates that you want to manually set the width > and if it is absent you want to use the IC default. All functions reading > property-values indicate if the property is missing. > > But it looks like there is already a precendence in > snps,tx_de_emphasis(_quirk), so maybe Felipe has a different opinion here? > > > >> - snps,is-utmi-l1-suspend: true when DWC3 asserts output signal >> utmi_l1_suspend_n, false when asserts utmi_sleep_n >> - snps,hird-threshold: HIRD threshold >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 0b7bfd2..94036b1 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -408,6 +408,7 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc) >> static int dwc3_phy_setup(struct dwc3 *dwc) >> { >> u32 reg; >> + u32 usbtrdtim; >> int ret; >> >> reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); >> @@ -503,6 +504,15 @@ static int dwc3_phy_setup(struct dwc3 *dwc) >> if (dwc->dis_u2_freeclk_exists_quirk) >> reg &= ~DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS; >> >> + if (dwc->phyif_utmi_quirk) { >> + reg &= ~(DWC3_GUSB2PHYCFG_PHYIF_MASK | >> + DWC3_GUSB2PHYCFG_USBTRDTIM_MASK); >> + usbtrdtim = dwc->phyif_utmi ? USBTRDTIM_UTMI_16_BIT : >> + USBTRDTIM_UTMI_8_BIT; >> + reg |= DWC3_GUSB2PHYCFG_PHYIF(dwc->phyif_utmi) | >> + DWC3_GUSB2PHYCFG_USBTRDTIM(usbtrdtim); >> + } >> + >> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); >> >> return 0; >> @@ -834,6 +844,7 @@ static int dwc3_probe(struct platform_device *pdev) >> struct resource *res; >> struct dwc3 *dwc; >> u8 lpm_nyet_threshold; >> + u8 phyif_utmi; >> u8 tx_de_emphasis; >> u8 hird_threshold; >> >> @@ -880,6 +891,9 @@ static int dwc3_probe(struct platform_device *pdev) >> /* default to highest possible threshold */ >> lpm_nyet_threshold = 0xff; >> >> + /* default to UTMI+ 8-bit interface */ >> + phyif_utmi = 0; >> + >> /* default to -3.5dB de-emphasis */ >> tx_de_emphasis = 1; >> >> @@ -929,6 +943,10 @@ static int dwc3_probe(struct platform_device *pdev) >> "snps,dis_rxdet_inp3_quirk"); >> dwc->dis_u2_freeclk_exists_quirk = device_property_read_bool(dev, >> "snps,dis-u2-freeclk-exists-quirk"); >> + dwc->phyif_utmi_quirk = device_property_read_bool(dev, >> + "snps,phyif-utmi-quirk"); >> + device_property_read_u8(dev, "snps,phyif-utmi", >> + &phyif_utmi); > > > As described above device_property_read_u8 will return an error if the > property is not present, so you could fill your dwc->phyif_utmi_quirk from > that: > > ret = device_property_read_u8(dev, "snps,phyif-utmi", > &phyif_utmi); > dwc->phyif_utmi_quirk = (ret == 0) ? true : false; I like this much better :-) Unfortunately can't fix tx_deemphasis due to backwards compatibility :-s
Dear Heiko & Balbi， On 2016/7/8 21:29, Felipe Balbi wrote: > Hi, > > Heiko Stuebner <heiko@sntech.de> writes: >> Am Donnerstag, 7. Juli 2016, 10:54:24 schrieb William Wu: >>> Add a quirk to configure the core to support the >>> UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY >>> interface is hardware property, and it's platform >>> dependent. Normall, the PHYIf can be configured >>> during coreconsultant. But for some specific usb >>> cores(e.g. rk3399 soc dwc3), the default PHYIf >>> configuration value is fault, so we need to >>> reconfigure it by software. >>> >>> And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM >>> must be set to the corresponding value according to >>> the UTMI+ PHY interface. >>> >>> Signed-off-by: William Wu <william.wu@rock-chips.com> >>> --- >> [...] >>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt >>> b/Documentation/devicetree/bindings/usb/dwc3.txt index 020b0e9..8d7317d >>> 100644 >>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >>> @@ -42,6 +42,10 @@ Optional properties: >>> - snps,dis-u2-freeclk-exists-quirk: when set, clear the >>> u2_freeclk_exists in GUSB2PHYCFG, specify that USB2 PHY doesn't provide >>> a free-running PHY clock. >>> + - snps,phyif-utmi-quirk: when set core will set phyif UTMI+ interface. >>> + - snps,phyif-utmi: the value to configure the core to support a UTMI+ >>> PHY + with an 8- or 16-bit interface. Value 0 select 8-bit >>> + interface, value 1 select 16-bit interface. >> maybe >> snps,phyif-utmi-width = <8> or <16>; >> >> devicetree is about describing the hardware, not the things that get written >> to registers :-) . The conversion from the described width to the register >> value can easily be done in the driver. Thanks for your suggestion:-) Yes， “snps,phyif-utmi-width = <8> or <16>” is much clearer and easier to understand. And I have considered the same dts property for phyif-utmi, but I have no good idea about the conversion from described width to the registers value for the time being. About phyif utmi width configuration， we need to set two places in GUSB2PHYCFG register， according to DWC3 USB3.0 controller databook version3.00a，6.3.46 GUSB2PHYCFG ---------------------------------------------------------------------------------------------- Bits | Name | Description ---------------------------------------------------------------------------------------------- 13:10 | USBTRDTIM | Sets the turnaround time in PHY clocks. | | 4'h5: When the MAC interface is 16-bit UTMI+ | | 4'h9: When the MAC interface is 8-bit UTMI+/ULPI. ---------------------------------------------------------------------------------------------- 3 | PHYIF | If UTMI+ is selected, the application uses this bit to configure | | core to support a UTMI+ PHY with an 8- or 16-bit interface. | | 1'b0: 8 bits | | 1'b1: 16 bits ---------------------------------------------------------------------------------------------- And I think maybe I can try to do this: change it in dts: snps,phyif-utmi-width = <8> or <16>; Then convert to register value like this： device_property_read_u8(dev, "snps,phyif-utmi-width", &phyif_utmi_width); dwc->phyif_utmi = phyif_utmi_width >> 4; Ater the conversion， dwc->phyif_utmi value 0 means 8 bits, value 1 means 16 bits, and it's easier for us to config GUSB2PHYCFG. Is it OK? >> >> >> Also I don't think you need two properties for this. If the snps,phyif-utmi >> property is specified it indicates that you want to manually set the width >> and if it is absent you want to use the IC default. All functions reading >> property-values indicate if the property is missing. Ah, it seems very good to me. One dts property "snps,phyif-utmi" can help to reconfig phyif utmi width. And it seems that Felipe likes it very much too. :-D >> But it looks like there is already a precendence in >> snps,tx_de_emphasis(_quirk), so maybe Felipe has a different opinion here? >> >> >> >>> - snps,is-utmi-l1-suspend: true when DWC3 asserts output signal >>> utmi_l1_suspend_n, false when asserts utmi_sleep_n >>> - snps,hird-threshold: HIRD threshold >>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>> index 0b7bfd2..94036b1 100644 >>> --- a/drivers/usb/dwc3/core.c >>> +++ b/drivers/usb/dwc3/core.c >>> @@ -408,6 +408,7 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc) >>> static int dwc3_phy_setup(struct dwc3 *dwc) >>> { >>> u32 reg; >>> + u32 usbtrdtim; >>> int ret; >>> >>> reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); >>> @@ -503,6 +504,15 @@ static int dwc3_phy_setup(struct dwc3 *dwc) >>> if (dwc->dis_u2_freeclk_exists_quirk) >>> reg &= ~DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS; >>> >>> + if (dwc->phyif_utmi_quirk) { >>> + reg &= ~(DWC3_GUSB2PHYCFG_PHYIF_MASK | >>> + DWC3_GUSB2PHYCFG_USBTRDTIM_MASK); >>> + usbtrdtim = dwc->phyif_utmi ? USBTRDTIM_UTMI_16_BIT : >>> + USBTRDTIM_UTMI_8_BIT; >>> + reg |= DWC3_GUSB2PHYCFG_PHYIF(dwc->phyif_utmi) | >>> + DWC3_GUSB2PHYCFG_USBTRDTIM(usbtrdtim); >>> + } >>> + >>> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); >>> >>> return 0; >>> @@ -834,6 +844,7 @@ static int dwc3_probe(struct platform_device *pdev) >>> struct resource *res; >>> struct dwc3 *dwc; >>> u8 lpm_nyet_threshold; >>> + u8 phyif_utmi; >>> u8 tx_de_emphasis; >>> u8 hird_threshold; >>> >>> @@ -880,6 +891,9 @@ static int dwc3_probe(struct platform_device *pdev) >>> /* default to highest possible threshold */ >>> lpm_nyet_threshold = 0xff; >>> >>> + /* default to UTMI+ 8-bit interface */ >>> + phyif_utmi = 0; >>> + >>> /* default to -3.5dB de-emphasis */ >>> tx_de_emphasis = 1; >>> >>> @@ -929,6 +943,10 @@ static int dwc3_probe(struct platform_device *pdev) >>> "snps,dis_rxdet_inp3_quirk"); >>> dwc->dis_u2_freeclk_exists_quirk = device_property_read_bool(dev, >>> "snps,dis-u2-freeclk-exists-quirk"); >>> + dwc->phyif_utmi_quirk = device_property_read_bool(dev, >>> + "snps,phyif-utmi-quirk"); >>> + device_property_read_u8(dev, "snps,phyif-utmi", >>> + &phyif_utmi); >> >> As described above device_property_read_u8 will return an error if the >> property is not present, so you could fill your dwc->phyif_utmi_quirk from >> that: >> >> ret = device_property_read_u8(dev, "snps,phyif-utmi", >> &phyif_utmi); >> dwc->phyif_utmi_quirk = (ret == 0) ? true : false; > I like this much better :-) Unfortunately can't fix tx_deemphasis due to > backwards compatibility :-s I'll try to follow Heiko's suggestion and fix the dwc->phyif_utmi_quirk next patch. Best regards, William.wu >
Am Samstag, 9. Juli 2016, 11:38:00 schrieb William.wu: > Dear Heiko & Balbi， > > On 2016/7/8 21:29, Felipe Balbi wrote: > > Hi, > > > > Heiko Stuebner <heiko@sntech.de> writes: > >> Am Donnerstag, 7. Juli 2016, 10:54:24 schrieb William Wu: > >>> Add a quirk to configure the core to support the > >>> UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY > >>> interface is hardware property, and it's platform > >>> dependent. Normall, the PHYIf can be configured > >>> during coreconsultant. But for some specific usb > >>> cores(e.g. rk3399 soc dwc3), the default PHYIf > >>> configuration value is fault, so we need to > >>> reconfigure it by software. > >>> > >>> And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM > >>> must be set to the corresponding value according to > >>> the UTMI+ PHY interface. > >>> > >>> Signed-off-by: William Wu <william.wu@rock-chips.com> > >>> --- > >> > >> [...] > >> > >>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt > >>> b/Documentation/devicetree/bindings/usb/dwc3.txt index > >>> 020b0e9..8d7317d > >>> 100644 > >>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt > >>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt > >>> > >>> @@ -42,6 +42,10 @@ Optional properties: > >>> - snps,dis-u2-freeclk-exists-quirk: when set, clear the > >>> > >>> u2_freeclk_exists in GUSB2PHYCFG, specify that USB2 PHY doesn't > >>> provide > >>> > >>> a free-running PHY clock. > >>> > >>> + - snps,phyif-utmi-quirk: when set core will set phyif UTMI+ > >>> interface. > >>> + - snps,phyif-utmi: the value to configure the core to support a > >>> UTMI+ > >>> PHY + with an 8- or 16-bit interface. Value 0 select 8-bit > >>> + interface, value 1 select 16-bit interface. > >> > >> maybe > >> > >> snps,phyif-utmi-width = <8> or <16>; > >> > >> devicetree is about describing the hardware, not the things that get > >> written to registers :-) . The conversion from the described width to > >> the register value can easily be done in the driver. > > Thanks for your suggestion:-) > Yes， “snps,phyif-utmi-width = <8> or <16>” is much clearer and easier to > understand. > And I have considered the same dts property for phyif-utmi, but I have > no good idea about > the conversion from described width to the registers value for the time > being. > > About phyif utmi width configuration， we need to set two places in > GUSB2PHYCFG register， > according to DWC3 USB3.0 controller databook version3.00a，6.3.46 > GUSB2PHYCFG > > -------------------------------------------------------------------------- > -------------------- Bits | Name | Description > -------------------------------------------------------------------------- > -------------------- 13:10 | USBTRDTIM | Sets the turnaround > time in PHY clocks. > | | 4'h5: When the MAC > > interface is 16-bit UTMI+ > > | | 4'h9: When the MAC > > interface is 8-bit UTMI+/ULPI. > -------------------------------------------------------------------------- > -------------------- 3 | PHYIF | If UTMI+ is > selected, the application uses this bit to configure > > | | core to support a UTMI+ > > PHY with an 8- or 16-bit interface. > > | | 1'b0: 8 bits > | | 1'b1: 16 bits > > -------------------------------------------------------------------------- > -------------------- > > And I think maybe I can try to do this: > change it in dts: > snps,phyif-utmi-width = <8> or <16>; > > Then convert to register value like this： > device_property_read_u8(dev, "snps,phyif-utmi-width", > &phyif_utmi_width); > > dwc->phyif_utmi = phyif_utmi_width >> 4; > > Ater the conversion， dwc->phyif_utmi value 0 means 8 bits, value 1 > means 16 bits, > and it's easier for us to config GUSB2PHYCFG. > > Is it OK? or you could just store the actual width value read from the dts and make the core handle accordingly, making everything a bit more explicit. I guess personally I'd do something like: make dwc->phyif_utmi a regular unsigned int in probe: ret = device_property_read_u8(dev, "snps,phyif-utmi-width", &dwc->phyif_utmi); if (ret < 0) { dwc->phyif_utmi = 0; else if (dwc->phyif_utmi != 16 && dwc->phyif_utmi != 8) { dev_err(dev, "unsupported utmi interface width %d\n", dwc->phyif_utmi); return -EINVAL; } when setting your GUSB2PHYCFG register: if (dwc->phyif_utmi > 0) { reg &= ~(DWC3_GUSB2PHYCFG_PHYIF_MASK | DWC3_GUSB2PHYCFG_USBTRDTIM_MASK); usbtrdtim = (dwc->phyif_utmi == 16) ? USBTRDTIM_UTMI_16_BIT : USBTRDTIM_UTMI_8_BIT; phyif = (dwc->phyif_utmi == 16) ? 1 : 0; reg |= DWC3_GUSB2PHYCFG_PHYIF(phyif) | DWC3_GUSB2PHYCFG_USBTRDTIM(usbtrdtim) | } > >> Also I don't think you need two properties for this. If the > >> snps,phyif-utmi property is specified it indicates that you want to > >> manually set the width and if it is absent you want to use the IC > >> default. All functions reading property-values indicate if the > >> property is missing. > > Ah, it seems very good to me. One dts property "snps,phyif-utmi" can help > to reconfig phyif utmi width. And it seems that Felipe likes it very much > too. :-D yes, but please name it snps,phyif-utmi-width :-) Heiko
On Fri, Jul 08, 2016 at 02:33:09PM +0200, Heiko Stuebner wrote: > Hi William, > > Am Donnerstag, 7. Juli 2016, 10:54:24 schrieb William Wu: > > Add a quirk to configure the core to support the > > UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY > > interface is hardware property, and it's platform > > dependent. Normall, the PHYIf can be configured > > during coreconsultant. But for some specific usb > > cores(e.g. rk3399 soc dwc3), the default PHYIf > > configuration value is fault, so we need to > > reconfigure it by software. > > > > And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM > > must be set to the corresponding value according to > > the UTMI+ PHY interface. > > > > Signed-off-by: William Wu <william.wu@rock-chips.com> > > --- > [...] > > diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt > > b/Documentation/devicetree/bindings/usb/dwc3.txt index 020b0e9..8d7317d > > 100644 > > --- a/Documentation/devicetree/bindings/usb/dwc3.txt > > +++ b/Documentation/devicetree/bindings/usb/dwc3.txt > > @@ -42,6 +42,10 @@ Optional properties: > > - snps,dis-u2-freeclk-exists-quirk: when set, clear the > > u2_freeclk_exists in GUSB2PHYCFG, specify that USB2 PHY doesn't provide > > a free-running PHY clock. > > + - snps,phyif-utmi-quirk: when set core will set phyif UTMI+ interface. > > + - snps,phyif-utmi: the value to configure the core to support a UTMI+ > > PHY + with an 8- or 16-bit interface. Value 0 select 8-bit > > + interface, value 1 select 16-bit interface. > > maybe > snps,phyif-utmi-width = <8> or <16>; Seems like this could be common. Any other bindings have something similar already? If not "utmi-width" is fine. > > devicetree is about describing the hardware, not the things that get written > to registers :-) . The conversion from the described width to the register > value can easily be done in the driver. > > > Also I don't think you need two properties for this. If the snps,phyif-utmi > property is specified it indicates that you want to manually set the width > and if it is absent you want to use the IC default. All functions reading > property-values indicate if the property is missing. Agreed. Rob
On Thu, Jul 07, 2016 at 10:54:24AM +0800, William Wu wrote: > Add a quirk to configure the core to support the > UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY > interface is hardware property, and it's platform > dependent. Normall, the PHYIf can be configured s/Normall/Normally/ s/PHYIf/PHYIF/ > during coreconsultant. But for some specific usb s/usb/USB/ > cores(e.g. rk3399 soc dwc3), the default PHYIf > configuration value is fault, so we need to > reconfigure it by software. > > And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM s/dwc3/DWC3/ > must be set to the corresponding value according to > the UTMI+ PHY interface. And wrap your lines at 70-74 characters. Rob
Dear Heiko, On 2016/7/10 7:47, Heiko Stuebner wrote: > Am Samstag, 9. Juli 2016, 11:38:00 schrieb William.wu: >> Dear Heiko & Balbi， >> >> On 2016/7/8 21:29, Felipe Balbi wrote: >>> Hi, >>> >>> Heiko Stuebner <heiko@sntech.de> writes: >>>> Am Donnerstag, 7. Juli 2016, 10:54:24 schrieb William Wu: >>>>> Add a quirk to configure the core to support the >>>>> UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY >>>>> interface is hardware property, and it's platform >>>>> dependent. Normall, the PHYIf can be configured >>>>> during coreconsultant. But for some specific usb >>>>> cores(e.g. rk3399 soc dwc3), the default PHYIf >>>>> configuration value is fault, so we need to >>>>> reconfigure it by software. >>>>> >>>>> And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM >>>>> must be set to the corresponding value according to >>>>> the UTMI+ PHY interface. >>>>> >>>>> Signed-off-by: William Wu <william.wu@rock-chips.com> >>>>> --- >>>> [...] >>>> >>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt >>>>> b/Documentation/devicetree/bindings/usb/dwc3.txt index >>>>> 020b0e9..8d7317d >>>>> 100644 >>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >>>>> >>>>> @@ -42,6 +42,10 @@ Optional properties: >>>>> - snps,dis-u2-freeclk-exists-quirk: when set, clear the >>>>> >>>>> u2_freeclk_exists in GUSB2PHYCFG, specify that USB2 PHY doesn't >>>>> provide >>>>> >>>>> a free-running PHY clock. >>>>> >>>>> + - snps,phyif-utmi-quirk: when set core will set phyif UTMI+ >>>>> interface. >>>>> + - snps,phyif-utmi: the value to configure the core to support a >>>>> UTMI+ >>>>> PHY + with an 8- or 16-bit interface. Value 0 > select 8-bit >>>>> + interface, value 1 select 16-bit interface. >>>> maybe >>>> >>>> snps,phyif-utmi-width = <8> or <16>; >>>> >>>> devicetree is about describing the hardware, not the things that get >>>> written to registers :-) . The conversion from the described width to >>>> the register value can easily be done in the driver. >> Thanks for your suggestion:-) >> Yes， “snps,phyif-utmi-width = <8> or <16>” is much clearer and easier to >> understand. >> And I have considered the same dts property for phyif-utmi, but I have >> no good idea about >> the conversion from described width to the registers value for the time >> being. >> >> About phyif utmi width configuration， we need to set two places in >> GUSB2PHYCFG register， >> according to DWC3 USB3.0 controller databook version3.00a，6.3.46 >> GUSB2PHYCFG >> >> -------------------------------------------------------------------------- >> -------------------- Bits | Name | Description >> -------------------------------------------------------------------------- >> -------------------- 13:10 | USBTRDTIM | Sets the turnaround >> time in PHY clocks. >> | | 4'h5: When the MAC >> >> interface is 16-bit UTMI+ >> >> | | 4'h9: When the MAC >> >> interface is 8-bit UTMI+/ULPI. >> -------------------------------------------------------------------------- >> -------------------- 3 | PHYIF | If UTMI+ is >> selected, the application uses this bit to configure >> >> | | core to support a UTMI+ >> >> PHY with an 8- or 16-bit interface. >> >> | | 1'b0: 8 bits >> | | 1'b1: 16 bits >> >> -------------------------------------------------------------------------- >> -------------------- >> >> And I think maybe I can try to do this: >> change it in dts: >> snps,phyif-utmi-width = <8> or <16>; >> >> Then convert to register value like this： >> device_property_read_u8(dev, "snps,phyif-utmi-width", >> &phyif_utmi_width); >> >> dwc->phyif_utmi = phyif_utmi_width >> 4; >> >> Ater the conversion， dwc->phyif_utmi value 0 means 8 bits, value 1 >> means 16 bits, >> and it's easier for us to config GUSB2PHYCFG. >> >> Is it OK? > or you could just store the actual width value read from the dts and make > the core handle accordingly, making everything a bit more explicit. > > I guess personally I'd do something like: > > make dwc->phyif_utmi a regular unsigned int > > in probe: > ret = device_property_read_u8(dev, "snps,phyif-utmi-width", > &dwc->phyif_utmi); > if (ret < 0) { > dwc->phyif_utmi = 0; > else if (dwc->phyif_utmi != 16 && dwc->phyif_utmi != 8) { > dev_err(dev, "unsupported utmi interface width %d\n", > dwc->phyif_utmi); > return -EINVAL; > } > > > when setting your GUSB2PHYCFG register: > > if (dwc->phyif_utmi > 0) { > reg &= ~(DWC3_GUSB2PHYCFG_PHYIF_MASK | > DWC3_GUSB2PHYCFG_USBTRDTIM_MASK); > usbtrdtim = (dwc->phyif_utmi == 16) ? USBTRDTIM_UTMI_16_BIT : > USBTRDTIM_UTMI_8_BIT; > phyif = (dwc->phyif_utmi == 16) ? 1 : 0; > reg |= DWC3_GUSB2PHYCFG_PHYIF(phyif) | > DWC3_GUSB2PHYCFG_USBTRDTIM(usbtrdtim) | > } > Ah yes， it seems very good to me, very explicit. I'll upload a new patch according to your suggestion today. Thanks a lot~:-D >>>> Also I don't think you need two properties for this. If the >>>> snps,phyif-utmi property is specified it indicates that you want to >>>> manually set the width and if it is absent you want to use the IC >>>> default. All functions reading property-values indicate if the >>>> property is missing. >> Ah, it seems very good to me. One dts property "snps,phyif-utmi" can help >> to reconfig phyif utmi width. And it seems that Felipe likes it very much >> too. :-D > yes, but please name it snps,phyif-utmi-width :-) Yeah, thank you very much:-) Best Regards William Wu > > > Heiko > > >
Dear Rob, On 2016/7/11 22:54, Rob Herring wrote: > On Fri, Jul 08, 2016 at 02:33:09PM +0200, Heiko Stuebner wrote: >> Hi William, >> >> Am Donnerstag, 7. Juli 2016, 10:54:24 schrieb William Wu: >>> Add a quirk to configure the core to support the >>> UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY >>> interface is hardware property, and it's platform >>> dependent. Normall, the PHYIf can be configured >>> during coreconsultant. But for some specific usb >>> cores(e.g. rk3399 soc dwc3), the default PHYIf >>> configuration value is fault, so we need to >>> reconfigure it by software. >>> >>> And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM >>> must be set to the corresponding value according to >>> the UTMI+ PHY interface. >>> >>> Signed-off-by: William Wu <william.wu@rock-chips.com> >>> --- >> [...] >>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt >>> b/Documentation/devicetree/bindings/usb/dwc3.txt index 020b0e9..8d7317d >>> 100644 >>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >>> @@ -42,6 +42,10 @@ Optional properties: >>> - snps,dis-u2-freeclk-exists-quirk: when set, clear the >>> u2_freeclk_exists in GUSB2PHYCFG, specify that USB2 PHY doesn't provide >>> a free-running PHY clock. >>> + - snps,phyif-utmi-quirk: when set core will set phyif UTMI+ interface. >>> + - snps,phyif-utmi: the value to configure the core to support a UTMI+ >>> PHY + with an 8- or 16-bit interface. Value 0 select 8-bit >>> + interface, value 1 select 16-bit interface. >> maybe >> snps,phyif-utmi-width = <8> or <16>; > Seems like this could be common. Any other bindings have something > similar already? If not "utmi-width" is fine. It seems that there's not any dwc3 binding similar to this. So I prefer to use “utmi-width”. :-) > >> devicetree is about describing the hardware, not the things that get written >> to registers :-) . The conversion from the described width to the register >> value can easily be done in the driver. >> >> >> Also I don't think you need two properties for this. If the snps,phyif-utmi >> property is specified it indicates that you want to manually set the width >> and if it is absent you want to use the IC default. All functions reading >> property-values indicate if the property is missing. > Agreed. > > Rob > > >
Dear Rob, On 2016/7/11 22:58, Rob Herring wrote: > On Thu, Jul 07, 2016 at 10:54:24AM +0800, William Wu wrote: >> Add a quirk to configure the core to support the >> UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY >> interface is hardware property, and it's platform >> dependent. Normall, the PHYIf can be configured > s/Normall/Normally/ Yeah，I'll fix it.:-) > s/PHYIf/PHYIF/ Refer to DWC3 controller databook, "PHY Interface" called "PHYIf", so I describe "PHYIf" here. However， "PHYIF”seems more the norm, I'll fix it.:-) > >> during coreconsultant. But for some specific usb > s/usb/USB/ Thanks, I'll fix it.:-) > >> cores(e.g. rk3399 soc dwc3), the default PHYIf >> configuration value is fault, so we need to >> reconfigure it by software. >> >> And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM > s/dwc3/DWC3/ Thanks, I'll fix it too. > >> must be set to the corresponding value according to >> the UTMI+ PHY interface. > And wrap your lines at 70-74 characters. Thanks for your suggestion, I'll pay attention to this problem next patch.:-) Best Regards， William Wu > > Rob > > >
diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt index 020b0e9..8d7317d 100644 --- a/Documentation/devicetree/bindings/usb/dwc3.txt +++ b/Documentation/devicetree/bindings/usb/dwc3.txt @@ -42,6 +42,10 @@ Optional properties: - snps,dis-u2-freeclk-exists-quirk: when set, clear the u2_freeclk_exists in GUSB2PHYCFG, specify that USB2 PHY doesn't provide a free-running PHY clock. + - snps,phyif-utmi-quirk: when set core will set phyif UTMI+ interface. + - snps,phyif-utmi: the value to configure the core to support a UTMI+ PHY + with an 8- or 16-bit interface. Value 0 select 8-bit + interface, value 1 select 16-bit interface. - snps,is-utmi-l1-suspend: true when DWC3 asserts output signal utmi_l1_suspend_n, false when asserts utmi_sleep_n - snps,hird-threshold: HIRD threshold diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 0b7bfd2..94036b1 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -408,6 +408,7 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc) static int dwc3_phy_setup(struct dwc3 *dwc) { u32 reg; + u32 usbtrdtim; int ret; reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); @@ -503,6 +504,15 @@ static int dwc3_phy_setup(struct dwc3 *dwc) if (dwc->dis_u2_freeclk_exists_quirk) reg &= ~DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS; + if (dwc->phyif_utmi_quirk) { + reg &= ~(DWC3_GUSB2PHYCFG_PHYIF_MASK | + DWC3_GUSB2PHYCFG_USBTRDTIM_MASK); + usbtrdtim = dwc->phyif_utmi ? USBTRDTIM_UTMI_16_BIT : + USBTRDTIM_UTMI_8_BIT; + reg |= DWC3_GUSB2PHYCFG_PHYIF(dwc->phyif_utmi) | + DWC3_GUSB2PHYCFG_USBTRDTIM(usbtrdtim); + } + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); return 0; @@ -834,6 +844,7 @@ static int dwc3_probe(struct platform_device *pdev) struct resource *res; struct dwc3 *dwc; u8 lpm_nyet_threshold; + u8 phyif_utmi; u8 tx_de_emphasis; u8 hird_threshold; @@ -880,6 +891,9 @@ static int dwc3_probe(struct platform_device *pdev) /* default to highest possible threshold */ lpm_nyet_threshold = 0xff; + /* default to UTMI+ 8-bit interface */ + phyif_utmi = 0; + /* default to -3.5dB de-emphasis */ tx_de_emphasis = 1; @@ -929,6 +943,10 @@ static int dwc3_probe(struct platform_device *pdev) "snps,dis_rxdet_inp3_quirk"); dwc->dis_u2_freeclk_exists_quirk = device_property_read_bool(dev, "snps,dis-u2-freeclk-exists-quirk"); + dwc->phyif_utmi_quirk = device_property_read_bool(dev, + "snps,phyif-utmi-quirk"); + device_property_read_u8(dev, "snps,phyif-utmi", + &phyif_utmi); dwc->tx_de_emphasis_quirk = device_property_read_bool(dev, "snps,tx_de_emphasis_quirk"); @@ -940,6 +958,7 @@ static int dwc3_probe(struct platform_device *pdev) &dwc->fladj); dwc->lpm_nyet_threshold = lpm_nyet_threshold; + dwc->phyif_utmi = phyif_utmi; dwc->tx_de_emphasis = tx_de_emphasis; dwc->hird_threshold = hird_threshold diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index f321a5c..cf6696c 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -203,6 +203,12 @@ #define DWC3_GUSB2PHYCFG_SUSPHY (1 << 6) #define DWC3_GUSB2PHYCFG_ULPI_UTMI (1 << 4) #define DWC3_GUSB2PHYCFG_ENBLSLPM (1 << 8) +#define DWC3_GUSB2PHYCFG_PHYIF(n) (n << 3) +#define DWC3_GUSB2PHYCFG_PHYIF_MASK DWC3_GUSB2PHYCFG_PHYIF(1) +#define DWC3_GUSB2PHYCFG_USBTRDTIM(n) (n << 10) +#define DWC3_GUSB2PHYCFG_USBTRDTIM_MASK DWC3_GUSB2PHYCFG_USBTRDTIM(0xf) +#define USBTRDTIM_UTMI_8_BIT 9 +#define USBTRDTIM_UTMI_16_BIT 5 /* Global USB2 PHY Vendor Control Register */ #define DWC3_GUSB2PHYACC_NEWREGREQ (1 << 25) @@ -803,6 +809,10 @@ struct dwc3_scratchpad_array { * @dis_u2_freeclk_exists_quirk : set if we clear u2_freeclk_exists * in GUSB2PHYCFG, specify that USB2 PHY doesn't * provide a free-running PHY clock. + * @phyif_utmi_quirk: set if we enable phyif UTMI+ quirk + * @phyif_utmi: UTMI+ PHY interface value + * 0 - 8 bits + * 1 - 16 bits * @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk * @tx_de_emphasis: Tx de-emphasis value * 0 - -6dB de-emphasis @@ -948,6 +958,8 @@ struct dwc3 { unsigned dis_rxdet_inp3_quirk:1; unsigned dis_u2_freeclk_exists_quirk:1; + unsigned phyif_utmi_quirk:1; + unsigned phyif_utmi:1; unsigned tx_de_emphasis_quirk:1; unsigned tx_de_emphasis:2; };
Add a quirk to configure the core to support the UTMI+ PHY with an 8- or 16-bit interface. UTMI+ PHY interface is hardware property, and it's platform dependent. Normall, the PHYIf can be configured during coreconsultant. But for some specific usb cores(e.g. rk3399 soc dwc3), the default PHYIf configuration value is fault, so we need to reconfigure it by software. And refer to the dwc3 databook, the GUSB2PHYCFG.USBTRDTIM must be set to the corresponding value according to the UTMI+ PHY interface. Signed-off-by: William Wu <william.wu@rock-chips.com> --- Changes in v6: - use '-' instead of '_' in dts (Rob Herring) Changes in v5: - None Changes in v4: - rebase on top of balbi testing/next, remove pdata (balbi) Changes in v3: - None Changes in v2: - add a quirk for phyif_utmi (balbi) Documentation/devicetree/bindings/usb/dwc3.txt | 4 ++++ drivers/usb/dwc3/core.c | 19 +++++++++++++++++++ drivers/usb/dwc3/core.h | 12 ++++++++++++ 3 files changed, 35 insertions(+)