Re: [PATCH 03/46] common/sfc_efx/base: add Medford4 support to NIC module

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 4/16/25 16:59, Ivan Malov wrote:
> From: Denis Pryazhennikov <denis.pryazhennikov@arknetworks.am>
> 
> Implement NIC family discovery and minimum probe support.
> 
> Signed-off-by: Denis Pryazhennikov <denis.pryazhennikov@arknetworks.am>
> Reviewed-by: Andy Moreton <andy.moreton@amd.com>

[snip]

> @@ -1922,6 +1923,36 @@ static struct ef10_external_port_map_s {
>   		(1U << TLV_PORT_MODE_1x1_1x1),			/* mode 2 */
>   		{ 0, 1, EFX_EXT_PORT_NA, EFX_EXT_PORT_NA }
>   	},
> +	/*
> +	 * Modes that on Medford4 allocate 2 adjacent port numbers to cage 1
> +	 * and the rest to cage 2.
> +	 *	port 0 -> cage 1
> +	 *	port 1 -> cage 1
> +	 *	port 2 -> cage 2
> +	 *	port 3 -> cage 2
> +	 */
> +	{
> +		EFX_FAMILY_MEDFORD4,
> +		(1U << TLV_PORT_MODE_2x1_2x1) |			/* mode 5 */
> +		(1U << TLV_PORT_MODE_2x1_1x4) |			/* mode 7 */
> +		(1U << TLV_PORT_MODE_2x2_NA) |			/* mode 13 */
> +		(1U << TLV_PORT_MODE_2x1_1x2),			/* mode 18 */
> +		{ 0, 2, EFX_EXT_PORT_NA, EFX_EXT_PORT_NA }
> +	},
> +	/*
> +	 * Modes that on Medford4 allocate up to 4 adjacent port numbers
> +	 * to cage 1.
> +	 *	port 0 -> cage 1
> +	 *	port 1 -> cage 1
> +	 *	port 2 -> cage 1
> +	 *	port 3 -> cage 1
> +	 */
> +	{
> +		EFX_FAMILY_MEDFORD4,
> +		(1U << TLV_PORT_MODE_4x1_NA),			/* mode 4 */
> +		{ 0, EFX_EXT_PORT_NA, EFX_EXT_PORT_NA, EFX_EXT_PORT_NA }
> +	},
> +	/* FIXME: review Medford4 port modes */

Is it still true at the moment of submission?

>   };
>   
>   static	__checkReturn	efx_rc_t
> diff --git a/drivers/common/sfc_efx/base/efx.h b/drivers/common/sfc_efx/base/efx.h
> index dabf2e0e0b..442dfa0830 100644
> --- a/drivers/common/sfc_efx/base/efx.h
> +++ b/drivers/common/sfc_efx/base/efx.h
> @@ -190,6 +190,7 @@ efx_family_probe_bar(
>   /* FIXME Fix it when memory bar is fixed in FPGA image. It must be 0. */
>   #define	EFX_MEM_BAR_RIVERHEAD			2
>   
> +#define	EFX_MEM_BAR_MEDFORD4			0

IMHO it is wrong that you add it here and fix in later patches.
It just adds noise to mailing list and git blame.
IMHO fix should be squashed and correct BAR should be specified
from the very beginning.

>   
>   /* Error codes */
>   

[snip]

> diff --git a/drivers/common/sfc_efx/base/efx_nic.c b/drivers/common/sfc_efx/base/efx_nic.c
> index 172488e083..5bcc0a04ff 100644
> --- a/drivers/common/sfc_efx/base/efx_nic.c
> +++ b/drivers/common/sfc_efx/base/efx_nic.c
> @@ -79,6 +79,19 @@ efx_family(
>   			return (0);
>   #endif /* EFSYS_OPT_MEDFORD2 */
>   
> +#if EFSYS_OPT_MEDFORD4
> +		case EFX_PCI_DEVID_MEDFORD4_PF_UNINIT:
> +			/*
> +			 * Hardware default for PF0 of uninitialised Medford4.
> +			 * manftest must be able to cope with this device id.
> +			 */
> +		case EFX_PCI_DEVID_MEDFORD4:
> +		case EFX_PCI_DEVID_MEDFORD4_VF:
> +			*efp = EFX_FAMILY_MEDFORD4;
> +			*membarp = EFX_MEM_BAR_MEDFORD4;
> +			return (0);
> +#endif /* EFSYS_OPT_MEDFORD4 */
> +

It is a dead code from build point of view if I stop on the patch since
EFSYS_OPT_MEDFORD4 is 0. May be it is OK, but I'd like to ensure that
you have checked it if EFSYS_OPT_MEDFORD4==1 before the patch to avoid
build fixes noise later.

Same for all similar blocks below and subsequent patches.

>   		case EFX_PCI_DEVID_FALCON:	/* Obsolete, not supported */
>   		default:
>   			break;

[snip]