Re: [PATCH v3 2/2] remoteproc: qcom: Introduce Non-PAS ADSP PIL driver

[Rohit Kumar]

Thanks Sibi for reviewing.


On 9/22/2018 1:11 AM, Sibi Sankar wrote:
> Hi Rohit,
>
> On 2018-09-03 17:22, Rohit kumar wrote:
> > This adds Non PAS ADSP PIL driver for Qualcomm
> > Technologies Inc SoCs.
> > Added initial support for SDM845 with ADSP bootup and
> > shutdown operation handled from Application Processor
> > SubSystem(APSS).
> >
> > Signed-off-by: Rohit kumar <rohitkr@xxxxxxxxxxxxxx>
> > ---
> ....
> > +ÂÂÂ select QCOM_MDT_LOADER
> > +ÂÂÂ select QCOM_Q6V5_COMMON
> > +ÂÂÂ select QCOM_RPROC_COMMON
> > +ÂÂÂ help
> > +ÂÂÂÂÂ Say y here to support the Peripherial Image Loader
>
> replace Peripherial/Peripheral.

sure
> ....
> > +#include <linux/clk.h>
> > +#include <linux/firmware.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/io.h>
> > +#include <linux/reset.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/pm_domain.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/remoteproc.h>
> > +#include <linux/soc/qcom/mdt_loader.h>
> > +#include <linux/soc/qcom/smem.h>
> > +#include <linux/soc/qcom/smem_state.h>
> > +
>
> The headers should be in alphabetical order.

ok, will do
> ....
> > +ÂÂÂ struct reset_control *pdc_sync_reset;
> > +ÂÂÂ struct reset_control *cc_lpass_restart;
> > +
> > +ÂÂÂ struct regmap *halt_map;
> > + unsigned int halt_lpass;
>
> remove the double spaces above.

ok
> ....
> > +ÂÂÂ if (ret || val)
> > +ÂÂÂÂÂÂÂ goto reset;
> > +
> > +ÂÂÂ regmap_write(adsp->halt_map,
> > +ÂÂÂÂÂÂÂÂÂÂÂ adsp->halt_lpass + LPASS_HALTREQ_REG, 1);
> > +
> > +ÂÂÂ /*Â Wait for halt ACK from QDSP6 */
>
> Minor nit, please remove the double spaces in the comment above.

ok
> > +ÂÂÂ timeout = jiffies + msecs_to_jiffies(ACK_TIMEOUT);
> > +ÂÂÂ for (;;) {
> > +ÂÂÂÂÂÂÂ ret = regmap_read(adsp->halt_map,
> > +ÂÂÂÂÂÂÂÂÂÂÂ adsp->halt_lpass + LPASS_HALTACK_REG, &val);
> > +ÂÂÂÂÂÂÂ if (ret || val || time_after(jiffies, timeout))
> > +ÂÂÂÂÂÂÂÂÂÂÂ break;
> > +
> > +ÂÂÂÂÂÂÂ udelay(1000);
>
> According to Documentation/timers/timers-howto.txt please use 
> usleep_range()
> when the delay range is between(10us - 20ms).

okay, will update in next spin.
> > +ÂÂÂ }
> > +
> > +ÂÂÂ ret = regmap_read(adsp->halt_map,
> > +ÂÂÂÂÂÂÂÂÂÂÂ adsp->halt_lpass + LPASS_MASTER_IDLE_REG, &val);
> ....
> > +ÂÂÂ /* wait after asserting subsystem restart from AOSS */
> > +ÂÂÂ udelay(200);
>
> ditto

ok
> > +
> > +ÂÂÂ /* Clear the halt request for the AXIM and AHBM for Q6 */
> > +ÂÂÂ regmap_write(adsp->halt_map, adsp->halt_lpass + 
> > LPASS_HALTREQ_REG, 0);
> > +
> > +ÂÂÂ /* De-assert the LPASS PDC Reset */
> > +ÂÂÂ reset_control_deassert(adsp->pdc_sync_reset);
> > +ÂÂÂ /* Remove the LPASS reset */
> > +ÂÂÂ reset_control_deassert(adsp->cc_lpass_restart);
> > +ÂÂÂ /* wait after de-asserting subsystem restart from AOSS */
> > +ÂÂÂ udelay(200);
>
> ditto
>
> > +
> > +ÂÂÂ return 0;
> > +}
> ....
> > +static int adsp_start(struct rproc *rproc)
> > +{
> > +ÂÂÂ struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
> > +ÂÂÂ int ret;
> > +ÂÂÂ unsigned int val;
> > +
> > +ÂÂÂ qcom_q6v5_prepare(&adsp->q6v5);
> > +
> > +ÂÂÂ ret = clk_prepare_enable(adsp->xo);
> > +ÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂ return ret;
>
> please call qcom_q6v5_unprepare on clk_prepare_enable failure to 
> disable_irqs

sure, will do that
> > +
> > +ÂÂÂ dev_pm_genpd_set_performance_state(adsp->dev, INT_MAX);
> > +ÂÂÂ ret = pm_runtime_get_sync(adsp->dev);
> > +ÂÂÂ if (ret)
> > +ÂÂÂÂÂÂÂ goto disable_xo_clk;
> > +
> ....
> > +static int adsp_init_reset(struct qcom_adsp *adsp)
> > +{
> > +ÂÂÂ adsp->pdc_sync_reset = devm_reset_control_get_exclusive(adsp->dev,
> > +ÂÂÂÂÂÂÂÂÂÂÂ "pdc_sync");
> > +ÂÂÂ if (IS_ERR(adsp->pdc_sync_reset)) {
> > +ÂÂÂÂÂÂÂ dev_err(adsp->dev, "failed to acquire pdc_sync reset\n");
> > +ÂÂÂÂÂÂÂ return PTR_ERR(adsp->pdc_sync_reset);
> > +ÂÂÂ }
>
> Bjorn, should we return EPROBE_DEFER here since PDC global reset 
> controller can be a module?

devm_reset_control_get_exclusive itself returns EPROBE_DEFER until PDC 
reset driver is probed.
return PTR_ERR(adsp->pdc_sync_reset) handles this case.

> > +
> > +ÂÂÂ adsp->cc_lpass_restart = 
> > devm_reset_control_get_exclusive(adsp->dev,
> > +ÂÂÂÂÂÂÂÂÂÂÂ "cc_lpass");
> > +ÂÂÂ if (IS_ERR(adsp->cc_lpass_restart)) {
> > +ÂÂÂÂÂÂÂ dev_err(adsp->dev, "failed to acquire cc_lpass restart\n");
> > +ÂÂÂÂÂÂÂ return PTR_ERR(adsp->cc_lpass_restart);
> > +ÂÂÂ }
> > +
> > +ÂÂÂ return 0;
> ....
> > +static int adsp_remove(struct platform_device *pdev)
> > +{
> > +ÂÂÂ struct qcom_adsp *adsp = platform_get_drvdata(pdev);
> > +
> > +ÂÂÂ rproc_del(adsp->rproc);
> > +
> > +ÂÂÂ qcom_remove_glink_subdev(adsp->rproc, &adsp->glink_subdev);
> > +ÂÂÂ qcom_remove_sysmon_subdev(adsp->sysmon);
> > +ÂÂÂ qcom_remove_ssr_subdev(adsp->rproc, &adsp->ssr_subdev);
> > +ÂÂÂ rproc_free(adsp->rproc);
> > +ÂÂÂ pm_runtime_disable(adsp->dev);
> > +
>
> rmmod of the driver resulted in the following kernel panic:
> having a pm_runtime_disable after rproc_free seems to be the cause of 
> the kernel panic.
> Please call pm_runtime_disable before rproc_free.

Thanks for pointing out, will update.
> do_raw_spin_lock+0x28/0x118
> __raw_spin_lock_irq+0x30/0x3c
> __pm_runtime_disable+0x28/0xf4
> adsp_remove+0x4c/0x5c [qcom_adsp_pil]
> platform_drv_remove+0x28/0x50
> device_release_driver_internal+0x124/0x1c8
> driver_detach+0x44/0x80
> bus_remove_driver+0x78/0x9c
> driver_unregister+0x34/0x54
> platform_driver_unregister+0x1c/0x28
> cleanup_module+0x14/0x6bc [qcom_adsp_pil]
> SyS_delete_module+0x1c4/0x214
>
> > +ÂÂÂ return 0;
> > +}
> > +
> > +static const struct adsp_pil_data adsp_resource_init = {
> > +ÂÂÂ .crash_reason_smem = 423,
> > +ÂÂÂ .firmware_name = "adsp.mdt",
> > +ÂÂÂ .ssr_name = "lpass",
> > +ÂÂÂ .sysmon_name = "adsp",
> > +ÂÂÂ .ssctl_id = 0x14,
> > +};
> ....
> > +module_platform_driver(adsp_pil_driver);
> > +MODULE_DESCRIPTION("QTi SDM845 ADSP Peripherial Image Loader");
>
> replace QTi/QTI and Peripherial/Peripheral.
ok
> ....
> Also I see the following warns on stopping the adsp remoteproc, 
> couldn't root cause it though:

It should be issue in Q6 drivers. I will check and update q6 drivers. 
Thanks for reporting.

> Âdevice_del+0x84/0x29c
> Âplatform_device_del+0x2c/0x88
> Âplatform_device_unregister+0x1c/0x30
> Âof_platform_device_destroy+0x98/0xa8
> Âdevice_for_each_child+0x54/0xa4
> Âof_platform_depopulate+0x38/0x54
> Âq6asm_remove+0x1c/0x2c
> Âapr_device_remove+0x38/0x70
> Âdevice_release_driver_internal+0x124/0x1c8
> Âdevice_release_driver+0x24/0x30
> Âbus_remove_device+0xcc/0xe4
> Âdevice_del+0x1f8/0x29c
> Âdevice_unregister+0x1c/0x30
> Âapr_remove_device+0x1c/0x2c
> Âdevice_for_each_child+0x54/0xa4
> Âapr_remove+0x28/0x34
> Ârpmsg_dev_remove+0x48/0x70
> Âdevice_release_driver_internal+0x124/0x1c8
> Âdevice_release_driver+0x24/0x30
> Âbus_remove_device+0xcc/0xe4
> Âdevice_del+0x1f8/0x29c
> Âdevice_unregister+0x1c/0x30
> Âqcom_glink_remove_device+0x1c/0x2c
> Âdevice_for_each_child+0x54/0xa4
> Âqcom_glink_native_remove+0x54/0x15c
> Âqcom_glink_smem_unregister+0x1c/0x30
> Âglink_subdev_stop+0x1c/0x2c [qcom_common]
> Ârproc_stop+0x40/0xc0
> Ârproc_shutdown+0x6c/0xc0
> Ârproc_del+0x28/0xa0
> Âadsp_remove+0x20/0x5c [qcom_adsp_pil]
> Âplatform_drv_remove+0x28/0x50
> Âdevice_release_driver_internal+0x124/0x1c8
> Âdriver_detach+0x44/0x80
> Âbus_remove_driver+0x78/0x9c
> Âdriver_unregister+0x34/0x54
> Âplatform_driver_unregister+0x1c/0x28
> Âcleanup_module+0x14/0x6bc [qcom_adsp_pil]
> ÂSyS_delete_module+0x1c4/0x214

Thanks,
Rohit