search

MarvellEmbeddedProcessors / mv-ddr-marvell

20 stars 23 forks source link

changes from Globalscaletechnologies for EspressoBin 1GB DDR4 1cs #25

Closed heaterC closed 3 years ago

heaterC commented 3 years ago

These changes were introduced in https://github.com/globalscaletechnologies/A3700-utils-marvell/blob/A3700_utils-armada-18.12.0/tim/ddr/espressobin-ddr4-1cs-1g.txt. I also included a fix suggested by @pali:

In commit 6285efb8a118 ("mv_ddr: add support for twin-die combined memory device") was added support for twin-die combined memory device and default value for explicitly uninitialized structure members is zero, so also twin_die_combined is initialized to zero. Which means COMBINED value.

As prior this commit there was no support for twin-die combined memory device, default value for twin_die_combined should be NOT_COMBINED. This change change order of enum mv_ddr_twin_die to ensure that NOT_COMBINED has value zero.

heaterC commented 3 years ago

I included the changes for DDR4 2cs also in this branch. Hope that is ok. They are reflecting file https://github.com/globalscaletechnologies/A3700-utils-marvell/blob/A3700_utils-armada-18.12.0/tim/ddr/espressobin-ddr4-2cs-2g.txt .

pali commented 3 years ago

And here are links to @globalscaletechnologies commits, source of this pull request: https://github.com/globalscaletechnologies/A3700-utils-marvell/commit/6071466e77f42fdda924c48bfe12cac8ff1d457d https://github.com/globalscaletechnologies/A3700-utils-marvell/commit/1421e17bd0261388522bf00eb72496c14f68e29c

In commit message for those changes is written:

the ddr configuration which generated by ddr tool may not
meet the ddr timing or not an optimal value
pali commented 3 years ago

@heaterC: I have compared generated file A3700-utils-marvell/tim/ddr/ddr_static.txt by this pull request and https://github.com/globalscaletechnologies/A3700-utils-marvell/blob/A3700_utils-armada-18.12.0/tim/ddr/espressobin-ddr4-1cs-1g.txt files. And they differs.

It looks like that you have included incorrect change in ddr4_1cs_tim_pre array. Can you look at it and fix it?

heaterC commented 3 years ago

So much for manual changes. Sorry, really hope it's correct now and many thanks for checking!

pali commented 3 years ago

Some parts you fixed, but there are still differences between espressobin-ddr4-1cs-1g.txt from globalscaletechnologies and your changes in this PR which generate file ddr_static.txt.

Seems that in your change in ddr4_1cs_tim_pre array you added new WRITE command but you forgot to remove old WRITE commands.

Here are differences:

--- espressobin-ddr4-1cs-1g.txt 2020-11-25 13:37:33.426788114 +0100
+++ ddr_static.txt  2020-11-25 20:22:56.145459471 +0100
@@ -37,6 +37,25 @@ WRITE: 0xC00003B8 0x00000605
 WRITE: 0xC0000200 0x000E0001
 WRITE: 0xC0000204 0x00000000
 WRITE: 0xC0000220 0x05020635
+WRITE: 0xC0000300 0x00000B0C
+WRITE: 0xC0000380 0x00061A80
+WRITE: 0xC0000384 0x00027100
+WRITE: 0xC0000388 0x00000050
+WRITE: 0xC000038C 0x00000400
+WRITE: 0xC0000390 0x00800200
+WRITE: 0xC0000394 0x011803CF
+WRITE: 0xC0000398 0x01200255
+WRITE: 0xC000039C 0x00000808
+WRITE: 0xC00003A0 0x04050500
+WRITE: 0xC00003A4 0x00000002
+WRITE: 0xC00003A8 0x00001808
+WRITE: 0xC00003AC 0x1C250C1A
+WRITE: 0xC00003B0 0x0C0C060C
+WRITE: 0xC00003B4 0x05040602
+WRITE: 0xC00003B8 0x00000605
+WRITE: 0xC0000200 0x000E0001
+WRITE: 0xC0000204 0x00000000
+WRITE: 0xC0000220 0x05020635
 WRITE: 0xC00003C0 0x00020205
 WRITE: 0xC00003C4 0x00000003
 WRITE: 0xC00003DC 0x00081239
heaterC commented 3 years ago

I did actually take the file espressobin-ddr4-1cs-1g.txtand added quotation signs and nextline char. Then I split it and copied the two blocks into mv_ddr_tim.h. If there is anything else, then this must be inserted from some other file I am afraid. @pali I am really grateful for your help, because I don't really know at all what's happening and what's executed in this code, I am just doing some word processing and recombination of files here (and learn how to handle git in the meantime ;-) ).

pali commented 3 years ago

Yes, this is just word processing, recompilation and looking at differences. (Probably I have access to documentation about meaning of those WRITE commands, but it would not help me so much)

I'm looking at your changes and generated diff between ddr_static.txt and espressobin-ddr4-1cs-1g.txt files. It looks like that your changes which you have added into ddr4_1cs_tim_pre array should not be there.

Could you try to remove changes which you have done into ddr4_1cs_tim_pre array?

heaterC commented 3 years ago

Could it be possible that you compare to a wrong version of espressobin-ddr4-1cs-1g.txt? The "+" lines of the diff you post above are all present in https://github.com/globalscaletechnologies/A3700-utils-marvell/blob/A3700_utils-armada-18.12.0/tim/ddr/espressobin-ddr4-1cs-1g.txt , or am I wrong?

pali commented 3 years ago

It looks like that some WRITE commands are generate also by other files (e.g. drivers/mv_ddr_mc6.c).

And seems that those commands you included manually also into ddr4_1cs_tim_pre array. So could you try to remove changes which you have done into ddr4_1cs_tim_pre array? I think it should help.

heaterC commented 3 years ago

Removed the lines, hope it helps.

pali commented 3 years ago

@heaterC: Perfect! Now I tested DDR_TOPOLOGY=5 (DDR4 1CS) and generated ddr_static.txt file is same as espressobin-ddr4-1cs-1g.txt.

Later I will look at DDR4 2CS

heaterC commented 3 years ago

DDR4 2CS will have the same problem. If you could post the diff I will correct it the same way.

pali commented 3 years ago

Here is diff. Seems that it has really same issue. Try to remove your changes from ddr4_2cs_tim_pre array.

--- espressobin-ddr4-2cs-2g.txt 2020-11-25 13:47:32.970113140 +0100
+++ ddr_static.txt  2020-11-26 11:21:22.435504106 +0100
@@ -41,6 +41,28 @@ WRITE: 0xC0000220 0x05020635
 WRITE: 0xC0000208 0x400E0001
 WRITE: 0xC000020C 0x00000000
 WRITE: 0xC0000224 0x05020635
+WRITE: 0xC0000300 0x00000B0C
+WRITE: 0xC0000380 0x00061A80
+WRITE: 0xC0000384 0x00027100
+WRITE: 0xC0000388 0x00000050
+WRITE: 0xC000038C 0x00000400
+WRITE: 0xC0000390 0x00800200
+WRITE: 0xC0000394 0x011803CF
+WRITE: 0xC0000398 0x01200255
+WRITE: 0xC000039C 0x00000808
+WRITE: 0xC00003A0 0x04050500
+WRITE: 0xC00003A4 0x00000002
+WRITE: 0xC00003A8 0x00001808
+WRITE: 0xC00003AC 0x1C250C1A
+WRITE: 0xC00003B0 0x0C0C060C
+WRITE: 0xC00003B4 0x05040602
+WRITE: 0xC00003B8 0x00000605
+WRITE: 0xC0000200 0x000E0001
+WRITE: 0xC0000204 0x00000000
+WRITE: 0xC0000220 0x05020635
+WRITE: 0xC0000208 0x400E0001
+WRITE: 0xC000020C 0x00000000
+WRITE: 0xC0000224 0x05020635
 WRITE: 0xC00003C0 0x00020205
 WRITE: 0xC00003C4 0x00000003
 WRITE: 0xC00003DC 0x00081239
heaterC commented 3 years ago

done

pali commented 3 years ago

Well done, now files are same! So I think this pull request is now ready for merge.

pali commented 3 years ago

@kostapr: Can you review & merge this pull request?

heaterC commented 3 years ago

Thanks a lot, especially @pali !

pali commented 3 years ago

@kostapr: Thanks! Could you look also at pull request https://github.com/MarvellEmbeddedProcessors/mv-ddr-marvell/pull/22 and merge it into correct branch mv-ddr-devel (as it was merged into older branch)?

kostapr commented 3 years ago

@pali I cherry-picked two patches from #22 to marvell-ddr-devel, please check.

pali commented 3 years ago

Ok! And can you look if it is possible to change github's default branch of this repository to mv-ddr-devel?

kostapr commented 3 years ago

@pali I asked for it :-)

pali commented 3 years ago

It is changed, @kostapr thanks!