driver: espi: add espi driver for rts5912 #86427
Conversation
zephyrbot
requested review from
albertofloyd,
JasonLin-RealTek,
jvasanth1 and
VenkatKotakonda
drivers/espi/espi_realtek_rts5912.c
Outdated
|
|
||
| static const struct espi_rts5912_config espi_rts5912_config = { | ||
| .espislv_base = DT_INST_REG_ADDR_BY_IDX(0, 0), | ||
| .espislv_clk_grp = DT_CLOCKS_CELL_BY_NAME(DT_NODELABEL(espi0), espi_slave, clk_grp), |
There was a problem hiding this comment.
You shouldn't rely on node labels inside a device drver. You can access the node ID using DT_DRV_INST.
| .espislv_clk_grp = DT_CLOCKS_CELL_BY_NAME(DT_NODELABEL(espi0), espi_slave, clk_grp), | |
| .espislv_clk_grp = DT_CLOCKS_CELL_BY_NAME(DT_DRV_INST(0), espi_slave, clk_grp), |
There was a problem hiding this comment.
This still isn't fixed. You're still using the nodelabel instead of the DT_DRV_INST
drivers/espi/espi_realtek_rts5912.c
Outdated
| .pcfg = PINCTRL_DT_INST_DEV_CONFIG_GET(0), | ||
| }; | ||
|
|
||
| DEVICE_DT_INST_DEFINE(0, &espi_rts5912_init, NULL, &espi_rts5912_data, &espi_rts5912_config, |
There was a problem hiding this comment.
Add a BUILD_ASSERT that checks that only one driver instance has been enabled.
drivers/espi/espi_realtek_rts5912.c
Outdated
| static struct espi_rts5912_data espi_rts5912_data; | ||
|
|
||
| static const struct espi_rts5912_config espi_rts5912_config = { | ||
| .espislv_base = DT_INST_REG_ADDR_BY_IDX(0, 0), |
There was a problem hiding this comment.
Add a reg-names property to the devicetree node and use DT_INST_REG_ADDR_BY_NAME instead.
drivers/espi/Kconfig.rts5912
Outdated
| config ESPI_OOB_CHANNEL | ||
| default y | ||
|
|
||
| config ESPI_FLASH_CHANNEL | ||
| default y | ||
|
|
||
| config ESPI_PERIPHERAL_8042_KBC | ||
| default y | ||
|
|
||
| config ESPI_PERIPHERAL_DEBUG_PORT_80 | ||
| default y | ||
|
|
||
| config ESPI_PERIPHERAL_CUSTOM_OPCODE | ||
| default y | ||
|
|
||
| config ESPI_PERIPHERAL_ACPI_SHM_REGION | ||
| default y | ||
|
|
||
| config ESPI_PERIPHERAL_EC_HOST_CMD | ||
| default y |
There was a problem hiding this comment.
This commit is very big. Is it possible to break up into smaller commits, adding in each ESPI feature separately?
JhanBoChao-Realtek
force-pushed
the
rtk_espi
branch
2 times, most recently
from
4a45d70 to
744ccd2
Compare
soc/realtek/ec/rts5912/reg/reg_emi.h
Outdated
| #ifndef ZEPHYR_SOC_REALTEK_RTS5912_REG_EMI_H | ||
| #define ZEPHYR_SOC_REALTEK_RTS5912_REG_EMI_H | ||
|
|
||
| typedef struct { |
There was a problem hiding this comment.
Don't define new typedefs. Use struct emi_regs instead.
Member names should use snake case.
Same comment applies to ESPI_Type, KBC_Type, MBX_Type, PORT80_Type.
drivers/espi/espi_realtek_rts5912.c
Outdated
| #define RTS5912_ESPI_RST_IRQ DT_INST_IRQ_BY_IDX(0, 0, irq) | ||
|
|
||
| struct espi_rts5912_config { | ||
| uint32_t espislv_base; |
There was a problem hiding this comment.
Use the ESPI register types directly. This avoids casting the pointer every time you access it.
This also need a volatile attribute.
| uint32_t espislv_base; | |
| volatile struct espi_regs *espi_base; |
drivers/espi/espi_realtek_rts5912.c
Outdated
| uint32_t espislv_base; | ||
| uint32_t espislv_clk_grp; | ||
| uint32_t espislv_clk_idx; | ||
| uint32_t port80_base; |
There was a problem hiding this comment.
| uint32_t port80_base; | |
| volatile struct port80_regs port80_base; |
drivers/espi/espi_realtek_rts5912.c
Outdated
| uint32_t port80_base; | ||
| uint32_t port80_clk_grp; | ||
| uint32_t port80_clk_idx; | ||
| uint32_t acpi_base; |
There was a problem hiding this comment.
| uint32_t acpi_base; | |
| volatile struct acpi_regs *acpi_base; |
You should update all the _base members to use the volatile struct pointers.
JhanBoChao-Realtek
force-pushed
the
rtk_espi
branch
6 times, most recently
from
bca495f to
3f79e9d
Compare
JhanBoChao-Realtek
force-pushed
the
rtk_espi
branch
4 times, most recently
from
2ab3ed8 to
db70df7
Compare
| config HEAP_MEM_POOL_SIZE | ||
| default 8192 | ||
| depends on ESPI_RTS5912 |
There was a problem hiding this comment.
If you never release this memory, I prefer that you statically allocate the buffer instead of bringing in the malloc code.
But the driver shouldn't set the heap size directly, you should create a symbol with the prefix HEAP_MEM_POOL_ADD_SIZE_ to reserve HEAP for the eSPI driver. The board code needs to set CONFIG_HEAP_MEM_POOL_SIZE.
config HEAP_MEM_POOL_ADD_SIZE_ESPI_RTS5912
int
default 8192 if ESPI_RTS5912
default 0
drivers/espi/espi_realtek_rts5912.c
Outdated
| #ifndef CONFIG_ESPI_OOB_CHANNEL_RX_ASYNC | ||
| int ret; | ||
| #endif |
There was a problem hiding this comment.
You didn't make this change.
drivers/espi/espi_realtek_rts5912.c
Outdated
|
|
||
| #ifndef CONFIG_ESPI_OOB_CHANNEL_RX_ASYNC | ||
| /* Wait until ISR or timeout */ | ||
| ret = k_sem_take(&espi_data->oob_rx_lock, K_MSEC(MAX_OOB_TIMEOUT)); |
There was a problem hiding this comment.
You didn't make this change.
drivers/espi/espi_realtek_rts5912.c
Outdated
| static K_WORK_DELAYABLE_DEFINE(vw_ch_isr_wa, vw_ch_isr_wa_cb); | ||
|
|
||
| #ifdef CONFIG_ESPI_AUTOMATIC_BOOT_DONE_ACKNOWLEDGE | ||
| static void send_slave_bootdone(const struct device *dev) |
There was a problem hiding this comment.
Switch to the inclusive language terms controller and target. send_target_bootdone
JhanBoChao-Realtek
force-pushed
the
rtk_espi
branch
from
6f1fd61 to
003a4a3
Compare
drivers/espi/espi_realtek_rts5912.c
Outdated
| while (espi_reg->EVSTS & ESPI_EVSTS_TXFULL) { | ||
| timeout_cnt++; | ||
| if (timeout_cnt >= ESPI_VW_EVENT_TIMEOUT_CNT) { | ||
| return -EBUSY; | ||
| } | ||
| } |
There was a problem hiding this comment.
Non-blocking comment. You can use the WAIT_FOR() macro to make the timeout predictable and the code easier to read. The simple counter doesn't guarantee how much time this code waits, so if the core CPU clock increases or decreases, the wait time will change.
if (!WAIT_FOR((espi_reg->EVSTS & ESPI_EVSTS_TXFULL) == 0, VW_EVENT_TIMEOUT_US, k_busy_wait(10))) {
return -EBUSY;
}
drivers/espi/espi_realtek_rts5912.c
Outdated
| while (espi_reg->EVSTS & ESPI_EVSTS_TXFULL && retry_counter) { | ||
| retry_counter--; | ||
| } |
There was a problem hiding this comment.
Non-blocking - this can also use WAIT_FOR to make this timeout predictable.
drivers/espi/espi_realtek_rts5912.c
Outdated
| while (espi_reg->EVSTS & ESPI_EVSTS_TXFULL) { | ||
| timeout_cnt++; | ||
| if (timeout_cnt >= ESPI_VW_EVENT_TIMEOUT_CNT) { | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Non-blocking: another place to use WAIT_FOR()
You also check the same condition TXFULL with the same timeout in multiple places. This can be moved to a helper function.
JhanBoChao-Realtek
force-pushed
the
rtk_espi
branch
from
003a4a3 to
c3b6548
Compare
drivers/espi/espi_realtek_rts5912.c
Outdated
| WAIT_FOR(!(espi_reg->EVSTS & ESPI_EVSTS_TXFULL), | ||
| MAX_VW_RETRY_CNT * 10, | ||
| k_busy_wait(10)); |
There was a problem hiding this comment.
The timeout parameter uses microseconds. Change the name of the macro to match.
| WAIT_FOR(!(espi_reg->EVSTS & ESPI_EVSTS_TXFULL), | |
| MAX_VW_RETRY_CNT * 10, | |
| k_busy_wait(10)); | |
| #define VW_TIMEOUT_US 1000 | |
| WAIT_FOR(!(espi_reg->EVSTS & ESPI_EVSTS_TXFULL), | |
| VW_TIMEOUT_US, | |
| k_busy_wait(10)); |
drivers/espi/espi_realtek_rts5912.c
Outdated
| WAIT_FOR(!(espi_reg->EVSTS & ESPI_EVSTS_TXFULL), | ||
| MAX_VW_RETRY_CNT * 10, | ||
| k_busy_wait(10)); | ||
| WAIT_FOR(!(espi_reg->EVSTS & ESPI_EVSTS_TXFULL), MAX_VW_RETRY_CNT * 10, k_busy_wait(10)); |
There was a problem hiding this comment.
Use the same VW timeout here
| WAIT_FOR(!(espi_reg->EVSTS & ESPI_EVSTS_TXFULL), MAX_VW_RETRY_CNT * 10, k_busy_wait(10)); | |
| WAIT_FOR(!(espi_reg->EVSTS & ESPI_EVSTS_TXFULL), VW_TIMEOUT_US, k_busy_wait(10)); |
drivers/espi/espi_realtek_rts5912.c
Outdated
| #define ESPI_VW_EVENT_RETRY_CNT 1024UL | ||
| #define ESPI_VW_EVENT_TIMEOUT_CNT 10000UL | ||
|
|
There was a problem hiding this comment.
These are not counters, but specify a timeout in microseconds. The name should
| #define ESPI_VW_EVENT_RETRY_CNT 1024UL | |
| #define ESPI_VW_EVENT_TIMEOUT_CNT 10000UL | |
| #define ESPI_VW_EVENT_IDLE_TIMEOUT_US 1024UL | |
| #define ESPI_VW_EVENT_COMPLETE_TIMEOUT_US 10000UL | |
add espi driver for rts5912 Signed-off-by: jhan bo chao <jhan_bo_chao@realtek.com>
add espi flash channel driver for rts5912 Signed-off-by: jhan bo chao <jhan_bo_chao@realtek.com>
add espi oob channel driver for rts5912 Signed-off-by: jhan bo chao <jhan_bo_chao@realtek.com>
add espi vw channel driver for rts5912 Signed-off-by: jhan bo chao <jhan_bo_chao@realtek.com>
add espi peripheral channel port 80 driver for rts5912 Signed-off-by: jhan bo chao <jhan_bo_chao@realtek.com>
espi: add espi peripheral channel HOST_CMD driver for rts5912 Unlike other chips using IO port 0x800-0x8ff, we utilize shared memory to transfer host command parameters. The AP firmware must have corresponding settings for this configuration. Signed-off-by: jhan bo chao <jhan_bo_chao@realtek.com>
add espi peripheral channel acpi driver for rts5912 Signed-off-by: jhan bo chao <jhan_bo_chao@realtek.com>
…5912 add espi peripheral channel acpi shd mem driver for rts5912 Signed-off-by: jhan bo chao <jhan_bo_chao@realtek.com>
add espi peripheral channel 8042_KBC driver for rts5912 Signed-off-by: jhan bo chao <jhan_bo_chao@realtek.com>
JhanBoChao-Realtek
force-pushed
the
rtk_espi
branch
from
c3b6548 to
1838980
Compare
add espi peripheral channel driver for rts5912 Signed-off-by: jhan bo chao <jhan_bo_chao@realtek.com>
JhanBoChao-Realtek
force-pushed
the
rtk_espi
branch
from
1838980 to
a1d362a
Compare
|
|
@albertofloyd can you take another look at this? |
add espi driver for rts5912.