Re: [PATCH 2/2] i2c: tegra20: Add driver for Tegra20 I2C target controller instances

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] i2c: tegra-target: stale nack state in
  tegra_i2c_target_init() and ISR
- [Medium] i2c: tegra-target: unclocked MMIO and unbalanced IRQ/clocks
  after resume error
- [Low] i2c: tegra-target: broken DT binding file path in MAINTAINERS
--

commit f38185c59a54e5417359880cac9a408a1a024e72
Author: Marc Dietrich <marvin24@gmx.de>

i2c: tegra20: Add driver for Tegra20 I2C target controller instances

This commit adds a driver for the NVIDIA Tegra20 I2C controller operating
exclusively in target (slave) mode.

> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3ee846e37066d..e543fde8bdae4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -27534,6 +27534,13 @@ R:	Dmitry Osipenko <digetx@gmail.com>
>  S:	Supported
>  F:	drivers/i2c/busses/i2c-tegra.c
>  
> +TEGRA I2C TARGET DRIVER
> +M:	Marc Dietrich <marvin24@gmx.de>
> +L:	linux-tegra@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c-target.yaml
> +F:	drivers/i2c/busses/i2c-tegra-target.c

[Severity: Low]
Does Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c-target.yaml
exist?  The nvidia,tegra20-i2c-target compatible string is documented in
Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.yaml, so this F:
entry points to a non-existent path and triggers a broken file pattern
warning in get_maintainer.pl.

> diff --git a/drivers/i2c/busses/i2c-tegra-target.c b/drivers/i2c/busses/i2c-tegra-target.c
> new file mode 100644
> index 0000000000000..6a8642983ea24
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-tegra-target.c

[ ... ]

> +static int tegra_i2c_target_init(struct tegra_i2c_target_dev *i2c_dev)
> +{
> +	u32 val;
> +	int err;
> +
> +	err = device_reset(i2c_dev->dev);
> +	if (err)
> +		return err;
> +
> +	val = I2C_CNFG_NEW_MASTER_FSM | I2C_CNFG_PACKET_MODE_EN |
> +	      FIELD_PREP(I2C_CNFG_DEBOUNCE_CNT, 2);
> +	i2c_writel(i2c_dev, val, I2C_CNFG);
> +
> +	err = clk_set_rate(i2c_dev->div_clk,
> +			   i2c_dev->timings.bus_freq_hz * I2C_CLK_MULTIPLIER);
> +	if (err) {
> +		dev_err(i2c_dev->dev, "failed to set div-clk rate: %d\n", err);
> +		return err;
> +	}
> +
> +	i2c_writel(i2c_dev, I2C_SL_CNFG_NEWSL, I2C_SL_CNFG);
> +
> +	/* only 7-bit target address is supported for now */
> +	val = FIELD_PREP(I2C_SL_ADDR1_7BIT_ADDR, i2c_dev->target->addr);
> +	i2c_writel(i2c_dev, val, I2C_SL_ADDR1);
> +	i2c_writel(i2c_dev, 0, I2C_SL_ADDR2);
> +
> +	return 0;
> +}

[ ... ]

> +static irqreturn_t tegra_i2c_target_isr(int irq, void *dev_id)
> +{
> +	struct tegra_i2c_target_dev *i2c_dev = dev_id;
> +	struct i2c_client *target = i2c_dev->target;
> +	bool read, new_xfer;
> +	u32 status;
> +	u8 val = 0xff;
> +
> +	status = i2c_readl(i2c_dev, I2C_SL_STATUS);
> +	if (!(status & I2C_SL_STATUS_SL_IRQ))
> +		return IRQ_NONE;
> +
> +	/* RNW is set if the controller reads from the target */
> +	read = status & I2C_SL_STATUS_RNW;
> +
> +	/* RCVD is set if the interrupt is for the address byte */
> +	new_xfer = status & I2C_SL_STATUS_RCVD;
> +
> +	if (!read) {
> +		val = i2c_readl(i2c_dev, I2C_SL_RCVD);
> +		if (new_xfer)
> +			i2c_writel(i2c_dev, 0, I2C_SL_RCVD);
> +	}
> +
> +	if (status & I2C_SL_STATUS_END_TRANS) {
> +		i2c_slave_event(target, I2C_SLAVE_STOP, &val);
> +
> +		if (i2c_dev->nack)
> +			tegra_i2c_target_set_nack(i2c_dev, false);
> +
> +		/* a new transfer may already have started */
> +		if (!new_xfer)
> +			return IRQ_HANDLED;
> +	}
> +
> +	if (read) {
> +		if (new_xfer) {
> +			udelay(I2C_READ_ACK_DELAY_US);
> +			i2c_slave_event(target, I2C_SLAVE_READ_REQUESTED, &val);
> +		} else {
> +			i2c_slave_event(target, I2C_SLAVE_READ_PROCESSED, &val);
> +		}
> +
> +		/* send the byte and release the bus */
> +		i2c_writel(i2c_dev, val, I2C_SL_RCVD);
> +	} else if (new_xfer) {
> +		/* the address phase is always acknowledged */
> +		if (i2c_slave_event(target, I2C_SLAVE_WRITE_REQUESTED, &val))
> +			tegra_i2c_target_set_nack(i2c_dev, true);
> +	} else if (!i2c_dev->nack) {
> +		if (i2c_slave_event(target, I2C_SLAVE_WRITE_RECEIVED, &val))
> +			tegra_i2c_target_set_nack(i2c_dev, true);
> +	}
> +
> +	return IRQ_HANDLED;
> +}

[Severity: Medium]
Can i2c_dev->nack remain stale across a Repeated START or across system
suspend and resume?

In tegra_i2c_target_isr(), tegra_i2c_target_set_nack(i2c_dev, false) is
only called when I2C_SL_STATUS_END_TRANS is set.  If a controller follows
a NACKed write with a Repeated START (where I2C_SL_STATUS_RCVD is set
without I2C_SL_STATUS_END_TRANS), i2c_dev->nack is never cleared, so a
new write transfer remains NACKed and skips I2C_SLAVE_WRITE_RECEIVED even
when I2C_SLAVE_WRITE_REQUESTED succeeds.

Similarly, tegra_i2c_target_init() writes I2C_SL_CNFG_NEWSL to
I2C_SL_CNFG, clearing I2C_SL_CNFG_NACK in hardware without resetting
i2c_dev->nack = false.  If the system suspends while i2c_dev->nack is
true, tegra_i2c_target_resume() -> tegra_i2c_target_init() leaves
hardware ACKing bytes on the bus while tegra_i2c_target_isr() silently
drops them because i2c_dev->nack is still true.

[ ... ]

> +static int tegra_i2c_target_suspend(struct device *dev)
> +{
> +	struct tegra_i2c_target_dev *i2c_dev = dev_get_drvdata(dev);
> +
> +	if (!i2c_dev->target)
> +		return 0;
> +
> +	disable_irq(i2c_dev->irq);
> +	tegra_i2c_target_deinit(i2c_dev);
> +	clk_bulk_disable_unprepare(ARRAY_SIZE(i2c_dev->clocks), i2c_dev->clocks);
> +
> +	return 0;
> +}
> +
> +static int tegra_i2c_target_resume(struct device *dev)
> +{
> +	struct tegra_i2c_target_dev *i2c_dev = dev_get_drvdata(dev);
> +	int err;
> +
> +	if (!i2c_dev->target)
> +		return 0;
> +
> +	err = clk_bulk_prepare_enable(ARRAY_SIZE(i2c_dev->clocks),
> +				      i2c_dev->clocks);
> +	if (err) {
> +		dev_err(dev, "failed to enable clocks: %d\n", err);
> +		return err;
> +	}
> +
> +	err = tegra_i2c_target_init(i2c_dev);
> +	if (err) {
> +		clk_bulk_disable_unprepare(ARRAY_SIZE(i2c_dev->clocks),
> +					   i2c_dev->clocks);
> +		return err;
> +	}
> +
> +	enable_irq(i2c_dev->irq);
> +
> +	return 0;
> +}

[Severity: Medium]
What happens in tegra_i2c_target_unreg_target() or a later
tegra_i2c_target_suspend() if tegra_i2c_target_resume() fails?

When clk_bulk_prepare_enable() or tegra_i2c_target_init() fails in
tegra_i2c_target_resume(), the clocks are left disabled and i2c_dev->irq
remains disabled, while i2c_dev->target is still non-NULL.  A subsequent
call to tegra_i2c_target_unreg_target() or tegra_i2c_target_suspend()
will then access I2C_SL_CNFG in tegra_i2c_target_deinit() with the
controller clocks disabled (which hangs the APB bus on Tegra20), call
disable_irq() a second time, and call clk_bulk_disable_unprepare() on
already disabled clocks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108212458.275980-1-marvin24@gmx.de?part=2