pinctrl: imx: work around select input quirk

PATCH Superseded Shawn Guo 3 participants 12 messages Aug 1, 2013 – Aug 13, 2013

Subsystems (3)

Device tree bindings Arm i.MX and Layerscape Pin control

lore

Share & tools

lore

Shawn Guo

author

2013-08-01 04:22 UTC #0 lore raw

PATCH 1/2 pinctrl: imx: work around select input quirk

The select input for some pin may not be implemented using the regular select input register but the general purpose register. A real example is that imx6q designers found the select input for USB OTG ID pin is missing at the very late stage, and can not add a new select input register but have to use a general purpose register bit to implement it.

The patch adds a workaround for such select input quirk by interpreting the input_val cell of pin function ID in a different way, so that all the info that needed for setting up select input bits in general purpose register could be decoded from there.

Signed-off-by Shawn Guo <[email protected]>

---
 drivers/pinctrl/pinctrl-imx.c |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)

drivers/pinctrl/pinctrl-imx.c +24 -1

index 57a4eb0..6ebe2e9 100644
@@ -241,7 +241,30 @@ static int imx_pmx_enable(struct pinctrl_dev *pctldev, unsigned selector,
 
 		/* some pins also need select input setting, set it if found */
 		if (input_reg[i]) {
-			writel(input_val[i], ipctl->base + input_reg[i]);
+			u32 val = input_val[i];
+			/*
+			 * If the select input value begins with 0xff, the value
+			 * will be interpreted as below.
+			 *     31     23      15      7        0
+			 *     | 0xff | shift | width | select |
+			 * It's used to work around the problem that the select
+			 * input for some pin is not implemented in the select
+			 * input register but in some general purpose register.
+			 * We encode the select input value, width and shift of
+			 * the bit field into input_val cell of pin function ID
+			 * in device tree, and then decode them here for setting
+			 * up the select input bits in general purpose register.
+			 */
+			if (val >> 24 == 0xff) {
+				u8 select = val & 0xff;
+				u8 width = (val >> 8) & 0xff;
+				u8 shift = (val >> 16) & 0xff;
+				u32 mask = ((1 << width) - 1) << shift;
+				val = readl(ipctl->base + input_reg[i]);
+				val &= ~mask;
+				val |= select << shift;
+			}
+			writel(val, ipctl->base + input_reg[i]);
 			dev_dbg(ipctl->dev,
 				"==>select_input: offset 0x%x val 0x%x\n",
 				input_reg[i], input_val[i]);

Signature · 1 line

1.7.1

Shawn Guo

author

2013-08-01 04:22 UTC #1 lore raw

PATCH 2/2 ARM: dts: imx6q: add quirky select input for USB_OTG_ID

For some reason, the select input of pin function USB_OTG_ID is not implemented via a regular select input register but using the bit USB_OTG_ID_ SEL (shift 13) of IOMUXC_GPR1 register (offset 0x4).

As per the workaround for such quirk implemented in pinctrl driver, we need to compose the input_val cell as below.

	31     23      15      7        0
	| 0xff | shift | width | select |

Thus, we have 0xff0d0100 for MX6QDL_PAD_ENET_RX_ER__USB_OTG_ID and 0xff0d0101 for MX6QDL_PAD_GPIO_1__USB_OTG_ID in input_val cell.

Signed-off-by Shawn Guo <[email protected]>

---
 arch/arm/boot/dts/imx6q-pinfunc.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

arch/arm/boot/dts/imx6q-pinfunc.h +2 -2

index c0e38a4..44507a3 100644
@@ -536,7 +536,7 @@
 #define MX6QDL_PAD_ENET_REF_CLK__ESAI_RX_FS         0x1d4 0x4e8 0x85c 0x2 0x0
 #define MX6QDL_PAD_ENET_REF_CLK__GPIO1_IO23         0x1d4 0x4e8 0x000 0x5 0x0
 #define MX6QDL_PAD_ENET_REF_CLK__SPDIF_SR_CLK       0x1d4 0x4e8 0x000 0x6 0x0
-#define MX6QDL_PAD_ENET_RX_ER__USB_OTG_ID           0x1d8 0x4ec 0x000 0x0 0x0
+#define MX6QDL_PAD_ENET_RX_ER__USB_OTG_ID           0x1d8 0x4ec 0x004 0x0 0xff0d0100
 #define MX6QDL_PAD_ENET_RX_ER__ENET_RX_ER           0x1d8 0x4ec 0x000 0x1 0x0
 #define MX6QDL_PAD_ENET_RX_ER__ESAI_RX_HF_CLK       0x1d8 0x4ec 0x864 0x2 0x0
 #define MX6QDL_PAD_ENET_RX_ER__SPDIF_IN             0x1d8 0x4ec 0x914 0x3 0x1

@@ -654,7 +654,7 @@
 #define MX6QDL_PAD_GPIO_1__ESAI_RX_CLK              0x224 0x5f4 0x86c 0x0 0x1
 #define MX6QDL_PAD_GPIO_1__WDOG2_B                  0x224 0x5f4 0x000 0x1 0x0
 #define MX6QDL_PAD_GPIO_1__KEY_ROW5                 0x224 0x5f4 0x8f4 0x2 0x0
-#define MX6QDL_PAD_GPIO_1__USB_OTG_ID               0x224 0x5f4 0x000 0x3 0x0
+#define MX6QDL_PAD_GPIO_1__USB_OTG_ID               0x224 0x5f4 0x004 0x3 0xff0d0101
 #define MX6QDL_PAD_GPIO_1__PWM2_OUT                 0x224 0x5f4 0x000 0x4 0x0
 #define MX6QDL_PAD_GPIO_1__GPIO1_IO01               0x224 0x5f4 0x000 0x5 0x0
 #define MX6QDL_PAD_GPIO_1__SD1_CD_B                 0x224 0x5f4 0x000 0x6 0x0

Signature · 1 line

1.7.1

Linus Walleij

2013-08-07 18:34 UTC #2 lore raw

PATCH 2/2 ARM: dts: imx6q: add quirky select input for USB_OTG_ID

Quoted 7 lines · Shawn Guo

For some reason, the select input of pin function USB_OTG_ID is not implemented via a regular select input register but using the bit USB_OTG_ID_ SEL (shift 13) of IOMUXC_GPR1 register (offset 0x4).
As per the workaround for such quirk implemented in pinctrl driver, we need to compose the input_val cell as below.

Quoted 7 lines

        31     23      15      7        0
        | 0xff | shift | width | select |
Thus, we have 0xff0d0100 for MX6QDL_PAD_ENET_RX_ER__USB_OTG_ID and 0xff0d0101 for MX6QDL_PAD_GPIO_1__USB_OTG_ID in input_val cell.
Signed-off-by Shawn Guo <[email protected]>

This does not apply on top of my "devel" branch in the pinctrl tree.

Could you respin this on top of devel and resend?

Or do I need some other patch(es) for this to apply?

Yours,
Linus Walleij

Shawn Guo

author

2013-08-08 01:27 UTC #3 lore raw

PATCH 2/2 ARM: dts: imx6q: add quirky select input for USB_OTG_ID

Quoted 15 lines · Linus Walleij

Quoted 13 lines · Shawn Guo
For some reason, the select input of pin function USB_OTG_ID is not implemented via a regular select input register but using the bit USB_OTG_ID_ SEL (shift 13) of IOMUXC_GPR1 register (offset 0x4).
As per the workaround for such quirk implemented in pinctrl driver, we need to compose the input_val cell as below.
        31     23      15      7        0
        | 0xff | shift | width | select |
Thus, we have 0xff0d0100 for MX6QDL_PAD_ENET_RX_ER__USB_OTG_ID and 0xff0d0101 for MX6QDL_PAD_GPIO_1__USB_OTG_ID in input_val cell.

Quoted 7 lines

Quoted 1 line
Signed-off-by Shawn Guo <[email protected]>
This does not apply on top of my "devel" branch in the pinctrl tree.
Could you respin this on top of devel and resend?
Or do I need some other patch(es) for this to apply?

Sorry for not making this clear. Due to the changes happened on IMX tree, we will have to queue this one for 3.13 merge window through IMX tree, after driver change hits mainline. In short, I will take care of the merge of this patch.

Shawn

Linus Walleij

2013-08-13 06:18 UTC #4 lore raw

PATCH 2/2 ARM: dts: imx6q: add quirky select input for USB_OTG_ID

Quoted 20 lines · Shawn Guo

Quoted 19 lines · Linus Walleij
Quoted 7 lines · Shawn Guo
For some reason, the select input of pin function USB_OTG_ID is not implemented via a regular select input register but using the bit USB_OTG_ID_ SEL (shift 13) of IOMUXC_GPR1 register (offset 0x4).
As per the workaround for such quirk implemented in pinctrl driver, we need to compose the input_val cell as below.
Quoted 7 lines
        31     23      15      7        0
        | 0xff | shift | width | select |
Thus, we have 0xff0d0100 for MX6QDL_PAD_ENET_RX_ER__USB_OTG_ID and 0xff0d0101 for MX6QDL_PAD_GPIO_1__USB_OTG_ID in input_val cell.
Signed-off-by Shawn Guo <[email protected]>
This does not apply on top of my "devel" branch in the pinctrl tree.

Quoted 8 lines

Quoted 3 lines
Could you respin this on top of devel and resend?
Or do I need some other patch(es) for this to apply?
Sorry for not making this clear. Due to the changes happened on IMX tree, we will have to queue this one for 3.13 merge window through IMX tree, after driver change hits mainline. In short, I will take care of the merge of this patch.

OK Acked-by: Linus Walleij <[email protected]>

Yours,
Linus Walleij

Peter Chen

2013-08-01 06:51 UTC #5 lore raw

PATCH 1/2 pinctrl: imx: work around select input quirk

In reply to Shawn Guo

Quoted 55 lines · Shawn Guo

Signed-off-by Shawn Guo <[email protected]>

---
 drivers/pinctrl/pinctrl-imx.c |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)

drivers/pinctrl/pinctrl-imx.c +24 -1

index 57a4eb0..6ebe2e9 100644
@@ -241,7 +241,30 @@ static int imx_pmx_enable(struct pinctrl_dev *pctldev, unsigned selector,
 
 		/* some pins also need select input setting, set it if found */
 		if (input_reg[i]) {
-			writel(input_val[i], ipctl->base + input_reg[i]);
+			u32 val = input_val[i];
+			/*
+			 * If the select input value begins with 0xff, the value
+			 * will be interpreted as below.
+			 *     31     23      15      7        0
+			 *     | 0xff | shift | width | select |
+			 * It's used to work around the problem that the select
+			 * input for some pin is not implemented in the select
+			 * input register but in some general purpose register.
+			 * We encode the select input value, width and shift of
+			 * the bit field into input_val cell of pin function ID
+			 * in device tree, and then decode them here for setting
+			 * up the select input bits in general purpose register.
+			 */
+			if (val >> 24 == 0xff) {
+				u8 select = val & 0xff;
+				u8 width = (val >> 8) & 0xff;
+				u8 shift = (val >> 16) & 0xff;
+				u32 mask = ((1 << width) - 1) << shift;
+				val = readl(ipctl->base + input_reg[i]);
+				val &= ~mask;
+				val |= select << shift;
+			}
+			writel(val, ipctl->base + input_reg[i]);
 			dev_dbg(ipctl->dev,
 				"==>select_input: offset 0x%x val 0x%x\n",
 				input_reg[i], input_val[i]);

Signature · 1 line

1.7.1

Shawn, input_reg[i] may be 0 if the fixed one at first register of IOMUXC

Signature · 2 lines

Best Regards,
Peter Chen

Shawn Guo

author

2013-08-01 07:08 UTC #6 lore raw

PATCH 1/2 pinctrl: imx: work around select input quirk

Quoted 1 line · Peter Chen

Shawn, input_reg[i] may be 0 if the fixed one at first register of IOMUXC

Ah, yes. Now select input register at offset 0 becomes possible. I will try to fix it.

Regardless of this, does the series work for you?

Shawn

Peter Chen

2013-08-01 08:32 UTC #7 lore raw

PATCH 1/2 pinctrl: imx: work around select input quirk

Quoted 8 lines · Shawn Guo

Quoted 1 line · Peter Chen
Shawn, input_reg[i] may be 0 if the fixed one at first register of IOMUXC
Ah, yes. Now select input register at offset 0 becomes possible. I will try to fix it.
Regardless of this, does the series work for you?

Yes, it works, you can add : Tested-by: Peter Chen <[email protected]>

I changed the second patch manually, have you changed the pin prefix at arch/arm/boot/dts/imx6q-pinfunc.h? At 3.10, it is MX6Q_XXX.

Besides, please add comments for u16 *input_reg at struct imx_pin_group.

Signature · 2 lines

Best Regards,
Peter Chen

Shawn Guo

author

2013-08-04 12:54 UTC #8 lore raw

PATCH 1/2 pinctrl: imx: work around select input quirk

Quoted 10 lines · Peter Chen

Quoted 8 lines · Shawn Guo
Quoted 1 line · Peter Chen
Shawn, input_reg[i] may be 0 if the fixed one at first register of IOMUXC
Ah, yes. Now select input register at offset 0 becomes possible. I will try to fix it.
Regardless of this, does the series work for you?
Yes, it works, you can add : Tested-by: Peter Chen <[email protected]>

Thanks.

Quoted 3 lines

I changed the second patch manually, have you changed the pin prefix at arch/arm/boot/dts/imx6q-pinfunc.h? At 3.10, it is MX6Q_XXX.

Yes, we changed the prefix to simplify the DTS files for imx6q and imx6dl.

Quoted 2 lines

Besides, please add comments for u16 *input_reg at struct imx_pin_group.

I'm not fond of documenting a workaround for a random quirky select input as a feature all over the files where input_reg is documented. It should be good enough to have it well documented at where the quirk is handled.

Shawn

Peter Chen

2013-08-05 01:14 UTC #9 lore raw

PATCH 1/2 pinctrl: imx: work around select input quirk

Quoted 10 lines · Shawn Guo

Yes, we changed the prefix to simplify the DTS files for imx6q and imx6dl.
Quoted 2 lines
Besides, please add comments for u16 *input_reg at struct imx_pin_group.
I'm not fond of documenting a workaround for a random quirky select input as a feature all over the files where input_reg is documented. It should be good enough to have it well documented at where the quirk is handled.

If the user finds "odd value" at xxx-pinfunc.h, how he knows what it stands for? At least, It should be documented where the user can find its meaning.

Best regards,
Peter

Shawn Guo

author

2013-08-05 03:26 UTC #10 lore raw

PATCH 1/2 pinctrl: imx: work around select input quirk

Quoted 7 lines · Peter Chen

Quoted 6 lines · Shawn Guo
Yes, we changed the prefix to simplify the DTS files for imx6q and imx6dl.
Quoted 2 lines
Besides, please add comments for u16 *input_reg at struct imx_pin_group.

Quoted 8 lines

Quoted 4 lines
I'm not fond of documenting a workaround for a random quirky select input as a feature all over the files where input_reg is documented. It should be good enough to have it well documented at where the quirk is handled.
If the user finds "odd value" at xxx-pinfunc.h, how he knows what it stands for? At least, It should be documented where the user can find its meaning.

We have it well documented in pinctrl-imx.c, function imx_pmx_enable() where the "odd value" is handled.

But I would try to look at the git log of xxx-pinfunc.h at the first place to see where and how the "odd value" comes.

Shawn

Peter Chen

2013-08-05 01:35 UTC #11 lore raw

PATCH 1/2 pinctrl: imx: work around select input quirk

Quoted 17 lines · Shawn Guo

Quoted 7 lines · Peter Chen
Quoted 6 lines · Shawn Guo
Yes, we changed the prefix to simplify the DTS files for imx6q and imx6dl.
Quoted 2 lines
Besides, please add comments for u16 *input_reg at struct imx_pin_group.
Quoted 8 lines
Quoted 4 lines
I'm not fond of documenting a workaround for a random quirky select input as a feature all over the files where input_reg is documented. It should be good enough to have it well documented at where the quirk is handled.
If the user finds "odd value" at xxx-pinfunc.h, how he knows what it stands for? At least, It should be documented where the user can find its meaning.

Quoted 6 lines

We have it well documented in pinctrl-imx.c, function imx_pmx_enable() where the "odd value" is handled.
But I would try to look at the git log of xxx-pinfunc.h at the first place to see where and how the "odd value" comes.

Yes, it is also OK.

Signature · 2 lines

Best Regards,
Peter Chen