| Message ID | 20210225225147.29920-1-fabrizio.castro.jz@renesas.com |
|---|---|
| Headers | show |
| Series | Add FFT Support for R-Car Gen3 devices | expand |
Hi Fabrizio, On Thu, Feb 25, 2021 at 11:52 PM Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote: > This patch adds the DAB clock to the R8A77990 SoC. > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> Thanks for your patch! > --- a/drivers/clk/renesas/r8a77990-cpg-mssr.c > +++ b/drivers/clk/renesas/r8a77990-cpg-mssr.c > @@ -232,6 +232,7 @@ static const struct mssr_mod_clk r8a77990_mod_clks[] __initconst = { > DEF_MOD("ssi2", 1013, MOD_CLK_ID(1005)), > DEF_MOD("ssi1", 1014, MOD_CLK_ID(1005)), > DEF_MOD("ssi0", 1015, MOD_CLK_ID(1005)), > + DEF_MOD("dab", 1016, R8A77990_CLK_S3D1), Unfortunately this bit is not documented in the R-Car Gen3 Hardware User's Manual, so I have to trust you on this. > DEF_MOD("scu-all", 1017, R8A77990_CLK_S3D4), > DEF_MOD("scu-dvc1", 1018, MOD_CLK_ID(1017)), > DEF_MOD("scu-dvc0", 1019, MOD_CLK_ID(1017)), 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
Hi Fabrizio, On Thu, Feb 25, 2021 at 11:53 PM Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote: > Document bindings for R-Car DAB hardware accelerator, currently > found on the r8a77990 SoC (a.k.a. R-Car E3) and on the r8a77965 > SoC (a.k.a. R-Car M3-N). > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> Thanks for your patch! > --- /dev/null > +++ b/Documentation/devicetree/bindings/misc/renesas,dab.yaml > +properties: > + compatible: > + items: > + - enum: > + - renesas,dab-r8a77965 # R-Car M3-N > + - renesas,dab-r8a77990 # R-Car E3 Please use the recommended order for new bindings: renesas,r8a77965-dab renesas,r8a77990-dab > + - const: renesas,rcar-gen3-dab # Generic fallback for R-Car Gen3 devices > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + clock-names: > + const: dab fck? 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
Hello! On 26.02.2021 1:51, Fabrizio Castro wrote: > Document bindings for R-Car DAB hardware accelerator, currently > found on the r8a77990 SoC (a.k.a. R-Car E3) and on the r8a77965 > SoC (a.k.a. R-Car M3-N). > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > --- > .../devicetree/bindings/misc/renesas,dab.yaml | 75 +++++++++++++++++++ > 1 file changed, 75 insertions(+) > create mode 100644 Documentation/devicetree/bindings/misc/renesas,dab.yaml > > diff --git a/Documentation/devicetree/bindings/misc/renesas,dab.yaml b/Documentation/devicetree/bindings/misc/renesas,dab.yaml > new file mode 100644 > index 000000000000..e9494add13d7 > --- /dev/null > +++ b/Documentation/devicetree/bindings/misc/renesas,dab.yaml > @@ -0,0 +1,75 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright (C) 2021 Renesas Electronics Corporation > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/misc/renesas,dab.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Renesas R-Car DAB Hardware Accelerator > + > +maintainers: > + - Fabrizio Castro <fabrizio.castro.jz@renesas.com> > + > +description: > + The DAB hardware accelerator found on some R-Car 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. > + > +properties: > + compatible: > + items: > + - enum: > + - renesas,dab-r8a77965 # R-Car M3-N > + - renesas,dab-r8a77990 # R-Car E3 Why not renesas,<soc>-dab? > + - const: renesas,rcar-gen3-dab # Generic fallback for R-Car Gen3 devices [...] MBR, Sergei
Hi Fabrizio, On Thu, Feb 25, 2021 at 11:53 PM Fabrizio Castro <fabrizio.castro.jz@renesas.com> 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@renesas.com> 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? > --- /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? > + tristate "DAB accelerator for Renesas R-Car devices" > + depends on ARCH_R8A77990 || ARCH_R8A77965 || COMPILE_TEST > + 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. > +{ > + 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. > + > + 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. > + > + 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. > + 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? > +}; > + > +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? 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? > + 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? #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? > + > +/* Register DAB_INTSR */ > +#define RCAR_DAB_INTSR_FFT_DONE 0x1 BIT(0) (there are more bits for FIC and MSC) > + > +/* Register DAB_INTCR1 */ > +#define RCAR_DAB_INTCR1_FFT_DONE_MASK 0x1 BIT(0) (there are more bits for FIC and MSC) > +#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. 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); > + > +struct rcar_dab_fft { > + bool done; /* > + * Condition for waking up the process > + * sleeping while FFT is in progress. > + */ Please use kerneldoc for documenting structures. > + 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). > +}; > --- /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? > + > + 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. > + 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(). > + rcar_dab_fft_disable(dab); > + ret = -EFAULT; -ETIMEOUT? > + } 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 > + * The number of points to use. > + * Legal values are 256, 512, 1024, and > + * 2048. > + */ Please use kerneldoc to document struct members. > + unsigned char ofdm_number; /* > + * Orthogonal Frequency Division > + * Multiplexing (OFDM). > + * Minimum value is 1, maximum value is > + * 255. > + */ Please make padding explicit. I'd also sort the members by decreasing size, i.e. pointers first. > + void __user *input_address; /* input_buf? > + * User space address for the input > + * buffer. > + */ > + void __user *output_address; /* output_buf? > + * User space address for the output > + * buffer. > + */ > +}; > + > +#define RCAR_DAB_IOC_FFT _IOWR(0x90, 1, struct rcar_dab_fft_req *) > + > +#endif /* _RCAR_DAB_H_ */ 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
On Thu, Feb 25, 2021 at 11:51 PM Fabrizio Castro <fabrizio.castro.jz@renesas.com> 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@renesas.com> > --- > MAINTAINERS | 7 ++ > drivers/misc/Kconfig | 1 + > drivers/misc/Makefile | 1 + > drivers/misc/rcar_dab/Kconfig | 11 ++ > drivers/misc/rcar_dab/Makefile | 8 ++ > drivers/misc/rcar_dab/rcar_dev.c | 176 +++++++++++++++++++++++++++++++ > drivers/misc/rcar_dab/rcar_dev.h | 116 ++++++++++++++++++++ > drivers/misc/rcar_dab/rcar_fft.c | 160 ++++++++++++++++++++++++++++ > include/uapi/linux/rcar_dab.h | 35 ++++++ Can you explain why this is not in drivers/media/? I don't think we want a custom ioctl interface for a device that implements a generic specification. My first feeling would be that this should not have a user-level API but instead get called by the DAB radio driver. What is the intended usage model here? I assume the idea is to use it in an application that receives audio or metadata from DAB. What driver do you use for that? > +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); > + break; > + default: > + ret = -ENOTTY; > + } > + > + return ret; > +} > + > +static const struct file_operations rcar_dab_fops = { > + .owner = THIS_MODULE, > + .unlocked_ioctl = rcar_dab_unlocked_ioctl, > +}; There should be a '.compat_ioctl = compat_ptr_ioctl' entry, provided that the arguments are compatible between 32-bit and 64-bit user space. > + > +static int rcar_dab_fft_init(struct rcar_dab *dab, struct rcar_dab_fft_req *fft) > +{ > + u32 mode; > + > + for (mode = 0; mode < ARRAY_SIZE(rcar_dab_fft_size_lut); mode++) > + if (rcar_dab_fft_size_lut[mode] == fft->points) > + break; > + if (mode == ARRAY_SIZE(rcar_dab_fft_size_lut)) > + return -EINVAL; > + if (fft->ofdm_number == 0) > + return -EINVAL; > + > + rcar_dab_write(dab, RCAR_DAB_FFTSSR, mode); > + rcar_dab_write(dab, RCAR_DAB_FFTNUMOFDMR, fft->ofdm_number); > + rcar_dab_write(dab, RCAR_DAB_FFTINADDR, (u32)dab->fft.dma_input_buf); > + rcar_dab_write(dab, RCAR_DAB_FFTOUTADDR, (u32)dab->fft.dma_output_buf); Maybe use lower_32_bits() instead of the (u32) cast. For clarity, you may also want to specifically ask for a 32-bit DMA mask in the probe function, with a comment that describes what the hardware limitation is. > + > + 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; > + 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) { > + rcar_dab_fft_disable(dab); > + ret = -EFAULT; -EFAULT doesn't look like the right error for timeout or signal handling. Better check the return code from wait_event_interruptible_timeout() instead. > + > +struct rcar_dab_fft_req { > + int points; /* > + * The number of points to use. > + * Legal values are 256, 512, 1024, and > + * 2048. > + */ > + unsigned char ofdm_number; /* > + * Orthogonal Frequency Division > + * Multiplexing (OFDM). > + * Minimum value is 1, maximum value is > + * 255. > + */ > + void __user *input_address; /* > + * User space address for the input > + * buffer. > + */ > + void __user *output_address; /* > + * User space address for the output > + * buffer. > + */ > +}; Please read Documentation/driver-api/ioctl.rst and make this a portable data structure. Arnd
Hi Fabrizio, On Thu, Feb 25, 2021 at 10:51:40PM +0000, Fabrizio Castro wrote: > The DAB hardware accelerator found on R-Car E3 (a.k.a. r8a77990) > and R-Car M3-N (a.k.a. r8a77965) 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 series adds FFT support only for R-Car E3 and R-Car M3-N, > FIC and MSC support will be added later on. Out of curiosity, could the FFT module be used as an accelerator for 2D FFT on images ? > Fabrizio Castro (7): > clk: renesas: r8a77990: Add DAB clock > clk: renesas: r8a77965: Add DAB clock > dt-bindings: misc: Add binding for R-Car DAB > misc: Add driver for DAB IP found on Renesas R-Car devices > arm64: dts: renesas: r8a77990: Add DAB support > arm64: dts: renesas: r8a77965: Add DAB support > arm64: configs: Add R-Car DAB support > > .../devicetree/bindings/misc/renesas,dab.yaml | 75 ++++++++ > MAINTAINERS | 7 + > arch/arm64/boot/dts/renesas/r8a77965.dtsi | 12 ++ > arch/arm64/boot/dts/renesas/r8a77990.dtsi | 12 ++ > arch/arm64/configs/defconfig | 1 + > drivers/clk/renesas/r8a77965-cpg-mssr.c | 1 + > drivers/clk/renesas/r8a77990-cpg-mssr.c | 1 + > drivers/misc/Kconfig | 1 + > drivers/misc/Makefile | 1 + > drivers/misc/rcar_dab/Kconfig | 11 ++ > drivers/misc/rcar_dab/Makefile | 8 + > drivers/misc/rcar_dab/rcar_dev.c | 176 ++++++++++++++++++ > drivers/misc/rcar_dab/rcar_dev.h | 116 ++++++++++++ > drivers/misc/rcar_dab/rcar_fft.c | 160 ++++++++++++++++ > include/uapi/linux/rcar_dab.h | 35 ++++ > 15 files changed, 617 insertions(+) > create mode 100644 Documentation/devicetree/bindings/misc/renesas,dab.yaml > create mode 100644 drivers/misc/rcar_dab/Kconfig > create mode 100644 drivers/misc/rcar_dab/Makefile > create mode 100644 drivers/misc/rcar_dab/rcar_dev.c > create mode 100644 drivers/misc/rcar_dab/rcar_dev.h > create mode 100644 drivers/misc/rcar_dab/rcar_fft.c > create mode 100644 include/uapi/linux/rcar_dab.h -- Regards, Laurent Pinchart
Hi Fabrizio, Thank you for the patch. On Thu, Feb 25, 2021 at 10:51:47PM +0000, Fabrizio Castro wrote: > Make sure that the R-Car DAB device driver gets compiled as a > module since R-Car E3 and R-Car M3-N come with the DAB IP. > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> Do we need this in the defconfig ? It's not required to have a bootable E3 or M3-N with the set of standard features, and would result in all ARM64 platforms having one module they don't care about. > --- > arch/arm64/configs/defconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > index d612f633b771..3b9996c7f1fc 100644 > --- a/arch/arm64/configs/defconfig > +++ b/arch/arm64/configs/defconfig > @@ -274,6 +274,7 @@ CONFIG_PCI_ENDPOINT_TEST=m > CONFIG_EEPROM_AT24=m > CONFIG_EEPROM_AT25=m > CONFIG_UACCE=m > +CONFIG_RCAR_DAB=m > # CONFIG_SCSI_PROC_FS is not set > CONFIG_BLK_DEV_SD=y > CONFIG_SCSI_SAS_ATA=y -- Regards, Laurent Pinchart
Hi Fabrizio, Thank you for the patch. On Thu, Feb 25, 2021 at 10:51:43PM +0000, Fabrizio Castro wrote: > Document bindings for R-Car DAB hardware accelerator, currently > found on the r8a77990 SoC (a.k.a. R-Car E3) and on the r8a77965 > SoC (a.k.a. R-Car M3-N). > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > --- > .../devicetree/bindings/misc/renesas,dab.yaml | 75 +++++++++++++++++++ > 1 file changed, 75 insertions(+) > create mode 100644 Documentation/devicetree/bindings/misc/renesas,dab.yaml > > diff --git a/Documentation/devicetree/bindings/misc/renesas,dab.yaml b/Documentation/devicetree/bindings/misc/renesas,dab.yaml > new file mode 100644 > index 000000000000..e9494add13d7 > --- /dev/null > +++ b/Documentation/devicetree/bindings/misc/renesas,dab.yaml > @@ -0,0 +1,75 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright (C) 2021 Renesas Electronics Corporation > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/misc/renesas,dab.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Renesas R-Car DAB Hardware Accelerator > + > +maintainers: > + - Fabrizio Castro <fabrizio.castro.jz@renesas.com> > + > +description: > + The DAB hardware accelerator found on some R-Car 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. > + > +properties: > + compatible: > + items: > + - enum: > + - renesas,dab-r8a77965 # R-Car M3-N > + - renesas,dab-r8a77990 # R-Car E3 > + - const: renesas,rcar-gen3-dab # Generic fallback for R-Car Gen3 devices > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + clocks: > + maxItems: 1 I usually try to describe clocks: clocks: items: - description: The module functional clock but as there's a single clock, it may not be worth it. Up to you. > + > + clock-names: > + const: dab With Geert's and Sergei's comments addressed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > + power-domains: > + maxItems: 1 > + > + resets: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + - clock-names > + - power-domains > + - resets > + > +additionalProperties: false > + > +examples: > + # R8A77990 (R-Car E3) > + - | > + #include <dt-bindings/clock/r8a77990-cpg-mssr.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/power/r8a77990-sysc.h> > + > + dab: dab@e6730000 { > + compatible = "renesas,dab-r8a77990", > + "renesas,rcar-gen3-dab"; > + reg = <0xe6730000 0x120>; > + interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&cpg CPG_MOD 1016>; > + clock-names = "dab"; > + power-domains = <&sysc R8A77990_PD_ALWAYS_ON>; > + resets = <&cpg 1016>; > + status = "disabled"; > + }; -- Regards, Laurent Pinchart
Hi Fabrizio, Thank you for the patch. On Fri, Feb 26, 2021 at 11:37:44AM +0100, Arnd Bergmann wrote: > On Thu, Feb 25, 2021 at 11:51 PM Fabrizio Castro 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@renesas.com> > > --- > > MAINTAINERS | 7 ++ > > drivers/misc/Kconfig | 1 + > > drivers/misc/Makefile | 1 + > > drivers/misc/rcar_dab/Kconfig | 11 ++ > > drivers/misc/rcar_dab/Makefile | 8 ++ > > drivers/misc/rcar_dab/rcar_dev.c | 176 +++++++++++++++++++++++++++++++ > > drivers/misc/rcar_dab/rcar_dev.h | 116 ++++++++++++++++++++ > > drivers/misc/rcar_dab/rcar_fft.c | 160 ++++++++++++++++++++++++++++ > > include/uapi/linux/rcar_dab.h | 35 ++++++ > > Can you explain why this is not in drivers/media/? > > I don't think we want a custom ioctl interface for a device that implements > a generic specification. My first feeling would be that this should not > have a user-level API but instead get called by the DAB radio driver. > > What is the intended usage model here? I assume the idea is to > use it in an application that receives audio or metadata from DAB. > What driver do you use for that? I second Arnd here, a standard API would be best. > > +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); > > + break; > > + default: > > + ret = -ENOTTY; > > + } > > + > > + return ret; > > +} > > + > > +static const struct file_operations rcar_dab_fops = { > > + .owner = THIS_MODULE, > > + .unlocked_ioctl = rcar_dab_unlocked_ioctl, > > +}; > > There should be a '.compat_ioctl = compat_ptr_ioctl' > entry, provided that the arguments are compatible between > 32-bit and 64-bit user space. > > > + > > +static int rcar_dab_fft_init(struct rcar_dab *dab, struct rcar_dab_fft_req *fft) > > +{ > > + u32 mode; > > + > > + for (mode = 0; mode < ARRAY_SIZE(rcar_dab_fft_size_lut); mode++) > > + if (rcar_dab_fft_size_lut[mode] == fft->points) > > + break; > > + if (mode == ARRAY_SIZE(rcar_dab_fft_size_lut)) > > + return -EINVAL; > > + if (fft->ofdm_number == 0) > > + return -EINVAL; > > + > > + rcar_dab_write(dab, RCAR_DAB_FFTSSR, mode); > > + rcar_dab_write(dab, RCAR_DAB_FFTNUMOFDMR, fft->ofdm_number); > > + rcar_dab_write(dab, RCAR_DAB_FFTINADDR, (u32)dab->fft.dma_input_buf); > > + rcar_dab_write(dab, RCAR_DAB_FFTOUTADDR, (u32)dab->fft.dma_output_buf); > > Maybe use lower_32_bits() instead of the (u32) cast. > > For clarity, you may also want to specifically ask for a 32-bit DMA mask > in the probe function, with a comment that describes what the hardware > limitation is. > > > + > > + 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; > > + 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) { > > + rcar_dab_fft_disable(dab); > > + ret = -EFAULT; > > -EFAULT doesn't look like the right error for timeout or signal > handling. Better check the return code from wait_event_interruptible_timeout() > instead. > > > + > > +struct rcar_dab_fft_req { > > + int points; /* > > + * The number of points to use. > > + * Legal values are 256, 512, 1024, and > > + * 2048. > > + */ > > + unsigned char ofdm_number; /* > > + * Orthogonal Frequency Division > > + * Multiplexing (OFDM). > > + * Minimum value is 1, maximum value is > > + * 255. > > + */ > > + void __user *input_address; /* > > + * User space address for the input > > + * buffer. > > + */ > > + void __user *output_address; /* > > + * User space address for the output > > + * buffer. > > + */ > > +}; > > Please read Documentation/driver-api/ioctl.rst and make this a portable > data structure. We've suffered enough with DMA to user pointers. Let's use dmabuf instead. -- Regards, Laurent Pinchart
Hi Laurent, On Fri, Feb 26, 2021 at 1:53 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Thu, Feb 25, 2021 at 10:51:47PM +0000, Fabrizio Castro wrote: > > Make sure that the R-Car DAB device driver gets compiled as a > > module since R-Car E3 and R-Car M3-N come with the DAB IP. > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > Do we need this in the defconfig ? It's not required to have a bootable > E3 or M3-N with the set of standard features, and would result in all That's why it's modular. > ARM64 platforms having one module they don't care about. We typically add all drivers used by all supported platforms, to increase build coverage. 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
Hi Geert, Thanks for your feedback! > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: 26 February 2021 08:35 > Subject: Re: [PATCH 1/7] clk: renesas: r8a77990: Add DAB clock > > Hi Fabrizio, > > On Thu, Feb 25, 2021 at 11:52 PM Fabrizio Castro > <fabrizio.castro.jz@renesas.com> wrote: > > This patch adds the DAB clock to the R8A77990 SoC. > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > Thanks for your patch! > > > --- a/drivers/clk/renesas/r8a77990-cpg-mssr.c > > +++ b/drivers/clk/renesas/r8a77990-cpg-mssr.c > > @@ -232,6 +232,7 @@ static const struct mssr_mod_clk r8a77990_mod_clks[] > __initconst = { > > DEF_MOD("ssi2", 1013, MOD_CLK_ID(1005)), > > DEF_MOD("ssi1", 1014, MOD_CLK_ID(1005)), > > DEF_MOD("ssi0", 1015, MOD_CLK_ID(1005)), > > + DEF_MOD("dab", 1016, R8A77990_CLK_S3D1), > > Unfortunately this bit is not documented in the R-Car Gen3 Hardware > User's Manual, so I have to trust you on this. Yeah, unfortunately there is no official document with this information as of yet. Cheers, Fab > > > DEF_MOD("scu-all", 1017, R8A77990_CLK_S3D4), > > DEF_MOD("scu-dvc1", 1018, MOD_CLK_ID(1017)), > > DEF_MOD("scu-dvc0", 1019, MOD_CLK_ID(1017)), > > 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
Hi Laurent, Thank you for your feedback! > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: 26 February 2021 12:21 > Subject: Re: [PATCH 0/7] Add FFT Support for R-Car Gen3 devices > > Hi Fabrizio, > > On Thu, Feb 25, 2021 at 10:51:40PM +0000, Fabrizio Castro wrote: > > The DAB hardware accelerator found on R-Car E3 (a.k.a. r8a77990) > > and R-Car M3-N (a.k.a. r8a77965) 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 series adds FFT support only for R-Car E3 and R-Car M3-N, > > FIC and MSC support will be added later on. > > Out of curiosity, could the FFT module be used as an accelerator for 2D > FFT on images ? I haven't tried using it that way but I don't think so. Thanks, Fab > > > Fabrizio Castro (7): > > clk: renesas: r8a77990: Add DAB clock > > clk: renesas: r8a77965: Add DAB clock > > dt-bindings: misc: Add binding for R-Car DAB > > misc: Add driver for DAB IP found on Renesas R-Car devices > > arm64: dts: renesas: r8a77990: Add DAB support > > arm64: dts: renesas: r8a77965: Add DAB support > > arm64: configs: Add R-Car DAB support > > > > .../devicetree/bindings/misc/renesas,dab.yaml | 75 ++++++++ > > MAINTAINERS | 7 + > > arch/arm64/boot/dts/renesas/r8a77965.dtsi | 12 ++ > > arch/arm64/boot/dts/renesas/r8a77990.dtsi | 12 ++ > > arch/arm64/configs/defconfig | 1 + > > drivers/clk/renesas/r8a77965-cpg-mssr.c | 1 + > > drivers/clk/renesas/r8a77990-cpg-mssr.c | 1 + > > drivers/misc/Kconfig | 1 + > > drivers/misc/Makefile | 1 + > > drivers/misc/rcar_dab/Kconfig | 11 ++ > > drivers/misc/rcar_dab/Makefile | 8 + > > drivers/misc/rcar_dab/rcar_dev.c | 176 ++++++++++++++++++ > > drivers/misc/rcar_dab/rcar_dev.h | 116 ++++++++++++ > > drivers/misc/rcar_dab/rcar_fft.c | 160 ++++++++++++++++ > > include/uapi/linux/rcar_dab.h | 35 ++++ > > 15 files changed, 617 insertions(+) > > create mode 100644 > Documentation/devicetree/bindings/misc/renesas,dab.yaml > > create mode 100644 drivers/misc/rcar_dab/Kconfig > > create mode 100644 drivers/misc/rcar_dab/Makefile > > create mode 100644 drivers/misc/rcar_dab/rcar_dev.c > > create mode 100644 drivers/misc/rcar_dab/rcar_dev.h > > create mode 100644 drivers/misc/rcar_dab/rcar_fft.c > > create mode 100644 include/uapi/linux/rcar_dab.h > > -- > Regards, > > Laurent Pinchart
Hi Geert, Thanks for your feedback! > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: 26 February 2021 08:41 > Subject: Re: [PATCH 3/7] dt-bindings: misc: Add binding for R-Car DAB > > Hi Fabrizio, > > On Thu, Feb 25, 2021 at 11:53 PM Fabrizio Castro > <fabrizio.castro.jz@renesas.com> wrote: > > Document bindings for R-Car DAB hardware accelerator, currently > > found on the r8a77990 SoC (a.k.a. R-Car E3) and on the r8a77965 > > SoC (a.k.a. R-Car M3-N). > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > Thanks for your patch! > > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/misc/renesas,dab.yaml > > > +properties: > > + compatible: > > + items: > > + - enum: > > + - renesas,dab-r8a77965 # R-Car M3-N > > + - renesas,dab-r8a77990 # R-Car E3 > > Please use the recommended order for new bindings: > > renesas,r8a77965-dab > renesas,r8a77990-dab Sorry about this, will change. > > > + - const: renesas,rcar-gen3-dab # Generic fallback for R-Car Gen3 > devices > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > + > > + clock-names: > > + const: dab > > fck? Agreed. 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
Hello Sergei, Thanks for your feedback! > From: Sergei Shtylyov <sergei.shtylyov@gmail.com> > Sent: 26 February 2021 09:07 > Subject: Re: [PATCH 3/7] dt-bindings: misc: Add binding for R-Car DAB > > Hello! > > On 26.02.2021 1:51, Fabrizio Castro wrote: > > > Document bindings for R-Car DAB hardware accelerator, currently > > found on the r8a77990 SoC (a.k.a. R-Car E3) and on the r8a77965 > > SoC (a.k.a. R-Car M3-N). > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > --- > > .../devicetree/bindings/misc/renesas,dab.yaml | 75 +++++++++++++++++++ > > 1 file changed, 75 insertions(+) > > create mode 100644 > Documentation/devicetree/bindings/misc/renesas,dab.yaml > > > > diff --git a/Documentation/devicetree/bindings/misc/renesas,dab.yaml > b/Documentation/devicetree/bindings/misc/renesas,dab.yaml > > new file mode 100644 > > index 000000000000..e9494add13d7 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/misc/renesas,dab.yaml > > @@ -0,0 +1,75 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +# Copyright (C) 2021 Renesas Electronics Corporation > > +%YAML 1.2 > > +--- > > +$id: > https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetre > e.org%2Fschemas%2Fmisc%2Frenesas%2Cdab.yaml%23&data=04%7C01%7Cfabrizio > .castro.jz%40renesas.com%7C4ea11a8f46fc4f4642ad08d8da35d682%7C53d82571da19 > 47e49cb4625a166a4a2a%7C0%7C0%7C637499272091083125%7CUnknown%7CTWFpbGZsb3d8 > eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000& > amp;sdata=p5MNncCLOIMaYLyBtmOl%2Br%2BIKe9ByUqxv1k05FbYj94%3D&reserved= > 0 > > +$schema: > https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetre > e.org%2Fmeta- > schemas%2Fcore.yaml%23&data=04%7C01%7Cfabrizio.castro.jz%40renesas.com > %7C4ea11a8f46fc4f4642ad08d8da35d682%7C53d82571da1947e49cb4625a166a4a2a%7C0 > %7C0%7C637499272091083125%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQ > IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=dBI8WMPjxAEvhY > 3cj7WKJW4FjCrfN5JpAoqC9XSPNaY%3D&reserved=0 > > + > > +title: Renesas R-Car DAB Hardware Accelerator > > + > > +maintainers: > > + - Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > + > > +description: > > + The DAB hardware accelerator found on some R-Car 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. > > + > > +properties: > > + compatible: > > + items: > > + - enum: > > + - renesas,dab-r8a77965 # R-Car M3-N > > + - renesas,dab-r8a77990 # R-Car E3 > > Why not renesas,<soc>-dab? Will change. Thanks, Fab > > > + - const: renesas,rcar-gen3-dab # Generic fallback for R-Car Gen3 > devices > [...] > > MBR, Sergei
Hi Laurent, Thanks for your feedback! > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: 26 February 2021 13:02 > Subject: Re: [PATCH 3/7] dt-bindings: misc: Add binding for R-Car DAB > > Hi Fabrizio, > > Thank you for the patch. > > On Thu, Feb 25, 2021 at 10:51:43PM +0000, Fabrizio Castro wrote: > > Document bindings for R-Car DAB hardware accelerator, currently > > found on the r8a77990 SoC (a.k.a. R-Car E3) and on the r8a77965 > > SoC (a.k.a. R-Car M3-N). > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > --- > > .../devicetree/bindings/misc/renesas,dab.yaml | 75 +++++++++++++++++++ > > 1 file changed, 75 insertions(+) > > create mode 100644 > Documentation/devicetree/bindings/misc/renesas,dab.yaml > > > > diff --git a/Documentation/devicetree/bindings/misc/renesas,dab.yaml > b/Documentation/devicetree/bindings/misc/renesas,dab.yaml > > new file mode 100644 > > index 000000000000..e9494add13d7 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/misc/renesas,dab.yaml > > @@ -0,0 +1,75 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +# Copyright (C) 2021 Renesas Electronics Corporation > > +%YAML 1.2 > > +--- > > +$id: > https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetre > e.org%2Fschemas%2Fmisc%2Frenesas%2Cdab.yaml%23&data=04%7C01%7Cfabrizio > .castro.jz%40renesas.com%7Cb383aa9cfef34b6653e008d8da56c204%7C53d82571da19 > 47e49cb4625a166a4a2a%7C0%7C0%7C637499413468300421%7CUnknown%7CTWFpbGZsb3d8 > eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000& > amp;sdata=r95fhwTCpf0gkNoRtCLrUbZtaCbI3da9sbFLv0UXipE%3D&reserved=0 > > +$schema: > https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetre > e.org%2Fmeta- > schemas%2Fcore.yaml%23&data=04%7C01%7Cfabrizio.castro.jz%40renesas.com > %7Cb383aa9cfef34b6653e008d8da56c204%7C53d82571da1947e49cb4625a166a4a2a%7C0 > %7C0%7C637499413468300421%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQ > IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ajJ%2BmBs4zOzK > cJghYY89B6PMgrxRFgoVVuflZCFmHYc%3D&reserved=0 > > + > > +title: Renesas R-Car DAB Hardware Accelerator > > + > > +maintainers: > > + - Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > + > > +description: > > + The DAB hardware accelerator found on some R-Car 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. > > + > > +properties: > > + compatible: > > + items: > > + - enum: > > + - renesas,dab-r8a77965 # R-Car M3-N > > + - renesas,dab-r8a77990 # R-Car E3 > > + - const: renesas,rcar-gen3-dab # Generic fallback for R-Car Gen3 > devices > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > + clocks: > > + maxItems: 1 > > I usually try to describe clocks: > > clocks: > items: > - description: The module functional clock > > but as there's a single clock, it may not be worth it. Up to you. Will change as per your suggestion. Thanks, Fab > > > + > > + clock-names: > > + const: dab > > With Geert's and Sergei's comments addressed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + > > + power-domains: > > + maxItems: 1 > > + > > + resets: > > + maxItems: 1 > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - clocks > > + - clock-names > > + - power-domains > > + - resets > > + > > +additionalProperties: false > > + > > +examples: > > + # R8A77990 (R-Car E3) > > + - | > > + #include <dt-bindings/clock/r8a77990-cpg-mssr.h> > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #include <dt-bindings/power/r8a77990-sysc.h> > > + > > + dab: dab@e6730000 { > > + compatible = "renesas,dab-r8a77990", > > + "renesas,rcar-gen3-dab"; > > + reg = <0xe6730000 0x120>; > > + interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&cpg CPG_MOD 1016>; > > + clock-names = "dab"; > > + power-domains = <&sysc R8A77990_PD_ALWAYS_ON>; > > + resets = <&cpg 1016>; > > + status = "disabled"; > > + }; > > -- > Regards, > > Laurent Pinchart
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 > <fabrizio.castro.jz@renesas.com> 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@renesas.com> > > 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
Hi Arnd, Thanks for your feedback! > From: Arnd Bergmann <arnd@kernel.org> > Sent: 26 February 2021 10:38 > Subject: Re: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas R- > Car devices > > On Thu, Feb 25, 2021 at 11:51 PM Fabrizio Castro > <fabrizio.castro.jz@renesas.com> 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@renesas.com> > > --- > > MAINTAINERS | 7 ++ > > drivers/misc/Kconfig | 1 + > > drivers/misc/Makefile | 1 + > > drivers/misc/rcar_dab/Kconfig | 11 ++ > > drivers/misc/rcar_dab/Makefile | 8 ++ > > drivers/misc/rcar_dab/rcar_dev.c | 176 +++++++++++++++++++++++++++++++ > > drivers/misc/rcar_dab/rcar_dev.h | 116 ++++++++++++++++++++ > > drivers/misc/rcar_dab/rcar_fft.c | 160 ++++++++++++++++++++++++++++ > > include/uapi/linux/rcar_dab.h | 35 ++++++ > > Can you explain why this is not in drivers/media/? I wasn't entirely sure were to put it to be honest as I couldn't find anything similar, that's why "misc" for v1, to mainly get a feedback and advice. > > I don't think we want a custom ioctl interface for a device that > implements > a generic specification. My first feeling would be that this should not > have a user-level API but instead get called by the DAB radio driver. I hear you, the trouble is that the modules of this IP should be seen as part of a SW and HW processing pipeline. Some of the SW components of the pipeline can be proprietary, and unfortunately we don't have access (yet) to a framework that allows us to test and demonstrate the whole pipeline, for the moment we can only test things in isolation. It'll take us a while to come up with a full solution (or to have access to one), and in the meantime our SoCs come with an IP that can't be used because there is no driver for it (yet). After discussing things further with Geert and Laurent, I think they are right in saying that the best path for this is probably to add this driver under "drivers/staging" as an interim solution, so that the IP will be accessible by developers, and when we have everything we need (a fully fledged processing framework), we will able to integrate it better with the other components (if possible). > > What is the intended usage model here? I assume the idea is to > use it in an application that receives audio or metadata from DAB. > What driver do you use for that? This IP is similar to a DMA to some extent, in the sense that it takes input data from a buffer in memory and it writes output data onto a buffer in memory, and of course it does some processing in between. It's not directly connected to any other Radio related IP. The user space application can read DAB IQ samples from the R-Car DRIF interface (via driver drivers/media/platform/rcar_drif.c, or from other sources since this IP is decoupled from DRIF), and then when it's time to accelerate FFT, FIC, or MSC, it can request services to the DAB IP, so that the app can go on and use the processor to do some work, while the DAB IP processes things. A framework to connect and exchange processing blocks (either SW or HW) and interfaces is therefore vital to properly place a kernel implementation in the great scheme of things, in its absence a simple driver can help us and users to integrate DAB acceleration within an interim DAB processing pipeline, and that will lead to better interfaces in the future. > > > +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); > > + break; > > + default: > > + ret = -ENOTTY; > > + } > > + > > + return ret; > > +} > > + > > +static const struct file_operations rcar_dab_fops = { > > + .owner = THIS_MODULE, > > + .unlocked_ioctl = rcar_dab_unlocked_ioctl, > > +}; > > There should be a '.compat_ioctl = compat_ptr_ioctl' > entry, provided that the arguments are compatible between > 32-bit and 64-bit user space. Will add > > > + > > +static int rcar_dab_fft_init(struct rcar_dab *dab, struct > rcar_dab_fft_req *fft) > > +{ > > + u32 mode; > > + > > + for (mode = 0; mode < ARRAY_SIZE(rcar_dab_fft_size_lut); mode++) > > + if (rcar_dab_fft_size_lut[mode] == fft->points) > > + break; > > + if (mode == ARRAY_SIZE(rcar_dab_fft_size_lut)) > > + return -EINVAL; > > + if (fft->ofdm_number == 0) > > + return -EINVAL; > > + > > + rcar_dab_write(dab, RCAR_DAB_FFTSSR, mode); > > + rcar_dab_write(dab, RCAR_DAB_FFTNUMOFDMR, fft->ofdm_number); > > + rcar_dab_write(dab, RCAR_DAB_FFTINADDR, (u32)dab- > >fft.dma_input_buf); > > + rcar_dab_write(dab, RCAR_DAB_FFTOUTADDR, (u32)dab- > >fft.dma_output_buf); > > Maybe use lower_32_bits() instead of the (u32) cast. > > For clarity, you may also want to specifically ask for a 32-bit DMA mask > in the probe function, with a comment that describes what the hardware > limitation is. Thanks. > > > + > > + 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; > > + 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) { > > + rcar_dab_fft_disable(dab); > > + ret = -EFAULT; > > -EFAULT doesn't look like the right error for timeout or signal > handling. Better check the return code from > wait_event_interruptible_timeout() > instead. Will do > > > + > > +struct rcar_dab_fft_req { > > + int points; /* > > + * The number of points to use. > > + * Legal values are 256, 512, > 1024, and > > + * 2048. > > + */ > > + unsigned char ofdm_number; /* > > + * Orthogonal Frequency Division > > + * Multiplexing (OFDM). > > + * Minimum value is 1, maximum > value is > > + * 255. > > + */ > > + void __user *input_address; /* > > + * User space address for the > input > > + * buffer. > > + */ > > + void __user *output_address; /* > > + * User space address for the > output > > + * buffer. > > + */ > > +}; > > Please read Documentation/driver-api/ioctl.rst and make this a portable > data structure. Thanks, Fab > > Arnd
Hi Laurent, Thank you for your feedback! > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: 26 February 2021 13:05 > Subject: Re: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas R- > Car devices > > Hi Fabrizio, > > Thank you for the patch. > > On Fri, Feb 26, 2021 at 11:37:44AM +0100, Arnd Bergmann wrote: > > On Thu, Feb 25, 2021 at 11:51 PM Fabrizio Castro 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@renesas.com> > > > --- > > > MAINTAINERS | 7 ++ > > > drivers/misc/Kconfig | 1 + > > > drivers/misc/Makefile | 1 + > > > drivers/misc/rcar_dab/Kconfig | 11 ++ > > > drivers/misc/rcar_dab/Makefile | 8 ++ > > > drivers/misc/rcar_dab/rcar_dev.c | 176 > +++++++++++++++++++++++++++++++ > > > drivers/misc/rcar_dab/rcar_dev.h | 116 ++++++++++++++++++++ > > > drivers/misc/rcar_dab/rcar_fft.c | 160 ++++++++++++++++++++++++++++ > > > include/uapi/linux/rcar_dab.h | 35 ++++++ > > > > Can you explain why this is not in drivers/media/? > > > > I don't think we want a custom ioctl interface for a device that > implements > > a generic specification. My first feeling would be that this should not > > have a user-level API but instead get called by the DAB radio driver. > > > > What is the intended usage model here? I assume the idea is to > > use it in an application that receives audio or metadata from DAB. > > What driver do you use for that? > > I second Arnd here, a standard API would be best. > > > > +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); > > > + break; > > > + default: > > > + ret = -ENOTTY; > > > + } > > > + > > > + return ret; > > > +} > > > + > > > +static const struct file_operations rcar_dab_fops = { > > > + .owner = THIS_MODULE, > > > + .unlocked_ioctl = rcar_dab_unlocked_ioctl, > > > +}; > > > > There should be a '.compat_ioctl = compat_ptr_ioctl' > > entry, provided that the arguments are compatible between > > 32-bit and 64-bit user space. > > > > > + > > > +static int rcar_dab_fft_init(struct rcar_dab *dab, struct > rcar_dab_fft_req *fft) > > > +{ > > > + u32 mode; > > > + > > > + for (mode = 0; mode < ARRAY_SIZE(rcar_dab_fft_size_lut); > mode++) > > > + if (rcar_dab_fft_size_lut[mode] == fft->points) > > > + break; > > > + if (mode == ARRAY_SIZE(rcar_dab_fft_size_lut)) > > > + return -EINVAL; > > > + if (fft->ofdm_number == 0) > > > + return -EINVAL; > > > + > > > + rcar_dab_write(dab, RCAR_DAB_FFTSSR, mode); > > > + rcar_dab_write(dab, RCAR_DAB_FFTNUMOFDMR, fft->ofdm_number); > > > + rcar_dab_write(dab, RCAR_DAB_FFTINADDR, (u32)dab- > >fft.dma_input_buf); > > > + rcar_dab_write(dab, RCAR_DAB_FFTOUTADDR, (u32)dab- > >fft.dma_output_buf); > > > > Maybe use lower_32_bits() instead of the (u32) cast. > > > > For clarity, you may also want to specifically ask for a 32-bit DMA mask > > in the probe function, with a comment that describes what the hardware > > limitation is. > > > > > + > > > + 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; > > > + 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) { > > > + rcar_dab_fft_disable(dab); > > > + ret = -EFAULT; > > > > -EFAULT doesn't look like the right error for timeout or signal > > handling. Better check the return code from > wait_event_interruptible_timeout() > > instead. > > > > > + > > > +struct rcar_dab_fft_req { > > > + int points; /* > > > + * The number of points to > use. > > > + * Legal values are 256, 512, > 1024, and > > > + * 2048. > > > + */ > > > + unsigned char ofdm_number; /* > > > + * Orthogonal Frequency > Division > > > + * Multiplexing (OFDM). > > > + * Minimum value is 1, maximum > value is > > > + * 255. > > > + */ > > > + void __user *input_address; /* > > > + * User space address for the > input > > > + * buffer. > > > + */ > > > + void __user *output_address; /* > > > + * User space address for the > output > > > + * buffer. > > > + */ > > > +}; > > > > Please read Documentation/driver-api/ioctl.rst and make this a portable > > data structure. > > We've suffered enough with DMA to user pointers. Let's use dmabuf > instead. Will give it a try Thanks, Fab > > -- > Regards, > > Laurent Pinchart
Hello everyone, On Mon, 1 Mar 2021 at 14:36, Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote: > > Hi Arnd, > > Thanks for your feedback! > > > From: Arnd Bergmann <arnd@kernel.org> > > Sent: 26 February 2021 10:38 > > Subject: Re: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas R- > > Car devices > > > > On Thu, Feb 25, 2021 at 11:51 PM Fabrizio Castro > > <fabrizio.castro.jz@renesas.com> 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@renesas.com> > > > --- > > > MAINTAINERS | 7 ++ > > > drivers/misc/Kconfig | 1 + > > > drivers/misc/Makefile | 1 + > > > drivers/misc/rcar_dab/Kconfig | 11 ++ > > > drivers/misc/rcar_dab/Makefile | 8 ++ > > > drivers/misc/rcar_dab/rcar_dev.c | 176 +++++++++++++++++++++++++++++++ > > > drivers/misc/rcar_dab/rcar_dev.h | 116 ++++++++++++++++++++ > > > drivers/misc/rcar_dab/rcar_fft.c | 160 ++++++++++++++++++++++++++++ > > > include/uapi/linux/rcar_dab.h | 35 ++++++ > > > > Can you explain why this is not in drivers/media/? > > I wasn't entirely sure were to put it to be honest as I couldn't find > anything similar, that's why "misc" for v1, to mainly get a feedback > and advice. > > > > > I don't think we want a custom ioctl interface for a device that > > implements > > a generic specification. My first feeling would be that this should not > > have a user-level API but instead get called by the DAB radio driver. > > I hear you, the trouble is that the modules of this IP should be seen > as part of a SW and HW processing pipeline. > Some of the SW components of the pipeline can be proprietary, and > unfortunately we don't have access (yet) to a framework that allows us to > test and demonstrate the whole pipeline, for the moment we can only test > things in isolation. It'll take us a while to come up with a full solution > (or to have access to one), and in the meantime our SoCs come with an IP > that can't be used because there is no driver for it (yet). > > After discussing things further with Geert and Laurent, I think they > are right in saying that the best path for this is probably to add this > driver under "drivers/staging" as an interim solution, so that the IP will > be accessible by developers, and when we have everything we need (a fully > fledged processing framework), we will able to integrate it better with > the other components (if possible). > > > > > What is the intended usage model here? I assume the idea is to > > use it in an application that receives audio or metadata from DAB. > > What driver do you use for that? > > This IP is similar to a DMA to some extent, in the sense that it takes > input data from a buffer in memory and it writes output data onto a buffer > in memory, and of course it does some processing in between. That sounds like something that can fit V4L2 MEM2MEM driver. You queue two buffers and an operation, and then run a job. > It's not directly connected to any other Radio related IP. > The user space application can read DAB IQ samples from the R-Car DRIF > interface (via driver drivers/media/platform/rcar_drif.c, or from other > sources since this IP is decoupled from DRIF), and then when it's time > to accelerate FFT, FIC, or MSC, it can request services to the DAB IP, so > that the app can go on and use the processor to do some work, while the DAB > IP processes things. > A framework to connect and exchange processing blocks (either SW or HW) and > interfaces is therefore vital to properly place a kernel implementation > in the great scheme of things, in its absence a simple driver can help I'm not entirely sure we are missing a framework. What's missing in V4L2 MEM2MEM? Before considering drivers/staging, it would be interesting to figure out if V4L2 can do it as-is, and if not, discuss what's missing. Thanks, Ezequiel
Hello Ezequiel, Thanks for your feedback! > From: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > Sent: 02 March 2021 11:17 > Subject: Re: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas R- > Car devices > > Hello everyone, > > On Mon, 1 Mar 2021 at 14:36, Fabrizio Castro > <fabrizio.castro.jz@renesas.com> wrote: > > > > Hi Arnd, > > > > Thanks for your feedback! > > > > > From: Arnd Bergmann <arnd@kernel.org> > > > Sent: 26 February 2021 10:38 > > > Subject: Re: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas > R- > > > Car devices > > > > > > On Thu, Feb 25, 2021 at 11:51 PM Fabrizio Castro > > > <fabrizio.castro.jz@renesas.com> 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@renesas.com> > > > > --- > > > > MAINTAINERS | 7 ++ > > > > drivers/misc/Kconfig | 1 + > > > > drivers/misc/Makefile | 1 + > > > > drivers/misc/rcar_dab/Kconfig | 11 ++ > > > > drivers/misc/rcar_dab/Makefile | 8 ++ > > > > drivers/misc/rcar_dab/rcar_dev.c | 176 > +++++++++++++++++++++++++++++++ > > > > drivers/misc/rcar_dab/rcar_dev.h | 116 ++++++++++++++++++++ > > > > drivers/misc/rcar_dab/rcar_fft.c | 160 ++++++++++++++++++++++++++++ > > > > include/uapi/linux/rcar_dab.h | 35 ++++++ > > > > > > Can you explain why this is not in drivers/media/? > > > > I wasn't entirely sure were to put it to be honest as I couldn't find > > anything similar, that's why "misc" for v1, to mainly get a feedback > > and advice. > > > > > > > > I don't think we want a custom ioctl interface for a device that > > > implements > > > a generic specification. My first feeling would be that this should > not > > > have a user-level API but instead get called by the DAB radio driver. > > > > I hear you, the trouble is that the modules of this IP should be seen > > as part of a SW and HW processing pipeline. > > Some of the SW components of the pipeline can be proprietary, and > > unfortunately we don't have access (yet) to a framework that allows us > to > > test and demonstrate the whole pipeline, for the moment we can only test > > things in isolation. It'll take us a while to come up with a full > solution > > (or to have access to one), and in the meantime our SoCs come with an IP > > that can't be used because there is no driver for it (yet). > > > > After discussing things further with Geert and Laurent, I think they > > are right in saying that the best path for this is probably to add this > > driver under "drivers/staging" as an interim solution, so that the IP > will > > be accessible by developers, and when we have everything we need (a > fully > > fledged processing framework), we will able to integrate it better with > > the other components (if possible). > > > > > > > > What is the intended usage model here? I assume the idea is to > > > use it in an application that receives audio or metadata from DAB. > > > What driver do you use for that? > > > > This IP is similar to a DMA to some extent, in the sense that it takes > > input data from a buffer in memory and it writes output data onto a > buffer > > in memory, and of course it does some processing in between. > > That sounds like something that can fit V4L2 MEM2MEM driver. > You queue two buffers and an operation, and then run a job. V4L2 MEM2MEM seems good for this in general, however the DAB IP is going to be shared by multiple processing pipelines (as usually we have several DRIF interfaces, and only 1 DAB IP), and for what concerns FFT specifically there is only 1 FFT module inside the DAB IP. My understanding is that the capabilities of V4L2 MEM2MEM devices are configured for the specific pipeline, but in the this context user space would have to continuously switch the capabilities of the DAB IP (at the right moment) to allow processing for data streams requiring different capabilities. Am I wrong? > > > It's not directly connected to any other Radio related IP. > > The user space application can read DAB IQ samples from the R-Car DRIF > > interface (via driver drivers/media/platform/rcar_drif.c, or from other > > sources since this IP is decoupled from DRIF), and then when it's time > > to accelerate FFT, FIC, or MSC, it can request services to the DAB IP, > so > > that the app can go on and use the processor to do some work, while the > DAB > > IP processes things. > > A framework to connect and exchange processing blocks (either SW or HW) > and > > interfaces is therefore vital to properly place a kernel implementation > > in the great scheme of things, in its absence a simple driver can help > > I'm not entirely sure we are missing a framework. What's missing in > V4L2 MEM2MEM? I was referring to a user space framework (I should have been more specific with my previous email). > > Before considering drivers/staging, it would be interesting to figure > out if V4L2 can do it as-is, and if not, discuss what's missing. I think an interim solution would allow us and users to evaluate things a little bit better, so that we can integrate this IP with V4L2 later on. Thanks, Fab > > Thanks, > Ezequiel
Hi Fabrizio, On Tue, Mar 02, 2021 at 12:20:17PM +0000, Fabrizio Castro wrote: > On 02 March 2021 11:17, Ezequiel Garcia wrote: > > On Mon, 1 Mar 2021 at 14:36, Fabrizio Castro wrote: > > > On 26 February 2021 10:38, Arnd Bergmann wrote: > > > > On Thu, Feb 25, 2021 at 11:51 PM Fabrizio Castro 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@renesas.com> > > > > > --- > > > > > MAINTAINERS | 7 ++ > > > > > drivers/misc/Kconfig | 1 + > > > > > drivers/misc/Makefile | 1 + > > > > > drivers/misc/rcar_dab/Kconfig | 11 ++ > > > > > drivers/misc/rcar_dab/Makefile | 8 ++ > > > > > drivers/misc/rcar_dab/rcar_dev.c | 176 +++++++++++++++++++++++++++++++ > > > > > drivers/misc/rcar_dab/rcar_dev.h | 116 ++++++++++++++++++++ > > > > > drivers/misc/rcar_dab/rcar_fft.c | 160 ++++++++++++++++++++++++++++ > > > > > include/uapi/linux/rcar_dab.h | 35 ++++++ > > > > > > > > Can you explain why this is not in drivers/media/? > > > > > > I wasn't entirely sure were to put it to be honest as I couldn't find > > > anything similar, that's why "misc" for v1, to mainly get a feedback > > > and advice. > > > > > > > I don't think we want a custom ioctl interface for a device that > > > > implements > > > > a generic specification. My first feeling would be that this should not > > > > have a user-level API but instead get called by the DAB radio driver. > > > > > > I hear you, the trouble is that the modules of this IP should be seen > > > as part of a SW and HW processing pipeline. > > > Some of the SW components of the pipeline can be proprietary, and > > > unfortunately we don't have access (yet) to a framework that allows us to > > > test and demonstrate the whole pipeline, for the moment we can only test > > > things in isolation. It'll take us a while to come up with a full solution > > > (or to have access to one), and in the meantime our SoCs come with an IP > > > that can't be used because there is no driver for it (yet). > > > > > > After discussing things further with Geert and Laurent, I think they > > > are right in saying that the best path for this is probably to add this > > > driver under "drivers/staging" as an interim solution, so that the IP will > > > be accessible by developers, and when we have everything we need (a fully > > > fledged processing framework), we will able to integrate it better with > > > the other components (if possible). > > > > > > > What is the intended usage model here? I assume the idea is to > > > > use it in an application that receives audio or metadata from DAB. > > > > What driver do you use for that? > > > > > > This IP is similar to a DMA to some extent, in the sense that it takes > > > input data from a buffer in memory and it writes output data onto a buffer > > > in memory, and of course it does some processing in between. > > > > That sounds like something that can fit V4L2 MEM2MEM driver. > > You queue two buffers and an operation, and then run a job. > > V4L2 MEM2MEM seems good for this in general, however the DAB IP is going > to be shared by multiple processing pipelines (as usually we have several > DRIF interfaces, and only 1 DAB IP), and for what concerns FFT specifically > there is only 1 FFT module inside the DAB IP. > My understanding is that the capabilities of V4L2 MEM2MEM devices are > configured for the specific pipeline, but in the this context user space > would have to continuously switch the capabilities of the DAB IP (at the > right moment) to allow processing for data streams requiring different > capabilities. > > Am I wrong? V4L2 M2M devices can be opened multiple times, but different processes, which can configure the device in different ways, and submit jobs that are then scheduled by the V4L2 M2M framework. > > > It's not directly connected to any other Radio related IP. > > > The user space application can read DAB IQ samples from the R-Car DRIF > > > interface (via driver drivers/media/platform/rcar_drif.c, or from other > > > sources since this IP is decoupled from DRIF), and then when it's time > > > to accelerate FFT, FIC, or MSC, it can request services to the DAB IP, so > > > that the app can go on and use the processor to do some work, while the DAB > > > IP processes things. > > > A framework to connect and exchange processing blocks (either SW or HW) and > > > interfaces is therefore vital to properly place a kernel implementation > > > in the great scheme of things, in its absence a simple driver can help > > > > I'm not entirely sure we are missing a framework. What's missing in > > V4L2 MEM2MEM? > > I was referring to a user space framework (I should have been more specific > with my previous email). > > > Before considering drivers/staging, it would be interesting to figure > > out if V4L2 can do it as-is, and if not, discuss what's missing. > > I think an interim solution would allow us and users to evaluate things a > little bit better, so that we can integrate this IP with V4L2 later on.
Hi Laurent, Thanks for your feedback! > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: 02 March 2021 12:32 > Subject: Re: [PATCH 4/7] misc: Add driver for DAB IP found on Renesas R- > Car devices > > Hi Fabrizio, > > On Tue, Mar 02, 2021 at 12:20:17PM +0000, Fabrizio Castro wrote: > > On 02 March 2021 11:17, Ezequiel Garcia wrote: > > > On Mon, 1 Mar 2021 at 14:36, Fabrizio Castro wrote: > > > > On 26 February 2021 10:38, Arnd Bergmann wrote: > > > > > On Thu, Feb 25, 2021 at 11:51 PM Fabrizio Castro 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@renesas.com> > > > > > > --- > > > > > > MAINTAINERS | 7 ++ > > > > > > drivers/misc/Kconfig | 1 + > > > > > > drivers/misc/Makefile | 1 + > > > > > > drivers/misc/rcar_dab/Kconfig | 11 ++ > > > > > > drivers/misc/rcar_dab/Makefile | 8 ++ > > > > > > drivers/misc/rcar_dab/rcar_dev.c | 176 > +++++++++++++++++++++++++++++++ > > > > > > drivers/misc/rcar_dab/rcar_dev.h | 116 ++++++++++++++++++++ > > > > > > drivers/misc/rcar_dab/rcar_fft.c | 160 > ++++++++++++++++++++++++++++ > > > > > > include/uapi/linux/rcar_dab.h | 35 ++++++ > > > > > > > > > > Can you explain why this is not in drivers/media/? > > > > > > > > I wasn't entirely sure were to put it to be honest as I couldn't > find > > > > anything similar, that's why "misc" for v1, to mainly get a feedback > > > > and advice. > > > > > > > > > I don't think we want a custom ioctl interface for a device that > > > > > implements > > > > > a generic specification. My first feeling would be that this > should not > > > > > have a user-level API but instead get called by the DAB radio > driver. > > > > > > > > I hear you, the trouble is that the modules of this IP should be > seen > > > > as part of a SW and HW processing pipeline. > > > > Some of the SW components of the pipeline can be proprietary, and > > > > unfortunately we don't have access (yet) to a framework that allows > us to > > > > test and demonstrate the whole pipeline, for the moment we can only > test > > > > things in isolation. It'll take us a while to come up with a full > solution > > > > (or to have access to one), and in the meantime our SoCs come with > an IP > > > > that can't be used because there is no driver for it (yet). > > > > > > > > After discussing things further with Geert and Laurent, I think they > > > > are right in saying that the best path for this is probably to add > this > > > > driver under "drivers/staging" as an interim solution, so that the > IP will > > > > be accessible by developers, and when we have everything we need (a > fully > > > > fledged processing framework), we will able to integrate it better > with > > > > the other components (if possible). > > > > > > > > > What is the intended usage model here? I assume the idea is to > > > > > use it in an application that receives audio or metadata from DAB. > > > > > What driver do you use for that? > > > > > > > > This IP is similar to a DMA to some extent, in the sense that it > takes > > > > input data from a buffer in memory and it writes output data onto a > buffer > > > > in memory, and of course it does some processing in between. > > > > > > That sounds like something that can fit V4L2 MEM2MEM driver. > > > You queue two buffers and an operation, and then run a job. > > > > V4L2 MEM2MEM seems good for this in general, however the DAB IP is going > > to be shared by multiple processing pipelines (as usually we have > several > > DRIF interfaces, and only 1 DAB IP), and for what concerns FFT > specifically > > there is only 1 FFT module inside the DAB IP. > > My understanding is that the capabilities of V4L2 MEM2MEM devices are > > configured for the specific pipeline, but in the this context user space > > would have to continuously switch the capabilities of the DAB IP (at the > > right moment) to allow processing for data streams requiring different > > capabilities. > > > > Am I wrong? > > V4L2 M2M devices can be opened multiple times, but different processes, > which can configure the device in different ways, and submit jobs that > are then scheduled by the V4L2 M2M framework. I'll give it a try in due time then. I think the clock related patches are worth merging. Thanks, Fab > > > > > It's not directly connected to any other Radio related IP. > > > > The user space application can read DAB IQ samples from the R-Car > DRIF > > > > interface (via driver drivers/media/platform/rcar_drif.c, or from > other > > > > sources since this IP is decoupled from DRIF), and then when it's > time > > > > to accelerate FFT, FIC, or MSC, it can request services to the DAB > IP, so > > > > that the app can go on and use the processor to do some work, while > the DAB > > > > IP processes things. > > > > A framework to connect and exchange processing blocks (either SW or > HW) and > > > > interfaces is therefore vital to properly place a kernel > implementation > > > > in the great scheme of things, in its absence a simple driver can > help > > > > > > I'm not entirely sure we are missing a framework. What's missing in > > > V4L2 MEM2MEM? > > > > I was referring to a user space framework (I should have been more > specific > > with my previous email). > > > > > Before considering drivers/staging, it would be interesting to figure > > > out if V4L2 can do it as-is, and if not, discuss what's missing. > > > > I think an interim solution would allow us and users to evaluate things > a > > little bit better, so that we can integrate this IP with V4L2 later on. > > -- > Regards, > > Laurent Pinchart
Hi Geert, > From: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > Sent: 01 March 2021 14:47 > Subject: RE: [PATCH 1/7] clk: renesas: r8a77990: Add DAB clock > > Hi Geert, > > Thanks for your feedback! > > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > Sent: 26 February 2021 08:35 > > Subject: Re: [PATCH 1/7] clk: renesas: r8a77990: Add DAB clock > > > > Hi Fabrizio, > > > > On Thu, Feb 25, 2021 at 11:52 PM Fabrizio Castro > > <fabrizio.castro.jz@renesas.com> wrote: > > > This patch adds the DAB clock to the R8A77990 SoC. > > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > > > Thanks for your patch! > > > > > --- a/drivers/clk/renesas/r8a77990-cpg-mssr.c > > > +++ b/drivers/clk/renesas/r8a77990-cpg-mssr.c > > > @@ -232,6 +232,7 @@ static const struct mssr_mod_clk > r8a77990_mod_clks[] > > __initconst = { > > > DEF_MOD("ssi2", 1013, MOD_CLK_ID(1005)), > > > DEF_MOD("ssi1", 1014, MOD_CLK_ID(1005)), > > > DEF_MOD("ssi0", 1015, MOD_CLK_ID(1005)), > > > + DEF_MOD("dab", 1016, R8A77990_CLK_S3D1), > > > > Unfortunately this bit is not documented in the R-Car Gen3 Hardware > > User's Manual, so I have to trust you on this. > > Yeah, unfortunately there is no official document with this information > as of yet. I think this patch is worth taking. Thanks, Fab > > Cheers, > Fab > > > > > > DEF_MOD("scu-all", 1017, R8A77990_CLK_S3D4), > > > DEF_MOD("scu-dvc1", 1018, MOD_CLK_ID(1017)), > > > DEF_MOD("scu-dvc0", 1019, MOD_CLK_ID(1017)), > > > > 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