On 2022-12-26 at 19:58:47 +0200, Ilpo Järvinen wrote: > The rsu_status field moves from the doorbell register to the auth > result register in the PMCI implementation of the MAX10 BMC. Refactor > the sec update driver code to handle two distinct registers (rsu_status > field was added into csr map already when it was introduced but it was > unused until now). > > Co-developed-by: Tianfei zhang <tianfei.zhang@xxxxxxxxx> > Signed-off-by: Tianfei zhang <tianfei.zhang@xxxxxxxxx> > Co-developed-by: Russ Weight <russell.h.weight@xxxxxxxxx> > Signed-off-by: Russ Weight <russell.h.weight@xxxxxxxxx> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx> > --- > drivers/fpga/intel-m10-bmc-sec-update.c | 68 ++++++++++++++++--------- > include/linux/mfd/intel-m10-bmc.h | 2 +- > 2 files changed, 46 insertions(+), 24 deletions(-) > > diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c > index 6e58a463619c..1fe8b7ff594c 100644 > --- a/drivers/fpga/intel-m10-bmc-sec-update.c > +++ b/drivers/fpga/intel-m10-bmc-sec-update.c > @@ -251,7 +251,7 @@ static void log_error_regs(struct m10bmc_sec *sec, u32 doorbell) > const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map; > u32 auth_result; > > - dev_err(sec->dev, "RSU error status: 0x%08x\n", doorbell); > + dev_err(sec->dev, "Doorbell: 0x%08x\n", doorbell); > > if (!m10bmc_sys_read(sec->m10bmc, csr_map->auth_result, &auth_result)) > dev_err(sec->dev, "RSU auth result: 0x%08x\n", auth_result); > @@ -279,6 +279,30 @@ static bool rsu_progress_busy(u32 progress) > progress == RSU_PROG_PROGRAM_KEY_HASH); > } > > +static int m10bmc_sec_progress_status(struct m10bmc_sec *sec, u32 *doorbell, Please try to rename the parameters, to indicate u32 *doorbell is the raw value from doorbell register, and u32 *progress & status are software managed info. > + u32 *progress, u32 *status) > +{ > + const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map; > + u32 status_reg; > + int ret; > + > + ret = m10bmc_sys_read(sec->m10bmc, csr_map->doorbell, doorbell); > + if (ret) > + return ret; > + > + if (csr_map->doorbell != csr_map->rsu_status) { I prefer not to complicate the csr map filling in intel-m10-bmc, just invalid the addr value if there is no such register for the board. > + ret = m10bmc_sys_read(sec->m10bmc, csr_map->rsu_status, &status_reg); > + if (ret) > + return ret; > + *status = rsu_stat(status_reg); > + } else { > + *status = rsu_stat(*doorbell); > + } > + *progress = rsu_prog(*doorbell); > + > + return 0; > +} > + > static enum fw_upload_err rsu_check_idle(struct m10bmc_sec *sec) > { > const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map; > @@ -297,18 +321,14 @@ static enum fw_upload_err rsu_check_idle(struct m10bmc_sec *sec) > return FW_UPLOAD_ERR_NONE; > } > > -static inline bool rsu_start_done(u32 doorbell) > +static inline bool rsu_start_done(u32 doorbell, u32 progress, u32 status) Same concern for the naming, some more below I didn't list. > { > - u32 status, progress; > - > if (doorbell & DRBL_RSU_REQUEST) > return false; > > - status = rsu_stat(doorbell); > if (status == RSU_STAT_ERASE_FAIL || status == RSU_STAT_WEAROUT) > return true; > > - progress = rsu_prog(doorbell); > if (!rsu_progress_done(progress)) > return true; > > @@ -318,8 +338,8 @@ static inline bool rsu_start_done(u32 doorbell) > static enum fw_upload_err rsu_update_init(struct m10bmc_sec *sec) > { > const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map; > - u32 doorbell, status; > - int ret; > + u32 doorbell, progress, status; > + int ret, err; > > ret = regmap_update_bits(sec->m10bmc->regmap, > csr_map->base + csr_map->doorbell, > @@ -330,21 +350,20 @@ static enum fw_upload_err rsu_update_init(struct m10bmc_sec *sec) > if (ret) > return FW_UPLOAD_ERR_RW_ERROR; > > - ret = regmap_read_poll_timeout(sec->m10bmc->regmap, > - csr_map->base + csr_map->doorbell, > - doorbell, > - rsu_start_done(doorbell), > - NIOS_HANDSHAKE_INTERVAL_US, > - NIOS_HANDSHAKE_TIMEOUT_US); > + ret = read_poll_timeout(m10bmc_sec_progress_status, err, > + err < 0 || rsu_start_done(doorbell, progress, status), > + NIOS_HANDSHAKE_INTERVAL_US, > + NIOS_HANDSHAKE_TIMEOUT_US, > + false, > + sec, &doorbell, &progress, &status); > > if (ret == -ETIMEDOUT) { > log_error_regs(sec, doorbell); > return FW_UPLOAD_ERR_TIMEOUT; > - } else if (ret) { > + } else if (err) { > return FW_UPLOAD_ERR_RW_ERROR; > } > > - status = rsu_stat(doorbell); > if (status == RSU_STAT_WEAROUT) { > dev_warn(sec->dev, "Excessive flash update count detected\n"); > return FW_UPLOAD_ERR_WEAROUT; > @@ -393,7 +412,7 @@ static enum fw_upload_err rsu_prog_ready(struct m10bmc_sec *sec) > static enum fw_upload_err rsu_send_data(struct m10bmc_sec *sec) > { > const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map; > - u32 doorbell; > + u32 doorbell, status; > int ret; > > ret = regmap_update_bits(sec->m10bmc->regmap, > @@ -418,7 +437,10 @@ static enum fw_upload_err rsu_send_data(struct m10bmc_sec *sec) > return FW_UPLOAD_ERR_RW_ERROR; > } > > - if (!rsu_status_ok(rsu_stat(doorbell))) { > + ret = m10bmc_sys_read(sec->m10bmc, csr_map->rsu_status, &status); Same as above, please just handle the detailed register definition differences in this driver, not in csr map. Thanks, Yilun > + if (ret) > + return ret; > + if (!rsu_status_ok(rsu_stat(status))) { > log_error_regs(sec, doorbell); > return FW_UPLOAD_ERR_HW_ERROR; > } > @@ -428,18 +450,18 @@ static enum fw_upload_err rsu_send_data(struct m10bmc_sec *sec) > > static int rsu_check_complete(struct m10bmc_sec *sec, u32 *doorbell) > { > - const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map; > + u32 progress, status; > > - if (m10bmc_sys_read(sec->m10bmc, csr_map->doorbell, doorbell)) > + if (m10bmc_sec_progress_status(sec, doorbell, &progress, &status)) > return -EIO; > > - if (!rsu_status_ok(rsu_stat(*doorbell))) > + if (!rsu_status_ok(status)) > return -EINVAL; > > - if (rsu_progress_done(rsu_prog(*doorbell))) > + if (rsu_progress_done(progress)) > return 0; > > - if (rsu_progress_busy(rsu_prog(*doorbell))) > + if (rsu_progress_busy(progress)) > return -EAGAIN; > > return -EINVAL; > diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h > index 42e2ce7fe439..cc2d9eb597b0 100644 > --- a/include/linux/mfd/intel-m10-bmc.h > +++ b/include/linux/mfd/intel-m10-bmc.h > @@ -58,7 +58,7 @@ > #define HOST_STATUS_ABORT_RSU 0x2 > > #define rsu_prog(doorbell) FIELD_GET(DRBL_RSU_PROGRESS, doorbell) > -#define rsu_stat(doorbell) FIELD_GET(DRBL_RSU_STATUS, doorbell) > +#define rsu_stat(status_reg) FIELD_GET(DRBL_RSU_STATUS, status_reg) > > /* interval 100ms and timeout 5s */ > #define NIOS_HANDSHAKE_INTERVAL_US (100 * 1000) > -- > 2.30.2 >