Date:   Tue, 22 May 2018 23:26:45 -0700
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Rohit kumar <rohitkr@...eaurora.org>
Cc:     ohad@...ery.com, robh+dt@...nel.org, mark.rutland@....com,
        linux-remoteproc@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, bgoswami@...eaurora.org,
        sbpata@...eaurora.org, asishb@...eaurora.org,
        rkarra@...eaurora.org,
        RajendraBabu Medisetti <rajendrabm@...eaurora.org>,
        Krishnamurthy Renu <krishnamurthy.renu@...eaurora.org>
Subject: Re: [PATCH] remoteproc:  Add APSS based Qualcomm ADSP PIL driver for
 SDM845

On Sun 13 May 00:01 PDT 2018, Rohit kumar wrote:

> This adds Qualcomm ADSP PIL driver support for SDM845 with ADSP bootup
> and shutdown operation handled from Application Processor SubSystem(APSS).
> 
> Signed-off-by: Rohit kumar <rohitkr@...eaurora.org>
> Signed-off-by: RajendraBabu Medisetti <rajendrabm@...eaurora.org>
> Signed-off-by: Krishnamurthy Renu <krishnamurthy.renu@...eaurora.org>
> ---
>  .../devicetree/bindings/remoteproc/qcom,adsp.txt   |   1 +
>  drivers/remoteproc/Makefile                        |   3 +-
>  drivers/remoteproc/qcom_adsp_pil.c                 | 122 ++++-----
>  drivers/remoteproc/qcom_adsp_pil.h                 |  86 ++++++
>  drivers/remoteproc/qcom_adsp_pil_sdm845.c          | 304 +++++++++++++++++++++
>  5 files changed, 454 insertions(+), 62 deletions(-)
>  create mode 100644 drivers/remoteproc/qcom_adsp_pil.h
>  create mode 100644 drivers/remoteproc/qcom_adsp_pil_sdm845.c
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt b/Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt
> index 728e419..a9fe033 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt
> @@ -10,6 +10,7 @@ on the Qualcomm ADSP Hexagon core.
>  		    "qcom,msm8974-adsp-pil"
>  		    "qcom,msm8996-adsp-pil"
>  		    "qcom,msm8996-slpi-pil"
> +		    "qcom,sdm845-apss-adsp-pil"

Afaict there's nothing in this binding that ties this to the apss, so I
don't think we should base the compatible on this. The differentiation
is PAS vs non-PAS; so let's start naming the PAS variants
"qcom,platform-subsystem-pas" and the non-PAS
"qcom,platform-subsystem-pil" instead.

I.e. please make this "qcom,sdm845-adsp-pil".

More importantly, any resources such as clocks or reset lines should
come from DT and as such you need to extend the binding quite a bit -
which I suggest you do by introducing a new binding document.

>  
>  - interrupts-extended:
>  	Usage: required
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index 02627ed..759831b 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -14,7 +14,8 @@ obj-$(CONFIG_OMAP_REMOTEPROC)		+= omap_remoteproc.o
>  obj-$(CONFIG_WKUP_M3_RPROC)		+= wkup_m3_rproc.o
>  obj-$(CONFIG_DA8XX_REMOTEPROC)		+= da8xx_remoteproc.o
>  obj-$(CONFIG_KEYSTONE_REMOTEPROC)	+= keystone_remoteproc.o
> -obj-$(CONFIG_QCOM_ADSP_PIL)		+= qcom_adsp_pil.o
> +obj-$(CONFIG_QCOM_ADSP_PIL)		+= qcom_adsp.o
> +qcom_adsp-objs				+= qcom_adsp_pil.o qcom_adsp_pil_sdm845.o
>  obj-$(CONFIG_QCOM_RPROC_COMMON)		+= qcom_common.o
>  obj-$(CONFIG_QCOM_Q6V5_PIL)		+= qcom_q6v5_pil.o
>  obj-$(CONFIG_QCOM_SYSMON)		+= qcom_sysmon.o
> diff --git a/drivers/remoteproc/qcom_adsp_pil.c b/drivers/remoteproc/qcom_adsp_pil.c

I get the feeling that the main reason for modifying this file is its
name, not that it reduces the complexity of the final solution. I
definitely think it's cleaner to have some structural duplication and
keep this driver handling the various PAS based remoteprocs.

Please see the RFC series I posted reducing the duplication between the
various "Q6V5 drivers".

[..]
> diff --git a/drivers/remoteproc/qcom_adsp_pil.h b/drivers/remoteproc/qcom_adsp_pil.h
[..]
> +static inline void update_bits(void *reg, u32 mask_val, u32 set_val, u32 shift)
> +{
> +	u32 reg_val = 0;
> +
> +	reg_val = ((readl(reg)) & ~mask_val) | ((set_val << shift) & mask_val);
> +	writel(reg_val, reg);
> +}
> +
> +static inline unsigned int read_bit(void *reg, u32 mask, int shift)
> +{
> +	return ((readl(reg) & mask) >> shift);
> +}

I don't like these helper functions, their prototype is nonstandard and
makes it really hard to read all the calling code.

I would prefer if you just inline the operations directly, to make it
clearer what's going on in each case - if not then at least follow the
prototype of e.g. regmap_udpate_bits(), which people might be used to.

> +
> +#endif
> diff --git a/drivers/remoteproc/qcom_adsp_pil_sdm845.c b/drivers/remoteproc/qcom_adsp_pil_sdm845.c
> new file mode 100644
> index 0000000..7518385
> --- /dev/null
> +++ b/drivers/remoteproc/qcom_adsp_pil_sdm845.c
> @@ -0,0 +1,304 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Qualcomm APSS Based ADSP bootup/shutdown ops for SDM845.
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +
> +#include "qcom_adsp_pil.h"
> +
> +/* set values */
> +#define CLK_ENABLE				0x1
> +#define CLK_DISABLE				0x0

Just write 0 and 1 in the code, it saves future readers the trouble of
having to remember if these are special in any way.

> +/* time out value */
> +#define ACK_TIMEOUT				200000

This is currently given in the rather awkward unit of 5uS. As it's input
to what should have been a call to readl_poll_timeout() please express
it in micro seconds.

> +/* mask values */
> +#define CLK_MASK				GENMASK(4, 0)
> +#define EVB_MASK				GENMASK(27, 4)
> +#define SPIN_CLKOFF_MASK			BIT(31)
> +#define AUDIO_SYNC_RESET_MASK			BIT(2)
> +#define CLK_ENABLE_MASK				BIT(0)
> +#define HAL_CLK_MASK				BIT(1)
> +/* GCC register offsets */
> +#define GCC_BASE				0x00147000

This doesn't belong here, expose the resource from the gcc driver using
existing frameworks.

> +#define SWAY_CBCR_OFFSET			0x00000008
> +/*LPASS register base address and offsets*/
> +#define LPASS_BASE				0x17000000

This should be in the lpass clock driver.

> +#define AON_CBCR_OFFSET				0x00014098
> +#define CMD_RCGR_OFFSET				0x00014000
> +#define CFG_RCGR_OFFSET				0x00014004
> +#define AHBS_AON_CBCR_OFFSET			0x00033000
> +#define AHBM_AON_CBCR_OFFSET			0x00026000
> +/*QDSP6SS register base address and offsets*/
> +#define QDSP6SS_BASE				0x17300000

This should come from the reg property in DT.

> +#define RST_EVB_OFFSET				0x00000010
> +#define SLEEP_CBCR_OFFSET			0x0000003C
> +#define XO_CBCR_OFFSET				0x00000038
> +#define CORE_CBCR_OFFSET			0x00000020
> +#define CORE_START_OFFSET			0x00000400
> +#define BOOT_CMD_OFFSET				0x00000404
> +#define BOOT_STATUS_OFFSET			0x00000408
> +#define RET_CFG_OFFSET				0x0000001C
> +/*TCSR register base address and offsets*/
> +#define TCSR_BASE				0x01F62000

Look at how we deal with TCSR in the MSS driver instead.

> +#define TCSR_LPASS_MASTER_IDLE_OFFSET		0x00000008
> +#define TCSR_LPASS_HALTACK_OFFSET		0x00000004
> +#define TCSR_LPASS_PWR_ON_OFFSET		0x00000010
> +#define TCSR_LPASS_HALTREQ_OFFSET		0X00000000
> +
> +#define RPMH_PDC_SYNC_RESET_ADDR		0x0B2E0100
> +#define AOSS_CC_LPASS_RESTART_ADDR		0x0C2D0000

Please expose these from an appropriate driver using appropriate
frameworks.

> +
> +struct sdm845_reg {
> +	void __iomem *gcc_base;
> +	void __iomem *lpass_base;
> +	void __iomem *qdsp6ss_base;
> +	void __iomem *tcsr_base;
> +	void __iomem *pdc_sync;
> +	void __iomem *cc_lpass;

I expect to see only qdsp6ss_base remain here.

> +};
> +
> +static int sdm845_map_registers(struct qcom_adsp *adsp,
> +				struct platform_device *pdev)
> +{
> +	struct sdm845_reg *reg;
> +
> +	adsp->priv_reg = devm_kzalloc(&pdev->dev, sizeof(struct sdm845_reg),
> +			GFP_KERNEL);
> +	if (!adsp->priv_reg)
> +		return -ENOMEM;
> +
> +	reg = adsp->priv_reg;
> +
> +	reg->gcc_base = devm_ioremap(adsp->dev, GCC_BASE, 0xc);
> +	if (!reg->gcc_base) {
> +		dev_err(adsp->dev, "%s: failed to map GCC base registers\n",
> +				__func__);
> +		return -ENOMEM;
> +	}
> +
> +	reg->lpass_base = devm_ioremap(adsp->dev, LPASS_BASE, 0x8E004);
> +	if (!reg->lpass_base) {
> +		dev_err(adsp->dev, "%s: failed to map LPASS base registers\n",
> +				__func__);
> +		return -ENOMEM;
> +	}
> +	reg->qdsp6ss_base =  devm_ioremap(adsp->dev, QDSP6SS_BASE, 0x40c);
> +	if (!reg->qdsp6ss_base) {
> +		dev_err(adsp->dev, "%s: failed to map QDSP6SS base registers\n",
> +				__func__);
> +		return -ENOMEM;
> +	}
> +	reg->tcsr_base = devm_ioremap(adsp->dev, TCSR_BASE, 0x14);
> +	if (!reg->tcsr_base) {
> +		dev_err(adsp->dev, "%s: failed to map TCSR base registers\n",
> +				__func__);
> +		return -ENOMEM;
> +	}
> +	reg->pdc_sync = devm_ioremap(adsp->dev, RPMH_PDC_SYNC_RESET_ADDR, 0x4);
> +	if (!reg->pdc_sync) {
> +		dev_err(adsp->dev, "%s: failed to map RPMH_PDC_SYNC_RESET register\n",
> +				__func__);
> +		return -ENOMEM;
> +	}
> +	reg->cc_lpass = devm_ioremap(adsp->dev, AOSS_CC_LPASS_RESTART_ADDR,
> +			0x4);
> +	if (!reg->cc_lpass) {
> +		dev_err(adsp->dev, "%s:failed to map AOSS_CC_LPASS_RESTART register\n",
> +				__func__);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int clk_enable_spin(void *reg, int read_shift, int write_shift)

This should be in the appropriate clock drivers.

> +{
> +	u32 maxDelay = 500;
> +	u32 val;
> +
> +	update_bits(reg, CLK_ENABLE_MASK, CLK_ENABLE, write_shift);
> +	val = readl(reg);
> +	if (!(readl(reg) & HAL_CLK_MASK)) {
> +		/*
> +		 * wait for disabling of HW signal CLK_OFF to confirm that
> +		 * clock is actually ON.
> +		 */
> +		while (maxDelay-- && read_bit(reg, SPIN_CLKOFF_MASK,
> +							read_shift))
> +			udelay(1);
> +	}
> +	if (!maxDelay) {
> +		pr_err("%s: fail to update register = %p\n", __func__, reg);
> +		return -ETIMEDOUT;
> +	}
> +	return 0;
> +}
> +
> +static int sdm845_adsp_clk_enable(struct qcom_adsp *adsp)
> +{
> +	u32 ret;
> +	u32 maxDelay = 100;
> +	struct sdm845_reg *reg = adsp->priv_reg;
> +
> +	/* Enable SWAY clock */
> +	ret = clk_enable_spin(reg->gcc_base + SWAY_CBCR_OFFSET, CLK_MASK, 0x0);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable LPASS AHB AON Bus */
> +	ret = clk_enable_spin(reg->lpass_base + AON_CBCR_OFFSET, CLK_MASK, 0x0);
> +	if (ret)
> +		return ret;
> +
> +	/* Set the AON clock root to be sourced by XO */
> +	writel(CLK_DISABLE, reg->lpass_base + CFG_RCGR_OFFSET);
> +	writel(CLK_ENABLE, reg->lpass_base + CMD_RCGR_OFFSET);
> +
> +	while (read_bit((reg->lpass_base + CMD_RCGR_OFFSET), CLK_ENABLE, 0)
> +						&& maxDelay--)
> +		udelay(2);
> +
> +	if (!maxDelay) {
> +		pr_err("%s: fail to enable CMD_RCGR clock\n", __func__);
> +		return -ETIMEDOUT;
> +	}
> +
> +	/* Enable the QDSP6SS AHBM and AHBS clocks */
> +	ret = clk_enable_spin(reg->lpass_base + AHBS_AON_CBCR_OFFSET,
> +				CLK_MASK, 0x0);
> +	if (ret)
> +		return ret;
> +	ret = clk_enable_spin(reg->lpass_base + AHBM_AON_CBCR_OFFSET,
> +				CLK_MASK, 0x0);
> +	if (ret)
> +		return ret;

Above should be calls to the clock framework.

> +
> +	/* Turn on the XO clock, required to boot FSM */
> +	update_bits(reg->qdsp6ss_base + XO_CBCR_OFFSET, CLK_ENABLE_MASK,
> +							CLK_ENABLE, 0x0);
> +
> +	/* Enable the QDSP6SS sleep clock for the QDSP6 watchdog enablement */
> +	update_bits(reg->qdsp6ss_base + SLEEP_CBCR_OFFSET,
> +					CLK_ENABLE_MASK, CLK_ENABLE, 0x0);
> +
> +	/* Configure QDSP6 core CBC to enable clock */
> +	update_bits(reg->qdsp6ss_base + CORE_CBCR_OFFSET, CLK_ENABLE_MASK,
> +					CLK_ENABLE, 0x0);
> +	return 0;
> +}
> +
> +static int sdm845_adsp_reset(struct qcom_adsp *adsp)
> +{
> +	u32 timeout = ACK_TIMEOUT;
> +	struct sdm845_reg *reg = adsp->priv_reg;
> +
> +	/* De-assert QDSP6 stop core. QDSP6 will execute after out of reset */
> +	update_bits(reg->qdsp6ss_base + CORE_START_OFFSET,
> +					CLK_ENABLE_MASK, CLK_ENABLE, 0x0);
> +	/* Trigger boot FSM to start QDSP6 */
> +	writel(CLK_ENABLE, reg->qdsp6ss_base + BOOT_CMD_OFFSET);
> +
> +	/* Wait for core to come out of reset */
> +	while ((!(readl(reg->qdsp6ss_base +
> +			BOOT_STATUS_OFFSET))) && (timeout-- > 0))
> +		udelay(5);

Use readl_poll_timeout() from linux/iopoll.h

> +
> +	if (!timeout)
> +		return -ETIMEDOUT;
> +
> +	return 0;
> +}
> +
> +static int sdm845_bringup(struct qcom_adsp *adsp)

This is called "start" in other places, please use existing naming
convention to make your code feel familiar to people reading other
drivers.

> +{
> +	u32 ret;

ret is exclusively used to store data of the type "int".

> +	struct sdm845_reg *reg = adsp->priv_reg;
> +
> +	ret = sdm845_adsp_clk_enable(adsp);
> +	if (ret) {
> +		dev_err(adsp->dev, "%s: sdm845_adsp_clk_enable failed\n",
> +				__func__);
> +		return ret;
> +	}
> +	/* Program boot address */
> +	update_bits(reg->qdsp6ss_base + RST_EVB_OFFSET,
> +				EVB_MASK, (adsp->mem_phys) >> 8, 0x4);

In the WCSS PIL driver this is:

	writel(rproc->bootaddr >> 4, wcss->reg_base + QDSP6SS_RST_EVB);          

Which I think is the same as you're doing here, although you're shifting
8 bits right and then 4 (base 16) to the left.

> +
> +	/* Wait for addresses to be programmed before starting adsp */

That's not what mb() does, it just ensures that any read and writes
coming after this point isn't reordered with any operations before it.

And as sdm845_adsp_reset() used writel() there is already a wmb() there,
so you can drop this one.

> +	mb();
> +	ret = sdm845_adsp_reset(adsp);
> +	if (ret)
> +		dev_err(adsp->dev, "%s: De-assert QDSP6 out of reset failed\n",
> +					__func__);

This string is unique in the kernel, so you don't need __func__.

> +	return ret;
> +}
> +
> +static int sdm845_bringdown(struct qcom_adsp *adsp)
> +{
> +	u32 acktimeout = ACK_TIMEOUT;
> +	u32 temp;

We know this is a temporary variable, name it "val" as we do in the
other functions.

> +	struct sdm845_reg *reg = adsp->priv_reg;
> +
> +	/* Reset the retention logic */
> +	update_bits(reg->qdsp6ss_base + RET_CFG_OFFSET,
> +			CLK_ENABLE_MASK, CLK_ENABLE, 0x0);
> +	/* Disable the slave way clock to LPASS */
> +	update_bits(reg->gcc_base + SWAY_CBCR_OFFSET,
> +			CLK_ENABLE_MASK, CLK_DISABLE, 0x0);
> +
> +	/* QDSP6 master port needs to be explicitly halted */
> +	temp = read_bit(reg->tcsr_base + TCSR_LPASS_PWR_ON_OFFSET,
> +			CLK_ENABLE, 0x0);
> +	temp = temp && !read_bit(reg->tcsr_base + TCSR_LPASS_MASTER_IDLE_OFFSET,
> +			CLK_ENABLE, 0x0);
> +	if (temp) {
> +		writel(CLK_ENABLE, reg->tcsr_base + TCSR_LPASS_HALTREQ_OFFSET);

CLK_ENABLE happens to have the right value, but the value you write is
"request halt" not "enable clock".

> +		/*  Wait for halt ACK from QDSP6 */
> +		while ((read_bit(reg->tcsr_base + TCSR_LPASS_HALTACK_OFFSET,
> +				CLK_DISABLE, 0x0) == 0) && (acktimeout-- > 0))
> +			udelay(5);
> +
> +		if (acktimeout) {
> +			if (read_bit(reg->tcsr_base +
> +					TCSR_LPASS_MASTER_IDLE_OFFSET,
> +						CLK_ENABLE, 0x0) != 1)
> +				dev_warn(adsp->dev,
> +						"%s: failed to receive %s\n",
> +						__func__, "TCSR MASTER ACK");
> +		} else {
> +			dev_err(adsp->dev, "%s: failed to receive halt ack\n",
> +					__func__);
> +			return -ETIMEDOUT;
> +		}
> +	}

Take a look at q6v5proc_halt_axi_port() in qcom_q6v5_pil.c instead of
this thing.

> +
> +	/* Assert the LPASS PDC Reset */
> +	update_bits(reg->pdc_sync,  AUDIO_SYNC_RESET_MASK,
> +			CLK_ENABLE, 0x2);

Use https://patchwork.kernel.org/patch/10415991/

reset_control_assert();

> +	/* Place the LPASS processor into reset */
> +	writel(CLK_ENABLE, reg->cc_lpass);
> +	/* wait after asserting subsystem restart from AOSS */
> +	udelay(200);
> +
> +	/* Clear the halt request for the AXIM and AHBM for Q6 */
> +	writel(CLK_DISABLE, reg->tcsr_base + TCSR_LPASS_HALTREQ_OFFSET);

Disable the clock that is the halt request register?

> +
> +	/* De-assert the LPASS PDC Reset */
> +	update_bits(reg->pdc_sync, AUDIO_SYNC_RESET_MASK,
> +			CLK_DISABLE, 0x2);

reset_control_deassert();

> +	/* Remove the LPASS reset */
> +	writel(CLK_DISABLE, reg->cc_lpass);
> +	/* wait after de-asserting subsystem restart from AOSS */
> +	udelay(200);
> +
> +	return 0;
> +}

Regards,
Bjorn

Powered by blists - more mailing lists