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 <[email protected]>
Signed-off-by: Tianfei zhang <[email protected]>
Co-developed-by: Russ Weight <[email protected]>
Signed-off-by: Russ Weight <[email protected]>
Signed-off-by: Ilpo Järvinen <[email protected]>
---
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,
+ 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)
--
2.30.2
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 <[email protected]>
> Signed-off-by: Tianfei zhang <[email protected]>
> Co-developed-by: Russ Weight <[email protected]>
> Signed-off-by: Russ Weight <[email protected]>
> Signed-off-by: Ilpo J?rvinen <[email protected]>
> ---
> 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 <[email protected]>
> > Signed-off-by: Tianfei zhang <[email protected]>
> > Co-developed-by: Russ Weight <[email protected]>
> > Signed-off-by: Russ Weight <[email protected]>
> > Signed-off-by: Ilpo J?rvinen <[email protected]>
> > ---
> > 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/[email protected]/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?
--
i.
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 <[email protected]>
> > > Signed-off-by: Tianfei zhang <[email protected]>
> > > Co-developed-by: Russ Weight <[email protected]>
> > > Signed-off-by: Russ Weight <[email protected]>
> > > Signed-off-by: Ilpo J?rvinen <[email protected]>
> > > ---
> > > 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/[email protected]/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 <[email protected]>
> > > > Signed-off-by: Tianfei zhang <[email protected]>
> > > > Co-developed-by: Russ Weight <[email protected]>
> > > > Signed-off-by: Russ Weight <[email protected]>
> > > > Signed-off-by: Ilpo J?rvinen <[email protected]>
> > > > ---
> > > > 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/[email protected]/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.
--
i.