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

Commit Message

Jeffy Chen June 13, 2017, 5:25 a.m. UTC

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 <[e-mail]>
---

Changes in v2: None

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

Comments

Brian Norris June 13, 2017, 5:29 p.m. UTC | #1

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 <[e-mail]>
> ---
> 
> 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 <[e-mail]>

>  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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jeffy Chen June 26, 2017, 3:26 a.m. UTC | #2

Hi Doug,

On 06/24/2017 12:14 AM, Doug Anderson wrote:
> Jeffy
>
> On Fri, Jun 23, 2017 at 5:18 AM, jeffy <[e-mail]> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jeffy Chen June 26, 2017, 3:27 a.m. UTC | #3

Hi Doug,

On 06/24/2017 12:14 AM, Doug Anderson wrote:
> Jeffy
>
> On Fri, Jun 23, 2017 at 5:18 AM, jeffy <[e-mail]> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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;