| Message ID | 20230623141806.13388-1-quic_kbajaj@quicinc.com |
|---|---|
| Headers | show |
| Series | soc: qcom: llcc: Add support for QDU1000/QRU1000 | expand |
On 26/06/2023 10:22, Komal Bajaj wrote: >> >>> + >>> +allOf: >>> + - $ref: nvmem.yaml# >>> + >>> +properties: >>> + compatible: >>> + items: >>> + - enum: >>> + - qcom,qdu1000-sec-qfprom >>> + - const: qcom,sec-qfprom >>> + >>> + reg: >>> + items: >>> + - description: The secure qfprom corrected region. >>> + >>> + # Needed if any child nodes are present. >>> + "#address-cells": >>> + const: 1 >>> + "#size-cells": >>> + const: 1 >> Drop both, they are not needed. > > I didn't get it. Can you please explain why these are not needed as this > node will have child nodes which will use single value for address and size. I suspect they are already defined. Do other bindings (for cases with children) have them? If not, why here it would be different? Best regards, Krzysztof
On 26/06/2023 11:02, Komal Bajaj wrote: > > > On 6/26/2023 2:00 PM, Krzysztof Kozlowski wrote: >> On 26/06/2023 10:22, Komal Bajaj wrote: >>>>> + >>>>> +allOf: >>>>> + - $ref: nvmem.yaml# >>>>> + >>>>> +properties: >>>>> + compatible: >>>>> + items: >>>>> + - enum: >>>>> + - qcom,qdu1000-sec-qfprom >>>>> + - const: qcom,sec-qfprom >>>>> + >>>>> + reg: >>>>> + items: >>>>> + - description: The secure qfprom corrected region. >>>>> + >>>>> + # Needed if any child nodes are present. >>>>> + "#address-cells": >>>>> + const: 1 >>>>> + "#size-cells": >>>>> + const: 1 >>>> Drop both, they are not needed. >>> I didn't get it. Can you please explain why these are not needed as this >>> node will have child nodes which will use single value for address and size. >> I suspect they are already defined. Do other bindings (for cases with >> children) have them? If not, why here it would be different? > > Yes, I see there are bindings that has these properties, listed a few of > them below - > > [1] > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml Please work on current development. It's a bit of waste of time to review old code... https://lore.kernel.org/all/20230611140330.154222-16-srinivas.kandagatla@linaro.org/ > [2] > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/firmware/arm,scmi.yaml That's not a nvmem provider. > [3] > https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/timer/arm,arch_timer_mmio.yaml That's not a nvmem provider. Best regards, Krzysztof
On 6/23/2023 8:23 PM, Konrad Dybcio wrote: > On 23.06.2023 16:18, Komal Bajaj wrote: >> For some of the Qualcomm SoC's, it is possible that >> some of the fuse regions or entire qfprom region is >> protected from non-secure access. In such situations, >> linux will have to use secure calls to read the region. >> With that motivation, add secure qfprom driver. Ensuring >> the address to read is word aligned since our secure I/O >> only supports word size I/O. >> >> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com> >> --- >> drivers/nvmem/Kconfig | 12 ++++ >> drivers/nvmem/Makefile | 2 + >> drivers/nvmem/sec-qfprom.c | 116 +++++++++++++++++++++++++++++++++++++ >> 3 files changed, 130 insertions(+) >> create mode 100644 drivers/nvmem/sec-qfprom.c >> >> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig >> index b291b27048c7..2b0dd73d899e 100644 >> --- a/drivers/nvmem/Kconfig >> +++ b/drivers/nvmem/Kconfig >> @@ -216,6 +216,18 @@ config NVMEM_QCOM_QFPROM >> This driver can also be built as a module. If so, the module >> will be called nvmem_qfprom. >> >> +config NVMEM_QCOM_SEC_QFPROM >> + tristate "QCOM SECURE QFPROM Support" >> + depends on ARCH_QCOM || COMPILE_TEST >> + depends on HAS_IOMEM >> + select QCOM_SCM > You also need OF Noted. > >> + help >> + Say y here to enable secure QFPROM support. The secure QFPROM provides access >> + functions for QFPROM data to rest of the drivers via nvmem interface. >> + >> + This driver can also be built as a module. If so, the module will be called >> + nvmem_sec_qfprom. >> + >> config NVMEM_RAVE_SP_EEPROM >> tristate "Rave SP EEPROM Support" >> depends on RAVE_SP_CORE >> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile >> index f82431ec8aef..4b4bf5880488 100644 >> --- a/drivers/nvmem/Makefile >> +++ b/drivers/nvmem/Makefile >> @@ -44,6 +44,8 @@ obj-$(CONFIG_NVMEM_NINTENDO_OTP) += nvmem-nintendo-otp.o >> nvmem-nintendo-otp-y := nintendo-otp.o >> obj-$(CONFIG_NVMEM_QCOM_QFPROM) += nvmem_qfprom.o >> nvmem_qfprom-y := qfprom.o >> +obj-$(CONFIG_NVMEM_QCOM_SEC_QFPROM) += nvmem_sec_qfprom.o >> +nvmem_sec_qfprom-y := sec-qfprom.o >> obj-$(CONFIG_NVMEM_RAVE_SP_EEPROM) += nvmem-rave-sp-eeprom.o >> nvmem-rave-sp-eeprom-y := rave-sp-eeprom.o >> obj-$(CONFIG_NVMEM_RMEM) += nvmem-rmem.o >> diff --git a/drivers/nvmem/sec-qfprom.c b/drivers/nvmem/sec-qfprom.c >> new file mode 100644 >> index 000000000000..47b2c71593dd >> --- /dev/null >> +++ b/drivers/nvmem/sec-qfprom.c >> @@ -0,0 +1,116 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved. >> + */ >> + >> +#include <linux/clk.h> >> +#include <linux/device.h> >> +#include <linux/firmware/qcom/qcom_scm.h> >> +#include <linux/io.h> >> +#include <linux/iopoll.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/mod_devicetable.h> >> +#include <linux/nvmem-provider.h> >> +#include <linux/platform_device.h> >> +#include <linux/pm_domain.h> >> +#include <linux/pm_runtime.h> >> +#include <linux/property.h> >> +#include <linux/regulator/consumer.h> >> + >> + >> +/** >> + * struct sec_sec_qfprom_priv - structure holding secure qfprom attributes >> + * >> + * @qfpseccorrected: iomapped memory space for secure qfprom corrected address space. >> + * @dev: qfprom device structure. >> + */ >> +struct sec_qfprom_priv { >> + phys_addr_t qfpseccorrected; >> + struct device *dev; >> +}; >> + >> +static int sec_qfprom_reg_read(void *context, unsigned int reg, void *_val, size_t bytes) >> +{ >> + struct sec_qfprom_priv *priv = context; >> + u8 *val = _val; >> + u8 *tmp; >> + u32 read_val; >> + unsigned int i; > Please sort this to look like a reverse-Christmas-tree Okay, will sort it in that way. > > >> + >> + for (i = 0; i < bytes; i++, reg++) { >> + if (i == 0 || reg % 4 == 0) { >> + if (qcom_scm_io_readl(priv->qfpseccorrected + (reg & ~3), &read_val)) { >> + dev_err(priv->dev, "Couldn't access fuse register\n"); >> + return -EINVAL; >> + } >> + tmp = (u8 *)&read_val; >> + } > I don't understand this read-4-times dance.. qcom_scm_io_readl() reads > u32, so this should be as simple as: > > val[i + 0] = FIELD_GET(GENMASK(7, 0), reg); > val[i + 1] = .. > val[i + 2] = .. > val[i + 3] = .. Won't it get too complex, I type-casted 32-bit read_val into u8 pointer, so that I can easily use it for the byte-level access of read_val's value. Doing the way that you mentioned would be something like below - val[i] = FIELD_GET(GENMASK((reg&3)*8+7, (reg&3)*8), read_val); Thanks Komal > >> + >> + val[i] = tmp[reg & 3]; >> + } >> + >> + return 0; >> +} >> + >> +static void sec_qfprom_runtime_disable(void *data) >> +{ >> + pm_runtime_disable(data); >> +} >> + >> +static int sec_qfprom_probe(struct platform_device *pdev) >> +{ >> + struct nvmem_config econfig = { >> + .name = "sec-qfprom", >> + .stride = 1, >> + .word_size = 1, >> + .id = NVMEM_DEVID_AUTO, >> + .reg_read = sec_qfprom_reg_read, >> + }; >> + struct device *dev = &pdev->dev; >> + struct resource *res; >> + struct nvmem_device *nvmem; >> + struct sec_qfprom_priv *priv; >> + int ret; >> + >> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return -ENOMEM; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + priv->qfpseccorrected = res->start; >> + if (!priv->qfpseccorrected) >> + return -ENOMEM; > While it works all the same, I think checking if(!res) would be more > logical > > Also, ENOMEM seems weird here.. Perhaps EINVAL? > >> + >> + econfig.size = resource_size(res); >> + econfig.dev = dev; >> + econfig.priv = priv; >> + >> + priv->dev = dev; >> + >> + pm_runtime_enable(dev); >> + ret = devm_add_action_or_reset(dev, sec_qfprom_runtime_disable, dev); >> + if (ret) >> + return ret; > Wouldn't devm_pm_runtime_enable() take care of this? Or do we need > for this block to be always-powered? > >> + >> + nvmem = devm_nvmem_register(dev, &econfig); >> + >> + return PTR_ERR_OR_ZERO(nvmem); >> +} >> + >> +static const struct of_device_id sec_qfprom_of_match[] = { >> + { .compatible = "qcom,sec-qfprom",}, > The comma inside is unnecessary, replacing it with a space would also > make the whitespacing match > >> + {/* sentinel */}, >> +}; >> +MODULE_DEVICE_TABLE(of, sec_qfprom_of_match); >> + >> +static struct platform_driver qfprom_driver = { >> + .probe = sec_qfprom_probe, >> + .driver = { >> + .name = "qcom,sec_qfprom", > Commas in driver names are rather.. rare? Let's make it qcom_ > >> + .of_match_table = sec_qfprom_of_match, >> + }, >> +}; >> +module_platform_driver(qfprom_driver); >> +MODULE_DESCRIPTION("Qualcomm Secure QFPROM driver"); >> +MODULE_LICENSE("GPL v2"); > Please run scripts/checkpatch.pl on your patches before sending. > > Konrad
On Fri, Jun 23, 2023 at 04:53:44PM +0200, Konrad Dybcio wrote: > On 23.06.2023 16:18, Komal Bajaj wrote: > > diff --git a/drivers/nvmem/sec-qfprom.c b/drivers/nvmem/sec-qfprom.c [..] > > +static int sec_qfprom_reg_read(void *context, unsigned int reg, void *_val, size_t bytes) > > +{ > > + struct sec_qfprom_priv *priv = context; > > + u8 *val = _val; > > + u8 *tmp; > > + u32 read_val; > > + unsigned int i; > Please sort this to look like a reverse-Christmas-tree > > > > + > > + for (i = 0; i < bytes; i++, reg++) { > > + if (i == 0 || reg % 4 == 0) { > > + if (qcom_scm_io_readl(priv->qfpseccorrected + (reg & ~3), &read_val)) { > > + dev_err(priv->dev, "Couldn't access fuse register\n"); > > + return -EINVAL; > > + } > > + tmp = (u8 *)&read_val; > > + } > I don't understand this read-4-times dance.. qcom_scm_io_readl() reads > u32, so this should be as simple as: > > val[i + 0] = FIELD_GET(GENMASK(7, 0), reg); > val[i + 1] = .. > val[i + 2] = .. > val[i + 3] = .. > No need for FIELD_GET() to do that, you can just do *(u32*)val = read_val; although this comes with alignment issues. But, that this the length of val is always divisible by 4, and I wasn't able to convince myself that it was the case... So this is, pretty much, what I proposed in v3: https://lore.kernel.org/linux-arm-msm/20230527205031.iwsujvlbxazukwfy@ripper/ Regards, Bjorn > > > + > > + val[i] = tmp[reg & 3]; > > + } > > + > > + return 0;
On Fri, Jun 23, 2023 at 07:48:03PM +0530, Komal Bajaj wrote: > For some of the Qualcomm SoC's, it is possible that > some of the fuse regions or entire qfprom region is > protected from non-secure access. In such situations, > linux will have to use secure calls to read the region. Linux > With that motivation, add secure qfprom driver. Ensuring > the address to read is word aligned since our secure I/O > only supports word size I/O. What do you mean with this last sentence? I don't see anything in your patch demanding this. Perhaps just drop this sentence? > > Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com> > --- > drivers/nvmem/Kconfig | 12 ++++ > drivers/nvmem/Makefile | 2 + > drivers/nvmem/sec-qfprom.c | 116 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 130 insertions(+) > create mode 100644 drivers/nvmem/sec-qfprom.c > > diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig > index b291b27048c7..2b0dd73d899e 100644 > --- a/drivers/nvmem/Kconfig > +++ b/drivers/nvmem/Kconfig > @@ -216,6 +216,18 @@ config NVMEM_QCOM_QFPROM > This driver can also be built as a module. If so, the module > will be called nvmem_qfprom. > > +config NVMEM_QCOM_SEC_QFPROM > + tristate "QCOM SECURE QFPROM Support" > + depends on ARCH_QCOM || COMPILE_TEST > + depends on HAS_IOMEM > + select QCOM_SCM > + help > + Say y here to enable secure QFPROM support. The secure QFPROM provides access > + functions for QFPROM data to rest of the drivers via nvmem interface. > + > + This driver can also be built as a module. If so, the module will be called > + nvmem_sec_qfprom. > + > config NVMEM_RAVE_SP_EEPROM > tristate "Rave SP EEPROM Support" > depends on RAVE_SP_CORE > diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile > index f82431ec8aef..4b4bf5880488 100644 > --- a/drivers/nvmem/Makefile > +++ b/drivers/nvmem/Makefile > @@ -44,6 +44,8 @@ obj-$(CONFIG_NVMEM_NINTENDO_OTP) += nvmem-nintendo-otp.o > nvmem-nintendo-otp-y := nintendo-otp.o > obj-$(CONFIG_NVMEM_QCOM_QFPROM) += nvmem_qfprom.o > nvmem_qfprom-y := qfprom.o > +obj-$(CONFIG_NVMEM_QCOM_SEC_QFPROM) += nvmem_sec_qfprom.o > +nvmem_sec_qfprom-y := sec-qfprom.o > obj-$(CONFIG_NVMEM_RAVE_SP_EEPROM) += nvmem-rave-sp-eeprom.o > nvmem-rave-sp-eeprom-y := rave-sp-eeprom.o > obj-$(CONFIG_NVMEM_RMEM) += nvmem-rmem.o > diff --git a/drivers/nvmem/sec-qfprom.c b/drivers/nvmem/sec-qfprom.c > new file mode 100644 > index 000000000000..47b2c71593dd > --- /dev/null > +++ b/drivers/nvmem/sec-qfprom.c > @@ -0,0 +1,116 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved. > + */ > + > +#include <linux/clk.h> Please review your include list, this and a few others doesn't seem to be used. > +#include <linux/device.h> > +#include <linux/firmware/qcom/qcom_scm.h> > +#include <linux/io.h> > +#include <linux/iopoll.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/mod_devicetable.h> > +#include <linux/nvmem-provider.h> > +#include <linux/platform_device.h> > +#include <linux/pm_domain.h> > +#include <linux/pm_runtime.h> > +#include <linux/property.h> > +#include <linux/regulator/consumer.h> > + > + > +/** > + * struct sec_sec_qfprom_priv - structure holding secure qfprom attributes > + * > + * @qfpseccorrected: iomapped memory space for secure qfprom corrected address space. > + * @dev: qfprom device structure. > + */ > +struct sec_qfprom_priv { There no risk of confusion here, so please drop the "_priv" suffix. > + phys_addr_t qfpseccorrected; Same thing here, so "base" or "addr" would suffice. > + struct device *dev; > +}; > + > +static int sec_qfprom_reg_read(void *context, unsigned int reg, void *_val, size_t bytes) > +{ > + struct sec_qfprom_priv *priv = context; > + u8 *val = _val; > + u8 *tmp; > + u32 read_val; > + unsigned int i; > + > + for (i = 0; i < bytes; i++, reg++) { > + if (i == 0 || reg % 4 == 0) { > + if (qcom_scm_io_readl(priv->qfpseccorrected + (reg & ~3), &read_val)) { > + dev_err(priv->dev, "Couldn't access fuse register\n"); > + return -EINVAL; > + } > + tmp = (u8 *)&read_val; > + } > + > + val[i] = tmp[reg & 3]; > + } > + > + return 0; > +} > + > +static void sec_qfprom_runtime_disable(void *data) > +{ > + pm_runtime_disable(data); > +} > + > +static int sec_qfprom_probe(struct platform_device *pdev) > +{ > + struct nvmem_config econfig = { > + .name = "sec-qfprom", > + .stride = 1, > + .word_size = 1, > + .id = NVMEM_DEVID_AUTO, > + .reg_read = sec_qfprom_reg_read, > + }; > + struct device *dev = &pdev->dev; > + struct resource *res; > + struct nvmem_device *nvmem; > + struct sec_qfprom_priv *priv; > + int ret; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + priv->qfpseccorrected = res->start; > + if (!priv->qfpseccorrected) > + return -ENOMEM; > + > + econfig.size = resource_size(res); > + econfig.dev = dev; > + econfig.priv = priv; > + > + priv->dev = dev; > + > + pm_runtime_enable(dev); > + ret = devm_add_action_or_reset(dev, sec_qfprom_runtime_disable, dev); > + if (ret) > + return ret; Please use devm_pm_runtime_enable(). > + > + nvmem = devm_nvmem_register(dev, &econfig); > + > + return PTR_ERR_OR_ZERO(nvmem); > +} > + > +static const struct of_device_id sec_qfprom_of_match[] = { > + { .compatible = "qcom,sec-qfprom",}, > + {/* sentinel */}, > +}; > +MODULE_DEVICE_TABLE(of, sec_qfprom_of_match); > + > +static struct platform_driver qfprom_driver = { > + .probe = sec_qfprom_probe, > + .driver = { > + .name = "qcom,sec_qfprom", No comma here please, qcom_sec_qfprom. > + .of_match_table = sec_qfprom_of_match, > + }, > +}; > +module_platform_driver(qfprom_driver); > +MODULE_DESCRIPTION("Qualcomm Secure QFPROM driver"); > +MODULE_LICENSE("GPL v2"); Please change this to "GPL". Regards, Bjorn
On Fri, Jun 23, 2023 at 07:48:00PM +0530, Komal Bajaj wrote: > From: Komal-Bajaj <quic_kbajaj@quicinc.com> > 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 >
From: Komal-Bajaj <quic_kbajaj@quicinc.com> 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