Re: [PATCH v3] mtd: rawnand: brcmnand: legacy exec_op implementation

Florian Fainelli
2025-05-15T07:55:02Z

On 5/15/2025 7:06 AM, Álvaro Fernández Rojas wrote:
> Hi William
>
> El jue, 15 may 2025 a las 3:42, William Zhang
> (<william.zhang@broadcom.com>) escribió:
>>
>> Hi Alvaro,
>>
>>> -----Original Message-----
>>> From: Álvaro Fernández Rojas <noltari@gmail.com>
>>> Sent: Tuesday, May 13, 2025 11:24 PM
>>> To: linux-mtd@lists.infradead.org; dregan@broadcom.com;
>>> miquel.raynal@bootlin.com; bcm-kernel-feedback-list@broadcom.com;
>>> florian.fainelli@broadcom.com; rafal@milecki.pl;
>>> computersforpeace@gmail.com; kamal.dasu@broadcom.com;
>>> dan.beygelman@broadcom.com; william.zhang@broadcom.com;
>>> frieder.schrempf@kontron.de; linux-kernel@vger.kernel.org;
>>> vigneshr@ti.com;
>>> richard@nod.at; bbrezillon@kernel.org; kdasu.kdev@gmail.com;
>>> jaimeliao.tw@gmail.com; kilobyte@angband.pl; jonas.gorski@gmail.com;
>>> dgcbueu@gmail.com
>>> Cc: Álvaro Fernández Rojas <noltari@gmail.com>
>>> Subject: [PATCH v3] mtd: rawnand: brcmnand: legacy exec_op implementation
>>>
>>> Commit 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation")
>>> removed legacy interface functions, breaking < v5.0 controllers support.
>>> In order to fix older controllers we need to add an alternative exec_op
>>> implementation which doesn't rely on low level registers.
>>>
>>> Fixes: 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation")
>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>>> ---
>>> drivers/mtd/nand/raw/brcmnand/brcmnand.c | 178
>>> ++++++++++++++++++++++-
>>> 1 file changed, 172 insertions(+), 6 deletions(-)
>>>
>>> v3: add changes requested by Florian and other improvements:
>>> - Add associative array for native command conversion.
>>> - Add function pointer to brcmnand_controller for exec_instr
>>> functionality.
>>> - Fix CMD_BLOCK_ERASE address.
>>> - Drop NAND_CMD_READOOB support.
>>>
>>> v2: multiple improvements:
>>> - Use proper native commands for checks.
>>> - Fix NAND_CMD_PARAM/NAND_CMD_RNDOUT addr calculation.
>>> - Remove host->last_addr usage.
>>> - Remove sector_size_1k since it only applies to v5.0+ controllers.
>>> - Remove brcmnand_wp since it doesn't exist for < v5.0 controllers.
>>> - Use j instead of i for flash_cache loop.
>>>
>>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>>> b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>>> index 17f6d9723df9..f4fabe7ffd9d 100644
>>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>>> @@ -65,6 +65,7 @@ module_param(wp_on, int, 0444);
>>> #define CMD_PARAMETER_READ 0x0e
>>> #define CMD_PARAMETER_CHANGE_COL 0x0f
>>> #define CMD_LOW_LEVEL_OP 0x10
>>> +#define CMD_NOT_SUPPORTED 0xff
>>>
>>> struct brcm_nand_dma_desc {
>>> u32 next_desc;
>>> @@ -199,6 +200,30 @@ static const u16 flash_dma_regs_v4[] = {
>>> [FLASH_DMA_CURRENT_DESC_EXT] = 0x34,
>>> };
>>>
>>> +/* Native command conversion */
>>> +static const u8 native_cmd_conv[] = {
>>> + [NAND_CMD_READ0] = CMD_NOT_SUPPORTED,
>>> + [NAND_CMD_READ1] = CMD_NOT_SUPPORTED,
>>> + [NAND_CMD_RNDOUT] = CMD_PARAMETER_CHANGE_COL,
>>> + [NAND_CMD_PAGEPROG] = CMD_NOT_SUPPORTED,
>>> + [NAND_CMD_READOOB] = CMD_NOT_SUPPORTED,
>>> + [NAND_CMD_ERASE1] = CMD_BLOCK_ERASE,
>>> + [NAND_CMD_STATUS] = CMD_NOT_SUPPORTED,
>> Do we not need to support nand_status_op()?
>
> We do, but NAND_CMD_STATUS and NAND_CMD_RESET are handled in brcmnand_exec_op():
> https://github.com/torvalds/linux/blob/546bce579204685a0b204beebab98c3aa496e651/drivers/mtd/nand/raw/brcmnand/brcmnand.c#L2506-L2523
>
>
>
>>
>>> + [NAND_CMD_SEQIN] = CMD_NOT_SUPPORTED,
>>> + [NAND_CMD_RNDIN] = CMD_NOT_SUPPORTED,
>>> + [NAND_CMD_READID] = CMD_DEVICE_ID_READ,
>>> + [NAND_CMD_ERASE2] = CMD_NULL,
>>> + [NAND_CMD_PARAM] = CMD_PARAMETER_READ,
>>> + [NAND_CMD_GET_FEATURES] = CMD_NOT_SUPPORTED,
>>> + [NAND_CMD_SET_FEATURES] = CMD_NOT_SUPPORTED,
>>> + [NAND_CMD_RESET] = CMD_NOT_SUPPORTED,
>>> + [NAND_CMD_READSTART] = CMD_NOT_SUPPORTED,
>>> + [NAND_CMD_READCACHESEQ] = CMD_NOT_SUPPORTED,
>>> + [NAND_CMD_READCACHEEND] = CMD_NOT_SUPPORTED,
>>> + [NAND_CMD_RNDOUTSTART] = CMD_NULL,
>>> + [NAND_CMD_CACHEDPROG] = CMD_NOT_SUPPORTED,
>>> +};
>>> +
>>> /* Controller feature flags */
>>> enum {
>>> BRCMNAND_HAS_1K_SECTORS = BIT(0),
>>> @@ -237,6 +262,10 @@ struct brcmnand_controller {
>>> /* List of NAND hosts (one for each chip-select) */
>>> struct list_head host_list;
>>>
>>> + /* Function to be called from exec_op */
>>> + int (*exec_instr)(struct nand_chip *chip,
>>> + const struct nand_operation *op);
>>> +
>>> /* EDU info, per-transaction */
>>> const u16 *edu_offsets;
>>> void __iomem *edu_base;
>>> @@ -2490,14 +2519,149 @@ static int brcmnand_op_is_reset(const struct
>>> nand_operation *op)
>>> return 0;
>>> }
>>>
>>> +static int brcmnand_exec_instructions(struct nand_chip *chip,
>>> + const struct nand_operation *op)
>>> +{
>>> + struct brcmnand_host *host = nand_get_controller_data(chip);
>>> + unsigned int i;
>>> + int ret = 0;
>>> +
>>> + for (i = 0; i < op->ninstrs; i++) {
>>> + ret = brcmnand_exec_instr(host, i, op);
>>> + if (ret)
>>> + break;
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int brcmnand_exec_instructions_legacy(struct nand_chip *chip,
>>> + const struct nand_operation *op)
>>> +{
>>> + struct mtd_info *mtd = nand_to_mtd(chip);
>>> + struct brcmnand_host *host = nand_get_controller_data(chip);
>>> + struct brcmnand_controller *ctrl = host->ctrl;
>>> + const struct nand_op_instr *instr;
>>> + unsigned int i, j;
>>> + u8 cmd = CMD_NULL, last_cmd = CMD_NULL;
>>> + int ret = 0;
>>> + u64 last_addr;
>>> +
>>> + for (i = 0; i < op->ninstrs; i++) {
>>> + instr = &op->instrs[i];
>>> +
>>> + if (instr->type == NAND_OP_CMD_INSTR) {
>>> + cmd = native_cmd_conv[instr->ctx.cmd.opcode];
>>> + if (cmd == CMD_NOT_SUPPORTED) {
>>> + dev_err(ctrl->dev, "unsupported cmd=%d\n",
>>> + instr->ctx.cmd.opcode);
>>> + ret = -EOPNOTSUPP;
>>> + break;
>>> + }
>>> + } else if (instr->type == NAND_OP_ADDR_INSTR) {
>>> + u64 addr = 0;
>>> +
>>> + if (cmd == CMD_NULL)
>>> + continue;
>>> +
>>> + if (instr->ctx.addr.naddrs > 8) {
>>> + dev_err(ctrl->dev, "unsupported naddrs=%u\n",
>>> + instr->ctx.addr.naddrs);
>>> + ret = -EOPNOTSUPP;
>>> + break;
>>> + }
>>> +
>>> + for (j = 0; j < instr->ctx.addr.naddrs; j++)
>>> + addr |= (instr->ctx.addr.addrs[j]) << (j << 3);
>>> +
>>> + if (cmd == CMD_BLOCK_ERASE)
>>> + addr <<= chip->page_shift;
>>> + else if (cmd == CMD_PARAMETER_CHANGE_COL)
>>> + addr &= ~((u64)(FC_BYTES - 1));
>>> +
>>> + brcmnand_set_cmd_addr(mtd, addr);
>>> + brcmnand_send_cmd(host, cmd);
>>> + last_addr = addr;
>>> + last_cmd = cmd;
>>> + cmd = CMD_NULL;
>>> + brcmnand_waitfunc(chip);
>>> +
>>> + if (last_cmd == CMD_PARAMETER_READ ||
>>> + last_cmd == CMD_PARAMETER_CHANGE_COL) {
>>> + /* Copy flash cache word-wise */
>>> + u32 *flash_cache = (u32 *)ctrl->flash_cache;
>>> +
>>> + brcmnand_soc_data_bus_prepare(ctrl->soc, true);
>>> +
>>> + /*
>>> + * Must cache the FLASH_CACHE now, since
>>> changes in
>>> + * SECTOR_SIZE_1K may invalidate it
>>> + */
>>> + for (j = 0; j < FC_WORDS; j++)
>>> + /*
>>> + * Flash cache is big endian for parameter
>>> pages, at
>>> + * least on STB SoCs
>>> + */
>>> + flash_cache[j] =
>>> be32_to_cpu(brcmnand_read_fc(ctrl, j));
>>> +
>>> + brcmnand_soc_data_bus_unprepare(ctrl->soc,
>>> true);
>>> + }
>>> + } else if (instr->type == NAND_OP_DATA_IN_INSTR) {
>>> + u8 *in = instr->ctx.data.buf.in;
>>> +
>>> + if (last_cmd == CMD_DEVICE_ID_READ) {
>>> + u32 val;
>>> +
>>> + if (instr->ctx.data.len > 8) {
>>> + dev_err(ctrl->dev, "unsupported
>>> len=%u\n",
>>> + instr->ctx.data.len);
>>> + ret = -EOPNOTSUPP;
>>> + break;
>>> + }
>>> +
>>> + for (j = 0; j < instr->ctx.data.len; j++) {
>>> + if (j == 0)
>>> + val = brcmnand_read_reg(ctrl,
>>> BRCMNAND_ID);
>>> + else if (j == 4)
>>> + val = brcmnand_read_reg(ctrl,
>>> BRCMNAND_ID_EXT);
>>> +
>>> + in[j] = (val >> (24 - ((j % 4) << 3))) & 0xff;
>>> + }
>>> + } else if (last_cmd == CMD_PARAMETER_READ ||
>>> + last_cmd == CMD_PARAMETER_CHANGE_COL) {
>>> + u64 addr;
>>> + u32 offs;
>>> +
>>> + for (j = 0; j < instr->ctx.data.len; j++) {
>>> + addr = last_addr + j;
>>> + offs = addr & (FC_BYTES - 1);
>>> +
>>> + if (j > 0 && offs == 0)
>>> +
>>> nand_change_read_column_op(chip, addr, NULL, 0,
>>> + false);
>>> +
>>> + in[j] = ctrl->flash_cache[offs];
>>> + }
>>> + }
>>> + } else if (instr->type == NAND_OP_WAITRDY_INSTR) {
>>> + ret = bcmnand_ctrl_poll_status(host, NAND_CTRL_RDY,
>>> NAND_CTRL_RDY, 0);
>>> + } else {
>>> + dev_err(ctrl->dev, "unsupported instruction type: %d\n",
>>> instr->type);
>>> + ret = -EINVAL;
>>> + }
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> static int brcmnand_exec_op(struct nand_chip *chip,
>>> const struct nand_operation *op,
>>> bool check_only)
>>> {
>>> struct brcmnand_host *host = nand_get_controller_data(chip);
>>> + struct brcmnand_controller *ctrl = host->ctrl;
>>> struct mtd_info *mtd = nand_to_mtd(chip);
>>> u8 *status;
>>> - unsigned int i;
>>> int ret = 0;
>>>
>>> if (check_only)
>>> @@ -2525,11 +2689,7 @@ static int brcmnand_exec_op(struct nand_chip *chip,
>>> if (op->deassert_wp)
>>> brcmnand_wp(mtd, 0);
>>>
>>> - for (i = 0; i < op->ninstrs; i++) {
>>> - ret = brcmnand_exec_instr(host, i, op);
>>> - if (ret)
>>> - break;
>>> - }
>>> + ctrl->exec_instr(chip, op);
>>>
>>> if (op->deassert_wp)
>>> brcmnand_wp(mtd, 1);
>>> @@ -3142,6 +3302,12 @@ int brcmnand_probe(struct platform_device *pdev,
>>> struct brcmnand_soc *soc)
>>> if (ret)
>>> goto err;
>>>
>>> + /* Only v5.0+ controllers have low level ops support */
>>> + if (ctrl->nand_version >= 0x0500)
>>> + ctrl->exec_instr = brcmnand_exec_instructions;
>>> + else
>>> + ctrl->exec_instr = brcmnand_exec_instructions_legacy;
>>> +
>>> /*
>>> * Most chips have this cache at a fixed offset within 'nand' block.
>>> * Some must specify this region separately.
>>> --
>>> 2.39.5
>
> BTW, can anyone from Broadcom confirm any of the following?
> 1. There are no low level registers on v2.1 and v2.2 controllers.

Correct.

> 2. Do low level registers exist on v4.0 controllers? They are defined
> on 63268_map_part.h but the NAND drivers I could find never use them.

They exist. The changelog for the NAND controller indicates that
starting from v4.0 onwards, the NAND LL operation is supported.

> 3. Are the low level registers bugged on v4.0 controllers?
> 4. Should the low level registers be handled differently on v4.0 controllers?
> The issue is that trying to use them on v4.0 controllers for
> GET_FEATURES would leave the NAND controller in a weird state that
> results in hangs or timeouts while trying to read the NAND.
> This happens on the Sercomm H500-s, which is a BCM63268 with a
> Macronix NAND that tries to unlock through ONFI.

That I do not know however, William and David hopefully do know.
--
Florian