[PATCH 1/2] pinctrl: pinctrl-imx: add support for set bits for general purpose registers

[PATCH 1/2] pinctrl: pinctrl-imx: add support for set bits for general purpose registers

[Dong Aisheng]

From: Dong Aisheng <dong.aisheng@linaro.org>

The General Purpose Registers (GPR) is used to select operating modes for
general features in the SoC, usually not related to the IOMUX itself,
but it does belong to IOMUX controller.
We simply provide an convient API for driver to call to set the general purpose
register bits if needed.

Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
---
 drivers/pinctrl/pinctrl-imx.c   |   19 +++++++++++++++++++
 include/linux/fsl/imx-pinctrl.h |   24 ++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/fsl/imx-pinctrl.h

diff --git a/drivers/pinctrl/pinctrl-imx.c b/drivers/pinctrl/pinctrl-imx.c
index 44e9726..b83b902 100644
--- a/drivers/pinctrl/pinctrl-imx.c
+++ b/drivers/pinctrl/pinctrl-imx.c
@@ -54,6 +54,24 @@ struct imx_pinctrl {
 	const struct imx_pinctrl_soc_info *info;
 };
 
+static struct imx_pinctrl *imx_pinctrl;
+/*
+ * Set bits for general purpose registers
+ */
+void imx_pinctrl_set_gpr_register(u8 gpr, u8 start_bit, u8 num_bits, u32 value)
+{
+	u32 reg;
+
+	/* general purpose register is 32 bits size */
+	WARN_ON(!imx_pinctrl || start_bit > 31 || num_bits > 32);
+
+	reg = readl(imx_pinctrl->base + gpr * 4);
+	reg &= ~(((1 << num_bits) - 1) << start_bit);
+	reg |= (value << start_bit);
+	writel(reg, imx_pinctrl->base + gpr * 4);
+}
+EXPORT_SYMBOL_GPL(imx_pinctrl_set_gpr_register);
+
 static const struct imx_pin_reg *imx_find_pin_reg(
 				const struct imx_pinctrl_soc_info *info,
 				unsigned pin, bool is_mux, unsigned mux)
@@ -587,6 +605,7 @@ int __devinit imx_pinctrl_probe(struct platform_device *pdev,
 	if (!ipctl->base)
 		return -EBUSY;
 
+	imx_pinctrl = ipctl;
 	imx_pinctrl_desc.name = dev_name(&pdev->dev);
 	imx_pinctrl_desc.pins = info->pins;
 	imx_pinctrl_desc.npins = info->npins;
diff --git a/include/linux/fsl/imx-pinctrl.h b/include/linux/fsl/imx-pinctrl.h
new file mode 100644
index 0000000..0212948
--- /dev/null
+++ b/include/linux/fsl/imx-pinctrl.h
@@ -0,0 +1,24 @@
+/*
+ * Copyright (C) 2012 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __FSL_IMX_PINCTRL_H__
+#define __FSL_IMX_PINCTRL_H__
+
+#ifdef CONFIG_PINCTRL_IMX
+extern void imx_pinctrl_set_gpr_register(u8 gpr, u8 start_bit,
+					u8 num_bits, u32 value);
+#else
+static inline void imx_pinctrl_set_gpr_register(u8 gpr, u8 start_bit,
+						u8 num_bits, u32 value)
+{
+	WARN(1, "CONFIG_PINCTRL_IMX is not selected, simply return\n");
+	return;
+}
+#endif
+
+#endif /* !__FSL_IMX_PINCTRL_H__ */
-- 
1.7.0.4

[PATCH 2/2] pinctrl: pinctrl-imx6q: add missed mux function for USBOTG_ID

[Dong Aisheng]

From: Dong Aisheng <dong.aisheng@linaro.org>

The original pin registers table is derived from u-boot mainline,
but somehow it was found missing some mux functions for USBOTG_ID.
We added it at the bottom by following the exist pin function ids,
then it will not break the exist using of pin function id in dts file.

Reported-by: Richard Zhao <richard.zhao@freescale.com>
Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
---
 .../bindings/pinctrl/fsl,imx6q-pinctrl.txt         |    2 ++
 drivers/pinctrl/pinctrl-imx6q.c                    |    2 ++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/fsl,imx6q-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/fsl,imx6q-pinctrl.txt
index 82b43f9..a4119f6 100644
--- a/Documentation/devicetree/bindings/pinctrl/fsl,imx6q-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/fsl,imx6q-pinctrl.txt
@@ -1626,3 +1626,5 @@ MX6Q_PAD_SD2_DAT3__PCIE_CTRL_MUX_11		1587
 MX6Q_PAD_SD2_DAT3__GPIO_1_12			1588
 MX6Q_PAD_SD2_DAT3__SJC_DONE			1589
 MX6Q_PAD_SD2_DAT3__ANATOP_TESTO_3		1590
+MX6Q_PAD_ENET_RX_ER__ANATOP_USBOTG_ID		1591
+MX6Q_PAD_GPIO_1__ANATOP_USBOTG_ID		1592
diff --git a/drivers/pinctrl/pinctrl-imx6q.c b/drivers/pinctrl/pinctrl-imx6q.c
index 7737d4d..e9bf71f 100644
--- a/drivers/pinctrl/pinctrl-imx6q.c
+++ b/drivers/pinctrl/pinctrl-imx6q.c
@@ -1950,6 +1950,8 @@ static struct imx_pin_reg imx6q_pin_regs[] = {
 	IMX_PIN_REG(MX6Q_PAD_SD2_DAT3, 0x0744, 0x035C, 5, 0x0000, 0), /* MX6Q_PAD_SD2_DAT3__GPIO_1_12 */
 	IMX_PIN_REG(MX6Q_PAD_SD2_DAT3, 0x0744, 0x035C, 6, 0x0000, 0), /* MX6Q_PAD_SD2_DAT3__SJC_DONE */
 	IMX_PIN_REG(MX6Q_PAD_SD2_DAT3, 0x0744, 0x035C, 7, 0x0000, 0), /* MX6Q_PAD_SD2_DAT3__ANATOP_TESTO_3 */
+	IMX_PIN_REG(MX6Q_PAD_ENET_RX_ER, 0x04EC, 0x01D8, 0, 0x0000, 0), /* MX6Q_PAD_ENET_RX_ER__ANATOP_USBOTG_ID */
+	IMX_PIN_REG(MX6Q_PAD_GPIO_1, 0x05F4, 0x0224, 3, 0x0000, 0), /* MX6Q_PAD_GPIO_1__ANATOP_USBOTG_ID */
 };
 
 /* Pad names for the pinmux subsystem */
-- 
1.7.0.4

[PATCH 1/2] pinctrl: pinctrl-imx: add support for set bits for general purpose registers

[Stephen Warren]

On 07/06/2012 03:09 AM, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> The General Purpose Registers (GPR) is used to select operating modes for
> general features in the SoC, usually not related to the IOMUX itself,
> but it does belong to IOMUX controller.
> We simply provide an convient API for driver to call to set the general purpose
> register bits if needed.

> +static struct imx_pinctrl *imx_pinctrl;
> +/*
> + * Set bits for general purpose registers
> + */
> +void imx_pinctrl_set_gpr_register(u8 gpr, u8 start_bit, u8 num_bits, u32 value)
> +{
> +	u32 reg;
> +
> +	/* general purpose register is 32 bits size */
> +	WARN_ON(!imx_pinctrl || start_bit > 31 || num_bits > 32);

Hmmm. It's going to be very hard to control the probe() order to ensure
that this WARN doesn't fire all the time.

I think it would be better to pass in a struct imx_pinctrl* or DT node
to this function. The DT node for the device that's using this function
should contain a phandle to the pinctrl device node, which it uses to
get that handle. Or in a non-DT case, the client driver needs to be
given the provider driver handle using some other mechanism.

For example, look at how the Tegra30 SMMU uses services from the Tegra30
AHB; see arch/arm/boot/dts/tegra30.dtsi node "smmu" (client) and node
"ahb" (provider), drivers/iommu/tegra-smmu.c functions probe() (saves
smmu->ahb) and smmu_setup_regs() (calls tegra_ahb_enable_smmu() with
this handle) and drivers/amba/tegra-ahb.c function
tegra_ahb_enable_smmu() (implements the deferred probe checking to
correctly order the client/provider driver probing)

[PATCH 1/2] pinctrl: pinctrl-imx: add support for set bits for general purpose registers

[Dong Aisheng]

On Fri, Jul 06, 2012 at 11:52:49PM +0800, Stephen Warren wrote:
> On 07/06/2012 03:09 AM, Dong Aisheng wrote:
> > From: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > The General Purpose Registers (GPR) is used to select operating modes for
> > general features in the SoC, usually not related to the IOMUX itself,
> > but it does belong to IOMUX controller.
> > We simply provide an convient API for driver to call to set the general purpose
> > register bits if needed.
> 
> > +static struct imx_pinctrl *imx_pinctrl;
> > +/*
> > + * Set bits for general purpose registers
> > + */
> > +void imx_pinctrl_set_gpr_register(u8 gpr, u8 start_bit, u8 num_bits, u32 value)
> > +{
> > +	u32 reg;
> > +
> > +	/* general purpose register is 32 bits size */
> > +	WARN_ON(!imx_pinctrl || start_bit > 31 || num_bits > 32);
> 
> Hmmm. It's going to be very hard to control the probe() order to ensure
> that this WARN doesn't fire all the time.
> 
Correct, if this api is used by client driver before the pinctrl driver is
registered, the warning will show.
To avoid it, the current pinctrl driver register priority is arch_initcall.
But maybe you're right, this may not be so sufficient since i'm afraid there
are still possible some devices may register earlier than pinctrl since it's not
controlled by pinctrl driver.
Then it's true that we need to resolve it.

> I think it would be better to pass in a struct imx_pinctrl* or DT node
> to this function. The DT node for the device that's using this function
> should contain a phandle to the pinctrl device node, which it uses to
> get that handle. Or in a non-DT case, the client driver needs to be
> given the provider driver handle using some other mechanism.
> 
> For example, look at how the Tegra30 SMMU uses services from the Tegra30
> AHB; see arch/arm/boot/dts/tegra30.dtsi node "smmu" (client) and node
> "ahb" (provider), drivers/iommu/tegra-smmu.c functions probe() (saves
> smmu->ahb) and smmu_setup_regs() (calls tegra_ahb_enable_smmu() with
> this handle) and drivers/amba/tegra-ahb.c function
> tegra_ahb_enable_smmu() (implements the deferred probe checking to
> correctly order the client/provider driver probing)
> 
Yes, i learned the code but i'm not sure it also fits for imx.
I have a few concerns:
1) i'm not sure if we really need the client to provide pinctrl device
node as parameter to set gpr registers since there is only one pinctrl device
for each imx soc. Client devices may also not want to care that parameter.
2) if passing device node to the api, that means every client driver
needs to define a phandle of pinctrl device in dts file and parsing it
in each driver respectively.
There're several client devices for imx6q, i'm not sure if it's worth to do that
considering this overhead.

I think either passing device node or not passing, the goal is the same,
guarantee this api to be used properly without being affected by driver
loading sequence.

If that, how about change to:
int imx_pinctrl_set_gpr_register(u8 gpr, u8 start_bit, u8 num_bits, u32 value)
{
        u32 reg;

	if (!imx_pinctrl)
		return -EDEFER_PROBE;

        /* general purpose register is 32 bits size */
        WARN_ON(start_bit > 31 || num_bits > 32);
...
}
EXPORT_SYMBOL_GPL(imx_pinctrl_set_gpr_register);

Then it looks to me it may be able to solve the same issue.

Regards
Dong Aisheng

[PATCH 1/2] pinctrl: pinctrl-imx: add support for set bits for general purpose registers

[Richard Zhao]

On Mon, Jul 09, 2012 at 03:10:21PM +0800, Dong Aisheng wrote:
> On Fri, Jul 06, 2012 at 11:52:49PM +0800, Stephen Warren wrote:
> > On 07/06/2012 03:09 AM, Dong Aisheng wrote:
> > > From: Dong Aisheng <dong.aisheng@linaro.org>
> > > 
> > > The General Purpose Registers (GPR) is used to select operating modes for
> > > general features in the SoC, usually not related to the IOMUX itself,
> > > but it does belong to IOMUX controller.
> > > We simply provide an convient API for driver to call to set the general purpose
> > > register bits if needed.
> > 
> > > +static struct imx_pinctrl *imx_pinctrl;
> > > +/*
> > > + * Set bits for general purpose registers
> > > + */
> > > +void imx_pinctrl_set_gpr_register(u8 gpr, u8 start_bit, u8 num_bits, u32 value)
> > > +{
> > > +	u32 reg;
> > > +
> > > +	/* general purpose register is 32 bits size */
> > > +	WARN_ON(!imx_pinctrl || start_bit > 31 || num_bits > 32);
> > 
> > Hmmm. It's going to be very hard to control the probe() order to ensure
> > that this WARN doesn't fire all the time.
> > 
> Correct, if this api is used by client driver before the pinctrl driver is
> registered, the warning will show.
> To avoid it, the current pinctrl driver register priority is arch_initcall.
+postcore_initcall is not enough. Could you use postcore_initcall? So people
can hack right after populate devices.

> But maybe you're right, this may not be so sufficient since i'm afraid there
> are still possible some devices may register earlier than pinctrl since it's not
> controlled by pinctrl driver.
> Then it's true that we need to resolve it.
imx_pinctrl_set_gpr_register is SoC specific. People who use it must
have sense of related registers and driver loading sequence.
> 
> > I think it would be better to pass in a struct imx_pinctrl* or DT node
> > to this function.
Maybe we can do it, but why? It's kind of over-engineering.

> The DT node for the device that's using this function
> > should contain a phandle to the pinctrl device node, which it uses to
> > get that handle. Or in a non-DT case, the client driver needs to be
> > given the provider driver handle using some other mechanism.
> > 
> > For example, look at how the Tegra30 SMMU uses services from the Tegra30
> > AHB; see arch/arm/boot/dts/tegra30.dtsi node "smmu" (client) and node
> > "ahb" (provider), drivers/iommu/tegra-smmu.c functions probe() (saves
> > smmu->ahb) and smmu_setup_regs() (calls tegra_ahb_enable_smmu() with
> > this handle) and drivers/amba/tegra-ahb.c function
> > tegra_ahb_enable_smmu() (implements the deferred probe checking to
> > correctly order the client/provider driver probing)
> > 
> Yes, i learned the code but i'm not sure it also fits for imx.
> I have a few concerns:
> 1) i'm not sure if we really need the client to provide pinctrl device
> node as parameter to set gpr registers since there is only one pinctrl device
> for each imx soc. Client devices may also not want to care that parameter.
> 2) if passing device node to the api, that means every client driver
> needs to define a phandle of pinctrl device in dts file and parsing it
> in each driver respectively.
> There're several client devices for imx6q, i'm not sure if it's worth to do that
> considering this overhead.
> 
> I think either passing device node or not passing, the goal is the same,
> guarantee this api to be used properly without being affected by driver
> loading sequence.
> 
> If that, how about change to:
> int imx_pinctrl_set_gpr_register(u8 gpr, u8 start_bit, u8 num_bits, u32 value)
> {
>         u32 reg;
> 
> 	if (!imx_pinctrl)
> 		return -EDEFER_PROBE;
> 
>         /* general purpose register is 32 bits size */
>         WARN_ON(start_bit > 31 || num_bits > 32);
Isn't it BUG ?
Normally, people like write_register(addr, mask, value),
and read_register(addr, mask);

Thanks
Richard

> ...
> }
> EXPORT_SYMBOL_GPL(imx_pinctrl_set_gpr_register);
> 
> Then it looks to me it may be able to solve the same issue.
> 
> Regards
> Dong Aisheng

[PATCH 1/2] pinctrl: pinctrl-imx: add support for set bits for general purpose registers

[Dong Aisheng]

On Wed, Jul 11, 2012 at 05:33:32PM +0800, Zhao Richard-B20223 wrote:
> On Mon, Jul 09, 2012 at 03:10:21PM +0800, Dong Aisheng wrote:
> > On Fri, Jul 06, 2012 at 11:52:49PM +0800, Stephen Warren wrote:
> > > On 07/06/2012 03:09 AM, Dong Aisheng wrote:
> > > > From: Dong Aisheng <dong.aisheng@linaro.org>
> > > > 
> > > > The General Purpose Registers (GPR) is used to select operating modes for
> > > > general features in the SoC, usually not related to the IOMUX itself,
> > > > but it does belong to IOMUX controller.
> > > > We simply provide an convient API for driver to call to set the general purpose
> > > > register bits if needed.
> > > 
> > > > +static struct imx_pinctrl *imx_pinctrl;
> > > > +/*
> > > > + * Set bits for general purpose registers
> > > > + */
> > > > +void imx_pinctrl_set_gpr_register(u8 gpr, u8 start_bit, u8 num_bits, u32 value)
> > > > +{
> > > > +	u32 reg;
> > > > +
> > > > +	/* general purpose register is 32 bits size */
> > > > +	WARN_ON(!imx_pinctrl || start_bit > 31 || num_bits > 32);
> > > 
> > > Hmmm. It's going to be very hard to control the probe() order to ensure
> > > that this WARN doesn't fire all the time.
> > > 
> > Correct, if this api is used by client driver before the pinctrl driver is
> > registered, the warning will show.
> > To avoid it, the current pinctrl driver register priority is arch_initcall.
> +postcore_initcall is not enough. Could you use postcore_initcall? So people
> can hack right after populate devices.
> 
Yes, i will change to postcore_initcall to satisfy the client devices at best.

> > But maybe you're right, this may not be so sufficient since i'm afraid there
> > are still possible some devices may register earlier than pinctrl since it's not
> > controlled by pinctrl driver.
> > Then it's true that we need to resolve it.
> imx_pinctrl_set_gpr_register is SoC specific. People who use it must
> have sense of related registers and driver loading sequence.
> > 
> > > I think it would be better to pass in a struct imx_pinctrl* or DT node
> > > to this function.
> Maybe we can do it, but why? It's kind of over-engineering.
> 
> > The DT node for the device that's using this function
> > > should contain a phandle to the pinctrl device node, which it uses to
> > > get that handle. Or in a non-DT case, the client driver needs to be
> > > given the provider driver handle using some other mechanism.
> > > 
> > > For example, look at how the Tegra30 SMMU uses services from the Tegra30
> > > AHB; see arch/arm/boot/dts/tegra30.dtsi node "smmu" (client) and node
> > > "ahb" (provider), drivers/iommu/tegra-smmu.c functions probe() (saves
> > > smmu->ahb) and smmu_setup_regs() (calls tegra_ahb_enable_smmu() with
> > > this handle) and drivers/amba/tegra-ahb.c function
> > > tegra_ahb_enable_smmu() (implements the deferred probe checking to
> > > correctly order the client/provider driver probing)
> > > 
> > Yes, i learned the code but i'm not sure it also fits for imx.
> > I have a few concerns:
> > 1) i'm not sure if we really need the client to provide pinctrl device
> > node as parameter to set gpr registers since there is only one pinctrl device
> > for each imx soc. Client devices may also not want to care that parameter.
> > 2) if passing device node to the api, that means every client driver
> > needs to define a phandle of pinctrl device in dts file and parsing it
> > in each driver respectively.
> > There're several client devices for imx6q, i'm not sure if it's worth to do that
> > considering this overhead.
> > 
> > I think either passing device node or not passing, the goal is the same,
> > guarantee this api to be used properly without being affected by driver
> > loading sequence.
> > 
> > If that, how about change to:
> > int imx_pinctrl_set_gpr_register(u8 gpr, u8 start_bit, u8 num_bits, u32 value)
> > {
> >         u32 reg;
> > 
> > 	if (!imx_pinctrl)
> > 		return -EDEFER_PROBE;
> > 
> >         /* general purpose register is 32 bits size */
> >         WARN_ON(start_bit > 31 || num_bits > 32);
> Isn't it BUG ?
Hmm, it seems it's not so seriously to hang kernel.

> Normally, people like write_register(addr, mask, value),
> and read_register(addr, mask);
> 
Correct, will try it.

Regards
Dong Aisheng

[PATCH 2/2] pinctrl: pinctrl-imx6q: add missed mux function for USBOTG_ID

[Linus Walleij]

On Fri, Jul 6, 2012 at 11:09 AM, Dong Aisheng <b29396@freescale.com> wrote:

> From: Dong Aisheng <dong.aisheng@linaro.org>
>
> The original pin registers table is derived from u-boot mainline,
> but somehow it was found missing some mux functions for USBOTG_ID.
> We added it at the bottom by following the exist pin function ids,
> then it will not break the exist using of pin function id in dts file.
>
> Reported-by: Richard Zhao <richard.zhao@freescale.com>
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>

Applied.

Thanks!
Linus Walleij

[PATCH 1/2] pinctrl: pinctrl-imx: add support for set bits for general purpose registers

[Linus Walleij]

On Fri, Jul 6, 2012 at 11:09 AM, Dong Aisheng <b29396@freescale.com> wrote:

> From: Dong Aisheng <dong.aisheng@linaro.org>
>
> The General Purpose Registers (GPR) is used to select operating modes for
> general features in the SoC, usually not related to the IOMUX itself,
> but it does belong to IOMUX controller.
> We simply provide an convient API for driver to call to set the general purpose
> register bits if needed.
>
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>

I have an overall objection to this:

I have no idea at all about what's going on, and why this belongs in the pin
control subsystem. On the contrary, from the commit description it seems to
*not* belong in this subsystem at all.

So: exactly what does this register do, and which are the consumers?

Notice: nothing at all stops you from mapping the same address range or
just this register in some other driver. So the same address range being
used is not a reason to have this in this driver, it could very well be
handled by drivers/misc/imx-syscon.c or maybe my postulated
(but non-existent) drivers/syscon/*.

The reason I react against it is that some GPIO drivers already have
custom API:s doing pinctrl stuff, and shunning this kind of stuff into
the pinctrl subsystem might hide it from the real subsystem where it
actually belongs and hinder development of the proper subsystems.

Yours,
Linus Walleij

[PATCH 1/2] pinctrl: pinctrl-imx: add support for set bits for general purpose registers

[Dong Aisheng]

On Sun, Jul 15, 2012 at 04:24:16AM +0800, Linus Walleij wrote:
> On Fri, Jul 6, 2012 at 11:09 AM, Dong Aisheng <b29396@freescale.com> wrote:
> 
> > From: Dong Aisheng <dong.aisheng@linaro.org>
> >
> > The General Purpose Registers (GPR) is used to select operating modes for
> > general features in the SoC, usually not related to the IOMUX itself,
> > but it does belong to IOMUX controller.
> > We simply provide an convient API for driver to call to set the general purpose
> > register bits if needed.
> >
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> 
> I have an overall objection to this:
> 
> I have no idea at all about what's going on, and why this belongs in the pin
> control subsystem. On the contrary, from the commit description it seems to
> *not* belong in this subsystem at all.
> 
Yes, it's just from hw point of view, these GPR registers are included in IOMUXC
block. But as the spec says in my commit messages, most of what they do are not
related to IOMUX itself.
So i would agree it doesn't belong to pinctrl subsystem if we can find a better
place.

> So: exactly what does this register do, and which are the consumers?
> 
They're used for general settings for different modules like DMA, USB, PCIe,
IPU...

Regards
Dong Aisheng