From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kever Yang Date: Tue, 14 Feb 2017 09:09:01 +0800 Subject: [U-Boot] U-Boot of-platdata issue In-Reply-To: <2f81919c-b561-a708-5cca-dd4cfabfa724@samsung.com> References: <587C21A4.400@rock-chips.com> <58A17B29.1070600@rock-chips.com> <2f81919c-b561-a708-5cca-dd4cfabfa724@samsung.com> Message-ID: <58A258AD.7070004@rock-chips.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 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. Thanks, - Kever >>> Another option would be for dtoc to #define each compatible string to >>> the first one. If you think that would work, I could do a patch. >> I think we can try with the first one in the driver for of-platdata, >> this would have problem for the first compatible name is different but >> they share the same driver, like syscon. For syscon, you have resolved with >> a special dirver, not sure if other driver has the same problem. >> >> Could you help to make a patch for this solution? >> >> Thanks, >> - Kever >>> Regards, >>> Simon >>> >>> >>> >> >> _______________________________________________ >> U-Boot mailing list >> U-Boot at lists.denx.de >> http://lists.denx.de/mailman/listinfo/u-boot > > >