On Tue, Mar 15, 2022 at 2:00 AM Vladimir Zapolskiy wrote: > On 3/14/22 2:20 PM, Robin Murphy wrote: > > On 2022-03-14 11:50, Vladimir Zapolskiy wrote: > >> On 3/14/22 1:43 PM, Robin Murphy wrote: > >>> On 2022-03-11 14:07, Vladimir Zapolskiy wrote: > >>>> On 3/11/22 3:38 PM, Arnd Bergmann wrote: > >>>>> On Fri, Mar 11, 2022 at 2:20 PM Vladimir Zapolskiy > >>>>> wrote: > >>>>>> > >>>>>> On 3/11/22 11:38 AM, Kuldeep Singh wrote: > >>>>>>> PL022 binding require two clocks to be defined but lpc platform > >>>>>>> doesn't > >>>>>>> comply with bindings and define only one clock i.e apb_pclk. > >>>>>>> > >>>>>>> Update spi clocks and clocks-names property by adding appropriate > >>>>>>> clock > >>>>>>> reference to make it compliant with bindings. > >>>>>>> > >>>>>>> CC: Vladimir Zapolskiy > >>>>>>> Signed-off-by: Kuldeep Singh > >>>>>>> --- > >>>>>>> v2: > >>>>>>> - New patch with similar changeset > >>>>>>> - Send to soc ML > >>>>>>> > >>>>>>> arch/arm/boot/dts/lpc32xx.dtsi | 8 ++++---- > >>>>>>> 1 file changed, 4 insertions(+), 4 deletions(-) > >>>>>>> > >>>>>>> diff --git a/arch/arm/boot/dts/lpc32xx.dtsi > >>>>>>> b/arch/arm/boot/dts/lpc32xx.dtsi > >>>>>>> index c87066d6c995..30958e02d5e2 100644 > >>>>>>> --- a/arch/arm/boot/dts/lpc32xx.dtsi > >>>>>>> +++ b/arch/arm/boot/dts/lpc32xx.dtsi > >>>>>>> @@ -178,8 +178,8 @@ ssp0: spi@20084000 { > >>>>>>> compatible = "arm,pl022", > >>>>>>> "arm,primecell"; > >>>>>>> reg = <0x20084000 0x1000>; > >>>>>>> interrupts = <20 > >>>>>>> IRQ_TYPE_LEVEL_HIGH>; > >>>>>>> - clocks = <&clk LPC32XX_CLK_SSP0>; > >>>>>>> - clock-names = "apb_pclk"; > >>>>>>> + clocks = <&clk LPC32XX_CLK_SSP0>, > >>>>>>> <&clk LPC32XX_CLK_SSP0>; > >>>>>>> + clock-names = "sspclk", "apb_pclk"; > >>>>>> > >>>>>> In fact I'm uncertain if it is the right change, could it happen > that > >>>>>> the commit > >>>>>> cc0f6e96c4fd ("spi: dt-bindings: Convert Arm pl022 to json-schema") > >>>>>> sets a wrong > >>>>>> schema pattern? > >>>>> > >>>>> Good pointm this doesn't quite seem right: it is unlikely that the > >>>>> same clock > >>>>> is used for both the SPI bus and the APB bus. > >>>>> > >>>>>> Apparently just one clock is wanted on all observed platforms and > >>>>>> cases, this > >>>>>> is implicitly confirmed by clock handling in the > >>>>>> drivers/spi/spi-pl022.c : > >>>>>> > >>>>>> pl022->clk = devm_clk_get(&adev->dev, NULL); > >>>>>> > >>>>>> So, I would vote to fix the device tree bindings schema. > >>>>> > >>>>> Isn't this just using the wrong name? The name of the macro > >>>>> LPC32XX_CLK_SSP0 might indicate that this is indeed the SPI clock > >>>>> rather than the APB clock, so we only need to change clock-names > >>>>> property here and leave it unchanged otherwise. > >>>> > >>>> Yes, the name is wrong, here I'm ready to take the blame: > >>>> > >>>> Fixes: 93898eb775e5 ("arm: dts: lpc32xx: add clock properties to > device > >>>> nodes") > >>>> > >>>> Noteworthy the commit above presets the same clock name to other > >>>> PrimeCell > >>>> controllers, namely pl110 (LCD), pl080 (DMA), pl175 (EMC) and pl18x > >>>> (SD), > >>>> plus this one pl022 (SSP), and all but SSP and SD are AHB slaves in > >>>> fact. > >>>> > >>>> On LPC32xx the bus clock source and function clock source for SSP is > >>>> HCLK. > >>>> > >>>> My guess is that the misnamed "apb_pclk" migrated into the schema from > >>>> the lpc32xx.dtsi, so I'd suggest, unless some platform really needs > it, > >>>> firstly fix the schema by removing "apb_pclk" clock. It will leave > >>>> just one > >>>> clock, so "clock-names" property can be set as optional, and the drop > >>>> the property from the lpc32xx.dtsi. > >>> > >>> No, "apb_pclk" is part of the common AMBA binding, and is required by > >>> the "arm,primecell" compatible. You won't (usually) find it referenced > >>> in drivers because it's dealt with by amba_get_enable_pclk() via > >>> amba_probe(). > >>> > >> > >> 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), > > Here I'm a bit ignorant, would it be totally reliable to assume that > clk_get(dev, NULL) gets the first clock from the list, and will it never > happen that one day it takes e.g. the last entry? > > I'm kind of surprised that the asked fix in the driver meets such a > resistance. > > > and because presumably the HCLK happens to be enabled already anyway. > > Yes, that's the case here. > > > Changing the driver behaviour would only stand to cause functional > > regressions. > > > > There are effectively two bugs in the DTS here, firstly that it only has > > one clock entry when it should have two, and secondly that the clock > > entry which *is* present has the wrong name (or the wrong clock > > specifier, depending on how you look at it). Kuldeep's patch merely > > fixes the first one by fully describing the way it's currently working > > in practice, so it's really just a choice of whether to treat "respect > > the binding" and "describe the hardware correctly" as separate issues > > and have a follow-up patch to correctly reference HCLK as the second > > clock, or whether they're trivial enough to squash together. > > > > The two problems in the DTS are not argued, the chosen way to correct them > is questionable though. Well, I won't object to see it split into two > changes, but please send them at least in one series then, so that it > won't be left forgotten. > > -- > Best wishes, > Vladimir > On the lpc32xx both the SPI and SSP peripherals are APB devices (low-speed) [image: lpc32xx-apb-peripherals.png] The APB devices on this SoC are driven by the PERIPH_CLK which can be derived from either the HCLK or the SYSCLK. [image: lpc32xx-clock.png] The default on reset is for PERIPH_CLK to be derived from the SYSCLK but both U-Boot and Linux run in "normal" mode, which is to say that PERIPH_CLK, HCLK, ARM_CLK, and DDRAM_CLK are derived from the HCLK PLL. There is no separate SSP clock, the SSP is driven by one clock: the PERIPH_CLK (or "apb_pclk").