Skip to content

Commit

Permalink
ath79: add new ar934x spi driver
Browse files Browse the repository at this point in the history 
A new shift mode was introduced since ar934x which has a way better
performance than current bitbang driver and can handle higher spi
clock properly. This commit adds a new driver to make use of this
new feature.
This new driver has chipselect properly configured and we don't need
cs-gpios hack in dts anymore. Remove them.

Signed-off-by: Chuanhong Guo <gch981213@gmail.com>
  • Loading branch information
@981213
981213 committed Feb 6, 2020
1 parent aca2740 commit ebf0d8d
Show file tree
Hide file tree
Showing 11 changed files with 346 additions and 15 deletions.
1 change: 1 addition & 0 deletions target/linux/ath79/config-4.19
View file
Expand Up @@ -211,6 +211,7 @@ CONFIG_SERIAL_AR933X_CONSOLE=y
CONFIG_SERIAL_AR933X_NR_UARTS=2
CONFIG_SERIAL_OF_PLATFORM=y
CONFIG_SPI=y
CONFIG_SPI_AR934X=y
CONFIG_SPI_ATH79=y
CONFIG_SPI_BITBANG=y
CONFIG_SPI_GPIO=y
Expand Down
1 change: 0 additions & 1 deletion target/linux/ath79/dts/ar9344_qihoo_c301.dts
View file
Expand Up @@ -110,7 +110,6 @@
status = "okay";

num-cs = <2>;
cs-gpios= <0>, <0>;

flash@0 {
#address-cells = <1>;
Expand Down
3 changes: 1 addition & 2 deletions target/linux/ath79/dts/ar934x.dtsi
View file
Expand Up @@ -183,11 +183,10 @@
};

spi: spi@1f000000 {
compatible = "qca,ar9340-spi", "qca,ar7100-spi";
compatible = "qca,ar934x-spi";
reg = <0x1f000000 0x1c>;

clocks = <&pll ATH79_CLK_AHB>;
clock-names = "ahb";

#address-cells = <1>;
#size-cells = <0>;
Expand Down
1 change: 0 additions & 1 deletion target/linux/ath79/dts/qca9531_glinet_gl-ar300m.dtsi
View file
Expand Up @@ -76,7 +76,6 @@
status = "okay";

num-cs = <2>;
cs-gpios = <0>, <0>;

flash@0 {
compatible = "jedec,spi-nor";
Expand Down
5 changes: 2 additions & 3 deletions target/linux/ath79/dts/qca953x.dtsi
View file
Expand Up @@ -201,11 +201,10 @@
};

spi: spi@1f000000 {
compatible = "qca,ar9530-spi", "qca,ar7100-spi";
reg = <0x1f000000 0x10>;
compatible = "qca,ar934x-spi";
reg = <0x1f000000 0x1c>;

clocks = <&pll ATH79_CLK_AHB>;
clock-names = "ahb";

status = "disabled";

Expand Down
5 changes: 2 additions & 3 deletions target/linux/ath79/dts/qca9557.dtsi
View file
Expand Up @@ -292,11 +292,10 @@
};

spi: spi@1f000000 {
compatible = "qca,ar9557-spi", "qca,ar7100-spi";
reg = <0x1f000000 0x10>;
compatible = "qca,ar934x-spi";
reg = <0x1f000000 0x1c>;

clocks = <&pll ATH79_CLK_AHB>;
clock-names = "ahb";

status = "disabled";

Expand Down
1 change: 0 additions & 1 deletion target/linux/ath79/dts/qca9563_glinet_gl-ar750s.dtsi
View file
Expand Up @@ -75,7 +75,6 @@
status = "okay";

num-cs = <2>;
cs-gpios = <0>, <0>;

flash_nor: flash@0 {
compatible = "jedec,spi-nor";
Expand Down
1 change: 0 additions & 1 deletion target/linux/ath79/dts/qca9563_netgear_wndr.dtsi
View file
Expand Up @@ -100,7 +100,6 @@
status = "okay";

num-cs = <2>;
cs-gpios = <0>, <0>;

flash@0 {
compatible = "jedec,spi-nor";
Expand Down
5 changes: 2 additions & 3 deletions target/linux/ath79/dts/qca956x.dtsi
View file
Expand Up @@ -215,11 +215,10 @@
};

spi: spi@1f000000 {
compatible = "qca,qca9560-spi", "qca,ar7100-spi";
reg = <0x1f000000 0x10>;
compatible = "qca,ar934x-spi";
reg = <0x1f000000 0x1c>;

clocks = <&pll ATH79_CLK_AHB>;
clock-names = "ahb";

status = "disabled";

Expand Down
61 changes: 61 additions & 0 deletions ...t/linux/ath79/patches-4.19/0050-v5.1-drivers-provide-devm_platform_ioremap_resource.patch
View file
@@ -0,0 +1,61 @@
From 7945f929f1a77a1c8887a97ca07f87626858ff42 Mon Sep 17 00:00:00 2001
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Date: Wed, 20 Feb 2019 11:12:39 +0000
Subject: [PATCH] drivers: provide devm_platform_ioremap_resource()

There are currently 1200+ instances of using platform_get_resource()
and devm_ioremap_resource() together in the kernel tree.

This patch wraps these two calls in a single helper. Thanks to that
we don't have to declare a local variable for struct resource * and can
omit the redundant argument for resource type. We also have one
function call less.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/base/platform.c | 18 ++++++++++++++++++
include/linux/platform_device.h | 3 +++
2 files changed, 21 insertions(+)

--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -80,6 +80,24 @@ struct resource *platform_get_resource(s
EXPORT_SYMBOL_GPL(platform_get_resource);

/**
+ * devm_platform_ioremap_resource - call devm_ioremap_resource() for a platform
+ * device
+ *
+ * @pdev: platform device to use both for memory resource lookup as well as
+ * resource managemend
+ * @index: resource index
+ */
+void __iomem *devm_platform_ioremap_resource(struct platform_device *pdev,
+ unsigned int index)
+{
+ struct resource *res;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, index);
+ return devm_ioremap_resource(&pdev->dev, res);
+}
+EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource);
+
+/**
* platform_get_irq - get an IRQ for a device
* @dev: platform device
* @num: IRQ number index
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -51,6 +51,9 @@ extern struct device platform_bus;
extern void arch_setup_pdev_archdata(struct platform_device *);
extern struct resource *platform_get_resource(struct platform_device *,
unsigned int, unsigned int);
+extern void __iomem *
+devm_platform_ioremap_resource(struct platform_device *pdev,
+ unsigned int index);
extern int platform_get_irq(struct platform_device *, unsigned int);
extern int platform_irq_count(struct platform_device *);
extern struct resource *platform_get_resource_byname(struct platform_device *,

8 comments on commit ebf0d8d

@jneuhauser
Copy link
Contributor

@jneuhauser jneuhauser commented on ebf0d8d Feb 6, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new driver has chipselect properly configured and we don't need
cs-gpios hack in dts anymore. Remove them.

Why do you think this is a hack?
This is the official way to define hw cs gpios!
Documentation/devicetree/bindings/spi/spi-controller.yaml
Do we lose support for non hw cs gpios with this driver?

@981213
Copy link
Member Author

@981213 981213 commented on ebf0d8d Feb 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these lines aren't defining cs-gpios, they just trick the framework to fix the incorrect chipselect number in ath79-spi for us.
ar934x and later SoCs can mux 3 hw cs to any gpio pins so losing the ability to set gpio cs isn't a big deal. And one could still use old driver by changing compatible string if they really need more than 3 cs pins.

@biemster
Copy link

@biemster biemster commented on ebf0d8d Oct 15, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so losing the ability to set gpio cs isn't a big deal.

cs-gpios is a nice and easy, straightforward and clear way to define chipselects, so this is definitely a bit of a deal at least. Using pinmuxes for this will complicate the already confusing world of device trees especially because cs-gpios doesn't do anything now anymore (which took me quite a while to realize).
Is there a way to get cs-gpios back with ar934x, so without reverting to the at7100-spi driver?

EDIT: just noticed that this was merged almost 4 years ago, but the argument still stands

@981213
Copy link
Member Author

@981213 981213 commented on ebf0d8d Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to get cs-gpios back with ar934x, so without reverting to the at7100-spi driver?

The shift-register mode in these SPI controller requires a hardware chipselect pin: The chipselect is asserted and de-asserted by hardware.
There are two possible ways to get cs-gpios working in the driver:

  1. Use a hardware CS alongside cs-gpios and don't mux this pin to any gpio. Let SPI controller assert an unused CS and control the actual GPIO CS with software before and after each transfer.
  2. Add support for the slow bit-bang mode, which is basically duplicating the ar7100-spi driver. (At which point it's easier to just change the compatible instead.)

I think neither solution worths the effort when configuring pinmux is enough for 3 or less SPI devices. Instead we need a proper pinctrl driver for ar934x and later SoCs to make the dt-bindings readable. (We are currently writing raw register values using pinctrl-single.)

@biemster
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to get cs-gpios back with ar934x, so without reverting to the at7100-spi driver?

We are currently writing raw register values using pinctrl-single.

Thanks @981213 . I agree both solutions are a lot of effort, and reverting to ar7100-spi seems to work for me at least for now.
I tried to get a mux working with pinctrl-single, but documentation for that is scarce to say the least (I'm working with a qca9533). Copying the code from mikrotik_routerboard-750-r2.dts:

openwrt/target/linux/ath79/dts/qca9533_mikrotik_routerboard-750-r2.dts

Lines 52 to 62 in 2e57028

&pinmux {
pmx_spi_cs1: pinmux_spi_cs1 {
pinctrl-single,bits = <0x8 0x0a000000 0xff000000>;
};
};
&spi {
pinctrl-names = "default";
pinctrl-0 = <&pmx_spi_cs1>;
cs-gpios = <0>, <&gpio 11 GPIO_ACTIVE_LOW>;

did not get me a CS line on pin 11 (also because cs-gpios doesn't do anything here as I learned now), and those register values are very difficult to track down.
Do you maybe have an example somewhere I can look at?

@981213
Copy link
Member Author

@981213 981213 commented on ebf0d8d Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pinctrl config seems correct to me. Could you try adding a gpio-hog and setting gpio 11 to output-high? According to the datasheet, GPIO_OE should be set to 0 (meaning the gpio should be in output mode) when using an alternative output function.

@biemster
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking, I'll try this. For my own education, how do I find the correct values in this pinctrl-single statement?
For example what would the equivalent of cs-gpios = <0>, <&gpio 4 0>, <&gpio 11 0>, <&gpio 7 0> look like in terms of pinctrl-single,bits?

@981213
Copy link
Member Author

@981213 981213 commented on ebf0d8d Oct 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking, I'll try this. For my own education, how do I find the correct values in this pinctrl-single statement?

Here's a datasheet for QCA9531:
https://github.com/Deoptim/atheros/blob/master/QCA9531_nowatermark.pdf

Currently pinctrl-single is used to write the GPIO_OUT_FUNCTION register.
The three parameters in pinctrl-single,bits are for register address, value, mask respectively.

pinctrl-single,bits = <A B C>

means

*A = (*A & (~C)) | B

Please sign in to comment.