Re: [PATCH net-next v2] net: stmmac: xgmac: Enable support for multiple Flexible PPS outputs

[Serge Semin]

On Mon, Oct 30, 2023 at 11:55:50AM +0800, Furong Xu wrote:
> From XGMAC Core 3.20 and later, each Flexible PPS has individual PPSEN bit
> to select Fixed mode or Flexible mode. The PPSEN must be set, or it stays
> in Fixed PPS mode by default.
> XGMAC Core prior 3.20, only PPSEN0(bit 4) is writable. PPSEN{1,2,3} are
> read-only reserved, and they are already in Flexible mode by default, our
> new code always set PPSEN{1,2,3} do not make things worse ;-)
> 
> Signed-off-by: Furong Xu <0x1207@xxxxxxxxx>

Please don't forget to add the already got tags shall you need more
patch revisions.

Reviewed-by: Serge Semin <fancer.lancer@xxxxxxxxx>

> ---
> Changes in v2:
>   - Add comment for XGMAC_PPSEN description among different XGMAC core versions.
>   - Update commit message, thanks Serge Semin and Jacob Keller for your advices.

BTW Why have you dropped the Fixes tag? Moreover you didn't address
Jacob' comment regarding the actual problematic commit. This isn't the
way the review process normally goes. I'll copy what Jacob said in v1
so to restore the discussion here:

On Mon, Oct 27, 2023 at 2:15:50AM +0800, Jacob Keller  wrote:
> The original code was added in 95eaf3cd0a90 ("net: stmmac: dwxgmac: Add
> Flexible PPS support"), which landed in v5.4
> 
> It looks like XGMAC Core 3.20 support was not added until possibly
> commit 669a55560e4b ("net: stmmac: Check more MAC HW features for XGMAC
> Core 3.20") which appears to be new enough that its not in any official
> Linux release, though it looks like it was already in net.
> 
> Perhaps this should be tagged Fixes: and sent through net, hopefully to
> try and hit 6.6 or at least a stable release shortly after?
> 

Regards
-Serge(y)

> ---
>  drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h     |  2 +-
>  .../net/ethernet/stmicro/stmmac/dwxgmac2_core.c    | 14 +++++++++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> index 7a8f47e7b728..a4e8b498dea9 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h
> @@ -259,7 +259,7 @@
>  	((val) << XGMAC_PPS_MINIDX(x))
>  #define XGMAC_PPSCMD_START		0x2
>  #define XGMAC_PPSCMD_STOP		0x5
> -#define XGMAC_PPSEN0			BIT(4)
> +#define XGMAC_PPSENx(x)			BIT(4 + (x) * 8)
>  #define XGMAC_PPSx_TARGET_TIME_SEC(x)	(0x00000d80 + (x) * 0x10)
>  #define XGMAC_PPSx_TARGET_TIME_NSEC(x)	(0x00000d84 + (x) * 0x10)
>  #define XGMAC_TRGTBUSY0			BIT(31)
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> index f352be269deb..453e88b75be0 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_core.c
> @@ -1178,7 +1178,19 @@ static int dwxgmac2_flex_pps_config(void __iomem *ioaddr, int index,
>  
>  	val |= XGMAC_PPSCMDx(index, XGMAC_PPSCMD_START);
>  	val |= XGMAC_TRGTMODSELx(index, XGMAC_PPSCMD_START);
> -	val |= XGMAC_PPSEN0;
> +
> +	/* XGMAC Core has 4 PPS outputs at most.
> +	 *
> +	 * Prior XGMAC Core 3.20, Fixed mode or Flexible mode are selectable for
> +	 * PPS0 only via PPSEN0. PPS{1,2,3} are in Flexible mode by default,
> +	 * and can not be switched to Fixed mode, since PPSEN{1,2,3} are
> +	 * read-only reserved to 0.
> +	 * But we always set PPSEN{1,2,3} do not make things worse ;-)
> +	 *
> +	 * From XGMAC Core 3.20 and later, PPSEN{0,1,2,3} are writable and must
> +	 * be set, or the PPS outputs stay in Fixed PPS mode by default.
> +	 */
> +	val |= XGMAC_PPSENx(index);
>  
>  	writel(cfg->start.tv_sec, ioaddr + XGMAC_PPSx_TARGET_TIME_SEC(index));
>  
> -- 
> 2.34.1
>