Hi Tony,
On 1/17/23 22:02, Tony Dinh wrote:
Hi Pali,
On Tue, Jan 17, 2023 at 12:35 AM Pali Rohár <p...@kernel.org> wrote:
Hello! Thank you for update. It is much better.
On Monday 16 January 2023 21:34:39 Tony Dinh wrote:
This syncs drivers/ddr/marvell/a38x/ with the master branch of repository
https://github.com/MarvellEmbeddedProcessors/mv-ddr-marvell.git
up to the commit "mv_ddr: a3700: Use the right size for memset to not
overflow"
d5acc10c287e40cc2feeb28710b92e45c93c702c
This patch was created by following steps:
1. Replace all a38x files in U-Boot tree by files from upstream github
Marvell mv-ddr-marvell repository.
2. Run following command to omit portions not relevant for a38x, ddr3, and
ddr4:
files=drivers/ddr/marvell/a38x/*
sed 's/#if defined(CONFIG_ARMADA_38X) ||
defined(CONFIG_ARMADA_39X)/#ifdef TRUE/' -i $files
unifdef -m -UMV_DDR -UMV_DDR_ATF -UCONFIG_APN806 \
-UCONFIG_MC_STATIC -UCONFIG_MC_STATIC_PRINT -UCONFIG_PHY_STATIC \
-UCONFIG_PHY_STATIC_PRINT -UCONFIG_CUSTOMER_BOARD_SUPPORT \
-UCONFIG_A3700 -UA3900 -UA80X0 -UA70X0 -DTRUE $files
Do not forget to also update commit message.
Yes, patman extracts and creates the patch description from the commit.
And btw, commit messages has on each line some leading spaces which is
not probably intended.
That was intentional to make the commit description (and patch
description) more readable. Is it not recommended?
Readability is just fine IMHO.
3. Manually change license to SPDX-License-Identifier
(upstream license in upstream github repository contains long license
texts and U-Boot is using just SPDX-License-Identifier.
After applying this patch, a38x ddr3 ddr4 code in upstream Marvell github
repository and in U-Boot would be fully identical. So in future applying
above steps could be used to sync code again.
The only change in this patch are:
- Removal of common board_topology_map code using ifdefs in mv_ddr_brd.c
- Some fixes with include files.
- Some basic type defines (original from ATF headers) in mv_ddr_plat.c
Reference:
"ddr: marvell: a38x: Sync code with Marvell mv-ddr-marvell repository"
https://source.denx.de/u-boot/u-boot/-/commit/107c3391b95bcc2ba09a876da4fa0c31b6c1e460
Signed-off-by: Tony Dinh <mibo...@gmail.com>
---
Changes in v2:
- Modified the filter scrip to explicitly include ARMADA_38X code
and exclude ARMADA_39X code; also remove 64BIT code. Reran it on
drivers/ddr/marvell/a38x/
- Updated script
files=drivers/ddr/marvell/a38x/*
sed 's/#if defined(CONFIG_ARMADA_38X) || defined(CONFIG_ARMADA_39X)/#ifdef
TRUE/' -i $files
You do not need this sed anymore. CONFIG_ARMADA_39X is explicitly
removed and CONFIG_ARMADA_38X already handled by unifdef.
Thanks, I was not sure if unifdef works in that "OR" condition. I will
update the commit message.
unifdef -m -UMV_DDR -UMV_DDR_ATF -UCONFIG_APN806 \
-UCONFIG_MC_STATIC -UCONFIG_MC_STATIC_PRINT
-UCONFIG_PHY_STATIC \
-UCONFIG_PHY_STATIC_PRINT -UCONFIG_CUSTOMER_BOARD_SUPPORT \
-UCONFIG_A3700 -UA3900 -UA80X0 -UA70X0 -DCONFIG_ARMADA_38X
-UCONFIG_ARMADA_39X \
-UCONFIG_64BIT $files
- Remove more dead code files
- Correct SPDX license header
drivers/ddr/marvell/a38x/Makefile | 8 +
drivers/ddr/marvell/a38x/ddr3_debug.c | 120 +
drivers/ddr/marvell/a38x/ddr3_init.c | 25 +
drivers/ddr/marvell/a38x/ddr3_init.h | 14 +
drivers/ddr/marvell/a38x/ddr3_logging_def.h | 27 +
drivers/ddr/marvell/a38x/ddr3_training.c | 131 +
drivers/ddr/marvell/a38x/ddr3_training_bist.c | 12 +
.../a38x/ddr3_training_centralization.c | 4 +
drivers/ddr/marvell/a38x/ddr3_training_db.c | 212 ++
drivers/ddr/marvell/a38x/ddr3_training_ip.h | 17 +
.../ddr/marvell/a38x/ddr3_training_ip_db.h | 61 +
.../marvell/a38x/ddr3_training_ip_engine.c | 145 +
.../ddr/marvell/a38x/ddr3_training_ip_flow.h | 5 +
.../ddr/marvell/a38x/ddr3_training_leveling.c | 135 +
drivers/ddr/marvell/a38x/dram_if.h | 13 -
drivers/ddr/marvell/a38x/mv_ddr4_mpr_pda_if.c | 674 +++++
drivers/ddr/marvell/a38x/mv_ddr4_mpr_pda_if.h | 59 +
drivers/ddr/marvell/a38x/mv_ddr4_training.c | 565 ++++
drivers/ddr/marvell/a38x/mv_ddr4_training.h | 32 +
.../a38x/mv_ddr4_training_calibration.c | 2336 +++++++++++++++++
.../a38x/mv_ddr4_training_calibration.h | 26 +
.../ddr/marvell/a38x/mv_ddr4_training_db.c | 545 ++++
.../marvell/a38x/mv_ddr4_training_leveling.c | 441 ++++
.../marvell/a38x/mv_ddr4_training_leveling.h | 11 +
drivers/ddr/marvell/a38x/mv_ddr_plat.c | 249 ++
drivers/ddr/marvell/a38x/mv_ddr_plat.h | 11 +
drivers/ddr/marvell/a38x/mv_ddr_regs.h | 59 +
drivers/ddr/marvell/a38x/mv_ddr_topology.h | 72 +
28 files changed, 5996 insertions(+), 13 deletions(-)
delete mode 100644 drivers/ddr/marvell/a38x/dram_if.h
I see that you are removing some existing file. If it is not needed
neither for DDR3 nor for DDR4 then please remove it in separate commit
or patch. So we do not mix different things into one commit.
Instead of making a different commit, will it work if we list the
files being removed in this commit message? It is part of removing
dead code.
I agree with Pali on this. Please keep logically unrelated changes in
separate commits if possible.
Thanks,
Stefan
create mode 100644 drivers/ddr/marvell/a38x/mv_ddr4_mpr_pda_if.c
create mode 100644 drivers/ddr/marvell/a38x/mv_ddr4_mpr_pda_if.h
create mode 100644 drivers/ddr/marvell/a38x/mv_ddr4_training.c
create mode 100644 drivers/ddr/marvell/a38x/mv_ddr4_training.h
create mode 100644 drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.c
create mode 100644 drivers/ddr/marvell/a38x/mv_ddr4_training_calibration.h
create mode 100644 drivers/ddr/marvell/a38x/mv_ddr4_training_db.c
create mode 100644 drivers/ddr/marvell/a38x/mv_ddr4_training_leveling.c
create mode 100644 drivers/ddr/marvell/a38x/mv_ddr4_training_leveling.h
...
diff --git a/drivers/ddr/marvell/a38x/mv_ddr_plat.c
b/drivers/ddr/marvell/a38x/mv_ddr_plat.c
index 7c7bce73a3..16d177b42f 100644
--- a/drivers/ddr/marvell/a38x/mv_ddr_plat.c
+++ b/drivers/ddr/marvell/a38x/mv_ddr_plat.c
@@ -12,6 +12,11 @@
#define DDR_INTERFACES_NUM 1
#define DDR_INTERFACE_OCTETS_NUM 5
+/* These were defined in ATF area that was stripped out */
+#define MV_STATUS int
+#define MV_U32 u32
+#define MV_U8 u8
+
This is something new which you added? Because I do not see it in
Marvell code.
Yes, those were in the original code after the initial copying from
the Marvell repo.
# grep -E '(MV_U32|MV_STATUS|MV_U8)' *.[ch] a38x/*.[ch]
ddr_init.c:MV_U32 ddr_init(void)
mv_ddr_atf_wrapper.h:#define MV_STATUS int
mv_ddr_atf_wrapper.h:#define MV_U8 u8
mv_ddr_atf_wrapper.h:#define MV_U32 u32
a38x/mv_ddr_plat.c:MV_STATUS mv_ddr4_calibration_validate(MV_U32 dev_num)
a38x/mv_ddr_plat.c: MV_STATUS status = MV_OK;
a38x/mv_ddr_plat.c: MV_U8 if_id = 0;
a38x/mv_ddr_plat.c: MV_U32 read_data[MAX_INTERFACE_NUM];
a38x/mv_ddr_plat.c: MV_U32 cal_n = 0, cal_p = 0;
Those 3 are defined in mv_ddr_atf_wrapper.h and used in mv_ddr_plat.c
(after we ran the filter script, this file is the only place that
needs those 3 defines). Since we removed the ATF code, we need to
define them here in mv_ddr_plat.c. Is this OK or do you have any
suggestions for a better approach?
Thanks,
Tony
Viele Grüße,
Stefan Roese
--
DENX Software Engineering GmbH, Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: s...@denx.de
