Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

Ok so I am going to send the v9 asap.
Thanks

2017-01-13 9:45 GMT+01:00 Uwe Kleine-König :
> On Fri, Jan 13, 2017 at 09:29:03AM +0100, Wolfram Sang wrote:
>>
>> > (But note that this is irrelevant for the patch as the driver doesn't
>> > claim to support this kind of transfer.)
>>
>> Yes, I wanted to mention that, too.
>>
>> I'd think the series is good to go in?
>
> AFAICT there are some unaddressed comments that Cedrics claimed to fix
> before our discussion was dominated by block transfers.
>
> Best regards
> Uwe
>
>
>
> --
> Pengutronix e.K.   | Uwe Kleine-König|
> Industrial Linux Solutions | http://www.pengutronix.de/  |

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

Ok so I am going to send the v9 asap.
Thanks

2017-01-13 9:45 GMT+01:00 Uwe Kleine-König :
> On Fri, Jan 13, 2017 at 09:29:03AM +0100, Wolfram Sang wrote:
>>
>> > (But note that this is irrelevant for the patch as the driver doesn't
>> > claim to support this kind of transfer.)
>>
>> Yes, I wanted to mention that, too.
>>
>> I'd think the series is good to go in?
>
> AFAICT there are some unaddressed comments that Cedrics claimed to fix
> before our discussion was dominated by block transfers.
>
> Best regards
> Uwe
>
>
>
> --
> Pengutronix e.K.   | Uwe Kleine-König|
> Industrial Linux Solutions | http://www.pengutronix.de/  |

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

On Fri, Jan 13, 2017 at 09:29:03AM +0100, Wolfram Sang wrote:
> 
> > (But note that this is irrelevant for the patch as the driver doesn't
> > claim to support this kind of transfer.)
> 
> Yes, I wanted to mention that, too.
> 
> I'd think the series is good to go in?

AFAICT there are some unaddressed comments that Cedrics claimed to fix
before our discussion was dominated by block transfers.

Best regards
Uwe



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

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

On Fri, Jan 13, 2017 at 09:29:03AM +0100, Wolfram Sang wrote:
> 
> > (But note that this is irrelevant for the patch as the driver doesn't
> > claim to support this kind of transfer.)
> 
> Yes, I wanted to mention that, too.
> 
> I'd think the series is good to go in?

AFAICT there are some unaddressed comments that Cedrics claimed to fix
before our discussion was dominated by block transfers.

Best regards
Uwe



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

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Wolfram Sang]

> (But note that this is irrelevant for the patch as the driver doesn't
> claim to support this kind of transfer.)

Yes, I wanted to mention that, too.

I'd think the series is good to go in?



signature.asc
Description: PGP signature

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Wolfram Sang]

> (But note that this is irrelevant for the patch as the driver doesn't
> claim to support this kind of transfer.)

Yes, I wanted to mention that, too.

I'd think the series is good to go in?



signature.asc
Description: PGP signature

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

Hello,

On Thu, Jan 12, 2017 at 10:28:20PM +0100, M'boumba Cedric Madianga wrote:
> Please see below a quote from datasheet that clearly described how to handle
> For 2-byte reception:
> ● Wait until ADDR = 1 (SCL stretched low until the ADDR flag is cleared)
> ● Set ACK low, set POS high
> ● Clear ADDR flag
> ● Wait until BTF = 1 (Data 1 in DR, Data2 in shift register, SCL
> stretched low until a data1 is read)
> ● Set STOP high
> ● Read data 1 and 2

The problem is that you only know that you have a 2 byte transfer after
you read the first byte (and it's a 1). (But note that this is
irrelevant for the patch as the driver doesn't claim to support this
kind of transfer.)

Best regards
Uwe

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

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

Hello,

On Thu, Jan 12, 2017 at 10:28:20PM +0100, M'boumba Cedric Madianga wrote:
> Please see below a quote from datasheet that clearly described how to handle
> For 2-byte reception:
> ● Wait until ADDR = 1 (SCL stretched low until the ADDR flag is cleared)
> ● Set ACK low, set POS high
> ● Clear ADDR flag
> ● Wait until BTF = 1 (Data 1 in DR, Data2 in shift register, SCL
> stretched low until a data1 is read)
> ● Set STOP high
> ● Read data 1 and 2

The problem is that you only know that you have a 2 byte transfer after
you read the first byte (and it's a 1). (But note that this is
irrelevant for the patch as the driver doesn't claim to support this
kind of transfer.)

Best regards
Uwe

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

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

2017-01-12 22:10 GMT+01:00 Uwe Kleine-König :
> 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 :
>> > 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 
>> >> :
>> >> > 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 
>> >> >> :
>> >> >> > 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 
>> >> >> >> :
>> >> >> >> > 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?

Please see below a quote from datasheet that clearly described how to handle
For 2-byte reception:
● Wait until ADDR = 1 (SCL stretched low until the ADDR flag is cleared)
● Set ACK low, set POS high
● Clear ADDR flag
● Wait until BTF = 1 (Data 1 in DR, Data2 in shift register, SCL
stretched low until a data1 is read)
● Set STOP high
● Read data 1 

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

2017-01-12 22:10 GMT+01:00 Uwe Kleine-König :
> 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 :
>> > 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 
>> >> :
>> >> > 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 
>> >> >> :
>> >> >> > 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 
>> >> >> >> :
>> >> >> >> > 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?

Please see below a quote from datasheet that clearly described how to handle
For 2-byte reception:
● Wait until ADDR = 1 (SCL stretched low until the ADDR flag is cleared)
● Set ACK low, set POS high
● Clear ADDR flag
● Wait until BTF = 1 (Data 1 in DR, Data2 in shift register, SCL
stretched low until a data1 is read)
● Set STOP high
● Read data 1 and 2

So we cannot set POS=1 and setup ACK/NACK later as you suggest.

Best regards,

Cedric

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

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 :
> > 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 
> >> :
> >> > 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 
> >> >> :
> >> >> > 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 
> >> >> >> :
> >> >> >> > 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/  |

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

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 :
> > 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 
> >> :
> >> > 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 
> >> >> :
> >> >> > 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 
> >> >> >> :
> >> >> >> > 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/  |

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

2017-01-12 18:49 GMT+01:00 Uwe Kleine-König :
> 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 :
>> > 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 
>> >> :
>> >> > 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 
>> >> >> :
>> >> >> > 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.

When the first data is received in the shift register,  the RXNE flag
is set and an interrupt is raised.
As it is a 2-byte reception, we just clear all interrupts buffer to
avoid another preemption due to RXNE but we does not read DR.

Then, the second and last byte is received in the shift register.
The NACK is automatically sent by I2C controller as it was configured
to do that in the address match phase described above.
Moereover, as the first byte has not been read in DR, the BTF event
flag is set and an interrupt is raised.
Again, the SCL line is stretching low as long as data register has not
been read.
In the meantime, we set STOP bit to generate the pulse and we launch 2
consecutive read of DR to retrieve the 2 data bytes and release SCL
stretching.

In that way, NA and STOP are generated as expected even for a 2-byte reception.

Best regards,

Cedric

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

2017-01-12 18:49 GMT+01:00 Uwe Kleine-König :
> 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 :
>> > 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 
>> >> :
>> >> > 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 
>> >> >> :
>> >> >> > 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.

When the first data is received in the shift register,  the RXNE flag
is set and an interrupt is raised.
As it is a 2-byte reception, we just clear all interrupts buffer to
avoid another preemption due to RXNE but we does not read DR.

Then, the second and last byte is received in the shift register.
The NACK is automatically sent by I2C controller as it was configured
to do that in the address match phase described above.
Moereover, as the first byte has not been read in DR, the BTF event
flag is set and an interrupt is raised.
Again, the SCL line is stretching low as long as data register has not
been read.
In the meantime, we set STOP bit to generate the pulse and we launch 2
consecutive read of DR to retrieve the 2 data bytes and release SCL
stretching.

In that way, NA and STOP are generated as expected even for a 2-byte reception.

Best regards,

Cedric

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

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 :
> > 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 
> >> :
> >> > 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 
> >> >> :
> >> >> > 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?

Best regards
Uwe

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

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

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 :
> > 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 
> >> :
> >> > 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 
> >> >> :
> >> >> > 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?

Best regards
Uwe

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

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

>>> > I don't understand scl_period = 1 µs for Fast Mode. For a bus freqency
>>> > of 400 kHz we need low + high = 2.5 µs. Is there a factor 10 missing
>>> > somewhere?
>>>
>>> As CCR = SCL_period * I2C parent clk frequency with minimal freq =
>>> 2Mhz and SCL_period = 1 we have:
>>> CCR = 1 * 2Mhz = 2.
>>> But to compute, scl_low and scl_high in Fast mode, we have to do the
>>> following thing as Duty=1:
>>> scl_high = 9 * CCR * I2C parent clk period
>>> scl_low = 16 * CCR * I2C parent clk period
>>> In our example:
>>> scl_high = 9 * 2 * 0,005 = 0,09 sec = 9 µs
>>> scl_low = 16 * 2 * 0.005 = 0,16 sec = 16 µs
>>> So low + high = 27 µs > 2,5 µs
>>
>> For me 9 µs + 16 µs is 25 µs, resulting in 40 kHz. That's why I wondered
>> if there is a factor 10 missing somewhere.
>
> Hum ok. I am going to double-check what is wrong because when I check
> with the scope I always reach 400Khz for SCL.
> I will let you know.

There is one point I miss here that is described in the reference manual:
To reach the 400 kHz maximum I²C fast mode clock, the I2C parent rate
must be a multiple of 10 MHz.
So, contrary to what we said in a previous thread, 400 kHz could not
be reached with low frequencies.
In that way, we could compute CCR with duty = 0 by default.
So, I find another formula very close to the first one I pushed in the
first version:

In fast mode, we compute CCR with duty = 0:
t_scl_high = CCR * I2C parent clk period
t_scl_low = 2 *CCR * I2C parent clk period
So, CCR = I2C parent rate / 400 kHz / 3

For example with parent rate = 40 MHz:
CCR = 4000 / 40 / 3 = 33.3 = 33
t_scl_high = 33 * (1 / 200) = 825 ns > 600 ns
t_scl_low = 2 * 16 * (1 / 200) = 1650 ns > 1300 ns

It seems ok now.

Best regards,

Cedric

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

>>> > I don't understand scl_period = 1 µs for Fast Mode. For a bus freqency
>>> > of 400 kHz we need low + high = 2.5 µs. Is there a factor 10 missing
>>> > somewhere?
>>>
>>> As CCR = SCL_period * I2C parent clk frequency with minimal freq =
>>> 2Mhz and SCL_period = 1 we have:
>>> CCR = 1 * 2Mhz = 2.
>>> But to compute, scl_low and scl_high in Fast mode, we have to do the
>>> following thing as Duty=1:
>>> scl_high = 9 * CCR * I2C parent clk period
>>> scl_low = 16 * CCR * I2C parent clk period
>>> In our example:
>>> scl_high = 9 * 2 * 0,005 = 0,09 sec = 9 µs
>>> scl_low = 16 * 2 * 0.005 = 0,16 sec = 16 µs
>>> So low + high = 27 µs > 2,5 µs
>>
>> For me 9 µs + 16 µs is 25 µs, resulting in 40 kHz. That's why I wondered
>> if there is a factor 10 missing somewhere.
>
> Hum ok. I am going to double-check what is wrong because when I check
> with the scope I always reach 400Khz for SCL.
> I will let you know.

There is one point I miss here that is described in the reference manual:
To reach the 400 kHz maximum I²C fast mode clock, the I2C parent rate
must be a multiple of 10 MHz.
So, contrary to what we said in a previous thread, 400 kHz could not
be reached with low frequencies.
In that way, we could compute CCR with duty = 0 by default.
So, I find another formula very close to the first one I pushed in the
first version:

In fast mode, we compute CCR with duty = 0:
t_scl_high = CCR * I2C parent clk period
t_scl_low = 2 *CCR * I2C parent clk period
So, CCR = I2C parent rate / 400 kHz / 3

For example with parent rate = 40 MHz:
CCR = 4000 / 40 / 3 = 33.3 = 33
t_scl_high = 33 * (1 / 200) = 825 ns > 600 ns
t_scl_low = 2 * 16 * (1 / 200) = 1650 ns > 1300 ns

It seems ok now.

Best regards,

Cedric

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

2017-01-12 13:03 GMT+01:00 Uwe Kleine-König :
> 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 :
>> > 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 
>> >> :
>> >> > 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.

After receiving the ACK, the device could send the second data byte (Data1).
When this byte is received in the shift register, an ACK is
automatically generated.
As the first data byte has not been read yet in DR, the BTF (Byte
Transfer Finihed) flag is set and an interrupt is raised.
So, in that case, the SCL line is also streched low as long as the
data register has not been read.
In the routine that handle BTF event, we enable NACK in order to
generate this pulse as soon as the last data byte will be received and
then we read DR register ([Count])
At this moment, SCL line is released and the device could send the
last data byte.

After receiving the ACK, the device could send the third and last data
byte (Data2)
When this byte is received in the shift register, a NACK is
automatically generated as we enable it as explained above.
As the second data byte  (Data1) has not been read yet in DR, the BTF
flag is set again and an interrupt is raised.
The SCL line is stretched low and in that way we could set the STOP
bit to generate this pulse.
Then we run 2 consecutives read of DR to retrieve [Data1] and [Data2]
and to set SCL high.

So, thanks to SCL stretching, it seems that NA and P will be generated
at the right time.

Please let me know if it is not correct.

Best regards,

Cedric

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

2017-01-12 13:03 GMT+01:00 Uwe Kleine-König :
> 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 :
>> > 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 
>> >> :
>> >> > 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.

After receiving the ACK, the device could send the second data byte (Data1).
When this byte is received in the shift register, an ACK is
automatically generated.
As the first data byte has not been read yet in DR, the BTF (Byte
Transfer Finihed) flag is set and an interrupt is raised.
So, in that case, the SCL line is also streched low as long as the
data register has not been read.
In the routine that handle BTF event, we enable NACK in order to
generate this pulse as soon as the last data byte will be received and
then we read DR register ([Count])
At this moment, SCL line is released and the device could send the
last data byte.

After receiving the ACK, the device could send the third and last data
byte (Data2)
When this byte is received in the shift register, a NACK is
automatically generated as we enable it as explained above.
As the second data byte  (Data1) has not been read yet in DR, the BTF
flag is set again and an interrupt is raised.
The SCL line is stretched low and in that way we could set the STOP
bit to generate this pulse.
Then we run 2 consecutives read of DR to retrieve [Data1] and [Data2]
and to set SCL high.

So, thanks to SCL stretching, it seems that NA and P will be generated
at the right time.

Please let me know if it is not correct.

Best regards,

Cedric

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

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 :
> > 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 
> >> :
> >> > 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.

Best regards
Uwe

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

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

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 :
> > 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 
> >> :
> >> > 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.

Best regards
Uwe

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

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

Hi Uwe,

2017-01-11 16:42 GMT+01:00 Uwe Kleine-König :
> Hello Cedric,
>
> On Wed, Jan 11, 2017 at 03:20:41PM +0100, M'boumba Cedric Madianga wrote:
>> >
>> >> +  */
>> >> + reg = 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.
>
> I didn't double check the manual, but I would expect that CR1 isn't
> modified by hardware. So you can cache the result in the driver data
> structure and do the necessary modifications with that one.

CR1 is modified by hardware to clear some bits set by software during
the communication.

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

Best regards,

Cedric

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

Hi Uwe,

2017-01-11 16:42 GMT+01:00 Uwe Kleine-König :
> Hello Cedric,
>
> On Wed, Jan 11, 2017 at 03:20:41PM +0100, M'boumba Cedric Madianga wrote:
>> >
>> >> +  */
>> >> + reg = 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.
>
> I didn't double check the manual, but I would expect that CR1 isn't
> modified by hardware. So you can cache the result in the driver data
> structure and do the necessary modifications with that one.

CR1 is modified by hardware to clear some bits set by software during
the communication.

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

Best regards,

Cedric

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

2017-01-11 16:39 GMT+01:00 Uwe Kleine-König :
> On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
>> Hi Uwe,
>>
>> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König :
>> > Hello Cedric,
>> >
>> > On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
>> >> +/*
>> >> + * In standard mode:
>> >> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk 
>> >> period
>> >> + *
>> >> + * In fast mode:
>> >> + * If Duty = 0; SCL high period = 1  * CCR * I2C parent clk period
>  ^^
>> >> + *   SCL low period  = 2  * CCR * I2C parent clk period
>   ^^
>> >> + * If Duty = 1; SCL high period = 9  * CCR * I2C parent clk period
>  ^^
>> >> + *   SCL low period  = 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
>
> There are several places with double spaces before * marked above.

Ok I see thanks.

>
>> >> + * In order to reach 400 kHz with lower I2C parent clk frequencies we 
>> >> always set
>> >> + * Duty = 1
>> >> + *
>> >> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
>> >> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
>> > s/mode/Mode/
>>
>> ok thanks
>>
>> >
>> >> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low 
>> >> periods
>> >> + * constraints defined by i2c bus specification
>> >
>> > I don't understand scl_period = 1 µs for Fast Mode. For a bus freqency
>> > of 400 kHz we need low + high = 2.5 µs. Is there a factor 10 missing
>> > somewhere?
>>
>> As CCR = SCL_period * I2C parent clk frequency with minimal freq =
>> 2Mhz and SCL_period = 1 we have:
>> CCR = 1 * 2Mhz = 2.
>> But to compute, scl_low and scl_high in Fast mode, we have to do the
>> following thing as Duty=1:
>> scl_high = 9 * CCR * I2C parent clk period
>> scl_low = 16 * CCR * I2C parent clk period
>> In our example:
>> scl_high = 9 * 2 * 0,005 = 0,09 sec = 9 µs
>> scl_low = 16 * 2 * 0.005 = 0,16 sec = 16 µs
>> So low + high = 27 µs > 2,5 µs
>
> For me 9 µs + 16 µs is 25 µs, resulting in 40 kHz. That's why I wondered
> if there is a factor 10 missing somewhere.

Hum ok. I am going to double-check what is wrong because when I check
with the scope I always reach 400Khz for SCL.
I will let you know.
>
>> >> + */
>> >> +static struct stm32f4_i2c_timings i2c_timings[] = {
>> >> [...]
>> >> +
>> >> +/**
>> >> + * 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 = i2c_dev->base + STM32F4_I2C_CR1;
>> >> + int ret = 0;
>> >> +
>> >> + /* Disable I2C */
>> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
>> >> +
>> >> + ret = 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 = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
>> >> +  status,
>> >> +  !(status & STM32F4_I2C_SR2_BUSY),
>> >> +  10, 1000);
>> >> + if (ret) {
>> >> + dev_dbg(i2c_dev->dev, "bus not free\n");
>> >> + ret = -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 

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

2017-01-11 16:39 GMT+01:00 Uwe Kleine-König :
> On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
>> Hi Uwe,
>>
>> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König :
>> > Hello Cedric,
>> >
>> > On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
>> >> +/*
>> >> + * In standard mode:
>> >> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk 
>> >> period
>> >> + *
>> >> + * In fast mode:
>> >> + * If Duty = 0; SCL high period = 1  * CCR * I2C parent clk period
>  ^^
>> >> + *   SCL low period  = 2  * CCR * I2C parent clk period
>   ^^
>> >> + * If Duty = 1; SCL high period = 9  * CCR * I2C parent clk period
>  ^^
>> >> + *   SCL low period  = 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
>
> There are several places with double spaces before * marked above.

Ok I see thanks.

>
>> >> + * In order to reach 400 kHz with lower I2C parent clk frequencies we 
>> >> always set
>> >> + * Duty = 1
>> >> + *
>> >> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
>> >> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
>> > s/mode/Mode/
>>
>> ok thanks
>>
>> >
>> >> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low 
>> >> periods
>> >> + * constraints defined by i2c bus specification
>> >
>> > I don't understand scl_period = 1 µs for Fast Mode. For a bus freqency
>> > of 400 kHz we need low + high = 2.5 µs. Is there a factor 10 missing
>> > somewhere?
>>
>> As CCR = SCL_period * I2C parent clk frequency with minimal freq =
>> 2Mhz and SCL_period = 1 we have:
>> CCR = 1 * 2Mhz = 2.
>> But to compute, scl_low and scl_high in Fast mode, we have to do the
>> following thing as Duty=1:
>> scl_high = 9 * CCR * I2C parent clk period
>> scl_low = 16 * CCR * I2C parent clk period
>> In our example:
>> scl_high = 9 * 2 * 0,005 = 0,09 sec = 9 µs
>> scl_low = 16 * 2 * 0.005 = 0,16 sec = 16 µs
>> So low + high = 27 µs > 2,5 µs
>
> For me 9 µs + 16 µs is 25 µs, resulting in 40 kHz. That's why I wondered
> if there is a factor 10 missing somewhere.

Hum ok. I am going to double-check what is wrong because when I check
with the scope I always reach 400Khz for SCL.
I will let you know.
>
>> >> + */
>> >> +static struct stm32f4_i2c_timings i2c_timings[] = {
>> >> [...]
>> >> +
>> >> +/**
>> >> + * 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 = i2c_dev->base + STM32F4_I2C_CR1;
>> >> + int ret = 0;
>> >> +
>> >> + /* Disable I2C */
>> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
>> >> +
>> >> + ret = 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 = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
>> >> +  status,
>> >> +  !(status & STM32F4_I2C_SR2_BUSY),
>> >> +  10, 1000);
>> >> + if (ret) {
>> >> + dev_dbg(i2c_dev->dev, "bus not free\n");
>> >> + ret = -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 

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
> Hi Uwe,
> 
> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König :
> > Hello Cedric,
> >
> > On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
> >> +/*
> >> + * In standard mode:
> >> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk 
> >> period
> >> + *
> >> + * In fast mode:
> >> + * If Duty = 0; SCL high period = 1  * CCR * I2C parent clk period
 ^^
> >> + *   SCL low period  = 2  * CCR * I2C parent clk period
  ^^
> >> + * If Duty = 1; SCL high period = 9  * CCR * I2C parent clk period
 ^^
> >> + *   SCL low period  = 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

There are several places with double spaces before * marked above.

> >> + * In order to reach 400 kHz with lower I2C parent clk frequencies we 
> >> always set
> >> + * Duty = 1
> >> + *
> >> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
> >> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
> > s/mode/Mode/
> 
> ok thanks
> 
> >
> >> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low 
> >> periods
> >> + * constraints defined by i2c bus specification
> >
> > I don't understand scl_period = 1 µs for Fast Mode. For a bus freqency
> > of 400 kHz we need low + high = 2.5 µs. Is there a factor 10 missing
> > somewhere?
> 
> As CCR = SCL_period * I2C parent clk frequency with minimal freq =
> 2Mhz and SCL_period = 1 we have:
> CCR = 1 * 2Mhz = 2.
> But to compute, scl_low and scl_high in Fast mode, we have to do the
> following thing as Duty=1:
> scl_high = 9 * CCR * I2C parent clk period
> scl_low = 16 * CCR * I2C parent clk period
> In our example:
> scl_high = 9 * 2 * 0,005 = 0,09 sec = 9 µs
> scl_low = 16 * 2 * 0.005 = 0,16 sec = 16 µs
> So low + high = 27 µs > 2,5 µs

For me 9 µs + 16 µs is 25 µs, resulting in 40 kHz. That's why I wondered
if there is a factor 10 missing somewhere.

> >> + */
> >> +static struct stm32f4_i2c_timings i2c_timings[] = {
> >> [...]
> >> +
> >> +/**
> >> + * 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 = i2c_dev->base + STM32F4_I2C_CR1;
> >> + int ret = 0;
> >> +
> >> + /* Disable I2C */
> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
> >> +
> >> + ret = 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 = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
> >> +  status,
> >> +  !(status & STM32F4_I2C_SR2_BUSY),
> >> +  10, 1000);
> >> + if (ret) {
> >> + dev_dbg(i2c_dev->dev, "bus not free\n");
> >> + ret = -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 = _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 = _dev->msg;
> >> + u32 rbuf;
> >> +
> >> + rbuf = 

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
> Hi Uwe,
> 
> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König :
> > Hello Cedric,
> >
> > On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
> >> +/*
> >> + * In standard mode:
> >> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk 
> >> period
> >> + *
> >> + * In fast mode:
> >> + * If Duty = 0; SCL high period = 1  * CCR * I2C parent clk period
 ^^
> >> + *   SCL low period  = 2  * CCR * I2C parent clk period
  ^^
> >> + * If Duty = 1; SCL high period = 9  * CCR * I2C parent clk period
 ^^
> >> + *   SCL low period  = 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

There are several places with double spaces before * marked above.

> >> + * In order to reach 400 kHz with lower I2C parent clk frequencies we 
> >> always set
> >> + * Duty = 1
> >> + *
> >> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
> >> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
> > s/mode/Mode/
> 
> ok thanks
> 
> >
> >> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low 
> >> periods
> >> + * constraints defined by i2c bus specification
> >
> > I don't understand scl_period = 1 µs for Fast Mode. For a bus freqency
> > of 400 kHz we need low + high = 2.5 µs. Is there a factor 10 missing
> > somewhere?
> 
> As CCR = SCL_period * I2C parent clk frequency with minimal freq =
> 2Mhz and SCL_period = 1 we have:
> CCR = 1 * 2Mhz = 2.
> But to compute, scl_low and scl_high in Fast mode, we have to do the
> following thing as Duty=1:
> scl_high = 9 * CCR * I2C parent clk period
> scl_low = 16 * CCR * I2C parent clk period
> In our example:
> scl_high = 9 * 2 * 0,005 = 0,09 sec = 9 µs
> scl_low = 16 * 2 * 0.005 = 0,16 sec = 16 µs
> So low + high = 27 µs > 2,5 µs

For me 9 µs + 16 µs is 25 µs, resulting in 40 kHz. That's why I wondered
if there is a factor 10 missing somewhere.

> >> + */
> >> +static struct stm32f4_i2c_timings i2c_timings[] = {
> >> [...]
> >> +
> >> +/**
> >> + * 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 = i2c_dev->base + STM32F4_I2C_CR1;
> >> + int ret = 0;
> >> +
> >> + /* Disable I2C */
> >> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
> >> +
> >> + ret = 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 = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
> >> +  status,
> >> +  !(status & STM32F4_I2C_SR2_BUSY),
> >> +  10, 1000);
> >> + if (ret) {
> >> + dev_dbg(i2c_dev->dev, "bus not free\n");
> >> + ret = -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 = _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 = _dev->msg;
> >> + u32 rbuf;
> >> +
> >> + rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
> 

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

Hello Cedric,

On Wed, Jan 11, 2017 at 03:20:41PM +0100, M'boumba Cedric Madianga wrote:
> >
> >> +  */
> >> + reg = 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.

I didn't double check the manual, but I would expect that CR1 isn't
modified by hardware. So you can cache the result in the driver data
structure and do the necessary modifications with that one.

Best regards
Uwe

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

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

Hello Cedric,

On Wed, Jan 11, 2017 at 03:20:41PM +0100, M'boumba Cedric Madianga wrote:
> >
> >> +  */
> >> + reg = 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.

I didn't double check the manual, but I would expect that CR1 isn't
modified by hardware. So you can cache the result in the driver data
structure and do the necessary modifications with that one.

Best regards
Uwe

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

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

>
>> +  */
>> + reg = 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önig :
>> Hello Cedric,
>>
>> On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
>>> +/*
>>> + * In standard mode:
>>> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk 
>>> period
>>> + *
>>> + * In fast mode:
>>> + * If Duty = 0; SCL high period = 1  * CCR * I2C parent clk period
>>> + *   SCL low period  = 2  * CCR * I2C parent clk period
>>> + * If Duty = 1; SCL high period = 9  * CCR * I2C parent clk period
>>> + *   SCL low period  = 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 = 1
>>> + *
>>> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
>>> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
>> s/mode/Mode/
>
> ok thanks
>
>>
>>> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low 
>>> periods
>>> + * constraints defined by i2c bus specification
>>
>> I don't understand scl_period = 1 µs for Fast Mode. For a bus freqency
>> of 400 kHz we need low + high = 2.5 µs. Is there a factor 10 missing
>> somewhere?
>
> As CCR = SCL_period * I2C parent clk frequency with minimal freq =
> 2Mhz and SCL_period = 1 we have:
> CCR = 1 * 2Mhz = 2.
> But to compute, scl_low and scl_high in Fast mode, we have to do the
> following thing as Duty=1:
> scl_high = 9 * CCR * I2C parent clk period
> scl_low = 16 * CCR * I2C parent clk period
> In our example:
> scl_high = 9 * 2 * 0,005 = 0,09 sec = 9 µs
> scl_low = 16 * 2 * 0.005 = 0,16 sec = 16 µs
> So low + high = 27 µs > 2,5 µs
>
>>
>>> + */
>>> +static struct stm32f4_i2c_timings i2c_timings[] = {
>>> [...]
>>> +
>>> +/**
>>> + * 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 = i2c_dev->base + STM32F4_I2C_CR1;
>>> + int ret = 0;
>>> +
>>> + /* Disable I2C */
>>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
>>> +
>>> + ret = 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 = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
>>> +  status,
>>> +  !(status & STM32F4_I2C_SR2_BUSY),
>>> +  10, 1000);
>>> + if (ret) {
>>> + dev_dbg(i2c_dev->dev, "bus not free\n");
>>> + ret = -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: 

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

>
>> +  */
>> + reg = 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önig :
>> Hello Cedric,
>>
>> On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
>>> +/*
>>> + * In standard mode:
>>> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk 
>>> period
>>> + *
>>> + * In fast mode:
>>> + * If Duty = 0; SCL high period = 1  * CCR * I2C parent clk period
>>> + *   SCL low period  = 2  * CCR * I2C parent clk period
>>> + * If Duty = 1; SCL high period = 9  * CCR * I2C parent clk period
>>> + *   SCL low period  = 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 = 1
>>> + *
>>> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
>>> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
>> s/mode/Mode/
>
> ok thanks
>
>>
>>> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low 
>>> periods
>>> + * constraints defined by i2c bus specification
>>
>> I don't understand scl_period = 1 µs for Fast Mode. For a bus freqency
>> of 400 kHz we need low + high = 2.5 µs. Is there a factor 10 missing
>> somewhere?
>
> As CCR = SCL_period * I2C parent clk frequency with minimal freq =
> 2Mhz and SCL_period = 1 we have:
> CCR = 1 * 2Mhz = 2.
> But to compute, scl_low and scl_high in Fast mode, we have to do the
> following thing as Duty=1:
> scl_high = 9 * CCR * I2C parent clk period
> scl_low = 16 * CCR * I2C parent clk period
> In our example:
> scl_high = 9 * 2 * 0,005 = 0,09 sec = 9 µs
> scl_low = 16 * 2 * 0.005 = 0,16 sec = 16 µs
> So low + high = 27 µs > 2,5 µs
>
>>
>>> + */
>>> +static struct stm32f4_i2c_timings i2c_timings[] = {
>>> [...]
>>> +
>>> +/**
>>> + * 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 = i2c_dev->base + STM32F4_I2C_CR1;
>>> + int ret = 0;
>>> +
>>> + /* Disable I2C */
>>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
>>> +
>>> + ret = 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 = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
>>> +  status,
>>> +  !(status & STM32F4_I2C_SR2_BUSY),
>>> +  10, 1000);
>>> + if (ret) {
>>> + dev_dbg(i2c_dev->dev, "bus not free\n");
>>> + ret = -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 

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

Hi Uwe,

2017-01-11 9:22 GMT+01:00 Uwe Kleine-König :
> Hello Cedric,
>
> On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
>> +/*
>> + * In standard mode:
>> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk 
>> period
>> + *
>> + * In fast mode:
>> + * If Duty = 0; SCL high period = 1  * CCR * I2C parent clk period
>> + *   SCL low period  = 2  * CCR * I2C parent clk period
>> + * If Duty = 1; SCL high period = 9  * CCR * I2C parent clk period
>> + *   SCL low period  = 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 = 1
>> + *
>> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
>> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
> s/mode/Mode/

ok thanks

>
>> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low periods
>> + * constraints defined by i2c bus specification
>
> I don't understand scl_period = 1 µs for Fast Mode. For a bus freqency
> of 400 kHz we need low + high = 2.5 µs. Is there a factor 10 missing
> somewhere?

As CCR = SCL_period * I2C parent clk frequency with minimal freq =
2Mhz and SCL_period = 1 we have:
CCR = 1 * 2Mhz = 2.
But to compute, scl_low and scl_high in Fast mode, we have to do the
following thing as Duty=1:
scl_high = 9 * CCR * I2C parent clk period
scl_low = 16 * CCR * I2C parent clk period
In our example:
scl_high = 9 * 2 * 0,005 = 0,09 sec = 9 µs
scl_low = 16 * 2 * 0.005 = 0,16 sec = 16 µs
So low + high = 27 µs > 2,5 µs

>
>> + */
>> +static struct stm32f4_i2c_timings i2c_timings[] = {
>> [...]
>> +
>> +/**
>> + * 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 = i2c_dev->base + STM32F4_I2C_CR1;
>> + int ret = 0;
>> +
>> + /* Disable I2C */
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
>> +
>> + ret = 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 = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
>> +  status,
>> +  !(status & STM32F4_I2C_SR2_BUSY),
>> +  10, 1000);
>> + if (ret) {
>> + dev_dbg(i2c_dev->dev, "bus not free\n");
>> + ret = -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 = _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 = _dev->msg;
>> + u32 rbuf;
>> +
>> + rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
>> + *msg->buf++ = rbuf & 0xff;
>
> This is unnecessary. buf has an 8 bit wide type so
>
> *msg->buf++ = 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 = _dev->msg;
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +
>> + stm32f4_i2c_disable_irq(i2c_dev);
>> +
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + if (msg->stop)
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> 

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[M'boumba Cedric Madianga]

Hi Uwe,

2017-01-11 9:22 GMT+01:00 Uwe Kleine-König :
> Hello Cedric,
>
> On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
>> +/*
>> + * In standard mode:
>> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk 
>> period
>> + *
>> + * In fast mode:
>> + * If Duty = 0; SCL high period = 1  * CCR * I2C parent clk period
>> + *   SCL low period  = 2  * CCR * I2C parent clk period
>> + * If Duty = 1; SCL high period = 9  * CCR * I2C parent clk period
>> + *   SCL low period  = 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 = 1
>> + *
>> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
>> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
> s/mode/Mode/

ok thanks

>
>> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low periods
>> + * constraints defined by i2c bus specification
>
> I don't understand scl_period = 1 µs for Fast Mode. For a bus freqency
> of 400 kHz we need low + high = 2.5 µs. Is there a factor 10 missing
> somewhere?

As CCR = SCL_period * I2C parent clk frequency with minimal freq =
2Mhz and SCL_period = 1 we have:
CCR = 1 * 2Mhz = 2.
But to compute, scl_low and scl_high in Fast mode, we have to do the
following thing as Duty=1:
scl_high = 9 * CCR * I2C parent clk period
scl_low = 16 * CCR * I2C parent clk period
In our example:
scl_high = 9 * 2 * 0,005 = 0,09 sec = 9 µs
scl_low = 16 * 2 * 0.005 = 0,16 sec = 16 µs
So low + high = 27 µs > 2,5 µs

>
>> + */
>> +static struct stm32f4_i2c_timings i2c_timings[] = {
>> [...]
>> +
>> +/**
>> + * 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 = i2c_dev->base + STM32F4_I2C_CR1;
>> + int ret = 0;
>> +
>> + /* Disable I2C */
>> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
>> +
>> + ret = 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 = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
>> +  status,
>> +  !(status & STM32F4_I2C_SR2_BUSY),
>> +  10, 1000);
>> + if (ret) {
>> + dev_dbg(i2c_dev->dev, "bus not free\n");
>> + ret = -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 = _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 = _dev->msg;
>> + u32 rbuf;
>> +
>> + rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
>> + *msg->buf++ = rbuf & 0xff;
>
> This is unnecessary. buf has an 8 bit wide type so
>
> *msg->buf++ = 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 = _dev->msg;
>> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
>> +
>> + stm32f4_i2c_disable_irq(i2c_dev);
>> +
>> + reg = i2c_dev->base + STM32F4_I2C_CR1;
>> + if (msg->stop)
>> + stm32f4_i2c_set_bits(reg, STM32F4_I2C_CR1_STOP);
>> + else
>> + 

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

Hello Cedric,

On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
> +/*
> + * In standard mode:
> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk 
> period
> + *
> + * In fast mode:
> + * If Duty = 0; SCL high period = 1  * CCR * I2C parent clk period
> + *   SCL low period  = 2  * CCR * I2C parent clk period
> + * If Duty = 1; SCL high period = 9  * CCR * I2C parent clk period
> + *   SCL low period  = 16 * CCR * I2C parent clk period
s/  \*/ */ several times

> + * In order to reach 400 kHz with lower I2C parent clk frequencies we always 
> set
> + * Duty = 1
> + *
> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
s/mode/Mode/

> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low periods
> + * constraints defined by i2c bus specification

I don't understand scl_period = 1 µs for Fast Mode. For a bus freqency
of 400 kHz we need low + high = 2.5 µs. Is there a factor 10 missing
somewhere?

> + */
> +static struct stm32f4_i2c_timings i2c_timings[] = {
> [...]
> +
> +/**
> + * 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 = i2c_dev->base + STM32F4_I2C_CR1;
> + int ret = 0;
> +
> + /* Disable I2C */
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
> +
> + ret = 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.

> + return ret;

return 0;

> +}
> +
> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + u32 status;
> + int ret;
> +
> + ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
> +  status,
> +  !(status & STM32F4_I2C_SR2_BUSY),
> +  10, 1000);
> + if (ret) {
> + dev_dbg(i2c_dev->dev, "bus not free\n");
> + ret = -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 = _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 = _dev->msg;
> + u32 rbuf;
> +
> + rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
> + *msg->buf++ = rbuf & 0xff;

This is unnecessary. buf has an 8 bit wide type so

*msg->buf++ = rbuf;

has the same effect. (ISTR this is something I already pointed out
earlier?)

> + msg->count--;
> +}
> +
> +static void stm32f4_i2c_terminate_xfer(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_msg *msg = _dev->msg;
> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> +
> + stm32f4_i2c_disable_irq(i2c_dev);
> +
> + reg = 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(_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 = _dev->msg;
> + void __iomem *reg = 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 

Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver

[Uwe Kleine-König]

Hello Cedric,

On Thu, Jan 05, 2017 at 10:07:23AM +0100, M'boumba Cedric Madianga wrote:
> +/*
> + * In standard mode:
> + * SCL period = SCL high period = SCL low period = CCR * I2C parent clk 
> period
> + *
> + * In fast mode:
> + * If Duty = 0; SCL high period = 1  * CCR * I2C parent clk period
> + *   SCL low period  = 2  * CCR * I2C parent clk period
> + * If Duty = 1; SCL high period = 9  * CCR * I2C parent clk period
> + *   SCL low period  = 16 * CCR * I2C parent clk period
s/  \*/ */ several times

> + * In order to reach 400 kHz with lower I2C parent clk frequencies we always 
> set
> + * Duty = 1
> + *
> + * For both modes, we have CCR = SCL period * I2C parent clk frequency
> + * with scl_period = 5 microseconds in Standard mode and scl_period = 1
s/mode/Mode/

> + * microsecond in Fast Mode in order to satisfy scl_high and scl_low periods
> + * constraints defined by i2c bus specification

I don't understand scl_period = 1 µs for Fast Mode. For a bus freqency
of 400 kHz we need low + high = 2.5 µs. Is there a factor 10 missing
somewhere?

> + */
> +static struct stm32f4_i2c_timings i2c_timings[] = {
> [...]
> +
> +/**
> + * 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 = i2c_dev->base + STM32F4_I2C_CR1;
> + int ret = 0;
> +
> + /* Disable I2C */
> + stm32f4_i2c_clr_bits(reg, STM32F4_I2C_CR1_PE);
> +
> + ret = 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.

> + return ret;

return 0;

> +}
> +
> +static int stm32f4_i2c_wait_free_bus(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + u32 status;
> + int ret;
> +
> + ret = readl_relaxed_poll_timeout(i2c_dev->base + STM32F4_I2C_SR2,
> +  status,
> +  !(status & STM32F4_I2C_SR2_BUSY),
> +  10, 1000);
> + if (ret) {
> + dev_dbg(i2c_dev->dev, "bus not free\n");
> + ret = -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 = _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 = _dev->msg;
> + u32 rbuf;
> +
> + rbuf = readl_relaxed(i2c_dev->base + STM32F4_I2C_DR);
> + *msg->buf++ = rbuf & 0xff;

This is unnecessary. buf has an 8 bit wide type so

*msg->buf++ = rbuf;

has the same effect. (ISTR this is something I already pointed out
earlier?)

> + msg->count--;
> +}
> +
> +static void stm32f4_i2c_terminate_xfer(struct stm32f4_i2c_dev *i2c_dev)
> +{
> + struct stm32f4_i2c_msg *msg = _dev->msg;
> + void __iomem *reg = i2c_dev->base + STM32F4_I2C_CR2;
> +
> + stm32f4_i2c_disable_irq(i2c_dev);
> +
> + reg = 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(_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 = _dev->msg;
> + void __iomem *reg = 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