[PATCH 0/2] gpio-rochchip

[PATCH 0/2] gpio-rochchip

[Jianqun Xu]

The first patch fixes gpio driver to support works without cru, and the second
patch try to support different dt node format.

Jianqun Xu (2):
  gpio: rockchip: make gpio work without cru module
  gpio: rockchip: get pinctrl node from 'gpio-ranges' property

 drivers/gpio/gpio-rockchip.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

-- 
2.25.1

[PATCH 1/2] gpio: rockchip: make gpio work without cru module

[Jianqun Xu]

In some case the system may has no builtin cru module, the gpio driver
will fail to get periph clock and debounce clock.

On rockchip SoCs, the pclk and dbg clk are default to be enabled and
ungated, the gpio possible to work without cru module.

This patch makes gpio work fine without cru module.

Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>
---
 drivers/gpio/gpio-rockchip.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index a4c4e4584f5b..1da0324445cc 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -195,6 +195,9 @@ static int rockchip_gpio_set_debounce(struct gpio_chip *gc,
 	unsigned int cur_div_reg;
 	u64 div;
 
+	if (!bank->db_clk)
+		return -ENOENT;
+
 	if (bank->gpio_type == GPIO_TYPE_V2 && !IS_ERR(bank->db_clk)) {
 		div_debounce_support = true;
 		freq = clk_get_rate(bank->db_clk);
@@ -654,8 +657,10 @@ static int rockchip_get_bank_data(struct rockchip_pin_bank *bank)
 		return -EINVAL;
 
 	bank->clk = of_clk_get(bank->of_node, 0);
-	if (IS_ERR(bank->clk))
-		return PTR_ERR(bank->clk);
+	if (IS_ERR(bank->clk)) {
+		bank->clk = NULL;
+		dev_warn(bank->dev, "works without clk pm\n");
+	}
 
 	clk_prepare_enable(bank->clk);
 	id = readl(bank->reg_base + gpio_regs_v2.version_id);
@@ -666,9 +671,8 @@ static int rockchip_get_bank_data(struct rockchip_pin_bank *bank)
 		bank->gpio_type = GPIO_TYPE_V2;
 		bank->db_clk = of_clk_get(bank->of_node, 1);
 		if (IS_ERR(bank->db_clk)) {
-			dev_err(bank->dev, "cannot find debounce clk\n");
-			clk_disable_unprepare(bank->clk);
-			return -EINVAL;
+			bank->db_clk = NULL;
+			dev_warn(bank->dev, "works without debounce clk pm\n");
 		}
 	} else {
 		bank->gpio_regs = &gpio_regs_v1;
-- 
2.25.1

Re: [PATCH 1/2] gpio: rockchip: make gpio work without cru module

[Heiko Stübner]

Hi Jianqun,

Am Donnerstag, 3. März 2022, 07:22:10 CET schrieb Jianqun Xu:
> In some case the system may has no builtin cru module, the gpio driver
> will fail to get periph clock and debounce clock.

can you elaborate a bit on what these cases are?

> On rockchip SoCs, the pclk and dbg clk are default to be enabled and
> ungated, the gpio possible to work without cru module.
> 
> This patch makes gpio work fine without cru module.
> 
> Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>
> ---
>  drivers/gpio/gpio-rockchip.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index a4c4e4584f5b..1da0324445cc 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c
> @@ -195,6 +195,9 @@ static int rockchip_gpio_set_debounce(struct gpio_chip *gc,
>  	unsigned int cur_div_reg;
>  	u64 div;
>  
> +	if (!bank->db_clk)
> +		return -ENOENT;
> +
>  	if (bank->gpio_type == GPIO_TYPE_V2 && !IS_ERR(bank->db_clk)) {
>  		div_debounce_support = true;
>  		freq = clk_get_rate(bank->db_clk);
> @@ -654,8 +657,10 @@ static int rockchip_get_bank_data(struct rockchip_pin_bank *bank)
>  		return -EINVAL;
>  
>  	bank->clk = of_clk_get(bank->of_node, 0);
> -	if (IS_ERR(bank->clk))
> -		return PTR_ERR(bank->clk);
> +	if (IS_ERR(bank->clk)) {
> +		bank->clk = NULL;
> +		dev_warn(bank->dev, "works without clk pm\n");

I'd definitly expect a more sensitive handling here (and below).

I.e. the change right now, simply disables all error handling.
But I do expect a handling difference between:
- clock described in DT, but not available - should fail
- clock not described in DT - can be allowed to go to NULL


Heiko


> +	}
>  
>  	clk_prepare_enable(bank->clk);
>  	id = readl(bank->reg_base + gpio_regs_v2.version_id);
> @@ -666,9 +671,8 @@ static int rockchip_get_bank_data(struct rockchip_pin_bank *bank)
>  		bank->gpio_type = GPIO_TYPE_V2;
>  		bank->db_clk = of_clk_get(bank->of_node, 1);
>  		if (IS_ERR(bank->db_clk)) {
> -			dev_err(bank->dev, "cannot find debounce clk\n");
> -			clk_disable_unprepare(bank->clk);
> -			return -EINVAL;
> +			bank->db_clk = NULL;
> +			dev_warn(bank->dev, "works without debounce clk pm\n");
>  		}
>  	} else {
>  		bank->gpio_regs = &gpio_regs_v1;
> 

[PATCH 2/2] gpio: rockchip: get pinctrl node from 'gpio-ranges' property

[Jianqun Xu]

The dt nodes for rockchip soc designes as

	pinctrl: pinctrl {
		gpio {
			gpio-ranges = <&pinctrl xxx>;
		};
	};

Currently, we get the pinctrl dt node from parent of gpio, this patch
try to get pinctrl dt node from 'gpio-ranges' property.

After this patch, the dt nodes possible to be

	gpio {
		gpio-ranges = <&pinctrl xxx>;
	};

	pinctrl: pinctrl {

	};

then the gpio driver could register as platform device itself, but not
populate from pinctrl driver.

Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>
---
 drivers/gpio/gpio-rockchip.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index 1da0324445cc..46c54dff92db 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -690,6 +690,9 @@ rockchip_gpio_find_bank(struct pinctrl_dev *pctldev, int id)
 	int i, found = 0;
 
 	info = pinctrl_dev_get_drvdata(pctldev);
+	if (!info)
+		return NULL;
+
 	bank = info->ctrl->pin_banks;
 	for (i = 0; i < info->ctrl->nr_banks; i++, bank++) {
 		if (bank->bank_num == id) {
@@ -705,15 +708,16 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
-	struct device_node *pctlnp = of_get_parent(np);
+	struct device_node *pctlnp = NULL;
 	struct pinctrl_dev *pctldev = NULL;
 	struct rockchip_pin_bank *bank = NULL;
 	struct rockchip_pin_output_deferred *cfg;
 	static int gpio;
 	int id, ret;
 
-	if (!np || !pctlnp)
-		return -ENODEV;
+	pctlnp = of_parse_phandle(np, "gpio-ranges", 0);
+	if (!pctlnp)
+		pctlnp = of_get_parent(np);
 
 	pctldev = of_pinctrl_get(pctlnp);
 	if (!pctldev)
-- 
2.25.1

Re: [PATCH 2/2] gpio: rockchip: get pinctrl node from 'gpio-ranges' property

[Heiko Stübner]

Hi Jianqun,

Am Donnerstag, 3. März 2022, 07:22:11 CET schrieb Jianqun Xu:
> The dt nodes for rockchip soc designes as
> 
> 	pinctrl: pinctrl {
> 		gpio {
> 			gpio-ranges = <&pinctrl xxx>;
> 		};
> 	};
> 
> Currently, we get the pinctrl dt node from parent of gpio, this patch
> try to get pinctrl dt node from 'gpio-ranges' property.
> 
> After this patch, the dt nodes possible to be
> 
> 	gpio {
> 		gpio-ranges = <&pinctrl xxx>;
> 	};
> 
> 	pinctrl: pinctrl {
> 
> 	};
> 
> then the gpio driver could register as platform device itself, but not
> populate from pinctrl driver.

The change looks interesting, as it would solve that long-standing
design-issue I "created" back in 2013 ;-) .

Though you need to keep some things in mind:

(1) Such a change should be reflected in the devicetree binding
    as it involves a different form of nodes and introduces.

    Looking at the binding description, using gpio-ranges to map
    to specific pinctrl pins really seems to be a valid use for this.


(2) Keep things backwards compatible.
    Old devicetrees should stay working with new kernel versions

    A common pattern is to try the new approach and if that fails
    try the "deprecated" method, which should be nicely doable
    when looking at the code change below.


Heiko

> Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>
> ---
>  drivers/gpio/gpio-rockchip.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index 1da0324445cc..46c54dff92db 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c
> @@ -690,6 +690,9 @@ rockchip_gpio_find_bank(struct pinctrl_dev *pctldev, int id)
>  	int i, found = 0;
>  
>  	info = pinctrl_dev_get_drvdata(pctldev);
> +	if (!info)
> +		return NULL;
> +
>  	bank = info->ctrl->pin_banks;
>  	for (i = 0; i < info->ctrl->nr_banks; i++, bank++) {
>  		if (bank->bank_num == id) {
> @@ -705,15 +708,16 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
> -	struct device_node *pctlnp = of_get_parent(np);
> +	struct device_node *pctlnp = NULL;
>  	struct pinctrl_dev *pctldev = NULL;
>  	struct rockchip_pin_bank *bank = NULL;
>  	struct rockchip_pin_output_deferred *cfg;
>  	static int gpio;
>  	int id, ret;
>  
> -	if (!np || !pctlnp)
> -		return -ENODEV;
> +	pctlnp = of_parse_phandle(np, "gpio-ranges", 0);
> +	if (!pctlnp)
> +		pctlnp = of_get_parent(np);
>  
>  	pctldev = of_pinctrl_get(pctlnp);
>  	if (!pctldev)
> 

Re: [PATCH 2/2] gpio: rockchip: get pinctrl node from 'gpio-ranges' property

[Linus Walleij]

On Thu, Mar 3, 2022 at 7:22 AM Jianqun Xu <jay.xu@rock-chips.com> wrote:

> The dt nodes for rockchip soc designes as
>
>         pinctrl: pinctrl {
>                 gpio {
>                         gpio-ranges = <&pinctrl xxx>;
>                 };
>         };
>
> Currently, we get the pinctrl dt node from parent of gpio, this patch
> try to get pinctrl dt node from 'gpio-ranges' property.
>
> After this patch, the dt nodes possible to be
>
>         gpio {
>                 gpio-ranges = <&pinctrl xxx>;
>         };
>
>         pinctrl: pinctrl {
>
>         };
>
> then the gpio driver could register as platform device itself, but not
> populate from pinctrl driver.
>
> Signed-off-by: Jianqun Xu <jay.xu@rock-chips.com>
(...)
> -       if (!np || !pctlnp)
> -               return -ENODEV;
> +       pctlnp = of_parse_phandle(np, "gpio-ranges", 0);
> +       if (!pctlnp)
> +               pctlnp = of_get_parent(np);

GPIO ranges can be several and point to several different pin controllers.

I understand it does not in your case, so I guess it is fine if Heiko
is fine with it.

Yours,
Linus Walleij