[PATCH 5/8] xhci: mediatek: Add support for MTK xHCI host controller

[Simon Glass]

HI Marek,

On Tue, 31 Mar 2020 at 07:29, Marek Vasut <marex at denx.de> wrote:
>
> On 3/31/20 3:24 PM, Simon Glass wrote:
> > Hi Marek,
> >
> > On Mon, 30 Mar 2020 at 18:39, Marek Vasut <marex at denx.de> wrote:
> >>
> >> On 3/31/20 1:31 AM, Simon Glass wrote:
> >>> Hi Marek,
> >>
> >> Hi,
> >>
> >>> On Sun, 22 Mar 2020 at 09:34, Marek Vasut <marex at denx.de> wrote:
> >>>>
> >>>> On 3/22/20 4:17 PM, Simon Glass wrote:
> >>>>> Hi Marek,
> >>>>
> >>>> Hi,
> >>>>
> >>>>> On Sat, 21 Mar 2020 at 20:15, Marek Vasut <marex at denx.de> wrote:
> >>>>>>
> >>>>>> On 3/22/20 3:08 AM, Simon Glass wrote:
> >>>>>>> Hi Marek,
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>>> I think at this point we've covered all the ground and mentioned the
> >>>>>>> pros and cons of each method, so I'll leave the discussion where it
> >>>>>>> is.
> >>>>>>
> >>>>>> Great, so let's remove the struct-based access from the driver and use
> >>>>>> regular #define REGISTER 0xoffset.
> >>>>>
> >>>>> I think any individual decision depends on the pros and cons we
> >>>>> outlined in our discussion. I don't have any information to suggest
> >>>>> that the Mediatek XHCI driver has any of the variations you talked
> >>>>> about in your worst-case scenarios, so I can't comment on that. I am
> >>>>> more concerned about this as a general rule as I feel that the
> >>>>> struct-based approach is generally best for U-Boot, except for the
> >>>>> cases you highlighted:
> >>>>>
> >>>>> - where the registers appear at different offsets in different
> >>>>> hardware revisions served by the same driver
> >>>>> - where the driver only uses a small subset of the registers and it is
> >>>>> not worth defining a struct to cover them all, with associated empty
> >>>>> regions, etc.
> >>>>>
> >>>>> Anything else?
> >>>>
> >>>> It's also very difficult to easily figure out the address of a register
> >>>> that's buried somewhere down in a long structure, possibly with embedded
> >>>> sub-structures.
> >>>
> >>> OK I have updated the coding style page with all of this.
> >>
> >> Which page ?
> >
> > https://www.denx.de/wiki/U-Boot/CodingStyle
>
> " U-Boot typically uses a C structure to map out the registers in an I/O
> region, rather than offsets. The reasons for this are: " is misleading
> and suggests that using structures is the best practice. This should be
> reworded to make it clear both options are equally valid.

I'd like to see a preference to use struct where it makes sense and
not use it when it doesn't, with the different tradeoffs clearly
written. Are asking that we say nothing about which is better in each
situation?

>
> > Separately, I sent a patch a while back which updated checkpatch for
> > U-Boot. I got some pushback
>
> Link ?
>

http://patchwork.ozlabs.org/patch/999510/

> I am very much opposed to this struct-based approach, so there is my
> pushback too. I think we should NOT do it.

Right, but there are arguments on both sides. I strongly prefer it
where it makes sense, for reasons that we've already discussed.

>
> >, but I think that was wrong and we should
> > do it. For example I am saying many of the same things in code reviews
> > and many of them could be caught by the script. Examples include using
> > if() instead of #if where possible.
> >
> > Regards,
> > Simon
> >

Regards,
Simon