Re: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk
Hi,
On Wed, Jul 29, 2015 at 11:19:01AM +, Badola Nikhil wrote:
> > > > yet another problem is that this register doesn't exist in *all*
> > > > versions of DWC3. It was introduced in version 2.50a so the branch I
> > > > typed above needs one extra check, and since it's getting so large,
> > > > it deserves be factored out into its own function.
> > > >
> > > > static int dwc3_frame_length_adjustment(struct dwc3 *dwc, u32 fladj) {
> > > > u32 reg;
> > > > u32 dft;
> > > >
> > > > if (dwc->revision <= DWC3_REVISION_250A)
> > > > return 0;
> > > >
> > > > if (fladj == 0)
> > > > return 0;
> > > >
> > > > reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
> > > > dft = reg & 0x3f; /* needs a mask macro */
> > > >
> > > > if (!dev_WARN_ONCE(dwc->dev, dft == fladj,
> > > > "requested value same as default, ignoring\n")) {
> > > > reg &= ~0x3f; /* needs a mask macro */
> > > > reg |= DWC3_GFLADJ_30MHZ_SDBND_SEL |
> > > > DWC3_GFLADJ_30MHZ(fladj_value);
> > > >
> > > > dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
> > > > }
> > > > }
> > > >
> > > > you really *MUST* check this sort of this out when writing patches.
> > > > It's not only about *your* SoC. You gotta remember we have a ton of
> > > > different users and those a prone to major grumpyness should a
> > > > completely unrelated patch break their use case.
> > > >
> > > > You have access to the IP's documentation, and that contains the
> > > > entire history of the IP itself, so it's easy to figure all of this
> > > > out with a simple search in the documentation.
> > > >
> > > > One extra detail is that you were very careless when writing to the
> > > > GFLADJ register too. You simply wrote your 30MHz sideband value,
> > > > potentially clearing other bits which shouldn't be touched. That
> > > > alone can add regressions.
> > > >
> > > > When resending, make sure all 3 patches reach linux-usb. I still
> > > > can't find patch 3/3.
> > > >
> > >
> > > Will take care of above scenarios and resend patches cc'ing linux-usb
> > > in each of them.
> > >
> > > Regarding acceptance of the patch only when it's used in glue layer,
> > > there is no freescale's glue layer present for dwc3 as of now.
> >
> > if there is no glue layer, how are you testing your patch ?
>
> We directly call dwc3_probe() . Please see usb node in file
> arch/arm/boot/dts/ls1021a.dtsi For your reference :
>
> usb3@310 {
> compatible = "snps,dwc3";
> reg = <0x0 0x310 0x0 0x1>;
> interrupts = ;
> dr_mode = "host";
> };
first time I see an SoC which needs nothing for its IP wrapper :-)
pretty cool.
> > > Furthermore, there is not any platform specific code required in glue
> > > layer apart from the ones present in dwc3/core.c.
> >
> > sorry ? core.c is generic for all users, the glue layer should somewhat hide
> > platform details such as clocks and PM. It's surprising if you don't need
> > anything on that side.
>
> We do not support power management for now. Also we are not sure If we
> need clocks and will look into it when we start working on power
> management.
> I would request you to accept this patch set (after modifications
> which you suggested) so that usb functionality doesn't break.
> I will send an incremental patch to use this frame length adjust patch
> via glue layer as soon as we introduce one.
sure, the only problem with that, is that if you end up adding a glue
layer, your old DTS sources will likely stop wotrking, but we'll get to
that when we get to that.
--
balbi
signature.asc
Description: Digital signature
RE: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk
> -Original Message-
> From: Felipe Balbi [mailto:ba...@ti.com]
> Sent: Monday, July 27, 2015 8:38 PM
> To: Badola Nikhil-B46172
> Cc: ba...@ti.com; linux-kernel@vger.kernel.org; linux-...@vger.kernel.org;
> linux-o...@vger.kernel.org; gre...@linuxfoundation.org
> Subject: Re: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk
>
> On Mon, Jul 27, 2015 at 06:56:48AM +, Badola Nikhil wrote:
> > > -Original Message-
> > > From: Felipe Balbi [mailto:ba...@ti.com]
> > > Sent: Thursday, July 23, 2015 8:39 PM
> > > To: Felipe Balbi
> > > Cc: Badola Nikhil-B46172; linux-kernel@vger.kernel.org; linux-
> > > u...@vger.kernel.org; linux-o...@vger.kernel.org;
> > > gre...@linuxfoundation.org
> > > Subject: Re: [PATCH 2/3] drivers: usb: dwc3: Add
> > > adjust_frame_length_quirk
> > >
> > > Hi again,
> > >
> > > On Thu, Jul 23, 2015 at 09:55:32AM -0500, Felipe Balbi wrote:
> > > > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> > > > > index
> > > > > 0447788..b7a5119 100644
> > > > > --- a/drivers/usb/dwc3/core.h
> > > > > +++ b/drivers/usb/dwc3/core.h
> > > > > @@ -124,6 +124,7 @@
> > > > > #define DWC3_GEVNTCOUNT(n) (0xc40c + (n * 0x10))
> > > > >
> > > > > #define DWC3_GHWPARAMS8 0xc600
> > > > > +#define DWC3_GFLADJ 0xc630
> > > > >
> > > > > /* Device Registers */
> > > > > #define DWC3_DCFG0xc700
> > > > > @@ -234,6 +235,10 @@
> > > > > /* Global HWPARAMS6 Register */
> > > > > #define DWC3_GHWPARAMS6_EN_FPGA (1 <<
> 7)
> > > > >
> > > > > +/* Global Frame Length Adjustment Register */
> > > > > +#define GFLADJ_30MHZ_REG_SEL (1 << 7)
> > > >
> > > > always prepend with DWC3_ like *all* other macros in this file.
> > > >
> > > > Also, match docs to ease grepping. This should be called
> > > > DWC3_GFLADJ_30MHZ_SDBND_SEL
> >
> > GFLADJ_30MHZ_REG_SEL is the field's name in LS1021A Reference Manual
> > as well as dwc3 databook. Though DWC3_GFLADJ_30MHZ_SDBND_SEL
> seems
> > more relevant.
>
> databook calls it GFLADJ_30MHZ_SDBND_SEL, I checked before sending my
> email.
>
> > > yet another problem is that this register doesn't exist in *all*
> > > versions of DWC3. It was introduced in version 2.50a so the branch I
> > > typed above needs one extra check, and since it's getting so large,
> > > it deserves be factored out into its own function.
> > >
> > > static int dwc3_frame_length_adjustment(struct dwc3 *dwc, u32 fladj) {
> > > u32 reg;
> > > u32 dft;
> > >
> > > if (dwc->revision <= DWC3_REVISION_250A)
> > > return 0;
> > >
> > > if (fladj == 0)
> > > return 0;
> > >
> > > reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
> > > dft = reg & 0x3f; /* needs a mask macro */
> > >
> > > if (!dev_WARN_ONCE(dwc->dev, dft == fladj,
> > > "requested value same as default, ignoring\n")) {
> > > reg &= ~0x3f; /* needs a mask macro */
> > > reg |= DWC3_GFLADJ_30MHZ_SDBND_SEL |
> > > DWC3_GFLADJ_30MHZ(fladj_value);
> > >
> > > dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
> > > }
> > > }
> > >
> > > you really *MUST* check this sort of this out when writing patches.
> > > It's not only about *your* SoC. You gotta remember we have a ton of
> > > different users and those a prone to major grumpyness should a
> > > completely unrelated patch break their use case.
> > >
> > > You have access to the IP's documentation, and that contains the
> > > entire history of the IP itself, so it's easy to figure all of this
> > > out with a simple search in the documentation.
> > >
> > > One extra detail is that you were very careless when writing to the
> > > GFLADJ register too. You simply wrote your 30MHz sideband value,
> > > potentially clearing other bits which shouldn't be touched. That
> > > alone can add regressions.
> > >
> > > When resending, make sure all 3 patches reach linux-usb. I still
> > > can't find patch 3/3.
> > >
> >
> > Will take care
RE: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk
-Original Message-
From: Felipe Balbi [mailto:ba...@ti.com]
Sent: Monday, July 27, 2015 8:38 PM
To: Badola Nikhil-B46172
Cc: ba...@ti.com; linux-kernel@vger.kernel.org; linux-...@vger.kernel.org;
linux-o...@vger.kernel.org; gre...@linuxfoundation.org
Subject: Re: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk
On Mon, Jul 27, 2015 at 06:56:48AM +, Badola Nikhil wrote:
-Original Message-
From: Felipe Balbi [mailto:ba...@ti.com]
Sent: Thursday, July 23, 2015 8:39 PM
To: Felipe Balbi
Cc: Badola Nikhil-B46172; linux-kernel@vger.kernel.org; linux-
u...@vger.kernel.org; linux-o...@vger.kernel.org;
gre...@linuxfoundation.org
Subject: Re: [PATCH 2/3] drivers: usb: dwc3: Add
adjust_frame_length_quirk
Hi again,
On Thu, Jul 23, 2015 at 09:55:32AM -0500, Felipe Balbi wrote:
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index
0447788..b7a5119 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -124,6 +124,7 @@
#define DWC3_GEVNTCOUNT(n) (0xc40c + (n * 0x10))
#define DWC3_GHWPARAMS8 0xc600
+#define DWC3_GFLADJ 0xc630
/* Device Registers */
#define DWC3_DCFG0xc700
@@ -234,6 +235,10 @@
/* Global HWPARAMS6 Register */
#define DWC3_GHWPARAMS6_EN_FPGA (1
7)
+/* Global Frame Length Adjustment Register */
+#define GFLADJ_30MHZ_REG_SEL (1 7)
always prepend with DWC3_ like *all* other macros in this file.
Also, match docs to ease grepping. This should be called
DWC3_GFLADJ_30MHZ_SDBND_SEL
GFLADJ_30MHZ_REG_SEL is the field's name in LS1021A Reference Manual
as well as dwc3 databook. Though DWC3_GFLADJ_30MHZ_SDBND_SEL
seems
more relevant.
databook calls it GFLADJ_30MHZ_SDBND_SEL, I checked before sending my
email.
yet another problem is that this register doesn't exist in *all*
versions of DWC3. It was introduced in version 2.50a so the branch I
typed above needs one extra check, and since it's getting so large,
it deserves be factored out into its own function.
static int dwc3_frame_length_adjustment(struct dwc3 *dwc, u32 fladj) {
u32 reg;
u32 dft;
if (dwc-revision = DWC3_REVISION_250A)
return 0;
if (fladj == 0)
return 0;
reg = dwc3_readl(dwc-regs, DWC3_GFLADJ);
dft = reg 0x3f; /* needs a mask macro */
if (!dev_WARN_ONCE(dwc-dev, dft == fladj,
requested value same as default, ignoring\n)) {
reg = ~0x3f; /* needs a mask macro */
reg |= DWC3_GFLADJ_30MHZ_SDBND_SEL |
DWC3_GFLADJ_30MHZ(fladj_value);
dwc3_writel(dwc-regs, DWC3_GFLADJ, reg);
}
}
you really *MUST* check this sort of this out when writing patches.
It's not only about *your* SoC. You gotta remember we have a ton of
different users and those a prone to major grumpyness should a
completely unrelated patch break their use case.
You have access to the IP's documentation, and that contains the
entire history of the IP itself, so it's easy to figure all of this
out with a simple search in the documentation.
One extra detail is that you were very careless when writing to the
GFLADJ register too. You simply wrote your 30MHz sideband value,
potentially clearing other bits which shouldn't be touched. That
alone can add regressions.
When resending, make sure all 3 patches reach linux-usb. I still
can't find patch 3/3.
Will take care of above scenarios and resend patches cc'ing linux-usb
in each of them.
Regarding acceptance of the patch only when it's used in glue layer,
there is no freescale's glue layer present for dwc3 as of now.
if there is no glue layer, how are you testing your patch ?
We directly call dwc3_probe() . Please see usb node in file
arch/arm/boot/dts/ls1021a.dtsi
For your reference :
usb3@310 {
compatible = snps,dwc3;
reg = 0x0 0x310 0x0 0x1;
interrupts = GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH;
dr_mode = host;
};
Furthermore, there is not any platform specific code required in glue
layer apart from the ones present in dwc3/core.c.
sorry ? core.c is generic for all users, the glue layer should somewhat hide
platform details such as clocks and PM. It's surprising if you don't need
anything on that side.
We do not support power management for now. Also we are not sure
If we need clocks and will look into it when we start working on power
management.
I would request you to accept this patch set (after modifications which you
suggested) so that usb functionality doesn't break.
I will send an incremental patch to use this frame length adjust patch via glue
layer
as soon as we
Re: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk
Hi,
On Wed, Jul 29, 2015 at 11:19:01AM +, Badola Nikhil wrote:
yet another problem is that this register doesn't exist in *all*
versions of DWC3. It was introduced in version 2.50a so the branch I
typed above needs one extra check, and since it's getting so large,
it deserves be factored out into its own function.
static int dwc3_frame_length_adjustment(struct dwc3 *dwc, u32 fladj) {
u32 reg;
u32 dft;
if (dwc-revision = DWC3_REVISION_250A)
return 0;
if (fladj == 0)
return 0;
reg = dwc3_readl(dwc-regs, DWC3_GFLADJ);
dft = reg 0x3f; /* needs a mask macro */
if (!dev_WARN_ONCE(dwc-dev, dft == fladj,
requested value same as default, ignoring\n)) {
reg = ~0x3f; /* needs a mask macro */
reg |= DWC3_GFLADJ_30MHZ_SDBND_SEL |
DWC3_GFLADJ_30MHZ(fladj_value);
dwc3_writel(dwc-regs, DWC3_GFLADJ, reg);
}
}
you really *MUST* check this sort of this out when writing patches.
It's not only about *your* SoC. You gotta remember we have a ton of
different users and those a prone to major grumpyness should a
completely unrelated patch break their use case.
You have access to the IP's documentation, and that contains the
entire history of the IP itself, so it's easy to figure all of this
out with a simple search in the documentation.
One extra detail is that you were very careless when writing to the
GFLADJ register too. You simply wrote your 30MHz sideband value,
potentially clearing other bits which shouldn't be touched. That
alone can add regressions.
When resending, make sure all 3 patches reach linux-usb. I still
can't find patch 3/3.
Will take care of above scenarios and resend patches cc'ing linux-usb
in each of them.
Regarding acceptance of the patch only when it's used in glue layer,
there is no freescale's glue layer present for dwc3 as of now.
if there is no glue layer, how are you testing your patch ?
We directly call dwc3_probe() . Please see usb node in file
arch/arm/boot/dts/ls1021a.dtsi For your reference :
usb3@310 {
compatible = snps,dwc3;
reg = 0x0 0x310 0x0 0x1;
interrupts = GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH;
dr_mode = host;
};
first time I see an SoC which needs nothing for its IP wrapper :-)
pretty cool.
Furthermore, there is not any platform specific code required in glue
layer apart from the ones present in dwc3/core.c.
sorry ? core.c is generic for all users, the glue layer should somewhat hide
platform details such as clocks and PM. It's surprising if you don't need
anything on that side.
We do not support power management for now. Also we are not sure If we
need clocks and will look into it when we start working on power
management.
I would request you to accept this patch set (after modifications
which you suggested) so that usb functionality doesn't break.
I will send an incremental patch to use this frame length adjust patch
via glue layer as soon as we introduce one.
sure, the only problem with that, is that if you end up adding a glue
layer, your old DTS sources will likely stop wotrking, but we'll get to
that when we get to that.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk
On Mon, Jul 27, 2015 at 06:56:48AM +, Badola Nikhil wrote:
> > -Original Message-
> > From: Felipe Balbi [mailto:ba...@ti.com]
> > Sent: Thursday, July 23, 2015 8:39 PM
> > To: Felipe Balbi
> > Cc: Badola Nikhil-B46172; linux-kernel@vger.kernel.org; linux-
> > u...@vger.kernel.org; linux-o...@vger.kernel.org;
> > gre...@linuxfoundation.org
> > Subject: Re: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk
> >
> > Hi again,
> >
> > On Thu, Jul 23, 2015 at 09:55:32AM -0500, Felipe Balbi wrote:
> > > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index
> > > > 0447788..b7a5119 100644
> > > > --- a/drivers/usb/dwc3/core.h
> > > > +++ b/drivers/usb/dwc3/core.h
> > > > @@ -124,6 +124,7 @@
> > > > #define DWC3_GEVNTCOUNT(n) (0xc40c + (n * 0x10))
> > > >
> > > > #define DWC3_GHWPARAMS80xc600
> > > > +#define DWC3_GFLADJ0xc630
> > > >
> > > > /* Device Registers */
> > > > #define DWC3_DCFG 0xc700
> > > > @@ -234,6 +235,10 @@
> > > > /* Global HWPARAMS6 Register */
> > > > #define DWC3_GHWPARAMS6_EN_FPGA(1 << 7)
> > > >
> > > > +/* Global Frame Length Adjustment Register */
> > > > +#define GFLADJ_30MHZ_REG_SEL (1 << 7)
> > >
> > > always prepend with DWC3_ like *all* other macros in this file.
> > >
> > > Also, match docs to ease grepping. This should be called
> > > DWC3_GFLADJ_30MHZ_SDBND_SEL
>
> GFLADJ_30MHZ_REG_SEL is the field's name in LS1021A Reference Manual
> as well as dwc3 databook. Though DWC3_GFLADJ_30MHZ_SDBND_SEL seems
> more relevant.
databook calls it GFLADJ_30MHZ_SDBND_SEL, I checked before sending my
email.
> > yet another problem is that this register doesn't exist in *all* versions of
> > DWC3. It was introduced in version 2.50a so the branch I typed above needs
> > one extra check, and since it's getting so large, it deserves be factored
> > out
> > into its own function.
> >
> > static int dwc3_frame_length_adjustment(struct dwc3 *dwc, u32 fladj) {
> > u32 reg;
> > u32 dft;
> >
> > if (dwc->revision <= DWC3_REVISION_250A)
> > return 0;
> >
> > if (fladj == 0)
> > return 0;
> >
> > reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
> > dft = reg & 0x3f; /* needs a mask macro */
> >
> > if (!dev_WARN_ONCE(dwc->dev, dft == fladj,
> > "requested value same as default, ignoring\n")) {
> > reg &= ~0x3f; /* needs a mask macro */
> > reg |= DWC3_GFLADJ_30MHZ_SDBND_SEL |
> > DWC3_GFLADJ_30MHZ(fladj_value);
> >
> > dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
> > }
> > }
> >
> > you really *MUST* check this sort of this out when writing patches. It's not
> > only about *your* SoC. You gotta remember we have a ton of different
> > users and those a prone to major grumpyness should a completely unrelated
> > patch break their use case.
> >
> > You have access to the IP's documentation, and that contains the entire
> > history of the IP itself, so it's easy to figure all of this out with a
> > simple search
> > in the documentation.
> >
> > One extra detail is that you were very careless when writing to the GFLADJ
> > register too. You simply wrote your 30MHz sideband value, potentially
> > clearing other bits which shouldn't be touched. That alone can add
> > regressions.
> >
> > When resending, make sure all 3 patches reach linux-usb. I still can't find
> > patch 3/3.
> >
>
> Will take care of above scenarios and resend patches cc'ing linux-usb
> in each of them.
>
> Regarding acceptance of the patch only when it's used in glue layer,
> there is no freescale's glue layer present for dwc3 as of now.
if there is no glue layer, how are you testing your patch ?
> Furthermore, there is not any platform specific code required in glue
> layer apart from the ones present in dwc3/core.c.
sorry ? core.c is generic for all users, the glue layer should somewhat
hide platform details such as clocks and PM. It's surprising if you
don't need anything on that side.
--
balbi
signature.asc
Description: Digital signature
RE: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk
> -Original Message-
> From: Felipe Balbi [mailto:ba...@ti.com]
> Sent: Thursday, July 23, 2015 8:39 PM
> To: Felipe Balbi
> Cc: Badola Nikhil-B46172; linux-kernel@vger.kernel.org; linux-
> u...@vger.kernel.org; linux-o...@vger.kernel.org;
> gre...@linuxfoundation.org
> Subject: Re: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk
>
> Hi again,
>
> On Thu, Jul 23, 2015 at 09:55:32AM -0500, Felipe Balbi wrote:
> > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index
> > > 0447788..b7a5119 100644
> > > --- a/drivers/usb/dwc3/core.h
> > > +++ b/drivers/usb/dwc3/core.h
> > > @@ -124,6 +124,7 @@
> > > #define DWC3_GEVNTCOUNT(n) (0xc40c + (n * 0x10))
> > >
> > > #define DWC3_GHWPARAMS8 0xc600
> > > +#define DWC3_GFLADJ 0xc630
> > >
> > > /* Device Registers */
> > > #define DWC3_DCFG0xc700
> > > @@ -234,6 +235,10 @@
> > > /* Global HWPARAMS6 Register */
> > > #define DWC3_GHWPARAMS6_EN_FPGA (1 << 7)
> > >
> > > +/* Global Frame Length Adjustment Register */
> > > +#define GFLADJ_30MHZ_REG_SEL (1 << 7)
> >
> > always prepend with DWC3_ like *all* other macros in this file.
> >
> > Also, match docs to ease grepping. This should be called
> > DWC3_GFLADJ_30MHZ_SDBND_SEL
GFLADJ_30MHZ_REG_SEL is the field's name in LS1021A Reference Manual as well
as dwc3 databook. Though DWC3_GFLADJ_30MHZ_SDBND_SEL seems more
relevant.
>
> yet another problem is that this register doesn't exist in *all* versions of
> DWC3. It was introduced in version 2.50a so the branch I typed above needs
> one extra check, and since it's getting so large, it deserves be factored out
> into its own function.
>
> static int dwc3_frame_length_adjustment(struct dwc3 *dwc, u32 fladj) {
> u32 reg;
> u32 dft;
>
> if (dwc->revision <= DWC3_REVISION_250A)
> return 0;
>
> if (fladj == 0)
> return 0;
>
> reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
> dft = reg & 0x3f; /* needs a mask macro */
>
> if (!dev_WARN_ONCE(dwc->dev, dft == fladj,
> "requested value same as default, ignoring\n")) {
> reg &= ~0x3f; /* needs a mask macro */
> reg |= DWC3_GFLADJ_30MHZ_SDBND_SEL |
> DWC3_GFLADJ_30MHZ(fladj_value);
>
> dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
> }
> }
>
> you really *MUST* check this sort of this out when writing patches. It's not
> only about *your* SoC. You gotta remember we have a ton of different
> users and those a prone to major grumpyness should a completely unrelated
> patch break their use case.
>
> You have access to the IP's documentation, and that contains the entire
> history of the IP itself, so it's easy to figure all of this out with a
> simple search
> in the documentation.
>
> One extra detail is that you were very careless when writing to the GFLADJ
> register too. You simply wrote your 30MHz sideband value, potentially
> clearing other bits which shouldn't be touched. That alone can add
> regressions.
>
> When resending, make sure all 3 patches reach linux-usb. I still can't find
> patch 3/3.
>
Will take care of above scenarios and resend patches cc'ing linux-usb in each
of them.
Regarding acceptance of the patch only when it's used in glue layer, there is
no freescale's
glue layer present for dwc3 as of now. Furthermore, there is not any platform
specific
code required in glue layer apart from the ones present in dwc3/core.c.
Please suggest.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk
On Mon, Jul 27, 2015 at 06:56:48AM +, Badola Nikhil wrote:
-Original Message-
From: Felipe Balbi [mailto:ba...@ti.com]
Sent: Thursday, July 23, 2015 8:39 PM
To: Felipe Balbi
Cc: Badola Nikhil-B46172; linux-kernel@vger.kernel.org; linux-
u...@vger.kernel.org; linux-o...@vger.kernel.org;
gre...@linuxfoundation.org
Subject: Re: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk
Hi again,
On Thu, Jul 23, 2015 at 09:55:32AM -0500, Felipe Balbi wrote:
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index
0447788..b7a5119 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -124,6 +124,7 @@
#define DWC3_GEVNTCOUNT(n) (0xc40c + (n * 0x10))
#define DWC3_GHWPARAMS80xc600
+#define DWC3_GFLADJ0xc630
/* Device Registers */
#define DWC3_DCFG 0xc700
@@ -234,6 +235,10 @@
/* Global HWPARAMS6 Register */
#define DWC3_GHWPARAMS6_EN_FPGA(1 7)
+/* Global Frame Length Adjustment Register */
+#define GFLADJ_30MHZ_REG_SEL (1 7)
always prepend with DWC3_ like *all* other macros in this file.
Also, match docs to ease grepping. This should be called
DWC3_GFLADJ_30MHZ_SDBND_SEL
GFLADJ_30MHZ_REG_SEL is the field's name in LS1021A Reference Manual
as well as dwc3 databook. Though DWC3_GFLADJ_30MHZ_SDBND_SEL seems
more relevant.
databook calls it GFLADJ_30MHZ_SDBND_SEL, I checked before sending my
email.
yet another problem is that this register doesn't exist in *all* versions of
DWC3. It was introduced in version 2.50a so the branch I typed above needs
one extra check, and since it's getting so large, it deserves be factored
out
into its own function.
static int dwc3_frame_length_adjustment(struct dwc3 *dwc, u32 fladj) {
u32 reg;
u32 dft;
if (dwc-revision = DWC3_REVISION_250A)
return 0;
if (fladj == 0)
return 0;
reg = dwc3_readl(dwc-regs, DWC3_GFLADJ);
dft = reg 0x3f; /* needs a mask macro */
if (!dev_WARN_ONCE(dwc-dev, dft == fladj,
requested value same as default, ignoring\n)) {
reg = ~0x3f; /* needs a mask macro */
reg |= DWC3_GFLADJ_30MHZ_SDBND_SEL |
DWC3_GFLADJ_30MHZ(fladj_value);
dwc3_writel(dwc-regs, DWC3_GFLADJ, reg);
}
}
you really *MUST* check this sort of this out when writing patches. It's not
only about *your* SoC. You gotta remember we have a ton of different
users and those a prone to major grumpyness should a completely unrelated
patch break their use case.
You have access to the IP's documentation, and that contains the entire
history of the IP itself, so it's easy to figure all of this out with a
simple search
in the documentation.
One extra detail is that you were very careless when writing to the GFLADJ
register too. You simply wrote your 30MHz sideband value, potentially
clearing other bits which shouldn't be touched. That alone can add
regressions.
When resending, make sure all 3 patches reach linux-usb. I still can't find
patch 3/3.
Will take care of above scenarios and resend patches cc'ing linux-usb
in each of them.
Regarding acceptance of the patch only when it's used in glue layer,
there is no freescale's glue layer present for dwc3 as of now.
if there is no glue layer, how are you testing your patch ?
Furthermore, there is not any platform specific code required in glue
layer apart from the ones present in dwc3/core.c.
sorry ? core.c is generic for all users, the glue layer should somewhat
hide platform details such as clocks and PM. It's surprising if you
don't need anything on that side.
--
balbi
signature.asc
Description: Digital signature
RE: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk
-Original Message-
From: Felipe Balbi [mailto:ba...@ti.com]
Sent: Thursday, July 23, 2015 8:39 PM
To: Felipe Balbi
Cc: Badola Nikhil-B46172; linux-kernel@vger.kernel.org; linux-
u...@vger.kernel.org; linux-o...@vger.kernel.org;
gre...@linuxfoundation.org
Subject: Re: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk
Hi again,
On Thu, Jul 23, 2015 at 09:55:32AM -0500, Felipe Balbi wrote:
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index
0447788..b7a5119 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -124,6 +124,7 @@
#define DWC3_GEVNTCOUNT(n) (0xc40c + (n * 0x10))
#define DWC3_GHWPARAMS8 0xc600
+#define DWC3_GFLADJ 0xc630
/* Device Registers */
#define DWC3_DCFG0xc700
@@ -234,6 +235,10 @@
/* Global HWPARAMS6 Register */
#define DWC3_GHWPARAMS6_EN_FPGA (1 7)
+/* Global Frame Length Adjustment Register */
+#define GFLADJ_30MHZ_REG_SEL (1 7)
always prepend with DWC3_ like *all* other macros in this file.
Also, match docs to ease grepping. This should be called
DWC3_GFLADJ_30MHZ_SDBND_SEL
GFLADJ_30MHZ_REG_SEL is the field's name in LS1021A Reference Manual as well
as dwc3 databook. Though DWC3_GFLADJ_30MHZ_SDBND_SEL seems more
relevant.
yet another problem is that this register doesn't exist in *all* versions of
DWC3. It was introduced in version 2.50a so the branch I typed above needs
one extra check, and since it's getting so large, it deserves be factored out
into its own function.
static int dwc3_frame_length_adjustment(struct dwc3 *dwc, u32 fladj) {
u32 reg;
u32 dft;
if (dwc-revision = DWC3_REVISION_250A)
return 0;
if (fladj == 0)
return 0;
reg = dwc3_readl(dwc-regs, DWC3_GFLADJ);
dft = reg 0x3f; /* needs a mask macro */
if (!dev_WARN_ONCE(dwc-dev, dft == fladj,
requested value same as default, ignoring\n)) {
reg = ~0x3f; /* needs a mask macro */
reg |= DWC3_GFLADJ_30MHZ_SDBND_SEL |
DWC3_GFLADJ_30MHZ(fladj_value);
dwc3_writel(dwc-regs, DWC3_GFLADJ, reg);
}
}
you really *MUST* check this sort of this out when writing patches. It's not
only about *your* SoC. You gotta remember we have a ton of different
users and those a prone to major grumpyness should a completely unrelated
patch break their use case.
You have access to the IP's documentation, and that contains the entire
history of the IP itself, so it's easy to figure all of this out with a
simple search
in the documentation.
One extra detail is that you were very careless when writing to the GFLADJ
register too. You simply wrote your 30MHz sideband value, potentially
clearing other bits which shouldn't be touched. That alone can add
regressions.
When resending, make sure all 3 patches reach linux-usb. I still can't find
patch 3/3.
Will take care of above scenarios and resend patches cc'ing linux-usb in each
of them.
Regarding acceptance of the patch only when it's used in glue layer, there is
no freescale's
glue layer present for dwc3 as of now. Furthermore, there is not any platform
specific
code required in glue layer apart from the ones present in dwc3/core.c.
Please suggest.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk
Hi again,
On Thu, Jul 23, 2015 at 09:55:32AM -0500, Felipe Balbi wrote:
> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> > index 0447788..b7a5119 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -124,6 +124,7 @@
> > #define DWC3_GEVNTCOUNT(n) (0xc40c + (n * 0x10))
> >
> > #define DWC3_GHWPARAMS80xc600
> > +#define DWC3_GFLADJ0xc630
> >
> > /* Device Registers */
> > #define DWC3_DCFG 0xc700
> > @@ -234,6 +235,10 @@
> > /* Global HWPARAMS6 Register */
> > #define DWC3_GHWPARAMS6_EN_FPGA(1 << 7)
> >
> > +/* Global Frame Length Adjustment Register */
> > +#define GFLADJ_30MHZ_REG_SEL (1 << 7)
>
> always prepend with DWC3_ like *all* other macros in this file.
>
> Also, match docs to ease grepping. This should be called
> DWC3_GFLADJ_30MHZ_SDBND_SEL
yet another problem is that this register doesn't exist in *all*
versions of DWC3. It was introduced in version 2.50a so the branch I
typed above needs one extra check, and since it's getting so large, it
deserves be factored out into its own function.
static int dwc3_frame_length_adjustment(struct dwc3 *dwc, u32 fladj)
{
u32 reg;
u32 dft;
if (dwc->revision <= DWC3_REVISION_250A)
return 0;
if (fladj == 0)
return 0;
reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
dft = reg & 0x3f; /* needs a mask macro */
if (!dev_WARN_ONCE(dwc->dev, dft == fladj,
"requested value same as default, ignoring\n")) {
reg &= ~0x3f; /* needs a mask macro */
reg |= DWC3_GFLADJ_30MHZ_SDBND_SEL |
DWC3_GFLADJ_30MHZ(fladj_value);
dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
}
}
you really *MUST* check this sort of this out when writing patches. It's
not only about *your* SoC. You gotta remember we have a ton of different
users and those a prone to major grumpyness should a completely
unrelated patch break their use case.
You have access to the IP's documentation, and that contains the entire
history of the IP itself, so it's easy to figure all of this out with a
simple search in the documentation.
One extra detail is that you were very careless when writing to the
GFLADJ register too. You simply wrote your 30MHz sideband value,
potentially clearing other bits which shouldn't be touched. That alone
can add regressions.
When resending, make sure all 3 patches reach linux-usb. I still can't
find patch 3/3.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk
Hi,
On Thu, Jul 23, 2015 at 03:41:35PM +0530, Nikhil Badola wrote:
> Add adjust_frame_length_quirk for writing to fladj register
> which adjusts (micro)frame length to value provided by
> "snps,configure-fladj" property thus avoiding USB 2.0 devices
> to time-out over a longer run
>
> Signed-off-by: Nikhil Badola
just like the other patch, I won't take this without a glue layer making
use of it.
> ---
> drivers/usb/dwc3/core.c | 12
> drivers/usb/dwc3/core.h | 7 +++
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 5c110d8..72ba025 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -779,6 +779,7 @@ static int dwc3_probe(struct platform_device *pdev)
> u8 lpm_nyet_threshold;
> u8 tx_de_emphasis;
> u8 hird_threshold;
> + u32 fladj_value;
>
> int ret;
>
> @@ -886,6 +887,12 @@ static int dwc3_probe(struct platform_device *pdev)
> _de_emphasis);
> of_property_read_string(node, "snps,hsphy_interface",
> >hsphy_interface);
> + ret = of_property_read_u32(node, "snps,configure-fladj",
This is not the correct name for the property, it should be something
like 'snps,quirk-frame-length-adjustment'. Quirk because this is only
needed when the 30MHz sideband signal is invalid for some reason.
This is also something that's only needed for host side operation, so
consider what would you do if you weren't using dwc3, if all you had was
XHCI.
I hope freescale will fix this silicon bug.
One extra thing: everything that can be done via DT, should be available
for pdata users as well.
> +_value);
> + if (!ret)
> + dwc->adjust_frame_length_quirk = 1;
> + else
> + dwc->adjust_frame_length_quirk = 0;
this flag is unnecessary. Just initialize fladj_value to 0 and check for
that:
> } else if (pdata) {
> dwc->maximum_speed = pdata->maximum_speed;
> dwc->has_lpm_erratum = pdata->has_lpm_erratum;
> @@ -957,6 +964,11 @@ static int dwc3_probe(struct platform_device *pdev)
> goto err1;
> }
>
> + /* Adjust Frame Length */
> + if (dwc->adjust_frame_length_quirk)
if (fladj) {
u32 reg;
u32 dft;
reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
dft = reg & 0x3f; /* needs a mask macro */
if (!dev_WARN_ONCE(dwc->dev, dft == fladj,
"request value same as default, ignoring\n")) {
reg &= ~0x3f; /* needs a mask macro */
reg |= DWC3_GFLADJ_30MHZ_SDBND_SEL |
DWC3_GFLADJ_30MHZ(fladj_value);
dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
}
}
> + dwc3_writel(dwc->regs, DWC3_GFLADJ, GFLADJ_30MHZ_REG_SEL |
> + GFLADJ_30MHZ(fladj_value));
> +
> if (IS_ENABLED(CONFIG_USB_DWC3_HOST))
> dwc->dr_mode = USB_DR_MODE_HOST;
> else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET))
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 0447788..b7a5119 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -124,6 +124,7 @@
> #define DWC3_GEVNTCOUNT(n) (0xc40c + (n * 0x10))
>
> #define DWC3_GHWPARAMS8 0xc600
> +#define DWC3_GFLADJ 0xc630
>
> /* Device Registers */
> #define DWC3_DCFG0xc700
> @@ -234,6 +235,10 @@
> /* Global HWPARAMS6 Register */
> #define DWC3_GHWPARAMS6_EN_FPGA (1 << 7)
>
> +/* Global Frame Length Adjustment Register */
> +#define GFLADJ_30MHZ_REG_SEL (1 << 7)
always prepend with DWC3_ like *all* other macros in this file.
Also, match docs to ease grepping. This should be called
DWC3_GFLADJ_30MHZ_SDBND_SEL
> +#define GFLADJ_30MHZ(n) ((n) & 0x3f)
> +
> /* Device Configuration Register */
> #define DWC3_DCFG_DEVADDR(addr) ((addr) << 3)
> #define DWC3_DCFG_DEVADDR_MASK DWC3_DCFG_DEVADDR(0x7f)
> @@ -712,6 +717,7 @@ struct dwc3_scratchpad_array {
> * @rx_detect_poll_quirk: set if we enable rx_detect to polling lfps quirk
> * @dis_u3_susphy_quirk: set if we disable usb3 suspend phy
> * @dis_u2_susphy_quirk: set if we disable usb2 suspend phy
> + * @adjust_frame_length_quirk: enables post-silicon frame length adjustment
> * @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk
> * @tx_de_emphasis: Tx de-emphasis value
> * 0 - -6dB de-emphasis
> @@ -841,6 +847,7 @@ struct dwc3 {
> unsignedrx_detect_poll_quirk:1;
> unsigneddis_u3_susphy_quirk:1;
>
[PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk
Add adjust_frame_length_quirk for writing to fladj register
which adjusts (micro)frame length to value provided by
"snps,configure-fladj" property thus avoiding USB 2.0 devices
to time-out over a longer run
Signed-off-by: Nikhil Badola
---
drivers/usb/dwc3/core.c | 12
drivers/usb/dwc3/core.h | 7 +++
2 files changed, 19 insertions(+)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 5c110d8..72ba025 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -779,6 +779,7 @@ static int dwc3_probe(struct platform_device *pdev)
u8 lpm_nyet_threshold;
u8 tx_de_emphasis;
u8 hird_threshold;
+ u32 fladj_value;
int ret;
@@ -886,6 +887,12 @@ static int dwc3_probe(struct platform_device *pdev)
_de_emphasis);
of_property_read_string(node, "snps,hsphy_interface",
>hsphy_interface);
+ ret = of_property_read_u32(node, "snps,configure-fladj",
+ _value);
+ if (!ret)
+ dwc->adjust_frame_length_quirk = 1;
+ else
+ dwc->adjust_frame_length_quirk = 0;
} else if (pdata) {
dwc->maximum_speed = pdata->maximum_speed;
dwc->has_lpm_erratum = pdata->has_lpm_erratum;
@@ -957,6 +964,11 @@ static int dwc3_probe(struct platform_device *pdev)
goto err1;
}
+ /* Adjust Frame Length */
+ if (dwc->adjust_frame_length_quirk)
+ dwc3_writel(dwc->regs, DWC3_GFLADJ, GFLADJ_30MHZ_REG_SEL |
+ GFLADJ_30MHZ(fladj_value));
+
if (IS_ENABLED(CONFIG_USB_DWC3_HOST))
dwc->dr_mode = USB_DR_MODE_HOST;
else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET))
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 0447788..b7a5119 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -124,6 +124,7 @@
#define DWC3_GEVNTCOUNT(n) (0xc40c + (n * 0x10))
#define DWC3_GHWPARAMS80xc600
+#define DWC3_GFLADJ0xc630
/* Device Registers */
#define DWC3_DCFG 0xc700
@@ -234,6 +235,10 @@
/* Global HWPARAMS6 Register */
#define DWC3_GHWPARAMS6_EN_FPGA(1 << 7)
+/* Global Frame Length Adjustment Register */
+#define GFLADJ_30MHZ_REG_SEL (1 << 7)
+#define GFLADJ_30MHZ(n)((n) & 0x3f)
+
/* Device Configuration Register */
#define DWC3_DCFG_DEVADDR(addr)((addr) << 3)
#define DWC3_DCFG_DEVADDR_MASK DWC3_DCFG_DEVADDR(0x7f)
@@ -712,6 +717,7 @@ struct dwc3_scratchpad_array {
* @rx_detect_poll_quirk: set if we enable rx_detect to polling lfps quirk
* @dis_u3_susphy_quirk: set if we disable usb3 suspend phy
* @dis_u2_susphy_quirk: set if we disable usb2 suspend phy
+ * @adjust_frame_length_quirk: enables post-silicon frame length adjustment
* @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk
* @tx_de_emphasis: Tx de-emphasis value
* 0 - -6dB de-emphasis
@@ -841,6 +847,7 @@ struct dwc3 {
unsignedrx_detect_poll_quirk:1;
unsigneddis_u3_susphy_quirk:1;
unsigneddis_u2_susphy_quirk:1;
+ unsignedadjust_frame_length_quirk:1;
unsignedtx_de_emphasis_quirk:1;
unsignedtx_de_emphasis:2;
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk
Hi,
On Thu, Jul 23, 2015 at 03:41:35PM +0530, Nikhil Badola wrote:
Add adjust_frame_length_quirk for writing to fladj register
which adjusts (micro)frame length to value provided by
snps,configure-fladj property thus avoiding USB 2.0 devices
to time-out over a longer run
Signed-off-by: Nikhil Badola nikhil.bad...@freescale.com
just like the other patch, I won't take this without a glue layer making
use of it.
---
drivers/usb/dwc3/core.c | 12
drivers/usb/dwc3/core.h | 7 +++
2 files changed, 19 insertions(+)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 5c110d8..72ba025 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -779,6 +779,7 @@ static int dwc3_probe(struct platform_device *pdev)
u8 lpm_nyet_threshold;
u8 tx_de_emphasis;
u8 hird_threshold;
+ u32 fladj_value;
int ret;
@@ -886,6 +887,12 @@ static int dwc3_probe(struct platform_device *pdev)
tx_de_emphasis);
of_property_read_string(node, snps,hsphy_interface,
dwc-hsphy_interface);
+ ret = of_property_read_u32(node, snps,configure-fladj,
This is not the correct name for the property, it should be something
like 'snps,quirk-frame-length-adjustment'. Quirk because this is only
needed when the 30MHz sideband signal is invalid for some reason.
This is also something that's only needed for host side operation, so
consider what would you do if you weren't using dwc3, if all you had was
XHCI.
I hope freescale will fix this silicon bug.
One extra thing: everything that can be done via DT, should be available
for pdata users as well.
+fladj_value);
+ if (!ret)
+ dwc-adjust_frame_length_quirk = 1;
+ else
+ dwc-adjust_frame_length_quirk = 0;
this flag is unnecessary. Just initialize fladj_value to 0 and check for
that:
} else if (pdata) {
dwc-maximum_speed = pdata-maximum_speed;
dwc-has_lpm_erratum = pdata-has_lpm_erratum;
@@ -957,6 +964,11 @@ static int dwc3_probe(struct platform_device *pdev)
goto err1;
}
+ /* Adjust Frame Length */
+ if (dwc-adjust_frame_length_quirk)
if (fladj) {
u32 reg;
u32 dft;
reg = dwc3_readl(dwc-regs, DWC3_GFLADJ);
dft = reg 0x3f; /* needs a mask macro */
if (!dev_WARN_ONCE(dwc-dev, dft == fladj,
request value same as default, ignoring\n)) {
reg = ~0x3f; /* needs a mask macro */
reg |= DWC3_GFLADJ_30MHZ_SDBND_SEL |
DWC3_GFLADJ_30MHZ(fladj_value);
dwc3_writel(dwc-regs, DWC3_GFLADJ, reg);
}
}
+ dwc3_writel(dwc-regs, DWC3_GFLADJ, GFLADJ_30MHZ_REG_SEL |
+ GFLADJ_30MHZ(fladj_value));
+
if (IS_ENABLED(CONFIG_USB_DWC3_HOST))
dwc-dr_mode = USB_DR_MODE_HOST;
else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET))
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 0447788..b7a5119 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -124,6 +124,7 @@
#define DWC3_GEVNTCOUNT(n) (0xc40c + (n * 0x10))
#define DWC3_GHWPARAMS8 0xc600
+#define DWC3_GFLADJ 0xc630
/* Device Registers */
#define DWC3_DCFG0xc700
@@ -234,6 +235,10 @@
/* Global HWPARAMS6 Register */
#define DWC3_GHWPARAMS6_EN_FPGA (1 7)
+/* Global Frame Length Adjustment Register */
+#define GFLADJ_30MHZ_REG_SEL (1 7)
always prepend with DWC3_ like *all* other macros in this file.
Also, match docs to ease grepping. This should be called
DWC3_GFLADJ_30MHZ_SDBND_SEL
+#define GFLADJ_30MHZ(n) ((n) 0x3f)
+
/* Device Configuration Register */
#define DWC3_DCFG_DEVADDR(addr) ((addr) 3)
#define DWC3_DCFG_DEVADDR_MASK DWC3_DCFG_DEVADDR(0x7f)
@@ -712,6 +717,7 @@ struct dwc3_scratchpad_array {
* @rx_detect_poll_quirk: set if we enable rx_detect to polling lfps quirk
* @dis_u3_susphy_quirk: set if we disable usb3 suspend phy
* @dis_u2_susphy_quirk: set if we disable usb2 suspend phy
+ * @adjust_frame_length_quirk: enables post-silicon frame length adjustment
* @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk
* @tx_de_emphasis: Tx de-emphasis value
* 0 - -6dB de-emphasis
@@ -841,6 +847,7 @@ struct dwc3 {
unsignedrx_detect_poll_quirk:1;
unsigneddis_u3_susphy_quirk:1;
unsigneddis_u2_susphy_quirk:1;
+ unsigned
Re: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk
Hi again,
On Thu, Jul 23, 2015 at 09:55:32AM -0500, Felipe Balbi wrote:
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 0447788..b7a5119 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -124,6 +124,7 @@
#define DWC3_GEVNTCOUNT(n) (0xc40c + (n * 0x10))
#define DWC3_GHWPARAMS80xc600
+#define DWC3_GFLADJ0xc630
/* Device Registers */
#define DWC3_DCFG 0xc700
@@ -234,6 +235,10 @@
/* Global HWPARAMS6 Register */
#define DWC3_GHWPARAMS6_EN_FPGA(1 7)
+/* Global Frame Length Adjustment Register */
+#define GFLADJ_30MHZ_REG_SEL (1 7)
always prepend with DWC3_ like *all* other macros in this file.
Also, match docs to ease grepping. This should be called
DWC3_GFLADJ_30MHZ_SDBND_SEL
yet another problem is that this register doesn't exist in *all*
versions of DWC3. It was introduced in version 2.50a so the branch I
typed above needs one extra check, and since it's getting so large, it
deserves be factored out into its own function.
static int dwc3_frame_length_adjustment(struct dwc3 *dwc, u32 fladj)
{
u32 reg;
u32 dft;
if (dwc-revision = DWC3_REVISION_250A)
return 0;
if (fladj == 0)
return 0;
reg = dwc3_readl(dwc-regs, DWC3_GFLADJ);
dft = reg 0x3f; /* needs a mask macro */
if (!dev_WARN_ONCE(dwc-dev, dft == fladj,
requested value same as default, ignoring\n)) {
reg = ~0x3f; /* needs a mask macro */
reg |= DWC3_GFLADJ_30MHZ_SDBND_SEL |
DWC3_GFLADJ_30MHZ(fladj_value);
dwc3_writel(dwc-regs, DWC3_GFLADJ, reg);
}
}
you really *MUST* check this sort of this out when writing patches. It's
not only about *your* SoC. You gotta remember we have a ton of different
users and those a prone to major grumpyness should a completely
unrelated patch break their use case.
You have access to the IP's documentation, and that contains the entire
history of the IP itself, so it's easy to figure all of this out with a
simple search in the documentation.
One extra detail is that you were very careless when writing to the
GFLADJ register too. You simply wrote your 30MHz sideband value,
potentially clearing other bits which shouldn't be touched. That alone
can add regressions.
When resending, make sure all 3 patches reach linux-usb. I still can't
find patch 3/3.
--
balbi
signature.asc
Description: Digital signature
[PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk
Add adjust_frame_length_quirk for writing to fladj register
which adjusts (micro)frame length to value provided by
snps,configure-fladj property thus avoiding USB 2.0 devices
to time-out over a longer run
Signed-off-by: Nikhil Badola nikhil.bad...@freescale.com
---
drivers/usb/dwc3/core.c | 12
drivers/usb/dwc3/core.h | 7 +++
2 files changed, 19 insertions(+)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 5c110d8..72ba025 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -779,6 +779,7 @@ static int dwc3_probe(struct platform_device *pdev)
u8 lpm_nyet_threshold;
u8 tx_de_emphasis;
u8 hird_threshold;
+ u32 fladj_value;
int ret;
@@ -886,6 +887,12 @@ static int dwc3_probe(struct platform_device *pdev)
tx_de_emphasis);
of_property_read_string(node, snps,hsphy_interface,
dwc-hsphy_interface);
+ ret = of_property_read_u32(node, snps,configure-fladj,
+ fladj_value);
+ if (!ret)
+ dwc-adjust_frame_length_quirk = 1;
+ else
+ dwc-adjust_frame_length_quirk = 0;
} else if (pdata) {
dwc-maximum_speed = pdata-maximum_speed;
dwc-has_lpm_erratum = pdata-has_lpm_erratum;
@@ -957,6 +964,11 @@ static int dwc3_probe(struct platform_device *pdev)
goto err1;
}
+ /* Adjust Frame Length */
+ if (dwc-adjust_frame_length_quirk)
+ dwc3_writel(dwc-regs, DWC3_GFLADJ, GFLADJ_30MHZ_REG_SEL |
+ GFLADJ_30MHZ(fladj_value));
+
if (IS_ENABLED(CONFIG_USB_DWC3_HOST))
dwc-dr_mode = USB_DR_MODE_HOST;
else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET))
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 0447788..b7a5119 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -124,6 +124,7 @@
#define DWC3_GEVNTCOUNT(n) (0xc40c + (n * 0x10))
#define DWC3_GHWPARAMS80xc600
+#define DWC3_GFLADJ0xc630
/* Device Registers */
#define DWC3_DCFG 0xc700
@@ -234,6 +235,10 @@
/* Global HWPARAMS6 Register */
#define DWC3_GHWPARAMS6_EN_FPGA(1 7)
+/* Global Frame Length Adjustment Register */
+#define GFLADJ_30MHZ_REG_SEL (1 7)
+#define GFLADJ_30MHZ(n)((n) 0x3f)
+
/* Device Configuration Register */
#define DWC3_DCFG_DEVADDR(addr)((addr) 3)
#define DWC3_DCFG_DEVADDR_MASK DWC3_DCFG_DEVADDR(0x7f)
@@ -712,6 +717,7 @@ struct dwc3_scratchpad_array {
* @rx_detect_poll_quirk: set if we enable rx_detect to polling lfps quirk
* @dis_u3_susphy_quirk: set if we disable usb3 suspend phy
* @dis_u2_susphy_quirk: set if we disable usb2 suspend phy
+ * @adjust_frame_length_quirk: enables post-silicon frame length adjustment
* @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk
* @tx_de_emphasis: Tx de-emphasis value
* 0 - -6dB de-emphasis
@@ -841,6 +847,7 @@ struct dwc3 {
unsignedrx_detect_poll_quirk:1;
unsigneddis_u3_susphy_quirk:1;
unsigneddis_u2_susphy_quirk:1;
+ unsignedadjust_frame_length_quirk:1;
unsignedtx_de_emphasis_quirk:1;
unsignedtx_de_emphasis:2;
--
2.1.0
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
