Hi Andy,
On Fri, 2014-02-07 at 11:32 -0600, Andy Gross wrote:
> On Fri, Feb 07, 2014 at 11:52:33AM +0200, Ivan T. Ivanov wrote:
> >
<snip>
> > > > +static int spi_qup_transfer_do(struct spi_qup *controller,
> > > > + struct spi_qup_device *chip,
> > > > + struct spi_transfer *xfer)
> > > > +{
> > > > + unsigned long timeout;
> > > > + int ret = -EIO;
> > > > +
> > > > + reinit_completion(&controller->done);
> > > > +
> > > > + timeout = DIV_ROUND_UP(controller->speed_hz, MSEC_PER_SEC);
> > > > + timeout = DIV_ROUND_UP(xfer->len * 8, timeout);
> > > > + timeout = 100 * msecs_to_jiffies(timeout);
> > > > +
> > > > + controller->rx_bytes = 0;
> > > > + controller->tx_bytes = 0;
> > > > + controller->error = 0;
> > > > + controller->xfer = xfer;
> > > > +
> > > > + if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
> > > > + dev_warn(controller->dev, "cannot set RUN state\n");
> > > > + goto exit;
> > > > + }
> > > > +
> > > > + if (spi_qup_set_state(controller, QUP_STATE_PAUSE)) {
> > > > + dev_warn(controller->dev, "cannot set PAUSE state\n");
> > > > + goto exit;
> > > > + }
> > > > +
> > > > + spi_qup_fifo_write(controller, xfer);
> > > > +
> > > > + if (spi_qup_set_state(controller, QUP_STATE_RUN)) {
> > > > + dev_warn(controller->dev, "cannot set EXECUTE state\n");
> > > > + goto exit;
> > > > + }
> > > > +
> > > > + if (!wait_for_completion_timeout(&controller->done, timeout))
> > > > + ret = -ETIMEDOUT;
> > > > + else
> > > > + ret = controller->error;
> > > > +exit:
> > > > + controller->xfer = NULL;
> > >
> > > Should the manipulation of controller->xfer be protected by spinlock?
> >
> > :-). Probably. I am wondering, could I avoid locking if firstly place
> > QUP into RESET state and then access these field. This should stop
> > all activities in it, right?
>
> It's generally safest to not assume the hardware is going to do sane things.
> I'm concerned about spurious IRQs.
Ok, will add protection.
<snip>
> > > > +
> > > > + if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
> > > > + max_freq = 19200000;
> > >
> > > I'd set the default to 50MHz as that is the max supported by hardware. I'd just
> > > set max_freq declaration to 50MHz and then check the value if it is changed via
> > > DT.
> >
> > 50MHz doesn't seems to be supported on all chip sets. Currently common
> > denominator on all chip sets, that I can see, is 19.2MHz. I have tried
> > to test it with more than 19.2MHz on APQ8074 and it fails.
> >
>
> I guess my stance is to set it to the hardware max supported frequency if it is
> not specified. If that needs to be lower on a board because of whatever reason,
> they override it.
Ok, I will do in this way.
<snip>
> > >
> > > > +
> > > > + ret = clk_set_rate(cclk, max_freq);
> > > > + if (ret)
> > > > + dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);
> > >
> > > Bail here?
> >
> > I don't know. What will be the consequences if controller continue to
> > operate on its default rate?
> >
>
> It is unclear. But if you can't set the rate that is configured or if there is
> a misconfiguration, it's probably better to exit the probe and catch it here.
My preference is to delay clock speed change till first
SPI transfer. And use wherever transfer itself mandate.
<snip>
> > > > +
> > > > +static int spi_qup_remove(struct platform_device *pdev)
> > > > +{
> > > > + struct spi_master *master = dev_get_drvdata(&pdev->dev);
> > > > + struct spi_qup *controller = spi_master_get_devdata(master);
> > > > +
> > > > + pm_runtime_get_sync(&pdev->dev);
> > > > +
> > >
> > > Do we need to wait for any current transactions to complete
> > > and do a devm_free_irq()?
> > >
> > > > + clk_disable_unprepare(controller->cclk);
> > > > + clk_disable_unprepare(controller->iclk);
> >
> > My understanding is:
> >
> > Disabling clocks will timeout transaction, if any. Core Device driver
> > will call: devm_spi_unregister(), which will wait pending transactions
> > to complete and then remove the SPI master.
>
> Disabling clocks will confuse the hardware. We cannot disable clocks while the
> spi core is active and transferring data.
I could follow approach taken by other SPI drivers, just reset
controller and disable clocks.
>
> >
> > > > +
> > > > + pm_runtime_put_noidle(&pdev->dev);
> > > > + pm_runtime_disable(&pdev->dev);
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static struct of_device_id spi_qup_dt_match[] = {
> > > > + { .compatible = "qcom,spi-qup-v2", },
> > >
> > > Need compatible tags of qcom,spi-qup-v2.1.1 (msm8974 v1) or qcom,spi-qup-v2.2.1
> > > (msm8974 v2)
> >
> > I am not aware of the difference. My board report v.20020000.
> > Is there difference of handling these controllers?
>
> There were some bug fixes between versions. None of those affect SPI (that I
> can tell), but it's better to be more descriptive and use the full versions in
> the compatible tags.
No strong preference here. Should I add qcom,spi-qup-v2.2.0, then? :-)
Regards,
Ivan
--
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
