* [PATCH v2 14/21] nvmem: bcm-ocotp: Convert to use devm_nvmem_register()
From: Andrey Smirnov @ 2018-01-01 23:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180101232310.30420-1-andrew.smirnov@gmail.com>
Drop all of the code related to .remove hook and make use of
devm_nvmem_register() instead.
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Carlo Caione <carlo@caione.org>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: cphealy at gmail.com
Cc: linux-kernel at vger.kernel.org
Cc: linux-mediatek at lists.infradead.org
Cc: linux-rockchip at lists.infradead.org
Cc: linux-amlogic at lists.infradead.org
Cc: linux-arm-kernel at lists.infradead.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
drivers/nvmem/bcm-ocotp.c | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)
diff --git a/drivers/nvmem/bcm-ocotp.c b/drivers/nvmem/bcm-ocotp.c
index 5e9e324427f9..24c30fa475cc 100644
--- a/drivers/nvmem/bcm-ocotp.c
+++ b/drivers/nvmem/bcm-ocotp.c
@@ -302,27 +302,17 @@ static int bcm_otpc_probe(struct platform_device *pdev)
priv->config = &bcm_otpc_nvmem_config;
- nvmem = nvmem_register(&bcm_otpc_nvmem_config);
+ nvmem = devm_nvmem_register(dev, &bcm_otpc_nvmem_config);
if (IS_ERR(nvmem)) {
dev_err(dev, "error registering nvmem config\n");
return PTR_ERR(nvmem);
}
- platform_set_drvdata(pdev, nvmem);
-
return 0;
}
-static int bcm_otpc_remove(struct platform_device *pdev)
-{
- struct nvmem_device *nvmem = platform_get_drvdata(pdev);
-
- return nvmem_unregister(nvmem);
-}
-
static struct platform_driver bcm_otpc_driver = {
.probe = bcm_otpc_probe,
- .remove = bcm_otpc_remove,
.driver = {
.name = "brcm-otpc",
.of_match_table = bcm_otpc_dt_ids,
--
2.14.3
^ permalink raw reply related
* [PATCH v2 15/21] nvmem: meson-efuse: Do no gate COMPILE_TEST with MESON_SM
From: Andrey Smirnov @ 2018-01-01 23:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180101232310.30420-1-andrew.smirnov@gmail.com>
Being able to build this driver when COMPILE_TEST is selected is still
useful even when MESON_SM is not selected, since selecting this
driver as a module and doing "make modules" will result in successful
build and would detect trivial coding errors. For an example of type
of errors that could be prevented see [1] and [2]
[1] lkml.kernel.org/r/20171227225956.14442-12-andrew.smirnov at gmail.com
[2] https://marc.info/?l=linux-arm-kernel&m=151461599104358&w=2
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Carlo Caione <carlo@caione.org>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: cphealy at gmail.com
Cc: linux-kernel at vger.kernel.org
Cc: linux-mediatek at lists.infradead.org
Cc: linux-rockchip at lists.infradead.org
Cc: linux-amlogic at lists.infradead.org
Cc: linux-arm-kernel at lists.infradead.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
drivers/nvmem/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index ff505af064ba..42e84e1b2a26 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -147,7 +147,7 @@ config NVMEM_VF610_OCOTP
config MESON_EFUSE
tristate "Amlogic Meson GX eFuse Support"
- depends on (ARCH_MESON || COMPILE_TEST) && MESON_SM
+ depends on (ARCH_MESON && MESON_SM) || COMPILE_TEST
help
This is a driver to retrieve specific values from the eFuse found on
the Amlogic Meson GX SoCs.
--
2.14.3
^ permalink raw reply related
* [PATCH v2 16/21] nvmem: snvs_lpgpr: Convert commas to semicolons
From: Andrey Smirnov @ 2018-01-01 23:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180101232310.30420-1-andrew.smirnov@gmail.com>
Looks like commas were accidentally used where semicolons were
supposed to be. Fix that.
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Carlo Caione <carlo@caione.org>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: cphealy at gmail.com
Cc: linux-kernel at vger.kernel.org
Cc: linux-mediatek at lists.infradead.org
Cc: linux-rockchip at lists.infradead.org
Cc: linux-amlogic at lists.infradead.org
Cc: linux-arm-kernel at lists.infradead.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
drivers/nvmem/snvs_lpgpr.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/nvmem/snvs_lpgpr.c b/drivers/nvmem/snvs_lpgpr.c
index 6a2fdd09e74a..90aaf818563b 100644
--- a/drivers/nvmem/snvs_lpgpr.c
+++ b/drivers/nvmem/snvs_lpgpr.c
@@ -110,12 +110,12 @@ static int snvs_lpgpr_probe(struct platform_device *pdev)
cfg->priv = priv;
cfg->name = dev_name(dev);
cfg->dev = dev;
- cfg->stride = 4,
- cfg->word_size = 4,
- cfg->size = 4,
- cfg->owner = THIS_MODULE,
- cfg->reg_read = snvs_lpgpr_read,
- cfg->reg_write = snvs_lpgpr_write,
+ cfg->stride = 4;
+ cfg->word_size = 4;
+ cfg->size = 4;
+ cfg->owner = THIS_MODULE;
+ cfg->reg_read = snvs_lpgpr_read;
+ cfg->reg_write = snvs_lpgpr_write;
nvmem = devm_nvmem_register(dev, cfg);
if (IS_ERR(nvmem))
--
2.14.3
^ permalink raw reply related
* [PATCH v2 17/21] nvmem: rockchip-efuse: Make use of of_device_get_match_data()
From: Andrey Smirnov @ 2018-01-01 23:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180101232310.30420-1-andrew.smirnov@gmail.com>
Simplify code a bit by using of_device_get_match_data() instead of
of_match_device().
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Carlo Caione <carlo@caione.org>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: cphealy at gmail.com
Cc: linux-kernel at vger.kernel.org
Cc: linux-mediatek at lists.infradead.org
Cc: linux-rockchip at lists.infradead.org
Cc: linux-amlogic at lists.infradead.org
Cc: linux-arm-kernel at lists.infradead.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
drivers/nvmem/rockchip-efuse.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/nvmem/rockchip-efuse.c b/drivers/nvmem/rockchip-efuse.c
index d6dc1330f895..979ba0a376a0 100644
--- a/drivers/nvmem/rockchip-efuse.c
+++ b/drivers/nvmem/rockchip-efuse.c
@@ -193,11 +193,11 @@ static int rockchip_efuse_probe(struct platform_device *pdev)
struct resource *res;
struct nvmem_device *nvmem;
struct rockchip_efuse_chip *efuse;
- const struct of_device_id *match;
+ const void *data;
struct device *dev = &pdev->dev;
- match = of_match_device(dev->driver->of_match_table, dev);
- if (!match || !match->data) {
+ data = of_device_get_match_data(dev);
+ if (!data) {
dev_err(dev, "failed to get match data\n");
return -EINVAL;
}
@@ -218,7 +218,7 @@ static int rockchip_efuse_probe(struct platform_device *pdev)
efuse->dev = &pdev->dev;
econfig.size = resource_size(res);
- econfig.reg_read = match->data;
+ econfig.reg_read = data;
econfig.priv = efuse;
econfig.dev = efuse->dev;
nvmem = devm_nvmem_register(dev, &econfig);
--
2.14.3
^ permalink raw reply related
* [PATCH v2 18/21] nvmem: vf610-ocotp: Do not use "&pdev->dev" explicitly
From: Andrey Smirnov @ 2018-01-01 23:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180101232310.30420-1-andrew.smirnov@gmail.com>
There already a "dev" variable for that. Use it.
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Carlo Caione <carlo@caione.org>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: cphealy at gmail.com
Cc: linux-kernel at vger.kernel.org
Cc: linux-mediatek at lists.infradead.org
Cc: linux-rockchip at lists.infradead.org
Cc: linux-amlogic at lists.infradead.org
Cc: linux-arm-kernel at lists.infradead.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
drivers/nvmem/vf610-ocotp.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/nvmem/vf610-ocotp.c b/drivers/nvmem/vf610-ocotp.c
index 752a0983e7fb..6e6bf7987d9d 100644
--- a/drivers/nvmem/vf610-ocotp.c
+++ b/drivers/nvmem/vf610-ocotp.c
@@ -223,8 +223,7 @@ static int vf610_ocotp_probe(struct platform_device *pdev)
struct resource *res;
struct vf610_ocotp *ocotp_dev;
- ocotp_dev = devm_kzalloc(&pdev->dev,
- sizeof(struct vf610_ocotp), GFP_KERNEL);
+ ocotp_dev = devm_kzalloc(dev, sizeof(struct vf610_ocotp), GFP_KERNEL);
if (!ocotp_dev)
return -ENOMEM;
--
2.14.3
^ permalink raw reply related
* [PATCH v2 19/21] nvmem: rockchip-efuse: Do not use "&pdev->dev" explicitly
From: Andrey Smirnov @ 2018-01-01 23:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180101232310.30420-1-andrew.smirnov@gmail.com>
There's "dev" variable for this already. Use it.
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Carlo Caione <carlo@caione.org>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: cphealy at gmail.com
Cc: linux-kernel at vger.kernel.org
Cc: linux-mediatek at lists.infradead.org
Cc: linux-rockchip at lists.infradead.org
Cc: linux-amlogic at lists.infradead.org
Cc: linux-arm-kernel at lists.infradead.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
drivers/nvmem/rockchip-efuse.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/nvmem/rockchip-efuse.c b/drivers/nvmem/rockchip-efuse.c
index 979ba0a376a0..3120329aea94 100644
--- a/drivers/nvmem/rockchip-efuse.c
+++ b/drivers/nvmem/rockchip-efuse.c
@@ -202,21 +202,21 @@ static int rockchip_efuse_probe(struct platform_device *pdev)
return -EINVAL;
}
- efuse = devm_kzalloc(&pdev->dev, sizeof(struct rockchip_efuse_chip),
+ efuse = devm_kzalloc(dev, sizeof(struct rockchip_efuse_chip),
GFP_KERNEL);
if (!efuse)
return -ENOMEM;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- efuse->base = devm_ioremap_resource(&pdev->dev, res);
+ efuse->base = devm_ioremap_resource(dev, res);
if (IS_ERR(efuse->base))
return PTR_ERR(efuse->base);
- efuse->clk = devm_clk_get(&pdev->dev, "pclk_efuse");
+ efuse->clk = devm_clk_get(dev, "pclk_efuse");
if (IS_ERR(efuse->clk))
return PTR_ERR(efuse->clk);
- efuse->dev = &pdev->dev;
+ efuse->dev = dev;
econfig.size = resource_size(res);
econfig.reg_read = data;
econfig.priv = efuse;
--
2.14.3
^ permalink raw reply related
* [PATCH v2 20/21] nvmem: imx-iim: Do not use "&pdev->dev" explicitly
From: Andrey Smirnov @ 2018-01-01 23:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180101232310.30420-1-andrew.smirnov@gmail.com>
There's already "dev" variable for that. Use it.
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Carlo Caione <carlo@caione.org>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: cphealy at gmail.com
Cc: linux-kernel at vger.kernel.org
Cc: linux-mediatek at lists.infradead.org
Cc: linux-rockchip at lists.infradead.org
Cc: linux-amlogic at lists.infradead.org
Cc: linux-arm-kernel at lists.infradead.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
drivers/nvmem/imx-iim.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nvmem/imx-iim.c b/drivers/nvmem/imx-iim.c
index 635561a441bd..3022bd96bd7e 100644
--- a/drivers/nvmem/imx-iim.c
+++ b/drivers/nvmem/imx-iim.c
@@ -125,7 +125,7 @@ static int imx_iim_probe(struct platform_device *pdev)
drvdata = of_id->data;
- iim->clk = devm_clk_get(&pdev->dev, NULL);
+ iim->clk = devm_clk_get(dev, NULL);
if (IS_ERR(iim->clk))
return PTR_ERR(iim->clk);
--
2.14.3
^ permalink raw reply related
* [PATCH v2 21/21] nvmem: bcm-ocotp: Do not use "&pdev->dev" explicitly
From: Andrey Smirnov @ 2018-01-01 23:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180101232310.30420-1-andrew.smirnov@gmail.com>
There's "dev" variable for this already. Use it.
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Carlo Caione <carlo@caione.org>
Cc: Kevin Hilman <khilman@baylibre.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: cphealy at gmail.com
Cc: linux-kernel at vger.kernel.org
Cc: linux-mediatek at lists.infradead.org
Cc: linux-rockchip at lists.infradead.org
Cc: linux-amlogic at lists.infradead.org
Cc: linux-arm-kernel at lists.infradead.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
drivers/nvmem/bcm-ocotp.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/nvmem/bcm-ocotp.c b/drivers/nvmem/bcm-ocotp.c
index 24c30fa475cc..4159b3f41d79 100644
--- a/drivers/nvmem/bcm-ocotp.c
+++ b/drivers/nvmem/bcm-ocotp.c
@@ -262,8 +262,7 @@ static int bcm_otpc_probe(struct platform_device *pdev)
else if (of_device_is_compatible(dev->of_node, "brcm,ocotp-v2"))
priv->map = &otp_map_v2;
else {
- dev_err(&pdev->dev,
- "%s otpc config map not defined\n", __func__);
+ dev_err(dev, "%s otpc config map not defined\n", __func__);
return -EINVAL;
}
--
2.14.3
^ permalink raw reply related
* [RESEND PATCH v2 02/15] soc: qcom: add support to APR bus driver
From: Bjorn Andersson @ 2018-01-01 23:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171214173402.19074-3-srinivas.kandagatla@linaro.org>
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 toi APR bus (Asynchronous Packet Router) driver.
> ARP driver is made as a bus driver so that the apr devices can added removed
> more dynamically depending on the state of the services on the dsp.
> APR is used for communication between application processor and QDSP to
> use services on QDSP like Audio and others.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
> drivers/soc/qcom/Kconfig | 8 +
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/apr.c | 395 ++++++++++++++++++++++++++++++++++++++++
> include/linux/mod_devicetable.h | 13 ++
> include/linux/soc/qcom/apr.h | 174 ++++++++++++++++++
> 5 files changed, 591 insertions(+)
> create mode 100644 drivers/soc/qcom/apr.c
> create mode 100644 include/linux/soc/qcom/apr.h
>
> 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)
PS, the glink config you're depending on is RPMSG_QCOM_GLINK_SMEM
> + help
> + Enable APR IPC protocol support between
> + application processor and QDSP6. APR is
> + used by audio driver to configure QDSP6
> + ASM, ADM and AFE modules.
> endmenu
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 40c56f67e94a..9daba7e6d20f 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
> obj-$(CONFIG_QCOM_SMP2P) += smp2p.o
> obj-$(CONFIG_QCOM_SMSM) += smsm.o
> obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
> +obj-$(CONFIG_QCOM_APR) += apr.o
> diff --git a/drivers/soc/qcom/apr.c b/drivers/soc/qcom/apr.c
> new file mode 100644
> index 000000000000..c6f3aa7a375b
> --- /dev/null
> +++ 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
*/
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/spinlock.h>
> +#include <linux/list.h>
> +#include <linux/slab.h>
> +#include <linux/of_device.h>
> +#include <linux/soc/qcom/apr.h>
> +#include <linux/rpmsg.h>
> +#include <linux/of.h>
> +
> +struct apr_data {
> + int (*get_data_src)(struct apr_hdr *hdr);
> + int dest_id;
> +};
> +
> +struct apr {
> + struct rpmsg_endpoint *ch;
> + struct device *dev;
> + spinlock_t svcs_lock;
> + struct list_head svcs;
> + int dest_id;
> + const struct apr_data *data;
> +};
> +
> +/* 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?
> +
> +/**
> + * 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?
> +{
> + struct apr *apr = dev_get_drvdata(adev->dev.parent);
> + struct apr_hdr *hdr;
> + unsigned long flags;
> + int ret;
> +
> + spin_lock_irqsave(&adev->lock, flags);
> +
> + hdr = (struct apr_hdr *)buf;
> + hdr->src_domain = APR_DOMAIN_APPS;
> + hdr->src_svc = adev->svc_id;
> + hdr->dest_domain = adev->domain_id;
> + hdr->dest_svc = adev->svc_id;
> +
> + ret = rpmsg_send(apr->ch, buf, hdr->pkt_size);
> + if (ret) {
> + dev_err(&adev->dev, "Unable to send APR pkt %d\n",
> + hdr->pkt_size);
> + } else {
> + ret = hdr->pkt_size;
> + }
> +
> + spin_unlock_irqrestore(&adev->lock, flags);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(apr_send_pkt);
> +
> +static void apr_dev_release(struct device *dev)
> +{
> + struct apr_device *adev = to_apr_device(dev);
> +
> + kfree(adev);
> +}
> +
> +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.
> + dev_err(apr->dev, "APR: Improper apr pkt received:%p %d\n",
> + buf, len);
> + return -EINVAL;
> + }
> +
> + hdr = buf;
> + ver = APR_HDR_FIELD_VER(hdr->hdr_field);
> + if (ver > APR_PKT_VER + 1)
> + return -EINVAL;
> +
> + hdr_size = APR_HDR_FIELD_SIZE_BYTES(hdr->hdr_field);
> + if (hdr_size < APR_HDR_SIZE) {
> + dev_err(apr->dev, "APR: Wrong hdr size:%d\n", hdr_size);
> + return -EINVAL;
> + }
> +
> + if (hdr->pkt_size < APR_HDR_SIZE) {
> + dev_err(apr->dev, "APR: Wrong paket size\n");
> + return -EINVAL;
> + }
> +
> + msg_type = APR_HDR_FIELD_MT(hdr->hdr_field);
> + if (msg_type >= APR_MSG_TYPE_MAX && msg_type != APR_BASIC_RSP_RESULT) {
> + dev_err(apr->dev, "APR: Wrong message type: %d\n", msg_type);
> + 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?
Also, looking at the send function, isn't this src just c_svc->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().
> + 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.
> +
> + 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?
> + 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.
> +}
> +
> +static int apr_device_probe(struct device *dev)
> +{
> + struct apr_device *adev;
> + struct apr_driver *adrv;
You don't indent things elsewhere.
> + 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()
> +
> + return ret;
> +}
> +
> +static int apr_device_remove(struct device *dev)
> +{
> + struct apr_device *adev = to_apr_device(dev);
> + struct apr_driver *adrv;
> + struct apr *apr = dev_get_drvdata(adev->dev.parent);
> +
> + if (dev->driver) {
> + adrv = to_apr_driver(dev->driver);
> + if (adrv->remove)
> + adrv->remove(adev);
> + spin_lock(&apr->svcs_lock);
> + list_del(&adev->node);
> + spin_unlock(&apr->svcs_lock);
> + }
> +
> + return 0;
> +}
> +
> +struct bus_type aprbus_type = {
> + .name = "aprbus",
> + .match = apr_device_match,
> + .probe = apr_device_probe,
> + .remove = apr_device_remove,
> +};
> +EXPORT_SYMBOL_GPL(aprbus_type);
> +
> +/**
> + * 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.
> + 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)
> + 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().
> +
> + return device_register(&adev->dev);
> +}
> +EXPORT_SYMBOL_GPL(apr_add_device);
> +
> +static int qcom_rpmsg_q6_probe(struct rpmsg_device *rpdev)
> +{
> + struct device *dev = &rpdev->dev;
> + struct apr *apr;
> +
> + apr = devm_kzalloc(dev, sizeof(*apr), GFP_KERNEL);
> + if (!apr)
> + return -ENOMEM;
> +
> + apr->data = of_device_get_match_data(dev);
> + if (!apr->data)
> + return -ENODEV;
> +
> + apr->dest_id = apr->data->dest_id;
> + dev_set_drvdata(dev, apr);
> + apr->ch = rpdev->ept;
> + apr->dev = dev;
> + INIT_LIST_HEAD(&apr->svcs);
> +
> + /* register core service */
> + apr_add_device(dev, &core_svc_device_id);
> +
> + return 0;
> +}
> +
> +static int apr_remove_device(struct device *dev, void *null)
> +{
> + struct apr_device *adev = to_apr_device(dev);
> +
> + device_unregister(&adev->dev);
> +
> + return 0;
> +}
> +
> +static void qcom_rpmsg_q6_remove(struct rpmsg_device *rpdev)
> +{
> + device_for_each_child(&rpdev->dev, NULL, apr_remove_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.
> +}
> +
> +/*
> + * __apr_driver_register() - Client driver registration with aprbus
> + *
> + * @drv:Client driver to be associated with client-device.
> + * @owner: owning module/driver
> + *
> + * This API will register the client driver with the aprbus
> + * It is called from the driver's module-init function.
> + */
> +int __apr_driver_register(struct apr_driver *drv, struct module *owner)
> +{
> + /* ID table is mandatory to match the devices to probe */
> + if (!drv->id_table)
> + return -EINVAL;
> +
> + drv->driver.bus = &aprbus_type;
> + drv->driver.owner = owner;
> +
> + return driver_register(&drv->driver);
> +}
> +EXPORT_SYMBOL_GPL(__apr_driver_register);
> +
> +/*
> + * apr_driver_unregister() - Undo effect of apr_driver_register
> + *
> + * @drv: Client driver to be unregistered
> + */
> +void apr_driver_unregister(struct apr_driver *drv)
> +{
> + driver_unregister(&drv->driver);
> +}
> +EXPORT_SYMBOL_GPL(apr_driver_unregister);
> +
> +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?
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.
> + {}
> +};
> +
> +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.
> + .of_match_table = qcom_rpmsg_q6_of_match,
> + },
Indentation.
> +};
> +
> +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.
> +
> + return ret;
> +}
> +
> +static void __exit apr_exit(void)
> +{
> + bus_unregister(&aprbus_type);
> + unregister_rpmsg_driver(&qcom_rpmsg_q6_driver);
> +}
> +
> +subsys_initcall(apr_init);
> +module_exit(apr_exit);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Qualcomm APR Bus");
> 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.
> +#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.
> + __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
> @@ -0,0 +1,174 @@
> +/* SPDX-License-Identifier: GPL-2.0
> +* Copyright (c) 2011-2016, The Linux Foundation
> +* Copyright (c) 2017, Linaro Limited
> +*/
> +
> +#ifndef __APR_H_
> +#define __APR_H_
> +
> +#include <linux/spinlock.h>
> +#include <linux/device.h>
> +#include <linux/mod_devicetable.h>
> +/* APR Client IDs */
> +#define APR_CLIENT_AUDIO 0x0
> +#define APR_CLIENT_VOICE 0x1
> +#define APR_CLIENT_MAX 0x2
> +
> +#define APR_DL_SMD 0
> +#define APR_DL_MAX 1
Unused.
> +
> +#define APR_DEST_MODEM 0
> +#define APR_DEST_QDSP6 1
> +#define APR_DEST_MAX 2
> +#define APR_MAX_BUF 8192
> +
> +#define APR_HDR_LEN(hdr_len) ((hdr_len)/4)
> +#define APR_PKT_SIZE(hdr_len, payload_len) ((hdr_len) + (payload_len))
> +#define APR_HDR_FIELD(msg_type, hdr_len, ver)\
> + (((msg_type & 0x3) << 8) | ((hdr_len & 0xF) << 4) | (ver & 0xF))
> +
> +#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.
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.
> +
> +/* Version */
> +#define APR_PKT_VER 0x0
> +
> +/* Command and Response Types */
> +#define APR_MSG_TYPE_EVENT 0x0
> +#define APR_MSG_TYPE_CMD_RSP 0x1
> +#define APR_MSG_TYPE_SEQ_CMD 0x2
> +#define APR_MSG_TYPE_NSEQ_CMD 0x3
> +#define APR_MSG_TYPE_MAX 0x04
> +
> +/* APR Basic Response Message */
> +#define APR_BASIC_RSP_RESULT 0x000110E8
> +#define APR_RSP_ACCEPTED 0x000100BE
> +
> +/* Domain IDs */
> +#define APR_DOMAIN_SIM 0x1
> +#define APR_DOMAIN_PC 0x2
> +#define APR_DOMAIN_MODEM 0x3
> +#define APR_DOMAIN_ADSP 0x4
> +#define APR_DOMAIN_APPS 0x5
> +#define APR_DOMAIN_MAX 0x6
> +
> +/* ADSP service IDs */
> +#define APR_SVC_TEST_CLIENT 0x2
> +#define APR_SVC_ADSP_CORE 0x3
> +#define APR_SVC_AFE 0x4
> +#define APR_SVC_VSM 0x5
> +#define APR_SVC_VPM 0x6
> +#define APR_SVC_ASM 0x7
> +#define APR_SVC_ADM 0x8
> +#define APR_SVC_ADSP_MVM 0x09
> +#define APR_SVC_ADSP_CVS 0x0A
> +#define APR_SVC_ADSP_CVP 0x0B
> +#define APR_SVC_USM 0x0C
> +#define APR_SVC_LSM 0x0D
> +#define APR_SVC_VIDC 0x16
> +#define APR_SVC_MAX 0x17
> +
> +/* Modem Service IDs */
> +#define APR_SVC_MVS 0x3
> +#define APR_SVC_MVM 0x4
> +#define APR_SVC_CVS 0x5
> +#define APR_SVC_CVP 0x6
> +#define APR_SVC_SRD 0x7
> +
> +/* APR Port IDs */
> +#define APR_MAX_PORTS 0x80
> +#define APR_NAME_MAX 0x40
> +#define RESET_EVENTS 0x000130D7
> +
> +/* hdr field Ver [0:3], Size [4:7], Message type [8:10] */
> +#define APR_HDR_FIELD_VER(h) (h & 0x000F)
> +#define APR_HDR_FIELD_SIZE(h) ((h & 0x00F0) >> 4)
> +#define APR_HDR_FIELD_SIZE_BYTES(h) (((h & 0x00F0) >> 4) * 4)
> +#define APR_HDR_FIELD_MT(h) ((h & 0x0300) >> 8)
> +
> +struct apr_hdr {
> + uint16_t hdr_field;
> + uint16_t pkt_size;
> + uint8_t src_svc;
> + uint8_t src_domain;
> + uint16_t src_port;
> + uint8_t dest_svc;
> + uint8_t dest_domain;
> + uint16_t dest_port;
> + uint32_t token;
> + uint32_t opcode;
> +};
> +
> +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.
> + uint16_t payload_size;
> + uint16_t hdr_len;
> + uint16_t msg_type;
> + uint16_t src;
> + uint16_t dest_svc;
> + uint16_t src_port;
> + uint16_t dest_port;
> + uint32_t token;
> + uint32_t opcode;
> + void *payload;
> +};
> +
Regards,
Bjorn
^ permalink raw reply
* [RESEND PATCH v2 03/15] ASoC: qcom: qdsp6: Add common qdsp6 helper functions
From: Bjorn Andersson @ 2018-01-02 0:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171214173402.19074-4-srinivas.kandagatla@linaro.org>
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]
> +
> + if (ch == 1) {
This is a switch statement.
> + 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...
> + { -ENOMEM, ADSP_ENOMEMORY_STR},
> + { -ENODEV, ADSP_ENOTEXIST_STR},
> + { -EADV, ADSP_ERR_MAX_STR},
"Advertise error"?
> +};
> +
> +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()?
> +{
> + 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)?
> +{
> + 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.
> +}
I'm puzzled to why these helper functions lives in a header file, at
least some aspects of this would better be hidden...
Regards,
Bjorn
^ permalink raw reply
* [RESEND PATCH v2 04/15] ASoC: qcom: qdsp6: Add support to Q6AFE
From: Bjorn Andersson @ 2018-01-02 0:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171214173402.19074-5-srinivas.kandagatla@linaro.org>
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?
> +
> +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.
> + 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.
> + 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.
> + 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.
> +
> +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).
> + 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.
> +
> + 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.
> +
> + 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.
> + 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).
> +{
> + 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.
> + 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.
> + 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.
> + 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.
> +
> + 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().
> +{
> + 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.
> +
> + 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?
> + 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
* [RFC] does ioremap() cause memory leak?
From: Hanjun Guo @ 2018-01-02 1:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5A3DEA6A.9080709@huawei.com>
On 2017/12/23 13:32, Xishi Qiu wrote:
> On 2017/12/21 16:55, Xishi Qiu wrote:
>
>> When we use iounmap() to free the mapping, it calls unmap_vmap_area() to clear page table,
>> but do not free the memory of page table, right?
>>
>> So when use ioremap() to mapping another area(incluce the area before), it may use
>> large mapping(e.g. ioremap_pmd_enabled()), so the original page table memory(e.g. pte memory)
>> will be lost, it cause memory leak, right?
>
>
>
> So I have two questions for this scene.
>
> 1. When the same virtual address allocated from ioremap, first is 4K size, second is 2M size, if Kernel would leak memory.
>
> 2. Kernel modifies the old invalid 4K pagetable to 2M, but doesn`t follow the ARM break-before-make flow, CPU maybe get the old invalid 4K pagetable information, then Kernel would panic.
I sent a RFC patch for this one [1].
[1]: https://patchwork.kernel.org/patch/10134581/
Thanks
Hanjun
^ permalink raw reply
* [PATCH 1/3] phy: phy-mtk-tphy: use auto instead of force to bypass utmi signals
From: Chunfeng Yun @ 2018-01-02 1:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3eb63ed0-5e56-d664-4953-b1998d7bd19f@ti.com>
On Thu, 2017-12-28 at 16:44 +0530, Kishon Vijay Abraham I wrote:
>
> On Thursday 07 December 2017 05:23 PM, Chunfeng Yun wrote:
> > When system is running, if usb2 phy is forced to bypass utmi signals,
> > all PLL will be turned off, and it can't detect device connection
> > anymore, so replace force mode with auto mode which can bypass utmi
> > signals automatically if no device attached for normal flow.
> > But keep the force mode to fix RX sensitivity degradation issue.
> >
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
>
> merged this series.
Thanks a lot
>
> Thanks
> Kishon
> > ---
> > drivers/phy/mediatek/phy-mtk-tphy.c | 19 +++++++------------
> > 1 file changed, 7 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/phy/mediatek/phy-mtk-tphy.c b/drivers/phy/mediatek/phy-mtk-tphy.c
> > index fb8aba4..5d9d7f3 100644
> > --- a/drivers/phy/mediatek/phy-mtk-tphy.c
> > +++ b/drivers/phy/mediatek/phy-mtk-tphy.c
> > @@ -440,9 +440,9 @@ static void u2_phy_instance_init(struct mtk_tphy *tphy,
> > u32 index = instance->index;
> > u32 tmp;
> >
> > - /* switch to USB function. (system register, force ip into usb mode) */
> > + /* switch to USB function, and enable usb pll */
> > tmp = readl(com + U3P_U2PHYDTM0);
> > - tmp &= ~P2C_FORCE_UART_EN;
> > + tmp &= ~(P2C_FORCE_UART_EN | P2C_FORCE_SUSPENDM);
> > tmp |= P2C_RG_XCVRSEL_VAL(1) | P2C_RG_DATAIN_VAL(0);
> > writel(tmp, com + U3P_U2PHYDTM0);
> >
> > @@ -502,10 +502,8 @@ static void u2_phy_instance_power_on(struct mtk_tphy *tphy,
> > u32 index = instance->index;
> > u32 tmp;
> >
> > - /* (force_suspendm=0) (let suspendm=1, enable usb 480MHz pll) */
> > tmp = readl(com + U3P_U2PHYDTM0);
> > - tmp &= ~(P2C_FORCE_SUSPENDM | P2C_RG_XCVRSEL);
> > - tmp &= ~(P2C_RG_DATAIN | P2C_DTM0_PART_MASK);
> > + tmp &= ~(P2C_RG_XCVRSEL | P2C_RG_DATAIN | P2C_DTM0_PART_MASK);
> > writel(tmp, com + U3P_U2PHYDTM0);
> >
> > /* OTG Enable */
> > @@ -540,7 +538,6 @@ static void u2_phy_instance_power_off(struct mtk_tphy *tphy,
> >
> > tmp = readl(com + U3P_U2PHYDTM0);
> > tmp &= ~(P2C_RG_XCVRSEL | P2C_RG_DATAIN);
> > - tmp |= P2C_FORCE_SUSPENDM;
> > writel(tmp, com + U3P_U2PHYDTM0);
> >
> > /* OTG Disable */
> > @@ -548,18 +545,16 @@ static void u2_phy_instance_power_off(struct mtk_tphy *tphy,
> > tmp &= ~PA6_RG_U2_OTG_VBUSCMP_EN;
> > writel(tmp, com + U3P_USBPHYACR6);
> >
> > - /* let suspendm=0, set utmi into analog power down */
> > - tmp = readl(com + U3P_U2PHYDTM0);
> > - tmp &= ~P2C_RG_SUSPENDM;
> > - writel(tmp, com + U3P_U2PHYDTM0);
> > - udelay(1);
> > -
> > tmp = readl(com + U3P_U2PHYDTM1);
> > tmp &= ~(P2C_RG_VBUSVALID | P2C_RG_AVALID);
> > tmp |= P2C_RG_SESSEND;
> > writel(tmp, com + U3P_U2PHYDTM1);
> >
> > if (tphy->pdata->avoid_rx_sen_degradation && index) {
> > + tmp = readl(com + U3P_U2PHYDTM0);
> > + tmp &= ~(P2C_RG_SUSPENDM | P2C_FORCE_SUSPENDM);
> > + writel(tmp, com + U3P_U2PHYDTM0);
> > +
> > tmp = readl(com + U3D_U2PHYDCR0);
> > tmp &= ~P2C_RG_SIF_U2PLL_FORCE_ON;
> > writel(tmp, com + U3D_U2PHYDCR0);
> >
^ permalink raw reply
* [RESEND PATCH v2 05/15] ASoC: qcom: qdsp6: Add support to Q6ADM
From: Bjorn Andersson @ 2018-01-02 1:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171214173402.19074-6-srinivas.kandagatla@linaro.org>
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"?
>
> This patch adds basic support to ADM.
Wouldn't s/to/for/ be better?
[..]
> +struct copp {
> + int afe_port;
> + int copp_idx;
> + int id;
> + int cnt;
Please rename this "refcnt" to match other kernel code.
> + int topology;
> + int mode;
> + int stat;
> + int rate;
> + int bit_width;
> + int channels;
> + int app_type;
> + int acdb_id;
> + wait_queue_head_t wait;
> + struct list_head node;
> + struct q6adm *adm;
> +};
> +
> +struct q6adm {
> + struct apr_device *apr;
> + struct device *dev;
> + unsigned long copp_bitmap[AFE_MAX_PORTS];
> + struct list_head copps_list;
> + spinlock_t copps_list_lock;
> + int matrix_map_stat;
> + struct platform_device *routing_dev;
> +
> + wait_queue_head_t matrix_map_wait;
> +};
> +
> +static struct copp *adm_find_copp(struct q6adm *adm, int port_idx, int copp_idx)
> +{
> + struct copp *c;
> +
> + spin_lock(&adm->copps_list_lock);
> + list_for_each_entry(c, &adm->copps_list, node) {
> + if ((port_idx == c->afe_port) && (copp_idx == c->copp_idx)) {
> + spin_unlock(&adm->copps_list_lock);
> + return c;
> + }
> + }
> +
> + spin_unlock(&adm->copps_list_lock);
> + return NULL;
> +
> +}
> +
> +static struct copp *adm_find_matching_copp(struct q6adm *adm,
> + int port_idx, int topology,
> + int mode, int rate,
> + int bit_width, int app_type)
> +{
> + struct copp *c;
> +
> + spin_lock(&adm->copps_list_lock);
> +
> + list_for_each_entry(c, &adm->copps_list, node) {
> + if ((port_idx == c->afe_port) && (topology == c->topology) &&
> + (mode == c->mode) && (rate == c->rate) &&
> + (bit_width == c->bit_width) && (app_type == c->app_type)) {
> + spin_unlock(&adm->copps_list_lock);
> + return c;
> + }
> + }
> + spin_unlock(&adm->copps_list_lock);
> +
> + return NULL;
> +
> +}
> +
> +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.
> + 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?
> + 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()
> + 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.
> + 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?
> +}
> +/**
> + * q6adm_open() - open adm to get hold of free copp
"open adm and grab a free copp"?
> + *
> + * @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.
> + 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.
> + 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.
> +
> + /* Create a COPP if port id are not enabled */
> + if (copp->cnt == 0) {
Doesn't this scheme require some locking? What about concurrent close()?
> +
> + 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.
> +
> + 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.
> + 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.
> + 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.
> + 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?
> +{
> + 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.
> + 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.
> + 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.
Regards,
Bjorn
^ permalink raw reply
* [PATCH] ARM: sunxi_defconfig: Enable SUNXI_CCU
From: Chen-Yu Tsai @ 2018-01-02 1:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180101192208.11738-1-clabbe.montjoie@gmail.com>
On Tue, Jan 2, 2018 at 3:22 AM, Corentin Labbe
<clabbe.montjoie@gmail.com> wrote:
> While looking for missing symbol by diffing sunxi_defconfig and my
> .config, I just found that no CLK symbol are present in sunxi_defconfig.
This does not take into account default values in Kconfig. Use
`make savedefconfg` then compare the resulting defconfig file.
>
> This patch add the missing CONFIG_SUNXI_CCU in sunxi_defconfig.
> All other symbol will be automatically selected via their default value.
>
> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> ---
> arch/arm/configs/sunxi_defconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/configs/sunxi_defconfig b/arch/arm/configs/sunxi_defconfig
> index 5caaf971fb50..5ed920e75842 100644
> --- a/arch/arm/configs/sunxi_defconfig
> +++ b/arch/arm/configs/sunxi_defconfig
> @@ -16,6 +16,7 @@ CONFIG_CPU_FREQ=y
> CONFIG_CPUFREQ_DT=y
> CONFIG_VFP=y
> CONFIG_NEON=y
> +CONFIG_SUNXI_CCU=y
Not needed. See the last line of:
config SUNXI_CCU
bool "Clock support for Allwinner SoCs"
depends on ARCH_SUNXI || COMPILE_TEST
select RESET_CONTROLLER
default ARCH_SUNXI
Surely kernelci hasn't been broken since the introduction of
sunxi-ng. :)
ChenYu
> CONFIG_NET=y
> CONFIG_PACKET=y
> CONFIG_UNIX=y
> --
> 2.13.6
>
^ permalink raw reply
* [PATCH v5 7/9] arm64: Topology, rename cluster_id
From: Xiongfeng Wang @ 2018-01-02 2:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171218124229.GG507@e105550-lin.cambridge.arm.com>
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 was hoping someone else would comment here, but my take at this point is
>> that it doesn't really matter in a functional sense at the moment.
>> Like the chiplet discussion it can be the subject of a future patch along
>> with the patches which tweak the scheduler to understand the split.
>>
>> BTW, given that i'm OoO next week, and the following that are the holidays,
>> I don't intend to repost this for a couple weeks. I don't think there are
>> any issues with this set.
>>
>>>
>>> There is also arch/arm to take into account, again, this patch is
>>> just renaming (as it should have named since the beginning) a
>>> topology level but we should consider everything from a legacy
>>> perspective.
>
> arch/arm has gone for thread/core/socket for the three topology levels
> it supports.
>
> I'm not sure what short term value keeping cluster_id has? Isn't it just
> about where we make the package = cluster assignment? Currently it is in
> the definition of topology_physical_package_id. If we keep cluster_id
> and add package_id, it gets moved into the MPIDR/DT parsing code.
>
> Keeping cluster_id and introducing a topology_cluster_id function could
> help cleaning up some of the users of topology_physical_package_id that
> currently assumes package_id == cluster_id though.
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)
So I think we still need variable to record which cores are in one sched_domain for future use.
Thanks,
Xiongfeng
>
> Morten
>
> .
>
^ permalink raw reply
* [RESEND PATCH v2 06/15] ASoC: qcom: qdsp6: Add support to Q6ASM
From: Bjorn Andersson @ 2018-01-02 4:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171214173402.19074-7-srinivas.kandagatla@linaro.org>
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.
[..]
> +struct audio_client {
> + int session;
> + app_cb cb;
> + int cmd_state;
> + void *priv;
> + uint32_t io_mode;
> + uint64_t time_stamp;
Unused.
> + 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.
> +{
> + int n = -EINVAL;
You're returning MAX_SESSIONS if no free sessions are found, but are
checking for <= 0 in the caller.
> +
> + mutex_lock(&a->session_lock);
> + for (n = 1; n <= MAX_SESSIONS; n++) {
Is there an external reason for session 0 not being considered?
> + 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).
> + 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"
> + }
> +
> + 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.
> + 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.
> + 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.
> + }
> +
> + 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.
> + uint32_t *payload;
payload is unused.
> +
> + 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
> +
> + 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_get_session_id() - get session id for audio client
> + *
> + * @ac: audio client pointer
> + *
> + * Return: Will be an session id of the audio client.
> + */
> +int q6asm_get_session_id(struct audio_client *c)
> +{
> + return c->session;
> +}
> +EXPORT_SYMBOL_GPL(q6asm_get_session_id);
> +
> +/**
> + * 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)
> + 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.
> + 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.
> + }
> +
> + 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.
> +static int q6asm_probe(struct apr_device *adev)
> +{
> + struct q6asm *q6asm;
> +
> + q6asm = devm_kzalloc(&adev->dev, sizeof(*q6asm), GFP_KERNEL);
> + if (!q6asm)
> + return -ENOMEM;
> +
> + q6asm->dev = &adev->dev;
> + q6asm->adev = adev;
> + q6asm->mem_state = 0;
> + init_waitqueue_head(&q6asm->mem_wait);
> + mutex_init(&q6asm->session_lock);
> + dev_set_drvdata(&adev->dev, q6asm);
> +
> + q6asm->pcmdev = platform_device_register_data(&adev->dev,
> + "q6asm_dai", -1, NULL, 0);
> +
> + return 0;
> +}
> +
> +static int q6asm_remove(struct apr_device *adev)
> +{
> + struct q6asm *q6asm = dev_get_drvdata(&adev->dev);
> +
> + platform_device_unregister(q6asm->pcmdev);
> +
> + return 0;
> +}
> +
> +static const struct apr_device_id q6asm_id[] = {
> + {"Q6ASM", APR_DOMAIN_ADSP, APR_SVC_ASM, APR_CLIENT_AUDIO},
> + {}
> +};
> +
> +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
> +};
> +
> +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.
Regards,
Bjorn
^ permalink raw reply
* [PATCH v6 4/5] clk: aspeed: Register gated clocks
From: Benjamin Herrenschmidt @ 2018-01-02 5:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1514584997.2743.107.camel@kernel.crashing.org>
On Sat, 2017-12-30 at 09:03 +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2017-12-26 at 17:32 -0800, Stephen Boyd wrote:
> > > I noticed we do have a few i2c based clock drivers... how are they ever
> > > supposed to work ? i2c bus controllers are allowed to sleep and the i2c
> > > core takes mutexes...
> >
> > We have clk_prepare()/clk_unprepare() for sleeping suckage. You
> > can use that, and i2c based clk drivers do that today.
>
> "suckage" ? Hehe ... the suckage should rather be stuff that cannot
> sleep. Arbitrary latencies and jitter caused by too much code wanting
> to be "atomic" when unnecessary are a bad thing.
>
> In the case of clocks like the aspeed where we have to wait for a
> rather long stabilization delay, way too long to legitimately do a non-
> sleepable delay with a lock held, do we need to do everything in
> prepare() then ?
BTW. Pls don't hold Joel's patches for this. Without that clk framework
a lot of the aspeed stuff already upstream doesn't actually work
without additional out-of-tree hacks or uboot black magic.
We can sort out the sleeping issues (and possibly move to using prepare
for the clocks that have that delay requirement) via subsequent
improvements.
Cheers,
Ben.
^ permalink raw reply
* [PATCH v7 1/5] clk: Add clock driver for ASPEED BMC SoCs
From: Benjamin Herrenschmidt @ 2018-01-02 5:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171222024522.10362-2-joel@jms.id.au>
On Fri, 2017-12-22 at 13:15 +1030, Joel Stanley wrote:
> This adds the stub of a driver for the ASPEED SoCs. The clocks are
> defined and the static registration is set up.
>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> v7:
> - Rebase on dt-bindings patch
> - Add ASPEED_NUM_CLKS as it no longer lives in the header
> v6:
> - Add SPDX copyright notices
> v5:
> - Add Andrew's reviewed-by
> - Make aspeed_gates not initconst to avoid section mismatch warning
> v3:
> - use named initlisers for aspeed_gates table
> - fix clocks typo
> - Move ASPEED_NUM_CLKS to the bottom of the list
> - Put gates at the start of the list, so we can use them to initalise
> the aspeed_gates table
> - Add ASPEED_CLK_SELECTION_2
> - Set parent of network MAC gates
> ---
> drivers/clk/Kconfig | 12 ++++
> drivers/clk/Makefile | 1 +
> drivers/clk/clk-aspeed.c | 141 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 154 insertions(+)
> create mode 100644 drivers/clk/clk-aspeed.c
>
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 1c4e1aa6767e..9abe063ef8d2 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -142,6 +142,18 @@ config COMMON_CLK_GEMINI
> This driver supports the SoC clocks on the Cortina Systems Gemini
> platform, also known as SL3516 or CS3516.
>
> +config COMMON_CLK_ASPEED
> + bool "Clock driver for Aspeed BMC SoCs"
> + depends on ARCH_ASPEED || COMPILE_TEST
> + default ARCH_ASPEED
> + select MFD_SYSCON
> + select RESET_CONTROLLER
> + ---help---
> + This driver supports the SoC clocks on the Aspeed BMC platforms.
> +
> + The G4 and G5 series, including the ast2400 and ast2500, are supported
> + by this driver.
> +
> config COMMON_CLK_S2MPS11
> tristate "Clock driver for S2MPS1X/S5M8767 MFD"
> depends on MFD_SEC_CORE || COMPILE_TEST
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index f7f761b02bed..d260da4809f8 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_ARCH_CLPS711X) += clk-clps711x.o
> obj-$(CONFIG_COMMON_CLK_CS2000_CP) += clk-cs2000-cp.o
> obj-$(CONFIG_ARCH_EFM32) += clk-efm32gg.o
> obj-$(CONFIG_COMMON_CLK_GEMINI) += clk-gemini.o
> +obj-$(CONFIG_COMMON_CLK_ASPEED) += clk-aspeed.o
> obj-$(CONFIG_ARCH_HIGHBANK) += clk-highbank.o
> obj-$(CONFIG_CLK_HSDK) += clk-hsdk-pll.o
> obj-$(CONFIG_COMMON_CLK_MAX77686) += clk-max77686.o
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> new file mode 100644
> index 000000000000..7a86ee08ea4f
> --- /dev/null
> +++ b/drivers/clk/clk-aspeed.c
> @@ -0,0 +1,141 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#define pr_fmt(fmt) "clk-aspeed: " fmt
> +
> +#include <linux/clk-provider.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_address.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include <dt-bindings/clock/aspeed-clock.h>
> +
> +#define ASPEED_NUM_CLKS 35
> +
> +#define ASPEED_STRAP 0x70
> +
> +/* Keeps track of all clocks */
> +static struct clk_hw_onecell_data *aspeed_clk_data;
> +
> +static void __iomem *scu_base;
> +
> +/**
> + * struct aspeed_gate_data - Aspeed gated clocks
> + * @clock_idx: bit used to gate this clock in the clock register
> + * @reset_idx: bit used to reset this IP in the reset register. -1 if no
> + * reset is required when enabling the clock
> + * @name: the clock name
> + * @parent_name: the name of the parent clock
> + * @flags: standard clock framework flags
> + */
> +struct aspeed_gate_data {
> + u8 clock_idx;
> + s8 reset_idx;
> + const char *name;
> + const char *parent_name;
> + unsigned long flags;
> +};
> +
> +/**
> + * struct aspeed_clk_gate - Aspeed specific clk_gate structure
> + * @hw: handle between common and hardware-specific interfaces
> + * @reg: register controlling gate
> + * @clock_idx: bit used to gate this clock in the clock register
> + * @reset_idx: bit used to reset this IP in the reset register. -1 if no
> + * reset is required when enabling the clock
> + * @flags: hardware-specific flags
> + * @lock: register lock
> + *
> + * Some of the clocks in the Aspeed SoC must be put in reset before enabling.
> + * This modified version of clk_gate allows an optional reset bit to be
> + * specified.
> + */
> +struct aspeed_clk_gate {
> + struct clk_hw hw;
> + struct regmap *map;
> + u8 clock_idx;
> + s8 reset_idx;
> + u8 flags;
> + spinlock_t *lock;
> +};
> +
> +#define to_aspeed_clk_gate(_hw) container_of(_hw, struct aspeed_clk_gate, hw)
> +
> +/* TODO: ask Aspeed about the actual parent data */
> +static const struct aspeed_gate_data aspeed_gates[] = {
> + /* clk rst name parent flags */
> + [ASPEED_CLK_GATE_ECLK] = { 0, -1, "eclk-gate", "eclk", 0 }, /* Video Engine */
> + [ASPEED_CLK_GATE_GCLK] = { 1, 7, "gclk-gate", NULL, 0 }, /* 2D engine */
> + [ASPEED_CLK_GATE_MCLK] = { 2, -1, "mclk-gate", "mpll", CLK_IS_CRITICAL }, /* SDRAM */
> + [ASPEED_CLK_GATE_VCLK] = { 3, 6, "vclk-gate", NULL, 0 }, /* Video Capture */
> + [ASPEED_CLK_GATE_BCLK] = { 4, 10, "bclk-gate", "bclk", 0 }, /* PCIe/PCI */
> + [ASPEED_CLK_GATE_DCLK] = { 5, -1, "dclk-gate", NULL, 0 }, /* DAC */
> + [ASPEED_CLK_GATE_REFCLK] = { 6, -1, "refclk-gate", "clkin", CLK_IS_CRITICAL },
> + [ASPEED_CLK_GATE_USBPORT2CLK] = { 7, 3, "usb-port2-gate", NULL, 0 }, /* USB2.0 Host port 2 */
> + [ASPEED_CLK_GATE_LCLK] = { 8, 5, "lclk-gate", NULL, 0 }, /* LPC */
> + [ASPEED_CLK_GATE_USBUHCICLK] = { 9, 15, "usb-uhci-gate", NULL, 0 }, /* USB1.1 (requires port 2 enabled) */
> + [ASPEED_CLK_GATE_D1CLK] = { 10, 13, "d1clk-gate", NULL, 0 }, /* GFX CRT */
> + [ASPEED_CLK_GATE_YCLK] = { 13, 4, "yclk-gate", NULL, 0 }, /* HAC */
> + [ASPEED_CLK_GATE_USBPORT1CLK] = { 14, 14, "usb-port1-gate", NULL, 0 }, /* USB2 hub/USB2 host port 1/USB1.1 dev */
> + [ASPEED_CLK_GATE_UART1CLK] = { 15, -1, "uart1clk-gate", "uart", 0 }, /* UART1 */
> + [ASPEED_CLK_GATE_UART2CLK] = { 16, -1, "uart2clk-gate", "uart", 0 }, /* UART2 */
> + [ASPEED_CLK_GATE_UART5CLK] = { 17, -1, "uart5clk-gate", "uart", 0 }, /* UART5 */
> + [ASPEED_CLK_GATE_ESPICLK] = { 19, -1, "espiclk-gate", NULL, 0 }, /* eSPI */
> + [ASPEED_CLK_GATE_MAC1CLK] = { 20, 11, "mac1clk-gate", "mac", 0 }, /* MAC1 */
> + [ASPEED_CLK_GATE_MAC2CLK] = { 21, 12, "mac2clk-gate", "mac", 0 }, /* MAC2 */
> + [ASPEED_CLK_GATE_RSACLK] = { 24, -1, "rsaclk-gate", NULL, 0 }, /* RSA */
> + [ASPEED_CLK_GATE_UART3CLK] = { 25, -1, "uart3clk-gate", "uart", 0 }, /* UART3 */
> + [ASPEED_CLK_GATE_UART4CLK] = { 26, -1, "uart4clk-gate", "uart", 0 }, /* UART4 */
> + [ASPEED_CLK_GATE_SDCLKCLK] = { 27, 16, "sdclk-gate", NULL, 0 }, /* SDIO/SD */
> + [ASPEED_CLK_GATE_LHCCLK] = { 28, -1, "lhclk-gate", "lhclk", 0 }, /* LPC master/LPC+ */
> +};
> +
> +static void __init aspeed_cc_init(struct device_node *np)
> +{
> + struct regmap *map;
> + u32 val;
> + int ret;
> + int i;
> +
> + scu_base = of_iomap(np, 0);
> + if (IS_ERR(scu_base))
> + return;
> +
> + aspeed_clk_data = kzalloc(sizeof(*aspeed_clk_data) +
> + sizeof(*aspeed_clk_data->hws) * ASPEED_NUM_CLKS,
> + GFP_KERNEL);
> + if (!aspeed_clk_data)
> + return;
> +
> + /*
> + * This way all clocks fetched before the platform device probes,
> + * except those we assign here for early use, will be deferred.
> + */
> + for (i = 0; i < ASPEED_NUM_CLKS; i++)
> + aspeed_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
> +
> + map = syscon_node_to_regmap(np);
> + if (IS_ERR(map)) {
> + pr_err("no syscon regmap\n");
> + return;
> + }
> + /*
> + * We check that the regmap works on this very first access,
> + * but as this is an MMIO-backed regmap, subsequent regmap
> + * access is not going to fail and we skip error checks from
> + * this point.
> + */
> + ret = regmap_read(map, ASPEED_STRAP, &val);
> + if (ret) {
> + pr_err("failed to read strapping register\n");
> + return;
> + }
> +
> + aspeed_clk_data->num = ASPEED_NUM_CLKS;
> + ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, aspeed_clk_data);
> + if (ret)
> + pr_err("failed to add DT provider: %d\n", ret);
> +};
> +CLK_OF_DECLARE_DRIVER(aspeed_cc_g5, "aspeed,ast2500-scu", aspeed_cc_init);
> +CLK_OF_DECLARE_DRIVER(aspeed_cc_g4, "aspeed,ast2400-scu", aspeed_cc_init);
^ permalink raw reply
* [RESEND PATCH v2 07/15] ASoC: qcom: q6asm: Add support to memory map and unmap
From: Bjorn Andersson @ 2018-01-02 5:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171214173402.19074-8-srinivas.kandagatla@linaro.org>
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
> @@ -14,9 +14,46 @@
> #include "q6asm.h"
> #include "common.h"
>
> +#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 TUN_READ_IO_MODE 0x0004 /* tunnel read write mode */
> #define SYNC_IO_MODE 0x0001
> #define ASYNC_IO_MODE 0x0002
> +#define ASM_SHIFT_GAPLESS_MODE_FLAG 31
> +#define ADSP_MEMORY_MAP_SHMEM8_4K_POOL 3
> +
> +struct avs_cmd_shared_mem_map_regions {
> + struct apr_hdr hdr;
> + u16 mem_pool_id;
> + u16 num_regions;
> + u32 property_flag;
> +} __packed;
> +
> +struct avs_shared_map_region_payload {
> + u32 shm_addr_lsw;
> + u32 shm_addr_msw;
> + u32 mem_size_bytes;
> +} __packed;
> +
> +struct avs_cmd_shared_mem_unmap_regions {
> + struct apr_hdr hdr;
> + u32 mem_map_handle;
> +} __packed;
> +
> +struct audio_buffer {
> + dma_addr_t phys;
> + uint32_t used;
> + uint32_t size; /* size of buffer */
> +};
> +
> +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_".
> + 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.
> +{
> + 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.
> + 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.
> + 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
> + 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?
> +
> + 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?
> + 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.
> + 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.
> +
> + 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.
> + 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.
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.
> +
> + 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.
> + ++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.
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.
> +{
> + 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.
> +
> + buf = kzalloc(((sizeof(struct audio_buffer)) * periods), GFP_KERNEL);
Loose a few of those parenthesis and use *buf in the sizeof.
> + 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.
> + 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.
> + 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".
> + 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.
> +
> + 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.
> + 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.
> + }
> +
> + 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
* [PATCH v7 2/5] clk: aspeed: Register core clocks
From: Benjamin Herrenschmidt @ 2018-01-02 5:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171222024522.10362-3-joel@jms.id.au>
On Fri, 2017-12-22 at 13:15 +1030, Joel Stanley wrote:
> This registers the core clocks; those which are required to calculate
> the rate of the timer peripheral so the system can load a clocksource
> driver.
>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> v5:
> - Add Andrew's Reviewed-by
> v4:
> - Add defines to document the BIT() macros
> v3:
> - Fix ast2400 ahb calculation
> - Remove incorrect 'this is wrong' comment
> - Separate out clkin calc to be per platform
> - Support 48MHz clkin on ast2400
> ---
> drivers/clk/clk-aspeed.c | 177 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 177 insertions(+)
>
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index 7a86ee08ea4f..5adedda82d26 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -13,7 +13,23 @@
>
> #define ASPEED_NUM_CLKS 35
>
> +#define ASPEED_RESET_CTRL 0x04
> +#define ASPEED_CLK_SELECTION 0x08
> +#define ASPEED_CLK_STOP_CTRL 0x0c
> +#define ASPEED_MPLL_PARAM 0x20
> +#define ASPEED_HPLL_PARAM 0x24
> +#define AST2500_HPLL_BYPASS_EN BIT(20)
> +#define AST2400_HPLL_STRAPPED BIT(18)
> +#define AST2400_HPLL_BYPASS_EN BIT(17)
> +#define ASPEED_MISC_CTRL 0x2c
> +#define UART_DIV13_EN BIT(12)
> #define ASPEED_STRAP 0x70
> +#define CLKIN_25MHZ_EN BIT(23)
> +#define AST2400_CLK_SOURCE_SEL BIT(18)
> +#define ASPEED_CLK_SELECTION_2 0xd8
> +
> +/* Globally visible clocks */
> +static DEFINE_SPINLOCK(aspeed_clk_lock);
>
> /* Keeps track of all clocks */
> static struct clk_hw_onecell_data *aspeed_clk_data;
> @@ -91,6 +107,160 @@ static const struct aspeed_gate_data aspeed_gates[] = {
> [ASPEED_CLK_GATE_LHCCLK] = { 28, -1, "lhclk-gate", "lhclk", 0 }, /* LPC master/LPC+ */
> };
>
> +static const struct clk_div_table ast2400_div_table[] = {
> + { 0x0, 2 },
> + { 0x1, 4 },
> + { 0x2, 6 },
> + { 0x3, 8 },
> + { 0x4, 10 },
> + { 0x5, 12 },
> + { 0x6, 14 },
> + { 0x7, 16 },
> + { 0 }
> +};
> +
> +static const struct clk_div_table ast2500_div_table[] = {
> + { 0x0, 4 },
> + { 0x1, 8 },
> + { 0x2, 12 },
> + { 0x3, 16 },
> + { 0x4, 20 },
> + { 0x5, 24 },
> + { 0x6, 28 },
> + { 0x7, 32 },
> + { 0 }
> +};
> +
> +static struct clk_hw *aspeed_ast2400_calc_pll(const char *name, u32 val)
> +{
> + unsigned int mult, div;
> +
> + if (val & AST2400_HPLL_BYPASS_EN) {
> + /* Pass through mode */
> + mult = div = 1;
> + } else {
> + /* F = 24Mhz * (2-OD) * [(N + 2) / (D + 1)] */
> + u32 n = (val >> 5) & 0x3f;
> + u32 od = (val >> 4) & 0x1;
> + u32 d = val & 0xf;
> +
> + mult = (2 - od) * (n + 2);
> + div = d + 1;
> + }
> + return clk_hw_register_fixed_factor(NULL, name, "clkin", 0,
> + mult, div);
> +};
> +
> +static struct clk_hw *aspeed_ast2500_calc_pll(const char *name, u32 val)
> +{
> + unsigned int mult, div;
> +
> + if (val & AST2500_HPLL_BYPASS_EN) {
> + /* Pass through mode */
> + mult = div = 1;
> + } else {
> + /* F = clkin * [(M+1) / (N+1)] / (P + 1) */
> + u32 p = (val >> 13) & 0x3f;
> + u32 m = (val >> 5) & 0xff;
> + u32 n = val & 0x1f;
> +
> + mult = (m + 1) / (n + 1);
> + div = p + 1;
> + }
> +
> + return clk_hw_register_fixed_factor(NULL, name, "clkin", 0,
> + mult, div);
> +}
> +
> +static void __init aspeed_ast2400_cc(struct regmap *map)
> +{
> + struct clk_hw *hw;
> + u32 val, freq, div;
> +
> + /*
> + * CLKIN is the crystal oscillator, 24, 48 or 25MHz selected by
> + * strapping
> + */
> + regmap_read(map, ASPEED_STRAP, &val);
> + if (val & CLKIN_25MHZ_EN)
> + freq = 25000000;
> + else if (val & AST2400_CLK_SOURCE_SEL)
> + freq = 48000000;
> + else
> + freq = 24000000;
> + hw = clk_hw_register_fixed_rate(NULL, "clkin", NULL, 0, freq);
> + pr_debug("clkin @%u MHz\n", freq / 1000000);
> +
> + /*
> + * High-speed PLL clock derived from the crystal. This the CPU clock,
> + * and we assume that it is enabled
> + */
> + regmap_read(map, ASPEED_HPLL_PARAM, &val);
> + WARN(val & AST2400_HPLL_STRAPPED, "hpll is strapped not configured");
> + aspeed_clk_data->hws[ASPEED_CLK_HPLL] = aspeed_ast2400_calc_pll("hpll", val);
> +
> + /*
> + * Strap bits 11:10 define the CPU/AHB clock frequency ratio (aka HCLK)
> + * 00: Select CPU:AHB = 1:1
> + * 01: Select CPU:AHB = 2:1
> + * 10: Select CPU:AHB = 4:1
> + * 11: Select CPU:AHB = 3:1
> + */
> + regmap_read(map, ASPEED_STRAP, &val);
> + val = (val >> 10) & 0x3;
> + div = val + 1;
> + if (div == 3)
> + div = 4;
> + else if (div == 4)
> + div = 3;
> + hw = clk_hw_register_fixed_factor(NULL, "ahb", "hpll", 0, 1, div);
> + aspeed_clk_data->hws[ASPEED_CLK_AHB] = hw;
> +
> + /* APB clock clock selection register SCU08 (aka PCLK) */
> + hw = clk_hw_register_divider_table(NULL, "apb", "hpll", 0,
> + scu_base + ASPEED_CLK_SELECTION, 23, 3, 0,
> + ast2400_div_table,
> + &aspeed_clk_lock);
> + aspeed_clk_data->hws[ASPEED_CLK_APB] = hw;
> +}
> +
> +static void __init aspeed_ast2500_cc(struct regmap *map)
> +{
> + struct clk_hw *hw;
> + u32 val, freq, div;
> +
> + /* CLKIN is the crystal oscillator, 24 or 25MHz selected by strapping */
> + regmap_read(map, ASPEED_STRAP, &val);
> + if (val & CLKIN_25MHZ_EN)
> + freq = 25000000;
> + else
> + freq = 24000000;
> + hw = clk_hw_register_fixed_rate(NULL, "clkin", NULL, 0, freq);
> + pr_debug("clkin @%u MHz\n", freq / 1000000);
> +
> + /*
> + * High-speed PLL clock derived from the crystal. This the CPU clock,
> + * and we assume that it is enabled
> + */
> + regmap_read(map, ASPEED_HPLL_PARAM, &val);
> + aspeed_clk_data->hws[ASPEED_CLK_HPLL] = aspeed_ast2500_calc_pll("hpll", val);
> +
> + /* Strap bits 11:9 define the AXI/AHB clock frequency ratio (aka HCLK)*/
> + regmap_read(map, ASPEED_STRAP, &val);
> + val = (val >> 9) & 0x7;
> + WARN(val == 0, "strapping is zero: cannot determine ahb clock");
> + div = 2 * (val + 1);
> + hw = clk_hw_register_fixed_factor(NULL, "ahb", "hpll", 0, 1, div);
> + aspeed_clk_data->hws[ASPEED_CLK_AHB] = hw;
> +
> + /* APB clock clock selection register SCU08 (aka PCLK) */
> + regmap_read(map, ASPEED_CLK_SELECTION, &val);
> + val = (val >> 23) & 0x7;
> + div = 4 * (val + 1);
> + hw = clk_hw_register_fixed_factor(NULL, "apb", "hpll", 0, 1, div);
> + aspeed_clk_data->hws[ASPEED_CLK_APB] = hw;
> +};
> +
> static void __init aspeed_cc_init(struct device_node *np)
> {
> struct regmap *map;
> @@ -132,6 +302,13 @@ static void __init aspeed_cc_init(struct device_node *np)
> return;
> }
>
> + if (of_device_is_compatible(np, "aspeed,ast2400-scu"))
> + aspeed_ast2400_cc(map);
> + else if (of_device_is_compatible(np, "aspeed,ast2500-scu"))
> + aspeed_ast2500_cc(map);
> + else
> + pr_err("unknown platform, failed to add clocks\n");
> +
> aspeed_clk_data->num = ASPEED_NUM_CLKS;
> ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, aspeed_clk_data);
> if (ret)
^ permalink raw reply
* [PATCH v7 3/5] clk: aspeed: Add platform driver and register PLLs
From: Benjamin Herrenschmidt @ 2018-01-02 5:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171222024522.10362-4-joel@jms.id.au>
On Fri, 2017-12-22 at 13:15 +1030, Joel Stanley wrote:
> This registers a platform driver to set up all of the non-core clocks.
>
> The clocks that have configurable rates are now registered.
>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> --
> v6:
> - Add Andrew's reviewed-by
> v5:
> - Remove eclk configuration. We do not have enough information to
> correctly implement the mux and divisor, so it will have to be
> implemented in the future
> v4:
> - Add eclk div table to fix ast2500 calculation
> - Add defines to document the BIT() macros
> - Pass dev where we can when registering clocks
> - Check for errors when registering clk_hws
> v3:
> - Fix bclk and eclk calculation
> - Separate out ast2400 and ast25000 for pll calculation
> ---
> drivers/clk/clk-aspeed.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 130 insertions(+)
>
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index 5adedda82d26..cf5ea63feb31 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -5,6 +5,8 @@
> #include <linux/clk-provider.h>
> #include <linux/mfd/syscon.h>
> #include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> #include <linux/regmap.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> @@ -107,6 +109,18 @@ static const struct aspeed_gate_data aspeed_gates[] = {
> [ASPEED_CLK_GATE_LHCCLK] = { 28, -1, "lhclk-gate", "lhclk", 0 }, /* LPC master/LPC+ */
> };
>
> +static const struct clk_div_table ast2500_mac_div_table[] = {
> + { 0x0, 4 }, /* Yep, really. Aspeed confirmed this is correct */
> + { 0x1, 4 },
> + { 0x2, 6 },
> + { 0x3, 8 },
> + { 0x4, 10 },
> + { 0x5, 12 },
> + { 0x6, 14 },
> + { 0x7, 16 },
> + { 0 }
> +};
> +
> static const struct clk_div_table ast2400_div_table[] = {
> { 0x0, 2 },
> { 0x1, 4 },
> @@ -172,6 +186,122 @@ static struct clk_hw *aspeed_ast2500_calc_pll(const char *name, u32 val)
> mult, div);
> }
>
> +struct aspeed_clk_soc_data {
> + const struct clk_div_table *div_table;
> + const struct clk_div_table *mac_div_table;
> + struct clk_hw *(*calc_pll)(const char *name, u32 val);
> +};
> +
> +static const struct aspeed_clk_soc_data ast2500_data = {
> + .div_table = ast2500_div_table,
> + .mac_div_table = ast2500_mac_div_table,
> + .calc_pll = aspeed_ast2500_calc_pll,
> +};
> +
> +static const struct aspeed_clk_soc_data ast2400_data = {
> + .div_table = ast2400_div_table,
> + .mac_div_table = ast2400_div_table,
> + .calc_pll = aspeed_ast2400_calc_pll,
> +};
> +
> +static int aspeed_clk_probe(struct platform_device *pdev)
> +{
> + const struct aspeed_clk_soc_data *soc_data;
> + struct device *dev = &pdev->dev;
> + struct regmap *map;
> + struct clk_hw *hw;
> + u32 val, rate;
> +
> + map = syscon_node_to_regmap(dev->of_node);
> + if (IS_ERR(map)) {
> + dev_err(dev, "no syscon regmap\n");
> + return PTR_ERR(map);
> + }
> +
> + /* SoC generations share common layouts but have different divisors */
> + soc_data = of_device_get_match_data(dev);
> + if (!soc_data) {
> + dev_err(dev, "no match data for platform\n");
> + return -EINVAL;
> + }
> +
> + /* UART clock div13 setting */
> + regmap_read(map, ASPEED_MISC_CTRL, &val);
> + if (val & UART_DIV13_EN)
> + rate = 24000000 / 13;
> + else
> + rate = 24000000;
> + /* TODO: Find the parent data for the uart clock */
> + hw = clk_hw_register_fixed_rate(dev, "uart", NULL, 0, rate);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> + aspeed_clk_data->hws[ASPEED_CLK_UART] = hw;
> +
> + /*
> + * Memory controller (M-PLL) PLL. This clock is configured by the
> + * bootloader, and is exposed to Linux as a read-only clock rate.
> + */
> + regmap_read(map, ASPEED_MPLL_PARAM, &val);
> + hw = soc_data->calc_pll("mpll", val);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> + aspeed_clk_data->hws[ASPEED_CLK_MPLL] = hw;
> +
> + /* SD/SDIO clock divider (TODO: There's a gate too) */
> + hw = clk_hw_register_divider_table(dev, "sdio", "hpll", 0,
> + scu_base + ASPEED_CLK_SELECTION, 12, 3, 0,
> + soc_data->div_table,
> + &aspeed_clk_lock);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> + aspeed_clk_data->hws[ASPEED_CLK_SDIO] = hw;
> +
> + /* MAC AHB bus clock divider */
> + hw = clk_hw_register_divider_table(dev, "mac", "hpll", 0,
> + scu_base + ASPEED_CLK_SELECTION, 16, 3, 0,
> + soc_data->mac_div_table,
> + &aspeed_clk_lock);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> + aspeed_clk_data->hws[ASPEED_CLK_MAC] = hw;
> +
> + /* LPC Host (LHCLK) clock divider */
> + hw = clk_hw_register_divider_table(dev, "lhclk", "hpll", 0,
> + scu_base + ASPEED_CLK_SELECTION, 20, 3, 0,
> + soc_data->div_table,
> + &aspeed_clk_lock);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> + aspeed_clk_data->hws[ASPEED_CLK_LHCLK] = hw;
> +
> + /* P-Bus (BCLK) clock divider */
> + hw = clk_hw_register_divider_table(dev, "bclk", "hpll", 0,
> + scu_base + ASPEED_CLK_SELECTION_2, 0, 2, 0,
> + soc_data->div_table,
> + &aspeed_clk_lock);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> + aspeed_clk_data->hws[ASPEED_CLK_BCLK] = hw;
> +
> + return 0;
> +};
> +
> +static const struct of_device_id aspeed_clk_dt_ids[] = {
> + { .compatible = "aspeed,ast2400-scu", .data = &ast2400_data },
> + { .compatible = "aspeed,ast2500-scu", .data = &ast2500_data },
> + { }
> +};
> +
> +static struct platform_driver aspeed_clk_driver = {
> + .probe = aspeed_clk_probe,
> + .driver = {
> + .name = "aspeed-clk",
> + .of_match_table = aspeed_clk_dt_ids,
> + .suppress_bind_attrs = true,
> + },
> +};
> +builtin_platform_driver(aspeed_clk_driver);
> +
> static void __init aspeed_ast2400_cc(struct regmap *map)
> {
> struct clk_hw *hw;
^ permalink raw reply
* [PATCH v7 4/5] clk: aspeed: Register gated clocks
From: Benjamin Herrenschmidt @ 2018-01-02 5:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171222024522.10362-5-joel@jms.id.au>
On Fri, 2017-12-22 at 13:15 +1030, Joel Stanley wrote:
> The majority of the clocks in the system are gates paired with a reset
> controller that holds the IP in reset.
>
> This borrows from clk_hw_register_gate, but registers two 'gates', one
> to control the clock enable register and the other to control the reset
> IP. This allows us to enforce the ordering:
>
> 1. Place IP in reset
> 2. Enable clock
> 3. Delay
> 4. Release reset
>
> There are some gates that do not have an associated reset; these are
> handled by using -1 as the index for the reset.
>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> v7:
> - Use mdelay instead of msleep and remove the todo. Stephen points out:
> > No you can't sleep here. It needs to delay because this is inside
> > spinlock_irqsave.
> v5:
> - Add Andrew's Reviewed-by
> v4:
> - Drop useless 'disable clock' comment
> - Drop CLK_IS_BASIC flag
> - Fix 'there are a number of clocks...' comment
> - Pass device to clk registration functions
> - Check for errors when registering clk_hws
> v3:
> - Remove gates offset as gates are now at the start of the list
>
> mdelay
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> drivers/clk/clk-aspeed.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 130 insertions(+)
>
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index cf5ea63feb31..dbd3c7774831 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -204,6 +204,106 @@ static const struct aspeed_clk_soc_data ast2400_data = {
> .calc_pll = aspeed_ast2400_calc_pll,
> };
>
> +static int aspeed_clk_enable(struct clk_hw *hw)
> +{
> + struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
> + unsigned long flags;
> + u32 clk = BIT(gate->clock_idx);
> + u32 rst = BIT(gate->reset_idx);
> +
> + spin_lock_irqsave(gate->lock, flags);
> +
> + if (gate->reset_idx >= 0) {
> + /* Put IP in reset */
> + regmap_update_bits(gate->map, ASPEED_RESET_CTRL, rst, rst);
> +
> + /* Delay 100us */
> + udelay(100);
> + }
> +
> + /* Enable clock */
> + regmap_update_bits(gate->map, ASPEED_CLK_STOP_CTRL, clk, 0);
> +
> + if (gate->reset_idx >= 0) {
> + /* A delay of 10ms is specified by the ASPEED docs */
> + mdelay(10);
> +
> + /* Take IP out of reset */
> + regmap_update_bits(gate->map, ASPEED_RESET_CTRL, rst, 0);
> + }
> +
> + spin_unlock_irqrestore(gate->lock, flags);
> +
> + return 0;
> +}
> +
> +static void aspeed_clk_disable(struct clk_hw *hw)
> +{
> + struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
> + unsigned long flags;
> + u32 clk = BIT(gate->clock_idx);
> +
> + spin_lock_irqsave(gate->lock, flags);
> +
> + regmap_update_bits(gate->map, ASPEED_CLK_STOP_CTRL, clk, clk);
> +
> + spin_unlock_irqrestore(gate->lock, flags);
> +}
> +
> +static int aspeed_clk_is_enabled(struct clk_hw *hw)
> +{
> + struct aspeed_clk_gate *gate = to_aspeed_clk_gate(hw);
> + u32 clk = BIT(gate->clock_idx);
> + u32 reg;
> +
> + regmap_read(gate->map, ASPEED_CLK_STOP_CTRL, ®);
> +
> + return (reg & clk) ? 0 : 1;
> +}
> +
> +static const struct clk_ops aspeed_clk_gate_ops = {
> + .enable = aspeed_clk_enable,
> + .disable = aspeed_clk_disable,
> + .is_enabled = aspeed_clk_is_enabled,
> +};
> +
> +static struct clk_hw *aspeed_clk_hw_register_gate(struct device *dev,
> + const char *name, const char *parent_name, unsigned long flags,
> + struct regmap *map, u8 clock_idx, u8 reset_idx,
> + u8 clk_gate_flags, spinlock_t *lock)
> +{
> + struct aspeed_clk_gate *gate;
> + struct clk_init_data init;
> + struct clk_hw *hw;
> + int ret;
> +
> + gate = kzalloc(sizeof(*gate), GFP_KERNEL);
> + if (!gate)
> + return ERR_PTR(-ENOMEM);
> +
> + init.name = name;
> + init.ops = &aspeed_clk_gate_ops;
> + init.flags = flags;
> + init.parent_names = parent_name ? &parent_name : NULL;
> + init.num_parents = parent_name ? 1 : 0;
> +
> + gate->map = map;
> + gate->clock_idx = clock_idx;
> + gate->reset_idx = reset_idx;
> + gate->flags = clk_gate_flags;
> + gate->lock = lock;
> + gate->hw.init = &init;
> +
> + hw = &gate->hw;
> + ret = clk_hw_register(dev, hw);
> + if (ret) {
> + kfree(gate);
> + hw = ERR_PTR(ret);
> + }
> +
> + return hw;
> +}
> +
> static int aspeed_clk_probe(struct platform_device *pdev)
> {
> const struct aspeed_clk_soc_data *soc_data;
> @@ -211,6 +311,7 @@ static int aspeed_clk_probe(struct platform_device *pdev)
> struct regmap *map;
> struct clk_hw *hw;
> u32 val, rate;
> + int i;
>
> map = syscon_node_to_regmap(dev->of_node);
> if (IS_ERR(map)) {
> @@ -283,6 +384,35 @@ static int aspeed_clk_probe(struct platform_device *pdev)
> return PTR_ERR(hw);
> aspeed_clk_data->hws[ASPEED_CLK_BCLK] = hw;
>
> + /*
> + * TODO: There are a number of clocks that not included in this driver
> + * as more information is required:
> + * D2-PLL
> + * D-PLL
> + * YCLK
> + * RGMII
> + * RMII
> + * UART[1..5] clock source mux
> + * Video Engine (ECLK) mux and clock divider
> + */
> +
> + for (i = 0; i < ARRAY_SIZE(aspeed_gates); i++) {
> + const struct aspeed_gate_data *gd = &aspeed_gates[i];
> +
> + hw = aspeed_clk_hw_register_gate(dev,
> + gd->name,
> + gd->parent_name,
> + gd->flags,
> + map,
> + gd->clock_idx,
> + gd->reset_idx,
> + CLK_GATE_SET_TO_DISABLE,
> + &aspeed_clk_lock);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> + aspeed_clk_data->hws[i] = hw;
> + }
> +
> return 0;
> };
>
^ permalink raw reply
* [PATCH v7 5/5] clk: aspeed: Add reset controller
From: Benjamin Herrenschmidt @ 2018-01-02 5:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20171222024522.10362-6-joel@jms.id.au>
On Fri, 2017-12-22 at 13:15 +1030, Joel Stanley wrote:
> There are some resets that are not associated with gates. These are
> represented by a reset controller.
>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> v7:
> - Rebase on dt-bindings patch
> v5:
> - Add Andrew's Reviewed-by
> v3:
> - Add named initalisers for the reset defines
> - Add define for ADC
> ---
> drivers/clk/clk-aspeed.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 81 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index dbd3c7774831..6fb344730cea 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -8,6 +8,7 @@
> #include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/regmap.h>
> +#include <linux/reset-controller.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
>
> @@ -267,6 +268,68 @@ static const struct clk_ops aspeed_clk_gate_ops = {
> .is_enabled = aspeed_clk_is_enabled,
> };
>
> +/**
> + * struct aspeed_reset - Aspeed reset controller
> + * @map: regmap to access the containing system controller
> + * @rcdev: reset controller device
> + */
> +struct aspeed_reset {
> + struct regmap *map;
> + struct reset_controller_dev rcdev;
> +};
> +
> +#define to_aspeed_reset(p) container_of((p), struct aspeed_reset, rcdev)
> +
> +static const u8 aspeed_resets[] = {
> + [ASPEED_RESET_XDMA] = 25,
> + [ASPEED_RESET_MCTP] = 24,
> + [ASPEED_RESET_ADC] = 23,
> + [ASPEED_RESET_JTAG_MASTER] = 22,
> + [ASPEED_RESET_MIC] = 18,
> + [ASPEED_RESET_PWM] = 9,
> + [ASPEED_RESET_PCIVGA] = 8,
> + [ASPEED_RESET_I2C] = 2,
> + [ASPEED_RESET_AHB] = 1,
> +};
> +
> +static int aspeed_reset_deassert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct aspeed_reset *ar = to_aspeed_reset(rcdev);
> + u32 rst = BIT(aspeed_resets[id]);
> +
> + return regmap_update_bits(ar->map, ASPEED_RESET_CTRL, rst, 0);
> +}
> +
> +static int aspeed_reset_assert(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct aspeed_reset *ar = to_aspeed_reset(rcdev);
> + u32 rst = BIT(aspeed_resets[id]);
> +
> + return regmap_update_bits(ar->map, ASPEED_RESET_CTRL, rst, rst);
> +}
> +
> +static int aspeed_reset_status(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct aspeed_reset *ar = to_aspeed_reset(rcdev);
> + u32 val, rst = BIT(aspeed_resets[id]);
> + int ret;
> +
> + ret = regmap_read(ar->map, ASPEED_RESET_CTRL, &val);
> + if (ret)
> + return ret;
> +
> + return !!(val & rst);
> +}
> +
> +static const struct reset_control_ops aspeed_reset_ops = {
> + .assert = aspeed_reset_assert,
> + .deassert = aspeed_reset_deassert,
> + .status = aspeed_reset_status,
> +};
> +
> static struct clk_hw *aspeed_clk_hw_register_gate(struct device *dev,
> const char *name, const char *parent_name, unsigned long flags,
> struct regmap *map, u8 clock_idx, u8 reset_idx,
> @@ -308,10 +371,11 @@ static int aspeed_clk_probe(struct platform_device *pdev)
> {
> const struct aspeed_clk_soc_data *soc_data;
> struct device *dev = &pdev->dev;
> + struct aspeed_reset *ar;
> struct regmap *map;
> struct clk_hw *hw;
> u32 val, rate;
> - int i;
> + int i, ret;
>
> map = syscon_node_to_regmap(dev->of_node);
> if (IS_ERR(map)) {
> @@ -319,6 +383,22 @@ static int aspeed_clk_probe(struct platform_device *pdev)
> return PTR_ERR(map);
> }
>
> + ar = devm_kzalloc(dev, sizeof(*ar), GFP_KERNEL);
> + if (!ar)
> + return -ENOMEM;
> +
> + ar->map = map;
> + ar->rcdev.owner = THIS_MODULE;
> + ar->rcdev.nr_resets = ARRAY_SIZE(aspeed_resets);
> + ar->rcdev.ops = &aspeed_reset_ops;
> + ar->rcdev.of_node = dev->of_node;
> +
> + ret = devm_reset_controller_register(dev, &ar->rcdev);
> + if (ret) {
> + dev_err(dev, "could not register reset controller\n");
> + return ret;
> + }
> +
> /* SoC generations share common layouts but have different divisors */
> soc_data = of_device_get_match_data(dev);
> if (!soc_data) {
^ permalink raw reply
* [PATCH] Input: misc: gpio_tilt: Delete driver
From: Dmitry Torokhov @ 2018-01-02 6:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <14171345.p7a3jPx4Si@phil>
On Wed, Dec 27, 2017 at 02:51:45PM +0100, Heiko Stuebner wrote:
> Hi Linus,
>
> Am Mittwoch, 27. Dezember 2017, 13:15:47 CET schrieb Linus Walleij:
> > This driver was merged in 2011 as a tool for detecting the orientation
> > of a screen. The device driver assumes board file setup using the
> > platform data from <linux/input/gpio_tilt.h>. But no boards in the
> > kernel tree defines this platform data.
> >
> > As I am faced with refactoring drivers to use GPIO descriptors and
> > pass decriptor tables from boards, or use the device tree device
> > drivers like these creates a serious problem: I cannot fix them and
> > cannot test them, not even compile-test them with a system actually
> > using it (no in-tree boardfile).
> >
> > I suggest to delete this driver and rewrite it using device tree if
> > it is still in use on actively maintained systems.
> >
> > I can also offer to rewrite it out of the blue using device tree if
> > someone promise to test it and help me iterate it.
> >
> > Cc: Heiko St?bner <heiko@sntech.de>
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> > Heiko: not meaning to be militant here, just contain the situation,
> > as stated: if you like the driver and can test it, I can reimplement
> > it from scratch using device tree.
>
> It seems that piece of hardware (gpio-connected orientation-sensors)
> was really only used in the one s3c24xx-based device I hacked on in 2011.
>
> I somehow lost focus from trying to do the s3c24xx devicetree migration
> when I started hacking on Rockchip stuff, so while I do have the devices
> still around, I don't think I'll find the time and energy trying to get a
> recent kernel to run on them anyway, so I'm fine with dropping the driver.
> It's simple enough to get reintroduced if someone really finds a device
> using it or time to redo the ereader support using devicetree.
>
> So long story short
> Acked-by: Heiko Stuebner <heiko@sntech.de>
Applied, thank you.
--
Dmitry
^ 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