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

[Simon Glass]

Hi Marek,

On Sat, 21 Mar 2020 at 13:01, Marek Vasut <marex at denx.de> wrote:
>
> On 3/21/20 7:41 PM, Simon Glass wrote:
> > Hi Marek,
>
> Hi,
>
> > On Sat, 21 Mar 2020 at 11:00, Marek Vasut <marex at denx.de> wrote:
> >>
> >> On 3/21/20 5:42 PM, Simon Glass wrote:
> >>> Hi Marek,
> >>
> >> Hi,
> >>
> >>> On Sat, 21 Mar 2020 at 09:09, Marek Vasut <marex at denx.de> wrote:
> >>>>
> >>>> On 3/21/20 3:12 PM, Simon Glass wrote:
> >>>>> Hi Marek,
> >>>>>
> >>>>> On Wed, 11 Mar 2020 at 01:11, Marek Vasut <marex at denx.de> wrote:
> >>>>>>
> >>>>>> On 3/11/20 7:50 AM, Chunfeng Yun wrote:
> >>>>>> [...]
> >>>>>>> + * @u3_ctrl_p[x]: ip usb3 port x control register, only low 4bytes are used
> >>>>>>> + * @u2_ctrl_p[x]: ip usb2 port x control register, only low 4bytes are used
> >>>>>>> + * @u2_phy_pll: usb2 phy pll control register
> >>>>>>> + */
> >>>>>>> +struct mtk_ippc_regs {
> >>>>>>> +     __le32 ip_pw_ctr0;
> >>>>>>> +     __le32 ip_pw_ctr1;
> >>>>>>> +     __le32 ip_pw_ctr2;
> >>>>>>
> >>>>>> Please define the registers with #define macros , this struct-based
> >>>>>> approach doesn't scale.
> >>>>>>
> >>>>>
> >>>>> What does this mean? I much prefer the struct approach, unless the
> >>>>> registers move around in future generations.
> >>>>
> >>>> This means I want to see #define MTK_XHCI_<register name> 0xf00
> >>>>
> >>>> The struct based approach doesn't scale and on e.g. altera is causing us
> >>>> a massive amount of problems now. It looked like a good idea back then,
> >>>> but now it's a massive burden, so I don't want that to proliferate. And
> >>>> here I would expect the registers to move around, just like everywhere else.
> >>>
> >>> That sounds like something that is easily avoided. For example,
> >>> Designware doesn't seem to move things around.
> >>>
> >>> Perhaps MediaTek has a policy of not doing this either?
> >>
> >> Such policies change as the hardware evolves. I got burned by this
> >> struct-based approach more often then it was beneficial, if it ever
> >> really was beneficial, hence I don't want to see it used.
> >
> > Some benefits:
> > - using C struct instead of 'assembler-like' addition is less error-prone
>
> In my experience, this is exactly the opposite. If I have a long
> structure describing some register set and I have to count lines to
> figure out which register it is at offset e.g. 0x124 , then I am very
> likely to make a mistake. However, if I have a macro which looks like
> #define REG_FOO 0x124 , I see it right away.

See check_member() though. How do you know it is as offset 0x124?

>
> And I am not even talking about modifying such structures, where one has
> to over and over recount and verify that nothing got padded the wrong
> way. Worse yet, that the structure might just be embedded in some other
> super-structure, in which case the superstructure gets padded
> differently if the sub-structure changes in size.

You always have the choice. You are over-dramatising the difference by
coming up with cases where 'struct' doesn't work. I'm not suggesting
struct should be used always, but to me it has clear benefits in many
drivers in terms of readability and maintainability. We have both
written and maintained a lot of drivers. Also see
ich_init_controller() for a hybrid approach.

>
> Let alone the problematic fact that you might have multiple versions of
> the same IP in the system, with just slightly different register
> layouts, at which point the structures become ridden with unions {} and
> it becomes a total hell.

I don't think structs should be used in that situation though, unless
it is just for a few fields.

>
> So no, this argument I am not buying, sorry.
>
> Also, how is C preprocessor macro assembler like ?

Well in assembler we used to have [BASE + REG_TX], [BASE + REG_RX].
That is what structs are for, so not using them is ignoring a useful
feature of the C language.

>
> > - you can pass the regs struct around between functions in the driver,
> > particularly helpful in large drivers
>
> You can pass base addresses around too.

Untyped though. Then you need casts and defeat the type system, etc.
For example in one driver there are two different register sets and it
is nice to pass just the pointer you need without any ambiguity.

>
> > - lower-case letters are reasier to read
>
> That's a matter of taste.
>
> I find it much easier to identify register offsets (and other macros)
> among functions if the offsets are written in uppercase while functions
> in lowercase.

OK

>
> > - you can specify the data size and endianness in the struct using C types
>
> Yes, you can, which is not a benefit but another problem, esp. if that
> structure gets used on systems where CPU and bus endianness can differ.
> (take a look e.g. at the macro monstrosity that is NS16550)

This is a good example of a driver which should be split out, rather
than cramming all the the little tweaks into ifdefs, etc. Also see
serial_out_dynamic() which heads in that direction. Anyway ns16550 has
to deal with registers being of different sizes. I agree it is a bad
candidate for struct, unless the driver is split.

>
> > If the hardware changes enough, then you need a new driver anyway, or
> > perhaps separate C implementations of the low-level register access if
> > the high-level flow is similar.
>
> Not really, there is a lot of hardware where registers just move around,
> but existing driver can very well handle all such instances.

In my experience things seldom move around and never within the same
SoC generation. In other words there are loads of drivers where it
makes sense to use struct.

>
> > In general you can't always determine
> > this sort of thing at compile time since the version of the hardware
> > may not be apparent until run-time. You end up with variables holding
> > the offset of each register.
>
> You can use some/any sort of register abstraction to do the remapping.
> This is completely independent of how you represent the register offsets.

Yes my point is that you go too far with trying to use the same drive
for different hardware.

>
> > Sometimes is it better to have an umbrella driver with a common core.
> >
> > So I would push back against a move in this direction in general. It
> > depends on the circumstances, and anyway, converting from struct to
> > offsets if desirable later is not that hard.
>
> NAK, I am opposed to the struct based access, sorry.

NAK to what exactly? If you are NAKing using struct when it has the
problems you mention above, I agree with you. If you are NAKing struct
when it has none of these problems, I disagree.

>
> We had massive problems moving toward it in multiple areas (socfpga was
> hit very bad and I regret it to this day), only to move back from it
> these days because it is borderline impossible to accommodate newly
> emerging hardware and we suffered problems outlined above often.

I'm sorry about your socfpga problems. But then, just use offsets, right?

>
> If you want one more argument, then Linux is not using this anywhere and
> it has much larger driver base. One might expect Linux to be somewhat
> ahead of U-Boot in adopting such techniques.

Yes I remember when I first moved from Linux to U-Boot I saw the
struct approach and was very impressed. Has there been any discussion
of moving Linux in that direction? When there is little to no
variability in the register offsets, it is far superior in the right
circumstances I think.

Regards,
Simon