On Thu, Oct 06, 2016 at 07:51:59AM -0700, Markus Mayer wrote:
> On 5 October 2016 at 21:01, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> > Thanks for accepting all the comments :)
Unfortunately, I'll have to take back one agreed-upon change.
In this piece of code, brcm_avs_is_firmware_loaded() has to come after
requesting the IRQ.
priv->base = __map_region(BRCM_AVS_CPU_DATA);
if (!priv->base) {
dev_err(dev, "Couldn't find property %s in device tree.\n",
BRCM_AVS_CPU_DATA);
return -ENOENT;
}
priv->avs_intr_base = __map_region(BRCM_AVS_CPU_INTR);
if (!priv->avs_intr_base) {
dev_err(dev, "Couldn't find property %s in device tree.\n",
BRCM_AVS_CPU_INTR);
ret = -ENOENT;
goto unmap_base;
}
host_irq = platform_get_irq_byname(pdev, BRCM_AVS_HOST_INTR);
if (host_irq < 0) {
dev_err(dev, "Couldn't find interrupt %s -- %d\n",
BRCM_AVS_HOST_INTR, host_irq);
ret = host_irq;
goto unmap_intr_base;
}
ret = devm_request_irq(dev, host_irq, irq_handler, IRQF_TRIGGER_RISING,
BRCM_AVS_HOST_INTR, priv);
if (ret) {
dev_err(dev, "IRQ request failed: %s (%d) -- %d\n",
BRCM_AVS_HOST_INTR, host_irq, ret);
goto unmap_intr_base;
}
if (brcm_avs_is_firmware_loaded(priv))
return 0;
The reason being that we need to be ready to receive interrupts from the
co-processor to tell us the GET_PMAP command completed.
brcm_avs_is_firmware_loaded() issues the GET_PMAP command to ensure the
firmware supports it. If I try to run brcm_avs_is_firmware_loaded() before
setting up interrupts, I get a timeout in the driver -- because it never
receives the interrupt saying the command is done.
And I have to check the firmware supports the GET_PMAP command, because it
is possible for somebody to choose to run a firmware that does not support
DVFS, even though the hardware would support it.
> >> Is there an easy way for me to know via the framework whether init is
> >> being called for the first time vs. init is being called on a
> >> different core after a previous attempt to initialize on another core
> >> failed?
> >>
> >> I could use a driver-global variable for the driver to remember if it
> >> has been initialized, but that seems a bit hacky.
> >
> > You don't really need to have any special code here, specially for the case that
> > may never get hit. For example, if we fail to initialize something for CPU0,
> > cpufreq core will try calling this routine for other CPUs as well. I don't think
> > there is anything wrong in letting cpufreq core trying that. Why stop it or
> > return early? It wouldn't happen normally, unless there is a bug in there.
>
> During early development, when the driver couldn't fully register, I
> would see the init() function called four times, i.e. once for each
> core. If the first call succeeded, that was it. It would only get
> called once. But if it failed, all cores would try to register. And I
> wanted to avoid spilling the same error message four times.
>
> I'll look at that again. It may have had something to do with how the
> driver worked back then. If it doesn't happen anymore, I'll just get
> rid of this code.
Okay, I looked some more and compared it to what cpufreq-dt is doing to
initialize. That lead me onto the right track. They simply do things they
only want done once via the driver's probe function rather than CPUfreq's
init function. So, I broke my initialization code up into two parts.
Everything up to checking if the firmware is loaded is now being called from
brcm_avs_cpufreq_probe(). The frequency table code is still being called
from brcm_avs_cpufreq_init().
That means that if the initial hardware checks fail, it won't try again.
Regards,
-Markus
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
