[v2,2/3] dt-bindings: spi: constrain minItems of clocks and clock-names

Message ID 20230713090015.127541-3-william.qiu@starfivetech.com
State New
Headers
Series Add SPI module for StarFive JH7110 SoC |

Commit Message

William Qiu July 13, 2023, 9 a.m. UTC
  The SPI controller only need apb_pclk clock to work properly on JH7110 SoC,
so there add minItems whose value is equal to 1. Other platforms do not
have this constraint.

Signed-off-by: William Qiu <william.qiu@starfivetech.com>
---
 Documentation/devicetree/bindings/spi/spi-pl022.yaml | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
  

Comments

Krzysztof Kozlowski July 13, 2023, 10:13 a.m. UTC | #1
On 13/07/2023 11:00, William Qiu wrote:
> The SPI controller only need apb_pclk clock to work properly on JH7110 SoC,
> so there add minItems whose value is equal to 1. Other platforms do not
> have this constraint.
> 
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof
  
Mark Brown July 13, 2023, 12:28 p.m. UTC | #2
On Thu, Jul 13, 2023 at 05:00:14PM +0800, William Qiu wrote:

> The SPI controller only need apb_pclk clock to work properly on JH7110 SoC,
> so there add minItems whose value is equal to 1. Other platforms do not
> have this constraint.

Presumably this means that this is some variant of the usual pl022 IP,
or that the clock is in fact present but is not modelled in your DT?
  
Krzysztof Kozlowski July 13, 2023, 12:39 p.m. UTC | #3
On 13/07/2023 14:28, Mark Brown wrote:
> On Thu, Jul 13, 2023 at 05:00:14PM +0800, William Qiu wrote:
> 
>> The SPI controller only need apb_pclk clock to work properly on JH7110 SoC,
>> so there add minItems whose value is equal to 1. Other platforms do not
>> have this constraint.
> 
> Presumably this means that this is some variant of the usual pl022 IP,

Hm, in such case this could mean we need dedicated compatible.

> or that the clock is in fact present but is not modelled in your DT?

Best regards,
Krzysztof
  
Mark Brown July 13, 2023, 1:48 p.m. UTC | #4
On Thu, Jul 13, 2023 at 02:39:19PM +0200, Krzysztof Kozlowski wrote:
> On 13/07/2023 14:28, Mark Brown wrote:
> > On Thu, Jul 13, 2023 at 05:00:14PM +0800, William Qiu wrote:

> >> The SPI controller only need apb_pclk clock to work properly on JH7110 SoC,
> >> so there add minItems whose value is equal to 1. Other platforms do not
> >> have this constraint.

> > Presumably this means that this is some variant of the usual pl022 IP,

> Hm, in such case this could mean we need dedicated compatible.

Yes, indeed.
  
Rob Herring July 13, 2023, 2:57 p.m. UTC | #5
On Thu, Jul 13, 2023 at 02:39:19PM +0200, Krzysztof Kozlowski wrote:
> On 13/07/2023 14:28, Mark Brown wrote:
> > On Thu, Jul 13, 2023 at 05:00:14PM +0800, William Qiu wrote:
> > 
> >> The SPI controller only need apb_pclk clock to work properly on JH7110 SoC,
> >> so there add minItems whose value is equal to 1. Other platforms do not
> >> have this constraint.
> > 
> > Presumably this means that this is some variant of the usual pl022 IP,
> 
> Hm, in such case this could mean we need dedicated compatible.

Except the vendor in the ID registers should be different if the IP is 
modified.

I suspect that PCLK and SSPCLK are tied to the same clock source. There 
must be an SSPCLK because that is the one used to clock the SPI bus and 
we need to know the frequency of it.

Rob
  
William Qiu July 14, 2023, 7:14 a.m. UTC | #6
On 2023/7/13 22:57, Rob Herring wrote:
> On Thu, Jul 13, 2023 at 02:39:19PM +0200, Krzysztof Kozlowski wrote:
>> On 13/07/2023 14:28, Mark Brown wrote:
>> > On Thu, Jul 13, 2023 at 05:00:14PM +0800, William Qiu wrote:
>> > 
>> >> The SPI controller only need apb_pclk clock to work properly on JH7110 SoC,
>> >> so there add minItems whose value is equal to 1. Other platforms do not
>> >> have this constraint.
>> > 
>> > Presumably this means that this is some variant of the usual pl022 IP,
>> 
>> Hm, in such case this could mean we need dedicated compatible.
> 
> Except the vendor in the ID registers should be different if the IP is 
> modified.
> 
> I suspect that PCLK and SSPCLK are tied to the same clock source. There 
> must be an SSPCLK because that is the one used to clock the SPI bus and 
> we need to know the frequency of it.
> 
> Rob

After communicating with colleagues in SoC FE, I learned that PCLK and
SSPCLK were homologous on JH7110. He said that SSPCLK would divide the
frequency internally anyway, and there was no need for external part frequency,
so he directly gave them together.

So, should I call this clock ssp_apb or keep it SSPCLK?

Best regards,
William
  
Mark Brown July 14, 2023, 11:52 a.m. UTC | #7
On Fri, Jul 14, 2023 at 03:14:59PM +0800, William Qiu wrote:
> On 2023/7/13 22:57, Rob Herring wrote:

> > I suspect that PCLK and SSPCLK are tied to the same clock source. There 
> > must be an SSPCLK because that is the one used to clock the SPI bus and 
> > we need to know the frequency of it.

> After communicating with colleagues in SoC FE, I learned that PCLK and
> SSPCLK were homologous on JH7110. He said that SSPCLK would divide the
> frequency internally anyway, and there was no need for external part frequency,
> so he directly gave them together.

> So, should I call this clock ssp_apb or keep it SSPCLK?

I'd expect this to be handled in the DTS for the SoC - connect both
clocks the binding requires to whatever the upstream clock is, it's not
clear to me that any binding change is required.
  
William Qiu July 18, 2023, 6:06 a.m. UTC | #8
On 2023/7/14 19:52, Mark Brown wrote:
> On Fri, Jul 14, 2023 at 03:14:59PM +0800, William Qiu wrote:
>> On 2023/7/13 22:57, Rob Herring wrote:
> 
>> > I suspect that PCLK and SSPCLK are tied to the same clock source. There 
>> > must be an SSPCLK because that is the one used to clock the SPI bus and 
>> > we need to know the frequency of it.
> 
>> After communicating with colleagues in SoC FE, I learned that PCLK and
>> SSPCLK were homologous on JH7110. He said that SSPCLK would divide the
>> frequency internally anyway, and there was no need for external part frequency,
>> so he directly gave them together.
> 
>> So, should I call this clock ssp_apb or keep it SSPCLK?
> 
> I'd expect this to be handled in the DTS for the SoC - connect both
> clocks the binding requires to whatever the upstream clock is, it's not
> clear to me that any binding change is required.

You mean binding two clocks, with the same clock source? Then there is no
need to modify YAML.

Best regards,
William
  
Mark Brown July 18, 2023, 2:07 p.m. UTC | #9
On Tue, Jul 18, 2023 at 02:06:01PM +0800, William Qiu wrote:
> On 2023/7/14 19:52, Mark Brown wrote:
> > On Fri, Jul 14, 2023 at 03:14:59PM +0800, William Qiu wrote:

> >> After communicating with colleagues in SoC FE, I learned that PCLK and
> >> SSPCLK were homologous on JH7110. He said that SSPCLK would divide the
> >> frequency internally anyway, and there was no need for external part frequency,
> >> so he directly gave them together.

> >> So, should I call this clock ssp_apb or keep it SSPCLK?

> > I'd expect this to be handled in the DTS for the SoC - connect both
> > clocks the binding requires to whatever the upstream clock is, it's not
> > clear to me that any binding change is required.

> You mean binding two clocks, with the same clock source? Then there is no
> need to modify YAML.

Yes, exactly.
  

Patch

diff --git a/Documentation/devicetree/bindings/spi/spi-pl022.yaml b/Documentation/devicetree/bindings/spi/spi-pl022.yaml
index 5e5a704a766e..42bb34c39971 100644
--- a/Documentation/devicetree/bindings/spi/spi-pl022.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-pl022.yaml
@@ -35,12 +35,16 @@  properties:
     maxItems: 1
 
   clocks:
+    minItems: 1
     maxItems: 2
 
   clock-names:
-    items:
-      - const: sspclk
-      - const: apb_pclk
+    oneOf:
+      - items:
+          - const: apb_pclk
+      - items:
+          - const: sspclk
+          - const: apb_pclk
 
   pl022,autosuspend-delay:
     description: delay in ms following transfer completion before the