On Thu, Apr 25, 2019 at 2:44 AM Simon Glass <[email protected]> wrote:
> HI Ramon, > > On Mon, 22 Apr 2019 at 10:33, Ramon Fried <[email protected]> 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 <[email protected]> wrote: > >> > >> Hi Ramon, > >> > >> On Fri, 5 Apr 2019 at 19:12, Ramon Fried <[email protected]> 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 <[email protected]> > >> > > >> > --- > >> > > >> > 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 > _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
