Re: [PATCH 2/2] pinctrl: samsung: support a bus clock

From: Krzysztof Kozlowski <krzk@kernel.org>
To: "André Draszik" <andre.draszik@linaro.org>,
	"Sylwester Nawrocki" <s.nawrocki@samsung.com>,
	"Alim Akhtar" <alim.akhtar@samsung.com>,
	"Linus Walleij" <linus.walleij@linaro.org>,
	"Rob Herring" <robh@kernel.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Tomasz Figa" <tomasz.figa@gmail.com>,
	"Peter Griffin" <peter.griffin@linaro.org>
Cc: Tudor Ambarus <tudor.ambarus@linaro.org>,
	Will McVicker <willmcvicker@google.com>,
	kernel-team@android.com, linux-arm-kernel@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org, linux-gpio@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] pinctrl: samsung: support a bus clock
Date: Thu, 25 Apr 2024 20:29:17 +0200	[thread overview]
Message-ID: <e7d9993e-b51d-4348-aa1d-de0671062c57@kernel.org> (raw)
In-Reply-To: <20240425-samsung-pinctrl-busclock-v1-2-898a200abe68@linaro.org>

On 25/04/2024 18:03, André Draszik wrote:
> On some Samsung-based SoCs there are separate bus clocks / gates each
> for each pinctrl instance. To be able to access each pinctrl instance's
> registers, this bus clock needs to be running, otherwise register
> access will hang. Google Tensor gs101 is one example for such an
> implementation.
> 
> Update the driver to handle this optional bus clock:
> * handle an optional bus clock from DT
> * prepare it during driver probe
> * enclose all relevant register accesses with a clock enable & disable
> 

...

>  	drvdata = pinctrl_dev_get_drvdata(pctldev);
>  	pin_to_reg_bank(drvdata, pin, &reg_base, &pin_offset, &bank);
> @@ -447,6 +456,12 @@ static int samsung_pinconf_rw(struct pinctrl_dev *pctldev, unsigned int pin,
>  	width = type->fld_width[cfg_type];
>  	cfg_reg = type->reg_offset[cfg_type];
>  
> +	ret = clk_enable(drvdata->pclk);
> +	if (ret) {
> +		dev_err(drvdata->dev, "failed to enable clock\n");
> +		return ret;
> +	}
> +
>  	raw_spin_lock_irqsave(&bank->slock, flags);
>  
>  	mask = (1 << width) - 1;
> @@ -466,6 +481,8 @@ static int samsung_pinconf_rw(struct pinctrl_dev *pctldev, unsigned int pin,
>  
>  	raw_spin_unlock_irqrestore(&bank->slock, flags);
>  
> +	clk_disable(drvdata->pclk);
> +
>  	return 0;
>  }
>  
> @@ -539,16 +556,24 @@ static void samsung_gpio_set_value(struct gpio_chip *gc,
>  {
>  	struct samsung_pin_bank *bank = gpiochip_get_data(gc);
>  	const struct samsung_pin_bank_type *type = bank->type;
> +	struct samsung_pinctrl_drv_data *drvdata = bank->drvdata;
>  	void __iomem *reg;
>  	u32 data;
>  
>  	reg = bank->pctl_base + bank->pctl_offset;
>  
> +	if (clk_enable(drvdata->pclk)) {

This is now called with bank->slock held, so you reversed the locking
thus creating possibility of ABBA deadlock.

Need to be moved to callers of samsung_gpio_set_value().

> +		dev_err(drvdata->dev, "failed to enable clock\n");
> +		return;
> +	}
> +
>  	data = readl(reg + type->reg_offset[PINCFG_TYPE_DAT]);
>  	data &= ~(1 << offset);
>  	if (value)
>  		data |= 1 << offset;
>  	writel(data, reg + type->reg_offset[PINCFG_TYPE_DAT]);
> +
> +	clk_disable(drvdata->pclk);
>  }
>  
>  /* gpiolib gpio_set callback function */
> @@ -569,12 +594,23 @@ static int samsung_gpio_get(struct gpio_chip *gc, unsigned offset)
>  	u32 data;
>  	struct samsung_pin_bank *bank = gpiochip_get_data(gc);
>  	const struct samsung_pin_bank_type *type = bank->type;
> +	struct samsung_pinctrl_drv_data *drvdata = bank->drvdata;
> +	int ret;
>  
>  	reg = bank->pctl_base + bank->pctl_offset;
>  
> +	ret = clk_enable(drvdata->pclk);
> +	if (ret) {
> +		dev_err(drvdata->dev, "failed to enable clock\n");
> +		return ret;
> +	}
> +
>  	data = readl(reg + type->reg_offset[PINCFG_TYPE_DAT]);
>  	data >>= offset;
>  	data &= 1;
> +
> +	clk_disable(drvdata->pclk);
> +
>  	return data;
>  }
>  
> @@ -591,9 +627,12 @@ static int samsung_gpio_set_direction(struct gpio_chip *gc,
>  	struct samsung_pin_bank *bank;
>  	void __iomem *reg;
>  	u32 data, mask, shift;
> +	struct samsung_pinctrl_drv_data *drvdata;
> +	int ret;
>  
>  	bank = gpiochip_get_data(gc);
>  	type = bank->type;
> +	drvdata = bank->drvdata;
>  
>  	reg = bank->pctl_base + bank->pctl_offset
>  			+ type->reg_offset[PINCFG_TYPE_FUNC];
> @@ -606,12 +645,20 @@ static int samsung_gpio_set_direction(struct gpio_chip *gc,
>  		reg += 4;
>  	}
>  
> +	ret = clk_enable(drvdata->pclk);

Same problem.

> +	if (ret) {
> +		dev_err(drvdata->dev, "failed to enable clock\n");
> +		return ret;
> +	}
> +
>  	data = readl(reg);
>  	data &= ~(mask << shift);
>  	if (!input)
>  		data |= PIN_CON_FUNC_OUTPUT << shift;
>  	writel(data, reg);
>  
> +	clk_disable(drvdata->pclk);
> +
>  	return 0;
>  }
>  
> @@ -1164,6 +1211,12 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	drvdata->pclk = devm_clk_get_optional_prepared(dev, "pclk");
> +	if (IS_ERR(drvdata->pclk)) {
> +		ret = PTR_ERR(drvdata->pclk);
> +		goto err_put_banks;
> +	}
> +
>  	ret = samsung_pinctrl_register(pdev, drvdata);
>  	if (ret)
>  		goto err_put_banks;
> @@ -1202,6 +1255,13 @@ static int __maybe_unused samsung_pinctrl_suspend(struct device *dev)
>  	struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev);
>  	int i;
>  
> +	i = clk_enable(drvdata->pclk);
> +	if (i) {
> +		dev_err(drvdata->dev,
> +			"failed to enable clock for saving state\n");
> +		return i;
> +	}
> +
>  	for (i = 0; i < drvdata->nr_banks; i++) {
>  		struct samsung_pin_bank *bank = &drvdata->pin_banks[i];
>  		const void __iomem *reg = bank->pctl_base + bank->pctl_offset;
> @@ -1231,6 +1291,8 @@ static int __maybe_unused samsung_pinctrl_suspend(struct device *dev)
>  		}
>  	}
>  
> +	clk_disable(drvdata->pclk);
> +
>  	if (drvdata->suspend)
>  		drvdata->suspend(drvdata);
>  	if (drvdata->retention_ctrl && drvdata->retention_ctrl->enable)
> @@ -1252,6 +1314,16 @@ static int __maybe_unused samsung_pinctrl_resume(struct device *dev)
>  	struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev);
>  	int i;
>  
> +	/* enable clock before the callback, as we don't want to have to deal

That's not netdev, so:

/*
 *

> +	 * with callback cleanup on clock failures.
> +	 */
> +	i = clk_enable(drvdata->pclk);

"i" is iterator, not return value. You want ret.

> +	if (i) {
> +		dev_err(drvdata->dev,
> +			"failed to enable clock for restoring state\n");
> +		return i;
> +	}
> +
>  	if (drvdata->resume)
>  		drvdata->resume(drvdata);
>  
> @@ -1286,6 +1358,8 @@ static int __maybe_unused samsung_pinctrl_resume(struct device *dev)
>  				writel(bank->pm_save[type], reg + offs[type]);
>  	}
>  
> +	clk_disable(drvdata->pclk);
> +
>  	if (drvdata->retention_ctrl && drvdata->retention_ctrl->disable)
>  		drvdata->retention_ctrl->disable(drvdata);
>  

Best regards,
Krzysztof