[PATCH] arm64: dts: smaug: Add EMC frequency change tables

[PATCH] arm64: dts: smaug: Add EMC frequency change tables

[Diogo Ivo]

Add the reserved-memory regions of the nominal and derated tables setup by
the bootloader so that the EMC frequency change code can access them.

Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
---
 arch/arm64/boot/dts/nvidia/tegra210-smaug.dts | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
index 709f3f417a19..a74826ae97b4 100644
--- a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
+++ b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
@@ -30,6 +30,22 @@ memory {
 		reg = <0x0 0x80000000 0x0 0xc0000000>;
 	};
 
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		nominal: emc-table@f67cc000 {
+			compatible = "nvidia,tegra210-emc-table";
+			reg = <0x0 0xf67cc000 0x0 0xbea0>;
+		};
+
+		derated: emc-table@f67d7ea0 {
+			compatible = "nvidia,tegra210-emc-table";
+			reg = <0x0 0xf67d7ea0 0x0 0xbea0>;
+		};
+	};
+
 	host1x@50000000 {
 		dc@54200000 {
 			status = "okay";
@@ -1735,6 +1751,11 @@ pmc@7000e400 {
 		status = "okay";
 	};
 
+	emc: external-memory-controller@7001b000 {
+		memory-region = <&nominal>, <&derated>;
+		memory-region-names = "nominal", "derated";
+	};
+
 	usb@70090000 {
 		phys = <&{/padctl@7009f000/pads/usb2/lanes/usb2-0}>,
 		       <&{/padctl@7009f000/pads/pcie/lanes/pcie-6}>;
-- 
2.40.0

Re: [PATCH] arm64: dts: smaug: Add EMC frequency change tables

[Krzysztof Kozlowski]

On 19/03/2023 20:42, Diogo Ivo wrote:
> Add the reserved-memory regions of the nominal and derated tables setup by
> the bootloader so that the EMC frequency change code can access them.
> 
> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
> ---
>  arch/arm64/boot/dts/nvidia/tegra210-smaug.dts | 21 +++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> index 709f3f417a19..a74826ae97b4 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> +++ b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> @@ -30,6 +30,22 @@ memory {
>  		reg = <0x0 0x80000000 0x0 0xc0000000>;
>  	};
>  
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		nominal: emc-table@f67cc000 {
> +			compatible = "nvidia,tegra210-emc-table";
> +			reg = <0x0 0xf67cc000 0x0 0xbea0>;
> +		};
> +
> +		derated: emc-table@f67d7ea0 {
> +			compatible = "nvidia,tegra210-emc-table";
> +			reg = <0x0 0xf67d7ea0 0x0 0xbea0>;
> +		};
> +	};
> +
>  	host1x@50000000 {
>  		dc@54200000 {
>  			status = "okay";
> @@ -1735,6 +1751,11 @@ pmc@7000e400 {
>  		status = "okay";
>  	};
>  
> +	emc: external-memory-controller@7001b000 {

Node names should be generic, so memory-controller.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Check your code for warnings (dtc, dtbs_check). Node with unit address
requires reg or ranges.

Best regards,
Krzysztof

Re: [PATCH] arm64: dts: smaug: Add EMC frequency change tables

[Diogo Ivo]

On Mon, Mar 20, 2023 at 07:50:27AM +0100, Krzysztof Kozlowski wrote:
> On 19/03/2023 20:42, Diogo Ivo wrote:
> > +	emc: external-memory-controller@7001b000 {
> 
> Node names should be generic, so memory-controller.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 
> Check your code for warnings (dtc, dtbs_check). Node with unit address
> requires reg or ranges.
> 
> Best regards,
> Krzysztof


Hello,

The external-memory-controller node is declared in tegra210.dtsi, so
here I am just adding more properties to it.

Best regards,
Diogo

Re: [PATCH] arm64: dts: smaug: Add EMC frequency change tables

[Krzysztof Kozlowski]

On 20/03/2023 12:34, Diogo Ivo wrote:
> On Mon, Mar 20, 2023 at 07:50:27AM +0100, Krzysztof Kozlowski wrote:
>> On 19/03/2023 20:42, Diogo Ivo wrote:
>>> +	emc: external-memory-controller@7001b000 {
>>
>> Node names should be generic, so memory-controller.
>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>>
>> Check your code for warnings (dtc, dtbs_check). Node with unit address
>> requires reg or ranges.
>>
>> Best regards,
>> Krzysztof
> 
> 
> Hello,
> 
> The external-memory-controller node is declared in tegra210.dtsi, so
> here I am just adding more properties to it.

Ah, ok, apologies then for false alarm. This Tegra notation of extending
existing nodes is crazy confusing...

Best regards,
Krzysztof

Re: [PATCH] arm64: dts: smaug: Add EMC frequency change tables

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

On Sun, Mar 19, 2023 at 07:42:55PM +0000, Diogo Ivo wrote:
> Add the reserved-memory regions of the nominal and derated tables setup by
> the bootloader so that the EMC frequency change code can access them.
> 
> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
> ---
>  arch/arm64/boot/dts/nvidia/tegra210-smaug.dts | 21 +++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> index 709f3f417a19..a74826ae97b4 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> +++ b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> @@ -30,6 +30,22 @@ memory {
>  		reg = <0x0 0x80000000 0x0 0xc0000000>;
>  	};
>  
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		nominal: emc-table@f67cc000 {
> +			compatible = "nvidia,tegra210-emc-table";
> +			reg = <0x0 0xf67cc000 0x0 0xbea0>;
> +		};
> +
> +		derated: emc-table@f67d7ea0 {
> +			compatible = "nvidia,tegra210-emc-table";
> +			reg = <0x0 0xf67d7ea0 0x0 0xbea0>;
> +		};
> +	};
> +

These tables are typically generated by the firmware/bootloader at
runtime. Often they'll use heap allocations for these, so the addresses
are not guaranteed to be static.

Can you share a few details about what set of components you've used to
set this up? If we add these we want to be as specific as possible that
people use exactly the same set of firmware files.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] arm64: dts: smaug: Add EMC frequency change tables

[Diogo Ivo]

On Wed, Apr 05, 2023 at 03:18:23PM +0200, Thierry Reding wrote:
> On Sun, Mar 19, 2023 at 07:42:55PM +0000, Diogo Ivo wrote:
> > Add the reserved-memory regions of the nominal and derated tables setup by
> > the bootloader so that the EMC frequency change code can access them.
> > 
> > Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
> > ---
> >  arch/arm64/boot/dts/nvidia/tegra210-smaug.dts | 21 +++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> > index 709f3f417a19..a74826ae97b4 100644
> > --- a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> > +++ b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> > @@ -30,6 +30,22 @@ memory {
> >  		reg = <0x0 0x80000000 0x0 0xc0000000>;
> >  	};
> >  
> > +	reserved-memory {
> > +		#address-cells = <2>;
> > +		#size-cells = <2>;
> > +		ranges;
> > +
> > +		nominal: emc-table@f67cc000 {
> > +			compatible = "nvidia,tegra210-emc-table";
> > +			reg = <0x0 0xf67cc000 0x0 0xbea0>;
> > +		};
> > +
> > +		derated: emc-table@f67d7ea0 {
> > +			compatible = "nvidia,tegra210-emc-table";
> > +			reg = <0x0 0xf67d7ea0 0x0 0xbea0>;
> > +		};
> > +	};
> > +
> 
> These tables are typically generated by the firmware/bootloader at
> runtime. Often they'll use heap allocations for these, so the addresses
> are not guaranteed to be static.
> 
> Can you share a few details about what set of components you've used to
> set this up? If we add these we want to be as specific as possible that
> people use exactly the same set of firmware files.
> 
> Thierry

In the case of the Pixel C, coreboot takes care of copying these tables
into a static cbmem region. The actual function taking care of this is
tegra210_run_mtc(), found in [1], and from here it is possible to
construct the cbmem memory layout. I have confirmed that the device is
actually running this version of coreboot from the cbmem_console logs,
where the commits are explicitly stated. These logs also indicate where
the tables are located,

Wrote coreboot table at: 00000000f66ca000, 0x2c0 bytes, checksum 3601
coreboot table: 728 bytes.
CBMEM ROOT  0. f67ff000 00001000
CONSOLE     1. f67ef000 00010000
TIME STAMP  2. f67ee000 00001000
VBOOT       3. f67ed000 00001000
VPD         4. f67e4000 00009000
MTC         5. f67cc000 00018000
RAMOOPS     6. f66cc000 00100000
COREBOOT    7. f66ca000 00002000

and

MTC: Copied 0x17d40 bytes from 0000000081008b70 to 00000000f67cc000

Diogo

[1]: https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/refs/heads/firmware-smaug-7900.B/src/soc/nvidia/tegra210/mtc.c

Re: [PATCH] arm64: dts: smaug: Add EMC frequency change tables

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3415 bytes --]

On Mon, Apr 10, 2023 at 10:17:56AM +0100, Diogo Ivo wrote:
> On Wed, Apr 05, 2023 at 03:18:23PM +0200, Thierry Reding wrote:
> > On Sun, Mar 19, 2023 at 07:42:55PM +0000, Diogo Ivo wrote:
> > > Add the reserved-memory regions of the nominal and derated tables setup by
> > > the bootloader so that the EMC frequency change code can access them.
> > > 
> > > Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
> > > ---
> > >  arch/arm64/boot/dts/nvidia/tegra210-smaug.dts | 21 +++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> > > index 709f3f417a19..a74826ae97b4 100644
> > > --- a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> > > +++ b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> > > @@ -30,6 +30,22 @@ memory {
> > >  		reg = <0x0 0x80000000 0x0 0xc0000000>;
> > >  	};
> > >  
> > > +	reserved-memory {
> > > +		#address-cells = <2>;
> > > +		#size-cells = <2>;
> > > +		ranges;
> > > +
> > > +		nominal: emc-table@f67cc000 {
> > > +			compatible = "nvidia,tegra210-emc-table";
> > > +			reg = <0x0 0xf67cc000 0x0 0xbea0>;
> > > +		};
> > > +
> > > +		derated: emc-table@f67d7ea0 {
> > > +			compatible = "nvidia,tegra210-emc-table";
> > > +			reg = <0x0 0xf67d7ea0 0x0 0xbea0>;
> > > +		};
> > > +	};
> > > +
> > 
> > These tables are typically generated by the firmware/bootloader at
> > runtime. Often they'll use heap allocations for these, so the addresses
> > are not guaranteed to be static.
> > 
> > Can you share a few details about what set of components you've used to
> > set this up? If we add these we want to be as specific as possible that
> > people use exactly the same set of firmware files.
> > 
> > Thierry
> 
> In the case of the Pixel C, coreboot takes care of copying these tables
> into a static cbmem region. The actual function taking care of this is
> tegra210_run_mtc(), found in [1], and from here it is possible to
> construct the cbmem memory layout. I have confirmed that the device is
> actually running this version of coreboot from the cbmem_console logs,
> where the commits are explicitly stated. These logs also indicate where
> the tables are located,
> 
> Wrote coreboot table at: 00000000f66ca000, 0x2c0 bytes, checksum 3601
> coreboot table: 728 bytes.
> CBMEM ROOT  0. f67ff000 00001000
> CONSOLE     1. f67ef000 00010000
> TIME STAMP  2. f67ee000 00001000
> VBOOT       3. f67ed000 00001000
> VPD         4. f67e4000 00009000
> MTC         5. f67cc000 00018000
> RAMOOPS     6. f66cc000 00100000
> COREBOOT    7. f66ca000 00002000
> 
> and
> 
> MTC: Copied 0x17d40 bytes from 0000000081008b70 to 00000000f67cc000

Okay, so this sounds good. My only worry is what could happen if
somebody were to run a slightly different version of coreboot that puts
these tables elsewhere. It's been a long time since I looked at
coreboot, but do you think perhaps it would be possible to update
coreboot to update the DTB at runtime to always point at the correct
location?

Alternatively, do we have an easy way of checking that the tables are
valid? Looking at the data structure that we're mapping, there's very
limited data there that could be used for validation, so garbage could
easily be mistaken for an actual table and mess things up.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] arm64: dts: smaug: Add EMC frequency change tables

[Diogo Ivo]

On Mon, Jul 10, 2023 at 05:32:15PM +0200, Thierry Reding wrote:
> On Mon, Apr 10, 2023 at 10:17:56AM +0100, Diogo Ivo wrote:
> > On Wed, Apr 05, 2023 at 03:18:23PM +0200, Thierry Reding wrote:
> > > On Sun, Mar 19, 2023 at 07:42:55PM +0000, Diogo Ivo wrote:
> > > > Add the reserved-memory regions of the nominal and derated tables setup by
> > > > the bootloader so that the EMC frequency change code can access them.
> > > > 
> > > > Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
> > > > ---
> > > >  arch/arm64/boot/dts/nvidia/tegra210-smaug.dts | 21 +++++++++++++++++++
> > > >  1 file changed, 21 insertions(+)
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> > > > index 709f3f417a19..a74826ae97b4 100644
> > > > --- a/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> > > > +++ b/arch/arm64/boot/dts/nvidia/tegra210-smaug.dts
> > > > @@ -30,6 +30,22 @@ memory {
> > > >  		reg = <0x0 0x80000000 0x0 0xc0000000>;
> > > >  	};
> > > >  
> > > > +	reserved-memory {
> > > > +		#address-cells = <2>;
> > > > +		#size-cells = <2>;
> > > > +		ranges;
> > > > +
> > > > +		nominal: emc-table@f67cc000 {
> > > > +			compatible = "nvidia,tegra210-emc-table";
> > > > +			reg = <0x0 0xf67cc000 0x0 0xbea0>;
> > > > +		};
> > > > +
> > > > +		derated: emc-table@f67d7ea0 {
> > > > +			compatible = "nvidia,tegra210-emc-table";
> > > > +			reg = <0x0 0xf67d7ea0 0x0 0xbea0>;
> > > > +		};
> > > > +	};
> > > > +
> > > 
> > > These tables are typically generated by the firmware/bootloader at
> > > runtime. Often they'll use heap allocations for these, so the addresses
> > > are not guaranteed to be static.
> > > 
> > > Can you share a few details about what set of components you've used to
> > > set this up? If we add these we want to be as specific as possible that
> > > people use exactly the same set of firmware files.
> > > 
> > > Thierry
> > 
> > In the case of the Pixel C, coreboot takes care of copying these tables
> > into a static cbmem region. The actual function taking care of this is
> > tegra210_run_mtc(), found in [1], and from here it is possible to
> > construct the cbmem memory layout. I have confirmed that the device is
> > actually running this version of coreboot from the cbmem_console logs,
> > where the commits are explicitly stated. These logs also indicate where
> > the tables are located,
> > 
> > Wrote coreboot table at: 00000000f66ca000, 0x2c0 bytes, checksum 3601
> > coreboot table: 728 bytes.
> > CBMEM ROOT  0. f67ff000 00001000
> > CONSOLE     1. f67ef000 00010000
> > TIME STAMP  2. f67ee000 00001000
> > VBOOT       3. f67ed000 00001000
> > VPD         4. f67e4000 00009000
> > MTC         5. f67cc000 00018000
> > RAMOOPS     6. f66cc000 00100000
> > COREBOOT    7. f66ca000 00002000
> > 
> > and
> > 
> > MTC: Copied 0x17d40 bytes from 0000000081008b70 to 00000000f67cc000
> 
> Okay, so this sounds good. My only worry is what could happen if
> somebody were to run a slightly different version of coreboot that puts
> these tables elsewhere. It's been a long time since I looked at
> coreboot, but do you think perhaps it would be possible to update
> coreboot to update the DTB at runtime to always point at the correct
> location?

As far as I know it is only possible to replace coreboot on the device 
after disconnecting a hard to access ribbon cable that sits behind the
screen, which combined with then every user having to do it makes this 
option suboptimal from my point of view.

> Alternatively, do we have an easy way of checking that the tables are
> valid? Looking at the data structure that we're mapping, there's very
> limited data there that could be used for validation, so garbage could
> easily be mistaken for an actual table and mess things up.
>
> Thierry

The kernel already does some validation on these tables by checking that
the revision field is the same across the table as well as veryfing the
voltages and rates are monotonically increasing. The revision check can
be made stronger by checking for a particular revision and not just the
same across the table. This is actually something I did as part of adding
downstream upstream since the Pixel C has revision 6 tables and the kernel
only supports revision 7 currently which in essence means that as of now
the tables in this patch will be ignored by the emc scaling driver.

Diogo

Re: [PATCH] arm64: dts: smaug: Add EMC frequency change tables

[Diogo Ivo]

On Sun, Mar 19, 2023 at 07:42:55PM +0000, Diogo Ivo wrote:
> Add the reserved-memory regions of the nominal and derated tables setup by
> the bootloader so that the EMC frequency change code can access them.
> 

Hello,

Gentle ping on this patch.

Thank you,

Diogo Ivo

Re: [PATCH] arm64: dts: smaug: Add EMC frequency change tables

[Diogo Ivo]

On Sun, Mar 19, 2023 at 07:42:55PM +0000, Diogo Ivo wrote:
> Add the reserved-memory regions of the nominal and derated tables setup by
> the bootloader so that the EMC frequency change code can access them.
> 
> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>

Hello,

Gentle ping on this patch.

Thank you,
Diogo Ivo