| Message ID | 20230623141806.13388-6-quic_kbajaj@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | soc: qcom: llcc: Add support for QDU1000/QRU1000 | expand |
On 28.06.2023 10:52, Komal Bajaj wrote: > > > On 6/23/2023 8:28 PM, Konrad Dybcio wrote: >> On 23.06.2023 16:18, Komal Bajaj wrote: >>> Add LLCC support for multi channel DDR configuration >>> based on a feature register. Reading DDR channel >>> confiuration uses nvmem framework, so select the >>> dependency in Kconfig. Without this, there will be >>> errors while building the driver with COMPILE_TEST only. >>> >>> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com> >>> --- >>> drivers/soc/qcom/Kconfig | 2 ++ >>> drivers/soc/qcom/llcc-qcom.c | 33 ++++++++++++++++++++++++++++++--- >>> 2 files changed, 32 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig >>> index a491718f8064..cc9ad41c63aa 100644 >>> --- a/drivers/soc/qcom/Kconfig >>> +++ b/drivers/soc/qcom/Kconfig >>> @@ -64,6 +64,8 @@ config QCOM_LLCC >>> tristate "Qualcomm Technologies, Inc. LLCC driver" >>> depends on ARCH_QCOM || COMPILE_TEST >>> select REGMAP_MMIO >>> + select NVMEM >>> + select QCOM_SCM >>> help >>> Qualcomm Technologies, Inc. platform specific >>> Last Level Cache Controller(LLCC) driver for platforms such as, >>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c >>> index 6cf373da5df9..3c29612da1c5 100644 >>> --- a/drivers/soc/qcom/llcc-qcom.c >>> +++ b/drivers/soc/qcom/llcc-qcom.c >>> @@ -12,6 +12,7 @@ >>> #include <linux/kernel.h> >>> #include <linux/module.h> >>> #include <linux/mutex.h> >>> +#include <linux/nvmem-consumer.h> >>> #include <linux/of.h> >>> #include <linux/of_device.h> >>> #include <linux/regmap.h> >>> @@ -943,6 +944,19 @@ static int qcom_llcc_cfg_program(struct platform_device *pdev, >>> return ret; >>> } >>> +static int qcom_llcc_get_cfg_index(struct platform_device *pdev, u8 *cfg_index) >>> +{ >>> + int ret; >>> + >>> + ret = nvmem_cell_read_u8(&pdev->dev, "multi-chan-ddr", cfg_index); >>> + if (ret == -ENOENT) { >>> + *cfg_index = 0; >>> + return 0; >>> + } >>> + >>> + return ret; >>> +} >>> + >>> static int qcom_llcc_remove(struct platform_device *pdev) >>> { >>> /* Set the global pointer to a error code to avoid referencing it */ >>> @@ -975,11 +989,13 @@ static int qcom_llcc_probe(struct platform_device *pdev) >>> struct device *dev = &pdev->dev; >>> int ret, i; >>> struct platform_device *llcc_edac; >>> - const struct qcom_llcc_config *cfg; >>> + const struct qcom_llcc_config *cfg, *entry; >>> const struct llcc_slice_config *llcc_cfg; >>> u32 sz; >>> + u8 cfg_index; >>> u32 version; >>> struct regmap *regmap; >>> + u32 num_entries = 0; >>> drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL); >>> if (!drv_data) { >>> @@ -1040,8 +1056,19 @@ static int qcom_llcc_probe(struct platform_device *pdev) >>> drv_data->version = version; >>> - llcc_cfg = cfg[0]->sct_data; >>> - sz = cfg[0]->size; >>> + ret = qcom_llcc_get_cfg_index(pdev, &cfg_index); >>> + if (ret) >>> + goto err; >>> + >> >>> + for (entry = cfg; entry->sct_data; entry++, num_entries++) >>> + ; >>> + if (cfg_index >= num_entries || cfg_index < 0) { >> cfg_index is an unsigned variable, it can never be < 0 > > Okay, will remove this condition. > >> >>> + ret = -EINVAL; >>> + goto err; >>> + } >>> + >> if (cfg_index >= entry->size)? With that, you can also keep the config >> entries non-0-terminated in the previous patch, saving a whole lot of RAM. > > entry->size represents the size of sct table whereas num_entries represents the number > of sct tables that we can have. And by this check we are validating the value read from the > fuse register. Am I understanding your comment correctly? Oh you're right. I still see room for improvement, though. For example, you duplicate assigning need_llcc_cfg, reg_offset and edac_reg_offset. You can add a new struct, like "sct_config" and add a pointer to sct_config[] & the length of this array to qcom_llcc_config. Konrad > >> >> Konrad >>> + llcc_cfg = cfg[cfg_index].sct_data; >>> + sz = cfg[cfg_index].size; >>> for (i = 0; i < sz; i++) >>> if (llcc_cfg[i].slice_id > drv_data->max_slices) >
On 6/28/2023 4:43 PM, Konrad Dybcio wrote: > On 28.06.2023 10:52, Komal Bajaj wrote: >> >> On 6/23/2023 8:28 PM, Konrad Dybcio wrote: >>> On 23.06.2023 16:18, Komal Bajaj wrote: >>>> Add LLCC support for multi channel DDR configuration >>>> based on a feature register. Reading DDR channel >>>> confiuration uses nvmem framework, so select the >>>> dependency in Kconfig. Without this, there will be >>>> errors while building the driver with COMPILE_TEST only. >>>> >>>> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com> >>>> --- >>>> drivers/soc/qcom/Kconfig | 2 ++ >>>> drivers/soc/qcom/llcc-qcom.c | 33 ++++++++++++++++++++++++++++++--- >>>> 2 files changed, 32 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig >>>> index a491718f8064..cc9ad41c63aa 100644 >>>> --- a/drivers/soc/qcom/Kconfig >>>> +++ b/drivers/soc/qcom/Kconfig >>>> @@ -64,6 +64,8 @@ config QCOM_LLCC >>>> tristate "Qualcomm Technologies, Inc. LLCC driver" >>>> depends on ARCH_QCOM || COMPILE_TEST >>>> select REGMAP_MMIO >>>> + select NVMEM >>>> + select QCOM_SCM >>>> help >>>> Qualcomm Technologies, Inc. platform specific >>>> Last Level Cache Controller(LLCC) driver for platforms such as, >>>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c >>>> index 6cf373da5df9..3c29612da1c5 100644 >>>> --- a/drivers/soc/qcom/llcc-qcom.c >>>> +++ b/drivers/soc/qcom/llcc-qcom.c >>>> @@ -12,6 +12,7 @@ >>>> #include <linux/kernel.h> >>>> #include <linux/module.h> >>>> #include <linux/mutex.h> >>>> +#include <linux/nvmem-consumer.h> >>>> #include <linux/of.h> >>>> #include <linux/of_device.h> >>>> #include <linux/regmap.h> >>>> @@ -943,6 +944,19 @@ static int qcom_llcc_cfg_program(struct platform_device *pdev, >>>> return ret; >>>> } >>>> +static int qcom_llcc_get_cfg_index(struct platform_device *pdev, u8 *cfg_index) >>>> +{ >>>> + int ret; >>>> + >>>> + ret = nvmem_cell_read_u8(&pdev->dev, "multi-chan-ddr", cfg_index); >>>> + if (ret == -ENOENT) { >>>> + *cfg_index = 0; >>>> + return 0; >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> + >>>> static int qcom_llcc_remove(struct platform_device *pdev) >>>> { >>>> /* Set the global pointer to a error code to avoid referencing it */ >>>> @@ -975,11 +989,13 @@ static int qcom_llcc_probe(struct platform_device *pdev) >>>> struct device *dev = &pdev->dev; >>>> int ret, i; >>>> struct platform_device *llcc_edac; >>>> - const struct qcom_llcc_config *cfg; >>>> + const struct qcom_llcc_config *cfg, *entry; >>>> const struct llcc_slice_config *llcc_cfg; >>>> u32 sz; >>>> + u8 cfg_index; >>>> u32 version; >>>> struct regmap *regmap; >>>> + u32 num_entries = 0; >>>> drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL); >>>> if (!drv_data) { >>>> @@ -1040,8 +1056,19 @@ static int qcom_llcc_probe(struct platform_device *pdev) >>>> drv_data->version = version; >>>> - llcc_cfg = cfg[0]->sct_data; >>>> - sz = cfg[0]->size; >>>> + ret = qcom_llcc_get_cfg_index(pdev, &cfg_index); >>>> + if (ret) >>>> + goto err; >>>> + >>>> + for (entry = cfg; entry->sct_data; entry++, num_entries++) >>>> + ; >>>> + if (cfg_index >= num_entries || cfg_index < 0) { >>> cfg_index is an unsigned variable, it can never be < 0 >> Okay, will remove this condition. >> >>>> + ret = -EINVAL; >>>> + goto err; >>>> + } >>>> + >>> if (cfg_index >= entry->size)? With that, you can also keep the config >>> entries non-0-terminated in the previous patch, saving a whole lot of RAM. >> entry->size represents the size of sct table whereas num_entries represents the number >> of sct tables that we can have. And by this check we are validating the value read from the >> fuse register. Am I understanding your comment correctly? > Oh you're right. > > I still see room for improvement, though. > > For example, you duplicate assigning need_llcc_cfg, reg_offset > and edac_reg_offset. You can add a new struct, like "sct_config" and add > a pointer to sct_config[] & the length of this array to qcom_llcc_config. > > Konrad Okay, will follow this approach. > >>> Konrad >>>> + llcc_cfg = cfg[cfg_index].sct_data; >>>> + sz = cfg[cfg_index].size; >>>> for (i = 0; i < sz; i++) >>>> if (llcc_cfg[i].slice_id > drv_data->max_slices)
On 6/28/2023 6:44 PM, Dmitry Baryshkov wrote: > On 28/06/2023 11:45, Komal Bajaj wrote: > > No HTML emails on public mailing lists, please. > >> >> >> On 6/23/2023 7:56 PM, Dmitry Baryshkov wrote: >>> On Fri, 23 Jun 2023 at 17:19, Komal Bajaj<quic_kbajaj@quicinc.com> >>> wrote: >>>> Add LLCC support for multi channel DDR configuration >>>> based on a feature register. Reading DDR channel >>>> confiuration uses nvmem framework, so select the >>>> dependency in Kconfig. Without this, there will be >>>> errors while building the driver with COMPILE_TEST only. >>>> >>>> Signed-off-by: Komal Bajaj<quic_kbajaj@quicinc.com> >>>> --- >>>> drivers/soc/qcom/Kconfig | 2 ++ >>>> drivers/soc/qcom/llcc-qcom.c | 33 ++++++++++++++++++++++++++++++--- >>>> 2 files changed, 32 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig >>>> index a491718f8064..cc9ad41c63aa 100644 >>>> --- a/drivers/soc/qcom/Kconfig >>>> +++ b/drivers/soc/qcom/Kconfig >>>> @@ -64,6 +64,8 @@ config QCOM_LLCC >>>> tristate "Qualcomm Technologies, Inc. LLCC driver" >>>> depends on ARCH_QCOM || COMPILE_TEST >>>> select REGMAP_MMIO >>>> + select NVMEM >>> No need to select NVMEM. The used functions are stubbed if NVMEM is >>> disabled >> >> With the previous patch, where this config was not selected, below >> error was flagged by kernel test robot - >> >> drivers/soc/qcom/llcc-qcom.c: In function 'qcom_llcc_get_cfg_index': >> >> drivers/soc/qcom/llcc-qcom.c:951:15: error: implicit declaration >> of function 'nvmem_cell_read_u8'; did you mean >> 'nvmem_cell_read_u64'? [-Werror=implicit-function-declaration] >> 951 | ret = nvmem_cell_read_u8(&pdev->dev, >> "multi_chan_ddr", cfg_index); >> | ^~~~~~~~~~~~~~~~~~ >> | nvmem_cell_read_u64 >> cc1: some warnings being treated as errors > > Judging from the rest of nvmem-consumer.h, it appears that not having > stubs for this function is an omission. Please fix the header instead. Okay, I will add the stub for this function in the header. > >> >>>> + select QCOM_SCM >>>> help >>>> Qualcomm Technologies, Inc. platform specific >>>> Last Level Cache Controller(LLCC) driver for platforms >>>> such as, >>>> diff --git a/drivers/soc/qcom/llcc-qcom.c >>>> b/drivers/soc/qcom/llcc-qcom.c >>>> index 6cf373da5df9..3c29612da1c5 100644 >>>> --- a/drivers/soc/qcom/llcc-qcom.c >>>> +++ b/drivers/soc/qcom/llcc-qcom.c >>>> @@ -12,6 +12,7 @@ >>>> #include <linux/kernel.h> >>>> #include <linux/module.h> >>>> #include <linux/mutex.h> >>>> +#include <linux/nvmem-consumer.h> >>>> #include <linux/of.h> >>>> #include <linux/of_device.h> >>>> #include <linux/regmap.h> >>>> @@ -943,6 +944,19 @@ static int qcom_llcc_cfg_program(struct >>>> platform_device *pdev, >>>> return ret; >>>> } >>>> >>>> +static int qcom_llcc_get_cfg_index(struct platform_device *pdev, >>>> u8 *cfg_index) >>>> +{ >>>> + int ret; >>>> + >>>> + ret = nvmem_cell_read_u8(&pdev->dev, "multi-chan-ddr", >>>> cfg_index); >>>> + if (ret == -ENOENT) { >>> || ret == -EOPNOTSUPP ? >> >> Okay >> >>>> + *cfg_index = 0; >>>> + return 0; >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> + >>>> static int qcom_llcc_remove(struct platform_device *pdev) >>>> { >>>> /* Set the global pointer to a error code to avoid >>>> referencing it */ >>>> @@ -975,11 +989,13 @@ static int qcom_llcc_probe(struct >>>> platform_device *pdev) >>>> struct device *dev = &pdev->dev; >>>> int ret, i; >>>> struct platform_device *llcc_edac; >>>> - const struct qcom_llcc_config *cfg; >>>> + const struct qcom_llcc_config *cfg, *entry; >>>> const struct llcc_slice_config *llcc_cfg; >>>> u32 sz; >>>> + u8 cfg_index; >>>> u32 version; >>>> struct regmap *regmap; >>>> + u32 num_entries = 0; >>>> >>>> drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL); >>>> if (!drv_data) { >>>> @@ -1040,8 +1056,19 @@ static int qcom_llcc_probe(struct >>>> platform_device *pdev) >>>> >>>> drv_data->version = version; >>>> >>>> - llcc_cfg = cfg[0]->sct_data; >>>> - sz = cfg[0]->size; >>>> + ret = qcom_llcc_get_cfg_index(pdev, &cfg_index); >>>> + if (ret) >>>> + goto err; >>>> + >>>> + for (entry = cfg; entry->sct_data; entry++, num_entries++) >>>> + ; >>> Please add num_cfgs to the configuration data instead. >> >> Shall I create a new wrapper struct having a field num_cfg and a >> pointer to those cfgs >> because configuration data is itself an instance of "struct >> qcom_llcc_config" and >> we can have multiple instances of it. > > A wrapper struct is a better approach in my opinion. Okay, will follow this approach then. Thanks Komal > >> >> >>>> + if (cfg_index >= num_entries || cfg_index < 0) { >>> cfg_index is unsigned, so it can not be less than 0. >> >> Okay. >> >>>> + ret = -EINVAL; >>>> + goto err; >>>> + } >>>> + >>>> + llcc_cfg = cfg[cfg_index].sct_data; >>>> + sz = cfg[cfg_index].size; >>>> >>>> for (i = 0; i < sz; i++) >>>> if (llcc_cfg[i].slice_id > drv_data->max_slices) >>>> -- >>>> 2.40.1 >>>> >> >
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig index a491718f8064..cc9ad41c63aa 100644 --- a/drivers/soc/qcom/Kconfig +++ b/drivers/soc/qcom/Kconfig @@ -64,6 +64,8 @@ config QCOM_LLCC tristate "Qualcomm Technologies, Inc. LLCC driver" depends on ARCH_QCOM || COMPILE_TEST select REGMAP_MMIO + select NVMEM + select QCOM_SCM help Qualcomm Technologies, Inc. platform specific Last Level Cache Controller(LLCC) driver for platforms such as, diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c index 6cf373da5df9..3c29612da1c5 100644 --- a/drivers/soc/qcom/llcc-qcom.c +++ b/drivers/soc/qcom/llcc-qcom.c @@ -12,6 +12,7 @@ #include <linux/kernel.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/nvmem-consumer.h> #include <linux/of.h> #include <linux/of_device.h> #include <linux/regmap.h> @@ -943,6 +944,19 @@ static int qcom_llcc_cfg_program(struct platform_device *pdev, return ret; } +static int qcom_llcc_get_cfg_index(struct platform_device *pdev, u8 *cfg_index) +{ + int ret; + + ret = nvmem_cell_read_u8(&pdev->dev, "multi-chan-ddr", cfg_index); + if (ret == -ENOENT) { + *cfg_index = 0; + return 0; + } + + return ret; +} + static int qcom_llcc_remove(struct platform_device *pdev) { /* Set the global pointer to a error code to avoid referencing it */ @@ -975,11 +989,13 @@ static int qcom_llcc_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; int ret, i; struct platform_device *llcc_edac; - const struct qcom_llcc_config *cfg; + const struct qcom_llcc_config *cfg, *entry; const struct llcc_slice_config *llcc_cfg; u32 sz; + u8 cfg_index; u32 version; struct regmap *regmap; + u32 num_entries = 0; drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL); if (!drv_data) { @@ -1040,8 +1056,19 @@ static int qcom_llcc_probe(struct platform_device *pdev) drv_data->version = version; - llcc_cfg = cfg[0]->sct_data; - sz = cfg[0]->size; + ret = qcom_llcc_get_cfg_index(pdev, &cfg_index); + if (ret) + goto err; + + for (entry = cfg; entry->sct_data; entry++, num_entries++) + ; + if (cfg_index >= num_entries || cfg_index < 0) { + ret = -EINVAL; + goto err; + } + + llcc_cfg = cfg[cfg_index].sct_data; + sz = cfg[cfg_index].size; for (i = 0; i < sz; i++) if (llcc_cfg[i].slice_id > drv_data->max_slices)
Add LLCC support for multi channel DDR configuration based on a feature register. Reading DDR channel confiuration uses nvmem framework, so select the dependency in Kconfig. Without this, there will be errors while building the driver with COMPILE_TEST only. Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com> --- drivers/soc/qcom/Kconfig | 2 ++ drivers/soc/qcom/llcc-qcom.c | 33 ++++++++++++++++++++++++++++++--- 2 files changed, 32 insertions(+), 3 deletions(-)