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

[Chen-Yu Tsai]

On Thu, Nov 30, 2023 at 1:03 AM Andre Przywara <andre.przywara at arm.com> wrote:
>
> Hi,
>
> On 28/11/2023 16:50, Rob Herring wrote:
> > 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.
>
> 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