[PATCH v2] ddr: marvell: a38x: Add support for DDR4 from Marvell mv-ddr-marvell repository
Stefan Roese
sr at denx.de
Wed Jan 18 12:01:15 CET 2023
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 <pali at 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 <mibodhi at 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: sr at denx.de
More information about the U-Boot
mailing list