[U-Boot] [PATCH v2 09/11] net: designware: Add support to PCI designware devices

[Joe Hershberger]

Hi Simon,

On Tue, Sep 8, 2015 at 9:22 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi Joe,
>
> On 4 September 2015 at 23:57, Bin Meng <bmeng.cn at gmail.com> wrote:
>> Hi Joe,
>>
>> On Fri, Sep 4, 2015 at 11:29 PM, Joe Hershberger
>> <joe.hershberger at gmail.com> wrote:
>>> Hi Bin,
>>>
>>> On Wed, Sep 2, 2015 at 4:17 AM, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>> The Designware ethernet controller is also seen on PCI bus, e.g.
>>>> on Intel Quark SoC. Add this support in the DM version driver.
>>>>
>>>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>>>
>>> Looks good. One question below.
>>>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - Change to use device_is_on_pci_bus()
>>>>
>>>>  drivers/net/designware.c | 39 +++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 39 insertions(+)
>>>>
>>>> diff --git a/drivers/net/designware.c b/drivers/net/designware.c
>>>> index ae78d21..39a9e6b 100644
>>>> --- a/drivers/net/designware.c
>>>> +++ b/drivers/net/designware.c
>>>> @@ -14,6 +14,7 @@
>>>>  #include <errno.h>
>>>>  #include <miiphy.h>
>>>>  #include <malloc.h>
>>>> +#include <pci.h>
>>>>  #include <linux/compiler.h>
>>>>  #include <linux/err.h>
>>>>  #include <asm/io.h>
>>>> @@ -558,6 +559,20 @@ static int designware_eth_write_hwaddr(struct udevice *dev)
>>>>         return _dw_write_hwaddr(priv, pdata->enetaddr);
>>>>  }
>>>>
>>>> +static int designware_eth_bind(struct udevice *dev)
>>>> +{
>>>> +       static int num_cards;
>>>> +       char name[20];
>>>> +
>>>> +       /* Create a unique device name for PCI type devices */
>>>> +       if (device_is_on_pci_bus(dev)) {
>>>> +               sprintf(name, "eth_designware#%u", num_cards++);
>>>> +               device_set_name(dev, name);
>>>> +       }
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>>  static int designware_eth_probe(struct udevice *dev)
>>>>  {
>>>>         struct eth_pdata *pdata = dev_get_platdata(dev);
>>>> @@ -565,6 +580,22 @@ static int designware_eth_probe(struct udevice *dev)
>>>>         u32 iobase = pdata->iobase;
>>>>         int ret;
>>>>
>>>> +       /*
>>>> +        * If we are on PCI bus, either directly attached to a PCI root port,
>>>> +        * or via a PCI bridge, fill in platdata before we probe the hardware.
>>>> +        */
>>>> +       if (device_is_on_pci_bus(dev)) {
>>>> +               pci_dev_t bdf;
>>>> +
>>>> +               bdf = pci_get_bdf(dev);
>>>> +               pci_read_config_dword(bdf, PCI_BASE_ADDRESS_0, &iobase);
>>>> +               iobase &= PCI_BASE_ADDRESS_MEM_MASK;
>>>> +               iobase = pci_mem_to_phys(bdf, iobase);
>>>> +
>>>> +               pdata->iobase = iobase;
>>>> +               pdata->phy_interface = PHY_INTERFACE_MODE_RMII;
>>>
>>> Is this just assumed for all PCI devices? Seems like it could vary,
>>> but I don't know the specifics of all the implementations this is
>>> expected to support. Should it be part of a quirk based on PID or
>>> something?
>>
>> I think it could vary, but given so far the driver only supports Intel
>> Quark PCI variant, so I did not put a test logic against PCI
>> vendor/device ID here. When in the future we support another PCI
>> variant, we can add such logic.
>>
>
> Does this sound OK to you? I'd like to apply this series to u-boot-x86
> but want to make sure you are happy with it.

This is fine. It will be up to the next developer to design the way to
select this attribute. I'm guessing this is the first driver of its
kind (PCI - different PHY connections) so there isn't anything to
model after yet.

>>>
>>>> +       }
>>>> +
>>>>         debug("%s, iobase=%x, priv=%p\n", __func__, iobase, priv);
>>>>         priv->mac_regs_p = (struct eth_mac_regs *)iobase;
>>>>         priv->dma_regs_p = (struct eth_dma_regs *)(iobase + DW_DMA_BASE_OFFSET);
>>>> @@ -617,10 +648,18 @@ U_BOOT_DRIVER(eth_designware) = {
>>>>         .id     = UCLASS_ETH,
>>>>         .of_match = designware_eth_ids,
>>>>         .ofdata_to_platdata = designware_eth_ofdata_to_platdata,
>>>> +       .bind   = designware_eth_bind,
>>>>         .probe  = designware_eth_probe,
>>>>         .ops    = &designware_eth_ops,
>>>>         .priv_auto_alloc_size = sizeof(struct dw_eth_dev),
>>>>         .platdata_auto_alloc_size = sizeof(struct eth_pdata),
>>>>         .flags = DM_FLAG_ALLOC_PRIV_DMA,
>>>>  };
>>>> +
>>>> +static struct pci_device_id supported[] = {
>>>> +       { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_QRK_EMAC) },
>>>> +       { }
>>>> +};
>>>> +
>>>> +U_BOOT_PCI_DEVICE(eth_designware, supported);
>>>>  #endif
>>>> --
>>
>> Regards,
>> Bin
>
>
> Regards,
> Simon