Re: [PATCH v5 00/20] EDAC/mc/synopsys: Various fixes and cleanups

[Shubhrajyoti Datta]

On Thu, Feb 22, 2024 at 11:52 PM Serge Semin <fancer.lancer@xxxxxxxxx> wrote:
>
> This patchset is a first one in the series created in the framework of
> my Synopsys DW uMCTL2 DDRC-related work:
>
> [1: In-progress v5] EDAC/mc/synopsys: Various fixes and cleanups
> Link: ---you are looking at it---
> [2: In-progress v4] EDAC/synopsys: Add generic DDRC info and address mapping
> Link: https://lore.kernel.org/linux-edac/20230920192806.29960-1-fancer.lancer@xxxxxxxxx
> [3: In-progress v4] EDAC/synopsys: Add generic resources and Scrub support
> Link: https://lore.kernel.org/linux-edac/20230920195720.32047-1-fancer.lancer@xxxxxxxxx
>
> 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.
>
> The main goal of the entire set of the changes provided in the mentioned
> patchsets is to as much as possible specialise the synopsys_edac.c driver
> to be working with the Synopsys DW uMCTL2 DDR controllers of various
> versions and synthesized parameters, and add useful error-detection
> features.
>
> 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 the 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/ECCCTL 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
> DDRCs) so the provided modifications would be useful in both drivers.
> First the platform resource open-coded IO-space remapping is replaced with
> the devm_platform_ioremap_resource() method call for the sake of the code
> simplification. Secondly the next redundant entities are dropped: 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 detaching the Zynq A05-related code an unique MC index
> allocation infrastructure is added to the MCI core. It's required since
> after splitting the driver up both supported types of memory devices could
> be correctly probed on the same platform. Note even though it's currently
> unsupported by the synsopsys_edac.c driver it's claimed to be possible by
> the original driver author (it was a reason of having two unrelated
> devices supported in a single driver). Finally the Xilinx Zynq A05 part of
> the driver is moved out to a dedicated driver. 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 a set of the local coding style
> cleanups are provided: unify the DW uMCTL2 DDRC driver entities naming and
> replace the open-coded "shift/mask" pattern with the kernel helpers like
> BIT/GENMASK/FIELD_x in there. It shall significantly improve the code
> readability.
>

For the zynqmp

Reviewed-by: Shubhrajyoti Datta <shubhrajyoti.datta@xxxxxxx>

Thanks,
> Changelog v2:
> - 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
>
> Changelog v3:
> - Drop the no longer used "priv" pointer from the mc_init() function.
>   (@tbot)
> - Drop the merged in patches:
> [PATCH v2 14/19] dt-bindings: memory: snps: Detach Zynq DDRC controller support
> [PATCH v2 15/19] dt-bindings: memory: snps: Use more descriptive device name
>   (@Krzysztof)
>
> Changelog v4:
> - Remove Rob, Krzysztof and DT-mailing list from Cc since the respective
>   patches have already been merged in.
> - Add a new patch
>   [PATCH v4 6/20] EDAC/synopsys: Fix misleading IRQ self-cleared quirk flag
>   detached from the very first patch of the series.
> - Add a new patch
>   [PATCH v4 15/20] EDAC/mc: Re-use generic unique MC index allocation procedure
> - Add a new patch
>   [PATCH v4 18/20] EDAC/synopsys: Unify CSRs macro declarations
>   collecting the changes from various patches of the series.
> - Drop redundant empty lines left by mistake.
> - Drop private counters access from the check_errors() method too.
> - Rebase onto the kernel v6.6-rcX.
>
> Link: https://lore.kernel.org/linux-edac/20230920191059.28395-1-fancer.lancer@xxxxxxxxx
> Changelog v5:
> - Fix function names in the zynq_edac.c kdoc.
> - Rebase onto the kernel 6.8-rc3.
>
> Signed-off-by: Serge Semin <fancer.lancer@xxxxxxxxx>
> Cc: Punnaiah Choudary Kalluri <punnaiah.choudary.kalluri@xxxxxxxxxx>
> Cc: Dinh Nguyen <dinguyen@xxxxxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Cc: linux-edac@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
>
> Serge Semin (20):
>   EDAC/synopsys: Fix ECC status data and IRQ disable race condition
>   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: Fix misleading IRQ self-cleared quirk flag
>   EDAC/synopsys: Use platform device devm ioremap method
>   EDAC/synopsys: Drop internal CE and UE counters
>   EDAC/synopsys: Drop local to_mci() macro definition
>   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 the error messages
>   EDAC/mc: Init DIMM labels in MC registration method
>   EDAC/mc: Add generic unique MC index allocation procedure
>   EDAC/mc: Re-use generic unique MC index allocation procedure
>   EDAC/synopsys: Detach Zynq A05 DDRC support to separate driver
>   EDAC/synopsys: Drop unused platform-specific setup API
>   EDAC/synopsys: Unify CSRs macro declarations
>   EDAC/synopsys: Unify struct/macro/function prefixes
>   EDAC/synopsys: Convert to using BIT/GENMASK/FIELD_x macros
>
>  MAINTAINERS                  |   1 +
>  drivers/edac/Kconfig         |   9 +-
>  drivers/edac/Makefile        |   1 +
>  drivers/edac/dmc520_edac.c   |   4 +-
>  drivers/edac/edac_mc.c       | 135 ++++-
>  drivers/edac/edac_mc.h       |   4 +
>  drivers/edac/pasemi_edac.c   |   5 +-
>  drivers/edac/ppc4xx_edac.c   |   5 +-
>  drivers/edac/synopsys_edac.c | 967 ++++++++++++-----------------------
>  drivers/edac/zynq_edac.c     | 501 ++++++++++++++++++
>  10 files changed, 963 insertions(+), 669 deletions(-)
>  create mode 100644 drivers/edac/zynq_edac.c
>
> --
> 2.43.0
>
>