Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TWI Accidentally bit left shift after byte transmission #339

Closed
andkae opened this issue Jun 8, 2022 · 27 comments · Fixed by #340
Closed

TWI Accidentally bit left shift after byte transmission #339

andkae opened this issue Jun 8, 2022 · 27 comments · Fixed by #340
Labels
HW Hardware-related

Comments

@andkae
Copy link

andkae commented Jun 8, 2022

Hi Stephan,

at the moment i try to read some bytes from an I2C slave. Therefore i use an properitary I2C macro and the TWI. My belonging testcode looks following:

/* Processor */
#include <neorv32.h>
#include <neorv32_twi.h>


/**
 *  main
 */
int main()
{
    /** Variables **/
    const uint8_t   uint8I2cAdr7 = 15;
    uint8_t         uint8I2cRdBuf[10];


    /* init UART */
    neorv32_uart0_setup(115200, PARITY_NONE, FLOW_CONTROL_NONE);

    /* entry message */
    neorv32_uart0_print("I2C\n");

    /* setup I2C */
    neorv32_twi_disable();
        // neorv32_twi_setup(uint8_t prsc)
    neorv32_twi_setup(3);
    neorv32_twi_mack_enable();
    neorv32_twi_enable();

    /* Read from CHC Dummy */
    neorv32_twi_start_trans((uint8I2cAdr7 << 1) | 0);   // write access
    neorv32_twi_trans(0x23);                            // read pointer
    neorv32_twi_start_trans((uint8I2cAdr7 << 1) | 1);   // read access
    for ( uint8_t i = 0; i < (sizeof(uint8I2cRdBuf)/sizeof(uint8I2cRdBuf[0])); i++ ) {
        neorv32_twi_trans(0xFF);    //  dominant bits '0', set by slave
        uint8I2cRdBuf[i] = neorv32_twi_get_data();
    }
    neorv32_twi_generate_stop();

    /* print to console */
    for ( uint8_t i = 0; i < (sizeof(uint8I2cRdBuf)/sizeof(uint8I2cRdBuf[0])); i++ ) {
        neorv32_uart0_printf("%x;", uint8I2cRdBuf[i]);
    }

    /* stop programm counter */
    while ( 1 ) { }
    return 0;
}

In my example has the first read byte the value 0xF9 (i2c_slave_d -> I2C_DATA).
image

When the processor reads the value is the SFR shifted one possition and i recieve 0xF2:
image

In my opinion is this caused by:

rtx_sreg <= rtx_sreg(7 downto 0) & twi_sda_in_ff(twi_sda_in_ff'left); -- sample and shift left

Whats your opinion?

Thanks for the support.

BR,
Andreas

@andkae andkae changed the title TWI Accidenttally bit left shift after byte transmission TWI Accidentally bit left shift after byte transmission Jun 8, 2022
@stnolting
Copy link
Owner

Hey Andreas!

I will have a closer look a this. I have tested several IC2 devices on real hardware using the the interactive demo_twi program and I thought the module was doing fine 😅


Some questions regarding your setup:

  • What kind of IC2 device are you trying to access?

  • This will activate the host-generated ACK bit. Is this really required (all the time) by your device?

neorv32_twi_mack_enable();

In my opinion is this caused by:

Yes and no. rtx_sreg is a 9-bit shift register. The upper most 8 bits will contain the received data while the LSB contains the
ACK sampled from the device. When reading the sampled data byte, the module only outputs the 8 most significant bits:

data_o(7 downto 0) <= rtx_sreg(8 downto 1);

@andkae
Copy link
Author

andkae commented Jun 8, 2022

Hi Stephan,

This will activate the host-generated ACK bit. Is this really required (all the time) by your device?

If i disable this, same result

//neorv32_twi_mack_enable();

@andkae
Copy link
Author

andkae commented Jun 8, 2022

What kind of IC2 device are you trying to access?

This is the MI2C Inventra Macro
https://git.lumina-sensum.com/LuminaSensum/arm-trusted-firmware/blob/09d40e0e08283a249e7dce0e106c07c5141f9b7e/drivers/mentor/i2c/mi2cv.c

This is a I2C macro which can work as slave as well as master

@andkae
Copy link
Author

andkae commented Jun 8, 2022

In addition the twi_clk_phase:

image

I will have a closer look a this.

Thanks very well :)

@andkae
Copy link
Author

andkae commented Jun 8, 2022

Okay, got it. The Problem is that from the Inventra macro the set I2C speed needs to match with the I2C speed of the TWI:

image

In my case with 60MHz clock is only 240kHz or 1.5MHz possible. This mismatches with the defined I2C modes:

  • 100kHz
  • 400kHz
  • 1Mhz

The shorting of the clock phase causes this issue:
image

@stnolting
Copy link
Owner

In addition the twi_clk_phase:

This signal does not look good... Seems like there is a problem with the clock stretching? Can you check the twi_clk_halt signal? Maybe your TWI device is trying to slow down the transmission.

Anyway, maybe there is a synchronization problem when the module initiates a new operation (START, STOP, transmission). I'll check that.

@stnolting
Copy link
Owner

Okay, got it. The Problem is that from the Inventra macro the set I2C speed needs to match with the I2C speed of the TWI:

Oh okay, so it's just a configuration problem?!

@andkae
Copy link
Author

andkae commented Jun 8, 2022

twi_clk_halt

image

@andkae
Copy link
Author

andkae commented Jun 8, 2022

Oh okay, so it's just a configuration problem?!

There are two views:

  1. The I2C Macro is configured for 400kHz
  2. The TWI runs at 60MHz only at 240kHz or 1.5MHz

The effect comes of the mismatch of both set-upped frequencies

@andkae
Copy link
Author

andkae commented Jun 8, 2022

What do you think are the changes to add a fine graned post divider into the TWI to match better the I2C standard frequencies?

@stnolting
Copy link
Owner

Could you zoom into a situation where ´twi_clk_halt´ is set and twi_clk_phase seems to have two bits set a the same time?

The effect comes of the mismatch of both set-upped frequencies

I do not know your TWI device module, but when just looking at the I2C specs there should be no problem if the master is running on a slower clock than the actual device..?!

@andkae
Copy link
Author

andkae commented Jun 8, 2022

Could you zoom into a situation where ´twi_clk_halt´ is set and twi_clk_phase seems to have two bits set a the same time?

image

@stnolting
Copy link
Owner

stnolting commented Jun 8, 2022

What do you think are the changes to add a fine graned post divider into the TWI to match better the I2C standard frequencies?

Sure, this would be possible by modifying the TWI's clock generator. Right now, we have the standard clock prescalers available:

enum NEORV32_CLOCK_PRSC_enum {
  CLK_PRSC_2    = 0, /**< CPU_CLK (from clk_i top signal) / 2 */
  CLK_PRSC_4    = 1, /**< CPU_CLK (from clk_i top signal) / 4 */
  CLK_PRSC_8    = 2, /**< CPU_CLK (from clk_i top signal) / 8 */
  CLK_PRSC_64   = 3, /**< CPU_CLK (from clk_i top signal) / 64 */
  CLK_PRSC_128  = 4, /**< CPU_CLK (from clk_i top signal) / 128 */
  CLK_PRSC_1024 = 5, /**< CPU_CLK (from clk_i top signal) / 1024 */
  CLK_PRSC_2048 = 6, /**< CPU_CLK (from clk_i top signal) / 2048 */
  CLK_PRSC_4096 = 7  /**< CPU_CLK (from clk_i top signal) / 4096 */
};

Internally, the TWI module adds another div4 scaling factor to this: $f_{SCL} = f_{CPU} / (4 * PRSC)$

How much more "precision" / fine-grained divider options do you thing might be suitable?

@stnolting
Copy link
Owner

Could you zoom into a situation where ´twi_clk_halt´ is set and twi_clk_phase seems to have two bits set a the same time?

I meant some situation like this:

172637309-91dda0df-937c-4b07-abfe-55df6e692cb6

@andkae
Copy link
Author

andkae commented Jun 8, 2022

I meant some situation like this:

Only a combinatoric spike, no problem.

image

@andkae
Copy link
Author

andkae commented Jun 8, 2022

How much more "precision" / fine-grained divider options do you thing might be suitable?

Let my think a while about it. In the mean time i think we can close this issue. Sorry for the "wrong alarm" The topic with the divider we can add as disccusion.

@stnolting
Copy link
Owner

Only a combinatoric spike, no problem.

Right, but unpretty nevertheless 😉

In the mean time i think we can close this issue. Sorry for the "wrong alarm" The topic with the divider we can add as disccusion.

No problem! I found a problem with synchronization due to this issue! Fortunately, this has not occurred so far.

@andkae
Copy link
Author

andkae commented Jun 8, 2022

I do not know your TWI device module, but when just looking at the I2C specs there should be no problem if the master is running on a slower clock than the actual device..?!

Yes, my TWI shorts the clock high phase, and this leads to a double shift in the neorv32_twi.

image

@andkae
Copy link
Author

andkae commented Jun 8, 2022

No problem! I found a problem with synchronization due to this issue! Fortunately, this has not occurred so far.

Oh nice :)

@stnolting
Copy link
Owner

Yes, my TWI shorts the clock high phase, and this leads to a double shift in the neorv32_twi.

Hmm, strange... This should halt the whole sampling logic...

@stnolting
Copy link
Owner

Currently I am doing a complete rework of the TWI module - with special focus on the clock generator and the bus sampling logic (timing). I will do a PR when I'm done. Then we can discuss how to implement a more fine-grained clock configuration.

@stnolting stnolting linked a pull request Jun 9, 2022 that will close this issue
@andkae
Copy link
Author

andkae commented Jun 9, 2022

I checked with #340, now is in the read byte case the ACK of the byte read by the master missing:

image

@stnolting
Copy link
Owner

What do you mean by "missing"?

The TWI module samples the ACK/NACK in the 9th clock cycle. In your waveform the accessed device does not pull SDA low in that cycle so this is interpreted as NACK.

@andkae
Copy link
Author

andkae commented Jun 9, 2022

What do you mean by "missing"?

In the Waveform, I do an byte read from an Slave (Direction: Slave -> Master) There i had expected that the Master confirms the Byte Receive with an ACK. Anyways, i need to check the I2C spec.

@stnolting
Copy link
Owner

stnolting commented Jun 9, 2022

The NEORV32 TWI module will never do the ACK (actively setting SDA low in the 9th clock cycle) unless explicitly enabled ("master-ACK") by

neorv32_twi_mack_enable();

@stnolting
Copy link
Owner

stnolting commented Jun 9, 2022

This gives a nice overview of ACK/NACK: https://www.ti.com/lit/pdf/slva704

Note that some I²C peripherals require the master to send an ACK during transmission (e.g. when reading data from the device).

@stnolting
Copy link
Owner

I think I'm done with reworking the TWI module. 🤔 The bus interaction now looks way better and it even ensures the bus staying claimed until a STOP condition is generated.

@stnolting stnolting added the HW Hardware-related label Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HW Hardware-related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants