| Message ID | 1494592686-30967-3-git-send-email-aisheng.dong@nxp.com |
|---|---|
| State | New |
| Headers | show |
On Fri, May 12, 2017 at 08:38:02PM +0800, Dong Aisheng wrote: > MX7ULP MUX mode mask and shift bit is different from VF610. > Let's make it a platform specific property for the later easy of > adding MX7ULP support. > > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Stefan Agner <stefan@agner.ch> > Cc: Bai Ping <ping.bai@nxp.com> > Signed-off-by: Fugang Duan <fugang.duan@nxp.com> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > --- > drivers/pinctrl/freescale/pinctrl-imx.c | 10 +++++----- > drivers/pinctrl/freescale/pinctrl-imx.h | 4 ++++ > drivers/pinctrl/freescale/pinctrl-vf610.c | 2 ++ > 3 files changed, 11 insertions(+), 5 deletions(-) > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c > index db76e9d..8347197 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx.c > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c > @@ -197,8 +197,8 @@ static int imx_pmx_set(struct pinctrl_dev *pctldev, unsigned selector, > if (info->flags & SHARE_MUX_CONF_REG) { > u32 reg; > reg = readl(ipctl->base + pin_reg->mux_reg); > - reg &= ~(0x7 << 20); > - reg |= (pin->mux_mode << 20); > + reg &= ~info->mux_mask; > + reg |= (pin->mux_mode << info->mux_shift); > writel(reg, ipctl->base + pin_reg->mux_reg); > } else { > writel(pin->mux_mode, ipctl->base + pin_reg->mux_reg); > @@ -288,7 +288,7 @@ static int imx_pmx_gpio_request_enable(struct pinctrl_dev *pctldev, > > mux_pin: > reg = readl(ipctl->base + pin_reg->mux_reg); > - reg &= ~(0x7 << 20); > + reg &= ~info->mux_mask; > reg |= imx_pin->config; > writel(reg, ipctl->base + pin_reg->mux_reg); > > @@ -432,7 +432,7 @@ static int imx_pinconf_get(struct pinctrl_dev *pctldev, > *config = readl(ipctl->base + pin_reg->conf_reg); > > if (info->flags & SHARE_MUX_CONF_REG) > - *config &= 0xffff; > + *config &= ~info->mux_mask; It changes the way how code works. We need a note in the commit log explaining why it's safe. Shawn > > return 0; > } > @@ -459,7 +459,7 @@ static int imx_pinconf_set(struct pinctrl_dev *pctldev, > if (info->flags & SHARE_MUX_CONF_REG) { > u32 reg; > reg = readl(ipctl->base + pin_reg->conf_reg); > - reg &= ~0xffff; > + reg &= info->mux_mask; > reg |= configs[i]; > writel(reg, ipctl->base + pin_reg->conf_reg); > } else { > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h b/drivers/pinctrl/freescale/pinctrl-imx.h > index b5c8fe1..eb0ce95 100644 > --- a/drivers/pinctrl/freescale/pinctrl-imx.h > +++ b/drivers/pinctrl/freescale/pinctrl-imx.h > @@ -64,6 +64,10 @@ struct imx_pinctrl_soc_info { > const char *gpr_compatible; > struct mutex mutex; > > + /* MUX_MODE shift and mask in case SHARE_MUX_CONF_REG */ > + unsigned int mux_mask; > + u8 mux_shift; > + > /* generic pinconf */ > bool generic_pinconf; > const struct pinconf_generic_params *custom_params; > diff --git a/drivers/pinctrl/freescale/pinctrl-vf610.c b/drivers/pinctrl/freescale/pinctrl-vf610.c > index 2b1e198..3bd8556 100644 > --- a/drivers/pinctrl/freescale/pinctrl-vf610.c > +++ b/drivers/pinctrl/freescale/pinctrl-vf610.c > @@ -299,6 +299,8 @@ static struct imx_pinctrl_soc_info vf610_pinctrl_info = { > .pins = vf610_pinctrl_pads, > .npins = ARRAY_SIZE(vf610_pinctrl_pads), > .flags = SHARE_MUX_CONF_REG | ZERO_OFFSET_VALID, > + .mux_mask = 0x700000, > + .mux_shift = 20, > }; > > static const struct of_device_id vf610_pinctrl_of_match[] = { > -- > 2.7.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Shawn Guo [mailto:shawnguo@kernel.org] > Sent: Monday, May 15, 2017 4:53 PM > To: A.S. Dong > Cc: linux-gpio@vger.kernel.org; Andy Duan; Jacky Bai; > linus.walleij@linaro.org; stefan@agner.ch; kernel@pengutronix.de; linux- > arm-kernel@lists.infradead.org > Subject: Re: [PATCH 2/5] pinctrl: imx: add soc specific mux_mode mask and > shift property > > On Fri, May 12, 2017 at 08:38:02PM +0800, Dong Aisheng wrote: > > MX7ULP MUX mode mask and shift bit is different from VF610. > > Let's make it a platform specific property for the later easy of > > adding MX7ULP support. > > > > Cc: Linus Walleij <linus.walleij@linaro.org> > > Cc: Shawn Guo <shawnguo@kernel.org> > > Cc: Stefan Agner <stefan@agner.ch> > > Cc: Bai Ping <ping.bai@nxp.com> > > Signed-off-by: Fugang Duan <fugang.duan@nxp.com> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> > > --- > > drivers/pinctrl/freescale/pinctrl-imx.c | 10 +++++----- > > drivers/pinctrl/freescale/pinctrl-imx.h | 4 ++++ > > drivers/pinctrl/freescale/pinctrl-vf610.c | 2 ++ > > 3 files changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c > > b/drivers/pinctrl/freescale/pinctrl-imx.c > > index db76e9d..8347197 100644 > > --- a/drivers/pinctrl/freescale/pinctrl-imx.c > > +++ b/drivers/pinctrl/freescale/pinctrl-imx.c > > @@ -197,8 +197,8 @@ static int imx_pmx_set(struct pinctrl_dev *pctldev, > unsigned selector, > > if (info->flags & SHARE_MUX_CONF_REG) { > > u32 reg; > > reg = readl(ipctl->base + pin_reg->mux_reg); > > - reg &= ~(0x7 << 20); > > - reg |= (pin->mux_mode << 20); > > + reg &= ~info->mux_mask; > > + reg |= (pin->mux_mode << info->mux_shift); > > writel(reg, ipctl->base + pin_reg->mux_reg); > > } else { > > writel(pin->mux_mode, ipctl->base + pin_reg->mux_reg); > @@ -288,7 > > +288,7 @@ static int imx_pmx_gpio_request_enable(struct pinctrl_dev > > *pctldev, > > > > mux_pin: > > reg = readl(ipctl->base + pin_reg->mux_reg); > > - reg &= ~(0x7 << 20); > > + reg &= ~info->mux_mask; > > reg |= imx_pin->config; > > writel(reg, ipctl->base + pin_reg->mux_reg); > > > > @@ -432,7 +432,7 @@ static int imx_pinconf_get(struct pinctrl_dev > *pctldev, > > *config = readl(ipctl->base + pin_reg->conf_reg); > > > > if (info->flags & SHARE_MUX_CONF_REG) > > - *config &= 0xffff; > > + *config &= ~info->mux_mask; > > It changes the way how code works. We need a note in the commit log > explaining why it's safe. > Well, that's Vybrid tricks that BIT[15-0] are all configs part. So it hardcoded 0xffff there. But it's not true in ULP, so use mux_mask instead to address the difference. Regards Dong Aisheng > Shawn > > > > > return 0; > > } > > @@ -459,7 +459,7 @@ static int imx_pinconf_set(struct pinctrl_dev > *pctldev, > > if (info->flags & SHARE_MUX_CONF_REG) { > > u32 reg; > > reg = readl(ipctl->base + pin_reg->conf_reg); > > - reg &= ~0xffff; > > + reg &= info->mux_mask; > > reg |= configs[i]; > > writel(reg, ipctl->base + pin_reg->conf_reg); > > } else { > > diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h > > b/drivers/pinctrl/freescale/pinctrl-imx.h > > index b5c8fe1..eb0ce95 100644 > > --- a/drivers/pinctrl/freescale/pinctrl-imx.h > > +++ b/drivers/pinctrl/freescale/pinctrl-imx.h > > @@ -64,6 +64,10 @@ struct imx_pinctrl_soc_info { > > const char *gpr_compatible; > > struct mutex mutex; > > > > + /* MUX_MODE shift and mask in case SHARE_MUX_CONF_REG */ > > + unsigned int mux_mask; > > + u8 mux_shift; > > + > > /* generic pinconf */ > > bool generic_pinconf; > > const struct pinconf_generic_params *custom_params; diff --git > > a/drivers/pinctrl/freescale/pinctrl-vf610.c > > b/drivers/pinctrl/freescale/pinctrl-vf610.c > > index 2b1e198..3bd8556 100644 > > --- a/drivers/pinctrl/freescale/pinctrl-vf610.c > > +++ b/drivers/pinctrl/freescale/pinctrl-vf610.c > > @@ -299,6 +299,8 @@ static struct imx_pinctrl_soc_info > vf610_pinctrl_info = { > > .pins = vf610_pinctrl_pads, > > .npins = ARRAY_SIZE(vf610_pinctrl_pads), > > .flags = SHARE_MUX_CONF_REG | ZERO_OFFSET_VALID, > > + .mux_mask = 0x700000, > > + .mux_shift = 20, > > }; > > > > static const struct of_device_id vf610_pinctrl_of_match[] = { > > -- > > 2.7.4 > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 15, 2017 at 09:00:32AM +0000, A.S. Dong wrote: > > > @@ -432,7 +432,7 @@ static int imx_pinconf_get(struct pinctrl_dev > > *pctldev, > > > *config = readl(ipctl->base + pin_reg->conf_reg); > > > > > > if (info->flags & SHARE_MUX_CONF_REG) > > > - *config &= 0xffff; > > > + *config &= ~info->mux_mask; > > > > It changes the way how code works. We need a note in the commit log > > explaining why it's safe. > > > > Well, that's Vybrid tricks that BIT[15-0] are all configs part. > So it hardcoded 0xffff there. > > But it's not true in ULP, so use mux_mask instead to address > the difference. So you make the assumption that for all SHARE_MUX_CONF_REG SoCs, all bits in the register except mux ones are config bits. You at least need to mention that in the commit log, IMO. Shawn -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Shawn Guo [mailto:shawnguo@kernel.org] > Sent: Monday, May 15, 2017 6:59 PM > To: A.S. Dong > Cc: Andy Duan; Jacky Bai; linus.walleij@linaro.org; stefan@agner.ch; > linux-gpio@vger.kernel.org; kernel@pengutronix.de; linux-arm- > kernel@lists.infradead.org > Subject: Re: [PATCH 2/5] pinctrl: imx: add soc specific mux_mode mask and > shift property > > On Mon, May 15, 2017 at 09:00:32AM +0000, A.S. Dong wrote: > > > > @@ -432,7 +432,7 @@ static int imx_pinconf_get(struct pinctrl_dev > > > *pctldev, > > > > *config = readl(ipctl->base + pin_reg->conf_reg); > > > > > > > > if (info->flags & SHARE_MUX_CONF_REG) > > > > - *config &= 0xffff; > > > > + *config &= ~info->mux_mask; > > > > > > It changes the way how code works. We need a note in the commit log > > > explaining why it's safe. > > > > > > > Well, that's Vybrid tricks that BIT[15-0] are all configs part. > > So it hardcoded 0xffff there. > > > > But it's not true in ULP, so use mux_mask instead to address the > > difference. > > So you make the assumption that for all SHARE_MUX_CONF_REG SoCs, all bits > in the register except mux ones are config bits. You at least need to > mention that in the commit log, IMO. Yes, it's true, at least for all current known SoCs. But your suggestion is good, I can add it in commit log. Thanks Regards Dong Aisheng > > Shawn -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c index db76e9d..8347197 100644 --- a/drivers/pinctrl/freescale/pinctrl-imx.c +++ b/drivers/pinctrl/freescale/pinctrl-imx.c @@ -197,8 +197,8 @@ static int imx_pmx_set(struct pinctrl_dev *pctldev, unsigned selector, if (info->flags & SHARE_MUX_CONF_REG) { u32 reg; reg = readl(ipctl->base + pin_reg->mux_reg); - reg &= ~(0x7 << 20); - reg |= (pin->mux_mode << 20); + reg &= ~info->mux_mask; + reg |= (pin->mux_mode << info->mux_shift); writel(reg, ipctl->base + pin_reg->mux_reg); } else { writel(pin->mux_mode, ipctl->base + pin_reg->mux_reg); @@ -288,7 +288,7 @@ static int imx_pmx_gpio_request_enable(struct pinctrl_dev *pctldev, mux_pin: reg = readl(ipctl->base + pin_reg->mux_reg); - reg &= ~(0x7 << 20); + reg &= ~info->mux_mask; reg |= imx_pin->config; writel(reg, ipctl->base + pin_reg->mux_reg); @@ -432,7 +432,7 @@ static int imx_pinconf_get(struct pinctrl_dev *pctldev, *config = readl(ipctl->base + pin_reg->conf_reg); if (info->flags & SHARE_MUX_CONF_REG) - *config &= 0xffff; + *config &= ~info->mux_mask; return 0; } @@ -459,7 +459,7 @@ static int imx_pinconf_set(struct pinctrl_dev *pctldev, if (info->flags & SHARE_MUX_CONF_REG) { u32 reg; reg = readl(ipctl->base + pin_reg->conf_reg); - reg &= ~0xffff; + reg &= info->mux_mask; reg |= configs[i]; writel(reg, ipctl->base + pin_reg->conf_reg); } else { diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h b/drivers/pinctrl/freescale/pinctrl-imx.h index b5c8fe1..eb0ce95 100644 --- a/drivers/pinctrl/freescale/pinctrl-imx.h +++ b/drivers/pinctrl/freescale/pinctrl-imx.h @@ -64,6 +64,10 @@ struct imx_pinctrl_soc_info { const char *gpr_compatible; struct mutex mutex; + /* MUX_MODE shift and mask in case SHARE_MUX_CONF_REG */ + unsigned int mux_mask; + u8 mux_shift; + /* generic pinconf */ bool generic_pinconf; const struct pinconf_generic_params *custom_params; diff --git a/drivers/pinctrl/freescale/pinctrl-vf610.c b/drivers/pinctrl/freescale/pinctrl-vf610.c index 2b1e198..3bd8556 100644 --- a/drivers/pinctrl/freescale/pinctrl-vf610.c +++ b/drivers/pinctrl/freescale/pinctrl-vf610.c @@ -299,6 +299,8 @@ static struct imx_pinctrl_soc_info vf610_pinctrl_info = { .pins = vf610_pinctrl_pads, .npins = ARRAY_SIZE(vf610_pinctrl_pads), .flags = SHARE_MUX_CONF_REG | ZERO_OFFSET_VALID, + .mux_mask = 0x700000, + .mux_shift = 20, }; static const struct of_device_id vf610_pinctrl_of_match[] = {