[U-Boot] [PATCH 12/14] net: gem: Move driver to DM

[Simon Glass]

Hi Michal,

On 1 December 2015 at 00:08, Michal Simek <michal.simek at xilinx.com> wrote:
> Hi Simon,
>
> On 1.12.2015 00:17, Simon Glass wrote:
>> Hi Michal,
>>
>
> ...
>
>>> diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c
>>> index e169b774932a..5ea992a3ce65 100644
>>> --- a/drivers/mmc/zynq_sdhci.c
>>> +++ b/drivers/mmc/zynq_sdhci.c
>>> @@ -33,6 +33,23 @@ int zynq_sdhci_init(phys_addr_t regbase)
>>>         return 0;
>>>  }
>>>
>>> +
>>> +
>>> +static const struct udevice_id arasan_sdhci_ids[] = {
>>> +       { .compatible = "arasan,sdhci-8.9a" },
>>> +       { }
>>> +};
>>> +
>>> +U_BOOT_DRIVER(arasan_sdhci_drv) = {
>>> +       .name           = "rockchip_dwmmc",
>>> +       .id             = UCLASS_MMC,
>>> +       .of_match       = rockchip_dwmmc_ids,
>>> +       .ofdata_to_platdata = rockchip_dwmmc_ofdata_to_platdata,
>>> +       .probe          = rockchip_dwmmc_probe,
>>> +       .priv_auto_alloc_size = sizeof(struct rockchip_dwmmc_priv),
>>> +};
>>> +
>>> +
>>
>> This seems unrelated / also rockchip stuff.
>
> I reported it in my reply that this was added by accident. As you know I
> am playing with SD DM and rockchip was that guy I use for inspiration.
>
>
>>
>>>  #if CONFIG_IS_ENABLED(OF_CONTROL)
>>>  int zynq_sdhci_of_init(const void *blob)
>>>  {
>>> diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c
>>> index 4e93707c7ab1..f2a14938036f 100644
>>> --- a/drivers/net/zynq_gem.c
>>> +++ b/drivers/net/zynq_gem.c
>>> @@ -13,6 +13,7 @@
>>>  #include <net.h>
>>>  #include <netdev.h>
>>>  #include <config.h>
>>> +#include <dm.h>
>>
>> Can you put <dm.h> up higher so that these are in order?
>
> sure
>
> ...
>
>>>
>>> -       miiphy_register(dev->name, zynq_gem_miiphy_read, zynq_gem_miiphy_write);
>>> -       priv->bus = miiphy_get_dev_by_name(dev->name);
>>> +static int zynq_gem_ofdata_to_platdata(struct udevice *dev)
>>> +{
>>> +       struct eth_pdata *pdata = dev_get_platdata(dev);
>>> +       struct zynq_gem_priv *priv = dev_get_priv(dev);
>>> +       int offset = 0;
>>>
>>> -       ret = zynq_phy_init(dev);
>>> -       if (ret)
>>> -               return ret;
>>> +       pdata->iobase = (phys_addr_t)dev_get_addr(dev);
>>> +       priv->iobase = (struct zynq_gem_regs *)dev_get_addr(dev);
>>
>> Better to use:
>>
>> priv->iobase = (struct zynq_gem_regs *)pdata->iobase
>>
>> I think. But is pdata->iobase ever used?
>
> That was one think I wanted to check. There is eth_pdata structure which
> has iobase, enetaddr and phy_interface.
>
> I do fill them here but driver is using iobase saved in private
> structure. I do need more information from private structure that's why
> I don't need to load it from pdata structure.
>
> I probably also miss to allocate pdata. Is this required?
> .platdata_auto_alloc_size = sizeof(struct eth_pdata)
>
> Can you please check logic around pdata if I use it right?

The platform data is supposed to survive probe()/remove() cycles. You
can avoid using it altogether if don't need this feature. You are
setting it up your ofdata_to_platdata() function which is correct. You
can use that platdata directly in your driver if you like, or copy it
to the private data (dev_get_priv()). I typically put the address in
the platdata and then convert this to a pointer and store it in priv,
in probe(). But there is no hard-and-fast rule.

But if you do create platdata, you should use it :-)

>
>
>>> +       /* Hardcode for now */
>>> +       priv->emio = 0;
>>> +
>>> +       offset = fdtdec_lookup_phandle(gd->fdt_blob, dev->of_offset,
>>> +                                      "phy-handle");
>>> +       if (offset != -1)
>>
>> I think this should be:
>>
>> offset > 0
>
> ok. Will fix it.
>
> Thanks,
> Michal

Regards,
Simon