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

[Ramon Fried]

On Thu, Apr 25, 2019 at 2:44 AM Simon Glass <sjg at chromium.org> wrote:

> 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?
>
Nevermind, I already changed my mind :)

>
> >> 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.
>
I'm here advocating for the Linux implementation, you're right, I'll change
it.

>
> [..]
>
> >> 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.
>
OK, will do.

>
> >
> >
> >>
> >>
> >> > + * @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.
>
Not mapped, phew.

>
> [..]
>
> >> > +       /**
> >> > +        * 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?
>
It's basically a request from the endpoint, it might not be fully fulfilled
by the host,
the endpoint needs to check later using get_msix() how many interrupts the
host allocated for him.
I'll add documentation for that.

>
> [..]
>
> Regards,
> Simon
>