adi-axi-adc issues and how to properly support this designs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


Hi Jonathan,

There are some major issues with the implementation we have upstream
for the adi-axi-adc driver. The only user is the ad9467 driver. In the
rest of the email I will refer to ad9467 as the converter device and
axi-adc as the IP core. 

Let me start to state that these designs, in a very a basic way, have a
converter connected to a IP core that typically lives in a FPGA. This
core connects to a DMA core (which is used by the DMA buffer
implementation) so we can keep up with the high rates of these
converters. So there's a link between these devices and we try to
implement that so far looking at them as one single IIO device.

Let's go to the major issues now:

1) There is a circular dependency between the two device. And when
compiled as modules for example, we cannot really rmmod the modules

"root@analog:~# rmmod ad9467
rmmod: ERROR: Module ad9467 is in use

root@analog:~# rmmod adi-axi-adc.ko
rmmod: ERROR: Module adi_axi_adc is in use by: ad9467

This easy to understand as the ad9467 driver as a direct symbol
dependency on the axi-adc one. And this one calls 'module_get()' as
soon as it attaches to a "client"

2) It's fairly easy to crash the kernel:

root@analog:/sys/bus/spi/drivers/ad9467# echo spi0.0 > unbind
root@analog:/sys/bus/iio/devices/iio:device2# echo 1 >
root@analog:/sys/bus/iio/devices/iio:device2# echo 1 > buffer0/enable
[  132.349133] 8<--- cut here ---
[  132.352193] Unable to handle kernel paging request at virtual
address e0940000 when read
[  132.360333] [e0940000] *pgd=0208b811, *pte=00000000, *ppte=00000000
[  132.366668] Internal error: Oops: 7 [#1] SMP ARM
[  132.371289] Modules linked in: ad9467 adi_axi_adc ad9517
[  132.376609] CPU: 1 PID: 444 Comm: bash Not tainted 6.2.9-dirty #3
[  132.382701] Hardware name: Xilinx Zynq Platform
[  132.387223] PC is at adi_axi_adc_update_scan_mode+0x34/0x88
[  132.394020] LR is at arm_heavy_mb+0x1c/0x38
[  132.398212] pc : [<bf0060c4>]    lr : [<c031820c>]    psr: a0000013
[  132.404392] sp : e0929e30  ip : deaddead  fp : c4430270
[  132.409678] r10: c8a0bc18  r9 : deaddeac  r8 : c89b1c00
[  132.414895] r7 : c4430340  r6 : c45db7c0  r5 : e093ffc0  r4 :
[  132.421413] r3 : 00010000  r2 : e0940000  r1 : 00000000  r0 :

3) This solution does not really scale. There are designs where we have
multiple instances of the IP core attached to the client (typically if
a converter like this has more than one channel, we have one instance
of the core per channel). The way we have it know, that would not me

Issues number 2) and 3) would more or less be easy to solve moving from
of_parse_phandle() and from devm_ (using plain kmalloc() + krefs). The
big issue to me is 1) because, for example, in the driver we have out
of tree for ad9467, we do a tuning on the adc LVDS interface that we do
not do in the upstream version. And why is this problematic? Because
this means the converter driver has to do some configurations on the IP
core device and hence we have a bidirectional link between these 2
devices which, while doable, is far from ideal and can become complex
to keep it sane.

To be fair, what we have in upstream today is inspired a lot in what
ADI has out of tree (likely even better) but I truly think we need to
do it better. As I want to start working on this, I want to share some
ideas that I have on my head and hopefuly get some feedback on the
direction to go. So, here it goes:

1) The obvious one is to flip the logic of the "communication" link.
Right now callbacks are done from the IP core driver to the converter.
As stated, the converter might need to access the IP core to ask/do
some configurations. Well, from what I've seen in other projects we
have, I'm positive everything can be done by being the converter the
"entry point". This effectively means, that IP core devices would
register somewhere (I'm thinking in a tiny interface module that can
likely be reused for the axi-dac counterpart of the axi-adc) and the
converter device would "get" it during probe.

2) With respect to 1), while treating the devices as 1 might look
appealing, I'm not really sure it's the right thing to do. This,
because these devices are effectively two different devices: the
converter is typically a SPI device while the IP core is a MMIO device.
Treating them as 1, makes things like the IIO direct_reg_access not
doable (unless we duplicate some code and manually add the second
debugfs interface). Another thing not ideal with 1) is that the DMA
buffer interface really belongs on the IP core (looking on how the HW
is designed) and not on the converter driver. However, one thing that
I'm not sure it will be ideal with having two devices is synchronizing
accesses to the IP core. For example, the axi-dac driver (that I also
intend to bring upstream as part of this work) also has an userspace
interface so we need to be carefull. But I guess this is no different
for devices which are accessed by the in-kernel IIO interface.

I'm way more inclined to go with 2).

Another thing that I've been thinking is how should we implement the IP
core driver... As stated, there are usecases where we have mulitple
instances of these devices. Well, that is how we've been doing things
in ADI tree but essentially the core is just one big "block/device"
[1]. However, the way we've been doing things is actually ok because
every sub-block on these IPs have their own memory map allowing to
control everything needed by the sub-blocks.

Another option and now that we support output buffers and multiple
buffers per device would be to support these cores as one device
(likely in the addac folder). However, I think that keeping each block
as a standalone device might keep things more simple. But I assume it's
tempting to get a first user to the multi buffer support :)

Now for the big elephant of all the above... Everything that I've been
saying means breaking the current ABI. I'm fairly confident that we
should not have any upstream user for these drivers because it lacks a
lot of features that are only present in ADI tree. So, I think it's ok
to destroy the ABI but of course I cannot guarantee there are no users

So, would it be fine for me to not care about the current ABI while
working on this? Because, it will be very difficult to come up with
something that is compatible with what we have. Well, there's always
the option of coming up with a -v2 (or something like that) for these
drivers keeping the ones we have now, untouched. Ideas?

So... feedback is very much appreciated :)


- Nuno Sá

[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     []

  Powered by Linux