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@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.
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@xxxxxxxxxxxxxxx/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.
