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

Marek Vasut marex at denx.de
Sat Mar 21 20:01:49 CET 2020


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.

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.

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.

So no, this argument I am not buying, sorry.

Also, how is C preprocessor macro assembler like ?

> - you can pass the regs struct around between functions in the driver,
> particularly helpful in large drivers

You can pass base addresses around too.

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

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

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

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

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.

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.

-- 
Best regards,
Marek Vasut


More information about the U-Boot mailing list