From: Ivan T. Ivanov <hidden> Date: 2014-02-06 16:59:20
From: "Ivan T. Ivanov" <redacted>
Hi,
Following two patches are adding initial support for SPI controller
available in Qualcomm SoC's.
Controller initialization is based on spi_qsd driver available in
CAF repository.
Controller supports SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LOOP modes,
up to 3 CS's and from 4 to 32 bits per word. SPI_LOOP mode is limited
to input FIFO buffer size.
Currently driver support only PIO mode, I am hopping to add also DMA
mode support with dmaengine patches developed by Andy.
Regards,
Ivan
Ivan T. Ivanov (2):
spi: qup: Add device tree bindings information
spi: Add Qualcomm QUP SPI controller support
.../devicetree/bindings/spi/qcom,spi-qup.txt | 86 ++
drivers/spi/Kconfig | 14 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-qup.c | 898 ++++++++++++++++++++
4 files changed, 999 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qup.txt
create mode 100644 drivers/spi/spi-qup.c
--
1.7.9.5
From: Ivan T. Ivanov <hidden> Date: 2014-02-06 16:59:30
From: "Ivan T. Ivanov" <redacted>
The Qualcomm Universal Peripheral (QUP) core is an
AHB slave that provides a common data path (an output
FIFO and an input FIFO) for serial peripheral interface
(SPI) mini-core.
Signed-off-by: Ivan T. Ivanov <redacted>
---
.../devicetree/bindings/spi/qcom,spi-qup.txt | 86 ++++++++++++++++++++
1 file changed, 86 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qup.txt
@@ -0,0 +1,86 @@+Qualcomm Universal Peripheral (QUP) Serial Peripheral Interface (SPI)++The QUP core is an AHB slave that provides a common data path (an output FIFO+and an input FIFO) for serial peripheral interface (SPI) mini-core.++SPI in master mode support up to 50MHz, up to four chip selects, and a+programmable data path from 4 bits to 32 bits; supports numerous protocol+variants.++Required properties:+- compatible: Should contain "qcom,spi-qup-v2".+- reg: Should contain base register location and length+- interrupts: Interrupt number used by this controller++- clocks: Should contain the core clock and the AHB clock.+- clock-names: Should be "core" for the core clock and "iface" for the+ AHB clock.++- #address-cells: Number of cells required to define a chip select+ address on the SPI bus. Should be set to 1.+- #size-cells: Should be zero.++Optional properties:+- spi-max-frequency: Specifies maximum SPI clock frequency, Units - Hz. Definition+ as per Documentation/devicetree/bindings/spi/spi-bus.txt++SPI slave nodes must be children of the SPI master node and can+contain properties described in Documentation/devicetree/bindings/spi/spi-bus.txt++Example:++ spi_8: spi@f9964000 { /* BLSP2 QUP2 */++ compatible = "qcom,spi-qup-v2";+ #address-cells = <1>;+ #size-cells = <0>;+ reg = <0xf9964000 0x1000>;+ interrupts = <0 102 0>;+ spi-max-frequency = <19200000>;++ clocks = <&gcc GCC_BLSP2_QUP2_SPI_APPS_CLK>, <&gcc GCC_BLSP2_AHB_CLK>;+ clock-names = "core", "iface";++ pinctrl-names = "default";+ pinctrl-0 = <&spi8_default>;++ device@0 {+ compatible = "arm,pl022-dummy";+ #address-cells = <1>;+ #size-cells = <1>;+ reg = <0>; /* Chip select 0 */+ spi-max-frequency = <19200000>;+ spi-cpol;+ };++ device@1 {+ compatible = "arm,pl022-dummy";+ #address-cells = <1>;+ #size-cells = <1>;+ reg = <1>; /* Chip select 1 */+ spi-max-frequency = <9600000>;+ spi-cpha;+ };++ device@2 {+ compatible = "arm,pl022-dummy";+ #address-cells = <1>;+ #size-cells = <1>;+ reg = <2>; /* Chip select 2 */+ spi-max-frequency = <19200000>;+ spi-cpol;+ spi-cpha;+ };++ device@3 {+ compatible = "arm,pl022-dummy";+ #address-cells = <1>;+ #size-cells = <1>;+ reg = <3>; /* Chip select 3 */+ spi-max-frequency = <19200000>;+ spi-cpol;+ spi-cpha;+ spi-cs-high;+ };+ };+
From: Ivan T. Ivanov <hidden> Date: 2014-02-06 16:59:47
From: "Ivan T. Ivanov" <redacted>
Qualcomm Universal Peripheral (QUP) core is an AHB slave that
provides a common data path (an output FIFO and an input FIFO)
for serial peripheral interface (SPI) mini-core. SPI in master mode
support up to 50MHz, up to four chip selects, and a programmable
data path from 4 bits to 32 bits; MODE0..3 protocols
Signed-off-by: Ivan T. Ivanov <redacted>
Cc: Alok Chauhan <redacted>
Cc: Gilad Avidov <redacted>
Cc: Kiran Gunda <redacted>
Cc: Sagar Dharia <redacted>
---
drivers/spi/Kconfig | 14 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-qup.c | 898 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 913 insertions(+)
create mode 100644 drivers/spi/spi-qup.c
From: Andy Gross <hidden> Date: 2014-02-07 07:39:59
On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote:
quoted hunk
From: "Ivan T. Ivanov" <redacted>
Qualcomm Universal Peripheral (QUP) core is an AHB slave that
provides a common data path (an output FIFO and an input FIFO)
for serial peripheral interface (SPI) mini-core. SPI in master mode
support up to 50MHz, up to four chip selects, and a programmable
data path from 4 bits to 32 bits; MODE0..3 protocols
Signed-off-by: Ivan T. Ivanov <redacted>
Cc: Alok Chauhan <redacted>
Cc: Gilad Avidov <redacted>
Cc: Kiran Gunda <redacted>
Cc: Sagar Dharia <redacted>
---
drivers/spi/Kconfig | 14 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-qup.c | 898 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 913 insertions(+)
create mode 100644 drivers/spi/spi-qup.c
@@ -381,6 +381,20 @@ config SPI_RSPIhelpSPIdriverforRenesasRSPIblocks.+configSPI_QUP+tristate"Qualcomm SPI Support with QUP interface"+depends onARCH_MSM
I'd change to ARCH_MSM_DT. This ensures the OF component is there.
quoted hunk
+ help
+ Qualcomm Universal Peripheral (QUP) core is an AHB slave that
+ provides a common data path (an output FIFO and an input FIFO)
+ for serial peripheral interface (SPI) mini-core. SPI in master
+ mode support up to 50MHz, up to four chip selects, and a
+ programmable data path from 4 bits to 32 bits; supports numerous
+ protocol variants.
+
+ This driver can also be built as a module. If so, the module
+ will be called spi_qup.
+
config SPI_S3C24XX
tristate "Samsung S3C24XX series SPI"
depends on ARCH_S3C24XX
no locking? This whole code block needs to have some type of mutex_lock to keep
others from trouncing the hardware while you are doing this transfer.
+ if (cs_change)
+ spi_qup_assert_cs(controller, chip);
Should the CS be done outside the loop? I'd expect the following sequence to
happen:
- change CS
- Loop and do some transfers
- deassert CS
In this code, you reinitialize and assert/deassert CS for every transaction.
+
+ cs_change = xfer->cs_change;
+
+ /* Do actual transfer */
+ status = spi_qup_transfer_do(controller, chip, xfer);
+ if (status)
+ break;
+
+ msg->actual_length += xfer->len;
+
+ if (xfer->delay_usecs)
+ udelay(xfer->delay_usecs);
+
+ if (cs_change)
+ spi_qup_deassert_cs(controller, chip);
+ }
+
+ if (status || !cs_change)
+ spi_qup_deassert_cs(controller, chip);
+
+ msg->status = status;
+ spi_finalize_current_message(master);
+ return status;
+}
+
+static int spi_qup_probe(struct platform_device *pdev)
+{
+ struct spi_master *master;
+ struct clk *iclk, *cclk;
+ struct spi_qup *controller;
+ struct resource *res;
+ struct device *dev;
+ void __iomem *base;
+ u32 data, max_freq, iomode;
+ int ret, irq, size;
+
+ dev = &pdev->dev;
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+
+ irq = platform_get_irq(pdev, 0);
+
+ if (irq < 0)
+ return irq;
+
+ cclk = devm_clk_get(dev, "core");
+ if (IS_ERR(cclk)) {
+ dev_err(dev, "cannot get core clock\n");
No need to error print. devm_clk_get already outputs something
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.
+
+ if (!max_freq) {
+ dev_err(dev, "invalid clock frequency %d\n", max_freq);
+ return -ENXIO;
+ }
This is buggy. Remove this and collapse into the of_property_read_u32 if
statement. On non-zero, check the range for validity.
+
+ ret = clk_set_rate(cclk, max_freq);
+ if (ret)
+ dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);
+
+static struct platform_driver spi_qup_driver = {
+ .driver = {
+ .name = "spi_qup",
+ .owner = THIS_MODULE,
+ .pm = &spi_qup_dev_pm_ops,
+ .of_match_table = spi_qup_dt_match,
+ },
+ .probe = spi_qup_probe,
+ .remove = spi_qup_remove,
+};
+module_platform_driver(spi_qup_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION("0.4");
+MODULE_ALIAS("platform:spi_qup");
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
From: Andy Gross <hidden> Date: 2014-02-07 07:43:25
On Thu, Feb 06, 2014 at 06:57:47PM +0200, Ivan T. Ivanov wrote:
quoted hunk
From: "Ivan T. Ivanov" <redacted>
The Qualcomm Universal Peripheral (QUP) core is an
AHB slave that provides a common data path (an output
FIFO and an input FIFO) for serial peripheral interface
(SPI) mini-core.
Signed-off-by: Ivan T. Ivanov <redacted>
---
.../devicetree/bindings/spi/qcom,spi-qup.txt | 86 ++++++++++++++++++++
1 file changed, 86 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qup.txt
@@ -0,0 +1,86 @@+Qualcomm Universal Peripheral (QUP) Serial Peripheral Interface (SPI)++The QUP core is an AHB slave that provides a common data path (an output FIFO+and an input FIFO) for serial peripheral interface (SPI) mini-core.++SPI in master mode support up to 50MHz, up to four chip selects, and a+programmable data path from 4 bits to 32 bits; supports numerous protocol+variants.++Required properties:+- compatible: Should contain "qcom,spi-qup-v2".
Could be more descriptive. qcom,spi-qup-v2.1.1 for MSM8974 v1 and
qcom,spi-qup-v2.2.1 for MSM8974 v2.
+- reg: Should contain base register location and length
+- interrupts: Interrupt number used by this controller
+
+- clocks: Should contain the core clock and the AHB clock.
+- clock-names: Should be "core" for the core clock and "iface" for the
+ AHB clock.
+
+- #address-cells: Number of cells required to define a chip select
+ address on the SPI bus. Should be set to 1.
+- #size-cells: Should be zero.
+
+Optional properties:
+- spi-max-frequency: Specifies maximum SPI clock frequency, Units - Hz. Definition
+ as per Documentation/devicetree/bindings/spi/spi-bus.txt
+
+SPI slave nodes must be children of the SPI master node and can
+contain properties described in Documentation/devicetree/bindings/spi/spi-bus.txt
+
+Example:
+
+ spi_8: spi@f9964000 { /* BLSP2 QUP2 */
+
+ compatible = "qcom,spi-qup-v2";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0xf9964000 0x1000>;
+ interrupts = <0 102 0>;
+ spi-max-frequency = <19200000>;
+
+ clocks = <&gcc GCC_BLSP2_QUP2_SPI_APPS_CLK>, <&gcc GCC_BLSP2_AHB_CLK>;
+ clock-names = "core", "iface";
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&spi8_default>;
+
+ device@0 {
+ compatible = "arm,pl022-dummy";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0>; /* Chip select 0 */
+ spi-max-frequency = <19200000>;
+ spi-cpol;
+ };
+
+ device@1 {
+ compatible = "arm,pl022-dummy";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <1>; /* Chip select 1 */
+ spi-max-frequency = <9600000>;
+ spi-cpha;
+ };
+
+ device@2 {
+ compatible = "arm,pl022-dummy";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <2>; /* Chip select 2 */
+ spi-max-frequency = <19200000>;
+ spi-cpol;
+ spi-cpha;
+ };
+
+ device@3 {
+ compatible = "arm,pl022-dummy";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <3>; /* Chip select 3 */
+ spi-max-frequency = <19200000>;
+ spi-cpol;
+ spi-cpha;
+ spi-cs-high;
+ };
+ };
+
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Ivan T. Ivanov <hidden> Date: 2014-02-07 07:45:06
From: "Ivan T. Ivanov" <redacted>
The Qualcomm Universal Peripheral (QUP) core is an
AHB slave that provides a common data path (an output
FIFO and an input FIFO) for serial peripheral interface
(SPI) mini-core.
Signed-off-by: Ivan T. Ivanov <redacted>
---
.../devicetree/bindings/spi/qcom,spi-qup.txt | 86 ++++++++++++++++++++
1 file changed, 86 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qup.txt
@@ -0,0 +1,86 @@+Qualcomm Universal Peripheral (QUP) Serial Peripheral Interface (SPI)++The QUP core is an AHB slave that provides a common data path (an output FIFO+and an input FIFO) for serial peripheral interface (SPI) mini-core.++SPI in master mode support up to 50MHz, up to four chip selects, and a+programmable data path from 4 bits to 32 bits; supports numerous protocol+variants.++Required properties:+- compatible: Should contain "qcom,spi-qup-v2".+- reg: Should contain base register location and length+- interrupts: Interrupt number used by this controller++- clocks: Should contain the core clock and the AHB clock.+- clock-names: Should be "core" for the core clock and "iface" for the+ AHB clock.++- #address-cells: Number of cells required to define a chip select+ address on the SPI bus. Should be set to 1.+- #size-cells: Should be zero.++Optional properties:+- spi-max-frequency: Specifies maximum SPI clock frequency, Units - Hz. Definition+ as per Documentation/devicetree/bindings/spi/spi-bus.txt++SPI slave nodes must be children of the SPI master node and can+contain properties described in Documentation/devicetree/bindings/spi/spi-bus.txt++Example:++ spi_8: spi@f9964000 { /* BLSP2 QUP2 */++ compatible = "qcom,spi-qup-v2";+ #address-cells = <1>;+ #size-cells = <0>;+ reg = <0xf9964000 0x1000>;+ interrupts = <0 102 0>;+ spi-max-frequency = <19200000>;++ clocks = <&gcc GCC_BLSP2_QUP2_SPI_APPS_CLK>, <&gcc GCC_BLSP2_AHB_CLK>;+ clock-names = "core", "iface";++ pinctrl-names = "default";+ pinctrl-0 = <&spi8_default>;++ device@0 {+ compatible = "arm,pl022-dummy";+ #address-cells = <1>;+ #size-cells = <1>;+ reg = <0>; /* Chip select 0 */+ spi-max-frequency = <19200000>;+ spi-cpol;+ };++ device@1 {+ compatible = "arm,pl022-dummy";+ #address-cells = <1>;+ #size-cells = <1>;+ reg = <1>; /* Chip select 1 */+ spi-max-frequency = <9600000>;+ spi-cpha;+ };++ device@2 {+ compatible = "arm,pl022-dummy";+ #address-cells = <1>;+ #size-cells = <1>;+ reg = <2>; /* Chip select 2 */+ spi-max-frequency = <19200000>;+ spi-cpol;+ spi-cpha;+ };++ device@3 {+ compatible = "arm,pl022-dummy";+ #address-cells = <1>;+ #size-cells = <1>;+ reg = <3>; /* Chip select 3 */+ spi-max-frequency = <19200000>;+ spi-cpol;+ spi-cpha;+ spi-cs-high;+ };+ };+
From: Ivan T. Ivanov <hidden> Date: 2014-02-07 09:53:56
Hi Andy,
On Fri, 2014-02-07 at 01:39 -0600, Andy Gross wrote:
On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote:
quoted
From: "Ivan T. Ivanov" <redacted>
Qualcomm Universal Peripheral (QUP) core is an AHB slave that
provides a common data path (an output FIFO and an input FIFO)
for serial peripheral interface (SPI) mini-core. SPI in master mode
support up to 50MHz, up to four chip selects, and a programmable
data path from 4 bits to 32 bits; MODE0..3 protocols
Signed-off-by: Ivan T. Ivanov <redacted>
Cc: Alok Chauhan <redacted>
Cc: Gilad Avidov <redacted>
Cc: Kiran Gunda <redacted>
Cc: Sagar Dharia <redacted>
---
drivers/spi/Kconfig | 14 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-qup.c | 898 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 913 insertions(+)
create mode 100644 drivers/spi/spi-qup.c
@@ -381,6 +381,20 @@ config SPI_RSPIhelpSPIdriverforRenesasRSPIblocks.+configSPI_QUP+tristate"Qualcomm SPI Support with QUP interface"+depends onARCH_MSM
I'd change to ARCH_MSM_DT. This ensures the OF component is there.
Ok. will change.
quoted
+ help
+ Qualcomm Universal Peripheral (QUP) core is an AHB slave that
+ provides a common data path (an output FIFO and an input FIFO)
+ for serial peripheral interface (SPI) mini-core. SPI in master
+ mode support up to 50MHz, up to four chip selects, and a
+ programmable data path from 4 bits to 32 bits; supports numerous
+ protocol variants.
+
+ This driver can also be built as a module. If so, the module
+ will be called spi_qup.
+
config SPI_S3C24XX
tristate "Samsung S3C24XX series SPI"
depends on ARCH_S3C24XX
Did you see any particular issue with the implementation
or this is just because this platform didn't have support
for power management?
quoted
+#include <linux/spi/spi.h>
+
<snip>
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?
no locking? This whole code block needs to have some type of mutex_lock to keep
others from trouncing the hardware while you are doing this transfer.
This is handled by SPI framework.
quoted
+ if (cs_change)
+ spi_qup_assert_cs(controller, chip);
Should the CS be done outside the loop? I'd expect the following sequence to
happen:
- change CS
- Loop and do some transfers
- deassert CS
In this code, you reinitialize and assert/deassert CS for every transaction.
quoted
+
+ cs_change = xfer->cs_change;
Not exactly. It is allowed that CS goes inactive after every
transaction. This is how I read struct spi_transfer description.
quoted
+
+ /* Do actual transfer */
+ status = spi_qup_transfer_do(controller, chip, xfer);
+ if (status)
+ break;
+
+ msg->actual_length += xfer->len;
+
+ if (xfer->delay_usecs)
+ udelay(xfer->delay_usecs);
+
+ if (cs_change)
+ spi_qup_deassert_cs(controller, chip);
+ }
+
+ if (status || !cs_change)
+ spi_qup_deassert_cs(controller, chip);
+
+ msg->status = status;
+ spi_finalize_current_message(master);
+ return status;
+}
+
+static int spi_qup_probe(struct platform_device *pdev)
+{
+ struct spi_master *master;
+ struct clk *iclk, *cclk;
+ struct spi_qup *controller;
+ struct resource *res;
+ struct device *dev;
+ void __iomem *base;
+ u32 data, max_freq, iomode;
+ int ret, irq, size;
+
+ dev = &pdev->dev;
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+
+ irq = platform_get_irq(pdev, 0);
+
+ if (irq < 0)
+ return irq;
+
+ cclk = devm_clk_get(dev, "core");
+ if (IS_ERR(cclk)) {
+ dev_err(dev, "cannot get core clock\n");
No need to error print. devm_clk_get already outputs something
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.
quoted
+
+ if (!max_freq) {
+ dev_err(dev, "invalid clock frequency %d\n", max_freq);
+ return -ENXIO;
+ }
This is buggy. Remove this and collapse into the of_property_read_u32 if
statement. On non-zero, check the range for validity.
True. Will fix.
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?
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.
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?
Thanks,
Ivan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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-02-07 12:52:40
On Fri, Feb 07, 2014 at 09:43:27AM +0200, Ivan T. Ivanov wrote:
From: "Ivan T. Ivanov" <redacted>
The Qualcomm Universal Peripheral (QUP) core is an
AHB slave that provides a common data path (an output
FIFO and an input FIFO) for serial peripheral interface
(SPI) mini-core.
Please fix the formatting of this document so the lines are less than 80
columns, it's hard to read as is. Otherwise this is fine.
From: Ivan T. Ivanov <hidden> Date: 2014-02-07 13:02:07
On Fri, 2014-02-07 at 12:27 +0000, Mark Brown wrote:
On Fri, Feb 07, 2014 at 09:43:27AM +0200, Ivan T. Ivanov wrote:
quoted
From: "Ivan T. Ivanov" <redacted>
The Qualcomm Universal Peripheral (QUP) core is an
AHB slave that provides a common data path (an output
FIFO and an input FIFO) for serial peripheral interface
(SPI) mini-core.
Please fix the formatting of this document so the lines are less than 80
columns, it's hard to read as is. Otherwise this is fine.
File is looking fine in editor with smart tab support
Regards,
Ivan
From: Mark Brown <broonie@kernel.org> Date: 2014-02-07 13:14:20
On Fri, Feb 07, 2014 at 03:00:43PM +0200, Ivan T. Ivanov wrote:
On Fri, 2014-02-07 at 12:27 +0000, Mark Brown wrote:
quoted
Please fix the formatting of this document so the lines are less than 80
columns, it's hard to read as is. Otherwise this is fine.
File is looking fine in editor with smart tab support
I don't know what "smart tab support" is intended to be in this context
but whatever it is it's not working well in the e-mail you sent, bear in
mind that you're looking for a readable patch.
From: Ivan T. Ivanov <hidden> Date: 2014-02-07 13:36:37
On Fri, 2014-02-07 at 13:13 +0000, Mark Brown wrote:
On Fri, Feb 07, 2014 at 03:00:43PM +0200, Ivan T. Ivanov wrote:
quoted
On Fri, 2014-02-07 at 12:27 +0000, Mark Brown wrote:
quoted
quoted
Please fix the formatting of this document so the lines are less than 80
columns, it's hard to read as is. Otherwise this is fine.
quoted
File is looking fine in editor with smart tab support
I don't know what "smart tab support" is intended to be in this context
but whatever it is it's not working well in the e-mail you sent, bear in
mind that you're looking for a readable patch.
On Fri, Feb 07, 2014 at 01:39:52AM -0600, Andy Gross wrote:
On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote:
quoted
From: "Ivan T. Ivanov" <redacted>
Qualcomm Universal Peripheral (QUP) core is an AHB slave that
provides a common data path (an output FIFO and an input FIFO)
for serial peripheral interface (SPI) mini-core. SPI in master mode
support up to 50MHz, up to four chip selects, and a programmable
data path from 4 bits to 32 bits; MODE0..3 protocols
Signed-off-by: Ivan T. Ivanov <redacted>
Cc: Alok Chauhan <redacted>
Cc: Gilad Avidov <redacted>
Cc: Kiran Gunda <redacted>
Cc: Sagar Dharia <redacted>
---
drivers/spi/Kconfig | 14 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-qup.c | 898 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 913 insertions(+)
create mode 100644 drivers/spi/spi-qup.c
@@ -381,6 +381,20 @@ config SPI_RSPIhelpSPIdriverforRenesasRSPIblocks.+configSPI_QUP+tristate"Qualcomm SPI Support with QUP interface"+depends onARCH_MSM
I'd change to ARCH_MSM_DT. This ensures the OF component is there.
I'd rather explicitly include the CONFIG_OF dependency, but I'm not too
opinionated.
config SPI_QUP
tristate "Qualcomm SPI Support with QUP interface"
depends on OF
depends on ARM
depends on ARCH_MSM_DT || COMPILE_TEST
With Kumar's pending the ARCH_MSM_DT -> ARCH_QCOM rename, we'll
introduce a arm-soc/spi tree dependency here that we'll need to keep
track of.
Kumar-
How would you like to handle this? Would it make sense for this to go
through the SPI tree with depending on ARCH_QCOM instead of ARCH_MSM_DT?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
From: Mark Brown <broonie@kernel.org> Date: 2014-02-07 17:12:19
On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote:
This looks mostly good, there's a few odd things and missing use of
framework features.
Qualcomm Universal Peripheral (QUP) core is an AHB slave that
provides a common data path (an output FIFO and an input FIFO)
for serial peripheral interface (SPI) mini-core. SPI in master mode
support up to 50MHz, up to four chip selects, and a programmable
data path from 4 bits to 32 bits; MODE0..3 protocols
The grammar in this and the Kconfig text is a bit garbled, might want to
give it a once over (support -> supports for example).
This looks like a transfer_one() function, please use the framework
features where you can.
+ if (controller->speed_hz != chip->speed_hz) {
+ ret = clk_set_rate(controller->cclk, chip->speed_hz);
+ if (ret) {
+ dev_err(controller->dev, "fail to set frequency %d",
+ chip->speed_hz);
+ return -EIO;
+ }
+ }
Is calling into the clock framework really so expensive that we need to
avoid doing it? You also shouldn't be interacting with the hardware in
setup().
This looks like a switch statement, and looking at the above it's not
clear that the device actually supports anything other than whole bytes.
I'm not sure what that would mean from an API point of view.
+static int spi_qup_transfer_one(struct spi_master *master,
+ struct spi_message *msg)
+{
This entire function can be removed, the core can do it for you.
+ if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
+ max_freq = 19200000;
+
+ if (!max_freq) {
+ dev_err(dev, "invalid clock frequency %d\n", max_freq);
+ return -ENXIO;
+ }
+
+ ret = clk_set_rate(cclk, max_freq);
+ if (ret)
+ dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);
You set the clock rate per transfer so why bother setting it here,
perhaps we support the rate the devices request but not this maximum
rate?
Hey Mark-
On Fri, Feb 07, 2014 at 05:18:34PM +0000, Mark Brown wrote:
On Fri, Feb 07, 2014 at 10:51:27AM -0600, Josh Cartwright wrote:
quoted
config SPI_QUP
tristate "Qualcomm SPI Support with QUP interface"
depends on OF
depends on ARM
Does this really depend on ARM? If so why?
The ARM dependency is there for the use of _relaxed io accessor
variants.
quoted
depends on ARCH_MSM_DT || COMPILE_TEST
quoted
With Kumar's pending the ARCH_MSM_DT -> ARCH_QCOM rename, we'll
introduce a arm-soc/spi tree dependency here that we'll need to keep
track of.
It seems simpler to just depend on MSM_DT || ARCH_QCOM or whatever.
ARCH_MSM_DT is going away, so maybe this is the best option for the
short term (a later patch can remove ARCH_MSM_DT from here at some point
in the future).
Josh
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
From: Andy Gross <hidden> Date: 2014-02-07 17:32:24
On Fri, Feb 07, 2014 at 11:52:33AM +0200, Ivan T. Ivanov wrote:
Hi Andy,
On Fri, 2014-02-07 at 01:39 -0600, Andy Gross wrote:
quoted
On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote:
quoted
From: "Ivan T. Ivanov" <redacted>
Qualcomm Universal Peripheral (QUP) core is an AHB slave that
provides a common data path (an output FIFO and an input FIFO)
for serial peripheral interface (SPI) mini-core. SPI in master mode
support up to 50MHz, up to four chip selects, and a programmable
data path from 4 bits to 32 bits; MODE0..3 protocols
Signed-off-by: Ivan T. Ivanov <redacted>
Cc: Alok Chauhan <redacted>
Cc: Gilad Avidov <redacted>
Cc: Kiran Gunda <redacted>
Cc: Sagar Dharia <redacted>
---
drivers/spi/Kconfig | 14 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-qup.c | 898 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 913 insertions(+)
create mode 100644 drivers/spi/spi-qup.c
@@ -381,6 +381,20 @@ config SPI_RSPIhelpSPIdriverforRenesasRSPIblocks.+configSPI_QUP+tristate"Qualcomm SPI Support with QUP interface"+depends onARCH_MSM
I'd change to ARCH_MSM_DT. This ensures the OF component is there.
Ok. will change.
quoted
quoted
+ help
+ Qualcomm Universal Peripheral (QUP) core is an AHB slave that
+ provides a common data path (an output FIFO and an input FIFO)
+ for serial peripheral interface (SPI) mini-core. SPI in master
+ mode support up to 50MHz, up to four chip selects, and a
+ programmable data path from 4 bits to 32 bits; supports numerous
+ protocol variants.
+
+ This driver can also be built as a module. If so, the module
+ will be called spi_qup.
+
config SPI_S3C24XX
tristate "Samsung S3C24XX series SPI"
depends on ARCH_S3C24XX
Did you see any particular issue with the implementation
or this is just because this platform didn't have support
for power management?
The platform doesn't have support for PM right now. So it's probably better to
remove all this and revisit later when it is in place.
quoted
quoted
+#include <linux/spi/spi.h>
+
<snip>
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.
no locking? This whole code block needs to have some type of mutex_lock to keep
others from trouncing the hardware while you are doing this transfer.
This is handled by SPI framework.
Ah I looked through that and didn't see it the first time. But looking again, I
see it. You're right, you can ignore this comment.
quoted
quoted
+ if (cs_change)
+ spi_qup_assert_cs(controller, chip);
Should the CS be done outside the loop? I'd expect the following sequence to
happen:
- change CS
- Loop and do some transfers
- deassert CS
In this code, you reinitialize and assert/deassert CS for every transaction.
quoted
+
+ cs_change = xfer->cs_change;
Not exactly. It is allowed that CS goes inactive after every
transaction. This is how I read struct spi_transfer description.
Ah ok. This is fine then.
quoted
quoted
+
+ /* Do actual transfer */
+ status = spi_qup_transfer_do(controller, chip, xfer);
+ if (status)
+ break;
+
+ msg->actual_length += xfer->len;
+
+ if (xfer->delay_usecs)
+ udelay(xfer->delay_usecs);
+
+ if (cs_change)
+ spi_qup_deassert_cs(controller, chip);
+ }
+
+ if (status || !cs_change)
+ spi_qup_deassert_cs(controller, chip);
+
+ msg->status = status;
+ spi_finalize_current_message(master);
+ return status;
+}
+
+static int spi_qup_probe(struct platform_device *pdev)
+{
+ struct spi_master *master;
+ struct clk *iclk, *cclk;
+ struct spi_qup *controller;
+ struct resource *res;
+ struct device *dev;
+ void __iomem *base;
+ u32 data, max_freq, iomode;
+ int ret, irq, size;
+
+ dev = &pdev->dev;
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+
+ irq = platform_get_irq(pdev, 0);
+
+ if (irq < 0)
+ return irq;
+
+ cclk = devm_clk_get(dev, "core");
+ if (IS_ERR(cclk)) {
+ dev_err(dev, "cannot get core clock\n");
No need to error print. devm_clk_get already outputs something
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.
quoted
quoted
+
+ if (!max_freq) {
+ dev_err(dev, "invalid clock frequency %d\n", max_freq);
+ return -ENXIO;
+ }
This is buggy. Remove this and collapse into the of_property_read_u32 if
statement. On non-zero, check the range for validity.
True. Will fix.
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 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.
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.
Thanks,
Ivan
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
--
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
On Fri, Feb 07, 2014 at 05:31:08PM +0000, Mark Brown wrote:
On Fri, Feb 07, 2014 at 11:20:51AM -0600, Josh Cartwright wrote:
quoted
On Fri, Feb 07, 2014 at 05:18:34PM +0000, Mark Brown wrote:
quoted
On Fri, Feb 07, 2014 at 10:51:27AM -0600, Josh Cartwright wrote:
quoted
config SPI_QUP
tristate "Qualcomm SPI Support with QUP interface"
depends on OF
depends on ARM
quoted
quoted
Does this really depend on ARM? If so why?
quoted
The ARM dependency is there for the use of _relaxed io accessor
variants.
That's not ARM only and I thought we were getting generic versions of it
anyway? ARMv8, MIPS, Microblaze, Hexagon and SH also define it.
Okay, that's fair. I'm only vaguely familiar with the generic _relaxed
variants, but until they land, how do we appropriately declare the
dependency to prevent breaking COMPILE_TEST builds on architectures that
don't have them? Or should we either bother?
Do we need to introduce a HAVE_RELAXED_IO_ACCESSORS selected by those
architectures with support?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
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-02-07 18:12:55
On Fri, Feb 07, 2014 at 11:46:43AM -0600, Josh Cartwright wrote:
On Fri, Feb 07, 2014 at 05:31:08PM +0000, Mark Brown wrote:
quoted
That's not ARM only and I thought we were getting generic versions of it
anyway? ARMv8, MIPS, Microblaze, Hexagon and SH also define it.
Okay, that's fair. I'm only vaguely familiar with the generic _relaxed
variants, but until they land, how do we appropriately declare the
dependency to prevent breaking COMPILE_TEST builds on architectures that
don't have them? Or should we either bother?
Do we need to introduce a HAVE_RELAXED_IO_ACCESSORS selected by those
architectures with support?
I think that or just getting generic versions done would be the way
forwards. Right now it's a bit of a shambles.
From: Andy Gross <hidden> Date: 2014-02-07 19:12:16
On Fri, Feb 07, 2014 at 05:52:34PM +0000, Mark Brown wrote:
On Fri, Feb 07, 2014 at 11:32:07AM -0600, Andy Gross wrote:
quoted
On Fri, Feb 07, 2014 at 11:52:33AM +0200, Ivan T. Ivanov wrote:
[... snip ...]
quoted
The platform doesn't have support for PM right now. So it's probably better to
remove all this and revisit later when it is in place.
No, runtime PM does not require any platform support at all and is good
practice - look at what the driver is doing with it, it's useful as-is.
Fair enough.
--
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
--
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: Ivan T. Ivanov <hidden> Date: 2014-02-10 16:30:55
Hi,
On Fri, 2014-02-07 at 17:12 +0000, Mark Brown wrote:
On Thu, Feb 06, 2014 at 06:57:48PM +0200, Ivan T. Ivanov wrote:
This looks mostly good, there's a few odd things and missing use of
framework features.
quoted
Qualcomm Universal Peripheral (QUP) core is an AHB slave that
provides a common data path (an output FIFO and an input FIFO)
for serial peripheral interface (SPI) mini-core. SPI in master mode
support up to 50MHz, up to four chip selects, and a programmable
data path from 4 bits to 32 bits; MODE0..3 protocols
The grammar in this and the Kconfig text is a bit garbled, might want to
give it a once over (support -> supports for example).
This looks like a switch statement, and looking at the above it's not
clear that the device actually supports anything other than whole bytes.
I'm not sure what that would mean from an API point of view.
SPI API didn't validate struct spi_transfer::len field.
The whole sniped looks like this:
chip->bits_per_word = spi->bits_per_word;
if (xfer->bits_per_word)
chip->bits_per_word = xfer->bits_per_word;
if (chip->bits_per_word <= 8)
controller->bytes_per_word = 1;
else if (chip->bits_per_word <= 16)
controller->bytes_per_word = 2;
else
controller->bytes_per_word = 4;
if (controller->bytes_per_word > xfer->len ||
xfer->len % controller->bytes_per_word != 0){
/* No partial transfers */
dev_err(controller->dev, "invalid len %d for %d bits\n",
xfer->len, chip->bits_per_word);
return -EIO;
}
n_words = xfer->len / controller->bytes_per_word;
'bytes_per_word' have to be power of 2. This is my understanding of
struct spi_transfer description. So I am discarding all transfers with
'len' non multiple of word size.
quoted
+static int spi_qup_transfer_one(struct spi_master *master,
+ struct spi_message *msg)
+{
This entire function can be removed, the core can do it for you.
Sure, will use it.
quoted
+ if (of_property_read_u32(dev->of_node, "spi-max-frequency", &max_freq))
+ max_freq = 19200000;
+
+ if (!max_freq) {
+ dev_err(dev, "invalid clock frequency %d\n", max_freq);
+ return -ENXIO;
+ }
+
+ ret = clk_set_rate(cclk, max_freq);
+ if (ret)
+ dev_warn(dev, "fail to set SPI frequency %d\n", max_freq);
You set the clock rate per transfer so why bother setting it here,
Only if differs from the current one.
perhaps we support the rate the devices request but not this maximum
rate?
Thats why it is just a warning. I will see how to handle this better.
Will fix.
Thanks,
Ivan
--
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: Ivan T. Ivanov <hidden> Date: 2014-02-10 16:56:30
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>
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.
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
This looks like a switch statement, and looking at the above it's not
clear that the device actually supports anything other than whole bytes.
I'm not sure what that would mean from an API point of view.
SPI API didn't validate struct spi_transfer::len field.
It's supposed to; if the validation is incomplete then that should be
fixed.
From: Andy Gross <hidden> Date: 2014-02-10 17:48:03
On Mon, Feb 10, 2014 at 06:55:02PM +0200, Ivan T. Ivanov wrote:
[....]
quoted
quoted
quoted
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.
That works. My only concern is that it might be nice to catch a configuration
problem early rather than wait for the SPI transfer to fail continuously.
[....]
quoted
quoted
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.
You have to wait until the hardware is in a sane state. For the QUP, that means
in a RUN/PAUSE/RESET state. It cannot be in transition when you cut the clocks.
The safest thing to do is to get the QUP into the RESET state and then cut the
clocks.
[.....]
quoted
quoted
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? :-)
According to the documentation, there is no v2.2.0. It appears there is some
disconnect between the specific HW revision and the documentation. I'll see if
I can get some clarification from the hardware guys. For now, I think the 2.1.1
and 2.2.1 tags are fine.
--
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Ivan T. Ivanov <hidden> Date: 2014-02-10 20:17:03
Hi,
On Mon, 2014-02-10 at 11:47 -0600, Andy Gross wrote:
On Mon, Feb 10, 2014 at 06:55:02PM +0200, Ivan T. Ivanov wrote:
[....]
quoted
quoted
quoted
quoted
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.
That works. My only concern is that it might be nice to catch a configuration
problem early rather than wait for the SPI transfer to fail continuously.
If developer is skilled enough to know which version controller is,
(s)he will be able to put the right frequency constrain here :-)
[....]
quoted
quoted
quoted
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.
You have to wait until the hardware is in a sane state. For the QUP, that means
in a RUN/PAUSE/RESET state. It cannot be in transition when you cut the clocks.
The safest thing to do is to get the QUP into the RESET state and then cut the
clocks.
Sure. will do.
[.....]
quoted
quoted
quoted
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? :-)
According to the documentation, there is no v2.2.0. It appears there is some
disconnect between the specific HW revision and the documentation. I'll see if
I can get some clarification from the hardware guys. For now, I think the 2.1.1
and 2.2.1 tags are fine.
Ok. Thanks,
Ivan
--
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: Courtney Cavin <hidden> Date: 2014-02-10 20:27:50
On Mon, Feb 10, 2014 at 08:41:44PM +0100, Ivan T. Ivanov wrote:
Hi,
On Mon, 2014-02-10 at 11:47 -0600, Andy Gross wrote:
quoted
On Mon, Feb 10, 2014 at 06:55:02PM +0200, Ivan T. Ivanov wrote:
[....]
quoted
quoted
quoted
quoted
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.
That works. My only concern is that it might be nice to catch a configuration
problem early rather than wait for the SPI transfer to fail continuously.
If developer is skilled enough to know which version controller is,
(s)he will be able to put the right frequency constrain here :-)
A developer doesn't have to have much skill at all to copy-paste DT
configurations around and muck with numbers.... I agree with Andy here,
early validation is a good idea here, at the very least, some sanity
checks.
-Courtney
From: Ivan T. Ivanov <hidden> Date: 2014-02-10 20:59:59
Hi,
On Mon, 2014-02-10 at 12:29 -0800, Courtney Cavin wrote:
On Mon, Feb 10, 2014 at 08:41:44PM +0100, Ivan T. Ivanov wrote:
quoted
Hi,
On Mon, 2014-02-10 at 11:47 -0600, Andy Gross wrote:
quoted
On Mon, Feb 10, 2014 at 06:55:02PM +0200, Ivan T. Ivanov wrote:
[....]
quoted
quoted
quoted
quoted
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.
That works. My only concern is that it might be nice to catch a configuration
problem early rather than wait for the SPI transfer to fail continuously.
If developer is skilled enough to know which version controller is,
(s)he will be able to put the right frequency constrain here :-)
A developer doesn't have to have much skill at all to copy-paste DT
configurations around and muck with numbers.... I agree with Andy here,
early validation is a good idea here, at the very least, some sanity
checks.
So, probably first variant with just warning will be good enough?
Regards,
Ivan
From: Mark Brown <broonie@kernel.org> Date: 2014-02-10 21:40:53
On Mon, Feb 10, 2014 at 10:59:54PM +0200, Ivan T. Ivanov wrote:
On Mon, 2014-02-10 at 12:29 -0800, Courtney Cavin wrote:
quoted
A developer doesn't have to have much skill at all to copy-paste DT
configurations around and muck with numbers.... I agree with Andy here,
early validation is a good idea here, at the very least, some sanity
checks.
So, probably first variant with just warning will be good enough?
I'm not sure it actually adds anything meaningful here - if the error
reporting isn't clear enough on use then that's probably an issue anyway
and we may never even use the default.
From: Ivan T. Ivanov <hidden> Date: 2014-02-11 15:47:54
Hi,
On Mon, 2014-02-10 at 12:29 -0800, Courtney Cavin wrote:
On Mon, Feb 10, 2014 at 08:41:44PM +0100, Ivan T. Ivanov wrote:
quoted
Hi,
On Mon, 2014-02-10 at 11:47 -0600, Andy Gross wrote:
quoted
On Mon, Feb 10, 2014 at 06:55:02PM +0200, Ivan T. Ivanov wrote:
[....]
quoted
quoted
quoted
quoted
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.
That works. My only concern is that it might be nice to catch a configuration
problem early rather than wait for the SPI transfer to fail continuously.
If developer is skilled enough to know which version controller is,
(s)he will be able to put the right frequency constrain here :-)
A developer doesn't have to have much skill at all to copy-paste DT
configurations around and muck with numbers.... I agree with Andy here,
early validation is a good idea here, at the very least, some sanity
checks.
Actually, thinking more on this. Supplying SPI controller with,
let say 50MHz, which is what success of clk_set_rate() means, doesn't
necessarily guaranteer that controller will be able to do transfers
properly, right? Setting frequency at this point didn't bring any
benefit.
Regards,
Ivan
-Courtney
--
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