From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_2 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 69375C2BB1D for ; Mon, 16 Mar 2020 10:08:34 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 3834E2051A for ; Mon, 16 Mar 2020 10:08:34 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="KzTESjMd" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3834E2051A Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=bootlin.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-mtd-bounces+linux-mtd=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=etEIZQqwpKutIts0polHh43Vz8fDPp9b9ncd671JcMQ=; b=KzTESjMdEndDOr 5wcJgXs8FUpSt4CF+ghzPgNRno+4iYFSd77YkTTsaQ2qq0DoD96YDp9+9Q5GCgzqxxnPdeoLL0WTV rVYelIlSsTRdzpeBYfDsBzUAhYd86Pn1UJGmJB06uke/nw1MDJY6JsBHrPOedzpdh82Oo/zT33zb4 97CFBGtuAmHpS7vPNqlIB0cpeMVbbOmFyKm76oVgARJtfXBCpbAPm3CaSOAQsTevRywm64tqj/n/8 CzY5wxE3pru8lL2RhoI3krFf9xz++MABnCyawplK2Gqy93UbAe0KihTIKONS6O12HKTq5Uqx+6Bv8 ob2L3xcA0WrbuznFQwyQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jDmfU-0004v7-QC; Mon, 16 Mar 2020 10:08:20 +0000 Received: from relay3-d.mail.gandi.net ([217.70.183.195]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jDmfQ-0004qs-Oy; Mon, 16 Mar 2020 10:08:18 +0000 X-Originating-IP: 90.89.41.158 Received: from xps13 (lfbn-tou-1-1473-158.w90-89.abo.wanadoo.fr [90.89.41.158]) (Authenticated sender: miquel.raynal@bootlin.com) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id B0FD26002D; Mon, 16 Mar 2020 10:06:58 +0000 (UTC) Date: Mon, 16 Mar 2020 11:06:58 +0100 From: Miquel Raynal To: =?UTF-8?B?6LW15Luq5bOw?= Subject: Re: [PATCH v3 1/3] mtd: rawnand: rockchip: NFC drivers for RK3308, RK3188 and others Message-ID: <20200316110658.43aea94a@xps13> In-Reply-To: <2020031617554207432140@rock-chips.com> References: <20200303094736.7490-1-yifeng.zhao@rock-chips.com> <20200303094736.7490-2-yifeng.zhao@rock-chips.com> <20200309121645.1fca069d@xps13> <2020031617554207432140@rock-chips.com> Organization: Bootlin X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200316_030817_080181_BFCC2D39 X-CRM114-Status: GOOD ( 21.99 ) X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree , vigneshr , richard , linux-rockchip , robh+dt , linux-mtd , =?UTF-8?B?SGVpa29TdMO8Ym5lcg==?= Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Sender: "linux-mtd" Errors-To: linux-mtd-bounces+linux-mtd=archiver.kernel.org@lists.infradead.org SGkgWWlmZW5nLAoK6LW15Luq5bOwIDx5aWZlbmcuemhhb0Byb2NrLWNoaXBzLmNvbT4gd3JvdGUg b24gTW9uLCAxNiBNYXIgMjAyMCAxNzo1OToyNgorMDgwMDoKCj4gSGkgbWlxdWVs77yMCj4gCj4g MS4KPiA+QSBjb21tZW50IGhlcmUgZXhwbGFpbmluZyB3aGF0IHRoZSBuZXh0IGZ1bmN0aW9uIGRv ZXMgYW5kIHdoeSB3b3VsZCBiZQo+ID5uaWNlLgo+ID4gIAo+ID4+ICtzdGF0aWMgdm9pZCBya19u ZmNfZm9ybWF0X3BhZ2Uoc3RydWN0IG10ZF9pbmZvICptdGQsIGNvbnN0IHU4ICpidWYpCj4gPj4g K3sKPiA+PiArCXN0cnVjdCBuYW5kX2NoaXAgKmNoaXAgPSBtdGRfdG9fbmFuZChtdGQpOwo+ID4+ ICsJc3RydWN0IHJrX25mYyAqbmZjID0gbmFuZF9nZXRfY29udHJvbGxlcl9kYXRhKGNoaXApOwo+ ID4+ICsJdTMyIGk7Cj4gPj4gKyAgIAo+IAo+IFRoZSBkYXRhIGxheW91dCBpcyBkaWZmZXJlbnQg YmV0d2VlbiBORkMgYW5kIG5hbmQgwqBkcml2ZXIKCnlvdSBwcm9iYWJseSBtZWFuIGJldHdlZW4g dGhlIE5BTkQgZmxhc2ggY29udHJvbGxlciBhbmQgd2jDqXQgdGhlIE5BTkQKY29yZSBleHBlY3Rz LCBidXQgb2sKCj4gVGhpcyBjb2RlIGlzIGRlc2lnbmVkIHdpdGggcmVmZXJlbmNlIHRvIG10a19u YW5kLmMKPiBUaGVyZSBpcyBhIGRlc2NyaXB0aW9uIG9mIHRoZSBkYXRhIGxheW91dCBhdCB0aGUg YmVnaW5uaW5nIG9mIHRoZSBmaWxlOgo+IMKgKiBORkMgUGFnZSBEYXRhIExheW91dDoKPiDCoCoJ MTAyNCBCeXRlcyBEYXRhICsgNEJ5dGVzIHN5cyBkYXRhICsgMjhCeXRlc34xMjRCeXRlcyBlY2Mg Kwo+IMKgKgkxMDI0IEJ5dGVzIERhdGEgKyA0Qnl0ZXMgc3lzIGRhdGEgKyAyOEJ5dGVzfjEyNEJ5 dGVzIGVjYyArCj4gwqAqCS4uLi4uLgo+IMKgKiBOQU5EIFBhZ2UgRGF0YSBMYXlvdXQ6Cj4gwqAq CTEwMjQgKiBuIERhdGEgKyBtIEJ5dGVzIG9vYgo+IMKgKiBPcmlnaW5hbCBCYWQgQmxvY2sgTWFz ayBMb2NhdGlvbjoKPiDCoCoJZmlyc3QgYnl0ZSBvZiBvb2Ioc3BhcmUpCj4gwqAqIG5hbmRfY2hp cC0+b29iX3BvaSBkYXRhIGxheW91dDoKPiDCoCoJNEJ5dGVzIHN5cyBkYXRhICsgLi4uLiArIDRC eXRlcyBzeXMgZGF0YSArIGVjYyBkYXRhCj4gCj4gMi7CoAo+ID4+ICsJZG1hX3JlZyA9IERNQV9T VCB8ICgoIXJ3KSA8PCBETUFfV1IpIMKgfCBETUFfRU4gfCAoMiA8PCBETUFfQUhCX1NJWkUpIHwK PiA+PiArCcKgIMKgIMKgKDcgPDwgRE1BX0JVUlNUX1NJWkUpIHwgKDE2IDw8IERNQV9JTkNfTlVN KTsKPiA+PiArCj4gPj4gKwlmbF9yZWcgPSAocncgPDwgRkxDVExfV1IpIHwgRkxDVExfWEZFUl9F TiB8IEZMQ1RMX0FDT1JSRUNUIHwKPiA+PiArCShuX0tCIDw8IEZMQ1RMX1hGRVJfU0VDVE9SKSB8 IEZMQ1RMX1RPR19GSVg7Cj4gPj4gKwo+ID4+ICsJaWYgKG5mYy0+bmZjX3ZlcnNpb24gPT0gNikg eyAgCj4gPgo+ID5JIHdvdWxkIHByZWZlciB1c2luZyBzd2l0Y2ggc3RhdGVtZW50cyBhbnkgdGlt ZSB5b3UgY2hlY2sgdGhlIHZlcnNpb24uCj4gPlRoZSB2ZXJzaW9uIHNob3VsZCBiZSBhbiBlbnVt Lgo+ID4KPiA+WW91IGNhbiBhbHNvIGRlZmluZSBhIHBsYXRmb3JtIGRhdGEgc3RydWN0dXJlIGZv ciB0aGUgcmVnaXN0ZXIgb2Zmc2V0cwo+ID50aGF0IGhhdmUgdGhlIHNhbWUgbmFtZSwgYnV0IG5v dCBuZWNlc3NhcmlseSB0aGUgc2FtZSBvZmZzZXQuIFRoZW4geW91Cj4gPmNhbiByZWZlcmVuY2Ug dGhlIHJpZ2h0IHZhbHVlIGRpcmVjdGx5Lgo+ID5lZy4KPiA+Cj4gPglzdHJ1Y3QgcmtfbmZjX3Bs YXRfZGF0YSB7Cj4gPgl1MzIgbmZjX2JjaGN0bF9vZmY7Cj4gPgkuLi4KPiA+CX07Cj4gPgo+ID4J c3RydWN0IHJrX25mY19wbGF0X2RhdGEgcmtfbmZjX3Y2X3BsYXRfZGF0YSA9IHsKPiA+CW5mY19i Y2hjdGxfb2ZmID0gLi4uOwo+ID4JLi4uCj4gPgl9Owo+ID4KPiA+CWJjaF9yZWcgPSByZWFkbChw ZGF0YS0+bmZjX2JjaGN0bF9vZmYpOyAgCj4gCj4gSSB3aWxsIG1vZGlmeSB0aGUgY29kZSB3aXRo IHN3aXRjaCBhbmQgZW51bSwgYnV0IGl0IGlzIGRpZmZpY3VsdCB0byB1c2UgcGxhdGZvcm0gZGF0 YSBzdHJ1Y3R1cmUswqAKPiBiZWNhdXNlIHRoZSBiaXQgb2Zmc2V0IGluc2lkZSB0aGUgcmVnaXN0 ZXIgaXMgYWxzbyBkaWZmZXJlbnQuCgppdCB3b3JrcyB0aGUgc2FtZSB3aXRoIGJpdGZpZWxkcyBh Y3R1YWxseSwgaWYgdGhlIGJpdGZpZWxkcyBoYXZlIGNsb3NlCm5hbWVzIGFuZCBiZWhhdmUgdGhl IHNhbWUgKG5vIG1hdHRlciB3aGVyZSB0aGV5IGFyZSBpbiByZWdpc3RlcnMpLCB5b3UKc2hvdWxk IHByb2JhYmx5IGRlZmluZSB0aGVtIGluIGEgcGxhdGZvcm0gZGF0YSBzdHJ1Y3R1cmUgYXMgd2Vs bC4KCj4gI2RlZmluZQlORkNfQkNIX1NUX1Y2CSgweDIwKQo+ICNkZWZpbmUJTkZDX0JDSF9TVF9W OQkoMHgxNTApCj4gI2RlZmluZQlCQ0hfU1RfRVJSMF9WNglCSVQoMikKPiAjZGVmaW5lCUJDSF9T VF9FUlIxX1Y2CUJJVCgxNSkKPiAjZGVmaW5lCUJDSF9TVF9FUlIwX1Y5CUJJVCgyKQo+ICNkZWZp bmUJQkNIX1NUX0VSUjFfVjkJQklUKDE4KQo+ICNkZWZpbmUJRUNDX0VSUl9DTlQwX1Y2KHgpICgo KCgoeCkgJiAoMHgxRiA8PCAzKSkgPj4gMykgXAo+IHwgKCgoeCkgJiAoMSA8PCAyNykpID4+IDIy KSkgJiAweDNGKQo+ICNkZWZpbmUJRUNDX0VSUl9DTlQxX1Y2KHgpICgoKCgoeCkgJiAoMHgxRiA8 PCAxNikpID4+IDE2KSBcCj4gfCAoKCh4KSAmICgxIDw8IDI5KSkgPj4gMjQpKSAmIDB4M0YpCj4g I2RlZmluZQlFQ0NfRVJSX0NOVDBfVjkoeCkgKCgoeCkgJiAoMHg3RiA8PCAzKSkgPj4gMykKPiAj ZGVmaW5lCUVDQ19FUlJfQ05UMV9WOSh4KSAoKCh4KSAmICgweDdGIDw8IDE5KSkgPj4gMTkpCj4g Cj4gMy4KPiA+PiArc3RhdGljIGludCBya19uZmNfd3JpdGVfcGFnZV9yYXcoc3RydWN0IG5hbmRf Y2hpcCAqY2hpcCwgY29uc3QgdTggKmJ1ZiwKPiA+PiArCcKgaW50IG9vYl9vbiwgaW50IHBhZ2Up Cj4gPj4gK3sKPiA+PiArCXN0cnVjdCBtdGRfaW5mbyAqbXRkID0gbmFuZF90b19tdGQoY2hpcCk7 Cj4gPj4gKwlzdHJ1Y3QgcmtfbmZjICpuZmMgPSBuYW5kX2dldF9jb250cm9sbGVyX2RhdGEoY2hp cCk7Cj4gPj4gKwo+ID4+ICsJcmtfbmZjX2Zvcm1hdF9wYWdlKG10ZCwgYnVmKTsKPiA+PiArCXJl dHVybiBya19uZmNfd3JpdGVfcGFnZShtdGQsIGNoaXAsIG5mYy0+YnVmZmVyLCBwYWdlLCAxKTsg IAo+ID4KPiA+SSB0aGluayB5b3Ugc2hvdWxkIGF2b2lkIGNhbGxpbmcgLT53cml0ZV9wYWdlLiBZ b3Ugd2lsbCBhdm9pZCBhbgo+ID5pbmRlbnRhdGlvbiBsZXZlbCBpbiB0aGlzIGZ1bmN0aW9uIGFu ZCBjbGFyaWZ5IHdoYXQgd3JpdGVfcGFnZV9yYXcgZG8uCj4gPlNhbWUgZm9yIHJlYWQsIGFuZCB0 aGUgX29vYiBhbHRlcm5hdGl2ZS4gQWxzbywgSSdtIHN1cmUgd3JpdGVfYnVmIGRvZXMKPiA+bm90 IG5lZWQgdG8gYmUgZXhwb3J0ZWQgYW5kIHlvdSBjYW4ganVzdCBtb3ZlIHRoZSBhY3R1YWwgY29k ZSBpbiB0aGlzCj4gPmZ1bmN0aW9uLiAgCj4gCj4gVGhlIGNvZGXCoMKgaXMgZGVzaWduZWQgd2l0 aCByZWZlcmVuY2UgdG8gbXRrX25hbmQuYy4KPiBUaGUgZnVuY3Rpb24gcmtfbmZjX2Zvcm1hdF9w YWdlIHdpbGwgY29weSBkYXRhIGZyb20gZXh0ZXJuIGJ1ZmZlciB0byBuZmMtPmJ1ZmZlci4KPiBJ IHdpbGwgbW92ZSB0aGUgY29kZSBpbiB0aGUgZnVuY3Rpb24gcmtfbmZjX2Zvcm1hdF9wYWdlIHRv IHJrX25mY193cml0ZV9wYWdlX3Jhdy4KPiAKPiA0Lgo+ID4+ICtzdGF0aWMgaW50IHJrX25mY193 cml0ZV9wYWdlX2h3ZWNjKHN0cnVjdCBuYW5kX2NoaXAgKmNoaXAsIGNvbnN0IHU4ICpidWYsCj4g Pj4gKwnCoCDCoGludCBvb2Jfb24sIGludCBwYWdlKQo+ID4+ICt7Cj4gPj4gKwlyZXR1cm4gcmtf bmZjX3dyaXRlX3BhZ2UobmFuZF90b19tdGQoY2hpcCksIGNoaXAsIGJ1ZiwgcGFnZSwgMCk7Cj4g Pj4gK30gIAo+ID4KPiA+V2hhdCBpcyB0aGUgcHVycG9zZSBvZiB0aGlzIGluZGlyZWN0aW9uPwo+ ID4gIAo+IAo+IFRoZSBmdW5jdGlvbiDCoHJrX25mY193cml0ZV9wYWdlIGFsc28gY2FsbCBiecKg cmtfbmZjX3dyaXRlX3BhZ2VfcmF3LMKgYSBwYXJhbWV0ZXIocmF3KQo+IGlzIHJlcXVpcmVkIHRv IGNvbmZpcm0gd2hldGhlciBFQ0MgaXMgdXNlZCBvciBub3QuCgpPay4KClRoYW5rcywKTWlxdcOo bAoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f CkxpbnV4IE1URCBkaXNjdXNzaW9uIG1haWxpbmcgbGlzdApodHRwOi8vbGlzdHMuaW5mcmFkZWFk Lm9yZy9tYWlsbWFuL2xpc3RpbmZvL2xpbnV4LW10ZC8K From mboxrd@z Thu Jan 1 00:00:00 1970 From: Miquel Raynal Subject: Re: [PATCH v3 1/3] mtd: rawnand: rockchip: NFC drivers for RK3308, RK3188 and others Date: Mon, 16 Mar 2020 11:06:58 +0100 Message-ID: <20200316110658.43aea94a@xps13> References: <20200303094736.7490-1-yifeng.zhao@rock-chips.com> <20200303094736.7490-2-yifeng.zhao@rock-chips.com> <20200309121645.1fca069d@xps13> <2020031617554207432140@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Return-path: In-Reply-To: <2020031617554207432140-TNX95d0MmH7DzftRWevZcw@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: =?UTF-8?B?6LW15Luq5bOw?= Cc: richard , vigneshr , robh+dt , devicetree , linux-mtd , =?UTF-8?B?SGVpa29TdMO8Ym5lcg==?= , linux-rockchip List-Id: linux-rockchip.vger.kernel.org Hi Yifeng, 赵仪峰 wrote on Mon, 16 Mar 2020 17:59:26 +0800: > Hi miquel， > > 1. > >A comment here explaining what the next function does and why would be > >nice. > > > >> +static void rk_nfc_format_page(struct mtd_info *mtd, const u8 *buf) > >> +{ > >> + struct nand_chip *chip = mtd_to_nand(mtd); > >> + struct rk_nfc *nfc = nand_get_controller_data(chip); > >> + u32 i; > >> + > > The data layout is different between NFC and nand  driver you probably mean between the NAND flash controller and whét the NAND core expects, but ok > This code is designed with reference to mtk_nand.c > There is a description of the data layout at the beginning of the file: >  * NFC Page Data Layout: >  * 1024 Bytes Data + 4Bytes sys data + 28Bytes~124Bytes ecc + >  * 1024 Bytes Data + 4Bytes sys data + 28Bytes~124Bytes ecc + >  * ...... >  * NAND Page Data Layout: >  * 1024 * n Data + m Bytes oob >  * Original Bad Block Mask Location: >  * first byte of oob(spare) >  * nand_chip->oob_poi data layout: >  * 4Bytes sys data + .... + 4Bytes sys data + ecc data > > 2.  > >> + dma_reg = DMA_ST | ((!rw) << DMA_WR)  | DMA_EN | (2 << DMA_AHB_SIZE) | > >> +      (7 << DMA_BURST_SIZE) | (16 << DMA_INC_NUM); > >> + > >> + fl_reg = (rw << FLCTL_WR) | FLCTL_XFER_EN | FLCTL_ACORRECT | > >> + (n_KB << FLCTL_XFER_SECTOR) | FLCTL_TOG_FIX; > >> + > >> + if (nfc->nfc_version == 6) { > > > >I would prefer using switch statements any time you check the version. > >The version should be an enum. > > > >You can also define a platform data structure for the register offsets > >that have the same name, but not necessarily the same offset. Then you > >can reference the right value directly. > >eg. > > > > struct rk_nfc_plat_data { > > u32 nfc_bchctl_off; > > ... > > }; > > > > struct rk_nfc_plat_data rk_nfc_v6_plat_data = { > > nfc_bchctl_off = ...; > > ... > > }; > > > > bch_reg = readl(pdata->nfc_bchctl_off); > > I will modify the code with switch and enum, but it is difficult to use platform data structure,  > because the bit offset inside the register is also different. it works the same with bitfields actually, if the bitfields have close names and behave the same (no matter where they are in registers), you should probably define them in a platform data structure as well. > #define NFC_BCH_ST_V6 (0x20) > #define NFC_BCH_ST_V9 (0x150) > #define BCH_ST_ERR0_V6 BIT(2) > #define BCH_ST_ERR1_V6 BIT(15) > #define BCH_ST_ERR0_V9 BIT(2) > #define BCH_ST_ERR1_V9 BIT(18) > #define ECC_ERR_CNT0_V6(x) (((((x) & (0x1F << 3)) >> 3) \ > | (((x) & (1 << 27)) >> 22)) & 0x3F) > #define ECC_ERR_CNT1_V6(x) (((((x) & (0x1F << 16)) >> 16) \ > | (((x) & (1 << 29)) >> 24)) & 0x3F) > #define ECC_ERR_CNT0_V9(x) (((x) & (0x7F << 3)) >> 3) > #define ECC_ERR_CNT1_V9(x) (((x) & (0x7F << 19)) >> 19) > > 3. > >> +static int rk_nfc_write_page_raw(struct nand_chip *chip, const u8 *buf, > >> +  int oob_on, int page) > >> +{ > >> + struct mtd_info *mtd = nand_to_mtd(chip); > >> + struct rk_nfc *nfc = nand_get_controller_data(chip); > >> + > >> + rk_nfc_format_page(mtd, buf); > >> + return rk_nfc_write_page(mtd, chip, nfc->buffer, page, 1); > > > >I think you should avoid calling ->write_page. You will avoid an > >indentation level in this function and clarify what write_page_raw do. > >Same for read, and the _oob alternative. Also, I'm sure write_buf does > >not need to be exported and you can just move the actual code in this > >function. > > The code  is designed with reference to mtk_nand.c. > The function rk_nfc_format_page will copy data from extern buffer to nfc->buffer. > I will move the code in the function rk_nfc_format_page to rk_nfc_write_page_raw. > > 4. > >> +static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf, > >> +    int oob_on, int page) > >> +{ > >> + return rk_nfc_write_page(nand_to_mtd(chip), chip, buf, page, 0); > >> +} > > > >What is the purpose of this indirection? > > > > The function  rk_nfc_write_page also call by rk_nfc_write_page_raw, a parameter(raw) > is required to confirm whether ECC is used or not. Ok. Thanks, Miquèl From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_2 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5FE66C0044D for ; Mon, 16 Mar 2020 10:08:11 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 39DFD20663 for ; Mon, 16 Mar 2020 10:08:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730517AbgCPKIK convert rfc822-to-8bit (ORCPT ); Mon, 16 Mar 2020 06:08:10 -0400 Received: from relay3-d.mail.gandi.net ([217.70.183.195]:43161 "EHLO relay3-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730497AbgCPKIK (ORCPT ); Mon, 16 Mar 2020 06:08:10 -0400 X-Originating-IP: 90.89.41.158 Received: from xps13 (lfbn-tou-1-1473-158.w90-89.abo.wanadoo.fr [90.89.41.158]) (Authenticated sender: miquel.raynal@bootlin.com) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id B0FD26002D; Mon, 16 Mar 2020 10:06:58 +0000 (UTC) Date: Mon, 16 Mar 2020 11:06:58 +0100 From: Miquel Raynal To: =?UTF-8?B?6LW15Luq5bOw?= Cc: richard , vigneshr , robh+dt , devicetree , linux-mtd , =?UTF-8?B?SGVpa29TdMO8Ym5lcg==?= , linux-rockchip Subject: Re: [PATCH v3 1/3] mtd: rawnand: rockchip: NFC drivers for RK3308, RK3188 and others Message-ID: <20200316110658.43aea94a@xps13> In-Reply-To: <2020031617554207432140@rock-chips.com> References: <20200303094736.7490-1-yifeng.zhao@rock-chips.com> <20200303094736.7490-2-yifeng.zhao@rock-chips.com> <20200309121645.1fca069d@xps13> <2020031617554207432140@rock-chips.com> Organization: Bootlin X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: devicetree-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org Hi Yifeng, 赵仪峰 wrote on Mon, 16 Mar 2020 17:59:26 +0800: > Hi miquel， > > 1. > >A comment here explaining what the next function does and why would be > >nice. > > > >> +static void rk_nfc_format_page(struct mtd_info *mtd, const u8 *buf) > >> +{ > >> + struct nand_chip *chip = mtd_to_nand(mtd); > >> + struct rk_nfc *nfc = nand_get_controller_data(chip); > >> + u32 i; > >> + > > The data layout is different between NFC and nand  driver you probably mean between the NAND flash controller and whét the NAND core expects, but ok > This code is designed with reference to mtk_nand.c > There is a description of the data layout at the beginning of the file: >  * NFC Page Data Layout: >  * 1024 Bytes Data + 4Bytes sys data + 28Bytes~124Bytes ecc + >  * 1024 Bytes Data + 4Bytes sys data + 28Bytes~124Bytes ecc + >  * ...... >  * NAND Page Data Layout: >  * 1024 * n Data + m Bytes oob >  * Original Bad Block Mask Location: >  * first byte of oob(spare) >  * nand_chip->oob_poi data layout: >  * 4Bytes sys data + .... + 4Bytes sys data + ecc data > > 2.  > >> + dma_reg = DMA_ST | ((!rw) << DMA_WR)  | DMA_EN | (2 << DMA_AHB_SIZE) | > >> +      (7 << DMA_BURST_SIZE) | (16 << DMA_INC_NUM); > >> + > >> + fl_reg = (rw << FLCTL_WR) | FLCTL_XFER_EN | FLCTL_ACORRECT | > >> + (n_KB << FLCTL_XFER_SECTOR) | FLCTL_TOG_FIX; > >> + > >> + if (nfc->nfc_version == 6) { > > > >I would prefer using switch statements any time you check the version. > >The version should be an enum. > > > >You can also define a platform data structure for the register offsets > >that have the same name, but not necessarily the same offset. Then you > >can reference the right value directly. > >eg. > > > > struct rk_nfc_plat_data { > > u32 nfc_bchctl_off; > > ... > > }; > > > > struct rk_nfc_plat_data rk_nfc_v6_plat_data = { > > nfc_bchctl_off = ...; > > ... > > }; > > > > bch_reg = readl(pdata->nfc_bchctl_off); > > I will modify the code with switch and enum, but it is difficult to use platform data structure,  > because the bit offset inside the register is also different. it works the same with bitfields actually, if the bitfields have close names and behave the same (no matter where they are in registers), you should probably define them in a platform data structure as well. > #define NFC_BCH_ST_V6 (0x20) > #define NFC_BCH_ST_V9 (0x150) > #define BCH_ST_ERR0_V6 BIT(2) > #define BCH_ST_ERR1_V6 BIT(15) > #define BCH_ST_ERR0_V9 BIT(2) > #define BCH_ST_ERR1_V9 BIT(18) > #define ECC_ERR_CNT0_V6(x) (((((x) & (0x1F << 3)) >> 3) \ > | (((x) & (1 << 27)) >> 22)) & 0x3F) > #define ECC_ERR_CNT1_V6(x) (((((x) & (0x1F << 16)) >> 16) \ > | (((x) & (1 << 29)) >> 24)) & 0x3F) > #define ECC_ERR_CNT0_V9(x) (((x) & (0x7F << 3)) >> 3) > #define ECC_ERR_CNT1_V9(x) (((x) & (0x7F << 19)) >> 19) > > 3. > >> +static int rk_nfc_write_page_raw(struct nand_chip *chip, const u8 *buf, > >> +  int oob_on, int page) > >> +{ > >> + struct mtd_info *mtd = nand_to_mtd(chip); > >> + struct rk_nfc *nfc = nand_get_controller_data(chip); > >> + > >> + rk_nfc_format_page(mtd, buf); > >> + return rk_nfc_write_page(mtd, chip, nfc->buffer, page, 1); > > > >I think you should avoid calling ->write_page. You will avoid an > >indentation level in this function and clarify what write_page_raw do. > >Same for read, and the _oob alternative. Also, I'm sure write_buf does > >not need to be exported and you can just move the actual code in this > >function. > > The code  is designed with reference to mtk_nand.c. > The function rk_nfc_format_page will copy data from extern buffer to nfc->buffer. > I will move the code in the function rk_nfc_format_page to rk_nfc_write_page_raw. > > 4. > >> +static int rk_nfc_write_page_hwecc(struct nand_chip *chip, const u8 *buf, > >> +    int oob_on, int page) > >> +{ > >> + return rk_nfc_write_page(nand_to_mtd(chip), chip, buf, page, 0); > >> +} > > > >What is the purpose of this indirection? > > > > The function  rk_nfc_write_page also call by rk_nfc_write_page_raw, a parameter(raw) > is required to confirm whether ECC is used or not. Ok. Thanks, Miquèl