From: William Zhang <william.zhang@broadcom.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Broadcom Kernel List <bcm-kernel-feedback-list@broadcom.com>,
Linux MTD List <linux-mtd@lists.infradead.org>,
f.fainelli@gmail.com, rafal@milecki.pl, kursad.oney@broadcom.com,
joel.peshkin@broadcom.com, computersforpeace@gmail.com,
anand.gore@broadcom.com, dregan@mail.com,
kamal.dasu@broadcom.com, tomer.yacoby@broadcom.com,
dan.beygelman@broadcom.com, linux-kernel@vger.kernel.org,
Vignesh Raghavendra <vigneshr@ti.com>,
Richard Weinberger <richard@nod.at>,
Kamal Dasu <kdasu.kdev@gmail.com>,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 10/12] mtd: rawnand: brcmnand: Add BCMBCA read data bus interface
Date: Thu, 8 Jun 2023 12:04:23 -0700 [thread overview]
Message-ID: <57e2977a-7046-c49d-4bf4-0486d38a0f12@broadcom.com> (raw)
In-Reply-To: <20230608081512.52fa07fb@xps-13>
[-- Attachment #1: Type: text/plain, Size: 8741 bytes --]
On 06/07/2023 11:15 PM, Miquel Raynal wrote:
> Hi William,
>
> william.zhang@broadcom.com wrote on Wed, 7 Jun 2023 13:12:02 -0700:
>
>> Hi Miquel,
>>
>> On 06/07/2023 01:20 AM, Miquel Raynal wrote:
>>> Hi William,
>>>
>>> william.zhang@broadcom.com wrote on Tue, 6 Jun 2023 16:12:50 -0700:
>>>
>>>> The BCMBCA broadband SoC integrates the NAND controller differently than
>>>> STB, iProc and other SoCs. It has different endianness for NAND cache
>>>> data and ONFI parameter data.
>>>>
>>>> Add a SoC read data bus shim for BCMBCA to meet the specific SoC need
>>>> and performance improvement using the optimized memcpy function on NAND
>>>> cache memory.
>>>>
>>>> Signed-off-by: William Zhang <william.zhang@broadcom.com>
>>>> ---
>>>>
>>>> drivers/mtd/nand/raw/brcmnand/bcmbca_nand.c | 36 +++++++++++++++++
>>>> drivers/mtd/nand/raw/brcmnand/brcmnand.c | 44 ++++++++++++++-------
>>>> drivers/mtd/nand/raw/brcmnand/brcmnand.h | 2 +
>>>> 3 files changed, 68 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/raw/brcmnand/bcmbca_nand.c b/drivers/mtd/nand/raw/brcmnand/bcmbca_nand.c
>>>> index 7e48b6a0bfa2..899103a62c98 100644
>>>> --- a/drivers/mtd/nand/raw/brcmnand/bcmbca_nand.c
>>>> +++ b/drivers/mtd/nand/raw/brcmnand/bcmbca_nand.c
>>>> @@ -26,6 +26,18 @@ enum {
>>>> BCMBCA_CTLRDY = BIT(4),
>>>> };
>>>> >> +#if defined(CONFIG_ARM64)
>>>> +#define ALIGN_REQ 8
>>>> +#else
>>>> +#define ALIGN_REQ 4
>>>> +#endif
>>>> +
>>>> +static inline bool bcmbca_nand_is_buf_aligned(void *flash_cache, void *buffer)
>>>> +{
>>>> + return IS_ALIGNED((uintptr_t)buffer, ALIGN_REQ) &&
>>>> + IS_ALIGNED((uintptr_t)flash_cache, ALIGN_REQ);
>>>> +}
>>>> +
>>>> static bool bcmbca_nand_intc_ack(struct brcmnand_soc *soc)
>>>> {
>>>> struct bcmbca_nand_soc *priv =
>>>> @@ -56,6 +68,29 @@ static void bcmbca_nand_intc_set(struct brcmnand_soc *soc, bool en)
>>>> brcmnand_writel(val, mmio);
>>>> }
>>>> >> +static void bcmbca_read_data_bus(struct brcmnand_soc *soc,
>>>> + void __iomem *flash_cache, u32 *buffer,
>>>> + int fc_words, bool is_param)
>>>> +{
>>>> + int i;
>>>> +
>>>> + if (!is_param) {
>>>> + /*
>>>> + * memcpy can do unaligned aligned access depending on source
>>>> + * and dest address, which is incompatible with nand cache. Fallback
>>>> + * to the memcpy for io version
>>>> + */
>>>> + if (bcmbca_nand_is_buf_aligned(flash_cache, buffer))
>>>> + memcpy((void *)buffer, (void *)flash_cache, fc_words * 4);
>>>> + else
>>>> + memcpy_fromio((void *)buffer, (void *)flash_cache, fc_words * 4);
>>>> + } else {
>>>> + /* Flash cache has same endian as the host for parameter pages */
>>>> + for (i = 0; i < fc_words; i++, buffer++)
>>>> + *buffer = __raw_readl(flash_cache + i * 4);
>>>> + }
>>>> +}
>>>> +
>>>> static int bcmbca_nand_probe(struct platform_device *pdev)
>>>> {
>>>> struct device *dev = &pdev->dev;
>>>> @@ -75,6 +110,7 @@ static int bcmbca_nand_probe(struct platform_device *pdev)
>>>> >> soc->ctlrdy_ack = bcmbca_nand_intc_ack;
>>>> soc->ctlrdy_set_enabled = bcmbca_nand_intc_set;
>>>> + soc->read_data_bus = bcmbca_read_data_bus;
>>>> >> return brcmnand_probe(pdev, soc);
>>>> }
>>>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>>>> index d920e88c7f5b..656be4d73016 100644
>>>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>>>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>>>> @@ -814,6 +814,30 @@ static inline u32 edu_readl(struct brcmnand_controller *ctrl,
>>>> return brcmnand_readl(ctrl->edu_base + offs);
>>>> }
>>>> >> +static inline void brcmnand_read_data_bus(struct brcmnand_controller *ctrl,
>>>> + void __iomem *flash_cache, u32 *buffer,
>>>> + int fc_words, bool is_param)
>>>
>>> I strongly dislike this "is_param" boolean.
>>>
>>> When is the data in host endianness? When is it not?
>> This is little bit complicated. We have two type data read from nand cache. One for page read and the other for parameter and onfi data read from the controller side. But it depends on how SoC integrate the nand cache to system. In broadband SoC, both page and parameter data are in host endianess but other SoCs is not the same.
>>
>> I am open to suggestion for is_param function argument but to factor out this common code in more structured way, I don't see other way around.
>
> Alright, so this is SoC dependent, very well -> a (sub)compatible per
> SoC + platform data associated to it with the right function.
>
Right we have per SoC compatible and can have per SoC implementation but
I prefer to have a default implementation in the brcmnand.c because
right now only bcmcba SoC need some different handling. The other four
implementations are the same.
To make the code a little more readable and less complicated, I am
thinking to separate the brcmnand_read_data_bus into
brcmnand_read_page_data and brcmnand_read_param_data as default in
brcmnand.c. But bcmbca will override them. Would that be okay with you?
>>> If we think about an exec_op() conversion and drop cmdfunc(), what
>>> would be the discriminant?
>>>
>> If we need to implement exec_op in the future, the data is not coming from nand cache but some other low level data register which may not subject to the endianess issue.
>
> Can't you use the same cache all the time here as well then? And avoid
> the need for this overly complex logic?
>
Unfortunately exec_op will not use nand cache for parameter data read
but some other low level data register. This is dictated by the controller.
>>
>>>> +{
>>>> + struct brcmnand_soc *soc = ctrl->soc;
>>>> + int i;
>>>> +
>>>> + if (soc->read_data_bus) {
>>>> + soc->read_data_bus(soc, flash_cache, buffer, fc_words, is_param);
>>>> + } else {
>>>> + if (!is_param) {
>>>> + for (i = 0; i < fc_words; i++, buffer++)
>>>> + *buffer = brcmnand_read_fc(ctrl, i);
>>>> + } else {
>>>> + for (i = 0; i < fc_words; i++)
>>>> + /*
>>>> + * Flash cache is big endian for parameter pages, at
>>>> + * least on STB SoCs
>>>> + */
>>>> + buffer[i] = be32_to_cpu(brcmnand_read_fc(ctrl, i));
>>>> + }
>>>> + }
>>>> +}
>>>> +
>>>> static void brcmnand_clear_ecc_addr(struct brcmnand_controller *ctrl)
>>>> {
>>>> >> @@ -1811,20 +1835,11 @@ static void brcmnand_cmdfunc(struct nand_chip *chip, unsigned command,
>>>> native_cmd == CMD_PARAMETER_CHANGE_COL) {
>>>> /* Copy flash cache word-wise */
>>>> u32 *flash_cache = (u32 *)ctrl->flash_cache;
>>>> - int i;
>>>> >> brcmnand_soc_data_bus_prepare(ctrl->soc, true);
>>>> >> - /*
>>>> - * Must cache the FLASH_CACHE now, since changes in
>>>> - * SECTOR_SIZE_1K may invalidate it
>>>> - */
>>>> - for (i = 0; i < FC_WORDS; i++)
>>>> - /*
>>>> - * Flash cache is big endian for parameter pages, at
>>>> - * least on STB SoCs
>>>> - */
>>>> - flash_cache[i] = be32_to_cpu(brcmnand_read_fc(ctrl, i));
>>>> + brcmnand_read_data_bus(ctrl, ctrl->nand_fc, flash_cache,
>>>> + FC_WORDS, true);
>>>> >> brcmnand_soc_data_bus_unprepare(ctrl->soc, true);
>>>> >> @@ -2137,7 +2152,7 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
>>>> {
>>>> struct brcmnand_host *host = nand_get_controller_data(chip);
>>>> struct brcmnand_controller *ctrl = host->ctrl;
>>>> - int i, j, ret = 0;
>>>> + int i, ret = 0;
>>>> >> brcmnand_clear_ecc_addr(ctrl);
>>>> >> @@ -2150,8 +2165,9 @@ static int brcmnand_read_by_pio(struct mtd_info *mtd, struct nand_chip *chip,
>>>> if (likely(buf)) {
>>>> brcmnand_soc_data_bus_prepare(ctrl->soc, false);
>>>> >> - for (j = 0; j < FC_WORDS; j++, buf++)
>>>> - *buf = brcmnand_read_fc(ctrl, j);
>>>> + brcmnand_read_data_bus(ctrl, ctrl->nand_fc, buf,
>>>> + FC_WORDS, false);
>>>> + buf += FC_WORDS;
>>>> >> brcmnand_soc_data_bus_unprepare(ctrl->soc, false);
>>>> }
>>>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.h b/drivers/mtd/nand/raw/brcmnand/brcmnand.h
>>>> index f1f93d85f50d..88819bc395f8 100644
>>>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.h
>>>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.h
>>>> @@ -24,6 +24,8 @@ struct brcmnand_soc {
>>>> void (*ctlrdy_set_enabled)(struct brcmnand_soc *soc, bool en);
>>>> void (*prepare_data_bus)(struct brcmnand_soc *soc, bool prepare,
>>>> bool is_param);
>>>> + void (*read_data_bus)(struct brcmnand_soc *soc, void __iomem *flash_cache,
>>>> + u32 *buffer, int fc_words, bool is_param);
>>>> const struct brcmnand_io_ops *ops;
>>>> };
>>>> > >
>>> Thanks,
>>> Miquèl
>>>
>
>
> Thanks,
> Miquèl
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]
next prev parent reply other threads:[~2023-06-08 19:04 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-06 23:12 [PATCH 00/12] mtd: rawnand: brcmnand: driver and doc updates William Zhang
2023-06-06 23:12 ` [PATCH 01/12] mtd: rawnand: brcmnand: Fix ECC level field setting for v7.2 controller William Zhang
2023-06-07 8:06 ` Miquel Raynal
2023-06-06 23:12 ` [PATCH 02/12] mtd: rawnand: brcmnand: Fix potential false time out warning William Zhang
2023-06-06 23:12 ` [PATCH 03/12] mtd: rawnand: brcmnand: Fix crash during the panic_write William Zhang
2023-06-06 23:12 ` [PATCH 04/12] mtd: rawnand: brcmnand: Fix potential out-of-bounds access in oob write William Zhang
2023-06-07 8:09 ` Miquel Raynal
2023-06-06 23:12 ` [PATCH 05/12] dt-bindings: mtd: brcmnand: Updates for bcmbca SoCs William Zhang
2023-06-07 8:14 ` Miquel Raynal
2023-06-07 20:01 ` William Zhang
2023-06-09 8:58 ` Miquel Raynal
2023-06-09 19:05 ` William Zhang
2023-06-12 17:43 ` Miquel Raynal
2023-06-14 22:46 ` Rob Herring
2023-06-15 0:40 ` William Zhang
2023-06-14 22:43 ` Rob Herring
2023-06-15 0:28 ` William Zhang
2023-06-06 23:12 ` [PATCH 06/12] ARM: dts: broadcom: bcmbca: Add NAND controller node William Zhang
2023-06-06 23:12 ` [PATCH 07/12] arm64: " William Zhang
2023-06-06 23:12 ` [PATCH 08/12] mtd: rawnand: brcmnand: Rename bcm63138 nand driver William Zhang
2023-06-06 23:12 ` [PATCH 09/12] mtd: rawnand: brcmnand: Add new compatible string William Zhang
2023-06-06 23:12 ` [PATCH 10/12] mtd: rawnand: brcmnand: Add BCMBCA read data bus interface William Zhang
2023-06-07 8:20 ` Miquel Raynal
2023-06-07 20:12 ` William Zhang
2023-06-08 6:15 ` Miquel Raynal
2023-06-08 19:04 ` William Zhang [this message]
2023-06-07 8:22 ` Miquel Raynal
2023-06-07 20:24 ` William Zhang
2023-06-08 6:18 ` Miquel Raynal
2023-06-08 19:10 ` William Zhang
2023-06-09 8:35 ` Miquel Raynal
2023-06-09 19:16 ` William Zhang
2023-06-12 17:49 ` Miquel Raynal
2023-06-12 17:53 ` Miquel Raynal
2023-06-12 19:18 ` William Zhang
2023-06-13 6:42 ` Miquel Raynal
2023-06-14 0:00 ` William Zhang
2023-06-14 6:22 ` Miquel Raynal
2023-06-14 23:52 ` William Zhang
2023-06-12 19:03 ` William Zhang
2023-06-11 9:54 ` kernel test robot
2023-06-06 23:12 ` [PATCH 11/12] mtd: rawnand: brcmnand: Add support for getting ecc setting from strap William Zhang
2023-06-07 8:25 ` Miquel Raynal
2023-06-06 23:12 ` [PATCH 12/12] mtd: rawnand: brcmnand: Support write protection setting from dts William Zhang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=57e2977a-7046-c49d-4bf4-0486d38a0f12@broadcom.com \
--to=william.zhang@broadcom.com \
--cc=anand.gore@broadcom.com \
--cc=bcm-kernel-feedback-list@broadcom.com \
--cc=computersforpeace@gmail.com \
--cc=dan.beygelman@broadcom.com \
--cc=dregan@mail.com \
--cc=f.fainelli@gmail.com \
--cc=joel.peshkin@broadcom.com \
--cc=kamal.dasu@broadcom.com \
--cc=kdasu.kdev@gmail.com \
--cc=kursad.oney@broadcom.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=miquel.raynal@bootlin.com \
--cc=rafal@milecki.pl \
--cc=richard@nod.at \
--cc=tomer.yacoby@broadcom.com \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).