Re: [RFC PATCH 00/13] Switchtec NTB Support

[Serge Semin]

Hello Logan.
Thanks for the new hardware driver. It's really great to see NTB subsystem
being developed.

New NTB API is going to be merged to mainline kernel within next features
merge window, so it's really recommended to use that API for new hardware.
Could you please rabase your driver on top of the tree?
https://github.com/jonmason/ntb.git

> See what is staged in https://github.com/jonmason/ntb.git ntb-next, with
> the addition of multi-peer support by Serge.  It would be good at this
> stage to understand whether the api changes there would also support the
> Switchtec driver, and what if anything must change, or be planned to change,
> to support the Switchtec driver.

My impression is that, there is no need for new NTB API changes, since it
was specifically designed for new type of multi-port switches with
additional message registers, which is exactly supported by switchtec
device.

> The Switchtec hardware supports doorbells, memory windows and messages.
> Seeing there is no native scratchpad support, 128 spads are emulated
> through the use of a pre-setup memory window.

According to the NTB philosophy, it's not good to have any hardware
emulation within hardware driver. Hardware driver must reflect the only
hardware abilities, nothing else. Could you please get rid of Scratchpad
emulation and add messaging as new NTB API has got a proper callback
functions for it?
Hmmm, I haven't seen the actual code (see my last comment), but
according to my impression of API, it's impossible to have memory window
accessed while NTB link is down, but Scratchpads still can be used.
In this case, if you got Scratchpads emulated over memory windows,
you must have got NTB link enabled before NTB device is registered, which
makes ntb_link_* methods kind of being useless unless Switchtec hardware
supports NTB link getting up/down for individual memory windows...

> configurable set of memory windows. While the hardware supports more
> than 2 partitions, this driver only supports the first two seeing
> the current NTB API only supports two hosts.

New NTB API is updated so to have access to any of peer ports. IDT driver
has got a special translation table to access peer functionality just by
providing an index to corresponding API callback. You can use it as
reference to have Switchtec driver accordingly altered. It would be vastly
useful to have one more multi-port hardware driver in the tree.

> switchtec: move structure definitions into a common header
> switchtec: export class symbol for use in upper layer driver
> switchtec: add ntb hardware register definitions
> switchtec: add link event notifier block
> switchtec_ntb: introduce initial ntb driver
> switchtec_ntb: initialize hardware for memory windows
> switchtec_ntb: initialize hardware for doorbells and messages
> switchtec_ntb: add skeleton ntb driver
> switchtec_ntb: add link management
> switchtec_ntb: implement doorbell registers
> switchtec_ntb: implement scratchpad registers
> switchtec_ntb: add memory window support
> switchtec_ntb: update switchtec documentation with notes for ntb

If I'm not mistaken, these patches can be combined the way so to have
just two big functionally split patches:
1) NTB: Microsemt Switchtec switch management driver alterations for NTB
2) NTB: Add Microsemi Switchtec PCIe-switches support
It would really simplify the review. Could you please combine them?


Thanks for submitting the patches. We really appreciate this and looking
forward to have it completely reviewed and added to the kernel tree.

Regards
-Sergey

On Fri, Jun 16, 2017 at 08:09:55AM -0600, Logan Gunthorpe <logang@xxxxxxxxxxxx> wrote:
> 
> 
> On 16/06/17 07:53 AM, Allen Hubbe wrote:
> > See what is staged in https://github.com/jonmason/ntb.git ntb-next, with the addition of multi-peer support by Serge.  It would be good at this stage to understand whether the api changes there would also support the Switchtec driver, and what if anything must change, or be planned to change, to support the Switchtec driver.
> 
> Ah, yes I had seen that patchset some time ago but I wasn't aware of
> it's status or that it was queued up in ntb-next. I think it will be no
> problem to reconcile with the switchtec driver and I'll rebase onto
> ntb-next for the next posting of the patch set. However, I *may* save
> full multi-host switchtec support for a follow up submission. My initial
> impression is the new API will support the switchtec hardware well.
> 
> Thanks,
> 
> Logan