[PATCH v2 1/4] spi: rockchip: fix error handling when probe

[PATCH v2 1/4] spi: rockchip: fix error handling when probe

[Jeffy Chen]

After failed to request dma tx chain, we need to disable pm_runtime.
Also cleanup error labels for better readability.

Signed-off-by: Jeffy Chen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

Changes in v2: None

 drivers/spi/spi-rockchip.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index acf31f3..bab9b13 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -684,33 +684,33 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 	rs->regs = devm_ioremap_resource(&pdev->dev, mem);
 	if (IS_ERR(rs->regs)) {
 		ret =  PTR_ERR(rs->regs);
-		goto err_ioremap_resource;
+		goto err_put_master;
 	}
 
 	rs->apb_pclk = devm_clk_get(&pdev->dev, "apb_pclk");
 	if (IS_ERR(rs->apb_pclk)) {
 		dev_err(&pdev->dev, "Failed to get apb_pclk\n");
 		ret = PTR_ERR(rs->apb_pclk);
-		goto err_ioremap_resource;
+		goto err_put_master;
 	}
 
 	rs->spiclk = devm_clk_get(&pdev->dev, "spiclk");
 	if (IS_ERR(rs->spiclk)) {
 		dev_err(&pdev->dev, "Failed to get spi_pclk\n");
 		ret = PTR_ERR(rs->spiclk);
-		goto err_ioremap_resource;
+		goto err_put_master;
 	}
 
 	ret = clk_prepare_enable(rs->apb_pclk);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to enable apb_pclk\n");
-		goto err_ioremap_resource;
+		goto err_put_master;
 	}
 
 	ret = clk_prepare_enable(rs->spiclk);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to enable spi_clk\n");
-		goto err_spiclk_enable;
+		goto err_disable_apbclk;
 	}
 
 	spi_enable_chip(rs, 0);
@@ -728,7 +728,7 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 	if (!rs->fifo_len) {
 		dev_err(&pdev->dev, "Failed to get fifo length\n");
 		ret = -EINVAL;
-		goto err_get_fifo_len;
+		goto err_disable_spiclk;
 	}
 
 	spin_lock_init(&rs->lock);
@@ -755,7 +755,7 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 		/* Check tx to see if we need defer probing driver */
 		if (PTR_ERR(rs->dma_tx.ch) == -EPROBE_DEFER) {
 			ret = -EPROBE_DEFER;
-			goto err_get_fifo_len;
+			goto err_disable_pm_runtime;
 		}
 		dev_warn(rs->dev, "Failed to request TX DMA channel\n");
 		rs->dma_tx.ch = NULL;
@@ -786,23 +786,24 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 	ret = devm_spi_register_master(&pdev->dev, master);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to register master\n");
-		goto err_register_master;
+		goto err_free_dma_rx;
 	}
 
 	return 0;
 
-err_register_master:
-	pm_runtime_disable(&pdev->dev);
+err_free_dma_rx:
 	if (rs->dma_rx.ch)
 		dma_release_channel(rs->dma_rx.ch);
 err_free_dma_tx:
 	if (rs->dma_tx.ch)
 		dma_release_channel(rs->dma_tx.ch);
-err_get_fifo_len:
+err_disable_pm_runtime:
+	pm_runtime_disable(&pdev->dev);
+err_disable_spiclk:
 	clk_disable_unprepare(rs->spiclk);
-err_spiclk_enable:
+err_disable_apbclk:
 	clk_disable_unprepare(rs->apb_pclk);
-err_ioremap_resource:
+err_put_master:
 	spi_master_put(master);
 
 	return ret;
-- 
2.1.4


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

[PATCH v2 2/4] spi: rockchip: add support for "cs-gpios" dts property

[Jeffy Chen]

Support using "cs-gpios" property to specify cs gpios.

Signed-off-by: Jeffy Chen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
1/ request cs gpios in probe for better error handling
2/ use gpiod* function
(suggested by Heiko Stuebner)

3/ split dt-binding changes to new patch
(suggested by Shawn Lin & Heiko Stuebner)

---

Changes in v2: None

 drivers/spi/spi-rockchip.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index bab9b13..ad8997b 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -16,7 +16,7 @@
 #include <linux/clk.h>
 #include <linux/dmaengine.h>
 #include <linux/module.h>
-#include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/spi/spi.h>
@@ -663,6 +663,27 @@ static bool rockchip_spi_can_dma(struct spi_master *master,
 	return (xfer->len > rs->fifo_len);
 }
 
+static int rockchip_spi_setup_cs_gpios(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct gpio_desc *cs_gpio;
+	int i, nb;
+
+	if (!np)
+		return 0;
+
+	nb = of_gpio_named_count(np, "cs-gpios");
+	for (i = 0; i < nb; i++) {
+		/* We support both GPIO CS and HW CS */
+		cs_gpio = devm_gpiod_get_index_optional(dev, "cs",
+							i, GPIOD_ASIS);
+		if (IS_ERR(cs_gpio))
+			return PTR_ERR(cs_gpio);
+	}
+
+	return 0;
+}
+
 static int rockchip_spi_probe(struct platform_device *pdev)
 {
 	int ret = 0;
@@ -749,6 +770,7 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 	master->transfer_one = rockchip_spi_transfer_one;
 	master->max_transfer_size = rockchip_spi_max_transfer_size;
 	master->handle_err = rockchip_spi_handle_err;
+	master->flags = SPI_MASTER_GPIO_SS;
 
 	rs->dma_tx.ch = dma_request_chan(rs->dev, "tx");
 	if (IS_ERR(rs->dma_tx.ch)) {
@@ -783,6 +805,12 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 		master->dma_rx = rs->dma_rx.ch;
 	}
 
+	ret = rockchip_spi_setup_cs_gpios(&pdev->dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to setup cs gpios\n");
+		goto err_free_dma_rx;
+	}
+
 	ret = devm_spi_register_master(&pdev->dev, master);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to register master\n");
-- 
2.1.4


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

Re: [PATCH v2 2/4] spi: rockchip: add support for "cs-gpios" dts property

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1879 bytes --]

Hi Jeffy,

[auto build test ERROR on rockchip/for-next]
[also build test ERROR on v4.12-rc5 next-20170613]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jeffy-Chen/spi-rockchip-fix-error-handling-when-probe/20170613-172725
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git for-next
config: x86_64-randconfig-s5-06132355 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/spi/spi-rockchip.c: In function 'rockchip_spi_setup_cs_gpios':
>> drivers/spi/spi-rockchip.c:678:13: error: implicit declaration of function 'devm_gpiod_get_index_optional' [-Werror=implicit-function-declaration]
      cs_gpio = devm_gpiod_get_index_optional(dev, "cs",
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/spi/spi-rockchip.c:679:11: error: 'GPIOD_ASIS' undeclared (first use in this function)
           i, GPIOD_ASIS);
              ^~~~~~~~~~
   drivers/spi/spi-rockchip.c:679:11: note: each undeclared identifier is reported only once for each function it appears in
   cc1: some warnings being treated as errors

vim +/devm_gpiod_get_index_optional +678 drivers/spi/spi-rockchip.c

   672		if (!np)
   673			return 0;
   674	
   675		nb = of_gpio_named_count(np, "cs-gpios");
   676		for (i = 0; i < nb; i++) {
   677			/* We support both GPIO CS and HW CS */
 > 678			cs_gpio = devm_gpiod_get_index_optional(dev, "cs",
 > 679								i, GPIOD_ASIS);
   680			if (IS_ERR(cs_gpio))
   681				return PTR_ERR(cs_gpio);
   682		}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 21781 bytes --]

Re: [PATCH v2 2/4] spi: rockchip: add support for "cs-gpios" dts property

[Brian Norris]

Hi Jeffy,

On Tue, Jun 13, 2017 at 01:25:41PM +0800, Jeffy Chen wrote:
> Support using "cs-gpios" property to specify cs gpios.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> 1/ request cs gpios in probe for better error handling
> 2/ use gpiod* function
> (suggested by Heiko Stuebner)
> 
> 3/ split dt-binding changes to new patch
> (suggested by Shawn Lin & Heiko Stuebner)
> 
> ---
> 
> Changes in v2: None
> 
>  drivers/spi/spi-rockchip.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> index bab9b13..ad8997b 100644
> --- a/drivers/spi/spi-rockchip.c
> +++ b/drivers/spi/spi-rockchip.c
> @@ -16,7 +16,7 @@
>  #include <linux/clk.h>
>  #include <linux/dmaengine.h>
>  #include <linux/module.h>
> -#include <linux/of.h>
> +#include <linux/of_gpio.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/spi/spi.h>
> @@ -663,6 +663,27 @@ static bool rockchip_spi_can_dma(struct spi_master *master,
>  	return (xfer->len > rs->fifo_len);
>  }
>  
> +static int rockchip_spi_setup_cs_gpios(struct device *dev)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct gpio_desc *cs_gpio;
> +	int i, nb;
> +
> +	if (!np)
> +		return 0;
> +
> +	nb = of_gpio_named_count(np, "cs-gpios");
> +	for (i = 0; i < nb; i++) {
> +		/* We support both GPIO CS and HW CS */
> +		cs_gpio = devm_gpiod_get_index_optional(dev, "cs",
> +							i, GPIOD_ASIS);
> +		if (IS_ERR(cs_gpio))
> +			return PTR_ERR(cs_gpio);

I'm a bit confused why you need this function at all. You aren't using
the references that you're grabbing here, so essentially this is just
error-checking.

Are you doing anything here that isn't covered in
of_spi_register_master()?

> +	}
> +
> +	return 0;
> +}
> +
>  static int rockchip_spi_probe(struct platform_device *pdev)
>  {
>  	int ret = 0;
> @@ -749,6 +770,7 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>  	master->transfer_one = rockchip_spi_transfer_one;
>  	master->max_transfer_size = rockchip_spi_max_transfer_size;
>  	master->handle_err = rockchip_spi_handle_err;
> +	master->flags = SPI_MASTER_GPIO_SS;

I'm curious, do you actually need to assert both the HW and GPIO CS?

Brian

>  
>  	rs->dma_tx.ch = dma_request_chan(rs->dev, "tx");
>  	if (IS_ERR(rs->dma_tx.ch)) {
> @@ -783,6 +805,12 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>  		master->dma_rx = rs->dma_rx.ch;
>  	}
>  
> +	ret = rockchip_spi_setup_cs_gpios(&pdev->dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to setup cs gpios\n");
> +		goto err_free_dma_rx;
> +	}
> +
>  	ret = devm_spi_register_master(&pdev->dev, master);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to register master\n");
> -- 
> 2.1.4
> 
> 

Re: [PATCH v2 2/4] spi: rockchip: add support for "cs-gpios" dts property

[jeffy]

Hi Brian,

Thanx for your comments :)

On 06/14/2017 01:33 AM, Brian Norris wrote:
> Hi Jeffy,
>
> On Tue, Jun 13, 2017 at 01:25:41PM +0800, Jeffy Chen wrote:
>> Support using "cs-gpios" property to specify cs gpios.
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> 1/ request cs gpios in probe for better error handling
>> 2/ use gpiod* function
>> (suggested by Heiko Stuebner)
>>
>> 3/ split dt-binding changes to new patch
>> (suggested by Shawn Lin & Heiko Stuebner)
>>
>> ---
>>
>> Changes in v2: None
>>
>>   drivers/spi/spi-rockchip.c | 30 +++++++++++++++++++++++++++++-
>>   1 file changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
>> index bab9b13..ad8997b 100644
>> --- a/drivers/spi/spi-rockchip.c
>> +++ b/drivers/spi/spi-rockchip.c
>> @@ -16,7 +16,7 @@
>>   #include <linux/clk.h>
>>   #include <linux/dmaengine.h>
>>   #include <linux/module.h>
>> -#include <linux/of.h>
>> +#include <linux/of_gpio.h>
>>   #include <linux/pinctrl/consumer.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/spi/spi.h>
>> @@ -663,6 +663,27 @@ static bool rockchip_spi_can_dma(struct spi_master *master,
>>   	return (xfer->len > rs->fifo_len);
>>   }
>>
>> +static int rockchip_spi_setup_cs_gpios(struct device *dev)
>> +{
>> +	struct device_node *np = dev->of_node;
>> +	struct gpio_desc *cs_gpio;
>> +	int i, nb;
>> +
>> +	if (!np)
>> +		return 0;
>> +
>> +	nb = of_gpio_named_count(np, "cs-gpios");
>> +	for (i = 0; i < nb; i++) {
>> +		/* We support both GPIO CS and HW CS */
>> +		cs_gpio = devm_gpiod_get_index_optional(dev, "cs",
>> +							i, GPIOD_ASIS);
>> +		if (IS_ERR(cs_gpio))
>> +			return PTR_ERR(cs_gpio);
>
> I'm a bit confused why you need this function at all. You aren't using
> the references that you're grabbing here, so essentially this is just
> error-checking.

actually this is error-checking plus gpiod_request(see gpiod_get_index 
in gpiolib.c)
>
> Are you doing anything here that isn't covered in
> of_spi_register_master()?

expect for gpiod_request, another difference would be
when the of_spi_register_master calls of_get_named_gpio to parse 
cs-gpios, it would not do error handling here(fallback to HW CS):

         for (i = 0; i < nb; i++)
                 cs[i] = of_get_named_gpio(np, "cs-gpios", i);

but in our case, if something wrong happens(except for ENOENT), we 
cannot fallback to HW CS, because we already let pinctrl config GPIO CS.

>
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static int rockchip_spi_probe(struct platform_device *pdev)
>>   {
>>   	int ret = 0;
>> @@ -749,6 +770,7 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>>   	master->transfer_one = rockchip_spi_transfer_one;
>>   	master->max_transfer_size = rockchip_spi_max_transfer_size;
>>   	master->handle_err = rockchip_spi_handle_err;
>> +	master->flags = SPI_MASTER_GPIO_SS;
>
> I'm curious, do you actually need to assert both the HW and GPIO CS?

yes, it would hang if we do spi xfer with wrong HW CS register 
config(seems to be another controller limit)...

>
> Brian
>
>>
>>   	rs->dma_tx.ch = dma_request_chan(rs->dev, "tx");
>>   	if (IS_ERR(rs->dma_tx.ch)) {
>> @@ -783,6 +805,12 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>>   		master->dma_rx = rs->dma_rx.ch;
>>   	}
>>
>> +	ret = rockchip_spi_setup_cs_gpios(&pdev->dev);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Failed to setup cs gpios\n");
>> +		goto err_free_dma_rx;
>> +	}
>> +
>>   	ret = devm_spi_register_master(&pdev->dev, master);
>>   	if (ret) {
>>   		dev_err(&pdev->dev, "Failed to register master\n");
>> --
>> 2.1.4
>>
>>
>
>
>


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

[PATCH v2 3/4] dt-bindings: spi/rockchip: add "cs-gpios" optional property

[Jeffy Chen]

Update document devicetree bindings to support "cs-gpios" property.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v2: None

 Documentation/devicetree/bindings/spi/spi-rockchip.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.txt b/Documentation/devicetree/bindings/spi/spi-rockchip.txt
index 83da493..d0be2e6 100644
--- a/Documentation/devicetree/bindings/spi/spi-rockchip.txt
+++ b/Documentation/devicetree/bindings/spi/spi-rockchip.txt
@@ -25,6 +25,7 @@ Required Properties:
 
 Optional Properties:
 
+- cs-gpios : Specifies the gpio pins to be used for chipselects.
 - dmas: DMA specifiers for tx and rx dma. See the DMA client binding,
 		Documentation/devicetree/bindings/dma/dma.txt
 - dma-names: DMA request names should include "tx" and "rx" if present.
@@ -48,6 +49,7 @@ Example:
 		#address-cells = <1>;
 		#size-cells = <0>;
 		interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
+		cs-gpios = <&gpio2 23 GPIO_ACTIVE_HIGH>;
 		clocks = <&cru SCLK_SPI0>, <&cru PCLK_SPI0>;
 		clock-names = "spiclk", "apb_pclk";
 		pinctrl-0 = <&spi1_pins>;
-- 
2.1.4

Re: [PATCH v2 1/4] spi: rockchip: fix error handling when probe

[Brian Norris]

On Tue, Jun 13, 2017 at 01:25:40PM +0800, Jeffy Chen wrote:
> After failed to request dma tx chain, we need to disable pm_runtime.
> Also cleanup error labels for better readability.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
> Changes in v2: None

Looks good to me. I guess the original code was sort of trying to do a
different style of error handling, where you name the labels after the
point where the failure comes from (i.e., if ioremap fails, you 'goto
ioremap_fail' or something like that). But since we have enough devm_*
usage here that doesn't need unwound explicitly, and we didn't add
extra labels for all them, then that "style" doesn't really make much
sense.

Reviewed-by: Brian Norris <briannorris@chromium.org>

>  drivers/spi/spi-rockchip.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
> index acf31f3..bab9b13 100644
> --- a/drivers/spi/spi-rockchip.c
> +++ b/drivers/spi/spi-rockchip.c
> @@ -684,33 +684,33 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>  	rs->regs = devm_ioremap_resource(&pdev->dev, mem);
>  	if (IS_ERR(rs->regs)) {
>  		ret =  PTR_ERR(rs->regs);
> -		goto err_ioremap_resource;
> +		goto err_put_master;
>  	}
>  
>  	rs->apb_pclk = devm_clk_get(&pdev->dev, "apb_pclk");
>  	if (IS_ERR(rs->apb_pclk)) {
>  		dev_err(&pdev->dev, "Failed to get apb_pclk\n");
>  		ret = PTR_ERR(rs->apb_pclk);
> -		goto err_ioremap_resource;
> +		goto err_put_master;
>  	}
>  
>  	rs->spiclk = devm_clk_get(&pdev->dev, "spiclk");
>  	if (IS_ERR(rs->spiclk)) {
>  		dev_err(&pdev->dev, "Failed to get spi_pclk\n");
>  		ret = PTR_ERR(rs->spiclk);
> -		goto err_ioremap_resource;
> +		goto err_put_master;
>  	}
>  
>  	ret = clk_prepare_enable(rs->apb_pclk);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to enable apb_pclk\n");
> -		goto err_ioremap_resource;
> +		goto err_put_master;
>  	}
>  
>  	ret = clk_prepare_enable(rs->spiclk);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to enable spi_clk\n");
> -		goto err_spiclk_enable;
> +		goto err_disable_apbclk;
>  	}
>  
>  	spi_enable_chip(rs, 0);
> @@ -728,7 +728,7 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>  	if (!rs->fifo_len) {
>  		dev_err(&pdev->dev, "Failed to get fifo length\n");
>  		ret = -EINVAL;
> -		goto err_get_fifo_len;
> +		goto err_disable_spiclk;
>  	}
>  
>  	spin_lock_init(&rs->lock);
> @@ -755,7 +755,7 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>  		/* Check tx to see if we need defer probing driver */
>  		if (PTR_ERR(rs->dma_tx.ch) == -EPROBE_DEFER) {
>  			ret = -EPROBE_DEFER;
> -			goto err_get_fifo_len;
> +			goto err_disable_pm_runtime;
>  		}
>  		dev_warn(rs->dev, "Failed to request TX DMA channel\n");
>  		rs->dma_tx.ch = NULL;
> @@ -786,23 +786,24 @@ static int rockchip_spi_probe(struct platform_device *pdev)
>  	ret = devm_spi_register_master(&pdev->dev, master);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to register master\n");
> -		goto err_register_master;
> +		goto err_free_dma_rx;
>  	}
>  
>  	return 0;
>  
> -err_register_master:
> -	pm_runtime_disable(&pdev->dev);
> +err_free_dma_rx:
>  	if (rs->dma_rx.ch)
>  		dma_release_channel(rs->dma_rx.ch);
>  err_free_dma_tx:
>  	if (rs->dma_tx.ch)
>  		dma_release_channel(rs->dma_tx.ch);
> -err_get_fifo_len:
> +err_disable_pm_runtime:
> +	pm_runtime_disable(&pdev->dev);
> +err_disable_spiclk:
>  	clk_disable_unprepare(rs->spiclk);
> -err_spiclk_enable:
> +err_disable_apbclk:
>  	clk_disable_unprepare(rs->apb_pclk);
> -err_ioremap_resource:
> +err_put_master:
>  	spi_master_put(master);
>  
>  	return ret;
> -- 
> 2.1.4
> 
> 

Re: [PATCH v2 4/4] arm64: dts: rockchip: use cs-gpios for cros_ec_spi

[jeffy]

Hi Doug,

On 06/24/2017 12:14 AM, Doug Anderson wrote:
> Jeffy
>
> On Fri, Jun 23, 2017 at 5:18 AM, jeffy <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>>>>> So how do we fix this?  IMHO:
>>>>>
>>>>> Add 4 new pinctrl states in rk3399.dtsi:
>>>>>
>>>>>      cs_low_clk_low, cs_low_clk_high, cs_high_clk_low, cs_high_clk_high
>>>>>
>>>>> These would each look something like this:
>>>>>
>>>>> spi5_cs_low_data_low: spi5-cs-low-data-low {
>>>>>      rockchip,pins = <2 22 RK_FUNC_0 &pcfg_output_low>,
>>>>>        <2 23 RK_FUNC_0 &pcfg_output_low>;
>>>>> };
>>>>>
>>>>> Where "pcfg_output_low" would be moved from the existing location in
>>>>> "rk3399-gru.dtsi" to the main "rk3399.dtsi" file.
>>>>>
>>>>>
>>>>> ...now, you'd define runtime_suspend and runtime_resume functions
>>>>> where you'd look at the current state of the chip select and output
>>>>> and select one of these 4 pinmuxes so that things _don't_ change.
>>
>
> *** NOTE *** The more I look at this, the more I'm getting convinced
> that the right thing to do is to just disable Runtime PM while the
> chip select is asserted.  ...so probably you can just skip the next
> chunk of text and go straight to "PROPOSAL".
ok
>
>> it looks like the clk would be low when spi idle, so do we only need
>> *_clk_low?
>
> You're only looking at one polarity.  From Wikipedia
> <https://en.wikipedia.org/wiki/Serial_Peripheral_Interface_Bus>, the
> source of all that is "true":
>
> * At CPOL=0 the base value of the clock is zero, i.e. the idle state
> is 0 and active state is 1.
> -> For CPHA=0, data are captured and output on the clock's rising edge
> (low→high transition)
> -> For CPHA=1, data are captured and output on the clock's falling edge.
>
> * At CPOL=1 the base value of the clock is one (inversion of CPOL=0),
> i.e. the idle state is 1 and active state is 0.
> -> For CPHA=0, data are captured and output on clock's falling edge.
> -> For CPHA=1, data are captured and output on clock's rising edge.
>
> If you're adding code to the generic Rockchip SPI driver you need to
> handle both polarities.
>
>
>> and the rockchip spi supports 2 cs, so should we use cs0_low_cs1_low_clk_low
>> or should we put these pinmux into sub spi device?
>
> By default the pinctrl for rk3399.dtsi just sets up cs0, so I'd worry
> about that.  If someone wants to make cs1 work then they'd have to
> specify the right pinctrl there.
>
>
>>> * You'd want to add the pinmux configs to the main rk3399.dtsi file
>>> and then add code to the rockchip SPI driver to select the right
>>> pinmux (depending on the current state of the chip select and the
>>> current polarity) at runtime suspend.  ...then go back to "default"
>>> mode at runtime resume.
>>
>> i uploaded 2 testonly CLs:
>> remote:   https://chromium-review.googlesource.com/544391 TESTONLY: spi:
>> rockchip: use pinmux to config cs
>> remote:   https://chromium-review.googlesource.com/544392 TESTONLY: dts:
>> bob: only enable ec spi
>>
>> but they are not working well, not sure why, maybe cs still toggled will
>> switching pinmux? will use scope to check it next week.
>
> Yeah, scope is probably the right thing to do.  One thing you'd have
> to make sure is that everything is being done glitch free.  In other
> words, if this is happening:
>
> A) Pinmux to GPIO
> B) Set GPIO to output
> C) Drive GPIO state low
>
> Then that's bad because:
>
> After step A but before step B, you might still be an input and pulls
> might take effect.  Thus you could glitch the line.
>
> After step B but before step C, you might be outputting something else
> if the GPIO had previously been configured to output high.
>
>
> You need to make sure that the sequence is instead:
>
> A) Drive GPIO state high
> B) Set GPIO to output
> C) Pinmux to GPIO
>
>
> Ensuring things are glitch free and dealing with two chip selects
> means it might be cleaner would be to do all the GPIO stuff yourself
> in the driver.  Then you'd have to specify the GPIOs in the device
> tree.
>
> ======
>
> OK, I took a look at your CL and I'm now of the opinion that we should
> disable Runtime PM when the chip select is asserted.  Maybe just
> ignore the above and instead look at:
>
>
> PROPOSAL: Disable Runtime PM when chip select is asserted
>
> I think this proposal will be very easy and is pretty clean.
> Basically go into "rockchip_spi_set_cs()" and adjust the
> "pm_runtime_get_sync()" and "pm_runtime_put_sync()" calls.  Only call
> the "get_sync" at the top of the function if you're asserting the chip
> select (and it wasn't already asserted).  Only call the "put_sync" at
> the bottom of the function if you're deasserting the chip select (and
> it wasn't already deasserted).
hmm, looks like a better way to solve this, but i think we need to call 
pm_runtime_get_sync unconditionally to make sure the read ser register safe.
>
> This should avoid entering PM sleep any time a transaction is midway
> through happening.
>
> Secondly, make sure that the chip selects have a pullup on them (they
> already do).  When you Runtime PM then the SPI part will stop driving
> the pins and the pull will take effect.  Since we can only Runtime PM
> when the chip select is deasserted then this pull will always be
> correct.  Also: since we Runtime PM when the chip select is deasserted
> then the state of the other pins isn't terribly important (though to
> avoid leakage it's probably good to choose a sane pull).
>
>
> How does that sound?  It should only be a few lines of code and only one patch.
>
sounds good, new patch sent(https://patchwork.kernel.org/patch/9808601)
>
> -Doug
>
>
>


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

Re: [PATCH v2 4/4] arm64: dts: rockchip: use cs-gpios for cros_ec_spi

[jeffy]

Hi Doug,

On 06/24/2017 12:14 AM, Doug Anderson wrote:
> Jeffy
>
> On Fri, Jun 23, 2017 at 5:18 AM, jeffy <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>>>>> So how do we fix this?  IMHO:
>>>>>
>>>>> Add 4 new pinctrl states in rk3399.dtsi:
>>>>>
>>>>>      cs_low_clk_low, cs_low_clk_high, cs_high_clk_low, cs_high_clk_high
>>>>>
>>>>> These would each look something like this:
>>>>>
>>>>> spi5_cs_low_data_low: spi5-cs-low-data-low {
>>>>>      rockchip,pins = <2 22 RK_FUNC_0 &pcfg_output_low>,
>>>>>        <2 23 RK_FUNC_0 &pcfg_output_low>;
>>>>> };
>>>>>
>>>>> Where "pcfg_output_low" would be moved from the existing location in
>>>>> "rk3399-gru.dtsi" to the main "rk3399.dtsi" file.
>>>>>
>>>>>
>>>>> ...now, you'd define runtime_suspend and runtime_resume functions
>>>>> where you'd look at the current state of the chip select and output
>>>>> and select one of these 4 pinmuxes so that things _don't_ change.
>>
>
> *** NOTE *** The more I look at this, the more I'm getting convinced
> that the right thing to do is to just disable Runtime PM while the
> chip select is asserted.  ...so probably you can just skip the next
> chunk of text and go straight to "PROPOSAL".
ok
>
>> it looks like the clk would be low when spi idle, so do we only need
>> *_clk_low?
>
> You're only looking at one polarity.  From Wikipedia
> <https://en.wikipedia.org/wiki/Serial_Peripheral_Interface_Bus>, the
> source of all that is "true":
>
> * At CPOL=0 the base value of the clock is zero, i.e. the idle state
> is 0 and active state is 1.
> -> For CPHA=0, data are captured and output on the clock's rising edge
> (low→high transition)
> -> For CPHA=1, data are captured and output on the clock's falling edge.
>
> * At CPOL=1 the base value of the clock is one (inversion of CPOL=0),
> i.e. the idle state is 1 and active state is 0.
> -> For CPHA=0, data are captured and output on clock's falling edge.
> -> For CPHA=1, data are captured and output on clock's rising edge.
>
> If you're adding code to the generic Rockchip SPI driver you need to
> handle both polarities.
>
>
>> and the rockchip spi supports 2 cs, so should we use cs0_low_cs1_low_clk_low
>> or should we put these pinmux into sub spi device?
>
> By default the pinctrl for rk3399.dtsi just sets up cs0, so I'd worry
> about that.  If someone wants to make cs1 work then they'd have to
> specify the right pinctrl there.
>
>
>>> * You'd want to add the pinmux configs to the main rk3399.dtsi file
>>> and then add code to the rockchip SPI driver to select the right
>>> pinmux (depending on the current state of the chip select and the
>>> current polarity) at runtime suspend.  ...then go back to "default"
>>> mode at runtime resume.
>>
>> i uploaded 2 testonly CLs:
>> remote:   https://chromium-review.googlesource.com/544391 TESTONLY: spi:
>> rockchip: use pinmux to config cs
>> remote:   https://chromium-review.googlesource.com/544392 TESTONLY: dts:
>> bob: only enable ec spi
>>
>> but they are not working well, not sure why, maybe cs still toggled will
>> switching pinmux? will use scope to check it next week.
>
> Yeah, scope is probably the right thing to do.  One thing you'd have
> to make sure is that everything is being done glitch free.  In other
> words, if this is happening:
>
> A) Pinmux to GPIO
> B) Set GPIO to output
> C) Drive GPIO state low
>
> Then that's bad because:
>
> After step A but before step B, you might still be an input and pulls
> might take effect.  Thus you could glitch the line.
>
> After step B but before step C, you might be outputting something else
> if the GPIO had previously been configured to output high.
>
>
> You need to make sure that the sequence is instead:
>
> A) Drive GPIO state high
> B) Set GPIO to output
> C) Pinmux to GPIO
>
>
> Ensuring things are glitch free and dealing with two chip selects
> means it might be cleaner would be to do all the GPIO stuff yourself
> in the driver.  Then you'd have to specify the GPIOs in the device
> tree.
>
> ======
>
> OK, I took a look at your CL and I'm now of the opinion that we should
> disable Runtime PM when the chip select is asserted.  Maybe just
> ignore the above and instead look at:
>
>
> PROPOSAL: Disable Runtime PM when chip select is asserted
>
> I think this proposal will be very easy and is pretty clean.
> Basically go into "rockchip_spi_set_cs()" and adjust the
> "pm_runtime_get_sync()" and "pm_runtime_put_sync()" calls.  Only call
> the "get_sync" at the top of the function if you're asserting the chip
> select (and it wasn't already asserted).  Only call the "put_sync" at
> the bottom of the function if you're deasserting the chip select (and
> it wasn't already deasserted).
hmm, looks like a better way to solve this, but i think we need to call 
pm_runtime_get_sync unconditionally to make sure the read ser register safe.
>
> This should avoid entering PM sleep any time a transaction is midway
> through happening.
>
> Secondly, make sure that the chip selects have a pullup on them (they
> already do).  When you Runtime PM then the SPI part will stop driving
> the pins and the pull will take effect.  Since we can only Runtime PM
> when the chip select is deasserted then this pull will always be
> correct.  Also: since we Runtime PM when the chip select is deasserted
> then the state of the other pins isn't terribly important (though to
> avoid leakage it's probably good to choose a sane pull).
>
>
> How does that sound?  It should only be a few lines of code and only one patch.
>
sounds good, new patch sent(https://patchwork.kernel.org/patch/9808601)
>
> -Doug
>
>
>


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