[PATCH v3 4/6] thermal: sun8i: add syscon register access code

[Rob Herring]

On Tue, Nov 28, 2023 at 10:10 AM Andre Przywara <andre.przywara at arm.com> wrote:
>
> On Tue, 28 Nov 2023 15:48:18 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski at linaro.org> 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 at linaro.org> 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.

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.

Rob