Re: [PATCH v2 3/3] ARM: dts: lpc32xx: Update spi clock properties

[Robin Murphy <robin.murphy@arm.com>]

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 <vz@mleia.com> 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 <vz@mleia.com>
>>>> Signed-off-by: Kuldeep Singh <singh.kuldeep87k@gmail.com>
>>>> ---
>>>> 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().

Robin.