linux-kernel - Re: [PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

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: <525F915D.9020501@st.com>
Date:	Thu, 17 Oct 2013 09:27:25 +0200
From:	Maxime COQUELIN <maxime.coquelin@...com>
To:	Jean-Christophe PLAGNIOL-VILLARD <plagnioj@...osoft.com>
Cc:	Wolfram Sang <wsa@...-dreams.de>,
	Srinivas KANDAGATLA <srinivas.kandagatla@...com>,
	Rob Herring <rob.herring@...xeda.com>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Stephen Warren <swarren@...dotorg.org>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Rob Landley <rob@...dley.net>,
	Russell King <linux@....linux.org.uk>,
	Grant Likely <grant.likely@...aro.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
	Stuart MENEFY <stuart.menefy@...com>,
	Lee Jones <lee.jones@...aro.org>,
	Stephen GALLIMORE <stephen.gallimore@...com>,
	Gabriel FERNANDEZ <gabriel.fernandez@...com>
Subject: Re: [PATCH v5 1/4] i2c: busses: i2c-st: Add ST I2C controller

Hi Jean-Christophe,

On 10/16/2013 05:14 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:


...
>> +
>> +static inline void st_i2c_set_bits(void __iomem *reg, u32 mask)
>> +{
>> +     writel(readl(reg) | mask, reg);
>> +}
>> +
>> +static inline void st_i2c_clr_bits(void __iomem *reg, u32 mask)
>> +{
>> +     writel(readl(reg) & ~mask, reg);
> use set_bit api and use relaxed version
Using the set_bit api here does not match with the purpose of these 
functions.
We want to be able to set/clear multiple bits, and AFAICS the set_bit 
api does not
provide this possibility.

I took example on i2c-nomadik for these functions.

>> +}
>> +
>> +/* From I2C Specifications v0.5 */
>> +static struct st_i2c_timings i2c_timings[] = {
>> +     [I2C_MODE_STANDARD] = {
>> +             .rate                   = 100000,
>> +             .rep_start_hold         = 4000,
>> +             .rep_start_setup        = 4700,
>> +             .start_hold             = 4000,
>> +             .data_setup_time        = 250,
>> +             .stop_setup_time        = 4000,
>> +             .bus_free_time          = 4700,
>> +     },
>> +     [I2C_MODE_FAST] = {
>> +             .rate                   = 400000,
>> +             .rep_start_hold         = 600,
>> +             .rep_start_setup        = 600,
>> +             .start_hold             = 600,
>> +             .data_setup_time        = 100,
>> +             .stop_setup_time        = 600,
>> +             .bus_free_time          = 1300,
>> +     },
>> +};
> so how do you plane to support other rate that 100k and 400k?
The SSC IP only supports Standard and Fast modes.
There are no plans to support faster modes.
>> +
>> +static void st_i2c_flush_rx_fifo(struct st_i2c_dev *i2c_dev)
>> +{
>> +     int count, i;
>> +
>> +     /*
>> +      * Counter only counts up to 7 but fifo size is 8...
>> +      * When fifo is full, counter is 0 and RIR bit of status register is
>> +      * set
>> +      */
>> +     if (readl(i2c_dev->base + SSC_STA) & SSC_STA_RIR)
>> +             count = SSC_RXFIFO_SIZE;
>> +     else
>> +             count = readl(i2c_dev->base + SSC_RX_FSTAT) &
>> +                     SSC_RX_FSTAT_STATUS;
>> +
>> +     for (i = 0; i < count; i++)
>> +             readl(i2c_dev->base + SSC_RBUF);
> use readsl
Since the read content is flushed, I prefer keeping it as it is instead 
of allocating
a buffer of the FIFO's size.
>
> use relaxed version as much as possible
I was not comfortable with the different possibilities (_raw_readl, 
readl_relaxed, readl...).
I found this interresting discussion: 
_http://comments.gmane.org/gmane.linux.ports.arm.kernel/117626
 From what I understood, you are right, I should be able to use 
readl_relaxed everywhere.

Maybe I should perform a readl on the interrupt mask register before 
returning from the interrupt handler,
in order to ensure that the write to the IEN register is effective 
before the IRQ for the device is re-enabled at GIC level.
Maybe this could avoid the few spurious interrupts I face sometimes, I 
will have a try.

>> +}
>> +
...
>> +
>> +static int st_i2c_wait_free_bus(struct st_i2c_dev *i2c_dev)
>> +{
>> +     u32 sta;
>> +     int i;
>> +
>> +     for (i = 0; i < 10; i++) {
>> +             sta = readl(i2c_dev->base + SSC_STA);
>> +             if (!(sta & SSC_STA_BUSY))
>> +                     return 0;
>> +
>> +             usleep_range(2000, 4000);
> can not use interrupt?
Sadly, no.
There is no interrupt linked to the busy bit.
>> +     }
>> +
...
>> +
>> +static struct of_device_id st_i2c_match[] = {
>> +     { .compatible = "st,comms-ssc-i2c", },
> the rules is to put the first soc that use the ip in the compatible
> as st,sti7100-scc-i2c
Ok. There are no plans to upstream the SH4 platforms, it will only 
remains in stlinux.com.
Maybe I can set the first ARM platform (STiH415)?
That would give st,stih415-ssc-i2c.

Thanks for the review,
Maxime

>
> Best Regards,
> J.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/