Skip to contentSkip to conversation

spi s3c64xx ioclk handling

PATCH Superseded Andi Shyti 6 participants 35 messages Jul 8, 2016 – Jul 14, 2016

Subsystems (6)
Device tree bindings Common clk SPI Arm Samsung Exynos +2 more subsystems

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Andi Shyti

author
#0 lore raw

v2 cover spi s3c64xx ioclk handling

Hi,

this patchset improves the compatibility between the s3c64xx driver and the exynos5433. One point of incompatibility, indeed, was that the SPI controller in exynos5433 is fed by three clocks which need to be handled by the driver. The third clock is the ioclk which support is the main purpose of this patchset.

Patch 6 is a bit unrelated with the rest, it restores a comment that has been removed earlier by mistake.

The last patch was first sent on its own and it was its 5th version, it has been queued to this patchset as there is a dependency with it.

Changelog V1 -> V2:

- patch 1: the exynos5433-spi compatible is placed in alphabetical order compared to exynos7-spi

- patch 1: deprecates exynos7-spi

- patch 2: [new] added the clocks and clock-name documentation in the bindings

- patch 3: fixed some typos, renamed the goto label from err_power_off to err_pm_put add the reviewd-by Krzysztof

- patch 4: new patch

- patch 5: fixed typos in commit message and use low letters for hex numbers

- patch 6: removed one line of comment

- patch 7: added to ensure sequentiality.

Thanks,
Andi

Andi Shyti (7):
bindings: spi-samsung: add exynos5433 spi compatible
bindings: spi-samsung: document the clocks and the clock-name property
spi: s3c64xx: rename goto labels to meaningful names
spi: s3c64xx: use error code from clk_prepare_enable()
spi: s3c64xx: add exynos5433 compatible for ioclk handling
spi: s3c64xx: restore removed comments
clk: exynos5433: remove CLK_IGNORE_UNUSED flag from SPI clocks

 .../devicetree/bindings/spi/spi-samsung.txt        | 11 ++-
 drivers/clk/samsung/clk-exynos5433.c               |  5 +-
 drivers/spi/spi-s3c64xx.c                          | 90 +++++++++++++++++-----
 3 files changed, 81 insertions(+), 25 deletions(-)
Signature · 1 line
2.8.1
author
#1 lore raw

PATCH v2 1/7 bindings: spi-samsung: add exynos5433 spi compatible

The samsung,exynos5433-spi has some peculiarities that bring the need of creating a new compatible in the binding.

One of those is the 3-clocks controller management where the spi is fed with three clocks: "spi", "busclkN" and "ioclk".

By adding the exynos5433-spi, we deprecate the exynos7 compatible and discourage its use.

Signed-off-by Andi Shyti <[email protected]>
---
 Documentation/devicetree/bindings/spi/spi-samsung.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/spi/spi-samsung.txt +2 -1
index 6dbdeb3..57d5539 100644
@@ -9,7 +9,8 @@ Required SoC Specific Properties:
     - samsung,s3c2443-spi: for s3c2443, s3c2416 and s3c2450 platforms
     - samsung,s3c6410-spi: for s3c6410 platforms
     - samsung,s5pv210-spi: for s5pv210 and s5pc110 platforms
-    - samsung,exynos7-spi: for exynos7 platforms
+    - samsung,exynos5433-spi: for exynos5433 compatible controllers
+    - samsung,exynos7-spi: for exynos7 platforms <DEPRECATED>
 
 - reg: physical base address of the controller and length of memory mapped
   region.
Signature · 1 line
2.8.1
#2 lore raw

PATCH v2 1/7 bindings: spi-samsung: add exynos5433 spi compatible

Quoted 6 lines · Andi Shyti

The samsung,exynos5433-spi has some peculiarities that bring the need of creating a new compatible in the binding.

One of those is the 3-clocks controller management where the spi is fed with three clocks: "spi", "busclkN" and "ioclk".

Quoted 7 lines

By adding the exynos5433-spi, we deprecate the exynos7 compatible and discourage its use.

Signed-off-by Andi Shyti <[email protected]>
---
 Documentation/devicetree/bindings/spi/spi-samsung.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

maintainer
#3 lore raw

PATCH v2 1/7 bindings: spi-samsung: add exynos5433 spi compatible

In reply to Andi Shyti
Quoted 10 lines · Andi Shyti

The samsung,exynos5433-spi has some peculiarities that bring the need of creating a new compatible in the binding.

One of those is the 3-clocks controller management where the spi is fed with three clocks: "spi", "busclkN" and "ioclk".

By adding the exynos5433-spi, we deprecate the exynos7 compatible and discourage its use.

Signed-off-by Andi Shyti <[email protected]>
Reviewed-by Sylwester Nawrocki <[email protected]>
maintainer
#4 lore raw

Applied "spi: s3c64xx: add exynos5433 spi compatible" to the spi tree

In reply to Andi Shyti

The patch

spi: s3c64xx: add exynos5433 spi compatible

has been applied to the spi tree at

git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From c068042bf0872c9121f4a6eab281dd63f61df5d3 Mon Sep 17 00:00:00 2001

From: Andi Shyti <[email protected]>
Date: Tue, 12 Jul 2016 19:02:10 +0900
Subject: [PATCH] spi: s3c64xx: add exynos5433 spi compatible

The samsung,exynos5433-spi has some peculiarities that bring the need of creating a new compatible in the binding.

One of those is the 3-clocks controller management where the spi is fed with three clocks: "spi", "busclkN" and "ioclk".

By adding the exynos5433-spi, we deprecate the exynos7 compatible and discourage its use.

Signed-off-by Andi Shyti <[email protected]>
Reviewed-by Michael Turquette <[email protected]>
Reviewed-by Krzysztof Kozlowski <[email protected]>
Reviewed-by Sylwester Nawrocki <[email protected]>
Signed-off-by Mark Brown <[email protected]>
---
 Documentation/devicetree/bindings/spi/spi-samsung.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/spi/spi-samsung.txt +2 -1
index 930bc7349271..ab4f0a46e859 100644
@@ -9,7 +9,8 @@ Required SoC Specific Properties:
     - samsung,s3c2443-spi: for s3c2443, s3c2416 and s3c2450 platforms
     - samsung,s3c6410-spi: for s3c6410 platforms
     - samsung,s5pv210-spi: for s5pv210 and s5pc110 platforms
-    - samsung,exynos7-spi: for exynos7 platforms
+    - samsung,exynos5433-spi: for exynos5433 compatible controllers
+    - samsung,exynos7-spi: for exynos7 platforms <DEPRECATED>
 
 - reg: physical base address of the controller and length of memory mapped
   region.
Signature · 1 line
2.8.1
author
#5 lore raw

PATCH v2 2/7 bindings: spi-samsung: document the clocks and the clock-name property

In reply to Andi Shyti

These two properties were not documented but used in the spi dts. Add the related documentation.

Signed-off-by Andi Shyti <[email protected]>
---
 Documentation/devicetree/bindings/spi/spi-samsung.txt | 8 ++++++++
 1 file changed, 8 insertions(+)
Documentation/devicetree/bindings/spi/spi-samsung.txt +8 -0
index 57d5539..2b7167e 100644
@@ -24,6 +24,14 @@ Required SoC Specific Properties:
 - dma-names: Names for the dma channels. There must be at least one channel
   named "tx" for transmit and named "rx" for receive.
 
+- clocks: specifies the clock IDs provided to the SPI controller; they are
+  required for interacting with the controller itself, for synchronizing the bus
+  and as I/O clock (the latter is required by exynos5433 and exynos7).
+
+- clock-names: string names of the clocks in the 'clocks' property; the names
+  mut be "spi", "spi_busclkN" and "spi_ioclk", where N is determined by
+  "samsung,spi-src-clk".
+
 Required Board Specific Properties:
 
 - #address-cells: should be 1.
Signature · 1 line
2.8.1
#6 lore raw

PATCH v2 2/7 bindings: spi-samsung: document the clocks and the clock-name property

Quoted 13 lines · Andi Shyti

These two properties were not documented but used in the spi dts. Add the related documentation.

Signed-off-by Andi Shyti <[email protected]>
---
 Documentation/devicetree/bindings/spi/spi-samsung.txt | 8 ++++++++
 1 file changed, 8 insertions(+)
Documentation/devicetree/bindings/spi/spi-samsung.txt +0 -0
index 57d5539..2b7167e 100644

Documentation/devicetree/bindings/spi/spi-samsung.txt Required SoC Specific Properties: · line 32

Quoted 9 lines
Documentation/devicetree/bindings/spi/spi-samsung.txt +6 -0
@@ -24,6 +24,14 @@ Required SoC Specific Properties:
 - dma-names: Names for the dma channels. There must be at least one channel
   named "tx" for transmit and named "rx" for receive.
 
+- clocks: specifies the clock IDs provided to the SPI controller; they are
+  required for interacting with the controller itself, for synchronizing the bus
+  and as I/O clock (the latter is required by exynos5433 and exynos7).
+
+- clock-names: string names of the clocks in the 'clocks' property; the names
+  mut be "spi", "spi_busclkN" and "spi_ioclk", where N is determined by

s/mut/must/

Documentation/devicetree/bindings/spi/spi-samsung.txt Required SoC Specific Properties: · line 33

Quoted 1 line
Documentation/devicetree/bindings/spi/spi-samsung.txt +1 -0
+  "samsung,spi-src-clk".

Thanks for taking care of this! Older SoCs (like s3c-s5p) even though they do not set clk_from_cmu, it seems they required them as well... this is a little bit strange but at least now the documentation matches the driver.

I would only describe the difference in number of clocks in more specific way, e.g.:

- clock-names: string names of the clocks in the 'clocks' property; for all devices the names must be "spi", "spi_busclkN" (where N is determined by "samsung,spi-src-clk"); for Exynos5433 it must contain third clock named "spi_ioclk"

... or something similar.

Anyway in general:

Reviewed-by Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

author
#7 lore raw

PATCH v3 1/1 bindings: spi-samsung: document the clocks and the clock-name property

These two properties were not documented but used in the spi dts. Add the related documentation.

Suggested-by Krzysztof Kozlowski <[email protected]>
Signed-off-by Andi Shyti <[email protected]>
Reviewed-by Michael Turquette <[email protected]>
Reviewed-by Krzysztof Kozlowski <[email protected]>

---
Hi,

Changelog related only to this patch:

V1 -> V2: [new] added the clocks and clock-name documentation
in the bindings

V2 -> V3: reworded the 'clock-names' as suggested by Krzysztof,
added Mike's and Krzysztof's review and Krzysztof's suggested-by.

Thanks,
Andi

 Documentation/devicetree/bindings/spi/spi-samsung.txt | 9 +++++++++
 1 file changed, 9 insertions(+)
Documentation/devicetree/bindings/spi/spi-samsung.txt +9 -0
index 57d5539..f45c6eb 100644
@@ -24,6 +24,15 @@ Required SoC Specific Properties:
 - dma-names: Names for the dma channels. There must be at least one channel
   named "tx" for transmit and named "rx" for receive.
 
+- clocks: specifies the clock IDs provided to the SPI controller; they are
+  required for interacting with the controller itself, for synchronizing the bus
+  and as I/O clock (the latter is required by exynos5433 and exynos7).
+
+- clock-names: string names of the clocks in the 'clocks' property; for all the
+  the devices the names must be "spi", "spi_busclkN" (where N is determined by
+  "samsung,spi-src-clk"), while Exynos5433 should specify a third clock
+  "spi_ioclk" for the I/O clock.
+
 Required Board Specific Properties:
 
 - #address-cells: should be 1.
Signature · 1 line
2.8.1
maintainer
#8 lore raw

Applied "spi: s3c64xx: document the clocks and the clock-name property" to the spi tree

The patch

spi: s3c64xx: document the clocks and the clock-name property

has been applied to the spi tree at

git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From 1ada32ede94fc91ff0f3236fcc70ae74dd0c2cb0 Mon Sep 17 00:00:00 2001

From: Andi Shyti <[email protected]>
Date: Tue, 12 Jul 2016 19:02:11 +0900
Subject: [PATCH] spi: s3c64xx: document the clocks and the clock-name property

These two properties were not documented but used in the spi dts. Add the related documentation.

Suggested-by Krzysztof Kozlowski <[email protected]>
Signed-off-by Andi Shyti <[email protected]>
Reviewed-by Michael Turquette <[email protected]>
Reviewed-by Krzysztof Kozlowski <[email protected]>
Signed-off-by Mark Brown <[email protected]>
---
 Documentation/devicetree/bindings/spi/spi-samsung.txt | 9 +++++++++
 1 file changed, 9 insertions(+)
Documentation/devicetree/bindings/spi/spi-samsung.txt +9 -0
index ab4f0a46e859..49028a4f5df1 100644
@@ -24,6 +24,15 @@ Required SoC Specific Properties:
 - dma-names: Names for the dma channels. There must be at least one channel
   named "tx" for transmit and named "rx" for receive.
 
+- clocks: specifies the clock IDs provided to the SPI controller; they are
+  required for interacting with the controller itself, for synchronizing the bus
+  and as I/O clock (the latter is required by exynos5433 and exynos7).
+
+- clock-names: string names of the clocks in the 'clocks' property; for all the
+  the devices the names must be "spi", "spi_busclkN" (where N is determined by
+  "samsung,spi-src-clk"), while Exynos5433 should specify a third clock
+  "spi_ioclk" for the I/O clock.
+
 Required Board Specific Properties:
 
 - #address-cells: should be 1.
Signature · 1 line
2.8.1
author
#9 lore raw

PATCH v2 3/7 spi: s3c64xx: rename goto labels to meaningful names

In reply to Andi Shyti

The goto labels of the style of

err4:
err3:
err2:
err1:

are complex to insert in between new errors without renaming all the goto statements. Replace the errX naming style to meaningful names in order to make it easier to insert new goto exit points.

Signed-off-by Andi Shyti <[email protected]>
Reviewed-by Krzysztof Kozlowski <[email protected]>
---
 drivers/spi/spi-s3c64xx.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)
drivers/spi/spi-s3c64xx.c +12 -12
index 6f2e91a..0784a4d 100644
@@ -1071,7 +1071,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 		if (ret < 0) {
 			dev_err(&pdev->dev, "failed to get alias id, errno %d\n",
 				ret);
-			goto err0;
+			goto err_deref_master;
 		}
 		sdd->port_id = ret;
 	} else {

@@ -1109,13 +1109,13 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	sdd->regs = devm_ioremap_resource(&pdev->dev, mem_res);
 	if (IS_ERR(sdd->regs)) {
 		ret = PTR_ERR(sdd->regs);
-		goto err0;
+		goto err_deref_master;
 	}
 
 	if (sci->cfg_gpio && sci->cfg_gpio()) {
 		dev_err(&pdev->dev, "Unable to config gpio\n");
 		ret = -EBUSY;
-		goto err0;
+		goto err_deref_master;
 	}
 
 	/* Setup clocks */

@@ -1123,13 +1123,13 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	if (IS_ERR(sdd->clk)) {
 		dev_err(&pdev->dev, "Unable to acquire clock 'spi'\n");
 		ret = PTR_ERR(sdd->clk);
-		goto err0;
+		goto err_deref_master;
 	}
 
 	if (clk_prepare_enable(sdd->clk)) {
 		dev_err(&pdev->dev, "Couldn't enable clock 'spi'\n");
 		ret = -EBUSY;
-		goto err0;
+		goto err_deref_master;
 	}
 
 	sprintf(clk_name, "spi_busclk%d", sci->src_clk_nr);

@@ -1138,13 +1138,13 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev,
 			"Unable to acquire clock '%s'\n", clk_name);
 		ret = PTR_ERR(sdd->src_clk);
-		goto err2;
+		goto err_disable_clk;
 	}
 
 	if (clk_prepare_enable(sdd->src_clk)) {
 		dev_err(&pdev->dev, "Couldn't enable clock '%s'\n", clk_name);
 		ret = -EBUSY;
-		goto err2;
+		goto err_disable_clk;
 	}
 
 	pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT);

@@ -1164,7 +1164,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	if (ret != 0) {
 		dev_err(&pdev->dev, "Failed to request IRQ %d: %d\n",
 			irq, ret);
-		goto err3;
+		goto err_pm_put;
 	}
 
 	writel(S3C64XX_SPI_INT_RX_OVERRUN_EN | S3C64XX_SPI_INT_RX_UNDERRUN_EN |

@@ -1174,7 +1174,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	ret = devm_spi_register_master(&pdev->dev, master);
 	if (ret != 0) {
 		dev_err(&pdev->dev, "cannot register SPI master: %d\n", ret);
-		goto err3;
+		goto err_pm_put;
 	}
 
 	dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Slaves attached\n",

@@ -1188,15 +1188,15 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 
 	return 0;
 
-err3:
+err_pm_put:
 	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_set_suspended(&pdev->dev);
 
 	clk_disable_unprepare(sdd->src_clk);
-err2:
+err_disable_clk:
 	clk_disable_unprepare(sdd->clk);
-err0:
+err_deref_master:
 	spi_master_put(master);
 
 	return ret;
Signature · 1 line
2.8.1
maintainer
#10 lore raw

Applied "spi: s3c64xx: rename goto labels to meaningful names" to the spi tree

The patch

spi: s3c64xx: rename goto labels to meaningful names

has been applied to the spi tree at

git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From 60a9a964420912a4c4b66efd210d98006177695a Mon Sep 17 00:00:00 2001

From: Andi Shyti <[email protected]>
Date: Tue, 12 Jul 2016 19:02:12 +0900
Subject: [PATCH] spi: s3c64xx: rename goto labels to meaningful names

The goto labels of the style of

err4:
err3:
err2:
err1:

are complex to insert in between new errors without renaming all the goto statements. Replace the errX naming style to meaningful names in order to make it easier to insert new goto exit points.

Signed-off-by Andi Shyti <[email protected]>
Reviewed-by Krzysztof Kozlowski <[email protected]>
Reviewed-by Michael Turquette <[email protected]>
Signed-off-by Mark Brown <[email protected]>
---
 drivers/spi/spi-s3c64xx.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)
drivers/spi/spi-s3c64xx.c +12 -12
index 001c9ebf884d..0a93eb98c2b6 100644
@@ -1078,7 +1078,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 		if (ret < 0) {
 			dev_err(&pdev->dev, "failed to get alias id, errno %d\n",
 				ret);
-			goto err0;
+			goto err_deref_master;
 		}
 		sdd->port_id = ret;
 	} else {

@@ -1116,13 +1116,13 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	sdd->regs = devm_ioremap_resource(&pdev->dev, mem_res);
 	if (IS_ERR(sdd->regs)) {
 		ret = PTR_ERR(sdd->regs);
-		goto err0;
+		goto err_deref_master;
 	}
 
 	if (sci->cfg_gpio && sci->cfg_gpio()) {
 		dev_err(&pdev->dev, "Unable to config gpio\n");
 		ret = -EBUSY;
-		goto err0;
+		goto err_deref_master;
 	}
 
 	/* Setup clocks */

@@ -1130,13 +1130,13 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	if (IS_ERR(sdd->clk)) {
 		dev_err(&pdev->dev, "Unable to acquire clock 'spi'\n");
 		ret = PTR_ERR(sdd->clk);
-		goto err0;
+		goto err_deref_master;
 	}
 
 	if (clk_prepare_enable(sdd->clk)) {
 		dev_err(&pdev->dev, "Couldn't enable clock 'spi'\n");
 		ret = -EBUSY;
-		goto err0;
+		goto err_deref_master;
 	}
 
 	sprintf(clk_name, "spi_busclk%d", sci->src_clk_nr);

@@ -1145,13 +1145,13 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev,
 			"Unable to acquire clock '%s'\n", clk_name);
 		ret = PTR_ERR(sdd->src_clk);
-		goto err2;
+		goto err_disable_clk;
 	}
 
 	if (clk_prepare_enable(sdd->src_clk)) {
 		dev_err(&pdev->dev, "Couldn't enable clock '%s'\n", clk_name);
 		ret = -EBUSY;
-		goto err2;
+		goto err_disable_clk;
 	}
 
 	pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT);

@@ -1171,7 +1171,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	if (ret != 0) {
 		dev_err(&pdev->dev, "Failed to request IRQ %d: %d\n",
 			irq, ret);
-		goto err3;
+		goto err_pm_put;
 	}
 
 	writel(S3C64XX_SPI_INT_RX_OVERRUN_EN | S3C64XX_SPI_INT_RX_UNDERRUN_EN |

@@ -1181,7 +1181,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	ret = devm_spi_register_master(&pdev->dev, master);
 	if (ret != 0) {
 		dev_err(&pdev->dev, "cannot register SPI master: %d\n", ret);
-		goto err3;
+		goto err_pm_put;
 	}
 
 	dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Slaves attached\n",

@@ -1195,15 +1195,15 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 
 	return 0;
 
-err3:
+err_pm_put:
 	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_set_suspended(&pdev->dev);
 
 	clk_disable_unprepare(sdd->src_clk);
-err2:
+err_disable_clk:
 	clk_disable_unprepare(sdd->clk);
-err0:
+err_deref_master:
 	spi_master_put(master);
 
 	return ret;
Signature · 1 line
2.8.1
author
#11 lore raw

PATCH v2 4/7 spi: s3c64xx: use error code from clk_prepare_enable()

In reply to Andi Shyti

If clk_prepare_enable() fails do not return -EBUSY but use the value provided by the function itself.

Suggested-by Krzysztof Kozlowski <[email protected]>
Signed-off-by Andi Shyti <[email protected]>
---
 drivers/spi/spi-s3c64xx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/spi/spi-s3c64xx.c +4 -4
index 0784a4d..3a65adf 100644
@@ -1126,9 +1126,9 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 		goto err_deref_master;
 	}
 
-	if (clk_prepare_enable(sdd->clk)) {
+	ret = clk_prepare_enable(sdd->clk);
+	if (ret) {
 		dev_err(&pdev->dev, "Couldn't enable clock 'spi'\n");
-		ret = -EBUSY;
 		goto err_deref_master;
 	}
 

@@ -1141,9 +1141,9 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 		goto err_disable_clk;
 	}
 
-	if (clk_prepare_enable(sdd->src_clk)) {
+	ret = clk_prepare_enable(sdd->src_clk);
+	if (ret) {
 		dev_err(&pdev->dev, "Couldn't enable clock '%s'\n", clk_name);
-		ret = -EBUSY;
 		goto err_disable_clk;
 	}
Signature · 1 line
2.8.1
#12 lore raw

PATCH v2 4/7 spi: s3c64xx: use error code from clk_prepare_enable()

Quoted 8 lines · Andi Shyti

If clk_prepare_enable() fails do not return -EBUSY but use the value provided by the function itself.

Suggested-by Krzysztof Kozlowski <[email protected]>
Signed-off-by Andi Shyti <[email protected]>
---
 drivers/spi/spi-s3c64xx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Cool, thanks for fixing existing pattern:

Reviewed-by Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

maintainer
#13 lore raw

Applied "spi: s3c64xx: use error code from clk_prepare_enable()" to the spi tree

In reply to Andi Shyti

The patch

spi: s3c64xx: use error code from clk_prepare_enable()

has been applied to the spi tree at

git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From 25981d8281ee4cbbd6f7c5bc0f8c048ad9548037 Mon Sep 17 00:00:00 2001

From: Andi Shyti <[email protected]>
Date: Tue, 12 Jul 2016 19:02:13 +0900
Subject: [PATCH] spi: s3c64xx: use error code from clk_prepare_enable()

If clk_prepare_enable() fails do not return -EBUSY but use the value provided by the function itself.

Suggested-by Krzysztof Kozlowski <[email protected]>
Signed-off-by Andi Shyti <[email protected]>
Reviewed-by Michael Turquette <[email protected]>
Reviewed-by Krzysztof Kozlowski <[email protected]>
Signed-off-by Mark Brown <[email protected]>
---
 drivers/spi/spi-s3c64xx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/spi/spi-s3c64xx.c +4 -4
index 0a93eb98c2b6..3d21f8a35837 100644
@@ -1133,9 +1133,9 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 		goto err_deref_master;
 	}
 
-	if (clk_prepare_enable(sdd->clk)) {
+	ret = clk_prepare_enable(sdd->clk);
+	if (ret) {
 		dev_err(&pdev->dev, "Couldn't enable clock 'spi'\n");
-		ret = -EBUSY;
 		goto err_deref_master;
 	}
 

@@ -1148,9 +1148,9 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 		goto err_disable_clk;
 	}
 
-	if (clk_prepare_enable(sdd->src_clk)) {
+	ret = clk_prepare_enable(sdd->src_clk);
+	if (ret) {
 		dev_err(&pdev->dev, "Couldn't enable clock '%s'\n", clk_name);
-		ret = -EBUSY;
 		goto err_disable_clk;
 	}
Signature · 1 line
2.8.1
author
#14 lore raw

PATCH v2 5/7 spi: s3c64xx: add exynos5433 compatible for ioclk handling

In reply to Andi Shyti

The new compatible is related to the Samsung Exynos5433 SoC. The difference between the previous is that in the exynos5433 the SPI controller is driven by three clocks instead of only one.

The new clock (ioclk) is controlling the input/output clock whenever the controller is slave or master.

The presence of the clock line is detected from the compatibility structure (exynos5433_spi_port_config) as a boolean value.

The probe function checks whether the ioclk is present and if so, it acquires.

The runtime suspend and resume functions will handle the clock enabling and disabling as well.

Signed-off-by Andi Shyti <[email protected]>
---
 drivers/spi/spi-s3c64xx.c | 57 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 52 insertions(+), 5 deletions(-)
drivers/spi/spi-s3c64xx.c +52 -5
index 3a65adf..6da663f 100644
@@ -156,12 +156,14 @@ struct s3c64xx_spi_port_config {
 	int	quirks;
 	bool	high_speed;
 	bool	clk_from_cmu;
+	bool	clk_ioclk;
 };
 
 /**
  * struct s3c64xx_spi_driver_data - Runtime info holder for SPI driver.
  * @clk: Pointer to the spi clock.
  * @src_clk: Pointer to the clock used to generate SPI signals.
+ * @ioclk: Pointer to the i/o clock between master and slave
  * @master: Pointer to the SPI Protocol master.
  * @cntrlr_info: Platform specific data for the controller this driver manages.
  * @tgl_spi: Pointer to the last CS left untoggled by the cs_change hint.

@@ -181,6 +183,7 @@ struct s3c64xx_spi_driver_data {
 	void __iomem                    *regs;
 	struct clk                      *clk;
 	struct clk                      *src_clk;
+	struct clk                      *ioclk;
 	struct platform_device          *pdev;
 	struct spi_master               *master;
 	struct s3c64xx_spi_info  *cntrlr_info;

@@ -1147,6 +1150,21 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 		goto err_disable_clk;
 	}
 
+	if (sdd->port_conf->clk_ioclk) {
+		sdd->ioclk = devm_clk_get(&pdev->dev, "spi_ioclk");
+		if (IS_ERR(sdd->ioclk)) {
+			dev_err(&pdev->dev, "Unable to acquire 'ioclk'\n");
+			ret = PTR_ERR(sdd->ioclk);
+			goto err_disable_src_clk;
+		}
+
+		ret = clk_prepare_enable(sdd->ioclk);
+		if (ret) {
+			dev_err(&pdev->dev, "Couldn't enable clock 'ioclk'\n");
+			goto err_disable_src_clk;
+		}
+	}
+
 	pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT);
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_active(&pdev->dev);

@@ -1193,6 +1211,8 @@ err_pm_put:
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_set_suspended(&pdev->dev);
 
+	clk_disable_unprepare(sdd->ioclk);
+err_disable_src_clk:
 	clk_disable_unprepare(sdd->src_clk);
 err_disable_clk:
 	clk_disable_unprepare(sdd->clk);

@@ -1211,6 +1231,8 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)
 
 	writel(0, sdd->regs + S3C64XX_SPI_INT_EN);
 
+	clk_disable_unprepare(sdd->ioclk);
+
 	clk_disable_unprepare(sdd->src_clk);
 
 	clk_disable_unprepare(sdd->clk);

@@ -1269,6 +1291,7 @@ static int s3c64xx_spi_runtime_suspend(struct device *dev)
 
 	clk_disable_unprepare(sdd->clk);
 	clk_disable_unprepare(sdd->src_clk);
+	clk_disable_unprepare(sdd->ioclk);
 
 	return 0;
 }

@@ -1279,17 +1302,28 @@ static int s3c64xx_spi_runtime_resume(struct device *dev)
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
 	int ret;
 
+	if (sdd->port_conf->clk_ioclk) {
+		ret = clk_prepare_enable(sdd->ioclk);
+		if (ret != 0)
+			return ret;
+	}
+
 	ret = clk_prepare_enable(sdd->src_clk);
 	if (ret != 0)
-		return ret;
+		goto err_disable_ioclk;
 
 	ret = clk_prepare_enable(sdd->clk);
-	if (ret != 0) {
-		clk_disable_unprepare(sdd->src_clk);
-		return ret;
-	}
+	if (ret != 0)
+		goto err_disable_src_clk;
 
 	return 0;
+
+err_disable_src_clk:
+	clk_disable_unprepare(sdd->src_clk);
+err_disable_ioclk:
+	clk_disable_unprepare(sdd->ioclk);
+
+	return ret;
 }
 #endif /* CONFIG_PM */
 

@@ -1345,6 +1379,16 @@ static struct s3c64xx_spi_port_config exynos7_spi_port_config = {
 	.quirks		= S3C64XX_SPI_QUIRK_CS_AUTO,
 };
 
+static struct s3c64xx_spi_port_config exynos5433_spi_port_config = {
+	.fifo_lvl_mask	= { 0x1ff, 0x7f, 0x7f, 0x7f, 0x7f, 0x1ff},
+	.rx_lvl_offset	= 15,
+	.tx_st_done	= 25,
+	.high_speed	= true,
+	.clk_from_cmu	= true,
+	.clk_ioclk	= true,
+	.quirks		= S3C64XX_SPI_QUIRK_CS_AUTO,
+};
+
 static const struct platform_device_id s3c64xx_spi_driver_ids[] = {
 	{
 		.name		= "s3c2443-spi",

@@ -1375,6 +1419,9 @@ static const struct of_device_id s3c64xx_spi_dt_match[] = {
 	{ .compatible = "samsung,exynos7-spi",
 			.data = (void *)&exynos7_spi_port_config,
 	},
+	{ .compatible = "samsung,exynos5433-spi",
+			.data = (void *)&exynos5433_spi_port_config,
+	},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, s3c64xx_spi_dt_match);
Signature · 1 line
2.8.1
#15 lore raw

PATCH v2 5/7 spi: s3c64xx: add exynos5433 compatible for ioclk handling

Quoting Andi Shyti (2016-07-08 07:46:39)

Quoted 23 lines
drivers/spi/spi-s3c64xx.c +15 -0
@@ -1147,6 +1150,21 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
                goto err_disable_clk;
        }
 
+       if (sdd->port_conf->clk_ioclk) {
+               sdd->ioclk = devm_clk_get(&pdev->dev, "spi_ioclk");
+               if (IS_ERR(sdd->ioclk)) {
+                       dev_err(&pdev->dev, "Unable to acquire 'ioclk'\n");
+                       ret = PTR_ERR(sdd->ioclk);
+                       goto err_disable_src_clk;
+               }
+
+               ret = clk_prepare_enable(sdd->ioclk);
+               if (ret) {
+                       dev_err(&pdev->dev, "Couldn't enable clock 'ioclk'\n");
+                       goto err_disable_src_clk;
+               }
+       }
+
        pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT);
        pm_runtime_use_autosuspend(&pdev->dev);
        pm_runtime_set_active(&pdev->dev);

drivers/spi/spi-s3c64xx.c err_pm_put: · line 1218

Quoted 8 lines
drivers/spi/spi-s3c64xx.c +2 -0
@@ -1193,6 +1211,8 @@ err_pm_put:
        pm_runtime_disable(&pdev->dev);
        pm_runtime_set_suspended(&pdev->dev);
 
+       clk_disable_unprepare(sdd->ioclk);
+err_disable_src_clk:
        clk_disable_unprepare(sdd->src_clk);
 err_disable_clk:
        clk_disable_unprepare(sdd->clk);

Why aren't these clocks enabled/disabled by calls to pm_runtime_get/put?

Regards,
Mike

#16 lore raw

PATCH v2 5/7 spi: s3c64xx: add exynos5433 compatible for ioclk handling

Hi Mike,

Quoted 7 lines
Quoted 7 lines
        pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT);
        pm_runtime_use_autosuspend(&pdev->dev);
        pm_runtime_set_active(&pdev->dev);
drivers/spi/spi-s3c64xx.c +0 -0
@@ -1193,6 +1211,8 @@ err_pm_put:
        pm_runtime_disable(&pdev->dev);
        pm_runtime_set_suspended(&pdev->dev);
Quoted 7 lines

drivers/spi/spi-s3c64xx.c err_pm_put: · line 1218

Quoted 5 lines
drivers/spi/spi-s3c64xx.c +2 -0
+       clk_disable_unprepare(sdd->ioclk);
+err_disable_src_clk:
        clk_disable_unprepare(sdd->src_clk);
 err_disable_clk:
        clk_disable_unprepare(sdd->clk);

Why aren't these clocks enabled/disabled by calls to pm_runtime_get/put?

yes, they are, but this should be fixed in a different patch.

I will send a follow-up, thanks for pointing this out.

Andi

#17 lore raw

PATCH v2 5/7 spi: s3c64xx: add exynos5433 compatible for ioclk handling

In reply to Andi Shyti
Quoted 13 lines · Andi Shyti

The new compatible is related to the Samsung Exynos5433 SoC. The difference between the previous is that in the exynos5433 the SPI controller is driven by three clocks instead of only one.

The new clock (ioclk) is controlling the input/output clock whenever the controller is slave or master.

The presence of the clock line is detected from the compatibility structure (exynos5433_spi_port_config) as a boolean value.

The probe function checks whether the ioclk is present and if so, it acquires.

Quoted 8 lines

The runtime suspend and resume functions will handle the clock enabling and disabling as well.

Signed-off-by Andi Shyti <[email protected]>
---
 drivers/spi/spi-s3c64xx.c | 57 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 52 insertions(+), 5 deletions(-)

Looks fine to me, thanks for updates.

Reviewed-by Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

maintainer
#18 lore raw

Applied "spi: s3c64xx: add Exynos5433 compatible for ioclk handling" to the spi tree

In reply to Andi Shyti

The patch

spi: s3c64xx: add Exynos5433 compatible for ioclk handling

has been applied to the spi tree at

git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From 7990b00819e765ca180d36b38969c2bde2734e53 Mon Sep 17 00:00:00 2001

From: Andi Shyti <[email protected]>
Date: Tue, 12 Jul 2016 19:02:14 +0900
Subject: [PATCH] spi: s3c64xx: add Exynos5433 compatible for ioclk handling

The new compatible is related to the Samsung Exynos5433 SoC. The difference between the previous is that in the exynos5433 the SPI controller is driven by three clocks instead of only one.

The new clock (ioclk) is controlling the input/output clock whenever the controller is slave or master.

The presence of the clock line is detected from the compatibility structure (exynos5433_spi_port_config) as a boolean value.

The probe function checks whether the ioclk is present and if so, it acquires.

The runtime suspend and resume functions will handle the clock enabling and disabling as well.

Signed-off-by Andi Shyti <[email protected]>
Reviewed-by Michael Turquette <[email protected]>
Reviewed-by Krzysztof Kozlowski <[email protected]>
Signed-off-by Mark Brown <[email protected]>
---
 drivers/spi/spi-s3c64xx.c | 57 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 52 insertions(+), 5 deletions(-)
drivers/spi/spi-s3c64xx.c +52 -5
index 3d21f8a35837..a1e84686c70c 100644
@@ -156,12 +156,14 @@ struct s3c64xx_spi_port_config {
 	int	quirks;
 	bool	high_speed;
 	bool	clk_from_cmu;
+	bool	clk_ioclk;
 };
 
 /**
  * struct s3c64xx_spi_driver_data - Runtime info holder for SPI driver.
  * @clk: Pointer to the spi clock.
  * @src_clk: Pointer to the clock used to generate SPI signals.
+ * @ioclk: Pointer to the i/o clock between master and slave
  * @master: Pointer to the SPI Protocol master.
  * @cntrlr_info: Platform specific data for the controller this driver manages.
  * @tgl_spi: Pointer to the last CS left untoggled by the cs_change hint.

@@ -181,6 +183,7 @@ struct s3c64xx_spi_driver_data {
 	void __iomem                    *regs;
 	struct clk                      *clk;
 	struct clk                      *src_clk;
+	struct clk                      *ioclk;
 	struct platform_device          *pdev;
 	struct spi_master               *master;
 	struct s3c64xx_spi_info  *cntrlr_info;

@@ -1154,6 +1157,21 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 		goto err_disable_clk;
 	}
 
+	if (sdd->port_conf->clk_ioclk) {
+		sdd->ioclk = devm_clk_get(&pdev->dev, "spi_ioclk");
+		if (IS_ERR(sdd->ioclk)) {
+			dev_err(&pdev->dev, "Unable to acquire 'ioclk'\n");
+			ret = PTR_ERR(sdd->ioclk);
+			goto err_disable_src_clk;
+		}
+
+		ret = clk_prepare_enable(sdd->ioclk);
+		if (ret) {
+			dev_err(&pdev->dev, "Couldn't enable clock 'ioclk'\n");
+			goto err_disable_src_clk;
+		}
+	}
+
 	pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT);
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_active(&pdev->dev);

@@ -1200,6 +1218,8 @@ err_pm_put:
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_set_suspended(&pdev->dev);
 
+	clk_disable_unprepare(sdd->ioclk);
+err_disable_src_clk:
 	clk_disable_unprepare(sdd->src_clk);
 err_disable_clk:
 	clk_disable_unprepare(sdd->clk);

@@ -1218,6 +1238,8 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)
 
 	writel(0, sdd->regs + S3C64XX_SPI_INT_EN);
 
+	clk_disable_unprepare(sdd->ioclk);
+
 	clk_disable_unprepare(sdd->src_clk);
 
 	clk_disable_unprepare(sdd->clk);

@@ -1276,6 +1298,7 @@ static int s3c64xx_spi_runtime_suspend(struct device *dev)
 
 	clk_disable_unprepare(sdd->clk);
 	clk_disable_unprepare(sdd->src_clk);
+	clk_disable_unprepare(sdd->ioclk);
 
 	return 0;
 }

@@ -1286,17 +1309,28 @@ static int s3c64xx_spi_runtime_resume(struct device *dev)
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
 	int ret;
 
+	if (sdd->port_conf->clk_ioclk) {
+		ret = clk_prepare_enable(sdd->ioclk);
+		if (ret != 0)
+			return ret;
+	}
+
 	ret = clk_prepare_enable(sdd->src_clk);
 	if (ret != 0)
-		return ret;
+		goto err_disable_ioclk;
 
 	ret = clk_prepare_enable(sdd->clk);
-	if (ret != 0) {
-		clk_disable_unprepare(sdd->src_clk);
-		return ret;
-	}
+	if (ret != 0)
+		goto err_disable_src_clk;
 
 	return 0;
+
+err_disable_src_clk:
+	clk_disable_unprepare(sdd->src_clk);
+err_disable_ioclk:
+	clk_disable_unprepare(sdd->ioclk);
+
+	return ret;
 }
 #endif /* CONFIG_PM */
 

@@ -1352,6 +1386,16 @@ static struct s3c64xx_spi_port_config exynos7_spi_port_config = {
 	.quirks		= S3C64XX_SPI_QUIRK_CS_AUTO,
 };
 
+static struct s3c64xx_spi_port_config exynos5433_spi_port_config = {
+	.fifo_lvl_mask	= { 0x1ff, 0x7f, 0x7f, 0x7f, 0x7f, 0x1ff},
+	.rx_lvl_offset	= 15,
+	.tx_st_done	= 25,
+	.high_speed	= true,
+	.clk_from_cmu	= true,
+	.clk_ioclk	= true,
+	.quirks		= S3C64XX_SPI_QUIRK_CS_AUTO,
+};
+
 static const struct platform_device_id s3c64xx_spi_driver_ids[] = {
 	{
 		.name		= "s3c2443-spi",

@@ -1382,6 +1426,9 @@ static const struct of_device_id s3c64xx_spi_dt_match[] = {
 	{ .compatible = "samsung,exynos7-spi",
 			.data = (void *)&exynos7_spi_port_config,
 	},
+	{ .compatible = "samsung,exynos5433-spi",
+			.data = (void *)&exynos5433_spi_port_config,
+	},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, s3c64xx_spi_dt_match);
Signature · 1 line
2.8.1
author
#19 lore raw

PATCH v2 6/7 spi: s3c64xx: restore removed comments

In reply to Andi Shyti

Patch a9e93e8 has erroneously removed some comments which are important to understand why the bus frequency is multiplied by two during the spi transfer.

Signed-off-by Andi Shyti <[email protected]>
---
 drivers/spi/spi-s3c64xx.c | 1 +
 1 file changed, 1 insertion(+)
drivers/spi/spi-s3c64xx.c +1 -0
index 6da663f..32b66f0 100644
@@ -639,6 +639,7 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
 	writel(val, regs + S3C64XX_SPI_MODE_CFG);
 
 	if (sdd->port_conf->clk_from_cmu) {
+		/* There is half-multiplier before the SPI */
 		clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
 	} else {
 		/* Configure Clock */
Signature · 1 line
2.8.1
#20 lore raw

PATCH v2 6/7 spi: s3c64xx: restore removed comments

Quoting Andi Shyti (2016-07-08 07:46:40)

Quoted 14 lines

Patch a9e93e8 has erroneously removed some comments which are important to understand why the bus frequency is multiplied by two during the spi transfer.

Signed-off-by Andi Shyti <[email protected]>
---
 drivers/spi/spi-s3c64xx.c | 1 +
 1 file changed, 1 insertion(+)
drivers/spi/spi-s3c64xx.c +0 -0
index 6da663f..32b66f0 100644

drivers/spi/spi-s3c64xx.c static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd) · line 643

Quoted 5 lines
drivers/spi/spi-s3c64xx.c +1 -0
@@ -639,6 +639,7 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
        writel(val, regs + S3C64XX_SPI_MODE_CFG);
 
        if (sdd->port_conf->clk_from_cmu) {
+               /* There is half-multiplier before the SPI */
                clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);

Just a small comment, but if the fixed-factor divide-by-two clock was modeled in Linux, then this driver could call clk_set_rate on that clock with the "correct" rate.

I guess that this driver would be the provider of that clock?

Regards,
Mike

Quoted 5 lines
drivers/spi/spi-s3c64xx.c +0 -0
        } else {
                /* Configure Clock */
Signature · 1 line
2.8.1
#21 lore raw

PATCH v2 6/7 spi: s3c64xx: restore removed comments

Hi Mike,

Quoted 9 lines
Quoted 3 lines
        if (sdd->port_conf->clk_from_cmu) {
+               /* There is half-multiplier before the SPI */
                clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);

Just a small comment, but if the fixed-factor divide-by-two clock was modeled in Linux, then this driver could call clk_set_rate on that clock with the "correct" rate.

I guess that this driver would be the provider of that clock?

Makes sense, I will check it.

Thanks again,
Andi

maintainer
#22 lore raw

PATCH v2 6/7 spi: s3c64xx: restore removed comments

In reply to Michael Turquette
Quoted 17 lines · Michael Turquette

Quoting Andi Shyti (2016-07-08 07:46:40)

drivers/spi/spi-s3c64xx.c static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd) · line 643

Quoted 10 lines
drivers/spi/spi-s3c64xx.c +1 -0
index 6da663f..32b66f0 100644
@@ -639,6 +639,7 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
        writel(val, regs + S3C64XX_SPI_MODE_CFG);
 
        if (sdd->port_conf->clk_from_cmu) {
+               /* There is half-multiplier before the SPI */
                clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);

Just a small comment, but if the fixed-factor divide-by-two clock was modeled in Linux, then this driver could call clk_set_rate on that clock with the "correct" rate.

Quoted 1 line

I guess that this driver would be the provider of that clock?

Good point, however I'm not sure if it is worth to model this divider with a clk object. It is an internal divider and the clock it provides is only used internally within the SPI controller. Thus the spi-s3c64xx driver would be provider on the only consumer of this clock.

Additionally, the "There is half-multiplier before the SPI" comment seems to be obfuscating how the hardware really looks like to me. It talks about multiplier (which reminds me of PLLs with a divider in the feedback loop) while there is a simple divider which should be considered as an integral part of the controller IP block.

While we are at it, I'd propose to change this comment to something like:

/* The SCLK_SPI clock is divided internally by 2 */

#23 lore raw

PATCH v2 6/7 spi: s3c64xx: restore removed comments

Quoting Sylwester Nawrocki (2016-07-11 03:33:42)

Quoted 18 lines · Michael Turquette
Quoted 17 lines

Quoting Andi Shyti (2016-07-08 07:46:40)

drivers/spi/spi-s3c64xx.c static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd) · line 643

Quoted 10 lines
drivers/spi/spi-s3c64xx.c +1 -0
index 6da663f..32b66f0 100644
@@ -639,6 +639,7 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
        writel(val, regs + S3C64XX_SPI_MODE_CFG);
 
        if (sdd->port_conf->clk_from_cmu) {
+               /* There is half-multiplier before the SPI */
                clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);

Just a small comment, but if the fixed-factor divide-by-two clock was modeled in Linux, then this driver could call clk_set_rate on that clock with the "correct" rate.

Quoted 6 lines
Quoted 1 line

I guess that this driver would be the provider of that clock?

Good point, however I'm not sure if it is worth to model this divider with a clk object. It is an internal divider and the clock it provides is only used internally within the SPI controller. Thus the spi-s3c64xx driver would be provider on the only consumer of this clock.

Sure, but it's only a few lines of code to this, and there are examples in the kernel already.

Quoted 11 lines

Additionally, the "There is half-multiplier before the SPI" comment seems to be obfuscating how the hardware really looks like to me. It talks about multiplier (which reminds me of PLLs with a divider in the feedback loop) while there is a simple divider which should be considered as an integral part of the controller IP block.

While we are at it, I'd propose to change this comment to something like:

/* The SCLK_SPI clock is divided internally by 2 */

It's your choice, but debug output would benefit from showing the real clock frequency at some point.

Regards,
Mike

maintainer
#24 lore raw

PATCH v2 6/7 spi: s3c64xx: restore removed comments

Quoted 12 lines · Michael Turquette

Quoting Sylwester Nawrocki (2016-07-11 03:33:42)

Quoted 1 line · Michael Turquette

Sure, but it's only a few lines of code to this, and there are examples in the kernel already.

Quoted 6 lines

Additionally, the "There is half-multiplier before the SPI" comment seems to be obfuscating how the hardware really looks like to me. It talks about multiplier (which reminds me of PLLs with a divider in the feedback loop) while there is a simple divider which should be considered as an integral part of the controller IP block.

Quoted 7 lines
Quoted 4 lines

While we are at it, I'd propose to change this comment to something like:

/* The SCLK_SPI clock is divided internally by 2 */

It's your choice, but debug output would benefit from showing the real clock frequency at some point.

OK, it's indeed fairly easy to add a fixed rate divide-by-two clock, but for the older SoCs we would need to also model the internal mux, gate, and an 8-bit divider. Then it becomes a bit bigger task. Anyway it might be worth to try it, this could let us deprecate the samsung,spi-src-clk property and for all the SoCs use assigned-clock-parents.

Signature · 2 lines
Thanks,
Sylwester
author
#25 lore raw

PATCH v3 6/7 spi: s3c64xx: restore removed comments

In reply to Sylwester Nawrocki

Patch a9e93e8 has erroneously removed some comments which are important to understand why the bus frequency is multiplied by two during the spi transfer.

Reword the previous comment to a more appropriate message.

Suggested-by Sylwester Nawrocki <[email protected]>
Signed-off-by Andi Shyti <[email protected]>
Reviewed-by Michael Turquette <[email protected]>

---
Hi,

despite Mike's comments I'm sending the patch as it was originally meant. I think that fixing the exact clk where to set the rate should anyway go in a different patch.

V2->V3
The comment has been reworded as Sylwester recommended.

Thanks,
Andi

 drivers/spi/spi-s3c64xx.c | 1 +
 1 file changed, 1 insertion(+)
drivers/spi/spi-s3c64xx.c +1 -0
index 6da663f..5bedafc 100644
@@ -639,6 +639,7 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
 	writel(val, regs + S3C64XX_SPI_MODE_CFG);
 
 	if (sdd->port_conf->clk_from_cmu) {
+		/* The src_clk clock is divided internally by 2 */
 		clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
 	} else {
 		/* Configure Clock */
Signature · 1 line
2.8.1
maintainer
#26 lore raw

PATCH v3 6/7 spi: s3c64xx: restore removed comments

Quoted 3 lines · Andi Shyti

Patch a9e93e8 has erroneously removed some comments which are important to understand why the bus frequency is multiplied by two during the spi transfer.

Please don't bury patches in the middle of old threads, it makes it very
confusing trying to figure out what's going on especially when you do
this one patch at a time in a series. Submit new patches as a complete
series, ideally not threaded in with anything else (at least for me).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160712/e32d1a91/attachment.sig>

author
#27 lore raw

PATCH v3 6/7 spi: s3c64xx: restore removed comments

Quoted 8 lines
Quoted 3 lines

Patch a9e93e8 has erroneously removed some comments which are important to understand why the bus frequency is multiplied by two during the spi transfer.

Please don't bury patches in the middle of old threads, it makes it very confusing trying to figure out what's going on especially when you do this one patch at a time in a series. Submit new patches as a complete series, ideally not threaded in with anything else (at least for me).

OK, sorry, do you want me to send everything again?

Andi

maintainer
#28 lore raw

PATCH v3 6/7 spi: s3c64xx: restore removed comments

Quoted 6 lines · Andi Shyti
Quoted 4 lines

Please don't bury patches in the middle of old threads, it makes it very confusing trying to figure out what's going on especially when you do this one patch at a time in a series. Submit new patches as a complete series, ideally not threaded in with anything else (at least for me).

OK, sorry, do you want me to send everything again?

Yes, please.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160712/bfa0f2dc/attachment-0001.sig>

#29 lore raw

PATCH v3 6/7 spi: s3c64xx: restore removed comments

In reply to Andi Shyti

Quoting Andi Shyti (2016-07-11 23:54:04)

Quoted 9 lines

Patch a9e93e8 has erroneously removed some comments which are important to understand why the bus frequency is multiplied by two during the spi transfer.

Reword the previous comment to a more appropriate message.

Suggested-by Sylwester Nawrocki <[email protected]>
Signed-off-by Andi Shyti <[email protected]>
Reviewed-by Michael Turquette <[email protected]>
Quoted 6 lines

---
Hi,

despite Mike's comments I'm sending the patch as it was originally meant. I think that fixing the exact clk where to set the rate should anyway go in a different patch.

That sounds fair.

Regards,
Mike

Quoted 25 lines

V2->V3
The comment has been reworded as Sylwester recommended.

Thanks,
Andi

 drivers/spi/spi-s3c64xx.c | 1 +
 1 file changed, 1 insertion(+)
drivers/spi/spi-s3c64xx.c +1 -0
index 6da663f..5bedafc 100644
@@ -639,6 +639,7 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
        writel(val, regs + S3C64XX_SPI_MODE_CFG);
 
        if (sdd->port_conf->clk_from_cmu) {
+               /* The src_clk clock is divided internally by 2 */
                clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
        } else {
                /* Configure Clock */
Signature · 1 line
2.8.1
maintainer
#30 lore raw

Applied "spi: s3c64xx: restore removed comments" to the spi tree

In reply to Andi Shyti

The patch

spi: s3c64xx: restore removed comments

has been applied to the spi tree at

git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git

All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying to this mail.

Thanks,
Mark

Quoted 1 line

From 0dbe70a1feb411d17fbcf8e73d61e6c7f33fda72 Mon Sep 17 00:00:00 2001

From: Andi Shyti <[email protected]>
Date: Tue, 12 Jul 2016 19:02:15 +0900
Subject: [PATCH] spi: s3c64xx: restore removed comments

Patch a9e93e8 has erroneously removed some comments which are important to understand why the bus frequency is multiplied by two during the spi transfer.

Reword the previous comment to a more appropriate message.

Suggested-by Sylwester Nawrocki <[email protected]>
Signed-off-by Andi Shyti <[email protected]>
Reviewed-by Michael Turquette <[email protected]>
Signed-off-by Mark Brown <[email protected]>
---
 drivers/spi/spi-s3c64xx.c | 1 +
 1 file changed, 1 insertion(+)
drivers/spi/spi-s3c64xx.c +1 -0
index a1e84686c70c..3c09e94cf827 100644
@@ -642,6 +642,7 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
 	writel(val, regs + S3C64XX_SPI_MODE_CFG);
 
 	if (sdd->port_conf->clk_from_cmu) {
+		/* The src_clk clock is divided internally by 2 */
 		clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
 	} else {
 		/* Configure Clock */
Signature · 1 line
2.8.1
#31 lore raw

PATCH v2 6/7 spi: s3c64xx: restore removed comments

In reply to Andi Shyti
Quoted 14 lines · Andi Shyti

Patch a9e93e8 has erroneously removed some comments which are important to understand why the bus frequency is multiplied by two during the spi transfer.

Signed-off-by Andi Shyti <[email protected]>
---
 drivers/spi/spi-s3c64xx.c | 1 +
 1 file changed, 1 insertion(+)
drivers/spi/spi-s3c64xx.c +0 -0
index 6da663f..32b66f0 100644

drivers/spi/spi-s3c64xx.c static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd) · line 645

Quoted 7 lines
drivers/spi/spi-s3c64xx.c +1 -0
@@ -639,6 +639,7 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
 	writel(val, regs + S3C64XX_SPI_MODE_CFG);
 
 	if (sdd->port_conf->clk_from_cmu) {
+		/* There is half-multiplier before the SPI */
 		clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
 	} else {
 		/* Configure Clock */

Regardless of possible followup after Michael's comments:

Reviewed-by Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

author
#32 lore raw

PATCH v2 7/7 clk: exynos5433: remove CLK_IGNORE_UNUSED flag from SPI clocks

In reply to Andi Shyti

Because the Exynos5433 SPI driver supports the ioclk handling and the following patch:

http://marc.info/?l=linux-kernel&m=146787645626318&w=2

fixes a synchronus abort issue, none of the SPI clocks require any critical handling: remove, then, the CLK_IGNORE_UNUSED flag for the SPI related clocks.

Signed-off-by Andi Shyti <[email protected]>
Reviewed-by Krzysztof Kozlowski <[email protected]>
---
 drivers/clk/samsung/clk-exynos5433.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/clk/samsung/clk-exynos5433.c +2 -3
index c3a5318..fb19525 100644
@@ -1661,8 +1661,7 @@ static struct samsung_gate_clock peric_gate_clks[] __initdata = {
 	GATE(CLK_SCLK_IOCLK_SPI2, "sclk_ioclk_spi2", "ioclk_spi2_clk_in",
 			ENABLE_SCLK_PERIC, 13, CLK_SET_RATE_PARENT, 0),
 	GATE(CLK_SCLK_IOCLK_SPI1, "sclk_ioclk_spi1", "ioclk_spi1_clk_in",
-			ENABLE_SCLK_PERIC, 12,
-			CLK_IGNORE_UNUSED | CLK_SET_RATE_PARENT, 0),
+			ENABLE_SCLK_PERIC, 12, CLK_SET_RATE_PARENT, 0),
 	GATE(CLK_SCLK_IOCLK_SPI0, "sclk_ioclk_spi0", "ioclk_spi0_clk_in",
 			ENABLE_SCLK_PERIC, 11, CLK_SET_RATE_PARENT, 0),
 	GATE(CLK_SCLK_IOCLK_I2S1_BCLK, "sclk_ioclk_i2s1_bclk",

@@ -1677,7 +1676,7 @@ static struct samsung_gate_clock peric_gate_clks[] __initdata = {
 	GATE(CLK_SCLK_SPI2, "sclk_spi2", "sclk_spi2_peric", ENABLE_SCLK_PERIC,
 			5, CLK_SET_RATE_PARENT, 0),
 	GATE(CLK_SCLK_SPI1, "sclk_spi1", "sclk_spi1_peric", ENABLE_SCLK_PERIC,
-			4, CLK_IGNORE_UNUSED | CLK_SET_RATE_PARENT, 0),
+			4, CLK_SET_RATE_PARENT, 0),
 	GATE(CLK_SCLK_SPI0, "sclk_spi0", "sclk_spi0_peric", ENABLE_SCLK_PERIC,
 			3, CLK_SET_RATE_PARENT, 0),
 	GATE(CLK_SCLK_UART2, "sclk_uart2", "sclk_uart2_peric",
Signature · 1 line
2.8.1
maintainer
#33 lore raw

PATCH v2 7/7 clk: exynos5433: remove CLK_IGNORE_UNUSED flag from SPI clocks

Quoted 6 lines · Andi Shyti

Because the Exynos5433 SPI driver supports the ioclk handling and the following patch:

http://marc.info/?l=linux-kernel&m=146787645626318&w=2

fixes a synchronus abort issue, none of the SPI clocks require

s/synchronus/synchronous

Quoted 5 lines

any critical handling: remove, then, the CLK_IGNORE_UNUSED flag for the SPI related clocks.

Signed-off-by Andi Shyti <[email protected]>
Reviewed-by Krzysztof Kozlowski <[email protected]>

In case this goes through the spi tree:

Acked-by Sylwester Nawrocki <[email protected]>
#34 lore raw

v2 cover spi s3c64xx ioclk handling

In reply to Andi Shyti

Quoting Andi Shyti (2016-07-08 07:46:34)

Quoted 8 lines

Hi,

this patchset improves the compatibility between the s3c64xx driver and the exynos5433. One point of incompatibility, indeed, was that the SPI controller in exynos5433 is fed by three clocks which need to be handled by the driver. The third clock is the ioclk which support is the main purpose of this patchset.

Quoted 6 lines

Patch 6 is a bit unrelated with the rest, it restores a comment that has been removed earlier by mistake.

The last patch was first sent on its own and it was its 5th version, it has been queued to this patchset as there is a dependency with it.

Feel free to add the following to all of the patches:

Reviewed-by Michael Turquette <[email protected]>

Regards,
Mike

Quoted 44 lines

Changelog V1 -> V2:

- patch 1: the exynos5433-spi compatible is placed in alphabetical order compared to exynos7-spi

- patch 1: deprecates exynos7-spi

- patch 2: [new] added the clocks and clock-name documentation in the bindings

- patch 3: fixed some typos, renamed the goto label from err_power_off to err_pm_put add the reviewd-by Krzysztof

- patch 4: new patch

- patch 5: fixed typos in commit message and use low letters for hex numbers

- patch 6: removed one line of comment

- patch 7: added to ensure sequentiality.

Thanks,
Andi

Andi Shyti (7):
bindings: spi-samsung: add exynos5433 spi compatible
bindings: spi-samsung: document the clocks and the clock-name property
spi: s3c64xx: rename goto labels to meaningful names
spi: s3c64xx: use error code from clk_prepare_enable()
spi: s3c64xx: add exynos5433 compatible for ioclk handling
spi: s3c64xx: restore removed comments
clk: exynos5433: remove CLK_IGNORE_UNUSED flag from SPI clocks

 .../devicetree/bindings/spi/spi-samsung.txt        | 11 ++-
 drivers/clk/samsung/clk-exynos5433.c               |  5 +-
 drivers/spi/spi-s3c64xx.c                          | 90 +++++++++++++++++-----
 3 files changed, 81 insertions(+), 25 deletions(-)
Signature · 1 line
2.8.1