[U-Boot] [PATCH 08/13] rockchip: mmc: support rk3066 mmc

[Kever Yang]

Simon,


On 06/07/2017 05:10 AM, Simon Glass wrote:
> Hi Pawel,
>
> On 6 June 2017 at 12:51, Paweł Jarosz <paweljarosz3691 at gmail.com> wrote:
>> rk3066 and rk3288 mmc designware ip's are very similiar. They differ in
>> internal dma support and max driver frequency.
>>
>> Signed-off-by: Paweł Jarosz <paweljarosz3691 at gmail.com>
>> ---
>>   drivers/mmc/rockchip_dw_mmc.c | 31 +++++++++++++++++++++++++++++--
>>   1 file changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/rockchip_dw_mmc.c b/drivers/mmc/rockchip_dw_mmc.c
>> index 25a21e2..d94c395 100644
>> --- a/drivers/mmc/rockchip_dw_mmc.c
>> +++ b/drivers/mmc/rockchip_dw_mmc.c
>> @@ -22,8 +22,14 @@ DECLARE_GLOBAL_DATA_PTR;
>>
>>   struct rockchip_mmc_plat {
>>   #if CONFIG_IS_ENABLED(OF_PLATDATA)
>> +
>> +#ifdef CONFIG_ROCKCHIP_RK3066
>> +       struct dtd_rockchip_rk2928_dw_mshc dtplat;
>> +#else
>>          struct dtd_rockchip_rk3288_dw_mshc dtplat;
>>   #endif
>> +
>> +#endif
>>          struct mmc_config cfg;
>>          struct mmc mmc;
>>   };
>> @@ -109,8 +115,11 @@ static int rockchip_dwmmc_probe(struct udevice *dev)
>>          int ret;
>>
>>   #if CONFIG_IS_ENABLED(OF_PLATDATA)
>> +#ifdef CONFIG_ROCKCHIP_RK3066
>> +       struct dtd_rockchip_rk2928_dw_mshc *dtplat = &plat->dtplat;
>> +#else
>>          struct dtd_rockchip_rk3288_dw_mshc *dtplat = &plat->dtplat;
>> -
>> +#endif
> I am not keen on this - it will get ugly. Can you please do this:
>
> Create a new driver for rk3288 which just has the platdata stuff,
> rockchip_dwmmc_ofdata_to_platdata() and the U_BOOT_DRIVER().
> Everything else should remain in this file.
>
> Then in a new patch, create a driver for rk3066 which uses the same
> common elements from rockchip_dw_mmc.c

I think I have discuss this with you online or offline  for many times,
when OF_PLATADATA enabled, the dts is pre-compile by dtoc and then
there is structure like 'dtd_rockchip_rk2928_dw_mshc' which is from the dts
node compatible name, I propose to use the last compatible name in dtoc
instead of the first one, then we can get the same structure name and used
in drivers.

I still not enable the dwmmc when OF_PLATDATA enabled on rk3399 because
of the same reason.

I think we should fix this from the root cause, but not make different 
driver
for different SoCs.

Thanks,
- Kever
>
>>          host->name = dev->name;
>>          host->ioaddr = map_sysmem(dtplat->reg[0], dtplat->reg[1]);
>>          host->buswidth = dtplat->bus_width;
>> @@ -118,7 +127,12 @@ static int rockchip_dwmmc_probe(struct udevice *dev)
>>          host->priv = dev;
>>          host->dev_index = 0;
>>          priv->fifo_depth = dtplat->fifo_depth;
>> +
>> +#ifdef CONFIG_ROCKCHIP_RK3066
>> +       priv->fifo_mode = 1;
>> +#else
>>          priv->fifo_mode = 0;
>> +#endif
> What about the fifo_mode property in the DT?
>
> If you have to hard-code this you should use a .data parameter in
> rockchip_dwmmc_ids (e.g. RK2928, RK3288) and use that to determine the
> mode. But hopefully the DT is enough.
>
> For OF_PLATDATA however I suggest you have a different probe() which
> either sets up this value and then calls rockchip_dwmmc_probe(), or
> add it as a parameter to rockchip_dwmmc_probe().
>
>>          memcpy(priv->minmax, dtplat->clock_freq_min_max, sizeof(priv->minmax));
>>
>>          ret = clk_get_by_index_platdata(dev, 0, dtplat->clocks, &priv->clk);
>> @@ -162,14 +176,27 @@ static int rockchip_dwmmc_bind(struct udevice *dev)
>>   }
>>
>>   static const struct udevice_id rockchip_dwmmc_ids[] = {
>> +       { .compatible = "rockchip,rk2928-dw-mshc" },
>>          { .compatible = "rockchip,rk3288-dw-mshc" },
>>          { }
>>   };
>>
>> +U_BOOT_DRIVER(rockchip_rk2928_dw_mshc) = {
>> +       .name           = "rockchip_rk2928_dw_mshc",
>> +       .id             = UCLASS_MMC,
>> +       .of_match       = rockchip_dwmmc_ids,
>> +       .ofdata_to_platdata = rockchip_dwmmc_ofdata_to_platdata,
>> +       .ops            = &dm_dwmci_ops,
>> +       .bind           = rockchip_dwmmc_bind,
>> +       .probe          = rockchip_dwmmc_probe,
>> +       .priv_auto_alloc_size = sizeof(struct rockchip_dwmmc_priv),
>> +       .platdata_auto_alloc_size = sizeof(struct rockchip_mmc_plat),
>> +};
>> +
>>   U_BOOT_DRIVER(rockchip_dwmmc_drv) = {
>>          .name           = "rockchip_rk3288_dw_mshc",
>>          .id             = UCLASS_MMC,
>> -       .of_match       = rockchip_dwmmc_ids,
> I think you ne
>>          .ofdata_to_platdata = rockchip_dwmmc_ofdata_to_platdata,
>>          .ops            = &dm_dwmci_ops,
>>          .bind           = rockchip_dwmmc_bind,
>> --
>> 2.7.4
>>
> Regards,
> Simon
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot