[U-Boot] U-Boot of-platdata issue

[Simon Glass]

Hi Kever,

On 13 February 2017 at 18:09, Kever Yang <kever.yang at rock-chips.com> wrote:
> Hi Simon, Jaehoon,
>
>
> On 02/13/2017 05:51 PM, Jaehoon Chung wrote:
>>
>> On 02/13/2017 06:23 PM, Kever Yang wrote:
>>>
>>> Hi Simon,
>>>
>>> On 01/16/2017 12:15 PM, Simon Glass wrote:
>>>>
>>>> Hi Kever,
>>>>
>>>> On 15 January 2017 at 18:28, Kever Yang <kever.yang at rock-chips.com>
>>>> wrote:
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>>       I met two issue when using of-platdata
>>>>>
>>>>> 1. compitable name with '.'
>>>>> I get compile error as below:
>>>>> In file included from include/dt-structs.h:16:0,
>>>>>                    from spl/dts/dt-platdata.c:3:
>>>>> include/generated/dt-structs.h:26:35: error: expected identifier or ‘(’
>>>>> before numeric constant
>>>>>    struct dtd_rockchip_rk3399_sdhci_5.1 {
>>>>>                                      ^
>>>>> spl/dts/dt-platdata.c:41:42: error: expected identifier or ‘(’ before
>>>>> numeric constant
>>>>>    static struct dtd_rockchip_rk3399_sdhci_5.1 dtv_sdhci_at_fe330000 =
>>>>> {
>>>>>                                             ^
>>>>> spl/dts/dt-platdata.c:55:15: error: ‘dtv_sdhci_at_fe330000’ undeclared
>>>>> here
>>>>> (not in a function)
>>>>>     .platdata = &dtv_sdhci_at_fe330000,
>>>>>                  ^
>>>>> make[2]: *** [spl/dts/dt-platdata.o] Error 1
>>>>> make[1]: *** [spl/u-boot-spl] Error 2
>>>>> make: *** [__build_one_by_one] Error 2
>>>>>
>>>>> The dts node starts like this:
>>>>>           sdhci: sdhci at fe330000 {
>>>>>                   u-boot,dm-pre-reloc;
>>>>>                   compatible = "rockchip,rk3399-sdhci-5.1",
>>>>> "arasan,sdhci-5.1";
>>>>> ...
>>>>
>>>> That just involves replacing '.' with '_'. I sent a patch.
>>>>
>>>>> 2. multi compatible name
>>>>> When a dts node have more than one compatible name, which is prefer to
>>>>> use?
>>>>> for example, we have two dwmmc compatible name in rk3399, the tool is
>>>>> using
>>>>> the first one,
>>>>> while the source code using the last one.
>>>>>
>>>>> "drivers/mmc/rockchip_dw_mmc.c"
>>>>>    23 struct rockchip_mmc_plat {
>>>>>    24 #if CONFIG_IS_ENABLED(OF_PLATDATA)
>>>>>    25         struct dtd_rockchip_rk3288_dw_mshc dtplat;
>>>>>    26 #endif
>>>>>    27         struct mmc_config cfg;
>>>>>    28         struct mmc mmc;
>>>>>    29 };
>>>>> ...
>>>>> dts node
>>>>>           sdmmc: dwmmc at fe320000 {
>>>>>                  compatible = "rockchip,rk3399-dw-mshc",
>>>>>                                "rockchip,rk3288-dw-mshc";
>>>>
>>>> I'm not sure of the best solution here (other than putting more
>>>> on-chip SRAM in your devices hint hint :-)
>>>>
>>>> One option is something like:
>>>>
>>>> struct rockchip_mmc_plat {
>>>> #if CONFIG_IS_ENABLED(OF_PLATDATA)
>>>> #ifdef CONFIG_ROCKCHIP_RK3288
>>>>           struct dtd_rockchip_rk3288_dw_mshc dtplat;
>>>> #elif defined(CONFIG_ROCKCHIP_RK399)
>>>>           struct dtd_rockchip_rk399_dw_mshc dtplat;
>>>> #endif
>>>> #endif
>>>>
>>>> Obviously we don't want that as it is putting SoC-specific stuff in the
>>>> driver.
>>>>
>>>> IMO the compatible strings are being misused a bit. Can there not be a
>>>> compatible string which is common to all rockchip devices which use
>>>> this IP? Something like "rockchip,dw-mshc-v1"? Then you can avoid
>>>> adding a new compatible string every time you use the same IP in a
>>>> device.
>>>
>>> Agree, but... this is from kernel, we can't control it unless all kernel
>>> maintainers
>>> have the same idea.
>>
>> does it use just "rockchip,dw-mshc"? Maybe this can be common compatible
>> for rockchip.
>> If it needs add the other compatible in future, it should be added the
>> specific compatible at that time.
>>
>
> I don't think we will have a change in dts compatible for U-Boot dts,
> because we will always using dts
> file from kernel, so we will use it as is.
>
> We can use "rockchip,rk3288-dw-mshc" for rk3288 and rk3399, here is the
> document.
> and for dw-mshc:
> Documentation/devicetree/bindings/mmc/rockchip-dw-mshc.txt
> * compatible: should be
>         - "rockchip,rk2928-dw-mshc": for Rockchip RK2928 and following,
>                                                         before RK3288
>         - "rockchip,rk3288-dw-mshc": for Rockchip RK3288
>         - "rockchip,rk3399-dw-mshc", "rockchip,rk3288-dw-mshc": for Rockchip
> RK3399
>
> For the compatible name, there had some discuss before, like this patch:
> https://lists.gt.net/linux/kernel/2372182
>
> So for the of-platdata, we can use "rockchip,rk3288-dw-mshc" to generate the
> structure.

So in this case, as you saying that you need dtoc to #define the
structs to be the same? Or can you do this yourself in a header file?

 [..]

Regards,
Simon