From mboxrd@z Thu Jan 1 00:00:00 1970 From: "M'boumba Cedric Madianga" Subject: Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver Date: Wed, 11 Jan 2017 15:20:41 +0100 Message-ID: References: <1483607246-14771-1-git-send-email-cedric.madianga@gmail.com> <1483607246-14771-3-git-send-email-cedric.madianga@gmail.com> <20170111082208.vzu7xgpd4eakyldl@pengutronix.de> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-wj0-f195.google.com ([209.85.210.195]:34157 "EHLO mail-wj0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934475AbdAKOUp (ORCPT ); Wed, 11 Jan 2017 09:20:45 -0500 In-Reply-To: Sender: linux-i2c-owner@vger.kernel.org List-Id: linux-i2c@vger.kernel.org To: =?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= Cc: Wolfram Sang , Rob Herring , Maxime Coquelin , Alexandre Torgue , Linus Walleij , Patrice Chotard , Russell King , linux-i2c@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org > >> + */ >> + reg =3D i2c_dev->base + STM32F4_I2C_CR1; >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK); >> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS); > > You could get rid of this, when caching the value of CR1. Would save two > register reads here. This doesn't work for all registers, but it should > be possible to apply for most of them, maybe enough to get rid of the > clr_bits and set_bits function. I agree at many places I could save registers read by not using clr_bits and set_bits function when the registers in question has been already read. But it is not enough to get rid of the clr_bits and set_bits function. For example when calling stm32f4_i2c_terminate_xfer(), the CR1 register is never read before so set_bits function is useful. Another example, when stm32f4_i2c_handle_rx_done(), the CR1 register is also never read before so clr_bits finction is again useful. 2017-01-11 14:58 GMT+01:00 M'boumba Cedric Madianga : > Hi Uwe, > > 2017-01-11 9:22 GMT+01:00 Uwe Kleine-K=C3=B6nig : >> Hello Cedric, >> >> On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote= : >>> +/* >>> + * In standard mode: >>> + * SCL period =3D SCL high period =3D SCL low period =3D CCR * I2C par= ent clk period >>> + * >>> + * In fast mode: >>> + * If Duty =3D 0; SCL high period =3D 1 * CCR * I2C parent clk period >>> + * SCL low period =3D 2 * CCR * I2C parent clk period >>> + * If Duty =3D 1; SCL high period =3D 9 * CCR * I2C parent clk period >>> + * SCL low period =3D 16 * CCR * I2C parent clk period >> s/ \*/ */ several times > > Sorry but I don't see where is the issue as the style for multi-line > comments seems ok. > Could you please clarify that point if possible ? Thanks in advance > >> >>> + * In order to reach 400 kHz with lower I2C parent clk frequencies we = always set >>> + * Duty =3D 1 >>> + * >>> + * For both modes, we have CCR =3D SCL period * I2C parent clk frequen= cy >>> + * with scl_period =3D 5 microseconds in Standard mode and scl_period = =3D 1 >> s/mode/Mode/ > > ok thanks > >> >>> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low p= eriods >>> + * constraints defined by i2c bus specification >> >> I don't understand scl_period =3D 1 =C2=B5s for Fast Mode. For a bus fre= qency >> of 400 kHz we need low + high =3D 2.5 =C2=B5s. Is there a factor 10 miss= ing >> somewhere? > > As CCR =3D SCL_period * I2C parent clk frequency with minimal freq =3D > 2Mhz and SCL_period =3D 1 we have: > CCR =3D 1 * 2Mhz =3D 2. > But to compute, scl_low and scl_high in Fast mode, we have to do the > following thing as Duty=3D1: > scl_high =3D 9 * CCR * I2C parent clk period > scl_low =3D 16 * CCR * I2C parent clk period > In our example: > scl_high =3D 9 * 2 * 0,0000005 =3D 0,000009 sec =3D 9 =C2=B5s > scl_low =3D 16 * 2 * 0.0000005 =3D 0,000016 sec =3D 16 =C2=B5s > So low + high =3D 27 =C2=B5s > 2,5 =C2=B5s > >> >>> + */ >>> +static struct stm32f4_i2c_timings i2c_timings[] =3D { >>> [...] >>> + >>> +/** >>> + * stm32f4_i2c_hw_config() - Prepare I2C block >>> + * @i2c_dev: Controller's private data >>> + */ >>> +static int stm32f4_i2c_hw_config(struct stm32f4_i2c_dev *i2c_dev) >>> +{ >>> + void __iomem *reg =3D i2c_dev->base + STM32F4_I2C_CR1; >>> + int ret =3D 0; >>> + >>> + /* Disable I2C */ >>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE); >>> + >>> + ret =3D stm32f4_i2c_set_periph_clk_freq(i2c_dev); >>> + if (ret) >>> + return ret; >>> + >>> + stm32f4_i2c_set_rise_time(i2c_dev); >>> + >>> + stm32f4_i2c_set_speed_mode(i2c_dev); >>> + >>> + stm32f4_i2c_set_filter(i2c_dev); >>> + >>> + /* Enable I2C */ >>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_PE); >> >> This function is called after a hw reset, so there should be no need to >> use clr_bits and set_bits because the value read from hw should be >> known. > > ok thanks > >> >>> + return ret; >> >> return 0; > > ok thanks > >> >>> +} >>> + >>> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev) >>> +{ >>> + u32 status; >>> + int ret; >>> + >>> + ret =3D readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR= 2, >>> + status, >>> + !(status & STM32F4_I2C_SR2_BUSY)= , >>> + 10, 1000); >>> + if (ret) { >>> + dev_dbg(i2c_dev->dev, "bus not free\n"); >>> + ret =3D -EBUSY; >>> + } >>> + >>> + return ret; >>> +} >>> + >>> +/** >>> + * stm32f4_i2c_write_ byte() - Write a byte in the data register >>> + * @i2c_dev: Controller's private data >>> + * @byte: Data to write in the register >>> + */ >>> +static void stm32f4_i2c_write_byte(struct stm32f4_i2c_dev *i2c_dev, u8= byte) >>> +{ >>> + writel_relaxed(byte, i2c_dev->base + STM32F4_I2C_DR); >>> +} >>> + >>> +/** >>> + * stm32f4_i2c_write_msg() - Fill the data register in write mode >>> + * @i2c_dev: Controller's private data >>> + * >>> + * This function fills the data register with I2C transfer buffer >>> + */ >>> +static void stm32f4_i2c_write_msg(struct stm32f4_i2c_dev *i2c_dev) >>> +{ >>> + struct stm32f4_i2c_msg *msg =3D &i2c_dev->msg; >>> + >>> + stm32f4_i2c_write_byte(i2c_dev, *msg->buf++); >>> + msg->count--; >>> +} >>> + >>> +static void stm32f4_i2c_read_msg(struct stm32f4_i2c_dev *i2c_dev) >>> +{ >>> + struct stm32f4_i2c_msg *msg =3D &i2c_dev->msg; >>> + u32 rbuf; >>> + >>> + rbuf =3D readl_relaxed(i2c_dev->base + STM32F4_I2C_DR); >>> + *msg->buf++ =3D rbuf & 0xff; >> >> This is unnecessary. buf has an 8 bit wide type so >> >> *msg->buf++ =3D rbuf; >> >> has the same effect. (ISTR this is something I already pointed out >> earlier?) > > Yes you are right. > >> >>> + msg->count--; >>> +} >>> + >>> +static void stm32f4_i2c_terminate_xfer(struct stm32f4_i2c_dev *i2c_dev= ) >>> +{ >>> + struct stm32f4_i2c_msg *msg =3D &i2c_dev->msg; >>> + void __iomem *reg =3D i2c_dev->base + STM32F4_I2C_CR2; >>> + >>> + stm32f4_i2c_disable_irq(i2c_dev); >>> + >>> + reg =3D i2c_dev->base + STM32F4_I2C_CR1; >>> + if (msg->stop) >>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP); >>> + else >>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START); >>> + >>> + complete(&i2c_dev->complete); >>> +} >>> + >>> +/** >>> + * stm32f4_i2c_handle_write() - Handle FIFO empty interrupt in case of= write >>> + * @i2c_dev: Controller's private data >>> + */ >>> +static void stm32f4_i2c_handle_write(struct stm32f4_i2c_dev *i2c_dev) >>> +{ >>> + struct stm32f4_i2c_msg *msg =3D &i2c_dev->msg; >>> + void __iomem *reg =3D i2c_dev->base + STM32F4_I2C_CR2; >>> + >>> + if (msg->count) { >>> + stm32f4_i2c_write_msg(i2c_dev); >>> + if (!msg->count) { >>> + /* Disable buffer interrupts for RXNE/TXE events = */ >>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN= ); >>> + } >>> + } else { >>> + stm32f4_i2c_terminate_xfer(i2c_dev); >> >> Is stm32f4_i2c_terminate_xfer also called when arbitration is lost? If >> yes, is it then right to set STM32F4_I2C_CR1_STOP or >> STM32F4_I2C_CR1_START? > > If arbitration is lost, stm32f4_i2c_terminate_xfer() is not called. > In that case, we return -EAGAIN and i2c-core will retry by calling > stm32f4_i2c_xfer() > >> >>> + } >>> +} >>> + >>> +/** >>> + * stm32f4_i2c_handle_read() - Handle FIFO empty interrupt in case of = read >>> + * @i2c_dev: Controller's private data >>> + */ >>> +static void stm32f4_i2c_handle_read(struct stm32f4_i2c_dev *i2c_dev) >>> +{ >>> + struct stm32f4_i2c_msg *msg =3D &i2c_dev->msg; >>> + void __iomem *reg =3D i2c_dev->base + STM32F4_I2C_CR2; >>> + >>> + switch (msg->count) { >>> + case 1: >>> + stm32f4_i2c_disable_irq(i2c_dev); >>> + stm32f4_i2c_read_msg(i2c_dev); >>> + complete(&i2c_dev->complete); >>> + break; >>> + /* >>> + * For 2 or 3-byte reception, we do not have to read the data reg= ister >>> + * when RXNE occurs as we have to wait for byte transferred finis= hed >> >> it's hard to understand because if you don't know the hardware the >> meaning of RXNE is unknown. > > Ok I will replace RXNE by RX not empty in that comment > >> >>> + * event before reading data. So, here we just disable buffer >>> + * interrupt in order to avoid another system preemption due to R= XNE >>> + * event >>> + */ >>> + case 2: >>> + case 3: >>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR2_ITBUFEN); >>> + break; >>> + /* For N byte reception with N > 3 we directly read data register= */ >>> + default: >>> + stm32f4_i2c_read_msg(i2c_dev); >>> + } >>> +} >>> + >>> +/** >>> + * stm32f4_i2c_handle_rx_btf() - Handle byte transfer finished interru= pt >>> + * in case of read >>> + * @i2c_dev: Controller's private data >>> + */ >>> +static void stm32f4_i2c_handle_rx_btf(struct stm32f4_i2c_dev *i2c_dev) >>> +{ >> >> btf is a hw-related name. Maybe better use _done which is easier to >> understand? > > OK > >> >>> + struct stm32f4_i2c_msg *msg =3D &i2c_dev->msg; >>> + void __iomem *reg; >>> + u32 mask; >>> + int i; >>> + >>> + switch (msg->count) { >>> + case 2: >>> + /* >>> + * In order to correctly send the Stop or Repeated Start >>> + * condition on the I2C bus, the STOP/START bit has to be= set >>> + * before reading the last two bytes. >>> + * After that, we could read the last two bytes, disable >>> + * remaining interrupts and notify the end of xfer to the >>> + * client >> >> This is surprising. I didn't recheck the manual, but that looks very >> uncomfortable. > > I agree but this exactly the hardware way of working described in the > reference manual. > >>How does this work, when I only want to read a single >> byte? Same problem for ACK below. > > For a single reception, we enable NACK and STOP or Repeatead START > bits during address match. > The NACK and STOP/START pulses are sent as soon as the data is > received in the shift register. > Please note that in that case, we don't have to wait BTF event to read th= e data. > Data is read as soon as RXNE event occurs. > >> >>> + */ >>> + reg =3D i2c_dev->base + STM32F4_I2C_CR1; >>> + if (msg->stop) >>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP); >>> + else >>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START); >>> + >>> + for (i =3D 2; i > 0; i--) >>> + stm32f4_i2c_read_msg(i2c_dev); >>> + >>> + reg =3D i2c_dev->base + STM32F4_I2C_CR2; >>> + mask =3D STM32F4_I2C_CR2_ITEVTEN | STM32F4_I2C_CR2_ITERRE= N; >>> + stm32f4_i2c_clr_bits(reg, mask); >>> + >>> + complete(&i2c_dev->complete); >>> + break; >>> + case 3: >>> + /* >>> + * In order to correctly send the ACK on the I2C bus for = the >>> + * last two bytes, we have to set ACK bit before reading = the >>> + * third last data byte >>> + */ >>> + reg =3D i2c_dev->base + STM32F4_I2C_CR1; >>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK); >>> + stm32f4_i2c_read_msg(i2c_dev); >>> + break; >>> + default: >>> + stm32f4_i2c_read_msg(i2c_dev); >>> + } >>> +} >>> + >>> +/** >>> + * stm32f4_i2c_handle_rx_addr() - Handle address matched interrupt in = case of >>> + * master receiver >>> + * @i2c_dev: Controller's private data >>> + */ >>> +static void stm32f4_i2c_handle_rx_addr(struct stm32f4_i2c_dev *i2c_dev= ) >>> +{ >>> + struct stm32f4_i2c_msg *msg =3D &i2c_dev->msg; >>> + void __iomem *reg; >>> + >>> + switch (msg->count) { >>> + case 0: >>> + stm32f4_i2c_terminate_xfer(i2c_dev); >>> + /* Clear ADDR flag */ >>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2); >>> + break; >>> + case 1: >>> + /* >>> + * Single byte reception: >> >> This also happens for the last byte of a 5 byte transfer, right? > > For a 5 byte transfer the behavior is different: > We have to read data from DR (data register) as soon as the RXNE (RX > not empty) event occurs for data1, data2 and data3 (until N-2 data for > a more generic case) > The ACK is automatically sent as soon as the data is received in the > shift register as the I2C controller was configured to do that during > adress match phase. > > For data3 (N-2 data), we wait for BTF (Byte Transfer finished) event > in order to set NACK before reading DR. > This event occurs when a new data has been received in shift register > (in our case data4 or N-1 data) but the prevoius data in DR (in our > case data3 or N-2 data) has not been read yet. > In that way, the NACK pulse will be correctly generated after the last > received data byte. > > For data4 and data5, we wait for BTF event (data4 or N-1 data in DR > and data5 or N data in shift register), set STOP or repeated Start in > order to correctly sent the right pulse after the last received data > byte and run 2 consecutives read of DR. > >> >>> + * Enable NACK, clear ADDR flag and generate STOP or RepS= TART >>> + */ >>> + reg =3D i2c_dev->base + STM32F4_I2C_CR1; >>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK); >>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2); >>> + if (msg->stop) >>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP); >>> + else >>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_START); >>> + break; >>> + case 2: >>> + /* >>> + * 2-byte reception: >>> + * Enable NACK and set POS >> >> What is POS? > POS is used to define the position of the (N)ACK pulse > 0: ACK is generated when the current is being received in the shift regis= ter > 1: ACK is generated when the next byte which will be received in the > shift register (used for 2-byte reception) > >> >>> + */ >>> + reg =3D i2c_dev->base + STM32F4_I2C_CR1; >>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_ACK); >>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_POS); >> >> You could get rid of this, when caching the value of CR1. Would save two >> register reads here. This doesn't work for all registers, but it should >> be possible to apply for most of them, maybe enough to get rid of the >> clr_bits and set_bits function. >> >>> + readl_relaxed(i2c_dev->base + STM32F4_I2C_SR2); >>> + break; >>> + >>> + default: >>> + /* N-byte reception: Enable ACK */ >>> + reg =3D i2c_dev->base + STM32F4_I2C_CR1; >>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_ACK); >> >> Do you need to set ACK for each byte transferred? > I need to do that in order to be SMBus compatible and the ACK/NACK > seems to be used by default in Documentation/i2c/i2c-protocol file. > >> >> I stopp reviewing here because of -ENOTIME on my side but don't want to >> delay discussion, so sent my comments up to here already now. >> >> Best regards >> Uwe >> >> -- >> Pengutronix e.K. | Uwe Kleine-K=C3=B6nig = | >> Industrial Linux Solutions | http://www.pengutronix.de/ = |