On Thu, Apr 25, 2019 at 2:44 AM Simon Glass sjg@chromium.org wrote:
HI Ramon,
On Mon, 22 Apr 2019 at 10:33, Ramon Fried ramon.fried@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@chromium.org wrote:
Hi Ramon,
On Fri, 5 Apr 2019 at 19:12, Ramon Fried ramon.fried@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@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 countThis 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