| Message ID | 20230623141806.13388-6-quic_kbajaj@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | soc: qcom: llcc: Add support for QDU1000/QRU1000 | expand |
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 > + 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 ? > + *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. > + if (cfg_index >= num_entries || cfg_index < 0) { cfg_index is unsigned, so it can not be less than 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) > -- > 2.40.1 >
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 > + 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. 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/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? > > 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 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 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. > >>> + 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. > > >>> + 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 >>> >
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 >>>> >> >
On Fri, Jun 23, 2023 at 07:48:05PM +0530, 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. You may drop the last sentence, I don't think it's entirely correct. > > 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 I don't see anything your patch that warrants adding QCOM_SCM here, is it needed, should it be a separate commit? > 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++) > + ; This is still unnecessarily "clever": "For each valid entry, do nothing, while incrementing num_entries". How about just writing: "For each valid entry, increment num_entries" for (entry = cfg; entry->sct_data; entry++) num_entries++; > + if (cfg_index >= num_entries || cfg_index < 0) { cfg_index is an unsiged number, so it's unlikely to be negative. Also, "cfg_index" and "num_entries" are values in the same domain, so keeping their names related is beneficial - i.e. rename num_entries to num_cfgs. Regards, Bjorn > + 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(-)