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:

quoted

<snip>

quoted

quoted

quoted

+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>

quoted

quoted

quoted

+
+	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>

quoted

quoted

quoted

+
+	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>

quoted

quoted

quoted

+
+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()?

quoted

+	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.

quoted

quoted

quoted

+
+	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