Re: [PATCH v2 1/4] ASoC: rockchip: add support for i2s-tdm controller【请注意，邮件由linux-rockchip-bounces+sugar.zhang=rock-chips.com@lists.infradead.org代发】

[sugar zhang <sugar.zhang@rock-chips.com>]

Hi Nicolas, Philipp,

On 2021/8/23 22:35, Nicolas Frattaroli wrote:
> Hi Philipp,
>
> On Montag, 23. August 2021 14:03:25 CEST Philipp Zabel wrote:
>> Hi Nicolas,
>>
>> On Fri, 2021-08-20 at 20:27 +0200, Nicolas Frattaroli wrote:
>> [...]
>>
>>> diff --git a/sound/soc/rockchip/rockchip_i2s_tdm.c
>>> b/sound/soc/rockchip/rockchip_i2s_tdm.c new file mode 100644
>>> index 000000000000..c02b66f3c913
>>> --- /dev/null
>>> +++ b/sound/soc/rockchip/rockchip_i2s_tdm.c
>>> @@ -0,0 +1,1737 @@
>> [...]
>>
>>> +static void rockchip_snd_xfer_reset_assert(struct rk_i2s_tdm_dev
>>> *i2s_tdm,
>>> +					   int tx_bank, int tx_offset,
>>> +					   int rx_bank, int rx_offset)
>>> +{
>>> +	void __iomem *cru_reset;
>>> +	unsigned long flags;
>>> +
>>> +	cru_reset = i2s_tdm->cru_base + i2s_tdm->soc_data->softrst_offset;
>>> +
>>> +	if (tx_bank == rx_bank) {
>>> +		writel(BIT(tx_offset) | BIT(rx_offset) |
>>> +		       (BIT(tx_offset) << 16) | (BIT(rx_offset) << 16),
>>> +		       cru_reset + (tx_bank * 4));
>>> +	} else {
>>> +		local_irq_save(flags);
>>> +		writel(BIT(tx_offset) | (BIT(tx_offset) << 16),
>>> +		       cru_reset + (tx_bank * 4));
>>> +		writel(BIT(rx_offset) | (BIT(rx_offset) << 16),
>>> +		       cru_reset + (rx_bank * 4));
>>> +		local_irq_restore(flags);
>>> +	}
>>> +}
>>> +
>>> +static void rockchip_snd_xfer_reset_deassert(struct rk_i2s_tdm_dev
>>> *i2s_tdm, +					     int tx_bank, int
> tx_offset,
>>> +					     int rx_bank, int rx_offset)
>>> +{
>>> +	void __iomem *cru_reset;
>>> +	unsigned long flags;
>>> +
>>> +	cru_reset = i2s_tdm->cru_base + i2s_tdm->soc_data->softrst_offset;
>>> +
>>> +	if (tx_bank == rx_bank) {
>>> +		writel((BIT(tx_offset) << 16) | (BIT(rx_offset) << 16),
>>> +		       cru_reset + (tx_bank * 4));
>>> +	} else {
>>> +		local_irq_save(flags);
>>> +		writel((BIT(tx_offset) << 16),
>>> +		       cru_reset + (tx_bank * 4));
>>> +		writel((BIT(rx_offset) << 16),
>>> +		       cru_reset + (rx_bank * 4));
>>> +		local_irq_restore(flags);
>>> +	}
>>> +}
>>> +
>>> +/*
>>> + * Makes sure that both tx and rx are reset at the same time to sync lrck
>>> + * when clk_trcm > 0.
>>> + */
>>> +static void rockchip_snd_xfer_sync_reset(struct rk_i2s_tdm_dev *i2s_tdm)
>>> +{
>>> +	int tx_id, rx_id;
>>> +	int tx_bank, rx_bank, tx_offset, rx_offset;
>>> +
>>> +	if (!i2s_tdm->cru_base || !i2s_tdm->soc_data)
>>> +		return;
>>> +
>>> +	tx_id = i2s_tdm->tx_reset_id;
>>> +	rx_id = i2s_tdm->rx_reset_id;
>>> +	if (tx_id < 0 || rx_id < 0)
>>> +		return;
>>> +
>>> +	tx_bank = tx_id / 16;
>>> +	tx_offset = tx_id % 16;
>>> +	rx_bank = rx_id / 16;
>>> +	rx_offset = rx_id % 16;
>>> +	dev_dbg(i2s_tdm->dev,
>>> +		"tx_bank: %d, rx_bank: %d, tx_offset: %d, rx_offset: %d\n",
>>> +		tx_bank, rx_bank, tx_offset, rx_offset);
>>> +
>>> +	rockchip_snd_xfer_reset_assert(i2s_tdm, tx_bank, tx_offset,
>>> +				       rx_bank, rx_offset);
>>> +
>>> +	udelay(150);
>>> +
>>> +	rockchip_snd_xfer_reset_deassert(i2s_tdm, tx_bank, tx_offset,
>>> +					 rx_bank, rx_offset);
>>> +}
>> I'm not too fond of reimplementing half a reset controller in here.
>> The reset framework does not support synchronized (de)assertion of
>> multiple reset controls, I wonder if this would be useful to add.
>> Without having thought about this too hard, I could imagine this as an
>> extension to the bulk API, or possibly a call to join multiple reset
>> controls into a reset control array.
> I agree, the code required for synchronised reset seems to be a good
> candidate for a generalised solution elsewhere.
> However, I'm not sure if I'm the right candidate to design this API
> as my first kernel contribution when the board I'm currently testing
> this with doesn't even utilise the synchronized reset.
>
> If I come across an opportunity to test synchronised resets, I'll
> definitely look into refactoring this. I'd also be happy to hear of
> any other driver which is currently implementing its own synchronised
> reset.
>
> [...]
>   
>>> +
>>> +	reset_control_assert(rc);
>>> +	udelay(1);
>> What is the reason for the different delays in
>> rockchip_snd_xfer_sync_reset() and rockchip_snd_reset()?
>>
> Simply put: I have no idea. This is what downstream does, and it
> appears to work for me. The git history of the downstream kernel
> also doesn't tell me anything about why the sync reset is 150
> and this one is 1.
>
> I've got no device to test the sync reset with at the moment so
> I'm wary of playing with the delay value.

acctually, a 10 us delay is enough, and both 
rockchip_snd_xfer_sync_reset() and rockchip_snd_reset() should be the same

fixed in downstream patch:

From: Sugar Zhang <sugar.zhang@rock-chips.com>
Date: Mon, 31 May 2021 10:31:20 +0800
Subject: [PATCH] ASoC: rockchip: i2s-tdm: Delay for reset successfully

This patch Adds delay after reset deassert to make sure
reset done before do enabling xfer.

Considering the follow situation:

- i2s_mclk for 8K capture [2.048M]
- i2s_hclk for i2s register access [150M]
- pclk_cru for cru register access [100M]

        SW                               HW

i2s reset assert   [pclk_cru]           |
         |                               |
    delay time                           |
         |                       i2s reset assert   [i2s_mclk]
i2s reset deassert [pclk_cru]           |
         |                               |
i2s xfer enable    [i2s_hclk]           |
         |                       i2s reset deassert [i2s_mclk]

Obviously, pclk_cru(10ns per cycle) is much faster than i2s_mclk
(500ns per cycle). so delay should be added after reset deassert
to make sure hw reset done. Otherwise, the race between reset and
enable xfer maybe break i2s data aligned.

Fixes: A 10us delay is enough

        SW                               HW

i2s reset assert   [pclk_cru]           |
         |                               |
    delay 10us                           |
         |                       i2s reset assert   [i2s_mclk]
i2s reset deassert [pclk_cru]           |
         |                               |
    delay 10us                           |
         |                       i2s reset deassert [i2s_mclk]
i2s xfer enable    [i2s_hclk]           |
         |                               |

Change-Id: Id370b0aa13f771053841ce04a554b408e9e3c831
Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>

---
  sound/soc/rockchip/rockchip_i2s_tdm.c | 12 ++++++++----
  1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/sound/soc/rockchip/rockchip_i2s_tdm.c 
b/sound/soc/rockchip/rockchip_i2s_tdm.c
index 42d1607..1c06585 100644
--- a/sound/soc/rockchip/rockchip_i2s_tdm.c
+++ b/sound/soc/rockchip/rockchip_i2s_tdm.c
@@ -216,6 +216,8 @@ static void rockchip_snd_xfer_reset_assert(struct 
rk_i2s_tdm_dev *i2s_tdm,
                 local_irq_restore(flags);
                 break;
         }
+       /* delay for reset assert done */
+       udelay(10);
  }

  static void rockchip_snd_xfer_reset_deassert(struct rk_i2s_tdm_dev 
*i2s_tdm,
@@ -260,6 +262,8 @@ static void rockchip_snd_xfer_reset_deassert(struct 
rk_i2s_tdm_dev *i2s_tdm,
                 local_irq_restore(flags);
                 break;
         }
+       /* delay for reset deassert done */
+       udelay(10);
  }

  /*
@@ -289,9 +293,6 @@ static void rockchip_snd_xfer_sync_reset(struct 
rk_i2s_tdm_dev *i2s_tdm)

         rockchip_snd_xfer_reset_assert(i2s_tdm, tx_bank, tx_offset,
                                        rx_bank, rx_offset);
-
-       udelay(150);
-
         rockchip_snd_xfer_reset_deassert(i2s_tdm, tx_bank, tx_offset,
                                          rx_bank, rx_offset);
  }
@@ -369,8 +370,11 @@ static void rockchip_snd_reset(struct reset_control 
*rc)
                 return;

         reset_control_assert(rc);
-       udelay(1);
+       /* delay for reset assert done */
+       udelay(10);
         reset_control_deassert(rc);
+       /* delay for reset deassert done */
+       udelay(10);
  }

>>> +	reset_control_deassert(rc);
>>> +}
>> [...]
>>
>>> +static int rockchip_i2s_tdm_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device_node *node = pdev->dev.of_node;
>>> +	struct device_node *cru_node;
>>> +	const struct of_device_id *of_id;
>>> +	struct rk_i2s_tdm_dev *i2s_tdm;
>>> +	struct resource *res;
>>> +	void __iomem *regs;
>>> +	int ret;
>>> +	int val;
>>> +
>>> +	i2s_tdm = devm_kzalloc(&pdev->dev, sizeof(*i2s_tdm), GFP_KERNEL);
>>> +	if (!i2s_tdm)
>>> +		return -ENOMEM;
>>> +
>>> +	i2s_tdm->dev = &pdev->dev;
>>> +
>>> +	of_id = of_match_device(rockchip_i2s_tdm_match, &pdev->dev);
>>> +	if (!of_id || !of_id->data)
>>> +		return -EINVAL;
>>> +
>>> +	spin_lock_init(&i2s_tdm->lock);
>>> +	i2s_tdm->soc_data = (struct rk_i2s_soc_data *)of_id->data;
>>> +
>>> +	i2s_tdm->frame_width = 64;
>>> +	if (!of_property_read_u32(node, "rockchip,frame-width", &val)) {
>>> +		if (val >= 32 && (val % 2 == 0) && val <= 512) {
>>> +			i2s_tdm->frame_width = val;
>>> +		} else {
>>> +			dev_err(i2s_tdm->dev, "unsupported frame width:
> '%d'\n",
>>> +				val);
>>> +			return -EINVAL;
>>> +		}
>>> +	}
>>> +
>>> +	i2s_tdm->clk_trcm = TRCM_TXRX;
>>> +	if (of_property_read_bool(node, "rockchip,trcm-sync-tx-only"))
>>> +		i2s_tdm->clk_trcm = TRCM_TX;
>>> +	if (of_property_read_bool(node, "rockchip,trcm-sync-rx-only")) {
>>> +		if (i2s_tdm->clk_trcm) {
>>> +			dev_err(i2s_tdm->dev, "invalid trcm-sync
> configuration\n");
>>> +			return -EINVAL;
>>> +		}
>>> +		i2s_tdm->clk_trcm = TRCM_RX;
>>> +	}
>>> +	if (i2s_tdm->clk_trcm != TRCM_TXRX)
>>> +		i2s_tdm_dai.symmetric_rate = 1;
>>> +
>>> +	i2s_tdm->tdm_fsync_half_frame =
>>> +		of_property_read_bool(node, "rockchip,tdm-fsync-half-frame");
>>> +
>>> +	i2s_tdm->grf = syscon_regmap_lookup_by_phandle(node, "rockchip,grf");
>>> +	if (IS_ERR(i2s_tdm->grf))
>>> +		return dev_err_probe(i2s_tdm->dev, PTR_ERR(i2s_tdm->grf),
>>> +				     "Error in rockchip,grf\n");
>>> +
>>> +	if (i2s_tdm->clk_trcm != TRCM_TXRX) {
>>> +		cru_node = of_parse_phandle(node, "rockchip,cru", 0);
>>> +		i2s_tdm->cru_base = of_iomap(cru_node, 0);
>> This is a bit ugly if there is another driver sitting on the
>> rockchip,cru compatible node. Which reset controller driver is backing
>> the reset controls below?
> I'm not sure if I understand the question (I only just today learned that
> reset controls have drivers) but I think the reset it is using in the
> Quartz64 dts is backed by rk3568-cru. Let me know if I misunderstood you.
>
> [...]
>
>>> +static int rockchip_i2s_tdm_remove(struct platform_device *pdev)
>>> +{
>>> +	struct rk_i2s_tdm_dev *i2s_tdm = dev_get_drvdata(&pdev->dev);
>>> +
>>> +	pm_runtime_disable(&pdev->dev);
>>> +	if (!pm_runtime_status_suspended(&pdev->dev))
>>> +		i2s_tdm_runtime_suspend(&pdev->dev);
>>> +
>>> +	if (!IS_ERR(i2s_tdm->mclk_tx))
>>> +		clk_prepare_enable(i2s_tdm->mclk_tx);
>>> +	if (!IS_ERR(i2s_tdm->mclk_rx))
>>> +		clk_prepare_enable(i2s_tdm->mclk_rx);
>> Why are we enabling these clocks now?
>>
> I wondered that too while I was looking into the pm_runtime stuff,
> and decided to just throw these two calls out. They don't seem to
> make sense to me, and nothing I tested broke when I removed them.
>
> If left in (and the code works as I hypothesise it works) then
> this would simply re-enable clocks we have just disabled, which
> would make the number of enable and disable calls unbalanced.
> Highly sus, as the kids would say, and completely omits the other
> mclk_tx_src etc. clocks. (Though those are forgotten elsewhere in
> the code as well, which I have since fixed, along with any of the
> review points I don't directly respond to.)
it's copy-pasted mistake, should be clk_disable_unprepare for them.
>>> +	if (!IS_ERR(i2s_tdm->hclk))
>>> +		clk_disable_unprepare(i2s_tdm->hclk);
>>> +
>>> +	return 0;
>>> +}
>> regards
>> Philipp
> Regards,
> Nicolas Frattaroli
>
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
>
-- 
Best Regards!
张学广/Sugar
瑞芯微电子股份有限公司
Rockchip Electronics Co., Ltd.




_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip