Re: [PATCH net-next v1 1/1] octeon_ep: reset firmware ready status

[Simon Horman <horms@kernel.org>]

On Thu, Nov 20, 2025 at 11:23:44AM +0000, Vimlesh Kumar wrote:
> Add support to reset firmware ready status
> when the driver is removed(either in unload
> or unbind)
> 
> Signed-off-by: Sathesh Edara <sedara@marvell.com>
> Signed-off-by: Shinas Rasheed <srasheed@marvell.com>
> Signed-off-by: Vimlesh Kumar <vimleshk@marvell.com>

I'm a little confused about the asymmetry of the cn9k and cnxk code
before this patch. Maybe it would make sense to split this
into two patches, one for cn9k and cnxk with more specific
commit messages.

And for both cn9k and cnxk, I'm unclear on the what the behaviour was
before this patch? IOW, is this a bug fix for either of both of xn9k and
cnkx?

> ---
>  .../marvell/octeon_ep/octep_cn9k_pf.c         | 22 +++++++++++++++++++
>  .../marvell/octeon_ep/octep_cnxk_pf.c         |  2 +-
>  .../marvell/octeon_ep/octep_regs_cn9k_pf.h    | 11 ++++++++++
>  .../marvell/octeon_ep/octep_regs_cnxk_pf.h    |  1 +
>  4 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_cn9k_pf.c b/drivers/net/ethernet/marvell/octeon_ep/octep_cn9k_pf.c
> index b5805969404f..6f926e82c17c 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_cn9k_pf.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_cn9k_pf.c
> @@ -637,6 +637,17 @@ static int octep_soft_reset_cn93_pf(struct octep_device *oct)
>  
>  	octep_write_csr64(oct, CN93_SDP_WIN_WR_MASK_REG, 0xFF);
>  
> +	/* Firmware status CSR is supposed to be cleared by
> +	 * core domain reset, but due to a hw bug, it is not.
> +	 * Set it to RUNNING right before reset so that it is not
> +	 * left in READY (1) state after a reset.  This is required
> +	 * in addition to the early setting to handle the case where
> +	 * the OcteonTX is unexpectedly reset, reboots, and then
> +	 * the module is removed.
> +	 */
> +	OCTEP_PCI_WIN_WRITE(oct, CN9K_PEMX_PFX_CSX_PFCFGX(0, 0, CN9K_PCIEEP_VSECST_CTL),
> +			    FW_STATUS_DOWNING);
> +

There seems to be some inconsistency between the comment,
which describes setting the status to RUNNING, and the code,
which sets the status to DOWNING.

Flagged by Claude Code with https://github.com/masoncl/review-prompts/

>  	/* Set core domain reset bit */
>  	OCTEP_PCI_WIN_WRITE(oct, CN93_RST_CORE_DOMAIN_W1S, 1);
>  	/* Wait for 100ms as Octeon resets. */
> @@ -894,4 +905,15 @@ void octep_device_setup_cn93_pf(struct octep_device *oct)
>  
>  	octep_init_config_cn93_pf(oct);
>  	octep_configure_ring_mapping_cn93_pf(oct);
> +
> +	if (oct->chip_id == OCTEP_PCI_DEVICE_ID_CN98_PF)
> +		return;
> +
> +	/* Firmware status CSR is supposed to be cleared by
> +	 * core domain reset, but due to IPBUPEM-38842, it is not.
> +	 * Set it to RUNNING early in boot, so that unexpected resets
> +	 * leave it in a state that is not READY (1).
> +	 */
> +	OCTEP_PCI_WIN_WRITE(oct, CN9K_PEMX_PFX_CSX_PFCFGX(0, 0, CN9K_PCIEEP_VSECST_CTL),
> +			    FW_STATUS_RUNNING);
>  }
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_cnxk_pf.c b/drivers/net/ethernet/marvell/octeon_ep/octep_cnxk_pf.c
> index 5de0b5ecbc5f..e07264b3dbf8 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_cnxk_pf.c
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_cnxk_pf.c
> @@ -660,7 +660,7 @@ static int octep_soft_reset_cnxk_pf(struct octep_device *oct)
>  	 * the module is removed.
>  	 */
>  	OCTEP_PCI_WIN_WRITE(oct, CNXK_PEMX_PFX_CSX_PFCFGX(0, 0, CNXK_PCIEEP_VSECST_CTL),
> -			    FW_STATUS_RUNNING);
> +			    FW_STATUS_DOWNING);

Likewise here.

>  
>  	/* Set chip domain reset bit */
>  	OCTEP_PCI_WIN_WRITE(oct, CNXK_RST_CHIP_DOMAIN_W1S, 1);
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_regs_cn9k_pf.h b/drivers/net/ethernet/marvell/octeon_ep/octep_regs_cn9k_pf.h
> index ca473502d7a0..d7fa5adbce98 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_regs_cn9k_pf.h
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_regs_cn9k_pf.h
> @@ -383,6 +383,17 @@
>  /* bit 1 for firmware heartbeat interrupt */
>  #define CN93_SDP_EPF_OEI_RINT_DATA_BIT_HBEAT	BIT_ULL(1)
>  
> +#define FW_STATUS_DOWNING      0ULL
> +#define FW_STATUS_RUNNING      2ULL
> +#define CN9K_PEMX_PFX_CSX_PFCFGX(pem, pf, offset)      ((0x8e0000008000 | (uint64_t)(pem) << 36 \
> +							| (pf) << 18 \
> +							| (((offset) >> 16) & 1) << 16 \
> +							| ((offset) >> 3) << 3) \
> +							+ ((((offset) >> 2) & 1) << 2))

I realise that this implementation mirrors that in octep_regs_cnxk_pf.h,
but I do think it would be better rexpressed in terms of FIELD_PREP(),
GETMASK_ULL, and BIT_ULL. With #defines so the masks (and bits are named).

Also, as the above duplicates what is present in octep_regs_cnxk_pf.h,
maybe it would be nice to share it somehow.

> +
> +/* Register defines for use with CN9K_PEMX_PFX_CSX_PFCFGX */
> +#define CN9K_PCIEEP_VSECST_CTL  0x4D0
> +
>  #define CN93_PEM_BAR4_INDEX            7
>  #define CN93_PEM_BAR4_INDEX_SIZE       0x400000ULL
>  #define CN93_PEM_BAR4_INDEX_OFFSET     (CN93_PEM_BAR4_INDEX * CN93_PEM_BAR4_INDEX_SIZE)
> diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_regs_cnxk_pf.h b/drivers/net/ethernet/marvell/octeon_ep/octep_regs_cnxk_pf.h
> index e637d7c8224d..a6b6c9f356de 100644
> --- a/drivers/net/ethernet/marvell/octeon_ep/octep_regs_cnxk_pf.h
> +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_regs_cnxk_pf.h
> @@ -396,6 +396,7 @@
>  #define CNXK_SDP_EPF_OEI_RINT_DATA_BIT_MBOX	BIT_ULL(0)
>  /* bit 1 for firmware heartbeat interrupt */
>  #define CNXK_SDP_EPF_OEI_RINT_DATA_BIT_HBEAT	BIT_ULL(1)
> +#define FW_STATUS_DOWNING      0ULL
>  #define FW_STATUS_RUNNING      2ULL
>  #define CNXK_PEMX_PFX_CSX_PFCFGX(pem, pf, offset)      ({ typeof(offset) _off = (offset); \
>  							  ((0x8e0000008000 | \
> -- 
> 2.34.1
> 
>