[LEDE-DEV] [PATCH 3/3] lantiq: add device tree binding for dwc2 on danube

[Antti Seppälä]

On 30 October 2016 at 14:09, Ben Mulvihill <ben.mulvihill at gmail.com> wrote:
> On Sun, 2016-10-30 at 12:27 +0200, Antti Seppälä wrote:
>> On 29 October 2016 at 18:31, Ben Mulvihill <ben.mulvihill at gmail.com> wrote:
>> > On Sat, 2016-10-29 at 13:28 +0200, Ben Mulvihill wrote:
>> >> On Fri, 2016-10-28 at 23:35 +0200, Ben Mulvihill wrote:
>> >> > On Fri, 2016-10-28 at 19:14 +0300, Antti Seppälä wrote:
>> >> > > On 28 October 2016 at 17:30, Ben Mulvihill <ben.mulvihill at gmail.com> wrote:
>> >> > > > Add device tree binding for dwc2 usb driver on lantiq danube
>> >> > > >
>> >> > > > Signed-off-by: Ben Mulvihill <ben.mulvihill at gmail.com>
>> >> > > > ---
>> >> > > > diff -uprN a/target/linux/lantiq/dts/danube.dtsi b/target/linux/lantiq/dts/danube.dtsi
>> >> > > > --- a/target/linux/lantiq/dts/danube.dtsi       2016-10-27 19:56:07.090392399 +0200
>> >> > > > +++ b/target/linux/lantiq/dts/danube.dtsi       2016-10-27 20:47:34.387511522 +0200
>> >> > > > @@ -140,7 +140,7 @@
>> >> > > >                 };
>> >> > > >
>> >> > > >                 ifxhcd at E101000 {
>> >> > > > -                       compatible = "lantiq,ifxhcd-danube";
>> >> > > > +                       compatible = "lantiq,ifxhcd-danube", "lantiq,ifxhcd-danube-dwc2";
>> >> > > >                         reg = <0xE101000 0x1000
>> >> > > >                                 0xE120000 0x3f000>;
>> >> > > >                         interrupt-parent = <&icu0>;
>> >> > > >
>> >> > > >
>> >> > >
>> >> > > Hi.
>> >> > >
>> >> > > Have you tried if danube can simply be compatible with vanilla "snps,dwc2"?
>> >> > >
>> >> > > The main reason we created our own definition for lantiq is that arx
>> >> > > and xrx have fifo sizes smaller than what the dwc2 autodetection
>> >> > > mechanism expects.
>> >> > > I remember finding some references in ifxhcd code which would suggest
>> >> > > that danube had bigger fifo and thus would maybe work without any
>> >> > > special treatment.
>> >> > >
>> >> > > Br,
>> >> >
>> >> > I'm pretty sure I tried it, but must have been a couple of years
>> >> > ago and I can't remember why it didn't work. I'll have another go.
>> >> >
>> >> > Thanks for the suggestion,
>> >> >
>> >> > Ben
>> >>
>> >> You are right. It works fine with "snps,dwc2". (Apart from the same
>> >> mode mismatch warnings) When I tried that before I didn't have the
>> >> benefit of your hardware initialisation code and my own version
>> >> must have been wrong.
>> >>
>> >> I'll submit a version 2 patch set.
>> >>
>> >> Ben
>> >
>> > No, sorry. I was testing the wrong build. I'm afraid "snps,dwc2" doesn't
>> > work after all. It tries to autodetect the parameters, doesn't log any
>> > errors, but then doesn't recognise devices which are plugged in,
>> > so presumably the parameter values it thinks it has detected are
>> > not correct.
>> >
>> > What do you think would be the best thing to do, stick with my original
>> > patch set using the same parameters as for arx and vrx, which appear
>> > to work fine, or investigate the ifxhcd code and add a separate
>> > dwc2_core_params structure called params_danube with (possibly) bigger
>> > fifos? I'd prefer the former for simplicity's sake.
>> >
>>
>> Could you make another simple test where you temporarily edit the
>> params_ltq structure to have default values (-1) for all other fields
>> than the .otg_cap that is set to 2? The .otg_cap is used to disable
>> OTG power and session negotiation that seems to be required for some
>> boards.
>>
>> The fact that the driver doesn't complain anything about fifo
>> initialization suggests that the autodetection is working and reason
>> for non-working usb traffic could be elsewhere. However it is possible
>> that the autodetection logic is flawed and we need indeed to
>> initialize fifos with hard-coded values in which case values from
>> params_ltq are probably "good enough".
>>
>
> Yes, setting -1 for all parameters apart from .otg_cap does works.

Oh goodie, I guess then a new definition such as params_ltq_danube
should be introduced?  Autodetected bigger fifos lead to better
performance.

> Does that definitely not work on arx and vrx?
>

Unfortunately not. The dwc2 driver cannot cope with total fifo size as
tiny as 512 without giving it values through the device parameters.
See e.g dwc2_calculate_dynamic_fifo from drivers/usb/dwc2/hcd.c

-- 
Antti