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

[Simon Glass]

Hi Marek,

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
- you can pass the regs struct around between functions in the driver,
particularly helpful in large drivers
- lower-case letters are reasier to read
- you can specify the data size and endianness in the struct using C types

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. 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.

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.

Regards,
Simon