Hi Marek,
On Sat, 21 Mar 2020 at 13:01, Marek Vasut marex@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@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@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@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