This patch is the driver for NTB hardware in AMD SoCs (ported from Linux) and enables the NTB infrastructure like Doorbells, Scratchpads and Memory window in AMD SoC. This driver has been validated using ntb_transport and if_ntb driver already available in FreeBSD.
I didn't really look at this driver thoroughly and don't plan to, but just skimmed through and pointed out some possible improvements.
Is there a public datasheet for this part or is this device spec NDA'd?
It seems like this needs to incorporate nc->dbmask (the value provided by hints) still. I'm not sure what the point of the hinting system is, though, you'd have to ask Mav.
style nit: a space goes between if and (.
Usual style for indentation on subsequent lines is 4 spaces. We have a style(9) manual page for more examples / specifics.
Starting a string sysctl with a newline is unconventional
M_WAITOK allocations cannot fail in FreeBSD.
I'd encourage adding a MODULE_PNP_INFO(), like the Intel NTB driver has.
Do you have AMD's permission to drop the 3rd clause of the BSDL present in the Linux driver?
Probably stale comment?
Thanks for looking into this patch. The details of this hardware is not yet made public. But let me know if you need any specific details. I will see if I can share you necessary details.
Corrected overall style mistakes. Also, moved "info" sysctl to global space, just to be in sync with other ntb_hw drivers.
In ntb.c, ntb_register_device function call dbt = flsll(NTB_DB_VALID_MASK(dev)). If nc->dbmask is returned from NTB_DB_VALID_MASK, it will be zero (irrespective of whatever the hardware driver says. So, hint becomes necessary to configured the db count. If we have the change in this patch, initially hardware driver provided db will be taken and later based on the hint, it will support only specific db_count. That's the reason I changed this. Please correct me if I am wrong.
Also, checked for other similar occurrences and corrected it.
Also, checked for other similar occurrences and corrected it.
Removed the check for NULL
I see this macro register device information. But how useful that will be with the system overall? It may be simple question, but just trying to understand.
Added the 3rd Clause. Copy/paste mistake.
Kept this as other ntb_hw drivers has the same comment in place.
I've quite briefly looked through the AMD part itself, but I have objections about interface part. See inline.
I am not sure about motivation of this function, but it ignores hints mechanism.
This function looks like a subset of ntb_mw_get_range(), except it does not respect hints mechanism, in particular nc->mwoff. I can understand addition of this function if it is needed to match Linux KPI, but otherwise I see no real need or benefits.
The system of hints allows to split resources of single NTB controller between several consumers. For example, on our hardware we split it between if_ntb/ntb_transport interface and large separate window for NVDIMM synchronization. If somebody wish to try it, it should be possible to set up two ntb_transport instances using different windows, scratchpads and doorbells, each with its own if_ntb interface.
I don't understand the provided explanation, I don't see what initial zero you are talking about. While we are not splitting doorbells on our setup, I tested this code and it worked just fine, while your code is obviously broken, since all NTB children will see the same mask despite hints, which is wrong.
I know Linux added those methods, but is there real usage for them? If we add them, we'll need either implement them for Intel and PLX or add default handlers, so that callers won't crash.
Sorry about the confusion here. I confused between "ntb_mw_count" and "NTB_MW_COUNT", which leads to that initial zero statement. Yes, it's not needed and is removed now.
I would be interested to try the scenario what is mentioned here. Can you please let me know how to configure the resources for two ntb_transport instances and get two if_ntb interfaces?
ntb_tool (D18819) uses this. That's the reason its been added. Yes I agree that we need default handlers for other hardware drivers. But they need to be implemented in the respective hardware drivers right (or) is there any other approach we can use, as in Linux?
Changed the PCI IDS to pci_device_table format and used PCI_PNP_INFO rather than MODULE_PNP_INFO.
I just made a pass through for anything that caught my eye, and found a couple. I have not reviewed it to see if it matches the hardware spec for what it's trying to control.
I'm not sure this is the right way to grab what you are grabbing. In the normal kernel build, all .h files are present in the current directory, or directly via -I lines. Having constructs like this leads to problems.
This is 0
This is 1
This a small positive integer of indeterminate value (which happens not to be 0 or 1 by chance).
So you have overlapping ranges here which is dangerous. There's other places as well that have this construct.
So *ntb doesn't ever change? If that's true, this is OK. If it changes, there's no locking in amd_ntb_hw_info_handler.
Strictly speaking, 'All Rights Reserved.' is not needed and baggage from a by-gone era no longer with any real meaning. You may drop it entirely, if you'd like (it's up to the copyright holder to determine the form of their copyright noice, but the project has recently dropped the phrase from its model license).
You should be able to attach two ntb_transport's by setting such loader tunable (if I understand properly what resources this AMD NTB has):
To attach third there probably won't be enough scratchpads, since ntb_transport uses at least 6 in case of one memory window.
As I mentioned, the spec for this hardware is not yet made public. If you wish to see, you can refer the Linux driver for the same (drivers/ntb/hw/amd). Please let me know if you have any specific questions.
Changed it to "dev/ntb/ntb.h". Other hardware drivers shall also be changed to be in sync. @mav , can we make this change for other hardware drivers as well.
I assume, the concern here is returning positive error value, so that value could wrongly be interpreted as a valid value. If so, that's a valid concern and it's been changed to negative value now.
Other than lnk_sta, rest of the ntb structure elements accessed are constants. Other values which could change, are being read from registers (using bus_space_read_X calls), which I believe is atomic and doesn't need any locking. "lnk_sta" is also changed only during link events. Please let me know if that needs a lock.
Removed. Also removed, unwanted Copy-right from other vendor