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
