Re: [PATCH] remoteproc: qcom: Introduce Hexagon V5 based WCSS driver

[Vinod <vkoul@kernel.org>]

On 05-06-18, 11:12, Sricharan R wrote:

> +config QCOM_Q6V5_WCSS
> +	tristate "Qualcomm Hexagon based WCSS Peripheral Image Loader"
> +	depends on OF && ARCH_QCOM
> +	depends on QCOM_SMEM
> +	depends on RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n)
> +	depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n

Is there a reason why it depends on RPMSG_QCOM_GLINK_SMEM=n? What would
happen if distro wants both this and RPMSG_QCOM_GLINK_SMEM

> +/* QDSP6SS Register Offsets */
> +#define QDSP6SS_RESET_REG		0x014
> +#define QDSP6SS_GFMUX_CTL_REG		0x020
> +#define QDSP6SS_PWR_CTL_REG		0x030
> +#define QDSP6SS_MEM_PWR_CTL		0x0B0
> +
> +/* AXI Halt Register Offsets */
> +#define AXI_HALTREQ_REG			0x0
> +#define AXI_HALTACK_REG			0x4
> +#define AXI_IDLE_REG			0x8
> +
> +#define HALT_ACK_TIMEOUT_MS		100
> +
> +/* QDSP6SS_RESET */
> +#define Q6SS_STOP_CORE			BIT(0)
> +#define Q6SS_CORE_ARES			BIT(1)
> +#define Q6SS_BUS_ARES_ENABLE		BIT(2)

Wouldn't it be nice if the defines are all consistent, some of them are
QDSP6SS_xxx, some Q6SS_ some are not

Please do pick one and make it consistent :)

> +/* QDSP6v56 parameters */
> +#define QDSP6v56_LDO_BYP		BIT(25)
> +#define QDSP6v56_BHS_ON		BIT(24)
> +#define QDSP6v56_CLAMP_WL		BIT(21)
> +#define QDSP6v56_CLAMP_QMC_MEM		BIT(22)
> +#define HALT_CHECK_MAX_LOOPS		200
> +#define QDSP6SS_XO_CBCR		0x0038

GENMASK() perhaps?

> +static int q6v5_wcss_reset(struct q6v5_wcss *wcss)
> +{
> +	int ret;
> +	u32 val;
> +	int i;

        int ret, i;

> +static int q6v5_wcss_start(struct rproc *rproc)
> +{
> +	struct q6v5_wcss *wcss = rproc->priv;
> +	int ret;
> +
> +	qcom_q6v5_prepare(&wcss->q6v5);
> +
> +	/* Release Q6 and WCSS reset */
> +	ret = reset_control_deassert(wcss->wcss_reset);
> +	if (ret) {
> +		dev_err(wcss->dev, "wcss_reset failed\n");
> +		return ret;
> +	}
> +
> +	ret = reset_control_deassert(wcss->wcss_q6_reset);
> +	if (ret) {
> +		dev_err(wcss->dev, "wcss_q6_reset failed\n");
> +		goto wcss_reset;
> +	}
> +
> +	/* Lithium configuration - clock gating and bus arbitration */
> +	ret = regmap_update_bits(wcss->halt_map,
> +				 wcss->halt_nc + TCSR_GLOBAL_CFG0,
> +				 0x1F, 0x14);

magic numbers??

> +static int q6v5_wcss_powerdown(struct q6v5_wcss *wcss)
> +{
> +	int ret;
> +	u32 val;
> +
> +	/* 1 - Assert WCSS/Q6 HALTREQ */
> +	q6v5_wcss_halt_axi_port(wcss, wcss->halt_map, wcss->halt_wcss);
> +
> +	/* 2 - Enable WCSSAON_CONFIG */
> +	val = readl(wcss->rmb_base + SSCAON_CONFIG);
> +	val |= SSCAON_ENABLE;
> +	writel(val, wcss->rmb_base + SSCAON_CONFIG);
> +
> +	/* 3 - Set SSCAON_CONFIG */
> +	val |= BIT(15);
> +	val &= ~BIT(16);
> +	val &= ~BIT(17);
> +	val &= ~BIT(18);

wouldn't it be nice to define these bits?

> +static int q6v5_q6_powerdown(struct q6v5_wcss *wcss)
> +{
> +	int ret;
> +	u32 val;
> +	int i;

        int ret, i;
-- 
~Vinod