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

[Marek Vasut]

On 3/21/20 8:34 PM, Simon Glass wrote:
> Hi Marek,

Hi,

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

This only adds duplication and more complexity. With this, not only do
you have a structure which is difficult and error-prone to work with,
but you also have a macro which defines the offset, but only for the
purpose of preventing errors originally induced by the complexity of
working with the structure.

So this is complexity on top of complexity.

> How do you know it is as offset 0x124?

I see this offset in the datasheet of the IP/SoC/whatever and/or I can
simply git grep for this value in the sources. And since this value is
there verbatim, I see #define REG_FOO 0x124, simple.

With the complexity presented above, this no longer works. I have to
comb through structures (possibly embedded in other structures)
manually, I cannot directly grep for the register offsets, and it's
complicated, cumbersome and massively error prone.

>> 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 am coming up with real world experience I have, which tells me that
this does not work in the long run in vast majority of cases and/or is a
massive pain to work with.

Obviously, I come up with cases where this struct based approach fails
and/or displays it's shortcomings, where else would I demonstrate all
the problems with this ?

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

Sorry, I just don't see the clear benefit.

Originally I thought this struct-based approach was a good idea too,
which is why I was pushing for it a lot back then, but now I think it
was a big mistake.

>> 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 this would imply you would have drivers which have struct based
access, but then suddenly when the hardware complexity increases, you
would convert them to plain macros ? In that case, we can very well just
stick with plain macros right from the beginning.

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

In my experience, this is not helpful at all, it only is in the way and
is cumbersome to deal with.

The base + offset is easy to calculate, which makes it easy and straight
forward to work with and to debug. But trying to find out what the type
of some structure is, and then calculate at what offset in &foo->bar the
BAR register really is, awful and error prone.

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

Or, you have a driver which handles two different version of the same
IP, with different register layouts. Suddenly you have two structures
and massive amount of casts.

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

It's a great driver for using macros though.

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

My experience is the exact opposite.

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

No. If the hardware is basically the same , except registers move
slightly , then it only makes sense to reuse the same driver.

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

I am NAKing the use of structs here, because in my experience this will
come back to bite us later on and because I don't see any benefits from
using them.

We can revisit this discussion a few years down the road and see who was
right if you want.

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

offsets ?

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

Most embedded hardware has this variability it seems.
Feel free to start debating this on the Linux side.

-- 
Best regards,
Marek Vasut