2013/12/20 Kukjin Kim <kgene.kim@xxxxxxxxxxx>:
> 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@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
