On Wed, Nov 29, 2023 at 12: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.
> > >
> > > 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?
Either way you would need to add locking around the register accesses,
or you could, however unlikely, end up with two simultaneous read-update-write
accesses by both consumers (THS and claiming C1).
If you add the syscon string back, then you'd have to convert the SRAM
controller driver to use syscon as well, as there is no way to provide
a custom spinlock for the syscon regmap. Another reason why a driver
would want to create its own regmap.
> 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).
dwmac-sun8i already falls back to syscon_regmap_lookup_by_phandle() because
of even older DTs. I'm the one that added the open coded stuff, mostly
because the R40 had the bits embedded in the clock controller, not the
system control, and it seemed error prone and hard to debug for some
other device to have full access.
So you'd just be reverting the driver to the old ways.
ChenYu
> 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
> >
>
