On Thu, Oct 06, 2016 at 07:51:59AM -0700, Markus Mayer wrote:

On 5 October 2016 at 21:01, Viresh Kumar [off-list ref] wrote:

quoted

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.

quoted

quoted

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