Re: [PATCH 2/6] staging: media: wave5: Add the vdi layer

[Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>]

Hello,

On Wed, 15 Sept 2021 at 13:24, Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
> Add the vdi directory of the wave5 codec driver.
> The vdi.h header defines common helper functions
> such as writing/reading register and handling endianness.
>
> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/wave5/vdi/vdi.c | 325 ++++++++++++++++++++++++++
>  drivers/staging/media/wave5/vdi/vdi.h |  79 +++++++
>  2 files changed, 404 insertions(+)
>  create mode 100644 drivers/staging/media/wave5/vdi/vdi.c
>  create mode 100644 drivers/staging/media/wave5/vdi/vdi.h
>
> diff --git a/drivers/staging/media/wave5/vdi/vdi.c b/drivers/staging/media/wave5/vdi/vdi.c
> new file mode 100644
> index 000000000000..95bc120407a4
> --- /dev/null
> +++ b/drivers/staging/media/wave5/vdi/vdi.c
> @@ -0,0 +1,325 @@
> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> +/*
> + * Wave5 series multi-standard codec IP - low level access functions
> + *
> + * Copyright (C) 2021 CHIPS&MEDIA INC
> + */
> +#include <linux/bug.h>
> +#include "vdi.h"
> +#include "../v4l2/vpu.h"
> +#include "../vpuapi/wave/wave5_regdefine.h"
> +#include <linux/delay.h>
> +#define VDI_SRAM_BASE_ADDR                  0x00
> +
> +#define VDI_SYSTEM_ENDIAN                VDI_LITTLE_ENDIAN
> +#define VDI_128BIT_BUS_SYSTEM_ENDIAN     VDI_128BIT_LITTLE_ENDIAN
> +
> +static int vdi_allocate_common_memory(struct device *dev)

I would get rid of this entirely and just use the DMA API allocators.

> +{
> +       int ret;
> +       struct vpu_device *vpu_dev = dev_get_drvdata(dev);
> +
> +       if (!vpu_dev->common_mem.vaddr) {
> +               vpu_dev->common_mem.size = SIZE_COMMON;
> +               ret = vdi_allocate_dma_memory(vpu_dev, &vpu_dev->common_mem);
> +               if (ret) {
> +                       dev_err(dev, "unable to allocate common buffer\n");
> +                       return ret;
> +               }
> +       }
> +
> +       dev_dbg(dev, "common_mem: daddr=%pad size=%zu vaddr=0x%p\n",
> +               &vpu_dev->common_mem.daddr, vpu_dev->common_mem.size,
> +                       vpu_dev->common_mem.vaddr);
> +
> +       return 0;
> +}
> +
> +int vdi_init(struct device *dev)

As mentioned on patch 3/6, please prefix functions, e.g. "wave5_vdi_init".

> +{
> +       struct vpu_platform_data *pdata = dev_get_platdata(dev);
> +       struct vpu_device *vpu_dev = dev_get_drvdata(dev);
> +       int i;
> +
> +       if (vdi_allocate_common_memory(dev) < 0) {
> +               pr_err("[VDI] fail to get vpu common buffer from driver\n");

As mentioned on patch 3/6, please clean the pr_{} with dev_{},
if possible.

If you need to use pr_{} for some reason, you can use pr_fmt
instead of having that "[VDI]". This stuff is even nicely
documented Documentation/core-api/printk-basics.rst :-)

> +               goto ERR_VDI_INIT;
> +       }
> +
> +       if (pdata && pdata->reset)
> +               pdata->reset(dev, vpu_dev->vdb_register.vaddr);
> +
> +       if (pdata && pdata->pre_fw_init) {
> +               if (pdata->pre_fw_init(dev, vpu_dev->vdb_register.vaddr))
> +                       goto ERR_VDI_INIT;
> +       }
> +
> +       if (PRODUCT_CODE_W_SERIES(vpu_dev->product_code)) {
> +               // if BIT processor is not running.
> +               if (vdi_read_register(vpu_dev, W5_VCPU_CUR_PC) == 0) {
> +                       for (i = 0; i < 64; i++)
> +                               vdi_write_register(vpu_dev, (i * 4) + 0x100, 0x0);
> +               }
> +       } else {
> +               WARN_ONCE(1, "unsupported product code 0x%x\n", vpu_dev->product_code);
> +       }
> +
> +       pr_info("[VDI] success to init driver\n");
> +
> +       return 0;
> +
> +ERR_VDI_INIT:
> +

Avoid caps.

> +       vdi_release(dev);
> +       return -1;
> +}
> +EXPORT_SYMBOL(vdi_init);
> +

Exported to whom?

> +int vdi_release(struct device *dev)
> +{
> +       struct vpu_device *vpu_dev = dev_get_drvdata(dev);
> +
> +       memset(&vpu_dev->vdb_register, 0x00, sizeof(vpu_dev->vdb_register));
> +       vdi_free_dma_memory(vpu_dev, &vpu_dev->common_mem);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(vdi_release);
> +
> +void vdi_write_register(struct vpu_device *vpu_dev, unsigned int addr, unsigned int data)
> +{
> +       struct vpu_platform_data *pdata;
> +
> +       pdata = dev_get_platdata(vpu_dev->dev);
> +       if (pdata && pdata->write_register)
> +               return pdata->write_register(vpu_dev->dev, vpu_dev->vdb_register.vaddr, addr, data);
> +
> +       writel(data, vpu_dev->vdb_register.vaddr + addr);
> +}
> +
> +unsigned int vdi_read_register(struct vpu_device *vpu_dev, unsigned int addr)
> +{
> +       struct vpu_platform_data *pdata;
> +
> +       pdata = dev_get_platdata(vpu_dev->dev);
> +       if (pdata && pdata->read_register)
> +               return pdata->read_register(vpu_dev->dev, vpu_dev->vdb_register.vaddr, addr);
> +
> +       return readl(vpu_dev->vdb_register.vaddr + addr);
> +}
> +EXPORT_SYMBOL(vdi_read_register);
> +
> +int vdi_buffer_sync(struct device *dev, struct vpu_buf *vb, int dir)
> +{
> +       struct vpu_device *vpu_dev = dev_get_drvdata(dev);
> +       struct vpu_platform_data *pdata;
> +
> +       if (!vb || !vb->vaddr) {
> +               pr_err("%s(): unable to clear unmapped buffer\n", __func__);
> +               return -EINVAL;
> +       }
> +
> +       pdata = dev_get_platdata(dev);
> +       if (pdata && pdata->buffer_sync)
> +               return pdata->buffer_sync(dev, vpu_dev->vdb_register.vaddr, vb, 0, vb->size, dir);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(vdi_buffer_sync);
> +
> +int vdi_clear_memory(struct vpu_device *vpu_dev, struct vpu_buf *vb)
> +{
> +       struct vpu_platform_data *pdata;
> +
> +       if (!vb || !vb->vaddr) {
> +               pr_err("%s(): unable to clear unmapped buffer\n", __func__);
> +               return -EINVAL;
> +       }
> +
> +       memset(vb->vaddr, 0, vb->size);
> +       pdata = dev_get_platdata(vpu_dev->dev);
> +       if (pdata && pdata->buffer_sync)
> +               pdata->buffer_sync(vpu_dev->dev, vpu_dev->vdb_register.vaddr, vb, 0, vb->size, VPU_BUF_SYNC_TO_DEVICE);
> +
> +       return vb->size;
> +}
> +
> +static int swap_endian(struct vpu_device *vpu_dev, unsigned char *data, int len, int endian);
> +
> +int vdi_write_memory(struct vpu_device *vpu_dev, struct vpu_buf *vb, size_t offset, unsigned char *data, int len, int endian)
> +{
> +       struct vpu_platform_data *pdata;
> +
> +       if (!vb || !vb->vaddr) {
> +               pr_err("%s(): unable to write to unmapped buffer\n", __func__);
> +               return -EINVAL;
> +       }
> +
> +       if ((offset + len) > vb->size) {
> +               pr_err("%s(): buffer too small\n", __func__);
> +               return -ENOSPC;
> +       }
> +
> +       swap_endian(vpu_dev, data, len, endian);
> +       memcpy(vb->vaddr + offset, data, len);
> +       pdata = dev_get_platdata(vpu_dev->dev);
> +       if (pdata && pdata->buffer_sync)
> +               pdata->buffer_sync(vpu_dev->dev, vpu_dev->vdb_register.vaddr, vb, offset, len, VPU_BUF_SYNC_TO_DEVICE);
> +
> +       return len;
> +}
> +
> +int vdi_allocate_dma_memory(struct vpu_device *vpu_dev, struct vpu_buf *vb)
> +{
> +       void *vaddr;
> +       dma_addr_t daddr;
> +       struct vpu_platform_data *pdata;
> +
> +       if (!vb->size) {
> +               dev_err(vpu_dev->dev, "%s(): requested size==0\n", __func__);
> +               return -EINVAL;
> +       }
> +
> +       pdata = dev_get_platdata(vpu_dev->dev);
> +       if (pdata && pdata->buffer_alloc)
> +               return pdata->buffer_alloc(vpu_dev->dev, vb);

I don't see buffer_alloc implemented, but like I said, better to just
ditch this ad-hoc allocator and do follow kernel-style, like all drivers.

> +
> +       vaddr = dma_alloc_coherent(vpu_dev->dev, vb->size, &daddr, GFP_KERNEL);
> +       if (!vaddr)
> +               return -ENOMEM;
> +       vb->vaddr = vaddr;
> +       vb->daddr = daddr;
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(vdi_allocate_dma_memory);
> +
> +void vdi_free_dma_memory(struct vpu_device *vpu_dev, struct vpu_buf *vb)
> +{
> +       struct vpu_platform_data *pdata;
> +
> +       if (vb->size == 0)
> +               return;
> +
> +       pdata = dev_get_platdata(vpu_dev->dev);
> +       if (pdata && pdata->buffer_free)
> +               return pdata->buffer_free(vpu_dev->dev, vb);
> +
> +       if (!vb->vaddr)
> +               dev_err(vpu_dev->dev, "%s(): requested free of unmapped buffer\n", __func__);
> +       else
> +               dma_free_coherent(vpu_dev->dev, vb->size, vb->vaddr, vb->daddr);
> +
> +       memset(vb, 0, sizeof(*vb));
> +}
> +EXPORT_SYMBOL(vdi_free_dma_memory);
> +
> +int vdi_convert_endian(struct vpu_device *vpu_dev, unsigned int endian)
> +{
> +       if (PRODUCT_CODE_W_SERIES(vpu_dev->product_code)) {
> +               switch (endian) {
> +               case VDI_LITTLE_ENDIAN:
> +                       endian = 0x00;
> +                       break;
> +               case VDI_BIG_ENDIAN:
> +                       endian = 0x0f;
> +                       break;
> +               case VDI_32BIT_LITTLE_ENDIAN:
> +                       endian = 0x04;
> +                       break;
> +               case VDI_32BIT_BIG_ENDIAN:
> +                       endian = 0x03;
> +                       break;
> +               }
> +       }
> +
> +       return (endian & 0x0f);
> +}
> +
> +void byte_swap(unsigned char *data, int len)
> +{
> +       u8 temp;
> +       s32 i;
> +
> +       for (i = 0; i < len; i += 2) {
> +               temp      = data[i];
> +               data[i]   = data[i + 1];
> +               data[i + 1] = temp;
> +       }
> +}
> +
> +void word_swap(unsigned char *data, int len)

Perhaps there's some library function for all these?

> +{
> +       u16  temp;
> +       u16 *ptr = (uint16_t *)data;
> +       s32   i, size = len / (int)sizeof(uint16_t);
> +
> +       for (i = 0; i < size; i += 2) {
> +               temp     = ptr[i];
> +               ptr[i]   = ptr[i + 1];
> +               ptr[i + 1] = temp;
> +       }
> +}
> +
> +void dword_swap(unsigned char *data, int len)
> +{
> +       u32  temp;
> +       u32 *ptr = (uint32_t *)data;
> +       s32   i, size = len / (int)sizeof(uint32_t);
> +
> +       for (i = 0; i < size; i += 2) {
> +               temp     = ptr[i];
> +               ptr[i]   = ptr[i + 1];
> +               ptr[i + 1] = temp;
> +       }
> +}
> +
> +void lword_swap(unsigned char *data, int len)
> +{
> +       u64  temp;
> +       u64 *ptr = (uint64_t *)data;
> +       s32   i, size = len / (int)sizeof(uint64_t);
> +
> +       for (i = 0; i < size; i += 2) {
> +               temp     = ptr[i];
> +               ptr[i]   = ptr[i + 1];
> +               ptr[i + 1] = temp;
> +       }
> +}
> +
> +static int swap_endian(struct vpu_device *vpu_dev, unsigned char *data, int len, int endian)
> +{
> +       int         changes;
> +       int         sys_endian;
> +       bool        byte_change, word_change, dword_change, lword_change;
> +
> +       if (PRODUCT_CODE_W_SERIES(vpu_dev->product_code)) {
> +               sys_endian = VDI_128BIT_BUS_SYSTEM_ENDIAN;
> +       } else {
> +               pr_err("unknown product id : %08x\n", vpu_dev->product_code);
> +               return -1;
> +       }
> +
> +       endian     = vdi_convert_endian(vpu_dev, endian);
> +       sys_endian = vdi_convert_endian(vpu_dev, sys_endian);
> +       if (endian == sys_endian)
> +               return 0;
> +
> +       changes     = endian ^ sys_endian;
> +       byte_change  = changes & 0x01;
> +       word_change  = ((changes & 0x02) == 0x02);
> +       dword_change = ((changes & 0x04) == 0x04);
> +       lword_change = ((changes & 0x08) == 0x08);
> +
> +       if (byte_change)
> +               byte_swap(data, len);
> +       if (word_change)
> +               word_swap(data, len);
> +       if (dword_change)
> +               dword_swap(data, len);
> +       if (lword_change)
> +               lword_swap(data, len);
> +
> +       return 1;
> +}
> +
> diff --git a/drivers/staging/media/wave5/vdi/vdi.h b/drivers/staging/media/wave5/vdi/vdi.h
> new file mode 100644
> index 000000000000..31fcea026755
> --- /dev/null
> +++ b/drivers/staging/media/wave5/vdi/vdi.h
> @@ -0,0 +1,79 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
> +/*
> + * Wave5 series multi-standard codec IP - low level access functions
> + *
> + * Copyright (C) 2021 CHIPS&MEDIA INC
> + */
> +
> +#ifndef _VDI_H_
> +#define _VDI_H_
> +
> +#include "../vpuapi/vpuconfig.h"
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +#include <linux/device.h>
> +
> +/************************************************************************/
> +/* COMMON REGISTERS                                                     */
> +/************************************************************************/
> +#define VPU_PRODUCT_CODE_REGISTER                 0x1044
> +
> +/* system register write */
> +#define vpu_write_reg(VPU_INST, ADDR, DATA) vdi_write_register(VPU_INST, ADDR, DATA)
> +// system register read
> +#define vpu_read_reg(CORE, ADDR) vdi_read_register(CORE, ADDR)
> +// system memory write
> +#define vpu_write_mem(CORE, VB, OFFSET, DATA, LEN, ENDIAN)    \
> +       vdi_write_memory(CORE, VB, OFFSET, DATA, LEN, ENDIAN)
> +
> +struct vpu_buf {
> +       size_t size;
> +       dma_addr_t daddr;
> +       void __iomem *vaddr;
> +};
> +
> +enum endian_mode {
> +       VDI_LITTLE_ENDIAN = 0,      /* 64bit LE */
> +       VDI_BIG_ENDIAN,             /* 64bit BE */
> +       VDI_32BIT_LITTLE_ENDIAN,
> +       VDI_32BIT_BIG_ENDIAN,
> +       /* WAVE PRODUCTS */
> +       VDI_128BIT_LITTLE_ENDIAN    = 16,
> +       VDI_128BIT_LE_BYTE_SWAP,
> +       VDI_128BIT_LE_WORD_SWAP,
> +       VDI_128BIT_LE_WORD_BYTE_SWAP,
> +       VDI_128BIT_LE_DWORD_SWAP,
> +       VDI_128BIT_LE_DWORD_BYTE_SWAP,
> +       VDI_128BIT_LE_DWORD_WORD_SWAP,
> +       VDI_128BIT_LE_DWORD_WORD_BYTE_SWAP,
> +       VDI_128BIT_BE_DWORD_WORD_BYTE_SWAP,
> +       VDI_128BIT_BE_DWORD_WORD_SWAP,
> +       VDI_128BIT_BE_DWORD_BYTE_SWAP,
> +       VDI_128BIT_BE_DWORD_SWAP,
> +       VDI_128BIT_BE_WORD_BYTE_SWAP,
> +       VDI_128BIT_BE_WORD_SWAP,
> +       VDI_128BIT_BE_BYTE_SWAP,
> +       VDI_128BIT_BIG_ENDIAN        = 31,
> +       VDI_ENDIAN_MAX
> +};
> +
> +#define VDI_128BIT_ENDIAN_MASK 0xf
> +
> +int vdi_init(struct device *dev);
> +int vdi_release(struct device *dev);   //this function may be called only at system off.
> +
> +/**
> + * @brief       make clock stable before changing clock frequency
> + * @detail      before inoking vdi_set_clock_freg caller MUST invoke vdi_ready_change_clock
> + *             function.
> + *              after changing clock frequency caller also invoke vdi_done_change_clock() function.
> + * @return  0   failure
> + *          1   success
> + */
> +int vdi_ready_change_clock(unsigned long core_idx);
> +int vdi_set_change_clock(unsigned long core_idx, unsigned long clock_mask);
> +int vdi_done_change_clock(unsigned long core_idx);
> +int vdi_buffer_sync(struct device *dev, struct vpu_buf *vb, int dir);
> +
> +#endif //#ifndef _VDI_H_
> +
> --
> 2.17.1
>

Regards,
Ezequiel