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

[Stanimir Varbanov]

Hi,

Please adjust your mail client to drop on new line on 80 column!

On 03/29/2018 08:55 PM, Channa wrote:
> On 2018-03-29 01:08, Stanimir Varbanov wrote:
>> Hi,
>>
>> On 03/27/2018 09:52 PM, Rishabh Bhatnagar wrote:
>>> LLCC (Last Level Cache Controller) provides additional cache memory
>>> in the system. LLCC is partitioned into muliple slices and each
>>> slice getting 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 interfaces 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ÂÂÂÂÂÂÂÂÂÂ |Â 16 ++
>>> Âdrivers/soc/qcom/MakefileÂÂÂÂÂÂÂÂÂ |ÂÂ 2 +
>>> Âdrivers/soc/qcom/llcc-sdm845.cÂÂÂÂ | 120 ++++++++++
>>> Âdrivers/soc/qcom/llcc-slice.cÂÂÂÂÂ | 454
>>> +++++++++++++++++++++++++++++++++++++
>>
>> I'd name it just llcc.c, slice suffix didn't add any value.
>>
>>> Âinclude/linux/soc/qcom/llcc-qcom.h | 178 +++++++++++++++
>>> Â5 files changed, 770 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/Kconfig b/drivers/soc/qcom/Kconfig
>>> index e050eb8..28237fc 100644
>>> --- a/drivers/soc/qcom/Kconfig
>>> +++ b/drivers/soc/qcom/Kconfig
>>> @@ -21,6 +21,22 @@ config QCOM_GSBI
>>> ÂÂÂÂÂÂÂÂÂÂ functions for connecting the underlying serial UART, SPI,
>>> and I2C
>>> ÂÂÂÂÂÂÂÂÂÂ devices to the output pins.
>>>
>>> +config QCOM_LLCC
>>> +ÂÂÂ tristate "Qualcomm Technologies, Inc. LLCC driver"
>>> +ÂÂÂ depends on ARCH_QCOM
>>> +ÂÂÂ help
>>> +ÂÂÂÂÂ Qualcomm Technologies, Inc. platform specific LLCC driver for
>>> Last
>>> +ÂÂÂÂÂ Level Cache. This provides interfaces to client's that use the
>>> LLCC.
>>> +ÂÂÂÂÂ Say yes here to enable LLCC slice driver.
>>> +
>>> +config QCOM_SDM845_LLCC
>>> +ÂÂÂ tristate "Qualcomm Technologies, Inc. SDM845 LLCC driver"
>>> +ÂÂÂ depends on QCOM_LLCC
>>> +ÂÂÂ help
>>> +ÂÂÂÂÂ Say yes here to enable the LLCC driver for SDM845. This is
>>> provides
>>> +ÂÂÂÂÂ data required to configure LLCC so that clients can start
>>> using the
>>> +ÂÂÂÂÂ LLCC slices.
>>> +
>>> Âconfig QCOM_MDT_LOADER
>>> ÂÂÂÂ tristate
>>> ÂÂÂÂ select QCOM_SCM
>>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>>> index dcebf28..e16d6a2 100644
>>> --- a/drivers/soc/qcom/Makefile
>>> +++ b/drivers/soc/qcom/Makefile
>>> @@ -12,3 +12,5 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
>>> Âobj-$(CONFIG_QCOM_SMP2P)ÂÂÂ += smp2p.o
>>> Âobj-$(CONFIG_QCOM_SMSM)ÂÂÂ += smsm.o
>>> Âobj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
>>> +obj-$(CONFIG_QCOM_LLCC) += llcc-slice.o
>>> +obj-$(CONFIG_QCOM_SDM845_LLCC) += llcc-sdm845.o
>>> diff --git a/drivers/soc/qcom/llcc-sdm845.c
>>> b/drivers/soc/qcom/llcc-sdm845.c
>>> new file mode 100644
>>> index 0000000..cd431d9
>>> --- /dev/null
>>> +++ b/drivers/soc/qcom/llcc-sdm845.c
>>> @@ -0,0 +1,120 @@
>>> +/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 and
>>> + * only version 2 as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/err.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/soc/qcom/llcc-qcom.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/regmap.h>
>>> +
>>> +
>>> +/*
>>> + * SCT entry contains of the following parameters
>>> + * name: Name of the client's use case for which the llcc slice is used
>>> + * uid: Unique id for the client's use case
>>> + * 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 of the slice has a fixed capacity
>>> + * bonus_ways: Bonus ways to be used by any slice, bonus way is used
>>> only if
>>> + *ÂÂÂÂÂÂÂÂÂÂÂÂ it't not a reserved way.
>>> + * res_ways: Reserved ways for the cache slice, the reserved ways
>>> cannot be used
>>> + *ÂÂÂÂÂÂÂÂÂÂ by any other client than the one its assigned to.
>>> + * cache_mode: Each slice operates as a cache, this controls the
>>> mode of the
>>> + *ÂÂÂÂÂÂÂÂÂÂÂÂ slice normal or TCM
>>> + * probe_target_ways: Determines what ways to probe for access hit.
>>> When
>>> + *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ configured to 1 only bonus and reseved ways
>>> are probed.
>>> + *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ when configured to 0 all ways in llcc are probed.
>>> + * dis_cap_alloc: Disable capacity based allocation for a client
>>> + * retain_on_pc: If this bit is set and client has maitained active
>>> vote
>>> + *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ then the ways assigned to this client are not
>>> flushed on power
>>> + *ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ collapse.
>>> + * activate_on_init: Activate the slice immidiately after the SCT is
>>> programmed
>>> + */
>>> +#define SCT_ENTRY(n, uid, sid, mc, p, fs, bway, rway, cmod, ptw,
>>> dca, rp, a) \
>>> +ÂÂÂ {ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ \
>>> +ÂÂÂÂÂÂÂ .name = n,ÂÂÂÂÂÂÂÂÂÂÂ \
>>> +ÂÂÂÂÂÂÂ .usecase_id = uid,ÂÂÂÂÂÂÂ \
>>> +ÂÂÂÂÂÂÂ .slice_id = sid,ÂÂÂÂÂÂÂ \
>>> +ÂÂÂÂÂÂÂ .max_cap = mc,ÂÂÂÂÂÂÂÂÂÂÂ \
>>> +ÂÂÂÂÂÂÂ .priority = p,ÂÂÂÂÂÂÂÂÂÂÂ \
>>> +ÂÂÂÂÂÂÂ .fixed_size = fs,ÂÂÂÂÂÂÂ \
>>> +ÂÂÂÂÂÂÂ .bonus_ways = bway,ÂÂÂÂÂÂÂ \
>>> +ÂÂÂÂÂÂÂ .res_ways = rway,ÂÂÂÂÂÂÂ \
>>> +ÂÂÂÂÂÂÂ .cache_mode = cmod,ÂÂÂÂÂÂÂ \
>>> +ÂÂÂÂÂÂÂ .probe_target_ways = ptw,ÂÂÂ \
>>> +ÂÂÂÂÂÂÂ .dis_cap_alloc = dca,ÂÂÂÂÂÂÂ \
>>> +ÂÂÂÂÂÂÂ .retain_on_pc = rp,ÂÂÂÂÂÂÂ \
>>> +ÂÂÂÂÂÂÂ .activate_on_init = a,ÂÂÂÂÂÂÂ \
>>> +ÂÂÂ }
>>> +
>>> +
>>> +static struct llcc_slice_config sdm845_data[] =Â {
>>> +ÂÂÂ SCT_ENTRY("cpuss",ÂÂÂÂÂÂ 1, 1, 2816, 1, 0, 0xFFC, 0x2, 0, 0, 1,
>>> 1, 1),
>>> + SCT_ENTRY("vidsc0", 2, 2, 512, 2, 1, 0x0, 0x0F0, 0, 0, 1,
>>> 1, 0),
>>> + SCT_ENTRY("vidsc1", 3, 3, 512, 2, 1, 0x0, 0x0F0, 0, 0, 1,
>>> 1, 0),
>>> + SCT_ENTRY("rotator", 4, 4, 563, 2, 1, 0x0, 0x00e, 2, 0, 1,
>>> 1, 0),
>>> +ÂÂÂ SCT_ENTRY("voice",ÂÂÂÂÂÂ 5, 5, 2816, 1, 0, 0xFFC, 0x2, 0, 0, 1,
>>> 1, 0),
>>> +ÂÂÂ SCT_ENTRY("audio",ÂÂÂÂÂÂ 6, 6, 2816, 1, 0, 0xFFC, 0x2, 0, 0, 1,
>>> 1, 0),
>>> +ÂÂÂ SCT_ENTRY("modemhp_grow", 7, 7, 1024, 2, 0, 0x0FC, 0xF00, 0, 0,
>>> 1, 1, 0),
>>> +ÂÂÂ SCT_ENTRY("modem",ÂÂÂÂÂÂ 8, 8, 2816, 1, 0, 0xFFC, 0x2, 0, 0, 1,
>>> 1, 0),
>>> +ÂÂÂ SCT_ENTRY("compute",ÂÂÂÂ 10, 10, 2816, 1, 0, 0xFFC, 0x2, 0, 0,
>>> 1, 1, 0),
>>> + SCT_ENTRY("gpuhtw", 11, 11, 512, 1, 1, 0xC, 0x0, 0, 0, 1,
>>> 1, 0),
>>> +ÂÂÂ SCT_ENTRY("gpu",ÂÂÂÂÂÂÂÂ 12, 12, 2304, 1, 0, 0xFF0, 0x2, 0, 0,
>>> 1, 1, 0),
>>> + SCT_ENTRY("mmuhwt", 13, 13, 256, 2, 0, 0x0, 0x1, 0, 0, 1,
>>> 0, 1),
>>> +ÂÂÂ SCT_ENTRY("compute_dma", 15, 15, 2816, 1, 0, 0xFFC, 0x2, 0, 0,
>>> 1, 1, 0),
>>> +ÂÂÂ SCT_ENTRY("display",ÂÂÂÂ 16, 16, 2816, 1, 0, 0xFFC, 0x2, 0, 0,
>>> 1, 1, 0),
>>> +ÂÂÂ SCT_ENTRY("videofw",ÂÂÂÂ 17, 17, 2816, 1, 0, 0xFFC, 0x2, 0, 0,
>>> 1, 1, 0),
>>> + SCT_ENTRY("modemhp_fix", 20, 20, 1024, 2, 1, 0x0, 0xF00, 0, 0,
>>> 1, 1, 0),
>>> +ÂÂÂ SCT_ENTRY("modem_paging", 21, 21, 1024, 0, 1, 0x1e, 0x0, 0, 0,
>>> 1, 1, 0),
>>> +ÂÂÂ SCT_ENTRY("audiohw",ÂÂÂÂ 22, 22, 1024, 1, 1, 0xFFC, 0x2, 0, 0,
>>> 1, 1, 0),
>>> +};
>>> +
>>> +
>>> +static int sdm845_qcom_llcc_probe(struct platform_device *pdev)
>>> +{
>>> +ÂÂÂ return qcom_llcc_probe(pdev, sdm845_data, ARRAY_SIZE(sdm845_data));
>>
>> I think having separate driver just for this config structure is
>> pointless. Please move this in llcc driver and select the configuration
>> based on compatible string.
> 
> Thanks Stan for your review.
> 
> Wanted to highlight the reason we chose this approach. The System cache
> table data
> changes from chipset to chipset and overtime the table could have more
> parameters.
> Adding it as part of the core driver and selecting the table based on
> compatible
> would clutter the core driver. If we have 4 chipsets supporting system
> cache then
> core driver would have 4 tables as above and we select one of them based
> on compatible
> string. That is why we followed the pin control approach to have data in
> per chipset
> driver and pass it to common driver.

OK, do you plan to extend this with some more code?

-- 
regards,
Stan