Hi Michal,
On 1 December 2015 at 00:08, Michal Simek <michal.si...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
