linux-kernel - Re: [PATCH v5 2/8] i2c: iproc: Add slave mode support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c67178db-9459-b424-a29a-23bb0e0c5ca9@broadcom.com>
Date:   Mon, 1 Apr 2019 14:33:47 -0700
From:   Ray Jui <ray.jui@...adcom.com>
To:     Wolfram Sang <wsa@...-dreams.de>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>, linux-i2c@...r.kernel.org,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org,
        bcm-kernel-feedback-list@...adcom.com,
        Rayagonda Kokatanur <rayagonda.kokatanur@...adcom.com>,
        Shreesha Rajashekar <shreesha.rajashekar@...adcom.com>,
        Michael Cheng <ccheng@...adcom.com>
Subject: Re: [PATCH v5 2/8] i2c: iproc: Add slave mode support

Hi Wolfram/Rayagonda,

On 3/27/2019 3:14 PM, Wolfram Sang wrote:
> 
>> +static void bcm_iproc_i2c_slave_init(
>> +	struct bcm_iproc_i2c_dev *iproc_i2c, bool need_reset)
>> +{
>> +	u32 val;
>> +
>> +	if (need_reset) {
>> +		/* put controller in reset */
>> +		val = readl(iproc_i2c->base + CFG_OFFSET);
>> +		val |= BIT(CFG_RESET_SHIFT);
>> +		writel(val, iproc_i2c->base + CFG_OFFSET);
>> +
>> +		/* wait 100 usec per spec */
>> +		udelay(100);
>> +
>> +		/* bring controller out of reset */
>> +		val &= ~(BIT(CFG_RESET_SHIFT));
>> +		writel(val, iproc_i2c->base + CFG_OFFSET);
>> +	}
>> +
>> +	/* flush TX/RX FIFOs */
>> +	val = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT));
>> +	writel(val, iproc_i2c->base + S_FIFO_CTRL_OFFSET);
> 
> Will flushing FIFOs work when a slave is register while a master
> transfer is on-going at the same time?
> 

Okay, as you pointed out in a subsequent email, this can't happen.

>> +
>> +	/* RANDOM SLAVE STRETCH time - 20ms*/
> 
> What is a "random stretch time"? 20ms sounds like a lot. Also, missing
> space before comment terminator.
> 

Rayagonda,

Could you please help to comment on the choice of the 20 ms to allow
clock stretch from the slave? In probably all cases, the slave should
not need more than 1 ms? 20 ms does seem way too long as Wolfram pointed
out.

Will fix the missing space before comment terminator.

>> @@ -224,22 +473,25 @@ static int bcm_iproc_i2c_init(struct bcm_iproc_i2c_dev *iproc_i2c)
>>  
>>  	/* put controller in reset */
>>  	val = readl(iproc_i2c->base + CFG_OFFSET);
>> -	val |= 1 << CFG_RESET_SHIFT;
>> -	val &= ~(1 << CFG_EN_SHIFT);
>> +	val |= BIT(CFG_RESET_SHIFT);
>> +	val &= ~(BIT(CFG_EN_SHIFT));
>>  	writel(val, iproc_i2c->base + CFG_OFFSET);
>>  
>>  	/* wait 100 usec per spec */
>>  	udelay(100);
>>  
>>  	/* bring controller out of reset */
>> -	val &= ~(1 << CFG_RESET_SHIFT);
>> +	val &= ~(BIT(CFG_RESET_SHIFT));
>>  	writel(val, iproc_i2c->base + CFG_OFFSET);
>>  
>>  	/* flush TX/RX FIFOs and set RX FIFO threshold to zero */
>> -	val = (1 << M_FIFO_RX_FLUSH_SHIFT) | (1 << M_FIFO_TX_FLUSH_SHIFT);
>> +	val = (BIT(M_FIFO_RX_FLUSH_SHIFT) | BIT(M_FIFO_TX_FLUSH_SHIFT));
>>  	writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
>>  	/* disable all interrupts */
>> -	writel(0, iproc_i2c->base + IE_OFFSET);
>> +	val = readl(iproc_i2c->base + IE_OFFSET);
>> +	val &= ~(IE_M_ALL_INTERRUPT_MASK <<
>> +			IE_M_ALL_INTERRUPT_SHIFT);
>> +	writel(val, iproc_i2c->base + IE_OFFSET);
> 
> This block looks unrelated, but I won't be too strict here...
> 
>> +	case M_CMD_STATUS_FIFO_UNDERRUN:
>> +		dev_dbg(iproc_i2c->device, "FIFO under-run\n");
>> +		return -ENXIO;
>> +
>> +	case M_CMD_STATUS_RX_FIFO_FULL:
>> +		dev_dbg(iproc_i2c->device, "Master Rx FIFO full > 10ms\n");
>> +		return -ETIMEDOUT;
>> +
> 
> ... however, this looks really unrelated to me. This is about master
> transmission, or?

This should be submitted in a separate commit. Will do that in the next
iteration of patch series.

> 
> Rest looks OK.
> 

Thanks,

Ray