From: Rob Herring <hidden> Date: 2014-03-17 12:47:32
On Mon, Mar 17, 2014 at 7:05 AM, Harini Katakam [off-list ref] wrote:
Add driver for Cadence SPI controller. This is used in Xilinx Zynq.
Signed-off-by: Harini Katakam <redacted>
---
.../devicetree/bindings/spi/spi-cadence.txt | 25 +
@@ -0,0 +1,25 @@+Cadence SPI controller Device Tree Bindings+-------------------------------------------++Required properties:+- compatible : Should be "cdns,spi-r1p6".
One problem with using IP vendor info in the compatible string is an
IP block typically has a variety of configuration options so the
actual implementations may be very different. I would recommend adding
a Xilinx compatible string as well even if you don't use it now.
+- reg : Physical base address and size of SPI registers map.
+- interrupts : Property with a value describing the interrupt
+ number.
+- interrupt-parent : Must be core interrupt controller
+- clock-names : List of input clock names - "ref_clk", "pclk"
+ (See clock bindings for details).
+- clocks : Clock phandles (see clock bindings for details).
+- num-chip-select : Number of chip selects used.
See Documentation/devicetree/bindings/spi/spi-bus.txt. Use "num-cs" here.
From: Mark Brown <broonie@kernel.org> Date: 2014-03-17 13:15:20
On Mon, Mar 17, 2014 at 07:47:24AM -0500, Rob Herring wrote:
Please delete irrelevant context from your replies, it makes it easier
to find the new content.
On Mon, Mar 17, 2014 at 7:05 AM, Harini Katakam [off-list ref] wrote:
quoted
+/* Macros for the SPI controller read/write */
+#define cdns_spi_read(addr) readl_relaxed(addr)
+#define cdns_spi_write(addr, val) writel_relaxed((val), (addr))
Just use readl/writel directly.
Or make them static inline structures which take the driver data
structure and an offset within the register map for the device and do
the maths to resolve the actual address.
+/* Work with hotplug and coldplug */
+MODULE_ALIAS("platform:" CDNS_SPI_NAME);
Not sure, but I don't think this should be needed.
It won't be used by DT systems but it is good practice, especially with
something like this that clearly is a generic IP which could be used on
non-DT systems too (it's not like it costs anything meaningful).
From: Michal Simek <monstr@monstr.eu> Date: 2014-03-17 13:23:08
Hi Rob,
On 03/17/2014 01:47 PM, Rob Herring wrote:
On Mon, Mar 17, 2014 at 7:05 AM, Harini Katakam [off-list ref] wrote:
quoted
Add driver for Cadence SPI controller. This is used in Xilinx Zynq.
Signed-off-by: Harini Katakam <redacted>
---
.../devicetree/bindings/spi/spi-cadence.txt | 25 +
We prefer binding docs in separate patch.
I have heard several times that also for binding review you need driver
to look if this binding make sense that's why Harini sent this together.
It means 2 emails instead of one.
But if you wish to have this in two files no problem to split it
but then I believe both should be copied to DT mailing list.
@@ -0,0 +1,25 @@+Cadence SPI controller Device Tree Bindings+-------------------------------------------++Required properties:+- compatible : Should be "cdns,spi-r1p6".
One problem with using IP vendor info in the compatible string is an
IP block typically has a variety of configuration options so the
actual implementations may be very different. I would recommend adding
a Xilinx compatible string as well even if you don't use it now.
It means xlnx,zynq-spi-r1p6 should be fine.
quoted
+- reg : Physical base address and size of SPI registers map.
+- interrupts : Property with a value describing the interrupt
+ number.
+- interrupt-parent : Must be core interrupt controller
+- clock-names : List of input clock names - "ref_clk", "pclk"
+ (See clock bindings for details).
+- clocks : Clock phandles (see clock bindings for details).
+- num-chip-select : Number of chip selects used.
See Documentation/devicetree/bindings/spi/spi-bus.txt. Use "num-cs" here.
I was expecting you will comment this a little bit. :-)
Because all just reading this num-cs as 32bit and then
assigning this value to master->num_chipselect which is 16bit.
<snip>
quoted
+/* Macros for the SPI controller read/write */
+#define cdns_spi_read(addr) readl_relaxed(addr)
+#define cdns_spi_write(addr, val) writel_relaxed((val), (addr))
Just use readl/writel directly.
There shouldn't be any problem to use these helper macros
and even I think this is better than using readl/writel directly
because you are more flexible if you want to change it to different
IO.
<snip>
quoted
+/**
+ * cdns_spi_probe - Probe method for the SPI driver
+ * @pdev: Pointer to the platform_device structure
+ *
+ * This function initializes the driver data structures and the hardware.
+ *
+ * Return: 0 on success and error value on error
+ */
+static int cdns_spi_probe(struct platform_device *pdev)
+{
+ int ret = 0, irq;
+ struct spi_master *master;
+ struct cdns_spi *xspi;
+ struct resource *res;
+
+ master = spi_alloc_master(&pdev->dev, sizeof(*xspi));
+ if (master == NULL)
+ return -ENOMEM;
+
+ xspi = spi_master_get_devdata(master);
+ master->dev.of_node = pdev->dev.of_node;
+ platform_set_drvdata(pdev, master);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ xspi->regs = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(xspi->regs)) {
+ ret = PTR_ERR(xspi->regs);
+ goto remove_master;
+ }
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
I believe this returns NO_IRQ which could be 0.
s/</<=/
Are you sure regarding this?
NO_IRQ on ARM is -1.
And IRC irq = 0 should be just valid number.
But if you are right then others drivers have to fixed too.
quoted
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(cdns_spi_dev_pm_ops, cdns_spi_suspend,
+ cdns_spi_resume);
+
+/* Work with hotplug and coldplug */
+MODULE_ALIAS("platform:" CDNS_SPI_NAME);
Not sure, but I don't think this should be needed.
I don't know too.
Thanks for review,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
On Mon, Mar 17, 2014 at 2:22 PM, Michal Simek [off-list ref] wrote:
quoted
quoted
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(cdns_spi_dev_pm_ops, cdns_spi_suspend,
+ cdns_spi_resume);
+
+/* Work with hotplug and coldplug */
+MODULE_ALIAS("platform:" CDNS_SPI_NAME);
Not sure, but I don't think this should be needed.
I don't know too.
A plain platform device driver needs the MODULE_ALIAS, unless there's
a "MODULE_DEVICE_TABLE(platform, ...)", which doesn't exist in this driver.
However, as the driver fails in the absence of DT:
ret = of_property_read_u16(pdev->dev.of_node, "num-chip-select",
&master->num_chipselect);
if (ret < 0) {
dev_err(&pdev->dev, "couldn't determine num-chip-select\n");
goto clk_dis_all;
}
it won't work (yet) as a plain platform device driver.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
-----Original Message-----
From: Michal Simek [mailto:monstr@monstr.eu]
Sent: Monday, March 17, 2014 6:53 PM
To: Rob Herring
Cc: Harini Katakam; Rob Herring; Pawel Moll; Mark Rutland; Ian Campbell;
Kumar Gala; Rob Landley; Mark Brown; Grant Likely;
devicetree@vger.kernel.org; linux-doc@vger.kernel.org; linux-
kernel@vger.kernel.org; linux-spi@vger.kernel.org
Subject: Re: [PATCH] SPI: Add driver for Cadence SPI controller
Hi Rob,
On 03/17/2014 01:47 PM, Rob Herring wrote:
quoted
On Mon, Mar 17, 2014 at 7:05 AM, Harini Katakam [off-list ref]
wrote:
quoted
quoted
Add driver for Cadence SPI controller. This is used in Xilinx Zynq.
Signed-off-by: Harini Katakam <redacted>
---
.../devicetree/bindings/spi/spi-cadence.txt | 25 +
We prefer binding docs in separate patch.
I have heard several times that also for binding review you need driver to
look if this binding make sense that's why Harini sent this together.
It means 2 emails instead of one.
But if you wish to have this in two files no problem to split it but then I
believe both should be copied to DT mailing list.
@@ -0,0 +1,25 @@+Cadence SPI controller Device Tree Bindings+-------------------------------------------++Required properties:+- compatible : Should be "cdns,spi-r1p6".
One problem with using IP vendor info in the compatible string is an
IP block typically has a variety of configuration options so the
actual implementations may be very different. I would recommend adding
a Xilinx compatible string as well even if you don't use it now.
It means xlnx,zynq-spi-r1p6 should be fine.
I can add this string. "r1p6" is specific to the cadence IP used.
Is that OK?
quoted
quoted
+- reg : Physical base address and size of SPI registers map.
+- interrupts : Property with a value describing the interrupt
+ number.
+- interrupt-parent : Must be core interrupt controller
+- clock-names : List of input clock names - "ref_clk", "pclk"
+ (See clock bindings for details).
+- clocks : Clock phandles (see clock bindings for details).
+- num-chip-select : Number of chip selects used.
See Documentation/devicetree/bindings/spi/spi-bus.txt. Use "num-cs"
I was expecting you will comment this a little bit. :-) Because all just reading
this num-cs as 32bit and then assigning this value to master-
quoted
num_chipselect which is 16bit.
<snip>
quoted
quoted
+/* Macros for the SPI controller read/write */
+#define cdns_spi_read(addr) readl_relaxed(addr)
+#define cdns_spi_write(addr, val) writel_relaxed((val), (addr))
Just use readl/writel directly.
There shouldn't be any problem to use these helper macros and even I think
this is better than using readl/writel directly because you are more flexible
if you want to change it to different IO.
Like Michal pointed out, this is more flexible.
Can try what Mark suggested:
"Or make them static inline structures which take the driver data structure and an
offset within the register map for the device and do the maths to resolve the actual address."
Thanks for the review.
Regards,
Harini
This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
-----Original Message-----
From: Rob Herring [mailto:robherring2@gmail.com]
Sent: Monday, March 17, 2014 6:17 PM
To: Harini Katakam
Cc: Rob Herring; Pawel Moll; Mark Rutland; Ian Campbell; Kumar Gala; Rob
Landley; Mark Brown; Grant Likely; devicetree@vger.kernel.org; linux-
doc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
spi@vger.kernel.org; Michal Simek
Subject: Re: [PATCH] SPI: Add driver for Cadence SPI controller
On Mon, Mar 17, 2014 at 7:05 AM, Harini Katakam [off-list ref]
wrote:
quoted
Add driver for Cadence SPI controller. This is used in Xilinx Zynq.
Signed-off-by: Harini Katakam <redacted>
---
quoted
+- reg : Physical base address and size of SPI registers map.
+- interrupts : Property with a value describing the interrupt
+ number.
+- interrupt-parent : Must be core interrupt controller
+- clock-names : List of input clock names - "ref_clk", "pclk"
+ (See clock bindings for details).
+- clocks : Clock phandles (see clock bindings for details).
+- num-chip-select : Number of chip selects used.
See Documentation/devicetree/bindings/spi/spi-bus.txt. Use "num-cs"
here.
I believe this returns NO_IRQ which could be 0.
s/</<=/
quoted
+ dev_dbg(&pdev->dev, "suspend succeeded\n");
Not needed. The kernel already has suspend/resume tracing printks.
Will remove.
quoted
+ dev_dbg(&pdev->dev, "resume succeeded\n");
Not needed. The kernel already has suspend/resume tracing printks.
Will remove.
Regards,
Harini
This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
From: Mark Brown <broonie@kernel.org> Date: 2014-03-17 17:30:49
On Mon, Mar 17, 2014 at 05:35:36PM +0530, Harini Katakam wrote:
+ bits_per_word = transfer ?
+ transfer->bits_per_word : spi->bits_per_word;
This would be a lot more legible without the ternery operator.
+ if (bits_per_word != 8) {
+ dev_err(&spi->dev, "%s, unsupported bits per word %x\n",
+ __func__, spi->bits_per_word);
+ return -EINVAL;
+ }
The core will check this for you.
+static int cdns_spi_setup(struct spi_device *spi)
+{
+ if (!spi->max_speed_hz)
+ return -EINVAL;
+
+ if (!spi->bits_per_word)
+ spi->bits_per_word = 8;
The core does this for you.
+ return cdns_spi_setup_transfer(spi, NULL);
+}
It's not clear to me why this has been split into a separate function
and the function will write to the hardware which you're not allowed to
do in setup() if it might affect an ongoing transfer.
+ intr_status = cdns_spi_read(xspi->regs + CDNS_SPI_ISR_OFFSET);
+ cdns_spi_write(xspi->regs + CDNS_SPI_ISR_OFFSET, intr_status);
+
+ if (intr_status & CDNS_SPI_IXR_MODF_MASK) {
+ /* Indicate that transfer is completed, the SPI subsystem will
+ * identify the error as the remaining bytes to be
+ * transferred is non-zero
+ */
+ cdns_spi_write(xspi->regs + CDNS_SPI_IDR_OFFSET,
+ CDNS_SPI_IXR_DEFAULT_MASK);
+ complete(&xspi->done);
+ } else if (intr_status & CDNS_SPI_IXR_TXOW_MASK) {
What happens if both interrupts are flagged at the same time?
+ if (xspi->remaining_bytes) {
+ /* There is more data to send */
+ cdns_spi_fill_tx_fifo(xspi);
+ } else {
+ /* Transfer is completed */
+ cdns_spi_write(xspi->regs + CDNS_SPI_IDR_OFFSET,
+ CDNS_SPI_IXR_DEFAULT_MASK);
+ complete(&xspi->done);
+ }
+ }
I see from further up the file that there are error interrupt states
which might be flagged but these are just being ignored.
+
+ return IRQ_HANDLED;
This should only be returned if an interrupt was actually handled - if
the line is shared in some system this will break.
+ cdns_spi_write(xspi->regs + CDNS_SPI_IER_OFFSET,
+ CDNS_SPI_IXR_DEFAULT_MASK);
+
+ ret = wait_for_completion_interruptible_timeout(&xspi->done,
+ CDNS_SPI_TIMEOUT);
+ if (ret < 1) {
If you return a positive value from transfer_one() the core will wait
for you.
You probably want to enable the clocks here (and disable them when
unpreparing too).
+static int cdns_transfer_one_message(struct spi_master *master,
+ struct spi_message *msg)
+{
Just implement transfer_one() and provide a set_cs() operation and most
of this code will go away.
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ ret = -ENXIO;
+ dev_err(&pdev->dev, "irq number is negative\n");
+ goto remove_master;
+ }
+
+ ret = devm_request_irq(&pdev->dev, irq, cdns_spi_irq,
+ 0, pdev->name, xspi);
+ if (ret != 0) {
+ ret = -ENXIO;
+ dev_err(&pdev->dev, "request_irq failed\n");
+ goto remove_master;
+ }
I'd expect to see something that initialises the hardware prior to
requesting the interrupt, you don't know what state the hardware is in
when the driver takes control.
+ init_completion(&xspi->done);
This needs to be done prior to the interrupt as well.
Don't do this, the spi_master_suspend() call will quiesce transfers (or
if open coding it should be doing something similar rather than just
breaking any ongoing transfer).
On Mon, Mar 17, 2014 at 05:30:17PM +0000, Mark Brown wrote:
On Mon, Mar 17, 2014 at 05:35:36PM +0530, Harini Katakam wrote:
[..]
quoted
+static int __maybe_unused cdns_spi_suspend(struct device *dev)
+{
This needs to call spi_master_suspend() as well (and similarly on
resume).
I'm not that familiar with the SPI core, but this seems like an
inversion. Is there a reason why the SPI master class doesn't implement
suspend/resume() callbacks which handle stopping/starting the queue
automatically for all masters?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
From: Mark Brown <broonie@kernel.org> Date: 2014-03-17 18:15:00
On Mon, Mar 17, 2014 at 12:59:11PM -0500, Josh Cartwright wrote:
On Mon, Mar 17, 2014 at 05:30:17PM +0000, Mark Brown wrote:
quoted
On Mon, Mar 17, 2014 at 05:35:36PM +0530, Harini Katakam wrote:
quoted
quoted
+static int __maybe_unused cdns_spi_suspend(struct device *dev)
+{
quoted
This needs to call spi_master_suspend() as well (and similarly on
resume).
I'm not that familiar with the SPI core, but this seems like an
inversion. Is there a reason why the SPI master class doesn't implement
suspend/resume() callbacks which handle stopping/starting the queue
automatically for all masters?
This is for users of an optional feature of the infrastructure. We
probably should just call it anyway since it does have checks for the
feature being used (but given all the open coding around this stuff I'd
need to verify that the class callbacks would reliably get called).
In any case that's not happening now and the driver as it stands is
buggy since it's trampling all over the hardware without syncing with
anything that isn't ongoing.
From: Rob Herring <hidden> Date: 2014-03-17 19:00:09
On Mon, Mar 17, 2014 at 8:22 AM, Michal Simek [off-list ref] wrote:
Hi Rob,
On 03/17/2014 01:47 PM, Rob Herring wrote:
quoted
On Mon, Mar 17, 2014 at 7:05 AM, Harini Katakam [off-list ref] wrote:
quoted
Add driver for Cadence SPI controller. This is used in Xilinx Zynq.
Signed-off-by: Harini Katakam <redacted>
---
.../devicetree/bindings/spi/spi-cadence.txt | 25 +
We prefer binding docs in separate patch.
I have heard several times that also for binding review you need driver
to look if this binding make sense that's why Harini sent this together.
It means 2 emails instead of one.
But if you wish to have this in two files no problem to split it
but then I believe both should be copied to DT mailing list.
Yes, for 2 reasons:
- To prepare for DT bindings to get merged to separate repo if we ever
get there.
- So DT maintainers can review/ack just the bindings.
quoted
quoted
+- reg : Physical base address and size of SPI registers map.
+- interrupts : Property with a value describing the interrupt
+ number.
+- interrupt-parent : Must be core interrupt controller
+- clock-names : List of input clock names - "ref_clk", "pclk"
+ (See clock bindings for details).
+- clocks : Clock phandles (see clock bindings for details).
+- num-chip-select : Number of chip selects used.
See Documentation/devicetree/bindings/spi/spi-bus.txt. Use "num-cs" here.
I was expecting you will comment this a little bit. :-)
Because all just reading this num-cs as 32bit and then
assigning this value to master->num_chipselect which is 16bit.
Well, everyone else has that problem then. Obviously it takes a bit
more care than just reading into a u32, but that is a kernel problem
and not a problem of the binding.
quoted
quoted
+/* Macros for the SPI controller read/write */
+#define cdns_spi_read(addr) readl_relaxed(addr)
+#define cdns_spi_write(addr, val) writel_relaxed((val), (addr))
Just use readl/writel directly.
There shouldn't be any problem to use these helper macros
and even I think this is better than using readl/writel directly
because you are more flexible if you want to change it to different
IO.
Then when I read the code I have to go lookup what they do while I
know exactly what writel/readl do already. It is really the same
reasons as why the kernel doesn't have register set and clear bits
accessors.
I believe this returns NO_IRQ which could be 0.
s/</<=/
Are you sure regarding this?
NO_IRQ on ARM is -1.
And IRC irq = 0 should be just valid number.
But if you are right then others drivers have to fixed too.
The definition varies by arch being 0 or -1, so drivers need to deal
with both. The preference is 0 is NO_IRQ. It has been decreed by
Linus. ARM is actually pretty close to being able to change NO_IRQ to
0.
Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
-----Original Message-----
From: Mark Brown [mailto:broonie@kernel.org]
Sent: Monday, March 17, 2014 11:00 PM
To: Harini Katakam
Cc: robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com;
ijc+devicetree@hellion.org.uk; galak@codeaurora.org; rob@landley.net;
grant.likely@linaro.org; devicetree@vger.kernel.org; linux-
doc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
spi@vger.kernel.org; Michal Simek
Subject: Re: [PATCH] SPI: Add driver for Cadence SPI controller
On Mon, Mar 17, 2014 at 05:35:36PM +0530, Harini Katakam wrote:
quoted
+ bits_per_word = transfer ?
+ transfer->bits_per_word : spi->bits_per_word;
This would be a lot more legible without the ternery operator.
quoted
+ if (bits_per_word != 8) {
+ dev_err(&spi->dev, "%s, unsupported bits per word %x\n",
+ __func__, spi->bits_per_word);
+ return -EINVAL;
+ }
The core will check this for you.
I dint find that the core does this.
quoted
+static int cdns_spi_setup(struct spi_device *spi) {
+ if (!spi->max_speed_hz)
+ return -EINVAL;
+
+ if (!spi->bits_per_word)
+ spi->bits_per_word = 8;
The core does this for you.
I understand that the core does this.
quoted
+ return cdns_spi_setup_transfer(spi, NULL); }
It's not clear to me why this has been split into a separate function and the
function will write to the hardware which you're not allowed to do in
setup() if it might affect an ongoing transfer.
Are you saying that there's a chance cdns_spi_setup() will be called when there's an ongoing transfer?
In that case I have to remove the cdns_setup_transfer() call from here but then there's nothing left to do in setup.
The function was split into two because cdns_spi_setup_transfer() is called internally
by transfer_one_message() everytime.
Is it always possible that the spi_transfer paramters will remain the same;
In that case this call is probably not necessary in transfer_one_message.
quoted
+ intr_status = cdns_spi_read(xspi->regs + CDNS_SPI_ISR_OFFSET);
+ cdns_spi_write(xspi->regs + CDNS_SPI_ISR_OFFSET, intr_status);
+
+ if (intr_status & CDNS_SPI_IXR_MODF_MASK) {
+ /* Indicate that transfer is completed, the SPI subsystem will
+ * identify the error as the remaining bytes to be
+ * transferred is non-zero
+ */
+ cdns_spi_write(xspi->regs + CDNS_SPI_IDR_OFFSET,
+ CDNS_SPI_IXR_DEFAULT_MASK);
+ complete(&xspi->done);
+ } else if (intr_status & CDNS_SPI_IXR_TXOW_MASK) {
What happens if both interrupts are flagged at the same time?
The MODF is an error interrupt and so if TXOW is raised along with it,
TXOW will be ignored (it will be cleared but no data is read).
Completion is indicated and since the remaining bytes is non-zero,
The transfer function will understand that it is an error.
quoted
+ if (xspi->remaining_bytes) {
+ /* There is more data to send */
+ cdns_spi_fill_tx_fifo(xspi);
+ } else {
+ /* Transfer is completed */
+ cdns_spi_write(xspi->regs + CDNS_SPI_IDR_OFFSET,
+ CDNS_SPI_IXR_DEFAULT_MASK);
+ complete(&xspi->done);
+ }
+ }
I see from further up the file that there are error interrupt states which
might be flagged but these are just being ignored.
I'm not sure I understand what you are referring to -
The only interrupts enabled (See CNDS_IXR_DEFAULT_MASK) are TXOW and MODF.
quoted
+
+ return IRQ_HANDLED;
This should only be returned if an interrupt was actually handled - if the line
is shared in some system this will break.
In this case both possible interrupt conditions are handled.
quoted
+ cdns_spi_write(xspi->regs + CDNS_SPI_IER_OFFSET,
+ CDNS_SPI_IXR_DEFAULT_MASK);
+
+ ret = wait_for_completion_interruptible_timeout(&xspi->done,
+ CDNS_SPI_TIMEOUT);
+ if (ret < 1) {
If you return a positive value from transfer_one() the core will wait for you.
I don't understand. Here, if the ret from wait_for_completion_interruptible_timeout is
positive, then this function (spi_start_transfer) will return (transfer->len) - (xspi->remaining_bytes)
to cdns_transfer_one_message(the calling funtion). Which will just use this return as
information on how many bytes were transferred (see variable length).
cdns_transfer_one_messag() only returns 0 or error value (-ve) (see variable status).
It doesn't return positive value to core.
You probably want to enable the clocks here (and disable them when
unpreparing too).
OK
quoted
+static int cdns_transfer_one_message(struct spi_master *master,
+ struct spi_message *msg)
+{
Just implement transfer_one() and provide a set_cs() operation and most of
this code will go away.
I dint realise there was a hook for set_cs(). Yeah I will simplify this.
quoted
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0) {
+ ret = -ENXIO;
+ dev_err(&pdev->dev, "irq number is negative\n");
+ goto remove_master;
+ }
+
+ ret = devm_request_irq(&pdev->dev, irq, cdns_spi_irq,
+ 0, pdev->name, xspi);
+ if (ret != 0) {
+ ret = -ENXIO;
+ dev_err(&pdev->dev, "request_irq failed\n");
+ goto remove_master;
+ }
I'd expect to see something that initialises the hardware prior to requesting
the interrupt, you don't know what state the hardware is in when the driver
takes control.
quoted
+ init_completion(&xspi->done);
This needs to be done prior to the interrupt as well.
I will move the devm_request_irq here.
quoted
+ ret = of_property_read_u16(pdev->dev.of_node, "num-chip-select",
+ &master->num_chipselect);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "couldn't determine num-chip-
select\n");
quoted
+ goto clk_dis_all;
+ }
Is there no reasonable default?
Are you saying if I cant read num-chipselect from dts,
I should set it to a default (that will be 4) and proceed?
quoted
+ /* Set to default valid value */
+ xspi->speed_hz = clk_get_rate(xspi->ref_clk) / 4;
The driver should set max_speed_hz as well.
This is the max_speed_hz - 4 is the lowest divisor possible. 2 is invalid.
It is set in init_hw (see config register default mask).
-----Original Message-----
From: Mark Brown [mailto:broonie@kernel.org]
Sent: Monday, March 17, 2014 11:45 PM
To: Josh Cartwright
Cc: Harini Katakam; robh+dt@kernel.org; pawel.moll@arm.com;
mark.rutland@arm.com; ijc+devicetree@hellion.org.uk;
galak@codeaurora.org; rob@landley.net; grant.likely@linaro.org;
devicetree@vger.kernel.org; linux-doc@vger.kernel.org; linux-
kernel@vger.kernel.org; linux-spi@vger.kernel.org; Michal Simek
Subject: Re: [PATCH] SPI: Add driver for Cadence SPI controller
On Mon, Mar 17, 2014 at 12:59:11PM -0500, Josh Cartwright wrote:
quoted
On Mon, Mar 17, 2014 at 05:30:17PM +0000, Mark Brown wrote:
quoted
On Mon, Mar 17, 2014 at 05:35:36PM +0530, Harini Katakam wrote:
quoted
quoted
quoted
+static int __maybe_unused cdns_spi_suspend(struct device *dev) {
quoted
quoted
This needs to call spi_master_suspend() as well (and similarly on
resume).
quoted
I'm not that familiar with the SPI core, but this seems like an
inversion. Is there a reason why the SPI master class doesn't
implement
suspend/resume() callbacks which handle stopping/starting the queue
automatically for all masters?
This is for users of an optional feature of the infrastructure. We probably
should just call it anyway since it does have checks for the feature being
used (but given all the open coding around this stuff I'd need to verify that
the class callbacks would reliably get called).
In any case that's not happening now and the driver as it stands is buggy
since it's trampling all over the hardware without syncing with anything that
isn't ongoing.
In case of a suspend, we are stopping an ongoing transfer and disabling the interface.
In case I add clock disable and anything else to unprepared too, it will be a cleaner
exit but it will still stop the transfer right? What do you suggest?
Should we wait for transfer to complete or a timeout to occur?
Regards,
Harini
From: Mark Brown <broonie@kernel.org> Date: 2014-03-18 11:04:26
On Tue, Mar 18, 2014 at 05:16:26AM +0000, Harini Katakam wrote:
Please fix your mailer to word wrap within paragraphs, this will make
your mail much more legible.
quoted
quoted
+ if (bits_per_word != 8) {
+ dev_err(&spi->dev, "%s, unsupported bits per word %x\n",
+ __func__, spi->bits_per_word);
+ return -EINVAL;
+ }
quoted
The core will check this for you.
I dint find that the core does this.
bpw_mask.
quoted
It's not clear to me why this has been split into a separate function and the
function will write to the hardware which you're not allowed to do in
setup() if it might affect an ongoing transfer.
Are you saying that there's a chance cdns_spi_setup() will be called
when there's an ongoing transfer? In that case I have to remove the
cdns_setup_transfer() call from here but then there's nothing left to
do in setup.
yes.
quoted
I see from further up the file that there are error interrupt states which
might be flagged but these are just being ignored.
I'm not sure I understand what you are referring to -
The only interrupts enabled (See CNDS_IXR_DEFAULT_MASK) are TXOW and MODF.
+#define CDNS_SPI_IXR_TXFULL_MASK 0x00000008 /* SPI TX Full */
and defined:
+#define CDNS_SPI_IXR_ALL_MASK 0x0000007F /* SPI all interrupts */
quoted
quoted
+ return IRQ_HANDLED;
quoted
This should only be returned if an interrupt was actually handled - if the line
is shared in some system this will break.
In this case both possible interrupt conditions are handled.
Are you sure that's the case, and even if you are that's still not
handling the case where the device isn't flagging an interrupt at all.
quoted
quoted
+ cdns_spi_write(xspi->regs + CDNS_SPI_IER_OFFSET,
+ CDNS_SPI_IXR_DEFAULT_MASK);
+
+ ret = wait_for_completion_interruptible_timeout(&xspi->done,
+ CDNS_SPI_TIMEOUT);
+ if (ret < 1) {
quoted
If you return a positive value from transfer_one() the core will wait for you.
I don't understand. Here, if the ret from
wait_for_completion_interruptible_timeout is positive, then this
function (spi_start_transfer) will return (transfer->len) -
(xspi->remaining_bytes) to cdns_transfer_one_message(the calling
funtion). Which will just use this return as information on how many
bytes were transferred (see variable length).
cdns_transfer_one_messag() only returns 0 or error value (-ve) (see
variable status). It doesn't return positive value to core.
I'm saying you should be implementing a transfer_one() operation and
returning a positive value from it so the core can do the delay for you.
quoted
quoted
+ ret = of_property_read_u16(pdev->dev.of_node, "num-chip-select",
+ &master->num_chipselect);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "couldn't determine num-chip-
select\n");
quoted
+ goto clk_dis_all;
+ }
quoted
Is there no reasonable default?
Are you saying if I cant read num-chipselect from dts,
I should set it to a default (that will be 4) and proceed?
Yes.
quoted
quoted
+ /* Set to default valid value */
+ xspi->speed_hz = clk_get_rate(xspi->ref_clk) / 4;
quoted
The driver should set max_speed_hz as well.
This is the max_speed_hz - 4 is the lowest divisor possible. 2 is invalid.
It is set in init_hw (see config register default mask).
I'm saying you should set max_speed_hz, not set speed_hz to the maximum
value. This tells the core what the maximum speed is so it can error
check the driver operations.
From: Mark Brown <broonie@kernel.org> Date: 2014-03-18 11:06:57
On Tue, Mar 18, 2014 at 05:22:37AM +0000, Harini Katakam wrote:
quoted
quoted
quoted
This needs to call spi_master_suspend() as well (and similarly on
resume).
...
In case of a suspend, we are stopping an ongoing transfer and
disabling the interface. In case I add clock disable and anything
else to unprepared too, it will be a cleaner exit but it will still
stop the transfer right? What do you suggest? Should we wait for
transfer to complete or a timeout to occur?
I suggest doing what I said above and calling spi_master_suspend().
Right now what the driver is doing is just halting the hardware if a
transfer is in progress which will break any ongoing transfers.
-----Original Message-----
From: Mark Brown [mailto:broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
Sent: Tuesday, March 18, 2014 4:34 PM
To: Harini Katakam
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; pawel.moll-5wv7dgnIgG8@public.gmane.org; mark.rutland-5wv7dgnIgG8@public.gmane.org;
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org; galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org; rob-VoJi6FS/r0vR7s880joybQ@public.gmane.org;
grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Michal Simek
Subject: Re: [PATCH] SPI: Add driver for Cadence SPI controller
On Tue, Mar 18, 2014 at 05:16:26AM +0000, Harini Katakam wrote:
Please fix your mailer to word wrap within paragraphs, this will make
your mail much more legible.
quoted
quoted
quoted
+ if (bits_per_word != 8) {
+ dev_err(&spi->dev, "%s, unsupported bits per word %x\n",
+ __func__, spi->bits_per_word);
+ return -EINVAL;
+ }
quoted
quoted
The core will check this for you.
quoted
I dint find that the core does this.
bpw_mask.
OK. Will set master->bits_per_word_mask.
quoted
quoted
It's not clear to me why this has been split into a separate function and
the
quoted
quoted
function will write to the hardware which you're not allowed to do in
setup() if it might affect an ongoing transfer.
quoted
Are you saying that there's a chance cdns_spi_setup() will be called
when there's an ongoing transfer? In that case I have to remove the
cdns_setup_transfer() call from here but then there's nothing left to
do in setup.
yes.
I'm going to remove the bits_per_word check anyway.
But the clock configuration still needs to be done.
Where should it be done spi_setup() or transfer?
Can you please comment on this?
"The function was split into two because cdns_spi_setup_transfer() is called internally by transfer_one_message() everytime.
Is it always possible that the spi_transfer paramters will remain the same?
In that case this call is probably not necessary in transfer_one_message."
quoted
quoted
I see from further up the file that there are error interrupt states which
might be flagged but these are just being ignored.
quoted
I'm not sure I understand what you are referring to -
The only interrupts enabled (See CNDS_IXR_DEFAULT_MASK) are TXOW
+#define CDNS_SPI_IXR_TXFULL_MASK 0x00000008 /* SPI TX Full */
and defined:
quoted
+#define CDNS_SPI_IXR_ALL_MASK 0x0000007F /* SPI all interrupts */
quoted
quoted
quoted
+ return IRQ_HANDLED;
quoted
quoted
This should only be returned if an interrupt was actually handled - if the
line
quoted
quoted
is shared in some system this will break.
quoted
In this case both possible interrupt conditions are handled.
Are you sure that's the case, and even if you are that's still not
handling the case where the device isn't flagging an interrupt at all.
The IXR_ALL mask is only used to disable all the interrupts in the beginning.
These two are the only interrupts enabled.
And RXNEMPTY status is just polled. That interrupt is not enabled either
Regards,
Harini
This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Mark Brown <broonie@kernel.org> Date: 2014-03-18 12:34:09
On Tue, Mar 18, 2014 at 12:13:45PM +0000, Harini Katakam wrote:
I'm going to remove the bits_per_word check anyway.
But the clock configuration still needs to be done.
Where should it be done spi_setup() or transfer?
It needs to be done on the transfer - that is required anyway since the
clock rate is specified per transfer.
quoted
quoted
In this case both possible interrupt conditions are handled.
quoted
Are you sure that's the case, and even if you are that's still not
handling the case where the device isn't flagging an interrupt at all.
The IXR_ALL mask is only used to disable all the interrupts in the beginning.
These two are the only interrupts enabled.
And RXNEMPTY status is just polled. That interrupt is not enabled either
This is all going to be fragile in the face of bugs or changes in the
code though and like I keep saying it doesn't handle interrupt sharing.
-----Original Message-----
From: Mark Brown [mailto:broonie@kernel.org]
Sent: Tuesday, March 18, 2014 6:04 PM
To: Harini Katakam
Cc: robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com;
ijc+devicetree@hellion.org.uk; galak@codeaurora.org; rob@landley.net;
grant.likely@linaro.org; devicetree@vger.kernel.org; linux-
doc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
spi@vger.kernel.org; Michal Simek
Subject: Re: [PATCH] SPI: Add driver for Cadence SPI controller
On Tue, Mar 18, 2014 at 12:13:45PM +0000, Harini Katakam wrote:
quoted
quoted
quoted
In this case both possible interrupt conditions are handled.
quoted
quoted
Are you sure that's the case, and even if you are that's still not
handling the case where the device isn't flagging an interrupt at all.
quoted
The IXR_ALL mask is only used to disable all the interrupts in the
beginning.
quoted
These two are the only interrupts enabled.
And RXNEMPTY status is just polled. That interrupt is not enabled either
This is all going to be fragile in the face of bugs or changes in the
code though and like I keep saying it doesn't handle interrupt sharing.
OK. I dint consider interrupt sharing.
Do you think the following implementation would be better?
status = IRQ_NONE;
if (intr_status & CDNS_SPI_IXR_MODF_MASK) {
/* Handle this interrupt here */
status = IRQ_HANDLED;
} else if (intr_status & CDNS_SPI_IXR_TXOW_MASK) {
/* Handle this interrupt here */
status = IRQ_HANDLED;
}
return status;
Regards,
Harini
From: Mark Brown <broonie@kernel.org> Date: 2014-03-18 15:59:39
On Tue, Mar 18, 2014 at 02:45:09PM +0000, Harini Katakam wrote:
OK. I dint consider interrupt sharing.
Do you think the following implementation would be better?
status = IRQ_NONE;
if (intr_status & CDNS_SPI_IXR_MODF_MASK) {
/* Handle this interrupt here */
status = IRQ_HANDLED;
} else if (intr_status & CDNS_SPI_IXR_TXOW_MASK) {
/* Handle this interrupt here */
status = IRQ_HANDLED;
}
From: Michal Simek <monstr@monstr.eu> Date: 2014-03-20 11:23:26
On 03/17/2014 08:00 PM, Rob Herring wrote:
On Mon, Mar 17, 2014 at 8:22 AM, Michal Simek [off-list ref] wrote:
quoted
Hi Rob,
On 03/17/2014 01:47 PM, Rob Herring wrote:
quoted
On Mon, Mar 17, 2014 at 7:05 AM, Harini Katakam [off-list ref] wrote:
quoted
Add driver for Cadence SPI controller. This is used in Xilinx Zynq.
Signed-off-by: Harini Katakam <redacted>
---
.../devicetree/bindings/spi/spi-cadence.txt | 25 +
We prefer binding docs in separate patch.
I have heard several times that also for binding review you need driver
to look if this binding make sense that's why Harini sent this together.
It means 2 emails instead of one.
But if you wish to have this in two files no problem to split it
but then I believe both should be copied to DT mailing list.
Yes, for 2 reasons:
- To prepare for DT bindings to get merged to separate repo if we ever
get there.
- So DT maintainers can review/ack just the bindings.
ok. No problem to divide it.
quoted
quoted
quoted
+- reg : Physical base address and size of SPI registers map.
+- interrupts : Property with a value describing the interrupt
+ number.
+- interrupt-parent : Must be core interrupt controller
+- clock-names : List of input clock names - "ref_clk", "pclk"
+ (See clock bindings for details).
+- clocks : Clock phandles (see clock bindings for details).
+- num-chip-select : Number of chip selects used.
See Documentation/devicetree/bindings/spi/spi-bus.txt. Use "num-cs" here.
I was expecting you will comment this a little bit. :-)
Because all just reading this num-cs as 32bit and then
assigning this value to master->num_chipselect which is 16bit.
Well, everyone else has that problem then. Obviously it takes a bit
more care than just reading into a u32, but that is a kernel problem
and not a problem of the binding.
They are not reading it directly with read_u32 but they are using
intermediate u32 value which is assigned to u16 which is fine.
This pattern is in most drivers(maybe all).
The point is if binding should or can't simplify driver code.
And from your reaction above I expect that it is up to driver
owner and binding doc how you want to do it.
quoted
quoted
quoted
+/* Macros for the SPI controller read/write */
+#define cdns_spi_read(addr) readl_relaxed(addr)
+#define cdns_spi_write(addr, val) writel_relaxed((val), (addr))
Just use readl/writel directly.
There shouldn't be any problem to use these helper macros
and even I think this is better than using readl/writel directly
because you are more flexible if you want to change it to different
IO.
Then when I read the code I have to go lookup what they do while I
know exactly what writel/readl do already. It is really the same
reasons as why the kernel doesn't have register set and clear bits
accessors.
I understand your reasons but on the other hand if we want to run
ARM-BE then using direct readl/writel functions is also not correct.
Mark proposed one solution and I will see how it looks like.
I believe this returns NO_IRQ which could be 0.
s/</<=/
Are you sure regarding this?
NO_IRQ on ARM is -1.
And IRC irq = 0 should be just valid number.
But if you are right then others drivers have to fixed too.
The definition varies by arch being 0 or -1, so drivers need to deal
with both. The preference is 0 is NO_IRQ. It has been decreed by
Linus. ARM is actually pretty close to being able to change NO_IRQ to
0.
I think that resolution was to completely remove NO_IRQ.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform