| Message ID | 1375330924-27384-1-git-send-email-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 01, 2013 at 12:22:03PM +0800, Shawn Guo wrote: > 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 <shawn.guo@linaro.org> > --- > drivers/pinctrl/pinctrl-imx.c | 25 ++++++++++++++++++++++++- > 1 files changed, 24 insertions(+), 1 deletions(-) > > diff --git a/drivers/pinctrl/pinctrl-imx.c b/drivers/pinctrl/pinctrl-imx.c > index 57a4eb0..6ebe2e9 100644 > --- a/drivers/pinctrl/pinctrl-imx.c > +++ b/drivers/pinctrl/pinctrl-imx.c > @@ -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]); > -- > 1.7.1 > Shawn, input_reg[i] may be 0 if the fixed one at first register of IOMUXC
On 1 August 2013 14:51, Peter Chen <peter.chen@freescale.com> wrote:
> 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
On Thu, Aug 01, 2013 at 03:08:06PM +0800, Shawn Guo wrote: > On 1 August 2013 14:51, Peter Chen <peter.chen@freescale.com> wrote: > > 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 <peter.chen@freescale.com> 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.
On Thu, Aug 01, 2013 at 04:32:23PM +0800, Peter Chen wrote: > On Thu, Aug 01, 2013 at 03:08:06PM +0800, Shawn Guo wrote: > > On 1 August 2013 14:51, Peter Chen <peter.chen@freescale.com> wrote: > > > 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 <peter.chen@freescale.com> Thanks. > > 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. > > 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
On Sun, Aug 04, 2013 at 08:54:54PM +0800, Shawn Guo wrote: > Yes, we changed the prefix to simplify the DTS files for imx6q and > imx6dl. > > > > > 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
On Mon, Aug 05, 2013 at 11:26:14AM +0800, Shawn Guo wrote: > On Mon, Aug 05, 2013 at 09:14:50AM +0800, Peter Chen wrote: > > On Sun, Aug 04, 2013 at 08:54:54PM +0800, Shawn Guo wrote: > > > Yes, we changed the prefix to simplify the DTS files for imx6q and > > > imx6dl. > > > > > > > > > > > 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. > > 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.
On Mon, Aug 05, 2013 at 09:14:50AM +0800, Peter Chen wrote: > On Sun, Aug 04, 2013 at 08:54:54PM +0800, Shawn Guo wrote: > > Yes, we changed the prefix to simplify the DTS files for imx6q and > > imx6dl. > > > > > > > > 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. 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
diff --git a/drivers/pinctrl/pinctrl-imx.c b/drivers/pinctrl/pinctrl-imx.c index 57a4eb0..6ebe2e9 100644 --- a/drivers/pinctrl/pinctrl-imx.c +++ b/drivers/pinctrl/pinctrl-imx.c @@ -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]);
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 <shawn.guo@linaro.org> --- drivers/pinctrl/pinctrl-imx.c | 25 ++++++++++++++++++++++++- 1 files changed, 24 insertions(+), 1 deletions(-)