Hi Marek,
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 - 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