Re: [PATCH 2/3] drivers: usb: dwc3: Add adjust_frame_length_quirk

[Felipe Balbi]

Hi,

On Wed, Jul 29, 2015 at 11:19:01AM +0000, 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@3100000 {
>                         compatible = "snps,dwc3";
>                         reg = <0x0 0x3100000 0x0 0x10000>;
>                         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

Attachment: signature.asc
Description: Digital signature