by Bjorn Andersson

[permalink] [raw]

Subject: Re: [PATCH v4 5/6] soc: qcom: Add LLCC support for multi channel DDR

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 <[email protected]>
> ---
> 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
>

2023-06-30 21:25:19

by Bjorn Andersson

[permalink] [raw]

Subject: Re: [PATCH v4 0/6] soc: qcom: llcc: Add support for QDU1000/QRU1000

On Fri, Jun 23, 2023 at 07:48:00PM +0530, Komal Bajaj wrote:
> From: Komal-Bajaj <[email protected]>
>

The patches in this series are going to be merged by two different
maintainers, the interface between them is an existing, clean API, so it
will be possible to merge the two halfs independently.

So please split this into one series for the addition of the nvmem
driver and one for the llcc pieces (with the nvmem interface/stub update
in the llcc one).

Thanks,
Bjorn

> This patch series does the following -
> * Add secure qfprom driver for reading secure fuse region in qfprom driver
> * Add dt-bindings for secure qfprom
> * Refactor LLCC driver to support multiple configuration
> * Add support for multi channel DDR configuration in LLCC
> * Add LLCC support for the Qualcomm QDU1000 and QRU1000 SoCs
>
> Changes in v4 -
> - Created a separate driver for reading from secure fuse region as suggested.
> - Added patch for dt-bindings of secure qfprom driver accordingly.
> - Added new properties in the dt-bindings for LLCC.
> - Implemented new logic to read the nvmem cell as suggested by Bjorn.
> - Separating the DT patches from this series as per suggestion.
>
> Changes in v3-
> - Addressed comments from Krzysztof and Mani.
> - Using qfprom to read DDR configuration from feature register.
>
> Changes in v2:
> - Addressing comments from Konrad.
>
> Komal Bajaj (6):
> dt-bindings: nvmem: sec-qfprom: Add bindings for secure qfprom
> dt-bindings: cache: qcom,llcc: Add LLCC compatible for QDU1000/QRU1000
> nvmem: sec-qfprom: Add Qualcomm secure QFPROM support.
> soc: qcom: llcc: Refactor llcc driver to support multiple
> configuration
> soc: qcom: Add LLCC support for multi channel DDR
> soc: qcom: llcc: Add QDU1000 and QRU1000 LLCC support
>
> .../devicetree/bindings/cache/qcom,llcc.yaml | 10 +
> .../bindings/nvmem/qcom,sec-qfprom.yaml | 58 ++++
> drivers/nvmem/Kconfig | 12 +
> drivers/nvmem/Makefile | 2 +
> drivers/nvmem/sec-qfprom.c | 116 +++++++
> drivers/soc/qcom/Kconfig | 2 +
> drivers/soc/qcom/llcc-qcom.c | 304 +++++++++++++-----
> include/linux/soc/qcom/llcc-qcom.h | 2 +-
> 8 files changed, 416 insertions(+), 90 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml
> create mode 100644 drivers/nvmem/sec-qfprom.c
>
> --
> 2.40.1
>