Re: [PATCH v7 net-next 6/8] net: ethernet: ti: am65-cpsw-qos: Add Frame Preemption MAC Merge support

[Roger Quadros <rogerq@kernel.org>]

On 08/12/2023 12:26, Roger Quadros wrote:
> 
> 
> On 08/12/2023 12:13, Roger Quadros wrote:
>>
>>
>> On 04/12/2023 14:35, Vladimir Oltean wrote:
>>> On Fri, Dec 01, 2023 at 03:58:00PM +0200, Roger Quadros wrote:
>>>> Add driver support for viewing / changing the MAC Merge sublayer
>>>> parameters and seeing the verification state machine's current state
>>>> via ethtool.
>>>>
>>>> As hardware does not support interrupt notification for verification
>>>> events we resort to polling on link up. On link up we try a couple of
>>>> times for verification success and if unsuccessful then give up.
>>>>
>>>> The Frame Preemption feature is described in the Technical Reference
>>>> Manual [1] in section:
>>>> 	12.3.1.4.6.7 Intersperced Express Traffic (IET – P802.3br/D2.0)
>>>>
>>>> Due to Silicon Errata i2208 [2] we set limit min IET fragment size to 124.
>>>>
>>>> [1] AM62x TRM - https://www.ti.com/lit/ug/spruiv7a/spruiv7a.pdf
>>>> [2] AM62x Silicon Errata - https://www.ti.com/lit/er/sprz487c/sprz487c.pdf
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>>>> ---
>>>
>>> Actually...
>>>
>>> ld.lld: error: undefined symbol: am65_cpsw_iet_common_enable
>>>>>> referenced by am65-cpsw-ethtool.c:755 (drivers/net/ethernet/ti/am65-cpsw-ethtool.c:755)
>>>>>>               drivers/net/ethernet/ti/am65-cpsw-ethtool.o:(am65_cpsw_set_mm) in archive vmlinux.a
>>>
>>> ld.lld: error: undefined symbol: am65_cpsw_iet_commit_preemptible_tcs
>>>>>> referenced by am65-cpsw-ethtool.c:876 (drivers/net/ethernet/ti/am65-cpsw-ethtool.c:876)
>>>>>>               drivers/net/ethernet/ti/am65-cpsw-ethtool.o:(am65_cpsw_set_mm) in archive vmlinux.a
>>>
>>> cat $KBUILD_OUTPUT/.config | grep AM65
>>> CONFIG_TI_K3_AM65_CPSW_NUSS=y
>>> # CONFIG_TI_K3_AM65_CPSW_SWITCHDEV is not set
>>> # CONFIG_TI_K3_AM65_CPTS is not set
>>> CONFIG_MMC_SDHCI_AM654=y
>>> CONFIG_PHY_AM654_SERDES=m
>>>
>>> am65-cpsw-qos.c is built only if CONFIG_TI_AM65_CPSW_TAS is enabled, yet am65-cpsw-ethtool.c,
>>> built by CONFIG_TI_K3_AM65_CPSW_NUSS, depends on it.
>>
>> Wondering how to fix this the right way. Should set/get_mm fail if CONFIG_TI_AM65_CPSW_TAS is not enabled?
>>
> 
> How about this fix?
> 
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-ethtool.c b/drivers/net/ethernet/ti/am65-cpsw-ethtool.c
> index 1ac4b9b53c93..688291d6038f 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-ethtool.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-ethtool.c
> @@ -775,6 +775,9 @@ static int am65_cpsw_get_mm(struct net_device *ndev, struct ethtool_mm_state *st
>  	u32 port_ctrl, iet_ctrl, iet_status;
>  	u32 add_frag_size;
>  
> +	if (!IS_ENABLED(CONFIG_TI_AM65_CPSW_TAS))
> +		return -EOPNOTSUPP;
> +
>  	mutex_lock(&priv->mm_lock);
>  
>  	iet_ctrl = readl(port->port_base + AM65_CPSW_PN_REG_IET_CTRL);
> @@ -827,6 +830,9 @@ static int am65_cpsw_set_mm(struct net_device *ndev, struct ethtool_mm_cfg *cfg,
>  	u32 val, add_frag_size;
>  	int err;
>  
> +	if (!IS_ENABLED(CONFIG_TI_AM65_CPSW_TAS))
> +		return -EOPNOTSUPP;
> +
>  	err = ethtool_mm_frag_size_min_to_add(cfg->tx_min_frag_size, &add_frag_size, extack);
>  	if (err)
>  		return err;
> diff --git a/drivers/net/ethernet/ti/am65-cpsw-qos.h b/drivers/net/ethernet/ti/am65-cpsw-qos.h
> index 6df3c2c5a04b..946e89fbb314 100644
> --- a/drivers/net/ethernet/ti/am65-cpsw-qos.h
> +++ b/drivers/net/ethernet/ti/am65-cpsw-qos.h
> @@ -100,6 +100,8 @@ void am65_cpsw_qos_link_up(struct net_device *ndev, int link_speed);
>  void am65_cpsw_qos_link_down(struct net_device *ndev);
>  int am65_cpsw_qos_ndo_tx_p0_set_maxrate(struct net_device *ndev, int queue, u32 rate_mbps);
>  void am65_cpsw_qos_tx_p0_rate_init(struct am65_cpsw_common *common);
> +void am65_cpsw_iet_commit_preemptible_tcs(struct am65_cpsw_port *port);
> +void am65_cpsw_iet_common_enable(struct am65_cpsw_common *common);
>  #else
>  static inline int am65_cpsw_qos_ndo_setup_tc(struct net_device *ndev,
>  					     enum tc_setup_type type,
> @@ -124,10 +126,12 @@ static inline int am65_cpsw_qos_ndo_tx_p0_set_maxrate(struct net_device *ndev,
>  
>  static inline void am65_cpsw_qos_tx_p0_rate_init(struct am65_cpsw_common *common)
>  { }
> +static inline void am65_cpsw_iet_commit_preemptible_tcs(struct am65_cpsw_port *port)
> +{ }
> +static inline void am65_cpsw_iet_common_enable(struct am65_cpsw_common *common)
> +{ }
>  #endif
>  
> -void am65_cpsw_iet_commit_preemptible_tcs(struct am65_cpsw_port *port);
> -void am65_cpsw_iet_common_enable(struct am65_cpsw_common *common);
>  
>  #define AM65_CPSW_REG_CTL			0x004
>  #define AM65_CPSW_PN_REG_CTL			0x004
> 
> 

But,

bool __ethtool_dev_mm_supported(struct net_device *dev)
{
	const struct ethtool_ops *ops = dev->ethtool_ops;
	struct ethtool_mm_state state = {};
	int ret = -EOPNOTSUPP;

	if (ops && ops->get_mm)
		ret = ops->get_mm(dev, &state);

	return !ret;
}

So looks like it is better to not define get_mm/set_mm if CONFIG_TI_AM65_CPSW_TAS is disabled.

-- 
cheers,
-roger