Re: [PATCH 2/2] arm64: dts: imx93: add DDR controller node

Re: [PATCH 2/2] arm64: dts: imx93: add DDR controller node

[Krzysztof Kozlowski]

On 14/09/2023 12:20, Xu Yang wrote:
> Add DDR controller node which will be used by EDAC driver later, also
> move the DDR PMU node as the subnode of the DDR controller.
> 
> Signed-off-by: Ye Li <ye.li@nxp.com>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> ---
>  arch/arm64/boot/dts/freescale/imx93.dtsi | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx93.dtsi b/arch/arm64/boot/dts/freescale/imx93.dtsi
> index 6f85a05ee7e1..992bdeef70cd 100644
> --- a/arch/arm64/boot/dts/freescale/imx93.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx93.dtsi
> @@ -917,10 +917,20 @@ media_blk_ctrl: system-controller@4ac10000 {
>  			status = "disabled";
>  		};
>  
> -		ddr-pmu@4e300dc0 {
> -			compatible = "fsl,imx93-ddr-pmu";
> -			reg = <0x4e300dc0 0x200>;
> -			interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
> +		ddr: memory-controller@4e300000 {
> +			compatible = "simple-mfd";

No, that's not allowed alone.

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

Best regards,
Krzysztof

[PATCH 1/2] perf: imx9_ddr_perf: resolve resource map conflict

[Xu Yang]

Usually, the ddr pmu node will be a subnode of DDR controller, then using
devm_platform_ioremap_resource will report conflict with DDR controller
resource. So update the driver to use devm_ioremap to avoid such
resource check.

Signed-off-by: Ye Li <ye.li@nxp.com>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 drivers/perf/fsl_imx9_ddr_perf.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/perf/fsl_imx9_ddr_perf.c b/drivers/perf/fsl_imx9_ddr_perf.c
index 5cf770a1bc31..885024665968 100644
--- a/drivers/perf/fsl_imx9_ddr_perf.c
+++ b/drivers/perf/fsl_imx9_ddr_perf.c
@@ -602,8 +602,15 @@ static int ddr_perf_probe(struct platform_device *pdev)
 	void __iomem *base;
 	int ret, irq;
 	char *name;
+	struct resource *r;
 
-	base = devm_platform_ioremap_resource(pdev, 0);
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!r) {
+		dev_err(&pdev->dev, "platform_get_resource() failed\n");
+		return -ENOMEM;
+	}
+
+	base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
 	if (IS_ERR(base))
 		return PTR_ERR(base);
 
-- 
2.34.1

[PATCH 2/2] arm64: dts: imx93: add DDR controller node

[Xu Yang]

Add DDR controller node which will be used by EDAC driver later, also
move the DDR PMU node as the subnode of the DDR controller.

Signed-off-by: Ye Li <ye.li@nxp.com>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx93.dtsi | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx93.dtsi b/arch/arm64/boot/dts/freescale/imx93.dtsi
index 6f85a05ee7e1..992bdeef70cd 100644
--- a/arch/arm64/boot/dts/freescale/imx93.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx93.dtsi
@@ -917,10 +917,20 @@ media_blk_ctrl: system-controller@4ac10000 {
 			status = "disabled";
 		};
 
-		ddr-pmu@4e300dc0 {
-			compatible = "fsl,imx93-ddr-pmu";
-			reg = <0x4e300dc0 0x200>;
-			interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
+		ddr: memory-controller@4e300000 {
+			compatible = "simple-mfd";
+			reg = <0x4e300000 0x2000>;
+			interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
+			little-endian;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+
+			ddr-pmu@4e300dc0 {
+				compatible = "fsl,imx93-ddr-pmu";
+				reg = <0x4e300dc0 0x200>;
+				interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
+			};
 		};
 	};
 };
-- 
2.34.1

Re: [PATCH 1/2] perf: imx9_ddr_perf: resolve resource map conflict

[Krzysztof Kozlowski]

On 14/09/2023 12:20, Xu Yang wrote:
> Usually, the ddr pmu node will be a subnode of DDR controller, then using
> devm_platform_ioremap_resource will report conflict with DDR controller
> resource. So update the driver to use devm_ioremap to avoid such
> resource check.
> 

Why would you like to map same region twice? The resource check is for
purpose there...

> Signed-off-by: Ye Li <ye.li@nxp.com>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> ---
>  drivers/perf/fsl_imx9_ddr_perf.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/fsl_imx9_ddr_perf.c b/drivers/perf/fsl_imx9_ddr_perf.c
> index 5cf770a1bc31..885024665968 100644
> --- a/drivers/perf/fsl_imx9_ddr_perf.c
> +++ b/drivers/perf/fsl_imx9_ddr_perf.c
> @@ -602,8 +602,15 @@ static int ddr_perf_probe(struct platform_device *pdev)
>  	void __iomem *base;
>  	int ret, irq;
>  	char *name;
> +	struct resource *r;
>  
> -	base = devm_platform_ioremap_resource(pdev, 0);
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!r) {
> +		dev_err(&pdev->dev, "platform_get_resource() failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	base = devm_ioremap(&pdev->dev, r->start, resource_size(r));

You need to document this, otherwise someone will revert your commit soon.


Best regards,
Krzysztof

RE: [EXT] [PATCH 2/2] arm64: dts: imx93: add DDR controller node

[Frank Li]

> -----Original Message-----
> From: Xu Yang <xu.yang_2@nxp.com>
> Sent: Thursday, September 14, 2023 5:21 AM
> To: will@kernel.org; mark.rutland@arm.com; robh+dt@kernel.org;
> shawnguo@kernel.org
> Cc: krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; devicetree@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; Ye Li <ye.li@nxp.com>
> Subject: [EXT] [PATCH 2/2] arm64: dts: imx93: add DDR controller node
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> Add DDR controller node which will be used by EDAC driver later, also
> move the DDR PMU node as the subnode of the DDR controller.
> 
> Signed-off-by: Ye Li <ye.li@nxp.com>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> ---
>  arch/arm64/boot/dts/freescale/imx93.dtsi | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx93.dtsi
> b/arch/arm64/boot/dts/freescale/imx93.dtsi
> index 6f85a05ee7e1..992bdeef70cd 100644
> --- a/arch/arm64/boot/dts/freescale/imx93.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx93.dtsi
> @@ -917,10 +917,20 @@ media_blk_ctrl: system-controller@4ac10000 {
>                         status = "disabled";
>                 };
> 
> -               ddr-pmu@4e300dc0 {
> -                       compatible = "fsl,imx93-ddr-pmu";
> -                       reg = <0x4e300dc0 0x200>;
> -                       interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
> +               ddr: memory-controller@4e300000 {
> +                       compatible = "simple-mfd";
> +                       reg = <0x4e300000 0x2000>;

[Frank Li] Can you just use EDAC register space size? 
I suppose EDAC and PMU's register space is not over lapped.  

> +                       interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
> +                       little-endian;
> +                       #address-cells = <1>;
> +                       #size-cells = <1>;
> +                       ranges;
> +
> +                       ddr-pmu@4e300dc0 {
> +                               compatible = "fsl,imx93-ddr-pmu";
> +                               reg = <0x4e300dc0 0x200>;
> +                               interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
> +                       };
>                 };
>         };
>  };
> --
> 2.34.1
> 

RE: [EXT] [PATCH 2/2] arm64: dts: imx93: add DDR controller node

[Ye Li]

Hi Frank,

> -----Original Message-----
> From: Frank Li <frank.li@nxp.com>
> Sent: Thursday, September 14, 2023 10:21 PM
> To: Xu Yang <xu.yang_2@nxp.com>; will@kernel.org; mark.rutland@arm.com;
> robh+dt@kernel.org; shawnguo@kernel.org
> Cc: krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; Ye Li <ye.li@nxp.com>
> Subject: RE: [EXT] [PATCH 2/2] arm64: dts: imx93: add DDR controller node
> 
> 
> 
> > -----Original Message-----
> > From: Xu Yang <xu.yang_2@nxp.com>
> > Sent: Thursday, September 14, 2023 5:21 AM
> > To: will@kernel.org; mark.rutland@arm.com; robh+dt@kernel.org;
> > shawnguo@kernel.org
> > Cc: krzysztof.kozlowski+dt@linaro.org; conor+dt@kernel.org;
> > dl-linux-imx <linux-imx@nxp.com>; devicetree@vger.kernel.org;
> > linux-arm- kernel@lists.infradead.org; Ye Li <ye.li@nxp.com>
> > Subject: [EXT] [PATCH 2/2] arm64: dts: imx93: add DDR controller node
> >
> > Caution: This is an external email. Please take care when clicking
> > links or opening attachments. When in doubt, report the message using
> > the 'Report this email' button
> >
> >
> > Add DDR controller node which will be used by EDAC driver later, also
> > move the DDR PMU node as the subnode of the DDR controller.
> >
> > Signed-off-by: Ye Li <ye.li@nxp.com>
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > ---
> >  arch/arm64/boot/dts/freescale/imx93.dtsi | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx93.dtsi
> > b/arch/arm64/boot/dts/freescale/imx93.dtsi
> > index 6f85a05ee7e1..992bdeef70cd 100644
> > --- a/arch/arm64/boot/dts/freescale/imx93.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx93.dtsi
> > @@ -917,10 +917,20 @@ media_blk_ctrl: system-controller@4ac10000 {
> >                         status = "disabled";
> >                 };
> >
> > -               ddr-pmu@4e300dc0 {
> > -                       compatible = "fsl,imx93-ddr-pmu";
> > -                       reg = <0x4e300dc0 0x200>;
> > -                       interrupts = <GIC_SPI 90
> IRQ_TYPE_LEVEL_HIGH>;
> > +               ddr: memory-controller@4e300000 {
> > +                       compatible = "simple-mfd";
> > +                       reg = <0x4e300000 0x2000>;
> 
> [Frank Li] Can you just use EDAC register space size?
> I suppose EDAC and PMU's register space is not over lapped.
> 
We will re-use layerscape EDAC driver which needs DDR controller's base address.

Best regards,
Ye Li
> > +                       interrupts = <GIC_SPI 91
> IRQ_TYPE_LEVEL_HIGH>;
> > +                       little-endian;
> > +                       #address-cells = <1>;
> > +                       #size-cells = <1>;
> > +                       ranges;
> > +
> > +                       ddr-pmu@4e300dc0 {
> > +                               compatible = "fsl,imx93-ddr-pmu";
> > +                               reg = <0x4e300dc0 0x200>;
> > +                               interrupts = <GIC_SPI 90
> IRQ_TYPE_LEVEL_HIGH>;
> > +                       };
> >                 };
> >         };
> >  };
> > --
> > 2.34.1
> >

RE: [EXT] Re: [PATCH 2/2] arm64: dts: imx93: add DDR controller node

[Xu Yang]

Hi Krzysztof,

> On 14/09/2023 12:20, Xu Yang wrote:
> > Add DDR controller node which will be used by EDAC driver later, also
> > move the DDR PMU node as the subnode of the DDR controller.
> >
> > Signed-off-by: Ye Li <ye.li@nxp.com>
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > ---
> >  arch/arm64/boot/dts/freescale/imx93.dtsi | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx93.dtsi b/arch/arm64/boot/dts/freescale/imx93.dtsi
> > index 6f85a05ee7e1..992bdeef70cd 100644
> > --- a/arch/arm64/boot/dts/freescale/imx93.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx93.dtsi
> > @@ -917,10 +917,20 @@ media_blk_ctrl: system-controller@4ac10000 {
> >                       status = "disabled";
> >               };
> >
> > -             ddr-pmu@4e300dc0 {
> > -                     compatible = "fsl,imx93-ddr-pmu";
> > -                     reg = <0x4e300dc0 0x200>;
> > -                     interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
> > +             ddr: memory-controller@4e300000 {
> > +                     compatible = "simple-mfd";
> 
> No, that's not allowed alone.

Well, then how should I modify this compatible?

> 
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` 
> 

I just run the check script, it seems no warnings for this node.

$ dt-validate -s Documentation/devicetree/bindings/processed-schema.json arch/arm64/boot/dts/freescale/imx93-11x11-evk.dtb
/home/nxf75279/work/linux-next/arch/arm64/boot/dts/freescale/imx93-11x11-evk.dtb: tmu@44482000: fsl,tmu-range: 'oneOf' conditional failed, one must be fixed:
        [2147483866, 2147483881, 2147483906, 2147483946, 2147484006, 2147484071, 2147484086] is too long
        From schema: /home/nxf75279/work/linux-next/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml
/home/nxf75279/work/linux-next/arch/arm64/boot/dts/freescale/imx93-11x11-evk.dtb: tmu@44482000: 'oneOf' conditional failed, one must be fixed:
        'interrupts' is a required property
        'interrupts-extended' is a required property
        From schema: /home/nxf75279/work/linux-next/Documentation/devicetree/bindings/thermal/qoriq-thermal.yaml
/home/nxf75279/work/linux-next/arch/arm64/boot/dts/freescale/imx93-11x11-evk.dtb: gpio@43820080: gpio-ranges: [[30, 0, 84, 8], [30, 8, 66, 18], [30, 26, 34, 2], [30, 28, 0, 4]] is too long
        From schema: /home/nxf75279/work/linux-next/Documentation/devicetree/bindings/gpio/gpio-vf610.yaml
/home/nxf75279/work/linux-next/arch/arm64/boot/dts/freescale/imx93-11x11-evk.dtb: gpio@43830080: gpio-ranges: [[30, 0, 38, 28], [30, 28, 36, 2]] is too long
        From schema: /home/nxf75279/work/linux-next/Documentation/devicetree/bindings/gpio/gpio-vf610.yaml

Thanks,
Xu Yang

RE: [EXT] Re: [PATCH 1/2] perf: imx9_ddr_perf: resolve resource map conflict

[Xu Yang]

Hi Krzysztof,

> On 14/09/2023 12:20, Xu Yang wrote:
> > Usually, the ddr pmu node will be a subnode of DDR controller, then using
> > devm_platform_ioremap_resource will report conflict with DDR controller
> > resource. So update the driver to use devm_ioremap to avoid such
> > resource check.
> >
> 
> Why would you like to map same region twice? The resource check is for
> purpose there...
> 

Because the ddr pmu region is a subset of ddr controller region. When
edac driver is enabled, it will map the whole region firstly. I will check
if this can be changed.

> > Signed-off-by: Ye Li <ye.li@nxp.com>
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > ---
> >  drivers/perf/fsl_imx9_ddr_perf.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/perf/fsl_imx9_ddr_perf.c b/drivers/perf/fsl_imx9_ddr_perf.c
> > index 5cf770a1bc31..885024665968 100644
> > --- a/drivers/perf/fsl_imx9_ddr_perf.c
> > +++ b/drivers/perf/fsl_imx9_ddr_perf.c
> > @@ -602,8 +602,15 @@ static int ddr_perf_probe(struct platform_device *pdev)
> >       void __iomem *base;
> >       int ret, irq;
> >       char *name;
> > +     struct resource *r;
> >
> > -     base = devm_platform_ioremap_resource(pdev, 0);
> > +     r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +     if (!r) {
> > +             dev_err(&pdev->dev, "platform_get_resource() failed\n");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
> 
> You need to document this, otherwise someone will revert your commit soon.
> 
> 
> Best regards,
> Krzysztof

Thanks,
Xu Yang

Re: [EXT] Re: [PATCH 2/2] arm64: dts: imx93: add DDR controller node

[Krzysztof Kozlowski]

On 15/09/2023 04:39, Xu Yang wrote:
> Hi Krzysztof,
> 
>> On 14/09/2023 12:20, Xu Yang wrote:
>>> Add DDR controller node which will be used by EDAC driver later, also
>>> move the DDR PMU node as the subnode of the DDR controller.
>>>
>>> Signed-off-by: Ye Li <ye.li@nxp.com>
>>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
>>> ---
>>>  arch/arm64/boot/dts/freescale/imx93.dtsi | 18 ++++++++++++++----
>>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/freescale/imx93.dtsi b/arch/arm64/boot/dts/freescale/imx93.dtsi
>>> index 6f85a05ee7e1..992bdeef70cd 100644
>>> --- a/arch/arm64/boot/dts/freescale/imx93.dtsi
>>> +++ b/arch/arm64/boot/dts/freescale/imx93.dtsi
>>> @@ -917,10 +917,20 @@ media_blk_ctrl: system-controller@4ac10000 {
>>>                       status = "disabled";
>>>               };
>>>
>>> -             ddr-pmu@4e300dc0 {
>>> -                     compatible = "fsl,imx93-ddr-pmu";
>>> -                     reg = <0x4e300dc0 0x200>;
>>> -                     interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
>>> +             ddr: memory-controller@4e300000 {
>>> +                     compatible = "simple-mfd";
>>
>> No, that's not allowed alone.
> 
> Well, then how should I modify this compatible?

You need specific compatible, just like everywhere else. You can use
"git grep simple-mfd" or "git grep simple-mfd -- arch/arm*" or on
bindings to figure this out.

Just remember that simple-mfd means children do not depend on anything
provided by the parent. You cannot later remove it "just because" or
"oh, now I want driver". That would affect users of DTS.

> 
>>
>> It does not look like you tested the DTS against bindings. Please run
>> `make dtbs_check W=1` 
>>
> 
> I just run the check script, it seems no warnings for this node.

Right, I always forget we did not convert simple-mfd to DT schema, so
the warnings are only for syscon.

Best regards,
Krzysztof

RE: [EXT] Re: [PATCH 2/2] arm64: dts: imx93: add DDR controller node

[Xu Yang]

> On 15/09/2023 04:39, Xu Yang wrote:
> > Hi Krzysztof,
> >
> >> On 14/09/2023 12:20, Xu Yang wrote:
> >>> Add DDR controller node which will be used by EDAC driver later, also
> >>> move the DDR PMU node as the subnode of the DDR controller.
> >>>
> >>> Signed-off-by: Ye Li <ye.li@nxp.com>
> >>> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> >>> ---
> >>>  arch/arm64/boot/dts/freescale/imx93.dtsi | 18 ++++++++++++++----
> >>>  1 file changed, 14 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/freescale/imx93.dtsi b/arch/arm64/boot/dts/freescale/imx93.dtsi
> >>> index 6f85a05ee7e1..992bdeef70cd 100644
> >>> --- a/arch/arm64/boot/dts/freescale/imx93.dtsi
> >>> +++ b/arch/arm64/boot/dts/freescale/imx93.dtsi
> >>> @@ -917,10 +917,20 @@ media_blk_ctrl: system-controller@4ac10000 {
> >>>                       status = "disabled";
> >>>               };
> >>>
> >>> -             ddr-pmu@4e300dc0 {
> >>> -                     compatible = "fsl,imx93-ddr-pmu";
> >>> -                     reg = <0x4e300dc0 0x200>;
> >>> -                     interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
> >>> +             ddr: memory-controller@4e300000 {
> >>> +                     compatible = "simple-mfd";
> >>
> >> No, that's not allowed alone.
> >
> > Well, then how should I modify this compatible?
> 
> You need specific compatible, just like everywhere else. You can use
> "git grep simple-mfd" or "git grep simple-mfd -- arch/arm*" or on
> bindings to figure this out.
> 
> Just remember that simple-mfd means children do not depend on anything
> provided by the parent. You cannot later remove it "just because" or
> "oh, now I want driver". That would affect users of DTS.

I see. Thanks for your explanation.

Best Regards,
Xu Yang

> 
> >
> >>
> >> It does not look like you tested the DTS against bindings. Please run
> >> `make dtbs_check W=1`
> >>
> >
> > I just run the check script, it seems no warnings for this node.
> 
> Right, I always forget we did not convert simple-mfd to DT schema, so
> the warnings are only for syscon.
> 
> Best regards,
> Krzysztof