Page MenuHomeFreeBSD

Add AMD NTB Hardware Driver for AMD SoC
Needs ReviewPublic

Authored by rajfbsd_gmail.com on Mon, Jan 7, 4:46 PM.

Details

Reviewers
mav
cem
Summary

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

cem added a comment.Tue, Jan 8, 1:08 AM

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?

Thanks!

sys/dev/ntb/ntb.c
485–488

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.

sys/dev/ntb/ntb_hw/ntb_hw_amd.c
298

style nit: a space goes between if and (.

305

Usual style for indentation on subsequent lines is 4 spaces. We have a style(9) manual page for more examples / specifics.

659

Starting a string sysctl with a newline is unconventional

873

M_WAITOK allocations cannot fail in FreeBSD.

1293

I'd encourage adding a MODULE_PNP_INFO(), like the Intel NTB driver has.

sys/dev/ntb/ntb_hw/ntb_hw_amd.h
25

Do you have AMD's permission to drop the 3rd clause of the BSDL present in the Linux driver?

218

Probably stale comment?

rajfbsd_gmail.com marked 6 inline comments as done.Wed, Jan 9, 2:27 PM

Hi cem,

In D18774#400705, @cem wrote:

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?

Thanks!

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.

sys/dev/ntb/ntb.c
485–488

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.

sys/dev/ntb/ntb_hw/ntb_hw_amd.c
298

Also, checked for other similar occurrences and corrected it.

305

Also, checked for other similar occurrences and corrected it.

873

Removed the check for NULL

1293

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.

sys/dev/ntb/ntb_hw/ntb_hw_amd.h
25

Added the 3rd Clause. Copy/paste mistake.

218

Kept this as other ntb_hw drivers has the same comment in place.

rajfbsd_gmail.com marked 5 inline comments as done.

V2 patch addressing the previous comments from cem.

imp added inline comments.Wed, Jan 9, 4:07 PM
sys/dev/ntb/ntb_hw/ntb_hw_amd.c
1179

I'd encourage the use of pci_device_table here and the use of PCI_PNP_INFO instead below. That's not as important as the PNP_INFO macro though.

1293

It's used by devmatch to automatically load modules based on plug and play information.

mav requested changes to this revision.Wed, Jan 16, 10:20 PM

I've quite briefly looked through the AMD part itself, but I have objections about interface part. See inline.

sys/dev/ntb/ntb.c
424

I am not sure about motivation of this function, but it ignores hints mechanism.

431

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.

485–488

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.

sys/dev/ntb/ntb.h
111

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.

This revision now requires changes to proceed.Wed, Jan 16, 10:20 PM
rajfbsd_gmail.com marked an inline comment as done.Fri, Jan 18, 2:16 PM
rajfbsd_gmail.com added inline comments.
sys/dev/ntb/ntb.c
424

This was added for ntb_tool (D18819) and ntb_perf(D18832). But this function is currently not used by those drivers. So removed this.

431

This was also added for ntb_tool (D18819) and ntb_perf(D18832). But this function is currently not used by those drivers. So removed this also.

485–488

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?

sys/dev/ntb/ntb.h
111

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?

sys/dev/ntb/ntb_hw/ntb_hw_amd.c
1179

Changed the PCI IDS to pci_device_table format and used PCI_PNP_INFO rather than MODULE_PNP_INFO.

rajfbsd_gmail.com marked an inline comment as done.

Addressed commented from @imp and @mav and submitted V3 patch. Also, added amd_ntb_mw_set_wc routine.

imp added a comment.Fri, Jan 18, 5:28 PM

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.

sys/dev/ntb/ntb_hw/ntb_hw_amd.c
80

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.

110

This is 0

112

This is 1

117

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.

716

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.

sys/dev/ntb/ntb_hw/ntb_hw_amd.h
8

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).

mav added inline comments.Fri, Jan 18, 6:15 PM
sys/dev/ntb/ntb.c
485–488

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):
hint.ntb_hw.0.config="ntb_transport:1:6:8,ntb_transport"

To attach third there probably won't be enough scratchpads, since ntb_transport uses at least 6 in case of one memory window.

rajfbsd_gmail.com marked 3 inline comments as done.Mon, Jan 21, 5:27 PM
In D18774#403351, @imp wrote:

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.

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.

sys/dev/ntb/ntb_hw/ntb_hw_amd.c
80

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.

117

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.

716

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.

sys/dev/ntb/ntb_hw/ntb_hw_amd.h
8

Removed. Also removed, unwanted Copy-right from other vendor

rajfbsd_gmail.com marked 3 inline comments as done.

Submitting V4 patch, addressing comment from @imp