RE: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas R-Car devices

Hi Geert,

Thanks for your feedback!

From: Geert Uytterhoeven <geert@linux-m68k.org>
Sent: 26 February 2021 09:34
Subject: Re: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas R-
Car devices

Hi Fabrizio,

On Thu, Feb 25, 2021 at 11:53 PM Fabrizio Castro
[off-list ref] wrote:

quoted

The DAB hardware accelerator found on R-Car E3 and R-Car M3-N devices is
a hardware accelerator for software DAB demodulators.
It consists of one FFT (Fast Fourier Transform) module and one decoder
module, compatible with DAB specification (ETSI EN 300 401 and
ETSI TS 102 563).
The decoder module can perform FIC decoding and MSC decoding processing
from de-puncture to final decoded result.

This patch adds a device driver to support the FFT module only.

Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>

Thanks for your patch!

quoted

--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig

@@ -461,4 +461,5 @@ source "drivers/misc/bcm-vk/Kconfig"
 source "drivers/misc/cardreader/Kconfig"
 source "drivers/misc/habanalabs/Kconfig"
 source "drivers/misc/uacce/Kconfig"
+source "drivers/misc/rcar_dab/Kconfig"

rcar-dab?

I will change the directory name

quoted

--- /dev/null
+++ b/drivers/misc/rcar_dab/Kconfig

@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0+
+#
+# R-Car DAB Hardware Accelerator
+#
+
+config RCAR_DAB

DAB_RCAR?

Agreed

quoted

+       tristate "DAB accelerator for Renesas R-Car devices"
+       depends on ARCH_R8A77990 || ARCH_R8A77965

|| COMPILE_TEST

Will do

quoted

+       help
+         Some R-Car devices come with an IP to accelerate DAB decoding.
+         Enable this option to add driver support.

quoted

--- /dev/null
+++ b/drivers/misc/rcar_dab/rcar_dev.c

@@ -0,0 +1,176 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * R-Car Gen3 DAB hardware accelerator driver
+ *
+ * Copyright (C) 2021 Renesas Electronics Corporation
+ *
+ */
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/uaccess.h>
+
+#include <uapi/linux/rcar_dab.h>
+#include "rcar_dev.h"
+
+irqreturn_t rcar_dab_irq(int irq, void *devid)

static, as discovered by the robot.

Yep

quoted

+{
+       struct rcar_dab *dab = devid;
+       u32 intsr, intcr1;
+
+       spin_lock(&dab->shared_regs_lock);
+
+       intcr1 = rcar_dab_read(dab, RCAR_DAB_INTCR1);
+       rcar_dab_write(dab, RCAR_DAB_INTCR1, 0x000003FF);

Magic value (setting undocumented bits?), please use a define.

The documentation (Section 44B.2.21 of the R-Car Gen3 HW User
manual) says to write (reserved) bits 31 to 10 as 0, to write
(reserved) bits 9 to 3 as 1, and the remaining 3 bits (2 to 0),
are interrupt masking bits set to 1 in this case to temporarily
disable interrupts.

I can of course add a define for this.

quoted

+
+       intsr = rcar_dab_read(dab, RCAR_DAB_INTSR);
+       if (!intsr) {
+               rcar_dab_write(dab, RCAR_DAB_INTCR1, intcr1);
+               spin_unlock(&dab->shared_regs_lock);
+               return IRQ_NONE;
+       }
+
+       /* Re-enable interrupts that haven't fired */
+       rcar_dab_write(dab, RCAR_DAB_INTCR1, 0x3FF & (intsr | intcr1));
+       /* Clear interrupts */
+       rcar_dab_write(dab, RCAR_DAB_INTSR, 0x7 & ~intsr);

More magic values.

As per section 44B.2.20 from the R-Car Gen3 HW User Manual,
Bit 31 to 3 are reserved and reads and writes as 0, and a
0 has to be written to bits 2 to 0 to clear the interrupts.

I will use a define for this as well.

quoted

+
+       spin_unlock(&dab->shared_regs_lock);
+
+       if (intsr & RCAR_DAB_INTSR_FFT_DONE)
+               rcar_dab_fft_irq(dab);
+
+       return IRQ_HANDLED;
+}
+
+static long rcar_dab_unlocked_ioctl(struct file *file, unsigned int

cmd,

quoted

+                                   unsigned long arg)
+{
+       void __user *argp = (void __user *)arg;
+       struct rcar_dab *dab;
+       int ret;
+
+       dab = container_of(file->private_data, struct rcar_dab, misc);
+
+       switch (cmd) {
+       case RCAR_DAB_IOC_FFT:
+               if (!access_ok(argp, sizeof(struct rcar_dab_fft_req)))
+                       return -EFAULT;
+               ret = rcar_dab_fft(dab, argp);

Do you envision ever using the FFT operation from kernel space?
Might be easier if you handle the copy_{from,to}_user() here.

Buffers are pre-allocated and shared among requests, therefore access to the
buffers has to be protected with a mutex. To keep things tidy (all FFT related
work in the FFT related source file) I prefer to keep this as it is, as FIC
and MSC will add more to the ioctl.

quoted

+               break;
+       default:
+               ret = -ENOTTY;
+       }
+
+       return ret;
+}
+
+static const struct file_operations rcar_dab_fops = {
+       .owner          = THIS_MODULE,
+       .unlocked_ioctl = rcar_dab_unlocked_ioctl,

Does this need compat_ioctl handling?

Will add

quoted

+};
+
+static int rcar_dab_probe(struct platform_device *pdev)
+{
+       struct device *dev = &pdev->dev;
+       struct rcar_dab *dab;
+       int ret, irq;
+
+       dab = devm_kzalloc(dev, sizeof(*dab), GFP_KERNEL);
+       if (!dab)
+               return -ENOMEM;
+       dab->pdev = pdev;
+
+       dab->base = devm_platform_ioremap_resource(pdev, 0);
+       if (IS_ERR(dab->base))
+               return PTR_ERR(dab->base);
+
+       irq = platform_get_irq(pdev, 0);
+       if (irq < 0) {
+               dev_err(dev, "Can't get the irq.\n");
+               return -EINVAL;
+       }
+
+       dab->clk = devm_clk_get(&pdev->dev, "dab");
+       if (IS_ERR(dab->clk)) {
+               ret = PTR_ERR(dab->clk);
+               dev_err(dev, "Can't get the clock (%d).\n", ret);
+               return ret;
+       }
+
+       spin_lock_init(&dab->shared_regs_lock);
+
+       ret = clk_prepare_enable(dab->clk);

Does the module clock need to be enabled all the time?

No, it doesn't.

What about using Runtime PM instead of explicit clock handling, so your
driver will keep on working on future SoCs where the DAB block is part of
a power domain?

Can do, will switch to using Runtime PM.

quoted

+       if (ret)
+               return ret;
+
+       ret = rcar_dab_fft_probe(dab);
+       if (ret)
+               goto error_clock_disable;
+
+       ret = devm_request_irq(dev, irq, rcar_dab_irq, 0, dev_name(dev),

dab);

quoted

+       if (ret) {
+               dev_err(dev, "Can't request the irq (%d).\n", ret);
+               goto error_remove;
+       }
+
+       platform_set_drvdata(pdev, dab);
+
+       dab->misc.minor = MISC_DYNAMIC_MINOR;
+       dab->misc.name = RCAR_DAB_DRV_NAME;
+       dab->misc.fops = &rcar_dab_fops;
+       ret = misc_register(&dab->misc);
+       if (ret) {
+               dev_err(dev, "Can't register misc device (%d).\n", ret);
+               goto error_remove;
+       }
+
+       dev_info(dev, "R-Car Gen3 DAB misc driver ready.\n");
+
+       return 0;
+
+error_remove:
+       rcar_dab_fft_remove(dab);
+
+error_clock_disable:
+       clk_disable_unprepare(dab->clk);
+
+       return ret;
+}

quoted

--- /dev/null
+++ b/drivers/misc/rcar_dab/rcar_dev.h

quoted

+/* Register DAB_FFTCR */
+#define RCAR_DAB_FFTCR_FFT_EN_DISABLED         0
+#define RCAR_DAB_FFTCR_FFT_EN_ENABLED          1

Do you need both?

We don't, I have just thought it made things clearer.

#define RCAR_DAB_FFTCR_FFT_EN        BIT(0)

quoted

+
+/* Register DAB_FFTREQCR */
+#define RCAR_DAB_FFTREQCR_FFT_REQ_INACTIVE     0
+#define RCAR_DAB_FFTREQCR_FFT_REQ_ACTIVE       1

Do you need both?

We don't, I have just thought it made things clearer.

quoted

+
+/* Register DAB_INTSR */
+#define RCAR_DAB_INTSR_FFT_DONE                        0x1

BIT(0) (there are more bits for FIC and MSC)

Will change

quoted

+
+/* Register DAB_INTCR1 */
+#define RCAR_DAB_INTCR1_FFT_DONE_MASK          0x1

BIT(0) (there are more bits for FIC and MSC)

Will change

quoted

+#define RCAR_DAB_INTCR1_FFT_DONE_INT_ENABLED   0
+#define RCAR_DAB_INTCR1_FFT_DONE_INT_DISABLED  1

Do you need these?
I'd just retain RCAR_DAB_INTCR1_FFT_DONE.

Agreed

For enabling interrupts:

    rcar_dab_update_bits_locked(dab, RCAR_DAB_INTCR1,
                                RCAR_DAB_INTCR1_FFT_DONE,
                                RCAR_DAB_INTCR1_FFT_DONE);

and for disabling:

    rcar_dab_update_bits_locked(dab, RCAR_DAB_INTCR1,
                                CAR_DAB_INTCR1_FFT_DONE, 0);

Okay

quoted

+
+struct rcar_dab_fft {
+       bool done;                      /*
+                                        * Condition for waking up the

process

quoted

+                                        * sleeping while FFT is in

progress.

quoted

+                                        */

Please use kerneldoc for documenting structures.

Ok

quoted

+       wait_queue_head_t wait;         /* Wait queue for FFT. */
+       struct mutex lock;              /* Mutex for FFT. */
+       dma_addr_t dma_input_buf;       /*
+                                        * Input buffer seen by the FFT
+                                        * module.
+                                        */
+       dma_addr_t dma_output_buf;      /*
+                                        * Output buffer seen by the FFT
+                                        * module.
+                                        */
+       void *input_buffer;             /* Input buffer seen by the CPU

*/

quoted

+       void *output_buffer;            /* Output buffer seen by the CPU

*/

Please use consistent naming (buf vs buffer).

Ok

quoted

+};

quoted

--- /dev/null
+++ b/drivers/misc/rcar_dab/rcar_fft.c

quoted

+int rcar_dab_fft(struct rcar_dab *dab, struct rcar_dab_fft_req __user

*fft_req)

quoted

+{
+       int buffer_size, ret;
+
+       buffer_size = fft_req->points * 4;

Missing validation of buffer_size?

Will add

quoted

+
+       mutex_lock(&dab->fft.lock);
+
+       if (copy_from_user(dab->fft.input_buffer, fft_req-
input_address,
+                          buffer_size)) {
+               mutex_unlock(&dab->fft.lock);
+               return -EFAULT;
+       }
+
+       dab->fft.done = false;

You can init done in rcar_dab_fft_init(), too.

Will move

quoted

+       ret = rcar_dab_fft_init(dab, fft_req);
+       if (ret) {
+               mutex_unlock(&dab->fft.lock);
+               return ret;
+       }
+
+       rcar_dab_fft_enable(dab);
+       wait_event_interruptible_timeout(dab->fft.wait, dab->fft.done,

HZ);

quoted

+       if (!dab->fft.done) {

You can just check the return value of wait_event_interruptible_timeout().

Will change

quoted

+               rcar_dab_fft_disable(dab);
+               ret = -EFAULT;

-ETIMEOUT?

Yeah, better, thanks

quoted

+       } else if (copy_to_user(fft_req->output_address, dab-
fft.output_buffer,
+                               buffer_size)) {
+               ret = -EFAULT;
+       }
+
+       mutex_unlock(&dab->fft.lock);
+
+       return ret;
+}

quoted

--- /dev/null
+++ b/include/uapi/linux/rcar_dab.h

@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+/*
+ * R-Car Gen3 DAB user space interface data structures
+ *
+ * Copyright (C) 2021 Renesas Electronics Corporation
+ *
+ */
+#ifndef _RCAR_DAB_H_
+#define _RCAR_DAB_H_
+
+struct rcar_dab_fft_req {
+       int points;                     /*

unsigned int

Will change

quoted

+                                        * The number of points to use.
+                                        * Legal values are 256, 512,

1024, and

quoted

+                                        * 2048.
+                                        */

Please use kerneldoc to document struct members.

Ok

quoted

+       unsigned char ofdm_number;      /*
+                                        * Orthogonal Frequency Division
+                                        * Multiplexing (OFDM).
+                                        * Minimum value is 1, maximum

value is

quoted

+                                        * 255.
+                                        */

Please make padding explicit.

Okay

I'd also sort the members by decreasing size, i.e. pointers first.

Okay

quoted

+       void __user *input_address;     /*

input_buf?

Sure

quoted

+                                        * User space address for the

input

quoted

+                                        * buffer.
+                                        */
+       void __user *output_address;    /*

output_buf?

Sure

quoted

+                                        * User space address for the

output

quoted

+                                        * buffer.
+                                        */
+};
+
+#define        RCAR_DAB_IOC_FFT                _IOWR(0x90, 1, struct

rcar_dab_fft_req *)

quoted

+
+#endif /* _RCAR_DAB_H_ */

Thanks,
Fab

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
m68k.org

In personal conversations with technical people, I call myself a hacker.
But
when I'm talking to journalists I just say "programmer" or something like
that.
                                -- Linus Torvalds