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

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

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.  
> > 
> > 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).

What do people think about this?
Samuel, does this affect the D1 LDO driver as well?

Cheers,
Andre

> 
> > 
> > 
> > And btw: it would have been about the same effort (and more helpful!) to
> > type:
> > 
> > "This cannot be generic, please check writing-bindings.rst."    ;-)
> >   
> >>  
> >>> +	if (!syscon_node)
> >>> +		return ERR_PTR(-ENODEV);
> >>> +
> >>> +	syscon_pdev = of_find_device_by_node(syscon_node);
> >>> +	if (!syscon_pdev) {
> >>> +		/* platform device might not be probed yet */
> >>> +		regmap = ERR_PTR(-EPROBE_DEFER);
> >>> +		goto out_put_node;
> >>> +	}
> >>> +
> >>> +	/* If no regmap is found then the other device driver is at fault */
> >>> +	regmap = dev_get_regmap(&syscon_pdev->dev, NULL);
> >>> +	if (!regmap)
> >>> +		regmap = ERR_PTR(-EINVAL);    
> >>
> >> Aren't you open-coding existing API to get regmap from syscon?  
> > 
> > That's a good point, I lifted that code from sun8i-emac.c, where we have
> > the exact same problem. 
> > Unfortunately syscon_regmap_lookup_by_phandle() requires the syscon DT
> > node to have "syscon" in its compatible string list, which we
> > don't have. We actually explicitly dropped this for the A64 (with
> > 1f1f5183981d70bf0950), and never added this for later SoCs in the first place.
> > I guess we could add it back, and it would work for this case here (tested
> > that), but then cannot replace the sun8i-emac.c code, because that would
> > break older DTs.
> > So is there any chance we can drop the requirement for "syscon" in the
> > compatible string list, in the implementation of
> > syscon_regmap_lookup_by_phandle()? Maybe optionally, using a different
> > prototype? Or is there another existing API that does this already?  
> 
> I must correct myself: I was wrong. You are not open-coding, because as
> pointed out, this is not a phandle to syscon (even if you call it like
> "syscon").
> 
> The code is fine, maybe except missing links (needs double checking,
> because maybe regmap creates links?). The DT binding and DTS needs
> fixing, because it is not a syscon.
> 
> Best regards,
> Krzysztof
>