Hi Bjorn,
Thanks for the review
On 03/19/2018 04:16 AM, Bjorn Andersson wrote:
On Wed 14 Mar 02:21 PDT 2018, Sibi S wrote:
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
[..]
+struct q6v5_reset_ops {
+ int (*reset_start)(struct q6v5 *qproc);
+ int (*reset_stop)(struct q6v5 *qproc);
+};
Please add these two ops directly in q6v5 instead and please keep the
naming "reset_assert" and "reset_deassert".
sure will do
+
enum {
MSS_MSM8916,
MSS_MSM8974,
@@ -343,6 +354,52 @@ static int q6v5_load(struct rproc *rproc, const struct firmware *fw)
return 0;
}
+static void alt_reset_restart(struct q6v5 *qproc, u32 restart)
+{
+ writel(restart, qproc->rmb_base + RMB_MBA_ALT_RESET);
Just move this write into q6v5_sdm_reset_start()
sure will do
+}
+
+static int q6v5_msm_reset_stop(struct q6v5 *qproc)
+{
+ return reset_control_assert(qproc->mss_restart);
+}
+
+static int q6v5_msm_reset_start(struct q6v5 *qproc)
+{
+ return reset_control_deassert(qproc->mss_restart);
+}
+
+static int q6v5_sdm_reset_stop(struct q6v5 *qproc)
+{
+ return reset_control_reset(qproc->mss_restart);
+}
+
+static int q6v5_sdm_reset_start(struct q6v5 *qproc)
+{
+ int ret;
+
+ alt_reset_restart(qproc, 1);
+ /* Ensure alt reset is written before restart reg */
That's not what udelay does ;)
If you want to make sure that the register is written and then give it
100us delay until you reset the reset you have to readl() the same
register back after the writel()
I think the delay deserves a comment on what we're waiting for.
the delay can be removed for the reasons stated below
+ udelay(100);
Use usleep_range() for delays longer than 10us.
+
+ ret = reset_control_reset(qproc->mss_restart);
+
+ udelay(100);
Same as the delay above, what are we waiting for?
The point is to wait for 6 32khz sleep cycles both after assert and
after de-assert of the aoss mss reset line. So moving the udelay to
the aoss reset controller should have been right thing to do. alt_reset
shouldn't need delays in between will remove them.
+ alt_reset_restart(qproc, 0);
+
+ return ret;
+}
+
[..]
@@ -1179,6 +1251,12 @@ static int q6v5_probe(struct platform_device *pdev)
qproc->dev = &pdev->dev;
qproc->rproc = rproc;
platform_set_drvdata(pdev, qproc);
+ qproc->version = desc->version;
+
+ if (qproc->version == MSS_SDM845)
+ qproc->ops = &q6v5_sdm_ops;
+ else
+ qproc->ops = &q6v5_msm_ops;
Can we carry a boolean for "has_alt_reset" or something that picks the
new reset ops, rather than picking by version?
Though alt_reset is specific to SDM845 SoCs it also includes another
reset controller (pdc_sync) in the modem bringup sequence, it isn't
included it in the patch series since it doesn't seem to affect the
modem start/stop functionality. Knowing that it may be added wouldn't
version be a better choice here?
Regards,
Bjorn
--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
