[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