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

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