Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Naga Sureshkumar Relli <nagasure@xilinx.com>
To:     Helmut Grohne <helmut.grohne@intenta.de>
CC:     "bbrezillon@kernel.org" <bbrezillon@kernel.org>,
        "miquel.raynal@bootlin.com" <miquel.raynal@bootlin.com>,
        "richard@nod.at" <richard@nod.at>,
        "dwmw2@infradead.org" <dwmw2@infradead.org>,
        "computersforpeace@gmail.com" <computersforpeace@gmail.com>,
        "marek.vasut@gmail.com" <marek.vasut@gmail.com>,
        "linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Michal Simek <michals@xilinx.com>,
        "nagasureshkumarrelli@gmail.com" <nagasureshkumarrelli@gmail.com>
Subject: RE: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm
 pl353 smc nand interface
Thread-Topic: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for arm
 pl353 smc nand interface
Thread-Index: AQHU83vggkxmg2rBCEyEcIHuXS/Vp6ZMy7GAgAFFM5CABRNXAIAAA45Q
Date:   Mon, 29 Apr 2019 12:35:38 +0000
Message-ID: <DM6PR02MB477988A7F04027F41834AC72AF390@DM6PR02MB4779.namprd02.prod.outlook.com>
References: <1555326613-26739-1-git-send-email-naga.sureshkumar.relli@xilinx.com>
 <20190425112338.dipgmqqfuj45gx6s@laureti-dev>
 <DM6PR02MB4779EE37978EC0E6475C55D7AF390@DM6PR02MB4779.namprd02.prod.outlook.com>
 <20190429121804.4jzspv4goehwdpez@laureti-dev>
In-Reply-To: <20190429121804.4jzspv4goehwdpez@laureti-dev>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: xilinx.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 2cdabeda-79c1-419a-59f5-08d6cc9f2f35
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Apr 2019 12:35:38.2114
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR02MB4236
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Helmut,

> -----Original Message-----
> From: Helmut Grohne <helmut.grohne@intenta.de>
> Sent: Monday, April 29, 2019 5:48 PM
> To: Naga Sureshkumar Relli <nagasure@xilinx.com>
> Cc: bbrezillon@kernel.org; miquel.raynal@bootlin.com; richard@nod.at;
> dwmw2@infradead.org; computersforpeace@gmail.com; marek.vasut@gmail.com; =
linux-
> mtd@lists.infradead.org; linux-kernel@vger.kernel.org; Michal Simek <mich=
als@xilinx.com>;
> nagasureshkumarrelli@gmail.com
> Subject: Re: [LINUX PATCH v14] mtd: rawnand: pl353: Add basic driver for =
arm pl353 smc
> nand interface
>=20
> On Mon, Apr 29, 2019 at 11:31:14AM +0000, Naga Sureshkumar Relli wrote:
> > But just wanted to know, do you see issues with these __force and __iom=
em castings?
>=20
> I only see a minor issue: They're (deliberately) lengthy. Using many of t=
hem diverts attention
> of the reader. Therefore, my proposal attempted to reduce their frequency=
. The only issue I see
> here is readability.
Ok then, I will update it.
>=20
> > >
> > > > +	u8 addr_cycles;
> > > > +	struct clk *mclk;
> > >
> > > All you need here is the memory clock frequency. Wouldn't it be
> > > easier to extract that frequency once during probe and store it
> > > here? That assumes a constant frequency, but if the frequency isn't c=
onstant, you have a
> race condition.
> > That is what we are doing in the probe.
> > In the probe, we are getting mclk using of_clk_get() and then we are
> > getting the actual frequency Using clk_get_rate().
> > And this is constant frequency only(getting from dts)
>=20
> Not quite. You're getting a clock reference in probe and then repeatedly =
access the frequency
> elswhere. I am suggesting that you get the clock frequency during probe a=
nd never save the
> clock reference to a struct.
Ok. got it. Will update.
>=20
> > > > +		case NAND_OP_ADDR_INSTR:
> > > > +			offset =3D nand_subop_get_addr_start_off(subop, op_id);
> > > > +			naddrs =3D nand_subop_get_num_addr_cyc(subop, op_id);
> > > > +			addrs =3D &instr->ctx.addr.addrs[offset];
> > > > +			nfc_op->addrs =3D instr->ctx.addr.addrs[offset];
> > > > +			for (i =3D 0; i < min_t(unsigned int, 4, naddrs); i++) {
> > > > +				nfc_op->addrs |=3D instr->ctx.addr.addrs[i] <<
> > >
> > > I don't quite understand what this code does, but it looks strange
> > > to me. I compared it to other drivers. The code here is quite
> > > similar to marvell_nand.c. It seems like we are copying a varying
> > > number (0 to 6) of addresses from the buffer instr->ctx.addr.addrs.
> > > However their indices are special: 0, 1, 2, 3, offset + 4, offset + 5=
. This is non-consecutive
> and different from marvell_nand.c in this regard. Could it be that you re=
ally meant index
> offset+i here?
> > I didn't get, what you are saying here.
> > It is about updating page and column addresses.
> > Are you asking me to remove nfc_op->addrs =3D instr->ctx.addr.addrs[off=
set]; before for
> loop?
>=20
> I compared this code to marvell_nand.c and noticed a subtle difference.
> Both snippets read 6 address bytes and consume them in a driver-specific =
way. Now which
> address bytes are consumed differs.
>=20
> marvell_nand.c consumes instr->ctx.addr.addrs at indices offset,
> offset+1, offset+2, offset+3, offset+4, offset+5. pl353_nand.c consumes
> instr->ctx.addr.addrs at indices 0, 1, 2, 3, offset, offset+4, offset+5.
> (In my previous mail, I didn't notice that it was also consuming the offs=
et index.)
>=20
> I would have expected this behaviour to be consistent between different d=
rivers. If I assume
> marvell_nand.c to do the right thing and pl353_nand.c to be wrong (which =
is not necessarily a
> correct assumption), then the code woule likely becom:
>=20
> 	addrs =3D &instr->ctx.addr.addrs[offset];
> 	for (i =3D 0; i < min_t(unsigned int, 4, naddrs); i++) {
> 		nfc_op->addrs |=3D addrs[i] << (8 * i);
> 		              // ^^^^^
> 	}
>=20
> Hope this helps.
Ok. let me re check this and I will update this accordingly.

Thanks,
Naga Sureshkumar Relli
>=20
> Helmut