Re: [PATCH 3/9] watchdog: s3c2410_wdt: update to use new exynos_pmu_*() apis

[Saravana Kannan]

On Thu, Jan 25, 2024 at 3:46 AM Lee Jones <lee@xxxxxxxxxx> wrote:
>
> On Thu, 25 Jan 2024, Krzysztof Kozlowski wrote:
>
> > On 24/01/2024 22:27, Saravana Kannan wrote:
> > > On Tue, Jan 23, 2024 at 10:27 PM Krzysztof Kozlowski
> > > <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> > >>
> > >> On 24/01/2024 04:37, Saravana Kannan wrote:
> > >>> On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski
> > >>> <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> > >>>>
> > >>>> On 23/01/2024 18:30, Peter Griffin wrote:
> > >>>>>>>               dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
> > >>>>>>>       else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> > >>>>>>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> > >>>>>>>       if (ret)
> > >>>>>>>               return ret;
> > >>>>>>>
> > >>>>>>> -     if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> > >>>>>>> -             wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> > >>>>>>> -                                             "samsung,syscon-phandle");
> > >>>>>>> -             if (IS_ERR(wdt->pmureg))
> > >>>>>>> -                     return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
> > >>>>>>> -                                          "syscon regmap lookup failed.\n");
> > >>>>>>
> > >>>>>>
> > >>>>>> Continuing topic from the binding: I don't see how you handle probe
> > >>>>>> deferral, suspend ordering.
> > >>>>>
> > >>>>> The current implementation is simply relying on exynos-pmu being
> > >>>>> postcore_initcall level.
> > >>>>>
> > >>>>> I was just looking around for any existing Linux APIs that could be a
> > >>>>> more robust solution. It looks like
> > >>>>>
> > >>>>> of_parse_phandle()
> > >>>>> and
> > >>>>> of_find_device_by_node();
> > >>>>>
> > >>>>> Are often used to solve this type of probe deferral issue between
> > >>>>> devices. Is that what you would recommend using? Or is there something
> > >>>>> even better?
> > >>>>
> > >>>> I think you should keep the phandle and then set device link based on
> > >>>> of_find_device_by_node(). This would actually improve the code, because
> > >>>> syscon_regmap_lookup_by_phandle() does not create device links.
> > >>>
> > >>> I kinda agree with this. Just because we no longer use a syscon API to
> > >>> find the PMU register address doesn't mean the WDT doesn't depend on
> > >>> the PMU.
> > >>>
> > >>> However, I think we should move to a generic "syscon" property. Then I
> > >>> can add support for "syscon" property to fw_devlink and then things
> > >>> will just work in terms of probe ordering, suspend/resume and also
> > >>> showing the dependency in DT even if you don't use the syscon APIs.
> > >>>
> > >>> Side note 1:
> > >>>
> > >>> I think we really should officially document a generic syscon DT
> > >>> property similar to how we have a generic "clocks" or "dmas" property.
> > >>> Then we can have a syscon_get_regmap() that's like so:
> > >>>
> > >>> struct regmap *syscon_get_regmap(struct device *dev)
> > >>> {
> > >>>         return syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
> > >>> }
> > >>>
> > >>> Instead of every device defining its own bespoke DT property to do the
> > >>> exact same thing. I did a quick "back of the envelope" grep on this
> > >>> and I get about 143 unique properties just to get the syscon regmap.
> > >>> $ git grep -A1 syscon_regmap_lookup_by_phandle | grep '"' | sed -e
> > >>> 's/^[^"]*//' -e 's/"[^"]*$/"/' | sort | uniq | wc -l
> > >>> 143
> > >>
> > >> Sorry, generic "syscon" property won't fly with DT maintainers, because
> > >> there is no such thing as syscon in any of hardware.
> > >
> > > Then why do we allow a "syscon" compatible string and nodes? If the
> >
> > To bind Linux drivers.
> >
> > > "syscon" property isn't clear enough, we can make it something like
> > > gpios and have it be <whatever>-syscon or have syscon-names property
> > > if you want to give it a name.
> >
> > This could work.
>
> I'm not opposed to this idea.  The issue you'll have is keeping the
> kernel backwards compatible with older DTBs, thus this solution may only
> be possible for newly created bindings.  More than happy to be proven
> wrong here though.

You are right about backwards compatibility. Technically, we might be
able to fix up the DT at runtime (by keeping a list of those 143
property names) to maintain backward compatibility, but I'm not
suggesting that.

We can leave the existing ones as is, but we can at least use the new
property going forward to make dependencies easier to track and handle

-Saravana

>
> > >>> How are we making sure that it's the exynos-pmu driver that ends up
> > >>> probing the PMU and not the generic syscon driver? Both of these are
> > >>> platform drivers. And the exynos PMU device lists both the exynos
> > >>> compatible string and the syscon property. Is it purely a link order
> > >>> coincidence?
> > >>
> > >> initcall ordering
> > >
> > > Both these drivers usr postcore_initcall(). So it's purely because
> > > soc/ is listed earlier in drivers/Makefile than mfd/. And as soon as
> >
> > Oh... great :/.
>
> Agree.
>
> Even using initcalls for ordering is fragile.  Relying on the
> lexicographical order of a directory / filename structure is akin to
> rolling a dice.  It would be far nicer if you are able to find a more
> robust method of ensuring load order e.g. dynamically poking at
> hardware and / or utilising -EPROBE_DEFER.

Let me dig in to see if all the existing examples of listing syscon in
compatible AND have a different driver that needs to probe it always
list syscon as a secondary compatible string. In that case, we might
be able to make the syscon driver only match with the device it it's
the first entry in the compatible string.

-Saravana