Re: [PATCH] spi: tegra114: add spi driver

2013-02-20 Thread Laxman Dewangan 
On Tuesday 19 February 2013 11:46 PM, Stephen Warren wrote:
 On 02/19/2013 06:38 AM, Laxman Dewangan wrote:
 +bits_per_word = t-bits_per_word ? t-bits_per_word :
 +spi-bits_per_word;
 I thought I'd seen patches so this conditional wasn't needed any more;
 isn't t-bit_per_word always set correctly by the SPI core now?
 Certainly the existing spi-tegra20-slink.c doesn't seem to have any
 conditional here.

Yes, core have changes. I will remove this check.



 A similar comment applies in tegra_spi_read_rx_fifo_to_client_rxbuf()
 and tegra_spi_copy_spi_rxbuf_to_client_rxbuf().

 +total_fifo_words = (max_len + 3)/4;
 Need spaces around /. The same comment applies in some other places;
 please search for them. Was checkpatch run? I'm not sure if catches this.

 spi-tegra20-slink.c doesn't have that rounding; is just says:

  total_fifo_words = max_len / 4;

 Is that a bug in the old driver?


I will check and fix the either place.



 +if (tspi-cur_direction  DATA_DIR_TX) {
 +tegra_spi_copy_client_txbuf_to_spi_txbuf(tspi, t);
 +ret = tegra_spi_start_tx_dma(tspi, len);
 In spi-tegra20-slink.c, there's a wmb() right between those last two
 lines. Is it needed here?

I think wmb() is no require and hence not keeping here. Perhaps I got 
the review feedback when I was working on serial tegra driver.



 +static int tegra_spi_start_transfer_one(struct spi_device *spi,
 +struct spi_transfer *t, bool is_first_of_msg,
 +bool is_single_xfer)
 ...
 +/* possibly use the hw based chip select */
 +command1 |= SPI_CS_SW_HW;
 +if (spi-mode  SPI_CS_HIGH)
 +command1 |= SPI_CS_SS_VAL;
 +else
 +command1 = ~SPI_CS_SS_VAL;
 Why possibly; the code seems to always use HW chip select.


Yes, wrong comment. Remove the controller_data from driver but forgot to 
remove this comment.

 +ret = pm_runtime_get_sync(tspi-dev);
 +if (ret  0) {
 +dev_err(tspi-dev, runtime PM get failed: %d\n, ret);
 +msg-status = ret;
 +spi_finalize_current_message(master);
 +return ret;
 +}
 In the older Tegra SPI drivers, the PM runtime logic was was of
 master-{un,}prepare_transfer. I'm curious why it's implemented
 differently here.

The prepare  is called in atomic context and in this we are calling 
pm_runtime_get_sync() which is blocking and it can cause issue.

I have already bug reported by you that sometimes you saw locking in 
tegra20 slink driver which we need to fix. When testing this, I ran into 
similar case and hence now moving this out or prepare.

I will push the change for fixing this in tegra20_slink driver also.

 +prop = of_get_property(np, spi-max-frequency, NULL);
 +if (prop)
 +tspi-spi_max_frequency = be32_to_cpup(prop);
 The following might be better:

  if (of_property_read_u32(np, spi-max-frequency,
  tspi-spi_max_frequency))
  tspi-spi_max_frequency = 2500; /* 25MHz */

 (and you can remove the check of !tspi-spi_max_frequency from probe()
 then too)

Yes, Agree. Will do.


 +static int tegra_spi_probe(struct platform_device *pdev)
 ...
 +if (!pdev-dev.of_node) {
 +dev_err(pdev-dev, Driver support DT registration only\n);
 +return -ENODEV;
 +}
 I don't think there's much point checking that; see the Tegra20 SPI
 cleanup patches I posted a couple days ago.

 +tspi-base = devm_request_and_ioremap(pdev-dev, r);
 +if (!tspi-base) {
 The existing Tegra20 driver checks if (IS_ERR(tspi-base)) here. Which
 is wrong?
The tegra20 driver use the devm_ioremap_resource() which is new API get 
added recently. I will change this driver to use this one.



 +tspi-clk = devm_clk_get(pdev-dev, spi);
 Does this HW block use multiple clocks? If not, I think s/spi/NULL/
 there, just like the Tegra20 driver.

No, spi controller uses the only one clock. I will change to NULL.



 As an overall comment, this driver is textually perhaps 80-90% the same
 as spi-tegra20-slink.c. Instead of creating a completely new driver, how
 nasty would a unified driver look; one which contained some runtime
 conditionals for the register layout and programming differences? It
 might be worth looking at, although perhaps it would turn out to be a
 crazy mess, so a separate driver really is appropriate.

We had created the similarly in past where common part is in same file 
and controller specific is in the different file as hal file but the 
driver becomes complex.
It was not easy to add any feature as the require api need to be added 
in all places for hal.  We were about to split the driver separately but 
before then we moved to Linux.

So first look, it is great but adding feature/maintaining/enhancing is 
difficult.
I like to go with separate driver until there is much pushback

Re: [PATCH] spi: tegra114: add spi driver

2013-02-20 Thread Laxman Dewangan 
On Wednesday 20 February 2013 06:41 PM, Mark Brown wrote:
 * PGP Signed by an unknown key

 On Wed, Feb 20, 2013 at 05:59:03PM +0530, Laxman Dewangan wrote:
 +  tspi-clk = devm_clk_get(pdev-dev, spi);
 Does this HW block use multiple clocks? If not, I think s/spi/NULL/
 there, just like the Tegra20 driver.
 No, spi controller uses the only one clock. I will change to NULL.
 I'm never convinced that NULL is a helpful clock name to pick, it's not
 awesome if you ever acquire a second clock.

I had other idea of okeeping clock name here for power optimization.
Spi controller can take the power source ffrom different clock source 
say clk_m (crystal) and pllp.

I want to set the parent clock dynamically based on required speed so 
that if the desire speed can be meet from clk_m, no need to increase pll 
count and possible we may endup with siwtchng off pllp which can save power.

So for this I may require

spi_clk = devm_clk_get(pdev-dev, spi);
spi_clkm = devm_clk_get(pdev-dev, clmkm);
spi_pllp = devm_clk_get(pdev-dev, pllp);

and call clk_set_parent(spi_clk, spi_clkm) or clk_set_parent(spi_clk, spi_pllp).








--
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_feb
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

Re: [PATCH] spi: tegra114: add spi driver

2013-02-20 Thread Stephen Warren 
On 02/20/2013 06:26 AM, Laxman Dewangan wrote:
 On Wednesday 20 February 2013 06:41 PM, Mark Brown wrote:
 * PGP Signed by an unknown key

 On Wed, Feb 20, 2013 at 05:59:03PM +0530, Laxman Dewangan wrote:
 +tspi-clk = devm_clk_get(pdev-dev, spi);
 Does this HW block use multiple clocks? If not, I think s/spi/NULL/
 there, just like the Tegra20 driver.
 No, spi controller uses the only one clock. I will change to NULL.
 I'm never convinced that NULL is a helpful clock name to pick, it's not
 awesome if you ever acquire a second clock.
 
 I had other idea of okeeping clock name here for power optimization.
 Spi controller can take the power source ffrom different clock source
 say clk_m (crystal) and pllp.
 
 I want to set the parent clock dynamically based on required speed so
 that if the desire speed can be meet from clk_m, no need to increase pll
 count and possible we may endup with siwtchng off pllp which can save
 power.
 
 So for this I may require
 
 spi_clk = devm_clk_get(pdev-dev, spi);
 spi_clkm = devm_clk_get(pdev-dev, clmkm);
 spi_pllp = devm_clk_get(pdev-dev, pllp);
 
 and call clk_set_parent(spi_clk, spi_clkm) or clk_set_parent(spi_clk,
 spi_pllp).

OK, so that's certainly an argument for requesting a specific clock name
rather than NULL.

But, please do think this approach through fully. The DT binding needs
to define which clock-names the driver requires to be present, and any
optional clock names. DT bindings are supposed to be immutable, or
perhaps extendible in a completely backwards-compatible fashion. This
implies that you need to have thought through the entire list of clocks
that the driver might want in the DT clock-names property when you first
write the DT binding documentation...

--
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_feb
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

Re: [PATCH] spi: tegra114: add spi driver

2013-02-20 Thread Stephen Warren 
On 02/20/2013 10:31 AM, Mark Brown wrote:
 On Wed, Feb 20, 2013 at 10:25:13AM -0700, Stephen Warren wrote:
 
 But, please do think this approach through fully. The DT binding
 needs to define which clock-names the driver requires to be
 present, and any optional clock names. DT bindings are supposed
 to be immutable, or perhaps extendible in a completely
 backwards-compatible fashion. This implies that you need to have
 thought through the entire list of clocks that the driver might
 want in the DT clock-names property when you first write the DT
 binding documentation...
 
 Since we can extend the list of clocks it doesn't seem like there's
 much issue here, especially if some of them are optional?

Yes, there's certainly a way to extend the binding in a
backwards-compatible way.

However, I have seen in Rob and/or Grant push back on not fully
defining bindings in the past - i.e. actively planning to initially
create a minimal binding and extend it in the future, rather than
completely defining it up-front.

I don't know how strong of a rule they intend that to be though. If we
get to the point of moving the DT bindings out of the kernel, it'd be
good to get a concrete definition of what can and can't be changed in
bindings.

 Though in general it seems like this sort of mux really should be
 in the clock stuff anyway.

How do you see that working: something automatic inside clk_set_rate()
seeing that some other parent could provide the rate, so the clock
could be reparented, or ...?

--
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_feb
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

Re: [PATCH] spi: tegra114: add spi driver

2013-02-20 Thread Stephen Warren 
On 02/20/2013 10:57 AM, Mark Brown wrote:
 On Wed, Feb 20, 2013 at 10:36:41AM -0700, Stephen Warren wrote:
 On 02/20/2013 10:31 AM, Mark Brown wrote:
 
 Since we can extend the list of clocks it doesn't seem like
 there's much issue here, especially if some of them are
 optional?
 
 Yes, there's certainly a way to extend the binding in a 
 backwards-compatible way.
 
 However, I have seen in Rob and/or Grant push back on not fully 
 defining bindings in the past - i.e. actively planning to
 initially create a minimal binding and extend it in the future,
 rather than completely defining it up-front.
 
 That sounds like the current stuff with a minimal definition is
 OK?

I'm personally OK with defining a minimal binding first and extending
it later. But, I'm worried if when we actually try to extend the
binding later, we'll get push-back.

--
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_feb
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

Re: [PATCH] spi: tegra114: add spi driver

2013-02-20 Thread Laxman Dewangan 
On Wednesday 20 February 2013 11:30 PM, Stephen Warren wrote:
 On 02/20/2013 10:57 AM, Mark Brown wrote:
 On Wed, Feb 20, 2013 at 10:36:41AM -0700, Stephen Warren wrote:
 On 02/20/2013 10:31 AM, Mark Brown wrote:
 Since we can extend the list of clocks it doesn't seem like
 there's much issue here, especially if some of them are
 optional?
 Yes, there's certainly a way to extend the binding in a
 backwards-compatible way.
 However, I have seen in Rob and/or Grant push back on not fully
 defining bindings in the past - i.e. actively planning to
 initially create a minimal binding and extend it in the future,
 rather than completely defining it up-front.
 That sounds like the current stuff with a minimal definition is
 OK?
 I'm personally OK with defining a minimal binding first and extending
 it later. But, I'm worried if when we actually try to extend the
 binding later, we'll get push-back.

Yes, for a given controller there is lots of input sources which can be 
mux but we can not use all option as some of source is changeable based 
on DVFS policy or other constraints. Like one of controller has the 
input clock source as PLLC which is again used by CPU and it varies for 
requested CPU frequency. In this context, we would like to not choose 
PLLC as clock source for given controller.

So we may need to provide the list of valid clock source/option from DT 
file and clock muxing should be done from that source list only in place 
of super set supported by SoCs.


--
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_feb
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general

[PATCH V2] spi: tegra114: add spi driver

2013-02-20 Thread Laxman Dewangan 
Add SPI driver for NVIDIA's Tegra114 SPI controller. This controller
is different than the older SoCs SPI controller in internal design as
well as register interface.

This driver supports the:
- non DMA based transfer for smaller transfer i.e. less than FIFO depth.
- APB DMA based transfer for lager transfer i.e. more than FIFO depth.
- Clock gating through runtime PM callbacks.
- registration through DT only.

Signed-off-by: Laxman Dewangan ldewan...@nvidia.com
---
Changes from V1:
- All nit cleanups for nomenclature like Nvidia to NVIDIA, spi to SPI etc.
- remove bits_per_word check as all transfer have this valid parameter.
- Cleanups in dt parsing and remove dt node validity.
- use devm_ioremap_resource for mapping physical address.

 .../bindings/spi/nvidia,tegra114-spi.txt   |   25 +
 drivers/spi/Kconfig|8 +
 drivers/spi/Makefile   |1 +
 drivers/spi/spi-tegra114.c | 1246 
 4 files changed, 1280 insertions(+), 0 deletions(-)
 create mode 100644 
Documentation/devicetree/bindings/spi/nvidia,tegra114-spi.txt
 create mode 100644 drivers/spi/spi-tegra114.c

diff --git a/Documentation/devicetree/bindings/spi/nvidia,tegra114-spi.txt 
b/Documentation/devicetree/bindings/spi/nvidia,tegra114-spi.txt
new file mode 100644
index 000..c6457e9
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/nvidia,tegra114-spi.txt
@@ -0,0 +1,25 @@
+NVIDIA Tegra114 SPI controller.
+
+Required properties:
+- compatible : should be nvidia,tegra114-spi.
+- reg: Should contain SPI registers location and length.
+- interrupts: Should contain SPI interrupts.
+- nvidia,dma-request-selector : The Tegra DMA controller's phandle and
+  request selector for this SPI controller.
+
+Recommended properties:
+- spi-max-frequency: Definition as per
+ Documentation/devicetree/bindings/spi/spi-bus.txt
+Example:
+
+spi@7000d600 {
+   compatible = nvidia,tegra114-spi;
+   reg = 0x7000d600 0x200;
+   interrupts = 0 82 0x04;
+   nvidia,dma-request-selector = apbdma 16;
+   spi-max-frequency = 2500;
+   #address-cells = 1;
+   #size-cells = 0;
+   status = disabled;
+};
+
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index f80eee7..10699bb 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -405,6 +405,14 @@ config SPI_TEGRA20_SFLASH
  The main usecase of this controller is to use spi flash as boot
  device.
 
+config SPI_TEGRA114
+   tristate NVIDIA Tegra114 SPI Controller
+   depends on ARCH_TEGRA  TEGRA20_APB_DMA
+   help
+ SPI driver for NVIDIA Tegra114 SPI Controller interface. This 
controller
+ is different than the older SoCs SPI controller and also register 
interface
+ get changed with this controller.
+
 config SPI_TEGRA20_SLINK
tristate Nvidia Tegra20/Tegra30 SLINK Controller
depends on ARCH_TEGRA  TEGRA20_APB_DMA
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index e53c309..ee2a119 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_SPI_SH_SCI)  += spi-sh-sci.o
 obj-$(CONFIG_SPI_SIRF) += spi-sirf.o
 obj-$(CONFIG_SPI_TEGRA20_SFLASH)   += spi-tegra20-sflash.o
 obj-$(CONFIG_SPI_TEGRA20_SLINK)+= spi-tegra20-slink.o
+obj-$(CONFIG_SPI_TEGRA114) += spi-tegra114.o
 obj-$(CONFIG_SPI_TI_SSP)   += spi-ti-ssp.o
 obj-$(CONFIG_SPI_TLE62X0)  += spi-tle62x0.o
 obj-$(CONFIG_SPI_TOPCLIFF_PCH) += spi-topcliff-pch.o
diff --git a/drivers/spi/spi-tegra114.c b/drivers/spi/spi-tegra114.c
new file mode 100644
index 000..35b1e94
--- /dev/null
+++ b/drivers/spi/spi-tegra114.c
@@ -0,0 +1,1246 @@
+/*
+ * SPI driver for NVIDIA's Tegra114 SPI Controller.
+ *
+ * Copyright (c) 2013, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/.
+ */
+
+#include linux/clk.h
+#include linux/clk/tegra.h
+#include linux/completion.h
+#include linux/delay.h
+#include linux/dmaengine.h
+#include linux/dma-mapping.h
+#include linux/dmapool.h
+#include linux/err.h
+#include linux/init.h
+#include linux/interrupt.h
+#include linux/io.h
+#include linux/kernel.h
+#include linux/kthread.h
+#include linux/module.h
+#include linux/platform_device.h
+#include linux/pm_runtime.h
+#include linux/of.h