Hi Vinod,
On 6/5/2018 11:49 AM, Vinod wrote:
> 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
>
RPMSG_QCOM_GLINK_SMEM=n should be for the COMPILE_TEST. Probably that
means that it should be corrected here and for ADSP, Q6V5_PIL as well.
Bjorn, is that correct ?, should it be, below ?
depends on (RPMSG_QCOM_SMD || (COMPILE_TEST && RPMSG_QCOM_SMD=n)) || (RPMSG_QCOM_GLINK_SMEM || (COMPILE_TEST && RPMSG_QCOM_GLINK_SMEM=n))
>> +/* 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 :)
>
ok.
>> +/* 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?
>
ok.
>> +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??
>
ok, will find out what it is for this one and below.
But atleast from register map could not find out and
these are sort of hardcoded sequences coming from the hw
folks. So will have to reach out to them to find the specifics.
>> +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;
>
Regards,
Sricharan
--
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus
--
To unsubscribe from this list: send the line "unsubscribe linux-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
