Re: [PATCH 3/5] ASoC: rt5682: clock driver must use the clock provider API
On Mon 12 Apr 2021 at 22:27, Stephen Boyd wrote: > Quoting Jerome Brunet (2021-04-10 04:13:54) >> Clock drivers ops should not the clk API but the clock provider (clk_hw) >> instead. >> >> Signed-off-by: Jerome Brunet >> --- >> sound/soc/codecs/rt5682.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/sound/soc/codecs/rt5682.c b/sound/soc/codecs/rt5682.c >> index 0e2a10ed11da..2eee02ac8d49 100644 >> --- a/sound/soc/codecs/rt5682.c >> +++ b/sound/soc/codecs/rt5682.c >> @@ -2634,7 +2634,7 @@ static int rt5682_wclk_set_rate(struct clk_hw *hw, >> unsigned long rate, >> container_of(hw, struct rt5682_priv, >> dai_clks_hw[RT5682_DAI_WCLK_IDX]); >> struct snd_soc_component *component = rt5682->component; >> - struct clk *parent_clk; >> + struct clk_hw *parent_hw; >> const char * const clk_name = clk_hw_get_name(hw); >> int pre_div; >> unsigned int clk_pll2_out; >> @@ -2649,8 +2649,8 @@ static int rt5682_wclk_set_rate(struct clk_hw *hw, >> unsigned long rate, >> * >> * It will set the codec anyway by assuming mclk is 48MHz. >> */ >> - parent_clk = clk_get_parent(hw->clk); >> - if (!parent_clk) >> + parent_hw = clk_hw_get_parent(hw); >> + if (!parent_hw) >> dev_warn(component->dev, >> "Parent mclk of wclk not acquired in driver. Please >> ensure mclk was provided as %d Hz.\n", >> CLK_PLL2_FIN); > > Can this code be removed? I don't know why we care to check if the clk > has a parent or not. I'm focusing on removing "hw->clk" where they are - w/o changing too much what the driver does. I don't have the HW nor the story behind it and there is about 50 more drivers to be fixed ... thankfully, most are in drivers/clk/ ;) Here, at least the clock consummer API is not longer used within a clock ops, which is not great considering the locking scheme (among other things)
[PATCH] ASoC: meson: axg-frddr: fix fifo depth on g12 and sm1
Previous fifo depth patch was only tested on axg, not g12 or sm1.
Of course, while adding hw_params dai callback for the axg, I forgot to do
the same for g12 and sm1, leaving the depth unset and breaking playback on
these SoCs.
Add hw_params callback to the g12 dai_ops to fix the problem.
Fixes: 6f68accaa864 ("ASoC: meson: axg-frddr: set fifo depth according to the
period")
Reported-by: Christian Hewitt
Tested-by: Christian Hewitt
Signed-off-by: Jerome Brunet
---
sound/soc/meson/axg-frddr.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/sound/soc/meson/axg-frddr.c b/sound/soc/meson/axg-frddr.c
index 8ed114de0bdf..37f4bb3469b5 100644
--- a/sound/soc/meson/axg-frddr.c
+++ b/sound/soc/meson/axg-frddr.c
@@ -171,6 +171,7 @@ static const struct axg_fifo_match_data
axg_frddr_match_data = {
static const struct snd_soc_dai_ops g12a_frddr_ops = {
.prepare= g12a_frddr_dai_prepare,
+ .hw_params = axg_frddr_dai_hw_params,
.startup= axg_frddr_dai_startup,
.shutdown = axg_frddr_dai_shutdown,
};
--
2.30.2
Re: [PATCH 4/5] ASoC: lpass: use the clock provider API
On Mon 12 Apr 2021 at 11:38, Srinivas Kandagatla
wrote:
> Thanks Jerome for the patch,
>
>
> On 10/04/2021 12:13, Jerome Brunet wrote:
>> Clock providers should be registered using the clk_hw API.
>> Signed-off-by: Jerome Brunet
>> ---
>> sound/soc/codecs/lpass-va-macro.c | 2 +-
>> sound/soc/codecs/lpass-wsa-macro.c | 9 +++--
>> 2 files changed, 4 insertions(+), 7 deletions(-)
>> diff --git a/sound/soc/codecs/lpass-va-macro.c
>> b/sound/soc/codecs/lpass-va-macro.c
>> index 5294c57b2cd4..56b887301172 100644
>> --- a/sound/soc/codecs/lpass-va-macro.c
>> +++ b/sound/soc/codecs/lpass-va-macro.c
>> @@ -1343,7 +1343,7 @@ static int va_macro_register_fsgen_output(struct
>> va_macro *va)
>> if (ret)
>> return ret;
>> - return of_clk_add_provider(np, of_clk_src_simple_get, va->hw.clk);
>> +return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, >hw);
>
> Now that we convert this to devm, You missed error path and driver remove
> where we delete clk provider. This should be removed as well as part of
> this patch.
Indeed. I should not have switched to devm here - It was not really the
purpose of the patch. Habits I guess.
Do you prefer I stick with devm (with the suggested fix) or revert to the
no-devm way for the v2 ? It makes no difference to me TBH.
>
>
> This applies to both wsa and va macro.
>
> Thanks,
> srini
>> }
>> static int va_macro_validate_dmic_sample_rate(u32 dmic_sample_rate,
>> diff --git a/sound/soc/codecs/lpass-wsa-macro.c
>> b/sound/soc/codecs/lpass-wsa-macro.c
>> index e79a70386b4b..acb95e83c788 100644
>> --- a/sound/soc/codecs/lpass-wsa-macro.c
>> +++ b/sound/soc/codecs/lpass-wsa-macro.c
>> @@ -2337,10 +2337,9 @@ static const struct clk_ops swclk_gate_ops = {
>> .recalc_rate = swclk_recalc_rate,
>> };
>> -static struct clk *wsa_macro_register_mclk_output(struct wsa_macro
>> *wsa)
>> +static int wsa_macro_register_mclk_output(struct wsa_macro *wsa)
>> {
>> struct device *dev = wsa->dev;
>> -struct device_node *np = dev->of_node;
>> const char *parent_clk_name;
>> const char *clk_name = "mclk";
>> struct clk_hw *hw;
>> @@ -2358,11 +2357,9 @@ static struct clk
>> *wsa_macro_register_mclk_output(struct wsa_macro *wsa)
>> hw = >hw;
>> ret = clk_hw_register(wsa->dev, hw);
>> if (ret)
>> -return ERR_PTR(ret);
>> -
>> -of_clk_add_provider(np, of_clk_src_simple_get, hw->clk);
>> +return ret;
>> - return NULL;
>> +return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);
>> }
>> static const struct snd_soc_component_driver wsa_macro_component_drv
>> = {
>>
[PATCH 5/5] ASoC: da7219: properly get clk from the provider
Instead of using the clk embedded in the clk_hw (which is meant to go
away), a clock provider which need to interact with its own clock should
request clk reference through the clock provider API.
Signed-off-by: Jerome Brunet
---
sound/soc/codecs/da7219.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index 13009d08b09a..bd3c523a8617 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -2181,7 +2181,10 @@ static int da7219_register_dai_clks(struct
snd_soc_component *component)
ret);
goto err;
}
- da7219->dai_clks[i] = dai_clk_hw->clk;
+
+ da7219->dai_clks[i] = devm_clk_hw_get_clk(dev, dai_clk_hw,
NULL);
+ if (IS_ERR(da7219->dai_clks[i]))
+ return PTR_ERR(da7219->dai_clks[i]);
/* For DT setup onecell data, otherwise create lookup */
if (np) {
--
2.30.2
[PATCH 2/5] ASoC: wcd934x: use the clock provider API
Clock providers should use the clk_hw API Signed-off-by: Jerome Brunet --- sound/soc/codecs/wcd934x.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/sound/soc/codecs/wcd934x.c b/sound/soc/codecs/wcd934x.c index 5fe403307b72..ae3ea136a9f8 100644 --- a/sound/soc/codecs/wcd934x.c +++ b/sound/soc/codecs/wcd934x.c @@ -2116,11 +2116,13 @@ static struct clk *wcd934x_register_mclk_output(struct wcd934x_codec *wcd) wcd->hw.init = hw = >hw; - ret = clk_hw_register(wcd->dev->parent, hw); + ret = devm_clk_hw_register(wcd->dev->parent, hw); if (ret) return ERR_PTR(ret); - of_clk_add_provider(np, of_clk_src_simple_get, hw->clk); + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw); + if (ret) + return ERR_PTR(ret); return NULL; } -- 2.30.2
[PATCH 4/5] ASoC: lpass: use the clock provider API
Clock providers should be registered using the clk_hw API.
Signed-off-by: Jerome Brunet
---
sound/soc/codecs/lpass-va-macro.c | 2 +-
sound/soc/codecs/lpass-wsa-macro.c | 9 +++--
2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/sound/soc/codecs/lpass-va-macro.c
b/sound/soc/codecs/lpass-va-macro.c
index 5294c57b2cd4..56b887301172 100644
--- a/sound/soc/codecs/lpass-va-macro.c
+++ b/sound/soc/codecs/lpass-va-macro.c
@@ -1343,7 +1343,7 @@ static int va_macro_register_fsgen_output(struct va_macro
*va)
if (ret)
return ret;
- return of_clk_add_provider(np, of_clk_src_simple_get, va->hw.clk);
+ return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, >hw);
}
static int va_macro_validate_dmic_sample_rate(u32 dmic_sample_rate,
diff --git a/sound/soc/codecs/lpass-wsa-macro.c
b/sound/soc/codecs/lpass-wsa-macro.c
index e79a70386b4b..acb95e83c788 100644
--- a/sound/soc/codecs/lpass-wsa-macro.c
+++ b/sound/soc/codecs/lpass-wsa-macro.c
@@ -2337,10 +2337,9 @@ static const struct clk_ops swclk_gate_ops = {
.recalc_rate = swclk_recalc_rate,
};
-static struct clk *wsa_macro_register_mclk_output(struct wsa_macro *wsa)
+static int wsa_macro_register_mclk_output(struct wsa_macro *wsa)
{
struct device *dev = wsa->dev;
- struct device_node *np = dev->of_node;
const char *parent_clk_name;
const char *clk_name = "mclk";
struct clk_hw *hw;
@@ -2358,11 +2357,9 @@ static struct clk *wsa_macro_register_mclk_output(struct
wsa_macro *wsa)
hw = >hw;
ret = clk_hw_register(wsa->dev, hw);
if (ret)
- return ERR_PTR(ret);
-
- of_clk_add_provider(np, of_clk_src_simple_get, hw->clk);
+ return ret;
- return NULL;
+ return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);
}
static const struct snd_soc_component_driver wsa_macro_component_drv = {
--
2.30.2
[PATCH 3/5] ASoC: rt5682: clock driver must use the clock provider API
Clock drivers ops should not the clk API but the clock provider (clk_hw) instead. Signed-off-by: Jerome Brunet --- sound/soc/codecs/rt5682.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/sound/soc/codecs/rt5682.c b/sound/soc/codecs/rt5682.c index 0e2a10ed11da..2eee02ac8d49 100644 --- a/sound/soc/codecs/rt5682.c +++ b/sound/soc/codecs/rt5682.c @@ -2634,7 +2634,7 @@ static int rt5682_wclk_set_rate(struct clk_hw *hw, unsigned long rate, container_of(hw, struct rt5682_priv, dai_clks_hw[RT5682_DAI_WCLK_IDX]); struct snd_soc_component *component = rt5682->component; - struct clk *parent_clk; + struct clk_hw *parent_hw; const char * const clk_name = clk_hw_get_name(hw); int pre_div; unsigned int clk_pll2_out; @@ -2649,8 +2649,8 @@ static int rt5682_wclk_set_rate(struct clk_hw *hw, unsigned long rate, * * It will set the codec anyway by assuming mclk is 48MHz. */ - parent_clk = clk_get_parent(hw->clk); - if (!parent_clk) + parent_hw = clk_hw_get_parent(hw); + if (!parent_hw) dev_warn(component->dev, "Parent mclk of wclk not acquired in driver. Please ensure mclk was provided as %d Hz.\n", CLK_PLL2_FIN); -- 2.30.2
[PATCH 1/5] ASoC: stm32: properly get clk from the provider
Instead of using the clk embedded in the clk_hw (which is meant to go away), a clock provider which need to interact with its own clock should request clk reference through the clock provider API. Signed-off-by: Jerome Brunet --- sound/soc/stm/stm32_sai_sub.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/sound/soc/stm/stm32_sai_sub.c b/sound/soc/stm/stm32_sai_sub.c index 3aa1cf262402..c1561237ee24 100644 --- a/sound/soc/stm/stm32_sai_sub.c +++ b/sound/soc/stm/stm32_sai_sub.c @@ -484,7 +484,10 @@ static int stm32_sai_add_mclk_provider(struct stm32_sai_sub_data *sai) dev_err(dev, "mclk register returned %d\n", ret); return ret; } - sai->sai_mclk = hw->clk; + + sai->sai_mclk = devm_clk_hw_get_clk(dev, hw, NULL); + if (IS_ERR(sai->sai_mclk)) + return PTR_ERR(sai->sai_mclk); /* register mclk provider */ return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw); -- 2.30.2
[PATCH 0/5] ASoC: clock provider clean-up
The purpose of this patchset it remove the use the clk member of 'struct clk_hw' in ASoC. 'struct clk' is a per-user reference to an actual clock. In the future, the clk member in 'struct clk_hw' may go away. The usage of this member by a clock provider usually falls into either of following categories: * Mis-usage of the clock consumer API by a clock provider. * Clock provider also being a user of its own clocks. In this case the provider should request a 'struct clk' through the appropriate API instead of poking in 'struct clk_hw' internals. Jerome Brunet (5): ASoC: stm32: properly get clk from the provider ASoC: wcd934x: use the clock provider API ASoC: rt5682: clock driver must use the clock provider API ASoC: lpass: use the clock provider API ASoC: da7219: properly get clk from the provider sound/soc/codecs/da7219.c | 5 - sound/soc/codecs/lpass-va-macro.c | 2 +- sound/soc/codecs/lpass-wsa-macro.c | 9 +++-- sound/soc/codecs/rt5682.c | 6 +++--- sound/soc/codecs/wcd934x.c | 6 -- sound/soc/stm/stm32_sai_sub.c | 5 - 6 files changed, 19 insertions(+), 14 deletions(-) -- 2.30.2
[PATCH] ASoC: meson: axg-fifo: add NO_PERIOD_WAKEUP support
On the AXG family, the fifo irq is not necessary for the HW to operate.
It is just used to notify that a period has elapsed. If userpace does not
care for these wakeups (such as pipewire), we are just wasting CPU cycles.
Add support for NO_PERIOD_WAKEUP and disable irq when they are no needed.
Signed-off-by: Jerome Brunet
---
sound/soc/meson/axg-fifo.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/sound/soc/meson/axg-fifo.c b/sound/soc/meson/axg-fifo.c
index b2e867113226..b9af2d513e09 100644
--- a/sound/soc/meson/axg-fifo.c
+++ b/sound/soc/meson/axg-fifo.c
@@ -27,8 +27,8 @@ static struct snd_pcm_hardware axg_fifo_hw = {
SNDRV_PCM_INFO_MMAP |
SNDRV_PCM_INFO_MMAP_VALID |
SNDRV_PCM_INFO_BLOCK_TRANSFER |
-SNDRV_PCM_INFO_PAUSE),
-
+SNDRV_PCM_INFO_PAUSE |
+SNDRV_PCM_INFO_NO_PERIOD_WAKEUP),
.formats = AXG_FIFO_FORMATS,
.rate_min = 5512,
.rate_max = 192000,
@@ -113,7 +113,7 @@ int axg_fifo_pcm_hw_params(struct snd_soc_component
*component,
{
struct snd_pcm_runtime *runtime = ss->runtime;
struct axg_fifo *fifo = axg_fifo_data(ss);
- unsigned int burst_num, period, threshold;
+ unsigned int burst_num, period, threshold, irq_en;
dma_addr_t end_ptr;
period = params_period_bytes(params);
@@ -142,10 +142,11 @@ int axg_fifo_pcm_hw_params(struct snd_soc_component
*component,
regmap_field_write(fifo->field_threshold,
threshold ? threshold - 1 : 0);
- /* Enable block count irq */
+ /* Enable irq if necessary */
+ irq_en = runtime->no_period_wakeup ? 0 : FIFO_INT_COUNT_REPEAT;
regmap_update_bits(fifo->map, FIFO_CTRL0,
CTRL0_INT_EN(FIFO_INT_COUNT_REPEAT),
- CTRL0_INT_EN(FIFO_INT_COUNT_REPEAT));
+ CTRL0_INT_EN(irq_en));
return 0;
}
--
2.30.2
[PATCH] ASoC: meson: axg-frddr: set fifo depth according to the period
When the period is small, using all the FRDDR fifo depth increases the
latency of the playback because the following device won't start pulling
data until the fifo reaches the depth set. We can adjust this depth so trim
it down for small periods.
Signed-off-by: Jerome Brunet
---
sound/soc/meson/axg-frddr.c | 26 --
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/sound/soc/meson/axg-frddr.c b/sound/soc/meson/axg-frddr.c
index c3ae8ac30745..8ed114de0bdf 100644
--- a/sound/soc/meson/axg-frddr.c
+++ b/sound/soc/meson/axg-frddr.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
#include
@@ -46,11 +47,28 @@ static int g12a_frddr_dai_prepare(struct snd_pcm_substream
*substream,
return 0;
}
+static int axg_frddr_dai_hw_params(struct snd_pcm_substream *substream,
+ struct snd_pcm_hw_params *params,
+ struct snd_soc_dai *dai)
+{
+ struct axg_fifo *fifo = snd_soc_dai_get_drvdata(dai);
+ unsigned int period, depth, val;
+
+ period = params_period_bytes(params);
+
+ /* Trim the FIFO depth if the period is small to improve latency */
+ depth = min(period, fifo->depth);
+ val = (depth / AXG_FIFO_BURST) - 1;
+ regmap_update_bits(fifo->map, FIFO_CTRL1, CTRL1_FRDDR_DEPTH_MASK,
+ CTRL1_FRDDR_DEPTH(val));
+
+ return 0;
+}
+
static int axg_frddr_dai_startup(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)
{
struct axg_fifo *fifo = snd_soc_dai_get_drvdata(dai);
- unsigned int val;
int ret;
/* Enable pclk to access registers and clock the fifo ip */
@@ -61,11 +79,6 @@ static int axg_frddr_dai_startup(struct snd_pcm_substream
*substream,
/* Apply single buffer mode to the interface */
regmap_update_bits(fifo->map, FIFO_CTRL0, CTRL0_FRDDR_PP_MODE, 0);
- /* Use all fifo depth */
- val = (fifo->depth / AXG_FIFO_BURST) - 1;
- regmap_update_bits(fifo->map, FIFO_CTRL1, CTRL1_FRDDR_DEPTH_MASK,
- CTRL1_FRDDR_DEPTH(val));
-
return 0;
}
@@ -84,6 +97,7 @@ static int axg_frddr_pcm_new(struct snd_soc_pcm_runtime *rtd,
}
static const struct snd_soc_dai_ops axg_frddr_ops = {
+ .hw_params = axg_frddr_dai_hw_params,
.startup= axg_frddr_dai_startup,
.shutdown = axg_frddr_dai_shutdown,
};
--
2.30.2
[PATCH v3 4/4] usb: gadget: u_audio: clean up locking
snd_pcm_stream_lock() is held when the ALSA .trigger() callback is called.
The lock of 'struct uac_rtd_params' is not necessary since all its locking
operation are done under the snd_pcm_stream_lock() too.
Also, usb_request .complete() is called with irqs disabled, so saving and
restoring the irqs is not necessary.
Acked-by: Felipe Balbi
Signed-off-by: Jerome Brunet
---
drivers/usb/gadget/function/u_audio.c | 23 ---
1 file changed, 4 insertions(+), 19 deletions(-)
diff --git a/drivers/usb/gadget/function/u_audio.c
b/drivers/usb/gadget/function/u_audio.c
index a1a1f4c8685c..265c4d805f81 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -36,9 +36,8 @@ struct uac_rtd_params {
void *rbuf;
unsigned int max_psize; /* MaxPacketSize of endpoint */
- struct usb_request **reqs;
- spinlock_t lock;
+ struct usb_request **reqs;
};
struct snd_uac_chip {
@@ -74,7 +73,6 @@ static const struct snd_pcm_hardware uac_pcm_hardware = {
static void u_audio_iso_complete(struct usb_ep *ep, struct usb_request *req)
{
unsigned int pending;
- unsigned long flags, flags2;
unsigned int hw_ptr;
int status = req->status;
struct snd_pcm_substream *substream;
@@ -105,16 +103,14 @@ static void u_audio_iso_complete(struct usb_ep *ep,
struct usb_request *req)
if (!substream)
goto exit;
- snd_pcm_stream_lock_irqsave(substream, flags2);
+ snd_pcm_stream_lock(substream);
runtime = substream->runtime;
if (!runtime || !snd_pcm_running(substream)) {
- snd_pcm_stream_unlock_irqrestore(substream, flags2);
+ snd_pcm_stream_unlock(substream);
goto exit;
}
- spin_lock_irqsave(>lock, flags);
-
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
/*
* For each IN packet, take the quotient of the current data
@@ -141,8 +137,6 @@ static void u_audio_iso_complete(struct usb_ep *ep, struct
usb_request *req)
hw_ptr = prm->hw_ptr;
- spin_unlock_irqrestore(>lock, flags);
-
/* Pack USB load in ALSA ring buffer */
pending = runtime->dma_bytes - hw_ptr;
@@ -166,12 +160,10 @@ static void u_audio_iso_complete(struct usb_ep *ep,
struct usb_request *req)
}
}
- spin_lock_irqsave(>lock, flags);
/* update hw_ptr after data is copied to memory */
prm->hw_ptr = (hw_ptr + req->actual) % runtime->dma_bytes;
hw_ptr = prm->hw_ptr;
- spin_unlock_irqrestore(>lock, flags);
- snd_pcm_stream_unlock_irqrestore(substream, flags2);
+ snd_pcm_stream_unlock(substream);
if ((hw_ptr % snd_pcm_lib_period_bytes(substream)) < req->actual)
snd_pcm_period_elapsed(substream);
@@ -187,7 +179,6 @@ static int uac_pcm_trigger(struct snd_pcm_substream
*substream, int cmd)
struct uac_rtd_params *prm;
struct g_audio *audio_dev;
struct uac_params *params;
- unsigned long flags;
int err = 0;
audio_dev = uac->audio_dev;
@@ -198,8 +189,6 @@ static int uac_pcm_trigger(struct snd_pcm_substream
*substream, int cmd)
else
prm = >c_prm;
- spin_lock_irqsave(>lock, flags);
-
/* Reset */
prm->hw_ptr = 0;
@@ -216,8 +205,6 @@ static int uac_pcm_trigger(struct snd_pcm_substream
*substream, int cmd)
err = -EINVAL;
}
- spin_unlock_irqrestore(>lock, flags);
-
/* Clear buffer after Play stops */
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && !prm->ss)
memset(prm->rbuf, 0, prm->max_psize * params->req_number);
@@ -280,14 +267,12 @@ static int uac_pcm_open(struct snd_pcm_substream
*substream)
runtime->hw = uac_pcm_hardware;
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
- spin_lock_init(>p_prm.lock);
runtime->hw.rate_min = p_srate;
runtime->hw.formats = uac_ssize_to_fmt(p_ssize);
runtime->hw.channels_min = num_channels(p_chmask);
runtime->hw.period_bytes_min = 2 * uac->p_prm.max_psize
/ runtime->hw.periods_min;
} else {
- spin_lock_init(>c_prm.lock);
runtime->hw.rate_min = c_srate;
runtime->hw.formats = uac_ssize_to_fmt(c_ssize);
runtime->hw.channels_min = num_channels(c_chmask);
--
2.29.2
[PATCH v3 3/4] usb: gadget: u_audio: remove struct uac_req
'struct uac_req' purpose is to link 'struct usb_request' to the
corresponding 'struct uac_rtd_params'. However member req is never
used. Using the context of the usb request, we can keep track of the
corresponding 'struct uac_rtd_params' just as well, without allocating
extra memory.
Acked-by: Felipe Balbi
Signed-off-by: Jerome Brunet
---
drivers/usb/gadget/function/u_audio.c | 58 ---
1 file changed, 26 insertions(+), 32 deletions(-)
diff --git a/drivers/usb/gadget/function/u_audio.c
b/drivers/usb/gadget/function/u_audio.c
index 27f941f71a9d..a1a1f4c8685c 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -23,11 +23,6 @@
#define PRD_SIZE_MAX PAGE_SIZE
#define MIN_PERIODS4
-struct uac_req {
- struct uac_rtd_params *pp; /* parent param */
- struct usb_request *req;
-};
-
/* Runtime data params for one stream */
struct uac_rtd_params {
struct snd_uac_chip *uac; /* parent chip */
@@ -41,7 +36,7 @@ struct uac_rtd_params {
void *rbuf;
unsigned int max_psize; /* MaxPacketSize of endpoint */
- struct uac_req *ureq;
+ struct usb_request **reqs;
spinlock_t lock;
};
@@ -82,10 +77,9 @@ static void u_audio_iso_complete(struct usb_ep *ep, struct
usb_request *req)
unsigned long flags, flags2;
unsigned int hw_ptr;
int status = req->status;
- struct uac_req *ur = req->context;
struct snd_pcm_substream *substream;
struct snd_pcm_runtime *runtime;
- struct uac_rtd_params *prm = ur->pp;
+ struct uac_rtd_params *prm = req->context;
struct snd_uac_chip *uac = prm->uac;
/* i/f shutting down */
@@ -339,16 +333,16 @@ static inline void free_ep(struct uac_rtd_params *prm,
struct usb_ep *ep)
params = _dev->params;
for (i = 0; i < params->req_number; i++) {
- if (prm->ureq[i].req) {
- if (usb_ep_dequeue(ep, prm->ureq[i].req))
- usb_ep_free_request(ep, prm->ureq[i].req);
+ if (prm->reqs[i]) {
+ if (usb_ep_dequeue(ep, prm->reqs[i]))
+ usb_ep_free_request(ep, prm->reqs[i]);
/*
* If usb_ep_dequeue() cannot successfully dequeue the
* request, the request will be freed by the completion
* callback.
*/
- prm->ureq[i].req = NULL;
+ prm->reqs[i] = NULL;
}
}
@@ -377,22 +371,21 @@ int u_audio_start_capture(struct g_audio *audio_dev)
usb_ep_enable(ep);
for (i = 0; i < params->req_number; i++) {
- if (!prm->ureq[i].req) {
+ if (!prm->reqs[i]) {
req = usb_ep_alloc_request(ep, GFP_ATOMIC);
if (req == NULL)
return -ENOMEM;
- prm->ureq[i].req = req;
- prm->ureq[i].pp = prm;
+ prm->reqs[i] = req;
req->zero = 0;
- req->context = >ureq[i];
+ req->context = prm;
req->length = req_len;
req->complete = u_audio_iso_complete;
req->buf = prm->rbuf + i * ep->maxpacket;
}
- if (usb_ep_queue(ep, prm->ureq[i].req, GFP_ATOMIC))
+ if (usb_ep_queue(ep, prm->reqs[i], GFP_ATOMIC))
dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
}
@@ -455,22 +448,21 @@ int u_audio_start_playback(struct g_audio *audio_dev)
usb_ep_enable(ep);
for (i = 0; i < params->req_number; i++) {
- if (!prm->ureq[i].req) {
+ if (!prm->reqs[i]) {
req = usb_ep_alloc_request(ep, GFP_ATOMIC);
if (req == NULL)
return -ENOMEM;
- prm->ureq[i].req = req;
- prm->ureq[i].pp = prm;
+ prm->reqs[i] = req;
req->zero = 0;
- req->context = >ureq[i];
+ req->context = prm;
req->length = req_len;
req->complete = u_audio_iso_complete;
req->buf = prm->rbuf + i * ep->maxpacket;
}
- if (usb_ep_queue(ep, prm->ureq[i].req, GFP_ATOMIC))
+ if (usb_ep_queue(ep, prm->reqs[i], GFP_ATOMIC))
dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
}
@@ -515,9 +507,10 @@ int g_au
[PATCH v3 1/4] usb: gadget: u_audio: Free requests only after callback
From: Jack Pham
As per the kernel doc for usb_ep_dequeue(), it states that "this
routine is asynchronous, that is, it may return before the completion
routine runs". And indeed since v5.0 the dwc3 gadget driver updated
its behavior to place dequeued requests on to a cancelled list to be
given back later after the endpoint is stopped.
The free_ep() was incorrectly assuming that a request was ready to
be freed after calling dequeue which results in a use-after-free
in dwc3 when it traverses its cancelled list. Fix this by moving
the usb_ep_free_request() call to the callback itself in case the
ep is disabled.
Fixes: eb9fecb9e69b0 ("usb: gadget: f_uac2: split out audio core")
Reported-and-tested-by: Ferry Toth
Reviewed-and-tested-by: Peter Chen
Signed-off-by: Jack Pham
Signed-off-by: Jerome Brunet
Acked-by: Felipe Balbi
---
drivers/usb/gadget/function/u_audio.c | 17 ++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/gadget/function/u_audio.c
b/drivers/usb/gadget/function/u_audio.c
index e6d32c536781..908e49dafd62 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -89,7 +89,12 @@ static void u_audio_iso_complete(struct usb_ep *ep, struct
usb_request *req)
struct snd_uac_chip *uac = prm->uac;
/* i/f shutting down */
- if (!prm->ep_enabled || req->status == -ESHUTDOWN)
+ if (!prm->ep_enabled) {
+ usb_ep_free_request(ep, req);
+ return;
+ }
+
+ if (req->status == -ESHUTDOWN)
return;
/*
@@ -336,8 +341,14 @@ static inline void free_ep(struct uac_rtd_params *prm,
struct usb_ep *ep)
for (i = 0; i < params->req_number; i++) {
if (prm->ureq[i].req) {
- usb_ep_dequeue(ep, prm->ureq[i].req);
- usb_ep_free_request(ep, prm->ureq[i].req);
+ if (usb_ep_dequeue(ep, prm->ureq[i].req))
+ usb_ep_free_request(ep, prm->ureq[i].req);
+ /*
+* If usb_ep_dequeue() cannot successfully dequeue the
+* request, the request will be freed by the completion
+* callback.
+*/
+
prm->ureq[i].req = NULL;
}
}
--
2.29.2
[PATCH v3 0/4] usb: gadget: audio fixes and clean ups
This patchset is a collection of fixes and clean ups found while working on the uac2 gadget. Details are provided in each change. Changes since v2: [3] * Slightly amend comment on dequeue error as requested by Felipe * Drop applied patch Changes since v1: [1] * Jack's patch added to the series (no more deps) * Warning [2] on Patch 3 fixed [1]: https://lore.kernel.org/r/20201221173531.215169-1-jbru...@baylibre.com [2]: https://lore.kernel.org/r/202012291638.qidqi3gs-...@intel.com [3]: https://lore.kernel.org/r/20210106133652.512178-1-jbru...@baylibre.com Jack Pham (1): usb: gadget: u_audio: Free requests only after callback Jerome Brunet (3): usb: gadget: u_audio: factorize ssize to alsa fmt conversion usb: gadget: u_audio: remove struct uac_req usb: gadget: u_audio: clean up locking drivers/usb/gadget/function/u_audio.c | 135 -- 1 file changed, 62 insertions(+), 73 deletions(-) -- 2.29.2
[PATCH v3 2/4] usb: gadget: u_audio: factorize ssize to alsa fmt conversion
Factorize format related code common to the capture and playback path.
Acked-by: Felipe Balbi
Signed-off-by: Jerome Brunet
---
drivers/usb/gadget/function/u_audio.c | 43 +--
1 file changed, 21 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/gadget/function/u_audio.c
b/drivers/usb/gadget/function/u_audio.c
index 908e49dafd62..27f941f71a9d 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -244,6 +244,25 @@ static snd_pcm_uframes_t uac_pcm_pointer(struct
snd_pcm_substream *substream)
return bytes_to_frames(substream->runtime, prm->hw_ptr);
}
+static u64 uac_ssize_to_fmt(int ssize)
+{
+ u64 ret;
+
+ switch (ssize) {
+ case 3:
+ ret = SNDRV_PCM_FMTBIT_S24_3LE;
+ break;
+ case 4:
+ ret = SNDRV_PCM_FMTBIT_S32_LE;
+ break;
+ default:
+ ret = SNDRV_PCM_FMTBIT_S16_LE;
+ break;
+ }
+
+ return ret;
+}
+
static int uac_pcm_open(struct snd_pcm_substream *substream)
{
struct snd_uac_chip *uac = snd_pcm_substream_chip(substream);
@@ -269,34 +288,14 @@ static int uac_pcm_open(struct snd_pcm_substream
*substream)
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
spin_lock_init(>p_prm.lock);
runtime->hw.rate_min = p_srate;
- switch (p_ssize) {
- case 3:
- runtime->hw.formats = SNDRV_PCM_FMTBIT_S24_3LE;
- break;
- case 4:
- runtime->hw.formats = SNDRV_PCM_FMTBIT_S32_LE;
- break;
- default:
- runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
- break;
- }
+ runtime->hw.formats = uac_ssize_to_fmt(p_ssize);
runtime->hw.channels_min = num_channels(p_chmask);
runtime->hw.period_bytes_min = 2 * uac->p_prm.max_psize
/ runtime->hw.periods_min;
} else {
spin_lock_init(>c_prm.lock);
runtime->hw.rate_min = c_srate;
- switch (c_ssize) {
- case 3:
- runtime->hw.formats = SNDRV_PCM_FMTBIT_S24_3LE;
- break;
- case 4:
- runtime->hw.formats = SNDRV_PCM_FMTBIT_S32_LE;
- break;
- default:
- runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
- break;
- }
+ runtime->hw.formats = uac_ssize_to_fmt(c_ssize);
runtime->hw.channels_min = num_channels(c_chmask);
runtime->hw.period_bytes_min = 2 * uac->c_prm.max_psize
/ runtime->hw.periods_min;
--
2.29.2
[PATCH v2 5/5] usb: gadget: u_audio: clean up locking
snd_pcm_stream_lock() is held when the ALSA .trigger() callback is called.
The lock of 'struct uac_rtd_params' is not necessary since all its locking
operation are done under the snd_pcm_stream_lock() too.
Also, usb_request .complete() is called with irqs disabled, so saving and
restoring the irqs is not necessary.
Signed-off-by: Jerome Brunet
---
drivers/usb/gadget/function/u_audio.c | 23 ---
1 file changed, 4 insertions(+), 19 deletions(-)
diff --git a/drivers/usb/gadget/function/u_audio.c
b/drivers/usb/gadget/function/u_audio.c
index 1d12657b3b73..e985630fbe6e 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -36,9 +36,8 @@ struct uac_rtd_params {
void *rbuf;
unsigned int max_psize; /* MaxPacketSize of endpoint */
- struct usb_request **reqs;
- spinlock_t lock;
+ struct usb_request **reqs;
};
struct snd_uac_chip {
@@ -74,7 +73,6 @@ static const struct snd_pcm_hardware uac_pcm_hardware = {
static void u_audio_iso_complete(struct usb_ep *ep, struct usb_request *req)
{
unsigned int pending;
- unsigned long flags, flags2;
unsigned int hw_ptr;
int status = req->status;
struct snd_pcm_substream *substream;
@@ -105,16 +103,14 @@ static void u_audio_iso_complete(struct usb_ep *ep,
struct usb_request *req)
if (!substream)
goto exit;
- snd_pcm_stream_lock_irqsave(substream, flags2);
+ snd_pcm_stream_lock(substream);
runtime = substream->runtime;
if (!runtime || !snd_pcm_running(substream)) {
- snd_pcm_stream_unlock_irqrestore(substream, flags2);
+ snd_pcm_stream_unlock(substream);
goto exit;
}
- spin_lock_irqsave(>lock, flags);
-
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
/*
* For each IN packet, take the quotient of the current data
@@ -141,8 +137,6 @@ static void u_audio_iso_complete(struct usb_ep *ep, struct
usb_request *req)
hw_ptr = prm->hw_ptr;
- spin_unlock_irqrestore(>lock, flags);
-
/* Pack USB load in ALSA ring buffer */
pending = runtime->dma_bytes - hw_ptr;
@@ -166,12 +160,10 @@ static void u_audio_iso_complete(struct usb_ep *ep,
struct usb_request *req)
}
}
- spin_lock_irqsave(>lock, flags);
/* update hw_ptr after data is copied to memory */
prm->hw_ptr = (hw_ptr + req->actual) % runtime->dma_bytes;
hw_ptr = prm->hw_ptr;
- spin_unlock_irqrestore(>lock, flags);
- snd_pcm_stream_unlock_irqrestore(substream, flags2);
+ snd_pcm_stream_unlock(substream);
if ((hw_ptr % snd_pcm_lib_period_bytes(substream)) < req->actual)
snd_pcm_period_elapsed(substream);
@@ -187,7 +179,6 @@ static int uac_pcm_trigger(struct snd_pcm_substream
*substream, int cmd)
struct uac_rtd_params *prm;
struct g_audio *audio_dev;
struct uac_params *params;
- unsigned long flags;
int err = 0;
audio_dev = uac->audio_dev;
@@ -198,8 +189,6 @@ static int uac_pcm_trigger(struct snd_pcm_substream
*substream, int cmd)
else
prm = >c_prm;
- spin_lock_irqsave(>lock, flags);
-
/* Reset */
prm->hw_ptr = 0;
@@ -216,8 +205,6 @@ static int uac_pcm_trigger(struct snd_pcm_substream
*substream, int cmd)
err = -EINVAL;
}
- spin_unlock_irqrestore(>lock, flags);
-
/* Clear buffer after Play stops */
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && !prm->ss)
memset(prm->rbuf, 0, prm->max_psize * params->req_number);
@@ -280,14 +267,12 @@ static int uac_pcm_open(struct snd_pcm_substream
*substream)
runtime->hw = uac_pcm_hardware;
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
- spin_lock_init(>p_prm.lock);
runtime->hw.rate_min = p_srate;
runtime->hw.formats = uac_ssize_to_fmt(p_ssize);
runtime->hw.channels_min = num_channels(p_chmask);
runtime->hw.period_bytes_min = 2 * uac->p_prm.max_psize
/ runtime->hw.periods_min;
} else {
- spin_lock_init(>c_prm.lock);
runtime->hw.rate_min = c_srate;
runtime->hw.formats = uac_ssize_to_fmt(c_ssize);
runtime->hw.channels_min = num_channels(c_chmask);
--
2.29.2
[PATCH v2 4/5] usb: gadget: u_audio: remove struct uac_req
'struct uac_req' purpose is to link 'struct usb_request' to the
corresponding 'struct uac_rtd_params'. However member req is never
used. Using the context of the usb request, we can keep track of the
corresponding 'struct uac_rtd_params' just as well, without allocating
extra memory.
Signed-off-by: Jerome Brunet
---
drivers/usb/gadget/function/u_audio.c | 58 ---
1 file changed, 26 insertions(+), 32 deletions(-)
diff --git a/drivers/usb/gadget/function/u_audio.c
b/drivers/usb/gadget/function/u_audio.c
index 045f237472a7..1d12657b3b73 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -23,11 +23,6 @@
#define PRD_SIZE_MAX PAGE_SIZE
#define MIN_PERIODS4
-struct uac_req {
- struct uac_rtd_params *pp; /* parent param */
- struct usb_request *req;
-};
-
/* Runtime data params for one stream */
struct uac_rtd_params {
struct snd_uac_chip *uac; /* parent chip */
@@ -41,7 +36,7 @@ struct uac_rtd_params {
void *rbuf;
unsigned int max_psize; /* MaxPacketSize of endpoint */
- struct uac_req *ureq;
+ struct usb_request **reqs;
spinlock_t lock;
};
@@ -82,10 +77,9 @@ static void u_audio_iso_complete(struct usb_ep *ep, struct
usb_request *req)
unsigned long flags, flags2;
unsigned int hw_ptr;
int status = req->status;
- struct uac_req *ur = req->context;
struct snd_pcm_substream *substream;
struct snd_pcm_runtime *runtime;
- struct uac_rtd_params *prm = ur->pp;
+ struct uac_rtd_params *prm = req->context;
struct snd_uac_chip *uac = prm->uac;
/* i/f shutting down */
@@ -339,11 +333,11 @@ static inline void free_ep(struct uac_rtd_params *prm,
struct usb_ep *ep)
params = _dev->params;
for (i = 0; i < params->req_number; i++) {
- if (prm->ureq[i].req) {
- if (usb_ep_dequeue(ep, prm->ureq[i].req))
- usb_ep_free_request(ep, prm->ureq[i].req);
+ if (prm->reqs[i]) {
+ if (usb_ep_dequeue(ep, prm->reqs[i]))
+ usb_ep_free_request(ep, prm->reqs[i]);
/* else will be freed in u_audio_iso_complete() */
- prm->ureq[i].req = NULL;
+ prm->reqs[i] = NULL;
}
}
@@ -372,22 +366,21 @@ int u_audio_start_capture(struct g_audio *audio_dev)
usb_ep_enable(ep);
for (i = 0; i < params->req_number; i++) {
- if (!prm->ureq[i].req) {
+ if (!prm->reqs[i]) {
req = usb_ep_alloc_request(ep, GFP_ATOMIC);
if (req == NULL)
return -ENOMEM;
- prm->ureq[i].req = req;
- prm->ureq[i].pp = prm;
+ prm->reqs[i] = req;
req->zero = 0;
- req->context = >ureq[i];
+ req->context = prm;
req->length = req_len;
req->complete = u_audio_iso_complete;
req->buf = prm->rbuf + i * ep->maxpacket;
}
- if (usb_ep_queue(ep, prm->ureq[i].req, GFP_ATOMIC))
+ if (usb_ep_queue(ep, prm->reqs[i], GFP_ATOMIC))
dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
}
@@ -450,22 +443,21 @@ int u_audio_start_playback(struct g_audio *audio_dev)
usb_ep_enable(ep);
for (i = 0; i < params->req_number; i++) {
- if (!prm->ureq[i].req) {
+ if (!prm->reqs[i]) {
req = usb_ep_alloc_request(ep, GFP_ATOMIC);
if (req == NULL)
return -ENOMEM;
- prm->ureq[i].req = req;
- prm->ureq[i].pp = prm;
+ prm->reqs[i] = req;
req->zero = 0;
- req->context = >ureq[i];
+ req->context = prm;
req->length = req_len;
req->complete = u_audio_iso_complete;
req->buf = prm->rbuf + i * ep->maxpacket;
}
- if (usb_ep_queue(ep, prm->ureq[i].req, GFP_ATOMIC))
+ if (usb_ep_queue(ep, prm->reqs[i], GFP_ATOMIC))
dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
}
@@ -510,9 +502,10 @@ int g_audio_setup(struct g_audio *g_audio, const char
*pcm_name,
uac->c_prm.uac = uac;
prm->max_psize = g_audio->out_ep_maxpsize;
- prm->ureq
[PATCH v2 3/5] usb: gadget: u_audio: factorize ssize to alsa fmt conversion
Factorize format related code common to the capture and playback path.
Signed-off-by: Jerome Brunet
---
drivers/usb/gadget/function/u_audio.c | 43 +--
1 file changed, 21 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/gadget/function/u_audio.c
b/drivers/usb/gadget/function/u_audio.c
index 71dd9f16c246..045f237472a7 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -244,6 +244,25 @@ static snd_pcm_uframes_t uac_pcm_pointer(struct
snd_pcm_substream *substream)
return bytes_to_frames(substream->runtime, prm->hw_ptr);
}
+static u64 uac_ssize_to_fmt(int ssize)
+{
+ u64 ret;
+
+ switch (ssize) {
+ case 3:
+ ret = SNDRV_PCM_FMTBIT_S24_3LE;
+ break;
+ case 4:
+ ret = SNDRV_PCM_FMTBIT_S32_LE;
+ break;
+ default:
+ ret = SNDRV_PCM_FMTBIT_S16_LE;
+ break;
+ }
+
+ return ret;
+}
+
static int uac_pcm_open(struct snd_pcm_substream *substream)
{
struct snd_uac_chip *uac = snd_pcm_substream_chip(substream);
@@ -269,34 +288,14 @@ static int uac_pcm_open(struct snd_pcm_substream
*substream)
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
spin_lock_init(>p_prm.lock);
runtime->hw.rate_min = p_srate;
- switch (p_ssize) {
- case 3:
- runtime->hw.formats = SNDRV_PCM_FMTBIT_S24_3LE;
- break;
- case 4:
- runtime->hw.formats = SNDRV_PCM_FMTBIT_S32_LE;
- break;
- default:
- runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
- break;
- }
+ runtime->hw.formats = uac_ssize_to_fmt(p_ssize);
runtime->hw.channels_min = num_channels(p_chmask);
runtime->hw.period_bytes_min = 2 * uac->p_prm.max_psize
/ runtime->hw.periods_min;
} else {
spin_lock_init(>c_prm.lock);
runtime->hw.rate_min = c_srate;
- switch (c_ssize) {
- case 3:
- runtime->hw.formats = SNDRV_PCM_FMTBIT_S24_3LE;
- break;
- case 4:
- runtime->hw.formats = SNDRV_PCM_FMTBIT_S32_LE;
- break;
- default:
- runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
- break;
- }
+ runtime->hw.formats = uac_ssize_to_fmt(c_ssize);
runtime->hw.channels_min = num_channels(c_chmask);
runtime->hw.period_bytes_min = 2 * uac->c_prm.max_psize
/ runtime->hw.periods_min;
--
2.29.2
[PATCH v2 1/5] usb: gadget: u_audio: Free requests only after callback
From: Jack Pham
As per the kernel doc for usb_ep_dequeue(), it states that "this
routine is asynchronous, that is, it may return before the completion
routine runs". And indeed since v5.0 the dwc3 gadget driver updated
its behavior to place dequeued requests on to a cancelled list to be
given back later after the endpoint is stopped.
The free_ep() was incorrectly assuming that a request was ready to
be freed after calling dequeue which results in a use-after-free
in dwc3 when it traverses its cancelled list. Fix this by moving
the usb_ep_free_request() call to the callback itself in case the
ep is disabled.
Fixes: eb9fecb9e69b0 ("usb: gadget: f_uac2: split out audio core")
Reported-and-tested-by: Ferry Toth
Reviewed-and-tested-by: Peter Chen
Signed-off-by: Jack Pham
Signed-off-by: Jerome Brunet
---
drivers/usb/gadget/function/u_audio.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/gadget/function/u_audio.c
b/drivers/usb/gadget/function/u_audio.c
index e6d32c536781..71dd9f16c246 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -89,7 +89,12 @@ static void u_audio_iso_complete(struct usb_ep *ep, struct
usb_request *req)
struct snd_uac_chip *uac = prm->uac;
/* i/f shutting down */
- if (!prm->ep_enabled || req->status == -ESHUTDOWN)
+ if (!prm->ep_enabled) {
+ usb_ep_free_request(ep, req);
+ return;
+ }
+
+ if (req->status == -ESHUTDOWN)
return;
/*
@@ -336,8 +341,9 @@ static inline void free_ep(struct uac_rtd_params *prm,
struct usb_ep *ep)
for (i = 0; i < params->req_number; i++) {
if (prm->ureq[i].req) {
- usb_ep_dequeue(ep, prm->ureq[i].req);
- usb_ep_free_request(ep, prm->ureq[i].req);
+ if (usb_ep_dequeue(ep, prm->ureq[i].req))
+ usb_ep_free_request(ep, prm->ureq[i].req);
+ /* else will be freed in u_audio_iso_complete() */
prm->ureq[i].req = NULL;
}
}
--
2.29.2
[PATCH v2 2/5] usb: gadget: f_uac2: reset wMaxPacketSize
With commit 913e4a90b6f9 ("usb: gadget: f_uac2: finalize wMaxPacketSize
according to bandwidth")
wMaxPacketSize is computed dynamically but the value is never reset.
Because of this, the actual maximum packet size can only decrease each time
the audio gadget is instantiated.
Reset the endpoint maximum packet size and mark wMaxPacketSize as dynamic
to solve the problem.
Fixes: 913e4a90b6f9 ("usb: gadget: f_uac2: finalize wMaxPacketSize according to
bandwidth")
Signed-off-by: Jerome Brunet
---
drivers/usb/gadget/function/f_uac2.c | 69 ++--
1 file changed, 55 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 3633df6d7610..5d960b6603b6 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -271,7 +271,7 @@ static struct usb_endpoint_descriptor fs_epout_desc = {
.bEndpointAddress = USB_DIR_OUT,
.bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
- .wMaxPacketSize = cpu_to_le16(1023),
+ /* .wMaxPacketSize = DYNAMIC */
.bInterval = 1,
};
@@ -280,7 +280,7 @@ static struct usb_endpoint_descriptor hs_epout_desc = {
.bDescriptorType = USB_DT_ENDPOINT,
.bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
- .wMaxPacketSize = cpu_to_le16(1024),
+ /* .wMaxPacketSize = DYNAMIC */
.bInterval = 4,
};
@@ -348,7 +348,7 @@ static struct usb_endpoint_descriptor fs_epin_desc = {
.bEndpointAddress = USB_DIR_IN,
.bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
- .wMaxPacketSize = cpu_to_le16(1023),
+ /* .wMaxPacketSize = DYNAMIC */
.bInterval = 1,
};
@@ -357,7 +357,7 @@ static struct usb_endpoint_descriptor hs_epin_desc = {
.bDescriptorType = USB_DT_ENDPOINT,
.bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
- .wMaxPacketSize = cpu_to_le16(1024),
+ /* .wMaxPacketSize = DYNAMIC */
.bInterval = 4,
};
@@ -444,12 +444,28 @@ struct cntrl_range_lay3 {
__le32 dRES;
} __packed;
-static void set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
+static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
struct usb_endpoint_descriptor *ep_desc,
- unsigned int factor, bool is_playback)
+ enum usb_device_speed speed, bool is_playback)
{
int chmask, srate, ssize;
- u16 max_packet_size;
+ u16 max_size_bw, max_size_ep;
+ unsigned int factor;
+
+ switch (speed) {
+ case USB_SPEED_FULL:
+ max_size_ep = 1023;
+ factor = 1000;
+ break;
+
+ case USB_SPEED_HIGH:
+ max_size_ep = 1024;
+ factor = 8000;
+ break;
+
+ default:
+ return -EINVAL;
+ }
if (is_playback) {
chmask = uac2_opts->p_chmask;
@@ -461,10 +477,12 @@ static void set_ep_max_packet_size(const struct
f_uac2_opts *uac2_opts,
ssize = uac2_opts->c_ssize;
}
- max_packet_size = num_channels(chmask) * ssize *
+ max_size_bw = num_channels(chmask) * ssize *
DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
- ep_desc->wMaxPacketSize = cpu_to_le16(min_t(u16, max_packet_size,
- le16_to_cpu(ep_desc->wMaxPacketSize)));
+ ep_desc->wMaxPacketSize = cpu_to_le16(min_t(u16, max_size_bw,
+ max_size_ep));
+
+ return 0;
}
/* Use macro to overcome line length limitation */
@@ -670,10 +688,33 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
}
/* Calculate wMaxPacketSize according to audio bandwidth */
- set_ep_max_packet_size(uac2_opts, _epin_desc, 1000, true);
- set_ep_max_packet_size(uac2_opts, _epout_desc, 1000, false);
- set_ep_max_packet_size(uac2_opts, _epin_desc, 8000, true);
- set_ep_max_packet_size(uac2_opts, _epout_desc, 8000, false);
+ ret = set_ep_max_packet_size(uac2_opts, _epin_desc, USB_SPEED_FULL,
+true);
+ if (ret < 0) {
+ dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
+ return ret;
+ }
+
+ ret = set_ep_max_packet_size(uac2_opts, _epout_desc, USB_SPEED_FULL,
+false);
+ if (ret < 0) {
+ dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
+ return ret;
+ }
+
+ ret = set_ep_max_packet_size(uac2_opts, _epin_desc, USB_SPEED_HIGH,
+true);
+ if (ret < 0) {
+ dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
+ return ret;
+
[PATCH v2 0/5] usb: gadget: audio fixes and clean ups
This patchset is a collection of fixes and clean ups found while working on the uac2 gadget. Details are provided in each change. Changes since v1: [1] * Jack's patch added to the series (no more deps) * Warning [2] on Patch 3 fixed [1]: https://lore.kernel.org/r/20201221173531.215169-1-jbru...@baylibre.com [2]: https://lore.kernel.org/r/202012291638.qidqi3gs-...@intel.com Jack Pham (1): usb: gadget: u_audio: Free requests only after callback Jerome Brunet (4): usb: gadget: f_uac2: reset wMaxPacketSize usb: gadget: u_audio: factorize ssize to alsa fmt conversion usb: gadget: u_audio: remove struct uac_req usb: gadget: u_audio: clean up locking drivers/usb/gadget/function/f_uac2.c | 69 +++--- drivers/usb/gadget/function/u_audio.c | 130 +++--- 2 files changed, 112 insertions(+), 87 deletions(-) -- 2.29.2
Re: [PATCH 3/4] usb: gadget: u_audio: remove struct uac_req
On Tue 29 Dec 2020 at 23:30, Jack Pham wrote: > Hi Greg and Jerome, > > On Mon, Dec 28, 2020 at 04:01:46PM +0100, Greg Kroah-Hartman wrote: >> On Mon, Dec 21, 2020 at 06:35:30PM +0100, Jerome Brunet wrote: >> > 'struct uac_req' purpose is to link 'struct usb_request' to the >> > corresponding 'struct uac_rtd_params'. However member req is never >> > used. Using the context of the usb request, we can keep track of the >> > corresponding 'struct uac_rtd_params' just as well, without allocating >> > extra memory. >> > >> > Signed-off-by: Jerome Brunet >> > --- >> > drivers/usb/gadget/function/u_audio.c | 58 --- >> > 1 file changed, 26 insertions(+), 32 deletions(-) >> >> This patch doesn't apply, so I can't apply patches 3 or 4 of this series >> :( >> >> Can you rebase against my usb-testing branch and resend? > > From the cover letter: > > On Mon, Dec 21, 2020 at 06:35:27PM +0100, Jerome Brunet wrote: >> The series depends on this fix [0] by Jack Pham to apply cleanly >> >> [0]: >> https://lore.kernel.org/linux-usb/20201029175949.6052-1-ja...@codeaurora.org/ > > My patch hadn't been picked up by Felipe, so it's not in your tree > either, Greg. Should I just resend it to you first? Or shall I invite > Jerome to just include it in v2 of this series? Indeed. I rebased on usb-testing and the series applies cleanly with Jack's changes, as decribed in the cover-letter. If it is easier, I'm happy to include Jack's change in the v2, along with the fixed PATCH 2 fixed. Greg, would it be OK with you ? > > Thanks, > Jack
Re: [PATCH 0/2] clk: meson8b: cleanup unused code
On Mon 21 Dec 2020 at 19:36, Martin Blumenstingl wrote: > Hi Jerome, > > these patches are two small cleanups for code we don't need anymore. > The first patch removes support for old .dtbs. I am not sure if the > "fallback" logic still works as I have not tried this in a long time. > > > Martin Blumenstingl (2): > clk: meson: meson8b: remove compatibility code for old .dtbs > dt-bindings: clock: meson8b: remove non-existing clock macros > > drivers/clk/meson/meson8b.c | 45 +++- > include/dt-bindings/clock/meson8b-clkc.h | 2 -- > 2 files changed, 5 insertions(+), 42 deletions(-) Applied, Thx
Re: [PATCH 0/3] clk: meson: three small clk-pll fixes
On Sat 26 Dec 2020 at 13:15, Martin Blumenstingl wrote: > Hi Jerome, > > while working on some changes for the 32-bit SoCs I hit a corner-case > in the HDMI PLL: there's some rate doubling going. The PLL only locks > in a specific rate range but the M/N table is not aware of that. This > means for now (I am planning to fix that) that we can end up in a > ituation where the PLL doesn't lock and meson_clk_pll_set_rate() is > supposed to still behave in this case. So here's three small patches > for that. > > For me it's fine to queue these patches for -next. I am not aware of > any breakage upstream, only some of my pending patches prefer to have > these fixes. > > Slightly unrelated: if you know anything about that clock doubling then > please let me know! > > > Best regards, > Martin > > > Martin Blumenstingl (3): > clk: meson: clk-pll: fix initializing the old rate (fallback) for a > PLL > clk: meson: clk-pll: make "ret" a signed integer > clk: meson: clk-pll: propagate the error from meson_clk_pll_set_rate() > > drivers/clk/meson/clk-pll.c | 10 ++ > 1 file changed, 6 insertions(+), 4 deletions(-) Applied, Thx
Re: [PATCH 1/3] clk: meson: clk-pll: fix initializing the old rate (fallback) for a PLL
On Sat 26 Dec 2020 at 13:15, Martin Blumenstingl
wrote:
> The "rate" parameter in meson_clk_pll_set_rate() contains the new rate.
> Retrieve the old rate with clk_hw_get_rate() so we don't inifinitely try
> to switch from the new rate to the same ratte again.
Small typo above fixed while applying
>
> Fixes: 7a29a869434e8b ("clk: meson: Add support for Meson clock controller")
> Signed-off-by: Martin Blumenstingl
> ---
> drivers/clk/meson/clk-pll.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> index b17a13e9337c..9404609b5ebf 100644
> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
> @@ -371,7 +371,7 @@ static int meson_clk_pll_set_rate(struct clk_hw *hw,
> unsigned long rate,
> if (parent_rate == 0 || rate == 0)
> return -EINVAL;
>
> - old_rate = rate;
> + old_rate = clk_hw_get_rate(hw);
>
> ret = meson_clk_get_pll_settings(rate, parent_rate, , , pll);
> if (ret)
Re: [PATCH 0/2] clk: meson: axg: Remove MIPI enable clock gate
On Tue 10 Mar 2020 at 09:05, Jerome Brunet wrote: > On Mon 09 Mar 2020 at 22:01, Remi Pommarel wrote: > >> As discussed here [0], HHI_MIPI_CNTL0 is part of the MIPI/PCIe analog >> PHY region and is not related to clock one. Since MIPI/PCIe PHY driver >> has been added with [1], this region can be removed from the clock >> driver. >> >> Please not that this serie depends on [1] to be merged first. >> >> [0] https://lkml.org/lkml/2019/12/16/119 >> [1] https://lkml.org/lkml/2020/1/23/945 > > Series look good. Will apply after v5.7-rc1 > Finally applied ... sorry for the delay >> >> Remi Pommarel (2): >> clk: meson: axg: Remove MIPI enable clock gate >> clk: meson-axg: remove CLKID_MIPI_ENABLE >> >> drivers/clk/meson/axg.c | 3 --- >> drivers/clk/meson/axg.h | 1 - >> include/dt-bindings/clock/axg-clkc.h | 1 - >> 3 files changed, 5 deletions(-) > > > ___ > linux-amlogic mailing list > linux-amlo...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-amlogic
[PATCH 2/4] usb: gadget: u_audio: factorize ssize to alsa fmt conversion
Factorize format related code common to the capture and playback path.
Signed-off-by: Jerome Brunet
---
drivers/usb/gadget/function/u_audio.c | 43 +--
1 file changed, 21 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/gadget/function/u_audio.c
b/drivers/usb/gadget/function/u_audio.c
index 71dd9f16c246..2f69bd572ed7 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -244,6 +244,25 @@ static snd_pcm_uframes_t uac_pcm_pointer(struct
snd_pcm_substream *substream)
return bytes_to_frames(substream->runtime, prm->hw_ptr);
}
+static unsigned long uac_ssize_to_fmt(int ssize)
+{
+ unsigned long ret;
+
+ switch (ssize) {
+ case 3:
+ ret = SNDRV_PCM_FMTBIT_S24_3LE;
+ break;
+ case 4:
+ ret = SNDRV_PCM_FMTBIT_S32_LE;
+ break;
+ default:
+ ret = SNDRV_PCM_FMTBIT_S16_LE;
+ break;
+ }
+
+ return ret;
+}
+
static int uac_pcm_open(struct snd_pcm_substream *substream)
{
struct snd_uac_chip *uac = snd_pcm_substream_chip(substream);
@@ -269,34 +288,14 @@ static int uac_pcm_open(struct snd_pcm_substream
*substream)
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
spin_lock_init(>p_prm.lock);
runtime->hw.rate_min = p_srate;
- switch (p_ssize) {
- case 3:
- runtime->hw.formats = SNDRV_PCM_FMTBIT_S24_3LE;
- break;
- case 4:
- runtime->hw.formats = SNDRV_PCM_FMTBIT_S32_LE;
- break;
- default:
- runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
- break;
- }
+ runtime->hw.formats = uac_ssize_to_fmt(p_ssize);
runtime->hw.channels_min = num_channels(p_chmask);
runtime->hw.period_bytes_min = 2 * uac->p_prm.max_psize
/ runtime->hw.periods_min;
} else {
spin_lock_init(>c_prm.lock);
runtime->hw.rate_min = c_srate;
- switch (c_ssize) {
- case 3:
- runtime->hw.formats = SNDRV_PCM_FMTBIT_S24_3LE;
- break;
- case 4:
- runtime->hw.formats = SNDRV_PCM_FMTBIT_S32_LE;
- break;
- default:
- runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
- break;
- }
+ runtime->hw.formats = uac_ssize_to_fmt(c_ssize);
runtime->hw.channels_min = num_channels(c_chmask);
runtime->hw.period_bytes_min = 2 * uac->c_prm.max_psize
/ runtime->hw.periods_min;
--
2.29.2
[PATCH 1/4] usb: gadget: f_uac2: reset wMaxPacketSize
With commit 913e4a90b6f9 ("usb: gadget: f_uac2: finalize wMaxPacketSize
according to bandwidth")
wMaxPacketSize is computed dynamically but the value is never reset.
Because of this, the actual maximum packet size can only decrease each time
the audio gadget is instantiated.
Reset the endpoint maximum packet size and mark wMaxPacketSize as dynamic
to solve the problem.
Fixes: 913e4a90b6f9 ("usb: gadget: f_uac2: finalize wMaxPacketSize according to
bandwidth")
Signed-off-by: Jerome Brunet
---
drivers/usb/gadget/function/f_uac2.c | 69 ++--
1 file changed, 55 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/gadget/function/f_uac2.c
b/drivers/usb/gadget/function/f_uac2.c
index 3633df6d7610..5d960b6603b6 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -271,7 +271,7 @@ static struct usb_endpoint_descriptor fs_epout_desc = {
.bEndpointAddress = USB_DIR_OUT,
.bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
- .wMaxPacketSize = cpu_to_le16(1023),
+ /* .wMaxPacketSize = DYNAMIC */
.bInterval = 1,
};
@@ -280,7 +280,7 @@ static struct usb_endpoint_descriptor hs_epout_desc = {
.bDescriptorType = USB_DT_ENDPOINT,
.bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
- .wMaxPacketSize = cpu_to_le16(1024),
+ /* .wMaxPacketSize = DYNAMIC */
.bInterval = 4,
};
@@ -348,7 +348,7 @@ static struct usb_endpoint_descriptor fs_epin_desc = {
.bEndpointAddress = USB_DIR_IN,
.bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
- .wMaxPacketSize = cpu_to_le16(1023),
+ /* .wMaxPacketSize = DYNAMIC */
.bInterval = 1,
};
@@ -357,7 +357,7 @@ static struct usb_endpoint_descriptor hs_epin_desc = {
.bDescriptorType = USB_DT_ENDPOINT,
.bmAttributes = USB_ENDPOINT_XFER_ISOC | USB_ENDPOINT_SYNC_ASYNC,
- .wMaxPacketSize = cpu_to_le16(1024),
+ /* .wMaxPacketSize = DYNAMIC */
.bInterval = 4,
};
@@ -444,12 +444,28 @@ struct cntrl_range_lay3 {
__le32 dRES;
} __packed;
-static void set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
+static int set_ep_max_packet_size(const struct f_uac2_opts *uac2_opts,
struct usb_endpoint_descriptor *ep_desc,
- unsigned int factor, bool is_playback)
+ enum usb_device_speed speed, bool is_playback)
{
int chmask, srate, ssize;
- u16 max_packet_size;
+ u16 max_size_bw, max_size_ep;
+ unsigned int factor;
+
+ switch (speed) {
+ case USB_SPEED_FULL:
+ max_size_ep = 1023;
+ factor = 1000;
+ break;
+
+ case USB_SPEED_HIGH:
+ max_size_ep = 1024;
+ factor = 8000;
+ break;
+
+ default:
+ return -EINVAL;
+ }
if (is_playback) {
chmask = uac2_opts->p_chmask;
@@ -461,10 +477,12 @@ static void set_ep_max_packet_size(const struct
f_uac2_opts *uac2_opts,
ssize = uac2_opts->c_ssize;
}
- max_packet_size = num_channels(chmask) * ssize *
+ max_size_bw = num_channels(chmask) * ssize *
DIV_ROUND_UP(srate, factor / (1 << (ep_desc->bInterval - 1)));
- ep_desc->wMaxPacketSize = cpu_to_le16(min_t(u16, max_packet_size,
- le16_to_cpu(ep_desc->wMaxPacketSize)));
+ ep_desc->wMaxPacketSize = cpu_to_le16(min_t(u16, max_size_bw,
+ max_size_ep));
+
+ return 0;
}
/* Use macro to overcome line length limitation */
@@ -670,10 +688,33 @@ afunc_bind(struct usb_configuration *cfg, struct
usb_function *fn)
}
/* Calculate wMaxPacketSize according to audio bandwidth */
- set_ep_max_packet_size(uac2_opts, _epin_desc, 1000, true);
- set_ep_max_packet_size(uac2_opts, _epout_desc, 1000, false);
- set_ep_max_packet_size(uac2_opts, _epin_desc, 8000, true);
- set_ep_max_packet_size(uac2_opts, _epout_desc, 8000, false);
+ ret = set_ep_max_packet_size(uac2_opts, _epin_desc, USB_SPEED_FULL,
+true);
+ if (ret < 0) {
+ dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
+ return ret;
+ }
+
+ ret = set_ep_max_packet_size(uac2_opts, _epout_desc, USB_SPEED_FULL,
+false);
+ if (ret < 0) {
+ dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
+ return ret;
+ }
+
+ ret = set_ep_max_packet_size(uac2_opts, _epin_desc, USB_SPEED_HIGH,
+true);
+ if (ret < 0) {
+ dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
+ return ret;
+
[PATCH 4/4] usb: gadget: u_audio: clean up locking
snd_pcm_stream_lock() is held when the ALSA .trigger() callback is called.
The lock of 'struct uac_rtd_params' is not necessary since all its locking
operation are done under the snd_pcm_stream_lock() too.
Also, usb_request .complete() is called with irqs disabled, so saving and
restoring the irqs is not necessary.
Signed-off-by: Jerome Brunet
---
drivers/usb/gadget/function/u_audio.c | 23 ---
1 file changed, 4 insertions(+), 19 deletions(-)
diff --git a/drivers/usb/gadget/function/u_audio.c
b/drivers/usb/gadget/function/u_audio.c
index 3eba31b8ebcb..d94f95edca40 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -36,9 +36,8 @@ struct uac_rtd_params {
void *rbuf;
unsigned int max_psize; /* MaxPacketSize of endpoint */
- struct usb_request **reqs;
- spinlock_t lock;
+ struct usb_request **reqs;
};
struct snd_uac_chip {
@@ -74,7 +73,6 @@ static const struct snd_pcm_hardware uac_pcm_hardware = {
static void u_audio_iso_complete(struct usb_ep *ep, struct usb_request *req)
{
unsigned int pending;
- unsigned long flags, flags2;
unsigned int hw_ptr;
int status = req->status;
struct snd_pcm_substream *substream;
@@ -105,16 +103,14 @@ static void u_audio_iso_complete(struct usb_ep *ep,
struct usb_request *req)
if (!substream)
goto exit;
- snd_pcm_stream_lock_irqsave(substream, flags2);
+ snd_pcm_stream_lock(substream);
runtime = substream->runtime;
if (!runtime || !snd_pcm_running(substream)) {
- snd_pcm_stream_unlock_irqrestore(substream, flags2);
+ snd_pcm_stream_unlock(substream);
goto exit;
}
- spin_lock_irqsave(>lock, flags);
-
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
/*
* For each IN packet, take the quotient of the current data
@@ -141,8 +137,6 @@ static void u_audio_iso_complete(struct usb_ep *ep, struct
usb_request *req)
hw_ptr = prm->hw_ptr;
- spin_unlock_irqrestore(>lock, flags);
-
/* Pack USB load in ALSA ring buffer */
pending = runtime->dma_bytes - hw_ptr;
@@ -166,12 +160,10 @@ static void u_audio_iso_complete(struct usb_ep *ep,
struct usb_request *req)
}
}
- spin_lock_irqsave(>lock, flags);
/* update hw_ptr after data is copied to memory */
prm->hw_ptr = (hw_ptr + req->actual) % runtime->dma_bytes;
hw_ptr = prm->hw_ptr;
- spin_unlock_irqrestore(>lock, flags);
- snd_pcm_stream_unlock_irqrestore(substream, flags2);
+ snd_pcm_stream_unlock(substream);
if ((hw_ptr % snd_pcm_lib_period_bytes(substream)) < req->actual)
snd_pcm_period_elapsed(substream);
@@ -187,7 +179,6 @@ static int uac_pcm_trigger(struct snd_pcm_substream
*substream, int cmd)
struct uac_rtd_params *prm;
struct g_audio *audio_dev;
struct uac_params *params;
- unsigned long flags;
int err = 0;
audio_dev = uac->audio_dev;
@@ -198,8 +189,6 @@ static int uac_pcm_trigger(struct snd_pcm_substream
*substream, int cmd)
else
prm = >c_prm;
- spin_lock_irqsave(>lock, flags);
-
/* Reset */
prm->hw_ptr = 0;
@@ -216,8 +205,6 @@ static int uac_pcm_trigger(struct snd_pcm_substream
*substream, int cmd)
err = -EINVAL;
}
- spin_unlock_irqrestore(>lock, flags);
-
/* Clear buffer after Play stops */
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK && !prm->ss)
memset(prm->rbuf, 0, prm->max_psize * params->req_number);
@@ -280,14 +267,12 @@ static int uac_pcm_open(struct snd_pcm_substream
*substream)
runtime->hw = uac_pcm_hardware;
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
- spin_lock_init(>p_prm.lock);
runtime->hw.rate_min = p_srate;
runtime->hw.formats = uac_ssize_to_fmt(p_ssize);
runtime->hw.channels_min = num_channels(p_chmask);
runtime->hw.period_bytes_min = 2 * uac->p_prm.max_psize
/ runtime->hw.periods_min;
} else {
- spin_lock_init(>c_prm.lock);
runtime->hw.rate_min = c_srate;
runtime->hw.formats = uac_ssize_to_fmt(c_ssize);
runtime->hw.channels_min = num_channels(c_chmask);
--
2.29.2
[PATCH 0/4] usb: gadget: audio fixes and clean ups
This patchset is a collection of fixes and clean ups found while working on the uac2 gadget. Details are provided in each change. The series depends on this fix [0] by Jack Pham to apply cleanly [0]: https://lore.kernel.org/linux-usb/20201029175949.6052-1-ja...@codeaurora.org/ Jerome Brunet (4): usb: gadget: f_uac2: reset wMaxPacketSize usb: gadget: u_audio: factorize ssize to alsa fmt conversion usb: gadget: u_audio: remove struct uac_req usb: gadget: u_audio: clean up locking drivers/usb/gadget/function/f_uac2.c | 69 --- drivers/usb/gadget/function/u_audio.c | 122 +++--- 2 files changed, 105 insertions(+), 86 deletions(-) -- 2.29.2
[PATCH 3/4] usb: gadget: u_audio: remove struct uac_req
'struct uac_req' purpose is to link 'struct usb_request' to the
corresponding 'struct uac_rtd_params'. However member req is never
used. Using the context of the usb request, we can keep track of the
corresponding 'struct uac_rtd_params' just as well, without allocating
extra memory.
Signed-off-by: Jerome Brunet
---
drivers/usb/gadget/function/u_audio.c | 58 ---
1 file changed, 26 insertions(+), 32 deletions(-)
diff --git a/drivers/usb/gadget/function/u_audio.c
b/drivers/usb/gadget/function/u_audio.c
index 2f69bd572ed7..3eba31b8ebcb 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -23,11 +23,6 @@
#define PRD_SIZE_MAX PAGE_SIZE
#define MIN_PERIODS4
-struct uac_req {
- struct uac_rtd_params *pp; /* parent param */
- struct usb_request *req;
-};
-
/* Runtime data params for one stream */
struct uac_rtd_params {
struct snd_uac_chip *uac; /* parent chip */
@@ -41,7 +36,7 @@ struct uac_rtd_params {
void *rbuf;
unsigned int max_psize; /* MaxPacketSize of endpoint */
- struct uac_req *ureq;
+ struct usb_request **reqs;
spinlock_t lock;
};
@@ -82,10 +77,9 @@ static void u_audio_iso_complete(struct usb_ep *ep, struct
usb_request *req)
unsigned long flags, flags2;
unsigned int hw_ptr;
int status = req->status;
- struct uac_req *ur = req->context;
struct snd_pcm_substream *substream;
struct snd_pcm_runtime *runtime;
- struct uac_rtd_params *prm = ur->pp;
+ struct uac_rtd_params *prm = req->context;
struct snd_uac_chip *uac = prm->uac;
/* i/f shutting down */
@@ -339,11 +333,11 @@ static inline void free_ep(struct uac_rtd_params *prm,
struct usb_ep *ep)
params = _dev->params;
for (i = 0; i < params->req_number; i++) {
- if (prm->ureq[i].req) {
- if (usb_ep_dequeue(ep, prm->ureq[i].req))
- usb_ep_free_request(ep, prm->ureq[i].req);
+ if (prm->reqs[i]) {
+ if (usb_ep_dequeue(ep, prm->reqs[i]))
+ usb_ep_free_request(ep, prm->reqs[i]);
/* else will be freed in u_audio_iso_complete() */
- prm->ureq[i].req = NULL;
+ prm->reqs[i] = NULL;
}
}
@@ -372,22 +366,21 @@ int u_audio_start_capture(struct g_audio *audio_dev)
usb_ep_enable(ep);
for (i = 0; i < params->req_number; i++) {
- if (!prm->ureq[i].req) {
+ if (!prm->reqs[i]) {
req = usb_ep_alloc_request(ep, GFP_ATOMIC);
if (req == NULL)
return -ENOMEM;
- prm->ureq[i].req = req;
- prm->ureq[i].pp = prm;
+ prm->reqs[i] = req;
req->zero = 0;
- req->context = >ureq[i];
+ req->context = prm;
req->length = req_len;
req->complete = u_audio_iso_complete;
req->buf = prm->rbuf + i * ep->maxpacket;
}
- if (usb_ep_queue(ep, prm->ureq[i].req, GFP_ATOMIC))
+ if (usb_ep_queue(ep, prm->reqs[i], GFP_ATOMIC))
dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
}
@@ -450,22 +443,21 @@ int u_audio_start_playback(struct g_audio *audio_dev)
usb_ep_enable(ep);
for (i = 0; i < params->req_number; i++) {
- if (!prm->ureq[i].req) {
+ if (!prm->reqs[i]) {
req = usb_ep_alloc_request(ep, GFP_ATOMIC);
if (req == NULL)
return -ENOMEM;
- prm->ureq[i].req = req;
- prm->ureq[i].pp = prm;
+ prm->reqs[i] = req;
req->zero = 0;
- req->context = >ureq[i];
+ req->context = prm;
req->length = req_len;
req->complete = u_audio_iso_complete;
req->buf = prm->rbuf + i * ep->maxpacket;
}
- if (usb_ep_queue(ep, prm->ureq[i].req, GFP_ATOMIC))
+ if (usb_ep_queue(ep, prm->reqs[i], GFP_ATOMIC))
dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
}
@@ -510,9 +502,10 @@ int g_audio_setup(struct g_audio *g_audio, const char
*pcm_name,
uac->c_prm.uac = uac;
prm->max_psize = g_audio->out_ep_maxpsize;
- prm->ureq
[PATCH] ASoC: meson: axg-tdm-interface: fix loopback
When the axg-tdm-interface was introduced, the backend DAI was marked as an
endpoint when DPCM was walking the DAPM graph to find a its BE.
It is no longer the case since this
commit 8dd26dff00c0 ("ASoC: dapm: Fix handling of custom_stop_condition on DAPM
graph walks")
Because of this, when DPCM finds a BE it does everything it needs on the
DAIs but it won't power up the widgets between the FE and the BE if there
is no actual endpoint after the BE.
On meson-axg HWs, the loopback is a special DAI of the tdm-interface BE.
It is only linked to the dummy codec since there no actual HW after it.
>From the DAPM perspective, the DAI has no endpoint. Because of this, the TDM
decoder, which is a widget between the FE and BE is not powered up.
>From the user perspective, everything seems fine but no data is produced.
Connecting the Loopback DAI to a dummy DAPM endpoint solves the problem.
Fixes: 8dd26dff00c0 ("ASoC: dapm: Fix handling of custom_stop_condition on DAPM
graph walks")
Cc: Charles Keepax
Signed-off-by: Jerome Brunet
---
sound/soc/meson/axg-tdm-interface.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/sound/soc/meson/axg-tdm-interface.c
b/sound/soc/meson/axg-tdm-interface.c
index c8664ab80d45..87cac440b369 100644
--- a/sound/soc/meson/axg-tdm-interface.c
+++ b/sound/soc/meson/axg-tdm-interface.c
@@ -467,8 +467,20 @@ static int axg_tdm_iface_set_bias_level(struct
snd_soc_component *component,
return ret;
}
+static const struct snd_soc_dapm_widget axg_tdm_iface_dapm_widgets[] = {
+ SND_SOC_DAPM_SIGGEN("Playback Signal"),
+};
+
+static const struct snd_soc_dapm_route axg_tdm_iface_dapm_routes[] = {
+ { "Loopback", NULL, "Playback Signal" },
+};
+
static const struct snd_soc_component_driver axg_tdm_iface_component_drv = {
- .set_bias_level = axg_tdm_iface_set_bias_level,
+ .dapm_widgets = axg_tdm_iface_dapm_widgets,
+ .num_dapm_widgets = ARRAY_SIZE(axg_tdm_iface_dapm_widgets),
+ .dapm_routes= axg_tdm_iface_dapm_routes,
+ .num_dapm_routes= ARRAY_SIZE(axg_tdm_iface_dapm_routes),
+ .set_bias_level = axg_tdm_iface_set_bias_level,
};
static const struct of_device_id axg_tdm_iface_of_match[] = {
--
2.29.2
[PATCH] ASoC: meson: axg-tdmin: fix axg skew offset
The signal captured on from tdm decoder of the AXG SoC is incorrect. It
appears amplified. The skew offset of the decoder is wrong.
Setting the skew offset to 3, like the g12 and sm1 SoCs, solves and gives
correct data.
Fixes: 13a22e6a98f8 ("ASoC: meson: add tdm input driver")
Signed-off-by: Jerome Brunet
---
sound/soc/meson/axg-tdmin.c | 13 ++---
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/sound/soc/meson/axg-tdmin.c b/sound/soc/meson/axg-tdmin.c
index 88ed95ae886b..b4faf9d5c1aa 100644
--- a/sound/soc/meson/axg-tdmin.c
+++ b/sound/soc/meson/axg-tdmin.c
@@ -224,15 +224,6 @@ static const struct axg_tdm_formatter_ops axg_tdmin_ops = {
};
static const struct axg_tdm_formatter_driver axg_tdmin_drv = {
- .component_drv = _tdmin_component_drv,
- .regmap_cfg = _tdmin_regmap_cfg,
- .ops= _tdmin_ops,
- .quirks = &(const struct axg_tdm_formatter_hw) {
- .skew_offset= 2,
- },
-};
-
-static const struct axg_tdm_formatter_driver g12a_tdmin_drv = {
.component_drv = _tdmin_component_drv,
.regmap_cfg = _tdmin_regmap_cfg,
.ops= _tdmin_ops,
@@ -247,10 +238,10 @@ static const struct of_device_id axg_tdmin_of_match[] = {
.data = _tdmin_drv,
}, {
.compatible = "amlogic,g12a-tdmin",
- .data = _tdmin_drv,
+ .data = _tdmin_drv,
}, {
.compatible = "amlogic,sm1-tdmin",
- .data = _tdmin_drv,
+ .data = _tdmin_drv,
}, {}
};
MODULE_DEVICE_TABLE(of, axg_tdmin_of_match);
--
2.29.2
Re: [PATCH 2/2] arm64: dts: meson: vim3: enable hdmi audio loopback
On Mon 07 Dec 2020 at 10:53, Jerome Brunet wrote:
> Enable audio capture frontends and a tdm decoder.
> This makes it possible to loopback the audio played on the hdmi codec,
> which is the only output interface at the moment.
>
> Of course, one TODDR device would be enough to do that but since
> the 3 FRDDRs are enabled on the playback side, let's do the same on the
> capture side.
>
> Signed-off-by: Jerome Brunet
> ---
> .../boot/dts/amlogic/meson-khadas-vim3.dtsi | 41 +--
> 1 file changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-khadas-vim3.dtsi
> b/arch/arm64/boot/dts/amlogic/meson-khadas-vim3.dtsi
> index 12465c4becc7..4cf2c193d168 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-khadas-vim3.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-khadas-vim3.dtsi
> @@ -166,11 +166,16 @@ hdmi_connector_in: endpoint {
> sound {
> compatible = "amlogic,axg-sound-card";
> model = "G12B-KHADAS-VIM3";
> - audio-aux-devs = <_a>;
> + audio-aux-devs = <_a>, <_a>;
> audio-routing = "TDMOUT_A IN 0", "FRDDR_A OUT 0",
> "TDMOUT_A IN 1", "FRDDR_B OUT 0",
> "TDMOUT_A IN 2", "FRDDR_C OUT 0",
> - "TDM_A Playback", "TDMOUT_A OUT";
> + "TDM_A Playback", "TDMOUT_A OUT",
> + "TDMIN_A IN 1", "TDM_A Capture",
Oops this is wrong
> + "TDMIN_A IN 3", "TDM_A Loopback",
And this correct for the vim3 but not the vim3l ...
Please don't take this patch. Patch 1 can still be applied though.
> + "TODDR_A IN 0", "TDMIN_A OUT",
> + "TODDR_B IN 0", "TDMIN_A OUT",
> + "TODDR_C IN 0", "TDMIN_A OUT";
>
> assigned-clocks = < CLKID_MPLL2>,
> < CLKID_MPLL0>,
> @@ -193,8 +198,20 @@ dai-link-2 {
> sound-dai = <_c>;
> };
>
> - /* 8ch hdmi interface */
> dai-link-3 {
> + sound-dai = <_a>;
> + };
> +
> + dai-link-4 {
> + sound-dai = <_b>;
> + };
> +
> + dai-link-5 {
> + sound-dai = <_c>;
> + };
> +
> + /* 8ch hdmi interface */
> + dai-link-6 {
> sound-dai = <_a>;
> dai-format = "i2s";
> dai-tdm-slot-tx-mask-0 = <1 1>;
> @@ -209,7 +226,7 @@ codec {
> };
>
> /* hdmi glue */
> - dai-link-4 {
> + dai-link-7 {
> sound-dai = < TOHDMITX_I2S_OUT>;
>
> codec {
> @@ -449,10 +466,26 @@ _a {
> status = "okay";
> };
>
> +_a {
> + status = "okay";
> +};
> +
> _a {
> status = "okay";
> };
>
> +_a {
> + status = "okay";
> +};
> +
> +_b {
> + status = "okay";
> +};
> +
> +_c {
> + status = "okay";
> +};
> +
> {
> status = "okay";
> };
Re: 0001-add-amlogic-gpio-to-irq
On Mon 07 Dec 2020 at 13:34, Linus Walleij wrote:
> On Mon, Dec 7, 2020 at 12:07 PM Jerome Brunet wrote:
>> On Mon 07 Dec 2020 at 11:18, Andy Shevchenko
>> wrote:
>> > On Fri, Dec 4, 2020 at 4:25 PM Jerome Brunet wrote:
>> >> On Fri 04 Dec 2020 at 10:13, Linus Walleij
>> >> wrote:
>> >
>> >> This HW only has 8 irqs that can each be mapped to a pin. No direct
>> >> translation can be made, we have to allocate an irq to monitor the line.
>> >> So when gpio_to_irq() was called, we had to do that allocation dynamically
>> >> to return a valid irq number. Since there was no counter part to
>> >> gpio_to_irq(), those allocation cannot be freed during the lifetime of
>> >> the device.
>
> gpio_to_irq() is just a helper really and should not really be used to
> allocate
> anything.
Agreed
>
> In device tree systems, the GPIO provider should nominally present itsel
> as a dual-mode gpio-controller and interrupt-controller for example:
>
> gpio1: gpio@4e00 {
> compatible = "cortina,gemini-gpio",
> "faraday,ftgpio010";
> reg = <0x4e00 0x100>;
> interrupts = <23 IRQ_TYPE_LEVEL_HIGH>;
> resets = < GEMINI_RESET_GPIO1>;
> clocks = < GEMINI_CLK_APB>;
> gpio-controller;
> #gpio-cells = <2>;
> interrupt-controller;
> #interrupt-cells = <2>;
> };
>
> The GPIOs are normally *not* translated to IRQs in this set-up. Rather the
> interrupts are requested by consumers using request_[threaded_]irq()
> which means you should be using the irqchip callbacks such as
> .irq_request_resources() and .irq_release_resources() to allocate one
> of the free irq lines to use. These will be called at the right points if a
> properly written driver requests an IRQ and when the driver is removed.
>
> In some rare cases gpio_to_irq() is used because all the driver knows is
> a GPIO number and it want to try to obtain an IRQ for it, and if a 1-to-1
> mapping exists it returns this number. This is not the norm, but the
> exception.
Sure
>
> So maybe the problem is that you need to go back and think about
> updating the DT bindings for this thing to include interrupt-controller
> as well?
We do
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-meson-gpio.c
That's actually the only thing we provide, on purpose.
>
>> * This HW has to create the mapping between GPIO and irq number
>>dynamically. The number of irqs available is very limited.
>
> This should be done using irq_chip callbacks.
>
Yes
>> * We only get to know a mapping is required when gpio_to_irq() is called
>
> No that callback should not be used for that.
Agreed ... I was trying explain why we did *not* push a patch similar to what
was proposed here, or use gpiolib irqchip.
>
>> * There is no way to know when it is safe to dispose of the created
>>mapping
>
> The way that is done is when .irq_release_resources() is called.
>
>> * Some drivers require a trigger type we don't support. These will create
>>mappings and not use it because of the failure when .set_type() is
>>called
>
> I don't quite understand this. Do you mean you are bombarded by pointless
> requests for interrupts that will not work anyways?
When we tried the approach suggested in this patch (again I agree it is
bad, which is why I'm against it), some drivers out there (I don't
remember which one TBH - that was 3 years ago) parsed the "gpio"
property and tried gpio_to_irq() and if it did not work then go
something else (like polling).
However the allocation stayed behind. It does not take much
"bombardment" when you only have 8.
> Then do not assign
> interrupts to these drivers in the device tree.
We don't.
> These requesting devices and their requests are under your control.
We control the ressources of the devices through DT, not the necessarily
drivers (which may be generic)
Some device needs the gpio, even if we don't want the irq.
We can't always prevent the driver to try gpio_to_irq().
This why I don't want gpio_to_irq() to be enabled on this HW, because it
would not be under our control anymore.
> The drivers should be able to
> back out and work without interrupt if request_irq() fails because it
> can't provide the type on interrupt you want:
>
> int irq = request_irq(irq, my_isr, IRQF_TRIGGER_RISING |
> IRQF_TRIGGER_FALLING, "My ISR", co
Re: 0001-add-amlogic-gpio-to-irq
On Mon 07 Dec 2020 at 11:18, Andy Shevchenko wrote: > On Fri, Dec 4, 2020 at 4:25 PM Jerome Brunet wrote: >> On Fri 04 Dec 2020 at 10:13, Linus Walleij wrote: > >> This HW only has 8 irqs that can each be mapped to a pin. No direct >> translation can be made, we have to allocate an irq to monitor the line. >> So when gpio_to_irq() was called, we had to do that allocation dynamically >> to return a valid irq number. Since there was no counter part to >> gpio_to_irq(), those allocation cannot be freed during the lifetime of >> the device. > > I'm not sure why we are talking about legacy API which should not be > used. I would have been happy to forget about it, but it seems to be the topic of the thread :) > Besides that I didn't get what you meant under counterpart API (IRQ > descriptor has a mapping to the IRQ chip which keeps the mapping to > whatever hardware wants). * This HW has to create the mapping between GPIO and irq number dynamically. The number of irqs available is very limited. * We only get to know a mapping is required when gpio_to_irq() is called * There is no way to know when it is safe to dispose of the created mapping * Some drivers require a trigger type we don't support. These will create mappings and not use it because of the failure when .set_type() is called To answer your question, there an API which lets us know a mapping is needed, but none to inform that it is not required anymore. The GPIO API was not meant to used like this. Not saying it is good or bad, this is just how it is. If there was a way to know it is safe to dispose of the mapping, then letting users of gpio_to_irq() try and fail would be OK, but we don't have that AFAIK. This is why gpio_to_irq() or gpiolib irqchip had not been added so far on this HW. I don't think it is worth fixing, especially if the API is considered to be legacy. On this HW, getting an interupt from a pin is done by going directly after the interrupt controller, like here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/amlogic/meson-gxbb-nanopi-k2.dts#n173 AFAICT, making pps-gpio parse an "interrupt" property should be doable too.
[PATCH 2/2] arm64: dts: meson: vim3: enable hdmi audio loopback
Enable audio capture frontends and a tdm decoder.
This makes it possible to loopback the audio played on the hdmi codec,
which is the only output interface at the moment.
Of course, one TODDR device would be enough to do that but since
the 3 FRDDRs are enabled on the playback side, let's do the same on the
capture side.
Signed-off-by: Jerome Brunet
---
.../boot/dts/amlogic/meson-khadas-vim3.dtsi | 41 +--
1 file changed, 37 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/boot/dts/amlogic/meson-khadas-vim3.dtsi
b/arch/arm64/boot/dts/amlogic/meson-khadas-vim3.dtsi
index 12465c4becc7..4cf2c193d168 100644
--- a/arch/arm64/boot/dts/amlogic/meson-khadas-vim3.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-khadas-vim3.dtsi
@@ -166,11 +166,16 @@ hdmi_connector_in: endpoint {
sound {
compatible = "amlogic,axg-sound-card";
model = "G12B-KHADAS-VIM3";
- audio-aux-devs = <_a>;
+ audio-aux-devs = <_a>, <_a>;
audio-routing = "TDMOUT_A IN 0", "FRDDR_A OUT 0",
"TDMOUT_A IN 1", "FRDDR_B OUT 0",
"TDMOUT_A IN 2", "FRDDR_C OUT 0",
- "TDM_A Playback", "TDMOUT_A OUT";
+ "TDM_A Playback", "TDMOUT_A OUT",
+ "TDMIN_A IN 1", "TDM_A Capture",
+ "TDMIN_A IN 3", "TDM_A Loopback",
+ "TODDR_A IN 0", "TDMIN_A OUT",
+ "TODDR_B IN 0", "TDMIN_A OUT",
+ "TODDR_C IN 0", "TDMIN_A OUT";
assigned-clocks = < CLKID_MPLL2>,
< CLKID_MPLL0>,
@@ -193,8 +198,20 @@ dai-link-2 {
sound-dai = <_c>;
};
- /* 8ch hdmi interface */
dai-link-3 {
+ sound-dai = <_a>;
+ };
+
+ dai-link-4 {
+ sound-dai = <_b>;
+ };
+
+ dai-link-5 {
+ sound-dai = <_c>;
+ };
+
+ /* 8ch hdmi interface */
+ dai-link-6 {
sound-dai = <_a>;
dai-format = "i2s";
dai-tdm-slot-tx-mask-0 = <1 1>;
@@ -209,7 +226,7 @@ codec {
};
/* hdmi glue */
- dai-link-4 {
+ dai-link-7 {
sound-dai = < TOHDMITX_I2S_OUT>;
codec {
@@ -449,10 +466,26 @@ _a {
status = "okay";
};
+_a {
+ status = "okay";
+};
+
_a {
status = "okay";
};
+_a {
+ status = "okay";
+};
+
+_b {
+ status = "okay";
+};
+
+_c {
+ status = "okay";
+};
+
{
status = "okay";
};
--
2.28.0
[PATCH 1/2] arm64: dts: meson: vim3: whitespace fixups
Spaces have been used to indent 2 nodes.
Replace those with tabs and remove one extra newline
Signed-off-by: Jerome Brunet
---
.../boot/dts/amlogic/meson-khadas-vim3.dtsi | 19 +--
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/boot/dts/amlogic/meson-khadas-vim3.dtsi
b/arch/arm64/boot/dts/amlogic/meson-khadas-vim3.dtsi
index 7b46555ac55a..12465c4becc7 100644
--- a/arch/arm64/boot/dts/amlogic/meson-khadas-vim3.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-khadas-vim3.dtsi
@@ -278,12 +278,12 @@ external_phy: ethernet-phy@0 {
};
{
-pinctrl-0 = <_pins>, <_rgmii_pins>;
-pinctrl-names = "default";
-status = "okay";
-phy-mode = "rgmii";
-phy-handle = <_phy>;
-amlogic,tx-delay-ns = <2>;
+ pinctrl-0 = <_pins>, <_rgmii_pins>;
+ pinctrl-names = "default";
+ status = "okay";
+ phy-mode = "rgmii";
+ phy-handle = <_phy>;
+ amlogic,tx-delay-ns = <2>;
};
_a {
@@ -349,9 +349,9 @@ {
};
_ef {
-status = "okay";
-pinctrl-0 = <_e_pins>;
-pinctrl-names = "default";
+ status = "okay";
+ pinctrl-0 = <_e_pins>;
+ pinctrl-names = "default";
};
{
@@ -445,7 +445,6 @@ w25q128: spi-flash@0 {
};
};
-
_a {
status = "okay";
};
--
2.28.0
[PATCH 0/2] arm64: dts: meson: vim3: enable hdmi audio loopback
This patchset enable the hdmi audio loopback on the vim3 and vim3l. I found a few whitespace errors while doing it. This is sent along to avoid annoying conflicts. Jerome Brunet (2): arm64: dts: meson: vim3: whitespace fixups arm64: dts: meson: vim3: enable hdmi audio loopback .../boot/dts/amlogic/meson-khadas-vim3.dtsi | 60 ++- 1 file changed, 46 insertions(+), 14 deletions(-) -- 2.28.0
Re: [PATCH] net: stmmac: dwmac-meson8b: fix mask definition of the m250_sel mux
On Sat 05 Dec 2020 at 22:32, Martin Blumenstingl
wrote:
> The m250_sel mux clock uses bit 4 in the PRG_ETH0 register. Fix this by
> shifting the PRG_ETH0_CLK_M250_SEL_MASK accordingly as the "mask" in
> struct clk_mux expects the mask relative to the "shift" field in the
> same struct.
>
> While here, get rid of the PRG_ETH0_CLK_M250_SEL_SHIFT macro and use
> __ffs() to determine it from the existing PRG_ETH0_CLK_M250_SEL_MASK
> macro.
>
> Fixes: 566e8251625304 ("net: stmmac: add a glue driver for the Amlogic Meson
> 8b / GXBB DWMAC")
> Signed-off-by: Martin Blumenstingl
Reviewed-by: Jerome Brunet
> ---
> drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
> b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
> index dc0b8b6d180d..459ae715b33d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
> @@ -30,7 +30,6 @@
> #define PRG_ETH0_EXT_RMII_MODE 4
>
> /* mux to choose between fclk_div2 (bit unset) and mpll2 (bit set) */
> -#define PRG_ETH0_CLK_M250_SEL_SHIFT 4
> #define PRG_ETH0_CLK_M250_SEL_MASK GENMASK(4, 4)
>
> /* TX clock delay in ns = "8ns / 4 * tx_dly_val" (where 8ns are exactly one
> @@ -155,8 +154,9 @@ static int meson8b_init_rgmii_tx_clk(struct meson8b_dwmac
> *dwmac)
> return -ENOMEM;
>
> clk_configs->m250_mux.reg = dwmac->regs + PRG_ETH0;
> - clk_configs->m250_mux.shift = PRG_ETH0_CLK_M250_SEL_SHIFT;
> - clk_configs->m250_mux.mask = PRG_ETH0_CLK_M250_SEL_MASK;
> + clk_configs->m250_mux.shift = __ffs(PRG_ETH0_CLK_M250_SEL_MASK);
> + clk_configs->m250_mux.mask = PRG_ETH0_CLK_M250_SEL_MASK >>
> + clk_configs->m250_mux.shift;
> clk = meson8b_dwmac_register_clk(dwmac, "m250_sel", mux_parents,
>ARRAY_SIZE(mux_parents), _mux_ops,
>_configs->m250_mux.hw);
Re: [PATCH] clk: meson: g12a: select COMMON_CLK_MESON_VID_PLL_DIV
On Thu 03 Dec 2020 at 23:26, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> Without this, a g12a-only config produces a link error:
>
> aarch64-linux-ld: drivers/clk/meson/g12a.o:(.data+0xcb68): undefined
> reference to `meson_vid_pll_div_ro_ops'
>
> Fixes: 085a4ea93d54 ("clk: meson: g12a: add peripheral clock controller")
> Signed-off-by: Arnd Bergmann
Hi Arnd,
Thanks for sending this fix.
Same change has already been applied:
https://patchwork.kernel.org/project/linux-clk/patch/20201118190930.34352-1-khil...@baylibre.com/
It was part of my last PR to Stephen
Jerome
> ---
> drivers/clk/meson/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index 034da203e8e0..9a8a548d839d 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -110,6 +110,7 @@ config COMMON_CLK_G12A
> select COMMON_CLK_MESON_AO_CLKC
> select COMMON_CLK_MESON_EE_CLKC
> select COMMON_CLK_MESON_CPU_DYNDIV
> + select COMMON_CLK_MESON_VID_PLL_DIV
> select MFD_SYSCON
> help
> Support for the clock controller on Amlogic S905D2, S905X2 and S905Y2
Re: 0001-add-amlogic-gpio-to-irq
On Fri 04 Dec 2020 at 10:13, Linus Walleij wrote: > Hi Lisheng, > > this patch got a bit mangled but I get where you're going. > > I think Meson needs to be augmented to use hierarchical gpiolib irqchip > because this seems to be what the system is doing. > > So start with drivers/pinctrl/meson/Kconfig and add: > > select GPIOLIB_IRQCHIP > select IRQ_DOMAIN_HIEARARCHY > > Then use the generic hierarchical gpiolib irqchip as described > in Documentation/driver-api/gpio/driver.rst > Type > git grep child_to_parent_hwirq > for several examples of how to do this. One reason the irqchip has not been linked to the gpio controller so far is IRQ_EDGE_BOTH which the irqchip does not support (expect for the latest sm1 family) This is a problem we discussed a couple of years ago. This HW only has 8 irqs that can each be mapped to a pin. No direct translation can be made, we have to allocate an irq to monitor the line. So when gpio_to_irq() was called, we had to do that allocation dynamically to return a valid irq number. Since there was no counter part to gpio_to_irq(), those allocation cannot be freed during the lifetime of the device. When drivers relying IRQ_EDGE_BOTH first try the `gpio_to_irq()`, allocating the irq works but setting the type does not. We are then left with unused allocated irqs (and we don't have much) Frameworks using gpio_to_irq() are often capable() of parsing interrupt properties directly too. So far, it was enough to work around the problem. I admit, I have not been following gpiolib closely since then, maybe some progress have been made > > Yours, > Linus Walleij
Re: [PATCH v3 0/7] arm64: dts: meson: add more GX soundcards
On Mon 16 Nov 2020 at 07:20, Christian Hewitt wrote: > This series adds basic support for LPCM audio over HDMI and S/PDIF > to GXBB/GXL/GXM devices that I own and have tested with. Audio can > be extended in the future (some devices have DACs and headphone > hardware to connect) but this gets the basics working. > > Changes from v2 > - Drop p200/p201/p212-s905x/vega-s95 changes > - Add khadas-vim(1) > > Changes from v1 > - Drop nexbox-a1 and rbox-pro > > Christian Hewitt (7): > arm64: dts: meson: add audio playback to a95x > arm64: dts: meson: add audio playback to khadas-vim > arm64: dts: meson: add audio playback to khadas-vim2 > arm64: dts: meson: add audio playback to nanopi-k2 > arm64: dts: meson: add audio playback to odroid-c2 > arm64: dts: meson: add audio playback to wetek-hub > arm64: dts: meson: add audio playback to wetek-play2 > > .../boot/dts/amlogic/meson-gxbb-nanopi-k2.dts | 40 > .../dts/amlogic/meson-gxbb-nexbox-a95x.dts| 40 > .../boot/dts/amlogic/meson-gxbb-odroidc2.dts | 40 > .../boot/dts/amlogic/meson-gxbb-wetek-hub.dts | 40 > .../dts/amlogic/meson-gxbb-wetek-play2.dts| 61 +++ > .../amlogic/meson-gxl-s905x-khadas-vim.dts| 43 - > .../dts/amlogic/meson-gxm-khadas-vim2.dts | 44 - > 7 files changed, 303 insertions(+), 5 deletions(-) Minor comment on patch 3 Minus that: Acked-by: Jerome Brunet
Re: [PATCH v3 3/7] arm64: dts: meson: add audio playback to khadas-vim2
On Mon 16 Nov 2020 at 07:20, Christian Hewitt
wrote:
> Add initial audio support limited to HDMI i2s.
>
> Signed-off-by: Christian Hewitt
> ---
> .../dts/amlogic/meson-gxm-khadas-vim2.dts | 44 +--
> 1 file changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts
> b/arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts
> index bff8ec2c1c70..d4734220443c 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts
> @@ -7,9 +7,9 @@
>
> /dts-v1/;
>
> -#include
> -
> #include "meson-gxm.dtsi"
> +#include
> +#include
It's ok to do clean up or refactoring but it should not be done silently
or mixed with something unrelated
Same in the previous patch
>
> / {
> compatible = "khadas,vim2", "amlogic,s912", "amlogic,meson-gxm";
> @@ -145,6 +145,45 @@
> clock-frequency = <32768>;
> pwms = <_ef 0 30518 0>; /* PWM_E at 32.768KHz */
> };
> +
> + sound {
> + compatible = "amlogic,gx-sound-card";
> + model = "GXM-KHADAS-VIM2";
> + assigned-clocks = < CLKID_MPLL0>,
> + < CLKID_MPLL1>,
> + < CLKID_MPLL2>;
> + assigned-clock-parents = <0>, <0>, <0>;
> + assigned-clock-rates = <294912000>,
> +<270950400>,
> +<393216000>;
> + status = "okay";
> +
> + dai-link-0 {
> + sound-dai = < AIU_CPU CPU_I2S_FIFO>;
> + };
> +
> + dai-link-1 {
> + sound-dai = < AIU_CPU CPU_I2S_ENCODER>;
> + dai-format = "i2s";
> + mclk-fs = <256>;
> +
> + codec-0 {
> + sound-dai = < AIU_HDMI CTRL_I2S>;
> + };
> + };
> +
> + dai-link-2 {
> + sound-dai = < AIU_HDMI CTRL_OUT>;
> +
> + codec-0 {
> + sound-dai = <_tx>;
> + };
> + };
> + };
> +};
> +
> + {
> + status = "okay";
> };
>
> _AO {
> @@ -154,7 +193,6 @@
> hdmi-phandle = <_tx>;
> };
>
> -
Same here
> _cooling_maps {
> map0 {
> cooling-device = <_fan THERMAL_NO_LIMIT 1>;
Re: [PATCH] arm64: dts: meson: fix PHY deassert timing requirements
On Tue 01 Dec 2020 at 01:25, Stefan Agner wrote:
> According to the datasheet (Rev. 1.4, page 30) the RTL8211F requires
> at least 50ms "for internal circuits settling time" before accessing
> the PHY registers. This fixes an issue where the Ethernet link doesn't
> come up when using ip link set down/up:
> [ 29.360965] meson8b-dwmac ff3f.ethernet eth0: Link is Down
> [ 34.569012] meson8b-dwmac ff3f.ethernet eth0: PHY [0.0:00] driver
> [RTL8211F Gigabit Ethernet] (irq=31)
> [ 34.676732] meson8b-dwmac ff3f.ethernet: Failed to reset the dma
> [ 34.678874] meson8b-dwmac ff3f.ethernet eth0: stmmac_hw_setup: DMA
> engine initialization failed
> [ 34.687850] meson8b-dwmac ff3f.ethernet eth0: stmmac_open: Hw setup
> failed
>
> Fixes: 658e4129bb81 ("arm64: dts: meson: g12b: odroid-n2: add the Ethernet
> PHY reset line")
> Signed-off-by: Stefan Agner
> ---
> arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtsi
> b/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtsi
> index 6982632ae646..a5652caacb27 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtsi
> @@ -413,7 +413,7 @@ external_phy: ethernet-phy@0 {
> max-speed = <1000>;
>
> reset-assert-us = <1>;
> - reset-deassert-us = <3>;
> + reset-deassert-us = <5>;
> reset-gpios = < GPIOZ_15 (GPIO_ACTIVE_LOW |
> GPIO_OPEN_DRAIN)>;
>
> interrupt-parent = <_intc>;
Thanks for sharing this is Stefan,
The title of your patch should probably be modified to show that it
addresses the odroid n2 only, as it stands.
I have checked the RTL8211F doc I have, v1.9, and this one mention
"72ms at least - not including the 1.0V supply rise time" before
accessing the PHY registers :/ ... so 80ms maybe ?
Re: [PATCH v2 0/2] clk: meson: g12a: add MIPI DSI Host Pixel Clock
On Thu 26 Nov 2020 at 15:15, Neil Armstrong wrote: > This serie adss the MIPI DSI Host Pixel Clock used to feed the DSI pixel > clock to the DSI Host controller. > > Unlike the AXG SoC, the DSI Pixel Clock has a supplementary mux, divider and > gate > stage before feeding the pixel clock to the MIPI DSI Host controller. > > Changes since v1 at [1]: > - switch g12a_mipi_dsi_pxclk_sel flags to CLK_SET_RATE_NO_REPARENT > - fix aligment of g12a_mipi_dsi_pxclk_div & g12a_mipi_dsi_pxclk parent_hws > > [1] https://lore.kernel.org/r/20201123163811.353444-1-narmstr...@baylibre.com > > Neil Armstrong (2): > dt-bindings: clk: g12a-clkc: add DSI Pixel clock bindings > clk: meson: g12a: add MIPI DSI Host Pixel Clock > > drivers/clk/meson/g12a.c | 74 +++ > drivers/clk/meson/g12a.h | 3 +- > include/dt-bindings/clock/g12a-clkc.h | 2 + > 3 files changed, 78 insertions(+), 1 deletion(-) Applied, Thx
Re: [PATCH net-next 06/15] net: phy: meson-gxl: remove the use of .ack_callback()
On Mon 23 Nov 2020 at 16:38, Ioana Ciornei wrote:
> From: Ioana Ciornei
>
> In preparation of removing the .ack_interrupt() callback, we must replace
> its occurrences (aka phy_clear_interrupt), from the 2 places where it is
> called from (phy_enable_interrupts and phy_disable_interrupts), with
> equivalent functionality.
>
> This means that clearing interrupts now becomes something that the PHY
> driver is responsible of doing, before enabling interrupts and after
> clearing them. Make this driver follow the new contract.
>
> Cc: Jerome Brunet
> Cc: Neil Armstrong
> Signed-off-by: Ioana Ciornei
> ---
> drivers/net/phy/meson-gxl.c | 19 +++
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c
> index b16b1cc89165..7e7904fee1d9 100644
> --- a/drivers/net/phy/meson-gxl.c
> +++ b/drivers/net/phy/meson-gxl.c
> @@ -204,22 +204,27 @@ static int meson_gxl_config_intr(struct phy_device
> *phydev)
> int ret;
>
> if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> + /* Ack any pending IRQ */
> + ret = meson_gxl_ack_interrupt(phydev);
> + if (ret)
> + return ret;
> +
> val = INTSRC_ANEG_PR
> | INTSRC_PARALLEL_FAULT
> | INTSRC_ANEG_LP_ACK
> | INTSRC_LINK_DOWN
> | INTSRC_REMOTE_FAULT
> | INTSRC_ANEG_COMPLETE;
> + ret = phy_write(phydev, INTSRC_MASK, val);
> } else {
> val = 0;
> - }
> + ret = phy_write(phydev, INTSRC_MASK, val);
>
> - /* Ack any pending IRQ */
> - ret = meson_gxl_ack_interrupt(phydev);
> - if (ret)
> - return ret;
> + /* Ack any pending IRQ */
> + ret = meson_gxl_ack_interrupt(phydev);
> + }
>
> - return phy_write(phydev, INTSRC_MASK, val);
> + return ret;
The only thing the above does is clear the irq *after* writing INTSRC_MASK
*only* when the interrupts are not enabled. If that was not the intent,
please let me know.
As it stands, I don't think this hunk is necessary and I would prefer if
it was not included.
> }
>
> static irqreturn_t meson_gxl_handle_interrupt(struct phy_device *phydev)
> @@ -249,7 +254,6 @@ static struct phy_driver meson_gxl_phy[] = {
> .soft_reset = genphy_soft_reset,
> .config_init= meson_gxl_config_init,
> .read_status= meson_gxl_read_status,
> - .ack_interrupt = meson_gxl_ack_interrupt,
> .config_intr= meson_gxl_config_intr,
> .handle_interrupt = meson_gxl_handle_interrupt,
> .suspend= genphy_suspend,
> @@ -260,7 +264,6 @@ static struct phy_driver meson_gxl_phy[] = {
> /* PHY_BASIC_FEATURES */
> .flags = PHY_IS_INTERNAL,
> .soft_reset = genphy_soft_reset,
> - .ack_interrupt = meson_gxl_ack_interrupt,
> .config_intr= meson_gxl_config_intr,
> .handle_interrupt = meson_gxl_handle_interrupt,
> .suspend= genphy_suspend,
Re: [PATCH 2/2] clk: meson: g12a: add MIPI DSI Host Pixel Clock
On Mon 23 Nov 2020 at 17:38, Neil Armstrong wrote:
> This adds the MIPI DSI Host Pixel Clock, unlike AXG, the pixel clock can be
> different
> from the VPU ENCL output clock to feed the DSI Host controller with a
> different clock rate.
>
> Signed-off-by: Neil Armstrong
Series looks good.
2 minor comments below
> ---
> drivers/clk/meson/g12a.c | 72
> drivers/clk/meson/g12a.h | 3 +-
> 2 files changed, 74 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
> index 3cb8196c8e29..3dedf8408405 100644
> --- a/drivers/clk/meson/g12a.c
> +++ b/drivers/clk/meson/g12a.c
> @@ -3658,6 +3658,66 @@ static struct clk_regmap g12a_hdmi_tx = {
> },
> };
>
> +/* MIPI DSI Host Clocks */
> +
> +static const struct clk_hw *g12a_mipi_dsi_pxclk_parent_hws[] = {
> + _vid_pll.hw,
> + _gp0_pll.hw,
> + _hifi_pll.hw,
> + _mpll1.hw,
> + _fclk_div2.hw,
> + _fclk_div2p5.hw,
> + _fclk_div3.hw,
> + _fclk_div7.hw,
> +};
> +
> +static struct clk_regmap g12a_mipi_dsi_pxclk_sel = {
> + .data = &(struct clk_regmap_mux_data){
> + .offset = HHI_MIPIDSI_PHY_CLK_CNTL,
> + .mask = 0x7,
> + .shift = 12,
> + .flags = CLK_MUX_ROUND_CLOSEST,
> + },
> + .hw.init = &(struct clk_init_data){
> + .name = "mipi_dsi_pxclk_sel",
> + .ops = _regmap_mux_ops,
> + .parent_hws = g12a_mipi_dsi_pxclk_parent_hws,
> + .num_parents = ARRAY_SIZE(g12a_mipi_dsi_pxclk_parent_hws),
> + .flags = CLK_SET_RATE_PARENT,
The id of the mux is exposed which seems to hint the mux will be
manually controller but CLK_SET_RATE_NO_REPARENT is not set. Is this on
purpose ?
> + },
> +};
> +
> +static struct clk_regmap g12a_mipi_dsi_pxclk_div = {
> + .data = &(struct clk_regmap_div_data){
> + .offset = HHI_MIPIDSI_PHY_CLK_CNTL,
> + .shift = 0,
> + .width = 7,
> + },
> + .hw.init = &(struct clk_init_data){
> + .name = "mipi_dsi_pxclk_div",
> + .ops = _regmap_divider_ops,
> + .parent_hws = (const struct clk_hw *[]) {
> + _mipi_dsi_pxclk_sel.hw },
Alignment here is weird compared to the reset of the file
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + },
> +};
> +
> +static struct clk_regmap g12a_mipi_dsi_pxclk = {
> + .data = &(struct clk_regmap_gate_data){
> + .offset = HHI_MIPIDSI_PHY_CLK_CNTL,
> + .bit_idx = 8,
> + },
> + .hw.init = &(struct clk_init_data) {
> + .name = "mipi_dsi_pxclk",
> + .ops = _regmap_gate_ops,
> + .parent_hws = (const struct clk_hw *[]) {
> + _mipi_dsi_pxclk_div.hw },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + },
> +};
> +
> /* HDMI Clocks */
>
> static const struct clk_parent_data g12a_hdmi_parent_data[] = {
> @@ -4403,6 +4463,9 @@ static struct clk_hw_onecell_data g12a_hw_onecell_data
> = {
> [CLKID_SPICC1_SCLK_SEL] = _spicc1_sclk_sel.hw,
> [CLKID_SPICC1_SCLK_DIV] = _spicc1_sclk_div.hw,
> [CLKID_SPICC1_SCLK] = _spicc1_sclk.hw,
> + [CLKID_MIPI_DSI_PXCLK_SEL] = _mipi_dsi_pxclk_sel.hw,
> + [CLKID_MIPI_DSI_PXCLK_DIV] = _mipi_dsi_pxclk_div.hw,
> + [CLKID_MIPI_DSI_PXCLK] = _mipi_dsi_pxclk.hw,
> [NR_CLKS] = NULL,
> },
> .num = NR_CLKS,
> @@ -4658,6 +4721,9 @@ static struct clk_hw_onecell_data g12b_hw_onecell_data
> = {
> [CLKID_SPICC1_SCLK_SEL] = _spicc1_sclk_sel.hw,
> [CLKID_SPICC1_SCLK_DIV] = _spicc1_sclk_div.hw,
> [CLKID_SPICC1_SCLK] = _spicc1_sclk.hw,
> + [CLKID_MIPI_DSI_PXCLK_SEL] = _mipi_dsi_pxclk_sel.hw,
> + [CLKID_MIPI_DSI_PXCLK_DIV] = _mipi_dsi_pxclk_div.hw,
> + [CLKID_MIPI_DSI_PXCLK] = _mipi_dsi_pxclk.hw,
> [NR_CLKS] = NULL,
> },
> .num = NR_CLKS,
> @@ -4904,6 +4970,9 @@ static struct clk_hw_onecell_data sm1_hw_onecell_data =
> {
> [CLKID_NNA_CORE_CLK_SEL]= _nna_core_clk_sel.hw,
> [CLKID_NNA_CORE_CLK_DIV]= _nna_core_clk_div.hw,
> [CLKID_NNA_CORE_CLK]= _nna_core_clk.hw,
> + [CLKID_MIPI_DSI_PXCLK_SEL] = _mipi_dsi_pxclk_sel.hw,
> + [CLKID_MIPI_DSI_PXCLK_DIV] = _mipi_dsi_pxclk_div.hw,
> + [CLKID_MIPI_DSI_PXCLK] = _mipi_dsi_pxclk.hw,
> [NR_CLKS] = NULL,
> },
> .num = NR_CLKS,
> @@ -5151,6 +5220,9 @@ static struct clk_regmap *const g12a_clk_regmaps[] = {
> _nna_core_clk_sel,
Re: [PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown
On Fri 20 Nov 2020 at 10:42, Marc Zyngier wrote:
> The HDMI driver request clocks early, but never disable them, leaving
> the clocks on even when the driver is removed.
>
> Fix it by slightly refactoring the clock code, and register a devm
> action that will eventually disable/unprepare the enabled clocks.
>
> Signed-off-by: Marc Zyngier
> ---
> drivers/gpu/drm/meson/meson_dw_hdmi.c | 43 ++-
> 1 file changed, 29 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index 7f8eea494147..29623b309cb1 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -145,8 +145,6 @@ struct meson_dw_hdmi {
> struct reset_control *hdmitx_apb;
> struct reset_control *hdmitx_ctrl;
> struct reset_control *hdmitx_phy;
> - struct clk *hdmi_pclk;
> - struct clk *venci_clk;
> struct regulator *hdmi_supply;
> u32 irq_stat;
> struct dw_hdmi *hdmi;
> @@ -946,6 +944,29 @@ static void meson_disable_regulator(void *data)
> regulator_disable(data);
> }
>
> +static void meson_disable_clk(void *data)
> +{
> + clk_disable_unprepare(data);
> +}
> +
> +static int meson_enable_clk(struct device *dev, char *name)
> +{
> + struct clk *clk;
> + int ret;
> +
> + clk = devm_clk_get(dev, name);
> + if (IS_ERR(clk)) {
> + dev_err(dev, "Unable to get %s pclk\n", name);
> + return PTR_ERR(clk);
> + }
> +
> + ret = clk_prepare_enable(clk);
> + if (!ret)
> + ret = devm_add_action_or_reset(dev, meson_disable_clk,
> clk);
Thanks for fixing this Marc.
FYI, while it is fine to declare a function to disable the clocks, a quick
cast may avoid it
devm_add_action_or_reset(dev, (void(*)(void *))clk_disable_unprepare, clk);
> +
> + return ret;
> +}
> +
> static int meson_dw_hdmi_bind(struct device *dev, struct device *master,
> void *data)
> {
> @@ -1026,19 +1047,13 @@ static int meson_dw_hdmi_bind(struct device *dev,
> struct device *master,
> if (IS_ERR(meson_dw_hdmi->hdmitx))
> return PTR_ERR(meson_dw_hdmi->hdmitx);
>
> - meson_dw_hdmi->hdmi_pclk = devm_clk_get(dev, "isfr");
> - if (IS_ERR(meson_dw_hdmi->hdmi_pclk)) {
> - dev_err(dev, "Unable to get HDMI pclk\n");
> - return PTR_ERR(meson_dw_hdmi->hdmi_pclk);
> - }
> - clk_prepare_enable(meson_dw_hdmi->hdmi_pclk);
> + ret = meson_enable_clk(dev, "isfr");
> + if (ret)
> + return ret;
>
> - meson_dw_hdmi->venci_clk = devm_clk_get(dev, "venci");
> - if (IS_ERR(meson_dw_hdmi->venci_clk)) {
> - dev_err(dev, "Unable to get venci clk\n");
> - return PTR_ERR(meson_dw_hdmi->venci_clk);
> - }
> - clk_prepare_enable(meson_dw_hdmi->venci_clk);
> + ret = meson_enable_clk(dev, "venci");
> + if (ret)
> + return ret;
>
> dw_plat_data->regm = devm_regmap_init(dev, NULL, meson_dw_hdmi,
> _dw_hdmi_regmap_config);
Re: next/master bisection: baseline.dmesg.emerg on meson-gxbb-p200
On Thu 19 Nov 2020 at 19:04, Guillaume Tucker
wrote:
> Hi Marc,
>
> On 19/11/2020 11:58, Marc Zyngier wrote:
>> On 2020-11-19 10:26, Neil Armstrong wrote:
>>> On 19/11/2020 11:20, Marc Zyngier wrote:
On 2020-11-19 08:50, Guillaume Tucker wrote:
> Please see the automated bisection report below about some kernel
> errors on meson-gxbb-p200.
>
> Reports aren't automatically sent to the public while we're
> trialing new bisection features on kernelci.org, however this one
> looks valid.
>
> The bisection started with next-20201118 but the errors are still
> present in next-20201119. Details for this regression:
>
> https://kernelci.org/test/case/id/5fb6196bfd0127fd68d8d902/
>
> The first error is:
>
> [ 14.757489] Internal error: synchronous external abort: 96000210
> [#1] PREEMPT SMP
Looks like yet another clock ordering setup. I guess different Amlogic
platforms have slightly different ordering requirements.
Neil, do you have any idea of which platform requires which ordering?
The variability in DT and platforms is pretty difficult to follow (and
I don't think I have such board around).
>>>
>>> The requirements should be the same, here the init was done before calling
>>> dw_hdmi_probe to be sure the clocks and internals resets were deasserted.
>>> But since you boot from u-boot already enabling these, it's already active.
>>>
>>> The solution would be to revert and do some check in meson_dw_hdmi_init() to
>>> check if already enabled and do nothing.
>>
>> A better fix seems to be this, which makes it explicit that there is
>> a dependency between some of the registers accessed from meson_dw_hdmi_init()
>> and the iahb clock.
>>
>> Guillaume, can you give this a go on your failing box?
>
> I confirm it solves the problem. Please add this to your fix
> patch if it's OK with you:
>
> Reported-by: "kernelci.org bot"
> Tested-by: Guillaume Tucker
>
>
> For the record, it passed all the tests when applied on top of
> the "bad" revision found by the bisection:
>
>
> http://lava.baylibre.com:10080/scheduler/alljobs?search=v5.10-rc3-1021-gb8668a2e5ea1
>
> and the exact same test on the "bad" revision without the fix
> consistently showed the error:
>
> http://lava.baylibre.com:10080/scheduler/job/374176
>
>
> Thanks,
> Guillaume
>
>
>> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> b/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> index 7f8eea494147..52af8ba94311 100644
>> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
>> @@ -146,6 +146,7 @@ struct meson_dw_hdmi {
>> struct reset_control *hdmitx_ctrl;
>> struct reset_control *hdmitx_phy;
>> struct clk *hdmi_pclk;
>> +struct clk *iahb_clk;
>> struct clk *venci_clk;
>> struct regulator *hdmi_supply;
>> u32 irq_stat;
>> @@ -1033,6 +1034,13 @@ static int meson_dw_hdmi_bind(struct device *dev,
>> struct device *master,
>> }
>> clk_prepare_enable(meson_dw_hdmi->hdmi_pclk);
>>
>> +meson_dw_hdmi->iahb_clk = devm_clk_get(dev, "iahb");
>> +if (IS_ERR(meson_dw_hdmi->iahb_clk)) {
>> +dev_err(dev, "Unable to get iahb clk\n");
>> +return PTR_ERR(meson_dw_hdmi->iahb_clk);
>> +}
>> +clk_prepare_enable(meson_dw_hdmi->iahb_clk);
If you guys are going ahead with this fix, this call to
clk_prepare_enable() needs to be balanced with clk_disable_unprepare() somehow
>> +
>> meson_dw_hdmi->venci_clk = devm_clk_get(dev, "venci");
>> if (IS_ERR(meson_dw_hdmi->venci_clk)) {
>> dev_err(dev, "Unable to get venci clk\n");
>> @@ -1071,6 +1079,8 @@ static int meson_dw_hdmi_bind(struct device *dev,
>> struct device *master,
>>
>> encoder->possible_crtcs = BIT(0);
>>
>> +meson_dw_hdmi_init(meson_dw_hdmi);
>> +
>> DRM_DEBUG_DRIVER("encoder initialized\n");
>>
>> /* Bridge / Connector */
>> @@ -1095,8 +1105,6 @@ static int meson_dw_hdmi_bind(struct device *dev,
>> struct device *master,
>> if (IS_ERR(meson_dw_hdmi->hdmi))
>> return PTR_ERR(meson_dw_hdmi->hdmi);
>>
>> -meson_dw_hdmi_init(meson_dw_hdmi);
>> -
>> next_bridge = of_drm_find_bridge(pdev->dev.of_node);
>> if (next_bridge)
>> drm_bridge_attach(encoder, next_bridge,
>>
>>
[PATCH] ASoC: meson: fix COMPILE_TEST error
When compiled with CONFIG_HAVE_CLK, the kernel need to get provider for the
clock API. This is usually selected by the platform and the sound drivers
should not really care about this. However COMPILE_TEST is special and the
platform required may not have been selected, leading to this type of
error:
> aiu-encoder-spdif.c:(.text+0x3a0): undefined reference to `clk_set_parent'
Since we need a sane provider of the API with COMPILE_TEST, depends on
COMMON_CLK.
Fixes: 6dc4fa179fb8 ("ASoC: meson: add axg fifo base driver")
Reported-by: kernel test robot
Signed-off-by: Jerome Brunet
---
sound/soc/meson/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/meson/Kconfig b/sound/soc/meson/Kconfig
index 363dc3b1bbe4..ce0cbdc69b2e 100644
--- a/sound/soc/meson/Kconfig
+++ b/sound/soc/meson/Kconfig
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0-only
menu "ASoC support for Amlogic platforms"
- depends on ARCH_MESON || COMPILE_TEST
+ depends on ARCH_MESON || (COMPILE_TEST && COMMON_CLK)
config SND_MESON_AIU
tristate "Amlogic AIU"
--
2.28.0
[PATCH] arm64: meson: select COMMON_CLK
This fix the recent removal of clock drivers selection.
While it is not necessary to select the clock drivers themselves, we need
to select a proper implementation of the clock API, which for the meson, is
CCF
Fixes: ba66a25536dd ("arm64: meson: ship only the necessary clock controllers")
Signed-off-by: Jerome Brunet
---
This fix is a side effect of this 0day report:
https://lore.kernel.org/r/202011151309.dvcsullh-...@intel.com
It shows that we need to select an implementation of clk.h
arch/arm64/Kconfig.platforms | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 329e22c09cad..484e81f39db7 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -151,6 +151,7 @@ config ARCH_MEDIATEK
config ARCH_MESON
bool "Amlogic Platforms"
+ select COMMON_CLK
select PINCTRL
select PINCTRL_MESON
select MESON_IRQ_GPIO
--
2.28.0
Re: [PATCH v2] reset: make shared pulsed reset controls re-triggerable
On Mon 16 Nov 2020 at 17:36, Philipp Zabel wrote: > On Fri, 2020-11-13 at 16:13 +0100, Jerome Brunet wrote: >> On Fri 13 Nov 2020 at 16:04, Philipp Zabel wrote: >> >> > On Fri, 2020-11-13 at 00:00 +0100, Amjad Ouled-Ameur wrote: >> > > The current reset framework API does not allow to release what is done by >> > > reset_control_reset(), IOW decrement triggered_count. Add the new >> > > reset_control_rearm() call to do so. >> > > >> > > When reset_control_reset() has been called once, the counter >> > > triggered_count, in the reset framework, is incremented i.e the resource >> > > under the reset is in-use and the reset should not be done again. >> > > reset_control_rearm() would be the way to state that the resource is >> > > no longer used and, that from the caller's perspective, the reset can be >> > > fired again if necessary. >> > > >> > > Signed-off-by: Amjad Ouled-Ameur >> > > Reported-by: Jerome Brunet >> > > --- >> > > Change since v1: [0] >> > > * Renamed the new call from reset_control_(array_)resettable to >> > > reset_control_(array_)rearm >> > > * Open-coded reset_control_array_rearm to check for errors before >> > > decrementing triggered_count because we cannot roll back in case an >> > > error occurs while decrementing one of the rstc. >> > > * Reworded the new call's description. >> > >> > Thank you, applied to reset/next. >> >> Hi Philipp, >> >> Would it be possible to get an immutable branch/tag with this ? >> It would allow to move forward on the USB side, without waiting for the >> next rc1. > > Here you go, > > The following changes since commit 3650b228f83adda7e5ee532e2b90429c03f7b9ec: > > Linux 5.10-rc1 (2020-10-25 15:14:11 -0700) > > are available in the Git repository at: > > git://git.pengutronix.de/git/pza/linux.git reset/shared-retrigger > > for you to fetch changes up to 557acb3d2cd9c82de19f944f6cc967a347735385: > > reset: make shared pulsed reset controls re-triggerable (2020-11-16 > 17:05:28 +0100) > > > Amjad Ouled-Ameur (1): > reset: make shared pulsed reset controls re-triggerable > > drivers/reset/core.c | 73 > +++ > include/linux/reset.h | 1 + > 2 files changed, 74 insertions(+) > Thx Philipp ! > regards > Philipp
Re: [PATCH] mmc: meson-gx: remove IRQF_ONESHOT
On Wed 11 Nov 2020 at 11:47, Ulf Hansson wrote: > On Tue, 10 Nov 2020 at 16:05, Jerome Brunet wrote: >> >> >> On Thu 08 Oct 2020 at 11:08, Ulf Hansson wrote: >> > >> > Thomas, thanks a lot for helping out and looking at this! >> > >> > It looks like the testing of the patch below went well. Are you >> > intending to queue up the patch via your tip tree? >> > >> > If you need any help, just tell us! >> > >> > Kind regards >> > Uffe >> > >> >> Hi everyone, >> >> Do we have a plan for this issue ? >> I've had Thomas's change in my tree for a month, so far, so good. > > Instead of waiting for Thomas, perhaps you can pick up his patch and > re-submit it? TBH, I'm not confortable signing off something when I have no idea about the implication, which is the case here. > > From my side, I can of course apply your original fix to the mmc > driver, as an intermediate step. In Thomas first reply, I did not really understand if it was bad from the driver to use IRQF_ONESHOT. If it is OK, i'd prefer if things stayed as they are. Otherwise, feel free to apply it. > Is there a hurry? > Absolutely no hurry, at least not for me. I noticed I still had Thomas's patch on top of the last rc which means it had not been applied yet. Fishing for news, that's all. > Kind regards > Uffe
Re: [PATCH v2] reset: make shared pulsed reset controls re-triggerable
On Fri 13 Nov 2020 at 16:04, Philipp Zabel wrote: > On Fri, 2020-11-13 at 00:00 +0100, Amjad Ouled-Ameur wrote: >> The current reset framework API does not allow to release what is done by >> reset_control_reset(), IOW decrement triggered_count. Add the new >> reset_control_rearm() call to do so. >> >> When reset_control_reset() has been called once, the counter >> triggered_count, in the reset framework, is incremented i.e the resource >> under the reset is in-use and the reset should not be done again. >> reset_control_rearm() would be the way to state that the resource is >> no longer used and, that from the caller's perspective, the reset can be >> fired again if necessary. >> >> Signed-off-by: Amjad Ouled-Ameur >> Reported-by: Jerome Brunet >> --- >> Change since v1: [0] >> * Renamed the new call from reset_control_(array_)resettable to >> reset_control_(array_)rearm >> * Open-coded reset_control_array_rearm to check for errors before >> decrementing triggered_count because we cannot roll back in case an >> error occurs while decrementing one of the rstc. >> * Reworded the new call's description. > > Thank you, applied to reset/next. Hi Philipp, Would it be possible to get an immutable branch/tag with this ? It would allow to move forward on the USB side, without waiting for the next rc1. Thx Jerome > > regards > Philipp
Re: [PATCH] mmc: meson-gx: remove IRQF_ONESHOT
On Thu 08 Oct 2020 at 11:08, Ulf Hansson wrote: > > Thomas, thanks a lot for helping out and looking at this! > > It looks like the testing of the patch below went well. Are you > intending to queue up the patch via your tip tree? > > If you need any help, just tell us! > > Kind regards > Uffe > Hi everyone, Do we have a plan for this issue ? I've had Thomas's change in my tree for a month, so far, so good. Cheers Jerome
Re: [PATCH] mmc: meson-mx-sdio: replace spin_lock_irqsave by spin_lock in hard IRQ
On Tue 03 Nov 2020 at 04:48, Tian Tao wrote:
> The code has been in a irq-disabled context since it is hard IRQ. There
> is no necessity to do it again.
>
> Signed-off-by: Tian Tao
Reviewed-by: Jerome Brunet
> ---
> drivers/mmc/host/meson-mx-sdio.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/meson-mx-sdio.c
> b/drivers/mmc/host/meson-mx-sdio.c
> index 1c5299c..d4a4891 100644
> --- a/drivers/mmc/host/meson-mx-sdio.c
> +++ b/drivers/mmc/host/meson-mx-sdio.c
> @@ -418,10 +418,9 @@ static irqreturn_t meson_mx_mmc_irq(int irq, void *data)
> {
> struct meson_mx_mmc_host *host = (void *) data;
> u32 irqs, send;
> - unsigned long irqflags;
> irqreturn_t ret;
>
> - spin_lock_irqsave(>irq_lock, irqflags);
> + spin_lock(>irq_lock);
>
> irqs = readl(host->base + MESON_MX_SDIO_IRQS);
> send = readl(host->base + MESON_MX_SDIO_SEND);
> @@ -434,7 +433,7 @@ static irqreturn_t meson_mx_mmc_irq(int irq, void *data)
> /* finally ACK all pending interrupts */
> writel(irqs, host->base + MESON_MX_SDIO_IRQS);
>
> - spin_unlock_irqrestore(>irq_lock, irqflags);
> + spin_unlock(>irq_lock);
>
> return ret;
> }
Re: [PATCH] clk: define to_clk_regmap() as inline function
On Mon 26 Oct 2020 at 17:13, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> Nesting container_of() causes warnings with W=2, which is
> annoying if it happens in headers and fills the build log
> like:
>
> In file included from drivers/clk/qcom/clk-alpha-pll.c:6:
> drivers/clk/qcom/clk-alpha-pll.c: In function 'clk_alpha_pll_hwfsm_enable':
> include/linux/kernel.h:852:8: warning: declaration of '__mptr' shadows a
> previous local [-Wshadow]
> 852 | void *__mptr = (void *)(ptr); \
> |^~
> drivers/clk/qcom/clk-alpha-pll.c:155:31: note: in expansion of macro
> 'container_of'
> 155 | #define to_clk_alpha_pll(_hw) container_of(to_clk_regmap(_hw), \
> | ^~~~
> drivers/clk/qcom/clk-regmap.h:27:28: note: in expansion of macro
> 'container_of'
>27 | #define to_clk_regmap(_hw) container_of(_hw, struct clk_regmap, hw)
> |^~~~
> drivers/clk/qcom/clk-alpha-pll.c:155:44: note: in expansion of macro
> 'to_clk_regmap'
> 155 | #define to_clk_alpha_pll(_hw) container_of(to_clk_regmap(_hw), \
> |^
> drivers/clk/qcom/clk-alpha-pll.c:254:30: note: in expansion of macro
> 'to_clk_alpha_pll'
> 254 | struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);
> | ^~~~
> include/linux/kernel.h:852:8: note: shadowed declaration is here
> 852 | void *__mptr = (void *)(ptr); \
> |^~
>
> Redefine two copies of the to_clk_regmap() macro as inline functions
> to avoid a lot of these.
>
> Fixes: ea11dda9e091 ("clk: meson: add regmap clocks")
> Fixes: 085d7a455444 ("clk: qcom: Add a regmap type clock struct")
> Signed-off-by: Arnd Bergmann
Acked-by: Jerome Brunet
> ---
> drivers/clk/meson/clk-regmap.h | 5 -
> drivers/clk/qcom/clk-regmap.h | 6 +-
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/meson/clk-regmap.h b/drivers/clk/meson/clk-regmap.h
> index c4a39604cffd..e365312da54e 100644
> --- a/drivers/clk/meson/clk-regmap.h
> +++ b/drivers/clk/meson/clk-regmap.h
> @@ -26,7 +26,10 @@ struct clk_regmap {
> void*data;
> };
>
> -#define to_clk_regmap(_hw) container_of(_hw, struct clk_regmap, hw)
> +static inline struct clk_regmap *to_clk_regmap(struct clk_hw *hw)
> +{
> + return container_of(hw, struct clk_regmap, hw);
> +}
>
> /**
> * struct clk_regmap_gate_data - regmap backed gate specific data
> diff --git a/drivers/clk/qcom/clk-regmap.h b/drivers/clk/qcom/clk-regmap.h
> index 6cfc1bccb255..14ec659a3a77 100644
> --- a/drivers/clk/qcom/clk-regmap.h
> +++ b/drivers/clk/qcom/clk-regmap.h
> @@ -24,7 +24,11 @@ struct clk_regmap {
> unsigned int enable_mask;
> bool enable_is_inverted;
> };
> -#define to_clk_regmap(_hw) container_of(_hw, struct clk_regmap, hw)
> +
> +static inline struct clk_regmap *to_clk_regmap(struct clk_hw *hw)
> +{
> + return container_of(hw, struct clk_regmap, hw);
> +}
>
> int clk_is_enabled_regmap(struct clk_hw *hw);
> int clk_enable_regmap(struct clk_hw *hw);
Re: [PATCH v2 3/4] clk: meson: axg: add Video Clocks
On Tue 15 Sep 2020 at 14:45, Neil Armstrong wrote:
> Add the clocks entries used in the video clock path, the clock path
> is doubled to permit having different synchronized clocks for different
> parts of the video pipeline.
>
> The AXG only has a single ENCL CTS clock and even if VCLK exist along VCLK2,
> only VCLK2 is used since it clocks the MIPI DSI IP directly.
>
> All dividers are flagged with CLK_GET_RATE_NOCACHE, and all gates are flagged
> with CLK_IGNORE_UNUSED since they are currently directly handled by the
> Meson DRM Driver.
> Once the DRM Driver is fully migrated to using the Common Clock Framework
> to handle the video clock tree, the CLK_GET_RATE_NOCACHE and CLK_IGNORE_UNUSED
> will be dropped.
As Kevin suggested on v1, I also would welcome some details on the
plan get there. Adding clocks like this can only treated as a temporary
work around.
>
> Signed-off-by: Neil Armstrong
> ---
> drivers/clk/meson/axg.c | 753
> drivers/clk/meson/axg.h | 21 +-
> 2 files changed, 773 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/meson/axg.c b/drivers/clk/meson/axg.c
> index 13fc0006f63d..a4e8949297cf 100644
> --- a/drivers/clk/meson/axg.c
> +++ b/drivers/clk/meson/axg.c
> @@ -1026,6 +1026,683 @@ static struct clk_regmap axg_sd_emmc_c_clk0 = {
> },
> };
>
> +/* VPU Clock */
> +
> +static const struct clk_hw *axg_vpu_parent_hws[] = {
> + _fclk_div4.hw,
> + _fclk_div3.hw,
> + _fclk_div5.hw,
> + _fclk_div7.hw,
> +};
> +
> +static struct clk_regmap axg_vpu_0_sel = {
> + .data = &(struct clk_regmap_mux_data){
> + .offset = HHI_VPU_CLK_CNTL,
> + .mask = 0x3,
> + .shift = 9,
> + },
> + .hw.init = &(struct clk_init_data){
> + .name = "vpu_0_sel",
> + .ops = _regmap_mux_ops,
> + .parent_hws = axg_vpu_parent_hws,
> + .num_parents = ARRAY_SIZE(axg_vpu_parent_hws),
> + /* We need a specific parent for VPU clock source, let it be
> set in DT */
> + .flags = CLK_SET_RATE_NO_REPARENT,
> + },
> +};
> +
> +static struct clk_regmap axg_vpu_0_div = {
> + .data = &(struct clk_regmap_div_data){
> + .offset = HHI_VPU_CLK_CNTL,
> + .shift = 0,
> + .width = 7,
> + },
> + .hw.init = &(struct clk_init_data){
> + .name = "vpu_0_div",
> + .ops = _regmap_divider_ops,
> + .parent_hws = (const struct clk_hw *[]) { _vpu_0_sel.hw },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + },
> +};
> +
> +static struct clk_regmap axg_vpu_0 = {
> + .data = &(struct clk_regmap_gate_data){
> + .offset = HHI_VPU_CLK_CNTL,
> + .bit_idx = 8,
> + },
> + .hw.init = &(struct clk_init_data) {
> + .name = "vpu_0",
> + .ops = _regmap_gate_ops,
> + .parent_hws = (const struct clk_hw *[]) { _vpu_0_div.hw },
> + .num_parents = 1,
> + /*
> + * We want to avoid CCF to disable the VPU clock if
> + * display has been set by Bootloader
> + */
> + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> + },
> +};
> +
> +static struct clk_regmap axg_vpu_1_sel = {
> + .data = &(struct clk_regmap_mux_data){
> + .offset = HHI_VPU_CLK_CNTL,
> + .mask = 0x3,
> + .shift = 25,
> + },
> + .hw.init = &(struct clk_init_data){
> + .name = "vpu_1_sel",
> + .ops = _regmap_mux_ops,
> + .parent_hws = axg_vpu_parent_hws,
> + .num_parents = ARRAY_SIZE(axg_vpu_parent_hws),
> + /* We need a specific parent for VPU clock source, let it be
> set in DT */
> + .flags = CLK_SET_RATE_NO_REPARENT,
> + },
> +};
> +
> +static struct clk_regmap axg_vpu_1_div = {
> + .data = &(struct clk_regmap_div_data){
> + .offset = HHI_VPU_CLK_CNTL,
> + .shift = 16,
> + .width = 7,
> + },
> + .hw.init = &(struct clk_init_data){
> + .name = "vpu_1_div",
> + .ops = _regmap_divider_ops,
> + .parent_hws = (const struct clk_hw *[]) { _vpu_1_sel.hw },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + },
> +};
> +
> +static struct clk_regmap axg_vpu_1 = {
> + .data = &(struct clk_regmap_gate_data){
> + .offset = HHI_VPU_CLK_CNTL,
> + .bit_idx = 24,
> + },
> + .hw.init = &(struct clk_init_data) {
> + .name = "vpu_1",
> + .ops = _regmap_gate_ops,
> + .parent_hws = (const struct clk_hw *[]) { _vpu_1_div.hw },
> + .num_parents = 1,
> + /*
> + * We want to avoid CCF to disable the VPU clock if
> + * display has been set by Bootloader
> + */
> + .flags =
Re: [PATCH v2 0/4] clk: meson: axg: add clocks for MIPI-DSI support
On Tue 15 Sep 2020 at 14:45, Neil Armstrong wrote: > This adds the VPU & VAPB clocks along the MIPI DSI Host clock. > > The clock scheme is based on the GXBB & G12A VPU clocks, with a different CTS > clock output used for MIPI-DSI. > > Changes since v1 at [1]: > - update patch 3 commit message to reflect drm driver state > - added comments in patch 3 for clock specificities > - removed useless parents comments in patch 2 > - fixed bad flags in patch 4 > - removed holes in axg_vdin_meas_parent_data in patch 4 > > [1] https://lkml.kernel.org/r/20200907093810.6585-1-narmstr...@baylibre.com > > Neil Armstrong (4): > dt-bindings: clk: axg-clkc: add Video Clocks > dt-bindings: clk: axg-clkc: add MIPI DSI Host clock binding > clk: meson: axg: add Video Clocks > clk: meson: axg: add MIPI DSI Host clock Applied for v5.11 fixing 75 char per line patches 3 & 4 commit descriptions > > drivers/clk/meson/axg.c | 819 +++ > drivers/clk/meson/axg.h | 23 +- > include/dt-bindings/clock/axg-clkc.h | 25 + > 3 files changed, 866 insertions(+), 1 deletion(-)
[PATCH] arm64: dts: meson: odroid-n2 plus: fix vddcpu_a pwm
On the odroid N2 plus, cpufreq is not available due to an error on the cpu
regulators. vddcpu a and b get the same PWM. The one provided to vddcpu A
is incorrect. Because vddcpu B PWM is busy the regulator cannot register:
> pwm-regulator regulator-vddcpu-b: Failed to get PWM: -16
Like on the odroid n2, use PWM A out of GPIOE_2 for vddcpu A to fix the
problem
Fixes: 98d24896ee11 ("arm64: dts: meson: add support for the ODROID-N2+")
Signed-off-by: Jerome Brunet
---
arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2-plus.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2-plus.dts
b/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2-plus.dts
index 5de2815ba99d..ce1198ad34e4 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2-plus.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2-plus.dts
@@ -19,7 +19,7 @@ _a {
regulator-min-microvolt = <68>;
regulator-max-microvolt = <104>;
- pwms = <_AO_cd 1 1500 0>;
+ pwms = <_ab 0 1500 0>;
};
_b {
--
2.25.4
[PATCH 2/2] clk: meson: g12: use devm variant to register notifiers
Until now, nothing was done to unregister the dvfs clock notifiers of the
Amlogic g12 SoC family. This is not great but this driver was not really
expected to be unloaded. With the ongoing effort to build everything as
module for this platform, this needs to be cleanly handled.
Signed-off-by: Jerome Brunet
---
This clean application of this patch depends on
https://lore.kernel.org/r/20201021162147.563655-4-jbru...@baylibre.com
drivers/clk/meson/g12a.c | 34 --
include/linux/clk.h | 10 +-
2 files changed, 29 insertions(+), 15 deletions(-)
diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
index bbb75541dad9..bb4fa19442be 100644
--- a/drivers/clk/meson/g12a.c
+++ b/drivers/clk/meson/g12a.c
@@ -5171,8 +5171,8 @@ static int meson_g12a_dvfs_setup_common(struct device
*dev,
g12a_cpu_clk_postmux0_nb_data.xtal = xtal;
notifier_clk = devm_clk_hw_get_clk(dev, _cpu_clk_postmux0.hw,
DVFS_CON_ID);
- ret = clk_notifier_register(notifier_clk,
- _cpu_clk_postmux0_nb_data.nb);
+ ret = devm_clk_notifier_register(dev, notifier_clk,
+_cpu_clk_postmux0_nb_data.nb);
if (ret) {
dev_err(dev, "failed to register the cpu_clk_postmux0
notifier\n");
return ret;
@@ -5181,7 +5181,8 @@ static int meson_g12a_dvfs_setup_common(struct device
*dev,
/* Setup clock notifier for cpu_clk_dyn mux */
notifier_clk = devm_clk_hw_get_clk(dev, _cpu_clk_dyn.hw,
DVFS_CON_ID);
- ret = clk_notifier_register(notifier_clk, _cpu_clk_mux_nb);
+ ret = devm_clk_notifier_register(dev, notifier_clk,
+_cpu_clk_mux_nb);
if (ret) {
dev_err(dev, "failed to register the cpu_clk_dyn notifier\n");
return ret;
@@ -5207,7 +5208,8 @@ static int meson_g12b_dvfs_setup(struct platform_device
*pdev)
/* Setup clock notifier for cpu_clk mux */
notifier_clk = devm_clk_hw_get_clk(dev, _cpu_clk.hw,
DVFS_CON_ID);
- ret = clk_notifier_register(notifier_clk, _cpu_clk_mux_nb);
+ ret = devm_clk_notifier_register(dev, notifier_clk,
+_cpu_clk_mux_nb);
if (ret) {
dev_err(dev, "failed to register the cpu_clk notifier\n");
return ret;
@@ -5216,8 +5218,8 @@ static int meson_g12b_dvfs_setup(struct platform_device
*pdev)
/* Setup clock notifier for sys1_pll */
notifier_clk = devm_clk_hw_get_clk(dev, _sys1_pll.hw,
DVFS_CON_ID);
- ret = clk_notifier_register(notifier_clk,
- _cpu_clk_sys1_pll_nb_data.nb);
+ ret = devm_clk_notifier_register(dev, notifier_clk,
+_cpu_clk_sys1_pll_nb_data.nb);
if (ret) {
dev_err(dev, "failed to register the sys1_pll notifier\n");
return ret;
@@ -5229,8 +5231,8 @@ static int meson_g12b_dvfs_setup(struct platform_device
*pdev)
g12b_cpub_clk_postmux0_nb_data.xtal = xtal;
notifier_clk = devm_clk_hw_get_clk(dev, _cpub_clk_postmux0.hw,
DVFS_CON_ID);
- ret = clk_notifier_register(notifier_clk,
- _cpub_clk_postmux0_nb_data.nb);
+ ret = devm_clk_notifier_register(dev, notifier_clk,
+_cpub_clk_postmux0_nb_data.nb);
if (ret) {
dev_err(dev, "failed to register the cpub_clk_postmux0
notifier\n");
return ret;
@@ -5238,7 +5240,8 @@ static int meson_g12b_dvfs_setup(struct platform_device
*pdev)
/* Setup clock notifier for cpub_clk_dyn mux */
notifier_clk = devm_clk_hw_get_clk(dev, _cpub_clk_dyn.hw, "dvfs");
- ret = clk_notifier_register(notifier_clk, _cpu_clk_mux_nb);
+ ret = devm_clk_notifier_register(dev, notifier_clk,
+_cpu_clk_mux_nb);
if (ret) {
dev_err(dev, "failed to register the cpub_clk_dyn notifier\n");
return ret;
@@ -5246,7 +5249,8 @@ static int meson_g12b_dvfs_setup(struct platform_device
*pdev)
/* Setup clock notifier for cpub_clk mux */
notifier_clk = devm_clk_hw_get_clk(dev, _cpub_clk.hw, DVFS_CON_ID);
- ret = clk_notifier_register(notifier_clk, _cpu_clk_mux_nb);
+ ret = devm_clk_notifier_register(dev, notifier_clk,
+_cpu_clk_mux_nb);
if (ret) {
dev_err(dev, "failed to register the cpub_clk notifier\n");
return ret;
@@ -5254,8 +5258,8 @@ static int meson_
[PATCH 1/2] clk: add devm variant of clk_notifier_register
Add a memory managed variant of clk_notifier_register() to make life easier
on clock consumers using notifiers
Signed-off-by: Jerome Brunet
---
drivers/clk/clk.c | 36
include/linux/clk.h | 10 ++
2 files changed, 46 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index d27153f26fa9..e9fdd1d9b3f5 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4395,6 +4395,42 @@ int clk_notifier_unregister(struct clk *clk, struct
notifier_block *nb)
}
EXPORT_SYMBOL_GPL(clk_notifier_unregister);
+struct clk_notifier_devres {
+ struct clk *clk;
+ struct notifier_block *nb;
+};
+
+static void devm_clk_notifier_release(struct device *dev, void *res)
+{
+ struct clk_notifier_devres *devres = res;
+
+ clk_notifier_unregister(devres->clk, devres->nb);
+}
+
+int devm_clk_notifier_register(struct device *dev, struct clk *clk,
+ struct notifier_block *nb)
+{
+ struct clk_notifier_devres *devres;
+ int ret;
+
+ devres = devres_alloc(devm_clk_notifier_release,
+ sizeof(*devres), GFP_KERNEL);
+
+ if (!devres)
+ return -ENOMEM;
+
+ ret = clk_notifier_register(clk, nb);
+ if (!ret) {
+ devres->clk = clk;
+ devres->nb = nb;
+ } else {
+ devres_free(devres);
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(devm_clk_notifier_register);
+
#ifdef CONFIG_OF
static void clk_core_reparent_orphans(void)
{
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 7fd6a1febcf4..79fb52f93053 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -109,6 +109,16 @@ int clk_notifier_register(struct clk *clk, struct
notifier_block *nb);
*/
int clk_notifier_unregister(struct clk *clk, struct notifier_block *nb);
+/**
+ * devm_clk_notifier_register: register a managed rate-change notifier callback
+ * @dev: device for clock "consumer"
+ * @clk: clock whose rate we are interested in
+ * @nb: notifier block with callback function pointer
+ *
+ * Returns 0 on success, -EERROR otherwise
+ */
+int devm_clk_notifier_register(struct device *dev, struct clk *clk, struct
notifier_block *nb);
+
/**
* clk_get_accuracy - obtain the clock accuracy in ppb (parts per billion)
* for a clock source.
--
2.25.4
[PATCH 0/2] clk: add devm variant of clk_notifier_register
This patchset adds memory managed variant of clk_notifier_register and a first usage of it the amlogic clock controller of the g12 SoC family. Jerome Brunet (2): clk: add devm variant of clk_notifier_register clk: meson: g12: use devm variant to register notifiers drivers/clk/clk.c| 36 drivers/clk/meson/g12a.c | 34 -- include/linux/clk.h | 18 ++ 3 files changed, 74 insertions(+), 14 deletions(-) -- 2.25.4
[PATCH v2 1/3] clk: avoid devm_clk_release name clash
In clk-devres.c, devm_clk_release() is used to call clk_put() memory
managed clock. In clk.c the same name, in a different scope is used to call
clk_unregister().
As it stands, it is not really a problem but it does not readability,
especially if we need to call clk_put() on managed clock in clk.c
Signed-off-by: Jerome Brunet
---
drivers/clk/clk.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 0a9261a099bd..88e5797bb6b4 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4068,12 +4068,12 @@ void clk_hw_unregister(struct clk_hw *hw)
}
EXPORT_SYMBOL_GPL(clk_hw_unregister);
-static void devm_clk_release(struct device *dev, void *res)
+static void devm_clk_unregister_cb(struct device *dev, void *res)
{
clk_unregister(*(struct clk **)res);
}
-static void devm_clk_hw_release(struct device *dev, void *res)
+static void devm_clk_hw_unregister_cb(struct device *dev, void *res)
{
clk_hw_unregister(*(struct clk_hw **)res);
}
@@ -4093,7 +4093,7 @@ struct clk *devm_clk_register(struct device *dev, struct
clk_hw *hw)
struct clk *clk;
struct clk **clkp;
- clkp = devres_alloc(devm_clk_release, sizeof(*clkp), GFP_KERNEL);
+ clkp = devres_alloc(devm_clk_unregister_cb, sizeof(*clkp), GFP_KERNEL);
if (!clkp)
return ERR_PTR(-ENOMEM);
@@ -4123,7 +4123,7 @@ int devm_clk_hw_register(struct device *dev, struct
clk_hw *hw)
struct clk_hw **hwp;
int ret;
- hwp = devres_alloc(devm_clk_hw_release, sizeof(*hwp), GFP_KERNEL);
+ hwp = devres_alloc(devm_clk_hw_unregister_cb, sizeof(*hwp), GFP_KERNEL);
if (!hwp)
return -ENOMEM;
@@ -4167,7 +4167,7 @@ static int devm_clk_hw_match(struct device *dev, void
*res, void *data)
*/
void devm_clk_unregister(struct device *dev, struct clk *clk)
{
- WARN_ON(devres_release(dev, devm_clk_release, devm_clk_match, clk));
+ WARN_ON(devres_release(dev, devm_clk_unregister_cb, devm_clk_match,
clk));
}
EXPORT_SYMBOL_GPL(devm_clk_unregister);
@@ -4182,7 +4182,7 @@ EXPORT_SYMBOL_GPL(devm_clk_unregister);
*/
void devm_clk_hw_unregister(struct device *dev, struct clk_hw *hw)
{
- WARN_ON(devres_release(dev, devm_clk_hw_release, devm_clk_hw_match,
+ WARN_ON(devres_release(dev, devm_clk_hw_unregister_cb,
devm_clk_hw_match,
hw));
}
EXPORT_SYMBOL_GPL(devm_clk_hw_unregister);
--
2.25.4
[PATCH v2 3/3] clk: meson: g12: drop use of __clk_lookup()
g12 clock controller used __clk_lookup() to get struct clk from a
struct clk_hw. This type of hack is no longer required as CCF now provides
the necessary functions to get this.
Signed-off-by: Jerome Brunet
---
drivers/clk/meson/g12a.c | 68 +++-
1 file changed, 32 insertions(+), 36 deletions(-)
diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
index 28f976dbdd24..bbb75541dad9 100644
--- a/drivers/clk/meson/g12a.c
+++ b/drivers/clk/meson/g12a.c
@@ -5156,10 +5156,11 @@ static const struct reg_sequence g12a_init_regs[] = {
{ .reg = HHI_MPLL_CNTL0,.def = 0x0543 },
};
-static int meson_g12a_dvfs_setup_common(struct platform_device *pdev,
+#define DVFS_CON_ID "dvfs"
+
+static int meson_g12a_dvfs_setup_common(struct device *dev,
struct clk_hw **hws)
{
- const char *notifier_clk_name;
struct clk *notifier_clk;
struct clk_hw *xtal;
int ret;
@@ -5168,21 +5169,21 @@ static int meson_g12a_dvfs_setup_common(struct
platform_device *pdev,
/* Setup clock notifier for cpu_clk_postmux0 */
g12a_cpu_clk_postmux0_nb_data.xtal = xtal;
- notifier_clk_name = clk_hw_get_name(_cpu_clk_postmux0.hw);
- notifier_clk = __clk_lookup(notifier_clk_name);
+ notifier_clk = devm_clk_hw_get_clk(dev, _cpu_clk_postmux0.hw,
+ DVFS_CON_ID);
ret = clk_notifier_register(notifier_clk,
_cpu_clk_postmux0_nb_data.nb);
if (ret) {
- dev_err(>dev, "failed to register the cpu_clk_postmux0
notifier\n");
+ dev_err(dev, "failed to register the cpu_clk_postmux0
notifier\n");
return ret;
}
/* Setup clock notifier for cpu_clk_dyn mux */
- notifier_clk_name = clk_hw_get_name(_cpu_clk_dyn.hw);
- notifier_clk = __clk_lookup(notifier_clk_name);
+ notifier_clk = devm_clk_hw_get_clk(dev, _cpu_clk_dyn.hw,
+ DVFS_CON_ID);
ret = clk_notifier_register(notifier_clk, _cpu_clk_mux_nb);
if (ret) {
- dev_err(>dev, "failed to register the cpu_clk_dyn
notifier\n");
+ dev_err(dev, "failed to register the cpu_clk_dyn notifier\n");
return ret;
}
@@ -5192,33 +5193,33 @@ static int meson_g12a_dvfs_setup_common(struct
platform_device *pdev,
static int meson_g12b_dvfs_setup(struct platform_device *pdev)
{
struct clk_hw **hws = g12b_hw_onecell_data.hws;
- const char *notifier_clk_name;
+ struct device *dev = >dev;
struct clk *notifier_clk;
struct clk_hw *xtal;
int ret;
- ret = meson_g12a_dvfs_setup_common(pdev, hws);
+ ret = meson_g12a_dvfs_setup_common(dev, hws);
if (ret)
return ret;
xtal = clk_hw_get_parent_by_index(hws[CLKID_CPU_CLK_DYN1_SEL], 0);
/* Setup clock notifier for cpu_clk mux */
- notifier_clk_name = clk_hw_get_name(_cpu_clk.hw);
- notifier_clk = __clk_lookup(notifier_clk_name);
+ notifier_clk = devm_clk_hw_get_clk(dev, _cpu_clk.hw,
+ DVFS_CON_ID);
ret = clk_notifier_register(notifier_clk, _cpu_clk_mux_nb);
if (ret) {
- dev_err(>dev, "failed to register the cpu_clk
notifier\n");
+ dev_err(dev, "failed to register the cpu_clk notifier\n");
return ret;
}
/* Setup clock notifier for sys1_pll */
- notifier_clk_name = clk_hw_get_name(_sys1_pll.hw);
- notifier_clk = __clk_lookup(notifier_clk_name);
+ notifier_clk = devm_clk_hw_get_clk(dev, _sys1_pll.hw,
+ DVFS_CON_ID);
ret = clk_notifier_register(notifier_clk,
_cpu_clk_sys1_pll_nb_data.nb);
if (ret) {
- dev_err(>dev, "failed to register the sys1_pll
notifier\n");
+ dev_err(dev, "failed to register the sys1_pll notifier\n");
return ret;
}
@@ -5226,40 +5227,37 @@ static int meson_g12b_dvfs_setup(struct platform_device
*pdev)
/* Setup clock notifier for cpub_clk_postmux0 */
g12b_cpub_clk_postmux0_nb_data.xtal = xtal;
- notifier_clk_name = clk_hw_get_name(_cpub_clk_postmux0.hw);
- notifier_clk = __clk_lookup(notifier_clk_name);
+ notifier_clk = devm_clk_hw_get_clk(dev, _cpub_clk_postmux0.hw,
+ DVFS_CON_ID);
ret = clk_notifier_register(notifier_clk,
_cpub_clk_postmux0_nb_data.nb);
if (ret) {
- dev_err(>dev, "failed to register the cpub_clk_postmux0
notifier\n");
+ dev_err(dev, "fail
[PATCH v2 2/3] clk: add api to get clk consumer from clk_hw
clk_register() is deprecated. Using 'clk' member of struct clk_hw is
discouraged. With this constraint, it is difficult for driver to
register clocks using the clk_hw API and then use the clock with
the consumer API
This adds a simple helper, clk_hw_get_clk(), to get a struct clk from
a struct clk_hw. Like other clk_get() variant, each call to this helper
must be balanced with a call to clk_put(). To make life easier on the
consumers, a memory managed version is provided as well.
Cc: Martin Blumenstingl
Signed-off-by: Jerome Brunet
---
drivers/clk/clk.c| 61
include/linux/clk-provider.h | 5 +++
2 files changed, 66 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 88e5797bb6b4..d27153f26fa9 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3667,6 +3667,24 @@ struct clk *clk_hw_create_clk(struct device *dev, struct
clk_hw *hw,
return clk;
}
+/**
+ * clk_hw_get_clk: get clk consumer given an clk_hw
+ * @hw: clk_hw associated with the clk being consumed
+ * @con_id: connection ID string on device
+ *
+ * Returns: new clk consumer
+ * This is the function to be used by providers which need
+ * to get a consumer clk and act on the clock element
+ * Calls to this function must be balanced with calls clk_put()
+ */
+struct clk *clk_hw_get_clk(struct clk_hw *hw, const char *con_id)
+{
+ struct device *dev = hw->core->dev;
+
+ return clk_hw_create_clk(dev, hw, dev_name(dev), con_id);
+}
+EXPORT_SYMBOL(clk_hw_get_clk);
+
static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)
{
const char *dst;
@@ -4187,6 +4205,49 @@ void devm_clk_hw_unregister(struct device *dev, struct
clk_hw *hw)
}
EXPORT_SYMBOL_GPL(devm_clk_hw_unregister);
+static void devm_clk_release(struct device *dev, void *res)
+{
+ clk_put(*(struct clk **)res);
+}
+
+/**
+ * devm_clk_hw_get_clk: resource managed clk_hw_get_clk()
+ * @dev: device that is registering this clock
+ * @hw: clk_hw associated with the clk being consumed
+ * @con_id: connection ID string on device
+ *
+ * Managed clk_hw_get_clk(). Clocks got with this function are
+ * automatically clk_put() on driver detach. See clk_put()
+ * for more information.
+ */
+struct clk *devm_clk_hw_get_clk(struct device *dev, struct clk_hw *hw,
+ const char *con_id)
+{
+ struct clk *clk;
+ struct clk **clkp;
+
+ /* This should not happen because it would mean we have drivers
+* passing around clk_hw pointers instead of having the caller use
+* proper clk_get() style APIs
+*/
+ WARN_ON_ONCE(dev != hw->core->dev);
+
+ clkp = devres_alloc(devm_clk_release, sizeof(*clkp), GFP_KERNEL);
+ if (!clkp)
+ return ERR_PTR(-ENOMEM);
+
+ clk = clk_hw_get_clk(hw, con_id);
+ if (!IS_ERR(clk)) {
+ *clkp = clk;
+ devres_add(dev, clkp);
+ } else {
+ devres_free(clkp);
+ }
+
+ return clk;
+}
+EXPORT_SYMBOL_GPL(devm_clk_hw_get_clk);
+
/*
* clkdev helpers
*/
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 03a5de5f99f4..86b707520ec0 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -1088,6 +1088,11 @@ static inline struct clk_hw *__clk_get_hw(struct clk
*clk)
return (struct clk_hw *)clk;
}
#endif
+
+struct clk *clk_hw_get_clk(struct clk_hw *hw, const char *con_id);
+struct clk *devm_clk_hw_get_clk(struct device *dev, struct clk_hw *hw,
+ const char *con_id);
+
unsigned int clk_hw_get_num_parents(const struct clk_hw *hw);
struct clk_hw *clk_hw_get_parent(const struct clk_hw *hw);
struct clk_hw *clk_hw_get_parent_by_index(const struct clk_hw *hw,
--
2.25.4
[PATCH v2 0/3] clk: add api to get clk consumer from clk_hw
This patchset a call in CCF to get "struct clk*" from "struct clk_hw*" Changes since v1: [0] * Add a con_id string to help keep track of the consumer * Add devm variant: - Following our discussion on V1, I choose to have the dev as argument as most devm function do. However, as Stephen pointed out we don't expect this to differ from the one linked to clk_hw. In this case a warning is thrown. * Add a first usage of this in the amlogic clock driver. [0]: https://lore.kernel.org/r/20200519170440.294601-1-jbru...@baylibre.com Jerome Brunet (3): clk: avoid devm_clk_release name clash clk: add api to get clk consumer from clk_hw clk: meson: g12: drop use of __clk_lookup() drivers/clk/clk.c| 73 +--- drivers/clk/meson/g12a.c | 68 - include/linux/clk-provider.h | 5 +++ 3 files changed, 104 insertions(+), 42 deletions(-) -- 2.25.4
Re: [PATCH] arm64: meson: ship only the necessary clock controllers
On Tue 20 Oct 2020 at 17:03, Kevin Hilman wrote: > Jerome Brunet writes: > >> There now the menu entries for the amlogic clock controllers. >> Do not select these when ARM64 is enabled so it possible to ship only the >> required. >> >> Signed-off-by: Jerome Brunet >> --- >> arch/arm64/Kconfig.platforms | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms >> index cd58f8495c45..b22d1bdd6eb6 100644 >> --- a/arch/arm64/Kconfig.platforms >> +++ b/arch/arm64/Kconfig.platforms >> @@ -154,9 +154,6 @@ config ARCH_MESON >> bool "Amlogic Platforms" >> select PINCTRL >> select PINCTRL_MESON >> -select COMMON_CLK_GXBB >> -select COMMON_CLK_AXG >> -select COMMON_CLK_G12A > > This patch alone will break boot when using the default, upstream > defconfig because these options will all now be disabled and we'll have > no clock providers. > > I think you also need a default value (e.g. `default y`) in > drivers/clk/meson/Kconfig for each of these entries to keep the same > defaults. But these defaults could be overridden by SoC-specific > defconfigs leading to more flexibilty. > > So, assuming you queue up a drivers/clk patch to go in when this > lands... Indeed. Please wait till the clock PR lands during the merge window, you'll see that it is already taken care of. I was not expecting you to look at it so soon ;) > > Acked-by: Kevin Hilman
[PATCH] arm64: dts: amlogic: add missing ethernet reset ID
From: Anand Moon
Add reset external reset of the ethernet mac controller
Signed-off-by: Anand Moon
Signed-off-by: Jerome Brunet
---
arch/arm64/boot/dts/amlogic/meson-axg.dtsi| 2 ++
arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi | 2 ++
arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 3 +++
3 files changed, 7 insertions(+)
diff --git a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
index fae48efae83e..724ee179b316 100644
--- a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
@@ -227,6 +227,8 @@ ethmac: ethernet@ff3f {
"timing-adjustment";
rx-fifo-depth = <4096>;
tx-fifo-depth = <2048>;
+ resets = < RESET_ETHERNET>;
+ reset-names = "stmmaceth";
status = "disabled";
};
diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
index c95ebe615176..8514fe6a275a 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
@@ -224,6 +224,8 @@ ethmac: ethernet@ff3f {
"timing-adjustment";
rx-fifo-depth = <4096>;
tx-fifo-depth = <2048>;
+ resets = < RESET_ETHERNET>;
+ reset-names = "stmmaceth";
status = "disabled";
mdio0: mdio {
diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
index 0edd137151f8..726b91d3a905 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
@@ -13,6 +13,7 @@
#include
#include
#include
+#include
#include
/ {
@@ -575,6 +576,8 @@ ethmac: ethernet@c941 {
interrupt-names = "macirq";
rx-fifo-depth = <4096>;
tx-fifo-depth = <2048>;
+ resets = < RESET_ETHERNET>;
+ reset-names = "stmmaceth";
power-domains = < PWRC_GXBB_ETHERNET_MEM_ID>;
status = "disabled";
};
--
2.25.4
[PATCH] arm64: meson: ship only the necessary clock controllers
There now the menu entries for the amlogic clock controllers. Do not select these when ARM64 is enabled so it possible to ship only the required. Signed-off-by: Jerome Brunet --- arch/arm64/Kconfig.platforms | 3 --- 1 file changed, 3 deletions(-) diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms index cd58f8495c45..b22d1bdd6eb6 100644 --- a/arch/arm64/Kconfig.platforms +++ b/arch/arm64/Kconfig.platforms @@ -154,9 +154,6 @@ config ARCH_MESON bool "Amlogic Platforms" select PINCTRL select PINCTRL_MESON - select COMMON_CLK_GXBB - select COMMON_CLK_AXG - select COMMON_CLK_G12A select MESON_IRQ_GPIO help This enables support for the arm64 based Amlogic SoCs -- 2.25.4
[PATCH] usb: cdc-acm: fix cooldown mechanism
Commit a4e7279cd1d1 ("cdc-acm: introduce a cool down") is causing
regression if there is some USB error, such as -EPROTO.
This has been reported on some samples of the Odroid-N2 using the Combee II
Zibgee USB dongle.
> struct acm *acm = container_of(work, struct acm, work)
is incorrect in case of a delayed work and causes warnings, usually from
the workqueue:
> WARNING: CPU: 0 PID: 0 at kernel/workqueue.c:1474 __queue_work+0x480/0x528.
When this happens, USB eventually stops working completely after a while.
Also the ACM_ERROR_DELAY bit is never set, so the cooldown mechanism
previously introduced cannot be triggered and acm_submit_read_urb() is
never called.
This changes makes the cdc-acm driver use a single delayed work, fixing the
pointer arithmetic in acm_softint() and set the ACM_ERROR_DELAY when the
cooldown mechanism appear to be needed.
Fixes: a4e7279cd1d1 ("cdc-acm: introduce a cool down")
Reported-by: Pascal Vizeli
Cc: Oliver Neukum
Signed-off-by: Jerome Brunet
---
Hi,
I tried to fix problem introduced by
commit a4e7279cd1d1 ("cdc-acm: introduce a cool down") while keeping the
original intent, or at least what I understand of it.
A plain revert of the original commit also works for me.
drivers/usb/class/cdc-acm.c | 12 +---
drivers/usb/class/cdc-acm.h | 3 +--
2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index f1a9043bdfe5..172cd2d21914 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -507,6 +507,7 @@ static void acm_read_bulk_callback(struct urb *urb)
"%s - cooling babbling device\n", __func__);
usb_mark_last_busy(acm->dev);
set_bit(rb->index, >urbs_in_error_delay);
+ set_bit(ACM_ERROR_DELAY, >flags);
cooldown = true;
break;
default:
@@ -532,7 +533,7 @@ static void acm_read_bulk_callback(struct urb *urb)
if (stopped || stalled || cooldown) {
if (stalled)
- schedule_work(>work);
+ schedule_delayed_work(>dwork, 0);
else if (cooldown)
schedule_delayed_work(>dwork, HZ / 2);
return;
@@ -562,13 +563,13 @@ static void acm_write_bulk(struct urb *urb)
acm_write_done(acm, wb);
spin_unlock_irqrestore(>write_lock, flags);
set_bit(EVENT_TTY_WAKEUP, >flags);
- schedule_work(>work);
+ schedule_delayed_work(>dwork, 0);
}
static void acm_softint(struct work_struct *work)
{
int i;
- struct acm *acm = container_of(work, struct acm, work);
+ struct acm *acm = container_of(work, struct acm, dwork.work);
if (test_bit(EVENT_RX_STALL, >flags)) {
smp_mb(); /* against acm_suspend() */
@@ -584,7 +585,7 @@ static void acm_softint(struct work_struct *work)
if (test_and_clear_bit(ACM_ERROR_DELAY, >flags)) {
for (i = 0; i < acm->rx_buflimit; i++)
if (test_and_clear_bit(i, >urbs_in_error_delay))
- acm_submit_read_urb(acm, i, GFP_NOIO);
+ acm_submit_read_urb(acm, i, GFP_KERNEL);
}
if (test_and_clear_bit(EVENT_TTY_WAKEUP, >flags))
@@ -1352,7 +1353,6 @@ static int acm_probe(struct usb_interface *intf,
acm->ctrlsize = ctrlsize;
acm->readsize = readsize;
acm->rx_buflimit = num_rx_buf;
- INIT_WORK(>work, acm_softint);
INIT_DELAYED_WORK(>dwork, acm_softint);
init_waitqueue_head(>wioctl);
spin_lock_init(>write_lock);
@@ -1562,7 +1562,6 @@ static void acm_disconnect(struct usb_interface *intf)
}
acm_kill_urbs(acm);
- cancel_work_sync(>work);
cancel_delayed_work_sync(>dwork);
tty_unregister_device(acm_tty_driver, acm->minor);
@@ -1605,7 +1604,6 @@ static int acm_suspend(struct usb_interface *intf,
pm_message_t message)
return 0;
acm_kill_urbs(acm);
- cancel_work_sync(>work);
cancel_delayed_work_sync(>dwork);
acm->urbs_in_error_delay = 0;
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index cd5e9d8ab237..b95ff769072e 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -112,8 +112,7 @@ struct acm {
# define ACM_ERROR_DELAY 3
unsigned long urbs_in_error_delay; /* these need to be
restarted after a delay */
struct usb_cdc_line_coding line;/* bits, stop, parity */
- struct work_struct work;/* work queue entry for
various purposes*/
- struct delayed_work dwork; /* for cool downs
needed in error reco
Re: [PATCH] mailbox: avoid timer start from callback
On Fri 16 Oct 2020 at 20:45, Jassi Brar wrote:
> On Fri, Oct 16, 2020 at 1:38 PM Jerome Brunet wrote:
>>
>>
>> On Fri 16 Oct 2020 at 19:30, jassisinghb...@gmail.com wrote:
>>
>> > From: Jassi Brar
>> >
>> > If the txdone is done by polling, it is possible for msg_submit() to start
>> > the timer while txdone_hrtimer() callback is running. If the timer needs
>> > recheduling, it could already be enqueued by the time hrtimer_forward_now()
>> > is called, leading hrtimer to loudly complain.
>> >
>> > WARNING: CPU: 3 PID: 74 at kernel/time/hrtimer.c:932
>> > hrtimer_forward+0xc4/0x110
>> > CPU: 3 PID: 74 Comm: kworker/u8:1 Not tainted
>> > 5.9.0-rc2-00236-gd3520067d01c-dirty #5
>> > Hardware name: Libre Computer AML-S805X-AC (DT)
>> > Workqueue: events_freezable_power_ thermal_zone_device_check
>> > pstate: 2085 (nzCv daIf -PAN -UAO BTYPE=--)
>> > pc : hrtimer_forward+0xc4/0x110
>> > lr : txdone_hrtimer+0xf8/0x118
>> > [...]
>> >
>> > This can be fixed by not starting the timer from the callback path. Which
>> > requires the timer reloading as long as any message is queued on the
>> > channel, and not just when current tx is not done yet.
>> >
>> > Signed-off-by: Jassi Brar
>> > ---
>> > drivers/mailbox/mailbox.c | 12 +++-
>> > 1 file changed, 7 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>> > index 0b821a5b2db8..a093a6ecaa66 100644
>> > --- a/drivers/mailbox/mailbox.c
>> > +++ b/drivers/mailbox/mailbox.c
>> > @@ -82,9 +82,12 @@ static void msg_submit(struct mbox_chan *chan)
>> > exit:
>> > spin_unlock_irqrestore(>lock, flags);
>> >
>> > - if (!err && (chan->txdone_method & TXDONE_BY_POLL))
>> > - /* kick start the timer immediately to avoid delays */
>> > - hrtimer_start(>mbox->poll_hrt, 0, HRTIMER_MODE_REL);
>> > + /* kick start the timer immediately to avoid delays */
>> > + if (!err && (chan->txdone_method & TXDONE_BY_POLL)) {
>> > + /* but only if not already active */
>>
>> It would solve the problem I reported as well but instead of running the
>> check immediately (timer with value 0), we will have to wait for the
>> next of the timer, it is already started. IOW, there might be a delay
>> now. I don't know if this important for the mailbox - the existing
>> comments in the code suggested it was.
>>
> That comment is for when the first message is queued on the channel,
> which remains unimpacted.
> So, do I have your tested/acked by ?
Sure go ahead
Acked-by: Jerome Brunet
Tested-by: Jerome Brunet
>
> thnx,
Re: [PATCH] mailbox: cancel timer before starting it
On Fri 16 Oct 2020 at 19:33, Jassi Brar wrote:
> On Fri, Oct 16, 2020 at 4:00 AM Jerome Brunet wrote:
>>
>>
>> On Fri 16 Oct 2020 at 10:52, Ionela Voinescu wrote:
>>
>> > On Thursday 15 Oct 2020 at 13:45:54 (-0500), Jassi Brar wrote:
>> > [..]
>> >> > >> --- a/drivers/mailbox/mailbox.c
>> >> > >> +++ b/drivers/mailbox/mailbox.c
>> >> > >> @@ -82,9 +82,13 @@ static void msg_submit(struct mbox_chan *chan)
>> >> > >> exit:
>> >> > >> spin_unlock_irqrestore(>lock, flags);
>> >> > >>
>> >> > >> -if (!err && (chan->txdone_method & TXDONE_BY_POLL))
>> >> > >> -/* kick start the timer immediately to avoid delays */
>> >> > >> +if (!err && (chan->txdone_method & TXDONE_BY_POLL)) {
>> >> > >> +/* Disable the timer if already active ... */
>> >> > >> +hrtimer_cancel(>mbox->poll_hrt);
>> >> > >> +
>> >> > >> +/* ... and kick start it immediately to avoid delays */
>> >> > >> hrtimer_start(>mbox->poll_hrt, 0,
>> >> > >> HRTIMER_MODE_REL);
>> >> > >> +}
>> >> > >> }
>> >> > >>
>> >> > >> static void tx_tick(struct mbox_chan *chan, int r)
>> >> > >
>> >> > > I've tracked a regression back to this commit. Details to reproduce:
>> >> >
>> >> > Hi Ionela,
>> >> >
>> >> > I don't have access to your platform and I don't get what is going on
>> >> > from the log below.
>> >> >
>> >> > Could you please give us a bit more details about what is going on ?
>> >> >
>> >> > All this patch does is add hrtimer_cancel().
>> >> > * It is needed if the timer had already been started, which is
>> >> > appropriate AFAIU
>> >> > * It is a NO-OP is the timer is not active.
>> >> >
>> >> Can you please try using hrtimer_try_to_cancel() instead ?
>> >>
>> >
>> > Yes, using hrtimer_try_to_cancel() instead works for me. But doesn't
>> > this limit how effective this change is? AFAIU, this will possibly only
>> > reduce the chances for the race condition, but not solve it.
>> >
>>
>> It is also my understanding, hrtimer_try_to_cancel() would remove a
>> timer which as not already started but would return withtout doing
>> anything if the callback is already running ... which is the original
>> problem
>>
> If we are running in the callback path, hrtimer_try_to_cancel will
> return -1, in which case we could skip hrtimer_start.
> Anyways, I think simply checking for hrtimer_active should effect the same.
> I have submitted a patch, of course not tested.
Yes it sloves this race but ...
If a race is possible between a timer callback rescheduling itself (which
is not that uncommon) and another thread trying to cancel it, maybe
there is something worth fixing in hrtimer ? Also, mailbox calls
hrtimer_cancel() in unregister ... are we confident this would work ?
Any fix is by me - yours avoid killing and restarting the timer :) but
it feels like we are working around an issue that might bite us back
later on.
>
> Thanks
Re: [PATCH] mailbox: avoid timer start from callback
On Fri 16 Oct 2020 at 19:30, jassisinghb...@gmail.com wrote:
> From: Jassi Brar
>
> If the txdone is done by polling, it is possible for msg_submit() to start
> the timer while txdone_hrtimer() callback is running. If the timer needs
> recheduling, it could already be enqueued by the time hrtimer_forward_now()
> is called, leading hrtimer to loudly complain.
>
> WARNING: CPU: 3 PID: 74 at kernel/time/hrtimer.c:932
> hrtimer_forward+0xc4/0x110
> CPU: 3 PID: 74 Comm: kworker/u8:1 Not tainted
> 5.9.0-rc2-00236-gd3520067d01c-dirty #5
> Hardware name: Libre Computer AML-S805X-AC (DT)
> Workqueue: events_freezable_power_ thermal_zone_device_check
> pstate: 2085 (nzCv daIf -PAN -UAO BTYPE=--)
> pc : hrtimer_forward+0xc4/0x110
> lr : txdone_hrtimer+0xf8/0x118
> [...]
>
> This can be fixed by not starting the timer from the callback path. Which
> requires the timer reloading as long as any message is queued on the
> channel, and not just when current tx is not done yet.
>
> Signed-off-by: Jassi Brar
> ---
> drivers/mailbox/mailbox.c | 12 +++-
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 0b821a5b2db8..a093a6ecaa66 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -82,9 +82,12 @@ static void msg_submit(struct mbox_chan *chan)
> exit:
> spin_unlock_irqrestore(>lock, flags);
>
> - if (!err && (chan->txdone_method & TXDONE_BY_POLL))
> - /* kick start the timer immediately to avoid delays */
> - hrtimer_start(>mbox->poll_hrt, 0, HRTIMER_MODE_REL);
> + /* kick start the timer immediately to avoid delays */
> + if (!err && (chan->txdone_method & TXDONE_BY_POLL)) {
> + /* but only if not already active */
It would solve the problem I reported as well but instead of running the
check immediately (timer with value 0), we will have to wait for the
next of the timer, it is already started. IOW, there might be a delay
now. I don't know if this important for the mailbox - the existing
comments in the code suggested it was.
> + if (!hrtimer_active(>mbox->poll_hrt))
> + hrtimer_start(>mbox->poll_hrt, 0,
> HRTIMER_MODE_REL);
> + }
> }
>
> static void tx_tick(struct mbox_chan *chan, int r)
> @@ -122,11 +125,10 @@ static enum hrtimer_restart txdone_hrtimer(struct
> hrtimer *hrtimer)
> struct mbox_chan *chan = >chans[i];
>
> if (chan->active_req && chan->cl) {
> + resched = true;
> txdone = chan->mbox->ops->last_tx_done(chan);
> if (txdone)
> tx_tick(chan, 0);
> - else
> - resched = true;
> }
> }
Re: [PATCH] mailbox: cancel timer before starting it
On Fri 16 Oct 2020 at 10:44, Sudeep Holla wrote:
> On Thu, Oct 15, 2020 at 03:29:35PM +0100, Ionela Voinescu wrote:
>> Hi Jerome,
>>
>> On Thursday 15 Oct 2020 at 15:58:30 (+0200), Jerome Brunet wrote:
>> >
>> > On Thu 15 Oct 2020 at 15:46, Ionela Voinescu
>> > wrote:
>> >
>> > > Hi guys,
>> > >
>> > > On Wednesday 23 Sep 2020 at 14:39:16 (+0200), Jerome Brunet wrote:
>> > >> If the txdone is done by polling, it is possible for msg_submit() to
>> > >> start
>> > >> the timer while txdone_hrtimer() callback is running. If the timer needs
>> > >> recheduling, it could already be enqueued by the time
>> > >> hrtimer_forward_now()
>> > >> is called, leading hrtimer to loudly complain.
>> > >>
>> > >> WARNING: CPU: 3 PID: 74 at kernel/time/hrtimer.c:932
>> > >> hrtimer_forward+0xc4/0x110
>> > >> CPU: 3 PID: 74 Comm: kworker/u8:1 Not tainted
>> > >> 5.9.0-rc2-00236-gd3520067d01c-dirty #5
>> > >> Hardware name: Libre Computer AML-S805X-AC (DT)
>> > >> Workqueue: events_freezable_power_ thermal_zone_device_check
>> > >> pstate: 2085 (nzCv daIf -PAN -UAO BTYPE=--)
>> > >> pc : hrtimer_forward+0xc4/0x110
>> > >> lr : txdone_hrtimer+0xf8/0x118
>> > >> [...]
>> > >>
>> > >> Canceling the timer before starting it ensure that the timer callback is
>> > >> not running when the timer is started, solving this race condition.
>> > >>
>> > >> Fixes: 0cc67945ea59 ("mailbox: switch to hrtimer for tx_complete
>> > >> polling")
>> > >> Reported-by: Da Xue
>> > >> Signed-off-by: Jerome Brunet
>> > >> ---
>> > >> drivers/mailbox/mailbox.c | 8 ++--
>> > >> 1 file changed, 6 insertions(+), 2 deletions(-)
>> > >>
>> > >> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>> > >> index 0b821a5b2db8..34f9ab01caef 100644
>> > >> --- a/drivers/mailbox/mailbox.c
>> > >> +++ b/drivers/mailbox/mailbox.c
>> > >> @@ -82,9 +82,13 @@ static void msg_submit(struct mbox_chan *chan)
>> > >> exit:
>> > >> spin_unlock_irqrestore(>lock, flags);
>> > >>
>> > >> - if (!err && (chan->txdone_method & TXDONE_BY_POLL))
>> > >> - /* kick start the timer immediately to avoid delays */
>> > >> + if (!err && (chan->txdone_method & TXDONE_BY_POLL)) {
>> > >> + /* Disable the timer if already active ... */
>> > >> + hrtimer_cancel(>mbox->poll_hrt);
>> > >> +
>> > >> + /* ... and kick start it immediately to avoid delays */
>> > >> hrtimer_start(>mbox->poll_hrt, 0,
>> > >> HRTIMER_MODE_REL);
>> > >> + }
>> > >> }
>> > >>
>> > >> static void tx_tick(struct mbox_chan *chan, int r)
>> > >
>> > > I've tracked a regression back to this commit. Details to reproduce:
>> >
>> > Hi Ionela,
>> >
>> > I don't have access to your platform and I don't get what is going on
>> > from the log below.
>> >
>> > Could you please give us a bit more details about what is going on ?
>> >
>>
>> I'm not familiar with the mailbox subsystem, so the best I can do right
>> now is to add Sudeep to Cc, in case this conflicts in some way with the
>> ARM MHU patches [1].
>>
>
> Not it can't be doorbell driver as we use SCPI(old firmware) with upstream
> MHU driver as is limiting the number of channels to be used.
>
>> In the meantime I'll get some traces and get more familiar with the
>> code.
>>
>
> I will try that too.
BTW, this issue was originally reported on amlogic platforms which also
use arm,mhu mailbox driver.
Re: [PATCH] mailbox: cancel timer before starting it
On Fri 16 Oct 2020 at 10:52, Ionela Voinescu wrote:
> On Thursday 15 Oct 2020 at 13:45:54 (-0500), Jassi Brar wrote:
> [..]
>> > >> --- a/drivers/mailbox/mailbox.c
>> > >> +++ b/drivers/mailbox/mailbox.c
>> > >> @@ -82,9 +82,13 @@ static void msg_submit(struct mbox_chan *chan)
>> > >> exit:
>> > >> spin_unlock_irqrestore(>lock, flags);
>> > >>
>> > >> -if (!err && (chan->txdone_method & TXDONE_BY_POLL))
>> > >> -/* kick start the timer immediately to avoid delays */
>> > >> +if (!err && (chan->txdone_method & TXDONE_BY_POLL)) {
>> > >> +/* Disable the timer if already active ... */
>> > >> +hrtimer_cancel(>mbox->poll_hrt);
>> > >> +
>> > >> +/* ... and kick start it immediately to avoid delays */
>> > >> hrtimer_start(>mbox->poll_hrt, 0, HRTIMER_MODE_REL);
>> > >> +}
>> > >> }
>> > >>
>> > >> static void tx_tick(struct mbox_chan *chan, int r)
>> > >
>> > > I've tracked a regression back to this commit. Details to reproduce:
>> >
>> > Hi Ionela,
>> >
>> > I don't have access to your platform and I don't get what is going on
>> > from the log below.
>> >
>> > Could you please give us a bit more details about what is going on ?
>> >
>> > All this patch does is add hrtimer_cancel().
>> > * It is needed if the timer had already been started, which is
>> > appropriate AFAIU
>> > * It is a NO-OP is the timer is not active.
>> >
>> Can you please try using hrtimer_try_to_cancel() instead ?
>>
>
> Yes, using hrtimer_try_to_cancel() instead works for me. But doesn't
> this limit how effective this change is? AFAIU, this will possibly only
> reduce the chances for the race condition, but not solve it.
>
It is also my understanding, hrtimer_try_to_cancel() would remove a
timer which as not already started but would return withtout doing
anything if the callback is already running ... which is the original
problem
> Thanks,
> Ionela.
>
>> thanks
Re: [PATCH] mailbox: cancel timer before starting it
On Thu 15 Oct 2020 at 15:46, Ionela Voinescu wrote:
> Hi guys,
>
> On Wednesday 23 Sep 2020 at 14:39:16 (+0200), Jerome Brunet wrote:
>> If the txdone is done by polling, it is possible for msg_submit() to start
>> the timer while txdone_hrtimer() callback is running. If the timer needs
>> recheduling, it could already be enqueued by the time hrtimer_forward_now()
>> is called, leading hrtimer to loudly complain.
>>
>> WARNING: CPU: 3 PID: 74 at kernel/time/hrtimer.c:932
>> hrtimer_forward+0xc4/0x110
>> CPU: 3 PID: 74 Comm: kworker/u8:1 Not tainted
>> 5.9.0-rc2-00236-gd3520067d01c-dirty #5
>> Hardware name: Libre Computer AML-S805X-AC (DT)
>> Workqueue: events_freezable_power_ thermal_zone_device_check
>> pstate: 2085 (nzCv daIf -PAN -UAO BTYPE=--)
>> pc : hrtimer_forward+0xc4/0x110
>> lr : txdone_hrtimer+0xf8/0x118
>> [...]
>>
>> Canceling the timer before starting it ensure that the timer callback is
>> not running when the timer is started, solving this race condition.
>>
>> Fixes: 0cc67945ea59 ("mailbox: switch to hrtimer for tx_complete polling")
>> Reported-by: Da Xue
>> Signed-off-by: Jerome Brunet
>> ---
>> drivers/mailbox/mailbox.c | 8 ++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
>> index 0b821a5b2db8..34f9ab01caef 100644
>> --- a/drivers/mailbox/mailbox.c
>> +++ b/drivers/mailbox/mailbox.c
>> @@ -82,9 +82,13 @@ static void msg_submit(struct mbox_chan *chan)
>> exit:
>> spin_unlock_irqrestore(>lock, flags);
>>
>> -if (!err && (chan->txdone_method & TXDONE_BY_POLL))
>> -/* kick start the timer immediately to avoid delays */
>> +if (!err && (chan->txdone_method & TXDONE_BY_POLL)) {
>> +/* Disable the timer if already active ... */
>> +hrtimer_cancel(>mbox->poll_hrt);
>> +
>> +/* ... and kick start it immediately to avoid delays */
>> hrtimer_start(>mbox->poll_hrt, 0, HRTIMER_MODE_REL);
>> +}
>> }
>>
>> static void tx_tick(struct mbox_chan *chan, int r)
>
> I've tracked a regression back to this commit. Details to reproduce:
Hi Ionela,
I don't have access to your platform and I don't get what is going on
from the log below.
Could you please give us a bit more details about what is going on ?
All this patch does is add hrtimer_cancel().
* It is needed if the timer had already been started, which is
appropriate AFAIU
* It is a NO-OP is the timer is not active.
>
>
> - HEAD: (linux-next)
>* 62c04453381e Jerome Brunet 3 weeks ago mailbox: cancel timer before
> starting it
>
> - Platform: arm64 Juno R0 and Juno R2 [1]
>
> - Partial log:
> [0.00] Booting Linux on physical CPU 0x000100 [0x410fd030]
> [0.00] Linux version 5.9.0-rc8-01722-g62c04453381e ()
> (aarch64-none-linux-gnu-gcc (GNU Toolchain for the A-profile Architecture
> 9.2-2019.12 (arm-9.10)) 9.2.1 20191025, GNU ld (GNU Toolchain for the
> A-profile Architecture 9.2-2019.12 (arm-9.10)) 2.33.1.20191209) #175 SMP
> PREEMPT Thu Oct 15 14:17:41 BST 2020
> [0.00] Machine model: ARM Juno development board (r0)
> [..]
> [1.714340] mhu 2b1f.mhu: ARM MHU Mailbox registered
> [1.722768] NET: Registered protocol family 17
> [1.727364] 9pnet: Installing 9P2000 support
> [1.731689] Key type dns_resolver registered
> [1.735474] usb 1-1: new high-speed USB device number 2 using
> ehci-platform
> [1.736407] registered taskstats version 1
> [1.747061] Loading compiled-in X.509 certificates
> [1.755885] scpi_protocol scpi: SCP Protocol 1.2 Firmware 1.21.0
> version
> [1.770484] cpu cpu0: EM: created perf domain
> [1.778505] cpu cpu1: EM: created perf domain
> [1.807449] scpi_clocks scpi:clocks: failed to register clock
> 'pxlclk'
> [1.897593] hub 1-1:1.0: USB hub found
> [1.901656] hub 1-1:1.0: 4 ports detected
> [2.559453] atkbd serio0: keyboard reset failed on 1c06.kmi
> [ 22.787431] rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
> [ 22.793536] rcu: 1-...0: (1 ticks this GP)
> idle=222/1/0x4002 softirq=63/64 fqs=2626
> [ 22.802421] (detected by 2, t=5255 jiffies, g=-991, q=9)
> [ 22.807823] Task dump for CPU 1:
> [ 22.811049] task:swapper/1 state:R running task stack:
> 0 pid:0 ppid: 1 flags:0x002a
>
Re: [PATCH] mmc: meson-gx: remove IRQF_ONESHOT
On Tue 06 Oct 2020 at 17:33, Thomas Gleixner wrote: > Brad, > > On Wed, Oct 07 2020 at 00:45, Brad Harper wrote: >> I'm happy to test anything on a range of amlogic hardware with standard >> / rt and multiple mmc devices. Ill test Jerome's patch in next 24 >> hours to report the results. > > please do not top-post and trim your replies. > >> On 6/10/2020 11:43 pm, Thomas Gleixner wrote: >>> We rather should make interrupts which need to have their primary >>> handler in hard interrupt context to set IRQF_NO_THREAD. That >>> should at the same time confirm that the primary handler is RT >>> safe. >>> >>> Let me stare at the core code and the actual usage sites some more. > > So there are a few nasties in there and I faintly remember that there > was an assumption that interrupts which are requested with both a > primary and a secondary handler should quiesce the device interrupt in > the primary handler if needed. OTOH, this also enforces that the primary > handler is RT safe, which is after a quick scan of all the usage sites > not a given and quite some of the users rely on IRQF_ONESHOT. > > The below untested patch should cure the problem and keep the interrupt > line masked if requested with IRQF_ONESHOT. > With arm64 defconfig on Khadas vim3, no obvious regression. Looks good. Tested-by: Jerome Brunet I did not test with RT. Brad, Could you let us know is Thomas's patch works for you ? Thx
[PATCH 0/3] i2c: meson: scl rate fixups
This patchset fixes various issues related to SCL rate on AML SoCs. We retain the method which was used so far to set the SCL rate. This method does not provide manual control of the clock duty cycle but so far it does seems to be a problem for anyone. Amlogic vendor kernel source uses "HIGH/LOW" method which allows to set the rate and the duty cycle of the clock. However the documentation around this method could be better and the result on actual HW is not perfectly aligned with the comments in AML code. In case the current method ever becomes a problem, we might consider switching to this HIGH/LOW method. Jerome Brunet (2): i2c: meson: fix clock setting overwrite i2c: meson: keep peripheral clock enabled Nicolas Belin (1): i2c: meson: fixup rate calculation with filter delay drivers/i2c/busses/i2c-meson.c | 52 +- 1 file changed, 33 insertions(+), 19 deletions(-) -- 2.25.4
[PATCH 3/3] i2c: meson: fixup rate calculation with filter delay
From: Nicolas Belin
Apparently, 15 cycles of the peripheral clock are used by the controller
for sampling and filtering. Because this was not known before, the rate
calculation is slightly off.
Clean up and fix the calculation taking this filtering delay into account.
Fixes: 30021e3707a7 ("i2c: add support for Amlogic Meson I2C controller")
Signed-off-by: Nicolas Belin
Signed-off-by: Jerome Brunet
---
drivers/i2c/busses/i2c-meson.c | 23 ---
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index e7ec2ab2a220..ef73a42577cc 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -34,10 +34,8 @@
#define REG_CTRL_ACK_IGNOREBIT(1)
#define REG_CTRL_STATUSBIT(2)
#define REG_CTRL_ERROR BIT(3)
-#define REG_CTRL_CLKDIV_SHIFT 12
-#define REG_CTRL_CLKDIV_MASK GENMASK(21, 12)
-#define REG_CTRL_CLKDIVEXT_SHIFT 28
-#define REG_CTRL_CLKDIVEXT_MASKGENMASK(29, 28)
+#define REG_CTRL_CLKDIVGENMASK(21, 12)
+#define REG_CTRL_CLKDIVEXT GENMASK(29, 28)
#define REG_SLV_ADDR GENMASK(7, 0)
#define REG_SLV_SDA_FILTER GENMASK(10, 8)
@@ -46,6 +44,7 @@
#define REG_SLV_SCL_LOW_EN BIT(28)
#define I2C_TIMEOUT_MS 500
+#define FILTER_DELAY 15
enum {
TOKEN_END = 0,
@@ -140,19 +139,21 @@ static void meson_i2c_set_clk_div(struct meson_i2c *i2c,
unsigned int freq)
unsigned long clk_rate = clk_get_rate(i2c->clk);
unsigned int div;
- div = DIV_ROUND_UP(clk_rate, freq * i2c->data->div_factor);
+ div = DIV_ROUND_UP(clk_rate, freq);
+ div -= FILTER_DELAY;
+ div = DIV_ROUND_UP(div, i2c->data->div_factor);
/* clock divider has 12 bits */
- if (div >= (1 << 12)) {
+ if (div > GENMASK(11, 0)) {
dev_err(i2c->dev, "requested bus frequency too low\n");
- div = (1 << 12) - 1;
+ div = GENMASK(11, 0);
}
- meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIV_MASK,
- (div & GENMASK(9, 0)) << REG_CTRL_CLKDIV_SHIFT);
+ meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIV,
+ FIELD_PREP(REG_CTRL_CLKDIV, div & GENMASK(9, 0)));
- meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIVEXT_MASK,
- (div >> 10) << REG_CTRL_CLKDIVEXT_SHIFT);
+ meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIVEXT,
+ FIELD_PREP(REG_CTRL_CLKDIVEXT, div >> 10));
/* Disable HIGH/LOW mode */
meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_SCL_LOW_EN, 0);
--
2.25.4
[PATCH 1/3] i2c: meson: fix clock setting overwrite
When the slave address is written in do_start(), SLAVE_ADDR is written
completely. This may overwrite some setting related to the clock rate
or signal filtering.
Fix this by writing only the bits related to slave address. To avoid
causing unexpected changed, explicitly disable filtering or high/low
clock mode which may have been left over by the bootloader.
Fixes: 30021e3707a7 ("i2c: add support for Amlogic Meson I2C controller")
Signed-off-by: Jerome Brunet
---
drivers/i2c/busses/i2c-meson.c | 19 ++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index c5dec572fc48..dac0d2a00cec 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -5,6 +5,7 @@
* Copyright (C) 2014 Beniamino Galvani
*/
+#include
#include
#include
#include
@@ -38,6 +39,12 @@
#define REG_CTRL_CLKDIVEXT_SHIFT 28
#define REG_CTRL_CLKDIVEXT_MASKGENMASK(29, 28)
+#define REG_SLV_ADDR GENMASK(7, 0)
+#define REG_SLV_SDA_FILTER GENMASK(10, 8)
+#define REG_SLV_SCL_FILTER GENMASK(13, 11)
+#define REG_SLV_SCL_LOWGENMASK(27, 16)
+#define REG_SLV_SCL_LOW_EN BIT(28)
+
#define I2C_TIMEOUT_MS 500
enum {
@@ -147,6 +154,9 @@ static void meson_i2c_set_clk_div(struct meson_i2c *i2c,
unsigned int freq)
meson_i2c_set_mask(i2c, REG_CTRL, REG_CTRL_CLKDIVEXT_MASK,
(div >> 10) << REG_CTRL_CLKDIVEXT_SHIFT);
+ /* Disable HIGH/LOW mode */
+ meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_SCL_LOW_EN, 0);
+
dev_dbg(i2c->dev, "%s: clk %lu, freq %u, div %u\n", __func__,
clk_rate, freq, div);
}
@@ -280,7 +290,10 @@ static void meson_i2c_do_start(struct meson_i2c *i2c,
struct i2c_msg *msg)
token = (msg->flags & I2C_M_RD) ? TOKEN_SLAVE_ADDR_READ :
TOKEN_SLAVE_ADDR_WRITE;
- writel(msg->addr << 1, i2c->regs + REG_SLAVE_ADDR);
+
+ meson_i2c_set_mask(i2c, REG_SLAVE_ADDR, REG_SLV_ADDR,
+ FIELD_PREP(REG_SLV_ADDR, msg->addr << 1));
+
meson_i2c_add_token(i2c, TOKEN_START);
meson_i2c_add_token(i2c, token);
}
@@ -461,6 +474,10 @@ static int meson_i2c_probe(struct platform_device *pdev)
return ret;
}
+ /* Disable filtering */
+ meson_i2c_set_mask(i2c, REG_SLAVE_ADDR,
+ REG_SLV_SDA_FILTER | REG_SLV_SCL_FILTER, 0);
+
meson_i2c_set_clk_div(i2c, timings.bus_freq_hz);
return 0;
--
2.25.4
[PATCH 2/3] i2c: meson: keep peripheral clock enabled
SCL rate appears to be different than what is expected. For example,
We get 164kHz on i2c3 of the vim3 when 400kHz is expected. This is
partially due to the peripheral clock being disabled when the clock is
set.
Let's keep the peripheral clock on after probe to fix the problem. This
does not affect the SCL output which is still gated when i2c is idle.
Fixes: 09af1c2fa490 ("i2c: meson: set clock divider in probe instead of setting
it for each transfer")
Signed-off-by: Jerome Brunet
---
drivers/i2c/busses/i2c-meson.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-meson.c b/drivers/i2c/busses/i2c-meson.c
index dac0d2a00cec..e7ec2ab2a220 100644
--- a/drivers/i2c/busses/i2c-meson.c
+++ b/drivers/i2c/busses/i2c-meson.c
@@ -370,16 +370,12 @@ static int meson_i2c_xfer_messages(struct i2c_adapter
*adap,
struct meson_i2c *i2c = adap->algo_data;
int i, ret = 0;
- clk_enable(i2c->clk);
-
for (i = 0; i < num; i++) {
ret = meson_i2c_xfer_msg(i2c, msgs + i, i == num - 1, atomic);
if (ret)
break;
}
- clk_disable(i2c->clk);
-
return ret ?: i;
}
@@ -448,7 +444,7 @@ static int meson_i2c_probe(struct platform_device *pdev)
return ret;
}
- ret = clk_prepare(i2c->clk);
+ ret = clk_prepare_enable(i2c->clk);
if (ret < 0) {
dev_err(>dev, "can't prepare clock\n");
return ret;
@@ -470,7 +466,7 @@ static int meson_i2c_probe(struct platform_device *pdev)
ret = i2c_add_adapter(>adap);
if (ret < 0) {
- clk_unprepare(i2c->clk);
+ clk_disable_unprepare(i2c->clk);
return ret;
}
@@ -488,7 +484,7 @@ static int meson_i2c_remove(struct platform_device *pdev)
struct meson_i2c *i2c = platform_get_drvdata(pdev);
i2c_del_adapter(>adap);
- clk_unprepare(i2c->clk);
+ clk_disable_unprepare(i2c->clk);
return 0;
}
--
2.25.4
Re: [PATCH] mmc: meson-gx: remove IRQF_ONESHOT
On Mon 05 Oct 2020 at 10:55, Thomas Gleixner wrote: > On Mon, Oct 05 2020 at 10:22, Ulf Hansson wrote: >> On Fri, 2 Oct 2020 at 18:49, Jerome Brunet wrote: >>> >>> IRQF_ONESHOT was added to this driver to make sure the irq was not enabled >>> again until the thread part of the irq had finished doing its job. >>> >>> Doing so upsets RT because, under RT, the hardirq part of the irq handler >>> is not migrated to a thread if the irq is claimed with IRQF_ONESHOT. >>> In this case, it has been reported to eventually trigger a deadlock with >>> the led subsystem. >>> >>> Preventing RT from doing this migration was certainly not the intent, the >>> description of IRQF_ONESHOT does not really reflect this constraint: >>> >>> > IRQF_ONESHOT - Interrupt is not reenabled after the hardirq handler >>> finished. >>> > Used by threaded interrupts which need to keep the >>> > irq line disabled until the threaded handler has been run. >>> >>> This is exactly what this driver was trying to acheive so I'm still a bit >>> confused whether this is a driver or an RT issue. >>> >>> Anyway, this can be solved driver side by manually disabling the IRQs >>> instead of the relying on the IRQF_ONESHOT. IRQF_ONESHOT may then be removed >>> while still making sure the irq won't trigger until the threaded part of >>> the handler is done. >> >> Thomas, may I have your opinion on this one. >> >> I have no problem to apply $subject patch, but as Jerome also >> highlights above - this kind of makes me wonder if this is an RT >> issue, that perhaps deserves to be solved in a generic way. >> >> What do you think? > > Let me stare at the core code. Something smells fishy. FYI: initial discussion can be found here https://lore.kernel.org/linux-amlogic/24a844c3-c2e0-c735-ccb7-83736218b...@gmail.com/
Re: [PATCH] arm64: dts: meson: add SM1 soundcard name to VIM3L
On Fri 02 Oct 2020 at 20:45, Kevin Hilman wrote:
> Christian Hewitt writes:
>
>>> On 2 Oct 2020, at 6:44 pm, Jerome Brunet wrote:
>>>
>>> On Fri 02 Oct 2020 at 16:16, Christian Hewitt
>>> wrote:
>>>
>>>> VIM3L now inherits the sound node from the VIM3 common dtsi but is
>>>> an SM1 device, so label it as such, and stop users blaming future
>>>> support issues on the distro/app "wrongly detecting" their device.
>>>>
>>>> Signed-off-by: Christian Hewitt
>>>> ---
>>>> arch/arm64/boot/dts/amlogic/meson-sm1-khadas-vim3l.dts | 4
>>>> 1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/amlogic/meson-sm1-khadas-vim3l.dts
>>>> b/arch/arm64/boot/dts/amlogic/meson-sm1-khadas-vim3l.dts
>>>> index 4b517ca72059..f46f0ecc37ec 100644
>>>> --- a/arch/arm64/boot/dts/amlogic/meson-sm1-khadas-vim3l.dts
>>>> +++ b/arch/arm64/boot/dts/amlogic/meson-sm1-khadas-vim3l.dts
>>>> @@ -32,6 +32,10 @@
>>>>regulator-boot-on;
>>>>regulator-always-on;
>>>>};
>>>> +
>>>> + sound {
>>>> + model = "SM1-KHADAS-VIM3L";
>>>> + };
>>>
>>> The sound card is the same so I don't see why the sm1 board should have
>>> a different name. If you are not happy with the name, please update it
>>> in the common file.
>>
>> It’s a nice-to-have not a must-have, but the current LE images that are
>> in circulation use 5.7 with the previous board-correct name so I was
>> looking for continuity. We do see user forum reports (infrequent but
>> recurring) of wrongly detected hardware with other SoC platforms where
>> similar name inheritance surfaces the ‘wrong’ device name in GUIs, and
>> I like anything that avoids support work.
>>
>> I’d suggest KHADAS-VIM3-VIM3L as a common name, but then it’s the only
>> device in the current device-tree set that is not prefixed with the SoC
>> identifier, which (OCD) feels wrong.
>
> True, but turns out there's nothing SoC specific about this sound block
> since it's identical across SoCs, so specifying the SoC is being too
> specific.
>
> OTOH, while I agree it looks "wrong", it's pretty common in Linux DT to
> have the SoC prefix to mean only that it's "compatible" with that SoC,
> not that it *is* that SoC.
>
> However, I agree that that can lead to confusion with end users, so
> since this change has not functional change, and only a UX issue in
> userspace, I'm fine to apply it.
It is not UX only. This string is used by alsa-utils to match the
card. For example, the string will be matched to restore the controls
settings with alsactl on boot. VIM3 and VIM3L are the same sound card
AFAICT, so it should be the same string.
>
> Kevin
[PATCH] mmc: meson-gx: remove IRQF_ONESHOT
IRQF_ONESHOT was added to this driver to make sure the irq was not enabled
again until the thread part of the irq had finished doing its job.
Doing so upsets RT because, under RT, the hardirq part of the irq handler
is not migrated to a thread if the irq is claimed with IRQF_ONESHOT.
In this case, it has been reported to eventually trigger a deadlock with
the led subsystem.
Preventing RT from doing this migration was certainly not the intent, the
description of IRQF_ONESHOT does not really reflect this constraint:
> IRQF_ONESHOT - Interrupt is not reenabled after the hardirq handler finished.
> Used by threaded interrupts which need to keep the
> irq line disabled until the threaded handler has been run.
This is exactly what this driver was trying to acheive so I'm still a bit
confused whether this is a driver or an RT issue.
Anyway, this can be solved driver side by manually disabling the IRQs
instead of the relying on the IRQF_ONESHOT. IRQF_ONESHOT may then be removed
while still making sure the irq won't trigger until the threaded part of
the handler is done.
Fixes: eb4d81127746 ("mmc: meson-gx: correct irq flag")
Reported-by: Brad Harper
Cc: Sebastian Andrzej Siewior
Signed-off-by: Jerome Brunet
---
drivers/mmc/host/meson-gx-mmc.c | 47 -
1 file changed, 29 insertions(+), 18 deletions(-)
diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c
index 08a3b1c05acb..effc356db904 100644
--- a/drivers/mmc/host/meson-gx-mmc.c
+++ b/drivers/mmc/host/meson-gx-mmc.c
@@ -101,8 +101,7 @@
#define IRQ_RESP_STATUS BIT(14)
#define IRQ_SDIO BIT(15)
#define IRQ_EN_MASK \
- (IRQ_CRC_ERR | IRQ_TIMEOUTS | IRQ_END_OF_CHAIN | IRQ_RESP_STATUS |\
-IRQ_SDIO)
+ (IRQ_CRC_ERR | IRQ_TIMEOUTS | IRQ_END_OF_CHAIN)
#define SD_EMMC_CMD_CFG 0x50
#define SD_EMMC_CMD_ARG 0x54
@@ -170,6 +169,7 @@ struct meson_host {
dma_addr_t descs_dma_addr;
int irq;
+ u32 irq_en;
bool vqmmc_enabled;
};
@@ -842,22 +842,24 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id)
struct meson_host *host = dev_id;
struct mmc_command *cmd;
struct mmc_data *data;
- u32 irq_en, status, raw_status;
+ u32 status, raw_status;
irqreturn_t ret = IRQ_NONE;
- irq_en = readl(host->regs + SD_EMMC_IRQ_EN);
+ /* Disable irqs */
+ writel(0, host->regs + SD_EMMC_IRQ_EN);
+
raw_status = readl(host->regs + SD_EMMC_STATUS);
- status = raw_status & irq_en;
+ status = raw_status & host->irq_en;
if (!status) {
dev_dbg(host->dev,
"Unexpected IRQ! irq_en 0x%08x - status 0x%08x\n",
-irq_en, raw_status);
- return IRQ_NONE;
+host->irq_en, raw_status);
+ goto none;
}
if (WARN_ON(!host) || WARN_ON(!host->cmd))
- return IRQ_NONE;
+ goto none;
/* ack all raised interrupts */
writel(status, host->regs + SD_EMMC_STATUS);
@@ -908,6 +910,11 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id)
if (ret == IRQ_HANDLED)
meson_mmc_request_done(host->mmc, cmd->mrq);
+none:
+ /* Enable the irq again if the thread will not run */
+ if (ret != IRQ_WAKE_THREAD)
+ writel(host->irq_en, host->regs + SD_EMMC_IRQ_EN);
+
return ret;
}
@@ -934,15 +941,17 @@ static irqreturn_t meson_mmc_irq_thread(int irq, void
*dev_id)
struct mmc_command *next_cmd, *cmd = host->cmd;
struct mmc_data *data;
unsigned int xfer_bytes;
+ int ret = IRQ_HANDLED;
- if (WARN_ON(!cmd))
- return IRQ_NONE;
+ if (WARN_ON(!cmd)) {
+ ret = IRQ_NONE;
+ goto out;
+ }
if (cmd->error) {
meson_mmc_wait_desc_stop(host);
meson_mmc_request_done(host->mmc, cmd->mrq);
-
- return IRQ_HANDLED;
+ goto out;
}
data = cmd->data;
@@ -959,7 +968,10 @@ static irqreturn_t meson_mmc_irq_thread(int irq, void
*dev_id)
else
meson_mmc_request_done(host->mmc, cmd->mrq);
- return IRQ_HANDLED;
+out:
+ /* Re-enable the irqs */
+ writel(host->irq_en, host->regs + SD_EMMC_IRQ_EN);
+ return ret;
}
/*
@@ -1133,13 +1145,12 @@ static int meson_mmc_probe(struct platform_device *pdev)
/* clear, ack and enable interrupts */
writel(0, host->regs + SD_EMMC_IRQ_EN);
- writel(IRQ_CRC_ERR | IRQ_TIMEOUTS | IRQ_END_OF_CHAIN,
- host->regs + SD_EMMC_STATUS);
- writel(IRQ_CRC_ERR | IRQ_TIMEOUTS | IRQ_END_OF_CHAIN,
- host->regs + SD_EMMC_IRQ_EN);
+ host->irq_en = IRQ_EN_MASK;
+
Re: [PATCH v2 00/10] arm64: dts: meson: add more GX soundcards
On Fri 02 Oct 2020 at 16:31, Christian Hewitt wrote: > This series adds basic support for LPCM audio over HDMI and S/PDIF > interfaces to GXBB/GXL/GXM devices without support. I'm sure audio > support can be extended in places (some devices have internal DACs > and headphone hardware) but this gets the basics working. > > I have personally tested with the khadas-vim2, odroid-c2, and both > wetek devices as I have them, and there are positive forum reports > from users with vega-s95 and some no-name P20X box devices. It is fine to add what you have tested but I'm not confortable adding untested stuff which will later give the false idea that they are supposed to work. Amplifiers and codec may require different settings and ressources (such as GPIO and regulators) to actually operate properly. As far the p200 and p201, like the g12 u200, those are reference design with various sound card possibilities which usually don't apply to end products. For example the p200 is missing both input and output codecs, the sound amplifier and, as it stands, is likely to be muted. > > Changes from v1 > - Drop nexbox-a1 and rbox-pro changes - the regulator changes are > needed to get the dts to compile, but I do not have schematics to > validate the changes or the hardware to test with. > > Christian Hewitt (10): > arm64: dts: meson: add audio playback to a95x > arm64: dts: meson: add audio playback to khadas-vim2 > arm64: dts: meson: add audio playback to nanopi-k2 > arm64: dts: meson: add audio playback to odroid-c2 > arm64: dts: meson: add audio playback to p201 > arm64: dts: meson: add audio playback to p200 > arm64: dts: meson: add audio playback to p212-s905x dtsi > arm64: dts: meson: add audio playback to vega-s95 dtsi > arm64: dts: meson: add audio playback to wetek-hub > arm64: dts: meson: add audio playback to wetek-play2 > > .../boot/dts/amlogic/meson-gxbb-nanopi-k2.dts | 40 > .../dts/amlogic/meson-gxbb-nexbox-a95x.dts| 40 > .../boot/dts/amlogic/meson-gxbb-odroidc2.dts | 40 > .../boot/dts/amlogic/meson-gxbb-p200.dts | 61 +++ > .../boot/dts/amlogic/meson-gxbb-p201.dts | 40 > .../boot/dts/amlogic/meson-gxbb-vega-s95.dtsi | 61 +++ > .../boot/dts/amlogic/meson-gxbb-wetek-hub.dts | 40 > .../dts/amlogic/meson-gxbb-wetek-play2.dts| 61 +++ > .../dts/amlogic/meson-gxl-s905x-p212.dtsi | 40 > .../dts/amlogic/meson-gxm-khadas-vim2.dts | 44 - > 10 files changed, 464 insertions(+), 3 deletions(-)
Re: [PATCH] arm64: dts: meson: add SM1 soundcard name to VIM3L
On Fri 02 Oct 2020 at 16:16, Christian Hewitt
wrote:
> VIM3L now inherits the sound node from the VIM3 common dtsi but is
> an SM1 device, so label it as such, and stop users blaming future
> support issues on the distro/app "wrongly detecting" their device.
>
> Signed-off-by: Christian Hewitt
> ---
> arch/arm64/boot/dts/amlogic/meson-sm1-khadas-vim3l.dts | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-sm1-khadas-vim3l.dts
> b/arch/arm64/boot/dts/amlogic/meson-sm1-khadas-vim3l.dts
> index 4b517ca72059..f46f0ecc37ec 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-sm1-khadas-vim3l.dts
> +++ b/arch/arm64/boot/dts/amlogic/meson-sm1-khadas-vim3l.dts
> @@ -32,6 +32,10 @@
> regulator-boot-on;
> regulator-always-on;
> };
> +
> + sound {
> + model = "SM1-KHADAS-VIM3L";
> + };
The sound card is the same so I don't see why the sm1 board should have
a different name. If you are not happy with the name, please update it
in the common file.
> };
>
> {
Re: [PATCH 10/18] clk: meson: use semicolons rather than commas to separate statements
On Sun 27 Sep 2020 at 21:12, Julia Lawall wrote:
Hi Stephen,
Do you want to take all the clock related patches directly ?
> Replace commas with semicolons. What is done is essentially described by
> the following Coccinelle semantic patch (http://coccinelle.lip6.fr/):
>
> //
> @@ expression e1,e2; @@
> e1
> -,
> +;
> e2
> ... when any
> //
>
> Signed-off-by: Julia Lawall
>
> ---
> drivers/clk/meson/meson-aoclk.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/meson/meson-aoclk.c b/drivers/clk/meson/meson-aoclk.c
> index bf8bea675d24..3a6d84cd6601 100644
> --- a/drivers/clk/meson/meson-aoclk.c
> +++ b/drivers/clk/meson/meson-aoclk.c
> @@ -57,7 +57,7 @@ int meson_aoclkc_probe(struct platform_device *pdev)
> rstc->data = data;
> rstc->regmap = regmap;
> rstc->reset.ops = _aoclk_reset_ops;
> - rstc->reset.nr_resets = data->num_reset,
> + rstc->reset.nr_resets = data->num_reset;
> rstc->reset.of_node = dev->of_node;
> ret = devm_reset_controller_register(dev, >reset);
> if (ret) {
Re: [PATCH] mmc: host: meson-gx-mmc: fix possible deadlock condition for preempt_rt
On Fri 25 Sep 2020 at 15:44, Sebastian Andrzej Siewior wrote: > On 2020-09-25 11:11:42 [+0200], Jerome Brunet wrote: >> I'm not sure about this. >> As you have explained on IRC, I understand that IRQF_ONESHOT is causing >> trouble with RT as the hard IRQ part of the thread will not be migrated >> to a thread. That was certainly not the intent when putting this flag. > > That is my understanding as well. > >> This seems pretty unsafe to me. Maybe we could improve the driver so it >> copes with this case gracefully. ATM, I don't think it would. > > Running the primary handler in hardirq context is bad, because it > invokes meson_mmc_request_done() at the very end. And here: > - mmc_complete_cmd() -> complete_all() > There is a lockdep_assert_RT_in_threaded_ctx() which should trigger. > > - led_trigger_event() -> led_trigger_event() > This should trigger a might_sleep() warning somewhere. > > So removing IRQF_ONESHOT is okay but it should additionally disable the > IRQ source in meson_mmc_irq() and re-enable back in > meson_mmc_irq_thread(). Otherwise the IRQ remains asserted and may fire > multiple times before the thread has a chance to run. Looks like we need to do manually what IRQF_ONESHOT was doing for us :( This brings a few questions: * The consideration you described is not mentioned near the description of IRQF_ONESHOT. Maybe it should so other drivers with same intent don't end up in the same pitfall ? * Why doesn't RT move the IRQ with this flag ? Seems completly unrelated to RT (maybe it is the same documentation problem) * Can't we have flag doing the irq disable in the same way while still allowing to RT to do its magic ? seems better than open coding it in the driver ? > > Sebastian
Re: [PATCH] mmc: host: meson-gx-mmc: fix possible deadlock condition for preempt_rt
On Thu 24 Sep 2020 at 19:01, Kevin Hilman wrote: > Hi Brad, > > Brad Harper writes: > >> Force threaded interrupts for meson_mmc_irq to prevent possible deadlock >> condition >> during mmc operations when using preempt_rt with 5.9.0-rc3-rt3 patches >> on arm64. >> >> Using meson-gx-mmc with an emmc device on Hardkernel Odroid N2+ >> configured with >> preempt_rt resulted in the soc becoming unresponsive. With lock >> checking enabled >> the below inconsistent lock state was observed during boot. >> >> After some discussions with tglx in IRC #linux-rt the attached patch was >> suggested >> to remove IRQF_ONESHOT from request_threaded_irq. >> This has been tested and confirmed by me to resolve both the >> unresponsive soc and >> the inconsistent lock state warning when using 5.9.0-rc3-rt3 on arm64 >> Odroid N2+. >> >> Further review and testing is required to ensure there are no adverse >> impacts or >> concerns and that is the correct method to resolve the problem. I will >> continue >> to test on various amlogic devices with both standard mainline low >> latency kernel >> and preempt_rt kernel with -rt patches. > > This looks right to me, thanks for sending a fix. > > For broader testing, I can add this to my testing branch so it gets > booted on a bunch more platform in KernelCI also. > > However... > > [...] > >> Signed-off-by: Brad Harper >> --- >> drivers/mmc/host/meson-gx-mmc.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/host/meson-gx-mmc.c >> b/drivers/mmc/host/meson-gx-mmc.c >> index 08a3b1c05..130ac134d 100644 >> --- a/drivers/mmc/host/meson-gx-mmc.c >> +++ b/drivers/mmc/host/meson-gx-mmc.c >> @@ -1139,7 +1139,7 @@ static int meson_mmc_probe(struct platform_device >> *pdev) >> host->regs + SD_EMMC_IRQ_EN); >> >> ret = request_threaded_irq(host->irq, meson_mmc_irq, >> - meson_mmc_irq_thread, IRQF_ONESHOT, >> + meson_mmc_irq_thread, 0, >> dev_name(>dev), host); I'm not sure about this. As you have explained on IRC, I understand that IRQF_ONESHOT is causing trouble with RT as the hard IRQ part of the thread will not be migrated to a thread. That was certainly not the intent when putting this flag. As described in include/linux/interrupt.h: * IRQF_ONESHOT - Interrupt is not reenabled after the hardirq handler finished. *Used by threaded interrupts which need to keep the *irq line disabled until the threaded handler has been run. The intent was only that, making sure the irq is not re-enabled until the threaded part is done. AFAIU, removing this flag would allow the hard irq handler to fire again while we are not done handling the previous IRQ in threaded part. This seems pretty unsafe to me. Maybe we could improve the driver so it copes with this case gracefully. ATM, I don't think it would. Maybe I missed something and I am happy to be enlightened if that is the case :) >> if (ret) >> goto err_init_clk; > > This patch has been mangled by your mailer, so it doesn't apply cleanly. > If you're using the gmail web UI, this is a common problem. > > I strongly recommend using git-send-email to send directly via gmail > SMTP. The git-send-email docs[1] give some examples on how to set this > up. > > Kevin > > [1] https://git-scm.com/docs/git-send-email#_examples > > ___ > linux-amlogic mailing list > linux-amlo...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-amlogic
Re: [PATCH 8/8] arm64: dts: meson: enable RTC for VIM2 meson-gxm-khadas-vim2
On Fri 25 Sep 2020 at 05:30, Artem Lapkin wrote:
> enable RTC for VIM2 meson-gxm-khadas-vim2
>
> Signed-off-by: Artem Lapkin
> ---
> arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts
> b/arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts
> index 70343da2811..76b7e34a9a3 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts
> @@ -229,7 +229,7 @@ _B {
>
> rtc: rtc@51 {
> /* has to be enabled manually when a battery is connected: */
If going for this change, this comment should have been removed
> - status = "disabled";
> + status = "okay";
Unless the VIMs are provided with a battery by default, I believe this
should be kept disabled and only enabled by the bootloader if necessary.
If you think differently, feel free to resubmit with a complete commit
description and some details as to how this would be an improvement.
> compatible = "haoyu,hym8563";
> reg = <0x51>;
> #clock-cells = <0>;
[PATCH] mailbox: cancel timer before starting it
If the txdone is done by polling, it is possible for msg_submit() to start
the timer while txdone_hrtimer() callback is running. If the timer needs
recheduling, it could already be enqueued by the time hrtimer_forward_now()
is called, leading hrtimer to loudly complain.
WARNING: CPU: 3 PID: 74 at kernel/time/hrtimer.c:932 hrtimer_forward+0xc4/0x110
CPU: 3 PID: 74 Comm: kworker/u8:1 Not tainted
5.9.0-rc2-00236-gd3520067d01c-dirty #5
Hardware name: Libre Computer AML-S805X-AC (DT)
Workqueue: events_freezable_power_ thermal_zone_device_check
pstate: 2085 (nzCv daIf -PAN -UAO BTYPE=--)
pc : hrtimer_forward+0xc4/0x110
lr : txdone_hrtimer+0xf8/0x118
[...]
Canceling the timer before starting it ensure that the timer callback is
not running when the timer is started, solving this race condition.
Fixes: 0cc67945ea59 ("mailbox: switch to hrtimer for tx_complete polling")
Reported-by: Da Xue
Signed-off-by: Jerome Brunet
---
drivers/mailbox/mailbox.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 0b821a5b2db8..34f9ab01caef 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -82,9 +82,13 @@ static void msg_submit(struct mbox_chan *chan)
exit:
spin_unlock_irqrestore(>lock, flags);
- if (!err && (chan->txdone_method & TXDONE_BY_POLL))
- /* kick start the timer immediately to avoid delays */
+ if (!err && (chan->txdone_method & TXDONE_BY_POLL)) {
+ /* Disable the timer if already active ... */
+ hrtimer_cancel(>mbox->poll_hrt);
+
+ /* ... and kick start it immediately to avoid delays */
hrtimer_start(>mbox->poll_hrt, 0, HRTIMER_MODE_REL);
+ }
}
static void tx_tick(struct mbox_chan *chan, int r)
--
2.25.4
[PATCH 0/2] arm64: dts: meson: add aml-s905x-cc v2 support
This patchset adds initial support for the libretech aml-s905x-cc v2. Jerome Brunet (2): dt-bindings: arm: amlogic: add support for libretch s905x cc v2 arm64: dts: meson: initial support for aml-s905x-cc v2 .../devicetree/bindings/arm/amlogic.yaml | 1 + arch/arm64/boot/dts/amlogic/Makefile | 1 + .../meson-gxl-s905x-libretech-cc-v2.dts | 318 ++ 3 files changed, 320 insertions(+) create mode 100644 arch/arm64/boot/dts/amlogic/meson-gxl-s905x-libretech-cc-v2.dts -- 2.25.4
[PATCH 1/2] dt-bindings: arm: amlogic: add support for libretch s905x cc v2
Add support for the 2nd version of the libretch aml-s905x-cc. Signed-off-by: Jerome Brunet --- Documentation/devicetree/bindings/arm/amlogic.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/arm/amlogic.yaml b/Documentation/devicetree/bindings/arm/amlogic.yaml index 5eba9f48823e..1c29d7ff8edf 100644 --- a/Documentation/devicetree/bindings/arm/amlogic.yaml +++ b/Documentation/devicetree/bindings/arm/amlogic.yaml @@ -96,6 +96,7 @@ properties: - hwacom,amazetv - khadas,vim - libretech,aml-s905x-cc + - libretech,aml-s905x-cc-v2 - nexbox,a95x - const: amlogic,s905x - const: amlogic,meson-gxl -- 2.25.4
[PATCH 2/2] arm64: dts: meson: initial support for aml-s905x-cc v2
Add initial support for the libretech aml-s905x-cc (Le Potato) v2
Signed-off-by: Jerome Brunet
---
arch/arm64/boot/dts/amlogic/Makefile | 1 +
.../meson-gxl-s905x-libretech-cc-v2.dts | 318 ++
2 files changed, 319 insertions(+)
create mode 100644
arch/arm64/boot/dts/amlogic/meson-gxl-s905x-libretech-cc-v2.dts
diff --git a/arch/arm64/boot/dts/amlogic/Makefile
b/arch/arm64/boot/dts/amlogic/Makefile
index 4e2239ffcaa5..be5277b7fbf4 100644
--- a/arch/arm64/boot/dts/amlogic/Makefile
+++ b/arch/arm64/boot/dts/amlogic/Makefile
@@ -24,6 +24,7 @@ dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s805x-libretech-ac.dtb
dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905x-hwacom-amazetv.dtb
dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905x-khadas-vim.dtb
dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905x-libretech-cc.dtb
+dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905x-libretech-cc-v2.dtb
dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905x-nexbox-a95x.dtb
dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905x-p212.dtb
dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905d-p230.dtb
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-libretech-cc-v2.dts
b/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-libretech-cc-v2.dts
new file mode 100644
index ..675eaa87963e
--- /dev/null
+++ b/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-libretech-cc-v2.dts
@@ -0,0 +1,318 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2020 BayLibre, SAS.
+ * Author: Jerome Brunet
+ */
+
+/dts-v1/;
+
+#include
+#include
+#include
+
+#include "meson-gxl-s905x.dtsi"
+
+/ {
+ compatible = "libretech,aml-s905x-cc-v2", "amlogic,s905x",
+"amlogic,meson-gxl";
+ model = "Libre Computer AML-S905X-CC V2";
+
+ aliases {
+ serial0 = _AO;
+ ethernet0 =
+ spi0 =
+ };
+
+ chosen {
+ stdout-path = "serial0:115200n8";
+ };
+
+ emmc_pwrseq: emmc-pwrseq {
+ compatible = "mmc-pwrseq-emmc";
+ reset-gpios = < BOOT_9 GPIO_ACTIVE_LOW>;
+ };
+
+ hdmi-connector {
+ compatible = "hdmi-connector";
+ type = "a";
+
+ port {
+ hdmi_connector_in: endpoint {
+ remote-endpoint = <_tx_tmds_out>;
+ };
+ };
+ };
+
+ leds {
+ compatible = "gpio-leds";
+
+ led-blue {
+ color = ;
+ function = LED_FUNCTION_STATUS;
+ gpios = < GPIODV_24 GPIO_ACTIVE_HIGH>;
+ linux,default-trigger = "heartbeat";
+ panic-indicator;
+ };
+
+ led-green {
+ color = ;
+ function = LED_FUNCTION_DISK_ACTIVITY;
+ gpios = <_ao GPIOAO_2 GPIO_ACTIVE_HIGH>;
+ linux,default-trigger = "disk-activity";
+ };
+ };
+
+ memory@0 {
+ device_type = "memory";
+ reg = <0x0 0x0 0x0 0x8000>;
+ };
+
+ ao_5v: regulator-ao_5v {
+ compatible = "regulator-fixed";
+ regulator-name = "AO_5V";
+ regulator-min-microvolt = <500>;
+ regulator-max-microvolt = <500>;
+ vin-supply = <_in>;
+ regulator-always-on;
+ };
+
+ dc_in: regulator-dc_in {
+ compatible = "regulator-fixed";
+ regulator-name = "DC_IN";
+ regulator-min-microvolt = <500>;
+ regulator-max-microvolt = <500>;
+ regulator-always-on;
+ };
+
+
+ vcck: regulator-vcck {
+ compatible = "regulator-fixed";
+ regulator-name = "VCCK";
+ regulator-min-microvolt = <330>;
+ regulator-max-microvolt = <330>;
+ vin-supply = <_5v>;
+ regulator-always-on;
+ };
+
+ vcc_card: regulator-vcc_card {
+ compatible = "regulator-fixed";
+ regulator-name = "VCC_CARD";
+ regulator-min-microvolt = <330>;
+ regulator-max-microvolt = <330>;
+ vin-supply = <_ao3v3>;
+
+ gpio = < GPIOCLK_1 GPIO_ACTIVE_HIGH>;
+ enable-active-high;
+ };
+
+ vcc5v: regulator-vcc5v {
+ compatible = "regulator-fixed";
+ regulator-name = "VCC5V";
+ regulator-min-microvolt = <500>;
+ regulator-max-microv
Re: [PATCH 2/2] arm64: dts: meson: initial support for aml-s905x-cc v2
On Tue 15 Sep 2020 at 16:30, Neil Armstrong wrote:
> Hi,
>
> On 15/09/2020 16:19, Jerome Brunet wrote:
>> Add initial support for the libretech aml-s905x-cc (Le Potato) v2
>>
>> Signed-off-by: Jerome Brunet
>> ---
>> arch/arm64/boot/dts/amlogic/Makefile | 1 +
>> .../meson-gxl-s905x-libretech-cc-v2.dts | 318 ++
>> 2 files changed, 319 insertions(+)
>> create mode 100644
>> arch/arm64/boot/dts/amlogic/meson-gxl-s905x-libretech-cc-v2.dts
>>
>> diff --git a/arch/arm64/boot/dts/amlogic/Makefile
>> b/arch/arm64/boot/dts/amlogic/Makefile
>> index 4e2239ffcaa5..be5277b7fbf4 100644
>> --- a/arch/arm64/boot/dts/amlogic/Makefile
>> +++ b/arch/arm64/boot/dts/amlogic/Makefile
>> @@ -24,6 +24,7 @@ dtb-$(CONFIG_ARCH_MESON) +=
>> meson-gxl-s805x-libretech-ac.dtb
>> dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905x-hwacom-amazetv.dtb
>> dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905x-khadas-vim.dtb
>> dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905x-libretech-cc.dtb
>> +dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905x-libretech-cc-v2.dtb
>> dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905x-nexbox-a95x.dtb
>> dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905x-p212.dtb
>> dtb-$(CONFIG_ARCH_MESON) += meson-gxl-s905d-p230.dtb
>> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-libretech-cc-v2.dts
>> b/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-libretech-cc-v2.dts
>> new file mode 100644
>> index ..675eaa87963e
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-libretech-cc-v2.dts
>> @@ -0,0 +1,318 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Copyright (c) 2020 BayLibre, SAS.
>> + * Author: Jerome Brunet
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include
>> +#include
>> +#include
>> +
>> +#include "meson-gxl-s905x.dtsi"
>> +
>> +/ {
>> +compatible = "libretech,aml-s905x-cc-v2", "amlogic,s905x",
>> + "amlogic,meson-gxl";
>> +model = "Libre Computer AML-S905X-CC V2";
>> +
>> +aliases {
>> +serial0 = _AO;
>> +ethernet0 =
>> +spi0 =
>> +};
>> +
>> +chosen {
>> +stdout-path = "serial0:115200n8";
>> +};
>> +
>> +emmc_pwrseq: emmc-pwrseq {
>> +compatible = "mmc-pwrseq-emmc";
>> +reset-gpios = < BOOT_9 GPIO_ACTIVE_LOW>;
>> +};
>> +
>> +hdmi-connector {
>> +compatible = "hdmi-connector";
>> +type = "a";
>> +
>> +port {
>> +hdmi_connector_in: endpoint {
>> +remote-endpoint = <_tx_tmds_out>;
>> +};
>> +};
>> +};
>> +
>> +leds {
>> +compatible = "gpio-leds";
>> +
>> +led-blue {
>> +color = ;
>> +function = LED_FUNCTION_STATUS;
>> +gpios = < GPIODV_24 GPIO_ACTIVE_HIGH>;
>> +linux,default-trigger = "heartbeat";
>> +panic-indicator;
>> +};
>> +
>> +led-green {
>> +color = ;
>> +function = LED_FUNCTION_DISK_ACTIVITY;
>> +gpios = <_ao GPIOAO_2 GPIO_ACTIVE_HIGH>;
>> +linux,default-trigger = "disk-activity";
>> +};
>> +};
>> +
>> +memory@0 {
>> +device_type = "memory";
>> +reg = <0x0 0x0 0x0 0x8000>;
>> +};
>> +
>> +ao_5v: regulator-ao_5v {
>> +compatible = "regulator-fixed";
>> +regulator-name = "AO_5V";
>> +regulator-min-microvolt = <500>;
>> +regulator-max-microvolt = <500>;
>> +vin-supply = <_in>;
>> +regulator-always-on;
>> +};
>> +
>> +dc_in: regulator-dc_in {
>> +compatible = "regulator-fixed";
>> +regulator-name = "DC_IN";
>> +regulator-min-microvolt = <500>;
>> +regulator-max-microvolt = <500>;
>> +regulator-always-on;
>> +};
>> +
>> +
>> +
Re: [PATCH 4/4] clk: meson: axg: add MIPI DSI Host clock
On Mon 07 Sep 2020 at 11:38, Neil Armstrong wrote:
> This adds the MIPI DSI Host clock, used to measure the signal timings (ENC
> VSYNC or
> DW-MIPI-DSI eDPI timings).
>
> Signed-off-by: Neil Armstrong
> ---
> drivers/clk/meson/axg.c | 66 +
> drivers/clk/meson/axg.h | 4 ++-
> 2 files changed, 69 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/meson/axg.c b/drivers/clk/meson/axg.c
> index 2550616c14b0..0094ca6388d8 100644
> --- a/drivers/clk/meson/axg.c
> +++ b/drivers/clk/meson/axg.c
> @@ -1724,6 +1724,66 @@ static struct clk_regmap axg_cts_encl = {
> },
> };
>
> +/* MIPI DSI Host Clock */
> +
> +static const struct clk_parent_data axg_vdin_meas_parent_data[] = {
> + { .fw_name = "xtal", },
> + { .hw = _fclk_div4.hw },
> + { .hw = _fclk_div3.hw },
> + { .hw = _fclk_div5.hw },
> + { },
> + { },
As you've done in other patches, I'd prefer if you used an index table
instead of having empty entries.
> + { .hw = _fclk_div2.hw },
> + { .hw = _fclk_div7.hw },
> +};
> +
> +static struct clk_regmap axg_vdin_meas_sel = {
> + .data = &(struct clk_regmap_mux_data){
> + .offset = HHI_VDIN_MEAS_CLK_CNTL,
> + .mask = 0x7,
> + .shift = 21,
> + .flags = CLK_MUX_ROUND_CLOSEST,
> + },
> + .hw.init = &(struct clk_init_data){
> + .name = "vdin_meas_sel",
> + .ops = _regmap_mux_ops,
> + .parent_data = axg_vdin_meas_parent_data,
> + .num_parents = ARRAY_SIZE(axg_vdin_meas_parent_data),
> + .flags = CLK_SET_RATE_NO_REPARENT,
A comment about why manual control is required would be nice.
> + },
> +};
> +
> +static struct clk_regmap axg_vdin_meas_div = {
> + .data = &(struct clk_regmap_div_data){
> + .offset = HHI_VDIN_MEAS_CLK_CNTL,
> + .shift = 12,
> + .width = 7,
> + },
> + .hw.init = &(struct clk_init_data){
> + .name = "vdin_meas_div",
> + .ops = _regmap_divider_ops,
> + .parent_hws = (const struct clk_hw *[]) {
> + _vdin_meas_sel.hw },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + },
> +};
> +
> +static struct clk_regmap axg_vdin_meas = {
> + .data = &(struct clk_regmap_gate_data){
> + .offset = HHI_VDIN_MEAS_CLK_CNTL,
> + .bit_idx = 20,
> + },
> + .hw.init = &(struct clk_init_data) {
> + .name = "vdin_meas",
> + .ops = _regmap_gate_ops,
> + .parent_hws = (const struct clk_hw *[]) {
> + _vdin_meas_div.hw },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + },
> +};
> +
> static u32 mux_table_gen_clk[] = { 0, 4, 5, 6, 7, 8,
> 9, 10, 11, 13, 14, };
> static const struct clk_parent_data gen_clk_parent_data[] = {
> @@ -1987,6 +2047,9 @@ static struct clk_hw_onecell_data axg_hw_onecell_data =
> {
> [CLKID_VCLK2_DIV12] = _vclk2_div12.hw,
> [CLKID_CTS_ENCL_SEL]= _cts_encl_sel.hw,
> [CLKID_CTS_ENCL]= _cts_encl.hw,
> + [CLKID_VDIN_MEAS_SEL] = _vdin_meas_sel.hw,
> + [CLKID_VDIN_MEAS_DIV] = _vdin_meas_div.hw,
> + [CLKID_VDIN_MEAS] = _vdin_meas.hw,
> [NR_CLKS] = NULL,
> },
> .num = NR_CLKS,
> @@ -2115,6 +2178,9 @@ static struct clk_regmap *const axg_clk_regmaps[] = {
> _vclk2_div12_en,
> _cts_encl_sel,
> _cts_encl,
> + _vdin_meas_sel,
> + _vdin_meas_div,
> + _vdin_meas,
> };
>
> static const struct meson_eeclkc_data axg_clkc_data = {
> diff --git a/drivers/clk/meson/axg.h b/drivers/clk/meson/axg.h
> index a8787b394a47..481b307ea3cb 100644
> --- a/drivers/clk/meson/axg.h
> +++ b/drivers/clk/meson/axg.h
> @@ -158,8 +158,10 @@
> #define CLKID_VCLK2_DIV6_EN 120
> #define CLKID_VCLK2_DIV12_EN 121
> #define CLKID_CTS_ENCL_SEL 132
> +#define CLKID_VDIN_MEAS_SEL 134
> +#define CLKID_VDIN_MEAS_DIV 135
>
> -#define NR_CLKS 134
> +#define NR_CLKS 137
>
> /* include the CLKIDs that have been made part of the DT binding */
> #include
Re: [PATCH 3/4] clk: meson: axg: add Video Clocks
On Mon 07 Sep 2020 at 11:38, Neil Armstrong wrote:
> Add the Video Clocks present on the Amlogic AXg SoCs.
>
> The AXG only has a single ENCL CTS clock and even if VCLK exist along VCLK2,
> only VCLK2 is used since it clocks the MIPI DSI IP directly.
>
> Signed-off-by: Neil Armstrong
> ---
> drivers/clk/meson/axg.c | 774
> drivers/clk/meson/axg.h | 21 +-
> 2 files changed, 794 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/meson/axg.c b/drivers/clk/meson/axg.c
> index 13fc0006f63d..2550616c14b0 100644
> --- a/drivers/clk/meson/axg.c
> +++ b/drivers/clk/meson/axg.c
> @@ -1026,6 +1026,704 @@ static struct clk_regmap axg_sd_emmc_c_clk0 = {
> },
> };
>
> +/* VPU Clock */
> +
> +static const struct clk_hw *axg_vpu_parent_hws[] = {
> + _fclk_div4.hw,
> + _fclk_div3.hw,
> + _fclk_div5.hw,
> + _fclk_div7.hw,
> +};
> +
> +static struct clk_regmap axg_vpu_0_sel = {
> + .data = &(struct clk_regmap_mux_data){
> + .offset = HHI_VPU_CLK_CNTL,
> + .mask = 0x3,
> + .shift = 9,
> + },
> + .hw.init = &(struct clk_init_data){
> + .name = "vpu_0_sel",
> + .ops = _regmap_mux_ops,
> + /*
> + * bits 9:10 selects from 4 possible parents:
> + * fclk_div4, fclk_div3, fclk_div5, fclk_div7,
> + */
These comments (and the same bellow) are not very useful
> + .parent_hws = axg_vpu_parent_hws,
> + .num_parents = ARRAY_SIZE(axg_vpu_parent_hws),
Could you add a comment here explaining why parenting needs to be
manually controlled ?
> + .flags = CLK_SET_RATE_NO_REPARENT,
> + },
> +};
> +
> +static struct clk_regmap axg_vpu_0_div = {
> + .data = &(struct clk_regmap_div_data){
> + .offset = HHI_VPU_CLK_CNTL,
> + .shift = 0,
> + .width = 7,
> + },
> + .hw.init = &(struct clk_init_data){
> + .name = "vpu_0_div",
> + .ops = _regmap_divider_ops,
> + .parent_hws = (const struct clk_hw *[]) { _vpu_0_sel.hw },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + },
> +};
> +
> +static struct clk_regmap axg_vpu_0 = {
> + .data = &(struct clk_regmap_gate_data){
> + .offset = HHI_VPU_CLK_CNTL,
> + .bit_idx = 8,
> + },
> + .hw.init = &(struct clk_init_data) {
> + .name = "vpu_0",
> + .ops = _regmap_gate_ops,
> + .parent_hws = (const struct clk_hw *[]) { _vpu_0_div.hw },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
Why is UNUSED required ?
> + },
> +};
> +
> +static struct clk_regmap axg_vpu_1_sel = {
> + .data = &(struct clk_regmap_mux_data){
> + .offset = HHI_VPU_CLK_CNTL,
> + .mask = 0x3,
> + .shift = 25,
> + },
> + .hw.init = &(struct clk_init_data){
> + .name = "vpu_1_sel",
> + .ops = _regmap_mux_ops,
> + /*
> + * bits 25:26 selects from 4 possible parents:
> + * fclk_div4, fclk_div3, fclk_div5, fclk_div7,
> + */
> + .parent_hws = axg_vpu_parent_hws,
> + .num_parents = ARRAY_SIZE(axg_vpu_parent_hws),
> + .flags = CLK_SET_RATE_NO_REPARENT,
> + },
> +};
> +
> +static struct clk_regmap axg_vpu_1_div = {
> + .data = &(struct clk_regmap_div_data){
> + .offset = HHI_VPU_CLK_CNTL,
> + .shift = 16,
> + .width = 7,
> + },
> + .hw.init = &(struct clk_init_data){
> + .name = "vpu_1_div",
> + .ops = _regmap_divider_ops,
> + .parent_hws = (const struct clk_hw *[]) { _vpu_1_sel.hw },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + },
> +};
> +
> +static struct clk_regmap axg_vpu_1 = {
> + .data = &(struct clk_regmap_gate_data){
> + .offset = HHI_VPU_CLK_CNTL,
> + .bit_idx = 24,
> + },
> + .hw.init = &(struct clk_init_data) {
> + .name = "vpu_1",
> + .ops = _regmap_gate_ops,
> + .parent_hws = (const struct clk_hw *[]) { _vpu_1_div.hw },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> + },
> +};
> +
> +static struct clk_regmap axg_vpu = {
> + .data = &(struct clk_regmap_mux_data){
> + .offset = HHI_VPU_CLK_CNTL,
> + .mask = 1,
> + .shift = 31,
> + },
> + .hw.init = &(struct clk_init_data){
> + .name = "vpu",
> + .ops = _regmap_mux_ops,
> + /*
> + * bit 31 selects from 2 possible parents:
> + * vpu_0 or vpu_1
> + */
> + .parent_hws = (const struct clk_hw *[]) {
> + _vpu_0.hw,
> +
