raspberrypi / pico-feedback

24 stars 2 forks source link

Documentation: RP2040 Datasheet - MASTER_ON_HOLD bit in IC_RAW_INTR_STAT Register #133

Closed fivdi closed 2 years ago

fivdi commented 3 years ago

The description of the MASTER_ON_HOLD bit in the IC_RAW_INTR_STAT register on page 499 of release 1.3.1 of the RP2040 Datasheet is as follows:

Indicates whether master is holding the bus and TX FIFO is empty. Enabled only when I2C_DYNAMIC_TAR_UPDATE=1 and IC_EMPTYFIFO_HOLD_MASTER_EN=1.

Reset value: 0x0 0x0 → MASTER_ON_HOLD interrupt is inactive 0x1 → MASTER_ON_HOLD interrupt is active

Unfortunately, there is no documentation for I2C_DYNAMIC_TAR_UPDATE or IC_EMPTYFIFO_HOLD_MASTER_EN in the datasheet which makes things difficult to understand. My initial assumption was that these must be bits in a register but this is not the case.

In the pico-sdk, I2C_DYNAMIC_TAR_UPDATE is mentioned here and IC_EMPTYFIFO_HOLD_MASTER_EN is mentioned here indicating that they are configuration constants for the Synopsys I2C hardware. Both have the value one indicating that the MASTER_ON_HOLD bit in the IC_RAW_INTR_STAT is enabled.

It would be better if it was easier to figure things out here.

lurch commented 3 years ago

indicating that they are configuration constants for the Synopsys I2C hardware

I believe that's correct? ping @nickfrancis55

Wren6991 commented 3 years ago

So

So I think we need to figure out exactly how the parameters listed in that struct header ended up being incorrect, and fix them. We should also commit all the config-dependent if clauses in the register docs to avoid future confusion.

For reference here is a raw configuration export from the I2C hardware on RP2040

#define IC_ULTRA_FAST_MODE 0x0 #define IC_UFM_TBUF_CNT_DEFAULT 0x8 #define IC_UFM_SCL_LOW_COUNT 0x0008 #define IC_UFM_SCL_HIGH_COUNT 0x0006 #define IC_TX_TL 0x0 #define IC_TX_CMD_BLOCK 0x1 #define IC_HAS_DMA 0x1 #define IC_HAS_ASYNC_FIFO 0x0 #define IC_SMBUS_ARP 0x0 #define IC_FIRST_DATA_BYTE_STATUS 0x1 #define IC_INTR_IO 0x1 #define IC_MASTER_MODE 0x1 #define IC_DEFAULT_ACK_GENERAL_CALL 0x1 #define IC_INTR_POL 0x1 #define IC_OPTIONAL_SAR 0x0 #define IC_DEFAULT_TAR_SLAVE_ADDR 0x055 #define IC_DEFAULT_SLAVE_ADDR 0x055 #define IC_DEFAULT_HS_SPKLEN 0x1 #define IC_FS_SCL_HIGH_COUNT 0x0006 #define IC_HS_SCL_LOW_COUNT 0x0008 #define IC_DEVICE_ID_VALUE 0x0 #define IC_10BITADDR_MASTER 0x0 #define IC_CLK_FREQ_OPTIMIZATION 0x0 #define IC_DEFAULT_FS_SPKLEN 0x7 #define IC_ADD_ENCODED_PARAMS 0x0 #define IC_DEFAULT_SDA_HOLD 0x000001 #define IC_DEFAULT_SDA_SETUP 0x64 #define IC_AVOID_RX_FIFO_FLUSH_ON_TX_ABRT 0x0 #define IC_CLOCK_PERIOD 100 #define IC_EMPTYFIFO_HOLD_MASTER_EN 1 #define IC_RESTART_EN 0x1 #define IC_TX_CMD_BLOCK_DEFAULT 0x0 #define IC_BUS_CLEAR_FEATURE 0x0 #define IC_CAP_LOADING 100 #define IC_FS_SCL_LOW_COUNT 0x000d #define APB_DATA_WIDTH 32 #define IC_SDA_STUCK_TIMEOUT_DEFAULT 0xffffffff #define IC_SLV_DATA_NACK_ONLY 0x1 #define IC_10BITADDR_SLAVE 0x0 #define IC_CLK_TYPE 0x0 #define IC_SMBUS_UDID_MSB 0x0 #define IC_SMBUS_SUSPEND_ALERT 0x0 #define IC_HS_SCL_HIGH_COUNT 0x0006 #define IC_SLV_RESTART_DET_EN 0x1 #define IC_SMBUS 0x0 #define IC_OPTIONAL_SAR_DEFAULT 0x0 #define IC_PERSISTANT_SLV_ADDR_DEFAULT 0x0 #define IC_USE_COUNTS 0x0 #define IC_RX_BUFFER_DEPTH 16 #define IC_SCL_STUCK_TIMEOUT_DEFAULT 0xffffffff #define IC_RX_FULL_HLD_BUS_EN 0x1 #define IC_SLAVE_DISABLE 0x1 #define IC_RX_TL 0x0 #define IC_DEVICE_ID 0x0 #define IC_HC_COUNT_VALUES 0x0 #define I2C_DYNAMIC_TAR_UPDATE 0 #define IC_SMBUS_CLK_LOW_MEXT_DEFAULT 0xffffffff #define IC_SMBUS_CLK_LOW_SEXT_DEFAULT 0xffffffff #define IC_HS_MASTER_CODE 0x1 #define IC_SMBUS_RST_IDLE_CNT_DEFAULT 0xffff #define IC_SMBUS_UDID_LSB_DEFAULT 0xffffffff #define IC_SS_SCL_HIGH_COUNT 0x0028 #define IC_SS_SCL_LOW_COUNT 0x002f #define IC_MAX_SPEED_MODE 0x2 #define IC_STAT_FOR_CLK_STRETCH 0x0 #define IC_STOP_DET_IF_MASTER_ACTIVE 0x0 #define IC_DEFAULT_UFM_SPKLEN 0x1 #define IC_TX_BUFFER_DEPTH 16
lurch commented 3 years ago

This has been fixed in v1.4.1 of the PDF documentation.

fivdi commented 3 years ago

There's one occurrence left. MST_ON_HOLD on page 484.

lurch commented 3 years ago

Doh! I didn't spot that because it wasn't named MASTER_ON_HOLD :facepalm: Thanks, will fix...

aallan commented 3 years ago

Thanks, will fix...

@lurch Did you fix though? 😄

lurch commented 3 years ago

Still in backlog.

lurch commented 2 years ago

Thanks, will be fixed in the next PDF release of the databooks.