Hi,
On 28/11/2023 16:50, Rob Herring wrote:
On Tue, Nov 28, 2023 at 10:10 AM Andre Przywara <andre.przywara@xxxxxxx> wrote:
On Tue, 28 Nov 2023 15:48:18 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:
Hi,
(adding Maxime for the syscon question below)
On 28/11/2023 15:33, Andre Przywara wrote:
On Tue, 28 Nov 2023 08:43:32 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:
Hi,
On 28/11/2023 01:58, Andre Przywara wrote:
+static struct regmap *sun8i_ths_get_syscon_regmap(struct device_node *node)
+{
+ struct device_node *syscon_node;
+ struct platform_device *syscon_pdev;
+ struct regmap *regmap = NULL;
+
+ syscon_node = of_parse_phandle(node, "syscon", 0);
Nope. For the 100th time, this cannot be generic.
Unless it is the 100th time for the submitter, please just point to
the documentation.
Can we simply ban "syscon" as a property name? It looks like we have
65 cases in upstream dts files. Maybe that's doable. This is where we
need levels of warnings with okay for existing vs. don't use in new
designs.
OK. Shall this name refer to the required functionality (temperature
offset fix) or to the target syscon node (like allwinner,misc-syscon).
The problem is that this is really a syscon, as in: "random collection of
bits that we didn't know where else to put in", so "syscon" alone actually
says it all.
Every syscon is a "random collection of bits...", but not every "random
collection of bits..." is a syscon.
Your target device does not implement syscon nodes. Your Linux
implementation does not use it as syscon. Therefore if something does
not look like syscon and does not behave like syscon, it is not a syscon.
I looked at the bit and this is SRAM, not syscon. I am sorry, but it is
something entirely different and we have a binding for it: "sram", I think.
Well, it's somehow both: On the face of it it's a SRAM controller, indeed:
it can switch the control of certain SRAM regions between CPU access and
peripheral access (for the video and the display engine). But then it's
also a syscon, because on top of that, it also controls those random bits,
for instance the EMAC clock register, and this ominous THS bit.
I guess in hindsight we should have never dropped that "syscon" string
then, but I am not sure if adding it back has side effects?
And as I mentioned in the cover letter: modelling this as some SRAM
region, as you suggest, might be an alternative, but it doesn't sound right
either, as I don't think it really is one: I just tried in U-Boot, and I
can write and read the whole SRAM C region just fine, with and without the
bit set. And SRAM content is preserved, even with the thermal sensor
running and the bit cleared (or set).
So adding the "syscon" to the compatible would fix most things, but then
we need to keep the open coded lookup code in dwmac-sun8i.c (because older
DTs would break otherwise).
Really, I'd like to get rid of the "syscon" compatible. It is nothing
more than a flag for Linux to create a regmap.
Yeah, so thinking about it indeed feels a bit like we are changing the
DT here to cater for some Linux implementation detail. After all we
already access the regmap successfully in dwmac-sun8i.c, is that
approach frowned upon (because: driver model) and just tolerated because
it's already in the code base?
Not a fully baked idea, but perhaps what is needed is drivers that
request a regmap for a node simply get one regardless. That kind of > throws out the Linux driver model though. Alternatively with no
"syscon" compatible, we'd have to have table(s) of 100s of compatibles
in the kernel.
So do you mean to either just remove the explicit syscon compatible
check in syscon_node_to_regmap(), or replace it with a check against a
list of allowed devices?
Wouldn't it be sufficient to leave that check to the (syscon-like)
devices, by them exporting a regmap in the first place or not? And we
can do filtering of accesses there, like we do in sunxi_sram.c?
Cheers,
Andre
Rob
