Re: [PATCH v2 00/19] EDAC/mc/synopsys: Various fixes and cleanups

[Alexander Stein]

Hi,

Am Samstag, 10. September 2022, 21:42:18 CEST schrieb Serge Semin:
> This patchset is a first one in the series created in the framework of
> my Baikal-T1 DDRC-related work:
> 
> [1: In-progress] EDAC/mc/synopsys: Various fixes and cleanups
> Link: ---you are looking at it---
> [2: In-progress] EDAC/synopsys: Add generic DDRC info and address mapping
> Link:
> https://lore.kernel.org/linux-edac/20220822191427.27969-1-Sergey.Semin@baik
> alelectronics.ru [3: In-progress] EDAC/synopsys: Add generic resources and
> Baikal-T1 support Link:
> https://lore.kernel.org/linux-edac/20220822191957.28546-1-Sergey.Semin@baik
> alelectronics.ru
> 
> Note the patchsets above must be merged in the same order as they are
> placed in the list in order to prevent conflicts. Nothing prevents them
> from being reviewed synchronously though. Any tests are very welcome.
> Thanks in advance.

What is the state of this/these series? AFAICS only the DT patches got 
applied.
The synopsys driver got refactored quite a lot, so adding proper support for 
imx8mp from current state will conflict quite a lot.
It's a Synopsys V3.70a (without HW poisoning support!), refer to commit 
68b7cf5d91d4c ("arm64: dts: imx8mp: add ddr controller node to support EDAC on 
imx8mp").

Best regards,
Alexander

> Regarding this series content. It's an initial patchset which
> traditionally provides various fixes, cleanups and modifications required
> for the more comfortable further features development. The main goal of it
> though is to detach the Xilinx Zynq A05 DDRC related code into the
> dedicated driver since first it has nothing to do with the Synopsys DW
> uMCTL2 DDR controller and second it will be a great deal obstacle on the
> way of extending the Synopsys-part functionality.
> 
> The series starts with fixes patches, which in short concern the next
> aspects: touching the ZynqMP-specific CSRs on the Xilinx ZinqMP platform
> only, serializing an access to the ECCCLR register, adding correct memory
> devices type detection, setting a correct value to the
> mem_ctl_info.scrub_cap field, dropping an erroneous ADDRMAP[4] parsing and
> getting back a correct order of the ECC errors info detection procedure.
> 
> Afterwards the patchset provides several cleanup patches required for the
> more coherent code splitting up (Xilinx Zynq A05 and Synopsys DW uMCTL2)
> so the provided modifications would be useful in both drivers. First we
> get to replace the platform resource manual IO-remapping with the
> devm_platform_ioremap_resource() method call. Secondly we suggest to drop:
> internal CE/UE errors counters, local to_mci() macros definition, some
> redundant ecc_error_info structure fields and redundant info from the
> error message, duplicated dimm->nr_pages debug printout and spaces from
> the MEM_TYPE flags declarations. (The later two updates concern the MCI
> core part.) Thirdly before splitting up the driver we need to add an
> unique MC index allocation infrastructure to the MCI core.  It's required
> since after splitting the driver up we'll need to make sure both device
> types could be correctly probed on the same platform. Finally the Xilinx
> Zynq A05 part of the driver is moved out to a dedicated driver where it
> should been originally placed. After that the platform-specific setups API
> is removed from the Synopsys DW uMCTL2 DDRC driver since it's no longer
> required.
> 
> Finally as the cherry on the cake we suggest to unify the DW uMCTL2 DDRC
> driver entities naming and replace the open-coded "shift/mask" patter with
> the kernel helpers like BIT/GENMASK/FIELD_x in there. It shall
> significantly improve the code readability.
> 
> Link:
> https://lore.kernel.org/linux-edac/20220822190730.27277-1-Sergey.Semin@baik
> alelectronics.ru/ Changelog 2:
> - Move Synopsys DW uMCTL2 DDRC bindings file renaming to a separate patch.
>   (@Krzysztof)
> - Introduce a new compatible string "snps,dw-umctl2-ddrc" matching the new
>   DT-schema name.
> - Forgot to fix some of the prefix of the SYNPS_ZYNQMP_IRQ_REGS macro
>   in several places. (@tbot)
> - Drop the no longer used "priv" pointer from the mc_init() function.
>   (@tbot)
> - Include "linux/bitfield.h" header file to get the FIELD_GET macro
>   definition. (@tbot)
> - Drop the already merged in patches:
> [PATCH 12/20] EDAC/mc: Replace spaces with tabs in memtype flags definition
> [PATCH 13/20] EDAC/mc: Drop duplicated dimm->nr_pages debug printout
> 
> Signed-off-by: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> Cc: Alexey Malahov <Alexey.Malahov@xxxxxxxxxxxxxxxxxxxx>
> Cc: Michail Ivanov <Michail.Ivanov@xxxxxxxxxxxxxxxxxxxx>
> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@xxxxxxxxxxxxxxxxxxxx>
> Cc: Punnaiah Choudary Kalluri <punnaiah.choudary.kalluri@xxxxxxxxxx>
> Cc: Manish Narani <manish.narani@xxxxxxxxxx>
> Cc: Dinh Nguyen <dinguyen@xxxxxxxxxx>
> Cc: James Morse <james.morse@xxxxxxx>
> Cc: Robert Richter <rric@xxxxxxxxxx>
> Cc: Rob Herring <robh@xxxxxxxxxx>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Cc: devicetree@xxxxxxxxxxxxxxx
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: linux-edac@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> 
> Serge Semin (19):
>   EDAC/synopsys: Fix native uMCTL2 IRQs handling procedure
>   EDAC/synopsys: Fix generic device type detection procedure
>   EDAC/synopsys: Fix mci->scrub_cap field setting
>   EDAC/synopsys: Drop erroneous ADDRMAP4.addrmap_col_b10 parse
>   EDAC/synopsys: Fix reading errors count before ECC status
>   EDAC/synopsys: Use platform device devm ioremap method
>   EDAC/synopsys: Drop internal CE and UE counters
>   EDAC/synopsys: Drop local to_mci macro implementation
>   EDAC/synopsys: Drop struct ecc_error_info.blknr field
>   EDAC/synopsys: Shorten out struct ecc_error_info.bankgrpnr field name
>   EDAC/synopsys: Drop redundant info from error message
>   EDAC/mc: Init DIMM labels in MC registration method
>   EDAC/mc: Add MC unique index allocation procedure
>   dt-bindings: memory: snps: Detach Zynq DDRC controller support
>   dt-bindings: memory: snps: Use more descriptive device name
>   EDAC/synopsys: Detach Zynq DDRC controller support
>   EDAC/synopsys: Drop unused platform-specific setup API
>   EDAC/synopsys: Unify the driver entities naming
>   EDAC/synopsys: Convert to using BIT/GENMASK/FIELD_x macros
> 
>  .../snps,dw-umctl2-ddrc.yaml                  |  56 ++
>  .../memory-controllers/synopsys,ddrc-ecc.yaml |  76 --
>  .../xlnx,zynq-ddrc-a05.yaml                   |  38 +
>  MAINTAINERS                                   |   3 +
>  drivers/edac/Kconfig                          |   9 +-
>  drivers/edac/Makefile                         |   1 +
>  drivers/edac/edac_mc.c                        | 135 ++-
>  drivers/edac/edac_mc.h                        |   4 +
>  drivers/edac/synopsys_edac.c                  | 903 ++++++------------
>  drivers/edac/zynq_edac.c                      | 504 ++++++++++
>  10 files changed, 1026 insertions(+), 703 deletions(-)
>  create mode 100644
> Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.ya
> ml delete mode 100644
> Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml
> create mode 100644
> Documentation/devicetree/bindings/memory-controllers/xlnx,zynq-ddrc-a05.yam
> l create mode 100644 drivers/edac/zynq_edac.c


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/