From: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
To: Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Cc: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>,
Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>,
Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
Linus Walleij
<linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>,
linux-clk <linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
devicetree <devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
linux-arm-kernel
<linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>,
linux-kernel
<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-sunxi <linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org>
Subject: Re: [linux-sunxi] [PATCH 1/7] pinctrl: sunxi: add support for pin controllers without bus gate
Date: Thu, 11 Jan 2018 11:48:13 +0000 [thread overview]
Message-ID: <cc84e3f9-84da-4642-fdf3-bd9679a7f79b@arm.com> (raw)
In-Reply-To: <20180111104100.j5rwitma3wgtdivm-ZC1Zs529Oq4@public.gmane.org>
Hi,
On 11/01/18 10:41, Maxime Ripard wrote:
> On Thu, Jan 11, 2018 at 10:23:52AM +0000, Andre Przywara wrote:
>> Hi,
>>
>> On 11/01/18 10:14, Chen-Yu Tsai wrote:
>>> On Thu, Jan 11, 2018 at 6:08 PM, Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote:
>>>> Hi,
>>>>
>>>> On 06/01/18 04:23, Icenowy Zheng wrote:
>>>>> The Allwinner H6 pin controllers (both the main one and the CPUs one)
>>>>> have no bus gate clocks.
>>>>>
>>>>> Add support for this kind of pin controllers.
>>>>>
>>>>> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
>>>>> ---
>>>>> drivers/pinctrl/sunxi/pinctrl-sunxi.c | 30 ++++++++++++++++++++----------
>>>>> drivers/pinctrl/sunxi/pinctrl-sunxi.h | 1 +
>>>>> 2 files changed, 21 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>>>>> index 4b6cb25bc796..68cd505679d9 100644
>>>>> --- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>>>>> +++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
>>>>> @@ -1182,7 +1182,12 @@ static int sunxi_pinctrl_setup_debounce(struct sunxi_pinctrl *pctl,
>>>>> unsigned int hosc_div, losc_div;
>>>>> struct clk *hosc, *losc;
>>>>> u8 div, src;
>>>>> - int i, ret;
>>>>> + int i, ret, clk_count;
>>>>> +
>>>>> + if (pctl->desc->without_bus_gate)
>>>>> + clk_count = 2;
>>>>> + else
>>>>> + clk_count = 3;
>>>>>
>>>>> /* Deal with old DTs that didn't have the oscillators */
>>>>> if (of_count_phandle_with_args(node, "clocks", "#clock-cells") != 3)
>>>>> @@ -1360,15 +1365,19 @@ int sunxi_pinctrl_init_with_variant(struct platform_device *pdev,
>>>>> goto gpiochip_error;
>>>>> }
>>>>>
>>>>> - clk = devm_clk_get(&pdev->dev, NULL);
>>>>> - if (IS_ERR(clk)) {
>>>>> - ret = PTR_ERR(clk);
>>>>> - goto gpiochip_error;
>>>>> - }
>>>>> + if (!desc->without_bus_gate) {
>>>>
>>>> Do we really need explicit support for that case?
>>>> Can't we have something that works automatically?
>>>>
>>>> if (node has clock-names property) (A)
>>>> use clocks as enumerated and named there
>>>
>>> You still need to know if the hardware has a bus gate or not.
>>> If it's missing, and it's disabled, you end up with unusable
>>> hardware.
>>
>> Yes. So what? If you have a broken DT, it will not work. Just don't do
>> it. I don't understand why we want to defend against this case.
>
> This is not the point, but rather: if we have a way to detect easily
> that the device tree is missing a property that is missing in our
> binding, why shouldn't we do it?
>
> We're already doing it for reg and interrupts for example, why not for
> the clocks?
>
>>> Unless you are fully trusting the device tree to be correct.
>>
>> Sorry, but what else do we trust?
>>
>>> IMHO that makes for hard to find bugs during SoC bringup.
>>
>> I am not sure if that is really an issue. I would expect people
>> doing SoC bringup to be able to cope with those kinds of problems.
>
> Riiiight, because it worked so well in the past. We definitely didn't
> overlooked some clocks used for debouncing in this particular driver,
> or some to get the timekeeping right in the RTC.
I think that's a different issue, because debouncing is an optional
feature. How would those kind of explicit molly guards here have
prevented this omission in the past, when we only discovered that later?
> The argument that "anyone who codes in the kernel should just know
> better" doesn't work, on multiple levels. Because anyone that actually
> knows better can make a mistake or overlook some feature (because you
> didn't have your morning coffee yet, or because it was undocumented)
> and because you just make someone that doesn't feel bad.
I agree to that. But: If something doesn't work, checking clocks and
reset would be my first impulse. And Icenowy did exactly that and
quickly found it.
Plus this only protects against known pitfalls.
> So, yes, we cannot not trust the device tree. But if we have a way to
> detect simple mistakes in the binding, we should also do it.
I totally honour that, I am just wondering what price we pay for that.
This kind of: "We need three clocks here, or wait, two clock in this
particular case" sounds a bit dodgy and little future proof to me.
Which is somewhat confirmed by the fact that we need to adjust this
check now. So I suggest we remove it, as we have more, actual checks
afterwards anyway. That should cover future extensions without further ado:
The clock-names property should cater nicely for those cases, hence my
suggestion to rely on it. Plus we need to support the legacy DTs with
just a single clock and no clock-names. Done.
So I think we should change the devm_get_clk(..., NULL) to
devm_get_clk(..., "apb"), and then check for just a single unnamed clock
if that fails (older DTs), or no clock at all, if we need to support
future SoCs without debouncing.
Looking deeper I actually think we are not binding compliant at the
moment, as we rely on the "apb" clock to be the first one, however
clock-names = "hosc", "losc", "apb" would be perfectly legal as well, as
we don't document a certain order of the clock - which is not necessary
with clock-names.
I can make a patch if we agree on that.
Cheers,
Andre.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2018-01-11 11:48 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-06 4:23 [PATCH 1/7] pinctrl: sunxi: add support for pin controllers without bus gate Icenowy Zheng
2018-01-06 4:23 ` [PATCH 2/7] pinctrl: sunxi: support pin controllers with holes among IRQ banks Icenowy Zheng
[not found] ` <20180106042326.46519-1-icenowy-h8G6r0blFSE@public.gmane.org>
2018-01-06 4:23 ` [PATCH 3/7] pinctrl: sunxi: add support for the Allwinner H6 main pin controller Icenowy Zheng
2018-01-11 20:24 ` Rob Herring
2018-01-11 10:08 ` [linux-sunxi] [PATCH 1/7] pinctrl: sunxi: add support for pin controllers without bus gate Andre Przywara
[not found] ` <2ca1ee96-8dc6-c80b-ae11-45895d6a8484-5wv7dgnIgG8@public.gmane.org>
2018-01-11 10:14 ` Chen-Yu Tsai
[not found] ` <CAGb2v64E-XOHVJG0Q-uZeCAdxF37QSVCDfiYmqtv7ydvseQwrw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2018-01-11 10:23 ` Andre Przywara
[not found] ` <ba83f00d-c1e5-6f6a-84df-04169860c7eb-5wv7dgnIgG8@public.gmane.org>
2018-01-11 10:41 ` Maxime Ripard
[not found] ` <20180111104100.j5rwitma3wgtdivm-ZC1Zs529Oq4@public.gmane.org>
2018-01-11 10:43 ` Icenowy Zheng
2018-01-11 11:48 ` Andre Przywara [this message]
2018-01-11 10:15 ` Icenowy Zheng
[not found] ` <08CB03C2-1253-4072-B8AC-D3F1253C8A85-h8G6r0blFSE@public.gmane.org>
2018-01-11 10:41 ` Andre Przywara
2018-01-11 11:48 ` [linux-sunxi] " Andre Przywara
[not found] ` <ef82983f-6f91-46c2-1f67-7d4efc10c905-5wv7dgnIgG8@public.gmane.org>
2018-01-11 13:21 ` Icenowy Zheng
[not found] ` <3A97A26E-0774-4279-B7B9-8312225E2C72-h8G6r0blFSE@public.gmane.org>
2018-01-12 8:51 ` Maxime Ripard
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=cc84e3f9-84da-4642-fdf3-bd9679a7f79b@arm.com \
--to=andre.przywara-5wv7dgnigg8@public.gmane.org \
--cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=icenowy-h8G6r0blFSE@public.gmane.org \
--cc=linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \
--cc=linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \
--cc=linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org \
--cc=maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org \
--cc=robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
--cc=wens-jdAy2FN1RRM@public.gmane.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 NNTP newsgroup(s).