Re: [PATCH v4 2/2] drivers: soc: Add LLCC driver
From: Evan Green
Date: Thu Apr 12 2018 - 18:04:14 EST
Hi Rishabh,
On Tue, Apr 10, 2018 at 1:09 PM Rishabh Bhatnagar <rishabhb@xxxxxxxxxxxxxx>
wrote:
> LLCC (Last Level Cache Controller) provides additional cache memory
> in the system. LLCC is partitioned into multiple slices and each
> slice gets its own priority, size, ID and other config parameters.
> LLCC driver programs these parameters for each slice. Clients that
> are assigned to use LLCC need to get information such size & ID of the
> slice they get and activate or deactivate the slice as needed. LLCC driver
> provides API for the clients to perform these operations.
> Signed-off-by: Channagoud Kadabi <ckadabi@xxxxxxxxxxxxxx>
> Signed-off-by: Rishabh Bhatnagar <rishabhb@xxxxxxxxxxxxxx>
> ---
> drivers/soc/qcom/Kconfig | 17 ++
> drivers/soc/qcom/Makefile | 2 +
> drivers/soc/qcom/llcc-sdm845.c | 110 ++++++++++
> drivers/soc/qcom/llcc-slice.c | 404
+++++++++++++++++++++++++++++++++++++
> include/linux/soc/qcom/llcc-qcom.h | 168 +++++++++++++++
> 5 files changed, 701 insertions(+)
> create mode 100644 drivers/soc/qcom/llcc-sdm845.c
> create mode 100644 drivers/soc/qcom/llcc-slice.c
> create mode 100644 include/linux/soc/qcom/llcc-qcom.h
[...]
> diff --git a/drivers/soc/qcom/llcc-sdm845.c
b/drivers/soc/qcom/llcc-sdm845.c
> new file mode 100644
> index 0000000..619b226
> --- /dev/null
> +++ b/drivers/soc/qcom/llcc-sdm845.c
> @@ -0,0 +1,110 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/soc/qcom/llcc-qcom.h>
> +
> +/*
> + * SCT(System Cache Table) entry contains of the following parameters
contains the following members:
> + * name: Name of the client's use case for which the llcc slice is used
> + * uid: Unique id for the client's use case
s/uid/usecase_id/
> + * slice_id: llcc slice id for each client
> + * max_cap: The maximum capacity of the cache slice provided in KB
> + * priority: Priority of the client used to select victim line for
replacement
> + * fixed_size: Determine if the slice has a fixed capacity
"Boolean indicating if the slice has a fixed capacity" might be better
> diff --git a/drivers/soc/qcom/llcc-slice.c b/drivers/soc/qcom/llcc-slice.c
> new file mode 100644
> index 0000000..67a81b0
> --- /dev/null
> +++ b/drivers/soc/qcom/llcc-slice.c
...
> +static int llcc_update_act_ctrl(struct llcc_drv_data *drv, u32 sid,
> + u32 act_ctrl_reg_val, u32 status)
> +{
> + u32 act_ctrl_reg;
> + u32 status_reg;
> + u32 slice_status;
> + int ret = 0;
> +
> + act_ctrl_reg = drv->bcast_off + LLCC_TRP_ACT_CTRLn(sid);
> + status_reg = drv->bcast_off + LLCC_TRP_STATUSn(sid);
> +
> + /*Set the ACTIVE trigger*/
Add spaces around /* */
> + act_ctrl_reg_val |= ACT_CTRL_ACT_TRIG;
> + ret = regmap_write(drv->regmap, act_ctrl_reg, act_ctrl_reg_val);
> + if (ret)
> + return ret;
> +
> + /* Clear the ACTIVE trigger */
> + act_ctrl_reg_val &= ~ACT_CTRL_ACT_TRIG;
> + ret = regmap_write(drv->regmap, act_ctrl_reg, act_ctrl_reg_val);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read_poll_timeout(drv->regmap, status_reg,
slice_status,
> + !(slice_status & status), 0,
LLCC_STATUS_READ_DELAY);
> + return ret;
> +}
> +
> +/**
> + * llcc_slice_activate - Activate the llcc slice
> + * @desc: Pointer to llcc slice descriptor
> + *
> + * A value zero will be returned on success and a negative errno will
a value of zero
> + * be returned in error cases
> + */
> +int llcc_slice_activate(struct llcc_slice_desc *desc)
> +{
> + int ret;
> + u32 act_ctrl_val;
> + struct llcc_drv_data *drv;
> +
> + if (desc == NULL)
> + return -EINVAL;
I think we can remove this check, right?
> +
> + drv = dev_get_drvdata(desc->dev);
> + if (!drv)
> + return -EINVAL;
> +
> + mutex_lock(&drv->lock);
> + if (test_bit(desc->slice_id, drv->bitmap)) {
> + mutex_unlock(&drv->lock);
> + return 0;
> + }
> +
> + act_ctrl_val = ACT_CTRL_OPCODE_ACTIVATE << ACT_CTRL_OPCODE_SHIFT;
> +
> + ret = llcc_update_act_ctrl(drv, desc->slice_id, act_ctrl_val,
> + DEACTIVATE);
> +
> + __set_bit(desc->slice_id, drv->bitmap);
> + mutex_unlock(&drv->lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(llcc_slice_activate);
> +
> +/**
> + * llcc_slice_deactivate - Deactivate the llcc slice
> + * @desc: Pointer to llcc slice descriptor
> + *
> + * A value zero will be returned on success and a negative errno will
> + * be returned in error cases
> + */
> +int llcc_slice_deactivate(struct llcc_slice_desc *desc)
> +{
> + u32 act_ctrl_val;
> + int ret;
> + struct llcc_drv_data *drv;
> +
> + if (desc == NULL)
> + return -EINVAL;
> +
> + drv = dev_get_drvdata(desc->dev);
> + if (!drv)
> + return -EINVAL;
> +
> + mutex_lock(&drv->lock);
> + if (!test_bit(desc->slice_id, drv->bitmap)) {
> + mutex_unlock(&drv->lock);
> + return 0;
> + }
> + act_ctrl_val = ACT_CTRL_OPCODE_DEACTIVATE <<
ACT_CTRL_OPCODE_SHIFT;
> +
> + ret = llcc_update_act_ctrl(drv, desc->slice_id, act_ctrl_val,
> + ACTIVATE);
> +
> + __clear_bit(desc->slice_id, drv->bitmap);
> + mutex_unlock(&drv->lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(llcc_slice_deactivate);
> +
> +/**
> + * llcc_get_slice_id - return the slice id
> + * @desc: Pointer to llcc slice descriptor
> + *
> + * A positive value will be returned on success and a negative errno will
> + * be returned on error
> + */
> +int llcc_get_slice_id(struct llcc_slice_desc *desc)
> +{
> + if (!desc)
> + return -EINVAL;
Can we remove this check too?
> +
> + return desc->slice_id;
> +}
> +EXPORT_SYMBOL_GPL(llcc_get_slice_id);
> +
> +/**
> + * llcc_get_slice_size - return the slice id
> + * @desc: Pointer to llcc slice descriptor
> + *
> + * A positive value will be returned on success and zero will returned on
> + * error
> + */
> +size_t llcc_get_slice_size(struct llcc_slice_desc *desc)
> +{
> + if (!desc)
> + return 0;
And this one?
-Evan