This series enables clock support on the Samsung Exynos pin controller
driver.
This is required on Socs like Google Tensor gs101, which implement
fine-grained clock control / gating, and as such a running bus clock is
required for register access to work.
Signed-off-by: André Draszik <[email protected]>
---
Changes in v3:
- fix binding for non-gs101 platforms (Krzysztof), sorry I missed that
initially :-(
- Link to v2: https://lore.kernel.org/r/[email protected]
Changes in v2:
- propagate clk_enable() errors in samsung_pinmux_setup(), i.e.
struct pinmux_ops::set_mux()
- move clk_enable()/disable() outside bank->slock lock, to avoid
possible deadlocks due to locking inversion (Krzysztof)
- fix some comments (Krzysztof)
- use 'ret' instead of 'i' in samsung_pinctrl_resume() (Krzysztof)
- Link to v1: https://lore.kernel.org/r/[email protected]
---
André Draszik (2):
dt-bindings: pinctrl: samsung: google,gs101-pinctrl needs a clock
pinctrl: samsung: support a bus clock
.../bindings/pinctrl/samsung,pinctrl.yaml | 21 ++++
drivers/pinctrl/samsung/pinctrl-exynos.c | 112 +++++++++++++++++++++
drivers/pinctrl/samsung/pinctrl-samsung.c | 95 ++++++++++++++++-
drivers/pinctrl/samsung/pinctrl-samsung.h | 2 +
4 files changed, 227 insertions(+), 3 deletions(-)
---
base-commit: a59668a9397e7245b26e9be85d23f242ff757ae8
change-id: 20240425-samsung-pinctrl-busclock-151c23d76860
Best regards,
--
André Draszik <[email protected]>
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
Signed-off-by: André Draszik <[email protected]>
---
v2:
- propagate clk_enable() errors in samsung_pinmux_setup(), i.e.
struct pinmux_ops::set_mux()
- move clk_enable()/disable() outside bank->slock lock, to avoid
possible deadlocks due to locking inversion
- fix some comments
- use 'ret' instead of 'i' in samsung_pinctrl_resume()
---
drivers/pinctrl/samsung/pinctrl-exynos.c | 112 ++++++++++++++++++++++++++++++
drivers/pinctrl/samsung/pinctrl-samsung.c | 95 ++++++++++++++++++++++++-
drivers/pinctrl/samsung/pinctrl-samsung.h | 2 +
3 files changed, 206 insertions(+), 3 deletions(-)
diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
index 871c1eb46ddf..ce5e6783b5b9 100644
--- a/drivers/pinctrl/samsung/pinctrl-exynos.c
+++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
@@ -13,6 +13,7 @@
// the Samsung pinctrl/gpiolib driver. It also includes the implementation of
// external gpio and wakeup interrupt support.
+#include <linux/clk.h>
#include <linux/device.h>
#include <linux/interrupt.h>
#include <linux/irqdomain.h>
@@ -61,6 +62,12 @@ static void exynos_irq_mask(struct irq_data *irqd)
else
reg_mask = our_chip->eint_mask + bank->eint_offset;
+ if (clk_enable(bank->drvdata->pclk)) {
+ dev_err(bank->gpio_chip.parent,
+ "unable to enable clock for masking IRQ\n");
+ return;
+ }
+
raw_spin_lock_irqsave(&bank->slock, flags);
mask = readl(bank->eint_base + reg_mask);
@@ -68,6 +75,8 @@ static void exynos_irq_mask(struct irq_data *irqd)
writel(mask, bank->eint_base + reg_mask);
raw_spin_unlock_irqrestore(&bank->slock, flags);
+
+ clk_disable(bank->drvdata->pclk);
}
static void exynos_irq_ack(struct irq_data *irqd)
@@ -82,7 +91,15 @@ static void exynos_irq_ack(struct irq_data *irqd)
else
reg_pend = our_chip->eint_pend + bank->eint_offset;
+ if (clk_enable(bank->drvdata->pclk)) {
+ dev_err(bank->gpio_chip.parent,
+ "unable to enable clock to ack IRQ\n");
+ return;
+ }
+
writel(1 << irqd->hwirq, bank->eint_base + reg_pend);
+
+ clk_disable(bank->drvdata->pclk);
}
static void exynos_irq_unmask(struct irq_data *irqd)
@@ -110,6 +127,12 @@ static void exynos_irq_unmask(struct irq_data *irqd)
else
reg_mask = our_chip->eint_mask + bank->eint_offset;
+ if (clk_enable(bank->drvdata->pclk)) {
+ dev_err(bank->gpio_chip.parent,
+ "unable to enable clock for unmasking IRQ\n");
+ return;
+ }
+
raw_spin_lock_irqsave(&bank->slock, flags);
mask = readl(bank->eint_base + reg_mask);
@@ -117,6 +140,8 @@ static void exynos_irq_unmask(struct irq_data *irqd)
writel(mask, bank->eint_base + reg_mask);
raw_spin_unlock_irqrestore(&bank->slock, flags);
+
+ clk_disable(bank->drvdata->pclk);
}
static int exynos_irq_set_type(struct irq_data *irqd, unsigned int type)
@@ -127,6 +152,7 @@ static int exynos_irq_set_type(struct irq_data *irqd, unsigned int type)
unsigned int shift = EXYNOS_EINT_CON_LEN * irqd->hwirq;
unsigned int con, trig_type;
unsigned long reg_con;
+ int ret;
switch (type) {
case IRQ_TYPE_EDGE_RISING:
@@ -159,11 +185,20 @@ static int exynos_irq_set_type(struct irq_data *irqd, unsigned int type)
else
reg_con = our_chip->eint_con + bank->eint_offset;
+ ret = clk_enable(bank->drvdata->pclk);
+ if (ret) {
+ dev_err(bank->gpio_chip.parent,
+ "unable to enable clock for configuring IRQ type\n");
+ return ret;
+ }
+
con = readl(bank->eint_base + reg_con);
con &= ~(EXYNOS_EINT_CON_MASK << shift);
con |= trig_type << shift;
writel(con, bank->eint_base + reg_con);
+ clk_disable(bank->drvdata->pclk);
+
return 0;
}
@@ -200,6 +235,14 @@ static int exynos_irq_request_resources(struct irq_data *irqd)
shift = irqd->hwirq * bank_type->fld_width[PINCFG_TYPE_FUNC];
mask = (1 << bank_type->fld_width[PINCFG_TYPE_FUNC]) - 1;
+ ret = clk_enable(bank->drvdata->pclk);
+ if (ret) {
+ dev_err(bank->gpio_chip.parent,
+ "unable to enable clock for configuring pin %s-%lu\n",
+ bank->name, irqd->hwirq);
+ return ret;
+ }
+
raw_spin_lock_irqsave(&bank->slock, flags);
con = readl(bank->pctl_base + reg_con);
@@ -209,6 +252,8 @@ static int exynos_irq_request_resources(struct irq_data *irqd)
raw_spin_unlock_irqrestore(&bank->slock, flags);
+ clk_disable(bank->drvdata->pclk);
+
return 0;
}
@@ -223,6 +268,13 @@ static void exynos_irq_release_resources(struct irq_data *irqd)
shift = irqd->hwirq * bank_type->fld_width[PINCFG_TYPE_FUNC];
mask = (1 << bank_type->fld_width[PINCFG_TYPE_FUNC]) - 1;
+ if (clk_enable(bank->drvdata->pclk)) {
+ dev_err(bank->gpio_chip.parent,
+ "unable to enable clock for deconfiguring pin %s-%lu\n",
+ bank->name, irqd->hwirq);
+ return;
+ }
+
raw_spin_lock_irqsave(&bank->slock, flags);
con = readl(bank->pctl_base + reg_con);
@@ -232,6 +284,8 @@ static void exynos_irq_release_resources(struct irq_data *irqd)
raw_spin_unlock_irqrestore(&bank->slock, flags);
+ clk_disable(bank->drvdata->pclk);
+
gpiochip_unlock_as_irq(&bank->gpio_chip, irqd->hwirq);
}
@@ -281,10 +335,19 @@ static irqreturn_t exynos_eint_gpio_irq(int irq, void *data)
unsigned int svc, group, pin;
int ret;
+ if (clk_enable(bank->drvdata->pclk)) {
+ dev_err(bank->gpio_chip.parent,
+ "unable to enable clock for handling IRQ\n");
+ return IRQ_NONE;
+ }
+
if (bank->eint_con_offset)
svc = readl(bank->eint_base + EXYNOSAUTO_SVC_OFFSET);
else
svc = readl(bank->eint_base + EXYNOS_SVC_OFFSET);
+
+ clk_disable(bank->drvdata->pclk);
+
group = EXYNOS_SVC_GROUP(svc);
pin = svc & EXYNOS_SVC_NUM_MASK;
@@ -563,6 +626,20 @@ static void exynos_irq_demux_eint16_31(struct irq_desc *desc)
chained_irq_enter(chip, desc);
+ /*
+ * just enable the clock once here, to avoid an enable/disable dance for
+ * each bank.
+ */
+ if (eintd->nr_banks) {
+ struct samsung_pin_bank *b = eintd->banks[0];
+
+ if (clk_enable(b->drvdata->pclk)) {
+ dev_err(b->gpio_chip.parent,
+ "unable to enable clock for pending IRQs\n");
+ return;
+ }
+ }
+
for (i = 0; i < eintd->nr_banks; ++i) {
struct samsung_pin_bank *b = eintd->banks[i];
pend = readl(b->eint_base + b->irq_chip->eint_pend
@@ -572,6 +649,9 @@ static void exynos_irq_demux_eint16_31(struct irq_desc *desc)
exynos_irq_demux_eint(pend & ~mask, b->irq_domain);
}
+ if (eintd->nr_banks)
+ clk_disable(eintd->banks[0]->drvdata->pclk);
+
chained_irq_exit(chip, desc);
}
@@ -695,6 +775,12 @@ static void exynos_pinctrl_suspend_bank(
struct exynos_eint_gpio_save *save = bank->soc_priv;
const void __iomem *regs = bank->eint_base;
+ if (clk_enable(bank->drvdata->pclk)) {
+ dev_err(bank->gpio_chip.parent,
+ "unable to enable clock for saving state\n");
+ return;
+ }
+
save->eint_con = readl(regs + EXYNOS_GPIO_ECON_OFFSET
+ bank->eint_offset);
save->eint_fltcon0 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
@@ -704,6 +790,8 @@ static void exynos_pinctrl_suspend_bank(
save->eint_mask = readl(regs + bank->irq_chip->eint_mask
+ bank->eint_offset);
+ clk_disable(bank->drvdata->pclk);
+
pr_debug("%s: save con %#010x\n", bank->name, save->eint_con);
pr_debug("%s: save fltcon0 %#010x\n", bank->name, save->eint_fltcon0);
pr_debug("%s: save fltcon1 %#010x\n", bank->name, save->eint_fltcon1);
@@ -716,9 +804,17 @@ static void exynosauto_pinctrl_suspend_bank(struct samsung_pinctrl_drv_data *drv
struct exynos_eint_gpio_save *save = bank->soc_priv;
const void __iomem *regs = bank->eint_base;
+ if (clk_enable(bank->drvdata->pclk)) {
+ dev_err(bank->gpio_chip.parent,
+ "unable to enable clock for saving state\n");
+ return;
+ }
+
save->eint_con = readl(regs + bank->pctl_offset + bank->eint_con_offset);
save->eint_mask = readl(regs + bank->pctl_offset + bank->eint_mask_offset);
+ clk_disable(bank->drvdata->pclk);
+
pr_debug("%s: save con %#010x\n", bank->name, save->eint_con);
pr_debug("%s: save mask %#010x\n", bank->name, save->eint_mask);
}
@@ -753,6 +849,12 @@ static void exynos_pinctrl_resume_bank(
struct exynos_eint_gpio_save *save = bank->soc_priv;
void __iomem *regs = bank->eint_base;
+ if (clk_enable(bank->drvdata->pclk)) {
+ dev_err(bank->gpio_chip.parent,
+ "unable to enable clock for restoring state\n");
+ return;
+ }
+
pr_debug("%s: con %#010x => %#010x\n", bank->name,
readl(regs + EXYNOS_GPIO_ECON_OFFSET
+ bank->eint_offset), save->eint_con);
@@ -774,6 +876,8 @@ static void exynos_pinctrl_resume_bank(
+ 2 * bank->eint_offset + 4);
writel(save->eint_mask, regs + bank->irq_chip->eint_mask
+ bank->eint_offset);
+
+ clk_disable(bank->drvdata->pclk);
}
static void exynosauto_pinctrl_resume_bank(struct samsung_pinctrl_drv_data *drvdata,
@@ -782,6 +886,12 @@ static void exynosauto_pinctrl_resume_bank(struct samsung_pinctrl_drv_data *drvd
struct exynos_eint_gpio_save *save = bank->soc_priv;
void __iomem *regs = bank->eint_base;
+ if (clk_enable(bank->drvdata->pclk)) {
+ dev_err(bank->gpio_chip.parent,
+ "unable to enable clock for restoring state\n");
+ return;
+ }
+
pr_debug("%s: con %#010x => %#010x\n", bank->name,
readl(regs + bank->pctl_offset + bank->eint_con_offset), save->eint_con);
pr_debug("%s: mask %#010x => %#010x\n", bank->name,
@@ -789,6 +899,8 @@ static void exynosauto_pinctrl_resume_bank(struct samsung_pinctrl_drv_data *drvd
writel(save->eint_con, regs + bank->pctl_offset + bank->eint_con_offset);
writel(save->eint_mask, regs + bank->pctl_offset + bank->eint_mask_offset);
+
+ clk_disable(bank->drvdata->pclk);
}
void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
index ed07e23e0912..acde42983934 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.c
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
@@ -15,6 +15,7 @@
// but provides extensions to which platform specific implementation of the gpio
// and wakeup interrupts can be hooked to.
+#include <linux/clk.h>
#include <linux/err.h>
#include <linux/gpio/driver.h>
#include <linux/init.h>
@@ -371,7 +372,7 @@ static void pin_to_reg_bank(struct samsung_pinctrl_drv_data *drvdata,
}
/* enable or disable a pinmux function */
-static void samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,
+static int samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,
unsigned group)
{
struct samsung_pinctrl_drv_data *drvdata;
@@ -382,6 +383,7 @@ static void samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,
unsigned long flags;
const struct samsung_pmx_func *func;
const struct samsung_pin_group *grp;
+ int ret;
drvdata = pinctrl_dev_get_drvdata(pctldev);
func = &drvdata->pmx_functions[selector];
@@ -397,6 +399,12 @@ static void samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,
reg += 4;
}
+ ret = clk_enable(drvdata->pclk);
+ if (ret) {
+ dev_err(pctldev->dev, "failed to enable clock for setup\n");
+ return ret;
+ }
+
raw_spin_lock_irqsave(&bank->slock, flags);
data = readl(reg + type->reg_offset[PINCFG_TYPE_FUNC]);
@@ -405,6 +413,10 @@ static void samsung_pinmux_setup(struct pinctrl_dev *pctldev, unsigned selector,
writel(data, reg + type->reg_offset[PINCFG_TYPE_FUNC]);
raw_spin_unlock_irqrestore(&bank->slock, flags);
+
+ clk_disable(drvdata->pclk);
+
+ return 0;
}
/* enable a specified pinmux by writing to registers */
@@ -412,8 +424,7 @@ static int samsung_pinmux_set_mux(struct pinctrl_dev *pctldev,
unsigned selector,
unsigned group)
{
- samsung_pinmux_setup(pctldev, selector, group);
- return 0;
+ return samsung_pinmux_setup(pctldev, selector, group);
}
/* list of pinmux callbacks for the pinmux vertical in pinctrl core */
@@ -436,6 +447,7 @@ static int samsung_pinconf_rw(struct pinctrl_dev *pctldev, unsigned int pin,
u32 data, width, pin_offset, mask, shift;
u32 cfg_value, cfg_reg;
unsigned long flags;
+ int ret;
drvdata = pinctrl_dev_get_drvdata(pctldev);
pin_to_reg_bank(drvdata, pin, ®_base, &pin_offset, &bank);
@@ -447,6 +459,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 +484,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;
}
@@ -555,11 +575,19 @@ static void samsung_gpio_set_value(struct gpio_chip *gc,
static void samsung_gpio_set(struct gpio_chip *gc, unsigned offset, int value)
{
struct samsung_pin_bank *bank = gpiochip_get_data(gc);
+ struct samsung_pinctrl_drv_data *drvdata = bank->drvdata;
unsigned long flags;
+ if (clk_enable(drvdata->pclk)) {
+ dev_err(drvdata->dev, "failed to enable clock\n");
+ return;
+ }
+
raw_spin_lock_irqsave(&bank->slock, flags);
samsung_gpio_set_value(gc, offset, value);
raw_spin_unlock_irqrestore(&bank->slock, flags);
+
+ clk_disable(drvdata->pclk);
}
/* gpiolib gpio_get callback function */
@@ -569,12 +597,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 +630,11 @@ 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;
bank = gpiochip_get_data(gc);
type = bank->type;
+ drvdata = bank->drvdata;
reg = bank->pctl_base + bank->pctl_offset
+ type->reg_offset[PINCFG_TYPE_FUNC];
@@ -619,12 +660,22 @@ static int samsung_gpio_set_direction(struct gpio_chip *gc,
static int samsung_gpio_direction_input(struct gpio_chip *gc, unsigned offset)
{
struct samsung_pin_bank *bank = gpiochip_get_data(gc);
+ struct samsung_pinctrl_drv_data *drvdata = bank->drvdata;
unsigned long flags;
int ret;
+ 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);
ret = samsung_gpio_set_direction(gc, offset, true);
raw_spin_unlock_irqrestore(&bank->slock, flags);
+
+ clk_disable(drvdata->pclk);
+
return ret;
}
@@ -633,14 +684,23 @@ static int samsung_gpio_direction_output(struct gpio_chip *gc, unsigned offset,
int value)
{
struct samsung_pin_bank *bank = gpiochip_get_data(gc);
+ struct samsung_pinctrl_drv_data *drvdata = bank->drvdata;
unsigned long flags;
int ret;
+ 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);
samsung_gpio_set_value(gc, offset, value);
ret = samsung_gpio_set_direction(gc, offset, false);
raw_spin_unlock_irqrestore(&bank->slock, flags);
+ clk_disable(drvdata->pclk);
+
return ret;
}
@@ -1164,6 +1224,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 +1268,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 +1304,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)
@@ -1250,8 +1325,20 @@ static int __maybe_unused samsung_pinctrl_suspend(struct device *dev)
static int __maybe_unused samsung_pinctrl_resume(struct device *dev)
{
struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev);
+ int ret;
int i;
+ /*
+ * enable clock before the callback, as we don't want to have to deal
+ * with callback cleanup on clock failures.
+ */
+ ret = clk_enable(drvdata->pclk);
+ if (ret) {
+ dev_err(drvdata->dev,
+ "failed to enable clock for restoring state\n");
+ return ret;
+ }
+
if (drvdata->resume)
drvdata->resume(drvdata);
@@ -1286,6 +1373,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);
diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
index ab791afaabf5..d50ba6f07d5d 100644
--- a/drivers/pinctrl/samsung/pinctrl-samsung.h
+++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
@@ -274,6 +274,7 @@ struct samsung_pin_ctrl {
* through samsung_pinctrl_drv_data, not samsung_pin_bank).
* @dev: device instance representing the controller.
* @irq: interrpt number used by the controller to notify gpio interrupts.
+ * @pclk: optional bus clock if required for accessing registers
* @ctrl: pin controller instance managed by the driver.
* @pctl: pin controller descriptor registered with the pinctrl subsystem.
* @pctl_dev: cookie representing pinctrl device instance.
@@ -293,6 +294,7 @@ struct samsung_pinctrl_drv_data {
void __iomem *virt_base;
struct device *dev;
int irq;
+ struct clk *pclk;
struct pinctrl_desc pctl;
struct pinctrl_dev *pctl_dev;
--
2.44.0.769.g3c40516874-goog
On Fri, 26 Apr 2024 14:25:13 +0100, André Draszik wrote:
> This series enables clock support on the Samsung Exynos pin controller
> driver.
>
> This is required on Socs like Google Tensor gs101, which implement
> fine-grained clock control / gating, and as such a running bus clock is
> required for register access to work.
>
> [...]
Applied, thanks!
[1/2] dt-bindings: pinctrl: samsung: google,gs101-pinctrl needs a clock
https://git.kernel.org/pinctrl/samsung/c/dff9f3fb6ba4f74eb805bc172cc16ff2c91648bf
[2/2] pinctrl: samsung: support a bus clock
https://git.kernel.org/pinctrl/samsung/c/f9c74474797351c60e009ebc59a798fcfd93ee57
Best regards,
--
Krzysztof Kozlowski <[email protected]>
On 29/04/2024 19:28, Krzysztof Kozlowski wrote:
>
> On Fri, 26 Apr 2024 14:25:13 +0100, André Draszik wrote:
>> This series enables clock support on the Samsung Exynos pin controller
>> driver.
>>
>> This is required on Socs like Google Tensor gs101, which implement
>> fine-grained clock control / gating, and as such a running bus clock is
>> required for register access to work.
>>
Where's the DTS?
Best regards,
Krzysztof
Hi Krzysztof,
On Mon, 2024-04-29 at 19:45 +0200, Krzysztof Kozlowski wrote:
> On 29/04/2024 19:28, Krzysztof Kozlowski wrote:
> >
> > On Fri, 26 Apr 2024 14:25:13 +0100, André Draszik wrote:
> > > This series enables clock support on the Samsung Exynos pin controller
> > > driver.
> > >
> > > This is required on Socs like Google Tensor gs101, which implement
> > > fine-grained clock control / gating, and as such a running bus clock is
> > > required for register access to work.
> > >
>
> Where's the DTS?
Here: https://lore.kernel.org/r/20240429-samsung-pinctrl-busclock-dts-v1-0-5e935179f3ca@linaro.org
(I was waiting to see how the HSI2 patches pan out)
Thanks,
Andre
On Mon, 2024-04-29 at 21:06 +0100, André Draszik wrote:
> Hi Krzysztof,
>
> On Mon, 2024-04-29 at 19:45 +0200, Krzysztof Kozlowski wrote:
> > On 29/04/2024 19:28, Krzysztof Kozlowski wrote:
> > >
> > > On Fri, 26 Apr 2024 14:25:13 +0100, André Draszik wrote:
> > > > This series enables clock support on the Samsung Exynos pin controller
> > > > driver.
> > > >
> > > > This is required on Socs like Google Tensor gs101, which implement
> > > > fine-grained clock control / gating, and as such a running bus clock is
> > > > required for register access to work.
> > > >
> >
> > Where's the DTS?
>
> Here: https://lore.kernel.org/r/20240429-samsung-pinctrl-busclock-dts-v1-0-5e935179f3ca@linaro.org
>
> (I was waiting to see how the HSI2 patches pan out)
.. and potential binding feedback of course :-)
Hi, André!
On 4/26/24 14:25, André Draszik wrote:
> @@ -200,6 +235,14 @@ static int exynos_irq_request_resources(struct irq_data *irqd)
> shift = irqd->hwirq * bank_type->fld_width[PINCFG_TYPE_FUNC];
> mask = (1 << bank_type->fld_width[PINCFG_TYPE_FUNC]) - 1;
>
> + ret = clk_enable(bank->drvdata->pclk);
> + if (ret) {
> + dev_err(bank->gpio_chip.parent,
> + "unable to enable clock for configuring pin %s-%lu\n",
> + bank->name, irqd->hwirq);
> + return ret;
here we return an error
> + }
> +
> raw_spin_lock_irqsave(&bank->slock, flags);
>
> con = readl(bank->pctl_base + reg_con);
> @@ -209,6 +252,8 @@ static int exynos_irq_request_resources(struct irq_data *irqd)
>
> raw_spin_unlock_irqrestore(&bank->slock, flags);
>
> + clk_disable(bank->drvdata->pclk);
> +
> return 0;
> }
>
> @@ -223,6 +268,13 @@ static void exynos_irq_release_resources(struct irq_data *irqd)
> shift = irqd->hwirq * bank_type->fld_width[PINCFG_TYPE_FUNC];
> mask = (1 << bank_type->fld_width[PINCFG_TYPE_FUNC]) - 1;
>
> + if (clk_enable(bank->drvdata->pclk)) {
> + dev_err(bank->gpio_chip.parent,
> + "unable to enable clock for deconfiguring pin %s-%lu\n",
> + bank->name, irqd->hwirq);
> + return;
but here we just print an error. I guess that for consistency reasons it
would be good to follow up with a patch and change the return types of
these methods and return the error too when the clock enable fails.
Cheers,
ta
On 02/05/2024 09:41, Tudor Ambarus wrote:
>>
>> @@ -223,6 +268,13 @@ static void exynos_irq_release_resources(struct irq_data *irqd)
>> shift = irqd->hwirq * bank_type->fld_width[PINCFG_TYPE_FUNC];
>> mask = (1 << bank_type->fld_width[PINCFG_TYPE_FUNC]) - 1;
>>
>> + if (clk_enable(bank->drvdata->pclk)) {
>> + dev_err(bank->gpio_chip.parent,
>> + "unable to enable clock for deconfiguring pin %s-%lu\n",
>> + bank->name, irqd->hwirq);
>> + return;
>
> but here we just print an error. I guess that for consistency reasons it
> would be good to follow up with a patch and change the return types of
> these methods and return the error too when the clock enable fails.
That's a release, so usually void callback. The true issue is that we
expect release to always succeed, I think.
This points to issue with this patchset: looks like some patchwork all
around the places having register accesses. But how do you even expect
interrupts and pins to work if entire pinctrl block is clock gated?
Best regards,
Krzysztof
On Thu, 2024-05-02 at 09:46 +0200, Krzysztof Kozlowski wrote:
> On 02/05/2024 09:41, Tudor Ambarus wrote:
> > >
> > > @@ -223,6 +268,13 @@ static void exynos_irq_release_resources(struct irq_data *irqd)
> > > shift = irqd->hwirq * bank_type->fld_width[PINCFG_TYPE_FUNC];
> > > mask = (1 << bank_type->fld_width[PINCFG_TYPE_FUNC]) - 1;
> > >
> > > + if (clk_enable(bank->drvdata->pclk)) {
> > > + dev_err(bank->gpio_chip.parent,
> > > + "unable to enable clock for deconfiguring pin %s-%lu\n",
> > > + bank->name, irqd->hwirq);
> > > + return;
> >
> > but here we just print an error. I guess that for consistency reasons it
> > would be good to follow up with a patch and change the return types of
> > these methods and return the error too when the clock enable fails.
>
> That's a release, so usually void callback. The true issue is that we
> expect release to always succeed, I think.
>
> This points to issue with this patchset: looks like some patchwork all
> around the places having register accesses. But how do you even expect
> interrupts and pins to work if entire pinctrl block is clock gated?
I was initially thinking the same, but the clock seems to be required for
register access only, interrupts are still being received and triggered
with pclk turned off as per my testing.
Cheers,
Andre'
On 02/05/2024 12:41, André Draszik wrote:
> On Thu, 2024-05-02 at 09:46 +0200, Krzysztof Kozlowski wrote:
>> On 02/05/2024 09:41, Tudor Ambarus wrote:
>>>>
>>>> @@ -223,6 +268,13 @@ static void exynos_irq_release_resources(struct irq_data *irqd)
>>>> shift = irqd->hwirq * bank_type->fld_width[PINCFG_TYPE_FUNC];
>>>> mask = (1 << bank_type->fld_width[PINCFG_TYPE_FUNC]) - 1;
>>>>
>>>> + if (clk_enable(bank->drvdata->pclk)) {
>>>> + dev_err(bank->gpio_chip.parent,
>>>> + "unable to enable clock for deconfiguring pin %s-%lu\n",
>>>> + bank->name, irqd->hwirq);
>>>> + return;
>>>
>>> but here we just print an error. I guess that for consistency reasons it
>>> would be good to follow up with a patch and change the return types of
>>> these methods and return the error too when the clock enable fails.
>>
>> That's a release, so usually void callback. The true issue is that we
>> expect release to always succeed, I think.
>>
>> This points to issue with this patchset: looks like some patchwork all
>> around the places having register accesses. But how do you even expect
>> interrupts and pins to work if entire pinctrl block is clock gated?
>
> I was initially thinking the same, but the clock seems to be required for
> register access only, interrupts are still being received and triggered
> with pclk turned off as per my testing.
Probably we could simplify this all and keep the clock enabled always,
when device is not suspended. Toggling clock on/off for every pin change
is also an overhead. Anyway, I merged the patches for now, because it
addresses real problem and seems like one of reasonable solutions.
Best regards,
Krzysztof
Hi Krzysztof,
On Fri, 2024-05-03 at 11:13 +0200, Krzysztof Kozlowski wrote:
> On 02/05/2024 12:41, André Draszik wrote:
> > I was initially thinking the same, but the clock seems to be required for
> > register access only, interrupts are still being received and triggered
> > with pclk turned off as per my testing.
>
> Probably we could simplify this all and keep the clock enabled always,
> when device is not suspended. Toggling clock on/off for every pin change
> is also an overhead. Anyway, I merged the patches for now, because it
> addresses real problem and seems like one of reasonable solutions.
I had contemplated a global enable of the clock on driver instantiation
as well, but in the end for me the reasons why I chose the fine-grained
approach here instead were:
* Since the clock is only needed for register access, it seems only
natural to enable it during register accesses only. (The same would
happen if we had support for automatic clock gating in Linux).
* If we think about external GPIO interrupts, they are likely to occur
very rarely (think button press by operator on some external keys or
I2C interrupts), it seems a waste to have the clock running all the
time.
* drivers/i2c/busses/i2c-exynos5.c and drivers/soc/samsung/exynos-usi.c
also kinda do it this way. Bus clocks are only enabled when needed
(e.g. during transfer) (granted, the IPs (IP clocks) are also fully
enabled/disabled in those drivers when idle, and there is no such
thing here)
Cheers,
Andre'