Re: [PATCH v4 2/2] drivers: soc: Add LLCC driver

[rishabhb@codeaurora.org]

On 2018-04-12 15:02, Evan Green wrote:
> Hi Rishabh,
> 
> On Tue, Apr 10, 2018 at 1:09 PM Rishabh Bhatnagar 
> <rishabhb@codeaurora.org>
> 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@codeaurora.org>
>> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
>> ---
>>   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?
> 
We need this check and the following one to protect against the
null pointer access which might happen if client doesn't get
the descriptor before accessing size and slice_id.
>> +
>> +       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