On 3/5/19 2:01 AM, Arnd Bergmann wrote:
On Mon, Mar 4, 2019 at 10:37 PM Eddie James <eajames@xxxxxxxxxxxxx> wrote:
The XDMA engine embedded in the AST2500 SOC performs PCI DMA operations
between the SOC (acting as a BMC) and a host processor in a server.
This commit adds a driver to control the XDMA engine and adds functions
to initialize the hardware and memory and start DMA operations.
Signed-off-by: Eddie James <eajames@xxxxxxxxxxxxx>
Hi Eddie,
Thanks for your submission! Overall this looks well-implemented, but
I fear we already have too many ways of doing the same thing at
the moment, and I would hope to avoid adding yet another user space
interface for a specific hardware that does this.
Your interface appears to be a fairly low-level variant, just doing
single DMA transfers through ioctls, but configuring the PCIe
endpoint over sysfs.
Hi, thanks for the quick response!
There is actually no PCIe configuration done in this driver. The two
sysfs entries control the system control unit (SCU) on the AST2500
purely to enable and disable entire PCIe devices. It might be possible
to control those devices more finely with a PCI endpoint driver, but
there is no need to do so. The XDMA engine does that by itself to
perform DMA fairly automatically.
If the sysfs entries are really troublesome, we can probably remove
those and find another way to control the SCU.
Please have a look at the drivers/pci/endpoint framework first
and see if you can work on top of that interface instead.
Even if it doesn't quite do what you need here, we may be
able to extend it in a way that works for you, and lets others
use the same user interface extensions in the future.
It may also be necessary to split out the DMA engine portion
into a regular drivers/dma/ back-end to make that fit in with
the PCIe endpoint framework.
Right, I did look into the normal DMA framework. There were a couple of
problems. First and foremost, the "device" (really, host processor)
address that we use is 64 bit, but the AST2500 is of course 32 bit. So I
couldn't find a good way to get the address through the DMA API into the
driver. It's entirely possible I missed something there though.
The other issue was that the vast majority of the DMA framework was
unused, resulting in a large amount of boilerplate that did nothing
except satisfy the API... I thought simplicity would be better in this case.
Let me know what you think... I could certainly switch to ioctl instead
of the write() if that's better. Or if you really think the DMA
framework is required here, let me know.
Thanks,
Eddie
If you have already tried this without success, please let us
know in the description what problems you have hit, and why you
decided to create a new framework instead.
+/*
+ * aspeed_xdma_op
+ *
+ * upstream: boolean indicating the direction of the DMA operation; upstream
+ * means a transfer from the BMC to the host
+ *
+ * host_addr: the DMA address on the host side, typically configured by PCI
+ * subsystem
+ *
+ * len: the size of the transfer in bytes; it should be a multiple of 16 bytes
+ */
+struct aspeed_xdma_op {
+ __u8 upstream;
+ __u64 host_addr;
+ __u32 len;
+} __packed;
Side-note: packed structures are generally not great user space
interfaces. Regardless of where we end up with this, I'd recommend
naturally aligning each member inside of the structure, and using
explicit padding here.
Understood, thanks.
Arnd
