* RE: [PATCH 1/2] ARM: dts: Enable RTC node in exynos4.dtsi file
[not found] <1382438215-13215-1-git-send-email-sachin.kamat@linaro.org>
@ 2013-11-12 11:01 ` Kukjin Kim
2013-11-13 3:31 ` Sachin Kamat
0 siblings, 1 reply; 10+ messages in thread
From: Kukjin Kim @ 2013-11-12 11:01 UTC (permalink / raw)
To: 'Sachin Kamat', linux-samsung-soc; +Cc: devicetree
Sachin Kamat wrote:
>
> Since there are no board specific properties for the RTC node,
> keep it enabled in the dtsi file.
>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> ---
> arch/arm/boot/dts/exynos4.dtsi | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/exynos4.dtsi
> b/arch/arm/boot/dts/exynos4.dtsi
> index a73eeb5..e262946 100644
> --- a/arch/arm/boot/dts/exynos4.dtsi
> +++ b/arch/arm/boot/dts/exynos4.dtsi
> @@ -205,7 +205,6 @@
> interrupts = <0 44 0>, <0 45 0>;
> clocks = <&clock 346>;
> clock-names = "rtc";
> - status = "disabled";
> };
>
> keypad@100A0000 {
> --
> 1.7.9.5
Well, I don't think every exynos4 boards want to make the default
enabled...so I don't want to take this. If any opinions, please let me know.
Thanks,
Kukjin
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ARM: dts: Enable RTC node in exynos4.dtsi file
2013-11-12 11:01 ` [PATCH 1/2] ARM: dts: Enable RTC node in exynos4.dtsi file Kukjin Kim
@ 2013-11-13 3:31 ` Sachin Kamat
2013-11-13 8:08 ` Tomasz Figa
0 siblings, 1 reply; 10+ messages in thread
From: Sachin Kamat @ 2013-11-13 3:31 UTC (permalink / raw)
To: Kukjin Kim; +Cc: linux-samsung-soc, devicetree@vger.kernel.org
Hi Kukjin,
On 12 November 2013 16:31, Kukjin Kim <kgene@kernel.org> wrote:
> Sachin Kamat wrote:
>>
>> Since there are no board specific properties for the RTC node,
>> keep it enabled in the dtsi file.
>>
>> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
>> ---
>> arch/arm/boot/dts/exynos4.dtsi | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/exynos4.dtsi
>> b/arch/arm/boot/dts/exynos4.dtsi
>> index a73eeb5..e262946 100644
>> --- a/arch/arm/boot/dts/exynos4.dtsi
>> +++ b/arch/arm/boot/dts/exynos4.dtsi
>> @@ -205,7 +205,6 @@
>> interrupts = <0 44 0>, <0 45 0>;
>> clocks = <&clock 346>;
>> clock-names = "rtc";
>> - status = "disabled";
>> };
>>
>> keypad@100A0000 {
>> --
>> 1.7.9.5
>
> Well, I don't think every exynos4 boards want to make the default
> enabled...so I don't want to take this. If any opinions, please let me know.
>
As was discussed earlier too, status field of DT node is not supposed
to be used for
keeping an IP enabled or disabled. That should be done via the kernel
config. The DT status
is mostly to indicate the hardware status of the IP on the SoC/board.
If the node fully defines the hardware,
then it should be kept enabled by default unless such enabling causes
some issues with other IPs due to
pin sharing conflicts, etc. In the above case the node completely
defines the hardware and hence there is no
reason to keep it disabled.
--
With warm regards,
Sachin
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ARM: dts: Enable RTC node in exynos4.dtsi file
2013-11-13 3:31 ` Sachin Kamat
@ 2013-11-13 8:08 ` Tomasz Figa
2013-11-13 10:27 ` Sylwester Nawrocki
0 siblings, 1 reply; 10+ messages in thread
From: Tomasz Figa @ 2013-11-13 8:08 UTC (permalink / raw)
To: Sachin Kamat; +Cc: Kukjin Kim, linux-samsung-soc, devicetree@vger.kernel.org
On Wednesday 13 of November 2013 09:01:36 Sachin Kamat wrote:
> Hi Kukjin,
>
> On 12 November 2013 16:31, Kukjin Kim <kgene@kernel.org> wrote:
> > Sachin Kamat wrote:
> >>
> >> Since there are no board specific properties for the RTC node,
> >> keep it enabled in the dtsi file.
> >>
> >> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> >> ---
> >> arch/arm/boot/dts/exynos4.dtsi | 1 -
> >> 1 file changed, 1 deletion(-)
> >>
> >> diff --git a/arch/arm/boot/dts/exynos4.dtsi
> >> b/arch/arm/boot/dts/exynos4.dtsi
> >> index a73eeb5..e262946 100644
> >> --- a/arch/arm/boot/dts/exynos4.dtsi
> >> +++ b/arch/arm/boot/dts/exynos4.dtsi
> >> @@ -205,7 +205,6 @@
> >> interrupts = <0 44 0>, <0 45 0>;
> >> clocks = <&clock 346>;
> >> clock-names = "rtc";
> >> - status = "disabled";
> >> };
> >>
> >> keypad@100A0000 {
> >> --
> >> 1.7.9.5
> >
> > Well, I don't think every exynos4 boards want to make the default
> > enabled...so I don't want to take this. If any opinions, please let me know.
> >
>
> As was discussed earlier too, status field of DT node is not supposed
> to be used for
> keeping an IP enabled or disabled. That should be done via the kernel
> config. The DT status
> is mostly to indicate the hardware status of the IP on the SoC/board.
> If the node fully defines the hardware,
> then it should be kept enabled by default unless such enabling causes
> some issues with other IPs due to
> pin sharing conflicts, etc. In the above case the node completely
> defines the hardware and hence there is no
> reason to keep it disabled.
That's correct. (Unless I'm missing some board specific dependency of RTC.
If so, please correct me.)
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ARM: dts: Enable RTC node in exynos4.dtsi file
2013-11-13 8:08 ` Tomasz Figa
@ 2013-11-13 10:27 ` Sylwester Nawrocki
2013-11-13 11:52 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 10+ messages in thread
From: Sylwester Nawrocki @ 2013-11-13 10:27 UTC (permalink / raw)
To: Tomasz Figa, Sachin Kamat
Cc: Kukjin Kim, linux-samsung-soc, devicetree@vger.kernel.org,
Stephen Warren
On 13/11/13 09:08, Tomasz Figa wrote:
>> As was discussed earlier too, status field of DT node is not supposed
>> > to be used for
>> > keeping an IP enabled or disabled. That should be done via the kernel
>> > config. The DT status
>> > is mostly to indicate the hardware status of the IP on the SoC/board.
>> > If the node fully defines the hardware,
>> > then it should be kept enabled by default unless such enabling causes
>> > some issues with other IPs due to
>> > pin sharing conflicts, etc. In the above case the node completely
>> > defines the hardware and hence there is no
>> > reason to keep it disabled.
>
> That's correct. (Unless I'm missing some board specific dependency of RTC.
> If so, please correct me.)
I don't really like this argument. Why not allow the firmware to decide
which devices are relevant and should be handled by the kernel ?
And since we are aiming at single kernel config, if I understand things
correctly, I can't see anything else than dts that could hold the machine
*configuration*.
So let's not make all stuff enabled by default, that's not something we
want on those mobile device SoCs. We should not be making fine system
tuning more difficult than necessary.
I'm with Kukjin on this matter and would prefer patches like the $subject
patch not be merged.
--
Thanks,
Sylwester
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ARM: dts: Enable RTC node in exynos4.dtsi file
2013-11-13 10:27 ` Sylwester Nawrocki
@ 2013-11-13 11:52 ` Bartlomiej Zolnierkiewicz
2013-11-13 12:21 ` Tomasz Figa
0 siblings, 1 reply; 10+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2013-11-13 11:52 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Tomasz Figa, Sachin Kamat, Kukjin Kim, linux-samsung-soc,
devicetree@vger.kernel.org, Stephen Warren
Hi,
On Wednesday, November 13, 2013 11:27:03 AM Sylwester Nawrocki wrote:
> On 13/11/13 09:08, Tomasz Figa wrote:
> >> As was discussed earlier too, status field of DT node is not supposed
> >> > to be used for
> >> > keeping an IP enabled or disabled. That should be done via the kernel
> >> > config. The DT status
> >> > is mostly to indicate the hardware status of the IP on the SoC/board.
> >> > If the node fully defines the hardware,
> >> > then it should be kept enabled by default unless such enabling causes
> >> > some issues with other IPs due to
> >> > pin sharing conflicts, etc. In the above case the node completely
> >> > defines the hardware and hence there is no
> >> > reason to keep it disabled.
> >
> > That's correct. (Unless I'm missing some board specific dependency of RTC.
> > If so, please correct me.)
>
> I don't really like this argument. Why not allow the firmware to decide
> which devices are relevant and should be handled by the kernel ?
> And since we are aiming at single kernel config, if I understand things
> correctly, I can't see anything else than dts that could hold the machine
> *configuration*.
>
> So let's not make all stuff enabled by default, that's not something we
> want on those mobile device SoCs. We should not be making fine system
> tuning more difficult than necessary.
>
> I'm with Kukjin on this matter and would prefer patches like the $subject
> patch not be merged.
I generally agree with Sylwester and Kukjin that devices should not be
enabled by default in dtsi files. However in a particular case of RTC
support there should be an exception from the generic rule and RTC
should be enabled for all EXYNOS boards (we have RTC driver config
option already enabled in our exynos_defconfig and we are also already
enabling RTC device explicitly in EXYNOS5250 dtsi file).
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ARM: dts: Enable RTC node in exynos4.dtsi file
2013-11-13 11:52 ` Bartlomiej Zolnierkiewicz
@ 2013-11-13 12:21 ` Tomasz Figa
2013-12-10 9:07 ` Sachin Kamat
0 siblings, 1 reply; 10+ messages in thread
From: Tomasz Figa @ 2013-11-13 12:21 UTC (permalink / raw)
To: Bartlomiej Zolnierkiewicz
Cc: Sylwester Nawrocki, Sachin Kamat, Kukjin Kim, linux-samsung-soc,
devicetree@vger.kernel.org, Stephen Warren, Mark Rutland,
Grant Likely, Kumar Gala, Pawel Moll, Rob Herring
On Wednesday 13 of November 2013 12:52:05 Bartlomiej Zolnierkiewicz wrote:
[+ DT maintainers]
>
> Hi,
>
> On Wednesday, November 13, 2013 11:27:03 AM Sylwester Nawrocki wrote:
> > On 13/11/13 09:08, Tomasz Figa wrote:
> > >> As was discussed earlier too, status field of DT node is not supposed
> > >> > to be used for
> > >> > keeping an IP enabled or disabled. That should be done via the kernel
> > >> > config. The DT status
> > >> > is mostly to indicate the hardware status of the IP on the SoC/board.
> > >> > If the node fully defines the hardware,
> > >> > then it should be kept enabled by default unless such enabling causes
> > >> > some issues with other IPs due to
> > >> > pin sharing conflicts, etc. In the above case the node completely
> > >> > defines the hardware and hence there is no
> > >> > reason to keep it disabled.
> > >
> > > That's correct. (Unless I'm missing some board specific dependency of RTC.
> > > If so, please correct me.)
Well, I was missing one indeed. RTC requires an external 32.768KHz clock,
which must be provided by an oscillator or any other clock source on the
board. However...
> >
> > I don't really like this argument. Why not allow the firmware to decide
> > which devices are relevant and should be handled by the kernel ?
> > And since we are aiming at single kernel config, if I understand things
> > correctly, I can't see anything else than dts that could hold the machine
> > *configuration*.
> >
> > So let's not make all stuff enabled by default, that's not something we
> > want on those mobile device SoCs. We should not be making fine system
> > tuning more difficult than necessary.
> >
> > I'm with Kukjin on this matter and would prefer patches like the $subject
> > patch not be merged.
>
> I generally agree with Sylwester and Kukjin that devices should not be
> enabled by default in dtsi files. However in a particular case of RTC
> support there should be an exception from the generic rule and RTC
> should be enabled for all EXYNOS boards (we have RTC driver config
> option already enabled in our exynos_defconfig and we are also already
> enabling RTC device explicitly in EXYNOS5250 dtsi file).
I believe the rule is clear and we already went through enough discussion
about this. To clarify again:
Device Tree should not restrict possible use cases of particular hardware
that are allowed by particular integration of such hardware on particular
board. This means that if there are no technical reasons to mark such
hardware as disabled on particular board, this should not be done. Let
me show you this on several examples:
1. SoC X has an MMC controller, which needs N pins of the SoC to be
routed to an MMC slot. Our board X1 does _not_ have necessary traces on
its PCB. In this case the MMC controller must be marked as disabled.
2. SoC X has an MMC controller, which needs N pins of the SoC to be
routed to an MMC slot. Our board X2 _does_ have necessary traces on its
PCB, so when user inserts an MMC card into the slot he expects that the
system detects it. In this case the MMC controller must _not_ be marked
as disabled.
3. SoC X has a built-in 2D graphics accelerator. It does not have any
output pads nor any requirements with respect to the board - it's purely
a mem-to-mem device. A user might want to run a rootfs that uses it to
accelerate his applications, so this device must _not_ be marked as
disabled.
4. SoC X has a image processing block, consisting of two functions:
- a camera interface, that requires SoC pads to be connected to a camera
sensor,
- general-purpose image scaler and format converter, that can operate
either on input of camera interface or on images stored in memory.
This case is more interesting. Even if board does not have a physical
camera interface, the binding should be designed in a way that allows
enabling only the memory-to-memory scaler. Then such IP must be marked
okay regardless of board support, because the camera interface will be
used only if relevant board-specific properties are provided.
To sum up, I would not interpret the "disabled" value of "status" property
as the opposite of "enabled", but rather as "disabled" in "disabled
person".
Anyway, I'd like to get a confirmation (or denial) from other Device Tree
maintainers and if what I've written above is correct, maybe we should
put this somewhere in kernel documentation.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ARM: dts: Enable RTC node in exynos4.dtsi file
2013-11-13 12:21 ` Tomasz Figa
@ 2013-12-10 9:07 ` Sachin Kamat
2013-12-18 15:55 ` Tomasz Figa
0 siblings, 1 reply; 10+ messages in thread
From: Sachin Kamat @ 2013-12-10 9:07 UTC (permalink / raw)
To: Tomasz Figa
Cc: Bartlomiej Zolnierkiewicz, Sylwester Nawrocki, Kukjin Kim,
linux-samsung-soc, devicetree@vger.kernel.org, Stephen Warren,
Mark Rutland, Grant Likely, Kumar Gala, Pawel Moll, Rob Herring
On 13 November 2013 17:51, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On Wednesday 13 of November 2013 12:52:05 Bartlomiej Zolnierkiewicz wrote:
>
> [+ DT maintainers]
>
>>
>> Hi,
>>
>> On Wednesday, November 13, 2013 11:27:03 AM Sylwester Nawrocki wrote:
>> > On 13/11/13 09:08, Tomasz Figa wrote:
>> > >> As was discussed earlier too, status field of DT node is not supposed
>> > >> > to be used for
>> > >> > keeping an IP enabled or disabled. That should be done via the kernel
>> > >> > config. The DT status
>> > >> > is mostly to indicate the hardware status of the IP on the SoC/board.
>> > >> > If the node fully defines the hardware,
>> > >> > then it should be kept enabled by default unless such enabling causes
>> > >> > some issues with other IPs due to
>> > >> > pin sharing conflicts, etc. In the above case the node completely
>> > >> > defines the hardware and hence there is no
>> > >> > reason to keep it disabled.
>> > >
>> > > That's correct. (Unless I'm missing some board specific dependency of RTC.
>> > > If so, please correct me.)
>
> Well, I was missing one indeed. RTC requires an external 32.768KHz clock,
> which must be provided by an oscillator or any other clock source on the
> board. However...
>
>> >
>> > I don't really like this argument. Why not allow the firmware to decide
>> > which devices are relevant and should be handled by the kernel ?
>> > And since we are aiming at single kernel config, if I understand things
>> > correctly, I can't see anything else than dts that could hold the machine
>> > *configuration*.
>> >
>> > So let's not make all stuff enabled by default, that's not something we
>> > want on those mobile device SoCs. We should not be making fine system
>> > tuning more difficult than necessary.
>> >
>> > I'm with Kukjin on this matter and would prefer patches like the $subject
>> > patch not be merged.
>>
>> I generally agree with Sylwester and Kukjin that devices should not be
>> enabled by default in dtsi files. However in a particular case of RTC
>> support there should be an exception from the generic rule and RTC
>> should be enabled for all EXYNOS boards (we have RTC driver config
>> option already enabled in our exynos_defconfig and we are also already
>> enabling RTC device explicitly in EXYNOS5250 dtsi file).
>
> I believe the rule is clear and we already went through enough discussion
> about this. To clarify again:
>
> Device Tree should not restrict possible use cases of particular hardware
> that are allowed by particular integration of such hardware on particular
> board. This means that if there are no technical reasons to mark such
> hardware as disabled on particular board, this should not be done. Let
> me show you this on several examples:
>
> 1. SoC X has an MMC controller, which needs N pins of the SoC to be
> routed to an MMC slot. Our board X1 does _not_ have necessary traces on
> its PCB. In this case the MMC controller must be marked as disabled.
>
> 2. SoC X has an MMC controller, which needs N pins of the SoC to be
> routed to an MMC slot. Our board X2 _does_ have necessary traces on its
> PCB, so when user inserts an MMC card into the slot he expects that the
> system detects it. In this case the MMC controller must _not_ be marked
> as disabled.
>
> 3. SoC X has a built-in 2D graphics accelerator. It does not have any
> output pads nor any requirements with respect to the board - it's purely
> a mem-to-mem device. A user might want to run a rootfs that uses it to
> accelerate his applications, so this device must _not_ be marked as
> disabled.
>
> 4. SoC X has a image processing block, consisting of two functions:
> - a camera interface, that requires SoC pads to be connected to a camera
> sensor,
> - general-purpose image scaler and format converter, that can operate
> either on input of camera interface or on images stored in memory.
> This case is more interesting. Even if board does not have a physical
> camera interface, the binding should be designed in a way that allows
> enabling only the memory-to-memory scaler. Then such IP must be marked
> okay regardless of board support, because the camera interface will be
> used only if relevant board-specific properties are provided.
>
> To sum up, I would not interpret the "disabled" value of "status" property
> as the opposite of "enabled", but rather as "disabled" in "disabled
> person".
>
> Anyway, I'd like to get a confirmation (or denial) from other Device Tree
> maintainers and if what I've written above is correct, maybe we should
> put this somewhere in kernel documentation.
Ping..
In the absence of kernel documentation formulating the guidelines for
enabling/disabling
of IP (nodes), the current code should be the reference for someone
doing this for a given
platform. However, the current code (Exynos DT files) itself is not
consistent one way or the
other w.r.t to the above points. For e.g. commit 73784475febf ("ARM:
dts: Update the "status"
property of RTC DT node for Exynos5250 SoC") does the exact same thing
done by this patch.
Similarly in exynos5420.dtsi. I am sure there are other similar
examples as well. I think there should be
one consistent guideline, atleast for accepting patches of this nature
(for this platform).
--
With warm regards,
Sachin
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ARM: dts: Enable RTC node in exynos4.dtsi file
2013-12-10 9:07 ` Sachin Kamat
@ 2013-12-18 15:55 ` Tomasz Figa
2013-12-20 21:48 ` Kukjin Kim
0 siblings, 1 reply; 10+ messages in thread
From: Tomasz Figa @ 2013-12-18 15:55 UTC (permalink / raw)
To: Sachin Kamat
Cc: Tomasz Figa, Bartlomiej Zolnierkiewicz, Sylwester Nawrocki,
Kukjin Kim, linux-samsung-soc, devicetree@vger.kernel.org,
Stephen Warren, Mark Rutland, Grant Likely, Kumar Gala,
Pawel Moll, Rob Herring
On Tuesday 10 of December 2013 14:37:13 Sachin Kamat wrote:
> On 13 November 2013 17:51, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> > On Wednesday 13 of November 2013 12:52:05 Bartlomiej Zolnierkiewicz wrote:
> >
> > [+ DT maintainers]
> >
> >>
> >> Hi,
> >>
> >> On Wednesday, November 13, 2013 11:27:03 AM Sylwester Nawrocki wrote:
> >> > On 13/11/13 09:08, Tomasz Figa wrote:
> >> > >> As was discussed earlier too, status field of DT node is not supposed
> >> > >> > to be used for
> >> > >> > keeping an IP enabled or disabled. That should be done via the kernel
> >> > >> > config. The DT status
> >> > >> > is mostly to indicate the hardware status of the IP on the SoC/board.
> >> > >> > If the node fully defines the hardware,
> >> > >> > then it should be kept enabled by default unless such enabling causes
> >> > >> > some issues with other IPs due to
> >> > >> > pin sharing conflicts, etc. In the above case the node completely
> >> > >> > defines the hardware and hence there is no
> >> > >> > reason to keep it disabled.
> >> > >
> >> > > That's correct. (Unless I'm missing some board specific dependency of RTC.
> >> > > If so, please correct me.)
> >
> > Well, I was missing one indeed. RTC requires an external 32.768KHz clock,
> > which must be provided by an oscillator or any other clock source on the
> > board. However...
> >
> >> >
> >> > I don't really like this argument. Why not allow the firmware to decide
> >> > which devices are relevant and should be handled by the kernel ?
> >> > And since we are aiming at single kernel config, if I understand things
> >> > correctly, I can't see anything else than dts that could hold the machine
> >> > *configuration*.
> >> >
> >> > So let's not make all stuff enabled by default, that's not something we
> >> > want on those mobile device SoCs. We should not be making fine system
> >> > tuning more difficult than necessary.
> >> >
> >> > I'm with Kukjin on this matter and would prefer patches like the $subject
> >> > patch not be merged.
> >>
> >> I generally agree with Sylwester and Kukjin that devices should not be
> >> enabled by default in dtsi files. However in a particular case of RTC
> >> support there should be an exception from the generic rule and RTC
> >> should be enabled for all EXYNOS boards (we have RTC driver config
> >> option already enabled in our exynos_defconfig and we are also already
> >> enabling RTC device explicitly in EXYNOS5250 dtsi file).
> >
> > I believe the rule is clear and we already went through enough discussion
> > about this. To clarify again:
> >
> > Device Tree should not restrict possible use cases of particular hardware
> > that are allowed by particular integration of such hardware on particular
> > board. This means that if there are no technical reasons to mark such
> > hardware as disabled on particular board, this should not be done. Let
> > me show you this on several examples:
> >
> > 1. SoC X has an MMC controller, which needs N pins of the SoC to be
> > routed to an MMC slot. Our board X1 does _not_ have necessary traces on
> > its PCB. In this case the MMC controller must be marked as disabled.
> >
> > 2. SoC X has an MMC controller, which needs N pins of the SoC to be
> > routed to an MMC slot. Our board X2 _does_ have necessary traces on its
> > PCB, so when user inserts an MMC card into the slot he expects that the
> > system detects it. In this case the MMC controller must _not_ be marked
> > as disabled.
> >
> > 3. SoC X has a built-in 2D graphics accelerator. It does not have any
> > output pads nor any requirements with respect to the board - it's purely
> > a mem-to-mem device. A user might want to run a rootfs that uses it to
> > accelerate his applications, so this device must _not_ be marked as
> > disabled.
> >
> > 4. SoC X has a image processing block, consisting of two functions:
> > - a camera interface, that requires SoC pads to be connected to a camera
> > sensor,
> > - general-purpose image scaler and format converter, that can operate
> > either on input of camera interface or on images stored in memory.
> > This case is more interesting. Even if board does not have a physical
> > camera interface, the binding should be designed in a way that allows
> > enabling only the memory-to-memory scaler. Then such IP must be marked
> > okay regardless of board support, because the camera interface will be
> > used only if relevant board-specific properties are provided.
> >
> > To sum up, I would not interpret the "disabled" value of "status" property
> > as the opposite of "enabled", but rather as "disabled" in "disabled
> > person".
> >
> > Anyway, I'd like to get a confirmation (or denial) from other Device Tree
> > maintainers and if what I've written above is correct, maybe we should
> > put this somewhere in kernel documentation.
>
> Ping..
>
> In the absence of kernel documentation formulating the guidelines for
> enabling/disabling
> of IP (nodes), the current code should be the reference for someone
> doing this for a given
> platform. However, the current code (Exynos DT files) itself is not
> consistent one way or the
> other w.r.t to the above points. For e.g. commit 73784475febf ("ARM:
> dts: Update the "status"
> property of RTC DT node for Exynos5250 SoC") does the exact same thing
> done by this patch.
> Similarly in exynos5420.dtsi. I am sure there are other similar
> examples as well. I think there should be
> one consistent guideline, atleast for accepting patches of this nature
> (for this platform).
Yes, there should be a document somewhere describing such guidelines.
I believe it's already being written by some people, though.
(Mark, Stephen?)
For this patch alone and earlier patches doing the same, we missed the
fact that some boards might not have RTC xtal, so RTC shouldn't really be
enabled by default. This isn't really anything that can't be changed
later, though, and I believe we should make this consistent for all Exynos
SoC dtsi files.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ARM: dts: Enable RTC node in exynos4.dtsi file
2013-12-18 15:55 ` Tomasz Figa
@ 2013-12-20 21:48 ` Kukjin Kim
2013-12-20 22:07 ` Tomasz Figa
0 siblings, 1 reply; 10+ messages in thread
From: Kukjin Kim @ 2013-12-20 21:48 UTC (permalink / raw)
To: Tomasz Figa
Cc: Sachin Kamat, Tomasz Figa, Bartlomiej Zolnierkiewicz,
Sylwester Nawrocki, Kukjin Kim, linux-samsung-soc,
devicetree@vger.kernel.org, Stephen Warren, Mark Rutland,
Grant Likely, Kumar Gala, Pawel Moll, Rob Herring
On 12/19/13 00:55, Tomasz Figa wrote:
> On Tuesday 10 of December 2013 14:37:13 Sachin Kamat wrote:
>> On 13 November 2013 17:51, Tomasz Figa<tomasz.figa@gmail.com> wrote:
>>> On Wednesday 13 of November 2013 12:52:05 Bartlomiej Zolnierkiewicz wrote:
>>>
>>> [+ DT maintainers]
>>>
>>>>
>>>> Hi,
>>>>
>>>> On Wednesday, November 13, 2013 11:27:03 AM Sylwester Nawrocki wrote:
>>>>> On 13/11/13 09:08, Tomasz Figa wrote:
>>>>>>> As was discussed earlier too, status field of DT node is not supposed
>>>>>>>> to be used for
>>>>>>>> keeping an IP enabled or disabled. That should be done via the kernel
>>>>>>>> config. The DT status
>>>>>>>> is mostly to indicate the hardware status of the IP on the SoC/board.
>>>>>>>> If the node fully defines the hardware,
>>>>>>>> then it should be kept enabled by default unless such enabling causes
>>>>>>>> some issues with other IPs due to
>>>>>>>> pin sharing conflicts, etc. In the above case the node completely
>>>>>>>> defines the hardware and hence there is no
>>>>>>>> reason to keep it disabled.
>>>>>>
>>>>>> That's correct. (Unless I'm missing some board specific dependency of RTC.
>>>>>> If so, please correct me.)
>>>
>>> Well, I was missing one indeed. RTC requires an external 32.768KHz clock,
>>> which must be provided by an oscillator or any other clock source on the
>>> board. However...
>>>
>>>>>
>>>>> I don't really like this argument. Why not allow the firmware to decide
>>>>> which devices are relevant and should be handled by the kernel ?
>>>>> And since we are aiming at single kernel config, if I understand things
>>>>> correctly, I can't see anything else than dts that could hold the machine
>>>>> *configuration*.
>>>>>
>>>>> So let's not make all stuff enabled by default, that's not something we
>>>>> want on those mobile device SoCs. We should not be making fine system
>>>>> tuning more difficult than necessary.
>>>>>
>>>>> I'm with Kukjin on this matter and would prefer patches like the $subject
>>>>> patch not be merged.
>>>>
>>>> I generally agree with Sylwester and Kukjin that devices should not be
>>>> enabled by default in dtsi files. However in a particular case of RTC
>>>> support there should be an exception from the generic rule and RTC
>>>> should be enabled for all EXYNOS boards (we have RTC driver config
>>>> option already enabled in our exynos_defconfig and we are also already
>>>> enabling RTC device explicitly in EXYNOS5250 dtsi file).
>>>
>>> I believe the rule is clear and we already went through enough discussion
>>> about this. To clarify again:
>>>
>>> Device Tree should not restrict possible use cases of particular hardware
>>> that are allowed by particular integration of such hardware on particular
>>> board. This means that if there are no technical reasons to mark such
>>> hardware as disabled on particular board, this should not be done. Let
>>> me show you this on several examples:
>>>
>>> 1. SoC X has an MMC controller, which needs N pins of the SoC to be
>>> routed to an MMC slot. Our board X1 does _not_ have necessary traces on
>>> its PCB. In this case the MMC controller must be marked as disabled.
>>>
>>> 2. SoC X has an MMC controller, which needs N pins of the SoC to be
>>> routed to an MMC slot. Our board X2 _does_ have necessary traces on its
>>> PCB, so when user inserts an MMC card into the slot he expects that the
>>> system detects it. In this case the MMC controller must _not_ be marked
>>> as disabled.
>>>
>>> 3. SoC X has a built-in 2D graphics accelerator. It does not have any
>>> output pads nor any requirements with respect to the board - it's purely
>>> a mem-to-mem device. A user might want to run a rootfs that uses it to
>>> accelerate his applications, so this device must _not_ be marked as
>>> disabled.
>>>
>>> 4. SoC X has a image processing block, consisting of two functions:
>>> - a camera interface, that requires SoC pads to be connected to a camera
>>> sensor,
>>> - general-purpose image scaler and format converter, that can operate
>>> either on input of camera interface or on images stored in memory.
>>> This case is more interesting. Even if board does not have a physical
>>> camera interface, the binding should be designed in a way that allows
>>> enabling only the memory-to-memory scaler. Then such IP must be marked
>>> okay regardless of board support, because the camera interface will be
>>> used only if relevant board-specific properties are provided.
>>>
>>> To sum up, I would not interpret the "disabled" value of "status" property
>>> as the opposite of "enabled", but rather as "disabled" in "disabled
>>> person".
>>>
>>> Anyway, I'd like to get a confirmation (or denial) from other Device Tree
>>> maintainers and if what I've written above is correct, maybe we should
>>> put this somewhere in kernel documentation.
>>
>> Ping..
>>
>> In the absence of kernel documentation formulating the guidelines for
>> enabling/disabling
>> of IP (nodes), the current code should be the reference for someone
>> doing this for a given
>> platform. However, the current code (Exynos DT files) itself is not
>> consistent one way or the
>> other w.r.t to the above points. For e.g. commit 73784475febf ("ARM:
>> dts: Update the "status"
>> property of RTC DT node for Exynos5250 SoC") does the exact same thing
>> done by this patch.
>> Similarly in exynos5420.dtsi. I am sure there are other similar
>> examples as well. I think there should be
>> one consistent guideline, atleast for accepting patches of this nature
>> (for this platform).
>
> Yes, there should be a document somewhere describing such guidelines.
> I believe it's already being written by some people, though.
> (Mark, Stephen?)
>
> For this patch alone and earlier patches doing the same, we missed the
> fact that some boards might not have RTC xtal, so RTC shouldn't really be
> enabled by default. This isn't really anything that can't be changed
> later, though, and I believe we should make this consistent for all Exynos
> SoC dtsi files.
>
Yes, we need to make a common rule about that for exynos stuff and I
think we could do for 3.15... from maybe mid of Jan...
Thanks,
Kukjin
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] ARM: dts: Enable RTC node in exynos4.dtsi file
2013-12-20 21:48 ` Kukjin Kim
@ 2013-12-20 22:07 ` Tomasz Figa
0 siblings, 0 replies; 10+ messages in thread
From: Tomasz Figa @ 2013-12-20 22:07 UTC (permalink / raw)
To: Kukjin Kim
Cc: Tomasz Figa, Sachin Kamat, Bartlomiej Zolnierkiewicz,
Sylwester Nawrocki, Kukjin Kim, linux-samsung-soc,
devicetree@vger.kernel.org, Stephen Warren, Mark Rutland,
Grant Likely, Kumar Gala, Pawel Moll, Rob Herring
2013/12/20 Kukjin Kim <kgene.kim@samsung.com>:
> On 12/19/13 00:55, Tomasz Figa wrote:
>>
>> On Tuesday 10 of December 2013 14:37:13 Sachin Kamat wrote:
>>>
>>> On 13 November 2013 17:51, Tomasz Figa<tomasz.figa@gmail.com> wrote:
>>>>
>>>> On Wednesday 13 of November 2013 12:52:05 Bartlomiej Zolnierkiewicz
>>>> wrote:
>>>>
>>>> [+ DT maintainers]
>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> On Wednesday, November 13, 2013 11:27:03 AM Sylwester Nawrocki wrote:
>>>>>>
>>>>>> On 13/11/13 09:08, Tomasz Figa wrote:
>>>>>>>>
>>>>>>>> As was discussed earlier too, status field of DT node is not
>>>>>>>> supposed
>>>>>>>>>
>>>>>>>>> to be used for
>>>>>>>>> keeping an IP enabled or disabled. That should be done via the
>>>>>>>>> kernel
>>>>>>>>> config. The DT status
>>>>>>>>> is mostly to indicate the hardware status of the IP on the
>>>>>>>>> SoC/board.
>>>>>>>>> If the node fully defines the hardware,
>>>>>>>>> then it should be kept enabled by default unless such enabling
>>>>>>>>> causes
>>>>>>>>> some issues with other IPs due to
>>>>>>>>> pin sharing conflicts, etc. In the above case the node completely
>>>>>>>>> defines the hardware and hence there is no
>>>>>>>>> reason to keep it disabled.
>>>>>>>
>>>>>>>
>>>>>>> That's correct. (Unless I'm missing some board specific dependency of
>>>>>>> RTC.
>>>>>>> If so, please correct me.)
>>>>
>>>>
>>>> Well, I was missing one indeed. RTC requires an external 32.768KHz
>>>> clock,
>>>> which must be provided by an oscillator or any other clock source on the
>>>> board. However...
>>>>
>>>>>>
>>>>>> I don't really like this argument. Why not allow the firmware to
>>>>>> decide
>>>>>> which devices are relevant and should be handled by the kernel ?
>>>>>> And since we are aiming at single kernel config, if I understand
>>>>>> things
>>>>>> correctly, I can't see anything else than dts that could hold the
>>>>>> machine
>>>>>> *configuration*.
>>>>>>
>>>>>> So let's not make all stuff enabled by default, that's not something
>>>>>> we
>>>>>> want on those mobile device SoCs. We should not be making fine system
>>>>>> tuning more difficult than necessary.
>>>>>>
>>>>>> I'm with Kukjin on this matter and would prefer patches like the
>>>>>> $subject
>>>>>> patch not be merged.
>>>>>
>>>>>
>>>>> I generally agree with Sylwester and Kukjin that devices should not be
>>>>> enabled by default in dtsi files. However in a particular case of RTC
>>>>> support there should be an exception from the generic rule and RTC
>>>>> should be enabled for all EXYNOS boards (we have RTC driver config
>>>>> option already enabled in our exynos_defconfig and we are also already
>>>>> enabling RTC device explicitly in EXYNOS5250 dtsi file).
>>>>
>>>>
>>>> I believe the rule is clear and we already went through enough
>>>> discussion
>>>> about this. To clarify again:
>>>>
>>>> Device Tree should not restrict possible use cases of particular
>>>> hardware
>>>> that are allowed by particular integration of such hardware on
>>>> particular
>>>> board. This means that if there are no technical reasons to mark such
>>>> hardware as disabled on particular board, this should not be done. Let
>>>> me show you this on several examples:
>>>>
>>>> 1. SoC X has an MMC controller, which needs N pins of the SoC to be
>>>> routed to an MMC slot. Our board X1 does _not_ have necessary traces on
>>>> its PCB. In this case the MMC controller must be marked as disabled.
>>>>
>>>> 2. SoC X has an MMC controller, which needs N pins of the SoC to be
>>>> routed to an MMC slot. Our board X2 _does_ have necessary traces on its
>>>> PCB, so when user inserts an MMC card into the slot he expects that the
>>>> system detects it. In this case the MMC controller must _not_ be marked
>>>> as disabled.
>>>>
>>>> 3. SoC X has a built-in 2D graphics accelerator. It does not have any
>>>> output pads nor any requirements with respect to the board - it's purely
>>>> a mem-to-mem device. A user might want to run a rootfs that uses it to
>>>> accelerate his applications, so this device must _not_ be marked as
>>>> disabled.
>>>>
>>>> 4. SoC X has a image processing block, consisting of two functions:
>>>> - a camera interface, that requires SoC pads to be connected to a
>>>> camera
>>>> sensor,
>>>> - general-purpose image scaler and format converter, that can operate
>>>> either on input of camera interface or on images stored in memory.
>>>> This case is more interesting. Even if board does not have a physical
>>>> camera interface, the binding should be designed in a way that allows
>>>> enabling only the memory-to-memory scaler. Then such IP must be marked
>>>> okay regardless of board support, because the camera interface will be
>>>> used only if relevant board-specific properties are provided.
>>>>
>>>> To sum up, I would not interpret the "disabled" value of "status"
>>>> property
>>>> as the opposite of "enabled", but rather as "disabled" in "disabled
>>>> person".
>>>>
>>>> Anyway, I'd like to get a confirmation (or denial) from other Device
>>>> Tree
>>>> maintainers and if what I've written above is correct, maybe we should
>>>> put this somewhere in kernel documentation.
>>>
>>>
>>> Ping..
>>>
>>> In the absence of kernel documentation formulating the guidelines for
>>> enabling/disabling
>>> of IP (nodes), the current code should be the reference for someone
>>> doing this for a given
>>> platform. However, the current code (Exynos DT files) itself is not
>>> consistent one way or the
>>> other w.r.t to the above points. For e.g. commit 73784475febf ("ARM:
>>> dts: Update the "status"
>>> property of RTC DT node for Exynos5250 SoC") does the exact same thing
>>> done by this patch.
>>> Similarly in exynos5420.dtsi. I am sure there are other similar
>>> examples as well. I think there should be
>>> one consistent guideline, atleast for accepting patches of this nature
>>> (for this platform).
>>
>>
>> Yes, there should be a document somewhere describing such guidelines.
>> I believe it's already being written by some people, though.
>> (Mark, Stephen?)
>>
>> For this patch alone and earlier patches doing the same, we missed the
>> fact that some boards might not have RTC xtal, so RTC shouldn't really be
>> enabled by default. This isn't really anything that can't be changed
>> later, though, and I believe we should make this consistent for all Exynos
>> SoC dtsi files.
>>
>
> Yes, we need to make a common rule about that for exynos stuff and I think
> we could do for 3.15... from maybe mid of Jan...
Hmm, I think most of the devices follow the convention already. We can
recheck for any inconsistencies again, though.
By the way, Merry Christmas.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-12-20 22:07 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1382438215-13215-1-git-send-email-sachin.kamat@linaro.org>
2013-11-12 11:01 ` [PATCH 1/2] ARM: dts: Enable RTC node in exynos4.dtsi file Kukjin Kim
2013-11-13 3:31 ` Sachin Kamat
2013-11-13 8:08 ` Tomasz Figa
2013-11-13 10:27 ` Sylwester Nawrocki
2013-11-13 11:52 ` Bartlomiej Zolnierkiewicz
2013-11-13 12:21 ` Tomasz Figa
2013-12-10 9:07 ` Sachin Kamat
2013-12-18 15:55 ` Tomasz Figa
2013-12-20 21:48 ` Kukjin Kim
2013-12-20 22:07 ` Tomasz Figa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).