* [PATCHv2] nokia N9: Add support for magnetometer
From: Pavel Machek @ 2018-01-03 14:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180102172720.ghtez4d3d2fgcmj6@earth.universe>
This adds dts support for magnetometer on Nokia N9.
Signed-off-by: Pavel Machek <pavel@ucw.cz>
diff --git a/arch/arm/boot/dts/omap3-n9.dts b/arch/arm/boot/dts/omap3-n9.dts
index 39e35f8..af321d8 100644
--- a/arch/arm/boot/dts/omap3-n9.dts
+++ b/arch/arm/boot/dts/omap3-n9.dts
@@ -36,6 +57,12 @@
};
};
};
+
+&i2c3 {
+ ak8975 at 0f {
+ compatible = "asahi-kasei,ak8975";
+ reg = <0x0f>;
+ };
};
&isp {
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180103/657d091d/attachment.sig>
^ permalink raw reply related
* [PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables
From: Sudeep Holla @ 2018-01-03 14:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <fdb60100-3973-b9e3-487a-882836f8751e@arm.com>
(Sorry for the delay, just returning from vacation)
On 12/12/17 23:37, Jeremy Linton wrote:
> On 12/12/2017 05:02 PM, Rafael J. Wysocki wrote:
[...]
>>
>> So call this field "token" or similar.? Don't call it "of_node" and
>> don't introduce another "firmware_node" thing in addition to that.
>> That just is a mess, sorry.
I completely agree. Both me and Lorenzo pointed that out in previous
revisions and fair enough you have a valid concern it's use with PPTT.
>
> I sort of agree, I think I can just change the whole of_node to a
> generic 'void *firmware_unique' which works fine for the PPTT code, it
> should also work for the DT code in cache_leaves_are_shared().
>
Should be fine.
> The slight gocha is there is a bit of DT code which initially runs
> earlier that uses of_node as an indirect parameter to a couple functions
> (by just passing the cacheinfo). Let me see if I can tweak that a bit.
>
May be use a simple inline wrapper functions to convert, might help if
we diverge too.
> Frankly, If I understood completely all the *priv cases I suspect it
> might be possible to collapse *of_node into that as well. That is as
> long as no one decides to flush out DT on x86, or PPTT on x86.
>
priv is used to save architecture/cache specific details that can't be
generalized. I doubt if this of_node or PPTT pointer/offset falls in
that category.
--
Regards,
Sudeep
^ permalink raw reply
* [PATCH v5 7/9] arm64: Topology, rename cluster_id
From: Sudeep Holla @ 2018-01-03 14:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <965127a6-816b-8e0c-d228-a3d73a8c643a@huawei.com>
On 02/01/18 02:29, Xiongfeng Wang wrote:
> Hi,
>
> On 2017/12/18 20:42, Morten Rasmussen wrote:
>> On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
>>> Hi,
>>>
>>> On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
>>>> [+Morten, Dietmar]
>>>>
>>>> $SUBJECT should be:
>>>>
>>>> arm64: topology: rename cluster_id
>>>
> [cut]
>>>
> I think we still need the information describing which cores are in one
> cluster. Many arm64 chips have the architecture core/cluster/socket. Cores
> in one cluster may share a same L2 cache. That information can be used to
> build the sched_domain. If we put cores in one cluster in one sched_domain,
> the performance will be better.(please see kernel/sched/topology.c:1197,
> cpu_coregroup_mask() uses 'core_sibling' to build a multi-core
> sched_domain).
We get all the cache information from DT/ACPI PPTT(mainly topology) and now
even the geometry. So ideally, the sharing information must come from that.
Any other solution might end up in conflict if DT/PPTT and that mismatch.
> So I think we still need variable to record which cores are in one
> sched_domain for future use.
I tend to say no, at-least not as is.
--
Regards,
Sudeep
^ permalink raw reply
* [PATCH net-next 1/6] phy: add 2.5G SGMII mode to the phy_mode enum
From: Antoine Tenart @ 2018-01-03 14:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <91838ce5-a1a8-c41a-36e8-bef7adaf82fd@gmail.com>
Hi Florian,
On Thu, Dec 28, 2017 at 06:16:51AM -0800, Florian Fainelli wrote:
> On 12/28/2017 02:06 AM, Antoine Tenart wrote:
> > On Thu, Dec 28, 2017 at 08:20:53AM +0100, Andrew Lunn wrote:
> >> On Wed, Dec 27, 2017 at 11:14:41PM +0100, Antoine Tenart wrote:
> >>> This patch adds one more generic PHY mode to the phy_mode enum, to allow
> >>> configuring generic PHYs to the 2.5G SGMII mode by using the set_mode
> >>> callback.
> >>>
> >>> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> >>> ---
> >>> include/linux/phy/phy.h | 1 +
> >>> 1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> >>> index 4f8423a948d5..70459a28f3a1 100644
> >>> --- a/include/linux/phy/phy.h
> >>> +++ b/include/linux/phy/phy.h
> >>> @@ -28,6 +28,7 @@ enum phy_mode {
> >>> PHY_MODE_USB_DEVICE,
> >>> PHY_MODE_USB_OTG,
> >>> PHY_MODE_SGMII,
> >>> + PHY_MODE_SGMII_2_5G,
> >>> PHY_MODE_10GKR,
> >>> PHY_MODE_UFS_HS_A,
> >>> PHY_MODE_UFS_HS_B,
> >>
> >> There was a discussion maybe last month about adding 2.5G SGMII. I
> >> would prefer 2500SGMII. Putting the number first makes it uniform with
> >> the other defines, 1000BASEX, 25000BASEX, 10GKR.
> >
> > Good to know. I wasn't completely sure how to name this mode properly,
> > but I'm fine with PHY_MODE_2500SGMII. I'll update the patches and send a
> > v2 (without the dt part).
>
> And since you are respinning, please make sure you update phy_modes() in
> the same header file as well as
> Documentation/devicetree/bindings/net/ethernet.txt with the newly added
> PHY interface mode.
Actually it's a generic PHY mode I'm adding, not a network PHY mode.
There's no phy_modes() function for generic PHYs (and this 2500BaseX
mode already is supported in the network PHY modes).
Thanks!
Antoine
--
Antoine T?nart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* [PATCH net-next 1/6] phy: add 2.5G SGMII mode to the phy_mode enum
From: Andrew Lunn @ 2018-01-03 15:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180103143541.GE21727@kwain>
> > >>> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> > >>> index 4f8423a948d5..70459a28f3a1 100644
> > >>> --- a/include/linux/phy/phy.h
> > >>> +++ b/include/linux/phy/phy.h
> > >>> @@ -28,6 +28,7 @@ enum phy_mode {
> > >>> PHY_MODE_USB_DEVICE,
> > >>> PHY_MODE_USB_OTG,
> > >>> PHY_MODE_SGMII,
> > >>> + PHY_MODE_SGMII_2_5G,
> > >>> PHY_MODE_10GKR,
> > >>> PHY_MODE_UFS_HS_A,
> > >>> PHY_MODE_UFS_HS_B,
> > >>
> > >> There was a discussion maybe last month about adding 2.5G SGMII. I
> > >> would prefer 2500SGMII. Putting the number first makes it uniform with
> > >> the other defines, 1000BASEX, 25000BASEX, 10GKR.
> > >
> > > Good to know. I wasn't completely sure how to name this mode properly,
> > > but I'm fine with PHY_MODE_2500SGMII. I'll update the patches and send a
> > > v2 (without the dt part).
> >
> > And since you are respinning, please make sure you update phy_modes() in
> > the same header file as well as
> > Documentation/devicetree/bindings/net/ethernet.txt with the newly added
> > PHY interface mode.
>
> Actually it's a generic PHY mode I'm adding, not a network PHY mode.
> There's no phy_modes() function for generic PHYs (and this 2500BaseX
> mode already is supported in the network PHY modes).
Hi Antoine
Don't you need it in both include/linux/phy/phy.h and
include/linux/phy.h?
Andrew
^ permalink raw reply
* [PATCH] arm64: dts: marvell: add Ethernet aliases
From: Antoine Tenart @ 2018-01-03 15:18 UTC (permalink / raw)
To: linux-arm-kernel
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>
---
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
^ permalink raw reply related
* Vibrations, audio, charging, radio on N9/N950
From: Filip Matijević @ 2018-01-03 15:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180103135635.GA17854@amd>
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.
>
> Pavel
>
Best regards,
Filip
^ permalink raw reply
* Vibrations, audio, charging, radio on N9/N950
From: Pali Rohár @ 2018-01-03 15:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <64f11661-794d-826e-89fc-08b4c15ee673@gmail.com>
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.
--
Pali Roh?r
pali.rohar at gmail.com
^ permalink raw reply
* Applied "ASoC: mediatek: cleanup audio driver for MT2701" to the asoc tree
From: Mark Brown @ 2018-01-03 15:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <eadb3c22b52b8631319b1aae53d9282bc3bdf328.1514881870.git.ryder.lee@mediatek.com>
The patch
ASoC: mediatek: cleanup audio driver 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 600b2fd4f0f7ae5ebcb604c39c9a97e573f9d23e Mon Sep 17 00:00:00 2001
From: Ryder Lee <ryder.lee@mediatek.com>
Date: Tue, 2 Jan 2018 19:47:20 +0800
Subject: [PATCH] ASoC: mediatek: cleanup audio driver for MT2701
Cleanup unused code such as 'i2s_num' guard, headers, indentation
and some defines.
Signed-off-by: Ryder Lee <ryder.lee@mediatek.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
sound/soc/mediatek/mt2701/mt2701-afe-clock-ctrl.c | 14 +---
sound/soc/mediatek/mt2701/mt2701-afe-common.h | 20 +----
sound/soc/mediatek/mt2701/mt2701-afe-pcm.c | 94 ++++-------------------
sound/soc/mediatek/mt2701/mt2701-reg.h | 41 +---------
4 files changed, 24 insertions(+), 145 deletions(-)
diff --git a/sound/soc/mediatek/mt2701/mt2701-afe-clock-ctrl.c b/sound/soc/mediatek/mt2701/mt2701-afe-clock-ctrl.c
index 75ccdca5811d..56a057c78c9a 100644
--- a/sound/soc/mediatek/mt2701/mt2701-afe-clock-ctrl.c
+++ b/sound/soc/mediatek/mt2701/mt2701-afe-clock-ctrl.c
@@ -14,10 +14,6 @@
* GNU General Public License for more details.
*/
-#include <sound/soc.h>
-#include <linux/regmap.h>
-#include <linux/pm_runtime.h>
-
#include "mt2701-afe-common.h"
#include "mt2701-afe-clock-ctrl.h"
@@ -223,8 +219,8 @@ int mt2701_afe_enable_clock(struct mtk_base_afe *afe)
}
regmap_update_bits(afe->regmap, ASYS_TOP_CON,
- AUDIO_TOP_CON0_A1SYS_A2SYS_ON,
- AUDIO_TOP_CON0_A1SYS_A2SYS_ON);
+ ASYS_TOP_CON_ASYS_TIMING_ON,
+ ASYS_TOP_CON_ASYS_TIMING_ON);
regmap_update_bits(afe->regmap, AFE_DAC_CON0,
AFE_DAC_CON0_AFE_ON,
AFE_DAC_CON0_AFE_ON);
@@ -239,7 +235,7 @@ int mt2701_afe_enable_clock(struct mtk_base_afe *afe)
int mt2701_afe_disable_clock(struct mtk_base_afe *afe)
{
regmap_update_bits(afe->regmap, ASYS_TOP_CON,
- AUDIO_TOP_CON0_A1SYS_A2SYS_ON, 0);
+ ASYS_TOP_CON_ASYS_TIMING_ON, 0);
regmap_update_bits(afe->regmap, AFE_DAC_CON0,
AFE_DAC_CON0_AFE_ON, 0);
@@ -272,7 +268,3 @@ void mt2701_mclk_configuration(struct mtk_base_afe *afe, int id, int domain,
if (ret)
dev_err(afe->dev, "failed to set mclk divider %d\n", ret);
}
-
-MODULE_DESCRIPTION("MT2701 afe clock control");
-MODULE_AUTHOR("Garlic Tseng <garlic.tseng@mediatek.com>");
-MODULE_LICENSE("GPL v2");
diff --git a/sound/soc/mediatek/mt2701/mt2701-afe-common.h b/sound/soc/mediatek/mt2701/mt2701-afe-common.h
index ce5bd4dc864d..9a2b301a4c21 100644
--- a/sound/soc/mediatek/mt2701/mt2701-afe-common.h
+++ b/sound/soc/mediatek/mt2701/mt2701-afe-common.h
@@ -16,6 +16,7 @@
#ifndef _MT_2701_AFE_COMMON_H_
#define _MT_2701_AFE_COMMON_H_
+
#include <sound/soc.h>
#include <linux/clk.h>
#include <linux/regmap.h>
@@ -25,16 +26,7 @@
#define MT2701_STREAM_DIR_NUM (SNDRV_PCM_STREAM_LAST + 1)
#define MT2701_PLL_DOMAIN_0_RATE 98304000
#define MT2701_PLL_DOMAIN_1_RATE 90316800
-#define MT2701_AUD_AUD_MUX1_DIV_RATE (MT2701_PLL_DOMAIN_0_RATE / 2)
-#define MT2701_AUD_AUD_MUX2_DIV_RATE (MT2701_PLL_DOMAIN_1_RATE / 2)
-
-enum {
- MT2701_I2S_1,
- MT2701_I2S_2,
- MT2701_I2S_3,
- MT2701_I2S_4,
- MT2701_I2S_NUM,
-};
+#define MT2701_I2S_NUM 4
enum {
MT2701_MEMIF_DL1,
@@ -62,8 +54,7 @@ enum {
};
enum {
- MT2701_IRQ_ASYS_START,
- MT2701_IRQ_ASYS_IRQ1 = MT2701_IRQ_ASYS_START,
+ MT2701_IRQ_ASYS_IRQ1,
MT2701_IRQ_ASYS_IRQ2,
MT2701_IRQ_ASYS_IRQ3,
MT2701_IRQ_ASYS_END,
@@ -100,9 +91,6 @@ static const unsigned int mt2701_afe_backup_list[] = {
AFE_MEMIF_PBUF_SIZE,
};
-struct snd_pcm_substream;
-struct mtk_base_irq_data;
-
struct mt2701_i2s_data {
int i2s_ctrl_reg;
int i2s_asrc_fs_shift;
@@ -120,7 +108,7 @@ struct mt2701_i2s_path {
int mclk_rate;
int on[I2S_DIR_NUM];
int occupied[I2S_DIR_NUM];
- const struct mt2701_i2s_data *i2s_data[2];
+ const struct mt2701_i2s_data *i2s_data[I2S_DIR_NUM];
struct clk *hop_ck[I2S_DIR_NUM];
struct clk *sel_ck;
struct clk *div_ck;
diff --git a/sound/soc/mediatek/mt2701/mt2701-afe-pcm.c b/sound/soc/mediatek/mt2701/mt2701-afe-pcm.c
index 33f809228f25..0edadca12a5e 100644
--- a/sound/soc/mediatek/mt2701/mt2701-afe-pcm.c
+++ b/sound/soc/mediatek/mt2701/mt2701-afe-pcm.c
@@ -20,16 +20,12 @@
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/pm_runtime.h>
-#include <sound/soc.h>
#include "mt2701-afe-common.h"
-
#include "mt2701-afe-clock-ctrl.h"
#include "../common/mtk-afe-platform-driver.h"
#include "../common/mtk-afe-fe-dai.h"
-#define AFE_IRQ_STATUS_BITS 0xff
-
static const struct snd_pcm_hardware mt2701_afe_hardware = {
.info = SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_INTERLEAVED
| SNDRV_PCM_INFO_RESUME | SNDRV_PCM_INFO_MMAP_VALID,
@@ -107,21 +103,16 @@ static int mt2701_afe_i2s_startup(struct snd_pcm_substream *substream,
static int mt2701_afe_i2s_path_shutdown(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai,
+ int i2s_num,
int dir_invert)
{
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);
- struct mt2701_i2s_path *i2s_path;
+ struct mt2701_i2s_path *i2s_path = &afe_priv->i2s_path[i2s_num];
const struct mt2701_i2s_data *i2s_data;
int stream_dir = substream->stream;
- if (i2s_num < 0)
- return i2s_num;
-
- i2s_path = &afe_priv->i2s_path[i2s_num];
-
if (dir_invert) {
if (stream_dir == SNDRV_PCM_STREAM_PLAYBACK)
stream_dir = SNDRV_PCM_STREAM_CAPTURE;
@@ -167,11 +158,11 @@ static void mt2701_afe_i2s_shutdown(struct snd_pcm_substream *substream,
else
goto I2S_UNSTART;
- mt2701_afe_i2s_path_shutdown(substream, dai, 0);
+ mt2701_afe_i2s_path_shutdown(substream, dai, i2s_num, 0);
/* need to disable i2s-out path when disable i2s-in */
if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
- mt2701_afe_i2s_path_shutdown(substream, dai, 1);
+ mt2701_afe_i2s_path_shutdown(substream, dai, i2s_num, 1);
I2S_UNSTART:
/* disable mclk */
@@ -180,24 +171,19 @@ static void mt2701_afe_i2s_shutdown(struct snd_pcm_substream *substream,
static int mt2701_i2s_path_prepare_enable(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai,
+ int i2s_num,
int dir_invert)
{
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);
- struct mt2701_i2s_path *i2s_path;
+ struct mt2701_i2s_path *i2s_path = &afe_priv->i2s_path[i2s_num];
const struct mt2701_i2s_data *i2s_data;
struct snd_pcm_runtime * const runtime = substream->runtime;
int reg, fs, w_len = 1; /* now we support bck 64bits only */
int stream_dir = substream->stream;
unsigned int mask = 0, val = 0;
- if (i2s_num < 0)
- return i2s_num;
-
- i2s_path = &afe_priv->i2s_path[i2s_num];
-
if (dir_invert) {
if (stream_dir == SNDRV_PCM_STREAM_PLAYBACK)
stream_dir = SNDRV_PCM_STREAM_CAPTURE;
@@ -288,13 +274,13 @@ static int mt2701_afe_i2s_prepare(struct snd_pcm_substream *substream,
mt2701_mclk_configuration(afe, i2s_num, clk_domain, mclk_rate);
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
- mt2701_i2s_path_prepare_enable(substream, dai, 0);
+ mt2701_i2s_path_prepare_enable(substream, dai, i2s_num, 0);
} else {
/* need to enable i2s-out path when enable i2s-in */
/* prepare for another direction "out" */
- mt2701_i2s_path_prepare_enable(substream, dai, 1);
+ mt2701_i2s_path_prepare_enable(substream, dai, i2s_num, 1);
/* prepare for "in" */
- mt2701_i2s_path_prepare_enable(substream, dai, 0);
+ mt2701_i2s_path_prepare_enable(substream, dai, i2s_num, 0);
}
return 0;
@@ -562,7 +548,6 @@ static const struct snd_soc_dai_ops mt2701_single_memif_dai_ops = {
.hw_free = mtk_afe_fe_hw_free,
.prepare = mtk_afe_fe_prepare,
.trigger = mtk_afe_fe_trigger,
-
};
static const struct snd_soc_dai_ops mt2701_dlm_memif_dai_ops = {
@@ -903,31 +888,6 @@ static const struct snd_kcontrol_new mt2701_afe_multi_ch_out_i2s4[] = {
PWR2_TOP_CON, 19, 1, 0),
};
-static const struct snd_kcontrol_new mt2701_afe_multi_ch_out_asrc0[] = {
- SOC_DAPM_SINGLE_AUTODISABLE("Asrc0 out Switch", AUDIO_TOP_CON4, 14, 1,
- 1),
-};
-
-static const struct snd_kcontrol_new mt2701_afe_multi_ch_out_asrc1[] = {
- SOC_DAPM_SINGLE_AUTODISABLE("Asrc1 out Switch", AUDIO_TOP_CON4, 15, 1,
- 1),
-};
-
-static const struct snd_kcontrol_new mt2701_afe_multi_ch_out_asrc2[] = {
- SOC_DAPM_SINGLE_AUTODISABLE("Asrc2 out Switch", PWR2_TOP_CON, 6, 1,
- 1),
-};
-
-static const struct snd_kcontrol_new mt2701_afe_multi_ch_out_asrc3[] = {
- SOC_DAPM_SINGLE_AUTODISABLE("Asrc3 out Switch", PWR2_TOP_CON, 7, 1,
- 1),
-};
-
-static const struct snd_kcontrol_new mt2701_afe_multi_ch_out_asrc4[] = {
- SOC_DAPM_SINGLE_AUTODISABLE("Asrc4 out Switch", PWR2_TOP_CON, 8, 1,
- 1),
-};
-
static const struct snd_soc_dapm_widget mt2701_afe_pcm_widgets[] = {
/* inter-connections */
SND_SOC_DAPM_MIXER("I00", SND_SOC_NOPM, 0, 0, NULL, 0),
@@ -987,19 +947,6 @@ static const struct snd_soc_dapm_widget mt2701_afe_pcm_widgets[] = {
SND_SOC_DAPM_MIXER("I18I19", SND_SOC_NOPM, 0, 0,
mt2701_afe_multi_ch_out_i2s3,
ARRAY_SIZE(mt2701_afe_multi_ch_out_i2s3)),
-
- SND_SOC_DAPM_MIXER("ASRC_O0", SND_SOC_NOPM, 0, 0,
- mt2701_afe_multi_ch_out_asrc0,
- ARRAY_SIZE(mt2701_afe_multi_ch_out_asrc0)),
- SND_SOC_DAPM_MIXER("ASRC_O1", SND_SOC_NOPM, 0, 0,
- mt2701_afe_multi_ch_out_asrc1,
- ARRAY_SIZE(mt2701_afe_multi_ch_out_asrc1)),
- SND_SOC_DAPM_MIXER("ASRC_O2", SND_SOC_NOPM, 0, 0,
- mt2701_afe_multi_ch_out_asrc2,
- ARRAY_SIZE(mt2701_afe_multi_ch_out_asrc2)),
- SND_SOC_DAPM_MIXER("ASRC_O3", SND_SOC_NOPM, 0, 0,
- mt2701_afe_multi_ch_out_asrc3,
- ARRAY_SIZE(mt2701_afe_multi_ch_out_asrc3)),
};
static const struct snd_soc_dapm_route mt2701_afe_pcm_routes[] = {
@@ -1009,7 +956,6 @@ static const struct snd_soc_dapm_route mt2701_afe_pcm_routes[] = {
{"I2S0 Playback", NULL, "O15"},
{"I2S0 Playback", NULL, "O16"},
-
{"I2S1 Playback", NULL, "O17"},
{"I2S1 Playback", NULL, "O18"},
{"I2S2 Playback", NULL, "O19"},
@@ -1026,7 +972,6 @@ static const struct snd_soc_dapm_route mt2701_afe_pcm_routes[] = {
{"I00", NULL, "I2S0 Capture"},
{"I01", NULL, "I2S0 Capture"},
-
{"I02", NULL, "I2S1 Capture"},
{"I03", NULL, "I2S1 Capture"},
/* I02,03 link to UL2, also need to open I2S0 */
@@ -1034,15 +979,10 @@ static const struct snd_soc_dapm_route mt2701_afe_pcm_routes[] = {
{"I26", NULL, "BT Capture"},
- {"ASRC_O0", "Asrc0 out Switch", "DLM"},
- {"ASRC_O1", "Asrc1 out Switch", "DLM"},
- {"ASRC_O2", "Asrc2 out Switch", "DLM"},
- {"ASRC_O3", "Asrc3 out Switch", "DLM"},
-
- {"I12I13", "Multich I2S0 Out Switch", "ASRC_O0"},
- {"I14I15", "Multich I2S1 Out Switch", "ASRC_O1"},
- {"I16I17", "Multich I2S2 Out Switch", "ASRC_O2"},
- {"I18I19", "Multich I2S3 Out Switch", "ASRC_O3"},
+ {"I12I13", "Multich I2S0 Out Switch", "DLM"},
+ {"I14I15", "Multich I2S1 Out Switch", "DLM"},
+ {"I16I17", "Multich I2S2 Out Switch", "DLM"},
+ {"I18I19", "Multich I2S3 Out Switch", "DLM"},
{ "I12", NULL, "I12I13" },
{ "I13", NULL, "I12I13" },
@@ -1067,7 +1007,6 @@ static const struct snd_soc_dapm_route mt2701_afe_pcm_routes[] = {
{ "O21", "I18 Switch", "I18" },
{ "O22", "I19 Switch", "I19" },
{ "O31", "I35 Switch", "I35" },
-
};
static const struct snd_soc_component_driver mt2701_afe_pcm_dai_component = {
@@ -1484,12 +1423,13 @@ static int mt2701_afe_pcm_dev_probe(struct platform_device *pdev)
afe = devm_kzalloc(&pdev->dev, sizeof(*afe), GFP_KERNEL);
if (!afe)
return -ENOMEM;
+
afe->platform_priv = devm_kzalloc(&pdev->dev, sizeof(*afe_priv),
GFP_KERNEL);
if (!afe->platform_priv)
return -ENOMEM;
- afe_priv = afe->platform_priv;
+ afe_priv = afe->platform_priv;
afe->dev = &pdev->dev;
dev = afe->dev;
@@ -1524,7 +1464,6 @@ static int mt2701_afe_pcm_dev_probe(struct platform_device *pdev)
afe->memif_size = MT2701_MEMIF_NUM;
afe->memif = devm_kcalloc(dev, afe->memif_size, sizeof(*afe->memif),
GFP_KERNEL);
-
if (!afe->memif)
return -ENOMEM;
@@ -1537,7 +1476,6 @@ static int mt2701_afe_pcm_dev_probe(struct platform_device *pdev)
afe->irqs_size = MT2701_IRQ_ASYS_END;
afe->irqs = devm_kcalloc(dev, afe->irqs_size, sizeof(*afe->irqs),
GFP_KERNEL);
-
if (!afe->irqs)
return -ENOMEM;
@@ -1555,7 +1493,6 @@ static int mt2701_afe_pcm_dev_probe(struct platform_device *pdev)
afe->mtk_afe_hardware = &mt2701_afe_hardware;
afe->memif_fs = mt2701_memif_fs;
afe->irq_fs = mt2701_irq_fs;
-
afe->reg_back_up_list = mt2701_afe_backup_list;
afe->reg_back_up_list_num = ARRAY_SIZE(mt2701_afe_backup_list);
afe->runtime_resume = mt2701_afe_runtime_resume;
@@ -1646,4 +1583,3 @@ module_platform_driver(mt2701_afe_pcm_driver);
MODULE_DESCRIPTION("Mediatek ALSA SoC AFE platform driver for 2701");
MODULE_AUTHOR("Garlic Tseng <garlic.tseng@mediatek.com>");
MODULE_LICENSE("GPL v2");
-
diff --git a/sound/soc/mediatek/mt2701/mt2701-reg.h b/sound/soc/mediatek/mt2701/mt2701-reg.h
index bb62b1c55957..f17c76f37b5f 100644
--- a/sound/soc/mediatek/mt2701/mt2701-reg.h
+++ b/sound/soc/mediatek/mt2701/mt2701-reg.h
@@ -17,17 +17,6 @@
#ifndef _MT2701_REG_H_
#define _MT2701_REG_H_
-#include <linux/delay.h>
-#include <linux/module.h>
-#include <linux/of.h>
-#include <linux/of_address.h>
-#include <linux/pm_runtime.h>
-#include <sound/soc.h>
-#include "mt2701-afe-common.h"
-
-/*****************************************************************************
- * R E G I S T E R D E F I N I T I O N
- *****************************************************************************/
#define AUDIO_TOP_CON0 0x0000
#define AUDIO_TOP_CON4 0x0010
#define AUDIO_TOP_CON5 0x0014
@@ -109,18 +98,6 @@
#define AFE_DAI_BASE 0x1370
#define AFE_DAI_CUR 0x137c
-/* AUDIO_TOP_CON0 (0x0000) */
-#define AUDIO_TOP_CON0_A1SYS_A2SYS_ON (0x3 << 0)
-#define AUDIO_TOP_CON0_PDN_AFE (0x1 << 2)
-#define AUDIO_TOP_CON0_PDN_APLL_CK (0x1 << 23)
-
-/* AUDIO_TOP_CON4 (0x0010) */
-#define AUDIO_TOP_CON4_I2SO1_PWN (0x1 << 6)
-#define AUDIO_TOP_CON4_PDN_A1SYS (0x1 << 21)
-#define AUDIO_TOP_CON4_PDN_A2SYS (0x1 << 22)
-#define AUDIO_TOP_CON4_PDN_AFE_CONN (0x1 << 23)
-#define AUDIO_TOP_CON4_PDN_MRGIF (0x1 << 25)
-
/* AFE_DAIBT_CON0 (0x001c) */
#define AFE_DAIBT_CON0_DAIBT_EN (0x1 << 0)
#define AFE_DAIBT_CON0_BT_FUNC_EN (0x1 << 1)
@@ -137,22 +114,8 @@
#define AFE_MRGIF_CON_I2S_MODE_MASK (0xf << 20)
#define AFE_MRGIF_CON_I2S_MODE_32K (0x4 << 20)
-/* ASYS_I2SO1_CON (0x061c) */
-#define ASYS_I2SO1_CON_FS (0x1f << 8)
-#define ASYS_I2SO1_CON_FS_SET(x) ((x) << 8)
-#define ASYS_I2SO1_CON_MULTI_CH (0x1 << 16)
-#define ASYS_I2SO1_CON_SIDEGEN (0x1 << 30)
-#define ASYS_I2SO1_CON_I2S_EN (0x1 << 0)
-/* 0:EIAJ 1:I2S */
-#define ASYS_I2SO1_CON_I2S_MODE (0x1 << 3)
-#define ASYS_I2SO1_CON_WIDE_MODE (0x1 << 1)
-#define ASYS_I2SO1_CON_WIDE_MODE_SET(x) ((x) << 1)
-
-/* PWR2_TOP_CON (0x0634) */
-#define PWR2_TOP_CON_INIT_VAL (0xffe1ffff)
-
-/* ASYS_IRQ_CLR (0x07c0) */
-#define ASYS_IRQ_CLR_ALL (0xffffffff)
+/* ASYS_TOP_CON (0x0600) */
+#define ASYS_TOP_CON_ASYS_TIMING_ON (0x3 << 0)
/* PWR2_ASM_CON1 (0x1070) */
#define PWR2_ASM_CON1_INIT_VAL (0x492492)
--
2.15.1
^ permalink raw reply related
* 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
* 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
* [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
* [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 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: 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 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
* [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
* [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 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 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 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 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 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 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 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
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