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

[Andre Przywara <andre.przywara@arm.com>]

Hi,

On 28/11/2023 16:50, Rob Herring wrote:
> On Tue, Nov 28, 2023 at 10:10 AM Andre Przywara <andre.przywara@arm.com> wrote:
>>
>> On Tue, 28 Nov 2023 15:48:18 +0100
>> Krzysztof Kozlowski <krzysztof.kozlowski@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@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?
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