On Thu, Nov 30, 2023 at 1:03 AM Andre Przywara <andre.przywara@xxxxxxx> wrote: > > 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? There is already device_node_to_regmap() which skips the check. It still bypasses the driver model though. > 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