Received-SPF: pass (google.com: domain of linux-kernel+bounces-201247-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 4 Jun 2024 20:38:15 +0200
From: Borislav Petkov <bp@alien8.de>
To: Serge Semin <fancer.lancer@gmail.com>
Cc: Michal Simek <michal.simek@amd.com>,
	Alexander Stein <alexander.stein@ew.tq-group.com>,
	Tony Luck <tony.luck@intel.com>, James Morse <james.morse@arm.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Robert Richter <rric@kernel.org>,
	Manish Narani <manish.narani@xilinx.com>,
	Punnaiah Choudary Kalluri <punnaiah.choudary.kalluri@xilinx.com>,
	Dinh Nguyen <dinguyen@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-arm-kernel@lists.infradead.org, linux-edac@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 02/20] EDAC/synopsys: Fix generic device type
 detection procedure
Message-ID: <20240604183803.GJZl9fC9R5M2NSQ01O@fat_crate.local>
References: <20240222181324.28242-1-fancer.lancer@gmail.com>
 <20240222181324.28242-3-fancer.lancer@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240222181324.28242-3-fancer.lancer@gmail.com>
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 22, 2024 at 09:12:47PM +0300, Serge Semin wrote:
> First of all the enum dev_type constants describe the memory DRAM chips
> used at the stick, not the entire DQ-bus width (see the enumeration kdo=
c

Which kdoc?

The kernel doc above enum dev_type in include/linux/edac.h?

In any case, you need to be precise pls.

> for details). So what is returned from the zynqmp_get_dtype() function =
and
> then specified to the dimm_info->dtype field is definitely incorrect.

Whoops, you lost me here. Why is it incorrect?

You want

"zynqmp_get_dtype - Return the controller memory width."

to return the memory width supported by the controller?

	dimm->dtype     =3D p_data->get_dtype(priv->baseaddr);

	enum dev_type dtype;    /* memory device type */

Yeah, no, that function returns the DIMM device type.

/me looks at the code.

Aha, so you mean the device width should be determined from that
DDRC_MSTR_CFG* thing.

> Secondly the DRAM chips type has nothing to do with the data bus width
> specified in the MSTR.data_bus_width CSR field. That CSR field just
> determines the part of the whole DQ-bus currently used to access the da=
ta
> from the all DRAM memory chips. So it doesn't indicate the individual
> chips type. Thirdly the DRAM chips type can be determined only in case =
of
> the DDR4 protocol by means of the MSTR.device_config field state (it is

Hold on, this driver runs on all kinds of hardware I presume. Are you
thinking about older ones which don't do DDR4?

Or does that thing do DDR4 only?

> supposed to be set by the system firmware). Finally the DW uMCTL2 DDRC =
ECC
> capability doesn't depend on the memory chips type. Moreover it doesn't
> depend on the utilized data bus width in runtime either. The IP-core
> reference manual says in [1,2] that the ECC support can't be enabled
> during the IP-core synthesizes for the DRAM data bus widths other than =
16,

This sentence is missing something.

> 32 or 64.  At the same time the bus width mode (MSTR.data_bus_width)
> doesn't change the ECC feature availability. Thus it was wrong to
> determine the ECC state with respect to the DQ-bus width mode.

You need to split your paragraphs with newlines to help readability.
Right now it is a blob of hard to parse text. For example, when you have
to write "Secondly, " that's your split right there. "Thirdly," is your
next newline. And so on.

> Fix all of the mistakes described above in the zynqmp_get_dtype() and
> zynqmp_get_ecc_state() methods: specify actual DRAM chips data width on=
ly
> for the DDR4 protocol and return that it's UNKNOWN in the rest of the
> cases;

What are the rest of the cases and why is it ok to return UNKNOWN all of
a sudden? IOW, how was the old code even tested?!

> determine ECC availability by the ECCCFG0.ecc_mode field state
> only (that field can't be modified anyway if the IP-core was synthesize=
d
> with no ECC support).
>=20
> [1] DesignWare=C2=AE Cores Enhanced Universal DDR Memory Controller (uM=
CTL2)
> Databook, Version 3.91a, October 2020, p. 421.
> [2] DesignWare=C2=AE Cores Enhanced Universal DDR Memory Controller (uM=
CTL2)
> Databook, Version 3.91a, October 2020, p. 633.

Can those be freely accessed?

If not, you should say so.

> Fixes: b500b4a029d5 ("EDAC, synopsys: Add ECC support for ZynqMP DDR co=
ntroller")

So this commit is in 4.20.

Does that mean that this fix needs to get backported to all stable
kernels?

Have you tested this on all hw this driver supports and made sure no
regressions are introduced?

Thx.

--=20
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette