Hi Pali, On Tue, Jan 17, 2023 at 1:25 PM Pali Rohár <p...@kernel.org> wrote: > > Hello! > > On Tuesday 17 January 2023 13:02:46 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. > > My reaction was just because you forgot to update undef for a39x.
I see. Yes, I did miss that! I only described the changes in v2 description. > > > > > > 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? > > I'm not sure if we are talking about the same thing. When I read this > your patch I saw that every time, even the first one "This sync drivers/..." > has 4 spaces before word "This". And I'm not sure if this is just my > email client or not and there is some reason for it. Look at indentation > of line "Signed-off-by:" and line "Reference:". Should not be those two > lines at same indentation level? Or I did not understand it? :D > > I agree that adding indentation inside of 1. 2. 3. parts is fully > recommended as it makes text more readable. When I do git log I also see an extra 4 or 8 spaces on each line:) so not sure what we are seeing here. But yes it seems some of the indentation is inconsistent. Will fix that. > > > > > > > > 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. > > It should work if at least one of the option in OR condition is > specified with -D on command line. But if you are unsure then it is > better to test it (should be quite easy and fast). > > > > > > > > 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 do not know. I'm always trying to put different thing into different > commits. Reason is that if in some case it would be needed to revert > commit then unrelated cleanup does not need to be reverted :-) There is only one existing file removed (dram_if.h). The other are new files that become dead code after we run the filter script. Do you think those should also be kept in this patch, and then we'll have a cleanup patch later? The alternative is I can just list the new-but-deleted files in the commit description so they can be kept tracked of (easier for the future code sync if we know what should be candidates for removal). > > > > > > > > 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? > > Hmm... You found another bug in Marvell code: > > $ git grep mv_ddr4_calibration_validate > a38x/mv_ddr_plat.c:MV_STATUS mv_ddr4_calibration_validate(MV_U32 dev_num) > apn806/mv_ddr_plat.c:int mv_ddr4_calibration_validate(u32 dev_num) > mv_ddr4_training.c: return mv_ddr4_calibration_validate(dev_num); > mv_ddr4_training.h:int mv_ddr4_calibration_validate(u32 dev_num); > > That function mv_ddr4_calibration_validate() should return int type (as > defined in header file) and not MV_STATUS type. So rather fix return > type of the function to match what is in header file. Also there is > mismatch with its argument u32 vs MV_u32! > > Next, MV_U8 is used only at one place (a37xx defines it moreover locally): > > $ git grep MV_U8 > a3700/mv_ddr_a3700_wrapper.h:#define MV_U8 u8 > a38x/mv_ddr_plat.c: MV_U8 if_id = 0; > mv_ddr_atf_wrapper.h:#define MV_U8 u8 > > So replace MV_U8 directly by u8. And same for MV_U32 for a38x code. Cool. > And ideally, send a pull request to Marvell repo with these fixes (and > also with floating point), so code can be synced easily also again in > future. Would you do that when you have time after DDR4 gets merged? Thanks, Tony