[04/12] drivers: soundwire: refactor soundwire pads enable

  As sdw pads enable sequence is executed only once, invoke it
from probe sequence.

Program required pads for both manager instances
based on link_mask during probe sequence. This will avoid
acquiring mutex lock.
Remove unnecessary delay after programming ACP_SW_PAD_KEEPER_EN
register.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_init.c    | 45 +++++++++++++++++++++++++++++++++
 drivers/soundwire/amd_manager.c | 18 -------------
 2 files changed, 45 insertions(+), 18 deletions(-)
  

On 21-12-23, 13:05, Vijendar Mukunda wrote:
> As sdw pads enable sequence is executed only once, invoke it
> from probe sequence.
> 
> Program required pads for both manager instances
> based on link_mask during probe sequence. This will avoid
> acquiring mutex lock.

something wrong with your editor to have this formatting, you can use
upto 80 chars here!


> Remove unnecessary delay after programming ACP_SW_PAD_KEEPER_EN
> register.
> 
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
> ---
>  drivers/soundwire/amd_init.c    | 45 +++++++++++++++++++++++++++++++++
>  drivers/soundwire/amd_manager.c | 18 -------------
>  2 files changed, 45 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/soundwire/amd_init.c b/drivers/soundwire/amd_init.c
> index 5c9569d9ad01..b3b3c7266384 100644
> --- a/drivers/soundwire/amd_init.c
> +++ b/drivers/soundwire/amd_init.c
> @@ -15,6 +15,47 @@
>  
>  #include "amd_init.h"
>  
> +#define ACP_PAD_PULLDOWN_CTRL				0x0001448
> +#define ACP_SW_PAD_KEEPER_EN				0x0001454
> +#define AMD_SDW_PAD_PULLDOWN_CTRL_ENABLE_MASK		0x7f9a
> +#define AMD_SDW0_PAD_PULLDOWN_CTRL_ENABLE_MASK		0x7f9f
> +#define AMD_SDW1_PAD_PULLDOWN_CTRL_ENABLE_MASK		0x7ffa
> +#define AMD_SDW0_PAD_EN_MASK				1
> +#define AMD_SDW1_PAD_EN_MASK				0x10
> +#define AMD_SDW_PAD_EN_MASK	(AMD_SDW0_PAD_EN_MASK | AMD_SDW1_PAD_EN_MASK)
> +
> +static int amd_enable_sdw_pads(void __iomem *mmio, u32 link_mask, struct device *dev)
> +{
> +	u32 val;
> +	u32 pad_keeper_en_mask, pad_pulldown_ctrl_mask;
> +
> +	switch (link_mask) {
> +	case 1:
> +		pad_keeper_en_mask = AMD_SDW0_PAD_EN_MASK;
> +		pad_pulldown_ctrl_mask = AMD_SDW0_PAD_PULLDOWN_CTRL_ENABLE_MASK;
> +		break;
> +	case 2:
> +		pad_keeper_en_mask = AMD_SDW1_PAD_EN_MASK;
> +		pad_pulldown_ctrl_mask = AMD_SDW1_PAD_PULLDOWN_CTRL_ENABLE_MASK;
> +		break;
> +	case 3:
> +		pad_keeper_en_mask = AMD_SDW_PAD_EN_MASK;
> +		pad_pulldown_ctrl_mask = AMD_SDW_PAD_PULLDOWN_CTRL_ENABLE_MASK;
> +		break;
> +	default:
> +		dev_err(dev, "No SDW Links are enabled\n");
> +		return -ENODEV;
> +	}
> +
> +	val = readl(mmio + ACP_SW_PAD_KEEPER_EN);
> +	val |= pad_keeper_en_mask;
> +	writel(val, mmio + ACP_SW_PAD_KEEPER_EN);
> +	val = readl(mmio + ACP_PAD_PULLDOWN_CTRL);
> +	val &= pad_pulldown_ctrl_mask;
> +	writel(val, mmio + ACP_PAD_PULLDOWN_CTRL);

updatel() local macro?

> +	return 0;
> +}
> +
>  static int sdw_amd_cleanup(struct sdw_amd_ctx *ctx)
>  {
>  	int i;
> @@ -37,6 +78,7 @@ static struct sdw_amd_ctx *sdw_amd_probe_controller(struct sdw_amd_res *res)
>  	struct platform_device_info pdevinfo[2];
>  	u32 link_mask;
>  	int count, index;
> +	int ret;
>  
>  	if (!res)
>  		return NULL;
> @@ -50,6 +92,9 @@ static struct sdw_amd_ctx *sdw_amd_probe_controller(struct sdw_amd_res *res)
>  
>  	count = res->count;
>  	dev_dbg(&adev->dev, "Creating %d SDW Link devices\n", count);
> +	ret = amd_enable_sdw_pads(res->mmio_base, res->link_mask, res->parent);
> +	if (ret)
> +		return NULL;
>  
>  	/*
>  	 * we need to alloc/free memory manually and can't use devm:
> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index c27b0b0f33a6..1427cccfc309 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -26,23 +26,6 @@
>  
>  #define to_amd_sdw(b)	container_of(b, struct amd_sdw_manager, bus)
>  
> -static void amd_enable_sdw_pads(struct amd_sdw_manager *amd_manager)
> -{
> -	u32 sw_pad_pulldown_val;
> -	u32 val;
> -
> -	mutex_lock(amd_manager->acp_sdw_lock);
> -	val = readl(amd_manager->acp_mmio + ACP_SW_PAD_KEEPER_EN);
> -	val |= amd_manager->reg_mask->sw_pad_enable_mask;
> -	writel(val, amd_manager->acp_mmio + ACP_SW_PAD_KEEPER_EN);
> -	usleep_range(1000, 1500);
> -
> -	sw_pad_pulldown_val = readl(amd_manager->acp_mmio + ACP_PAD_PULLDOWN_CTRL);
> -	sw_pad_pulldown_val &= amd_manager->reg_mask->sw_pad_pulldown_mask;
> -	writel(sw_pad_pulldown_val, amd_manager->acp_mmio + ACP_PAD_PULLDOWN_CTRL);
> -	mutex_unlock(amd_manager->acp_sdw_lock);

so the code is copied from a GPL declared file to now and GPL + BSD one!
Have you had lawyers look into this... why change one file license ?

> -}
> -
>  static int amd_init_sdw_manager(struct amd_sdw_manager *amd_manager)
>  {
>  	u32 val;
> @@ -872,7 +855,6 @@ int amd_sdw_manager_start(struct amd_sdw_manager *amd_manager)
>  
>  	prop = &amd_manager->bus.prop;
>  	if (!prop->hw_disabled) {
> -		amd_enable_sdw_pads(amd_manager);
>  		ret = amd_init_sdw_manager(amd_manager);
>  		if (ret)
>  			return ret;
> -- 
> 2.34.1
  

On 21/12/23 22:08, Vinod Koul wrote:
> On 21-12-23, 13:05, Vijendar Mukunda wrote:
>> As sdw pads enable sequence is executed only once, invoke it
>> from probe sequence.
>>
>> Program required pads for both manager instances
>> based on link_mask during probe sequence. This will avoid
>> acquiring mutex lock.
> something wrong with your editor to have this formatting, you can use
> upto 80 chars here!

Will check.
>> Remove unnecessary delay after programming ACP_SW_PAD_KEEPER_EN
>> register.
>>
>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>> ---
>>  drivers/soundwire/amd_init.c    | 45 +++++++++++++++++++++++++++++++++
>>  drivers/soundwire/amd_manager.c | 18 -------------
>>  2 files changed, 45 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/soundwire/amd_init.c b/drivers/soundwire/amd_init.c
>> index 5c9569d9ad01..b3b3c7266384 100644
>> --- a/drivers/soundwire/amd_init.c
>> +++ b/drivers/soundwire/amd_init.c
>> @@ -15,6 +15,47 @@
>>  
>>  #include "amd_init.h"
>>  
>> +#define ACP_PAD_PULLDOWN_CTRL				0x0001448
>> +#define ACP_SW_PAD_KEEPER_EN				0x0001454
>> +#define AMD_SDW_PAD_PULLDOWN_CTRL_ENABLE_MASK		0x7f9a
>> +#define AMD_SDW0_PAD_PULLDOWN_CTRL_ENABLE_MASK		0x7f9f
>> +#define AMD_SDW1_PAD_PULLDOWN_CTRL_ENABLE_MASK		0x7ffa
>> +#define AMD_SDW0_PAD_EN_MASK				1
>> +#define AMD_SDW1_PAD_EN_MASK				0x10
>> +#define AMD_SDW_PAD_EN_MASK	(AMD_SDW0_PAD_EN_MASK | AMD_SDW1_PAD_EN_MASK)
>> +
>> +static int amd_enable_sdw_pads(void __iomem *mmio, u32 link_mask, struct device *dev)
>> +{
>> +	u32 val;
>> +	u32 pad_keeper_en_mask, pad_pulldown_ctrl_mask;
>> +
>> +	switch (link_mask) {
>> +	case 1:
>> +		pad_keeper_en_mask = AMD_SDW0_PAD_EN_MASK;
>> +		pad_pulldown_ctrl_mask = AMD_SDW0_PAD_PULLDOWN_CTRL_ENABLE_MASK;
>> +		break;
>> +	case 2:
>> +		pad_keeper_en_mask = AMD_SDW1_PAD_EN_MASK;
>> +		pad_pulldown_ctrl_mask = AMD_SDW1_PAD_PULLDOWN_CTRL_ENABLE_MASK;
>> +		break;
>> +	case 3:
>> +		pad_keeper_en_mask = AMD_SDW_PAD_EN_MASK;
>> +		pad_pulldown_ctrl_mask = AMD_SDW_PAD_PULLDOWN_CTRL_ENABLE_MASK;
>> +		break;
>> +	default:
>> +		dev_err(dev, "No SDW Links are enabled\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	val = readl(mmio + ACP_SW_PAD_KEEPER_EN);
>> +	val |= pad_keeper_en_mask;
>> +	writel(val, mmio + ACP_SW_PAD_KEEPER_EN);
>> +	val = readl(mmio + ACP_PAD_PULLDOWN_CTRL);
>> +	val &= pad_pulldown_ctrl_mask;
>> +	writel(val, mmio + ACP_PAD_PULLDOWN_CTRL);
> updatel() local macro?
We have identified similar update() inline function should be used in other
parts of the code with different mmio base address. We will push it as a
separate patch.

>> +	return 0;
>> +}
>> +
>>  static int sdw_amd_cleanup(struct sdw_amd_ctx *ctx)
>>  {
>>  	int i;
>> @@ -37,6 +78,7 @@ static struct sdw_amd_ctx *sdw_amd_probe_controller(struct sdw_amd_res *res)
>>  	struct platform_device_info pdevinfo[2];
>>  	u32 link_mask;
>>  	int count, index;
>> +	int ret;
>>  
>>  	if (!res)
>>  		return NULL;
>> @@ -50,6 +92,9 @@ static struct sdw_amd_ctx *sdw_amd_probe_controller(struct sdw_amd_res *res)
>>  
>>  	count = res->count;
>>  	dev_dbg(&adev->dev, "Creating %d SDW Link devices\n", count);
>> +	ret = amd_enable_sdw_pads(res->mmio_base, res->link_mask, res->parent);
>> +	if (ret)
>> +		return NULL;
>>  
>>  	/*
>>  	 * we need to alloc/free memory manually and can't use devm:
>> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
>> index c27b0b0f33a6..1427cccfc309 100644
>> --- a/drivers/soundwire/amd_manager.c
>> +++ b/drivers/soundwire/amd_manager.c
>> @@ -26,23 +26,6 @@
>>  
>>  #define to_amd_sdw(b)	container_of(b, struct amd_sdw_manager, bus)
>>  
>> -static void amd_enable_sdw_pads(struct amd_sdw_manager *amd_manager)
>> -{
>> -	u32 sw_pad_pulldown_val;
>> -	u32 val;
>> -
>> -	mutex_lock(amd_manager->acp_sdw_lock);
>> -	val = readl(amd_manager->acp_mmio + ACP_SW_PAD_KEEPER_EN);
>> -	val |= amd_manager->reg_mask->sw_pad_enable_mask;
>> -	writel(val, amd_manager->acp_mmio + ACP_SW_PAD_KEEPER_EN);
>> -	usleep_range(1000, 1500);
>> -
>> -	sw_pad_pulldown_val = readl(amd_manager->acp_mmio + ACP_PAD_PULLDOWN_CTRL);
>> -	sw_pad_pulldown_val &= amd_manager->reg_mask->sw_pad_pulldown_mask;
>> -	writel(sw_pad_pulldown_val, amd_manager->acp_mmio + ACP_PAD_PULLDOWN_CTRL);
>> -	mutex_unlock(amd_manager->acp_sdw_lock);
> so the code is copied from a GPL declared file to now and GPL + BSD one!
> Have you had lawyers look into this... why change one file license ?
As per recommendations from our legal team, we have updated the license as dual
one for amd_init.c file.
We have also observed that license terms should be updated for other files as
well (amd_manager.c, amd_manager.h & sdw_amd.h) as dual one, which we have
planned to submit as a supplement patch.

>> -}
>> -
>>  static int amd_init_sdw_manager(struct amd_sdw_manager *amd_manager)
>>  {
>>  	u32 val;
>> @@ -872,7 +855,6 @@ int amd_sdw_manager_start(struct amd_sdw_manager *amd_manager)
>>  
>>  	prop = &amd_manager->bus.prop;
>>  	if (!prop->hw_disabled) {
>> -		amd_enable_sdw_pads(amd_manager);
>>  		ret = amd_init_sdw_manager(amd_manager);
>>  		if (ret)
>>  			return ret;
>> -- 
>> 2.34.1
  

On 22-12-23, 12:45, Mukunda,Vijendar wrote:
> On 21/12/23 22:08, Vinod Koul wrote:

> > so the code is copied from a GPL declared file to now and GPL + BSD one!
> > Have you had lawyers look into this... why change one file license ?
> As per recommendations from our legal team, we have updated the license as dual
> one for amd_init.c file.
> We have also observed that license terms should be updated for other files as
> well (amd_manager.c, amd_manager.h & sdw_amd.h) as dual one, which we have
> planned to submit as a supplement patch.

Lets change that first before we move code from one license file to
another

Btw why would you want to do the change of license form GPL to dual?
  

On 22/12/23 14:51, Vinod Koul wrote:
> On 22-12-23, 12:45, Mukunda,Vijendar wrote:
>> On 21/12/23 22:08, Vinod Koul wrote:
>>> so the code is copied from a GPL declared file to now and GPL + BSD one!
>>> Have you had lawyers look into this... why change one file license ?
>> As per recommendations from our legal team, we have updated the license as dual
>> one for amd_init.c file.
>> We have also observed that license terms should be updated for other files as
>> well (amd_manager.c, amd_manager.h & sdw_amd.h) as dual one, which we have
>> planned to submit as a supplement patch.
> Lets change that first before we move code from one license file to
> another
Will push the license update patch first.
>
> Btw why would you want to do the change of license form GPL to dual?
As this code being used by AMD SOF stack which uses dual license,
So we want to maintain the same license terms.
>
  

On 22-12-23, 16:04, Mukunda,Vijendar wrote:
> On 22/12/23 14:51, Vinod Koul wrote:
> > On 22-12-23, 12:45, Mukunda,Vijendar wrote:
> >> On 21/12/23 22:08, Vinod Koul wrote:
> >>> so the code is copied from a GPL declared file to now and GPL + BSD one!
> >>> Have you had lawyers look into this... why change one file license ?
> >> As per recommendations from our legal team, we have updated the license as dual
> >> one for amd_init.c file.
> >> We have also observed that license terms should be updated for other files as
> >> well (amd_manager.c, amd_manager.h & sdw_amd.h) as dual one, which we have
> >> planned to submit as a supplement patch.
> > Lets change that first before we move code from one license file to
> > another
> Will push the license update patch first.
> >
> > Btw why would you want to do the change of license form GPL to dual?
> As this code being used by AMD SOF stack which uses dual license,
> So we want to maintain the same license terms.

SOF is firmware, do you share this kernel code with sofproject, that
doesnt make sense to me, maybe I am missing something
  

On 22/12/23 21:15, Vinod Koul wrote:
> On 22-12-23, 16:04, Mukunda,Vijendar wrote:
>> On 22/12/23 14:51, Vinod Koul wrote:
>>> On 22-12-23, 12:45, Mukunda,Vijendar wrote:
>>>> On 21/12/23 22:08, Vinod Koul wrote:
>>>>> so the code is copied from a GPL declared file to now and GPL + BSD one!
>>>>> Have you had lawyers look into this... why change one file license ?
>>>> As per recommendations from our legal team, we have updated the license as dual
>>>> one for amd_init.c file.
>>>> We have also observed that license terms should be updated for other files as
>>>> well (amd_manager.c, amd_manager.h & sdw_amd.h) as dual one, which we have
>>>> planned to submit as a supplement patch.
>>> Lets change that first before we move code from one license file to
>>> another
>> Will push the license update patch first.
>>> Btw why would you want to do the change of license form GPL to dual?
>> As this code being used by AMD SOF stack which uses dual license,
>> So we want to maintain the same license terms.
> SOF is firmware, do you share this kernel code with sofproject, that
> doesnt make sense to me, maybe I am missing something
We meant to say this code being used by AMD sof driver stack
(sound/soc/sof/amd) which has dual license.