diff mbox series

[07/14] riscv: dts: canaan: fix the k210's memory node

Message ID 20220618123035.563070-8-mail@conchuod.ie (mailing list archive)
State Superseded
Headers show
Series Canaan devicetree fixes | expand

Commit Message

Conor Dooley June 18, 2022, 12:30 p.m. UTC
From: Conor Dooley <conor.dooley@microchip.com>

The k210 memory node has a compatible string that does not match with
any driver or dt-binding & has several non standard properties.
Replace the reg names with a comment and delete the rest.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
---
 arch/riscv/boot/dts/canaan/k210.dtsi | 6 ------
 1 file changed, 6 deletions(-)

Comments

Conor Dooley June 18, 2022, 12:35 p.m. UTC | #1
On 18/06/2022 13:30, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> The k210 memory node has a compatible string that does not match with
> any driver or dt-binding & has several non standard properties.
> Replace the reg names with a comment and delete the rest.

Gah, should've fixed that during rebase.
I'm sure there'll need to be (at least) a v2 & I'll wait rather
than resend.
Thanks,
Conor.

> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> ---
>  arch/riscv/boot/dts/canaan/k210.dtsi | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/arch/riscv/boot/dts/canaan/k210.dtsi b/arch/riscv/boot/dts/canaan/k210.dtsi
> index 44d338514761..287ea6eebe47 100644
> --- a/arch/riscv/boot/dts/canaan/k210.dtsi
> +++ b/arch/riscv/boot/dts/canaan/k210.dtsi
> @@ -69,15 +69,9 @@ cpu1_intc: interrupt-controller {
>  
>  	sram: memory@80000000 {
>  		device_type = "memory";
> -		compatible = "canaan,k210-sram";
>  		reg = <0x80000000 0x400000>,
>  		      <0x80400000 0x200000>,
>  		      <0x80600000 0x200000>;
> -		reg-names = "sram0", "sram1", "aisram";
> -		clocks = <&sysclk K210_CLK_SRAM0>,
> -			 <&sysclk K210_CLK_SRAM1>,
> -			 <&sysclk K210_CLK_AI>;
> -		clock-names = "sram0", "sram1", "aisram";
>  	};
>  
>  	clocks {
Damien Le Moal June 19, 2022, 11:38 p.m. UTC | #2
On 6/18/22 21:30, Conor Dooley wrote:
> From: Conor Dooley <conor.dooley@microchip.com>
> 
> The k210 memory node has a compatible string that does not match with
> any driver or dt-binding & has several non standard properties.
> Replace the reg names with a comment and delete the rest.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
> ---
>  arch/riscv/boot/dts/canaan/k210.dtsi | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/arch/riscv/boot/dts/canaan/k210.dtsi b/arch/riscv/boot/dts/canaan/k210.dtsi
> index 44d338514761..287ea6eebe47 100644
> --- a/arch/riscv/boot/dts/canaan/k210.dtsi
> +++ b/arch/riscv/boot/dts/canaan/k210.dtsi
> @@ -69,15 +69,9 @@ cpu1_intc: interrupt-controller {
>  
>  	sram: memory@80000000 {
>  		device_type = "memory";
> -		compatible = "canaan,k210-sram";
>  		reg = <0x80000000 0x400000>,
>  		      <0x80400000 0x200000>,
>  		      <0x80600000 0x200000>;
> -		reg-names = "sram0", "sram1", "aisram";
> -		clocks = <&sysclk K210_CLK_SRAM0>,
> -			 <&sysclk K210_CLK_SRAM1>,
> -			 <&sysclk K210_CLK_AI>;
> -		clock-names = "sram0", "sram1", "aisram";
>  	};

These are used by u-boot to setup the memory clocks and initialize the
aisram. Sure the kernel actually does not use this, but to be in sync with
u-boot DT, I would prefer keeping this as is. Right now, u-boot *and* the
kernel work fine with both u-boot internal DT and the kernel DT.
Conor Dooley June 19, 2022, 11:54 p.m. UTC | #3
On 20/06/2022 00:38, Damien Le Moal wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 6/18/22 21:30, Conor Dooley wrote:
>> From: Conor Dooley <conor.dooley@microchip.com>
>>
>> The k210 memory node has a compatible string that does not match with
>> any driver or dt-binding & has several non standard properties.
>> Replace the reg names with a comment and delete the rest.
>>
>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>> ---
>> ---
>>  arch/riscv/boot/dts/canaan/k210.dtsi | 6 ------
>>  1 file changed, 6 deletions(-)
>>
>> diff --git a/arch/riscv/boot/dts/canaan/k210.dtsi b/arch/riscv/boot/dts/canaan/k210.dtsi
>> index 44d338514761..287ea6eebe47 100644
>> --- a/arch/riscv/boot/dts/canaan/k210.dtsi
>> +++ b/arch/riscv/boot/dts/canaan/k210.dtsi
>> @@ -69,15 +69,9 @@ cpu1_intc: interrupt-controller {
>>
>>       sram: memory@80000000 {
>>               device_type = "memory";
>> -             compatible = "canaan,k210-sram";
>>               reg = <0x80000000 0x400000>,
>>                     <0x80400000 0x200000>,
>>                     <0x80600000 0x200000>;
>> -             reg-names = "sram0", "sram1", "aisram";
>> -             clocks = <&sysclk K210_CLK_SRAM0>,
>> -                      <&sysclk K210_CLK_SRAM1>,
>> -                      <&sysclk K210_CLK_AI>;
>> -             clock-names = "sram0", "sram1", "aisram";
>>       };
> 
> These are used by u-boot to setup the memory clocks and initialize the
> aisram. Sure the kernel actually does not use this, but to be in sync with
> u-boot DT, I would prefer keeping this as is. Right now, u-boot *and* the
> kernel work fine with both u-boot internal DT and the kernel DT.

Right, but unfortunately that desire alone doesn't do anything about
the dtbs_check complaints.

I guess the alternative approach of actually documenting the compatible
would be more palatable?

Thanks,
Conor.
Damien Le Moal June 20, 2022, 12:25 a.m. UTC | #4
On 6/20/22 08:54, Conor.Dooley@microchip.com wrote:
> On 20/06/2022 00:38, Damien Le Moal wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 6/18/22 21:30, Conor Dooley wrote:
>>> From: Conor Dooley <conor.dooley@microchip.com>
>>>
>>> The k210 memory node has a compatible string that does not match with
>>> any driver or dt-binding & has several non standard properties.
>>> Replace the reg names with a comment and delete the rest.
>>>
>>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>>> ---
>>> ---
>>>  arch/riscv/boot/dts/canaan/k210.dtsi | 6 ------
>>>  1 file changed, 6 deletions(-)
>>>
>>> diff --git a/arch/riscv/boot/dts/canaan/k210.dtsi b/arch/riscv/boot/dts/canaan/k210.dtsi
>>> index 44d338514761..287ea6eebe47 100644
>>> --- a/arch/riscv/boot/dts/canaan/k210.dtsi
>>> +++ b/arch/riscv/boot/dts/canaan/k210.dtsi
>>> @@ -69,15 +69,9 @@ cpu1_intc: interrupt-controller {
>>>
>>>       sram: memory@80000000 {
>>>               device_type = "memory";
>>> -             compatible = "canaan,k210-sram";
>>>               reg = <0x80000000 0x400000>,
>>>                     <0x80400000 0x200000>,
>>>                     <0x80600000 0x200000>;
>>> -             reg-names = "sram0", "sram1", "aisram";
>>> -             clocks = <&sysclk K210_CLK_SRAM0>,
>>> -                      <&sysclk K210_CLK_SRAM1>,
>>> -                      <&sysclk K210_CLK_AI>;
>>> -             clock-names = "sram0", "sram1", "aisram";
>>>       };
>>
>> These are used by u-boot to setup the memory clocks and initialize the
>> aisram. Sure the kernel actually does not use this, but to be in sync with
>> u-boot DT, I would prefer keeping this as is. Right now, u-boot *and* the
>> kernel work fine with both u-boot internal DT and the kernel DT.
> 
> Right, but unfortunately that desire alone doesn't do anything about
> the dtbs_check complaints.
> 
> I guess the alternative approach of actually documenting the compatible
> would be more palatable?

Yes, I think so. That would allow keeping the fields without the DTB build
warnings.

> 
> Thanks,
> Conor.
> 
> 
>
Conor Dooley June 21, 2022, 9:49 a.m. UTC | #5
On 20/06/2022 01:25, Damien Le Moal wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 6/20/22 08:54, Conor.Dooley@microchip.com wrote:
>> On 20/06/2022 00:38, Damien Le Moal wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 6/18/22 21:30, Conor Dooley wrote:
>>>> From: Conor Dooley <conor.dooley@microchip.com>
>>>>
>>>> The k210 memory node has a compatible string that does not match with
>>>> any driver or dt-binding & has several non standard properties.
>>>> Replace the reg names with a comment and delete the rest.
>>>>
>>>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>>>> ---
>>>> ---
>>>>   arch/riscv/boot/dts/canaan/k210.dtsi | 6 ------
>>>>   1 file changed, 6 deletions(-)
>>>>
>>>> diff --git a/arch/riscv/boot/dts/canaan/k210.dtsi b/arch/riscv/boot/dts/canaan/k210.dtsi
>>>> index 44d338514761..287ea6eebe47 100644
>>>> --- a/arch/riscv/boot/dts/canaan/k210.dtsi
>>>> +++ b/arch/riscv/boot/dts/canaan/k210.dtsi
>>>> @@ -69,15 +69,9 @@ cpu1_intc: interrupt-controller {
>>>>
>>>>        sram: memory@80000000 {
>>>>                device_type = "memory";
>>>> -             compatible = "canaan,k210-sram";
>>>>                reg = <0x80000000 0x400000>,
>>>>                      <0x80400000 0x200000>,
>>>>                      <0x80600000 0x200000>;
>>>> -             reg-names = "sram0", "sram1", "aisram";
>>>> -             clocks = <&sysclk K210_CLK_SRAM0>,
>>>> -                      <&sysclk K210_CLK_SRAM1>,
>>>> -                      <&sysclk K210_CLK_AI>;
>>>> -             clock-names = "sram0", "sram1", "aisram";
>>>>        };
>>>
>>> These are used by u-boot to setup the memory clocks and initialize the
>>> aisram. Sure the kernel actually does not use this, but to be in sync with
>>> u-boot DT, I would prefer keeping this as is. Right now, u-boot *and* the
>>> kernel work fine with both u-boot internal DT and the kernel DT.
>>
>> Right, but unfortunately that desire alone doesn't do anything about
>> the dtbs_check complaints.
>>
>> I guess the alternative approach of actually documenting the compatible
>> would be more palatable?
> 
> Yes, I think so. That would allow keeping the fields without the DTB build
> warnings.

Hmm looks like that approach contradicts the dt-schema;
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/memory.yaml

@Rob,Krzysztof what is one meant to do here?

Thanks,
Conor.
Krzysztof Kozlowski June 27, 2022, 6:55 a.m. UTC | #6
On 21/06/2022 11:49, Conor.Dooley@microchip.com wrote:
> On 20/06/2022 01:25, Damien Le Moal wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 6/20/22 08:54, Conor.Dooley@microchip.com wrote:
>>> On 20/06/2022 00:38, Damien Le Moal wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> On 6/18/22 21:30, Conor Dooley wrote:
>>>>> From: Conor Dooley <conor.dooley@microchip.com>
>>>>>
>>>>> The k210 memory node has a compatible string that does not match with
>>>>> any driver or dt-binding & has several non standard properties.
>>>>> Replace the reg names with a comment and delete the rest.
>>>>>
>>>>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>>>>> ---
>>>>> ---
>>>>>   arch/riscv/boot/dts/canaan/k210.dtsi | 6 ------
>>>>>   1 file changed, 6 deletions(-)
>>>>>
>>>>> diff --git a/arch/riscv/boot/dts/canaan/k210.dtsi b/arch/riscv/boot/dts/canaan/k210.dtsi
>>>>> index 44d338514761..287ea6eebe47 100644
>>>>> --- a/arch/riscv/boot/dts/canaan/k210.dtsi
>>>>> +++ b/arch/riscv/boot/dts/canaan/k210.dtsi
>>>>> @@ -69,15 +69,9 @@ cpu1_intc: interrupt-controller {
>>>>>
>>>>>        sram: memory@80000000 {
>>>>>                device_type = "memory";
>>>>> -             compatible = "canaan,k210-sram";
>>>>>                reg = <0x80000000 0x400000>,
>>>>>                      <0x80400000 0x200000>,
>>>>>                      <0x80600000 0x200000>;
>>>>> -             reg-names = "sram0", "sram1", "aisram";
>>>>> -             clocks = <&sysclk K210_CLK_SRAM0>,
>>>>> -                      <&sysclk K210_CLK_SRAM1>,
>>>>> -                      <&sysclk K210_CLK_AI>;
>>>>> -             clock-names = "sram0", "sram1", "aisram";
>>>>>        };
>>>>
>>>> These are used by u-boot to setup the memory clocks and initialize the
>>>> aisram. Sure the kernel actually does not use this, but to be in sync with
>>>> u-boot DT, I would prefer keeping this as is. Right now, u-boot *and* the
>>>> kernel work fine with both u-boot internal DT and the kernel DT.
>>>
>>> Right, but unfortunately that desire alone doesn't do anything about
>>> the dtbs_check complaints.
>>>
>>> I guess the alternative approach of actually documenting the compatible
>>> would be more palatable?
>>
>> Yes, I think so. That would allow keeping the fields without the DTB build
>> warnings.
> 
> Hmm looks like that approach contradicts the dt-schema;
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/memory.yaml
> 
> @Rob,Krzysztof what is one meant to do here?

Why do you think it contradict bindings? Bindings for memory allow
additional properties, so you just need to create binding for this one.
And make it a correct binding, IOW, be sure that these clocks are real etc.

Although usually we had separate bindings (and device drivers) for
memory controllers, instead of including them in the "memory" node.

Best regards,
Krzysztof
Conor Dooley June 27, 2022, 7:06 a.m. UTC | #7
On 27/06/2022 07:55, Krzysztof Kozlowski wrote:
> On 21/06/2022 11:49, Conor.Dooley@microchip.com wrote:
>> On 20/06/2022 01:25, Damien Le Moal wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 6/20/22 08:54, Conor.Dooley@microchip.com wrote:
>>>> On 20/06/2022 00:38, Damien Le Moal wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> On 6/18/22 21:30, Conor Dooley wrote:
>>>>>> From: Conor Dooley <conor.dooley@microchip.com>
>>>>>>
>>>>>> The k210 memory node has a compatible string that does not match with
>>>>>> any driver or dt-binding & has several non standard properties.
>>>>>> Replace the reg names with a comment and delete the rest.
>>>>>>
>>>>>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>>>>>> ---
>>>>>> ---
>>>>>>    arch/riscv/boot/dts/canaan/k210.dtsi | 6 ------
>>>>>>    1 file changed, 6 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/riscv/boot/dts/canaan/k210.dtsi b/arch/riscv/boot/dts/canaan/k210.dtsi
>>>>>> index 44d338514761..287ea6eebe47 100644
>>>>>> --- a/arch/riscv/boot/dts/canaan/k210.dtsi
>>>>>> +++ b/arch/riscv/boot/dts/canaan/k210.dtsi
>>>>>> @@ -69,15 +69,9 @@ cpu1_intc: interrupt-controller {
>>>>>>
>>>>>>         sram: memory@80000000 {
>>>>>>                 device_type = "memory";
>>>>>> -             compatible = "canaan,k210-sram";
>>>>>>                 reg = <0x80000000 0x400000>,
>>>>>>                       <0x80400000 0x200000>,
>>>>>>                       <0x80600000 0x200000>;
>>>>>> -             reg-names = "sram0", "sram1", "aisram";
>>>>>> -             clocks = <&sysclk K210_CLK_SRAM0>,
>>>>>> -                      <&sysclk K210_CLK_SRAM1>,
>>>>>> -                      <&sysclk K210_CLK_AI>;
>>>>>> -             clock-names = "sram0", "sram1", "aisram";
>>>>>>         };
>>>>>
>>>>> These are used by u-boot to setup the memory clocks and initialize the
>>>>> aisram. Sure the kernel actually does not use this, but to be in sync with
>>>>> u-boot DT, I would prefer keeping this as is. Right now, u-boot *and* the
>>>>> kernel work fine with both u-boot internal DT and the kernel DT.
>>>>
>>>> Right, but unfortunately that desire alone doesn't do anything about
>>>> the dtbs_check complaints.
>>>>
>>>> I guess the alternative approach of actually documenting the compatible
>>>> would be more palatable?
>>>
>>> Yes, I think so. That would allow keeping the fields without the DTB build
>>> warnings.
>>
>> Hmm looks like that approach contradicts the dt-schema;
>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/memory.yaml
>>
>> @Rob,Krzysztof what is one meant to do here?
> 
> Why do you think it contradict bindings? Bindings for memory allow

Because when I tried to write the binding, the memory node complained
about the clock properties etc and referenced the dt-schema (which
for memory@foo nodes has additionalProperties: false.

> additional properties, so you just need to create binding for this one.
> And make it a correct binding, IOW, be sure that these clocks are real etc.
> 
> Although usually we had separate bindings (and device drivers) for
> memory controllers, instead of including them in the "memory" node.

I guess changing to that format would probably require some changes on
the U-Boot side of things. Taking "calxeda,hb-ddr-ctrl" as an example,
looks like the clocks etc go in a controller node, which seems like a
"better" way of doing it - but would break existing dts in U-Boot
without changes to handle both methods there.

Thanks,
Conor.
Krzysztof Kozlowski June 27, 2022, 9:24 a.m. UTC | #8
On 27/06/2022 09:06, Conor.Dooley@microchip.com wrote:
> 
> 
> On 27/06/2022 07:55, Krzysztof Kozlowski wrote:
>> On 21/06/2022 11:49, Conor.Dooley@microchip.com wrote:
>>> On 20/06/2022 01:25, Damien Le Moal wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> On 6/20/22 08:54, Conor.Dooley@microchip.com wrote:
>>>>> On 20/06/2022 00:38, Damien Le Moal wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>
>>>>>> On 6/18/22 21:30, Conor Dooley wrote:
>>>>>>> From: Conor Dooley <conor.dooley@microchip.com>
>>>>>>>
>>>>>>> The k210 memory node has a compatible string that does not match with
>>>>>>> any driver or dt-binding & has several non standard properties.
>>>>>>> Replace the reg names with a comment and delete the rest.
>>>>>>>
>>>>>>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>>>>>>> ---
>>>>>>> ---
>>>>>>>    arch/riscv/boot/dts/canaan/k210.dtsi | 6 ------
>>>>>>>    1 file changed, 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/riscv/boot/dts/canaan/k210.dtsi b/arch/riscv/boot/dts/canaan/k210.dtsi
>>>>>>> index 44d338514761..287ea6eebe47 100644
>>>>>>> --- a/arch/riscv/boot/dts/canaan/k210.dtsi
>>>>>>> +++ b/arch/riscv/boot/dts/canaan/k210.dtsi
>>>>>>> @@ -69,15 +69,9 @@ cpu1_intc: interrupt-controller {
>>>>>>>
>>>>>>>         sram: memory@80000000 {
>>>>>>>                 device_type = "memory";
>>>>>>> -             compatible = "canaan,k210-sram";
>>>>>>>                 reg = <0x80000000 0x400000>,
>>>>>>>                       <0x80400000 0x200000>,
>>>>>>>                       <0x80600000 0x200000>;
>>>>>>> -             reg-names = "sram0", "sram1", "aisram";
>>>>>>> -             clocks = <&sysclk K210_CLK_SRAM0>,
>>>>>>> -                      <&sysclk K210_CLK_SRAM1>,
>>>>>>> -                      <&sysclk K210_CLK_AI>;
>>>>>>> -             clock-names = "sram0", "sram1", "aisram";
>>>>>>>         };
>>>>>>
>>>>>> These are used by u-boot to setup the memory clocks and initialize the
>>>>>> aisram. Sure the kernel actually does not use this, but to be in sync with
>>>>>> u-boot DT, I would prefer keeping this as is. Right now, u-boot *and* the
>>>>>> kernel work fine with both u-boot internal DT and the kernel DT.
>>>>>
>>>>> Right, but unfortunately that desire alone doesn't do anything about
>>>>> the dtbs_check complaints.
>>>>>
>>>>> I guess the alternative approach of actually documenting the compatible
>>>>> would be more palatable?
>>>>
>>>> Yes, I think so. That would allow keeping the fields without the DTB build
>>>> warnings.
>>>
>>> Hmm looks like that approach contradicts the dt-schema;
>>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/memory.yaml
>>>
>>> @Rob,Krzysztof what is one meant to do here?
>>
>> Why do you think it contradict bindings? Bindings for memory allow
> 
> Because when I tried to write the binding, the memory node complained
> about the clock properties etc and referenced the dt-schema (which
> for memory@foo nodes has additionalProperties: false.

Ah, I see, I looked at wrong level. Indeed memory node cannot have
anything else.

> 
>> additional properties, so you just need to create binding for this one.
>> And make it a correct binding, IOW, be sure that these clocks are real etc.
>>
>> Although usually we had separate bindings (and device drivers) for
>> memory controllers, instead of including them in the "memory" node.
> 
> I guess changing to that format would probably require some changes on
> the U-Boot side of things. Taking "calxeda,hb-ddr-ctrl" as an example,
> looks like the clocks etc go in a controller node, which seems like a
> "better" way of doing it - 

Yes, because I think memory node is kind of special. It describes the
physical memory layout for the system, not the memory controller or
memory characteristics (like timings).

What U-Boot needs is indeed memory controller node. It's not only
calxeda but also few others using JEDEC LPDDR bindings.

> but would break existing dts in U-Boot
> without changes to handle both methods there.

Yes, that's a bit inconvenient but also a price someone has to pay for
introducing DTS properties without bindings.

Best regards,
Krzysztof
Conor Dooley June 27, 2022, 11:03 a.m. UTC | #9
On 27/06/2022 10:24, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 27/06/2022 09:06, Conor.Dooley@microchip.com wrote:
>>
>>
>> On 27/06/2022 07:55, Krzysztof Kozlowski wrote:
>>> On 21/06/2022 11:49, Conor.Dooley@microchip.com wrote:
>>>> On 20/06/2022 01:25, Damien Le Moal wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> On 6/20/22 08:54, Conor.Dooley@microchip.com wrote:
>>>>>> On 20/06/2022 00:38, Damien Le Moal wrote:
>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>>
>>>>>>> On 6/18/22 21:30, Conor Dooley wrote:
>>>>>>>> From: Conor Dooley <conor.dooley@microchip.com>
>>>>>>>>
>>>>>>>> The k210 memory node has a compatible string that does not match with
>>>>>>>> any driver or dt-binding & has several non standard properties.
>>>>>>>> Replace the reg names with a comment and delete the rest.
>>>>>>>>
>>>>>>>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>>>>>>>> ---
>>>>>>>> ---
>>>>>>>>     arch/riscv/boot/dts/canaan/k210.dtsi | 6 ------
>>>>>>>>     1 file changed, 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/riscv/boot/dts/canaan/k210.dtsi b/arch/riscv/boot/dts/canaan/k210.dtsi
>>>>>>>> index 44d338514761..287ea6eebe47 100644
>>>>>>>> --- a/arch/riscv/boot/dts/canaan/k210.dtsi
>>>>>>>> +++ b/arch/riscv/boot/dts/canaan/k210.dtsi
>>>>>>>> @@ -69,15 +69,9 @@ cpu1_intc: interrupt-controller {
>>>>>>>>
>>>>>>>>          sram: memory@80000000 {
>>>>>>>>                  device_type = "memory";
>>>>>>>> -             compatible = "canaan,k210-sram";
>>>>>>>>                  reg = <0x80000000 0x400000>,
>>>>>>>>                        <0x80400000 0x200000>,
>>>>>>>>                        <0x80600000 0x200000>;
>>>>>>>> -             reg-names = "sram0", "sram1", "aisram";
>>>>>>>> -             clocks = <&sysclk K210_CLK_SRAM0>,
>>>>>>>> -                      <&sysclk K210_CLK_SRAM1>,
>>>>>>>> -                      <&sysclk K210_CLK_AI>;
>>>>>>>> -             clock-names = "sram0", "sram1", "aisram";
>>>>>>>>          };
>>>>>>>
>>>>>>> These are used by u-boot to setup the memory clocks and initialize the
>>>>>>> aisram. Sure the kernel actually does not use this, but to be in sync with
>>>>>>> u-boot DT, I would prefer keeping this as is. Right now, u-boot *and* the
>>>>>>> kernel work fine with both u-boot internal DT and the kernel DT.
>>>>>>
>>>>>> Right, but unfortunately that desire alone doesn't do anything about
>>>>>> the dtbs_check complaints.
>>>>>>
>>>>>> I guess the alternative approach of actually documenting the compatible
>>>>>> would be more palatable?
>>>>>
>>>>> Yes, I think so. That would allow keeping the fields without the DTB build
>>>>> warnings.
>>>>
>>>> Hmm looks like that approach contradicts the dt-schema;
>>>> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/memory.yaml
>>>>
>>>> @Rob,Krzysztof what is one meant to do here?
>>>
>>> Why do you think it contradict bindings? Bindings for memory allow
>>
>> Because when I tried to write the binding, the memory node complained
>> about the clock properties etc and referenced the dt-schema (which
>> for memory@foo nodes has additionalProperties: false.
> 
> Ah, I see, I looked at wrong level. Indeed memory node cannot have
> anything else.
> 
>>
>>> additional properties, so you just need to create binding for this one.
>>> And make it a correct binding, IOW, be sure that these clocks are real etc.
>>>
>>> Although usually we had separate bindings (and device drivers) for
>>> memory controllers, instead of including them in the "memory" node.
>>
>> I guess changing to that format would probably require some changes on
>> the U-Boot side of things. Taking "calxeda,hb-ddr-ctrl" as an example,
>> looks like the clocks etc go in a controller node, which seems like a
>> "better" way of doing it -
> 
> Yes, because I think memory node is kind of special. It describes the
> physical memory layout for the system, not the memory controller or
> memory characteristics (like timings).
> 
> What U-Boot needs is indeed memory controller node. It's not only
> calxeda but also few others using JEDEC LPDDR bindings.
> 
>> but would break existing dts in U-Boot
>> without changes to handle both methods there.
> 
> Yes, that's a bit inconvenient but also a price someone has to pay for
> introducing DTS properties without bindings.
> 

Alright so, I'll make it a memory controller and conjure up a v2.
As always, thanks for your help Krzysztof!
diff mbox series

Patch

diff --git a/arch/riscv/boot/dts/canaan/k210.dtsi b/arch/riscv/boot/dts/canaan/k210.dtsi
index 44d338514761..287ea6eebe47 100644
--- a/arch/riscv/boot/dts/canaan/k210.dtsi
+++ b/arch/riscv/boot/dts/canaan/k210.dtsi
@@ -69,15 +69,9 @@  cpu1_intc: interrupt-controller {
 
 	sram: memory@80000000 {
 		device_type = "memory";
-		compatible = "canaan,k210-sram";
 		reg = <0x80000000 0x400000>,
 		      <0x80400000 0x200000>,
 		      <0x80600000 0x200000>;
-		reg-names = "sram0", "sram1", "aisram";
-		clocks = <&sysclk K210_CLK_SRAM0>,
-			 <&sysclk K210_CLK_SRAM1>,
-			 <&sysclk K210_CLK_AI>;
-		clock-names = "sram0", "sram1", "aisram";
 	};
 
 	clocks {