[U-Boot] [PATCH 04/15] dm: pci: Provide friendly config access functions

[Simon Glass]

Hi Bin,

On 28 July 2015 at 01:47, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Tue, Jul 28, 2015 at 5:47 AM, Simon Glass <sjg at chromium.org> wrote:
>> At present there are no PCI functions which allow access to PCI
>> configuration using a struct udevice. This is a sad situation for driver
>> model as it makes use of PCI harder. Add these functions.
>>
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>> ---
>>
>>  drivers/pci/pci-uclass.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/pci.h            | 17 +++++++++++
>>  2 files changed, 93 insertions(+)
>>
>> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
>> index c7d93f9..8d3536c 100644
>> --- a/drivers/pci/pci-uclass.c
>> +++ b/drivers/pci/pci-uclass.c
>> @@ -210,6 +210,17 @@ int pci_write_config(pci_dev_t bdf, int offset, unsigned long value,
>>         return pci_bus_write_config(bus, bdf, offset, value, size);
>>  }
>>
>> +int dm_pci_write_config(struct udevice *dev, int offset, unsigned long value,
>> +                       enum pci_size_t size)
>> +{
>> +       struct udevice *bus;
>> +
>> +       for (bus = dev; device_get_uclass_id(bus->parent) == UCLASS_PCI;)
>> +               bus = bus->parent;
>
> I think we can simplify this by just doing:
>
> int dm_pci_write_config(struct udevice *dev, int offset, unsigned long value,
>                        enum pci_size_t size)
> {
>        return pci_write_config(pci_get_bdf(dev), offset, value, size);
> }

Yes but actually I want to remove those old functions. We should
support multiple PCI controllers and reference everything from a
device, I think.

>
>> +       return pci_bus_write_config(bus, pci_get_bdf(dev), offset, value, size);
>> +}
>> +
>> +
>>  int pci_write_config32(pci_dev_t bdf, int offset, u32 value)
>>  {
>>         return pci_write_config(bdf, offset, value, PCI_SIZE_32);
>> @@ -225,6 +236,21 @@ int pci_write_config8(pci_dev_t bdf, int offset, u8 value)
>>         return pci_write_config(bdf, offset, value, PCI_SIZE_8);
>>  }
>>
>> +int dm_pci_write_config8(struct udevice *dev, int offset, u8 value)
>> +{
>> +       return dm_pci_write_config(dev, offset, value, PCI_SIZE_8);
>> +}
>> +
>> +int dm_pci_write_config16(struct udevice *dev, int offset, u16 value)
>> +{
>> +       return dm_pci_write_config(dev, offset, value, PCI_SIZE_16);
>> +}
>> +
>> +int dm_pci_write_config32(struct udevice *dev, int offset, u32 value)
>> +{
>> +       return dm_pci_write_config(dev, offset, value, PCI_SIZE_32);
>> +}
>> +
>>  int pci_bus_read_config(struct udevice *bus, pci_dev_t bdf, int offset,
>>                         unsigned long *valuep, enum pci_size_t size)
>>  {
>> @@ -249,6 +275,17 @@ int pci_read_config(pci_dev_t bdf, int offset, unsigned long *valuep,
>>         return pci_bus_read_config(bus, bdf, offset, valuep, size);
>>  }
>>
>> +int dm_pci_read_config(struct udevice *dev, int offset, unsigned long *valuep,
>> +                      enum pci_size_t size)
>> +{
>> +       struct udevice *bus;
>> +
>> +       for (bus = dev; device_get_uclass_id(bus->parent) == UCLASS_PCI;)
>> +               bus = bus->parent;
>
> Ditto.
>
>> +       return pci_bus_read_config(bus, pci_get_bdf(dev), offset, valuep,
>> +                                  size);
>> +}
>> +
>>  int pci_read_config32(pci_dev_t bdf, int offset, u32 *valuep)
>>  {
>>         unsigned long value;
>> @@ -288,6 +325,45 @@ int pci_read_config8(pci_dev_t bdf, int offset, u8 *valuep)
>>         return 0;
>>  }
>>
>> +int dm_pci_read_config8(struct udevice *dev, int offset, u8 *valuep)
>> +{
>> +       unsigned long value;
>> +       int ret;
>> +
>> +       ret = dm_pci_read_config(dev, offset, &value, PCI_SIZE_8);
>> +       if (ret)
>> +               return ret;
>> +       *valuep = value;
>> +
>> +       return 0;
>> +}
>> +
>> +int dm_pci_read_config16(struct udevice *dev, int offset, u16 *valuep)
>> +{
>> +       unsigned long value;
>> +       int ret;
>> +
>> +       ret = dm_pci_read_config(dev, offset, &value, PCI_SIZE_16);
>> +       if (ret)
>> +               return ret;
>> +       *valuep = value;
>> +
>> +       return 0;
>> +}
>> +
>> +int dm_pci_read_config32(struct udevice *dev, int offset, u32 *valuep)
>> +{
>> +       unsigned long value;
>> +       int ret;
>> +
>> +       ret = dm_pci_read_config(dev, offset, &value, PCI_SIZE_32);
>> +       if (ret)
>> +               return ret;
>> +       *valuep = value;
>> +
>> +       return 0;
>> +}
>> +
>>  int pci_auto_config_devices(struct udevice *bus)
>>  {
>>         struct pci_controller *hose = bus->uclass_priv;
>> diff --git a/include/pci.h b/include/pci.h
>> index 26cd80b..0bb3090 100644
>> --- a/include/pci.h
>> +++ b/include/pci.h
>> @@ -947,6 +947,23 @@ int pci_bus_read_config(struct udevice *bus, pci_dev_t bdf, int offset,
>>  int pci_bus_write_config(struct udevice *bus, pci_dev_t bdf, int offset,
>>                          unsigned long value, enum pci_size_t size);
>>
>> +/**
>> + * Driver mode PCI config access functions. Use these in preference to others
>
> Driver model
>
>> + * when you have a valid device
>> + */
>> +int dm_pci_read_config(struct udevice *dev, int offset, unsigned long *valuep,
>> +                      enum pci_size_t size);
>> +int dm_pci_read_config8(struct udevice *dev, int offset, u8 *valuep);
>> +int dm_pci_read_config16(struct udevice *dev, int offset, u16 *valuep);
>> +int dm_pci_read_config32(struct udevice *dev, int offset, u32 *valuep);
>> +
>> +int dm_pci_write_config(struct udevice *dev, int offset, unsigned long value,
>> +                       enum pci_size_t size);
>> +
>
> Please either remove blank line here, or add a blank line between
> dm_pci_read_config() and dm_pci_read_config8() above.
>
>> +int dm_pci_write_config8(struct udevice *dev, int offset, u8 value);
>> +int dm_pci_write_config16(struct udevice *dev, int offset, u16 value);
>> +int dm_pci_write_config32(struct udevice *dev, int offset, u32 value);
>> +
>>  /*
>>   * The following functions provide access to the above without needing the
>>   * size parameter. We are trying to encourage the use of the 8/16/32-style
>> --
>
> Regards,
> Bin

Regards,
Simon