diff mbox

[v4,2/4] ARM: dts: Add SROMc to Exynos 5410

Message ID 56333731.1000400@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Pankaj Dubey Oct. 30, 2015, 9:24 a.m. UTC 
Hi Pavel,

On Friday 30 October 2015 12:11 PM, Pavel Fedin wrote:
>   Hello!
>
>> -----Original Message-----
>> From: linux-samsung-soc-owner@vger.kernel.org [mailto:linux-samsung-soc-owner@vger.kernel.org]
>> On Behalf Of Pankaj Dubey
>> Sent: Thursday, October 29, 2015 8:28 PM
>> To: Pavel Fedin
>> Cc: devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-samsung-soc;
>> linux-kernel@vger.kernel.org; Rob Herring; Pawel Moll; Mark Rutland; Ian Campbell; Kumar Gala;
>> Kukjin Kim; Krzysztof Kozlowski
>> Subject: Re: [PATCH v4 2/4] ARM: dts: Add SROMc to Exynos 5410
>>
>> Hi Pavel,
>>
>> On 29 October 2015 at 18:12, Pavel Fedin <p.fedin@samsung.com> wrote:
>>> This machine uses own SoC device tree file, add missing part.
>>>
>>> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
>>> ---
>>>   arch/arm/boot/dts/exynos5410.dtsi | 15 +++++++++++++++
>>>   1 file changed, 15 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/exynos5410.dtsi b/arch/arm/boot/dts/exynos5410.dtsi
>>> index 4603356..da6a8fa0e 100644
>>> --- a/arch/arm/boot/dts/exynos5410.dtsi
>>> +++ b/arch/arm/boot/dts/exynos5410.dtsi
>>> @@ -101,6 +101,15 @@
>>>                          reg = <0x10000000 0x100>;
>>>                  };
>>>
>>> +               sromc: sromc@12250000 {
>>> +                       #address-cells = <1>;
>>> +                       #size-cells = <1>;
>>> +                       ranges;
>>> +
>>
>> We do not need to specify these three properties as they are already
>> present in parent node "soc".
>
>   We do, otherwise dtc complains about defaults of #address-cells = 2 and #size-cells=1, and without empty "ranges" subnode's resources are not correctly translated.
>

First of all this patch will not give this error. So this part should 
not be a part of this patch.
You should be getting above error after applying v4 4/4 "ARM: dts: Add 
Ethernet chip to SMDK5410". So if its failing for ethernet subnode, you 
can move this address-cells and size-cells in dts file just above 
ethernet node as shown below:

index 311e7be..d69981d 100644
And another question still remains open, we can't just like that change 
"smsc,lan9115" binding by adding samsung specific properties. If you 
want to do and there is no other way, you need to update DT binding of 
"smsc,lan9115" and get it reviewed.
Probably you can check suggestions from Krzysztof, where he pointed out 
some hint on how other places this is getting used.


Thanks,
Pankaj Dubey
>>
>>> +                       compatible = "samsung,exynos-srom";
>>> +                       reg = <0x12250000 0x14>;
>>> +               };
>>> +
>>>                  pmu_system_controller: system-controller@10040000 {
>>>                          compatible = "samsung,exynos5410-pmu", "syscon";
>>>                          reg = <0x10040000 0x5000>;
>>> @@ -133,6 +142,12 @@
>>>                                                  <10 &gic 0 130 0>,
>>>                                                  <11 &gic 0 131 0>;
>>>                          };
>>> +
>>> +                       arch_timer {
>>> +                               compatible = "arm,armv7-timer";
>>> +                               clock-frequency = <24000000>;
>>> +                       };
>>> +
>>
>> This change should not be part of this patch.
>
>   Ooops, thank you very much, this should not have been here at all. This is a leftover from my experiments, i was tracing DT parsing code and added it just for test, to see why timer gets probed as a subnode.
>   Just forgot to remove it afterwards and it slipped into the patch.
>
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
>
>

Comments

Pavel Fedin Oct. 30, 2015, 10:51 a.m. UTC | #1 
Hello!

> >>> +               sromc: sromc@12250000 {
> >>> +                       #address-cells = <1>;
> >>> +                       #size-cells = <1>;
> >>> +                       ranges;
> >>> +
> >>
> >> We do not need to specify these three properties as they are already
> >> present in parent node "soc".
> >
> >   We do, otherwise dtc complains about defaults of #address-cells = 2 and #size-cells=1, and
> without empty "ranges" subnode's resources are not correctly translated.
> >
> 
> First of all this patch will not give this error. So this part should
> not be a part of this patch.
> You should be getting above error after applying v4 4/4 "ARM: dts: Add
> Ethernet chip to SMDK5410". So if its failing for ethernet subnode, you
> can move this address-cells and size-cells in dts file just above
> ethernet node as shown below:
> 
> index 311e7be..d69981d 100644
> --- a/arch/arm/boot/dts/exynos5410-smdk5410.dts
> +++ b/arch/arm/boot/dts/exynos5410-smdk5410.dts
> @@ -95,6 +95,9 @@
>   };
> 
>   &sromc {
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +       ranges;
>          pinctrl-names = "default";
>          pinctrl-0 = <&srom_ctl>, <&srom_ebi>;
> 

 IMHO this makes little of sense, because in this case you would have to duplicate these properties for every supported machine. address-cells, size-cells and ranges belong to the controller itself, not to its children.
 Of course i could split up the patch into two, first introduce sromc, second introduce mapping. But does it worth that? Both patches would be 3 lines of code each.
 Anyway, this specific talk is outdated by now, please check out neighbor topic, we are discussing significantly improved binding with Krzystof.

> And another question still remains open, we can't just like that change
> "smsc,lan9115" binding by adding samsung specific properties.

 I am sorry, but you either do not understand us with Krzystof, or not reading at all...
 This is not a change in the binding. And these properties do not belong to smsc at all. They would be added to *ANY* device which sits on top of SROMc. This is more like a bus-specific extra. It's similar to "reg" property having different meanings for different buses.

> Probably you can check suggestions from Krzysztof, where he pointed out
> some hint on how other places this is getting used.

 Did you read them? I did, and they do exactly the same thing as i do.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
diff mbox

Patch

--- a/arch/arm/boot/dts/exynos5410-smdk5410.dts
+++ b/arch/arm/boot/dts/exynos5410-smdk5410.dts
@@ -95,6 +95,9 @@ 
  };

  &sromc {
+       #address-cells = <1>;
+       #size-cells = <1>;
+       ranges;
         pinctrl-names = "default";
         pinctrl-0 = <&srom_ctl>, <&srom_ebi>;