[RESEND,v3,00/17] EDAC/mc/synopsys: Various fixes and cleanups

Message

Serge Semin Sept. 29, 2022, 11:26 p.m. UTC

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/[e-mail]
[3: In-progress] EDAC/synopsys: Add generic resources and Baikal-T1 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.

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/[e-mail]/
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

Link: https://lore.kernel.org/linux-edac/[e-mail]
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)

Signed-off-by: Serge Semin <[e-mail]>
Cc: Alexey Malahov <[e-mail]>
Cc: Michail Ivanov <[e-mail]>
Cc: Pavel Parkhomenko <[e-mail]>
Cc: Punnaiah Choudary Kalluri <[e-mail]>
Cc: Manish Narani <[e-mail]>
Cc: Dinh Nguyen <[e-mail]>
Cc: James Morse <[e-mail]>
Cc: Robert Richter <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: Krzysztof Kozlowski <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: Krzysztof Kozlowski <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]

Serge Semin (17):
  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
  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

 MAINTAINERS                  |   1 +
 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     | 501 +++++++++++++++++++
 7 files changed, 927 insertions(+), 627 deletions(-)
 create mode 100644 drivers/edac/zynq_edac.c

Comments

Borislav Petkov Sept. 30, 2022, 2:29 p.m. UTC | #1

On Fri, Sep 30, 2022 at 02:26:55AM +0300, Serge Semin wrote:
> 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/[e-mail]
> [3: In-progress] EDAC/synopsys: Add generic resources and Baikal-T1 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.

So I'd take a look slowly but I'd like for this driver's maintainer -
Michal Simek - to have a look first.

Thx.

Michal Simek Oct. 6, 2022, 7:13 a.m. UTC | #2

Hi Boris,

On 9/30/22 16:29, Borislav Petkov wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
> 
> 
> On Fri, Sep 30, 2022 at 02:26:55AM +0300, Serge Semin wrote:
>> 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/[e-mail]
>> [3: In-progress] EDAC/synopsys: Add generic resources and Baikal-T1 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.
> 
> So I'd take a look slowly but I'd like for this driver's maintainer -
> Michal Simek - to have a look first.

It is assigned to my fragment for quite a long time. Origin author Punnaiah is 
no longer working on this driver. But we have new owner Shubhrajyoti who will 
take a look this patchset and test it on Xilinx HW.
Shubhrajyoti: please take a look at this series.

Thanks,
Michal

Serge Semin Aug. 18, 2023, 11:06 p.m. UTC | #3

Hi Borislav

On Fri, Sep 30, 2022 at 02:26:55AM +0300, Serge Semin wrote:
> 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/[e-mail]
> [3: In-progress] EDAC/synopsys: Add generic resources and Baikal-T1 support
> Link: https://lore.kernel.org/linux-edac/[e-mail]

I'm terribly sorry for a very long delay with addressing your comments
in this patchset. I have been very busy all that time with another
kernel drivers. Now I am finally able to get back to this series.

This patchset and another two ones have been successfully rebased onto
the latest kernel version - 6.5-rc5. No regression has been spotted in
my environment.

Regarding your comments to this series. I've got a general gist
concerning my patchlog messages. I'll convert all of them to using the
imperative mood and drop "what" description in v4.

Regarding the main issue of detaching the Zynq A05 DDRC part from the
Synopsys uMCTL2 driver. I see there is another ZynqMP mem-ctl driver
has been added recently. It's ZynqMP OCM. During its review you
settled that it's possible to have two different EDAC drivers executed
on the system.
https://lore.kernel.org/linux-edac/[e-mail]/
So it's no longer issue in this series and the Zynq A05 DDRC part can
be freely moved to a separate driver as I originally intended.

Regarding the MC identifiers patch. I'll revise it one more time
whether it needs to be changed. In anyway I still find it very handy
to have a functionality of dynamic or alias-based MC identifier
allocation.

I'll respond to your comment sent to the patch 1 of this series. Then
I'll fix all the issues described above and resubmit v4. Then we'll be
able to get back to a standard review procedure.

Once again sorry for the huge delay with proceeding in this patchset
review.

Regards
-Serge(y)

> 
> 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.
> 
> 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/[e-mail]/
> 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
> 
> Link: https://lore.kernel.org/linux-edac/[e-mail]
> 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)
> 
> Signed-off-by: Serge Semin <[e-mail]>
> Cc: Alexey Malahov <[e-mail]>
> Cc: Michail Ivanov <[e-mail]>
> Cc: Pavel Parkhomenko <[e-mail]>
> Cc: Punnaiah Choudary Kalluri <[e-mail]>
> Cc: Manish Narani <[e-mail]>
> Cc: Dinh Nguyen <[e-mail]>
> Cc: James Morse <[e-mail]>
> Cc: Robert Richter <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: Krzysztof Kozlowski <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: Krzysztof Kozlowski <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> 
> Serge Semin (17):
>   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
>   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
> 
>  MAINTAINERS                  |   1 +
>  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     | 501 +++++++++++++++++++
>  7 files changed, 927 insertions(+), 627 deletions(-)
>  create mode 100644 drivers/edac/zynq_edac.c
> 
> -- 
> 2.37.3
> 
>