Hi Geert,
Thanks for your feedback!
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> 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
> <fabrizio.castro.jz@xxxxxxxxxxx> wrote:
> > 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@xxxxxxxxxxx>
>
> Thanks for your patch!
>
> > --- 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
>
>
> > --- /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
>
> > + tristate "DAB accelerator for Renesas R-Car devices"
> > + depends on ARCH_R8A77990 || ARCH_R8A77965
>
> || COMPILE_TEST
Will do
>
> > + help
> > + Some R-Car devices come with an IP to accelerate DAB decoding.
> > + Enable this option to add driver support.
>
> > --- /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
>
> > +{
> > + 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.
>
> > +
> > + 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.
>
> > +
> > + 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,
> > + 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.
>
> > + 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
>
> > +};
> > +
> > +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.
>
> > + 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);
> > + 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;
> > +}
>
> > --- /dev/null
> > +++ b/drivers/misc/rcar_dab/rcar_dev.h
>
> > +/* 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)
>
> > +
> > +/* 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.
>
> > +
> > +/* Register DAB_INTSR */
> > +#define RCAR_DAB_INTSR_FFT_DONE 0x1
>
> BIT(0) (there are more bits for FIC and MSC)
Will change
>
> > +
> > +/* Register DAB_INTCR1 */
> > +#define RCAR_DAB_INTCR1_FFT_DONE_MASK 0x1
>
> BIT(0) (there are more bits for FIC and MSC)
Will change
>
> > +#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
>
> > +
> > +struct rcar_dab_fft {
> > + bool done; /*
> > + * Condition for waking up the
> process
> > + * sleeping while FFT is in
> progress.
> > + */
>
> Please use kerneldoc for documenting structures.
Ok
>
> > + 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
> */
> > + void *output_buffer; /* Output buffer seen by the CPU
> */
>
> Please use consistent naming (buf vs buffer).
Ok
>
> > +};
>
> > --- /dev/null
> > +++ b/drivers/misc/rcar_dab/rcar_fft.c
>
> > +int rcar_dab_fft(struct rcar_dab *dab, struct rcar_dab_fft_req __user
> *fft_req)
> > +{
> > + int buffer_size, ret;
> > +
> > + buffer_size = fft_req->points * 4;
>
> Missing validation of buffer_size?
Will add
>
> > +
> > + 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
>
> > + 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);
> > + if (!dab->fft.done) {
>
> You can just check the return value of wait_event_interruptible_timeout().
Will change
>
> > + rcar_dab_fft_disable(dab);
> > + ret = -EFAULT;
>
> -ETIMEOUT?
Yeah, better, thanks
>
> > + } else if (copy_to_user(fft_req->output_address, dab-
> >fft.output_buffer,
> > + buffer_size)) {
> > + ret = -EFAULT;
> > + }
> > +
> > + mutex_unlock(&dab->fft.lock);
> > +
> > + return ret;
> > +}
>
> > --- /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
>
> > + * The number of points to use.
> > + * Legal values are 256, 512,
> 1024, and
> > + * 2048.
> > + */
>
> Please use kerneldoc to document struct members.
Ok
>
> > + unsigned char ofdm_number; /*
> > + * Orthogonal Frequency Division
> > + * Multiplexing (OFDM).
> > + * Minimum value is 1, maximum
> value is
> > + * 255.
> > + */
>
> Please make padding explicit.
Okay
> I'd also sort the members by decreasing size, i.e. pointers first.
Okay
>
> > + void __user *input_address; /*
>
> input_buf?
Sure
>
> > + * User space address for the
> input
> > + * buffer.
> > + */
> > + void __user *output_address; /*
>
> output_buf?
Sure
>
> > + * User space address for the
> output
> > + * buffer.
> > + */
> > +};
> > +
> > +#define RCAR_DAB_IOC_FFT _IOWR(0x90, 1, struct
> rcar_dab_fft_req *)
> > +
> > +#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
