Received: by 2002:a05:7412:da14:b0:e2:908c:2ebd with SMTP id fe20csp215412rdb; Fri, 6 Oct 2023 00:50:33 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFwxg3ahC3iVY49OElIeIua44SHntQW3Nyx7Hy3I3KecCDQVR4XzVzir+eriMKttZidP5SL X-Received: by 2002:a17:90b:4c92:b0:268:93d:b936 with SMTP id my18-20020a17090b4c9200b00268093db936mr6624627pjb.18.1696578633513; Fri, 06 Oct 2023 00:50:33 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1696578633; cv=none; d=google.com; s=arc-20160816; b=eoxB3vE4c6Y+17d+N573k6HWNtPYc/b9u2m96/TllMYgB6ZssAQGNRRrn4x0A+bH6z dfVu5BIeCj+DFUEQn0ylW/2cN0ISA/02KPgyIKn1rMCKRohyhB7fYZHQuNVAj6TjOWxu 1whMdwQs4Bub6iIGOQ6jRnQI47MsGYZDlX1iAeQuGj2Gcgo601jcqvFSSS7eY6YVqCXN UQRyustYJTis41PxdW2KHoR7A3F1jVdmOcApH24MbY84iYLHambPnhmGfsDw1F2vXg2t lfOlCeLz9E7CYEfdAC8hgez0W+XXO5NACRGGZYj25fQuT1qsO59V4zgBAaQy/ZS70Sbw rx3w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :organization:references:in-reply-to:message-id:subject:cc:to:from :date:dkim-signature; bh=KQFwrUeYJEDlw/TSLgMcUD55L/pInUxyhSLEs5AXqms=; fh=lKJG+6zrTZ7VDOkYdx/+4KawXfaON93FXBXHrKNfc9g=; b=Vn2eRu2ljEkhU7YBtyfGpIJYfNRSmFLYE2jaZyFgDeLHNFzRtJRmruyZqXEpqNifln PSLek1YKot1JmhdTuDxx8m8oU3t+qdtGrtMDsZWXn4eKHlYCRhgHD8IutkP7E4y6/uL0 kGJe+165wSWvajeoKw0VEzB5bhtpLWMdu+TEPY14pzOZVKi76L0UeWzwVJKsZg6mOsdE fgDO3btLqOXNh1/iCtfgNJTfuaQYQxHgV9vHSSmX2+lIiJz7ZXMfENek5DQKmfSAFLCm PORJAOnjy+t2OU6JhNvwEwjUPjwgF7UKC0CYASIM7aWRVKBhonIVWoZS7FvhQD33vmrZ Xdrw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@bootlin.com header.s=gm1 header.b=VJLR3AQC; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=bootlin.com Return-Path: Received: from morse.vger.email (morse.vger.email. [23.128.96.31]) by mx.google.com with ESMTPS id c12-20020a170902724c00b001c4401a7e18si3044535pll.382.2023.10.06.00.50.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 06 Oct 2023 00:50:33 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31; Authentication-Results: mx.google.com; dkim=pass header.i=@bootlin.com header.s=gm1 header.b=VJLR3AQC; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=bootlin.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by morse.vger.email (Postfix) with ESMTP id B15638065338; Fri, 6 Oct 2023 00:50:30 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at morse.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230406AbjJFHuV (ORCPT + 99 others); Fri, 6 Oct 2023 03:50:21 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45338 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230384AbjJFHuN (ORCPT ); Fri, 6 Oct 2023 03:50:13 -0400 Received: from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net [217.70.183.197]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 84A66FC for ; Fri, 6 Oct 2023 00:42:51 -0700 (PDT) Received: by mail.gandi.net (Postfix) with ESMTPSA id 3A4A91C0008; Fri, 6 Oct 2023 07:42:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1696578170; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=KQFwrUeYJEDlw/TSLgMcUD55L/pInUxyhSLEs5AXqms=; b=VJLR3AQC9+1xKQjLSwdQ3raBHu845I2YoP2u6Ky5XHpnYjv9gz2yDZOTA2zeNg2Ha+mlSy nn9CdZ7cU8OXPxk9ga5Iw8dH+anBW82dAHFHiS9fCWJ5dUe0zFXn6DMCNN4s/H1loT+RKo vk1rOFzpcUJ9X6AC+JxF7TvGKfAl5RIpvgdf5bIXvkqj5U7fcWz0CvutHgQp9ZLzzUb9ad 0+ZmBBpPlUS4W+xepzpoDXTcUk7oCg4hoif52eV7dDNo01pkzoA6a1dqU2Fk1US/dvykxX SY2xAMQbak5GwswsSADneVcE8RN3bSNS5o4tvsYSxMB7DCnbBm7sM5XAcuI+ng== Date: Fri, 6 Oct 2023 09:42:45 +0200 From: Miquel Raynal To: William Zhang Cc: dregan@mail.com, bcm-kernel-feedback-list@broadcom.com, linux-mtd@lists.infradead.org, f.fainelli@gmail.com, rafal@milecki.pl, joel.peshkin@broadcom.com, computersforpeace@gmail.com, dan.beygelman@broadcom.com, frieder.schrempf@kontron.de, linux-kernel@vger.kernel.org, vigneshr@ti.com, richard@nod.at, bbrezillon@kernel.org, kdasu.kdev@gmail.com Subject: Re: [PATCH v2] mtd: rawnand: brcmnand: Initial exec_op implementation Message-ID: <20231006094245.6cc24e03@xps-13> In-Reply-To: <0c72dbb4-8ee5-c9b0-b029-9689e9d72f5e@broadcom.com> References: <20230922162424.4a7b27ec@xps-13> <20231002143527.4ccf254a@xps-13> <04350e70-6ef0-4998-664f-20b96b63b0f4@broadcom.com> <20231003112819.53707d54@xps-13> <37416f2e-f150-cc8f-76bd-3d54f9e25d08@broadcom.com> <20231004005525.3f406823@xps-13> <0f6bd61c-19ed-8153-1763-eef2498726c7@broadcom.com> <0c72dbb4-8ee5-c9b0-b029-9689e9d72f5e@broadcom.com> Organization: Bootlin X-Mailer: Claws Mail 4.0.0 (GTK+ 3.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Sasl: miquel.raynal@bootlin.com X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on morse.vger.email Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (morse.vger.email [0.0.0.0]); Fri, 06 Oct 2023 00:50:30 -0700 (PDT) Hi William, william.zhang@broadcom.com wrote on Thu, 5 Oct 2023 17:42:21 -0700: > Hi Miquel, >=20 > On 10/03/2023 09:47 PM, William Zhang wrote: > >=20 > >=20 > > On 10/03/2023 03:55 PM, Miquel Raynal wrote: =20 > >> Hi William, > >> > >> william.zhang@broadcom.com wrote on Tue, 3 Oct 2023 11:46:25 -0700: > >> =20 > >>> Hi Miquel, > >>> > >>> On 10/03/2023 02:28 AM, Miquel Raynal wrote: =20 > >>>> Hi William, > >>>> > >>>> william.zhang@broadcom.com wrote on Mon, 2 Oct 2023 12:57:01 -0700: = =20 > >>>>> Hi Miquel, > >>>>> > >>>>> On 10/02/2023 05:35 AM, Miquel Raynal wrote: =20 > >>>>>> Hi David, > >>>>>> > >>>>>> dregan@mail.com wrote on Sat, 30 Sep 2023 03:57:35 +0200: > >>>>>> =C2=A0=C2=A0=C2=A0 >>>> Initial exec_op implementation for Broadco= m STB, >>>>>> Broadband and iProc SoC =20 > >>>>>>> This adds exec_op and removes the legacy interface. > >>>>>>> > >>>>>>> Signed-off-by: David Regan > >>>>>>> Reviewed-by: William Zhang > >>>>>>> > >>>>>>> --- > >>>>>>> =C2=A0=C2=A0 >>> =20 > >>>>>> ... > >>>>>> =C2=A0=C2=A0=C2=A0 >>>> +static int brcmnand_parser_exec_matched_o= p(struct >>>>>> nand_chip *chip, =20 > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const struct nand= _subop *subop) > >>>>>>> +{ > >>>>>>> +=C2=A0=C2=A0=C2=A0 struct brcmnand_host *host =3D nand_get_contr= oller_data(chip); > >>>>>>> +=C2=A0=C2=A0=C2=A0 struct brcmnand_controller *ctrl =3D host->ct= rl; > >>>>>>> +=C2=A0=C2=A0=C2=A0 struct mtd_info *mtd =3D nand_to_mtd(chip); > >>>>>>> +=C2=A0=C2=A0=C2=A0 const struct nand_op_instr *instr =3D &subop-= >instrs[0]; > >>>>>>> +=C2=A0=C2=A0=C2=A0 unsigned int i; > >>>>>>> +=C2=A0=C2=A0=C2=A0 int ret =3D 0; > >>>>>>> + > >>>>>>> +=C2=A0=C2=A0=C2=A0 for (i =3D 0; i < subop->ninstrs; i++) { > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 instr =3D &subop->ins= trs[i]; > >>>>>>> + > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if ((instr->type =3D= =3D NAND_OP_CMD_INSTR) && > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 (instr->ctx.cmd.opcode =3D=3D NAND_CMD_STATUS)) > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 ctrl->status_cmd =3D 1; > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 else if (ctrl->status= _cmd && (instr->type =3D=3D >>>>>>> NAND_OP_DATA_IN_INSTR)) { > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 /* > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 * need to fake the nand device write protect >>>>>>> because nand= _base does a > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 * nand_check_wp which calls nand_status_op >>>>>>> NAND_CMD_STATU= S which checks > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 * that the nand is not write protected before an >>>>>>> operatio= n starts. > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 * The problem with this is it's done outside >>>>>>> exec_op so t= he nand is > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 * write protected and this check will fail until >>>>>>> the writ= e or erase > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 * or write back operation actually happens where we >>>>>>> turn = off wp. > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 */ > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 u8 *in; > >>>>>>> + > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 ctrl->status_cmd =3D 0; > >>>>>>> + > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 instr =3D &subop->instrs[i]; > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 in =3D instr->ctx.data.buf.in; > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 in[0] =3D brcmnand_status(host) | NAND_STATUS_WP; /* >>>>>>> hide WP st= atus */ =20 > >>>>>> > >>>>>> I don't understand why you are faking the WP bit. If it's set, > >>>>>> brcmnand_status() should return it and you should not care about >= >>>>> it. If > >>>>>> it's not however, can you please give me the path used when we have > >>>>>> this issue? Either we need to modify the core or we need to provide > >>>>>> additional helpers in this driver to circumvent the faulty path. = =20 > >>>>> > >>>>> The reason we have to hide wp status for status command is because > >>>>> nand_base calls nand_check_wp at the very beginning of write and er= ase > >>>>> function. This applies to both exec_op path and legacy path. With > >>>>> Broadcom nand controller and most of our board design using the WP = pin > >>>>> and have it asserted by default, the nand_check_wp function will fa= il > >>>>> and write/erase aborts.=C2=A0 This workaround has been there before= this > >>>>> exec_op patch. > >>>>> > >>>>> I agree it is ugly and better to be addressed in the nand base >>>>= > code. And > >>>>> I understand Broadcom's WP approach may sound a bit over cautious >= >>>> but we > >>>>> want to make sure no spurious erase/write can happen under any > >>>>> circumstance except software explicitly want to write and erase. >= >>>> WP is > >>>>> standard nand chip pin and I think most the nand controller has that > >>>>> that pin in the design too but it is possible it is not used and > >>>>> bootloader can de-assert the pin and have a always-writable nand fl= ash > >>>>> for linux. So maybe we can add nand controller dts option >>>>> "na= nd-use-wp". > >>>>> If this property exist and set to 1,=C2=A0 wp control is in use and= nand > >>>>> driver need to control the pin on/ff as needed when doing write and > >>>>> erase function. Also nand base code should not call nand_check_wp w= hen > >>>>> wp is in use. Then we can remove the faking WP status workaround. = =20 > >>>>>> =C2=A0=C2=A0=C2=A0 >>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 } else if (instr->type =3D=3D NAND_OP_WAITRDY_INSTR) { =20 > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 ret =3D bcmnand_ctrl_poll_status(host, NAND_CTRL_RDY, >>>>>>> NAND_CTRL= _RDY, 0); > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 if (ctrl->wp_cmd) { > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 ctrl->wp_cmd =3D 0; > >>>>>>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0 brcmnand_wp(mtd, 1); =20 > >>>>>> > >>>>>> This ideally should disappear. > >>>>>> =C2=A0=C2=A0=C2=A0 >> Maybe we can have the destructive operation = patch from Borris. =20 > >>>>> Controller driver still need to assert/deassert the pin if it uses = >>>>> nand > >>>>> wp feature but at least it does not need to guess the op code. =20 > >>>> > >>>> Ah, yeah, I get it. > >>>> > >>>> Please be my guest, you can revive this patch series (might need lig= ht > >>>> tweaking, nothing big) and also take inspiration from it if necessar= y: > >>>> https://github.com/bbrezillon/linux/commit/e612e1f2c69a33ac5f2c91d13= 669f0f172d58717 >>>> > >>>> https://github.com/bbrezillon/linux/commit/4ec6f8d8d83f5aaca5d1877f0= 2d48da96d41fcba >>>> > >>>> https://github.com/bbrezillon/linux/commit/11b4acffd761c4928652d7028= d19fcd6f45e4696 >>>> =20 > >>> Sure we will incorporate the destructive operation patch and provide a > >>> new revision. > >>> > >>> The WP status workaround will stay at least for this change. If you > >>> think my suggestion using a dts setting above is okay, we can provide= a > >>> patch for that as well.=C2=A0 Or if you have any other idea or sugges= tion, > >>> we'd like to hear too. =20 > >> > >> I thought this was not needed as Boris initial conversion did not need > >> it. The goal is to get rid of this workaround. > >> Boris' initial patch did remove that workaround but it will break the = =20 > > board that uses WP pin because the nand_check_wp run before the exec_op= > and status returned is write-protected in the erase and write function. > > I explained that above and you can see the code here: > > https://elixir.bootlin.com/linux/v6.6-rc4/source/drivers/mtd/nand/raw/n= and_base.c#L4599 > >=20 > > I agree with your goal to remove this workaround and we have suggested > > one possible fix but we are also open to any other solution. > > =20 > We have integrated the destructive operation patch and are ready for the > v3. If you don't think my proposal on the WP status fix is a good idea, > can we get this exce_op conversion patch series going first? After all, > we don't modify the WP status handling behavior in this patch. We can > fix it in another patch whenever we agree on a solution. Please let me > know and thanks a lot for all your comments and thoughts. The NAND core has been a playground for coding horrors sometimes, and this ->exec_op() conversion is us the way to a cleaner and mastered approach, I am not willing to let something that obvious get in, I'm sorry. For you it's just a workaround, for me it means any change in the core will just break with this controller. This is of course not against you or your work, perhaps I should emphasize that I strongly appreciate your efforts and, besides this workaround the code is clean. The problem is that the WP pin can be used in two different ways: internally and externally. When it's used externally, you expect it to be deasserted before you start a destructive operation. When you use it internally, you expect it to be deasserted during the destructive operation. The final solution needs to be approved by comparing with similar drivers which perform this internal procedure themselves as well. Maybe we could add a flag somewhere in the core's controller structure to tell the core not to perform these checks as we master the handling of the WP pin, telling the controller will handle it correctly as long as the destructive flag is passed. Thanks, Miqu=C3=A8l