Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 5 Jun 2023 11:05:46 +0200
From:   Miquel Raynal <miquel.raynal@bootlin.com>
To:     Arseniy Krasnov <avkrasnov@sberdevices.ru>
Cc:     Liang Yang <liang.yang@amlogic.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Neil Armstrong <neil.armstrong@linaro.org>,
        Kevin Hilman <khilman@baylibre.com>,
        Jerome Brunet <jbrunet@baylibre.com>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        <oxffffaa@gmail.com>, <kernel@sberdevices.ru>,
        <linux-mtd@lists.infradead.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-amlogic@lists.infradead.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH v5 2/6] mtd: rawnand: meson: wait for command in
 polling mode
Message-ID: <20230605110546.6cb00a8d@xps-13>
In-Reply-To: <9e106d50-2524-c999-48b1-a20760238aaf@sberdevices.ru>
References: <20230601061850.3907800-1-AVKrasnov@sberdevices.ru>
        <20230601061850.3907800-3-AVKrasnov@sberdevices.ru>
        <20230601100751.41c3ff0b@xps-13>
        <9e106d50-2524-c999-48b1-a20760238aaf@sberdevices.ru>
Organization: Bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Arseniy,

> >> @@ -1412,6 +1419,8 @@ static int meson_nfc_probe(struct platform_devic=
e *pdev)
> >>  		return ret;
> >>  	}
> >> =20
> >> +	nfc->use_polling =3D of_property_read_bool(dev->of_node, "polling");=
 =20
> >=20
> > This is a problem. You cannot add a polling property like that.
> >=20
> > There is already a nand-rb property which is supposed to carry how are
> > wired the RB lines. I don't see any in-tree users of the compatibles, I
> > don't know how acceptable it is to consider using soft fallback when
> > this property is missing, otherwise take the values of the rb lines
> > provided in the DT and user hardware control, but I would definitely
> > prefer that. =20
>=20
> I see. So i need to implement processing of this property here? And if it
> is missed -> use software waiting. I think interesting thing will be that:
>=20
> 1) Even with support of this property here, I really don't know how to pa=
ss
>    RB values to this controller - I just have define for RB command and t=
hat's
>    it. I found that this property is an array of u32 - IIUC each element =
is
>    RB pin per chip. May be i need to dive into the old vendor's driver to=
 find
>    how to use RB values (although this driver uses software waiting so I'=
m not
>    sure that I'll find something in it).

Liang, can you please give use the relevant information here? How do we
target RB0 and RB1? It seems like you use the CS as only information
like if the RB lines where hardwired internally to a CS. Can we invert
the lines with a specific configuration?

Arseniy, if the answer to my above question is no, then you should
expect the nand-rb and reg arrays to be identical. If they are not,
then you can return -EINVAL.

If the nand-rb property is missing, then fallback to software wait.

> 2) I can't test RB mode - I don't have such device :(=20
>=20
> Also for example in arasan-nand-controller.c parsed 'nand-rb' values are =
used
> in controller specific register for waiting (I guess Meson controller has=
 something
> like that, but I don't have doc). While in marvell_nand.c it looks like t=
hat they parse
> 'nand-rb' property, but never use it.

Yes, the logic around the second RB line (taking care of CS1/CS3) is
slightly broken or at least badly documented, and thus should not be
used.

> > In any case you'll need a dt-binding update which must be acked by
> > dt-binding maintainers. =20
>=20
> You mean to add this property desc to Documentation/devicetree/bindings/m=
td/amlogic,meson-nand.yaml ?

Yes. In a dedicated patch. Something along the lines:

	nand-rb: true

inside the nand chip object should be fine. And flag the change as a
fix because we should have used and parsed this property since the
beginning.

Thanks,
Miqu=C3=A8l