Re: [PATCH v3 1/2] SPI: Add driver for Cadence SPI controller
Hi Mark,
On Fri, Apr 11, 2014 at 9:06 AM, Harini Katakam
wrote:
> Hi Mark,
>
> On Fri, Apr 11, 2014 at 4:18 AM, Mark Brown wrote:
>> On Thu, Apr 10, 2014 at 05:43:49PM +0530, Harini Katakam wrote:
>>> Add driver for Cadence SPI controller. This is used in Xilinx Zynq.
>>
>> This looks mostly good, the issues below are very small.
>>
>>> +static int __maybe_unused cdns_spi_suspend(struct device *dev)
>>> +{
>>> + struct platform_device *pdev = container_of(dev,
>>> + struct platform_device, dev);
>>> + struct spi_master *master = platform_get_drvdata(pdev);
>>> + struct cdns_spi *xspi = spi_master_get_devdata(master);
>>> +
>>> + spi_master_suspend(master);
>>> +
>>> + clk_disable(xspi->ref_clk);
>>> +
>>> + clk_disable(xspi->pclk);
>>
>> You're only doing clk_disable() here, I would expect the clocks to also
>> be unprepared over suspend - there is no value in leaving them prepared
>> that I can see. Just use clk_unprepare_disable().
>>
>> It would also be good (though not essential) to implement runtime PM and
>> disable the clocks while there are no transfers in progress for a small
>> power saving. The auto_runtime_pm feature in the core will do the
>> runtime PM calls for you.
>>
>> Otherwise this looks good.
>
> Thanks. I'll make these changes and send a v4 next week.
>
This driver without runtime PM will suffice our current requirements.
Disabling the clocks will offer some small power savings as you pointed out and
I will revisit this at a later time as an enhancement.
I have addressed the other review comment and sent out a v4.
Regards,
Harini
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] SPI: Add driver for Cadence SPI controller
Hi Mark,
On Fri, Apr 11, 2014 at 9:06 AM, Harini Katakam
harinikatakamli...@gmail.com wrote:
Hi Mark,
On Fri, Apr 11, 2014 at 4:18 AM, Mark Brown broo...@kernel.org wrote:
On Thu, Apr 10, 2014 at 05:43:49PM +0530, Harini Katakam wrote:
Add driver for Cadence SPI controller. This is used in Xilinx Zynq.
This looks mostly good, the issues below are very small.
+static int __maybe_unused cdns_spi_suspend(struct device *dev)
+{
+ struct platform_device *pdev = container_of(dev,
+ struct platform_device, dev);
+ struct spi_master *master = platform_get_drvdata(pdev);
+ struct cdns_spi *xspi = spi_master_get_devdata(master);
+
+ spi_master_suspend(master);
+
+ clk_disable(xspi-ref_clk);
+
+ clk_disable(xspi-pclk);
You're only doing clk_disable() here, I would expect the clocks to also
be unprepared over suspend - there is no value in leaving them prepared
that I can see. Just use clk_unprepare_disable().
It would also be good (though not essential) to implement runtime PM and
disable the clocks while there are no transfers in progress for a small
power saving. The auto_runtime_pm feature in the core will do the
runtime PM calls for you.
Otherwise this looks good.
Thanks. I'll make these changes and send a v4 next week.
This driver without runtime PM will suffice our current requirements.
Disabling the clocks will offer some small power savings as you pointed out and
I will revisit this at a later time as an enhancement.
I have addressed the other review comment and sent out a v4.
Regards,
Harini
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] SPI: Add driver for Cadence SPI controller
Hi Mark,
On Fri, Apr 11, 2014 at 4:18 AM, Mark Brown wrote:
> On Thu, Apr 10, 2014 at 05:43:49PM +0530, Harini Katakam wrote:
>> Add driver for Cadence SPI controller. This is used in Xilinx Zynq.
>
> This looks mostly good, the issues below are very small.
>
>> +static int __maybe_unused cdns_spi_suspend(struct device *dev)
>> +{
>> + struct platform_device *pdev = container_of(dev,
>> + struct platform_device, dev);
>> + struct spi_master *master = platform_get_drvdata(pdev);
>> + struct cdns_spi *xspi = spi_master_get_devdata(master);
>> +
>> + spi_master_suspend(master);
>> +
>> + clk_disable(xspi->ref_clk);
>> +
>> + clk_disable(xspi->pclk);
>
> You're only doing clk_disable() here, I would expect the clocks to also
> be unprepared over suspend - there is no value in leaving them prepared
> that I can see. Just use clk_unprepare_disable().
>
> It would also be good (though not essential) to implement runtime PM and
> disable the clocks while there are no transfers in progress for a small
> power saving. The auto_runtime_pm feature in the core will do the
> runtime PM calls for you.
>
> Otherwise this looks good.
Thanks. I'll make these changes and send a v4 next week.
Regards,
Harini
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] SPI: Add driver for Cadence SPI controller
On Thu, Apr 10, 2014 at 05:43:49PM +0530, Harini Katakam wrote:
> Add driver for Cadence SPI controller. This is used in Xilinx Zynq.
This looks mostly good, the issues below are very small.
> +static int __maybe_unused cdns_spi_suspend(struct device *dev)
> +{
> + struct platform_device *pdev = container_of(dev,
> + struct platform_device, dev);
> + struct spi_master *master = platform_get_drvdata(pdev);
> + struct cdns_spi *xspi = spi_master_get_devdata(master);
> +
> + spi_master_suspend(master);
> +
> + clk_disable(xspi->ref_clk);
> +
> + clk_disable(xspi->pclk);
You're only doing clk_disable() here, I would expect the clocks to also
be unprepared over suspend - there is no value in leaving them prepared
that I can see. Just use clk_unprepare_disable().
It would also be good (though not essential) to implement runtime PM and
disable the clocks while there are no transfers in progress for a small
power saving. The auto_runtime_pm feature in the core will do the
runtime PM calls for you.
Otherwise this looks good.
signature.asc
Description: Digital signature
Re: [PATCH v3 1/2] SPI: Add driver for Cadence SPI controller
Hi Mark, On Thu, Apr 10, 2014 at 5:51 PM, Mark Brown wrote: > On Thu, Apr 10, 2014 at 05:43:49PM +0530, Harini Katakam wrote: > >> Firstly, the timeout value obtained from this is a too low. >> This timeout is typically used for an error conditions such as >> hardware hang etc. and using a value >1*HZ would be better. >> This driver used to use similar timeout and as I understand, other >> drivers also use similar values. > > Send patches... Ok thanks, I will. Regards, Harini -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] SPI: Add driver for Cadence SPI controller
On Thu, Apr 10, 2014 at 05:43:49PM +0530, Harini Katakam wrote: > Firstly, the timeout value obtained from this is a too low. > This timeout is typically used for an error conditions such as > hardware hang etc. and using a value >1*HZ would be better. > This driver used to use similar timeout and as I understand, other > drivers also use similar values. Send patches... signature.asc Description: Digital signature
[PATCH v3 1/2] SPI: Add driver for Cadence SPI controller
Add driver for Cadence SPI controller. This is used in Xilinx Zynq. Signed-off-by: Harini Katakam --- Here is the v3 series but I have one concern. The recent change in spi-core to use wait_for_completion_timeout uses a timeout value calculated as follows: ms = xfer->len * 8 * 1000/xfer->speed_hz; wait_for_completion_timeout(msecs_to_jiffies(ms)); Firstly, the timeout value obtained from this is a too low. This timeout is typically used for an error conditions such as hardware hang etc. and using a value >1*HZ would be better. This driver used to use similar timeout and as I understand, other drivers also use similar values. Second, the xfer->speed_hz used here is not necessarily the actual speed set. The IP allows a limited set of divisors and the divisor is chosen in driver such that the actual speed is <= requested speed. Due to this the timeout value set here, sometimes falls short of the actual time required to complete transfer; this happens especially with very low frequencies. The difference in time higher than 20msecs. And for very small transfers, the timeout is set to 1 jiffy. Also, this is not considering any interrupt latencies or if the system is fully loaded. This might be cutting it too close, can the timeout value in core be increased? v3 changes: - Remove setup function. Make clock CPOL/CPHA setup a separate funtion and call this from prepare_transfer_hardware before enabling. - Remove clk_enable/disable in prepare/unprepare_tranfer_hardware. Clock enable/diable pairs will be retained as before only in probe/remove and suspend/resume. Use of runtime_pms for efficiency will be planned later. - Remove /bits/ in bindings for num-cs, use temporary u32 variable in driver. - Rename requested_bytes and remaining_bytes to tx_bytes/rx_bytes. - Remove unused macros. - Add is-decoded-cs property and add support in driver set_cs funtion. v2 changes: - Use xilinx compatible string too. - Changes read register and write register functions to static inline. - Removed unecessary dev_info and dev_dbg prints. - Return IRQ_HANDLED only when interrupt is handled. - Use a default num-cs value. - Do init_hardware before requesting irq. - Remove unecessary master_put() - Set master->max_speed_hz - Check for busy in cdns_setup(). Retained this function with this check as opposed to removing. The reason for this is clock configuration needs to be done for the first time before enable is done in prepare_hardware; prepare_hardware however, doesn't receive spi_device structure. - Implememnt transfer_one instead of transfer_one_message. Remove wait_for_completion as this is done by core. - Implement set_cs. - Clock enable/disable in prepare/unprepare respectively. - In suspend, remove reset of hardware; simply call unprepare_hardware. - In suspend/resume call master_suspend/resume respectively. - Check for irq<=0 in probe. - Remove MODULE_ALIAS. --- drivers/spi/Kconfig |7 + drivers/spi/Makefile |1 + drivers/spi/spi-cadence.c | 673 + 3 files changed, 681 insertions(+) create mode 100644 drivers/spi/spi-cadence.c diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index efe1960..b0f091b 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -148,6 +148,13 @@ config SPI_BUTTERFLY inexpensive battery powered microcontroller evaluation board. This same cable can be used to flash new firmware. +config SPI_CADENCE + tristate "Cadence SPI controller" + depends on SPI_MASTER + help + This selects the Cadence SPI controller master driver + used by Xilinx Zynq. + config SPI_CLPS711X tristate "CLPS711X host SPI controller" depends on ARCH_CLPS711X || COMPILE_TEST diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index bd79266..3e503d6 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -22,6 +22,7 @@ obj-$(CONFIG_SPI_BFIN_V3) += spi-bfin-v3.o obj-$(CONFIG_SPI_BFIN_SPORT) += spi-bfin-sport.o obj-$(CONFIG_SPI_BITBANG) += spi-bitbang.o obj-$(CONFIG_SPI_BUTTERFLY)+= spi-butterfly.o +obj-$(CONFIG_SPI_CADENCE) += spi-cadence.o obj-$(CONFIG_SPI_CLPS711X) += spi-clps711x.o obj-$(CONFIG_SPI_COLDFIRE_QSPI)+= spi-coldfire-qspi.o obj-$(CONFIG_SPI_DAVINCI) += spi-davinci.o diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c new file mode 100644 index 000..9a8289e --- /dev/null +++ b/drivers/spi/spi-cadence.c @@ -0,0 +1,673 @@ +/* + * Cadence SPI controller driver (master mode only) + * + * Copyright (C) 2008 - 2014 Xilinx, Inc. + * + * based on Blackfin On-Chip SPI Driver (spi_bfin5xx.c) + * + * This program is free software; you can redistribute it and/or modify it under + * the terms of the GNU General Public License version 2 as published by the + * Free Software Foundation; either version 2 of the License, or (at
[PATCH v3 1/2] SPI: Add driver for Cadence SPI controller
Add driver for Cadence SPI controller. This is used in Xilinx Zynq. Signed-off-by: Harini Katakam hari...@xilinx.com --- Here is the v3 series but I have one concern. The recent change in spi-core to use wait_for_completion_timeout uses a timeout value calculated as follows: ms = xfer-len * 8 * 1000/xfer-speed_hz; wait_for_completion_timeout(msecs_to_jiffies(ms)); Firstly, the timeout value obtained from this is a too low. This timeout is typically used for an error conditions such as hardware hang etc. and using a value 1*HZ would be better. This driver used to use similar timeout and as I understand, other drivers also use similar values. Second, the xfer-speed_hz used here is not necessarily the actual speed set. The IP allows a limited set of divisors and the divisor is chosen in driver such that the actual speed is = requested speed. Due to this the timeout value set here, sometimes falls short of the actual time required to complete transfer; this happens especially with very low frequencies. The difference in time higher than 20msecs. And for very small transfers, the timeout is set to 1 jiffy. Also, this is not considering any interrupt latencies or if the system is fully loaded. This might be cutting it too close, can the timeout value in core be increased? v3 changes: - Remove setup function. Make clock CPOL/CPHA setup a separate funtion and call this from prepare_transfer_hardware before enabling. - Remove clk_enable/disable in prepare/unprepare_tranfer_hardware. Clock enable/diable pairs will be retained as before only in probe/remove and suspend/resume. Use of runtime_pms for efficiency will be planned later. - Remove /bits/ in bindings for num-cs, use temporary u32 variable in driver. - Rename requested_bytes and remaining_bytes to tx_bytes/rx_bytes. - Remove unused macros. - Add is-decoded-cs property and add support in driver set_cs funtion. v2 changes: - Use xilinx compatible string too. - Changes read register and write register functions to static inline. - Removed unecessary dev_info and dev_dbg prints. - Return IRQ_HANDLED only when interrupt is handled. - Use a default num-cs value. - Do init_hardware before requesting irq. - Remove unecessary master_put() - Set master-max_speed_hz - Check for busy in cdns_setup(). Retained this function with this check as opposed to removing. The reason for this is clock configuration needs to be done for the first time before enable is done in prepare_hardware; prepare_hardware however, doesn't receive spi_device structure. - Implememnt transfer_one instead of transfer_one_message. Remove wait_for_completion as this is done by core. - Implement set_cs. - Clock enable/disable in prepare/unprepare respectively. - In suspend, remove reset of hardware; simply call unprepare_hardware. - In suspend/resume call master_suspend/resume respectively. - Check for irq=0 in probe. - Remove MODULE_ALIAS. --- drivers/spi/Kconfig |7 + drivers/spi/Makefile |1 + drivers/spi/spi-cadence.c | 673 + 3 files changed, 681 insertions(+) create mode 100644 drivers/spi/spi-cadence.c diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index efe1960..b0f091b 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -148,6 +148,13 @@ config SPI_BUTTERFLY inexpensive battery powered microcontroller evaluation board. This same cable can be used to flash new firmware. +config SPI_CADENCE + tristate Cadence SPI controller + depends on SPI_MASTER + help + This selects the Cadence SPI controller master driver + used by Xilinx Zynq. + config SPI_CLPS711X tristate CLPS711X host SPI controller depends on ARCH_CLPS711X || COMPILE_TEST diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index bd79266..3e503d6 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -22,6 +22,7 @@ obj-$(CONFIG_SPI_BFIN_V3) += spi-bfin-v3.o obj-$(CONFIG_SPI_BFIN_SPORT) += spi-bfin-sport.o obj-$(CONFIG_SPI_BITBANG) += spi-bitbang.o obj-$(CONFIG_SPI_BUTTERFLY)+= spi-butterfly.o +obj-$(CONFIG_SPI_CADENCE) += spi-cadence.o obj-$(CONFIG_SPI_CLPS711X) += spi-clps711x.o obj-$(CONFIG_SPI_COLDFIRE_QSPI)+= spi-coldfire-qspi.o obj-$(CONFIG_SPI_DAVINCI) += spi-davinci.o diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c new file mode 100644 index 000..9a8289e --- /dev/null +++ b/drivers/spi/spi-cadence.c @@ -0,0 +1,673 @@ +/* + * Cadence SPI controller driver (master mode only) + * + * Copyright (C) 2008 - 2014 Xilinx, Inc. + * + * based on Blackfin On-Chip SPI Driver (spi_bfin5xx.c) + * + * This program is free software; you can redistribute it and/or modify it under + * the terms of the GNU General Public License version 2 as published by the + * Free Software Foundation; either version 2 of the License,
Re: [PATCH v3 1/2] SPI: Add driver for Cadence SPI controller
On Thu, Apr 10, 2014 at 05:43:49PM +0530, Harini Katakam wrote: Firstly, the timeout value obtained from this is a too low. This timeout is typically used for an error conditions such as hardware hang etc. and using a value 1*HZ would be better. This driver used to use similar timeout and as I understand, other drivers also use similar values. Send patches... signature.asc Description: Digital signature
Re: [PATCH v3 1/2] SPI: Add driver for Cadence SPI controller
Hi Mark, On Thu, Apr 10, 2014 at 5:51 PM, Mark Brown broo...@kernel.org wrote: On Thu, Apr 10, 2014 at 05:43:49PM +0530, Harini Katakam wrote: Firstly, the timeout value obtained from this is a too low. This timeout is typically used for an error conditions such as hardware hang etc. and using a value 1*HZ would be better. This driver used to use similar timeout and as I understand, other drivers also use similar values. Send patches... Ok thanks, I will. Regards, Harini -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/2] SPI: Add driver for Cadence SPI controller
On Thu, Apr 10, 2014 at 05:43:49PM +0530, Harini Katakam wrote:
Add driver for Cadence SPI controller. This is used in Xilinx Zynq.
This looks mostly good, the issues below are very small.
+static int __maybe_unused cdns_spi_suspend(struct device *dev)
+{
+ struct platform_device *pdev = container_of(dev,
+ struct platform_device, dev);
+ struct spi_master *master = platform_get_drvdata(pdev);
+ struct cdns_spi *xspi = spi_master_get_devdata(master);
+
+ spi_master_suspend(master);
+
+ clk_disable(xspi-ref_clk);
+
+ clk_disable(xspi-pclk);
You're only doing clk_disable() here, I would expect the clocks to also
be unprepared over suspend - there is no value in leaving them prepared
that I can see. Just use clk_unprepare_disable().
It would also be good (though not essential) to implement runtime PM and
disable the clocks while there are no transfers in progress for a small
power saving. The auto_runtime_pm feature in the core will do the
runtime PM calls for you.
Otherwise this looks good.
signature.asc
Description: Digital signature
Re: [PATCH v3 1/2] SPI: Add driver for Cadence SPI controller
Hi Mark,
On Fri, Apr 11, 2014 at 4:18 AM, Mark Brown broo...@kernel.org wrote:
On Thu, Apr 10, 2014 at 05:43:49PM +0530, Harini Katakam wrote:
Add driver for Cadence SPI controller. This is used in Xilinx Zynq.
This looks mostly good, the issues below are very small.
+static int __maybe_unused cdns_spi_suspend(struct device *dev)
+{
+ struct platform_device *pdev = container_of(dev,
+ struct platform_device, dev);
+ struct spi_master *master = platform_get_drvdata(pdev);
+ struct cdns_spi *xspi = spi_master_get_devdata(master);
+
+ spi_master_suspend(master);
+
+ clk_disable(xspi-ref_clk);
+
+ clk_disable(xspi-pclk);
You're only doing clk_disable() here, I would expect the clocks to also
be unprepared over suspend - there is no value in leaving them prepared
that I can see. Just use clk_unprepare_disable().
It would also be good (though not essential) to implement runtime PM and
disable the clocks while there are no transfers in progress for a small
power saving. The auto_runtime_pm feature in the core will do the
runtime PM calls for you.
Otherwise this looks good.
Thanks. I'll make these changes and send a v4 next week.
Regards,
Harini
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
