| Message ID | 20221226175849.13056-9-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | intel-m10-bmc: Split BMC to core and SPI parts & add PMCI+N6000 support | expand |
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@intel.com> > Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com> > Co-developed-by: Russ Weight <russell.h.weight@intel.com> > Signed-off-by: Russ Weight <russell.h.weight@intel.com> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > 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 >
On Fri, 30 Dec 2022, Xu Yilun wrote: > 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@intel.com> > > Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com> > > Co-developed-by: Russ Weight <russell.h.weight@intel.com> > > Signed-off-by: Russ Weight <russell.h.weight@intel.com> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > --- > > 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. I'll try to do that. > > + 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. I'm sorry but I didn't get the meaning of your comment. Could you please rephrase? My guess is that you might have tried to say that if there's no register for rsu_status, mark it not existing in csr map? But the field exists in both cases, it's just part of a different register (doorbell or auth_result) so if I use that kind of "register doesn't exist" condition, it would apply to both cases. > > @@ -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. Earlier you were having the exactly opposite opinion: https://lore.kernel.org/linux-fpga/20221108144305.45424-1-ilpo.jarvinen@linux.intel.com/T/#me2d20e60d7feeafcdeeab4d58bd82787acf3ada9 So which way you want it? Should I have the board types here in the sec update drivers as a second layer of differentiation or not?
On 2022-12-30 at 12:23:18 +0200, Ilpo Järvinen wrote: > On Fri, 30 Dec 2022, Xu Yilun wrote: > > > 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@intel.com> > > > Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com> > > > Co-developed-by: Russ Weight <russell.h.weight@intel.com> > > > Signed-off-by: Russ Weight <russell.h.weight@intel.com> > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > --- > > > 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. > > I'll try to do that. > > > > + 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. > > I'm sorry but I didn't get the meaning of your comment. Could you please > rephrase? > > My guess is that you might have tried to say that if there's no register > for rsu_status, mark it not existing in csr map? But the field exists in Yes, this is what I mean, but I see I was wrong. > both cases, it's just part of a different register (doorbell or I was thinking there was no AUTH_RESULT for N3000, sorry for the mistake. > auth_result) so if I use that kind of "register doesn't exist" condition, > it would apply to both cases. > > > > @@ -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. > > Earlier you were having the exactly opposite opinion: > > https://lore.kernel.org/linux-fpga/20221108144305.45424-1-ilpo.jarvinen@linux.intel.com/T/#me2d20e60d7feeafcdeeab4d58bd82787acf3ada9 Ah, I'm sorry. I was thinking just move one register to another addr at that time. I was not aware that actually the detailed register field definitions are changed in same registers. > > So which way you want it? Should I have the board types here in the sec > update drivers as a second layer of differentiation or not? I think the different register field definitions for the same registers are specific to secure driver. So please differentiate them in secure driver. But with the change, enum m10bmc_type could still be removed, is it? And having the register addr differentiations in m10bmc mfd driver is good to me, cause with a different board type, the register offsets for all subdevs are often globally re-arranged. But I don't want the HW change within a single IP block been specified in m10bmc mfd driver. Thanks, Yilun > > > -- > i.
On Tue, 3 Jan 2023, Xu Yilun wrote: > On 2022-12-30 at 12:23:18 +0200, Ilpo Järvinen wrote: > > On Fri, 30 Dec 2022, Xu Yilun wrote: > > > > > 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@intel.com> > > > > Signed-off-by: Tianfei zhang <tianfei.zhang@intel.com> > > > > Co-developed-by: Russ Weight <russell.h.weight@intel.com> > > > > Signed-off-by: Russ Weight <russell.h.weight@intel.com> > > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > > --- > > > > 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. > > > > I'll try to do that. > > > > > > + 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. > > > > I'm sorry but I didn't get the meaning of your comment. Could you please > > rephrase? > > > > My guess is that you might have tried to say that if there's no register > > for rsu_status, mark it not existing in csr map? But the field exists in > > Yes, this is what I mean, but I see I was wrong. > > > both cases, it's just part of a different register (doorbell or > > I was thinking there was no AUTH_RESULT for N3000, sorry for the > mistake. > > > auth_result) so if I use that kind of "register doesn't exist" condition, > > it would apply to both cases. > > > > > > @@ -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. > > > > Earlier you were having the exactly opposite opinion: > > > > https://lore.kernel.org/linux-fpga/20221108144305.45424-1-ilpo.jarvinen@linux.intel.com/T/#me2d20e60d7feeafcdeeab4d58bd82787acf3ada9 > > Ah, I'm sorry. I was thinking just move one register to another addr at > that time. I was not aware that actually the detailed register field > definitions are changed in same registers. > > > > > So which way you want it? Should I have the board types here in the sec > > update drivers as a second layer of differentiation or not? > > I think the different register field definitions for the same registers > are specific to secure driver. So please differentiate them in secure > driver. > > But with the change, enum m10bmc_type could still be removed, is it? > > And having the register addr differentiations in m10bmc mfd driver is good to > me, cause with a different board type, the register offsets for all subdevs > are often globally re-arranged. But I don't want the HW change within a > single IP block been specified in m10bmc mfd driver. Okay. I'll add ops for it then: struct m10bmc_sec_ops { int (*rsu_status)(struct m10bmc_sec *sec); }; Type enum won't be necessary. Those ops will be useful for other things too which are not included to this patch set.
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, + 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) { + 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) { - 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); + 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)