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

Message

Serge Semin Feb. 22, 2024, 6:12 p.m. UTC

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/[e-mail]
[3: In-progress v4] EDAC/synopsys: Add generic resources and Scrub support
Link: https://lore.kernel.org/linux-edac/[e-mail]

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.

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/[e-mail]
Changelog v5:
- Fix function names in the zynq_edac.c kdoc.
- Rebase onto the kernel 6.8-rc3.

Signed-off-by: Serge Semin <[e-mail]>
Cc: Punnaiah Choudary Kalluri <[e-mail]>
Cc: Dinh Nguyen <[e-mail]>
Cc: Arnd Bergmann <[e-mail]>
Cc: Greg Kroah-Hartman <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]

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

Comments

Shubhrajyoti Datta March 6, 2024, 5:27 a.m. UTC | #1

On Thu, Feb 22, 2024 at 11:52 PM Serge Semin <[e-mail]> 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/[e-mail]
> [3: In-progress v4] EDAC/synopsys: Add generic resources and Scrub support
> Link: https://lore.kernel.org/linux-edac/[e-mail]
>
> 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 <[e-mail]>

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/[e-mail]
> Changelog v5:
> - Fix function names in the zynq_edac.c kdoc.
> - Rebase onto the kernel 6.8-rc3.
>
> Signed-off-by: Serge Semin <[e-mail]>
> Cc: Punnaiah Choudary Kalluri <[e-mail]>
> Cc: Dinh Nguyen <[e-mail]>
> Cc: Arnd Bergmann <[e-mail]>
> Cc: Greg Kroah-Hartman <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
>
> 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
>
>