Hi Daniel,
On Di, 2025-02-25 at 15:41 +0200, Daniel Baluta wrote:
> Hello Philipp,
>
> Thanks for your comments!
>
> > The DAP core reset is mentioned in the commit message. Why is it
> > missing here? After reading the discussion in [1], I'd expect both the
> > stall and the (core) reset signal to be documented, something like:
>
> There is no reset controller driver for DAP area yet.
This is about the device tree bindings, not the driver. Even if the
driver maps DAP address space with a hard-coded address, ...
> We manipulate the bits directly by remapping the DAP address space
> inside remoteproc driver.
... which it should not, the bindings could already correctly describe
the core reset.
I'd just like to make sure that there will be no confusion about the
stall "reset" signal, and adding a reset-names property would be an
easy way to do it.
> See for example: drivers/remoteproc/imx_dsp_rproc.c
>
> /* Reset function for DSP on i.MX8MP */
> static int imx8mp_dsp_reset(struct imx_dsp_rproc *priv)
> {
> » void __iomem *dap = ioremap_wc(IMX8M_DAP_DEBUG,
> IMX8M_DAP_DEBUG_SIZE);
> » int pwrctl;
>
> » /* Put DSP into reset and stall */
> » pwrctl = readl(dap + IMX8M_DAP_PWRCTL);
> » pwrctl |= IMX8M_PWRCTL_CORERESET;
> » writel(pwrctl, dap + IMX8M_DAP_PWRCTL);
>
>
> If we agree that this is the right way to go, the next step would be
> to create a new reset controller driver for DAP area.
>
> I want to keep this as a follow up patch in order to not compilate
> this patch series even more.
I have no issues with adding a reset driver for the DAP region and
hooking it up to the DSP driver in a separate series, but the device
tree bindings could be correct from the start.
> > Is there nothing else in this range that will have to be controlled by
> > the DSP driver in the future, such as the IMPWIRE register or the
> > XOCDMODE[OCDHALTONRESET] bit?
>
> We are internally running SOF for couple of years now and we didn't
> need any of these bits.
Ok.
regards
Philipp
