All of lore.kernel.org
 help / color / mirror / Atom feed
From: Robin Murphy <robin.murphy@arm.com>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Vladimir Zapolskiy <vz@mleia.com>,
	Kuldeep Singh <singh.kuldeep87k@gmail.com>,
	Olof Johansson <olof@lixom.net>, SoC Team <soc@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	DTML <devicetree@vger.kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 3/3] ARM: dts: lpc32xx: Update spi clock properties
Date: Mon, 14 Mar 2022 17:55:59 +0000	[thread overview]
Message-ID: <5107aba4-a03d-a0c9-dc89-0e8472be1bd8@arm.com> (raw)
In-Reply-To: <CAK8P3a1KLHwABtXwu-8Kg-GyKbETz1uRhdCz5edELOGG7JdaMg@mail.gmail.com>

On 2022-03-14 12:32, Arnd Bergmann wrote:
> On Mon, Mar 14, 2022 at 1:20 PM Robin Murphy <robin.murphy@arm.com> wrote:
>> On 2022-03-14 11:50, Vladimir Zapolskiy wrote:
>>> On 3/14/22 1:43 PM, Robin Murphy wrote:
> 
>>>
>>> Thank you, it explains, why "apb_pclk" is required for all PrimeCell
>>> controllers on the SoC. Nevertheless, in commit 93898eb775e5 it was
>>> misidentified with the sspclk clock, the latter one is the only clock
>>> explicitly utilized by the driver in 2015 and till today. Fixes in dts
>>> files should be preceded by a fix in the driver.
>>
>> There's nothing to fix in the driver, though. In fact it can only be
>> working today because the Linux driver isn't very strict and simply
>> assumes that the first clock entry is SSPCLK *without* considering its
>> name (other consumers of the binding might be stricter; I don't know),
>> and because presumably the HCLK happens to be enabled already anyway.
>> Changing the driver behaviour would only stand to cause functional
>> regressions.
> 
> We can change the driver to look for an sspclk by name first, and
> then fall back to the first clk if none is found. This would be backwards
> compatible and allow new dts files to have an arbitrary order, though
> we still need to be careful about changing the existing dts files after
> that, to avoid breaking older kernels.

Yeah, I discounted that since schema is strict about the order of 
entries even when they're named, so we'd have nothing to gain except the 
potential to break compatibility in one direction and annoy users in the 
other. There's little point in having code to look up a clock by name 
when we know that for compatibility reasons it has to be the first 
clock, so it would be functionally identical to the fallback code that 
we also still have to have. All it could offer is additionally printing 
a message about it, and experience says that that achieves little except 
the aforementioned annoyance of users - I look as far as my Rockchip 
boards that have all been screaming an error at me every boot for nearly 
4 years now, where the ethernet driver fails to get a clock that only 
exists on some SoCs *other* than the ones I have :(

Robin.

WARNING: multiple messages have this Message-ID (diff)
From: Robin Murphy <robin.murphy@arm.com>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Vladimir Zapolskiy <vz@mleia.com>,
	Kuldeep Singh <singh.kuldeep87k@gmail.com>,
	Olof Johansson <olof@lixom.net>, SoC Team <soc@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	DTML <devicetree@vger.kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 3/3] ARM: dts: lpc32xx: Update spi clock properties
Date: Mon, 14 Mar 2022 17:55:59 +0000	[thread overview]
Message-ID: <5107aba4-a03d-a0c9-dc89-0e8472be1bd8@arm.com> (raw)
In-Reply-To: <CAK8P3a1KLHwABtXwu-8Kg-GyKbETz1uRhdCz5edELOGG7JdaMg@mail.gmail.com>

On 2022-03-14 12:32, Arnd Bergmann wrote:
> On Mon, Mar 14, 2022 at 1:20 PM Robin Murphy <robin.murphy@arm.com> wrote:
>> On 2022-03-14 11:50, Vladimir Zapolskiy wrote:
>>> On 3/14/22 1:43 PM, Robin Murphy wrote:
> 
>>>
>>> Thank you, it explains, why "apb_pclk" is required for all PrimeCell
>>> controllers on the SoC. Nevertheless, in commit 93898eb775e5 it was
>>> misidentified with the sspclk clock, the latter one is the only clock
>>> explicitly utilized by the driver in 2015 and till today. Fixes in dts
>>> files should be preceded by a fix in the driver.
>>
>> There's nothing to fix in the driver, though. In fact it can only be
>> working today because the Linux driver isn't very strict and simply
>> assumes that the first clock entry is SSPCLK *without* considering its
>> name (other consumers of the binding might be stricter; I don't know),
>> and because presumably the HCLK happens to be enabled already anyway.
>> Changing the driver behaviour would only stand to cause functional
>> regressions.
> 
> We can change the driver to look for an sspclk by name first, and
> then fall back to the first clk if none is found. This would be backwards
> compatible and allow new dts files to have an arbitrary order, though
> we still need to be careful about changing the existing dts files after
> that, to avoid breaking older kernels.

Yeah, I discounted that since schema is strict about the order of 
entries even when they're named, so we'd have nothing to gain except the 
potential to break compatibility in one direction and annoy users in the 
other. There's little point in having code to look up a clock by name 
when we know that for compatibility reasons it has to be the first 
clock, so it would be functionally identical to the fallback code that 
we also still have to have. All it could offer is additionally printing 
a message about it, and experience says that that achieves little except 
the aforementioned annoyance of users - I look as far as my Rockchip 
boards that have all been screaming an error at me every boot for nearly 
4 years now, where the ethernet driver fails to get a clock that only 
exists on some SoCs *other* than the ones I have :(

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2022-03-14 17:56 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-11  9:37 [PATCH v2 0/3] PL022 DT fixes for seattle, lg and lpc platform Kuldeep Singh
2022-03-11  9:37 ` Kuldeep Singh
2022-03-11  9:37 ` [PATCH v2 1/3] arm64: dts: seattle: Update spi clock properties Kuldeep Singh
2022-03-11  9:37   ` Kuldeep Singh
2022-03-11  9:37 ` [PATCH v2 2/3] arm64: dts: lg131x: " Kuldeep Singh
2022-03-11  9:37   ` Kuldeep Singh
2022-03-11  9:38 ` [PATCH v2 3/3] ARM: dts: lpc32xx: " Kuldeep Singh
2022-03-11  9:38   ` Kuldeep Singh
2022-03-11 13:20   ` Vladimir Zapolskiy
2022-03-11 13:20     ` Vladimir Zapolskiy
2022-03-11 13:38     ` Arnd Bergmann
2022-03-11 13:38       ` Arnd Bergmann
2022-03-11 14:07       ` Vladimir Zapolskiy
2022-03-11 14:07         ` Vladimir Zapolskiy
2022-03-11 14:33         ` Krzysztof Kozlowski
2022-03-11 14:33           ` Krzysztof Kozlowski
2022-03-11 21:26           ` Vladimir Zapolskiy
2022-03-11 21:26             ` Vladimir Zapolskiy
2022-03-12 10:23             ` Krzysztof Kozlowski
2022-03-12 10:23               ` Krzysztof Kozlowski
2022-03-12 12:26               ` Vladimir Zapolskiy
2022-03-12 12:26                 ` Vladimir Zapolskiy
2022-03-14 11:43         ` Robin Murphy
2022-03-14 11:43           ` Robin Murphy
2022-03-14 11:50           ` Vladimir Zapolskiy
2022-03-14 11:50             ` Vladimir Zapolskiy
2022-03-14 12:20             ` Robin Murphy
2022-03-14 12:20               ` Robin Murphy
2022-03-14 12:32               ` Arnd Bergmann
2022-03-14 12:32                 ` Arnd Bergmann
2022-03-14 17:55                 ` Robin Murphy [this message]
2022-03-14 17:55                   ` Robin Murphy
2022-03-14 12:34               ` Vladimir Zapolskiy
2022-03-14 12:34                 ` Vladimir Zapolskiy
2022-03-27  2:57                 ` Trevor Woerner
2022-03-27 16:27                   ` Vladimir Zapolskiy
2022-03-27 16:27                     ` Vladimir Zapolskiy
2022-03-27 18:32                     ` Trevor Woerner
2022-03-27 18:32                       ` Trevor Woerner
2022-03-11 10:50 ` [PATCH v2 0/3] PL022 DT fixes for seattle, lg and lpc platform patchwork-bot+linux-soc

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=5107aba4-a03d-a0c9-dc89-0e8472be1bd8@arm.com \
    --to=robin.murphy@arm.com \
    --cc=arnd@arndb.de \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=olof@lixom.net \
    --cc=robh+dt@kernel.org \
    --cc=singh.kuldeep87k@gmail.com \
    --cc=soc@kernel.org \
    --cc=vz@mleia.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.