On Mon 29 Jul 2019 at 19:39, Carlo Caione <ccaione@xxxxxxxxxxxx> wrote:
> The secure monitor driver is currently a frankenstein driver which is
> registered as a platform driver but its functionality goes through a
> global struct accessed by the consumer drivers using exported helper
> functions.
>
> Try to tidy up the driver moving the firmware struct into the driver
> data and make the consumer drivers referencing the secure-monitor using
> a new property in the DT.
>
> Currently only the nvmem driver is using this API so we can fix it in
> the same commit.
Indeed, I don't have another idea to deal with it without breaking bisect
Thanks for this !
Reviewed-by: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
>
> Signed-off-by: Carlo Caione <ccaione@xxxxxxxxxxxx>
> ---
> drivers/firmware/meson/meson_sm.c | 94 +++++++++++++++++--------
> drivers/nvmem/meson-efuse.c | 23 +++++-
> include/linux/firmware/meson/meson_sm.h | 15 ++--
> 3 files changed, 93 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/firmware/meson/meson_sm.c b/drivers/firmware/meson/meson_sm.c
> index 772ca6726e7b..2e36a2aa274c 100644
> --- a/drivers/firmware/meson/meson_sm.c
> +++ b/drivers/firmware/meson/meson_sm.c
> @@ -54,8 +54,6 @@ struct meson_sm_firmware {
> void __iomem *sm_shmem_out_base;
> };
>
> -static struct meson_sm_firmware fw;
> -
> static u32 meson_sm_get_cmd(const struct meson_sm_chip *chip,
> unsigned int cmd_index)
> {
> @@ -90,6 +88,7 @@ static void __iomem *meson_sm_map_shmem(u32 cmd_shmem, unsigned int size)
> /**
> * meson_sm_call - generic SMC32 call to the secure-monitor
> *
> + * @fw: Pointer to secure-monitor firmware
> * @cmd_index: Index of the SMC32 function ID
> * @ret: Returned value
> * @arg0: SMC32 Argument 0
> @@ -100,15 +99,15 @@ static void __iomem *meson_sm_map_shmem(u32 cmd_shmem, unsigned int size)
> *
> * Return: 0 on success, a negative value on error
> */
> -int meson_sm_call(unsigned int cmd_index, u32 *ret, u32 arg0,
> - u32 arg1, u32 arg2, u32 arg3, u32 arg4)
> +int meson_sm_call(struct meson_sm_firmware *fw, unsigned int cmd_index,
> + u32 *ret, u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4)
> {
> u32 cmd, lret;
>
> - if (!fw.chip)
> + if (!fw->chip)
> return -ENOENT;
>
> - cmd = meson_sm_get_cmd(fw.chip, cmd_index);
> + cmd = meson_sm_get_cmd(fw->chip, cmd_index);
> if (!cmd)
> return -EINVAL;
>
> @@ -124,6 +123,7 @@ EXPORT_SYMBOL(meson_sm_call);
> /**
> * meson_sm_call_read - retrieve data from secure-monitor
> *
> + * @fw: Pointer to secure-monitor firmware
> * @buffer: Buffer to store the retrieved data
> * @bsize: Size of the buffer
> * @cmd_index: Index of the SMC32 function ID
> @@ -137,22 +137,23 @@ EXPORT_SYMBOL(meson_sm_call);
> * When 0 is returned there is no guarantee about the amount of
> * data read and bsize bytes are copied in buffer.
> */
> -int meson_sm_call_read(void *buffer, unsigned int bsize, unsigned int cmd_index,
> - u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4)
> +int meson_sm_call_read(struct meson_sm_firmware *fw, void *buffer,
> + unsigned int bsize, unsigned int cmd_index, u32 arg0,
> + u32 arg1, u32 arg2, u32 arg3, u32 arg4)
> {
> u32 size;
> int ret;
>
> - if (!fw.chip)
> + if (!fw->chip)
> return -ENOENT;
>
> - if (!fw.chip->cmd_shmem_out_base)
> + if (!fw->chip->cmd_shmem_out_base)
> return -EINVAL;
>
> - if (bsize > fw.chip->shmem_size)
> + if (bsize > fw->chip->shmem_size)
> return -EINVAL;
>
> - if (meson_sm_call(cmd_index, &size, arg0, arg1, arg2, arg3, arg4) < 0)
> + if (meson_sm_call(fw, cmd_index, &size, arg0, arg1, arg2, arg3, arg4) < 0)
> return -EINVAL;
>
> if (size > bsize)
> @@ -164,7 +165,7 @@ int meson_sm_call_read(void *buffer, unsigned int bsize, unsigned int cmd_index,
> size = bsize;
>
> if (buffer)
> - memcpy(buffer, fw.sm_shmem_out_base, size);
> + memcpy(buffer, fw->sm_shmem_out_base, size);
>
> return ret;
> }
> @@ -173,6 +174,7 @@ EXPORT_SYMBOL(meson_sm_call_read);
> /**
> * meson_sm_call_write - send data to secure-monitor
> *
> + * @fw: Pointer to secure-monitor firmware
> * @buffer: Buffer containing data to send
> * @size: Size of the data to send
> * @cmd_index: Index of the SMC32 function ID
> @@ -184,23 +186,24 @@ EXPORT_SYMBOL(meson_sm_call_read);
> *
> * Return: size of sent data on success, a negative value on error
> */
> -int meson_sm_call_write(void *buffer, unsigned int size, unsigned int cmd_index,
> - u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4)
> +int meson_sm_call_write(struct meson_sm_firmware *fw, void *buffer,
> + unsigned int size, unsigned int cmd_index, u32 arg0,
> + u32 arg1, u32 arg2, u32 arg3, u32 arg4)
> {
> u32 written;
>
> - if (!fw.chip)
> + if (!fw->chip)
> return -ENOENT;
>
> - if (size > fw.chip->shmem_size)
> + if (size > fw->chip->shmem_size)
> return -EINVAL;
>
> - if (!fw.chip->cmd_shmem_in_base)
> + if (!fw->chip->cmd_shmem_in_base)
> return -EINVAL;
>
> - memcpy(fw.sm_shmem_in_base, buffer, size);
> + memcpy(fw->sm_shmem_in_base, buffer, size);
>
> - if (meson_sm_call(cmd_index, &written, arg0, arg1, arg2, arg3, arg4) < 0)
> + if (meson_sm_call(fw, cmd_index, &written, arg0, arg1, arg2, arg3, arg4) < 0)
> return -EINVAL;
>
> if (!written)
> @@ -210,6 +213,24 @@ int meson_sm_call_write(void *buffer, unsigned int size, unsigned int cmd_index,
> }
> EXPORT_SYMBOL(meson_sm_call_write);
>
> +/**
> + * meson_sm_get - get pointer to meson_sm_firmware structure.
> + *
> + * @sm_node: Pointer to the secure-monitor Device Tree node.
> + *
> + * Return: NULL is the secure-monitor device is not ready.
> + */
> +struct meson_sm_firmware *meson_sm_get(struct device_node *sm_node)
> +{
> + struct platform_device *pdev = of_find_device_by_node(sm_node);
> +
> + if (!pdev)
> + return NULL;
> +
> + return platform_get_drvdata(pdev);
> +}
> +EXPORT_SYMBOL_GPL(meson_sm_get);
> +
> #define SM_CHIP_ID_LENGTH 119
> #define SM_CHIP_ID_OFFSET 4
> #define SM_CHIP_ID_SIZE 12
> @@ -217,14 +238,18 @@ EXPORT_SYMBOL(meson_sm_call_write);
> static ssize_t serial_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> + struct platform_device *pdev = to_platform_device(dev);
> + struct meson_sm_firmware *fw;
> uint8_t *id_buf;
> int ret;
>
> + fw = platform_get_drvdata(pdev);
> +
> id_buf = kmalloc(SM_CHIP_ID_LENGTH, GFP_KERNEL);
> if (!id_buf)
> return -ENOMEM;
>
> - ret = meson_sm_call_read(id_buf, SM_CHIP_ID_LENGTH, SM_GET_CHIP_ID,
> + ret = meson_sm_call_read(fw, id_buf, SM_CHIP_ID_LENGTH, SM_GET_CHIP_ID,
> 0, 0, 0, 0, 0);
> if (ret < 0) {
> kfree(id_buf);
> @@ -268,25 +293,34 @@ static const struct of_device_id meson_sm_ids[] = {
>
> static int __init meson_sm_probe(struct platform_device *pdev)
> {
> + struct device *dev = &pdev->dev;
> const struct meson_sm_chip *chip;
> + struct meson_sm_firmware *fw;
> +
> + fw = devm_kzalloc(dev, sizeof(*fw), GFP_KERNEL);
> + if (!fw)
> + return -ENOMEM;
>
> - chip = of_match_device(meson_sm_ids, &pdev->dev)->data;
> + chip = of_match_device(meson_sm_ids, dev)->data;
>
> if (chip->cmd_shmem_in_base) {
> - fw.sm_shmem_in_base = meson_sm_map_shmem(chip->cmd_shmem_in_base,
> - chip->shmem_size);
> - if (WARN_ON(!fw.sm_shmem_in_base))
> + fw->sm_shmem_in_base = meson_sm_map_shmem(chip->cmd_shmem_in_base,
> + chip->shmem_size);
> + if (WARN_ON(!fw->sm_shmem_in_base))
> goto out;
> }
>
> if (chip->cmd_shmem_out_base) {
> - fw.sm_shmem_out_base = meson_sm_map_shmem(chip->cmd_shmem_out_base,
> - chip->shmem_size);
> - if (WARN_ON(!fw.sm_shmem_out_base))
> + fw->sm_shmem_out_base = meson_sm_map_shmem(chip->cmd_shmem_out_base,
> + chip->shmem_size);
> + if (WARN_ON(!fw->sm_shmem_out_base))
> goto out_in_base;
> }
>
> - fw.chip = chip;
> + fw->chip = chip;
> +
> + platform_set_drvdata(pdev, fw);
> +
> pr_info("secure-monitor enabled\n");
>
> if (sysfs_create_group(&pdev->dev.kobj, &meson_sm_sysfs_attr_group))
> @@ -295,7 +329,7 @@ static int __init meson_sm_probe(struct platform_device *pdev)
> return 0;
>
> out_in_base:
> - iounmap(fw.sm_shmem_in_base);
> + iounmap(fw->sm_shmem_in_base);
> out:
> return -EINVAL;
> }
> diff --git a/drivers/nvmem/meson-efuse.c b/drivers/nvmem/meson-efuse.c
> index 9924b98db772..669d20d73877 100644
> --- a/drivers/nvmem/meson-efuse.c
> +++ b/drivers/nvmem/meson-efuse.c
> @@ -17,20 +17,25 @@
> struct meson_efuse {
> struct nvmem_device *nvmem;
> struct nvmem_config config;
> + struct meson_sm_firmware *fw;
> struct clk *clk;
> };
>
> static int meson_efuse_read(void *context, unsigned int offset,
> void *val, size_t bytes)
> {
> - return meson_sm_call_read((u8 *)val, bytes, SM_EFUSE_READ, offset,
> + struct meson_efuse *efuse = context;
> +
> + return meson_sm_call_read(efuse->fw, (u8 *)val, bytes, SM_EFUSE_READ, offset,
> bytes, 0, 0, 0);
> }
>
> static int meson_efuse_write(void *context, unsigned int offset,
> void *val, size_t bytes)
> {
> - return meson_sm_call_write((u8 *)val, bytes, SM_EFUSE_WRITE, offset,
> + struct meson_efuse *efuse = context;
> +
> + return meson_sm_call_write(efuse->fw, (u8 *)val, bytes, SM_EFUSE_WRITE, offset,
> bytes, 0, 0, 0);
> }
>
> @@ -43,6 +48,7 @@ MODULE_DEVICE_TABLE(of, meson_efuse_match);
> static int meson_efuse_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> + struct device_node *sm_np;
> struct meson_efuse *efuse;
> unsigned int size;
> int ret;
> @@ -51,6 +57,17 @@ static int meson_efuse_probe(struct platform_device *pdev)
> if (!efuse)
> return -ENOMEM;
>
> + sm_np = of_parse_phandle(pdev->dev.of_node, "secure-monitor", 0);
> + if (!sm_np) {
> + dev_err(&pdev->dev, "no secure-monitor node\n");
> + return -ENODEV;
> + }
> +
> + efuse->fw = meson_sm_get(sm_np);
> + of_node_put(sm_np);
> + if (!efuse->fw)
> + return -EPROBE_DEFER;
> +
> efuse->clk = devm_clk_get(dev, NULL);
> if (IS_ERR(efuse->clk)) {
> ret = PTR_ERR(efuse->clk);
> @@ -73,7 +90,7 @@ static int meson_efuse_probe(struct platform_device *pdev)
> return ret;
> }
>
> - if (meson_sm_call(SM_EFUSE_USER_MAX, &size, 0, 0, 0, 0, 0) < 0) {
> + if (meson_sm_call(efuse->fw, SM_EFUSE_USER_MAX, &size, 0, 0, 0, 0, 0) < 0) {
> dev_err(dev, "failed to get max user");
> return -EINVAL;
> }
> diff --git a/include/linux/firmware/meson/meson_sm.h b/include/linux/firmware/meson/meson_sm.h
> index 7613bf7c9442..6669e2a1d5fd 100644
> --- a/include/linux/firmware/meson/meson_sm.h
> +++ b/include/linux/firmware/meson/meson_sm.h
> @@ -16,11 +16,14 @@ enum {
>
> struct meson_sm_firmware;
>
> -int meson_sm_call(unsigned int cmd_index, u32 *ret, u32 arg0, u32 arg1,
> - u32 arg2, u32 arg3, u32 arg4);
> -int meson_sm_call_write(void *buffer, unsigned int b_size, unsigned int cmd_index,
> - u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4);
> -int meson_sm_call_read(void *buffer, unsigned int bsize, unsigned int cmd_index,
> - u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4);
> +int meson_sm_call(struct meson_sm_firmware *fw, unsigned int cmd_index,
> + u32 *ret, u32 arg0, u32 arg1, u32 arg2, u32 arg3, u32 arg4);
> +int meson_sm_call_write(struct meson_sm_firmware *fw, void *buffer,
> + unsigned int b_size, unsigned int cmd_index, u32 arg0,
> + u32 arg1, u32 arg2, u32 arg3, u32 arg4);
> +int meson_sm_call_read(struct meson_sm_firmware *fw, void *buffer,
> + unsigned int bsize, unsigned int cmd_index, u32 arg0,
> + u32 arg1, u32 arg2, u32 arg3, u32 arg4);
> +struct meson_sm_firmware *meson_sm_get(struct device_node *firmware_node);
>
> #endif /* _MESON_SM_FW_H_ */
> --
> 2.20.1
