| Message ID | 20230313124040.9463-5-quic_kbajaj@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | soc: qcom: llcc: Add support for QDU1000/QRU1000 | expand |
Hi Komal,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.3-rc2 next-20230314]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Komal-Bajaj/soc-qcom-llcc-Refactor-llcc-driver-to-support-multiple-configuration/20230313-204310
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20230313124040.9463-5-quic_kbajaj%40quicinc.com
patch subject: [PATCH v2 4/5] soc: qcom: Add LLCC support for multi channel DDR
config: mips-randconfig-r021-20230312 (https://download.01.org/0day-ci/archive/20230315/202303150713.cMgW2Qnu-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install mips cross compiling tool for clang build
# apt-get install binutils-mips-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/fe14182092383680f24bc88d81d199261453fa71
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Komal-Bajaj/soc-qcom-llcc-Refactor-llcc-driver-to-support-multiple-configuration/20230313-204310
git checkout fe14182092383680f24bc88d81d199261453fa71
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303150713.cMgW2Qnu-lkp@intel.com/
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "qcom_scm_io_readl" [drivers/soc/qcom/llcc-qcom.ko] undefined!
On 13/03/2023 13:40, Komal Bajaj wrote: > Add LLCC support for multi channel DDR configurations > based off of a feature register. > > Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com> > --- > drivers/soc/qcom/llcc-qcom.c | 56 ++++++++++++++++++++++++++++-- kernel robot still reports some issues here. You might be missing some dependency or symbol export. Best regards, Krzysztof
On Mon, Mar 13, 2023 at 06:10:39PM +0530, Komal Bajaj wrote: > Add LLCC support for multi channel DDR configurations > based off of a feature register. > Please elaborate more in the commit message on why this patch is needed and how it is implemented. > Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com> > --- > drivers/soc/qcom/llcc-qcom.c | 56 ++++++++++++++++++++++++++++-- > include/linux/soc/qcom/llcc-qcom.h | 2 ++ > 2 files changed, 55 insertions(+), 3 deletions(-) > > diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c > index 00699a0c047e..f4d3e266c629 100644 > --- a/drivers/soc/qcom/llcc-qcom.c > +++ b/drivers/soc/qcom/llcc-qcom.c > @@ -17,6 +17,7 @@ > #include <linux/regmap.h> > #include <linux/sizes.h> > #include <linux/slab.h> > +#include <linux/firmware/qcom/qcom_scm.h> Sort the includes alphabetically > #include <linux/soc/qcom/llcc-qcom.h> > > #define ACTIVATE BIT(0) > @@ -924,6 +925,40 @@ static int qcom_llcc_cfg_program(struct platform_device *pdev, > return ret; > } > > +static int qcom_llcc_get_cfg_index(struct platform_device *pdev, u32 *cfg_index) > +{ > + struct device *dev = &pdev->dev; > + struct resource *ch_res = NULL; No need to initialize the pointer > + No need of a newline > + u32 ch_reg_sz; > + u32 ch_reg_off; > + u32 val; > + int ret = 0; > + > + ch_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "multi_channel_register"); > + if (ch_res) { > + if (of_property_read_u32(dev->of_node, "multi-ch-bit-off", &ch_reg_off)) { > + dev_err(&pdev->dev, > + "Couldn't get offset for multi channel feature register\n"); > + return -ENODEV; > + } > + if (of_property_read_u32_index(dev->of_node, "multi-ch-bit-off", 1, &ch_reg_sz)) { > + dev_err(&pdev->dev, > + "Couldn't get size of multi channel feature register\n"); > + return -ENODEV; > + } > + > + if (qcom_scm_io_readl(ch_res->start, &val)) { You didn't mention this SCM call in the commit message. Also, for SCM, you need to select the driver in Kconfig. > + dev_err(&pdev->dev, "Couldn't access multi channel feature register\n"); > + ret = -EINVAL; Catch the actual error no from qcom_scm_io_readl(). So in the case of failure, you still want to calculate cfg_index? > + } > + *cfg_index = (val >> ch_reg_off) & ((1 << ch_reg_sz) - 1); > + } else Use braces for else condition > + *cfg_index = 0; > + > + return ret; > +} > + > static int qcom_llcc_remove(struct platform_device *pdev) > { > /* Set the global pointer to a error code to avoid referencing it */ > @@ -956,10 +991,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; > + No need of newline > u32 sz; > + u32 cfg_index; > u32 version; > + u32 no_of_entries = 0; num_entries? > > drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL); > if (!drv_data) { > @@ -999,8 +1037,20 @@ static int qcom_llcc_probe(struct platform_device *pdev) > num_banks >>= LLCC_LB_CNT_SHIFT; > drv_data->num_banks = num_banks; > > - 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++, no_of_entries++) > + ; Wrap ; in the above line itself > + if (cfg_index >= no_of_entries) { > + ret = -EINVAL; > + goto err; > + } > + > + drv_data->cfg_index = cfg_index; Where is this cached value used? Thanks, Mani > + 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) > diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h > index ad1fd718169d..225891a02f5d 100644 > --- a/include/linux/soc/qcom/llcc-qcom.h > +++ b/include/linux/soc/qcom/llcc-qcom.h > @@ -125,6 +125,7 @@ struct llcc_edac_reg_offset { > * @cfg: pointer to the data structure for slice configuration > * @edac_reg_offset: Offset of the LLCC EDAC registers > * @lock: mutex associated with each slice > + * @cfg_index: index of config table if multiple configs present for a target > * @cfg_size: size of the config data table > * @max_slices: max slices as read from device tree > * @num_banks: Number of llcc banks > @@ -139,6 +140,7 @@ struct llcc_drv_data { > const struct llcc_slice_config *cfg; > const struct llcc_edac_reg_offset *edac_reg_offset; > struct mutex lock; > + u32 cfg_index; > u32 cfg_size; > u32 max_slices; > u32 num_banks; > -- > 2.39.1 >
diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c index 00699a0c047e..f4d3e266c629 100644 --- a/drivers/soc/qcom/llcc-qcom.c +++ b/drivers/soc/qcom/llcc-qcom.c @@ -17,6 +17,7 @@ #include <linux/regmap.h> #include <linux/sizes.h> #include <linux/slab.h> +#include <linux/firmware/qcom/qcom_scm.h> #include <linux/soc/qcom/llcc-qcom.h> #define ACTIVATE BIT(0) @@ -924,6 +925,40 @@ static int qcom_llcc_cfg_program(struct platform_device *pdev, return ret; } +static int qcom_llcc_get_cfg_index(struct platform_device *pdev, u32 *cfg_index) +{ + struct device *dev = &pdev->dev; + struct resource *ch_res = NULL; + + u32 ch_reg_sz; + u32 ch_reg_off; + u32 val; + int ret = 0; + + ch_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "multi_channel_register"); + if (ch_res) { + if (of_property_read_u32(dev->of_node, "multi-ch-bit-off", &ch_reg_off)) { + dev_err(&pdev->dev, + "Couldn't get offset for multi channel feature register\n"); + return -ENODEV; + } + if (of_property_read_u32_index(dev->of_node, "multi-ch-bit-off", 1, &ch_reg_sz)) { + dev_err(&pdev->dev, + "Couldn't get size of multi channel feature register\n"); + return -ENODEV; + } + + if (qcom_scm_io_readl(ch_res->start, &val)) { + dev_err(&pdev->dev, "Couldn't access multi channel feature register\n"); + ret = -EINVAL; + } + *cfg_index = (val >> ch_reg_off) & ((1 << ch_reg_sz) - 1); + } else + *cfg_index = 0; + + return ret; +} + static int qcom_llcc_remove(struct platform_device *pdev) { /* Set the global pointer to a error code to avoid referencing it */ @@ -956,10 +991,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; + u32 cfg_index; u32 version; + u32 no_of_entries = 0; drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL); if (!drv_data) { @@ -999,8 +1037,20 @@ static int qcom_llcc_probe(struct platform_device *pdev) num_banks >>= LLCC_LB_CNT_SHIFT; drv_data->num_banks = num_banks; - 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++, no_of_entries++) + ; + if (cfg_index >= no_of_entries) { + ret = -EINVAL; + goto err; + } + + drv_data->cfg_index = cfg_index; + 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) diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h index ad1fd718169d..225891a02f5d 100644 --- a/include/linux/soc/qcom/llcc-qcom.h +++ b/include/linux/soc/qcom/llcc-qcom.h @@ -125,6 +125,7 @@ struct llcc_edac_reg_offset { * @cfg: pointer to the data structure for slice configuration * @edac_reg_offset: Offset of the LLCC EDAC registers * @lock: mutex associated with each slice + * @cfg_index: index of config table if multiple configs present for a target * @cfg_size: size of the config data table * @max_slices: max slices as read from device tree * @num_banks: Number of llcc banks @@ -139,6 +140,7 @@ struct llcc_drv_data { const struct llcc_slice_config *cfg; const struct llcc_edac_reg_offset *edac_reg_offset; struct mutex lock; + u32 cfg_index; u32 cfg_size; u32 max_slices; u32 num_banks;
Add LLCC support for multi channel DDR configurations based off of a feature register. Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com> --- drivers/soc/qcom/llcc-qcom.c | 56 ++++++++++++++++++++++++++++-- include/linux/soc/qcom/llcc-qcom.h | 2 ++ 2 files changed, 55 insertions(+), 3 deletions(-)