Message ID | 20201120101112.31819-1-alice.guo@nxp.com |
---|---|
State | Superseded, archived |
Headers | show |
Series | [v4,1/4] dt-bindings: soc: imx8m: add DT Binding doc for soc unique ID | expand |
Context | Check | Description |
---|---|---|
robh/checkpatch | success | |
robh/dt-meta-schema | success |
On Fri, Nov 20, 2020 at 06:11:12PM +0800, Alice Guo wrote: > Directly reading ocotp register depends on that bootloader enables ocotp > clk, which is not always effective, so change to use nvmem API. Using > nvmem API requires to support driver defer probe and thus change > soc-imx8m.c to use platform driver. > > The other reason is that directly reading ocotp register causes kexec > kernel hang because the 1st kernel running will disable unused clks > after kernel boots up, and then ocotp clk will be disabled even if > bootloader enables it. When kexec kernel, ocotp clk needs to be enabled > before reading ocotp registers, and nvmem API with platform driver > supported can accomplish this. > > v2: remove the subject prefix "LF-2571-4" > v3: Keep the original way which uses device_initcall to read soc unique > ID, and add the other way which uses module_platform_driver and > nvmem API, so that it will not break the old version DTBs. > v4: delete "__maybe_unused" > delete MODULE_DEVICE_TABLE(of, imx8m_soc_match); > rename match table, "fsl,imx8mm/n/q/p" is actually a machine > compabile and "fsl,imx8mm/n/q/p-soc" is a compabile of soc@0 > delete "flag" and change to determine whether the pointer is NULL > ues of_find_matching_node_and_match() > delete of_match_ptr() Put all the patch version changelogs after --- separator, so they will not go to the commit. > > Signed-off-by: Alice Guo <alice.guo@nxp.com> > --- > drivers/soc/imx/soc-imx8m.c | 85 +++++++++++++++++++++++++++++++------ > 1 file changed, 73 insertions(+), 12 deletions(-) > > diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c > index cc57a384d74d..1b0a34e545ae 100644 > --- a/drivers/soc/imx/soc-imx8m.c > +++ b/drivers/soc/imx/soc-imx8m.c > @@ -5,6 +5,8 @@ > > #include <linux/init.h> > #include <linux/io.h> > +#include <linux/module.h> > +#include <linux/nvmem-consumer.h> > #include <linux/of_address.h> > #include <linux/slab.h> > #include <linux/sys_soc.h> > @@ -29,7 +31,7 @@ > > struct imx8_soc_data { > char *name; > - u32 (*soc_revision)(void); > + u32 (*soc_revision)(struct device *dev); > }; > > static u64 soc_uid; > @@ -50,7 +52,7 @@ static u32 imx8mq_soc_revision_from_atf(void) > static inline u32 imx8mq_soc_revision_from_atf(void) { return 0; }; > #endif > > -static u32 __init imx8mq_soc_revision(void) > +static u32 __init imx8mq_soc_revision(struct device *dev) > { > struct device_node *np; > void __iomem *ocotp_base; > @@ -75,9 +77,19 @@ static u32 __init imx8mq_soc_revision(void) > rev = REV_B1; > } > > - soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH); > - soc_uid <<= 32; > - soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW); > + if (dev) { > + int ret = 0; > + > + ret = nvmem_cell_read_u64(dev, "soc_unique_id", &soc_uid); > + if (ret) { > + iounmap(ocotp_base); What about other cleanup parts? of_node_put? > + return ret; > + } > + } else { > + soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH); > + soc_uid <<= 32; > + soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW); > + } > > iounmap(ocotp_base); > of_node_put(np); > @@ -107,7 +119,7 @@ static void __init imx8mm_soc_uid(void) > of_node_put(np); > } > > -static u32 __init imx8mm_soc_revision(void) > +static u32 __init imx8mm_soc_revision(struct device *dev) > { > struct device_node *np; > void __iomem *anatop_base; > @@ -125,7 +137,15 @@ static u32 __init imx8mm_soc_revision(void) > iounmap(anatop_base); > of_node_put(np); > > - imx8mm_soc_uid(); > + if (dev) { > + int ret = 0; > + > + ret = nvmem_cell_read_u64(dev, "soc_unique_id", &soc_uid); > + if (ret) > + return ret; > + } else { > + imx8mm_soc_uid(); > + } > > return rev; > } > @@ -150,7 +170,7 @@ static const struct imx8_soc_data imx8mp_soc_data = { > .soc_revision = imx8mm_soc_revision, > }; > > -static __maybe_unused const struct of_device_id imx8_soc_match[] = { > +static const struct of_device_id imx8_machine_match[] = { > { .compatible = "fsl,imx8mq", .data = &imx8mq_soc_data, }, > { .compatible = "fsl,imx8mm", .data = &imx8mm_soc_data, }, > { .compatible = "fsl,imx8mn", .data = &imx8mn_soc_data, }, > @@ -158,12 +178,20 @@ static __maybe_unused const struct of_device_id imx8_soc_match[] = { > { } > }; > > +static const struct of_device_id imx8_soc_match[] = { > + { .compatible = "fsl,imx8mq-soc", .data = &imx8mq_soc_data, }, > + { .compatible = "fsl,imx8mm-soc", .data = &imx8mm_soc_data, }, > + { .compatible = "fsl,imx8mn-soc", .data = &imx8mn_soc_data, }, > + { .compatible = "fsl,imx8mp-soc", .data = &imx8mp_soc_data, }, > + { } > +}; > + > #define imx8_revision(soc_rev) \ > soc_rev ? \ > kasprintf(GFP_KERNEL, "%d.%d", (soc_rev >> 4) & 0xf, soc_rev & 0xf) : \ > "unknown" > > -static int __init imx8_soc_init(void) > +static int imx8_soc_info(struct platform_device *pdev) > { > struct soc_device_attribute *soc_dev_attr; > struct soc_device *soc_dev; > @@ -182,7 +210,10 @@ static int __init imx8_soc_init(void) > if (ret) > goto free_soc; > > - id = of_match_node(imx8_soc_match, of_root); > + if (pdev) > + id = of_match_node(imx8_soc_match, pdev->dev.of_node); > + else > + id = of_match_node(imx8_machine_match, of_root); > if (!id) { > ret = -ENODEV; > goto free_soc; > @@ -191,8 +222,16 @@ static int __init imx8_soc_init(void) > data = id->data; > if (data) { > soc_dev_attr->soc_id = data->name; > - if (data->soc_revision) > - soc_rev = data->soc_revision(); > + if (data->soc_revision) { > + if (pdev) { > + soc_rev = data->soc_revision(&pdev->dev); > + ret = soc_rev; > + if (ret < 0) > + goto free_soc; > + } else { > + soc_rev = data->soc_revision(NULL); > + } > + } > } > > soc_dev_attr->revision = imx8_revision(soc_rev); > @@ -230,4 +269,26 @@ static int __init imx8_soc_init(void) > kfree(soc_dev_attr); > return ret; > } > + > +static int __init imx8_soc_init(void) > +{ > + int ret = 0; Skip the initialization. You need to document why the initcal is still there. Please write that it is purely for backward compatibility. Best regards, Krzysztof
On Fri, Nov 20, 2020 at 06:11:09PM +0800, Alice Guo wrote: > Add DT Binding doc for the Unique ID of i.MX 8M series. > > v2: remove the subject prefix "LF-2571-1" > v3: put it into Documentation/devicetree/bindings/arm/fsl.yaml > modify the description of nvmem-cells > use "make ARCH=arm64 dtbs_check" to test it and fix errors > v4: use allOf to limit new version DTS files for i.MX8M to include > "fsl,imx8mm/n/p/q-soc", nvmem-cells and nvmem-cells-names > > Signed-off-by: Alice Guo <alice.guo@nxp.com> > --- > .../devicetree/bindings/arm/fsl.yaml | 51 +++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml > index 67980dcef66d..d8048323a290 100644 > --- a/Documentation/devicetree/bindings/arm/fsl.yaml > +++ b/Documentation/devicetree/bindings/arm/fsl.yaml > @@ -918,6 +918,57 @@ properties: > - fsl,s32v234-evb # S32V234-EVB2 Customer Evaluation Board > - const: fsl,s32v234 > > + soc: > + type: object > + properties: > + compatible: > + oneOf: > + - description: new version compatible for i.MX8M SoCs > + items: > + - enum: > + - fsl,imx8mm-soc > + - fsl,imx8mn-soc > + - fsl,imx8mp-soc > + - fsl,imx8mq-soc > + - const: simple-bus > + > + - description: old version compatible for i.MX8M SoCs > + items: > + - const: simple-bus > + > + nvmem-cells: > + maxItems: 1 > + description: Phandle to the SOC Unique ID provided by a nvmem node > + > + nvmem-cells-names: > + const: soc_unique_id > + > + allOf: Nothing changed here comparing to previous version. Still does not work. The allOf should not be part of soc node because the "if" below won't match. Instead, it should be against root node. Best regards, Krzysztof > + - if: > + properties: > + compatible: > + contains: > + enum: > + - fsl,imx8mm > + - fsl,imx8mn > + - fsl,imx8mp > + - fsl,imx8mq > + > + then: > + properties: > + compatible: > + items: > + - enum: > + - fsl,imx8mm-soc > + - fsl,imx8mn-soc > + - fsl,imx8mp-soc > + - fsl,imx8mq-soc > + - const: simple-bus > + > + required: > + - nvmem-cells > + - nvmem-cells-names > + > additionalProperties: true > > ... > -- > 2.17.1 >
On Fri, Nov 20, 2020 at 06:11:11PM +0800, Alice Guo wrote: > In order to be able to use NVMEM APIs to read soc unique ID, add the > nvmem data cell and name for nvmem-cells to the "soc" node, and add a > nvmem node which provides soc unique ID to efuse@30350000. > > v2: remove the subject prefix "LF-2571-3" > v3: convert register addresses and sizes to hex > v4: delete "stuff" in subject and commit message, add detailed > description > > Signed-off-by: Alice Guo <alice.guo@nxp.com> > --- > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 6 ++++++ > arch/arm64/boot/dts/freescale/imx8mn.dtsi | 6 ++++++ > arch/arm64/boot/dts/freescale/imx8mp.dtsi | 6 ++++++ > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 6 ++++++ > 4 files changed, 24 insertions(+) > > diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > index d457ce815e68..0e0edd5db07b 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi > +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi > @@ -261,6 +261,8 @@ > #address-cells = <1>; > #size-cells = <1>; > ranges = <0x0 0x0 0x0 0x3e000000>; > + nvmem-cells = <&imx8mm_uid>; > + nvmem-cell-names = "soc_unique_id"; > > aips1: bus@30000000 { > compatible = "fsl,aips-bus", "simple-bus"; > @@ -518,6 +520,10 @@ > #address-cells = <1>; > #size-cells = <1>; > > + imx8mm_uid: unique_id@410 { Any reason why device node uses underscore, not a hyphen/dash? Other fields are proper (it's also naming convention of DT spec and dtc W=2 will point it). Best regards, Krzysztof > + reg = <0x4 0x8>; > + }; > + > cpu_speed_grade: speed-grade@10 { > reg = <0x10 4>; > };
> -----Original Message----- > From: Krzysztof Kozlowski <krzk@kernel.org> > Sent: 2020年11月20日 18:51 > To: Alice Guo <alice.guo@nxp.com> > Cc: robh+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de; > dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-arm-kernel@lists.infradead.org > Subject: [EXT] Re: [PATCH v4 1/4] dt-bindings: soc: imx8m: add DT Binding doc > for soc unique ID > > Caution: EXT Email > > On Fri, Nov 20, 2020 at 06:11:09PM +0800, Alice Guo wrote: > > Add DT Binding doc for the Unique ID of i.MX 8M series. > > > > v2: remove the subject prefix "LF-2571-1" > > v3: put it into Documentation/devicetree/bindings/arm/fsl.yaml > > modify the description of nvmem-cells > > use "make ARCH=arm64 dtbs_check" to test it and fix errors > > v4: use allOf to limit new version DTS files for i.MX8M to include > > "fsl,imx8mm/n/p/q-soc", nvmem-cells and nvmem-cells-names > > > > Signed-off-by: Alice Guo <alice.guo@nxp.com> > > --- > > .../devicetree/bindings/arm/fsl.yaml | 51 > +++++++++++++++++++ > > 1 file changed, 51 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml > > b/Documentation/devicetree/bindings/arm/fsl.yaml > > index 67980dcef66d..d8048323a290 100644 > > --- a/Documentation/devicetree/bindings/arm/fsl.yaml > > +++ b/Documentation/devicetree/bindings/arm/fsl.yaml > > @@ -918,6 +918,57 @@ properties: > > - fsl,s32v234-evb # S32V234-EVB2 Customer > Evaluation Board > > - const: fsl,s32v234 > > > > + soc: > > + type: object > > + properties: > > + compatible: > > + oneOf: > > + - description: new version compatible for i.MX8M SoCs > > + items: > > + - enum: > > + - fsl,imx8mm-soc > > + - fsl,imx8mn-soc > > + - fsl,imx8mp-soc > > + - fsl,imx8mq-soc > > + - const: simple-bus > > + > > + - description: old version compatible for i.MX8M SoCs > > + items: > > + - const: simple-bus > > + > > + nvmem-cells: > > + maxItems: 1 > > + description: Phandle to the SOC Unique ID provided by a nvmem > > + node > > + > > + nvmem-cells-names: > > + const: soc_unique_id > > + > > + allOf: > > Nothing changed here comparing to previous version. Still does not work. > > The allOf should not be part of soc node because the "if" below won't match. > Instead, it should be against root node. > > Best regards, > Krzysztof I'm sorry to disturb you. I don't very clear about the grammar rules of yaml, so don't know how to make allOf be part of root node. If allof is part of root node, how does it specify the soc node? The following is my change which is definitely wrong. soc: properties: compatible: - description: new version compatible for i.MX8M SoCs items: - enum: - fsl,imx8mm-soc - fsl,imx8mn-soc - fsl,imx8mp-soc - fsl,imx8mq-soc - const: simple-bus nvmem-cells: maxItems: 1 description: Phandle to the SOC Unique ID provided by a nvmem node nvmem-cells-names: const: soc_unique_id allOf: - if: properties: compatible: contains: enum: - fsl,imx8mm - fsl,imx8mn - fsl,imx8mp - fsl,imx8mq then: required: - soc Please give me suggestion. Thank you. Best regards, Alice Guo > > + - if: > > + properties: > > + compatible: > > + contains: > > + enum: > > + - fsl,imx8mm > > + - fsl,imx8mn > > + - fsl,imx8mp > > + - fsl,imx8mq > > + > > + then: > > + properties: > > + compatible: > > + items: > > + - enum: > > + - fsl,imx8mm-soc > > + - fsl,imx8mn-soc > > + - fsl,imx8mp-soc > > + - fsl,imx8mq-soc > > + - const: simple-bus > > + > > + required: > > + - nvmem-cells > > + - nvmem-cells-names > > + > > additionalProperties: true > > > > ... > > -- > > 2.17.1 > >
On Mon, Nov 23, 2020 at 04:45:13AM +0000, Alice Guo wrote: > > > > -----Original Message----- > > From: Krzysztof Kozlowski <krzk@kernel.org> > > Sent: 2020年11月20日 18:51 > > To: Alice Guo <alice.guo@nxp.com> > > Cc: robh+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de; > > dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>; > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > > linux-arm-kernel@lists.infradead.org > > Subject: [EXT] Re: [PATCH v4 1/4] dt-bindings: soc: imx8m: add DT Binding doc > > for soc unique ID > > > > Caution: EXT Email > > > > On Fri, Nov 20, 2020 at 06:11:09PM +0800, Alice Guo wrote: > > > Add DT Binding doc for the Unique ID of i.MX 8M series. > > > > > > v2: remove the subject prefix "LF-2571-1" > > > v3: put it into Documentation/devicetree/bindings/arm/fsl.yaml > > > modify the description of nvmem-cells > > > use "make ARCH=arm64 dtbs_check" to test it and fix errors > > > v4: use allOf to limit new version DTS files for i.MX8M to include > > > "fsl,imx8mm/n/p/q-soc", nvmem-cells and nvmem-cells-names > > > > > > Signed-off-by: Alice Guo <alice.guo@nxp.com> > > > --- > > > .../devicetree/bindings/arm/fsl.yaml | 51 > > +++++++++++++++++++ > > > 1 file changed, 51 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml > > > b/Documentation/devicetree/bindings/arm/fsl.yaml > > > index 67980dcef66d..d8048323a290 100644 > > > --- a/Documentation/devicetree/bindings/arm/fsl.yaml > > > +++ b/Documentation/devicetree/bindings/arm/fsl.yaml > > > @@ -918,6 +918,57 @@ properties: > > > - fsl,s32v234-evb # S32V234-EVB2 Customer > > Evaluation Board > > > - const: fsl,s32v234 > > > > > > + soc: > > > + type: object > > > + properties: > > > + compatible: > > > + oneOf: > > > + - description: new version compatible for i.MX8M SoCs > > > + items: > > > + - enum: > > > + - fsl,imx8mm-soc > > > + - fsl,imx8mn-soc > > > + - fsl,imx8mp-soc > > > + - fsl,imx8mq-soc > > > + - const: simple-bus > > > + > > > + - description: old version compatible for i.MX8M SoCs > > > + items: > > > + - const: simple-bus > > > + > > > + nvmem-cells: > > > + maxItems: 1 > > > + description: Phandle to the SOC Unique ID provided by a nvmem > > > + node > > > + > > > + nvmem-cells-names: > > > + const: soc_unique_id > > > + > > > + allOf: > > > > Nothing changed here comparing to previous version. Still does not work. > > > > The allOf should not be part of soc node because the "if" below won't match. > > Instead, it should be against root node. > > > > Best regards, > > Krzysztof > > I'm sorry to disturb you. I don't very clear about the grammar rules of yaml, so don't know how to make allOf be part of root node. > If allof is part of root node, how does it specify the soc node? The following is my change which is definitely wrong. > soc: > properties: > compatible: > - description: new version compatible for i.MX8M SoCs > items: > - enum: > - fsl,imx8mm-soc > - fsl,imx8mn-soc > - fsl,imx8mp-soc > - fsl,imx8mq-soc > - const: simple-bus > > nvmem-cells: > maxItems: 1 > description: Phandle to the SOC Unique ID provided by a nvmem node > > nvmem-cells-names: > const: soc_unique_id > > allOf: > - if: > properties: > compatible: > contains: > enum: > - fsl,imx8mm > - fsl,imx8mn > - fsl,imx8mp > - fsl,imx8mq > > then: > required: > - soc > > Please give me suggestion. Thank you. This should work: 940 allOf: 941 - if: 942 properties: 943 compatible: 944 contains: 945 enum: 946 - fsl,imx8mm 947 - fsl,imx8mn 948 - fsl,imx8mp 949 - fsl,imx8mq 950 951 then: 952 patternProperties: 953 "^soc@[0-9a-f]+$": 954 properties: 955 compatible: 956 items: 957 - enum: 958 - fsl,imx8mm-soc 959 - fsl,imx8mn-soc 960 - fsl,imx8mp-soc 961 - fsl,imx8mq-soc 962 - const: simple-bus 963 964 required: 965 - nvmem-cells 966 - nvmem-cells-names And probablt "soc" should also be added to required, just below the "then:". You need to test it and find the proper solution. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml index 67980dcef66d..d8048323a290 100644 --- a/Documentation/devicetree/bindings/arm/fsl.yaml +++ b/Documentation/devicetree/bindings/arm/fsl.yaml @@ -918,6 +918,57 @@ properties: - fsl,s32v234-evb # S32V234-EVB2 Customer Evaluation Board - const: fsl,s32v234 + soc: + type: object + properties: + compatible: + oneOf: + - description: new version compatible for i.MX8M SoCs + items: + - enum: + - fsl,imx8mm-soc + - fsl,imx8mn-soc + - fsl,imx8mp-soc + - fsl,imx8mq-soc + - const: simple-bus + + - description: old version compatible for i.MX8M SoCs + items: + - const: simple-bus + + nvmem-cells: + maxItems: 1 + description: Phandle to the SOC Unique ID provided by a nvmem node + + nvmem-cells-names: + const: soc_unique_id + + allOf: + - if: + properties: + compatible: + contains: + enum: + - fsl,imx8mm + - fsl,imx8mn + - fsl,imx8mp + - fsl,imx8mq + + then: + properties: + compatible: + items: + - enum: + - fsl,imx8mm-soc + - fsl,imx8mn-soc + - fsl,imx8mp-soc + - fsl,imx8mq-soc + - const: simple-bus + + required: + - nvmem-cells + - nvmem-cells-names + additionalProperties: true ...
Add DT Binding doc for the Unique ID of i.MX 8M series. v2: remove the subject prefix "LF-2571-1" v3: put it into Documentation/devicetree/bindings/arm/fsl.yaml modify the description of nvmem-cells use "make ARCH=arm64 dtbs_check" to test it and fix errors v4: use allOf to limit new version DTS files for i.MX8M to include "fsl,imx8mm/n/p/q-soc", nvmem-cells and nvmem-cells-names Signed-off-by: Alice Guo <alice.guo@nxp.com> --- .../devicetree/bindings/arm/fsl.yaml | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) -- 2.17.1