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

[Simon Glass]

Hi Kever,

On 6 June 2017 at 21:23, Kever Yang <kever.yang at rock-chips.com> wrote:
> 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.

OK, well if you think that will work it is fine with me. I will take a
look at the change.

Regards,
Simon

>
> 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
>
>
>