Re: [U-Boot] [PATCH 4/4] syscon: add Linux-compatible syscon API

[Simon Glass]

On 22 April 2018 at 22:58, Masahiro Yamada 
wrote:
> Hi Simon,
>
>
> 2018-04-23 5:11 GMT+09:00 Simon Glass :
>> Hi Masahiro,
>>
>> On 17 April 2018 at 20:38, Masahiro Yamada 
>> wrote:
>>> The syscon implementation in U-Boot is different from that in Linux.
>>> Thus, DT files imported from Linux do not work for U-Boot.
>>>
>>> In U-Boot driver model, each node is bound to a dedicated driver
>>> that is the most compatible to it.  This design gets along with the
>>> concept of DT, and the syscon in Linux originally worked like that.
>>>
>>> However, Linux commit bdb0066df96e ("mfd: syscon: Decouple syscon
>>> interface from platform devices") changed the behavior because it is
>>> useful to let a device bind to another driver, but still works as a
>>> syscon provider.
>>>
>>> That change had happened before U-Boot initially supported the syscon
>>> driver by commit 6f98b7504f70 ("dm: Add support for generic system
>>> controllers (syscon)").  So, the U-Boot's syscon works differently
>>> from the beginning.  I'd say this is mis-implementation given that
>>> DT is not oriented to a particular project, but Linux is the canon
>>> of DT in practice.
>>>
>>> The problem typically arises in the combination of "syscon" and
>>> "simple-mfd" compatibles.
>>>
>>> In Linux, they are orthogonal, i.e., the order between "syscon" and
>>> "simple-mfd" does not matter at all.
>>>
>>> Assume the following compatible.
>>>
>>>compatible = "foo,bar-syscon", "syscon", "simple-mfd";
>>>
>>> In U-Boot, this device node is bound to the syscon driver
>>> (driver/core/syscon-uclass.c) since the "syscon" is found to be the
>>> most compatible.  Then, syscon_get_regmap() succeeds.
>>>
>>> However,
>>>
>>>compatible = "foo,bar-syscon", "simple-mfd", "syscon";
>>>
>>> does not work because this node is bound to the simple-bus driver
>>> (drivers/core/simple-bus.c) in the favor of "simple-mfd" compatible.
>>> The compatible string "syscon" is just dismissed.
>>>
>>> Moreover,
>>>
>>>compatible = "foo,bar-syscon", "syscon";
>>>
>>> works like the first case because the syscon driver populates the
>>> child devices.  This is wrong because populating children is the job
>>> of "simple-mfd" (or "simple-bus").
>>>
>>> This commit ports syscon_node_to_regmap() from Linux.  This API
>>> does not require the given node to be bound to a driver in any way.
>>>
>>> Reported-by: Kunihiko Hayashi 
>>> Signed-off-by: Masahiro Yamada 
>>> ---
>>>
>>>  drivers/core/syscon-uclass.c | 63
>> 
>>>  include/syscon.h |  8 ++
>>>  2 files changed, 71 insertions(+)
>>
>> I was not aware of this change in how Linux worked, but it makes sense.
>>
>> Please can you add a test for this?
>
>
> I do not believe a missing test would block this patch,
> but I volunteered to add it (as a separate patch).
>
> http://patchwork.ozlabs.org/patch/902733/

Thanks!
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 4/4] syscon: add Linux-compatible syscon API

[Masahiro Yamada]

Hi Simon,


2018-04-23 5:11 GMT+09:00 Simon Glass :
> Hi Masahiro,
>
> On 17 April 2018 at 20:38, Masahiro Yamada 
> wrote:
>> The syscon implementation in U-Boot is different from that in Linux.
>> Thus, DT files imported from Linux do not work for U-Boot.
>>
>> In U-Boot driver model, each node is bound to a dedicated driver
>> that is the most compatible to it.  This design gets along with the
>> concept of DT, and the syscon in Linux originally worked like that.
>>
>> However, Linux commit bdb0066df96e ("mfd: syscon: Decouple syscon
>> interface from platform devices") changed the behavior because it is
>> useful to let a device bind to another driver, but still works as a
>> syscon provider.
>>
>> That change had happened before U-Boot initially supported the syscon
>> driver by commit 6f98b7504f70 ("dm: Add support for generic system
>> controllers (syscon)").  So, the U-Boot's syscon works differently
>> from the beginning.  I'd say this is mis-implementation given that
>> DT is not oriented to a particular project, but Linux is the canon
>> of DT in practice.
>>
>> The problem typically arises in the combination of "syscon" and
>> "simple-mfd" compatibles.
>>
>> In Linux, they are orthogonal, i.e., the order between "syscon" and
>> "simple-mfd" does not matter at all.
>>
>> Assume the following compatible.
>>
>>compatible = "foo,bar-syscon", "syscon", "simple-mfd";
>>
>> In U-Boot, this device node is bound to the syscon driver
>> (driver/core/syscon-uclass.c) since the "syscon" is found to be the
>> most compatible.  Then, syscon_get_regmap() succeeds.
>>
>> However,
>>
>>compatible = "foo,bar-syscon", "simple-mfd", "syscon";
>>
>> does not work because this node is bound to the simple-bus driver
>> (drivers/core/simple-bus.c) in the favor of "simple-mfd" compatible.
>> The compatible string "syscon" is just dismissed.
>>
>> Moreover,
>>
>>compatible = "foo,bar-syscon", "syscon";
>>
>> works like the first case because the syscon driver populates the
>> child devices.  This is wrong because populating children is the job
>> of "simple-mfd" (or "simple-bus").
>>
>> This commit ports syscon_node_to_regmap() from Linux.  This API
>> does not require the given node to be bound to a driver in any way.
>>
>> Reported-by: Kunihiko Hayashi 
>> Signed-off-by: Masahiro Yamada 
>> ---
>>
>>  drivers/core/syscon-uclass.c | 63
> 
>>  include/syscon.h |  8 ++
>>  2 files changed, 71 insertions(+)
>
> I was not aware of this change in how Linux worked, but it makes sense.
>
> Please can you add a test for this?


I do not believe a missing test would block this patch,
but I volunteered to add it (as a separate patch).

http://patchwork.ozlabs.org/patch/902733/




-- 
Best Regards
Masahiro Yamada
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 4/4] syscon: add Linux-compatible syscon API

[Simon Glass]

Hi Masahiro,

On 17 April 2018 at 20:38, Masahiro Yamada 
wrote:
> The syscon implementation in U-Boot is different from that in Linux.
> Thus, DT files imported from Linux do not work for U-Boot.
>
> In U-Boot driver model, each node is bound to a dedicated driver
> that is the most compatible to it.  This design gets along with the
> concept of DT, and the syscon in Linux originally worked like that.
>
> However, Linux commit bdb0066df96e ("mfd: syscon: Decouple syscon
> interface from platform devices") changed the behavior because it is
> useful to let a device bind to another driver, but still works as a
> syscon provider.
>
> That change had happened before U-Boot initially supported the syscon
> driver by commit 6f98b7504f70 ("dm: Add support for generic system
> controllers (syscon)").  So, the U-Boot's syscon works differently
> from the beginning.  I'd say this is mis-implementation given that
> DT is not oriented to a particular project, but Linux is the canon
> of DT in practice.
>
> The problem typically arises in the combination of "syscon" and
> "simple-mfd" compatibles.
>
> In Linux, they are orthogonal, i.e., the order between "syscon" and
> "simple-mfd" does not matter at all.
>
> Assume the following compatible.
>
>compatible = "foo,bar-syscon", "syscon", "simple-mfd";
>
> In U-Boot, this device node is bound to the syscon driver
> (driver/core/syscon-uclass.c) since the "syscon" is found to be the
> most compatible.  Then, syscon_get_regmap() succeeds.
>
> However,
>
>compatible = "foo,bar-syscon", "simple-mfd", "syscon";
>
> does not work because this node is bound to the simple-bus driver
> (drivers/core/simple-bus.c) in the favor of "simple-mfd" compatible.
> The compatible string "syscon" is just dismissed.
>
> Moreover,
>
>compatible = "foo,bar-syscon", "syscon";
>
> works like the first case because the syscon driver populates the
> child devices.  This is wrong because populating children is the job
> of "simple-mfd" (or "simple-bus").
>
> This commit ports syscon_node_to_regmap() from Linux.  This API
> does not require the given node to be bound to a driver in any way.
>
> Reported-by: Kunihiko Hayashi 
> Signed-off-by: Masahiro Yamada 
> ---
>
>  drivers/core/syscon-uclass.c | 63

>  include/syscon.h |  8 ++
>  2 files changed, 71 insertions(+)

I was not aware of this change in how Linux worked, but it makes sense.

Please can you add a test for this?

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot