| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z7ZIb3OpSdHw1w1Z@lizhi-Precision-Tower-5810>
Date: Wed, 19 Feb 2025 16:09:03 -0500
From: Frank Li <Frank.li@....com>
To: Stanley Chu <stanley.chuys@...il.com>
Cc: miquel.raynal@...tlin.com, alexandre.belloni@...tlin.com,
linux-i3c@...ts.infradead.org, linux-kernel@...r.kernel.org,
tomer.maimon@...oton.com, kwliu@...oton.com, yschu@...oton.com
Subject: Re: [PATCH v1 2/3] i3c: master: svc: fix npcm845 FIFO empty issue
On Wed, Feb 19, 2025 at 09:10:18AM +0800, Stanley Chu wrote:
> npcm845 HW issue:
> I3C HW stalls the write transfer if the transmit FIFO becomes empty,
> when new data is written to FIFO, I3C HW resumes the transfer but
> the first transmitted data bit may have the wrong value.
> Workaround:
> Fill the FIFO in advance to prevent FIFO from becoming empty.
>
> Signed-off-by: Stanley Chu <yschu@...oton.com>
> ---
> drivers/i3c/master/svc-i3c-master.c | 58 ++++++++++++++++++++++++-----
> 1 file changed, 49 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 813839498b2b..22cb1f1c1fdc 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -102,6 +102,7 @@
> #define SVC_I3C_MDATACTRL_TXTRIG_FIFO_NOT_FULL GENMASK(5, 4)
> #define SVC_I3C_MDATACTRL_RXTRIG_FIFO_NOT_EMPTY 0
> #define SVC_I3C_MDATACTRL_RXCOUNT(x) FIELD_GET(GENMASK(28, 24), (x))
> +#define SVC_I3C_MDATACTRL_TXCOUNT(x) FIELD_GET(GENMASK(20, 16), (x))
> #define SVC_I3C_MDATACTRL_TXFULL BIT(30)
> #define SVC_I3C_MDATACTRL_RXEMPTY BIT(31)
>
> @@ -133,6 +134,16 @@
> #define SVC_I3C_EVENT_IBI GENMASK(7, 0)
> #define SVC_I3C_EVENT_HOTJOIN BIT(31)
>
> +/*
> + * SVC_I3C_QUIRK_FIFO_EMPTY:
> + * I3C HW stalls the write transfer if the transmit FIFO becomes empty,
> + * when new data is written to FIFO, I3C HW resumes the transfer but
> + * the first transmitted data bit may have the wrong value.
> + * Workaround:
> + * Fill the FIFO in advance to prevent FIFO from becoming empty.
> + */
> +#define SVC_I3C_QUIRK_FIFO_EMPTY BIT(0)
> +
> struct svc_i3c_cmd {
> u8 addr;
> bool rnw;
> @@ -216,6 +227,7 @@ struct svc_i3c_master {
> struct mutex lock;
> u32 enabled_events;
> u32 mctrl_config;
> + u32 quirks;
> };
>
> /**
> @@ -891,6 +903,7 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
> unsigned int dev_nb = 0, last_addr = 0;
> u32 reg;
> int ret, i;
> + u32 dyn_addr;
>
> while (true) {
> /* clean SVC_I3C_MINT_IBIWON w1c bits */
> @@ -931,6 +944,15 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
> if (SVC_I3C_MSTATUS_RXPEND(reg)) {
> u8 data[6];
>
> + /*
> + * SVC_I3C_QUIRK_FIFO_EMPTY fix:
> + * The TX FIFO should be ready before the TX transmission is starting.
> + */
> + dyn_addr = i3c_master_get_free_addr(&master->base, last_addr + 1);
> + if (dyn_addr < 0)
> + return dyn_addr;
> + writel(dyn_addr, master->regs + SVC_I3C_MWDATAB);
> +
I think it should work for new svc controller. Fill data advance to avoid
stall SCL clock. If it is true, comments/commit message need be updated.
> /*
> * We only care about the 48-bit provisioned ID yet to
> * be sure a device does not nack an address twice.
> @@ -1009,21 +1031,20 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
> if (ret)
> break;
>
> - /* Give the slave device a suitable dynamic address */
> - ret = i3c_master_get_free_addr(&master->base, last_addr + 1);
> - if (ret < 0)
> - break;
> -
> - addrs[dev_nb] = ret;
> + addrs[dev_nb] = dyn_addr;
> dev_dbg(master->dev, "DAA: device %d assigned to 0x%02x\n",
> dev_nb, addrs[dev_nb]);
> -
> - writel(addrs[dev_nb], master->regs + SVC_I3C_MWDATAB);
> last_addr = addrs[dev_nb++];
> }
>
> /* Need manual issue STOP except for Complete condition */
> svc_i3c_master_emit_stop(master);
> + /*
> + * Dynamic address is written to FIFO in advance for SVC_I3C_QUIRK_FIFO_EMPTY fix.
> + * Flush FIFO in the failed case.
> + */
> + if (ret)
> + svc_i3c_master_flush_fifo(master);
Needn't check ret, flash fifo unconditional should be fine and avoid left
any data in fifo.
> return ret;
> }
>
> @@ -1210,7 +1231,6 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> /* clean SVC_I3C_MINT_IBIWON w1c bits */
> writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS);
>
> -
> while (retry--) {
> writel(SVC_I3C_MCTRL_REQUEST_START_ADDR |
> xfer_type |
> @@ -1220,6 +1240,22 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> SVC_I3C_MCTRL_RDTERM(*actual_len),
> master->regs + SVC_I3C_MCTRL);
>
> + if ((master->quirks & SVC_I3C_QUIRK_FIFO_EMPTY) && !rnw && xfer_len) {
> + int i, count, space;
> +
> + reg = readl(master->regs + SVC_I3C_MDATACTRL);
> + space = SVC_I3C_FIFO_SIZE - SVC_I3C_MDATACTRL_TXCOUNT(reg);
> + count = xfer_len > space ? space : xfer_len;
> + for (i = 0; i < count; i++) {
> + if (i == xfer_len - 1)
> + writel(out[0], master->regs + SVC_I3C_MWDATABE);
> + else
> + writel(out[0], master->regs + SVC_I3C_MWDATAB);
> + out++;
> + }
> + xfer_len -= count;
I think you can call svc_i3c_master_write(),
or improve svc_i3c_master_write(..., bool b_max_space)
ret = svc_i3c_master_write(master, out, count);
xfer_len -= ret;
out += ret;
> + }
> +
> ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> SVC_I3C_MSTATUS_MCTRLDONE(reg), 0, 1000);
> if (ret)
> @@ -1308,6 +1344,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> emit_stop:
> svc_i3c_master_emit_stop(master);
> svc_i3c_master_clear_merrwarn(master);
> + svc_i3c_master_flush_fifo(master);
>
> return ret;
> }
> @@ -1877,6 +1914,9 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
>
> svc_i3c_master_reset(master);
>
> + if (device_is_compatible(master->dev, "nuvoton,npcm845-i3c"))
> + master->quirks = SVC_I3C_QUIRK_FIFO_EMPTY;
> +
Generally, it'd better to add drvdata such as
struct svc_i3c_drvdata {
u32 quirks;
};
const struct svc_i3c_drvdata nutoton = {
.quirks = SVC_I3C_QUIRK_FIFO_EMPTY;
}
static const struct of_device_id svc_i3c_master_of_match_tbl[] = {
...
{ .compatible = "nuvoton,npcm845-i3c", data = &nutoton },
...
}
Frank
> /* Register the master */
> ret = i3c_master_register(&master->base, &pdev->dev,
> &svc_i3c_master_ops, false);
> --
> 2.34.1
>
Powered by blists - more mailing lists