| 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: <aVKZIwZf6_NJJRz8@aurel32.net>
Date: Mon, 29 Dec 2025 16:07:15 +0100
From: Aurelien Jarno <aurelien@...el32.net>
To: Troy Mitchell <troy.mitchell@...ux.spacemit.com>
Cc: Andi Shyti <andi.shyti@...nel.org>, Yixun Lan <dlan@...too.org>,
Alex Elder <elder@...cstar.com>,
Michael Opdenacker <michael.opdenacker@...tcommit.com>,
Troy Mitchell <troymitchell988@...il.com>,
linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-riscv@...ts.infradead.org, spacemit@...ts.linux.dev
Subject: Re: [PATCH v5 2/2] i2c: spacemit: introduce pio for k1
Hi,
On 2025-12-26 11:31, Troy Mitchell wrote:
> This patch introduces I2C PIO functionality for the Spacemit K1 SoC,
> enabling the use of I2C in atomic context.
>
> When i2c xfer_atomic is invoked, use_pio is set accordingly.
>
> Since an atomic context is required, all interrupts are disabled when
> operating in PIO mode. Even with interrupts disabled, the bits in the
> ISR (Interrupt Status Register) will still be set, so error handling can
> be performed by polling the relevant status bits in the ISR.
>
> Signed-off-by: Troy Mitchell <troy.mitchell@...ux.spacemit.com>
> ---
> Changes in v5:
> - optimize code logic
> - refactor delay handling into spacemit_i2c_delay() helper
> - introduce spacemit_i2c_complete() to centralize transfer completion
> - rework PIO transfer wait logic for clarity and correctness
> - modify and add some comments
> - modify commit message
> - Link to v4: https://lore.kernel.org/all/20251009-k1-i2c-atomic-v4-1-a89367870286@linux.spacemit.com/
>
> Changes in v4:
> - refactor for better readability: simplify condition check and moving if/else (timeout/
> wait_xfer_complete) logic into a function
> - remove irrelevant changes
> - remove the status clear call in spacemit_i2c_xfer_common()
> - sort functions to avoid forward declarations,
> move unavoidable ones above function definitions
> - use udelay() in atomic context to avoid sleeping
> - wait for MSD on the last byte in wait_pio_xfer()
> - Link to v3: https://lore.kernel.org/r/20250929-k1-i2c-atomic-v3-1-f7e660c138b6@linux.spacemit.com
>
> Changes in v3:
> - drop 1-5 patches (have been merged)
> - modify commit message
> - use readl_poll_timeout_atomic() in wait_pio_xfer()
> - use msecs_to_jiffies() when get PIO mode timeout value
> - factor out transfer state handling into spacemit_i2c_handle_state().
> - do not disable/enable the controller IRQ around PIO transfers.
> - consolidate spacemit_i2c_init() interrupt setup
> - rename is_pio -> use_pio
> - rename spacemit_i2c_xfer() -> spacemit_i2c_xfer_common()
> - rename spacemit_i2c_int_xfer() -> spacemit_i2c_xfer()
> - rename spacemit_i2c_pio_xfer() -> spacemit_i2c_pio_xfer_atomic()
> - call spacemit_i2c_err_check() in wait_pio_xfer() when write last byte
> - Link to v2: https://lore.kernel.org/r/20250925-k1-i2c-atomic-v2-0-46dc13311cda@linux.spacemit.com
>
> Changes in v2:
> - add is_pio judgement in irq_handler()
> - use a fixed timeout value when PIO
> - use readl_poll_timeout() in spacemit_i2c_wait_bus_idle() when PIO
> - Link to v1: https://lore.kernel.org/r/20250827-k1-i2c-atomic-v1-0-e59bea02d680@linux.spacemit.com
> ---
> drivers/i2c/busses/i2c-k1.c | 297 +++++++++++++++++++++++++++++++++-----------
> 1 file changed, 225 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
> index accef6653b56bd3505770328af17e441fad613a7..78a2de2c517e51e6ff997cc21402eb8f85054f85 100644
> --- a/drivers/i2c/busses/i2c-k1.c
> +++ b/drivers/i2c/busses/i2c-k1.c
...
> @@ -383,8 +424,134 @@ static void spacemit_i2c_err_check(struct spacemit_i2c_dev *i2c)
>
> spacemit_i2c_clear_int_status(i2c, SPACEMIT_I2C_INT_STATUS_MASK);
>
> - i2c->state = SPACEMIT_STATE_IDLE;
> - complete(&i2c->complete);
> + spacemit_i2c_complete(i2c);
> +}
> +
> +static void spacemit_i2c_handle_state(struct spacemit_i2c_dev *i2c)
> +{
> + u32 val;
> +
> + if (i2c->status & SPACEMIT_SR_ERR)
> + goto err_out;
> +
> + val = readl(i2c->base + SPACEMIT_ICR);
> + val &= ~(SPACEMIT_CR_TB | SPACEMIT_CR_ACKNAK | SPACEMIT_CR_STOP | SPACEMIT_CR_START);
> +
> + switch (i2c->state) {
> + case SPACEMIT_STATE_START:
> + spacemit_i2c_handle_start(i2c);
> + break;
> + case SPACEMIT_STATE_READ:
> + spacemit_i2c_handle_read(i2c);
> + break;
> + case SPACEMIT_STATE_WRITE:
> + spacemit_i2c_handle_write(i2c);
> + break;
> + default:
> + break;
> + }
> +
> + if (i2c->state != SPACEMIT_STATE_IDLE) {
> + val |= SPACEMIT_CR_TB;
> + if (i2c->use_pio)
> + val |= SPACEMIT_CR_ALDIE;
> +
> +
> + if (spacemit_i2c_is_last_msg(i2c)) {
> + /* trigger next byte with stop */
> + val |= SPACEMIT_CR_STOP;
> +
> + if (i2c->read)
> + val |= SPACEMIT_CR_ACKNAK;
> + }
> + writel(val, i2c->base + SPACEMIT_ICR);
> + }
> +
> +err_out:
> + spacemit_i2c_err_check(i2c);
> +}
> +
> +/*
> + * In PIO mode, this function is used as a replacement for
> + * wait_for_completion_timeout(), whose return value indicates
> + * the remaining time.
> + *
> + * We do not have a meaningful remaining-time value here, so
> + * return a non-zero value on success to indicate "not timed out".
> + * Returning 1 ensures callers treating the return value as
> + * time_left will not incorrectly report a timeout.
> + */
> +static int spacemit_i2c_wait_pio_xfer(struct spacemit_i2c_dev *i2c)
> +{
> + u32 mask, msec = jiffies_to_msecs(i2c->adapt.timeout);
> + ktime_t timeout = ktime_add_ms(ktime_get(), msec);
> + int ret;
> +
> + mask = SPACEMIT_SR_IRF | SPACEMIT_SR_ITE;
> +
> + do {
> + i2c->status = readl(i2c->base + SPACEMIT_ISR);
> +
> + spacemit_i2c_clear_int_status(i2c, i2c->status);
Do we actually need to clear the interrupt status even if none of above
bits are set? Said otherwise, can we move this line after the if and
before the handle_state?
> +
> + if (!(i2c->status & mask)) {
> + udelay(10);
It seems that the poll delay elsewhere in this patch is 30 µs.
Maybe use a consistent value.
> + continue;
> + }
> +
> + spacemit_i2c_handle_state(i2c);
> +
> +
Please delete the extra blank lines here.
> + } while (i2c->unprocessed && ktime_compare(ktime_get(), timeout) < 0);
> +
Otherwise it sounds good, thanks for the changes.
Regards
Aurelien
--
Aurelien Jarno GPG: 4096R/1DDD8C9B
aurelien@...el32.net http://aurel32.net
Powered by blists - more mailing lists