From: Levin Du <[email protected]>
Hi all, this is an attemp to add sdmmc UHS support to the
ROC-RK3328-CC board.
This patch series adds a new compatible `rockchip,gpio-syscon` to
the gpio-syscon driver for general Rockchip SoC usage..
A new gpio controller named `gpio_syscon10` is defined in
rk3328.dtsi so that all rk3328 boards has access to it.
The ROC-RK3328-CC board use the new gpio <&gpio_syscon10 1> in
gpio-regulator to control the signal voltage of the sdmmc.
It is essential for UHS support which requires 1.8V signal voltage.
Many thanks to Heiko's great advice!
Changes in v1:
- Refactured for general gpio-syscon usage for Rockchip SoCs.
- Add doc rockchip,gpio-syscon.txt .
- Split into small patches.
- Add gpio-syscon10 to rk3328 for general use.
- Sort dts properties in sdmmc node.
Heiko Stuebner (1):
gpio: syscon: allow fetching syscon from parent node
Levin Du (4):
gpio: syscon: Add gpio-syscon for rockchip
arm64: dts: rockchip: Add gpio-syscon10 to rk3328
arm64: dts: rockchip: Add io-domain to roc-rk3328-cc
arm64: dts: rockchip: Add sdmmc UHS support for roc-rk3328-cc
.../bindings/gpio/rockchip,gpio-syscon.txt | 41 ++++++++++++++++++++++
arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts | 30 ++++++++++++++++
arch/arm64/boot/dts/rockchip/rk3328.dtsi | 6 ++++
drivers/gpio/gpio-syscon.c | 32 +++++++++++++++++
4 files changed, 109 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
--
2.7.4
From: Heiko Stuebner <[email protected]>
Syscon nodes can be a simple-mfd and the syscon-users then be declared
as children of this node. That way the parent-child structure can be
better represented for devices that are fully embedded in the syscon.
Therefore allow getting the syscon from the parent if neither
a special compatible nor a gpio,syscon-dev property is defined.
Signed-off-by: Heiko Stuebner <[email protected]>
Signed-off-by: Levin Du <[email protected]>
---
Changes in v1:
- New
drivers/gpio/gpio-syscon.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c
index 537cec7..7325b86 100644
--- a/drivers/gpio/gpio-syscon.c
+++ b/drivers/gpio/gpio-syscon.c
@@ -205,6 +205,8 @@ static int syscon_gpio_probe(struct platform_device *pdev)
} else {
priv->syscon =
syscon_regmap_lookup_by_phandle(np, "gpio,syscon-dev");
+ if (IS_ERR(priv->syscon) && np->parent)
+ priv->syscon = syscon_node_to_regmap(np->parent);
if (IS_ERR(priv->syscon))
return PTR_ERR(priv->syscon);
--
2.7.4
From: Levin Du <[email protected]>
Adding a new gpio controller named "gpio-syscon10" to rk3328, providing
access to the pins defined in the syscon GRF_SOC_CON10.
Boards using these special pins to control regulators or LEDs, can now
utilize existing drivers like gpio-regulator and leds-gpio.
Signed-off-by: Levin Du <[email protected]>
---
Changes in v1:
- Split from V0 and add to rk3328.dtsi for general use.
arch/arm64/boot/dts/rockchip/rk3328.dtsi | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
index b8e9da1..73a822d 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
@@ -309,6 +309,12 @@
mode-loader = <BOOT_BL_DOWNLOAD>;
};
+ gpio_syscon10: gpio-syscon10 {
+ compatible = "rockchip,gpio-syscon";
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio,syscon-dev = <0 0x0428 0>;
+ };
};
uart0: serial@ff110000 {
--
2.7.4
From: Levin Du <[email protected]>
It is necessary for the io domain setting of the SoC to match
the voltage supplied by the regulators.
Signed-off-by: Levin Du <[email protected]>
---
Changes in v1:
- Split from V0.
arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts b/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
index 246c317..b983abd 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
@@ -208,6 +208,18 @@
};
};
+&io_domains {
+ status = "okay";
+
+ vccio1-supply = <&vcc_io>;
+ vccio2-supply = <&vcc18_emmc>;
+ vccio3-supply = <&vcc_io>;
+ vccio4-supply = <&vcc_18>;
+ vccio5-supply = <&vcc_io>;
+ vccio6-supply = <&vcc_io>;
+ pmuio-supply = <&vcc_io>;
+};
+
&pinctrl {
pmic {
pmic_int_l: pmic-int-l {
--
2.7.4
From: Levin Du <[email protected]>
Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
which do not belong to the general pinctrl.
Adding gpio-syscon support makes controlling regulator or
LED using these special pins very easy by reusing existing
drivers, such as gpio-regulator and led-gpio.
Signed-off-by: Levin Du <[email protected]>
---
Changes in v1:
- Refactured for general gpio-syscon usage for Rockchip SoCs.
- Add doc rockchip,gpio-syscon.txt
.../bindings/gpio/rockchip,gpio-syscon.txt | 41 ++++++++++++++++++++++
drivers/gpio/gpio-syscon.c | 30 ++++++++++++++++
2 files changed, 71 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
diff --git a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
new file mode 100644
index 0000000..e4c1650
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
@@ -0,0 +1,41 @@
+* Rockchip GPIO support for GRF_SOC_CON registers
+
+Required properties:
+- compatible: Should contain "rockchip,gpio-syscon".
+- gpio-controller: Marks the device node as a gpio controller.
+- #gpio-cells: Should be two. The first cell is the pin number and
+ the second cell is used to specify the gpio polarity:
+ 0 = Active high,
+ 1 = Active low.
+- gpio,syscon-dev: Should contain <grf_phandle syscon_offset 0>.
+ If declared as child of the grf node, the grf_phandle can be 0.
+
+Example:
+
+1. As child of grf node:
+
+ grf: syscon@ff100000 {
+ compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
+
+ gpio_syscon10: gpio-syscon10 {
+ compatible = "rockchip,gpio-syscon";
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio,syscon-dev = <0 0x0428 0>;
+ };
+ };
+
+
+2. Not child of grf node:
+
+ grf: syscon@ff100000 {
+ compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
+ //...
+ };
+
+ gpio_syscon10: gpio-syscon10 {
+ compatible = "rockchip,gpio-syscon";
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio,syscon-dev = <&grf 0x0428 0>;
+ };
diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c
index 7325b86..e24b408 100644
--- a/drivers/gpio/gpio-syscon.c
+++ b/drivers/gpio/gpio-syscon.c
@@ -135,6 +135,32 @@ static const struct syscon_gpio_data clps711x_mctrl_gpio = {
.dat_bit_offset = 0x40 * 8 + 8,
};
+static void rockchip_gpio_set(struct gpio_chip *chip, unsigned int offset,
+ int val)
+{
+ struct syscon_gpio_priv *priv = gpiochip_get_data(chip);
+ unsigned int offs;
+ u8 bit;
+ u32 data;
+ int ret;
+
+ offs = priv->dreg_offset + priv->data->dat_bit_offset + offset;
+ bit = offs % SYSCON_REG_BITS;
+ data = (val ? BIT(bit) : 0) | BIT(bit + 16);
+ ret = regmap_write(priv->syscon,
+ (offs / SYSCON_REG_BITS) * SYSCON_REG_SIZE,
+ data);
+ if (ret < 0)
+ dev_err(chip->parent, "gpio write failed ret(%d)\n", ret);
+}
+
+static const struct syscon_gpio_data rockchip_gpio_syscon = {
+ /* Rockchip GRF_SOC_CON Bits 0-15 */
+ .flags = GPIO_SYSCON_FEAT_OUT,
+ .bit_count = 16,
+ .set = rockchip_gpio_set,
+};
+
#define KEYSTONE_LOCK_BIT BIT(0)
static void keystone_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
@@ -175,6 +201,10 @@ static const struct of_device_id syscon_gpio_ids[] = {
.compatible = "ti,keystone-dsp-gpio",
.data = &keystone_dsp_gpio,
},
+ {
+ .compatible = "rockchip,gpio-syscon",
+ .data = &rockchip_gpio_syscon,
+ },
{ }
};
MODULE_DEVICE_TABLE(of, syscon_gpio_ids);
--
2.7.4
From: Levin Du <[email protected]>
It is necessary for the io domain setting of the SoC to match
the voltage supplied by the regulators.
Signed-off-by: Levin Du <[email protected]>
---
Changes in v1:
- Split from V0.
arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts b/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
index 246c317..b983abd 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
@@ -208,6 +208,18 @@
};
};
+&io_domains {
+ status = "okay";
+
+ vccio1-supply = <&vcc_io>;
+ vccio2-supply = <&vcc18_emmc>;
+ vccio3-supply = <&vcc_io>;
+ vccio4-supply = <&vcc_18>;
+ vccio5-supply = <&vcc_io>;
+ vccio6-supply = <&vcc_io>;
+ pmuio-supply = <&vcc_io>;
+};
+
&pinctrl {
pmic {
pmic_int_l: pmic-int-l {
--
2.7.4
From: Levin Du <[email protected]>
In roc-rk3328-cc board, the signal voltage of sdmmc is supplied by
the vcc_sdio regulator, which is a mux between 1.8V and 3.3V,
controlled by a special output only gpio pin labeled
"gpiomut_pmuio_iout", corresponding bit 1 of the syscon GRF_SOC_CON10.
This special pin can now be reference as <&gpio_syscon10 1>, thanks
to the gpio-syscon driver, which makes writing regulator-gpio possible.
If the signal voltage changes, the io domain needs to change
correspondingly.
To use this feature, the following options are required in kernel config:
- CONFIG_GPIO_SYSCON=y
- CONFIG_POWER_AVS=y
- CONFIG_ROCKCHIP_IODOMAIN=y
Signed-off-by: Levin Du <[email protected]>
---
Changes in v1:
- Split into small patches
- Sort dts properties in sdmmc node
arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts b/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
index b983abd..3f33e42 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts
@@ -41,6 +41,19 @@
vin-supply = <&vcc_io>;
};
+ vcc_sdio: sdmmcio-regulator {
+ compatible = "regulator-gpio";
+ gpios = <&gpio_syscon10 1 GPIO_ACTIVE_HIGH>;
+ states = <1800000 0x1
+ 3300000 0x0>;
+ regulator-name = "vcc_sdio";
+ regulator-type = "voltage";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-always-on;
+ vin-supply = <&vcc_sys>;
+ };
+
vcc_host1_5v: vcc_otg_5v: vcc-host1-5v-regulator {
compatible = "regulator-fixed";
enable-active-high;
@@ -213,7 +226,7 @@
vccio1-supply = <&vcc_io>;
vccio2-supply = <&vcc18_emmc>;
- vccio3-supply = <&vcc_io>;
+ vccio3-supply = <&vcc_sdio>;
vccio4-supply = <&vcc_18>;
vccio5-supply = <&vcc_io>;
vccio6-supply = <&vcc_io>;
@@ -242,7 +255,12 @@
max-frequency = <150000000>;
pinctrl-names = "default";
pinctrl-0 = <&sdmmc0_clk &sdmmc0_cmd &sdmmc0_dectn &sdmmc0_bus4>;
+ sd-uhs-sdr12;
+ sd-uhs-sdr25;
+ sd-uhs-sdr50;
+ sd-uhs-sdr104;
vmmc-supply = <&vcc_sd>;
+ vqmmc-supply = <&vcc_sdio>;
status = "okay";
};
--
2.7.4
On 10/05/18 10:16, [email protected] wrote:
> From: Levin Du <[email protected]>
>
> Adding a new gpio controller named "gpio-syscon10" to rk3328, providing
> access to the pins defined in the syscon GRF_SOC_CON10.
This is the GPIO_MUTE pin, right? The public TRM is rather vague, but
cross-referencing against the datasheet and schematics implies that it's
the "gpiomut_*" part of the GRF bit names which is most significant.
It might be worth using a more descriptive name here, since "syscon10"
is pretty much meaningless at the board level.
Robin.
> Boards using these special pins to control regulators or LEDs, can now
> utilize existing drivers like gpio-regulator and leds-gpio.
>
> Signed-off-by: Levin Du <[email protected]>
>
> ---
>
> Changes in v1:
> - Split from V0 and add to rk3328.dtsi for general use.
>
> arch/arm64/boot/dts/rockchip/rk3328.dtsi | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> index b8e9da1..73a822d 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> @@ -309,6 +309,12 @@
> mode-loader = <BOOT_BL_DOWNLOAD>;
> };
>
> + gpio_syscon10: gpio-syscon10 {
> + compatible = "rockchip,gpio-syscon";
> + gpio-controller;
> + #gpio-cells = <2>;
> + gpio,syscon-dev = <0 0x0428 0>;
> + };
> };
>
> uart0: serial@ff110000 {
>
On 10/05/18 10:16, [email protected] wrote:
> From: Levin Du <[email protected]>
>
> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
> which do not belong to the general pinctrl.
>
> Adding gpio-syscon support makes controlling regulator or
> LED using these special pins very easy by reusing existing
> drivers, such as gpio-regulator and led-gpio.
>
> Signed-off-by: Levin Du <[email protected]>
>
> ---
>
> Changes in v1:
> - Refactured for general gpio-syscon usage for Rockchip SoCs.
> - Add doc rockchip,gpio-syscon.txt
>
> .../bindings/gpio/rockchip,gpio-syscon.txt | 41 ++++++++++++++++++++++
> drivers/gpio/gpio-syscon.c | 30 ++++++++++++++++
> 2 files changed, 71 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> new file mode 100644
> index 0000000..e4c1650
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
> @@ -0,0 +1,41 @@
> +* Rockchip GPIO support for GRF_SOC_CON registers
> +
> +Required properties:
> +- compatible: Should contain "rockchip,gpio-syscon".
> +- gpio-controller: Marks the device node as a gpio controller.
> +- #gpio-cells: Should be two. The first cell is the pin number and
I would suggest s/pin number/bit number in the associated GRF register/
here. At least in this RK3328 case there's only one pin, which isn't
numbered, and if you naively considered it pin 0 of this 'bank' you'd
already have the wrong number. Since we're dealing with the "random
SoC-specific controls" region of the GRF as opposed to the
relatively-consistent and organised pinmux parts, I don't think we
should rely on any assumptions about how things are laid out.
I was initially going to suggest a more specific compatible string as
well, but on reflection I think the generic "rockchip,gpio-syscon" for
basic "flip this single GRF bit" functionality actually is the right way
to go. In the specific RK3328 GPIO_MUTE case, there look to be 4 bits in
total related to this pin - the enable, value, and some pull controls
(which I assume apply when the output is disabled) - if at some point in
future we *did* want to start explicitly controlling the rest of them
too, then would be a good time to define a separate
"rockchip,rk3328-gpio-mute" binding (and probably a dedicated driver)
for that specialised functionality, independently of this basic one.
> + the second cell is used to specify the gpio polarity:
> + 0 = Active high,
> + 1 = Active low.
> +- gpio,syscon-dev: Should contain <grf_phandle syscon_offset 0>.
> + If declared as child of the grf node, the grf_phandle can be 0.
> +
> +Example:
> +
> +1. As child of grf node:
> +
> + grf: syscon@ff100000 {
> + compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
> +
> + gpio_syscon10: gpio-syscon10 {
> + compatible = "rockchip,gpio-syscon";
> + gpio-controller;
> + #gpio-cells = <2>;
> + gpio,syscon-dev = <0 0x0428 0>;
> + };
> + };
> +
> +
> +2. Not child of grf node:
> +
> + grf: syscon@ff100000 {
> + compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd";
> + //...
> + };
> +
> + gpio_syscon10: gpio-syscon10 {
> + compatible = "rockchip,gpio-syscon";
> + gpio-controller;
> + #gpio-cells = <2>;
> + gpio,syscon-dev = <&grf 0x0428 0>;
> + };
> diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c
> index 7325b86..e24b408 100644
> --- a/drivers/gpio/gpio-syscon.c
> +++ b/drivers/gpio/gpio-syscon.c
> @@ -135,6 +135,32 @@ static const struct syscon_gpio_data clps711x_mctrl_gpio = {
> .dat_bit_offset = 0x40 * 8 + 8,
> };
>
> +static void rockchip_gpio_set(struct gpio_chip *chip, unsigned int offset,
> + int val)
> +{
> + struct syscon_gpio_priv *priv = gpiochip_get_data(chip);
> + unsigned int offs;
> + u8 bit;
> + u32 data;
> + int ret;
> +
> + offs = priv->dreg_offset + priv->data->dat_bit_offset + offset;
data->dat_bit_offset is always 0 here, but given that wrapping large
offsets to successive GRF registers doesn't make sense (and wouldn't
work anyway with this arithmetic) I don't think you even need this
calculation of offs at all...
> + bit = offs % SYSCON_REG_BITS;
... since it would suffice to use offset here...
> + data = (val ? BIT(bit) : 0) | BIT(bit + 16);
> + ret = regmap_write(priv->syscon,
> + (offs / SYSCON_REG_BITS) * SYSCON_REG_SIZE,
... and priv->dreg_offset here.
Robin.
> + data);
> + if (ret < 0)
> + dev_err(chip->parent, "gpio write failed ret(%d)\n", ret);
> +}
> +
> +static const struct syscon_gpio_data rockchip_gpio_syscon = {
> + /* Rockchip GRF_SOC_CON Bits 0-15 */
> + .flags = GPIO_SYSCON_FEAT_OUT,
> + .bit_count = 16,
> + .set = rockchip_gpio_set,
> +};
> +
> #define KEYSTONE_LOCK_BIT BIT(0)
>
> static void keystone_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
> @@ -175,6 +201,10 @@ static const struct of_device_id syscon_gpio_ids[] = {
> .compatible = "ti,keystone-dsp-gpio",
> .data = &keystone_dsp_gpio,
> },
> + {
> + .compatible = "rockchip,gpio-syscon",
> + .data = &rockchip_gpio_syscon,
> + },
> { }
> };
> MODULE_DEVICE_TABLE(of, syscon_gpio_ids);
>
On 2018-05-10 10:56 PM, Robin Murphy wrote:
>
>> diff --git
>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> new file mode 100644
>> index 0000000..e4c1650
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> @@ -0,0 +1,41 @@
>> +* Rockchip GPIO support for GRF_SOC_CON registers
>> +
>> +Required properties:
>> +- compatible: Should contain "rockchip,gpio-syscon".
>> +- gpio-controller: Marks the device node as a gpio controller.
>> +- #gpio-cells: Should be two. The first cell is the pin number and
>
> I would suggest s/pin number/bit number in the associated GRF
> register/ here. At least in this RK3328 case there's only one pin,
> which isn't numbered, and if you naively considered it pin 0 of this
> 'bank' you'd already have the wrong number. Since we're dealing with
> the "random SoC-specific controls" region of the GRF as opposed to the
> relatively-consistent and organised pinmux parts, I don't think we
> should rely on any assumptions about how things are laid out.
>
> I was initially going to suggest a more specific compatible string as
> well, but on reflection I think the generic "rockchip,gpio-syscon" for
> basic "flip this single GRF bit" functionality actually is the right
> way to go. In the specific RK3328 GPIO_MUTE case, there look to be 4
> bits in total related to this pin - the enable, value, and some pull
> controls (which I assume apply when the output is disabled) - if at
> some point in future we *did* want to start explicitly controlling the
> rest of them too, then would be a good time to define a separate
> "rockchip,rk3328-gpio-mute" binding (and probably a dedicated driver)
> for that specialised functionality, independently of this basic one.
>
Good point! I'll fix the doc.
>> +static void rockchip_gpio_set(struct gpio_chip *chip, unsigned int
>> offset,
>> + int val)
>> +{
>> + struct syscon_gpio_priv *priv = gpiochip_get_data(chip);
>> + unsigned int offs;
>> + u8 bit;
>> + u32 data;
>> + int ret;
>> +
>> + offs = priv->dreg_offset + priv->data->dat_bit_offset + offset;
>
> data->dat_bit_offset is always 0 here, but given that wrapping large
> offsets to successive GRF registers doesn't make sense (and wouldn't
> work anyway with this arithmetic) I don't think you even need this
> calculation of offs at all...
>
>> + bit = offs % SYSCON_REG_BITS;
>
> ... since it would suffice to use offset here...
>
>> + data = (val ? BIT(bit) : 0) | BIT(bit + 16);
>> + ret = regmap_write(priv->syscon,
>> + (offs / SYSCON_REG_BITS) * SYSCON_REG_SIZE,
>
> ... and priv->dreg_offset here.
>
rockchip_gpio_set() follows the conventional offset handling in the
gpio-syscon driver.
dreg_offset will get multiplied by 8 (dreg_offset <<= 3) in
syscon_gpio_probe().
I'm not sure if this needs to simplify, or stay the same as others.
Thanks
Levin
On 2018-05-10 8:50 PM, Robin Murphy wrote:
> On 10/05/18 10:16, [email protected] wrote:
>> From: Levin Du <[email protected]>
>>
>> Adding a new gpio controller named "gpio-syscon10" to rk3328, providing
>> access to the pins defined in the syscon GRF_SOC_CON10.
>
> This is the GPIO_MUTE pin, right? The public TRM is rather vague, but
> cross-referencing against the datasheet and schematics implies that
> it's the "gpiomut_*" part of the GRF bit names which is most significant.
>
> It might be worth using a more descriptive name here, since "syscon10"
> is pretty much meaningless at the board level.
>
> Robin.
>
Previously I though other bits might be able to reference from syscon10,
other than GPIO_MUTE alone.
If it is renamed to gpio-mute, then the GPIO_MUTE pin is accessed as
`<&gpio-mute 1>`. Yet other
bits in syscon10 can also be referenced, say, `<&gpio-mute 10>`, which
is not good.
I'd like to add a `gpio,syscon-bit` property to gpio-syscon, which
overrides the properties
of bit_count, data_bit_offset and dir_bit_offset in the driver. For
example:
gpio_mute: gpio-mute {
compatible = "rockchip,gpio-syscon";
gpio-controller;
#gpio-cells = <2>;
gpio,syscon-dev = <0 0x0428 0>;
gpio,syscon-bit = <1 1 0>;
};
That way, the mute pin is strictly specified as <&gpio_mute 0>, and
<&gpio_mute 1> will be invalid.
I think that is neat, and consistent with the gpio_mute name.
Thanks
Levin
On Thu, May 10, 2018 at 4:16 AM, <[email protected]> wrote:
> From: Levin Du <[email protected]>
>
> Adding a new gpio controller named "gpio-syscon10" to rk3328, providing
> access to the pins defined in the syscon GRF_SOC_CON10.
>
> Boards using these special pins to control regulators or LEDs, can now
> utilize existing drivers like gpio-regulator and leds-gpio.
>
> Signed-off-by: Levin Du <[email protected]>
>
> ---
>
> Changes in v1:
> - Split from V0 and add to rk3328.dtsi for general use.
>
> arch/arm64/boot/dts/rockchip/rk3328.dtsi | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> index b8e9da1..73a822d 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> @@ -309,6 +309,12 @@
> mode-loader = <BOOT_BL_DOWNLOAD>;
> };
>
> + gpio_syscon10: gpio-syscon10 {
GPIO controller nodes should be named just 'gpio'.
> + compatible = "rockchip,gpio-syscon";
> + gpio-controller;
> + #gpio-cells = <2>;
> + gpio,syscon-dev = <0 0x0428 0>;
This property is not documented and takes a phandle.
> + };
> };
>
> uart0: serial@ff110000 {
> --
> 2.7.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 10, 2018 at 10:45 PM, Levin Du <[email protected]> wrote:
> On 2018-05-10 8:50 PM, Robin Murphy wrote:
>>
>> On 10/05/18 10:16, [email protected] wrote:
>>>
>>> From: Levin Du <[email protected]>
>>>
>>> Adding a new gpio controller named "gpio-syscon10" to rk3328, providing
>>> access to the pins defined in the syscon GRF_SOC_CON10.
>>
>>
>> This is the GPIO_MUTE pin, right? The public TRM is rather vague, but
>> cross-referencing against the datasheet and schematics implies that it's the
>> "gpiomut_*" part of the GRF bit names which is most significant.
>>
>> It might be worth using a more descriptive name here, since "syscon10" is
>> pretty much meaningless at the board level.
>>
>> Robin.
>>
> Previously I though other bits might be able to reference from syscon10,
> other than GPIO_MUTE alone.
> If it is renamed to gpio-mute, then the GPIO_MUTE pin is accessed as
> `<&gpio-mute 1>`. Yet other
> bits in syscon10 can also be referenced, say, `<&gpio-mute 10>`, which is
> not good.
>
> I'd like to add a `gpio,syscon-bit` property to gpio-syscon, which overrides
> the properties
> of bit_count, data_bit_offset and dir_bit_offset in the driver. For
No. Once you are describing individual register bits, it is too low
level for DT.
> example:
>
> gpio_mute: gpio-mute {
> compatible = "rockchip,gpio-syscon";
> gpio-controller;
> #gpio-cells = <2>;
> gpio,syscon-dev = <0 0x0428 0>;
> gpio,syscon-bit = <1 1 0>;
> };
>
> That way, the mute pin is strictly specified as <&gpio_mute 0>, and
> <&gpio_mute 1> will be invalid.
> I think that is neat, and consistent with the gpio_mute name.
>
> Thanks
> Levin
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018-05-11 8:22 PM, Rob Herring wrote:
> On Thu, May 10, 2018 at 4:16 AM, <[email protected]> wrote:
>> From: Levin Du <[email protected]>
>>
>> Adding a new gpio controller named "gpio-syscon10" to rk3328, providing
>> access to the pins defined in the syscon GRF_SOC_CON10.
>>
>> Boards using these special pins to control regulators or LEDs, can now
>> utilize existing drivers like gpio-regulator and leds-gpio.
>>
>> Signed-off-by: Levin Du <[email protected]>
>>
>> ---
>>
>> Changes in v1:
>> - Split from V0 and add to rk3328.dtsi for general use.
>>
>> arch/arm64/boot/dts/rockchip/rk3328.dtsi | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>> index b8e9da1..73a822d 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>> @@ -309,6 +309,12 @@
>> mode-loader = <BOOT_BL_DOWNLOAD>;
>> };
>>
>> + gpio_syscon10: gpio-syscon10 {
> GPIO controller nodes should be named just 'gpio'.
'gpio' is a general name, and there're already gpio0~gpio3 for pinctrl
GPIOs.
>> + compatible = "rockchip,gpio-syscon";
>> + gpio-controller;
>> + #gpio-cells = <2>;
>> + gpio,syscon-dev = <0 0x0428 0>;
> This property is not documented and takes a phandle.
See PATCH1 which allows fetching syscon from parent node .
This is also documented in
Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
in PATCH2.
Thanks
Levin
On 2018-05-11 8:24 PM, Rob Herring wrote:
> On Thu, May 10, 2018 at 10:45 PM, Levin Du <[email protected]> wrote:
>> On 2018-05-10 8:50 PM, Robin Murphy wrote:
>>> On 10/05/18 10:16, [email protected] wrote:
>>>> From: Levin Du <[email protected]>
>>>>
>>>> Adding a new gpio controller named "gpio-syscon10" to rk3328, providing
>>>> access to the pins defined in the syscon GRF_SOC_CON10.
>>>
>>> This is the GPIO_MUTE pin, right? The public TRM is rather vague, but
>>> cross-referencing against the datasheet and schematics implies that it's the
>>> "gpiomut_*" part of the GRF bit names which is most significant.
>>>
>>> It might be worth using a more descriptive name here, since "syscon10" is
>>> pretty much meaningless at the board level.
>>>
>>> Robin.
>>>
>> Previously I though other bits might be able to reference from syscon10,
>> other than GPIO_MUTE alone.
>> If it is renamed to gpio-mute, then the GPIO_MUTE pin is accessed as
>> `<&gpio-mute 1>`. Yet other
>> bits in syscon10 can also be referenced, say, `<&gpio-mute 10>`, which is
>> not good.
>>
>> I'd like to add a `gpio,syscon-bit` property to gpio-syscon, which overrides
>> the properties
>> of bit_count, data_bit_offset and dir_bit_offset in the driver. For
> No. Once you are describing individual register bits, it is too low
> level for DT.
Okay. So I'll rename it to gpio_mute, and reference the output pin as
<&gpio_mute 1>:
+ // Use <&gpio_mute 1> to ref to GPIO_MUTE pin
+ gpio_mute: gpio-mute {
+ compatible = "rockchip,gpio-syscon";
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio,syscon-dev = <0 0x0428 0>;
+ };
};
Thanks
Levin