From: Saravana Kannan <saravanak@google.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: Peter Griffin <peter.griffin@linaro.org>,
arnd@arndb.de, robh+dt@kernel.org,
krzysztof.kozlowski+dt@linaro.org, linux@roeck-us.net,
wim@linux-watchdog.org, conor+dt@kernel.org,
alim.akhtar@samsung.com, jaewon02.kim@samsung.com,
chanho61.park@samsung.com, semen.protsenko@linaro.org,
kernel-team@android.com, tudor.ambarus@linaro.org,
andre.draszik@linaro.org, willmcvicker@google.com,
linux-fsd@tesla.com, linux-watchdog@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH 3/9] watchdog: s3c2410_wdt: update to use new exynos_pmu_*() apis
Date: Thu, 25 Jan 2024 18:17:29 -0800 [thread overview]
Message-ID: <CAGETcx-LtFAWvs6+=1BBP8LVJuBVJxs9+qHSzhdk=Uip_rRAEg@mail.gmail.com> (raw)
In-Reply-To: <04411aaf-6f2c-4f43-83b4-aa0741ccd25f@linaro.org>
On Wed, Jan 24, 2024 at 11:37 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 24/01/2024 22:27, Saravana Kannan wrote:
> > On Tue, Jan 23, 2024 at 10:27 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 24/01/2024 04:37, Saravana Kannan wrote:
> >>> On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski
> >>> <krzysztof.kozlowski@linaro.org> 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.
My point was that if your statement "there's no such thing as syscon
in any of hardware" is true, then we shouldn't have a syscon node
either because I don't think there's any "syscon" IP block. It's just
a random set of registers grouped together for system control.
>
> > "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.
>
> > 143 bespoke properties all to say "here are some registers I need to
> > twiddle that's outside my regmap" doesn't seem great.
>
> Why? 143 of these registers are not the same.
You could make the same argument about clocks. None of the "clocks"
listed in DT is the same clock across all devices. But the idea behind
the clocks property is that in DT we are representing that the device
needs clocks and in DT we don't really care how/what it is used for
(the driver deals with that). Similarly "syscon" is a system control
register needed by a device (the why/how is left to the driver).
-Saravana
> >
> >>>
> >>> Side note 2:
> >>>
> >>> 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 :/.
>
> > drivers are made into modules this is going to break. This is
> > terrible. If you want to have a modular system, this is going to throw
> > in a wrench.
>
> Best regards,
> Krzysztof
>
next prev parent reply other threads:[~2024-01-26 2:18 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-22 22:57 [PATCH 0/9] Add exynos_pmu_update/read/write() APIs to exynos-pmu Peter Griffin
2024-01-22 22:57 ` [PATCH 1/9] dt-bindings: watchdog: samsung-wdt: deprecate samsung,syscon-phandle Peter Griffin
2024-01-23 11:09 ` Krzysztof Kozlowski
2024-01-22 22:57 ` [PATCH 2/9] soc: samsung: exynos-pmu: Add exynos_pmu_update/read/write APIs and SoC quirks Peter Griffin
2024-01-23 8:10 ` Arnd Bergmann
2024-01-24 10:21 ` Peter Griffin
2024-01-23 11:17 ` Krzysztof Kozlowski
2024-01-24 10:41 ` Peter Griffin
2024-01-23 18:56 ` Sam Protsenko
2024-01-24 10:02 ` Peter Griffin
2024-01-24 20:23 ` Sam Protsenko
2024-01-25 10:48 ` Peter Griffin
2024-01-26 17:26 ` kernel test robot
2024-01-26 23:07 ` kernel test robot
2024-01-22 22:57 ` [PATCH 3/9] watchdog: s3c2410_wdt: update to use new exynos_pmu_*() apis Peter Griffin
2024-01-23 10:33 ` Guenter Roeck
2024-01-23 15:35 ` Peter Griffin
2024-01-23 11:19 ` Krzysztof Kozlowski
2024-01-23 17:30 ` Peter Griffin
2024-01-23 18:12 ` Krzysztof Kozlowski
2024-01-24 3:37 ` Saravana Kannan
2024-01-24 6:27 ` Krzysztof Kozlowski
2024-01-24 21:27 ` Saravana Kannan
2024-01-25 7:37 ` Krzysztof Kozlowski
2024-01-25 11:46 ` Lee Jones
2024-01-26 2:23 ` Saravana Kannan
2024-01-26 8:43 ` Lee Jones
2024-01-26 2:17 ` Saravana Kannan [this message]
2024-01-25 13:19 ` Peter Griffin
2024-01-30 20:27 ` Rob Herring
2024-01-26 17:04 ` kernel test robot
2024-01-22 22:57 ` [PATCH 4/9] arm64: dts: fsd: remove deprecated samsung,syscon-phandle Peter Griffin
2024-01-22 22:57 ` [PATCH 5/9] arm64: dts: exynosautov9: " Peter Griffin
2024-01-22 22:57 ` [PATCH 6/9] arm64: dts: exynos850: " Peter Griffin
2024-01-22 22:57 ` [PATCH 7/9] arm64: dts: exynos7: " Peter Griffin
2024-01-22 22:57 ` [PATCH 8/9] ARM: dts: samsung: exynos4: " Peter Griffin
2024-01-22 22:57 ` [PATCH 9/9] ARM: dts: exynos5250: " Peter Griffin
2024-01-23 7:41 ` Arnd Bergmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAGETcx-LtFAWvs6+=1BBP8LVJuBVJxs9+qHSzhdk=Uip_rRAEg@mail.gmail.com' \
--to=saravanak@google.com \
--cc=alim.akhtar@samsung.com \
--cc=andre.draszik@linaro.org \
--cc=arnd@arndb.de \
--cc=chanho61.park@samsung.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=jaewon02.kim@samsung.com \
--cc=kernel-team@android.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=krzysztof.kozlowski@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-fsd@tesla.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=peter.griffin@linaro.org \
--cc=robh+dt@kernel.org \
--cc=semen.protsenko@linaro.org \
--cc=tudor.ambarus@linaro.org \
--cc=willmcvicker@google.com \
--cc=wim@linux-watchdog.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).