Re: [PATCH v5 02/25] soundwire: amd: program pad SCHMEN and OE control for ACP 7.0/7.1/7.2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] soundwire: amd: Fixes tag cites ACP 6.3 refactor instead of ACP
  7.x commits
- [Low] soundwire: amd: amd_enable_sdw_pads() writes pad registers
  before acp_rev check
--

commit feea96d1feca02ca2e3cb0b4f0b3f33fab424dca
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

> soundwire: amd: program pad SCHMEN and OE control for ACP 7.0/7.1/7.2
>
> ACP 7.0/7.1/7.2 hardware requires SCHMEN control and
> output-enable (OE) registers to be programmed as part of the pad enable
> sequence, in addition to the existing keeper and pulldown writes.
>
> Update amd_enable_sdw_pads() to program ACP_PAD_SCHMEN_CTRL and
> ACP_PAD_DISABLE_OE_CTRL for active pads. Pass acp_rev to
> amd_enable_sdw_pads() and restrict these writes to ACP 7.0/7.1/7.2;
> ACP 6.3 is unaffected.
>
> Define per-register masks in amd_init.h replacing generic local defines
> in amd_init.c.
>
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
> Fixes: cf0ddbc29dfa ("soundwire: amd: refactor soundwire pads enable")

[Severity: Low]
cf0ddbc29dfa ("soundwire: amd: refactor soundwire pads enable") only
supported ACP 6.3, which is unaffected by this change, and predates both
struct sdw_amd_res.acp_rev and the ACP 7.x definitions.  Should this cite
2c0ae8ef1e5e ("soundwire: amd: add support for ACP7.0 & ACP7.1 platforms")
and 34b1cb4ec286 ("soundwire: amd: Add support for acp7.2 platform")
instead so stable tooling does not attempt backports to ACP 6.3-only trees?

> diff --git a/drivers/soundwire/amd_init.c b/drivers/soundwire/amd_init.c
> index b4f45bdf3c171..032697ea35906 100644
> --- a/drivers/soundwire/amd_init.c
> +++ b/drivers/soundwire/amd_init.c
> @@ -16,37 +16,49 @@
>  
>  #include "amd_init.h"
>  
> -#define AMD_SDW0_PAD_CTRL_MASK				0x60
> -#define AMD_SDW1_PAD_CTRL_MASK				5
> -#define AMD_SDW_PAD_CTRL_MASK		(AMD_SDW0_PAD_CTRL_MASK | AMD_SDW1_PAD_CTRL_MASK)
> -#define AMD_SDW0_PAD_EN					1
> -#define AMD_SDW1_PAD_EN					0x10
> -#define AMD_SDW_PAD_EN			(AMD_SDW0_PAD_EN | AMD_SDW1_PAD_EN)
> -
> -static int amd_enable_sdw_pads(void __iomem *mmio, u32 link_mask, struct device *dev)
> +static int amd_enable_sdw_pads(void __iomem *mmio, u32 link_mask, u32 acp_rev, struct device *dev)
>  {
> -	u32 pad_keeper_en, pad_pulldown_ctrl_mask;
> +	u32 pad_keeper_mask, pad_pulldown_mask, pad_schmen_mask, pad_oe_mask;
>  
>  	switch (link_mask) {
>  	case 1:
> -		pad_keeper_en = AMD_SDW0_PAD_EN;
> -		pad_pulldown_ctrl_mask = AMD_SDW0_PAD_CTRL_MASK;
> +		pad_keeper_mask = AMD_SDW0_PAD_KEEPER_EN_MASK;
> +		pad_pulldown_mask = AMD_SDW0_PAD_PULLDOWN_MASK;
> +		pad_schmen_mask = AMD_SDW0_PAD_SCHMEN_MASK;
> +		pad_oe_mask = AMD_SDW0_PAD_OE_CTRL_MASK;
>  		break;
>  	case 2:
> -		pad_keeper_en = AMD_SDW1_PAD_EN;
> -		pad_pulldown_ctrl_mask = AMD_SDW1_PAD_CTRL_MASK;
> +		pad_keeper_mask = AMD_SDW1_PAD_KEEPER_EN_MASK;
> +		pad_pulldown_mask = AMD_SDW1_PAD_PULLDOWN_MASK;
> +		pad_schmen_mask = AMD_SDW1_PAD_SCHMEN_MASK;
> +		pad_oe_mask = AMD_SDW1_PAD_OE_CTRL_MASK;
>  		break;
>  	case 3:
> -		pad_keeper_en = AMD_SDW_PAD_EN;
> -		pad_pulldown_ctrl_mask = AMD_SDW_PAD_CTRL_MASK;
> +		pad_keeper_mask = AMD_SDW_PAD_KEEPER_EN_MASK;
> +		pad_pulldown_mask = AMD_SDW_PAD_PULLDOWN_MASK;
> +		pad_schmen_mask = AMD_SDW_PAD_SCHMEN_MASK;
> +		pad_oe_mask = AMD_SDW_PAD_OE_CTRL_MASK;
>  		break;
>  	default:
>  		dev_err(dev, "No SDW Links are enabled\n");
>  		return -ENODEV;
>  	}
>  
> -	amd_updatel(mmio, ACP_SW_PAD_KEEPER_EN, pad_keeper_mask, pad_keeper_mask);
> -	amd_updatel(mmio, ACP_PAD_PULLDOWN_CTRL, pad_pulldown_ctrl_mask, 0);
> +	amd_updatel(mmio, ACP_SW_PAD_KEEPER_EN, pad_keeper_mask, pad_keeper_mask);
> +	amd_updatel(mmio, ACP_PAD_PULLDOWN_CTRL, pad_pulldown_mask, 0);
> +	switch (acp_rev) {
> +	case ACP70_PCI_REV_ID:
> +	case ACP71_PCI_REV_ID:
> +	case ACP72_PCI_REV_ID:
> +		amd_updatel(mmio, ACP_PAD_SCHMEN_CTRL, pad_schmen_mask, pad_schmen_mask);
> +		amd_updatel(mmio, ACP_PAD_DISABLE_OE_CTRL, pad_oe_mask, 0);
> +		break;
> +	case ACP63_PCI_REV_ID:
> +		break;
> +	default:
> +		dev_err(dev, "unsupported ACP revision 0x%x\n", acp_rev);
> +		return -EINVAL;
> +	}

[Severity: Low]
Should acp_rev be checked before programming ACP_SW_PAD_KEEPER_EN and
ACP_PAD_PULLDOWN_CTRL so that an unsupported acp_rev (such as 0x7b or 0x7f
from amd_sof_acp7x_probe()) does not modify pad registers before returning
-EINVAL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107154659.3358223-1-Vijendar.Mukunda@amd.com?part=2