[U-Boot] [PATCH v1 1/3] drivers: pci_ep: Introduce UCLASS_PCI_EP uclass

[Simon Glass]

HI Ramon,

On Mon, 22 Apr 2019 at 10:33, Ramon Fried <ramon.fried at gmail.com> wrote:
>
>
> Hi Simon,
> Thanks for the review.
> please see inline, I have few questions/suggestions regarding
> your notes.
>
> Thanks,
> Ramon.
> On Mon, Apr 22, 2019 at 5:56 AM Simon Glass <sjg at chromium.org> wrote:
>>
>> Hi Ramon,
>>
>> On Fri, 5 Apr 2019 at 19:12, Ramon Fried <ramon.fried at gmail.com> wrote:
>> >
>> > Introduce new UCLASS_PCI_EP class for handling PCI endpoint
>> > devices, allowing to set various attributes of the PCI endpoint
>> > device, such as:
>> > * configuration space header
>> > * BAR definitions
>> > * outband memory mapping
>> > * start/stop PCI link
>> >
>> > Signed-off-by: Ramon Fried <ramon.fried at gmail.com>
>> >
>> > ---
>> >
>> >  drivers/Kconfig                      |   2 +
>> >  drivers/Makefile                     |   1 +
>> >  drivers/pci_endpoint/Kconfig         |  16 ++
>> >  drivers/pci_endpoint/Makefile        |   6 +
>> >  drivers/pci_endpoint/pci_ep-uclass.c | 192 ++++++++++++++
>> >  include/dm/uclass-id.h               |   1 +
>> >  include/pci_ep.h                     | 375 +++++++++++++++++++++++++++
>> >  7 files changed, 593 insertions(+)
>> >  create mode 100644 drivers/pci_endpoint/Kconfig
>> >  create mode 100644 drivers/pci_endpoint/Makefile
>> >  create mode 100644 drivers/pci_endpoint/pci_ep-uclass.c
>> >  create mode 100644 include/pci_ep.h
>>

[..]

>> > +int dm_pci_ep_set_bar(struct udevice *dev, u8 func_no,
>>
>> Please use uint for func_no. There is no need to limit it to 8 bits,
>> and this may not be efficient for non-8-bit machines. Please fix
>> globally.
>
> I tried to keep the API as similar as it can to the Linux API.
> I can change it, no problem, but IMHO I think it's better to keep it similar.

Hmm, why?

>> Perhaps you should declare a 'mask' variable? In any case, it is
>> confusing for the same variable to have two different meanings, and it
>> does not save code at run-time.
>
> as before, this is practically a copy-paste from Linux, I can change it, but I think it's clearer if the code looks the same as in Linux,
> might be easier the port patches like that.

It's a minor thing so you can keep the linux code if you like.

[..]

>> > +int dm_pci_ep_get_msix(struct udevice *dev, u8 func_no)
>> > +{
>> > +       struct dm_pci_ep_ops *ops = pci_ep_get_ops(dev);
>> > +       int interrupt;
>> > +
>> > +       if (!ops->get_msix)
>> > +               return -ENODEV;
>> > +
>> > +       interrupt = ops->get_msix(dev, func_no);
>> > +
>> > +       if (interrupt < 0)
>> > +               return 0;
>> > +
>> > +       return interrupt + 1;
>>
>> It's odd that your uclass function returns a different value from your
>> driver function. I think that will be confusing. Is it necessary?
>
> As you can understand from the code, 0 means 1 interrupt. basically the driver functions just return the msix field in the PCI registers,
> The translation to real number is done by the uclass wrapper.

OK, but why? Why not use the same return value for the drive methods
and the uclass? If you are using a different API, then please rename
one of the functions.

[..]

>> What is a DWORD? I think i is 32-bits, so just say that, since WORD is
>> a confusing term on a non-16-bit machine.
>
> hehe, I presume it's just a copy-paste from the PCI spec. I'll check, if so, I'll prefer to keep the original naming.

OK, then how about adding the length as well, since DWORD is pretty
obscure these days.

>
>
>>
>>
>> > + * @subsys_vendor_id: vendor of the add-in card or subsystem
>> > + * @subsys_id: id specific to vendor
>> > + * @interrupt_pin: interrupt pin the device (or device function) uses
>> > + */
>> > +struct pci_ep_header {
>> > +       u16     vendorid;
>> > +       u16     deviceid;
>> > +       u8      revid;
>> > +       u8      progif_code;
>> > +       u8      subclass_code;
>> > +       u8      baseclass_code;
>> > +       u8      cache_line_size;
>> > +       u16     subsys_vendor_id;
>> > +       u16     subsys_id;
>> > +       enum pci_interrupt_pin interrupt_pin;
>>
>> Shouldn't that be a u16 or something? The enum does not have a defined
>> size, I think.
>
> well, there can be only 4 different interrupt pins, so it doesn't matter as long as the rage
> is checked.
>

My concern is that if this is a hardware-mapped register, then the
enum could be any size, and may not map over the correct bits.

If this is not a hardware-mapped register, then OK.

[..]

>> > +       /**
>> > +        * set_msix() - set msix capability property
>> > +        *
>> > +        * set the number of required MSIx vectors the device
>> > +        * needs for operation.
>> > +        *
>> > +        * @dev:        device to set
>> > +        * @func_num:   Function to set
>> > +        * @interrupts: required interrupts count
>>
>> This is too vague, please expand it.
>
> You're referring to the set_msix() or the whole section,
> can you elaborate ?

I mean the interrupts line. Can you given example values perhaps? Does
it mean 'number of interrupts required to be alllocated' or something
like that?

[..]

Regards,
Simon