* [RFC] ARM: decompressor: Make RAM sections Outer non-cacheable
From: Silvano di Ninno @ 2018-01-03 16:32 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
There has been few attempts [1] [2] to make i.MX6 SoC run linux in Non-secure world,
with OP-TEE running in Secure world.
As you may know, the OP-TEE firmware (on Cortex A9) enables PL310 cache
controller at boot time and before the switch to Non-secure. The Way to allow
Linux to boot normally is currently to lock the ways so that L2 cache lines
are not filled.
It looks like Linux Kernel itself does not suffer from this (as it
does not have to perform cache management at boot time prior to l2 controller
initialization).
However, the decompressor code does and (at least for i.MX6 SoC families) this
is why Linux cannot boot correctly.
The patch [3] makes the RAM section outer non-cacheable so that the Linux code gets
correctly flush to DDR.
Is that solution acceptable?
Note on write_sec:
I am not a fan of the write_sec solution.
The idea behing Trustzone and Secure World is that the code being run in Non-secure
world (in this case linux) should not be (or is less) trusted.
In this context, it makes sense to rely as less as possible on Linux and so if
a solution can be found that does not require Linux intervention, I would favor that one.
Thanks,
Silvano
[1] http://archive.armlinux.org.uk/lurker/message/20171230.123403.ea9ef177.en.html
[2] http://archive.armlinux.org.uk/lurker/message/20171126.122530.ea0a4791.en.html
[3] [PATCH] ARM: decompressor: Make RAM sections Outer non-cacheable
Silvano di Ninno (1):
ARM: decompressor: Make RAM sections Outer non-cacheable
arch/arm/boot/compressed/head.S | 1 +
1 file changed, 1 insertion(+)
--
2.7.4
^ permalink raw reply
* [PATCH] ARM: dts: sun[47]i: Fix display backend 1 output to TCON0 remote endpoint
From: Chen-Yu Tsai @ 2018-01-03 16:31 UTC (permalink / raw)
To: linux-arm-kernel
There is a copy-paste error in the display pipeline device tree graph.
The remote endpoint of the display backend 1's output to TCON0 points
to the wrong endpoint. This will result in the driver incorrectly
parsing the relationship of the components.
Reported-by: Andrea Venturi <ennesimamail.av@gmail.com>
Fixes: 0df4cf33a594 ("ARM: dts: sun4i: Add device nodes for display
pipelines")
Fixes: 5b92b29bed45 ("ARM: dts: sun7i: Add device nodes for display
pipelines")
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
This is a late fix for 4.15 that corrects an error in the newly
added display pipeline graph for the A10 and A20.
---
arch/arm/boot/dts/sun4i-a10.dtsi | 2 +-
arch/arm/boot/dts/sun7i-a20.dtsi | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
index 5840f5c75c3b..4f2f2eea0755 100644
--- a/arch/arm/boot/dts/sun4i-a10.dtsi
+++ b/arch/arm/boot/dts/sun4i-a10.dtsi
@@ -1104,7 +1104,7 @@
be1_out_tcon0: endpoint at 0 {
reg = <0>;
- remote-endpoint = <&tcon1_in_be0>;
+ remote-endpoint = <&tcon0_in_be1>;
};
be1_out_tcon1: endpoint at 1 {
diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index 59655e42e4b0..bd0cd3204273 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -1354,7 +1354,7 @@
be1_out_tcon0: endpoint at 0 {
reg = <0>;
- remote-endpoint = <&tcon1_in_be0>;
+ remote-endpoint = <&tcon0_in_be1>;
};
be1_out_tcon1: endpoint at 1 {
--
2.15.1
^ permalink raw reply related
* Applied "ASoC: mediatek: update clock related properties of MT2701 AFE" to the asoc tree
From: Mark Brown @ 2018-01-03 16:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <8693f44aac4304ce99836fc5e715a796347541d3.1514881870.git.ryder.lee@mediatek.com>
The patch
ASoC: mediatek: update clock related properties of MT2701 AFE
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
>From 0739fdfc0617a86781799d033e8fe758e8e48554 Mon Sep 17 00:00:00 2001
From: Ryder Lee <ryder.lee@mediatek.com>
Date: Tue, 2 Jan 2018 19:47:21 +0800
Subject: [PATCH] ASoC: mediatek: update clock related properties of MT2701 AFE
Add 'assigned-clocks*' properties which are used to initialize default
domain sources of audio system. we could configure different sets of
input clocks through DTS now. Hence driver no longer cares about that.
Also we change some 'clock-names' to make them more generic so that
other chips can reuse gracefully.
Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
.../devicetree/bindings/sound/mt2701-afe-pcm.txt | 207 +++++++++------------
1 file changed, 91 insertions(+), 116 deletions(-)
diff --git a/Documentation/devicetree/bindings/sound/mt2701-afe-pcm.txt b/Documentation/devicetree/bindings/sound/mt2701-afe-pcm.txt
index 77a57f84bed4..0450baad2813 100644
--- a/Documentation/devicetree/bindings/sound/mt2701-afe-pcm.txt
+++ b/Documentation/devicetree/bindings/sound/mt2701-afe-pcm.txt
@@ -6,51 +6,44 @@ Required properties:
- interrupts: should contain AFE and ASYS interrupts
- interrupt-names: should be "afe" and "asys"
- power-domains: should define the power domain
+- clocks: Must contain an entry for each entry in clock-names
+ See ../clocks/clock-bindings.txt for details
- clock-names: should have these clock names:
- "infra_sys_audio_clk",
"top_audio_mux1_sel",
"top_audio_mux2_sel",
- "top_audio_mux1_div",
- "top_audio_mux2_div",
- "top_audio_48k_timing",
- "top_audio_44k_timing",
- "top_audpll_mux_sel",
- "top_apll_sel",
- "top_aud1_pll_98M",
- "top_aud2_pll_90M",
- "top_hadds2_pll_98M",
- "top_hadds2_pll_294M",
- "top_audpll",
- "top_audpll_d4",
- "top_audpll_d8",
- "top_audpll_d16",
- "top_audpll_d24",
- "top_audintbus_sel",
- "clk_26m",
- "top_syspll1_d4",
- "top_aud_k1_src_sel",
- "top_aud_k2_src_sel",
- "top_aud_k3_src_sel",
- "top_aud_k4_src_sel",
- "top_aud_k5_src_sel",
- "top_aud_k6_src_sel",
- "top_aud_k1_src_div",
- "top_aud_k2_src_div",
- "top_aud_k3_src_div",
- "top_aud_k4_src_div",
- "top_aud_k5_src_div",
- "top_aud_k6_src_div",
- "top_aud_i2s1_mclk",
- "top_aud_i2s2_mclk",
- "top_aud_i2s3_mclk",
- "top_aud_i2s4_mclk",
- "top_aud_i2s5_mclk",
- "top_aud_i2s6_mclk",
- "top_asm_m_sel",
- "top_asm_h_sel",
- "top_univpll2_d4",
- "top_univpll2_d2",
- "top_syspll_d5";
+ "i2s0_src_sel",
+ "i2s1_src_sel",
+ "i2s2_src_sel",
+ "i2s3_src_sel",
+ "i2s0_src_div",
+ "i2s1_src_div",
+ "i2s2_src_div",
+ "i2s3_src_div",
+ "i2s0_mclk_en",
+ "i2s1_mclk_en",
+ "i2s2_mclk_en",
+ "i2s3_mclk_en",
+ "i2so0_hop_ck",
+ "i2so1_hop_ck",
+ "i2so2_hop_ck",
+ "i2so3_hop_ck",
+ "i2si0_hop_ck",
+ "i2si1_hop_ck",
+ "i2si2_hop_ck",
+ "i2si3_hop_ck",
+ "asrc0_out_ck",
+ "asrc1_out_ck",
+ "asrc2_out_ck",
+ "asrc3_out_ck",
+ "audio_afe_pd",
+ "audio_afe_conn_pd",
+ "audio_a1sys_pd",
+ "audio_a2sys_pd",
+ "audio_mrgif_pd";
+- assigned-clocks: list of input clocks and dividers for the audio system.
+ See ../clocks/clock-bindings.txt for details.
+- assigned-clocks-parents: parent of input clocks of assigned clocks.
+- assigned-clock-rates: list of clock frequencies of assigned clocks.
Example:
@@ -62,93 +55,75 @@ Example:
<GIC_SPI 132 IRQ_TYPE_LEVEL_LOW>;
interrupt-names = "afe", "asys";
power-domains = <&scpsys MT2701_POWER_DOMAIN_IFR_MSC>;
- clocks = <&infracfg CLK_INFRA_AUDIO>,
- <&topckgen CLK_TOP_AUD_MUX1_SEL>,
+ clocks = <&topckgen CLK_TOP_AUD_MUX1_SEL>,
<&topckgen CLK_TOP_AUD_MUX2_SEL>,
- <&topckgen CLK_TOP_AUD_MUX1_DIV>,
- <&topckgen CLK_TOP_AUD_MUX2_DIV>,
- <&topckgen CLK_TOP_AUD_48K_TIMING>,
- <&topckgen CLK_TOP_AUD_44K_TIMING>,
- <&topckgen CLK_TOP_AUDPLL_MUX_SEL>,
- <&topckgen CLK_TOP_APLL_SEL>,
- <&topckgen CLK_TOP_AUD1PLL_98M>,
- <&topckgen CLK_TOP_AUD2PLL_90M>,
- <&topckgen CLK_TOP_HADDS2PLL_98M>,
- <&topckgen CLK_TOP_HADDS2PLL_294M>,
- <&topckgen CLK_TOP_AUDPLL>,
- <&topckgen CLK_TOP_AUDPLL_D4>,
- <&topckgen CLK_TOP_AUDPLL_D8>,
- <&topckgen CLK_TOP_AUDPLL_D16>,
- <&topckgen CLK_TOP_AUDPLL_D24>,
- <&topckgen CLK_TOP_AUDINTBUS_SEL>,
- <&clk26m>,
- <&topckgen CLK_TOP_SYSPLL1_D4>,
<&topckgen CLK_TOP_AUD_K1_SRC_SEL>,
<&topckgen CLK_TOP_AUD_K2_SRC_SEL>,
<&topckgen CLK_TOP_AUD_K3_SRC_SEL>,
<&topckgen CLK_TOP_AUD_K4_SRC_SEL>,
- <&topckgen CLK_TOP_AUD_K5_SRC_SEL>,
- <&topckgen CLK_TOP_AUD_K6_SRC_SEL>,
<&topckgen CLK_TOP_AUD_K1_SRC_DIV>,
<&topckgen CLK_TOP_AUD_K2_SRC_DIV>,
<&topckgen CLK_TOP_AUD_K3_SRC_DIV>,
<&topckgen CLK_TOP_AUD_K4_SRC_DIV>,
- <&topckgen CLK_TOP_AUD_K5_SRC_DIV>,
- <&topckgen CLK_TOP_AUD_K6_SRC_DIV>,
<&topckgen CLK_TOP_AUD_I2S1_MCLK>,
<&topckgen CLK_TOP_AUD_I2S2_MCLK>,
<&topckgen CLK_TOP_AUD_I2S3_MCLK>,
<&topckgen CLK_TOP_AUD_I2S4_MCLK>,
- <&topckgen CLK_TOP_AUD_I2S5_MCLK>,
- <&topckgen CLK_TOP_AUD_I2S6_MCLK>,
- <&topckgen CLK_TOP_ASM_M_SEL>,
- <&topckgen CLK_TOP_ASM_H_SEL>,
- <&topckgen CLK_TOP_UNIVPLL2_D4>,
- <&topckgen CLK_TOP_UNIVPLL2_D2>,
- <&topckgen CLK_TOP_SYSPLL_D5>;
+ <&audiosys CLK_AUD_I2SO1>,
+ <&audiosys CLK_AUD_I2SO2>,
+ <&audiosys CLK_AUD_I2SO3>,
+ <&audiosys CLK_AUD_I2SO4>,
+ <&audiosys CLK_AUD_I2SIN1>,
+ <&audiosys CLK_AUD_I2SIN2>,
+ <&audiosys CLK_AUD_I2SIN3>,
+ <&audiosys CLK_AUD_I2SIN4>,
+ <&audiosys CLK_AUD_ASRCO1>,
+ <&audiosys CLK_AUD_ASRCO2>,
+ <&audiosys CLK_AUD_ASRCO3>,
+ <&audiosys CLK_AUD_ASRCO4>,
+ <&audiosys CLK_AUD_AFE>,
+ <&audiosys CLK_AUD_AFE_CONN>,
+ <&audiosys CLK_AUD_A1SYS>,
+ <&audiosys CLK_AUD_A2SYS>,
+ <&audiosys CLK_AUD_AFE_MRGIF>;
- clock-names = "infra_sys_audio_clk",
- "top_audio_mux1_sel",
+ clock-names = "top_audio_mux1_sel",
"top_audio_mux2_sel",
- "top_audio_mux1_div",
- "top_audio_mux2_div",
- "top_audio_48k_timing",
- "top_audio_44k_timing",
- "top_audpll_mux_sel",
- "top_apll_sel",
- "top_aud1_pll_98M",
- "top_aud2_pll_90M",
- "top_hadds2_pll_98M",
- "top_hadds2_pll_294M",
- "top_audpll",
- "top_audpll_d4",
- "top_audpll_d8",
- "top_audpll_d16",
- "top_audpll_d24",
- "top_audintbus_sel",
- "clk_26m",
- "top_syspll1_d4",
- "top_aud_k1_src_sel",
- "top_aud_k2_src_sel",
- "top_aud_k3_src_sel",
- "top_aud_k4_src_sel",
- "top_aud_k5_src_sel",
- "top_aud_k6_src_sel",
- "top_aud_k1_src_div",
- "top_aud_k2_src_div",
- "top_aud_k3_src_div",
- "top_aud_k4_src_div",
- "top_aud_k5_src_div",
- "top_aud_k6_src_div",
- "top_aud_i2s1_mclk",
- "top_aud_i2s2_mclk",
- "top_aud_i2s3_mclk",
- "top_aud_i2s4_mclk",
- "top_aud_i2s5_mclk",
- "top_aud_i2s6_mclk",
- "top_asm_m_sel",
- "top_asm_h_sel",
- "top_univpll2_d4",
- "top_univpll2_d2",
- "top_syspll_d5";
+ "i2s0_src_sel",
+ "i2s1_src_sel",
+ "i2s2_src_sel",
+ "i2s3_src_sel",
+ "i2s0_src_div",
+ "i2s1_src_div",
+ "i2s2_src_div",
+ "i2s3_src_div",
+ "i2s0_mclk_en",
+ "i2s1_mclk_en",
+ "i2s2_mclk_en",
+ "i2s3_mclk_en",
+ "i2so0_hop_ck",
+ "i2so1_hop_ck",
+ "i2so2_hop_ck",
+ "i2so3_hop_ck",
+ "i2si0_hop_ck",
+ "i2si1_hop_ck",
+ "i2si2_hop_ck",
+ "i2si3_hop_ck",
+ "asrc0_out_ck",
+ "asrc1_out_ck",
+ "asrc2_out_ck",
+ "asrc3_out_ck",
+ "audio_afe_pd",
+ "audio_afe_conn_pd",
+ "audio_a1sys_pd",
+ "audio_a2sys_pd",
+ "audio_mrgif_pd";
+
+ assigned-clocks = <&topckgen CLK_TOP_AUD_MUX1_SEL>,
+ <&topckgen CLK_TOP_AUD_MUX2_SEL>,
+ <&topckgen CLK_TOP_AUD_MUX1_DIV>,
+ <&topckgen CLK_TOP_AUD_MUX2_DIV>;
+ assigned-clock-parents = <&topckgen CLK_TOP_AUD1PLL_98M>,
+ <&topckgen CLK_TOP_AUD2PLL_90M>;
+ assigned-clock-rates = <0>, <0>, <49152000>, <45158400>;
};
--
2.15.1
^ permalink raw reply related
* [RESEND PATCH v2 15/15] arm64: dts: msm8996: db820c: Add sound card support
From: Srinivas Kandagatla @ 2018-01-03 16:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180103002229.GV478@tuxbook>
Thanks for the comments,
On 03/01/18 00:22, Bjorn Andersson wrote:
> On Thu 14 Dec 09:34 PST 2017, srinivas.kandagatla at linaro.org wrote:
>
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> This patch adds hdmi sound card support to db820c via qdsp.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi | 5 +++++
>> arch/arm64/boot/dts/qcom/msm8996.dtsi | 33 ++++++++++++++++++++++++++++
>> 2 files changed, 38 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
>> index 9769053957af..b955769b100d 100644
>> --- a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
>> @@ -190,6 +190,11 @@
>> };
>> };
>>
>> + snd {
>> + compatible = "qcom,apq8096-sndcard";
>> + qcom,model = "DB820c";
>> + iommus = <&lpass_q6_smmu 1>;
>> + };
>>
>> gpio_keys {
>> compatible = "gpio-keys";
>> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
>> index a144cec7bb71..25c43fb8ab49 100644
>> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
>> @@ -1262,6 +1262,7 @@
>>
>> phys = <&hdmi_phy>;
>> phy-names = "hdmi_phy";
>> + #sound-dai-cells = <0>;
>>
>> ports {
>> #address-cells = <1>;
>> @@ -1297,6 +1298,33 @@
>> "ref_clk";
>> };
>> };
>> +
>> + lpass_q6_smmu: arm,smmu-lpass_q6 at 1600000 {
>
> name this node "iommu"
will rename it to arm,smmu at 1600000
>
>> + compatible = "qcom,msm8996-smmu-v2";
>> + reg = <0x1600000 0x20000>;
>> + #iommu-cells = <1>;
>> + power-domains = <&gcc HLOS1_VOTE_LPASS_CORE_GDSC>;
>
> Indentation
sure.
>
>> +
>> + #global-interrupts = <1>;
>> + interrupts = <GIC_SPI 404 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 393 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 394 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 395 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 396 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 397 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 398 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 399 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 400 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 401 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 402 IRQ_TYPE_LEVEL_HIGH>,
>> + <GIC_SPI 403 IRQ_TYPE_LEVEL_HIGH>;
>> +
>> + clocks = <&gcc GCC_HLOS1_VOTE_LPASS_CORE_SMMU_CLK>,
>> + <&gcc GCC_HLOS1_VOTE_LPASS_ADSP_SMMU_CLK>;
>> + clock-names = "iface", "bus";
>> + status = "okay";
>> + };
>> };
>>
>> adsp-pil {
>> @@ -1325,6 +1353,11 @@
>> qcom,ipc = <&apcs 16 8>;
>> qcom,smd-edge = <1>;
>> qcom,remote-pid = <2>;
>> +
>> + apr {
>
> "apr-audio-svc", as this is not the only apr channel on this edge.
yep.
>
>> + compatible = "qcom,apr-msm8996";
>> + qcom,smd-channels = "apr_audio_svc";
>> + };
>> };
>
> Regards,
> Bjorn
>
^ permalink raw reply
* [RESEND PATCH v2 14/15] ASoC: qcom: apq8096: Add db820c machine driver
From: Srinivas Kandagatla @ 2018-01-03 16:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180103001654.GU478@tuxbook>
Thanks for the review comments.
On 03/01/18 00:16, Bjorn Andersson wrote:
> On Thu 14 Dec 09:34 PST 2017, srinivas.kandagatla at linaro.org wrote:
>
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> uThis patch adds support to DB820c machine driver.
>
> Drop 'u' and expand the message to claim that this is the machine driver
> for 8996, used by the db820c.
>
> [..]
>> +static struct snd_soc_dai_link msm8996_dai_links[] = {
>
> Are there any differences between the DAI links of apq8096 and msm8996?
>
This driver is more of board specific,
Am not 100% sure about msm8996, on apq8096 in particularly on db820c we
got hdmi and analog audio connected via slimbus and also I2S on lowspeed
connector.
>> + /* FrontEnd DAI Links */
>> + {
>> + .name = "MultiMedia1 Playback",
>> + .stream_name = "MultiMedia1",
>> + .cpu_dai_name = "MM_DL1",
>> + .platform_name = "q6asm_dai",
>> + .dynamic = 1,
>> + .dpcm_playback = 1,
>> +
>> + .codec_dai_name = "snd-soc-dummy-dai",
>> + .codec_name = "snd-soc-dummy",
>> + },
>> + /* Backend DAI Links */
>> + {
>> + .name = "HDMI Playback",
>> + .stream_name = "q6afe_dai",
>> + .cpu_dai_name = "HDMI",
>> + .platform_name = "q6routing",
>> + .no_pcm = 1,
>> + .dpcm_playback = 1,
>> + .be_hw_params_fixup = msm8996_be_hw_params_fixup,
>> + .codec_dai_name = "i2s-hifi",
>> + .codec_name = "hdmi-audio-codec.0.auto",
>> + },
>> +};
>> +
>> +static int apq8096_sbc_parse_of(struct snd_soc_card *card)
>
> msm8996_parse_of()
sure if it helps.
>
>> +{
>> + struct device *dev = card->dev;
>> + int ret;
>> +
>> + ret = snd_soc_of_parse_card_name(card, "qcom,model");
>> + if (ret)
>> + dev_err(dev, "Error parsing card name: %d\n", ret);
>> +
>> + return ret;
>> +}
>> +
>> +static int msm_snd_apq8096_probe(struct platform_device *pdev)
>
> msm_snd_msm8996_probe()?
sure
>
>> +{
>> + int ret;
>> + struct snd_soc_card *card;
>> +
>> + card = devm_kzalloc(&pdev->dev, sizeof(*card), GFP_KERNEL);
>> + if (!card)
>> + return -ENOMEM;
>> +
>> + card->dev = &pdev->dev;
>> +
>> + ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32));
>> + if (ret)
>> + return ret;
>> +
>> + card->dai_link = msm8996_dai_links;
>> + card->num_links = ARRAY_SIZE(msm8996_dai_links);
>> +
>> + ret = apq8096_sbc_parse_of(card);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Error parsing OF data\n");
>
> No need to print in both parse_of() and here.
>
yep.
>> + return ret;
>> + }
>> +
>> + ret = devm_snd_soc_register_card(&pdev->dev, card);
>> + if (ret)
>> + dev_err(&pdev->dev, "sound card register failed (%d)!\n", ret);
>> + else
>> + dev_err(&pdev->dev, "sound card register Sucessfull\n");
>
> This isn't an error, skip the print here.
>
yep.
>> +
>> + return ret;
>> +}
>> +
>> +static const struct of_device_id msm_snd_apq8096_dt_match[] = {
>> + {.compatible = "qcom,apq8096-sndcard"},
>> + {}
>> +};
>> +
>> +static struct platform_driver msm_snd_apq8096_driver = {
>> + .probe = msm_snd_apq8096_probe,
>> + .driver = {
>> + .name = "msm-snd-apq8096",
>> + .owner = THIS_MODULE,
>
> Drop the .owner
>
yep
> Regards,
> Bjorn
>
^ permalink raw reply
* [RESEND PATCH v2 13/15] dt-bindings: sound: qcom: Add devicetree bindings for apq8096
From: Srinivas Kandagatla @ 2018-01-03 16:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180103002835.GW478@tuxbook>
On 03/01/18 00:28, Bjorn Andersson wrote:
> On Thu 14 Dec 09:34 PST 2017, srinivas.kandagatla at linaro.org wrote:
>
>> +++ b/Documentation/devicetree/bindings/sound/qcom,apq8096.txt
>
> Wouldn't it be possible to describe all(?) qdsp based machines in this
> one document? I.e. should we name it a little bit more generic?
You mean like downstream ?
I see no harm in trying it out and see how it looks like.
>
>> @@ -0,0 +1,22 @@
>> +* Qualcomm Technologies APQ8096 ASoC sound card driver
>> +
>> +This binding describes the APQ8096 sound card, which uses qdsp for audio.
>> +
>> +- compatible:
>> + Usage: required
>> + Value type: <stringlist>
>> + Definition: must be "qcom,apq8096-sndcard"
>> +
>> +- qcom,audio-routing:
>> + Usage: Optional
>> + Value type: <stringlist>
>> + Definition: A list of the connections between audio components.
>
> Double space before A
yep.
>
>> + Each entry is a pair of strings, the first being the
>> + connection's sink, the second being the connection's
>> + source. Valid names could be power supplies, MicBias
>> + of codec and the jacks on the board:
>> +Example:
>> + sound {
>> + compatible = "qcom,snd-apq8096";
>
> Indentation
yep.
>
>> + qcom,model = "DB820c";
>> + };
>
> Regards,
> Bjorn
>
^ permalink raw reply
* [RESEND PATCH v2 12/15] ASoC: qcom: qdsp6: Add support to q6asm dai driver
From: Srinivas Kandagatla @ 2018-01-03 16:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180103000306.GT478@tuxbook>
Thanks for the comments.
On 03/01/18 00:03, Bjorn Andersson wrote:
> On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla at linaro.org wrote:
>
> [..]
>> +
>> +enum stream_state {
>> + IDLE = 0,
>> + STOPPED,
>> + RUNNING,
>
> These are too generic.
>
Yep, I will prefix them with Q6ASM.
>> +};
>> +
>> +struct q6asm_dai_rtd {
>> + struct snd_pcm_substream *substream;
>> + dma_addr_t phys;
>> + unsigned int pcm_size;
>> + unsigned int pcm_count;
>> + unsigned int pcm_irq_pos; /* IRQ position */
>> + unsigned int periods;
>> + uint16_t bits_per_sample;
>> + uint16_t source; /* Encoding source bit mask */
>> +
>> + struct audio_client *audio_client;
>> + uint16_t session_id;
>> +
>> + enum stream_state state;
>> + bool set_channel_map;
>> + char channel_map[8];
>
> There's a define for this 8
Yes, this is max channels.
>
>> +};
>> +
>> +struct q6asm_dai_data {
>> + u64 sid;
>> +};
>> +
>> +static struct snd_pcm_hardware q6asm_dai_hardware_playback = {
>> + .info = (SNDRV_PCM_INFO_MMAP |
>> + SNDRV_PCM_INFO_BLOCK_TRANSFER |
>> + SNDRV_PCM_INFO_MMAP_VALID |
>> + SNDRV_PCM_INFO_INTERLEAVED |
>> + SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME),
>> + .formats = (SNDRV_PCM_FMTBIT_S16_LE |
>> + SNDRV_PCM_FMTBIT_S24_LE),
>> + .rates = SNDRV_PCM_RATE_8000_192000,
>> + .rate_min = 8000,
>> + .rate_max = 192000,
>> + .channels_min = 1,
>> + .channels_max = 8,
>> + .buffer_bytes_max = (PLAYBACK_MAX_NUM_PERIODS *
>> + PLAYBACK_MAX_PERIOD_SIZE),
>> + .period_bytes_min = PLAYBACK_MIN_PERIOD_SIZE,
>> + .period_bytes_max = PLAYBACK_MAX_PERIOD_SIZE,
>> + .periods_min = PLAYBACK_MIN_NUM_PERIODS,
>> + .periods_max = PLAYBACK_MAX_NUM_PERIODS,
>
> If you just put the numbers here, instead of using the PLAYBACK_
> defines, it's possible to grok the values of this struct without having
> to jump to the defines for each one.
This is usually done this way in may other drivers!,
>
>> + .fifo_size = 0,
>> +};
>> +
>> +/* Conventional and unconventional sample rate supported */
>> +static unsigned int supported_sample_rates[] = {
>> + 8000, 11025, 12000, 16000, 22050, 24000, 32000, 44100, 48000,
>> + 88200, 96000, 176400, 192000
>> +};
>> +
>> +static struct snd_pcm_hw_constraint_list constraints_sample_rates = {
>
It is used in q6asm_dai_open().
>
>> + .count = ARRAY_SIZE(supported_sample_rates),
>> + .list = supported_sample_rates,
>> + .mask = 0,
>> +};
>> +
>
>> +
>> +static int q6asm_dai_prepare(struct snd_pcm_substream *substream)
>> +{
>> + struct snd_pcm_runtime *runtime = substream->runtime;
>> + struct snd_soc_pcm_runtime *soc_prtd = substream->private_data;
>> + struct q6asm_dai_rtd *prtd = runtime->private_data;
>> + struct q6asm_dai_data *pdata;
>> + int ret;
>> +
>> + pdata = dev_get_drvdata(soc_prtd->platform->dev);
>> + if (!pdata)
>> + return -EINVAL;
>> +
>> + if (!prtd || !prtd->audio_client) {
>> + pr_err("%s: private data null or audio client freed\n",
>> + __func__);
>> + return -EINVAL;
>> + }
>> +
>> + prtd->pcm_count = snd_pcm_lib_period_bytes(substream);
>> + prtd->pcm_irq_pos = 0;
>> + /* rate and channels are sent to audio driver */
>> + if (prtd->state) {
>> + /* clear the previous setup if any */
>> + q6asm_cmd(prtd->audio_client, CMD_CLOSE);
>> + q6asm_unmap_memory_regions(substream->stream,
>> + prtd->audio_client);
>> + q6routing_dereg_phy_stream(soc_prtd->dai_link->id,
>> + SNDRV_PCM_STREAM_PLAYBACK);
>> + }
>> +
>> + ret = q6asm_map_memory_regions(substream->stream, prtd->audio_client,
>> + prtd->phys,
>> + (prtd->pcm_size / prtd->periods),
>> + prtd->periods);
>> +
>> + if (ret < 0) {
>> + pr_err("Audio Start: Buffer Allocation failed rc = %d\n",
>> + ret);
>> + return -ENOMEM;
>> + }
>> +
>> + ret = q6asm_open_write(prtd->audio_client, FORMAT_LINEAR_PCM,
>> + prtd->bits_per_sample);
>> + if (ret < 0) {
>> + pr_err("%s: q6asm_open_write failed\n", __func__);
>> + q6asm_audio_client_free(prtd->audio_client);
>> + prtd->audio_client = NULL;
>
> Do you need to roll back the q6asm_map_memory_regions?
>
yes you are correct, we should roll back the map.
>> + return -ENOMEM;
>> + }
>> +
>> + prtd->session_id = q6asm_get_session_id(prtd->audio_client);
>> + ret = q6routing_reg_phy_stream(soc_prtd->dai_link->id, LEGACY_PCM_MODE,
>> + prtd->session_id, substream->stream);
>> + if (ret) {
>> + pr_err("%s: stream reg failed ret:%d\n", __func__, ret);
>> + return ret;
>> + }
>> +
>> + ret = q6asm_media_format_block_multi_ch_pcm(
>> + prtd->audio_client, runtime->rate,
>> + runtime->channels, !prtd->set_channel_map,
>> + prtd->channel_map, prtd->bits_per_sample);
>
> set_channel_map and channel_map aren't referenced elsewhere. If this
> isn't used consider removing it for now.
>
Will take a closer look before sending next version.
>> + if (ret < 0)
>> + pr_info("%s: CMD Format block failed\n", __func__);
>> +
>> + prtd->state = RUNNING;
>> +
>> + return 0;
>> +}
>> +
> [..]
>> +static int q6asm_dai_pcm_new(struct snd_soc_pcm_runtime *rtd)
>> +{
>> + struct snd_pcm *pcm = rtd->pcm;
>> + struct snd_pcm_substream *substream;
>> + struct snd_card *card = rtd->card->snd_card;
>> + struct device *dev = card->dev;
>> + struct device_node *node = dev->of_node;
>> + struct q6asm_dai_data *pdata = dev_get_drvdata(rtd->platform->dev);
>> + struct of_phandle_args args;
>> +
>> + int size, ret = 0;
>> +
>> + ret = of_parse_phandle_with_fixed_args(node, "iommus", 1, 0, &args);
>> + if (ret < 0)
>> + pdata->sid = -1;
>> + else
>> + pdata->sid = args.args[0];
>> +
>
> Is this really how you're supposed to deal with the iommu?
>
Any suggestions are welcome, I did not find a better way to append sid
to iova address from iommu.
Currently downstream abstracts this in ion apis.
>> +
>> +
>> + substream = pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream;
>> + size = q6asm_dai_hardware_playback.buffer_bytes_max;
>> + ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, dev, size,
>> + &substream->dma_buffer);
>> + if (ret) {
>> + dev_err(dev, "Cannot allocate buffer(s)\n");
>> + return ret;
>
> Just fall through.
>
yep
>> + }
>> +
>> + return ret;
>> +}
>> +
> [..]
>> +static struct snd_soc_dai_driver q6asm_fe_dais[] = {
>> + {
>> + .playback = {
>> + .stream_name = "MultiMedia1 Playback",
>> + .rates = (SNDRV_PCM_RATE_8000_192000|
>> + SNDRV_PCM_RATE_KNOT),
>> + .formats = (SNDRV_PCM_FMTBIT_S16_LE |
>> + SNDRV_PCM_FMTBIT_S24_LE),
>> + .channels_min = 1,
>> + .channels_max = 8,
>> + .rate_min = 8000,
>> + .rate_max = 192000,
>> + },
>> + .name = "MM_DL1",
>> + .probe = fe_dai_probe,
>> + .id = MSM_FRONTEND_DAI_MULTIMEDIA1,
>> + },
>> + {
>> + .playback = {
>> + .stream_name = "MultiMedia2 Playback",
>> + .rates = (SNDRV_PCM_RATE_8000_192000|
>> + SNDRV_PCM_RATE_KNOT),
>> + .formats = (SNDRV_PCM_FMTBIT_S16_LE |
>> + SNDRV_PCM_FMTBIT_S24_LE),
>> + .channels_min = 1,
>> + .channels_max = 8,
>> + .rate_min = 8000,
>> + .rate_max = 192000,
>
> I presume the listed frontend DAIs needs to match the firmware of the
> DSP (and features of hardware)? Can we get away with a single list for
> all versions of the adsp?
>
Yes, DSP supports 8 concurrent streams both playback and record streams.
For now I have only added two entires to keep the patch simple but this
should be ideally 8 entries.
> In msm-4.4 the max rate for these where changed to 384000, see:
>
> 9c46f74b2724 ("ASoC: msm: add 384KHz playback support")
sure i will include that in next version.
>
>> + },
>> + .name = "MM_DL2",
>> + .probe = fe_dai_probe,
>> + .id = MSM_FRONTEND_DAI_MULTIMEDIA2,
>> + },
>> +};
>> +
>
> Regards,
> Bjorn
>
^ permalink raw reply
* [RESEND PATCH v2 11/15] ASoC: qcom: qdsp6: Add support to q6afe dai driver
From: Srinivas Kandagatla @ 2018-01-03 16:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180102232811.GS478@tuxbook>
On 02/01/18 23:28, Bjorn Andersson wrote:
> On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla at linaro.org wrote:
>
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> This patch adds support to q6afe backend dais driver.
>>
>
> Isn't the list of backend DAIs platform-dependent?
dai links and connections between backend and front ends are platform
dependent.
>
> [..]
>> +static const struct snd_soc_dapm_widget hdmi_dapm_widgets[] = {
>> + SND_SOC_DAPM_AIF_OUT("HDMI", "HDMI Playback", 0, 0, 0, 0),
>> + SND_SOC_DAPM_OUTPUT("HDMI-RX"),
>> +};
>> +
>> +static const struct snd_soc_component_driver msm_dai_hdmi_q6_component = {
>
> How will this look beyond HDMI? I'm having issues mapping this to
> downstream.
ex:
For slimbus dais, we would have more entries of "struct
snd_soc_dai_driver" in this file.
Basically these are the dais that are exposed by the dsp firmware.
Depending on the actually platform some of these dais would be setup
accordingly.
>
>> + .name = "msm-dai-q6-hdmi",
>> + .dapm_widgets = hdmi_dapm_widgets,
>> + .num_dapm_widgets = ARRAY_SIZE(hdmi_dapm_widgets),
>> + .controls = hdmi_config_controls,
>> + .num_controls = ARRAY_SIZE(hdmi_config_controls),
>> + .dapm_routes = hdmi_dapm_routes,
>> + .num_dapm_routes = ARRAY_SIZE(hdmi_dapm_routes),
>> +};
>> +
>
> Regards,
> Bjorn
>
^ permalink raw reply
* [RESEND PATCH v2 10/15] ASoC: qcom: qdsp6: Add support to q6routing driver
From: Srinivas Kandagatla @ 2018-01-03 16:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180102230007.GR478@tuxbook>
On 02/01/18 23:00, Bjorn Andersson wrote:
> On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla at linaro.org wrote:
>
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> This patch adds support to q6 routing driver which configures route
>> between ASM and AFE module using ADM apis.
>>
>> This driver uses dapm widgets to setup the matrix between AFE ports and
>> ASM streams.
>>
>
> Why is this a separate driver from the q6adm?
This is actually exposing the mixer controls for routing streams on to
different audio sink/sources using a matrix.
>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> sound/soc/qcom/Kconfig | 5 +
>> sound/soc/qcom/qdsp6/Makefile | 1 +
>> sound/soc/qcom/qdsp6/q6routing.c | 386 +++++++++++++++++++++++++++++++++++++++
>> sound/soc/qcom/qdsp6/q6routing.h | 9 +
>> 4 files changed, 401 insertions(+)
>> create mode 100644 sound/soc/qcom/qdsp6/q6routing.c
>> create mode 100644 sound/soc/qcom/qdsp6/q6routing.h
>> diff --git a/sound/soc/qcom/qdsp6/q6routing.c b/sound/soc/qcom/qdsp6/q6routing.c
>> +/**
>> + * q6routing_reg_phy_stream() - Register a new stream for route setup
>> + *
>> + * @fedai_id: Frontend dai id.
>> + * @perf_mode: Performace mode.
>
> "Performance"
yep.
>
>> + * @stream_id: ASM stream id to map.
>> + * @stream_type: Direction of stream
>> + *
>> + * Return: Will be an negative on error or a zero on success.
>> + */
>> +int q6routing_reg_phy_stream(int fedai_id, int perf_mode,
>
> q6routing_stream_open() ?
sure if it helps reading.
>
>> + int stream_id, int stream_type)
>> +{
>> + int j, topology, num_copps = 0;
>> + struct route_payload payload;
>> + int copp_idx;
>> + struct session_data *session;
>> +
>> + if (!routing_data) {
>> + pr_err("Routing driver not yet ready\n");
>> + return -EINVAL;
>> + }
>> +
>> + session = &routing_data->sessions[stream_id - 1];
>> + mutex_lock(&routing_data->lock);
>> + session->fedai_id = fedai_id;
>> + payload.num_copps = 0; /* only RX needs to use payload */
>> + topology = NULL_COPP_TOPOLOGY;
>> + copp_idx = q6adm_open(routing_data->dev, session->port_id,
>> + session->path_type, session->sample_rate,
>> + session->channels, topology, perf_mode,
>> + session->bits_per_sample, 0, 0);
>> + if ((copp_idx < 0) || (copp_idx >= MAX_COPPS_PER_PORT)) {
>
> Make q6adm_open() not return >= MAX_COPPS_PER_PORT.
>
> And drop the extra parenthesis.
>
>> + mutex_unlock(&routing_data->lock);
>> + return -EINVAL;
>> + }
>> +
>> + set_bit(copp_idx, &session->copp_map);
>> + for (j = 0; j < MAX_COPPS_PER_PORT; j++) {
>
> Use for_each_set_bit()
>
>> + unsigned long copp = session->copp_map;
>> +
>> + if (test_bit(j, &copp)) {
>> + payload.port_id[num_copps] = session->port_id;
>> + payload.copp_idx[num_copps] = j;
>> + num_copps++;
>> + }
>> + }
>> +
>> + if (num_copps) {
>> + payload.num_copps = num_copps;
>> + payload.session_id = stream_id;
>> + q6adm_matrix_map(routing_data->dev, session->path_type,
>> + payload, perf_mode);
>> + }
>> + mutex_unlock(&routing_data->lock);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(q6routing_reg_phy_stream);
>> +
>> +static struct session_data *routing_get_session(struct msm_routing_data *data,
>> + int port_id, int port_type)
>
> port_type is ignored
It will be used once we add capture support.
>
>> +{
>> + int i;
>> +
>> + for (i = 0; i < MAX_SESSIONS; i++)
>> + if (port_id == data->sessions[i].port_id)
>> + return &data->sessions[i];
>> +
>> + return NULL;
>> +}
>> +
>> +/**
>> + * q6routing_dereg_phy_stream() - Deregister a stream
>> + *
>> + * @fedai_id: Frontend dai id.
>> + * @stream_type: Direction of stream
>> + *
>> + * Return: Will be an negative on error or a zero on success.
>> + */
>> +void q6routing_dereg_phy_stream(int fedai_id, int stream_type)
>
> q6routing_stream_close()?
>
will rename it.
>> +{
>> + struct session_data *session;
>> + int idx;
>> +
>> + session = get_session_from_id(routing_data, fedai_id);
>> + if (!session)
>> + return;
>> +
>> + for_each_set_bit(idx, &session->copp_map, MAX_COPPS_PER_PORT)
>> + q6adm_close(routing_data->dev, session->port_id,
>> + session->perf_mode, idx);
>> +
>> + session->fedai_id = -1;
>> + session->copp_map = 0;
>> +}
>> +EXPORT_SYMBOL_GPL(q6routing_dereg_phy_stream);
>> +
>> +
>> +
>> +static const struct snd_soc_dapm_widget msm_qdsp6_widgets[] = {
>> + /* Frontend AIF */
>> + /* Widget name equals to Front-End DAI name<Need confirmation>,
>
> Perhaps this should be confirmed and the comment updated?
>
Some leftover from old code...
>> + * Stream name must contains substring of front-end dai name
>> + */
>> + SND_SOC_DAPM_AIF_IN("MM_DL1", "MultiMedia1 Playback", 0, 0, 0, 0),
>> + SND_SOC_DAPM_AIF_IN("MM_DL2", "MultiMedia2 Playback", 0, 0, 0, 0),
>> + SND_SOC_DAPM_AIF_IN("MM_DL3", "MultiMedia3 Playback", 0, 0, 0, 0),
>> + SND_SOC_DAPM_AIF_IN("MM_DL4", "MultiMedia4 Playback", 0, 0, 0, 0),
>> + SND_SOC_DAPM_AIF_IN("MM_DL5", "MultiMedia5 Playback", 0, 0, 0, 0),
>> + SND_SOC_DAPM_AIF_IN("MM_DL6", "MultiMedia6 Playback", 0, 0, 0, 0),
>> + SND_SOC_DAPM_AIF_IN("MM_DL7", "MultiMedia7 Playback", 0, 0, 0, 0),
>> + SND_SOC_DAPM_AIF_IN("MM_DL8", "MultiMedia8 Playback", 0, 0, 0, 0),
>> +
>> + /* Mixer definitions */
>> + SND_SOC_DAPM_MIXER("HDMI Mixer", SND_SOC_NOPM, 0, 0,
>> + hdmi_mixer_controls,
>> + ARRAY_SIZE(hdmi_mixer_controls)),
>> +};
>
>> +static int routing_hw_params(struct snd_pcm_substream *substream,
>> + struct snd_pcm_hw_params *params)
>> +{
>> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> + unsigned int be_id = rtd->cpu_dai->id;
>> + struct snd_soc_platform *platform = rtd->platform;
>> + struct msm_routing_data *data = snd_soc_platform_get_drvdata(platform);
>> + struct session_data *session;
>> + int port_id, port_type, path_type, bits_per_sample;
>
> bits_per_sample is likely unused.
>
yep.
>> +
>> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>> + path_type = ADM_PATH_PLAYBACK;
>> + port_type = MSM_AFE_PORT_TYPE_RX;
>> + }
>> +
>> + port_id = be_id;
>
> Why alias this variable?
will fix this in next version.
>
>> +
>> + session = routing_get_session(data, port_id, port_type);
>> +
>> + if (!session) {
>> + pr_err("No session matrix setup yet..\n");
>> + return -EINVAL;
>> + }
>> +
>> + mutex_lock(&data->lock);
>> +
>> + session->path_type = path_type;
>> + session->sample_rate = params_rate(params);
>> + session->channels = params_channels(params);
>> + session->format = params_format(params);
>> +
>> + if (session->format == SNDRV_PCM_FORMAT_S16_LE)
>> + session->bits_per_sample = 16;
>> + else if (session->format == SNDRV_PCM_FORMAT_S24_LE)
>> + bits_per_sample = 24;
>
> session-> ?
I agree, will fix it.
>
> Perhaps switch on params_format(params) instead? And fail in the default
> case.
will give that a go.
>
>> +
>> + mutex_unlock(&data->lock);
>> + return 0;
>> +}
>> +
>> +static struct snd_pcm_ops q6pcm_routing_ops = {
>> + .hw_params = routing_hw_params,
>> + .close = routing_close,
>> + .prepare = routing_prepare,
>> +};
>> +
>> +/* Not used but frame seems to require it */
>
> Remove comment?
>
looks like leftover.. will remove it.
[...]
>> +
>> +static int q6pcm_routing_remove(struct platform_device *pdev)
>> +{
>
> As you return here routing_data will be freed. The early check in
> q6routing_reg_phy_stream() seems to indicate that this driver can be
> called even though the routing device isn't available.
>
> So you probably want to clear that variable, at least.
sure, will fix this in next version.
>
>> + return 0;
>> +}
>> +
>> +static struct platform_driver q6pcm_routing_driver = {
>> + .driver = {
>> + .name = "q6routing",
>> + .owner = THIS_MODULE,
>
> Drop .owner
>
yep.
>> + },
>> + .probe = q6pcm_routing_probe,
>> + .remove = q6pcm_routing_remove,
>> +};
>>
^ permalink raw reply
* [RESEND PATCH v2 09/15] ASoC: qcom: qdsp6: Add support to Q6CORE
From: Srinivas Kandagatla @ 2018-01-03 16:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180102221520.GQ478@tuxbook>
Thanks for the review.
On 02/01/18 22:15, Bjorn Andersson wrote:
> On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla at linaro.org wrote:
>
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> This patch adds support to core apr service, which is used to query
>> status of other static and dynamic services on the dsp.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> sound/soc/qcom/Kconfig | 5 +
>> sound/soc/qcom/qdsp6/Makefile | 1 +
>> sound/soc/qcom/qdsp6/q6core.c | 227 ++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 233 insertions(+)
>> create mode 100644 sound/soc/qcom/qdsp6/q6core.c
>>
>> +obj-$(CONFIG_SND_SOC_QDSP6_CORE) += q6core.o
>> diff --git a/sound/soc/qcom/qdsp6/q6core.c b/sound/soc/qcom/qdsp6/q6core.c
>> new file mode 100644
>> index 000000000000..d4e2dbc62489
>> +struct q6core {
>> + struct apr_device *adev;
>> + wait_queue_head_t wait;
>> + uint32_t avcs_state;
>> + int resp_received;
>
> bool
yep.
>
>> + uint32_t num_services;
>> + struct avcs_svc_info *svcs_info;
>> +};
>> +static int core_callback(struct apr_device *adev, struct apr_client_data *data)
>> +{
>> + struct q6core *core = dev_get_drvdata(&adev->dev);
>> + uint32_t *payload;
>> +
>> + switch (data->opcode) {
>> + case AVCS_GET_VERSIONS_RSP:
>> + payload = data->payload;
>> + core->num_services = payload[1];
>
> Describe the payload in a struct (with flexible array member for the
> svcs_info list).
sure!
>
>> +
>> + if (!core->svcs_info)
>> + core->svcs_info = kcalloc(core->num_services,
>> + sizeof(*core->svcs_info),
>> + GFP_ATOMIC);
>> + if (!core->svcs_info)
>> + return -ENOMEM;
>> +
>
> If we receive this twice with different num_services for some reason the
> memcpy might trash the heap.
>
> But as this is the get_version response and we're only going to issue
> that once you should remove the check for !core->svcs_info above.
>
yes, I agree
> And don't forget to free svcs_info once you have added your services.
yep.
>
>> + /* svc info is after 8 bytes */
>> + memcpy(core->svcs_info, payload + 2,
>> + core->num_services * sizeof(*core->svcs_info));
>> +
>> + core->resp_received = 1;
>> + wake_up(&core->wait);
>> +
>> + break;
>> + case AVCS_CMDRSP_ADSP_EVENT_GET_STATE:
>> + payload = data->payload;
>> + core->avcs_state = payload[0];
>> +
>> + core->resp_received = 1;
>> + wake_up(&core->wait);
>> + break;
>> + default:
>> + dev_err(&adev->dev, "Message id from adsp core svc: 0x%x\n",
>> + data->opcode);
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +void q6core_add_service(struct device *dev, uint32_t svc_id, uint32_t version)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(static_services); i++) {
>> + if (static_services[i].svc_id == svc_id) {
>> + static_services[i].svc_version = version;
>> + apr_add_device(dev->parent, &static_services[i]);
>> + dev_info(dev,
>
> Please don't spam the logs, dev_dbg() should be enough. And as
> apr_add_device() returns you can find the devices registered in /sys
sure!
>
>> + "Adding SVC: %s: id 0x%x API Ver 0x%x:0x%x\n",
>> + static_services[i].name, svc_id,
>> + APR_SVC_MAJOR_VERSION(version),
>> + APR_SVC_MINOR_VERSION(version));
>> + }
>> + }
>> +}
>> +
>> +static void q6core_add_static_services(struct q6core *core)
>
> The name of this function is deceiving, it doesn't really add the static
> services. It adds devices for the services that we've been informed
> exists, by the other side - using the static list of services.
>
> Per the comment on a previous patch I don't think the "name" in
> apr_device_id provides any real value and in this case if forces you to
> perform a lookup using this table.
>
> If you drop the name, you can loop over the list of service ids returned
> from the remote and just register them with a hard coded domain id
> (based on apr instance?) and client_id. You don't need the lookup table.
>
yes, correct.
>> +{
>> + int i;
>> + struct apr_device *adev = core->adev;
>> + struct avcs_svc_info *svcs_info = core->svcs_info;
>> +
>> + for (i = 0; i < core->num_services; i++)
>> + q6core_add_service(&adev->dev, svcs_info[i].service_id,
>> + svcs_info[i].version);
>> +}
>> +
>> +static int q6core_get_svc_versions(struct q6core *core)
>> +{
>> + struct apr_device *adev = core->adev;
>> + struct apr_hdr hdr = {0};
>> + int rc;
>> +
>> + hdr.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD,
>> + APR_HDR_LEN(APR_HDR_SIZE), APR_PKT_VER);
>> + hdr.pkt_size = APR_PKT_SIZE(APR_HDR_SIZE, 0);
>> + hdr.opcode = AVCS_GET_VERSIONS;
>> +
>> + rc = apr_send_pkt(adev, (uint32_t *)&hdr);
>> + if (rc < 0)
>> + return rc;
>> +
>> + rc = wait_event_timeout(core->wait, (core->resp_received == 1),
>> + msecs_to_jiffies(Q6_READY_TIMEOUT_MS));
>
> The wait and resp_received could favourably be expressed as a completion
> instead, as all we care about is that this happened once.
will give that a try.
>
>> + if (rc > 0 && core->resp_received) {
>> + core->resp_received = 0;
>> + return 0;
>> + }
>
> It wasn't obvious at first sight that this is the success case and the
> return rc below was the error case...
>
okay, I can add some comments here to help.
>> +
>> + return rc;
>
> And this will actually be 0 if core->resp_received has not become 1 at
> the timeout.
>
yep, will return an error value here.
>> +}
>> +
>> +static bool q6core_is_adsp_ready(struct q6core *core)
>> +{
>> + struct apr_device *adev = core->adev;
>> + struct apr_hdr hdr = {0};
>> + int rc;
>> +
>> + hdr.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD,
>> + APR_HDR_LEN(APR_HDR_SIZE), APR_PKT_VER);
>> + hdr.pkt_size = APR_PKT_SIZE(APR_HDR_SIZE, 0);
>> + hdr.opcode = AVCS_CMD_ADSP_EVENT_GET_STATE;
>> +
>> + rc = apr_send_pkt(adev, (uint32_t *)&hdr);
>> + if (rc < 0)
>> + return false;
>> +
>> + rc = wait_event_timeout(core->wait, (core->resp_received == 1),
>> + msecs_to_jiffies(Q6_READY_TIMEOUT_MS));
>
> I think this too would be nicer to describe with a completion.
>
> Currently it's possible to ask is the dsp is ready, time out and ask
> again, just to receive the first ack and continue. The service request
> sleep might then wake up on this previous ack.
>
> If you describe this by two different completions for the two waits you
> avoid any such race conditions occurring.
>
sure i will make a note of it when I try completions.
>> + if (rc > 0 && core->resp_received) {
>> + core->resp_received = 0;
>> + if (core->avcs_state == 0x1)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static int q6core_probe(struct apr_device *adev)
>> +{
>> + struct q6core *core;
>> + unsigned long timeout = jiffies +
>> + msecs_to_jiffies(ADSP_STATE_READY_TIMEOUT_MS);
>> + int ret = 0;
>> +
>> + core = devm_kzalloc(&adev->dev, sizeof(*core), GFP_KERNEL);
>> + if (!core)
>> + return -ENOMEM;
>> +
>> + dev_set_drvdata(&adev->dev, core);
>> +
>> + core->adev = adev;
>> + init_waitqueue_head(&core->wait);
>> +
>> + do {
>> + if (!q6core_is_adsp_ready(core)) {
>> + dev_info(&adev->dev, "ADSP Audio isn't ready\n");
>> + } else {
>> + dev_info(&adev->dev, "ADSP Audio is ready\n");
>> +
>> + ret = q6core_get_svc_versions(core);
>> + if (!ret)
>> + q6core_add_static_services(core);
>> +
>> + break;
>> + }
>> + } while (time_after(timeout, jiffies));
>
> This would be much better rewritten as:
>
> for (;;) {
> if (q6core_is_adsp_ready(core))
> break;
>
> if (time_after(timeout, jiffies))
> return -ETIMEDOUT;
> }
>
> ret = q6core_get_svc_versions(core);
> if (ret)
> return ret;
>
> q6core_add_static_services(core);
>
Sure.
>> +
>> + return ret;
>> +}
>> +
>> +
>> +static struct apr_driver qcom_q6core_driver = {
>> + .probe = q6core_probe,
>> + .remove = q6core_exit,
>> + .callback = core_callback,
>> + .id_table = core_id,
>> + .driver = {
>> + .name = "qcom-q6core",
>> + },
>
> Indentation.
Sure.
^ permalink raw reply
* [RESEND PATCH v2 08/15] ASoC: qcom: q6asm: add support to audio stream apis
From: Srinivas Kandagatla @ 2018-01-03 16:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180102200813.GA8625@builder>
Thanks for your comments.
On 02/01/18 20:08, Bjorn Andersson wrote:
> On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla at linaro.org wrote:
>
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> This patch adds support to open, write and media format commands
>> in the q6asm module.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> sound/soc/qcom/qdsp6/q6asm.c | 530 ++++++++++++++++++++++++++++++++++++++++++-
>> sound/soc/qcom/qdsp6/q6asm.h | 42 ++++
>> 2 files changed, 571 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/qcom/qdsp6/q6asm.c b/sound/soc/qcom/qdsp6/q6asm.c
>> index 4be92441f524..dabd6509ef99 100644
>> --- a/sound/soc/qcom/qdsp6/q6asm.c
>> +++ b/sound/soc/qcom/qdsp6/q6asm.c
>> @@ -8,16 +8,34 @@
>> #include <linux/soc/qcom/apr.h>
>> #include <linux/device.h>
>> #include <linux/platform_device.h>
>> +#include <uapi/sound/asound.h>
>> #include <linux/delay.h>
>> #include <linux/slab.h>
>> #include <linux/mm.h>
>> #include "q6asm.h"
>> #include "common.h"
>>
>> +#define ASM_STREAM_CMD_CLOSE 0x00010BCD
>> +#define ASM_STREAM_CMD_FLUSH 0x00010BCE
>> +#define ASM_SESSION_CMD_PAUSE 0x00010BD3
>> +#define ASM_DATA_CMD_EOS 0x00010BDB
>> +#define DEFAULT_POPP_TOPOLOGY 0x00010BE4
>> +#define ASM_STREAM_CMD_FLUSH_READBUFS 0x00010C09
>> #define ASM_CMD_SHARED_MEM_MAP_REGIONS 0x00010D92
>> #define ASM_CMDRSP_SHARED_MEM_MAP_REGIONS 0x00010D93
>> #define ASM_CMD_SHARED_MEM_UNMAP_REGIONS 0x00010D94
>> -
>> +#define ASM_DATA_CMD_MEDIA_FMT_UPDATE_V2 0x00010D98
>> +#define ASM_DATA_EVENT_WRITE_DONE_V2 0x00010D99
>> +#define ASM_SESSION_CMD_RUN_V2 0x00010DAA
>> +#define ASM_MEDIA_FMT_MULTI_CHANNEL_PCM_V2 0x00010DA5
>> +#define ASM_DATA_CMD_WRITE_V2 0x00010DAB
>> +#define ASM_SESSION_CMD_SUSPEND 0x00010DEC
>> +#define ASM_STREAM_CMD_OPEN_WRITE_V3 0x00010DB3
>> +
>> +#define ASM_LEGACY_STREAM_SESSION 0
>> +#define ASM_END_POINT_DEVICE_MATRIX 0
>> +#define DEFAULT_APP_TYPE 0
>> +#define TUN_WRITE_IO_MODE 0x0008 /* tunnel read write mode */
>> #define TUN_READ_IO_MODE 0x0004 /* tunnel read write mode */
>> #define SYNC_IO_MODE 0x0001
>> #define ASYNC_IO_MODE 0x0002
>
> Probably prettier to reorder these and make them Q6ASM_IO_MODE_xyz
Sure I will try that.
>
> [..]
>>
>> +static int32_t q6asm_callback(struct apr_device *adev,
>
> This callback is an extracted part of q6asm_srvc_callback(), can it be
> given a more descriptive name?
May be q6asm_stream_callback/q6asm_session_callback() should be better.
>
>> + struct apr_client_data *data, int session_id)
>> +{
>> + struct audio_client *ac;// = (struct audio_client *)priv;
>> + uint32_t token;
>> + uint32_t *payload;
>> + uint32_t wakeup_flag = 1;
>> + uint32_t client_event = 0;
>> + struct q6asm *q6asm = dev_get_drvdata(&adev->dev);
>> +
>> + if (data == NULL)
>> + return -EINVAL;
>> +
>> + ac = q6asm_get_audio_client(q6asm, session_id);
>> + if (!q6asm_is_valid_audio_client(ac))
>> + return -EINVAL;
>> +
>> + payload = data->payload;
>> +
>> + if (data->opcode == APR_BASIC_RSP_RESULT) {
>
> Move this into the switch.
Yep, will cleanup these instances.
>
>> + token = data->token;
>> + switch (payload[0]) {
>
> This is again that common response struct.
>
yep!
[...]
>> +
>> + return 0;
>> +}
>> +
>> static int q6asm_srvc_callback(struct apr_device *adev, struct apr_client_data *data)
>> {
>> struct q6asm *a, *q6asm = dev_get_drvdata(&adev->dev);
>> @@ -415,12 +581,16 @@ static int q6asm_srvc_callback(struct apr_device *adev, struct apr_client_data *
>> struct audio_port_data *port;
>> uint32_t dir = 0;
>> uint32_t sid = 0;
>> + int dest_port;
>> uint32_t *payload;
>>
>> if (!data) {
>> dev_err(&adev->dev, "%s: Invalid CB\n", __func__);
>> return 0;
>> }
>> + dest_port = (data->dest_port >> 8) & 0xFF;
>> + if (dest_port)
>> + return q6asm_callback(adev, data, dest_port);
>
> You call dest_port "session_id" above, this seems to be a better name
> for this variable.
>
yes
>>
>> payload = data->payload;
>> sid = (data->token >> 8) & 0x0F;
>> @@ -540,6 +710,364 @@ struct audio_client *q6asm_audio_client_alloc(struct device *dev,
>> }
>> EXPORT_SYMBOL_GPL(q6asm_audio_client_alloc);
>>
>> +static int __q6asm_open_write(struct audio_client *ac, uint32_t format,
>> + uint16_t bits_per_sample, uint32_t stream_id,
>> + bool is_gapless_mode)
>> +{
>> + struct asm_stream_cmd_open_write_v3 open;
>> + int rc;
>> +
>> + q6asm_add_hdr(ac, &open.hdr, sizeof(open), true, stream_id);
>> + ac->cmd_state = -1;
>> +
>> + open.hdr.opcode = ASM_STREAM_CMD_OPEN_WRITE_V3;
>> + open.mode_flags = 0x00;
>> + open.mode_flags |= ASM_LEGACY_STREAM_SESSION;
>> + if (is_gapless_mode)
>
> This is hard coded as false.
>
Will clean this up.
>> + open.mode_flags |= 1 << ASM_SHIFT_GAPLESS_MODE_FLAG;
>> +
>> + /* source endpoint : matrix */
>> + open.sink_endpointype = ASM_END_POINT_DEVICE_MATRIX;
>> + open.bits_per_sample = bits_per_sample;
>> + open.postprocopo_id = DEFAULT_POPP_TOPOLOGY;
>> +
>> + switch (format) {
>> + case FORMAT_LINEAR_PCM:
>> + open.dec_fmt_id = ASM_MEDIA_FMT_MULTI_CHANNEL_PCM_V2;
>> + break;
>> + default:
>> + dev_err(ac->dev, "Invalid format 0x%x\n", format);
>> + return -EINVAL;
>> + }
>> + rc = apr_send_pkt(ac->adev, (uint32_t *) &open);
>> + if (rc < 0)
>> + return rc;
>> +
>> + rc = wait_event_timeout(ac->cmd_wait, (ac->cmd_state >= 0), 5 * HZ);
>> + if (!rc) {
>> + dev_err(ac->dev, "timeout on open write\n");
>> + return -ETIMEDOUT;
>> + }
>
> Almost every time you apr_send_pkt() you have this wait with timeout,
> can this send/wait/return be wrapped in a helper function to reduce the
> duplication?
>
> Creating a q6asm_send_sync() and q6asm_send_async() pair with this logic
> should help quite a bit.
will do that with all the apr drivers.
>
>> +
>> + if (ac->cmd_state > 0)
>> + return adsp_err_get_lnx_err_code(ac->cmd_state);
>> +
>> + ac->io_mode |= TUN_WRITE_IO_MODE;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * q6asm_open_write() - Open audio client for writing
>> + *
>> + * @ac: audio client pointer
>> + * @format: audio sample format
>> + * @bits_per_sample: bits per sample
>> + *
>> + * Return: Will be an negative value on error or zero on success
>> + */
>> +int q6asm_open_write(struct audio_client *ac, uint32_t format,
>> + uint16_t bits_per_sample)
>> +{
>> + return __q6asm_open_write(ac, format, bits_per_sample,
>
> I don't see a particular reason for not inlining this, is there one
> coming later in the series?
No, will clean it up.
>
>> + ac->stream_id, false);
>> +}
>> +EXPORT_SYMBOL_GPL(q6asm_open_write);
>> +
>> +static int __q6asm_run(struct audio_client *ac, uint32_t flags,
>> + uint32_t msw_ts, uint32_t lsw_ts, bool wait)
>> +{
>> + struct asm_session_cmd_run_v2 run;
>> + int rc;
>> +
>> + q6asm_add_hdr(ac, &run.hdr, sizeof(run), true, ac->stream_id);
>> + ac->cmd_state = -1;
>> +
>> + run.hdr.opcode = ASM_SESSION_CMD_RUN_V2;
>> + run.flags = flags;
>> + run.time_lsw = lsw_ts;
>> + run.time_msw = msw_ts;
>> +
>> + rc = apr_send_pkt(ac->adev, (uint32_t *) &run);
>> + if (rc < 0)
>> + return rc;
>> +
>> + if (wait) {
>
> Rather than having half of the function conditional I would recommend
> inlining this function in the two callers.
>
> In particular if you can come up with a helper function for the
> send/wait/handle-error case.
sure.
>
>> + rc = wait_event_timeout(ac->cmd_wait, (ac->cmd_state >= 0),
>> + 5 * HZ);
>> + if (!rc) {
>> + dev_err(ac->dev, "timeout on run cmd\n");
>> + return -ETIMEDOUT;
>> + }
>> + if (ac->cmd_state > 0)
>> + return adsp_err_get_lnx_err_code(ac->cmd_state);
>> + }
>> +
>> + return 0;
>> +}
>>
>> +/**
>> + * q6asm_media_format_block_multi_ch_pcm() - setup pcm configuration
>> + *
>> + * @ac: audio client pointer
>> + * @rate: audio sample rate
>> + * @channels: number of audio channels.
>> + * @use_default_chmap: flag to use default ch map.
>> + * @channel_map: channel map pointer
>> + * @bits_per_sample: bits per sample
>> + *
>> + * Return: Will be an negative value on error or zero on success
>> + */
>> +int q6asm_media_format_block_multi_ch_pcm(struct audio_client *ac,
>> + uint32_t rate, uint32_t channels,
>> + bool use_default_chmap,
>> + char *channel_map,
>
> This should be u8 channel_map[PCM_FORMAT_MAX_NUM_CHANNEL], possibly
> char. Unless you, as I suggest below, want to be able to represent
> use_default_chmap = false, by setting this to NULL.
>
>> + uint16_t bits_per_sample)
>> +{
>> + struct asm_multi_channel_pcm_fmt_blk_v2 fmt;
>> + u8 *channel_mapping;
>> + int rc = 0;
>
> Unnecessary initialization.
yep.
>
>> +
>> + q6asm_add_hdr(ac, &fmt.hdr, sizeof(fmt), true, ac->stream_id);
>> + ac->cmd_state = -1;
>> +
>> + fmt.hdr.opcode = ASM_DATA_CMD_MEDIA_FMT_UPDATE_V2;
>> + fmt.fmt_blk.fmt_blk_size = sizeof(fmt) - sizeof(fmt.hdr) -
>> + sizeof(fmt.fmt_blk);
>> + fmt.num_channels = channels;
>> + fmt.bits_per_sample = bits_per_sample;
>> + fmt.sample_rate = rate;
>> + fmt.is_signed = 1;
>> +
>> + channel_mapping = fmt.channel_mapping;
>> +
>> + if (use_default_chmap) {
>
> Passing NULL as channel_map would probably be a nicer way to say this,
> instead of having a separate bool.
I will give it a go and see.
>
>> + if (q6dsp_map_channels(channel_mapping, channels)) {
>> + dev_err(ac->dev, " map channels failed %d\n", channels);
>> + return -EINVAL;
>> + }
>> + } else {
>> + memcpy(channel_mapping, channel_map,
>> + PCM_FORMAT_MAX_NUM_CHANNEL);
>> + }
>> +
>> + rc = apr_send_pkt(ac->adev, (uint32_t *) &fmt);
>> + if (rc < 0)
>> + goto fail_cmd;
>> +
>> + rc = wait_event_timeout(ac->cmd_wait, (ac->cmd_state >= 0), 5 * HZ);
>> + if (!rc) {
>> + dev_err(ac->dev, "timeout on format update\n");
>> + return -ETIMEDOUT;
>> + }
>> + if (ac->cmd_state > 0)
>> + return adsp_err_get_lnx_err_code(ac->cmd_state);
>> +
>> + return 0;
>> +fail_cmd:
>> + return rc;
>> +}
>> +EXPORT_SYMBOL_GPL(q6asm_media_format_block_multi_ch_pcm);
>> +
>> +/**
>> + * q6asm_write_nolock() - non blocking write
>> + *
>> + * @ac: audio client pointer
>> + * @len: lenght in bytes
>> + * @msw_ts: timestamp msw
>> + * @lsw_ts: timestamp lsw
>> + * @flags: flags associated with write
>> + *
>> + * Return: Will be an negative value on error or zero on success
>> + */
>> +int q6asm_write_nolock(struct audio_client *ac, uint32_t len, uint32_t msw_ts,
>> + uint32_t lsw_ts, uint32_t flags)
>
> q6asm_write_async() is probably a better name, nolock indicates some
> relationship to mutual exclusions...
>
yep.
>> +{
>> + struct asm_data_cmd_write_v2 write;
>> + struct audio_port_data *port;
>> + struct audio_buffer *ab;
>> + int dsp_buf = 0;
>> + int rc = 0;
>> +
>> + if (ac->io_mode & SYNC_IO_MODE) {
>
> Bail early if this isn't true, to save you the indentation level.
>
yep.
>> + port = &ac->port[SNDRV_PCM_STREAM_PLAYBACK];
>> + q6asm_add_hdr(ac, &write.hdr, sizeof(write), false,
>> + ac->stream_id);
>> +
>> + dsp_buf = port->dsp_buf;
>> + ab = &port->buf[dsp_buf];
>
> So we're just unconditionally telling the remote side about the next buf
> in our ring buffer. Do we need to ensure that this is available/ready?
>
This is already synchronized at the top layer in q6asm_dai driver.
>> +
>> + write.hdr.token = port->dsp_buf;
>> + write.hdr.opcode = ASM_DATA_CMD_WRITE_V2;
>> + write.buf_addr_lsw = lower_32_bits(ab->phys);
>> + write.buf_addr_msw = upper_32_bits(ab->phys);
>> + write.buf_size = len;
>> + write.seq_id = port->dsp_buf;
>> + write.timestamp_lsw = lsw_ts;
>> + write.timestamp_msw = msw_ts;
>> + write.mem_map_handle =
>> + ac->port[SNDRV_PCM_STREAM_PLAYBACK].mem_map_handle;
>> +
>> + if (flags == NO_TIMESTAMP)
>> + write.flags = (flags & 0x800000FF);
>
> Fill in the constant and this becomes
>
> if flags == 0xff00:
> write.flags = 0xff00 & 0x800000ff;
>
> Or in other words:
> if flags == 0xff00:
> write.flags = 0;
>
>> + else
>> + write.flags = (0x80000000 | flags);
>
> Drop the parenthesis and flip the |. It would be nice to have a define
> or a comment indicating what BIT(31) is...
sure, I will make add more information here on the flag and also cleanup
as suggested.
>
>> +
>> + port->dsp_buf++;
>> +
>> + if (port->dsp_buf >= port->max_buf_cnt)
>> + port->dsp_buf = 0;
>> +
>> + rc = apr_send_pkt(ac->adev, (uint32_t *) &write);
>> + if (rc < 0)
>> + return rc;
>> + }
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(q6asm_write_nolock);
>>
[...]
>> +
>> +static int __q6asm_cmd(struct audio_client *ac, int cmd, bool wait)
>> +{
>> + int stream_id = ac->stream_id;
>> + struct apr_hdr hdr;
>> + int rc;
>> +
>> + q6asm_add_hdr(ac, &hdr, sizeof(hdr), true, stream_id);
>> + ac->cmd_state = -1;
>
> Resetting cmd_state relates to the send, don't mix it with building the
> packet.
>
Sure.
>> + switch (cmd) {
>> + case CMD_PAUSE:
>> + hdr.opcode = ASM_SESSION_CMD_PAUSE;
>> + break;
>> + case CMD_SUSPEND:
>> + hdr.opcode = ASM_SESSION_CMD_SUSPEND;
>> + break;
>> + case CMD_FLUSH:
>> + hdr.opcode = ASM_STREAM_CMD_FLUSH;
>> + break;
>> + case CMD_OUT_FLUSH:
>> + hdr.opcode = ASM_STREAM_CMD_FLUSH_READBUFS;
>> + break;
>> + case CMD_EOS:
>> + hdr.opcode = ASM_DATA_CMD_EOS;
>> + ac->cmd_state = 0;
>> + break;
>> + case CMD_CLOSE:
>> + hdr.opcode = ASM_STREAM_CMD_CLOSE;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + rc = apr_send_pkt(ac->adev, (uint32_t *) &hdr);
>> + if (rc < 0)
>> + return rc;
>> +
>> + if (!wait)
>> + return 0;
>
> I've asked you to split the others into _sync() vs _async() operations.
>
> One particular concern I have is that I don't see any mutual exclusion
> protecting the cmd_state and a call with !wait will overwrite the
> existing value, which might be unexpected.
yes, this will be issue, we could move setting cmd_state to here.
Also I will revisit _sync() function to make sure that these are
sequenced correctly and async are not touching the cmd_state.
>
>> +
>> + rc = wait_event_timeout(ac->cmd_wait, (ac->cmd_state >= 0), 5 * HZ);
>> + if (!rc) {
>> + dev_err(ac->dev, "timeout response for opcode[0x%x]\n",
>> + hdr.opcode);
>> + return -ETIMEDOUT;
>> + }
>> + if (ac->cmd_state > 0)
>> + return adsp_err_get_lnx_err_code(ac->cmd_state);
>> +
>> + if (cmd == CMD_FLUSH)
>> + q6asm_reset_buf_state(ac);
>> +
>> + return 0;
>> +}
> [..]
>> diff --git a/sound/soc/qcom/qdsp6/q6asm.h b/sound/soc/qcom/qdsp6/q6asm.h
>> index e1409c368600..b4896059da79 100644
>> --- a/sound/soc/qcom/qdsp6/q6asm.h
>> +++ b/sound/soc/qcom/qdsp6/q6asm.h
>> @@ -2,7 +2,34 @@
>> #ifndef __Q6_ASM_H__
>> #define __Q6_ASM_H__
>>
>> +/* ASM client callback events */
>> +#define CMD_PAUSE 0x0001
>
> These defines has rather generic names...
I can prefix them with Q6ASM to make it much more specific to Q6ASM service.
>
> [..]
>> +
>> +#define MSM_FRONTEND_DAI_MULTIMEDIA1 0
>> +#define MSM_FRONTEND_DAI_MULTIMEDIA2 1
>> +#define MSM_FRONTEND_DAI_MULTIMEDIA3 2
>> +#define MSM_FRONTEND_DAI_MULTIMEDIA4 3
>> +#define MSM_FRONTEND_DAI_MULTIMEDIA5 4
>> +#define MSM_FRONTEND_DAI_MULTIMEDIA6 5
>> +#define MSM_FRONTEND_DAI_MULTIMEDIA7 6
>> +#define MSM_FRONTEND_DAI_MULTIMEDIA8 7
>> +
>> #define MAX_SESSIONS 16
>> +#define NO_TIMESTAMP 0xFF00
>> +#define FORMAT_LINEAR_PCM 0x0000
>
> Ditto.
>
> Regards,
> Bjorn
>
^ permalink raw reply
* [RESEND PATCH v2 07/15] ASoC: qcom: q6asm: Add support to memory map and unmap
From: Srinivas Kandagatla @ 2018-01-03 16:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180102054852.GP478@tuxbook>
Thanks for your review comments.
On 02/01/18 05:48, Bjorn Andersson wrote:
> On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla at linaro.org wrote:
>
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> This patch adds support to memory map and unmap regions commands in
>> q6asm module.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> sound/soc/qcom/qdsp6/q6asm.c | 343 ++++++++++++++++++++++++++++++++++++++++++-
>> sound/soc/qcom/qdsp6/q6asm.h | 5 +
>> 2 files changed, 347 insertions(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/qcom/qdsp6/q6asm.c b/sound/soc/qcom/qdsp6/q6asm.c
>> index 9cc583afef4d..4be92441f524 100644
>> --- a/sound/soc/qcom/qdsp6/q6asm.c
>> +++ b/sound/soc/qcom/qdsp6/q6asm.c
[...]
>> +};
>> +
>> +struct audio_port_data {
>> + struct audio_buffer *buf;
>> + uint32_t max_buf_cnt;
>
> This seems to denote the number of audio_buffers in the buf array, so
> I'm not sure about the meaning of "max_
I can rename it to buf_cnt if it makes it more readable.
>
>> + uint32_t dsp_buf;
>> + uint32_t mem_map_handle;
>> +};
>>
>> struct audio_client {
>> int session;
>> @@ -27,6 +64,8 @@ struct audio_client {
>> uint64_t time_stamp;
>> struct apr_device *adev;
>> struct mutex cmd_lock;
>> + /* idx:1 out port, 0: in port */
>> + struct audio_port_data port[2];
>> wait_queue_head_t cmd_wait;
>> int perf_mode;
>> int stream_id;
>> @@ -86,6 +125,260 @@ static void q6asm_session_free(struct audio_client *ac)
>> mutex_unlock(&a->session_lock);
>> }
>>
>> +static inline void q6asm_add_mmaphdr(struct audio_client *ac,
>> + struct apr_hdr *hdr, u32 pkt_size,
>> + bool cmd_flg, u32 token)
>
> cmd_flg is true in both callers, so this function ends up simply
> assigning hdr_field, pkt_size and token. Inlining these assignments
> would make for cleaner call sites as well.
>
yep, will try that.
>> +{
>> + hdr->hdr_field = APR_SEQ_CMD_HDR_FIELD;
>> + hdr->src_port = 0;
>> + hdr->dest_port = 0;
>> + hdr->pkt_size = pkt_size;
>> + if (cmd_flg)
>> + hdr->token = token;
>> +}
>> +
>> +static inline void q6asm_add_hdr(struct audio_client *ac, struct apr_hdr *hdr,
>
> This is unused.
This should actually go into the next patch.
>
>> + uint32_t pkt_size, bool cmd_flg,
>> + uint32_t stream_id)
>> +{
>> + hdr->hdr_field = APR_SEQ_CMD_HDR_FIELD;
>> + hdr->src_svc = ac->adev->svc_id;
>> + hdr->src_domain = APR_DOMAIN_APPS;
>> + hdr->dest_svc = APR_SVC_ASM;
>> + hdr->dest_domain = APR_DOMAIN_ADSP;
>> + hdr->src_port = ((ac->session << 8) & 0xFF00) | (stream_id);
>> + hdr->dest_port = ((ac->session << 8) & 0xFF00) | (stream_id);
>> + hdr->pkt_size = pkt_size;
>> + if (cmd_flg)
>> + hdr->token = ac->session;
>> +}
>> +
>> +static int __q6asm_memory_unmap(struct audio_client *ac,
>> + phys_addr_t buf_add, int dir)
>> +{
>> + struct avs_cmd_shared_mem_unmap_regions mem_unmap;
>
> If you name this "cmd" you will declutter below code a bit.
>
yep!
>> + struct q6asm *a = dev_get_drvdata(ac->dev->parent);
>> + int rc;
>> +
>> + if (!a)
>> + return -ENODEV;
>
> Does this NULL check add any real value?
>
>> +
>> + q6asm_add_mmaphdr(ac, &mem_unmap.hdr, sizeof(mem_unmap), true,
>> + ((ac->session << 8) | dir));
>> + a->mem_state = -1;
>> +
>> + mem_unmap.hdr.opcode = ASM_CMD_SHARED_MEM_UNMAP_REGIONS;
>> + mem_unmap.mem_map_handle = ac->port[dir].mem_map_handle;
>> +
>> + if (mem_unmap.mem_map_handle == 0) {
>
> Start the function by checking for !ac->port[dir].mem_map_handle
>
yes!
>> + dev_err(ac->dev, "invalid mem handle\n");
>> + return -EINVAL;
>> + }
>> +
>> + rc = apr_send_pkt(a->adev, (uint32_t *) &mem_unmap);
>> + if (rc < 0)
>> + return rc;
>> +
>> + rc = wait_event_timeout(a->mem_wait, (a->mem_state >= 0),
>> + 5 * HZ);
>> + if (!rc) {
>> + dev_err(ac->dev, "CMD timeout for memory_unmap 0x%x\n",
>> + mem_unmap.mem_map_handle);
>> + return -ETIMEDOUT;
>> + } else if (a->mem_state > 0) {
>> + return adsp_err_get_lnx_err_code(a->mem_state);
>> + }
>> + ac->port[dir].mem_map_handle = 0;
>
> Does all errors indicate that the region is left mapped?
>
No, caller should check the return value of this to verify that its
mapped or not.
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * q6asm_unmap_memory_regions() - unmap memory regions in the dsp.
>> + *
>> + * @dir: direction of audio stream
>> + * @ac: audio client instanace
>> + *
>> + * Return: Will be an negative value on failure or zero on success
>> + */
>> +int q6asm_unmap_memory_regions(unsigned int dir, struct audio_client *ac)
>> +{
>> + struct audio_port_data *port;
>> + int cnt = 0;
>> + int rc = 0;
>> +
>> + mutex_lock(&ac->cmd_lock);
>> + port = &ac->port[dir];
>> + if (!port->buf) {
>> + mutex_unlock(&ac->cmd_lock);
>> + return 0;
>
> Put a label right before the mutex_unlock below and return rc instead of
> 0, then you can replace these two lines with "goto unlock"
>
>> + }
>> + cnt = port->max_buf_cnt - 1;
>
> What if we mapped 1 period? Why shouldn't we enter the unmap path?
>
It would enter into unmap path, as cnt would be 0 for 1 period.
>> + if (cnt >= 0) {
>> + rc = __q6asm_memory_unmap(ac, port->buf[dir].phys, dir);
>> + if (rc < 0) {
>> + dev_err(ac->dev, "%s: Memory_unmap_regions failed %d\n",
>> + __func__, rc);
>
> Most of the code paths through __q6asm_memory_unmap() will print an
> error, make this consistent and print the warning once.
okay.
>
>> + mutex_unlock(&ac->cmd_lock);
>> + return rc;
>
> Same here.
>
>> + }
>> + }
>> +
>> + port->max_buf_cnt = 0;
>> + kfree(port->buf);
>> + port->buf = NULL;
>> + mutex_unlock(&ac->cmd_lock);
>
> I think however that if you rearrange this function somewhat you can
> start off by doing:
>
> mutex_lock(&ac->cmd_lock);
> port = &ac->port[dir];
>
> bufs = port->buf;
> cnt = port->max_buf_cnt;
>
> port->max_buf_cnt = 0;
> port->buf = NULL;
> mutex_unlock(&ac->cmd_lock);
>
> And then you can do the rest without locks.
>
will give that a go.
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(q6asm_unmap_memory_regions);
>> +
>> +static int __q6asm_memory_map_regions(struct audio_client *ac, int dir,
>> + uint32_t period_sz, uint32_t periods,
>
> period_sz is typical size_t material.
yep.
>
>> + bool is_contiguous)
>> +{
>> + struct avs_cmd_shared_mem_map_regions *mmap_regions = NULL;
>
> Calling this "cmd" would declutter the function.
>
>> + struct avs_shared_map_region_payload *mregions = NULL;
>> + struct q6asm *a = dev_get_drvdata(ac->dev->parent);
>> + struct audio_port_data *port = NULL;
>> + struct audio_buffer *ab = NULL;
>> + void *mmap_region_cmd = NULL;
>
> No need to initialize this.
yes, I agree.
>
> Also, this really is a avs_cmd_shared_mem_map_regions with some extra
> data at the end, not a void *.
>
>> + void *payload = NULL;
>> + int rc = 0;
>> + int i = 0;
>> + int cmd_size = 0;
>
> Most of these can be left uninitialized.
>
>> + uint32_t num_regions;
>> + uint32_t buf_sz;
>> +
>> + if (!a)
>> + return -ENODEV;
>> + num_regions = is_contiguous ? 1 : periods;
>> + buf_sz = is_contiguous ? (period_sz * periods) : period_sz;
>> + buf_sz = PAGE_ALIGN(buf_sz);
>> +
>> + cmd_size = sizeof(*mmap_regions) + (sizeof(*mregions) * num_regions);
>> +
>> + mmap_region_cmd = kzalloc(cmd_size, GFP_KERNEL);
>> + if (!mmap_region_cmd)
>> + return -ENOMEM;
>> +
>> + mmap_regions = (struct avs_cmd_shared_mem_map_regions *)mmap_region_cmd;
>> + q6asm_add_mmaphdr(ac, &mmap_regions->hdr, cmd_size, true,
>> + ((ac->session << 8) | dir));
>> + a->mem_state = -1;
>> +
>> + mmap_regions->hdr.opcode = ASM_CMD_SHARED_MEM_MAP_REGIONS;
>> + mmap_regions->mem_pool_id = ADSP_MEMORY_MAP_SHMEM8_4K_POOL;
>> + mmap_regions->num_regions = num_regions;
>> + mmap_regions->property_flag = 0x00;
>> +
>> + payload = ((u8 *) mmap_region_cmd +
>> + sizeof(struct avs_cmd_shared_mem_map_regions));
>
> mmap_region_cmd is void *, so no need to type cast.
>
yep.
>
>> +
>> + mregions = (struct avs_shared_map_region_payload *)payload;
>
> Payload is void *, so no need to type cast. But there's also no benefit
> of having "payload", as this line can be written as:
>
> mregions = mmap_region_cmd + sizeof(*mmap_regions);
>
>
> But adding a flexible array member to the avs_cmd_shared_mem_map_regions
> struct would make things even clearer, in particular you would be able
> to read the struct definition and see that there's a payload following
> the command.
>
>> +
>> + ac->port[dir].mem_map_handle = 0;
>
> Isn't this already 0?
>
>> + port = &ac->port[dir];
>> +
>> + for (i = 0; i < num_regions; i++) {
>> + ab = &port->buf[i];
>> + mregions->shm_addr_lsw = lower_32_bits(ab->phys);
>> + mregions->shm_addr_msw = upper_32_bits(ab->phys);
>> + mregions->mem_size_bytes = buf_sz;
>
> Here you're dereferencing port->buf without holding cmd_lock.
>
yep, will fix that in next version.
>> + ++mregions;
>> + }
>> +
>> + rc = apr_send_pkt(a->adev, (uint32_t *) mmap_region_cmd);
>> + if (rc < 0)
>> + goto fail_cmd;
>> +
>> + rc = wait_event_timeout(a->mem_wait, (a->mem_state >= 0),
>> + 5 * HZ);
>> + if (!rc) {
>> + dev_err(ac->dev, "timeout. waited for memory_map\n");
>> + rc = -ETIMEDOUT;
>> + goto fail_cmd;
>> + }
>> +
>> + if (a->mem_state > 0) {
>> + rc = adsp_err_get_lnx_err_code(a->mem_state);
>> + goto fail_cmd;
>> + }
>> + rc = 0;
>> +fail_cmd:
>> + kfree(mmap_region_cmd);
>> + return rc;
>> +}
>> +
>> +/**
>> + * q6asm_map_memory_regions() - map memory regions in the dsp.
>> + *
>> + * @dir: direction of audio stream
>
> This sounds boolean, perhaps worth mentioning here if true means rx or
> tx.
>
I will add a note in doc about this.
> And it's idiomatic to have such a parameter later in the list, it would
> probably be more natural to read the call sight if the order was:
>
> q6asm_map_memory_regions(ac, phys, periods, size, true);
>
>> + * @ac: audio client instanace
>> + * @phys: physcial address that needs mapping.
>> + * @period_sz: audio period size
>> + * @periods: number of periods
>> + *
>> + * Return: Will be an negative value on failure or zero on success
>> + */
>> +int q6asm_map_memory_regions(unsigned int dir, struct audio_client *ac,
>> + dma_addr_t phys,
>> + unsigned int period_sz, unsigned int periods)
>
> period_sz could with benefit be of type size_t.
>
yep.
>> +{
>> + struct audio_buffer *buf;
>> + int cnt;
>> + int rc;
>> +
>> + if (ac->port[dir].buf) {
>> + dev_err(ac->dev, "Buffer already allocated\n");
>> + return 0;
>> + }
>> +
>> + mutex_lock(&ac->cmd_lock);
>
> I believe this lock should cover above check.
>
yep.
>> +
>> + buf = kzalloc(((sizeof(struct audio_buffer)) * periods), GFP_KERNEL);
>
> Loose a few of those parenthesis and use *buf in the sizeof.
>
yes
>> + if (!buf) {
>> + mutex_unlock(&ac->cmd_lock);
>> + return -ENOMEM;
>> + }
>> +
>> +
>> + ac->port[dir].buf = buf;
>> +
>> + buf[0].phys = phys;
>> + buf[0].used = dir ^ 1;
>
> Why would this be called "used", and it's probably cleaner to just use
> !!dir.
We can get rid of this, it looks like leftover from old code.
>
>> + buf[0].size = period_sz;
>> + cnt = 1;
>> + while (cnt < periods) {
>
> cnt goes from 1 to periods and is incremented 1 each step, this would be
> more succinct as a for loop.
yep!
>
>> + if (period_sz > 0) {
>> + buf[cnt].phys = buf[0].phys + (cnt * period_sz);
>> + buf[cnt].used = dir ^ 1;
>> + buf[cnt].size = period_sz;
>> + }
>> + cnt++;
>> + }
>> +
>> + ac->port[dir].max_buf_cnt = periods;
>> + mutex_unlock(&ac->cmd_lock);
>
> The only possible purpose of taking cmd_lock here is to protect
> ac->port[dir].buf, but
>
>> +
>> + rc = __q6asm_memory_map_regions(ac, dir, period_sz, periods, 1);
>
> The last parameter should be "true".
>
yes.
>> + if (rc < 0) {
>> + dev_err(ac->dev,
>> + "CMD Memory_map_regions failed %d for size %d\n", rc,
>> + period_sz);
>> +
>> +
>> + ac->port[dir].max_buf_cnt = 0;
>> + kfree(buf);
>> + ac->port[dir].buf = NULL;
>
> These operations are done without holding cmd_lock.
>
I will revisit such instances where the buf is not protected.
>> +
>> + return rc;
>> + }
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(q6asm_map_memory_regions);
>> +
>> /**
>> * q6asm_audio_client_free() - Freee allocated audio client
>> *
>> @@ -117,8 +410,10 @@ static struct audio_client *q6asm_get_audio_client(struct q6asm *a,
>>
>> static int q6asm_srvc_callback(struct apr_device *adev, struct apr_client_data *data)
>> {
>> - struct q6asm *q6asm = dev_get_drvdata(&adev->dev);
>> + struct q6asm *a, *q6asm = dev_get_drvdata(&adev->dev);
>> struct audio_client *ac = NULL;
>> + struct audio_port_data *port;
>> + uint32_t dir = 0;
>> uint32_t sid = 0;
>> uint32_t *payload;
>>
>> @@ -135,6 +430,52 @@ static int q6asm_srvc_callback(struct apr_device *adev, struct apr_client_data *
>> return 0;
>> }
>>
>> + a = dev_get_drvdata(ac->dev->parent);
>> + if (data->opcode == APR_BASIC_RSP_RESULT) {
>
> This is a case in below switch statement.
>
sure.
>> + switch (payload[0]) {
>> + case ASM_CMD_SHARED_MEM_MAP_REGIONS:
>> + case ASM_CMD_SHARED_MEM_UNMAP_REGIONS:
>> + if (payload[1] != 0) {
>> + dev_err(ac->dev,
>> + "cmd = 0x%x returned error = 0x%x sid:%d\n",
>> + payload[0], payload[1], sid);
>> + a->mem_state = payload[1];
>> + } else {
>> + a->mem_state = 0;
>
> Just assign a->mem_state = payload[1] outside the conditional, as it
> will be the same value.
I agree, will fix such instances.
>
>> + }
>> +
>> + wake_up(&a->mem_wait);
>> + break;
>> + default:
>> + dev_err(&adev->dev, "command[0x%x] not expecting rsp\n",
>> + payload[0]);
>> + break;
>> + }
>> + return 0;
>> + }
>
> Regards,
> Bjorn
>
^ permalink raw reply
* [RESEND PATCH v2 06/15] ASoC: qcom: qdsp6: Add support to Q6ASM
From: Srinivas Kandagatla @ 2018-01-03 16:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180102044350.GO478@tuxbook>
On 02/01/18 04:43, Bjorn Andersson wrote:
> On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla at linaro.org wrote:
>
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> This patch adds basic support to Q6 ASM (Audio Stream Manager) module on
>> Q6DSP. ASM supports up to 8 concurrent streams. each stream can be setup
>> as playback/capture.
>
> "...streams, each one setup as either playback or capture".
>
> or "each" need to be capitalized.
>
>> ASM provides top control functions like
>> Pause/flush/resume for playback and record. ASM can Create/destroy encoder,
>
> lower case p and c
>
>> decoder and also provides POPP dynamic services.
>
> Please describe what POPP is.
Yep, will fix the commit log as suggested.
>
> [..]
>> +struct audio_client {
>> + int session;
>> + app_cb cb;
>> + int cmd_state;
>> + void *priv;
>> + uint32_t io_mode;
>> + uint64_t time_stamp;
>
> Unused.
>
will remove this in next version.
>> + struct apr_device *adev;
>> + struct mutex cmd_lock;
>> + wait_queue_head_t cmd_wait;
>> + int perf_mode;
>> + int stream_id;
>> + struct device *dev;
>> +};
>> +
>> +struct q6asm {
>> + struct apr_device *adev;
>> + int mem_state;
>> + struct device *dev;
>> + wait_queue_head_t mem_wait;
>> + struct mutex session_lock;
>> + struct platform_device *pcmdev;
>> + struct audio_client *session[MAX_SESSIONS + 1];
>> +};
>> +
>> +static int q6asm_session_alloc(struct audio_client *ac, struct q6asm *a)
>
> Move the allocation of ac into this function, and return the newly
> allocated ac - that way the name of this function makes more sense.
will try that, it should cleanup some code.
>
>> +{
>> + int n = -EINVAL;
>
> You're returning MAX_SESSIONS if no free sessions are found, but are
> checking for <= 0 in the caller.
I will make sure that its checked correctly and i will also update the
kernel doc to reflect this.
>
>> +
>> + mutex_lock(&a->session_lock);
>> + for (n = 1; n <= MAX_SESSIONS; n++) {
>
> Is there an external reason for session 0 not being considered?
>
Yes, session 0 is reserved.
>> + if (!a->session[n]) {
>> + a->session[n] = ac;
>> + break;
>> + }
>> + }
>
> If you make session an idr this function would become idr_alloc(1,
> MAX_SESSIONS + 1).
will try idr and see how it looks.
>
>> + mutex_unlock(&a->session_lock);
>> +
>> + return n;
>> +}
>> +
>> +static bool q6asm_is_valid_audio_client(struct audio_client *ac)
>> +{
>> + struct q6asm *a = dev_get_drvdata(ac->dev->parent);
>> + int n;
>> +
>> + for (n = 1; n <= MAX_SESSIONS; n++) {
>> + if (a->session[n] == ac)
>> + return 1;
>
> "true"
thanks, will fix these.
>
>> + }
>> +
>> + return 0;
>
> "false"
>
>> +}
>> +
>> +static void q6asm_session_free(struct audio_client *ac)
>> +{
>> + struct q6asm *a = dev_get_drvdata(ac->dev->parent);
>> +
>> + if (!a)
>> + return;
>> +
>> + mutex_lock(&a->session_lock);
>> + a->session[ac->session] = 0;
>> + ac->session = 0;
>> + ac->perf_mode = LEGACY_PCM_MODE;
>
> No need to update ac->*, as you kfree ac as soon as you return from
> here.
yep.
>
>> + mutex_unlock(&a->session_lock);
>> +}
>> +
>> +/**
>> + * q6asm_audio_client_free() - Freee allocated audio client
>> + *
>> + * @ac: audio client to free
>> + */
>> +void q6asm_audio_client_free(struct audio_client *ac)
>> +{
>> + q6asm_session_free(ac);
>
> Inline q6asm_session_free() here.
makes sense here.
>
>> + kfree(ac);
>> +}
>> +EXPORT_SYMBOL_GPL(q6asm_audio_client_free);
>> +
>> +static struct audio_client *q6asm_get_audio_client(struct q6asm *a,
>> + int session_id)
>> +{
>> + if ((session_id <= 0) || (session_id > MAX_SESSIONS)) {
>> + dev_err(a->dev, "invalid session: %d\n", session_id);
>> + goto err;
>
> Just return NULL here instead.
yep.
>
>> + }
>> +
>> + if (!a->session[session_id]) {
>> + dev_err(a->dev, "session not active: %d\n", session_id);
>> + goto err;
>
> Dito
>
>> + }
>
> But this is another place where an idr would be preferable, as both
> these cases would be covered with a call to idr_find()
>
>> + return a->session[session_id];
>> +err:
>> + return NULL;
>> +}
>> +
>> +static int q6asm_srvc_callback(struct apr_device *adev, struct apr_client_data *data)
>> +{
>> + struct q6asm *q6asm = dev_get_drvdata(&adev->dev);
>> + struct audio_client *ac = NULL;
>> + uint32_t sid = 0;
>
> This is 4 bits, so just use int.
>
makes sense.
>> + uint32_t *payload;
>
> payload is unused.
will remove this in next version.
>
>> +
>> + if (!data) {
>> + dev_err(&adev->dev, "%s: Invalid CB\n", __func__);
>> + return 0;
>> + }
>
> Again, define the apr to never invoke the callback with data = NULL
>
yep.
>> +
>> + payload = data->payload;
>> + sid = (data->token >> 8) & 0x0F;
>> + ac = q6asm_get_audio_client(q6asm, sid);
>> + if (!ac) {
>> + dev_err(&adev->dev, "Audio Client not active\n");
>> + return 0;
>> + }
>> +
>> + if (ac->cb)
>> + ac->cb(data->opcode, data->token, data->payload, ac->priv);
>> + return 0;
>> +}
>> +
[...]
>> +/**
>> + * q6asm_audio_client_alloc() - Allocate a new audio client
>> + *
>> + * @dev: Pointer to asm child device.
>> + * @cb: event callback.
>> + * @priv: private data associated with this client.
>> + *
>> + * Return: Will be an error pointer on error or a valid audio client
>> + * on success.
>> + */
>> +struct audio_client *q6asm_audio_client_alloc(struct device *dev,
>> + app_cb cb, void *priv)
>> +{
>> + struct q6asm *a = dev_get_drvdata(dev->parent);
>> + struct audio_client *ac;
>> + int n;
>> +
>> + ac = kzalloc(sizeof(struct audio_client), GFP_KERNEL);
>
> sizeof(*ac)
Yep.
>
>> + if (!ac)
>> + return NULL;
>> +
>> + n = q6asm_session_alloc(ac, a);
>
> As stated above, moving the kzalloc into q6asm_session_alloc() would
> clean the code up here, as you only need to deal with one possible
> error case here.
Will give it a go and see.
>
>> + if (n <= 0) {
>> + dev_err(dev, "ASM Session alloc fail n=%d\n", n);
>> + kfree(ac);
>> + return NULL;
>
> Per the kerneldoc I expect an ERR_PTR(n) here.
>
yep.
>> + }
>> +
>> + ac->session = n;
>> + ac->cb = cb;
>> + ac->dev = dev;
>> + ac->priv = priv;
>> + ac->io_mode = SYNC_IO_MODE;
>> + ac->perf_mode = LEGACY_PCM_MODE;
>> + /* DSP expects stream id from 1 */
>> + ac->stream_id = 1;
>> + ac->adev = a->adev;
>> +
>> + init_waitqueue_head(&ac->cmd_wait);
>> + mutex_init(&ac->cmd_lock);
>> + ac->cmd_state = 0;
>> +
>> + return ac;
>> +}
>> +EXPORT_SYMBOL_GPL(q6asm_audio_client_alloc);
>> +
>> +
>
> Extra newline.
>
yep, will fix it.
[...]
>> +
>> +static struct apr_driver qcom_q6asm_driver = {
>> + .probe = q6asm_probe,
>> + .remove = q6asm_remove,
>> + .callback = q6asm_srvc_callback,
>> + .id_table = q6asm_id,
>> + .driver = {
>> + .name = "qcom-q6asm",
>> + },
>
> Indentation
yep.
>
>> +};
>> +
>> +module_apr_driver(qcom_q6asm_driver);
>> +MODULE_DESCRIPTION("Q6 Audio Stream Manager driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/sound/soc/qcom/qdsp6/q6asm.h b/sound/soc/qcom/qdsp6/q6asm.h
>> new file mode 100644
>> index 000000000000..7a8a9039fd89
>> --- /dev/null
>> +++ b/sound/soc/qcom/qdsp6/q6asm.h
>> @@ -0,0 +1,14 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __Q6_ASM_H__
>> +#define __Q6_ASM_H__
>> +
>> +#define MAX_SESSIONS 16
>> +
>> +typedef void (*app_cb) (uint32_t opcode, uint32_t token,
>> + uint32_t *payload, void *priv);
>
> This name of a type is too generic.
>
> And make payload void *, unless the payload really really is an
> unstructured uint32_t array.
will do that as suggested.
>
> Regards,
> Bjorn
>
^ permalink raw reply
* [RESEND PATCH v2 05/15] ASoC: qcom: qdsp6: Add support to Q6ADM
From: Srinivas Kandagatla @ 2018-01-03 16:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180102015019.GN478@tuxbook>
Thanks for your review comments.
On 02/01/18 01:50, Bjorn Andersson wrote:
> On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla at linaro.org wrote:
>
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> This patch adds support to q6 ADM (Audio Device Manager) module in
>> q6dsp. ADM performs routing between audio streams and AFE ports.
>> It does Rate matching for streams going to devices driven by
>
> lower case "Rate"
>
>> different clocks, it handles volume ramping, Mixing with channel
>
> and "Mixing"
>
>> and bit-width. ADM creates and destroys dynamic COPP services
>> for device-related audio processing as needed.
>
> What's a "copp"?
Common post processing.
>
>>
>> This patch adds basic support to ADM.
>
> Wouldn't s/to/for/ be better?
Yes!
>
> [..]
>> +struct copp {
>> + int afe_port;
>> + int copp_idx;
>> + int id;
>> + int cnt;
>
> Please rename this "refcnt" to match other kernel code.
>
yep.
>> + int topology;
>> + int mode;
[...]
>> +};
>
[...]
>> +static int adm_callback(struct apr_device *adev, struct apr_client_data *data)
>> +{
>> + uint32_t *payload;
>> + int port_idx, copp_idx;
>> + struct copp *copp;
>> + struct q6adm *adm = dev_get_drvdata(&adev->dev);
>> +
>> + payload = data->payload;
>> +
>> + if (data->payload_size) {
>
> Bail if you don't have a payload and save yourself one indentation
> level.
yep!
>
>> + copp_idx = (data->token) & 0XFF;
>> + port_idx = ((data->token) >> 16) & 0xFF;
>> + if (port_idx < 0 || port_idx >= AFE_MAX_PORTS) {
>> + dev_err(&adev->dev, "Invalid port idx %d token %d\n",
>> + port_idx, data->token);
>> + return 0;
>> + }
>> + if (copp_idx < 0 || copp_idx >= MAX_COPPS_PER_PORT) {
>> + dev_err(&adev->dev, "Invalid copp idx %d token %d\n",
>> + copp_idx, data->token);
>> + return 0;
>> + }
>> +
>> + if (data->opcode == APR_BASIC_RSP_RESULT) {
>
> This is a case in the following switch statement.
>
>> + if (payload[1] != 0) {
>> + dev_err(&adev->dev, "cmd = 0x%x returned error = 0x%x\n",
>> + payload[0], payload[1]);
>
> This would again benefit from a small struct...
>
>> + }
>> + switch (payload[0]) {
>> + case ADM_CMD_DEVICE_OPEN_V5:
>> + case ADM_CMD_DEVICE_CLOSE_V5:
>> + copp = adm_find_copp(adm, port_idx, copp_idx);
>> + if (IS_ERR_OR_NULL(copp))
>> + return 0;
>> +
>> + copp->stat = payload[1];
>> + wake_up(&copp->wait);
>> + break;
>> + case ADM_CMD_MATRIX_MAP_ROUTINGS_V5:
>> + adm->matrix_map_stat = payload[1];
>> + wake_up(&adm->matrix_map_wait);
>> + break;
>> +
>> + default:
>> + dev_err(&adev->dev, "Unknown Cmd: 0x%x\n",
>> + payload[0]);
>> + break;
>> + }
>> + return 0;
>> + }
>> +
>> + switch (data->opcode) {
>> + case ADM_CMDRSP_DEVICE_OPEN_V5:{
>
> Perhaps it would be cleaner to break these out in separate functions?
will do!
>
>> + struct adm_cmd_rsp_device_open_v5 {
>> + u32 status;
>> + u16 copp_id;
>> + u16 reserved;
>> + } __packed * open = data->payload;
>> +
>> + open = data->payload;
>> + copp = adm_find_copp(adm, port_idx, copp_idx);
>> + if (IS_ERR_OR_NULL(copp))
>> + return 0;
>> +
>> + if (open->copp_id == INVALID_COPP_ID) {
>> + dev_err(&adev->dev, "Invalid coppid rxed %d\n",
>> + open->copp_id);
>> + copp->stat = ADSP_EBADPARAM;
>> + wake_up(&copp->wait);
>> + break;
>> + }
>> + copp->stat = payload[0];
>> + copp->id = open->copp_id;
>> + pr_debug("%s: coppid rxed=%d\n", __func__,
>
> You have a dev, use dev_dbg()
I think this was a leftover from previous versions, I will fix such
occurrences.
>
>> + open->copp_id);
>> + wake_up(&copp->wait);
>> +
>> + }
>> + break;
>> + default:
>> + dev_err(&adev->dev, "Unknown cmd:0x%x\n",
>> + data->opcode);
>> + break;
>> + }
>> + }
>> + return 0;
>> +}
>> +
>> +static struct copp *adm_alloc_copp(struct q6adm *adm, int port_idx)
>> +{
>> + struct copp *c;
>> + int idx;
>> +
>> + idx = find_first_zero_bit(&adm->copp_bitmap[port_idx],
>> + MAX_COPPS_PER_PORT);
>> +
>> + if (idx > MAX_COPPS_PER_PORT)
>> + return ERR_PTR(-EBUSY);
>> +
>> + set_bit(idx, &adm->copp_bitmap[port_idx]);
>> +
>> + c = devm_kzalloc(adm->dev, sizeof(*c), GFP_KERNEL);
>> + if (!c)
>
> Set the bit after doing the allocation and you don't need to clear it
> here.
>
Makes sense.
>> + return ERR_PTR(-ENOMEM);
>> + c->copp_idx = idx;
>> + c->afe_port = port_idx;
>> + c->adm = adm;
>> +
>> + init_waitqueue_head(&c->wait);
>> +
>> + spin_lock(&adm->copps_list_lock);
>> + list_add_tail(&c->node, &adm->copps_list);
>> + spin_unlock(&adm->copps_list_lock);
>> +
>> + return c;
>> +}
>> +
>> +static void adm_free_copp(struct q6adm *adm, struct copp *c, int port_idx)
>> +{
>> + clear_bit(c->copp_idx, &adm->copp_bitmap[port_idx]);
>> + spin_lock(&adm->copps_list_lock);
>> + list_del(&c->node);
>> + spin_unlock(&adm->copps_list_lock);
>
> This function clear the copp_bitmap, so after recycling this once
> c->copp_idx will be "dangling".
>
> This seems to put the copp in a state where it is invalid and as the
> copp is "reset" i don't think adm_find_matching_copp() will actually
> find this again, ever...
>
> So shouldn't you free the copp here too - rather than relying on devm
> doing that at some later point in time?
Yes I agree.
>
>> +}
>> +/**
>> + * q6adm_open() - open adm to get hold of free copp
>
> "open adm and grab a free copp"?
>
yep.
>> + *
>> + * @dev: Pointer to adm child device.
>> + * @port_id: port id
>> + * @path: playback or capture path.
>> + * @rate: rate at which copp is required.
>> + * @channel_mode: channel mode
>> + * @topology: adm topology id
>> + * @perf_mode: performace mode.
>> + * @bit_width: audio sample bit width
>> + * @app_type: Application type.
>> + * @acdb_id: ACDB id
>> + *
>> + * Return: Will be an negative on error or a valid copp index on success.
>> + */
>> +int q6adm_open(struct device *dev, int port_id, int path, int rate,
>> + int channel_mode, int topology, int perf_mode,
>> + uint16_t bit_width, int app_type, int acdb_id)
>> +{
>> + struct adm_cmd_device_open_v5 {
>> + struct apr_hdr hdr;
>> + u16 flags;
>> + u16 mode_of_operation;
>> + u16 endpoint_id_1;
>> + u16 endpoint_id_2;
>> + u32 topology_id;
>> + u16 dev_num_channel;
>> + u16 bit_width;
>> + u32 sample_rate;
>> + u8 dev_channel_mapping[8];
>> + } __packed open;
>> + int ret = 0;
>> + int port_idx, flags;
>> + int tmp_port = q6afe_get_port_id(port_id);
>> + struct copp *copp;
>> + struct q6adm *adm = dev_get_drvdata(dev->parent);
>> +
>> + port_idx = port_id;
>
> I'm not seeing the reason for having two different variables with the
> same value.
>
not sure why it ended up like this, will fix it.
>> + if (port_idx < 0) {
>> + dev_err(dev, "Invalid port_id 0x%x\n", port_id);
>> + return -EINVAL;
>> + }
>> +
>> + flags = ADM_LEGACY_DEVICE_SESSION;
>
> Just put ADM_LEGACY_DEVICE_SESSION in the assignment below.
>
yep!
>> + copp = adm_find_matching_copp(adm, port_idx, topology, perf_mode,
>> + rate, bit_width, app_type);
>> +
>> + if (!copp) {
>> + copp = adm_alloc_copp(adm, port_idx);
>> + if (IS_ERR_OR_NULL(copp))
>> + return PTR_ERR(copp);
>> +
>> + copp->cnt = 0;
>> + copp->topology = topology;
>> + copp->mode = perf_mode;
>> + copp->rate = rate;
>> + copp->channels = channel_mode;
>> + copp->bit_width = bit_width;
>> + copp->app_type = app_type;
>> + }
>
> I would suggest that you make adm_find_matching_copp() allocate the new
> copp if none is found.
>
will have a go and see!
>> +
>> + /* Create a COPP if port id are not enabled */
>> + if (copp->cnt == 0) {
>
> Doesn't this scheme require some locking? What about concurrent close()?
>
yes, it will be issue if we do concurrent closes(), will revisit locking
on this.
>> +
>> + open.hdr.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD,
>> + APR_HDR_LEN(APR_HDR_SIZE),
>> + APR_PKT_VER);
>> + open.hdr.pkt_size = sizeof(open);
>> + open.hdr.src_svc = APR_SVC_ADM;
>> + open.hdr.src_domain = APR_DOMAIN_APPS;
>> + open.hdr.src_port = tmp_port;
>> + open.hdr.dest_svc = APR_SVC_ADM;
>> + open.hdr.dest_domain = APR_DOMAIN_ADSP;
>> + open.hdr.dest_port = tmp_port;
>> + open.hdr.token = port_idx << 16 | copp->copp_idx;
>> + open.hdr.opcode = ADM_CMD_DEVICE_OPEN_V5;
>> + open.flags = flags;
>> + open.mode_of_operation = path;
>> + open.endpoint_id_1 = tmp_port;
>> + open.topology_id = topology;
>> + open.dev_num_channel = channel_mode & 0x00FF;
>> + open.bit_width = bit_width;
>> + open.sample_rate = rate;
>
> This looks like a q6adm_device_open() helper function.
>
yep!
>> +
>> + ret = q6dsp_map_channels(&open.dev_channel_mapping[0],
>> + channel_mode);
>> +
>> + if (ret)
>> + return ret;
>> +
>> + copp->stat = -1;
>> + ret = apr_send_pkt(adm->apr, (uint32_t *)&open);
>> + if (ret < 0) {
>> + dev_err(dev, "port_id: 0x%x for[0x%x] failed %d\n",
>> + tmp_port, port_id, ret);
>> + return -EINVAL;
>> + }
>> + /* Wait for the callback with copp id */
>> + ret =
>> + wait_event_timeout(copp->wait, copp->stat >= 0,
>> + msecs_to_jiffies(TIMEOUT_MS));
>> + if (!ret) {
>> + dev_err(dev, "ADM timedout port_id: 0x%x for [0x%x]\n",
>> + tmp_port, port_id);
>> + return -EINVAL;
>> + } else if (copp->stat > 0) {
>> + dev_err(dev, "DSP returned error[%s]\n",
>> + adsp_err_get_err_str(copp->stat));
>> + return adsp_err_get_lnx_err_code(copp->stat);
>> + }
>> + }
>> + copp->cnt++;
>> + return copp->copp_idx;
>> +}
>> +EXPORT_SYMBOL_GPL(q6adm_open);
>> +/**
>> + * q6adm_matrix_map() - Map asm streams and afe ports using payload
>> + *
>> + * @dev: Pointer to adm child device.
>> + * @path: playback or capture path.
>> + * @payload_map: map between session id and afe ports.
>> + * @perf_mode: Performace mode.
>> + *
>> + * Return: Will be an negative on error or a zero on success.
>> + */
>> +int q6adm_matrix_map(struct device *dev, int path,
>> + struct route_payload payload_map, int perf_mode)
>> +{
>> + struct adm_cmd_matrix_map_routings_v5 {
>> + struct apr_hdr hdr;
>> + u32 matrix_id;
>> + u32 num_sessions;
>> + } __packed * route;
>> +
>> + struct adm_session_map_node_v5 {
>> + u16 session_id;
>> + u16 num_copps;
>> + } __packed * node;
>> + struct q6adm *adm = dev_get_drvdata(dev->parent);
>> + uint16_t *copps_list;
>> + int cmd_size = 0;
>> + int ret = 0, i = 0;
>> + void *payload = NULL;
>> + void *matrix_map = NULL;
>> + int port_idx, copp_idx;
>> + struct copp *copp;
>> +
>> + /* Assumes port_ids have already been validated during adm_open */
>> + cmd_size = (sizeof(*route) +
>> + sizeof(*node) +
>> + (sizeof(uint32_t) * payload_map.num_copps));
>> + matrix_map = kzalloc(cmd_size, GFP_KERNEL);
>> + if (!matrix_map)
>> + return -ENOMEM;
>> +
>> + route = (struct adm_cmd_matrix_map_routings_v5 *)matrix_map;
>> + route->hdr.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD,
>> + APR_HDR_LEN(APR_HDR_SIZE),
>> + APR_PKT_VER);
>> + route->hdr.pkt_size = cmd_size;
>> + route->hdr.src_svc = 0;
>> + route->hdr.src_domain = APR_DOMAIN_APPS;
>> + route->hdr.src_port = 0; /* Ignored */
>
> Omit the ignored members instead.
yep!
>
>> + route->hdr.dest_svc = APR_SVC_ADM;
>> + route->hdr.dest_domain = APR_DOMAIN_ADSP;
>> + route->hdr.dest_port = 0; /* Ignored */
>> + route->hdr.token = 0;
>> + route->hdr.opcode = ADM_CMD_MATRIX_MAP_ROUTINGS_V5;
>> + route->num_sessions = 1;
>> +
>> + switch (path) {
>> + case ADM_PATH_PLAYBACK:
>> + route->matrix_id = ADM_MATRIX_ID_AUDIO_RX;
>> + break;
>> + default:
>> + dev_err(dev, "Wrong path set[%d]\n", path);
>> +
>> + break;
>> + }
>> +
>> + payload = ((u8 *) matrix_map + sizeof(*route));
>
> matrix_map is a void *, so no need to cast it to u8 * to calculate the
> payload address.
>
yep.
>> + node = (struct adm_session_map_node_v5 *)payload;
>
> payload is void *, so no need to typecast here. And for that matter, I'm
> not sure about the benefits of having this intermediate "payload"
> variable, just assign node directly.
>
>> +
>> + node->session_id = payload_map.session_id;
>> + node->num_copps = payload_map.num_copps;
>> + payload = (u8 *) node + sizeof(*node);
>> + copps_list = (uint16_t *) payload;
>
> As with node above, drop the temporary variable and drop the type casts.
>
>> +
>> + for (i = 0; i < payload_map.num_copps; i++) {
>> + port_idx = payload_map.port_id[i];
>> + if (port_idx < 0) {
>> + dev_err(dev, "Invalid port_id 0x%x\n",
>> + payload_map.port_id[i]);
>
> Leaking matrix_map.
>
yep!
>> + return -EINVAL;
>> + }
>> + copp_idx = payload_map.copp_idx[i];
>> +
>> + copp = adm_find_copp(adm, port_idx, copp_idx);
>> + if (IS_ERR_OR_NULL(copp))
>
> Dito.
>
>> + return -EINVAL;
>> +
>> + copps_list[i] = copp->id;
>> + }
>> +
>> + adm->matrix_map_stat = -1;
>> +
>> + ret = apr_send_pkt(adm->apr, (uint32_t *) matrix_map);
>> + if (ret < 0) {
>> + dev_err(dev, "routing for syream %d failed ret %d\n",
>> + payload_map.session_id, ret);
>> + ret = -EINVAL;
>> + goto fail_cmd;
>> + }
>> + ret = wait_event_timeout(adm->matrix_map_wait,
>> + adm->matrix_map_stat >= 0,
>> + msecs_to_jiffies(TIMEOUT_MS));
>> + if (!ret) {
>> + dev_err(dev, "routing for syream %d failed\n",
>> + payload_map.session_id);
>> + ret = -EINVAL;
>> + goto fail_cmd;
>> + } else if (adm->matrix_map_stat > 0) {
>> + dev_err(dev, "DSP returned error[%s]\n",
>> + adsp_err_get_err_str(adm->matrix_map_stat));
>> + ret = adsp_err_get_lnx_err_code(adm->matrix_map_stat);
>> + goto fail_cmd;
>> + }
>> +
>> +fail_cmd:
>> + kfree(matrix_map);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(q6adm_matrix_map);
>> +
>> +static void adm_reset_copp(struct copp *c)
>
> As far as I can see this will decommission the copp, so I don't think
> there is a point in updating any of this and then keep it around?
yes, if we free it as suggested above we can get rid of this totally.
>
>> +{
>> + c->id = RESET_COPP_ID;
>> + c->cnt = 0;
>> + c->topology = 0;
>> + c->mode = 0;
>> + c->stat = -1;
>> + c->rate = 0;
>> + c->channels = 0;
>> + c->bit_width = 0;
>> + c->app_type = 0;
>> +}
>> +/**
>> + * q6adm_close() - Close adm copp
>> + *
>> + * @dev: Pointer to adm child device.
>> + * @port_id: afe port id.
>> + * @perf_mode: perf_mode mode
>> + * @copp_idx: copp index to close
>> + *
>> + * Return: Will be an negative on error or a zero on success.
>> + */
>> +int q6adm_close(struct device *dev, int port_id, int perf_mode, int copp_idx)
>> +{
>> + struct apr_hdr close;
>> + struct copp *copp;
>> +
>> + int ret = 0, port_idx;
>> + int copp_id = RESET_COPP_ID;
>> + struct q6adm *adm = dev_get_drvdata(dev->parent);
>> +
>> + port_idx = port_id;
>> + if (port_idx < 0) {
>> + dev_err(dev, "Invalid port_id 0x%x\n", port_id);
>> + return -EINVAL;
>> + }
>> +
>> + if ((copp_idx < 0) || (copp_idx >= MAX_COPPS_PER_PORT)) {
>> + dev_err(dev, "Invalid copp idx: %d\n", copp_idx);
>> + return -EINVAL;
>> + }
>> +
>> + copp = adm_find_copp(adm, port_id, copp_idx);
>> + if (IS_ERR_OR_NULL(copp))
>> + return -EINVAL;
>> +
>> + copp->cnt--;
>> + if (!copp->cnt) {
>
> Again, this needs some locking.
Yes, this needs some protection, i will revisit this part.
>
>> + copp_id = copp->id;
>> +
>> + close.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD,
>> + APR_HDR_LEN(APR_HDR_SIZE),
>> + APR_PKT_VER);
>> + close.pkt_size = sizeof(close);
>> + close.src_svc = APR_SVC_ADM;
>> + close.src_domain = APR_DOMAIN_APPS;
>> + close.src_port = port_id;
>> + close.dest_svc = APR_SVC_ADM;
>> + close.dest_domain = APR_DOMAIN_ADSP;
>> + close.dest_port = copp_id;
>> + close.token = port_idx << 16 | copp_idx;
>> + close.opcode = ADM_CMD_DEVICE_CLOSE_V5;
>> +
>
> Split this out into a separate helper function.
yep!
>
>> + ret = apr_send_pkt(adm->apr, (uint32_t *) &close);
>> + if (ret < 0) {
>> + dev_err(dev, "ADM close failed %d\n", ret);
>> + return -EINVAL;
>> + }
>> +
>> + ret = wait_event_timeout(copp->wait, copp->stat >= 0,
>> + msecs_to_jiffies(TIMEOUT_MS));
>> + if (!ret) {
>> + dev_err(dev, "ADM cmd Route timedout for port 0x%x\n",
>> + port_id);
>> + return -EINVAL;
>> + } else if (copp->stat > 0) {
>> + dev_err(dev, "DSP returned error[%s]\n",
>> + adsp_err_get_err_str(copp->stat));
>> + return adsp_err_get_lnx_err_code(copp->stat);
>> + }
>> +
>> + adm_reset_copp(copp);
>> + adm_free_copp(adm, copp, port_id);
>> + }
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(q6adm_close);
> [..]
>> +static struct apr_driver qcom_q6adm_driver = {
>> + .probe = q6adm_probe,
>> + .remove = q6adm_exit,
>> + .callback = adm_callback,
>> + .id_table = adm_id,
>> + .driver = {
>> + .name = "qcom-q6adm",
>> + },
>
> Indentation.
yep.
>
> Regards,
> Bjorn
>
^ permalink raw reply
* [RESEND PATCH v2 04/15] ASoC: qcom: qdsp6: Add support to Q6AFE
From: Srinivas Kandagatla @ 2018-01-03 16:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180102004523.GM478@tuxbook>
Thanks for the comments!
On 02/01/18 00:45, Bjorn Andersson wrote:
> On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla at linaro.org wrote:
>
> [..]
>> +
>> +config SND_SOC_QDSP6_AFE
>> + tristate
>> + default n
>
> Do you see a particular benefit of having one kernel module per
> function? Why not just compile them all into the same q6dsp.ko?
>
No, I do not see any benefit in doing so, I can try to make it as single
module in next version.
>> +
>> +config SND_SOC_QDSP6
>> + tristate "SoC ALSA audio driver for QDSP6"
>> + select SND_SOC_QDSP6_AFE
>> + help
>> + To add support for MSM QDSP6 Soc Audio.
>> + This will enable sound soc platform specific
>> + audio drivers. This includes q6asm, q6adm,
>> + q6afe interfaces to DSP using apr.
> [..]
>> +struct q6afev2 {
>> + void *apr;
>
> apr has a type, even if it's definition is hidden you should use the
> proper type here.
>
I agree.
>> + struct device *dev;
>> + int state;
>> + int status;
>> + struct platform_device *daidev;
>> +
>> + struct mutex afe_cmd_lock;
>
> You shouldn't need to include afe_ in this name.
>
make sense!
>> + struct list_head port_list;
>> + spinlock_t port_list_lock;
>> + struct list_head node;
>> +};
>> +
> [..]
>> +/* Port map of index vs real hw port ids */
>> +static struct afe_port_map port_maps[AFE_PORT_MAX] = {
>> + [AFE_PORT_HDMI_RX] = { AFE_PORT_ID_MULTICHAN_HDMI_RX,
>
> Looks like you have an extra tab here, consider breaking the 80 char
> "rule" to not have to wrap these.
yep!
>
>> + AFE_PORT_HDMI_RX, 1},
>> +};
>> +
>> +static struct q6afe_port *afe_find_port(struct q6afev2 *afe, int token)
>> +{
>> + struct q6afe_port *p = NULL;
>> +
>> + spin_lock(&afe->port_list_lock);
>> + list_for_each_entry(p, &afe->port_list, node)
>> + if (p->token == token)
>> + break;
>> +
>> + spin_unlock(&afe->port_list_lock);
>> + return p;
>> +}
>
> Make port_list an idr and you can just use idr_find() instead of rolling
> your own search function.
>
Will give that a go.
>> +
>> +static int afe_callback(struct apr_device *adev, struct apr_client_data *data)
>> +{
>> + struct q6afev2 *afe = dev_get_drvdata(&adev->dev);//priv;
>
> This is perfectly fine, no need to extend the interface with a priv (so
> drop the comment).
I think it was a leftover, will clean such instances.
>
>> + struct q6afe_port *port;
>> +
>> + if (!data) {
>> + dev_err(afe->dev, "%s: Invalid param data\n", __func__);
>> + return -EINVAL;
>> + }
>
> Just define on in the apr layer that data will never be NULL, that will
> save you 4 lines of code in every apr client.
>
I agree!
>> +
>> + if (data->payload_size) {
>> + uint32_t *payload = data->payload;
>
> So the payload is 2 ints, where the first is a command and the second is
> the status of it. This you can express in a simple struct to make the
> code even easier on the eye.
>
Will do that, if it make code more readable.
>> +
>> + if (data->opcode == APR_BASIC_RSP_RESULT) {
>> + if (payload[1] != 0) {
>> + afe->status = payload[1];
>> + dev_err(afe->dev,
>> + "cmd = 0x%x returned error = 0x%x\n",
>> + payload[0], payload[1]);
>> + }
>> + switch (payload[0]) {
>> + case AFE_PORT_CMD_SET_PARAM_V2:
>> + case AFE_PORT_CMD_DEVICE_STOP:
>> + case AFE_PORT_CMD_DEVICE_START:
>> + afe->state = AFE_CMD_RESP_AVAIL;
>> + port = afe_find_port(afe, data->token);
>> + if (port)
>> + wake_up(&port->wait);
>> +
>> + break;
>> + default:
>> + dev_err(afe->dev, "Unknown cmd 0x%x\n",
>> + payload[0]);
>
> If you flip the check for payload_size to return early if there isn't a
> payload then you can reduce the indentation level one step and probably
> doesn't have to wrap this line.
yep make sense!
>
>> + break;
>> + }
>> + }
>> + }
>> + return 0;
>> +}
>> +/**
>> + * q6afe_get_port_id() - Get port id from a given port index
>> + *
>> + * @index: port index
>> + *
>> + * Return: Will be an negative on error or valid port_id on success
>> + */
>> +int q6afe_get_port_id(int index)
>> +{
>> + if (index < 0 || index > AFE_PORT_MAX)
>> + return -EINVAL;
>> +
>> + return port_maps[index].port_id;
>> +}
>> +EXPORT_SYMBOL_GPL(q6afe_get_port_id);
>> +
>> +static int afe_apr_send_pkt(struct q6afev2 *afe, void *data,
>> + wait_queue_head_t *wait)
>
> Rather than conditionally passing the wait, split this function in
> afe_send_sync(*afe, *data, wait) and afe_send_async(*afe, *data).
>
Will do that across other modules too.
>> +{
>> + int ret;
>> +
>> + if (wait)
>> + afe->state = AFE_CMD_RESP_NONE;
>> +
>> + afe->status = 0;
>> + ret = apr_send_pkt(afe->apr, data);
>> + if (ret > 0) {
>
> Check ret < 0 and return here, this saves you one indentation level in
> the following chunk.
>
> If you then check !wait and return early you can reduce another level.
>
okay!
>> + if (wait) {
>> + ret = wait_event_timeout(*wait,
>> + (afe->state ==
>> + AFE_CMD_RESP_AVAIL),
>> + msecs_to_jiffies(TIMEOUT_MS));
>> + if (!ret) {
>> + ret = -ETIMEDOUT;
>> + } else if (afe->status > 0) {
>> + dev_err(afe->dev, "DSP returned error[%s]\n",
>> + adsp_err_get_err_str(afe->status));
>> + ret = adsp_err_get_lnx_err_code(afe->status);
>> + } else {
>> + ret = 0;
>> + }
>> + } else {
>> + ret = 0;
>> + }
>> + } else {
>> + dev_err(afe->dev, "packet not transmitted\n");
>> + ret = -EINVAL;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int afe_send_cmd_port_start(struct q6afe_port *port)
>> +{
>> + u16 port_id = port->id;
>> + struct afe_port_cmd_device_start start;
>> + struct q6afev2 *afe = port->afe.v2;
>> + int ret, index;
>> +
>> + index = port->token;
>> + start.hdr.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD,
>> + APR_HDR_LEN(APR_HDR_SIZE),
>> + APR_PKT_VER);
>> + start.hdr.pkt_size = sizeof(start);
>> + start.hdr.src_port = 0;
>> + start.hdr.dest_port = 0;
>> + start.hdr.token = index;
>
> Just put port->token here, saves you a local variable.
>
yep!
>> + start.hdr.opcode = AFE_PORT_CMD_DEVICE_START;
>> + start.port_id = port_id;
>> +
>> + ret = afe_apr_send_pkt(afe, &start, &port->wait);
>> + if (ret)
>> + dev_err(afe->dev, "AFE enable for port 0x%x failed %d\n",
>> + port_id, ret);
>> +
>> + return ret;
>> +}
>> +
>> +static int afe_port_start(struct q6afe_port *port,
>> + union afe_port_config *afe_config)
>> +{
>> + struct afe_audioif_config_command config;
>> + struct q6afev2 *afe = port->afe.v2;
>> + int ret = 0;
>> + int port_id = port->id;
>> + int cfg_type;
>> + int index = 0;
>> +
>> + if (!afe_config) {
>
> Looking at the one caller of this function, afe_config can't be NULL, so
> no need for this error handling.
>
okay.
>> + dev_err(afe->dev, "Error, no configuration data\n");
>> + ret = -EINVAL;
>> + return ret;
>> + }
>> +
>> + index = port->token;
>> +
>> + mutex_lock(&afe->afe_cmd_lock);
>> + /* Also send the topology id here: */
>> + config.hdr.hdr_field = APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD,
>> + APR_HDR_LEN(APR_HDR_SIZE),
>> + APR_PKT_VER);
>> + config.hdr.pkt_size = sizeof(config);
>> + config.hdr.src_port = 0;
>> + config.hdr.dest_port = 0;
>> + config.hdr.token = index;
>> +
>> + cfg_type = port->cfg_type;
>> + config.hdr.opcode = AFE_PORT_CMD_SET_PARAM_V2;
>> + config.param.port_id = port_id;
>> + config.param.payload_size = sizeof(config) - sizeof(struct apr_hdr) -
>> + sizeof(config.param);
>> + config.param.payload_address_lsw = 0x00;
>> + config.param.payload_address_msw = 0x00;
>> + config.param.mem_map_handle = 0x00;
>> + config.pdata.module_id = AFE_MODULE_AUDIO_DEV_INTERFACE;
>> + config.pdata.param_id = cfg_type;
>> + config.pdata.param_size = sizeof(config.port);
>
> This looks like a good candidate for a afe_port_set_param() function.
>
makes sense.
>> +
>> + config.port = *afe_config;
>> +
>> + ret = afe_apr_send_pkt(afe, &config, &port->wait);
>> + if (ret) {
>> + dev_err(afe->dev, "AFE enable for port 0x%x failed %d\n",
>> + port_id, ret);
>> + goto fail_cmd;
>> + }
>> +
>> + ret = afe_send_cmd_port_start(port);
>> +
>> +fail_cmd:
>> + mutex_unlock(&afe->afe_cmd_lock);
>> + return ret;
>> +}
> [..]
>> +/**
>> + * q6afe_port_get_from_id() - Get port instance from a port id
>> + *
>> + * @dev: Pointer to afe child device.
>> + * @id: port id
>> + *
>> + * Return: Will be an error pointer on error or a valid afe port
>> + * on success.
>> + */
>> +struct q6afe_port *q6afe_port_get_from_id(struct device *dev, int id)
>
> Will there be any other getter? Otherwise you can just call this
> q6afe_port_get().
There is one more get, which is basically lookup from index to port number.
>
>> +{
>> + int port_id;
>> + struct q6afev2 *afe = dev_get_drvdata(dev->parent);
>> + struct q6afe_port *port;
>> + int token;
>> + int cfg_type;
>> +
>> + if (!afe) {
>> + dev_err(dev, "Unable to find instance of afe service\n");
>> + return ERR_PTR(-ENOENT);
>> + }
>
> As this comes from the passed dev this check will catch bugs withing
> this driver, but if the client accidentally passes the wrong dev it's
> likely that you don't catch it here anyways. Consider dropping the
> check.
yes!
>
>> +
>> + token = id;
>> + if (token < 0 || token > AFE_PORT_MAX) {
>> + dev_err(dev, "AFE port token[%d] invalid!\n", token);
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + port_id = port_maps[id].port_id;
>> +
>> + port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
>> + if (!port)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + init_waitqueue_head(&port->wait);
>> +
>> + port->token = token;
>> + port->id = port_id;
>> +
>> + port->afe.v2 = afe;
>> + switch (port_id) {
>
> How about moving this switch statement above the allocation?
>
Yes, this can be done!
>> + case AFE_PORT_ID_MULTICHAN_HDMI_RX:
>> + cfg_type = AFE_PARAM_ID_HDMI_CONFIG;
>> + break;
>> + default:
>> + dev_err(dev, "Invalid port id 0x%x\n", port_id);
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + port->cfg_type = cfg_type;
>> +
>> + spin_lock(&afe->port_list_lock);
>> + list_add_tail(&port->node, &afe->port_list);
>> + spin_unlock(&afe->port_list_lock);
>> +
>> + return port;
>> +
>> +}
>> +EXPORT_SYMBOL_GPL(q6afe_port_get_from_id);
>
> Regards,
> Bjorn
>
^ permalink raw reply
* [RESEND PATCH v2 03/15] ASoC: qcom: qdsp6: Add common qdsp6 helper functions
From: Srinivas Kandagatla @ 2018-01-03 16:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180102001907.GL478@tuxbook>
Thanks for the review comments,
On 02/01/18 00:19, Bjorn Andersson wrote:
> On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla at linaro.org wrote:
>> +static inline int q6dsp_map_channels(u8 *ch_map, int ch)
>> +{
>> + memset(ch_map, 0, PCM_FORMAT_MAX_NUM_CHANNEL);
>
> This implies that ch_map is always an array of
> PCM_FORMAT_MAX_NUM_CHANNEL elements. As such it would be better to
> express this in the prototype; i.e u8 ch_map[PCM_FORMAT_MAX_NUM_CHANNEL]
>
Yep, Will do that.
>> +
>> + if (ch == 1) {
>
> This is a switch statement.
>
Yes, makes more sense.
>> + ch_map[0] = PCM_CHANNEL_FC;
>> + } else if (ch == 2) {
> [..]
>> +struct adsp_err_code {
>> + int lnx_err_code;
>
> Indentation, and these could be given more succinct names.
>
>> + char *adsp_err_str;
>> +};
>> +
>> +static struct adsp_err_code adsp_err_code_info[ADSP_ERR_MAX+1] = {
>> + { 0, ADSP_EOK_STR},
>> + { -ENOTRECOVERABLE, ADSP_EFAILED_STR},
>> + { -EINVAL, ADSP_EBADPARAM_STR},
>> + { -ENOSYS, ADSP_EUNSUPPORTED_STR},
>> + { -ENOPROTOOPT, ADSP_EVERSION_STR},
>> + { -ENOTRECOVERABLE, ADSP_EUNEXPECTED_STR},
>> + { -ENOTRECOVERABLE, ADSP_EPANIC_STR},
>> + { -ENOSPC, ADSP_ENORESOURCE_STR},
>> + { -EBADR, ADSP_EHANDLE_STR},
>> + { -EALREADY, ADSP_EALREADY_STR},
>> + { -EPERM, ADSP_ENOTREADY_STR},
>> + { -EINPROGRESS, ADSP_EPENDING_STR},
>> + { -EBUSY, ADSP_EBUSY_STR},
>> + { -ECANCELED, ADSP_EABORTED_STR},
>> + { -EAGAIN, ADSP_EPREEMPTED_STR},
>> + { -EAGAIN, ADSP_ECONTINUE_STR},
>> + { -EAGAIN, ADSP_EIMMEDIATE_STR},
>> + { -EAGAIN, ADSP_ENOTIMPL_STR},
>> + { -ENODATA, ADSP_ENEEDMORE_STR},
>> + { -EADV, ADSP_ERR_MAX_STR},
>
> This, element 0x13, is not listed among the defined errors. Is this a
> placeholder?
>
> How about making this even more descriptive by using the format
>
> [ADSP_EBADPARAM] = { -EINVAL, ADSP_EBADPARAM_STR },
>
> That way the mapping table is self-describing.
>
> And you can use ARRAY_SIZE() instead of specifying the fixed size of
> ADSP_ERR_MAX + 1...
>
Will give that a try!
>> + { -ENOMEM, ADSP_ENOMEMORY_STR},
>> + { -ENODEV, ADSP_ENOTEXIST_STR},
>> + { -EADV, ADSP_ERR_MAX_STR},
>
> "Advertise error"?
No, downstream seems to define any unexpected error as -EADV, am not
sure if this correct, probably we should change this to be more sensible
one.
>
>> +};
>> +
>> +static inline int adsp_err_get_lnx_err_code(u32 adsp_error)
>
> Can this be made internal to some c-file? So that any third party deals
> only with linux error codes?
>
>
> How about renaming this q6dsp_errno()?
>
yep will do that.
>> +{
>> + if (adsp_error > ADSP_ERR_MAX)
>> + return adsp_err_code_info[ADSP_ERR_MAX].lnx_err_code;
>> + else
>> + return adsp_err_code_info[adsp_error].lnx_err_code;
>
> I think this would look better if you assign a local variable and have a
> single return. And just hard code the "invalid error code" errno, rather
> than looking up ADSP_ERR_MAX in the list.
>
>> +}
>> +
>> +static inline char *adsp_err_get_err_str(u32 adsp_error)
>
> q6dsp_strerror(), to match strerror(3)?
yep!
>
>> +{
>> + if (adsp_error > ADSP_ERR_MAX)
>> + return adsp_err_code_info[ADSP_ERR_MAX].adsp_err_str;
>> + else
>> + return adsp_err_code_info[adsp_error].adsp_err_str;
>
> And I do think that, as with strerror, this should return a human
> readable error, not the stringified define.
okay!
>
>> +}
>
>
> I'm puzzled to why these helper functions lives in a header file, at
> least some aspects of this would better be hidden...
Will try to improve on this in next version.
>
> Regards,
> Bjorn
>
^ permalink raw reply
* [RESEND PATCH v2 02/15] soc: qcom: add support to APR bus driver
From: Srinivas Kandagatla @ 2018-01-03 16:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180101232932.GK478@tuxbook>
Thank Bjorn for the Review.
On 01/01/18 23:29, Bjorn Andersson wrote:
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index b81374bb6713..1daa39925dd4 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -97,4 +97,12 @@ config QCOM_WCNSS_CTRL
>> Client driver for the WCNSS_CTRL SMD channel, used to download nv
>> firmware to a newly booted WCNSS chip.
>>
>> +config QCOM_APR
>> + tristate "Qualcomm APR Bus (Asynchronous Packet Router)"
>> + depends on (RPMSG_QCOM_SMD || RPMSG_QCOM_GLINK_RPM)
>
> The actual dependency you have is RPMSG. Specifying the individual
> transports here causes additional restrictions in how you can mix and
> match modules vs builtin (e.g. glink being builtin to get regulators
> early and smd built as a module forces apr to be built as a module)
>
I agree, Will fix this in next version.
>> +++ b/drivers/soc/qcom/apr.c
>> @@ -0,0 +1,395 @@
>> +/* SPDX-License-Identifier: GPL-2.0
>> +* Copyright (c) 2011-2016, The Linux Foundation
>> +* Copyright (c) 2017, Linaro Limited
>> +*/
>
> For some tooling reason the SPDX line should be // commented, i.e this
> should be:
>
> // SPDX-License-Identifier: GPL-2.0
> /*
> * Copyright (c) 2011-2016, The Linux Foundation
> * Copyright (c) 2017, Linaro Limited
> */
>
Yep, this was also considered for next version.
>> +
...
>> +
>> +/* Static CORE service on the ADSP */
>> +static const struct apr_device_id core_svc_device_id =
>> + ADSP_AUDIO_APR_DEV("CORE", APR_SVC_ADSP_CORE);
>
> How does this work out when we want to use APR for the modem services?
>
So the plan is, If the modem service can be queried using AVCS core
commands then it would be added dynamically from q6core driver else we
can add is as static service into this list.
>> +
>> +/**
>> + * apr_send_pkt() - Send a apr message from apr device
>> + *
>> + * @adev: Pointer to previously registered apr device.
>> + * @buf: Pointer to buffer to send
>> + *
>> + * Return: Will be an negative on packet size on success.
>> + */
>> +int apr_send_pkt(struct apr_device *adev, uint32_t *buf)
>
> So buf here is a struct apr_hdr followed by arbitrary data. Passing a
> reference to an arbitrary chunk of data should be done with a void *.
> But you could turn struct apr_hdr into struct apr_packet by adding a
> flexible array member at the end of the struct and having this function
> take that data type. This would make the prototype self-documenting.
>
>
> I do, however, not fancy the asymmetry of the send/callback interface
> exposed to the client. One takes the raw packet, as it sits in the
> transport and the other creates an abstract representation of the same.
>
> Can't you in the callback verify the data and then just pass the same
> object up to the client?
I can try it and see how it looks.
>
>> +{
...
>> +
>> +static int qcom_rpmsg_q6_callback(struct rpmsg_device *rpdev, void *buf,
>> + int len, void *priv, u32 addr)
>> +{
>> + struct apr *apr = dev_get_drvdata(&rpdev->dev);
>> + struct apr_client_data data;
>> + struct apr_device *p, *c_svc = NULL;
>> + struct apr_driver *adrv = NULL;
>> + struct apr_hdr *hdr;
>> + uint16_t hdr_size;
>> + uint16_t msg_type;
>> + uint16_t ver;
>> + uint16_t src;
>> + uint16_t svc;
>> +
>> + if (!buf || len <= APR_HDR_SIZE) {
>
> rpmsg should never call you with buf = NULL, so no reason to check for
> that.
Yep!
>
>> + dev_err(apr->dev, "APR: Improper apr pkt received:%p %d\n",
>> + buf, len);
>> + return -EINVAL;
>> + }
...
>> +
>> + if (hdr->src_domain >= APR_DOMAIN_MAX ||
>> + hdr->dest_domain >= APR_DOMAIN_MAX ||
>> + hdr->src_svc >= APR_SVC_MAX ||
>> + hdr->dest_svc >= APR_SVC_MAX) {
>> + dev_err(apr->dev, "APR: Wrong APR header\n");
>> + return -EINVAL;
>> + }
>> +
>> + svc = hdr->dest_svc;
>> + src = apr->data->get_data_src(hdr);
>
> Couldn't we just use src_domain here instead of maintaining this mapping
> table in the driver?
Yes, we can do that too, I will give that a go.
>
> Also, looking at the send function, isn't this src just c_svc->svc_id?
>
src here represents mapping between domain and dest id. It is not same
as svc_id.
>
>> + if (src == APR_DEST_MAX)
>> + return -EINVAL;
>> +
>> + spin_lock(&apr->svcs_lock);
>> + list_for_each_entry(p, &apr->svcs, node) {
>> + if (svc == p->svc_id) {
>> + c_svc = p;
>> + if (c_svc->dev.driver)
>> + adrv = to_apr_driver(c_svc->dev.driver);
>> + break;
>> + }
>> + }
>
> Keep your services in a idr, keyed by svc_id. This will make the search
> O(log(n)), but more importantly it would replace this loop with a single
> idr_find().
>
I will try it and see how it looks!
>> + spin_unlock(&apr->svcs_lock);
>> +
>> + if (!adrv) {
>> + dev_err(apr->dev, "APR: service is not registered\n");
>> + return -EINVAL;
>> + }
>> +
>> + data.payload_size = hdr->pkt_size - hdr_size;
>> + data.opcode = hdr->opcode;
>> + data.src = src;
>> + data.src_port = hdr->src_port;
>> + data.dest_port = hdr->dest_port;
>> + data.token = hdr->token;
>> + data.msg_type = msg_type;
>> +
>> + if (data.payload_size > 0)
>> + data.payload = (char *)hdr + hdr_size;
>
> Using buf + hdr_size probably makes even more sense, and saves you from
> the typecast.
>
Agree!
>> +
>> + adrv->callback(c_svc, &data);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct apr_device_id *apr_match(const struct apr_device_id *id,
>> + const struct apr_device *adev)
>> +{
>> + while (id->domain_id != 0 || id->svc_id != 0) {
>> + if (id->domain_id == adev->domain_id &&
>> + id->svc_id == adev->svc_id &&
>> + id->client_id == adev->client_id)
>
> Is the client_id significant here? As far as I can tell it's a
> identifier of the local "function". Are there multiple client drivers
> that would "attach" to the same remote service?
No. svc id is unique per client id, so it redundant check as you said.
>
>> + return id;
>> + id++;
>> + }
>> + return NULL;
>> +}
>> +
>> +static int apr_device_match(struct device *dev, struct device_driver *drv)
>> +{
>> + struct apr_device *adev = to_apr_device(dev);
>> + struct apr_driver *adrv = to_apr_driver(drv);
>> +
>> + return !!apr_match(adrv->id_table, adev);
>
> If this remain the only caller of apr_match() you could just inline the
> loop here.
Makes sense.
>
>> +}
>> +
>> +static int apr_device_probe(struct device *dev)
>> +{
>> + struct apr_device *adev;
>> + struct apr_driver *adrv;
>
> You don't indent things elsewhere.
Yep.
>
>> + int ret = 0;
>> +
>> + adev = to_apr_device(dev);
>> + adrv = to_apr_driver(dev->driver);
>> +
>> + ret = adrv->probe(adev);
>
> Drop ret and just return adrv->probe()
>
yep
>> +
>> + return ret;
>> +}
>> +
...
>> +
>> +/**
>> + * apr_add_device() - Add a new apr device
>> + *
>> + * @dev: Pointer to apr device.
>> + * @id: Pointer to apr device id to add.
>> + *
>> + * Return: Will be an negative on error or a zero on success.
>> + */
>> +int apr_add_device(struct device *dev, const struct apr_device_id *id)
>> +{
>> + struct apr *apr = dev_get_drvdata(dev);
>
> It's not obvious which dev this is, but it has to be the rpmsg device or
> this would dereference the drvdata of the wrong type and things would be
> very bad.
>
> How about making this more explicit by just taking a struct apr * as
> first argument, and then provide a helper for clients to acquire the
> struct apr * from the parent dev, if this is needed.
>
Yep, that makes it much cleaner, will do it in next version.
>> + struct apr_device *adev = NULL;
>> +
>> + if (!apr)
>> + return -EINVAL;
>> +
>> + adev = kzalloc(sizeof(*adev), GFP_KERNEL);
>> + if (!adev)
>> + return -ENOMEM;
>> +
>> + spin_lock_init(&adev->lock);
>> +
>> + adev->svc_id = id->svc_id;
>> + adev->dest_id = apr->dest_id;
>> + adev->client_id = id->client_id;
>> + adev->domain_id = id->domain_id;
>
> Wouldn't this always be the domain of the apr? (or we're adding a device
> to the wrong apr)
>
Yes, it is.
>> + adev->version = id->svc_version;
>> +
>> + adev->dev.bus = &aprbus_type;
>> + adev->dev.parent = dev;
>> + adev->dev.release = apr_dev_release;
>> + adev->dev.driver = NULL;
>> +
>> + dev_set_name(&adev->dev, "apr:%s:%x:%x:%x", id->name, id->domain_id,
>> + id->svc_id, id->client_id);
>> +
>> + spin_lock(&apr->svcs_lock);
>> + list_add_tail(&adev->node, &apr->svcs);
>> + spin_unlock(&apr->svcs_lock);
>
> This causes svcs to contain entries related to devices that has not been
> matched and probed yet (e.g. implementation is in a kernel module not
> loaded). I think you should move this to apr_device_probe().
>
Yep!
>> +
>> + return device_register(&adev->dev);
>> +}
>> +EXPORT_SYMBOL_GPL(apr_add_device);
>> +
...
>> +static int apr_v2_get_data_src(struct apr_hdr *hdr)
>> +{
>> + if (hdr->src_domain == APR_DOMAIN_MODEM)
>> + return APR_DEST_MODEM;
>> + else if (hdr->src_domain == APR_DOMAIN_ADSP)
>> + return APR_DEST_QDSP6;
>> +
>> + return APR_DEST_MAX;
>
> The idiomatic return value here would be -EINVAL.
>
yep!, I try it this would also change logic at caller side.
>> +}
>> +
>> +/*
>
>> +
>> +static const struct apr_data apr_v2_data = {
>> + .get_data_src = apr_v2_get_data_src,
>> + .dest_id = APR_DEST_QDSP6,
>> +};
>> +
>> +static const struct of_device_id qcom_rpmsg_q6_of_match[] = {
>> + { .compatible = "qcom,apr-msm8996", .data = &apr_v2_data},
>
> The dest_id of apr_v2_data and the use of "q6" in the name indicates
> that this is the "msm8996 adsp apr", what's your plan to support other
> apr remotes?
We would need one more entry for modem case, like db410c cases, Will add
this as we move on.
> How about making the domain id a property of the apr in DT and then just
> list the clients as nodes with svc_id, svc_version and client_id? You
> can still match by device_id (compatible can be optional), but that
> would allow you to describe either the adsp or modem apr link, of any
> platform (of that apr version), with the same compatible.
>
This will work too!
Current design I had in mind was to get the q6core service up and this
can query what AVCS static services are available on remote side and
then add apr devices.
If we go with dt approch we might not need q6core driver, but on the
other hand we need to be aware of svc major and minor version, which
might be specific to the dsp firmware running on. Also we might endup
talking on services without querying the dsp state.
May be we should do some offline disussion on this!
>> + {}
>> +};
>> +
>> +static struct rpmsg_driver qcom_rpmsg_q6_driver = {
>> + .probe = qcom_rpmsg_q6_probe,
>> + .remove = qcom_rpmsg_q6_remove,
>> + .callback = qcom_rpmsg_q6_callback,
>> + .drv = {
>> + .name = "qcom_rpmsg_q6",
>> + .owner = THIS_MODULE,
>
> Drop the owner.
>
Yep!
>> + .of_match_table = qcom_rpmsg_q6_of_match,
>> + },
>
> Indentation.
Sure!
>
>> +};
>> +
>> +static int __init apr_init(void)
>> +{
>> + int ret;
>> +
>> + ret = register_rpmsg_driver(&qcom_rpmsg_q6_driver);
>> + if (!ret)
>> + return bus_register(&aprbus_type);
>
> Register the bus first, then the rpmsg driver.
yep!
>
>> +
>> + return ret;
>> +}
>> +
...
>> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
>> index abb6dc2ebbf8..068d215c3be6 100644
>> --- a/include/linux/mod_devicetable.h
>> +++ b/include/linux/mod_devicetable.h
>> @@ -452,6 +452,19 @@ struct spi_device_id {
>> kernel_ulong_t driver_data; /* Data private to the driver */
>> };
>>
>> +
>
> One newline is enough.
yep
>
>> +#define APR_NAME_SIZE 32
>> +#define APR_MODULE_PREFIX "apr:"
>> +
>> +struct apr_device_id {
>> + char name[APR_NAME_SIZE];
>
> Does this name has any meaning? As far as I can see we're matching on
> the other parameters and use the name only to name the device.
No, it just to give the device a usefull name.
>
>> + __u32 domain_id;
>> + __u32 svc_id;
>> + __u32 client_id;
>> + __u32 svc_version;
>> + kernel_ulong_t driver_data; /* Data private to the driver */
>> +};
>> +
>> #define SPMI_NAME_SIZE 32
>> #define SPMI_MODULE_PREFIX "spmi:"
>>
>> diff --git a/include/linux/soc/qcom/apr.h b/include/linux/soc/qcom/apr.h
>> new file mode 100644
>> index 000000000000..8620289c34ab
>> --- /dev/null
>> +++ b/include/linux/soc/qcom/apr.h
...
>> +
>> +#define APR_DL_SMD 0
>> +#define APR_DL_MAX 1
>
> Unused.
will remove it.
>
>> +
...
>> +#define APR_HDR_SIZE sizeof(struct apr_hdr)
>> +#define APR_SEQ_CMD_HDR_FIELD APR_HDR_FIELD(APR_MSG_TYPE_SEQ_CMD, \
>> + APR_HDR_LEN(APR_HDR_SIZE), \
>> + APR_PKT_VER)
>
> So for the tx path these macros are to be used by the client and for the
> rx path they are to be used by the apr driver. Better make the api
> symmetrical.
Will try that in next version.
>
> One possible path is to use an sk_buf for the tx-path and reserve space
> for the header, then pass the abstract version of the packet and let the
> apr driver fill out the header.
>
In some cases like q6asm the apr header port numbers are much more
specific to the service and they change depening on stream and session
ids within the service.
>> +
>> +struct apr_client_data {
>
> Perhaps name this apr_client_message or similar, to indicate that this
> is not a per-client thing, but a description of a message/packet.
make sense, Will change it to apr_client_message.
>
>> + uint16_t payload_size;
...
>> + void *payload;
>> +};
>> +
>
> Regards,
> Bjorn
>
^ permalink raw reply
* [RESEND PATCH v2 01/15] dt-bindings: soc: qcom: Add bindings for APR bus
From: Srinivas Kandagatla @ 2018-01-03 16:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180103003504.GX478@tuxbook>
Thanks for review comments!
On 03/01/18 00:35, Bjorn Andersson wrote:
> On Thu 14 Dec 09:33 PST 2017, srinivas.kandagatla at linaro.org wrote:
>
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> This patch add dt bindings for Qualcomm APR bus driver
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>> .../devicetree/bindings/soc/qcom/qcom,apr.txt | 28 ++++++++++++++++++++++
>> 1 file changed, 28 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,apr.txt
>>
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,apr.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,apr.txt
>> new file mode 100644
>> index 000000000000..4e93213ae98d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,apr.txt
>> @@ -0,0 +1,28 @@
>> +Qualcomm APR (Asynchronous Packet Router) binding
>> +
>> +This binding describes the Qualcomm APR. APR is a IPC protocol for
>> +communication between Application processor and QDSP. APR is mainly
>> +used for audio/voice services on the QDSP.
>> +
>> +- compatible:
>> + Usage: required
>> + Value type: <stringlist>
>> + Definition: must be "qcom,apr-<SOC-NAME>" example: "qcom,apr-msm8996"
>
> This is not the only apr on msm8996 and some platform seems to have 3-4
> aprs. I'm therefor hesitant to use the compatible to pick the static
> list of services available on the particular firmware.
>
> If this scheme is followed we at least would need to rename this
> qcom,msm8996-apr-audio-svc
>
> But I think it would be preferable to go with qcom,apr-v2 and then list
> the static services as child nodes.
Am okay doing it, Only issue I have is the dsp state. If the dsp AVCS is
not ready yet then we would be chatting on the service without the
services being up on remote side?
Chances of this seems to be slim but we need to confirm this before we
make a call on this.
>
>> +
>> +
>> +- qcom,smd-channel:
>> + Usage: required
>> + Value type: <string>
>> + Definition: standard SMD property specifying the SMD channel used for
>> + communication with the APR on QDSP.
>> + Should be "apr_audio_svc".
>
> This is not the only APR channel, but for apr itself this doesn't matter
> and might as well be qcom,glink-channels; so perhaps we can omit this
> from this document?
Yes, I agree with you. will remove this in next version.
>
>> + Described in soc/qcom/qcom,smd.txt
>> +
>> += EXAMPLE
>> +The following example represents a QDSP based sound card on a MSM8996 device
>> +which uses apr as communication between Apps and QDSP.
>> +
>> + apr {
>> + compatible = "qcom,apr-msm8996";
>> + qcom,smd-channels = "apr_audio_svc";
>> + };
>
> Regards,
> Bjorn
>
^ permalink raw reply
* [PATCH V3 3/3] arm64: Extend early page table code to allow for larger kernels
From: Steve Capper @ 2018-01-03 16:20 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKv+Gu8tpRhri5CbvhYzE-ZZViqt7EAiDhf=EY_Ft=LoWu5-cQ@mail.gmail.com>
On Tue, Jan 02, 2018 at 10:01:29PM +0000, Ard Biesheuvel wrote:
> Hi Steve,
Hi Ard,
>
> On 2 January 2018 at 15:12, Steve Capper <steve.capper@arm.com> wrote:
> > Currently the early assembler page table code assumes that precisely
> > 1xpgd, 1xpud, 1xpmd are sufficient to represent the early kernel text
> > mappings.
> >
> > Unfortunately this is rarely the case when running with a 16KB granule,
> > and we also run into limits with 4KB granule when building much larger
> > kernels.
> >
> > This patch re-writes the early page table logic to compute indices of
> > mappings for each level of page table, and if multiple indices are
> > required, the next-level page table is scaled up accordingly.
> >
> > Also the required size of the swapper_pg_dir is computed at link time
> > to cover the mapping [KIMAGE_ADDR + VOFFSET, _end]. When KASLR is
> > enabled, an extra page is set aside for each level that may require extra
> > entries at runtime.
> >
> > Signed-off-by: Steve Capper <steve.capper@arm.com>
> >
> > ---
> > Changed in V3:
> > Corrected KASLR computation
> > Rebased against arm64/for-next/core, particularly Kristina's 52-bit
> > PA series.
> > ---
> > arch/arm64/include/asm/kernel-pgtable.h | 47 ++++++++++-
> > arch/arm64/include/asm/pgtable.h | 1 +
> > arch/arm64/kernel/head.S | 145 +++++++++++++++++++++++---------
> > arch/arm64/kernel/vmlinux.lds.S | 1 +
> > arch/arm64/mm/mmu.c | 3 +-
> > 5 files changed, 157 insertions(+), 40 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kernel-pgtable.h b/arch/arm64/include/asm/kernel-pgtable.h
> > index 77a27af01371..82386e860dd2 100644
> > --- a/arch/arm64/include/asm/kernel-pgtable.h
> > +++ b/arch/arm64/include/asm/kernel-pgtable.h
> > @@ -52,7 +52,52 @@
> > #define IDMAP_PGTABLE_LEVELS (ARM64_HW_PGTABLE_LEVELS(PHYS_MASK_SHIFT))
> > #endif
> >
> > -#define SWAPPER_DIR_SIZE (SWAPPER_PGTABLE_LEVELS * PAGE_SIZE)
> > +
> > +/*
> > + * If KASLR is enabled, then an offset K is added to the kernel address
> > + * space. The bottom 21 bits of this offset are zero to guarantee 2MB
> > + * alignment for PA and VA.
> > + *
> > + * For each pagetable level of the swapper, we know that the shift will
> > + * be larger than 21 (for the 4KB granule case we use section maps thus
> > + * the smallest shift is actually 30) thus there is the possibility that
> > + * KASLR can increase the number of pagetable entries by 1, so we make
> > + * room for this extra entry.
> > + *
> > + * Note KASLR cannot increase the number of required entries for a level
> > + * by more than one because it increments both the virtual start and end
> > + * addresses equally (the extra entry comes from the case where the end
> > + * address is just pushed over a boundary and the start address isn't).
> > + */
> > +
> > +#ifdef CONFIG_RANDOMIZE_BASE
> > +#define EARLY_KASLR (1)
> > +#else
> > +#define EARLY_KASLR (0)
> > +#endif
> > +
> > +#define EARLY_ENTRIES(vstart, vend, shift) (((vend) >> (shift)) \
> > + - ((vstart) >> (shift)) + 1 + EARLY_KASLR)
> > +
> > +#define EARLY_PGDS(vstart, vend) (EARLY_ENTRIES(vstart, vend, PGDIR_SHIFT))
> > +
> > +#if SWAPPER_PGTABLE_LEVELS > 3
> > +#define EARLY_PUDS(vstart, vend) (EARLY_ENTRIES(vstart, vend, PUD_SHIFT))
> > +#else
> > +#define EARLY_PUDS(vstart, vend) (0)
> > +#endif
> > +
> > +#if SWAPPER_PGTABLE_LEVELS > 2
> > +#define EARLY_PMDS(vstart, vend) (EARLY_ENTRIES(vstart, vend, SWAPPER_TABLE_SHIFT))
> > +#else
> > +#define EARLY_PMDS(vstart, vend) (0)
> > +#endif
> > +
> > +#define EARLY_PAGES(vstart, vend) ( 1 /* PGDIR page */ \
> > + + EARLY_PGDS((vstart), (vend)) /* each PGDIR needs a next level page table */ \
> > + + EARLY_PUDS((vstart), (vend)) /* each PUD needs a next level page table */ \
> > + + EARLY_PMDS((vstart), (vend))) /* each PMD needs a next level page table */
> > +#define SWAPPER_DIR_SIZE (PAGE_SIZE * EARLY_PAGES(KIMAGE_VADDR + TEXT_OFFSET, _end))
> > #define IDMAP_DIR_SIZE (IDMAP_PGTABLE_LEVELS * PAGE_SIZE)
> >
> > #ifdef CONFIG_ARM64_SW_TTBR0_PAN
> > diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> > index bfa237e892f1..54b0a8398055 100644
> > --- a/arch/arm64/include/asm/pgtable.h
> > +++ b/arch/arm64/include/asm/pgtable.h
> > @@ -706,6 +706,7 @@ static inline void pmdp_set_wrprotect(struct mm_struct *mm,
> > #endif
> >
> > extern pgd_t swapper_pg_dir[PTRS_PER_PGD];
> > +extern pgd_t swapper_pg_end[];
> > extern pgd_t idmap_pg_dir[PTRS_PER_PGD];
> > extern pgd_t tramp_pg_dir[PTRS_PER_PGD];
> >
> > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> > index 66f01869e97c..539e2642ed41 100644
> > --- a/arch/arm64/kernel/head.S
> > +++ b/arch/arm64/kernel/head.S
> > @@ -191,44 +191,110 @@ ENDPROC(preserve_boot_args)
> > .endm
> >
> > /*
> > - * Macro to populate the PGD (and possibily PUD) for the corresponding
> > - * block entry in the next level (tbl) for the given virtual address.
> > + * Macro to populate page table entries, these entries can be pointers to the next level
> > + * or last level entries pointing to physical memory.
> > *
> > - * Preserves: tbl, next, virt
> > - * Corrupts: ptrs_per_pgd, tmp1, tmp2
> > + * tbl: page table address
> > + * rtbl: pointer to page table or physical memory
> > + * index: start index to write
> > + * eindex: end index to write - [index, eindex] written to
> > + * flags: flags for pagetable entry to or in
> > + * inc: increment to rtbl between each entry
> > + * tmp1: temporary variable
> > + *
> > + * Preserves: tbl, eindex, flags, inc
> > + * Corrupts: index, tmp1
> > + * Returns: rtbl
> > */
> > - .macro create_pgd_entry, tbl, virt, ptrs_per_pgd, tmp1, tmp2
> > - create_table_entry \tbl, \virt, PGDIR_SHIFT, \ptrs_per_pgd, \tmp1, \tmp2
> > -#if SWAPPER_PGTABLE_LEVELS > 3
> > - mov \ptrs_per_pgd, PTRS_PER_PUD
> > - create_table_entry \tbl, \virt, PUD_SHIFT, \ptrs_per_pgd, \tmp1, \tmp2
> > -#endif
> > -#if SWAPPER_PGTABLE_LEVELS > 2
> > - mov \ptrs_per_pgd, PTRS_PER_PTE
> > - create_table_entry \tbl, \virt, SWAPPER_TABLE_SHIFT, \ptrs_per_pgd, \tmp1, \tmp2
> > -#endif
> > + .macro populate_entries, tbl, rtbl, index, eindex, flags, inc, tmp1
> > +9999: phys_to_pte \rtbl, \tmp1
>
> I know this is existing code, but you could take the opportunity to
> replace this label with
>
> .L\@ :
>
> (and update the branch instruction accordingly) so that we don't have
> to rely on the uniqueness of '9999'
Thanks, I must confess I was unaware of that technique and had to look
it up; I will incorporate this into the patch.
>
> > + orr \tmp1, \tmp1, \flags // tmp1 = table entry
> > + str \tmp1, [\tbl, \index, lsl #3]
> > + add \rtbl, \rtbl, \inc // rtbl = pa next level
> > + add \index, \index, #1
> > + cmp \index, \eindex
> > + b.ls 9999b
> > .endm
> >
> > /*
> > - * Macro to populate block entries in the page table for the start..end
> > - * virtual range (inclusive).
> > + * Compute indices of table entries from virtual address range. If multiple entries
> > + * were needed in the previous page table level then the next page table level is assumed
> > + * to be composed of multiple pages. (This effectively scales the end index).
> > + *
> > + * vstart: virtual address of start of range
> > + * vend: virtual address of end of range
> > + * shift: shift used to transform virtual address into index
> > + * ptrs: number of entries in page table
> > + * istart: index in table corresponding to vstart
> > + * iend: index in table corresponding to vend
> > + * count: On entry: how many entries required in previous level, scales our end index
> > + * On exit: returns how many entries required for next page table level
> > *
>
> If you make 'count' the number of /additional/ entries, you no longer
> have to add/sub #1 each time.
Agreed, I'll simplify this.
>
> > - * Preserves: tbl, flags
> > - * Corrupts: phys, start, end, tmp, pstate
> > + * Preserves: vstart, vend, shift, ptrs
> > + * Returns: istart, iend, count
> > */
> > - .macro create_block_map, tbl, flags, phys, start, end, tmp
> > - lsr \start, \start, #SWAPPER_BLOCK_SHIFT
> > - and \start, \start, #PTRS_PER_PTE - 1 // table index
> > - bic \phys, \phys, #SWAPPER_BLOCK_SIZE - 1
> > - lsr \end, \end, #SWAPPER_BLOCK_SHIFT
> > - and \end, \end, #PTRS_PER_PTE - 1 // table end index
> > -9999: phys_to_pte \phys, \tmp
> > - orr \tmp, \tmp, \flags // table entry
> > - str \tmp, [\tbl, \start, lsl #3] // store the entry
> > - add \start, \start, #1 // next entry
> > - add \phys, \phys, #SWAPPER_BLOCK_SIZE // next block
> > - cmp \start, \end
> > - b.ls 9999b
> > + .macro compute_indices, vstart, vend, shift, ptrs, istart, iend, count
> > + lsr \iend, \vend, \shift
> > + mov \istart, \ptrs
> > + sub \istart, \istart, #1
> > + and \iend, \iend, \istart // iend = (vend >> shift) & (ptrs - 1)
> > + mov \istart, \ptrs
> > + sub \count, \count, #1
> > + mul \istart, \istart, \count
> > + add \iend, \iend, \istart // iend += (count - 1) * ptrs
> > + // our entries span multiple tables
> > +
> > + lsr \istart, \vstart, \shift
> > + mov \count, \ptrs
> > + sub \count, \count, #1
> > + and \istart, \istart, \count
> > +
> > + sub \count, \iend, \istart
> > + add \count, \count, #1
> > + .endm
> > +
>
> You can simplify this macro by using an immediate for \ptrs. Please
> see the diff below [whitespace mangling courtesy of Gmail]
Thanks, I like the simplification a lot. For 52-bit kernel VAs though, I
will need a variable PTRS_PER_PGD at compile time.
For 52-bit kernel PAs one can just use the maximum number of ptrs available.
For a 48-bit PA the leading address bits will always be zero, thus we
will derive the same PGD indices from both 48 and 52 bit PTRS_PER_PGD.
For kernel VAs, because the leading address bits are one, we need to use a
PTRS_PER_PGD corresponding to the VA size.
>
> > +/*
> > + * Map memory for specified virtual address range. Each level of page table needed supports
> > + * multiple entries. If a level requires n entries the next page table level is assumed to be
> > + * formed from n pages.
> > + *
> > + * tbl: location of page table
> > + * rtbl: address to be used for first level page table entry (typically tbl + PAGE_SIZE)
> > + * vstart: start address to map
> > + * vend: end address to map - we map [vstart, vend]
> > + * flags: flags to use to map last level entries
> > + * phys: physical address corresponding to vstart - physical memory is contiguous
> > + * pgds: the number of pgd entries
> > + *
> > + * Temporaries: istart, iend, tmp, count, sv - these need to be different registers
> > + * Preserves: vstart, vend, flags
> > + * Corrupts: tbl, rtbl, istart, iend, tmp, count, sv
> > + */
> > + .macro map_memory, tbl, rtbl, vstart, vend, flags, phys, pgds, istart, iend, tmp, count, sv
> > + add \rtbl, \tbl, #PAGE_SIZE
> > + mov \sv, \rtbl
> > + mov \count, #1
>
> #0 if you make \count the number of additional entries.
>
> In any case, for the series:
>
> Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Thanks!
>
> > + compute_indices \vstart, \vend, #PGDIR_SHIFT, \pgds, \istart, \iend, \count
> > + populate_entries \tbl, \rtbl, \istart, \iend, #PMD_TYPE_TABLE, #PAGE_SIZE, \tmp
> > + mov \tbl, \sv
> > + mov \sv, \rtbl
> > +
> > +#if SWAPPER_PGTABLE_LEVELS > 3
> > + compute_indices \vstart, \vend, #PUD_SHIFT, #PTRS_PER_PUD, \istart, \iend, \count
> > + populate_entries \tbl, \rtbl, \istart, \iend, #PMD_TYPE_TABLE, #PAGE_SIZE, \tmp
> > + mov \tbl, \sv
> > + mov \sv, \rtbl
> > +#endif
> > +
> > +#if SWAPPER_PGTABLE_LEVELS > 2
> > + compute_indices \vstart, \vend, #SWAPPER_TABLE_SHIFT, #PTRS_PER_PMD, \istart, \iend, \count
> > + populate_entries \tbl, \rtbl, \istart, \iend, #PMD_TYPE_TABLE, #PAGE_SIZE, \tmp
> > + mov \tbl, \sv
> > +#endif
> > +
> > + compute_indices \vstart, \vend, #SWAPPER_BLOCK_SHIFT, #PTRS_PER_PTE, \istart, \iend, \count
> > + bic \count, \phys, #SWAPPER_BLOCK_SIZE - 1
> > + populate_entries \tbl, \count, \istart, \iend, \flags, #SWAPPER_BLOCK_SIZE, \tmp
> > .endm
> >
> > /*
> > @@ -246,14 +312,16 @@ __create_page_tables:
> > * dirty cache lines being evicted.
> > */
> > adrp x0, idmap_pg_dir
> > - ldr x1, =(IDMAP_DIR_SIZE + SWAPPER_DIR_SIZE + RESERVED_TTBR0_SIZE)
> > + adrp x1, swapper_pg_end
> > + sub x1, x1, x0
> > bl __inval_dcache_area
> >
> > /*
> > * Clear the idmap and swapper page tables.
> > */
> > adrp x0, idmap_pg_dir
> > - ldr x1, =(IDMAP_DIR_SIZE + SWAPPER_DIR_SIZE + RESERVED_TTBR0_SIZE)
> > + adrp x1, swapper_pg_end
> > + sub x1, x1, x0
> > 1: stp xzr, xzr, [x0], #16
> > stp xzr, xzr, [x0], #16
> > stp xzr, xzr, [x0], #16
> > @@ -318,10 +386,10 @@ __create_page_tables:
> > #endif
> > 1:
> > ldr_l x4, idmap_ptrs_per_pgd
> > - create_pgd_entry x0, x3, x4, x5, x6
> > mov x5, x3 // __pa(__idmap_text_start)
> > adr_l x6, __idmap_text_end // __pa(__idmap_text_end)
> > - create_block_map x0, x7, x3, x5, x6, x4
> > +
> > + map_memory x0, x1, x3, x6, x7, x3, x4, x10, x11, x12, x13, x14
> >
> > /*
> > * Map the kernel image (starting with PHYS_OFFSET).
> > @@ -330,12 +398,12 @@ __create_page_tables:
> > mov_q x5, KIMAGE_VADDR + TEXT_OFFSET // compile time __va(_text)
> > add x5, x5, x23 // add KASLR displacement
> > mov x4, PTRS_PER_PGD
> > - create_pgd_entry x0, x5, x4, x3, x6
> > adrp x6, _end // runtime __pa(_end)
> > adrp x3, _text // runtime __pa(_text)
> > sub x6, x6, x3 // _end - _text
> > add x6, x6, x5 // runtime __va(_end)
> > - create_block_map x0, x7, x3, x5, x6, x4
> > +
> > + map_memory x0, x1, x5, x6, x7, x3, x4, x10, x11, x12, x13, x14
> >
> > /*
> > * Since the page tables have been populated with non-cacheable
> > @@ -343,7 +411,8 @@ __create_page_tables:
> > * tables again to remove any speculatively loaded cache lines.
> > */
> > adrp x0, idmap_pg_dir
> > - ldr x1, =(IDMAP_DIR_SIZE + SWAPPER_DIR_SIZE + RESERVED_TTBR0_SIZE)
> > + adrp x1, swapper_pg_end
> > + sub x1, x1, x0
> > dmb sy
> > bl __inval_dcache_area
> >
> > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> > index 4c7112a47469..0221aca6493d 100644
> > --- a/arch/arm64/kernel/vmlinux.lds.S
> > +++ b/arch/arm64/kernel/vmlinux.lds.S
> > @@ -230,6 +230,7 @@ SECTIONS
> > #endif
> > swapper_pg_dir = .;
> > . += SWAPPER_DIR_SIZE;
> > + swapper_pg_end = .;
> >
> > __pecoff_data_size = ABSOLUTE(. - __initdata_begin);
> > _end = .;
> > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> > index 4071602031ed..fdac11979bae 100644
> > --- a/arch/arm64/mm/mmu.c
> > +++ b/arch/arm64/mm/mmu.c
> > @@ -644,7 +644,8 @@ void __init paging_init(void)
> > * allocated with it.
> > */
> > memblock_free(__pa_symbol(swapper_pg_dir) + PAGE_SIZE,
> > - SWAPPER_DIR_SIZE - PAGE_SIZE);
> > + __pa_symbol(swapper_pg_end) - __pa_symbol(swapper_pg_dir)
> > + - PAGE_SIZE);
> > }
> >
> > /*
> > --
> > 2.11.0
> >
>
>
> ------8<---------
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index 539e2642ed41..0432dd8d083c 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -235,9 +235,7 @@ ENDPROC(preserve_boot_args)
> */
> .macro compute_indices, vstart, vend, shift, ptrs, istart, iend, count
> lsr \iend, \vend, \shift
> - mov \istart, \ptrs
> - sub \istart, \istart, #1
> - and \iend, \iend, \istart // iend = (vend >> shift) & (ptrs - 1)
> + and \iend, \iend, \ptrs - 1 // iend = (vend >> shift) & (ptrs - 1)
> mov \istart, \ptrs
> sub \count, \count, #1
> mul \istart, \istart, \count
> @@ -245,9 +243,7 @@ ENDPROC(preserve_boot_args)
> // our entries span multiple tables
>
> lsr \istart, \vstart, \shift
> - mov \count, \ptrs
> - sub \count, \count, #1
> - and \istart, \istart, \count
> + and \istart, \istart, \ptrs - 1
>
> sub \count, \iend, \istart
> add \count, \count, #1
> @@ -376,6 +372,7 @@ __create_page_tables:
>
> mov x4, EXTRA_PTRS
> create_table_entry x0, x3, EXTRA_SHIFT, x4, x5, x6
> + .set .Lidmap_ptrs_per_pgd, PTRS_PER_PGD
> #else
> /*
> * If VA_BITS == 48, we don't have to configure an additional
> @@ -383,13 +380,13 @@ __create_page_tables:
> */
> mov x4, #1 << (PHYS_MASK_SHIFT - PGDIR_SHIFT)
> str_l x4, idmap_ptrs_per_pgd, x5
> + .set .Lidmap_ptrs_per_pgd, 1 << (PHYS_MASK_SHIFT - PGDIR_SHIFT)
> #endif
> 1:
> - ldr_l x4, idmap_ptrs_per_pgd
> mov x5, x3 // __pa(__idmap_text_start)
> adr_l x6, __idmap_text_end // __pa(__idmap_text_end)
>
> - map_memory x0, x1, x3, x6, x7, x3, x4, x10, x11, x12, x13, x14
> + map_memory x0, x1, x3, x6, x7, x3, .Lidmap_ptrs_per_pgd, x10,
> x11, x12, x13, x14
>
> /*
> * Map the kernel image (starting with PHYS_OFFSET).
> @@ -397,13 +394,12 @@ __create_page_tables:
> adrp x0, swapper_pg_dir
> mov_q x5, KIMAGE_VADDR + TEXT_OFFSET // compile time __va(_text)
> add x5, x5, x23 // add KASLR displacement
> - mov x4, PTRS_PER_PGD
> adrp x6, _end // runtime __pa(_end)
> adrp x3, _text // runtime __pa(_text)
> sub x6, x6, x3 // _end - _text
> add x6, x6, x5 // runtime __va(_end)
>
> - map_memory x0, x1, x5, x6, x7, x3, x4, x10, x11, x12, x13, x14
> + map_memory x0, x1, x5, x6, x7, x3, PTRS_PER_PGD, x10, x11, x12, x13, x14
>
> /*
> * Since the page tables have been populated with non-cacheable
Cheers,
--
Steve
^ permalink raw reply
* [PATCH] arm64: dts: marvell: add Ethernet aliases
From: Gregory CLEMENT @ 2018-01-03 16:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180103151852.15735-1-antoine.tenart@free-electrons.com>
Hi Antoine,
On mer., janv. 03 2018, Antoine Tenart <antoine.tenart@free-electrons.com> wrote:
> From: Yan Markman <ymarkman@marvell.com>
>
> This patch adds Ethernet aliases in the Marvell Armada 7040 DB, 8040 DB
> and 8040 mcbin device trees so that the bootloader setup the MAC
> addresses correctly.
>
> Signed-off-by: Yan Markman <ymarkman@marvell.com>
> [Antoine: commit message, small fixes]
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
Applied on mvebu/dt64
Thanks,
Gregory
> ---
>
> Hi,
>
> This patch is based on the latest mvebu/dt64 tree, which already
> contains Thomas' DT de-duplication patches, so there shouldn't be any
> conflict.
>
> Thanks!
> Antoine
>
> arch/arm64/boot/dts/marvell/armada-7040-db.dts | 6 ++++++
> arch/arm64/boot/dts/marvell/armada-8040-db.dts | 7 +++++++
> arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts | 6 ++++++
> 3 files changed, 19 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-7040-db.dts b/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> index 44c95b97a422..3ae05eee2c9a 100644
> --- a/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> @@ -61,6 +61,12 @@
> reg = <0x0 0x0 0x0 0x80000000>;
> };
>
> + aliases {
> + ethernet0 = &cp0_eth0;
> + ethernet1 = &cp0_eth1;
> + ethernet2 = &cp0_eth2;
> + };
> +
> cp0_reg_usb3_0_vbus: cp0-usb3-0-vbus {
> compatible = "regulator-fixed";
> regulator-name = "usb3h0-vbus";
> diff --git a/arch/arm64/boot/dts/marvell/armada-8040-db.dts b/arch/arm64/boot/dts/marvell/armada-8040-db.dts
> index 13e3209d554a..dba55baff20f 100644
> --- a/arch/arm64/boot/dts/marvell/armada-8040-db.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-8040-db.dts
> @@ -61,6 +61,13 @@
> reg = <0x0 0x0 0x0 0x80000000>;
> };
>
> + aliases {
> + ethernet0 = &cp0_eth0;
> + ethernet1 = &cp0_eth2;
> + ethernet2 = &cp1_eth0;
> + ethernet3 = &cp1_eth1;
> + };
> +
> cp0_reg_usb3_0_vbus: cp0-usb3-0-vbus {
> compatible = "regulator-fixed";
> regulator-name = "cp0-usb3h0-vbus";
> diff --git a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
> index cf0ebd38a2b9..91c19dd04082 100644
> --- a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dts
> @@ -62,6 +62,12 @@
> reg = <0x0 0x0 0x0 0x80000000>;
> };
>
> + aliases {
> + ethernet0 = &cp0_eth0;
> + ethernet1 = &cp1_eth0;
> + ethernet2 = &cp1_eth1;
> + };
> +
> /* Regulator labels correspond with schematics */
> v_3_3: regulator-3-3v {
> compatible = "regulator-fixed";
> --
> 2.14.3
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply
* [PATCH v4 10/21] arm64: entry.S: move SError handling into a C function for future expansion
From: James Morse @ 2018-01-03 16:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <b99e7738-f8dd-26bf-a050-f970c2f8d03b@codeaurora.org>
Hi Adam,
On 02/01/18 21:07, Adam Wallis wrote:
> On 10/19/2017 10:57 AM, James Morse wrote:
> [..]
>> kernel_ventry el1_fiq_invalid // FIQ EL1h
>> - kernel_ventry el1_error_invalid // Error EL1h
>> + kernel_ventry el1_error // Error EL1h
>> kernel_ventry el0_sync // Synchronous 64-bit EL0
>> kernel_ventry el0_irq // IRQ 64-bit EL0
>> kernel_ventry el0_fiq_invalid // FIQ 64-bit EL0
>> - kernel_ventry el0_error_invalid // Error 64-bit EL0
>> + kernel_ventry el0_error // Error 64-bit EL0
>>
>> #ifdef CONFIG_COMPAT
>> kernel_ventry el0_sync_compat // Synchronous 32-bit EL0
>> kernel_ventry el0_irq_compat // IRQ 32-bit EL0
>> kernel_ventry el0_fiq_invalid_compat // FIQ 32-bit EL0
>> - kernel_ventry el0_error_invalid_compat // Error 32-bit EL0
>> + kernel_ventry el0_error_compat // Error 32-bit EL0
>> #else
>> kernel_ventry el0_sync_invalid // Synchronous 32-bit EL0
>> kernel_ventry el0_irq_invalid // IRQ 32-bit EL0
>> @@ -455,10 +455,6 @@ ENDPROC(el0_error_invalid)
>> el0_fiq_invalid_compat:
>> inv_entry 0, BAD_FIQ, 32
>> ENDPROC(el0_fiq_invalid_compat)
>> -
>> -el0_error_invalid_compat:
>> - inv_entry 0, BAD_ERROR, 32
>> -ENDPROC(el0_error_invalid_compat)
>> #endif
> Perhaps I missed something quite obvious, but is there any reason to not also
> remove el1_error_invalid, since SError handling now jumps to el1_error?
There is still a caller for el1_error_invalid: depending on SPSel we are in
thread or handler mode, which causes exceptions to use a different entry in the
vectors. The kernel always uses handler mode, all the thread mode entries point
at their '_invalid' versions.
If we take an SError from EL1t, SPsel==0 then it uses vectors+0x180. (just cut
off the top of this diff). The el1_error change above is for EL1h, SPsel==1,
which uses vectors+0x380.
Thanks for taking a look!
James
^ permalink raw reply
* [PATCH] arm64: defconfig: enable MESON EFUSE
From: Jerome Brunet @ 2018-01-03 15:54 UTC (permalink / raw)
To: linux-arm-kernel
Enable nvmem meson efuse driver as a module
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
arch/arm64/configs/defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 6356c6da34ea..1acd8fb440de 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -558,6 +558,7 @@ CONFIG_PHY_XGENE=y
CONFIG_PHY_TEGRA_XUSB=y
CONFIG_QCOM_L2_PMU=y
CONFIG_QCOM_L3_PMU=y
+CONFIG_MESON_EFUSE=m
CONFIG_TEE=y
CONFIG_OPTEE=y
CONFIG_ARM_SCPI_PROTOCOL=y
--
2.14.3
^ permalink raw reply related
* [PATCH v2] ARM: dts: ls1021a: add nodes for on-chip ram
From: Rasmus Villemoes @ 2018-01-03 15:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1513870698-31264-1-git-send-email-rasmus.villemoes@prevas.dk>
Although the two nodes constitute one contiguous 128K region, still
describe them separately:
- That's how they are described in the reference manual: "Each OCRAM
occupies a 64 KB of address region...", and the names ocram1 and
ocram2 are also as used in the manual.
- The two areas are treated differently by the boot ROM code: OCRAM2 is
zero-initialized, while, again quoting the RM, "software must perform
the zero initialization of OCRAM1."
Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
v2: add changelog explaining the split in two nodes.
arch/arm/boot/dts/ls1021a.dtsi | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/arch/arm/boot/dts/ls1021a.dtsi b/arch/arm/boot/dts/ls1021a.dtsi
index 96aa7752dd0d..7bb5896b3726 100644
--- a/arch/arm/boot/dts/ls1021a.dtsi
+++ b/arch/arm/boot/dts/ls1021a.dtsi
@@ -748,5 +748,21 @@
<0000 0 0 3 &gic GIC_SPI 191 IRQ_TYPE_LEVEL_HIGH>,
<0000 0 0 4 &gic GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>;
};
+
+ ocram1: sram at 10000000 {
+ compatible = "mmio-sram";
+ reg = <0x0 0x10000000 0x0 0x10000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x0 0x10000000 0x10000>;
+ };
+
+ ocram2: sram at 10010000 {
+ compatible = "mmio-sram";
+ reg = <0x0 0x10010000 0x0 0x10000>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x0 0x10010000 0x10000>;
+ };
};
};
--
2.7.4
^ permalink raw reply related
* Vibrations, audio, charging, radio on N9/N950
From: Ivaylo Dimitrov @ 2018-01-03 15:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180103154012.ytasvkzyzo7xy35v@pali>
On 3.01.2018 17:40, Pali Roh?r wrote:
> On Wednesday 03 January 2018 16:34:15 Filip Matijevi? wrote:
>> Hi,
>>
>> On 01/03/2018 02:56 PM, Pavel Machek wrote:
>>> Hi!
>>>
>>> Sebasian, you submitted patch to enable vibrations on N950. I am
>>> trying to do the same now on N9... I guess I enabled the dts, but
>>> .. how do I actually ask for vibrations? /dev/input/eventX does not
>>> seem to be present.
>>>
>>> Did anyone get audio to run on N9/N950? It is marked as supported on
>>> https://elinux.org/N950 with dt glue missing...
>>>
>>
>> I have fairly recent patches (for v4.10) here:
>> https://github.com/filippz/pmbootstrap/commit/600473432d8ff61ae80a7e2a198d9442fd3a6f2e
>> (from 0015 trough 0022 w/o 0016). Vibration was working as was audio
>> playback from twl5031 and tvl320dac33 together with tpa6140a2. More
>> details here: http://talk.maemo.org/showpost.php?p=1492590&postcount=112
>> I hope that you can use those patches at least to some degree.
>>
>>> Even more importantly, does battery charging work for someone? It does
>>> not work here :-(. After USB is plugged in, it maybe tries to charge,
>>> but gives up after 30 seconds.
>>>
>>> Maybe least important, but... does anyone have FM radio working? I'd
>>> like to play some music...
>>
>> I haven't used FM radio nor BT (wl1273), so I can't comment on that, but
>> they are in there but probably need some work.
>
> IIRC there were some problems with FM chip on N9 or N950... something
> like chip was not connected to antenna... But do not remember details.
>
I guess you talk about FMTX here, yes, it is not connected AFAIK. But I
think they meant FMRX in BT module.
^ permalink raw reply
* Applied "ASoC: mediatek: rework clock functions for MT2701" to the asoc tree
From: Mark Brown @ 2018-01-03 15:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <0dc98a0b1d315a8b04472324b5789c640c337cb6.1514881870.git.ryder.lee@mediatek.com>
The patch
ASoC: mediatek: rework clock functions for MT2701
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
>From d8d99d8ed658a705909b07ba21b643c53851d70c Mon Sep 17 00:00:00 2001
From: Ryder Lee <ryder.lee@mediatek.com>
Date: Tue, 2 Jan 2018 19:47:19 +0800
Subject: [PATCH] ASoC: mediatek: rework clock functions for MT2701
Reworks clock part to make it more reasonable. The current changes are:
- Replace regmap operations by CCF APIs. Doing so, we just need to handle
the element clocks and can also get accurate information via CCF.
- Rename clocks to make them more generic so that the future revisions
of the IP can adapt gracefully.
- Regroup 'aud_clks[]' by usage - the basic needs and I2S parts:
The new code just keep the common clocks in array and let SoC self decide
I2S numbers - If future chips have different sets of channels we will
add a little more abstract here.
Moreover, this patch moves I2S clocks to the struct mt2701_i2s_data
so that we can easily manage them when calls .prepare() and .shutdown().
Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
Tested-by: Garlic Tseng <garlic.tseng@mediatek.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
sound/soc/mediatek/mt2701/mt2701-afe-clock-ctrl.c | 518 +++++++---------------
sound/soc/mediatek/mt2701/mt2701-afe-clock-ctrl.h | 15 +-
sound/soc/mediatek/mt2701/mt2701-afe-common.h | 64 +--
sound/soc/mediatek/mt2701/mt2701-afe-pcm.c | 45 +-
4 files changed, 200 insertions(+), 442 deletions(-)
diff --git a/sound/soc/mediatek/mt2701/mt2701-afe-clock-ctrl.c b/sound/soc/mediatek/mt2701/mt2701-afe-clock-ctrl.c
index affa7fb25dd9..75ccdca5811d 100644
--- a/sound/soc/mediatek/mt2701/mt2701-afe-clock-ctrl.c
+++ b/sound/soc/mediatek/mt2701/mt2701-afe-clock-ctrl.c
@@ -21,442 +21,256 @@
#include "mt2701-afe-common.h"
#include "mt2701-afe-clock-ctrl.h"
-static const char *aud_clks[MT2701_CLOCK_NUM] = {
- [MT2701_AUD_INFRA_SYS_AUDIO] = "infra_sys_audio_clk",
- [MT2701_AUD_AUD_MUX1_SEL] = "top_audio_mux1_sel",
- [MT2701_AUD_AUD_MUX2_SEL] = "top_audio_mux2_sel",
- [MT2701_AUD_AUD_MUX1_DIV] = "top_audio_mux1_div",
- [MT2701_AUD_AUD_MUX2_DIV] = "top_audio_mux2_div",
- [MT2701_AUD_AUD_48K_TIMING] = "top_audio_48k_timing",
- [MT2701_AUD_AUD_44K_TIMING] = "top_audio_44k_timing",
- [MT2701_AUD_AUDPLL_MUX_SEL] = "top_audpll_mux_sel",
- [MT2701_AUD_APLL_SEL] = "top_apll_sel",
- [MT2701_AUD_AUD1PLL_98M] = "top_aud1_pll_98M",
- [MT2701_AUD_AUD2PLL_90M] = "top_aud2_pll_90M",
- [MT2701_AUD_HADDS2PLL_98M] = "top_hadds2_pll_98M",
- [MT2701_AUD_HADDS2PLL_294M] = "top_hadds2_pll_294M",
- [MT2701_AUD_AUDPLL] = "top_audpll",
- [MT2701_AUD_AUDPLL_D4] = "top_audpll_d4",
- [MT2701_AUD_AUDPLL_D8] = "top_audpll_d8",
- [MT2701_AUD_AUDPLL_D16] = "top_audpll_d16",
- [MT2701_AUD_AUDPLL_D24] = "top_audpll_d24",
- [MT2701_AUD_AUDINTBUS] = "top_audintbus_sel",
- [MT2701_AUD_CLK_26M] = "clk_26m",
- [MT2701_AUD_SYSPLL1_D4] = "top_syspll1_d4",
- [MT2701_AUD_AUD_K1_SRC_SEL] = "top_aud_k1_src_sel",
- [MT2701_AUD_AUD_K2_SRC_SEL] = "top_aud_k2_src_sel",
- [MT2701_AUD_AUD_K3_SRC_SEL] = "top_aud_k3_src_sel",
- [MT2701_AUD_AUD_K4_SRC_SEL] = "top_aud_k4_src_sel",
- [MT2701_AUD_AUD_K5_SRC_SEL] = "top_aud_k5_src_sel",
- [MT2701_AUD_AUD_K6_SRC_SEL] = "top_aud_k6_src_sel",
- [MT2701_AUD_AUD_K1_SRC_DIV] = "top_aud_k1_src_div",
- [MT2701_AUD_AUD_K2_SRC_DIV] = "top_aud_k2_src_div",
- [MT2701_AUD_AUD_K3_SRC_DIV] = "top_aud_k3_src_div",
- [MT2701_AUD_AUD_K4_SRC_DIV] = "top_aud_k4_src_div",
- [MT2701_AUD_AUD_K5_SRC_DIV] = "top_aud_k5_src_div",
- [MT2701_AUD_AUD_K6_SRC_DIV] = "top_aud_k6_src_div",
- [MT2701_AUD_AUD_I2S1_MCLK] = "top_aud_i2s1_mclk",
- [MT2701_AUD_AUD_I2S2_MCLK] = "top_aud_i2s2_mclk",
- [MT2701_AUD_AUD_I2S3_MCLK] = "top_aud_i2s3_mclk",
- [MT2701_AUD_AUD_I2S4_MCLK] = "top_aud_i2s4_mclk",
- [MT2701_AUD_AUD_I2S5_MCLK] = "top_aud_i2s5_mclk",
- [MT2701_AUD_AUD_I2S6_MCLK] = "top_aud_i2s6_mclk",
- [MT2701_AUD_ASM_M_SEL] = "top_asm_m_sel",
- [MT2701_AUD_ASM_H_SEL] = "top_asm_h_sel",
- [MT2701_AUD_UNIVPLL2_D4] = "top_univpll2_d4",
- [MT2701_AUD_UNIVPLL2_D2] = "top_univpll2_d2",
- [MT2701_AUD_SYSPLL_D5] = "top_syspll_d5",
+static const char *const base_clks[] = {
+ [MT2701_TOP_AUD_MCLK_SRC0] = "top_audio_mux1_sel",
+ [MT2701_TOP_AUD_MCLK_SRC1] = "top_audio_mux2_sel",
+ [MT2701_AUDSYS_AFE] = "audio_afe_pd",
+ [MT2701_AUDSYS_AFE_CONN] = "audio_afe_conn_pd",
+ [MT2701_AUDSYS_A1SYS] = "audio_a1sys_pd",
+ [MT2701_AUDSYS_A2SYS] = "audio_a2sys_pd",
};
int mt2701_init_clock(struct mtk_base_afe *afe)
{
struct mt2701_afe_private *afe_priv = afe->platform_priv;
- int i = 0;
-
- for (i = 0; i < MT2701_CLOCK_NUM; i++) {
- afe_priv->clocks[i] = devm_clk_get(afe->dev, aud_clks[i]);
- if (IS_ERR(afe_priv->clocks[i])) {
- dev_warn(afe->dev, "%s devm_clk_get %s fail\n",
- __func__, aud_clks[i]);
- return PTR_ERR(aud_clks[i]);
+ int i;
+
+ for (i = 0; i < MT2701_BASE_CLK_NUM; i++) {
+ afe_priv->base_ck[i] = devm_clk_get(afe->dev, base_clks[i]);
+ if (IS_ERR(afe_priv->base_ck[i])) {
+ dev_err(afe->dev, "failed to get %s\n", base_clks[i]);
+ return PTR_ERR(afe_priv->base_ck[i]);
}
}
- return 0;
-}
+ /* Get I2S related clocks */
+ for (i = 0; i < MT2701_I2S_NUM; i++) {
+ struct mt2701_i2s_path *i2s_path = &afe_priv->i2s_path[i];
+ char name[13];
-int mt2701_afe_enable_clock(struct mtk_base_afe *afe)
-{
- int ret = 0;
+ snprintf(name, sizeof(name), "i2s%d_src_sel", i);
+ i2s_path->sel_ck = devm_clk_get(afe->dev, name);
+ if (IS_ERR(i2s_path->sel_ck)) {
+ dev_err(afe->dev, "failed to get %s\n", name);
+ return PTR_ERR(i2s_path->sel_ck);
+ }
- ret = mt2701_turn_on_a1sys_clock(afe);
- if (ret) {
- dev_err(afe->dev, "%s turn_on_a1sys_clock fail %d\n",
- __func__, ret);
- return ret;
- }
+ snprintf(name, sizeof(name), "i2s%d_src_div", i);
+ i2s_path->div_ck = devm_clk_get(afe->dev, name);
+ if (IS_ERR(i2s_path->div_ck)) {
+ dev_err(afe->dev, "failed to get %s\n", name);
+ return PTR_ERR(i2s_path->div_ck);
+ }
- ret = mt2701_turn_on_a2sys_clock(afe);
- if (ret) {
- dev_err(afe->dev, "%s turn_on_a2sys_clock fail %d\n",
- __func__, ret);
- mt2701_turn_off_a1sys_clock(afe);
- return ret;
- }
+ snprintf(name, sizeof(name), "i2s%d_mclk_en", i);
+ i2s_path->mclk_ck = devm_clk_get(afe->dev, name);
+ if (IS_ERR(i2s_path->mclk_ck)) {
+ dev_err(afe->dev, "failed to get %s\n", name);
+ return PTR_ERR(i2s_path->mclk_ck);
+ }
- ret = mt2701_turn_on_afe_clock(afe);
- if (ret) {
- dev_err(afe->dev, "%s turn_on_afe_clock fail %d\n",
- __func__, ret);
- mt2701_turn_off_a1sys_clock(afe);
- mt2701_turn_off_a2sys_clock(afe);
- return ret;
+ snprintf(name, sizeof(name), "i2so%d_hop_ck", i);
+ i2s_path->hop_ck[I2S_OUT] = devm_clk_get(afe->dev, name);
+ if (IS_ERR(i2s_path->hop_ck[I2S_OUT])) {
+ dev_err(afe->dev, "failed to get %s\n", name);
+ return PTR_ERR(i2s_path->hop_ck[I2S_OUT]);
+ }
+
+ snprintf(name, sizeof(name), "i2si%d_hop_ck", i);
+ i2s_path->hop_ck[I2S_IN] = devm_clk_get(afe->dev, name);
+ if (IS_ERR(i2s_path->hop_ck[I2S_IN])) {
+ dev_err(afe->dev, "failed to get %s\n", name);
+ return PTR_ERR(i2s_path->hop_ck[I2S_IN]);
+ }
+
+ snprintf(name, sizeof(name), "asrc%d_out_ck", i);
+ i2s_path->asrco_ck = devm_clk_get(afe->dev, name);
+ if (IS_ERR(i2s_path->asrco_ck)) {
+ dev_err(afe->dev, "failed to get %s\n", name);
+ return PTR_ERR(i2s_path->asrco_ck);
+ }
}
- regmap_update_bits(afe->regmap, ASYS_TOP_CON,
- AUDIO_TOP_CON0_A1SYS_A2SYS_ON,
- AUDIO_TOP_CON0_A1SYS_A2SYS_ON);
- regmap_update_bits(afe->regmap, AFE_DAC_CON0,
- AFE_DAC_CON0_AFE_ON,
- AFE_DAC_CON0_AFE_ON);
- regmap_write(afe->regmap, PWR2_TOP_CON,
- PWR2_TOP_CON_INIT_VAL);
- regmap_write(afe->regmap, PWR1_ASM_CON1,
- PWR1_ASM_CON1_INIT_VAL);
- regmap_write(afe->regmap, PWR2_ASM_CON1,
- PWR2_ASM_CON1_INIT_VAL);
+ /* Some platforms may support BT path */
+ afe_priv->mrgif_ck = devm_clk_get(afe->dev, "audio_mrgif_pd");
+ if (IS_ERR(afe_priv->mrgif_ck)) {
+ if (PTR_ERR(afe_priv->mrgif_ck) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
- return 0;
-}
+ afe_priv->mrgif_ck = NULL;
+ }
-void mt2701_afe_disable_clock(struct mtk_base_afe *afe)
-{
- mt2701_turn_off_afe_clock(afe);
- mt2701_turn_off_a1sys_clock(afe);
- mt2701_turn_off_a2sys_clock(afe);
- regmap_update_bits(afe->regmap, ASYS_TOP_CON,
- AUDIO_TOP_CON0_A1SYS_A2SYS_ON, 0);
- regmap_update_bits(afe->regmap, AFE_DAC_CON0,
- AFE_DAC_CON0_AFE_ON, 0);
+ return 0;
}
-int mt2701_turn_on_a1sys_clock(struct mtk_base_afe *afe)
+int mt2701_afe_enable_i2s(struct mtk_base_afe *afe, int id, int dir)
{
struct mt2701_afe_private *afe_priv = afe->platform_priv;
- int ret = 0;
+ struct mt2701_i2s_path *i2s_path = &afe_priv->i2s_path[id];
+ int ret;
- /* Set Mux */
- ret = clk_prepare_enable(afe_priv->clocks[MT2701_AUD_AUD_MUX1_SEL]);
+ ret = clk_prepare_enable(i2s_path->asrco_ck);
if (ret) {
- dev_err(afe->dev, "%s clk_prepare_enable %s fail %d\n",
- __func__, aud_clks[MT2701_AUD_AUD_MUX1_SEL], ret);
- goto A1SYS_CLK_AUD_MUX1_SEL_ERR;
+ dev_err(afe->dev, "failed to enable ASRC clock %d\n", ret);
+ return ret;
}
- ret = clk_set_parent(afe_priv->clocks[MT2701_AUD_AUD_MUX1_SEL],
- afe_priv->clocks[MT2701_AUD_AUD1PLL_98M]);
+ ret = clk_prepare_enable(i2s_path->hop_ck[dir]);
if (ret) {
- dev_err(afe->dev, "%s clk_set_parent %s-%s fail %d\n", __func__,
- aud_clks[MT2701_AUD_AUD_MUX1_SEL],
- aud_clks[MT2701_AUD_AUD1PLL_98M], ret);
- goto A1SYS_CLK_AUD_MUX1_SEL_ERR;
+ dev_err(afe->dev, "failed to enable I2S clock %d\n", ret);
+ goto err_hop_ck;
}
- /* Set Divider */
- ret = clk_prepare_enable(afe_priv->clocks[MT2701_AUD_AUD_MUX1_DIV]);
- if (ret) {
- dev_err(afe->dev, "%s clk_prepare_enable %s fail %d\n",
- __func__,
- aud_clks[MT2701_AUD_AUD_MUX1_DIV],
- ret);
- goto A1SYS_CLK_AUD_MUX1_DIV_ERR;
- }
+ return 0;
- ret = clk_set_rate(afe_priv->clocks[MT2701_AUD_AUD_MUX1_DIV],
- MT2701_AUD_AUD_MUX1_DIV_RATE);
- if (ret) {
- dev_err(afe->dev, "%s clk_set_parent %s-%d fail %d\n", __func__,
- aud_clks[MT2701_AUD_AUD_MUX1_DIV],
- MT2701_AUD_AUD_MUX1_DIV_RATE, ret);
- goto A1SYS_CLK_AUD_MUX1_DIV_ERR;
- }
+err_hop_ck:
+ clk_disable_unprepare(i2s_path->asrco_ck);
- /* Enable clock gate */
- ret = clk_prepare_enable(afe_priv->clocks[MT2701_AUD_AUD_48K_TIMING]);
- if (ret) {
- dev_err(afe->dev, "%s clk_prepare_enable %s fail %d\n",
- __func__, aud_clks[MT2701_AUD_AUD_48K_TIMING], ret);
- goto A1SYS_CLK_AUD_48K_ERR;
- }
+ return ret;
+}
- /* Enable infra audio */
- ret = clk_prepare_enable(afe_priv->clocks[MT2701_AUD_INFRA_SYS_AUDIO]);
- if (ret) {
- dev_err(afe->dev, "%s clk_prepare_enable %s fail %d\n",
- __func__, aud_clks[MT2701_AUD_INFRA_SYS_AUDIO], ret);
- goto A1SYS_CLK_INFRA_ERR;
- }
+void mt2701_afe_disable_i2s(struct mtk_base_afe *afe, int id, int dir)
+{
+ struct mt2701_afe_private *afe_priv = afe->platform_priv;
+ struct mt2701_i2s_path *i2s_path = &afe_priv->i2s_path[id];
- return 0;
+ clk_disable_unprepare(i2s_path->hop_ck[dir]);
+ clk_disable_unprepare(i2s_path->asrco_ck);
+}
-A1SYS_CLK_INFRA_ERR:
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_INFRA_SYS_AUDIO]);
-A1SYS_CLK_AUD_48K_ERR:
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_AUD_48K_TIMING]);
-A1SYS_CLK_AUD_MUX1_DIV_ERR:
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_AUD_MUX1_DIV]);
-A1SYS_CLK_AUD_MUX1_SEL_ERR:
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_AUD_MUX1_SEL]);
+int mt2701_afe_enable_mclk(struct mtk_base_afe *afe, int id)
+{
+ struct mt2701_afe_private *afe_priv = afe->platform_priv;
+ struct mt2701_i2s_path *i2s_path = &afe_priv->i2s_path[id];
- return ret;
+ return clk_prepare_enable(i2s_path->mclk_ck);
}
-void mt2701_turn_off_a1sys_clock(struct mtk_base_afe *afe)
+void mt2701_afe_disable_mclk(struct mtk_base_afe *afe, int id)
{
struct mt2701_afe_private *afe_priv = afe->platform_priv;
+ struct mt2701_i2s_path *i2s_path = &afe_priv->i2s_path[id];
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_INFRA_SYS_AUDIO]);
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_AUD_48K_TIMING]);
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_AUD_MUX1_DIV]);
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_AUD_MUX1_SEL]);
+ clk_disable_unprepare(i2s_path->mclk_ck);
}
-int mt2701_turn_on_a2sys_clock(struct mtk_base_afe *afe)
+int mt2701_enable_btmrg_clk(struct mtk_base_afe *afe)
{
struct mt2701_afe_private *afe_priv = afe->platform_priv;
- int ret = 0;
- /* Set Mux */
- ret = clk_prepare_enable(afe_priv->clocks[MT2701_AUD_AUD_MUX2_SEL]);
- if (ret) {
- dev_err(afe->dev, "%s clk_prepare_enable %s fail %d\n",
- __func__, aud_clks[MT2701_AUD_AUD_MUX2_SEL], ret);
- goto A2SYS_CLK_AUD_MUX2_SEL_ERR;
- }
+ return clk_prepare_enable(afe_priv->mrgif_ck);
+}
- ret = clk_set_parent(afe_priv->clocks[MT2701_AUD_AUD_MUX2_SEL],
- afe_priv->clocks[MT2701_AUD_AUD2PLL_90M]);
- if (ret) {
- dev_err(afe->dev, "%s clk_set_parent %s-%s fail %d\n", __func__,
- aud_clks[MT2701_AUD_AUD_MUX2_SEL],
- aud_clks[MT2701_AUD_AUD2PLL_90M], ret);
- goto A2SYS_CLK_AUD_MUX2_SEL_ERR;
- }
+void mt2701_disable_btmrg_clk(struct mtk_base_afe *afe)
+{
+ struct mt2701_afe_private *afe_priv = afe->platform_priv;
- /* Set Divider */
- ret = clk_prepare_enable(afe_priv->clocks[MT2701_AUD_AUD_MUX2_DIV]);
- if (ret) {
- dev_err(afe->dev, "%s clk_prepare_enable %s fail %d\n",
- __func__, aud_clks[MT2701_AUD_AUD_MUX2_DIV], ret);
- goto A2SYS_CLK_AUD_MUX2_DIV_ERR;
- }
+ clk_disable_unprepare(afe_priv->mrgif_ck);
+}
- ret = clk_set_rate(afe_priv->clocks[MT2701_AUD_AUD_MUX2_DIV],
- MT2701_AUD_AUD_MUX2_DIV_RATE);
- if (ret) {
- dev_err(afe->dev, "%s clk_set_parent %s-%d fail %d\n", __func__,
- aud_clks[MT2701_AUD_AUD_MUX2_DIV],
- MT2701_AUD_AUD_MUX2_DIV_RATE, ret);
- goto A2SYS_CLK_AUD_MUX2_DIV_ERR;
- }
+static int mt2701_afe_enable_audsys(struct mtk_base_afe *afe)
+{
+ struct mt2701_afe_private *afe_priv = afe->platform_priv;
+ int ret;
- /* Enable clock gate */
- ret = clk_prepare_enable(afe_priv->clocks[MT2701_AUD_AUD_44K_TIMING]);
- if (ret) {
- dev_err(afe->dev, "%s clk_prepare_enable %s fail %d\n",
- __func__, aud_clks[MT2701_AUD_AUD_44K_TIMING], ret);
- goto A2SYS_CLK_AUD_44K_ERR;
- }
+ ret = clk_prepare_enable(afe_priv->base_ck[MT2701_AUDSYS_AFE]);
+ if (ret)
+ return ret;
- /* Enable infra audio */
- ret = clk_prepare_enable(afe_priv->clocks[MT2701_AUD_INFRA_SYS_AUDIO]);
- if (ret) {
- dev_err(afe->dev, "%s clk_prepare_enable %s fail %d\n",
- __func__, aud_clks[MT2701_AUD_INFRA_SYS_AUDIO], ret);
- goto A2SYS_CLK_INFRA_ERR;
- }
+ ret = clk_prepare_enable(afe_priv->base_ck[MT2701_AUDSYS_A1SYS]);
+ if (ret)
+ goto err_audio_a1sys;
+
+ ret = clk_prepare_enable(afe_priv->base_ck[MT2701_AUDSYS_A2SYS]);
+ if (ret)
+ goto err_audio_a2sys;
+
+ ret = clk_prepare_enable(afe_priv->base_ck[MT2701_AUDSYS_AFE_CONN]);
+ if (ret)
+ goto err_afe_conn;
return 0;
-A2SYS_CLK_INFRA_ERR:
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_INFRA_SYS_AUDIO]);
-A2SYS_CLK_AUD_44K_ERR:
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_AUD_44K_TIMING]);
-A2SYS_CLK_AUD_MUX2_DIV_ERR:
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_AUD_MUX2_DIV]);
-A2SYS_CLK_AUD_MUX2_SEL_ERR:
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_AUD_MUX2_SEL]);
+err_afe_conn:
+ clk_disable_unprepare(afe_priv->base_ck[MT2701_AUDSYS_A2SYS]);
+err_audio_a2sys:
+ clk_disable_unprepare(afe_priv->base_ck[MT2701_AUDSYS_A1SYS]);
+err_audio_a1sys:
+ clk_disable_unprepare(afe_priv->base_ck[MT2701_AUDSYS_AFE]);
return ret;
}
-void mt2701_turn_off_a2sys_clock(struct mtk_base_afe *afe)
+static void mt2701_afe_disable_audsys(struct mtk_base_afe *afe)
{
struct mt2701_afe_private *afe_priv = afe->platform_priv;
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_INFRA_SYS_AUDIO]);
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_AUD_44K_TIMING]);
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_AUD_MUX2_DIV]);
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_AUD_MUX2_SEL]);
+ clk_disable_unprepare(afe_priv->base_ck[MT2701_AUDSYS_AFE_CONN]);
+ clk_disable_unprepare(afe_priv->base_ck[MT2701_AUDSYS_A2SYS]);
+ clk_disable_unprepare(afe_priv->base_ck[MT2701_AUDSYS_A1SYS]);
+ clk_disable_unprepare(afe_priv->base_ck[MT2701_AUDSYS_AFE]);
}
-int mt2701_turn_on_afe_clock(struct mtk_base_afe *afe)
+int mt2701_afe_enable_clock(struct mtk_base_afe *afe)
{
- struct mt2701_afe_private *afe_priv = afe->platform_priv;
int ret;
- /* enable INFRA_SYS */
- ret = clk_prepare_enable(afe_priv->clocks[MT2701_AUD_INFRA_SYS_AUDIO]);
- if (ret) {
- dev_err(afe->dev, "%s clk_prepare_enable %s fail %d\n",
- __func__, aud_clks[MT2701_AUD_INFRA_SYS_AUDIO], ret);
- goto AFE_AUD_INFRA_ERR;
- }
-
- /* Set MT2701_AUD_AUDINTBUS to MT2701_AUD_SYSPLL1_D4 */
- ret = clk_prepare_enable(afe_priv->clocks[MT2701_AUD_AUDINTBUS]);
- if (ret) {
- dev_err(afe->dev, "%s clk_prepare_enable %s fail %d\n",
- __func__, aud_clks[MT2701_AUD_AUDINTBUS], ret);
- goto AFE_AUD_AUDINTBUS_ERR;
- }
-
- ret = clk_set_parent(afe_priv->clocks[MT2701_AUD_AUDINTBUS],
- afe_priv->clocks[MT2701_AUD_SYSPLL1_D4]);
- if (ret) {
- dev_err(afe->dev, "%s clk_set_parent %s-%s fail %d\n", __func__,
- aud_clks[MT2701_AUD_AUDINTBUS],
- aud_clks[MT2701_AUD_SYSPLL1_D4], ret);
- goto AFE_AUD_AUDINTBUS_ERR;
- }
-
- /* Set MT2701_AUD_ASM_H_SEL to MT2701_AUD_UNIVPLL2_D2 */
- ret = clk_prepare_enable(afe_priv->clocks[MT2701_AUD_ASM_H_SEL]);
- if (ret) {
- dev_err(afe->dev, "%s clk_prepare_enable %s fail %d\n",
- __func__, aud_clks[MT2701_AUD_ASM_H_SEL], ret);
- goto AFE_AUD_ASM_H_ERR;
- }
-
- ret = clk_set_parent(afe_priv->clocks[MT2701_AUD_ASM_H_SEL],
- afe_priv->clocks[MT2701_AUD_UNIVPLL2_D2]);
- if (ret) {
- dev_err(afe->dev, "%s clk_set_parent %s-%s fail %d\n", __func__,
- aud_clks[MT2701_AUD_ASM_H_SEL],
- aud_clks[MT2701_AUD_UNIVPLL2_D2], ret);
- goto AFE_AUD_ASM_H_ERR;
- }
-
- /* Set MT2701_AUD_ASM_M_SEL to MT2701_AUD_UNIVPLL2_D4 */
- ret = clk_prepare_enable(afe_priv->clocks[MT2701_AUD_ASM_M_SEL]);
+ /* Enable audio system */
+ ret = mt2701_afe_enable_audsys(afe);
if (ret) {
- dev_err(afe->dev, "%s clk_prepare_enable %s fail %d\n",
- __func__, aud_clks[MT2701_AUD_ASM_M_SEL], ret);
- goto AFE_AUD_ASM_M_ERR;
+ dev_err(afe->dev, "failed to enable audio system %d\n", ret);
+ return ret;
}
- ret = clk_set_parent(afe_priv->clocks[MT2701_AUD_ASM_M_SEL],
- afe_priv->clocks[MT2701_AUD_UNIVPLL2_D4]);
- if (ret) {
- dev_err(afe->dev, "%s clk_set_parent %s-%s fail %d\n", __func__,
- aud_clks[MT2701_AUD_ASM_M_SEL],
- aud_clks[MT2701_AUD_UNIVPLL2_D4], ret);
- goto AFE_AUD_ASM_M_ERR;
- }
+ regmap_update_bits(afe->regmap, ASYS_TOP_CON,
+ AUDIO_TOP_CON0_A1SYS_A2SYS_ON,
+ AUDIO_TOP_CON0_A1SYS_A2SYS_ON);
+ regmap_update_bits(afe->regmap, AFE_DAC_CON0,
+ AFE_DAC_CON0_AFE_ON,
+ AFE_DAC_CON0_AFE_ON);
- regmap_update_bits(afe->regmap, AUDIO_TOP_CON0,
- AUDIO_TOP_CON0_PDN_AFE, 0);
- regmap_update_bits(afe->regmap, AUDIO_TOP_CON0,
- AUDIO_TOP_CON0_PDN_APLL_CK, 0);
- regmap_update_bits(afe->regmap, AUDIO_TOP_CON4,
- AUDIO_TOP_CON4_PDN_A1SYS, 0);
- regmap_update_bits(afe->regmap, AUDIO_TOP_CON4,
- AUDIO_TOP_CON4_PDN_A2SYS, 0);
- regmap_update_bits(afe->regmap, AUDIO_TOP_CON4,
- AUDIO_TOP_CON4_PDN_AFE_CONN, 0);
+ /* Configure ASRC */
+ regmap_write(afe->regmap, PWR1_ASM_CON1, PWR1_ASM_CON1_INIT_VAL);
+ regmap_write(afe->regmap, PWR2_ASM_CON1, PWR2_ASM_CON1_INIT_VAL);
return 0;
-
-AFE_AUD_ASM_M_ERR:
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_ASM_M_SEL]);
-AFE_AUD_ASM_H_ERR:
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_ASM_H_SEL]);
-AFE_AUD_AUDINTBUS_ERR:
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_AUDINTBUS]);
-AFE_AUD_INFRA_ERR:
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_INFRA_SYS_AUDIO]);
-
- return ret;
}
-void mt2701_turn_off_afe_clock(struct mtk_base_afe *afe)
+int mt2701_afe_disable_clock(struct mtk_base_afe *afe)
{
- struct mt2701_afe_private *afe_priv = afe->platform_priv;
+ regmap_update_bits(afe->regmap, ASYS_TOP_CON,
+ AUDIO_TOP_CON0_A1SYS_A2SYS_ON, 0);
+ regmap_update_bits(afe->regmap, AFE_DAC_CON0,
+ AFE_DAC_CON0_AFE_ON, 0);
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_INFRA_SYS_AUDIO]);
-
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_AUDINTBUS]);
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_ASM_H_SEL]);
- clk_disable_unprepare(afe_priv->clocks[MT2701_AUD_ASM_M_SEL]);
-
- regmap_update_bits(afe->regmap, AUDIO_TOP_CON0,
- AUDIO_TOP_CON0_PDN_AFE, AUDIO_TOP_CON0_PDN_AFE);
- regmap_update_bits(afe->regmap, AUDIO_TOP_CON0,
- AUDIO_TOP_CON0_PDN_APLL_CK,
- AUDIO_TOP_CON0_PDN_APLL_CK);
- regmap_update_bits(afe->regmap, AUDIO_TOP_CON4,
- AUDIO_TOP_CON4_PDN_A1SYS,
- AUDIO_TOP_CON4_PDN_A1SYS);
- regmap_update_bits(afe->regmap, AUDIO_TOP_CON4,
- AUDIO_TOP_CON4_PDN_A2SYS,
- AUDIO_TOP_CON4_PDN_A2SYS);
- regmap_update_bits(afe->regmap, AUDIO_TOP_CON4,
- AUDIO_TOP_CON4_PDN_AFE_CONN,
- AUDIO_TOP_CON4_PDN_AFE_CONN);
+ mt2701_afe_disable_audsys(afe);
+
+ return 0;
}
void mt2701_mclk_configuration(struct mtk_base_afe *afe, int id, int domain,
int mclk)
{
- struct mt2701_afe_private *afe_priv = afe->platform_priv;
+ struct mt2701_afe_private *priv = afe->platform_priv;
+ struct mt2701_i2s_path *i2s_path = &priv->i2s_path[id];
int ret;
- int aud_src_div_id = MT2701_AUD_AUD_K1_SRC_DIV + id;
- int aud_src_clk_id = MT2701_AUD_AUD_K1_SRC_SEL + id;
- /* Set MCLK Kx_SRC_SEL(domain) */
- ret = clk_prepare_enable(afe_priv->clocks[aud_src_clk_id]);
- if (ret)
- dev_err(afe->dev, "%s clk_prepare_enable %s fail %d\n",
- __func__, aud_clks[aud_src_clk_id], ret);
-
- if (domain == 0) {
- ret = clk_set_parent(afe_priv->clocks[aud_src_clk_id],
- afe_priv->clocks[MT2701_AUD_AUD_MUX1_SEL]);
- if (ret)
- dev_err(afe->dev, "%s clk_set_parent %s-%s fail %d\n",
- __func__, aud_clks[aud_src_clk_id],
- aud_clks[MT2701_AUD_AUD_MUX1_SEL], ret);
- } else {
- ret = clk_set_parent(afe_priv->clocks[aud_src_clk_id],
- afe_priv->clocks[MT2701_AUD_AUD_MUX2_SEL]);
- if (ret)
- dev_err(afe->dev, "%s clk_set_parent %s-%s fail %d\n",
- __func__, aud_clks[aud_src_clk_id],
- aud_clks[MT2701_AUD_AUD_MUX2_SEL], ret);
- }
- clk_disable_unprepare(afe_priv->clocks[aud_src_clk_id]);
+ /* Set mclk source */
+ if (domain == 0)
+ ret = clk_set_parent(i2s_path->sel_ck,
+ priv->base_ck[MT2701_TOP_AUD_MCLK_SRC0]);
+ else
+ ret = clk_set_parent(i2s_path->sel_ck,
+ priv->base_ck[MT2701_TOP_AUD_MCLK_SRC1]);
- /* Set MCLK Kx_SRC_DIV(divider) */
- ret = clk_prepare_enable(afe_priv->clocks[aud_src_div_id]);
if (ret)
- dev_err(afe->dev, "%s clk_prepare_enable %s fail %d\n",
- __func__, aud_clks[aud_src_div_id], ret);
+ dev_err(afe->dev, "failed to set domain%d mclk source %d\n",
+ domain, ret);
- ret = clk_set_rate(afe_priv->clocks[aud_src_div_id], mclk);
+ /* Set mclk divider */
+ ret = clk_set_rate(i2s_path->div_ck, mclk);
if (ret)
- dev_err(afe->dev, "%s clk_set_rate %s-%d fail %d\n", __func__,
- aud_clks[aud_src_div_id], mclk, ret);
- clk_disable_unprepare(afe_priv->clocks[aud_src_div_id]);
+ dev_err(afe->dev, "failed to set mclk divider %d\n", ret);
}
MODULE_DESCRIPTION("MT2701 afe clock control");
diff --git a/sound/soc/mediatek/mt2701/mt2701-afe-clock-ctrl.h b/sound/soc/mediatek/mt2701/mt2701-afe-clock-ctrl.h
index 6497d570cf09..15417d9d6597 100644
--- a/sound/soc/mediatek/mt2701/mt2701-afe-clock-ctrl.h
+++ b/sound/soc/mediatek/mt2701/mt2701-afe-clock-ctrl.h
@@ -21,16 +21,15 @@ struct mtk_base_afe;
int mt2701_init_clock(struct mtk_base_afe *afe);
int mt2701_afe_enable_clock(struct mtk_base_afe *afe);
-void mt2701_afe_disable_clock(struct mtk_base_afe *afe);
+int mt2701_afe_disable_clock(struct mtk_base_afe *afe);
-int mt2701_turn_on_a1sys_clock(struct mtk_base_afe *afe);
-void mt2701_turn_off_a1sys_clock(struct mtk_base_afe *afe);
+int mt2701_afe_enable_i2s(struct mtk_base_afe *afe, int id, int dir);
+void mt2701_afe_disable_i2s(struct mtk_base_afe *afe, int id, int dir);
+int mt2701_afe_enable_mclk(struct mtk_base_afe *afe, int id);
+void mt2701_afe_disable_mclk(struct mtk_base_afe *afe, int id);
-int mt2701_turn_on_a2sys_clock(struct mtk_base_afe *afe);
-void mt2701_turn_off_a2sys_clock(struct mtk_base_afe *afe);
-
-int mt2701_turn_on_afe_clock(struct mtk_base_afe *afe);
-void mt2701_turn_off_afe_clock(struct mtk_base_afe *afe);
+int mt2701_enable_btmrg_clk(struct mtk_base_afe *afe);
+void mt2701_disable_btmrg_clk(struct mtk_base_afe *afe);
void mt2701_mclk_configuration(struct mtk_base_afe *afe, int id, int domain,
int mclk);
diff --git a/sound/soc/mediatek/mt2701/mt2701-afe-common.h b/sound/soc/mediatek/mt2701/mt2701-afe-common.h
index c19430e98adf..ce5bd4dc864d 100644
--- a/sound/soc/mediatek/mt2701/mt2701-afe-common.h
+++ b/sound/soc/mediatek/mt2701/mt2701-afe-common.h
@@ -69,53 +69,14 @@ enum {
MT2701_IRQ_ASYS_END,
};
-/* 2701 clock def */
-enum audio_system_clock_type {
- MT2701_AUD_INFRA_SYS_AUDIO,
- MT2701_AUD_AUD_MUX1_SEL,
- MT2701_AUD_AUD_MUX2_SEL,
- MT2701_AUD_AUD_MUX1_DIV,
- MT2701_AUD_AUD_MUX2_DIV,
- MT2701_AUD_AUD_48K_TIMING,
- MT2701_AUD_AUD_44K_TIMING,
- MT2701_AUD_AUDPLL_MUX_SEL,
- MT2701_AUD_APLL_SEL,
- MT2701_AUD_AUD1PLL_98M,
- MT2701_AUD_AUD2PLL_90M,
- MT2701_AUD_HADDS2PLL_98M,
- MT2701_AUD_HADDS2PLL_294M,
- MT2701_AUD_AUDPLL,
- MT2701_AUD_AUDPLL_D4,
- MT2701_AUD_AUDPLL_D8,
- MT2701_AUD_AUDPLL_D16,
- MT2701_AUD_AUDPLL_D24,
- MT2701_AUD_AUDINTBUS,
- MT2701_AUD_CLK_26M,
- MT2701_AUD_SYSPLL1_D4,
- MT2701_AUD_AUD_K1_SRC_SEL,
- MT2701_AUD_AUD_K2_SRC_SEL,
- MT2701_AUD_AUD_K3_SRC_SEL,
- MT2701_AUD_AUD_K4_SRC_SEL,
- MT2701_AUD_AUD_K5_SRC_SEL,
- MT2701_AUD_AUD_K6_SRC_SEL,
- MT2701_AUD_AUD_K1_SRC_DIV,
- MT2701_AUD_AUD_K2_SRC_DIV,
- MT2701_AUD_AUD_K3_SRC_DIV,
- MT2701_AUD_AUD_K4_SRC_DIV,
- MT2701_AUD_AUD_K5_SRC_DIV,
- MT2701_AUD_AUD_K6_SRC_DIV,
- MT2701_AUD_AUD_I2S1_MCLK,
- MT2701_AUD_AUD_I2S2_MCLK,
- MT2701_AUD_AUD_I2S3_MCLK,
- MT2701_AUD_AUD_I2S4_MCLK,
- MT2701_AUD_AUD_I2S5_MCLK,
- MT2701_AUD_AUD_I2S6_MCLK,
- MT2701_AUD_ASM_M_SEL,
- MT2701_AUD_ASM_H_SEL,
- MT2701_AUD_UNIVPLL2_D4,
- MT2701_AUD_UNIVPLL2_D2,
- MT2701_AUD_SYSPLL_D5,
- MT2701_CLOCK_NUM
+enum audio_base_clock {
+ MT2701_TOP_AUD_MCLK_SRC0,
+ MT2701_TOP_AUD_MCLK_SRC1,
+ MT2701_AUDSYS_AFE,
+ MT2701_AUDSYS_AFE_CONN,
+ MT2701_AUDSYS_A1SYS,
+ MT2701_AUDSYS_A2SYS,
+ MT2701_BASE_CLK_NUM,
};
static const unsigned int mt2701_afe_backup_list[] = {
@@ -144,7 +105,6 @@ struct mtk_base_irq_data;
struct mt2701_i2s_data {
int i2s_ctrl_reg;
- int i2s_pwn_shift;
int i2s_asrc_fs_shift;
int i2s_asrc_fs_mask;
};
@@ -161,11 +121,17 @@ struct mt2701_i2s_path {
int on[I2S_DIR_NUM];
int occupied[I2S_DIR_NUM];
const struct mt2701_i2s_data *i2s_data[2];
+ struct clk *hop_ck[I2S_DIR_NUM];
+ struct clk *sel_ck;
+ struct clk *div_ck;
+ struct clk *mclk_ck;
+ struct clk *asrco_ck;
};
struct mt2701_afe_private {
- struct clk *clocks[MT2701_CLOCK_NUM];
struct mt2701_i2s_path i2s_path[MT2701_I2S_NUM];
+ struct clk *base_ck[MT2701_BASE_CLK_NUM];
+ struct clk *mrgif_ck;
bool mrg_enable[MT2701_STREAM_DIR_NUM];
};
diff --git a/sound/soc/mediatek/mt2701/mt2701-afe-pcm.c b/sound/soc/mediatek/mt2701/mt2701-afe-pcm.c
index a7362d1cda1b..33f809228f25 100644
--- a/sound/soc/mediatek/mt2701/mt2701-afe-pcm.c
+++ b/sound/soc/mediatek/mt2701/mt2701-afe-pcm.c
@@ -97,21 +97,12 @@ static int mt2701_afe_i2s_startup(struct snd_pcm_substream *substream,
{
struct snd_soc_pcm_runtime *rtd = substream->private_data;
struct mtk_base_afe *afe = snd_soc_platform_get_drvdata(rtd->platform);
- struct mt2701_afe_private *afe_priv = afe->platform_priv;
int i2s_num = mt2701_dai_num_to_i2s(afe, dai->id);
- int clk_num = MT2701_AUD_AUD_I2S1_MCLK + i2s_num;
- int ret = 0;
if (i2s_num < 0)
return i2s_num;
- /* enable mclk */
- ret = clk_prepare_enable(afe_priv->clocks[clk_num]);
- if (ret)
- dev_err(afe->dev, "Failed to enable mclk for I2S: %d\n",
- i2s_num);
-
- return ret;
+ return mt2701_afe_enable_mclk(afe, i2s_num);
}
static int mt2701_afe_i2s_path_shutdown(struct snd_pcm_substream *substream,
@@ -151,9 +142,9 @@ static int mt2701_afe_i2s_path_shutdown(struct snd_pcm_substream *substream,
/* disable i2s */
regmap_update_bits(afe->regmap, i2s_data->i2s_ctrl_reg,
ASYS_I2S_CON_I2S_EN, 0);
- regmap_update_bits(afe->regmap, AUDIO_TOP_CON4,
- 1 << i2s_data->i2s_pwn_shift,
- 1 << i2s_data->i2s_pwn_shift);
+
+ mt2701_afe_disable_i2s(afe, i2s_num, stream_dir);
+
return 0;
}
@@ -165,7 +156,6 @@ static void mt2701_afe_i2s_shutdown(struct snd_pcm_substream *substream,
struct mt2701_afe_private *afe_priv = afe->platform_priv;
int i2s_num = mt2701_dai_num_to_i2s(afe, dai->id);
struct mt2701_i2s_path *i2s_path;
- int clk_num = MT2701_AUD_AUD_I2S1_MCLK + i2s_num;
if (i2s_num < 0)
return;
@@ -185,7 +175,7 @@ static void mt2701_afe_i2s_shutdown(struct snd_pcm_substream *substream,
I2S_UNSTART:
/* disable mclk */
- clk_disable_unprepare(afe_priv->clocks[clk_num]);
+ mt2701_afe_disable_mclk(afe, i2s_num);
}
static int mt2701_i2s_path_prepare_enable(struct snd_pcm_substream *substream,
@@ -251,9 +241,7 @@ static int mt2701_i2s_path_prepare_enable(struct snd_pcm_substream *substream,
fs << i2s_data->i2s_asrc_fs_shift);
/* enable i2s */
- regmap_update_bits(afe->regmap, AUDIO_TOP_CON4,
- 1 << i2s_data->i2s_pwn_shift,
- 0 << i2s_data->i2s_pwn_shift);
+ mt2701_afe_enable_i2s(afe, i2s_num, stream_dir);
/* reset i2s hw status before enable */
regmap_update_bits(afe->regmap, i2s_data->i2s_ctrl_reg,
@@ -339,9 +327,11 @@ static int mt2701_btmrg_startup(struct snd_pcm_substream *substream,
struct snd_soc_pcm_runtime *rtd = substream->private_data;
struct mtk_base_afe *afe = snd_soc_platform_get_drvdata(rtd->platform);
struct mt2701_afe_private *afe_priv = afe->platform_priv;
+ int ret;
- regmap_update_bits(afe->regmap, AUDIO_TOP_CON4,
- AUDIO_TOP_CON4_PDN_MRGIF, 0);
+ ret = mt2701_enable_btmrg_clk(afe);
+ if (ret)
+ return ret;
afe_priv->mrg_enable[substream->stream] = 1;
return 0;
@@ -406,9 +396,7 @@ static void mt2701_btmrg_shutdown(struct snd_pcm_substream *substream,
AFE_MRGIF_CON_MRG_EN, 0);
regmap_update_bits(afe->regmap, AFE_MRGIF_CON,
AFE_MRGIF_CON_MRG_I2S_EN, 0);
- regmap_update_bits(afe->regmap, AUDIO_TOP_CON4,
- AUDIO_TOP_CON4_PDN_MRGIF,
- AUDIO_TOP_CON4_PDN_MRGIF);
+ mt2701_disable_btmrg_clk(afe);
}
afe_priv->mrg_enable[substream->stream] = 0;
}
@@ -1386,14 +1374,12 @@ static const struct mt2701_i2s_data mt2701_i2s_data[MT2701_I2S_NUM][2] = {
{
{
.i2s_ctrl_reg = ASYS_I2SO1_CON,
- .i2s_pwn_shift = 6,
.i2s_asrc_fs_shift = 0,
.i2s_asrc_fs_mask = 0x1f,
},
{
.i2s_ctrl_reg = ASYS_I2SIN1_CON,
- .i2s_pwn_shift = 0,
.i2s_asrc_fs_shift = 0,
.i2s_asrc_fs_mask = 0x1f,
@@ -1402,14 +1388,12 @@ static const struct mt2701_i2s_data mt2701_i2s_data[MT2701_I2S_NUM][2] = {
{
{
.i2s_ctrl_reg = ASYS_I2SO2_CON,
- .i2s_pwn_shift = 7,
.i2s_asrc_fs_shift = 5,
.i2s_asrc_fs_mask = 0x1f,
},
{
.i2s_ctrl_reg = ASYS_I2SIN2_CON,
- .i2s_pwn_shift = 1,
.i2s_asrc_fs_shift = 5,
.i2s_asrc_fs_mask = 0x1f,
@@ -1418,14 +1402,12 @@ static const struct mt2701_i2s_data mt2701_i2s_data[MT2701_I2S_NUM][2] = {
{
{
.i2s_ctrl_reg = ASYS_I2SO3_CON,
- .i2s_pwn_shift = 8,
.i2s_asrc_fs_shift = 10,
.i2s_asrc_fs_mask = 0x1f,
},
{
.i2s_ctrl_reg = ASYS_I2SIN3_CON,
- .i2s_pwn_shift = 2,
.i2s_asrc_fs_shift = 10,
.i2s_asrc_fs_mask = 0x1f,
@@ -1434,14 +1416,12 @@ static const struct mt2701_i2s_data mt2701_i2s_data[MT2701_I2S_NUM][2] = {
{
{
.i2s_ctrl_reg = ASYS_I2SO4_CON,
- .i2s_pwn_shift = 9,
.i2s_asrc_fs_shift = 15,
.i2s_asrc_fs_mask = 0x1f,
},
{
.i2s_ctrl_reg = ASYS_I2SIN4_CON,
- .i2s_pwn_shift = 3,
.i2s_asrc_fs_shift = 15,
.i2s_asrc_fs_mask = 0x1f,
@@ -1483,8 +1463,7 @@ static int mt2701_afe_runtime_suspend(struct device *dev)
{
struct mtk_base_afe *afe = dev_get_drvdata(dev);
- mt2701_afe_disable_clock(afe);
- return 0;
+ return mt2701_afe_disable_clock(afe);
}
static int mt2701_afe_runtime_resume(struct device *dev)
--
2.15.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox