[v3] ARM: imx: pinctrl-imx: imx7d: add support for iomuxc lpsr
diff mbox

Message ID 1436295725-19011-1-git-send-email-aalonso@freescale.com
State New
Headers show

Commit Message

Adrian Alonso July 7, 2015, 7:02 p.m. UTC
* Extend pinctrl-imx driver to support iomux lpsr conntroller,
* iMX7D has two iomuxc controllers, iomuxc controller similar as
  previous iMX SoC generation and iomuxc-lpsr which provides
  low power state rentetion capabilities on gpios that are part of
  iomuxc-lpsr (GPIO1_IO7..GPIO1_IO0).
* Use IOMUXC_LPSR_SUPPORT and iput_val most significant bits to
  properly configure iomuxc/iomuxc-lpsr settings.

Signed-off-by: Adrian Alonso <aalonso@freescale.com>

* Change from v1 to v2:
  - Add suggested comment for input select register shared between
    iomuxc-lpsr and normal iomuxc controller.
  - Use IOMUXC_LPSR_MASK to extract pad group id and aling pin_id to
    16 bit representation.
* Change from v2 to v3
  - Use devm_ioremap_resource instead of of_iomap to get iomuxc-lpsr
    base register address.
---
 drivers/pinctrl/freescale/pinctrl-imx.c | 72 ++++++++++++++++++++++-----------
 drivers/pinctrl/freescale/pinctrl-imx.h |  7 +++-
 2 files changed, 55 insertions(+), 24 deletions(-)

Comments

Linus Walleij July 14, 2015, 9:06 a.m. UTC | #1
On Tue, Jul 7, 2015 at 9:02 PM, Adrian Alonso <aalonso@freescale.com> wrote:

> * Extend pinctrl-imx driver to support iomux lpsr conntroller,
> * iMX7D has two iomuxc controllers, iomuxc controller similar as
>   previous iMX SoC generation and iomuxc-lpsr which provides
>   low power state rentetion capabilities on gpios that are part of
>   iomuxc-lpsr (GPIO1_IO7..GPIO1_IO0).
> * Use IOMUXC_LPSR_SUPPORT and iput_val most significant bits to
>   properly configure iomuxc/iomuxc-lpsr settings.
>
> Signed-off-by: Adrian Alonso <aalonso@freescale.com>
>
> * Change from v1 to v2:
>   - Add suggested comment for input select register shared between
>     iomuxc-lpsr and normal iomuxc controller.
>   - Use IOMUXC_LPSR_MASK to extract pad group id and aling pin_id to
>     16 bit representation.
> * Change from v2 to v3
>   - Use devm_ioremap_resource instead of of_iomap to get iomuxc-lpsr
>     base register address.

I'm waiting for Shawn to review these patches.

Yours,
Linus Walleij
--
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
Shawn Guo July 15, 2015, 7:23 a.m. UTC | #2
On Tue, Jul 07, 2015 at 02:02:05PM -0500, Adrian Alonso wrote:
> * Extend pinctrl-imx driver to support iomux lpsr conntroller,
> * iMX7D has two iomuxc controllers, iomuxc controller similar as
>   previous iMX SoC generation and iomuxc-lpsr which provides
>   low power state rentetion capabilities on gpios that are part of
>   iomuxc-lpsr (GPIO1_IO7..GPIO1_IO0).
> * Use IOMUXC_LPSR_SUPPORT and iput_val most significant bits to
>   properly configure iomuxc/iomuxc-lpsr settings.
> 
> Signed-off-by: Adrian Alonso <aalonso@freescale.com>

It took me quite some time to understand what the patch does.  Before I
gave specific comments on your implementation, I would discuss if there
is a better solution, as I do not like the idea of encoding these
artificial pin id of LPSR pads in the input_val.

Ideally, the LPSR controller should be implemented as a second instance
of IOMUXC.  But the problem seems to be the select input register is
shared between these two instances.  Is my understanding correct?

How select input register is shared?  With different bits in a single
register which is only laid on normal IOMUXC controller?

I need more details to understand the problem.

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
Zhi Li July 15, 2015, 1:57 p.m. UTC | #3
On Wed, Jul 15, 2015 at 2:23 AM, Shawn Guo <shawnguo@kernel.org> wrote:
> On Tue, Jul 07, 2015 at 02:02:05PM -0500, Adrian Alonso wrote:
>> * Extend pinctrl-imx driver to support iomux lpsr conntroller,
>> * iMX7D has two iomuxc controllers, iomuxc controller similar as
>>   previous iMX SoC generation and iomuxc-lpsr which provides
>>   low power state rentetion capabilities on gpios that are part of
>>   iomuxc-lpsr (GPIO1_IO7..GPIO1_IO0).
>> * Use IOMUXC_LPSR_SUPPORT and iput_val most significant bits to
>>   properly configure iomuxc/iomuxc-lpsr settings.
>>
>> Signed-off-by: Adrian Alonso <aalonso@freescale.com>
>
> It took me quite some time to understand what the patch does.  Before I
> gave specific comments on your implementation, I would discuss if there
> is a better solution, as I do not like the idea of encoding these
> artificial pin id of LPSR pads in the input_val.
>
> Ideally, the LPSR controller should be implemented as a second instance
> of IOMUXC.  But the problem seems to be the select input register is
> shared between these two instances.  Is my understanding correct?

There are two problem.
1. LPSR IOMUX share input select with IOMUX.
2. If use two instance,  the pin ID will be the same.  If there are a group
mix use LPSR IOMUX and normal IOMUX,  system will be confused.
For example:

UART_pin:
{
          LPSR_IOMUX_RX,
          NORMAL_IOMUX_TX,
}

IOMUX driver don't distinguish it.

best regards
Frank Li

>
> How select input register is shared?  With different bits in a single
> register which is only laid on normal IOMUXC controller?
>
> I need more details to understand the problem.
>
> 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
Adrian Alonso July 15, 2015, 3:55 p.m. UTC | #4
Hi Shawn,

Comments inline, thanks for reviewing :)

Regards
Adrian

-----Original Message-----
From: Shawn Guo [mailto:shawnguo@kernel.org] 
Sent: Wednesday, July 15, 2015 2:23 AM
To: Alonso Lazcano Adrian-B38018
Cc: linux-arm-kernel@lists.infradead.org; shawn.guo@linaro.org; linus.walleij@linaro.org; lznuaa@gmail.com; devicetree@vger.kernel.org; Li Frank-B20596; Garg Nitin-B37173; Huang Yongcai-B20788; linux-gpio@vger.kernel.org; robh+dt@kernel.org; Gong Yibin-B38343
Subject: Re: [PATCH][v3] ARM: imx: pinctrl-imx: imx7d: add support for iomuxc lpsr

On Tue, Jul 07, 2015 at 02:02:05PM -0500, Adrian Alonso wrote:
> * Extend pinctrl-imx driver to support iomux lpsr conntroller,
> * iMX7D has two iomuxc controllers, iomuxc controller similar as
>   previous iMX SoC generation and iomuxc-lpsr which provides
>   low power state rentetion capabilities on gpios that are part of
>   iomuxc-lpsr (GPIO1_IO7..GPIO1_IO0).
> * Use IOMUXC_LPSR_SUPPORT and iput_val most significant bits to
>   properly configure iomuxc/iomuxc-lpsr settings.
> 
> Signed-off-by: Adrian Alonso <aalonso@freescale.com>

It took me quite some time to understand what the patch does.  Before I gave specific comments on your implementation, I would discuss if there is a better solution, as I do not like the idea of encoding these artificial pin id of LPSR pads in the input_val.

Ideally, the LPSR controller should be implemented as a second instance of IOMUXC.  But the problem seems to be the select input register is shared between these two instances.  Is my understanding correct?

[Adrian] Yes that's the case SELECT_INPUT register is shared between IOMUXC and IOMUXC-LPSR controllers.

How select input register is shared?  With different bits in a single register which is only laid on normal IOMUXC controller?

[Adrian] IOMUXC and IOMUXC-LPSR are in different base address; each controller provides SW_PAD_CTL  (CONF_REG) and SW_MUX_CTL (MUX_REG) but only IOMUXC provides SELECT_INPUT registers for pad daisy chain configuration, so pads from IOMUXC-LPSR that need daisy chain settings need to access the IOMUXC register space address to access SELECT_INPUT.

[Adrian] One disadvantage that we found if we created two driver instances for IOMUXC and IOMUXC-LPSR controllers is that in the device tree machine files "end-users" could be creating pinctrl nodes mixing pads from different IOMUXC controllers resulting that pad that doesn't belong to that domain controller it will not be configured properly.

For example a valid pad configuration for I2C1 could use pads as shown below: 
&iomuxc {
	pinctrl_i2c1_1: i2c1grp-1 {
		fsl,pins = <
			MX7D_PAD_GPIO1_IO04__I2C1_SCL  	/* IOMUXC-LPSR domain */
			MX7D_PAD_I2C1_SDA__I2C1_SDA   	/* IOMUXC domain */
		>;
	};
};

By extending pinctrl-imx to support both controllers the above configuration is supported,
Otherwise using two driver instances "end-users" will need to do something like:

&iomuxc {
	pinctrl_i2c1_1: i2c1grp-1 {
		fsl,pins = <
			MX7D_PAD_I2C1_SDA__I2C1_SDA   	/* IOMUXC domain */
		>;
	};
};

&iomuxc_lpsr {
	pinctrl_i2c1_1: i2c1grp-1 {
		fsl,pins = <
			MX7D_PAD_GPIO1_IO04__I2C1_SCL  	/* IOMUXC-LPSR domain */
		>;
	};
};

And this is something that we will like to avoid so "end-user" don't have to worry about and configure pads as they are used to.

[Adrian] For the artificial encoding of pad group id's for IOMUXC-LPSR, I think there is no other way to do it (limited imagination here :P);
Pad group id's are extracted from MUX_REG offset address (pad_id = mux_reg / 4) but when combining both IOMUXC and IOMUXC-LPSR
Pad group ID's overlap so end up encoding the pad_id for IOMUXC-LPSR to resolve the proper pad group ID.

I need more details to understand the problem.

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
Shawn Guo July 16, 2015, 3:50 p.m. UTC | #5
Adrian,

Please configure your email client to use bottom-posting rather than
top-posting.

On Wed, Jul 15, 2015 at 03:55:59PM +0000, Alonso Adrian wrote:
> [Adrian] IOMUXC and IOMUXC-LPSR are in different base address; each controller provides SW_PAD_CTL  (CONF_REG) and SW_MUX_CTL (MUX_REG) but only IOMUXC provides SELECT_INPUT registers for pad daisy chain configuration, so pads from IOMUXC-LPSR that need daisy chain settings need to access the IOMUXC register space address to access SELECT_INPUT.
> 

Thanks for the explanation.

> [Adrian] One disadvantage that we found if we created two driver instances for IOMUXC and IOMUXC-LPSR controllers is that in the device tree machine files "end-users" could be creating pinctrl nodes mixing pads from different IOMUXC controllers resulting that pad that doesn't belong to that domain controller it will not be configured properly.
> 
> For example a valid pad configuration for I2C1 could use pads as shown below: 
> &iomuxc {
> 	pinctrl_i2c1_1: i2c1grp-1 {
> 		fsl,pins = <
> 			MX7D_PAD_GPIO1_IO04__I2C1_SCL  	/* IOMUXC-LPSR domain */
> 			MX7D_PAD_I2C1_SDA__I2C1_SDA   	/* IOMUXC domain */
> 		>;
> 	};
> };
> 
> By extending pinctrl-imx to support both controllers the above configuration is supported,
> Otherwise using two driver instances "end-users" will need to do something like:
> 
> &iomuxc {
> 	pinctrl_i2c1_1: i2c1grp-1 {
> 		fsl,pins = <
> 			MX7D_PAD_I2C1_SDA__I2C1_SDA   	/* IOMUXC domain */
> 		>;
> 	};
> };
> 
> &iomuxc_lpsr {
> 	pinctrl_i2c1_1: i2c1grp-1 {
> 		fsl,pins = <
> 			MX7D_PAD_GPIO1_IO04__I2C1_SCL  	/* IOMUXC-LPSR domain */
> 		>;
> 	};
> };
> 
> And this is something that we will like to avoid so "end-user" don't have to worry about and configure pads as they are used to.
> 

This is exactly how hardware is designed, and having device tree
describe how hardware is laid out is the right thing to do.  In that
case, when people check the device tree source with a i.MX7D Reference
Manual on hands, they can easily understand how hardware is represented
in device tree.

Also, your example with pins for a single device across IOMUXC and
IOMUXC-LPSR should be the rare case except GPIO pins.  For your I2C1
example, a sane hardware design would chose one group among the
following instead of the one you put above.

  MX7D_PAD_I2C1_SCL__I2C1_SCL
  MX7D_PAD_I2C1_SDA__I2C1_SDA

  MX7D_PAD_UART1_RX_DATA__I2C1_SCL
  MX7D_PAD_UART1_TX_DATA__I2C1_SDA

  MX7D_PAD_GPIO1_IO04__I2C1_SCL
  MX7D_PAD_GPIO1_IO05__I2C1_SDA

So this is not really a problem for us to implement two IOMUXC instances.
The only problem we need to solve is the select input register access
for IOMUXC-LPSR driver.  I have some idea for that, i.e. honestly
represent how hardware is laid out.  I will give more details on that
tomorrow.

> [Adrian] For the artificial encoding of pad group id's for IOMUXC-LPSR, I think there is no other way to do it (limited imagination here :P);
> Pad group id's are extracted from MUX_REG offset address (pad_id = mux_reg / 4) but when combining both IOMUXC and IOMUXC-LPSR
> Pad group ID's overlap so end up encoding the pad_id for IOMUXC-LPSR to resolve the proper pad group ID.

One big missing piece from your patch is the update to bindings document
Documentation/devicetree/bindings/pinctrl/fsl,imx-pinctrl.txt.  Have you
thought about how you will document all these magics and hacks you have
done here?

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
Zhi Li July 16, 2015, 4:22 p.m. UTC | #6
On Thu, Jul 16, 2015 at 10:50 AM, Shawn Guo <shawnguo@kernel.org> wrote:
> Adrian,
>
> Please configure your email client to use bottom-posting rather than
> top-posting.
>
> On Wed, Jul 15, 2015 at 03:55:59PM +0000, Alonso Adrian wrote:
>> [Adrian] IOMUXC and IOMUXC-LPSR are in different base address; each controller provides SW_PAD_CTL  (CONF_REG) and SW_MUX_CTL (MUX_REG) but only IOMUXC provides SELECT_INPUT registers for pad daisy chain configuration, so pads from IOMUXC-LPSR that need daisy chain settings need to access the IOMUXC register space address to access SELECT_INPUT.
>>
>
> Thanks for the explanation.
>
>> [Adrian] One disadvantage that we found if we created two driver instances for IOMUXC and IOMUXC-LPSR controllers is that in the device tree machine files "end-users" could be creating pinctrl nodes mixing pads from different IOMUXC controllers resulting that pad that doesn't belong to that domain controller it will not be configured properly.
>>
>> For example a valid pad configuration for I2C1 could use pads as shown below:
>> &iomuxc {
>>       pinctrl_i2c1_1: i2c1grp-1 {
>>               fsl,pins = <
>>                       MX7D_PAD_GPIO1_IO04__I2C1_SCL   /* IOMUXC-LPSR domain */
>>                       MX7D_PAD_I2C1_SDA__I2C1_SDA     /* IOMUXC domain */
>>               >;
>>       };
>> };
>>
>> By extending pinctrl-imx to support both controllers the above configuration is supported,
>> Otherwise using two driver instances "end-users" will need to do something like:
>>
>> &iomuxc {
>>       pinctrl_i2c1_1: i2c1grp-1 {
>>               fsl,pins = <
>>                       MX7D_PAD_I2C1_SDA__I2C1_SDA     /* IOMUXC domain */
>>               >;
>>       };
>> };
>>
>> &iomuxc_lpsr {
>>       pinctrl_i2c1_1: i2c1grp-1 {
>>               fsl,pins = <
>>                       MX7D_PAD_GPIO1_IO04__I2C1_SCL   /* IOMUXC-LPSR domain */
>>               >;
>>       };
>> };
>>
>> And this is something that we will like to avoid so "end-user" don't have to worry about and configure pads as they are used to.
>>
>
> This is exactly how hardware is designed, and having device tree
> describe how hardware is laid out is the right thing to do.  In that
> case, when people check the device tree source with a i.MX7D Reference
> Manual on hands, they can easily understand how hardware is represented
> in device tree.
>
> Also, your example with pins for a single device across IOMUXC and
> IOMUXC-LPSR should be the rare case except GPIO pins.

Although cross IOMUX and IOMUXC-LPSR is rare,  it will be headache if there are
one boards.

Some SD card, ENET have reset\wp\cd pin.

pinctrl_usdhc1: usdhc1grp {
                        fsl,pins = <
                                MX7D_PAD_SD1_CMD__SD1_CMD               0x59
                                MX7D_PAD_SD1_CLK__SD1_CLK               0x19
                                MX7D_PAD_SD1_DATA0__SD1_DATA0           0x59
                                MX7D_PAD_SD1_DATA1__SD1_DATA1           0x59
                                MX7D_PAD_SD1_DATA2__SD1_DATA2           0x59
                                MX7D_PAD_SD1_DATA3__SD1_DATA3           0x59
                                MX7D_PAD_SD1_CD_B__GPIO5_IO0
 0x59 /* CD */
                                MX7D_PAD_SD1_WP__GPIO5_IO1
 0x59 /* WP */
                                MX7D_PAD_SD1_RESET_B__GPIO5_IO2
 0x59 /* vmmc */
                        >;

Customer may choose below pin to IOMUX-lPSR.

MX7D_PAD_SD1_CD_B__GPIO5_IO0
MX7D_PAD_SD1_WP__GPIO5_IO1
MX7D_PAD_SD1_RESET_B__GPIO5_IO2

FEC have Phy Reset GPIO, which possibly choose IOMUX-lPSR.



> For your I2C1
> example, a sane hardware design would chose one group among the
> following instead of the one you put above.
>
>   MX7D_PAD_I2C1_SCL__I2C1_SCL
>   MX7D_PAD_I2C1_SDA__I2C1_SDA
>
>   MX7D_PAD_UART1_RX_DATA__I2C1_SCL
>   MX7D_PAD_UART1_TX_DATA__I2C1_SDA
>
>   MX7D_PAD_GPIO1_IO04__I2C1_SCL
>   MX7D_PAD_GPIO1_IO05__I2C1_SDA
>
> So this is not really a problem for us to implement two IOMUXC instances.
> The only problem we need to solve is the select input register access
> for IOMUXC-LPSR driver.  I have some idea for that, i.e. honestly
> represent how hardware is laid out.  I will give more details on that
> tomorrow.

It is very easy to make mistake.
According to pin NAME macro, it is not easy to distinguish between
IOMUX and IOMUX-LPSR.

Internal kernel tree use two pinctrl instance.

It will be simple if we think IOMUX and IOMUX-LPSR together, and one
module have two group registers.

best regards
Frank Li

>
>> [Adrian] For the artificial encoding of pad group id's for IOMUXC-LPSR, I think there is no other way to do it (limited imagination here :P);
>> Pad group id's are extracted from MUX_REG offset address (pad_id = mux_reg / 4) but when combining both IOMUXC and IOMUXC-LPSR
>> Pad group ID's overlap so end up encoding the pad_id for IOMUXC-LPSR to resolve the proper pad group ID.
>
> One big missing piece from your patch is the update to bindings document
> Documentation/devicetree/bindings/pinctrl/fsl,imx-pinctrl.txt.  Have you
> thought about how you will document all these magics and hacks you have
> done here?
>
> 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
Adrian Alonso July 16, 2015, 8:39 p.m. UTC | #7
Hi All,

I will sent a new patch series including documentation for pad group id encoding in
Documentation/devicetree/bindings/pinctrl/fsl,imx7d-pinctrl.txt

Thanks for your comments :)
-
Adrian

> -----Original Message-----

> From: Zhi Li [mailto:lznuaa@gmail.com]

> Sent: Thursday, July 16, 2015 11:22 AM

> To: Shawn Guo

> Cc: Alonso Lazcano Adrian-B38018; devicetree@vger.kernel.org; Li Frank-

> B20596; Garg Nitin-B37173; linus.walleij@linaro.org; linux-

> gpio@vger.kernel.org; robh+dt@kernel.org; shawn.guo@linaro.org; Huang

> Yongcai-B20788; linux-arm-kernel@lists.infradead.org; Gong Yibin-B38343

> Subject: Re: [PATCH][v3] ARM: imx: pinctrl-imx: imx7d: add support for iomuxc

> lpsr

> 

> On Thu, Jul 16, 2015 at 10:50 AM, Shawn Guo <shawnguo@kernel.org> wrote:

> > Adrian,

> >

> > Please configure your email client to use bottom-posting rather than

> > top-posting.

> >

> > On Wed, Jul 15, 2015 at 03:55:59PM +0000, Alonso Adrian wrote:

> >> [Adrian] IOMUXC and IOMUXC-LPSR are in different base address; each

> controller provides SW_PAD_CTL  (CONF_REG) and SW_MUX_CTL (MUX_REG)

> but only IOMUXC provides SELECT_INPUT registers for pad daisy chain

> configuration, so pads from IOMUXC-LPSR that need daisy chain settings need

> to access the IOMUXC register space address to access SELECT_INPUT.

> >>

> >

> > Thanks for the explanation.

> >

> >> [Adrian] One disadvantage that we found if we created two driver instances

> for IOMUXC and IOMUXC-LPSR controllers is that in the device tree machine

> files "end-users" could be creating pinctrl nodes mixing pads from different

> IOMUXC controllers resulting that pad that doesn't belong to that domain

> controller it will not be configured properly.

> >>

> >> For example a valid pad configuration for I2C1 could use pads as shown

> below:

> >> &iomuxc {

> >>       pinctrl_i2c1_1: i2c1grp-1 {

> >>               fsl,pins = <

> >>                       MX7D_PAD_GPIO1_IO04__I2C1_SCL   /* IOMUXC-LPSR domain

> */

> >>                       MX7D_PAD_I2C1_SDA__I2C1_SDA     /* IOMUXC domain */

> >>               >;

> >>       };

> >> };

> >>

> >> By extending pinctrl-imx to support both controllers the above

> >> configuration is supported, Otherwise using two driver instances "end-

> users" will need to do something like:

> >>

> >> &iomuxc {

> >>       pinctrl_i2c1_1: i2c1grp-1 {

> >>               fsl,pins = <

> >>                       MX7D_PAD_I2C1_SDA__I2C1_SDA     /* IOMUXC domain */

> >>               >;

> >>       };

> >> };

> >>

> >> &iomuxc_lpsr {

> >>       pinctrl_i2c1_1: i2c1grp-1 {

> >>               fsl,pins = <

> >>                       MX7D_PAD_GPIO1_IO04__I2C1_SCL   /* IOMUXC-LPSR domain

> */

> >>               >;

> >>       };

> >> };

> >>

> >> And this is something that we will like to avoid so "end-user" don't have to

> worry about and configure pads as they are used to.

> >>

> >

> > This is exactly how hardware is designed, and having device tree

> > describe how hardware is laid out is the right thing to do.  In that

> > case, when people check the device tree source with a i.MX7D Reference

> > Manual on hands, they can easily understand how hardware is

> > represented in device tree.

> >

> > Also, your example with pins for a single device across IOMUXC and

> > IOMUXC-LPSR should be the rare case except GPIO pins.

> 

> Although cross IOMUX and IOMUXC-LPSR is rare,  it will be headache if there

> are one boards.

> 

> Some SD card, ENET have reset\wp\cd pin.

> 

> pinctrl_usdhc1: usdhc1grp {

>                         fsl,pins = <

>                                 MX7D_PAD_SD1_CMD__SD1_CMD               0x59

>                                 MX7D_PAD_SD1_CLK__SD1_CLK               0x19

>                                 MX7D_PAD_SD1_DATA0__SD1_DATA0           0x59

>                                 MX7D_PAD_SD1_DATA1__SD1_DATA1           0x59

>                                 MX7D_PAD_SD1_DATA2__SD1_DATA2           0x59

>                                 MX7D_PAD_SD1_DATA3__SD1_DATA3           0x59

>                                 MX7D_PAD_SD1_CD_B__GPIO5_IO0

>  0x59 /* CD */

>                                 MX7D_PAD_SD1_WP__GPIO5_IO1

>  0x59 /* WP */

>                                 MX7D_PAD_SD1_RESET_B__GPIO5_IO2

>  0x59 /* vmmc */

>                         >;

> 

> Customer may choose below pin to IOMUX-lPSR.

> 

> MX7D_PAD_SD1_CD_B__GPIO5_IO0

> MX7D_PAD_SD1_WP__GPIO5_IO1

> MX7D_PAD_SD1_RESET_B__GPIO5_IO2

> 

> FEC have Phy Reset GPIO, which possibly choose IOMUX-lPSR.

> 

> 

> 

> > For your I2C1

> > example, a sane hardware design would chose one group among the

> > following instead of the one you put above.

> >

> >   MX7D_PAD_I2C1_SCL__I2C1_SCL

> >   MX7D_PAD_I2C1_SDA__I2C1_SDA

> >

> >   MX7D_PAD_UART1_RX_DATA__I2C1_SCL

> >   MX7D_PAD_UART1_TX_DATA__I2C1_SDA

> >

> >   MX7D_PAD_GPIO1_IO04__I2C1_SCL

> >   MX7D_PAD_GPIO1_IO05__I2C1_SDA

> >

> > So this is not really a problem for us to implement two IOMUXC instances.

> > The only problem we need to solve is the select input register access

> > for IOMUXC-LPSR driver.  I have some idea for that, i.e. honestly

> > represent how hardware is laid out.  I will give more details on that

> > tomorrow.

> 

> It is very easy to make mistake.

> According to pin NAME macro, it is not easy to distinguish between IOMUX and

> IOMUX-LPSR.

> 

> Internal kernel tree use two pinctrl instance.

> 

> It will be simple if we think IOMUX and IOMUX-LPSR together, and one module

> have two group registers.

> 

> best regards

> Frank Li

> 

> >

> >> [Adrian] For the artificial encoding of pad group id's for

> >> IOMUXC-LPSR, I think there is no other way to do it (limited

> >> imagination here :P); Pad group id's are extracted from MUX_REG offset

> address (pad_id = mux_reg / 4) but when combining both IOMUXC and

> IOMUXC-LPSR Pad group ID's overlap so end up encoding the pad_id for

> IOMUXC-LPSR to resolve the proper pad group ID.

> >

> > One big missing piece from your patch is the update to bindings

> > document

> > Documentation/devicetree/bindings/pinctrl/fsl,imx-pinctrl.txt.  Have

> > you thought about how you will document all these magics and hacks you

> have done here?

> >

> > Shawn
Shawn Guo July 17, 2015, 3:31 a.m. UTC | #8
On Thu, Jul 16, 2015 at 11:22:27AM -0500, Zhi Li wrote:
> On Thu, Jul 16, 2015 at 10:50 AM, Shawn Guo <shawnguo@kernel.org> wrote:
> Although cross IOMUX and IOMUXC-LPSR is rare,  it will be headache if there are
> one boards.
> 
> Some SD card, ENET have reset\wp\cd pin.
> 
> pinctrl_usdhc1: usdhc1grp {
>                         fsl,pins = <
>                                 MX7D_PAD_SD1_CMD__SD1_CMD               0x59
>                                 MX7D_PAD_SD1_CLK__SD1_CLK               0x19
>                                 MX7D_PAD_SD1_DATA0__SD1_DATA0           0x59
>                                 MX7D_PAD_SD1_DATA1__SD1_DATA1           0x59
>                                 MX7D_PAD_SD1_DATA2__SD1_DATA2           0x59
>                                 MX7D_PAD_SD1_DATA3__SD1_DATA3           0x59
>                                 MX7D_PAD_SD1_CD_B__GPIO5_IO0
>  0x59 /* CD */
>                                 MX7D_PAD_SD1_WP__GPIO5_IO1
>  0x59 /* WP */
>                                 MX7D_PAD_SD1_RESET_B__GPIO5_IO2
>  0x59 /* vmmc */
>                         >;
> 
> Customer may choose below pin to IOMUX-lPSR.
> 
> MX7D_PAD_SD1_CD_B__GPIO5_IO0
> MX7D_PAD_SD1_WP__GPIO5_IO1
> MX7D_PAD_SD1_RESET_B__GPIO5_IO2
> 
> FEC have Phy Reset GPIO, which possibly choose IOMUX-lPSR.

I do not understand why it would be a headache.  The following is what I
have in my head, and I think it's nicer and easier to implement.  Most
importantly, this maps how hardware is laid out exactly, save us all
those magics and hacks.

iomuxc: iomuxc@30330000 {
	compatible = "fsl,imx7d-iomuxc";
	reg = <0x30330000 0x10000>;
};

iomuxc_lpsr: iomuxc-lpsr@302c0000 {
	compatible = "fsl,imx7d-iomuxc-lpsr";
	reg = <0x302c0000 0x10000>;
	fsl,select-input-controller = <&iomuxc>;
};

&iomuxc {
	pinctrl_usdhc1: usdhc1grp {
		fsl,pins = <
			MX7D_PAD_SD1_CMD__SD1_CMD	0x59
			MX7D_PAD_SD1_CLK__SD1_CLK	0x19
			MX7D_PAD_SD1_DATA0__SD1_DATA0	0x59
			MX7D_PAD_SD1_DATA1__SD1_DATA1	0x59
			MX7D_PAD_SD1_DATA2__SD1_DATA2	0x59
			MX7D_PAD_SD1_DATA3__SD1_DATA3	0x59
		>;
	};
};

&iomuxc_lpsr {
	pinctrl_usdhc1_lpsr: usdhc1lpsrgrp {
		fsl,pins = <
			MX7D_PAD_LPSR_GPIO1_IO0__GPIO1_IO0  0x59 /* CD */
			MX7D_PAD_LPSR_GPIO1_IO1__GPIO1_IO1  0x59 /* WP */
			MX7D_PAD_LPSR_GPIO1_IO2__GPIO1_IO2  0x59 /* vmmc */
		>;
	};
};

&usdhc1 {
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_usdhc1 &pinctrl_usdhc1_lpsr>;
};

> It is very easy to make mistake.
> According to pin NAME macro, it is not easy to distinguish between
> IOMUX and IOMUX-LPSR.

That's easy to resolve.  As I demonstrated above, having LPSR in the
macro names and putting all those LPSR pad macros in a separate header
like imx7d-lpsr-pinfunc.h would make it easy to remember.

> Internal kernel tree use two pinctrl instance.

I think this is the right approach.  You should post that solution
for upstream instead.

> It will be simple if we think IOMUX and IOMUX-LPSR together, and one
> module have two group registers.

No, they are two instances of IOMUX controller from perspective of
hardware design.  And Linux pinctrl subsystem is well-designed to fit
there.

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
Shawn Guo July 17, 2015, 3:34 a.m. UTC | #9
On Thu, Jul 16, 2015 at 08:39:33PM +0000, Alonso Adrian wrote:
> Hi All,
> 
> I will sent a new patch series including documentation for pad group id encoding in
> Documentation/devicetree/bindings/pinctrl/fsl,imx7d-pinctrl.txt
> 
> Thanks for your comments :)

Do not get me wrong.  The problem with the patch is not merely missing
the document update.

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
Adrian Alonso July 23, 2015, 7:34 p.m. UTC | #10
Hi Shawn,

The approach you mention is already supported in FSL kernel tree see
http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/commit/?h=imx_3.14.38_6ul7d_beta&id=a37ea06ddccbefb8660af0ac259c30d98540bdc2

I will rework and proposed a new patch series soon.

Thanks for your reviews :)

Regards
Adrian

> -----Original Message-----
> From: linux-gpio-owner@vger.kernel.org [mailto:linux-gpio-
> owner@vger.kernel.org] On Behalf Of Shawn Guo
> Sent: Thursday, July 16, 2015 10:34 PM
> To: Alonso Lazcano Adrian-B38018
> Cc: Zhi Li; devicetree@vger.kernel.org; Li Frank-B20596; Garg Nitin-B37173;
> linus.walleij@linaro.org; linux-gpio@vger.kernel.org; robh+dt@kernel.org;
> shawn.guo@linaro.org; Huang Yongcai-B20788; linux-arm-
> kernel@lists.infradead.org; Gong Yibin-B38343
> Subject: Re: [PATCH][v3] ARM: imx: pinctrl-imx: imx7d: add support for iomuxc
> lpsr
> 
> On Thu, Jul 16, 2015 at 08:39:33PM +0000, Alonso Adrian wrote:
> > Hi All,
> >
> > I will sent a new patch series including documentation for pad group
> > id encoding in
> > Documentation/devicetree/bindings/pinctrl/fsl,imx7d-pinctrl.txt
> >
> > Thanks for your comments :)
> 
> Do not get me wrong.  The problem with the patch is not merely missing the
> document update.
> 
> 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
--
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

Patch
diff mbox

diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
index d7b98ba..aef4ca3 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -1,7 +1,7 @@ 
 /*
  * Core driver for the imx pin controller
  *
- * Copyright (C) 2012 Freescale Semiconductor, Inc.
+ * Copyright (C) 2012-2015 Freescale Semiconductor, Inc.
  * Copyright (C) 2012 Linaro Ltd.
  *
  * Author: Dong Aisheng <dong.aisheng@linaro.org>
@@ -38,7 +38,6 @@ 
 struct imx_pinctrl {
 	struct device *dev;
 	struct pinctrl_dev *pctl;
-	void __iomem *base;
 	const struct imx_pinctrl_soc_info *info;
 };
 
@@ -212,12 +211,12 @@  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 = readl(pin_reg->base + pin_reg->mux_reg);
 			reg &= ~(0x7 << 20);
 			reg |= (pin->mux_mode << 20);
-			writel(reg, ipctl->base + pin_reg->mux_reg);
+			writel(reg, pin_reg->base + pin_reg->mux_reg);
 		} else {
-			writel(pin->mux_mode, ipctl->base + pin_reg->mux_reg);
+			writel(pin->mux_mode, pin_reg->base + pin_reg->mux_reg);
 		}
 		dev_dbg(ipctl->dev, "write: offset 0x%x val 0x%x\n",
 			pin_reg->mux_reg, pin->mux_mode);
@@ -245,16 +244,22 @@  static int imx_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
 			 * The input_reg[i] here is actually some IOMUXC general
 			 * purpose register, not regular select input register.
 			 */
-			val = readl(ipctl->base + pin->input_reg);
+			val = readl(pin_reg->base + pin->input_reg);
 			val &= ~mask;
 			val |= select << shift;
-			writel(val, ipctl->base + pin->input_reg);
+			writel(val, pin_reg->base + pin->input_reg);
 		} else if (pin->input_reg) {
 			/*
 			 * Regular select input register can never be at offset
 			 * 0, and we only print register value for regular case.
 			 */
-			writel(pin->input_val, ipctl->base + pin->input_reg);
+			if (info->flags & IOMUXC_LPSR_SUPPORT &&
+					IOMUXC_LPSR_MASK(pin->input_val))
+			/* iomuxc-lpsr select input register shared with normal iomuxc */
+				writel(pin->input_val, info->base + pin->input_reg);
+			else
+				writel(pin->input_val, pin_reg->base + pin->input_reg);
+
 			dev_dbg(ipctl->dev,
 				"==>select_input: offset 0x%x val 0x%x\n",
 				pin->input_reg, pin->input_val);
@@ -326,10 +331,10 @@  static int imx_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
 	return -EINVAL;
 
 mux_pin:
-	reg = readl(ipctl->base + pin_reg->mux_reg);
+	reg = readl(pin_reg->base + pin_reg->mux_reg);
 	reg &= ~(0x7 << 20);
 	reg |= imx_pin->config;
-	writel(reg, ipctl->base + pin_reg->mux_reg);
+	writel(reg, pin_reg->base + pin_reg->mux_reg);
 
 	return 0;
 }
@@ -354,12 +359,12 @@  static int imx_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
 		return -EINVAL;
 
 	/* IBE always enabled allows us to read the value "on the wire" */
-	reg = readl(ipctl->base + pin_reg->mux_reg);
+	reg = readl(pin_reg->base + pin_reg->mux_reg);
 	if (input)
 		reg &= ~0x2;
 	else
 		reg |= 0x2;
-	writel(reg, ipctl->base + pin_reg->mux_reg);
+	writel(reg, pin_reg->base + pin_reg->mux_reg);
 
 	return 0;
 }
@@ -386,7 +391,7 @@  static int imx_pinconf_get(struct pinctrl_dev *pctldev,
 		return -EINVAL;
 	}
 
-	*config = readl(ipctl->base + pin_reg->conf_reg);
+	*config = readl(pin_reg->base + pin_reg->conf_reg);
 
 	if (info->flags & SHARE_MUX_CONF_REG)
 		*config &= 0xffff;
@@ -415,12 +420,12 @@  static int imx_pinconf_set(struct pinctrl_dev *pctldev,
 	for (i = 0; i < num_configs; i++) {
 		if (info->flags & SHARE_MUX_CONF_REG) {
 			u32 reg;
-			reg = readl(ipctl->base + pin_reg->conf_reg);
+			reg = readl(pin_reg->base + pin_reg->conf_reg);
 			reg &= ~0xffff;
 			reg |= configs[i];
-			writel(reg, ipctl->base + pin_reg->conf_reg);
+			writel(reg, pin_reg->base + pin_reg->conf_reg);
 		} else {
-			writel(configs[i], ipctl->base + pin_reg->conf_reg);
+			writel(configs[i], pin_reg->base + pin_reg->conf_reg);
 		}
 		dev_dbg(ipctl->dev, "write: offset 0x%x val 0x%lx\n",
 			pin_reg->conf_reg, configs[i]);
@@ -442,7 +447,7 @@  static void imx_pinconf_dbg_show(struct pinctrl_dev *pctldev,
 		return;
 	}
 
-	config = readl(ipctl->base + pin_reg->conf_reg);
+	config = readl(pin_reg->base + pin_reg->conf_reg);
 	seq_printf(s, "0x%lx", config);
 }
 
@@ -551,14 +556,25 @@  static int imx_pinctrl_parse_groups(struct device_node *np,
 		}
 
 		pin_id = mux_reg ? mux_reg / 4 : conf_reg / 4;
+
+		pin->input_reg = be32_to_cpu(*list++);
+		pin->mux_mode = be32_to_cpu(*list++);
+		pin->input_val = be32_to_cpu(*list++);
+
+		if (info->flags & IOMUXC_LPSR_SUPPORT &&
+			IOMUXC_LPSR_MASK(pin->input_val))
+				pin_id = IOMUXC_LPSR_MASK(pin->input_val);
+
 		pin_reg = &info->pin_regs[pin_id];
 		pin->pin = pin_id;
 		grp->pin_ids[i] = pin_id;
 		pin_reg->mux_reg = mux_reg;
 		pin_reg->conf_reg = conf_reg;
-		pin->input_reg = be32_to_cpu(*list++);
-		pin->mux_mode = be32_to_cpu(*list++);
-		pin->input_val = be32_to_cpu(*list++);
+		pin_reg->base = info->base;
+
+		if (info->flags & IOMUXC_LPSR_SUPPORT &&
+			IOMUXC_LPSR_MASK(pin->input_val))
+				pin_reg->base = info->base_lpsr;
 
 		/* SION bit is in mux register */
 		config = be32_to_cpu(*list++);
@@ -709,9 +725,19 @@  int imx_pinctrl_probe(struct platform_device *pdev,
 	}
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	ipctl->base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(ipctl->base))
-		return PTR_ERR(ipctl->base);
+	info->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(info->base))
+		return PTR_ERR(info->base);
+
+	if (info->flags & IOMUXC_LPSR_SUPPORT) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+		info->base_lpsr = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(info->base_lpsr)) {
+			dev_err(&pdev->dev,
+				"iomuxc-lpsr base address not found\n");
+			return PTR_ERR(info->base_lpsr);
+		}
+	}
 
 	imx_pinctrl_desc.name = dev_name(&pdev->dev);
 	imx_pinctrl_desc.pins = info->pins;
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h b/drivers/pinctrl/freescale/pinctrl-imx.h
index 49e55d3..5e0f2e0 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.h
+++ b/drivers/pinctrl/freescale/pinctrl-imx.h
@@ -1,7 +1,7 @@ 
 /*
  * IMX pinmux core definitions
  *
- * Copyright (C) 2012 Freescale Semiconductor, Inc.
+ * Copyright (C) 2012-2015 Freescale Semiconductor, Inc.
  * Copyright (C) 2012 Linaro Ltd.
  *
  * Author: Dong Aisheng <dong.aisheng@linaro.org>
@@ -69,6 +69,7 @@  struct imx_pmx_func {
 struct imx_pin_reg {
 	s16 mux_reg;
 	s16 conf_reg;
+	void __iomem *base;
 };
 
 struct imx_pinctrl_soc_info {
@@ -81,9 +82,13 @@  struct imx_pinctrl_soc_info {
 	struct imx_pmx_func *functions;
 	unsigned int nfunctions;
 	unsigned int flags;
+	void __iomem *base;
+	void __iomem *base_lpsr;
 };
 
 #define SHARE_MUX_CONF_REG	0x1
+#define IOMUXC_LPSR_SUPPORT 	0x2
+#define IOMUXC_LPSR_MASK(id)	((id >> 0x10) & 0xffff)
 
 #define NO_MUX		0x0
 #define NO_PAD		0x0