LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
To: Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>, Jaroslav Kysela <perex@perex.cz>,
	Takashi Iwai <tiwai@suse.com>, Heiko Stuebner <heiko@sntech.de>,
	Philipp Zabel <p.zabel@pengutronix.de>
Cc: linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org,
	linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org
Subject: Re: [PATCH v2 1/4] ASoC: rockchip: add support for i2s-tdm controller
Date: Mon, 23 Aug 2021 16:35:42 +0200	[thread overview]
Message-ID: <9368735.gdWEK3B62L@archbook> (raw)
In-Reply-To: <6bc6330215980f10853a2dac69f0bdfd9c8fb303.camel@pengutronix.de>

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.

> > +	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.)

> > +	if (!IS_ERR(i2s_tdm->hclk))
> > +		clk_disable_unprepare(i2s_tdm->hclk);
> > +
> > +	return 0;
> > +}
> 
> regards
> Philipp

Regards,
Nicolas Frattaroli



  reply	other threads:[~2021-08-23 14:35 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-20 18:27 [PATCH v2 0/4] Rockchip I2S/TDM controller Nicolas Frattaroli
2021-08-20 18:27 ` [PATCH v2 1/4] ASoC: rockchip: add support for i2s-tdm controller Nicolas Frattaroli
2021-08-20 19:02   ` Pierre-Louis Bossart
2021-08-21 20:45     ` Nicolas Frattaroli
2021-08-23 11:19       ` Mark Brown
2021-08-23 15:02       ` Pierre-Louis Bossart
2021-08-23 12:03   ` Philipp Zabel
2021-08-23 14:35     ` Nicolas Frattaroli [this message]
2021-08-23 16:03       ` Philipp Zabel
2021-08-20 18:27 ` [PATCH v2 2/4] dt-bindings: sound: add rockchip i2s-tdm binding Nicolas Frattaroli
2021-08-24 16:42   ` Rob Herring
2021-08-20 18:27 ` [PATCH v2 3/4] arm64: dts: rockchip: add i2s1 on rk356x Nicolas Frattaroli
2021-08-20 18:27 ` [PATCH v2 4/4] arm64: dts: rockchip: add analog audio on Quartz64 Nicolas Frattaroli

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9368735.gdWEK3B62L@archbook \
    --to=frattaroli.nicolas@gmail.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=heiko@sntech.de \
    --cc=lgirdwood@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=p.zabel@pengutronix.de \
    --cc=perex@perex.cz \
    --cc=tiwai@suse.com \
    --subject='Re: [PATCH v2 1/4] ASoC: rockchip: add support for i2s-tdm controller' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).