Received-SPF: pass (google.com: domain of linux-kernel+bounces-86554-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 29 Feb 2024 11:31:19 +0100
From: Miquel Raynal <miquel.raynal@bootlin.com>
To: William Zhang <william.zhang@broadcom.com>
Cc: Linux MTD List <linux-mtd@lists.infradead.org>, Linux ARM List
 <linux-arm-kernel@lists.infradead.org>, Broadcom Kernel List
 <bcm-kernel-feedback-list@broadcom.com>, f.fainelli@gmail.com,
 kursad.oney@broadcom.com, joel.peshkin@broadcom.com,
 anand.gore@broadcom.com, dregan@mail.com, kamal.dasu@broadcom.com,
 tomer.yacoby@broadcom.com, dan.beygelman@broadcom.com, David Regan
 <dregan@broadcom.com>, linux-kernel@vger.kernel.org, Vignesh Raghavendra
 <vigneshr@ti.com>, Brian Norris <computersforpeace@gmail.com>, Richard
 Weinberger <richard@nod.at>
Subject: Re: [PATCH v6 12/13] mtd: rawnand: brcmnand: Add support for
 getting ecc setting from strap
Message-ID: <20240229113119.30f7cee6@xps-13>
In-Reply-To: <be75db7d-a698-43ab-b29e-dea3a0f60ba2@broadcom.com>
References: <20240223034758.13753-1-william.zhang@broadcom.com>
	<20240223034758.13753-13-william.zhang@broadcom.com>
	<20240223101852.005da3ad@xps-13>
	<db1c1e68-40e2-4c6c-afb2-2fb591883a46@broadcom.com>
	<20240226093632.089d594c@xps-13>
	<be75db7d-a698-43ab-b29e-dea3a0f60ba2@broadcom.com>
Organization: Bootlin
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hi William,

william.zhang@broadcom.com wrote on Mon, 26 Feb 2024 12:05:18 -0800:

> On 2/26/24 00:36, Miquel Raynal wrote:
> > Hi William,
> >=20
> > william.zhang@broadcom.com wrote on Fri, 23 Feb 2024 09:25:09 -0800:
> >  =20
> >> Hi Miquel,
> >>
> >> On 2/23/24 01:18, Miquel Raynal wrote: =20
> >>> Hi William,
> >>>
> >>> william.zhang@broadcom.com wrote on Thu, 22 Feb 2024 19:47:57 -0800: =
=20
> >>>    >>>> BCMBCA broadband SoC based board design does not specify ecc =
setting in =20
> >>>> dts but rather use the SoC NAND strap info to obtain the ecc strength
> >>>> and spare area size setting. Add brcm,nand-ecc-use-strap dts propety=
 for
> >>>> this purpose and update driver to support this option. However these=
 two
> >>>> options can not be used at the same time.
> >>>>
> >>>> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> >>>> Reviewed-by: David Regan <dregan@broadcom.com> =20
> >>>>   >>> =20
> >>> FYI I did not receive patches 7, 8, 9, which makes the series numberi=
ng
> >>> very odd. =20
> >>>    >> I was using the get maintainer script mainly and it sends to th=
e linux MTD list.  I will add your email directly next time. =20
> >=20
> > Yes, I prefer to be in Cc of the whole series, please.
> >  =20
> Sure.  And thanks for applying other patches.  Do you want me to just sen=
d a new single patch for the update?

Yes just the missing patch.

> >>>>    >> +static int brcmnand_get_sector_size_1k(struct brcmnand_host *=
host) =20
> >>>> +{
> >>>> +	struct brcmnand_controller *ctrl =3D host->ctrl;
> >>>> +	int sector_size_bit =3D brcmnand_sector_1k_shift(ctrl);
> >>>> +	u16 acc_control_offs =3D brcmnand_cs_offset(ctrl, host->cs,
> >>>> +						  BRCMNAND_CS_ACC_CONTROL);
> >>>> +	u32 acc_control;
> >>>> +
> >>>> +	if (sector_size_bit < 0)
> >>>> +		return 0;
> >>>> +
> >>>> +	acc_control =3D nand_readreg(ctrl, acc_control_offs);
> >>>> +
> >>>> +	return (acc_control & BIT(sector_size_bit)) >> sector_size_bit; =20
> >>>
> >>> FIELD_PREP, FIELD_GET, *please*. =20
> >> You probably missed my reply to your comments on the same patch in v5.=
 Here is the link for the post in case it lost in your email:
> >> https://lore.kernel.org/lkml/c145b90c-e9f0-4d82-94cc-baf7bfda5954@gmai=
l.com/T/#m1d911d2f119f3bd345c575a81b60bc2bd8c461eb =20
> >=20
> > I didn't miss it, but the reason does not sound legitimate to me.
> > Please work on it, it will be so much cleaner.
> >  =20
> I understand FIELD_PREP/GET is the preferred way of linux accessing the r=
egister fields but it requires a constant MASK value and does not apply to =
our case as we have different versions of the register and have different m=
ask.  There is way to workaround it. i.e defining the multiple constants di=
rectly and using these macros with if/else based on reg version. But it is =
not clean and since we already have helper functions that handle and return=
 different shift/mask value, I see this is a perfect way for our situation =
and can adapt to future reg version change easily and cleanly.
>=20
> >> The mask is not constant here and cause build errors. =20

Which errors?

+       acc_control =3D nand_readreg(ctrl, acc_control_offs);
+       return FIELD_GET(BIT(sector_size_bit), acc_control);

Does not return any error here.

Thanks,
Miqu=C3=A8l