All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: "M'boumba Cedric Madianga" <cedric.madianga@gmail.com>
Cc: Wolfram Sang <wsa@the-dreams.de>,
	Rob Herring <robh+dt@kernel.org>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Alexandre Torgue <alexandre.torgue@st.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Patrice Chotard <patrice.chotard@st.com>,
	Russell King <linux@armlinux.org.uk>,
	linux-i2c@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
Date: Thu, 12 Jan 2017 22:10:04 +0100	[thread overview]
Message-ID: <20170112211004.z3wylc7vrubulc3x@pengutronix.de> (raw)
In-Reply-To: <CAOAejn3tPi0fjy+t-UhMTvAq7LfdKJJdbcLw2su1-YAGDZMxew@mail.gmail.com>

On Thu, Jan 12, 2017 at 09:58:23PM +0100, M'boumba Cedric Madianga wrote:
> 2017-01-12 18:49 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > On Thu, Jan 12, 2017 at 02:47:42PM +0100, M'boumba Cedric Madianga wrote:
> >> 2017-01-12 13:03 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> > Hello Cedric,
> >> >
> >> > On Thu, Jan 12, 2017 at 12:23:12PM +0100, M'boumba Cedric Madianga wrote:
> >> >> 2017-01-11 16:39 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> >> > On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
> >> >> >> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> >> >> > 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.
> >> >> >
> >> >> > IMHO that's a hw bug. This makes it for example impossible to implement
> >> >> > SMBus block transfers (I think).
> >> >>
> >> >> This is not correct.
> >> >> Setting STOP/START bit does not mean the the pulse will be sent right now.
> >> >> Here we have just to prepare the hardware for the 2 next pulse but the
> >> >> STOP/START/ACK pulse will be generated at the right time as required
> >> >> by I2C specification.
> >> >> So SMBus block transfer will be possible.
> >> >
> >> > A block transfer consists of a byte that specifies the count of bytes
> >> > yet to come. So the device sends for example:
> >> >
> >> >         0x01 0xab
> >> >
> >> > So when you read the 1 in the first byte it's already too late to set
> >> > STOP to get it after the 2nd byte.
> >> >
> >> > Not sure I got all the required details right, though.
> >>
> >> Ok I understand your use case but I always think that the harware manages it.
> >> If I take the above example, the I2C SMBus block read transaction will
> >> be as below:
> >> S Addr Wr [A] Comm [A]
> >>            S Addr Rd [A] [Count] A [Data1] A [Data2] NA P
> >>
> >> The first message is a single byte-transmission so there is no problem.
> >>
> >> The second message is a N-byte reception with N = 3
> >>
> >> When the I2C controller has finished to send the device address (S
> >> Addr Rd), the ADDR flag is set and an interrupt is raised.
> >> In the routine that handles ADDR event, we set ACK bit in order to
> >> generate ACK pulse as soon as a data byte is received in the shift
> >> register and then we clear the ADDR flag.
> >> Please note that the SCL line is stretched low until ADDR flag is cleared.
> >> So, as far I understand, the device could not sent any data as long as
> >> the SCL line is stretched low. Right ?
> >>
> >> Then, as soon as the SCL line is high, the device could send the first
> >> data byte (Count).
> >> When this byte is received in the shift register, an ACK is
> >> automatically generated as defined during adress match phase and the
> >> data byte is pushed in DR (data register).
> >> Then, an interrupt is raised as RXNE (RX not empty) flag is set.
> >> In the routine that handles RXNE event, as N=3, we just clear all
> >> buffer interrupts in order to avoid another system preemption due to
> >> RXNE event but we does not read the data in DR.
> >
> > In my example I want to receive a block of length 1, so only two bytes
> > are read, a 1 (the length) and the data byte (0xab in my example). I
> > think that as soon as you read the 1 it's already to late to schedule
> > the NA after the next byte?
> 
> Not really. This 2-byte reception is also correctly managed.
> Indeed, in this case, when the controller has sent the device address,
> the ADDR flag is set and an interrupt is raised.
> So, as long as the ADDR flag is not cleared, the SCL line is stretched
> low and the device could not send any data.
> During this address match phase, for a 2-byte reception, we enable
> NACK and set POS bit (ACK/NACK position).
> As POS=1, the NACK will be sent for the next byte which will be
> received in the shift register instead of the current one.
> So in this example, the next byte will be the last one.
> After that, we clear the ADDR flag and the device is allowed to send data.

I didn't follow, but if you are convinced it works that's good. I wonder
if it simplifies the driver if POS=1 is used and so ACK/NACK can be
setup later?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

WARNING: multiple messages have this Message-ID (diff)
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: M'boumba Cedric Madianga <cedric.madianga@gmail.com>
Cc: devicetree@vger.kernel.org,
	Alexandre Torgue <alexandre.torgue@st.com>,
	Wolfram Sang <wsa@the-dreams.de>,
	linux-kernel@vger.kernel.org,
	Linus Walleij <linus.walleij@linaro.org>,
	Patrice Chotard <patrice.chotard@st.com>,
	Russell King <linux@armlinux.org.uk>,
	Rob Herring <robh+dt@kernel.org>,
	linux-i2c@vger.kernel.org,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
Date: Thu, 12 Jan 2017 22:10:04 +0100	[thread overview]
Message-ID: <20170112211004.z3wylc7vrubulc3x@pengutronix.de> (raw)
In-Reply-To: <CAOAejn3tPi0fjy+t-UhMTvAq7LfdKJJdbcLw2su1-YAGDZMxew@mail.gmail.com>

On Thu, Jan 12, 2017 at 09:58:23PM +0100, M'boumba Cedric Madianga wrote:
> 2017-01-12 18:49 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > On Thu, Jan 12, 2017 at 02:47:42PM +0100, M'boumba Cedric Madianga wrote:
> >> 2017-01-12 13:03 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> > Hello Cedric,
> >> >
> >> > On Thu, Jan 12, 2017 at 12:23:12PM +0100, M'boumba Cedric Madianga wrote:
> >> >> 2017-01-11 16:39 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> >> > On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
> >> >> >> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> >> >> > 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.
> >> >> >
> >> >> > IMHO that's a hw bug. This makes it for example impossible to implement
> >> >> > SMBus block transfers (I think).
> >> >>
> >> >> This is not correct.
> >> >> Setting STOP/START bit does not mean the the pulse will be sent right now.
> >> >> Here we have just to prepare the hardware for the 2 next pulse but the
> >> >> STOP/START/ACK pulse will be generated at the right time as required
> >> >> by I2C specification.
> >> >> So SMBus block transfer will be possible.
> >> >
> >> > A block transfer consists of a byte that specifies the count of bytes
> >> > yet to come. So the device sends for example:
> >> >
> >> >         0x01 0xab
> >> >
> >> > So when you read the 1 in the first byte it's already too late to set
> >> > STOP to get it after the 2nd byte.
> >> >
> >> > Not sure I got all the required details right, though.
> >>
> >> Ok I understand your use case but I always think that the harware manages it.
> >> If I take the above example, the I2C SMBus block read transaction will
> >> be as below:
> >> S Addr Wr [A] Comm [A]
> >>            S Addr Rd [A] [Count] A [Data1] A [Data2] NA P
> >>
> >> The first message is a single byte-transmission so there is no problem.
> >>
> >> The second message is a N-byte reception with N = 3
> >>
> >> When the I2C controller has finished to send the device address (S
> >> Addr Rd), the ADDR flag is set and an interrupt is raised.
> >> In the routine that handles ADDR event, we set ACK bit in order to
> >> generate ACK pulse as soon as a data byte is received in the shift
> >> register and then we clear the ADDR flag.
> >> Please note that the SCL line is stretched low until ADDR flag is cleared.
> >> So, as far I understand, the device could not sent any data as long as
> >> the SCL line is stretched low. Right ?
> >>
> >> Then, as soon as the SCL line is high, the device could send the first
> >> data byte (Count).
> >> When this byte is received in the shift register, an ACK is
> >> automatically generated as defined during adress match phase and the
> >> data byte is pushed in DR (data register).
> >> Then, an interrupt is raised as RXNE (RX not empty) flag is set.
> >> In the routine that handles RXNE event, as N=3, we just clear all
> >> buffer interrupts in order to avoid another system preemption due to
> >> RXNE event but we does not read the data in DR.
> >
> > In my example I want to receive a block of length 1, so only two bytes
> > are read, a 1 (the length) and the data byte (0xab in my example). I
> > think that as soon as you read the 1 it's already to late to schedule
> > the NA after the next byte?
> 
> Not really. This 2-byte reception is also correctly managed.
> Indeed, in this case, when the controller has sent the device address,
> the ADDR flag is set and an interrupt is raised.
> So, as long as the ADDR flag is not cleared, the SCL line is stretched
> low and the device could not send any data.
> During this address match phase, for a 2-byte reception, we enable
> NACK and set POS bit (ACK/NACK position).
> As POS=1, the NACK will be sent for the next byte which will be
> received in the shift register instead of the current one.
> So in this example, the next byte will be the last one.
> After that, we clear the ADDR flag and the device is allowed to send data.

I didn't follow, but if you are convinced it works that's good. I wonder
if it simplifies the driver if POS=1 is used and so ACK/NACK can be
setup later?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

WARNING: multiple messages have this Message-ID (diff)
From: u.kleine-koenig@pengutronix.de (Uwe Kleine-König)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
Date: Thu, 12 Jan 2017 22:10:04 +0100	[thread overview]
Message-ID: <20170112211004.z3wylc7vrubulc3x@pengutronix.de> (raw)
In-Reply-To: <CAOAejn3tPi0fjy+t-UhMTvAq7LfdKJJdbcLw2su1-YAGDZMxew@mail.gmail.com>

On Thu, Jan 12, 2017 at 09:58:23PM +0100, M'boumba Cedric Madianga wrote:
> 2017-01-12 18:49 GMT+01:00 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> > On Thu, Jan 12, 2017 at 02:47:42PM +0100, M'boumba Cedric Madianga wrote:
> >> 2017-01-12 13:03 GMT+01:00 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> >> > Hello Cedric,
> >> >
> >> > On Thu, Jan 12, 2017 at 12:23:12PM +0100, M'boumba Cedric Madianga wrote:
> >> >> 2017-01-11 16:39 GMT+01:00 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> >> >> > On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
> >> >> >> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> >> >> >> > 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.
> >> >> >
> >> >> > IMHO that's a hw bug. This makes it for example impossible to implement
> >> >> > SMBus block transfers (I think).
> >> >>
> >> >> This is not correct.
> >> >> Setting STOP/START bit does not mean the the pulse will be sent right now.
> >> >> Here we have just to prepare the hardware for the 2 next pulse but the
> >> >> STOP/START/ACK pulse will be generated at the right time as required
> >> >> by I2C specification.
> >> >> So SMBus block transfer will be possible.
> >> >
> >> > A block transfer consists of a byte that specifies the count of bytes
> >> > yet to come. So the device sends for example:
> >> >
> >> >         0x01 0xab
> >> >
> >> > So when you read the 1 in the first byte it's already too late to set
> >> > STOP to get it after the 2nd byte.
> >> >
> >> > Not sure I got all the required details right, though.
> >>
> >> Ok I understand your use case but I always think that the harware manages it.
> >> If I take the above example, the I2C SMBus block read transaction will
> >> be as below:
> >> S Addr Wr [A] Comm [A]
> >>            S Addr Rd [A] [Count] A [Data1] A [Data2] NA P
> >>
> >> The first message is a single byte-transmission so there is no problem.
> >>
> >> The second message is a N-byte reception with N = 3
> >>
> >> When the I2C controller has finished to send the device address (S
> >> Addr Rd), the ADDR flag is set and an interrupt is raised.
> >> In the routine that handles ADDR event, we set ACK bit in order to
> >> generate ACK pulse as soon as a data byte is received in the shift
> >> register and then we clear the ADDR flag.
> >> Please note that the SCL line is stretched low until ADDR flag is cleared.
> >> So, as far I understand, the device could not sent any data as long as
> >> the SCL line is stretched low. Right ?
> >>
> >> Then, as soon as the SCL line is high, the device could send the first
> >> data byte (Count).
> >> When this byte is received in the shift register, an ACK is
> >> automatically generated as defined during adress match phase and the
> >> data byte is pushed in DR (data register).
> >> Then, an interrupt is raised as RXNE (RX not empty) flag is set.
> >> In the routine that handles RXNE event, as N=3, we just clear all
> >> buffer interrupts in order to avoid another system preemption due to
> >> RXNE event but we does not read the data in DR.
> >
> > In my example I want to receive a block of length 1, so only two bytes
> > are read, a 1 (the length) and the data byte (0xab in my example). I
> > think that as soon as you read the 1 it's already to late to schedule
> > the NA after the next byte?
> 
> Not really. This 2-byte reception is also correctly managed.
> Indeed, in this case, when the controller has sent the device address,
> the ADDR flag is set and an interrupt is raised.
> So, as long as the ADDR flag is not cleared, the SCL line is stretched
> low and the device could not send any data.
> During this address match phase, for a 2-byte reception, we enable
> NACK and set POS bit (ACK/NACK position).
> As POS=1, the NACK will be sent for the next byte which will be
> received in the shift register instead of the current one.
> So in this example, the next byte will be the last one.
> After that, we clear the ADDR flag and the device is allowed to send data.

I didn't follow, but if you are convinced it works that's good. I wonder
if it simplifies the driver if POS=1 is used and so ACK/NACK can be
setup later?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

  reply	other threads:[~2017-01-12 21:11 UTC|newest]

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-01-05  9:07 [PATCH v8 0/5] Add support for the STM32F4 I2C M'boumba Cedric Madianga
2017-01-05  9:07 ` M'boumba Cedric Madianga
2017-01-05  9:07 ` [PATCH v8 1/5] dt-bindings: Document the STM32 I2C bindings M'boumba Cedric Madianga
2017-01-05  9:07   ` M'boumba Cedric Madianga
2017-01-05  9:07 ` [PATCH v8 2/5] i2c: Add STM32F4 I2C driver M'boumba Cedric Madianga
2017-01-05  9:07   ` M'boumba Cedric Madianga
2017-01-11  8:22   ` Uwe Kleine-König
2017-01-11  8:22     ` Uwe Kleine-König
2017-01-11  8:22     ` Uwe Kleine-König
2017-01-11 13:58     ` M'boumba Cedric Madianga
2017-01-11 13:58       ` M'boumba Cedric Madianga
2017-01-11 13:58       ` M'boumba Cedric Madianga
2017-01-11 14:20       ` M'boumba Cedric Madianga
2017-01-11 14:20         ` M'boumba Cedric Madianga
2017-01-11 15:42         ` Uwe Kleine-König
2017-01-11 15:42           ` Uwe Kleine-König
2017-01-12 11:25           ` M'boumba Cedric Madianga
2017-01-12 11:25             ` M'boumba Cedric Madianga
2017-01-11 15:39       ` Uwe Kleine-König
2017-01-11 15:39         ` Uwe Kleine-König
2017-01-11 15:39         ` Uwe Kleine-König
2017-01-12 11:23         ` M'boumba Cedric Madianga
2017-01-12 11:23           ` M'boumba Cedric Madianga
2017-01-12 12:03           ` Uwe Kleine-König
2017-01-12 12:03             ` Uwe Kleine-König
2017-01-12 12:03             ` Uwe Kleine-König
2017-01-12 13:47             ` M'boumba Cedric Madianga
2017-01-12 13:47               ` M'boumba Cedric Madianga
2017-01-12 17:49               ` Uwe Kleine-König
2017-01-12 17:49                 ` Uwe Kleine-König
2017-01-12 17:49                 ` Uwe Kleine-König
2017-01-12 20:58                 ` M'boumba Cedric Madianga
2017-01-12 20:58                   ` M'boumba Cedric Madianga
2017-01-12 20:58                   ` M'boumba Cedric Madianga
2017-01-12 21:10                   ` Uwe Kleine-König [this message]
2017-01-12 21:10                     ` Uwe Kleine-König
2017-01-12 21:10                     ` Uwe Kleine-König
2017-01-12 21:28                     ` M'boumba Cedric Madianga
2017-01-12 21:28                       ` M'boumba Cedric Madianga
2017-01-12 21:28                       ` M'boumba Cedric Madianga
2017-01-13  7:26                       ` Uwe Kleine-König
2017-01-13  7:26                         ` Uwe Kleine-König
2017-01-13  8:29                         ` Wolfram Sang
2017-01-13  8:29                           ` Wolfram Sang
2017-01-13  8:29                           ` Wolfram Sang
2017-01-13  8:45                           ` Uwe Kleine-König
2017-01-13  8:45                             ` Uwe Kleine-König
2017-01-13  9:36                             ` M'boumba Cedric Madianga
2017-01-13  9:36                               ` M'boumba Cedric Madianga
2017-01-12 16:17           ` M'boumba Cedric Madianga
2017-01-12 16:17             ` M'boumba Cedric Madianga
2017-01-05  9:07 ` [PATCH v8 3/5] ARM: dts: stm32: Add I2C1 support for STM32F429 SoC M'boumba Cedric Madianga
2017-01-05  9:07   ` M'boumba Cedric Madianga
2017-01-05  9:07 ` [PATCH v8 4/5] ARM: dts: stm32: Add I2C1 support for STM32429 eval board M'boumba Cedric Madianga
2017-01-05  9:07   ` M'boumba Cedric Madianga
2017-01-05  9:07 ` [PATCH v8 5/5] ARM: configs: stm32: Add I2C support for STM32 defconfig M'boumba Cedric Madianga
2017-01-05  9:07   ` M'boumba Cedric Madianga
2017-01-10  9:26 ` [PATCH v8 0/5] Add support for the STM32F4 I2C Linus Walleij
2017-01-10  9:26   ` Linus Walleij
2017-01-10  9:26   ` Linus Walleij

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170112211004.z3wylc7vrubulc3x@pengutronix.de \
    --to=u.kleine-koenig@pengutronix.de \
    --cc=alexandre.torgue@st.com \
    --cc=cedric.madianga@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=mcoquelin.stm32@gmail.com \
    --cc=patrice.chotard@st.com \
    --cc=robh+dt@kernel.org \
    --cc=wsa@the-dreams.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.