Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 11 Aug 2023 16:59:27 +0300
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Yann Sionneau <yann@sionneau.net>
Cc:     Jarkko Nikula <jarkko.nikula@linux.intel.com>,
        Mika Westerberg <mika.westerberg@linux.intel.com>,
        linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
        Yann Sionneau <ysionneau@kalray.eu>,
        Jonathan Borne <jborne@kalray.eu>
Subject: Re: [PATCH 1/2] i2c: designware: fix __i2c_dw_disable in case master
 is holding SCL low
Message-ID: <ZNY+vyEsM/kNKgHt@smile.fi.intel.com>
References: <20230811124624.12792-1-yann@sionneau.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230811124624.12792-1-yann@sionneau.net>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Fri, Aug 11, 2023 at 02:46:23PM +0200, Yann Sionneau wrote:
> From: Yann Sionneau <ysionneau@kalray.eu>
> 
> The designware IP can be synthesized with the IC_EMPTYFIFO_HOLD_MASTER_EN

DesignWare

> parameter.
> In which case, if the TX FIFO gets empty and the last command didn't have

"In this case when the..."

> the STOP bit (IC_DATA_CMD[9]), the dw_apb_i2c will hold SCL low until

"the controller will..."

> a new command is pushed into the TX FIFO or the transfer is aborted.
> 
> When the dw_apb_i2c is holding SCL low, it cannot be disabled.

"When the controller..."

> The transfer must first be aborted.
> Also, the bus recover won't work because SCL is held low by the master.
> 
> This patch checks if the master is holding SCL low in __i2c_dw_disable

Grep for "This patch" in the Submitting Patches document and fix this
accordingly.

__i2c_dw_disable()

> before trying to disable the IP.
> If SCL is held low, an abort is initiated.
> When the abort is done, the disabling can then proceed.
> 
> This whole situation can happen for instance during SMBUS read data block
> if the slave just responds with "byte count == 0".
> This puts the master in an unrecoverable state, holding SCL low and the
> current __i2c_dw_disable procedure is not working. In this situation

__i2c_dw_disable()

> only a Linux reboot can fix the i2c bus.

If reboot helps, what magic does it do that Linux OS can't repeat in software?
Please, elaborate more.

...

>  	int timeout = 100;
>  	u32 status;
> +	u32 raw_intr_stats;
> +	u32 enable;
> +	bool abort_needed;
> +	bool abort_done = false;

Perhaps reversed xmas tree order?

	bool abort_done = false;
	bool abort_needed;
	u32 raw_intr_stats;
	int timeout = 100;
	u32 status;
	u32 enable;

...

> +	abort_needed = raw_intr_stats & DW_IC_INTR_MST_ON_HOLD;
> +	if (abort_needed)
> +		regmap_write(dev->map, DW_IC_ENABLE, enable | DW_IC_ENABLE_ABORT);
>  
>  	do {
> +		if (abort_needed && !abort_done) {
> +			regmap_read(dev->map, DW_IC_ENABLE, &enable);
> +			abort_done = !(enable & DW_IC_ENABLE_ABORT);
> +			continue;

This will exhaust the timeout and below can be run at most once,
is it a problem?

Also it's a tight busyloop, are you sure it's what you need?

> +		}

-- 
With Best Regards,
Andy Shevchenko