diff mbox

[v2,05/12] Document: dt: binding: imx: update pinctrl doc for imx6sll

Message ID 1482832070-22668-6-git-send-email-ping.bai@nxp.com
State New
Headers show

Commit Message

Jacky Bai Dec. 27, 2016, 9:47 a.m. UTC 
Add pinctrl binding doc update for imx6sll.

Signed-off-by: Bai Ping <ping.bai@nxp.com>
---
 .../bindings/pinctrl/fsl,imx6sll-pinctrl.txt       | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/fsl,imx6sll-pinctrl.txt

Comments

Linus Walleij Dec. 27, 2016, 12:59 p.m. UTC | #1 
On Tue, Dec 27, 2016 at 10:47 AM, Bai Ping <ping.bai@nxp.com> wrote:

> Add pinctrl binding doc update for imx6sll.
>
> Signed-off-by: Bai Ping <ping.bai@nxp.com>

I have to push back on this a bit.

> +Please refer to fsl,imx-pinctrl.txt in this directory for common binding part
> +and usage.

I understand that it is building on top of the old i.MX bindings and that
it has some kind of "tradition" coming with it.

At the same time, the i.MX bindings came about before we had the
generic pin control bindings defined.

> +CONFIG bits definition:
> +PAD_CTL_LVE                    (1 << 22)
> +PAD_CTL_HYS                     (1 << 16)
> +PAD_CTL_PUS_100K_DOWN           (0 << 14)
> +PAD_CTL_PUS_47K_UP              (1 << 14)
> +PAD_CTL_PUS_100K_UP             (2 << 14)
> +PAD_CTL_PUS_22K_UP              (3 << 14)
> +PAD_CTL_PUE                     (1 << 13)
> +PAD_CTL_PKE                     (1 << 12)
> +PAD_CTL_ODE                     (1 << 11)
> +PAD_CTL_SPEED_LOW               (0 << 6)
> +PAD_CTL_SPEED_MED               (1 << 6)
> +PAD_CTL_SPEED_HIGH              (3 << 6)
> +PAD_CTL_DSE_DISABLE             (0 << 3)
> +PAD_CTL_DSE_260ohm              (1 << 3)
> +PAD_CTL_DSE_130ohm              (2 << 3)
> +PAD_CTL_DSE_87ohm               (3 << 3)
> +PAD_CTL_DSE_65ohm               (4 << 3)
> +PAD_CTL_DSE_52ohm               (5 << 3)
> +PAD_CTL_DSE_43ohm               (6 << 3)
> +PAD_CTL_DSE_37ohm               (7 << 3)
> +PAD_CTL_SRE_FAST                (1 << 0)
> +PAD_CTL_SRE_SLOW                (0 << 0)

A whole slew of these if not all correspond to the generic bindings.

I would consider augmenting the code in the driver to handle the generic
bindings *in addition* to the old legacy bindings, and use those over these
random custom bits.

Read drivers using CONFIG_GENERIC_PINCONF as an inspiration.

For example see commit
cefbf1a1b29531a970bc2908a50a75d6474fcc38
"pinctrl: sunxi: Support generic binding"
from Maxime Ripard, where he does a similar thing for sunxi.

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
Jacky Bai Jan. 9, 2017, 2:32 a.m. UTC | #2 
> Subject: Re: [PATCH v2 05/12] Document: dt: binding: imx: update pinctrl doc

> for imx6sll

> 

> On Tue, Dec 27, 2016 at 10:47 AM, Bai Ping <ping.bai@nxp.com> wrote:

> 

> > Add pinctrl binding doc update for imx6sll.

> >

> > Signed-off-by: Bai Ping <ping.bai@nxp.com>

> 

> I have to push back on this a bit.

> 

> > +Please refer to fsl,imx-pinctrl.txt in this directory for common

> > +binding part and usage.

> 

> I understand that it is building on top of the old i.MX bindings and that it has

> some kind of "tradition" coming with it.

> 

> At the same time, the i.MX bindings came about before we had the generic pin

> control bindings defined.

> 

> > +CONFIG bits definition:

> > +PAD_CTL_LVE                    (1 << 22)

> > +PAD_CTL_HYS                     (1 << 16)

> > +PAD_CTL_PUS_100K_DOWN           (0 << 14)

> > +PAD_CTL_PUS_47K_UP              (1 << 14)

> > +PAD_CTL_PUS_100K_UP             (2 << 14)

> > +PAD_CTL_PUS_22K_UP              (3 << 14)

> > +PAD_CTL_PUE                     (1 << 13)

> > +PAD_CTL_PKE                     (1 << 12)

> > +PAD_CTL_ODE                     (1 << 11)

> > +PAD_CTL_SPEED_LOW               (0 << 6)

> > +PAD_CTL_SPEED_MED               (1 << 6)

> > +PAD_CTL_SPEED_HIGH              (3 << 6)

> > +PAD_CTL_DSE_DISABLE             (0 << 3)

> > +PAD_CTL_DSE_260ohm              (1 << 3)

> > +PAD_CTL_DSE_130ohm              (2 << 3)

> > +PAD_CTL_DSE_87ohm               (3 << 3)

> > +PAD_CTL_DSE_65ohm               (4 << 3)

> > +PAD_CTL_DSE_52ohm               (5 << 3)

> > +PAD_CTL_DSE_43ohm               (6 << 3)

> > +PAD_CTL_DSE_37ohm               (7 << 3)

> > +PAD_CTL_SRE_FAST                (1 << 0)

> > +PAD_CTL_SRE_SLOW                (0 << 0)

> 

> A whole slew of these if not all correspond to the generic bindings.

> 

> I would consider augmenting the code in the driver to handle the generic

> bindings *in addition* to the old legacy bindings, and use those over these

> random custom bits.

> 

> Read drivers using CONFIG_GENERIC_PINCONF as an inspiration.

> 

> For example see commit

> cefbf1a1b29531a970bc2908a50a75d6474fcc38

> "pinctrl: sunxi: Support generic binding"

> from Maxime Ripard, where he does a similar thing for sunxi.


I have look into the above commit on using generic binding. But I think the generic pinconf
is not very easy to add in imx pinctrl Driver. imx pinctrl use a different way to parse the pin configure.
 
Each fsl,pin entry  looks like <PIN_FUNC_ID  CONFIG> in dts, the CONFIG is the pad setting value like
pull-up, open-drain, drive strength etc. The above config bit definition is specific to each SOC in the PAD CTL register.

If we want set the pin config to enable hysteresis, 47KOhm Pull Up, 50Mhz speed, 80Ohm driver strength
and Fast Slew Rate, then the CONFIG value should be 0x17059( ORs corresponding bit definition). This value will be set in
PAD CTL register to config the corresponding pin.

BR
Jacky Bai

> 

> Yours,

> Linus Walleij
Linus Walleij Jan. 11, 2017, 2:33 p.m. UTC | #3 
On Mon, Jan 9, 2017 at 3:32 AM, Jacky Bai <ping.bai@nxp.com> wrote:

> I have look into the above commit on using generic binding. But I think the generic pinconf
> is not very easy to add in imx pinctrl Driver. imx pinctrl use a different way to parse the pin configure.

OK atleast I need an indication from one of the i.MX maintainers how they
want to proceed.

> Each fsl,pin entry  looks like <PIN_FUNC_ID  CONFIG> in dts, the CONFIG is the pad setting value like
> pull-up, open-drain, drive strength etc. The above config bit definition is specific to each SOC in the PAD CTL register.
>
> If we want set the pin config to enable hysteresis, 47KOhm Pull Up, 50Mhz speed, 80Ohm driver strength
> and Fast Slew Rate, then the CONFIG value should be 0x17059( ORs corresponding bit definition).

Hysteresis is an input property and does not make sense on something
that need drive
strength and slew rate configuration which are output properties.
(I guess you mean 80mA drive strength.)

Actually such oxymoronic settings is a good reason to migrate to
generic bindings
because when you describe stuff with generic strings you see better
what is going
on, and we can add sanity checks to cases like this in the generic code where it
would indeed be valid to ask why this combination of settings is being made.

> This value will be set in
> PAD CTL register to config the corresponding pin.

Yes? That is common. It looks like that in DT:

{
   input-schmitt-enable;
   bias-pull-up = <47000>;
   slew-rate = <50000000>;
   drive-strength = <80000>;
};

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
Jacky Bai Jan. 12, 2017, 2:57 a.m. UTC | #4 
> Subject: Re: [PATCH v2 05/12] Document: dt: binding: imx: update pinctrl doc

> for imx6sll

> 

> On Mon, Jan 9, 2017 at 3:32 AM, Jacky Bai <ping.bai@nxp.com> wrote:

> 

> > I have look into the above commit on using generic binding. But I

> > think the generic pinconf is not very easy to add in imx pinctrl Driver. imx

> pinctrl use a different way to parse the pin configure.

> 

> OK atleast I need an indication from one of the i.MX maintainers how they

> want to proceed.

> 

> > Each fsl,pin entry  looks like <PIN_FUNC_ID  CONFIG> in dts, the

> > CONFIG is the pad setting value like pull-up, open-drain, drive strength etc.

> The above config bit definition is specific to each SOC in the PAD CTL register.

> >

> > If we want set the pin config to enable hysteresis, 47KOhm Pull Up,

> > 50Mhz speed, 80Ohm driver strength and Fast Slew Rate, then the CONFIG

> value should be 0x17059( ORs corresponding bit definition).

> 

> Hysteresis is an input property and does not make sense on something that

> need drive strength and slew rate configuration which are output properties.

> (I guess you mean 80mA drive strength.)

> 


For some bi-direction pins, like SD DATA pin, we may need to enable the hysteresis configuration.

> Actually such oxymoronic settings is a good reason to migrate to generic

> bindings because when you describe stuff with generic strings you see better

> what is going on, and we can add sanity checks to cases like this in the generic

> code where it would indeed be valid to ask why this combination of settings is

> being made.

> 


Another thing is that we can use a pins-tool program developed by NXP to generate the pinctrl configuration code that can
be used directly in dts. This tiny program can avoid pin function conflict. As on i.MX, there are so may pins, each pin can be used
for up 8  function. Configuring the pins is a time-consuming work.  This tools is very useful for customer to generate the dts code.  

Using generic pinconf is another option, but it may need more time to add it and more work to do.  For now, I think we can still keep the legacy method?
 
> > This value will be set in

> > PAD CTL register to config the corresponding pin.

> 

> Yes? That is common. It looks like that in DT:

> 

> {

>    input-schmitt-enable;

>    bias-pull-up = <47000>;

>    slew-rate = <50000000>;

>    drive-strength = <80000>;

> };

> 

> Yours,

> Linus Walleij
Linus Walleij Jan. 13, 2017, 3:22 p.m. UTC | #5 
On Thu, Jan 12, 2017 at 3:57 AM, Jacky Bai <ping.bai@nxp.com> wrote:

> Another thing is that we can use a pins-tool program developed by NXP to
>  generate the pinctrl configuration code that can be used directly in dts. This
> tiny program can avoid pin function conflict. As on i.MX, there are so may pins,
> each pin can be used for up 8  function. Configuring the pins is a time-consuming
> work.  This tools is very useful for customer to generate the dts code.

I understand, but every silicon vendor has such a tool, all are different,
proprietary and unfriendly to programmers and open source developers, who
need to understand how the hardware is working without magic tools
and secret data sheets to fix bugs.

For the people working with maintaining the code it is paramount that
DTS files are self-descriptive.

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
Jacky Bai Jan. 17, 2017, 6:35 a.m. UTC | #6 
PiBTdWJqZWN0OiBSZTogW1BBVENIIHYyIDA1LzEyXSBEb2N1bWVudDogZHQ6IGJpbmRpbmc6IGlt
eDogdXBkYXRlIHBpbmN0cmwgZG9jDQo+IGZvciBpbXg2c2xsDQo+IA0KPiBPbiBUaHUsIEphbiAx
MiwgMjAxNyBhdCAzOjU3IEFNLCBKYWNreSBCYWkgPHBpbmcuYmFpQG54cC5jb20+IHdyb3RlOg0K
PiANCj4gPiBBbm90aGVyIHRoaW5nIGlzIHRoYXQgd2UgY2FuIHVzZSBhIHBpbnMtdG9vbCBwcm9n
cmFtIGRldmVsb3BlZCBieSBOWFANCj4gPiB0byAgZ2VuZXJhdGUgdGhlIHBpbmN0cmwgY29uZmln
dXJhdGlvbiBjb2RlIHRoYXQgY2FuIGJlIHVzZWQgZGlyZWN0bHkNCj4gPiBpbiBkdHMuIFRoaXMg
dGlueSBwcm9ncmFtIGNhbiBhdm9pZCBwaW4gZnVuY3Rpb24gY29uZmxpY3QuIEFzIG9uIGkuTVgs
DQo+ID4gdGhlcmUgYXJlIHNvIG1heSBwaW5zLCBlYWNoIHBpbiBjYW4gYmUgdXNlZCBmb3IgdXAg
OCAgZnVuY3Rpb24uDQo+ID4gQ29uZmlndXJpbmcgdGhlIHBpbnMgaXMgYSB0aW1lLWNvbnN1bWlu
ZyB3b3JrLiAgVGhpcyB0b29scyBpcyB2ZXJ5IHVzZWZ1bCBmb3INCj4gY3VzdG9tZXIgdG8gZ2Vu
ZXJhdGUgdGhlIGR0cyBjb2RlLg0KPiANCj4gSSB1bmRlcnN0YW5kLCBidXQgZXZlcnkgc2lsaWNv
biB2ZW5kb3IgaGFzIHN1Y2ggYSB0b29sLCBhbGwgYXJlIGRpZmZlcmVudCwNCj4gcHJvcHJpZXRh
cnkgYW5kIHVuZnJpZW5kbHkgdG8gcHJvZ3JhbW1lcnMgYW5kIG9wZW4gc291cmNlIGRldmVsb3Bl
cnMsIHdobw0KPiBuZWVkIHRvIHVuZGVyc3RhbmQgaG93IHRoZSBoYXJkd2FyZSBpcyB3b3JraW5n
IHdpdGhvdXQgbWFnaWMgdG9vbHMgYW5kDQo+IHNlY3JldCBkYXRhIHNoZWV0cyB0byBmaXggYnVn
cy4NCj4gDQo+IEZvciB0aGUgcGVvcGxlIHdvcmtpbmcgd2l0aCBtYWludGFpbmluZyB0aGUgY29k
ZSBpdCBpcyBwYXJhbW91bnQgdGhhdCBEVFMgZmlsZXMNCj4gYXJlIHNlbGYtZGVzY3JpcHRpdmUu
DQo+IA0KDQpPSy4gIFRoYW5rcyBmb3IgeW91ciBjb21tZW50cy4gIEFkZGluZyBnZW5lcmljLXBp
bmNvbmYgaW4gaW14IHBpbmN0cmwgbmVlZHMgc29tZSB0aW1lDQp0byBmaW5pc2ggYW5kIHRoZSBs
ZWdhY3kgbWV0aG9kIHN0aWxsIG5lZWQgYmUgaGVyZSBldmVuIGlmIGdlbmVyaWMtcGluY29uZiBp
cyBhZGRlZC4gDQpEbyB5b3UgcGxhbiB0byBwaWNrIHRoaXMgbGVnYWN5IGJpbmRpbmcgcGF0Y2gg
Zm9yIG5vdz8NCg0KPiBZb3VycywNCj4gTGludXMgV2FsbGVpag0K
--
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
Linus Walleij Jan. 18, 2017, 12:24 p.m. UTC | #7 
On Tue, Jan 17, 2017 at 7:35 AM, Jacky Bai <ping.bai@nxp.com> wrote:
>> Subject: Re: [PATCH v2 05/12] Document: dt: binding: imx: update pinctrl doc
>> On Thu, Jan 12, 2017 at 3:57 AM, Jacky Bai <ping.bai@nxp.com> wrote:
>>
>> > Another thing is that we can use a pins-tool program developed by NXP
>> > to  generate the pinctrl configuration code that can be used directly
>> > in dts. This tiny program can avoid pin function conflict. As on i.MX,
>> > there are so may pins, each pin can be used for up 8  function.
>> > Configuring the pins is a time-consuming work.  This tools is very useful for
>> customer to generate the dts code.
>>
>> I understand, but every silicon vendor has such a tool, all are different,
>> proprietary and unfriendly to programmers and open source developers, who
>> need to understand how the hardware is working without magic tools and
>> secret data sheets to fix bugs.
>>
>> For the people working with maintaining the code it is paramount that DTS files
>> are self-descriptive.
>
> OK.  Thanks for your comments.  Adding generic-pinconf in imx pinctrl needs some time
> to finish and the legacy method still need be here even if generic-pinconf is added.
> Do you plan to pick this legacy binding patch for now?

As I said earlier:

> atleast I need an indication from one of the i.MX maintainers how they
> want to proceed.

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 Jan. 23, 2017, 6:04 a.m. UTC | #8 
On Wed, Jan 18, 2017 at 01:24:30PM +0100, Linus Walleij wrote:
> On Tue, Jan 17, 2017 at 7:35 AM, Jacky Bai <ping.bai@nxp.com> wrote:
> >> Subject: Re: [PATCH v2 05/12] Document: dt: binding: imx: update pinctrl doc
> >> On Thu, Jan 12, 2017 at 3:57 AM, Jacky Bai <ping.bai@nxp.com> wrote:
> >>
> >> > Another thing is that we can use a pins-tool program developed by NXP
> >> > to  generate the pinctrl configuration code that can be used directly
> >> > in dts. This tiny program can avoid pin function conflict. As on i.MX,
> >> > there are so may pins, each pin can be used for up 8  function.
> >> > Configuring the pins is a time-consuming work.  This tools is very useful for
> >> customer to generate the dts code.
> >>
> >> I understand, but every silicon vendor has such a tool, all are different,
> >> proprietary and unfriendly to programmers and open source developers, who
> >> need to understand how the hardware is working without magic tools and
> >> secret data sheets to fix bugs.
> >>
> >> For the people working with maintaining the code it is paramount that DTS files
> >> are self-descriptive.
> >
> > OK.  Thanks for your comments.  Adding generic-pinconf in imx pinctrl needs some time
> > to finish and the legacy method still need be here even if generic-pinconf is added.
> > Do you plan to pick this legacy binding patch for now?
> 
> As I said earlier:
> 
> > atleast I need an indication from one of the i.MX maintainers how they
> > want to proceed.

Sorry for being late for the discussion.

To be honest, I did not really expect so many i.MX6 variants coming to
us.  Just out of curiosity, are there any more in the pipeline to come,
or is this the last known one?

I understand that there are now more generic support available at
pinctrl core level for us to use than the time we initial designed i.MX
pinctrl driver.  But I intend to agree with Jacky that imx6sll can be
supported by the existing driver, since it doesn't require any
additional driver changes.

If any new generation of i.MX family require changes on the existing
pinctrl driver, we should probably push back for a redesign to use
generic support as much as possible.  @Jacky, how does i.MX8 pinctrl
look like?  How same or different as/from i.MX6 pinctrl?

Just my opinion.

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
Jacky Bai Jan. 23, 2017, 7:17 a.m. UTC | #9 
> From: Shawn Guo [mailto:shawnguo@kernel.org]
> > >>
> > >> > Another thing is that we can use a pins-tool program developed by
> > >> > NXP to  generate the pinctrl configuration code that can be used
> > >> > directly in dts. This tiny program can avoid pin function
> > >> > conflict. As on i.MX, there are so may pins, each pin can be used for up 8
> function.
> > >> > Configuring the pins is a time-consuming work.  This tools is
> > >> > very useful for
> > >> customer to generate the dts code.
> > >>
> > >> I understand, but every silicon vendor has such a tool, all are
> > >> different, proprietary and unfriendly to programmers and open
> > >> source developers, who need to understand how the hardware is
> > >> working without magic tools and secret data sheets to fix bugs.
> > >>
> > >> For the people working with maintaining the code it is paramount
> > >> that DTS files are self-descriptive.
> > >
> > > OK.  Thanks for your comments.  Adding generic-pinconf in imx
> > > pinctrl needs some time to finish and the legacy method still need be here
> even if generic-pinconf is added.
> > > Do you plan to pick this legacy binding patch for now?
> >
> > As I said earlier:
> >
> > > atleast I need an indication from one of the i.MX maintainers how
> > > they want to proceed.
> 
> Sorry for being late for the discussion.
> 
> To be honest, I did not really expect so many i.MX6 variants coming to us.  Just
> out of curiosity, are there any more in the pipeline to come, or is this the last
> known one?
> 

AFAIK, this should be the last variant of i.MX6. But not the last one of i.MX family.

> I understand that there are now more generic support available at pinctrl core
> level for us to use than the time we initial designed i.MX pinctrl driver.  But I
> intend to agree with Jacky that imx6sll can be supported by the existing driver,
> since it doesn't require any additional driver changes.
> 
> If any new generation of i.MX family require changes on the existing pinctrl
> driver, we should probably push back for a redesign to use generic support as
> much as possible.  @Jacky, how does i.MX8 pinctrl look like?  How same or
> different as/from i.MX6 pinctrl?
> 

i.MX8 kernel support is in development stage. The code has not been finalized.
For now, i.MX8 shares most of the pinctrl code with i.MX6 and use the same pinconfig method.

> Just my opinion.
> 
> 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
Linus Walleij Jan. 26, 2017, 2:05 p.m. UTC | #10 
On Mon, Jan 23, 2017 at 8:17 AM, Jacky Bai <ping.bai@nxp.com> wrote:

> i.MX8 kernel support is in development stage. The code has not been finalized.
> For now, i.MX8 shares most of the pinctrl code with i.MX6 and use the same pinconfig method.

Please switch to generic bindings already.

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
Dong Aisheng Feb. 16, 2017, 7:51 a.m. UTC | #11 
Hi Linus,

On Tue, Dec 27, 2016 at 8:59 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Dec 27, 2016 at 10:47 AM, Bai Ping <ping.bai@nxp.com> wrote:
>
>> Add pinctrl binding doc update for imx6sll.
>>
>> Signed-off-by: Bai Ping <ping.bai@nxp.com>
>
> I have to push back on this a bit.
>
>> +Please refer to fsl,imx-pinctrl.txt in this directory for common binding part
>> +and usage.
>
> I understand that it is building on top of the old i.MX bindings and that
> it has some kind of "tradition" coming with it.
>
> At the same time, the i.MX bindings came about before we had the
> generic pin control bindings defined.
>
>> +CONFIG bits definition:
>> +PAD_CTL_LVE                    (1 << 22)
>> +PAD_CTL_HYS                     (1 << 16)
>> +PAD_CTL_PUS_100K_DOWN           (0 << 14)
>> +PAD_CTL_PUS_47K_UP              (1 << 14)
>> +PAD_CTL_PUS_100K_UP             (2 << 14)
>> +PAD_CTL_PUS_22K_UP              (3 << 14)
>> +PAD_CTL_PUE                     (1 << 13)
>> +PAD_CTL_PKE                     (1 << 12)
>> +PAD_CTL_ODE                     (1 << 11)
>> +PAD_CTL_SPEED_LOW               (0 << 6)
>> +PAD_CTL_SPEED_MED               (1 << 6)
>> +PAD_CTL_SPEED_HIGH              (3 << 6)
>> +PAD_CTL_DSE_DISABLE             (0 << 3)
>> +PAD_CTL_DSE_260ohm              (1 << 3)
>> +PAD_CTL_DSE_130ohm              (2 << 3)
>> +PAD_CTL_DSE_87ohm               (3 << 3)
>> +PAD_CTL_DSE_65ohm               (4 << 3)
>> +PAD_CTL_DSE_52ohm               (5 << 3)
>> +PAD_CTL_DSE_43ohm               (6 << 3)
>> +PAD_CTL_DSE_37ohm               (7 << 3)
>> +PAD_CTL_SRE_FAST                (1 << 0)
>> +PAD_CTL_SRE_SLOW                (0 << 0)
>
> A whole slew of these if not all correspond to the generic bindings.
>
> I would consider augmenting the code in the driver to handle the generic
> bindings *in addition* to the old legacy bindings, and use those over these
> random custom bits.
>
> Read drivers using CONFIG_GENERIC_PINCONF as an inspiration.
>
> For example see commit
> cefbf1a1b29531a970bc2908a50a75d6474fcc38
> "pinctrl: sunxi: Support generic binding"
> from Maxime Ripard, where he does a similar thing for sunxi.
>

First thanks for your good suggestion!

All we realized that the raw data of IMX pad config in dts is not good.
e.g.
pinctrl_ecspi3: ecspi3grp {
        fsl,pins = <
                MX7D_PAD_SAI2_TX_SYNC__ECSPI3_MISO      0x17059
        .........

I did a deep feasibility analysis these days on the migrating to generic
pinctrl binding you referred. It is regrettably that it may not be so suitable
for IMX to fully migrate right now.

Generic binding only supports parsing strings for pins and function
from dts right now, that means we need back to the old way of hardcoding
all register bits in driver which we actually chose to move out since
the commit "e164153 pinctrl: imx: move hard-coding data into device tree".

And IMX doesn't have PAD GROUP in HW, each pad theoretically can be muxed
as 8 single functions.
e.g.
For IOMUXC_SW_MUX_CTL_PAD_CSI_DATA07
MUX_MODE MUX Mode Select Field.
Select 1 of 8 iomux modes to be used for pad: CSI_DATA07.
000 ALT0 — Select signal CSI1_DATA09.
001 ALT1 — Select signal ESAI_TX3_RX2.
010 ALT2 — Select signal I2C4_SDA.
011 ALT3 — Select signal KPP_ROW7.
100 ALT4 — Select signal UART6_CTS_B.
101 ALT5 — Select signal GPIO1_IO21.
110 ALT6 — Select signal EIM_DATA16.
111 ALT7 — Select signal DCIC1_OUT.
By converting to generic binding, we need construct a huge function/group map
in driver for hundreds of pads for each SoC. That is a bit fear to us.

Current IMX method only generates the used function/groups dynamically
by parsing board dts which is much a lighter way.

But of course, we want to fix the raw config value issue as well which is
un-maintainable and un-readable as you pointed out.

I think there might be two options for us to go:
One option is using macro definitions instead of raw data as pinctrl-single
users as OMAP.
e.g.
art2_pins: pinmux_uart2_pins {
        pinctrl-single,pins = <
                OMAP4_IOPAD(0x118, PIN_INPUT_PULLUP | MUX_MODE0)
 /* uart2_cts.uart2_cts */
        ........

For this way, no driver changes required, just replace the raw data by macro
in device tree. But i wonder this may not be your original intention although
it's the easiest way for us.
Anyway, if you agree, we probably would prefer this way.

Another option is partially convert to generic binding for only pad
configuration
part. e.g.
pinctrl_usdhc1: usdhc1grp {
        fsl,pins = <
                MX7D_PAD_SD1_CMD__SD1_CMD
                MX7D_PAD_SD1_CLK__SD1_CLK
                MX7D_PAD_SD1_DATA0__SD1_DATA0
                ...
        >;
        bias-pull-up = <47KOHM>;
        drive-strength = <130OHM>;
        slew-rate = <FAST>;
        speed = <50MHZ>;
        ....
};
(Some of the property still not supported or not the same as generic
binding right now)

This way requires no big drivers changes and only extend the driver
to support the generic pinctrl dt configuration properties.

And it also fix the raw data issues and looks like more applicable than the full
migration.

So would you be willing to accept it?

Another potential benefits for this way is that we may can use PCONFDUMP
to dump a more readable data from pinctrl debug system.

> Yours,
> Linus Walleij
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regards
Dong Aisheng
--
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
Dong Aisheng Feb. 24, 2017, 9:02 a.m. UTC | #12 
Hi Linus

Gently Ping...

Any feedback about the proposal?

Regards
Dong Aisheng

On Thu, Feb 16, 2017 at 3:51 PM, Dong Aisheng <dongas86@gmail.com> wrote:
> Hi Linus,
>
> On Tue, Dec 27, 2016 at 8:59 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Tue, Dec 27, 2016 at 10:47 AM, Bai Ping <ping.bai@nxp.com> wrote:
>>
>>> Add pinctrl binding doc update for imx6sll.
>>>
>>> Signed-off-by: Bai Ping <ping.bai@nxp.com>
>>
>> I have to push back on this a bit.
>>
>>> +Please refer to fsl,imx-pinctrl.txt in this directory for common binding part
>>> +and usage.
>>
>> I understand that it is building on top of the old i.MX bindings and that
>> it has some kind of "tradition" coming with it.
>>
>> At the same time, the i.MX bindings came about before we had the
>> generic pin control bindings defined.
>>
>>> +CONFIG bits definition:
>>> +PAD_CTL_LVE                    (1 << 22)
>>> +PAD_CTL_HYS                     (1 << 16)
>>> +PAD_CTL_PUS_100K_DOWN           (0 << 14)
>>> +PAD_CTL_PUS_47K_UP              (1 << 14)
>>> +PAD_CTL_PUS_100K_UP             (2 << 14)
>>> +PAD_CTL_PUS_22K_UP              (3 << 14)
>>> +PAD_CTL_PUE                     (1 << 13)
>>> +PAD_CTL_PKE                     (1 << 12)
>>> +PAD_CTL_ODE                     (1 << 11)
>>> +PAD_CTL_SPEED_LOW               (0 << 6)
>>> +PAD_CTL_SPEED_MED               (1 << 6)
>>> +PAD_CTL_SPEED_HIGH              (3 << 6)
>>> +PAD_CTL_DSE_DISABLE             (0 << 3)
>>> +PAD_CTL_DSE_260ohm              (1 << 3)
>>> +PAD_CTL_DSE_130ohm              (2 << 3)
>>> +PAD_CTL_DSE_87ohm               (3 << 3)
>>> +PAD_CTL_DSE_65ohm               (4 << 3)
>>> +PAD_CTL_DSE_52ohm               (5 << 3)
>>> +PAD_CTL_DSE_43ohm               (6 << 3)
>>> +PAD_CTL_DSE_37ohm               (7 << 3)
>>> +PAD_CTL_SRE_FAST                (1 << 0)
>>> +PAD_CTL_SRE_SLOW                (0 << 0)
>>
>> A whole slew of these if not all correspond to the generic bindings.
>>
>> I would consider augmenting the code in the driver to handle the generic
>> bindings *in addition* to the old legacy bindings, and use those over these
>> random custom bits.
>>
>> Read drivers using CONFIG_GENERIC_PINCONF as an inspiration.
>>
>> For example see commit
>> cefbf1a1b29531a970bc2908a50a75d6474fcc38
>> "pinctrl: sunxi: Support generic binding"
>> from Maxime Ripard, where he does a similar thing for sunxi.
>>
>
> First thanks for your good suggestion!
>
> All we realized that the raw data of IMX pad config in dts is not good.
> e.g.
> pinctrl_ecspi3: ecspi3grp {
>         fsl,pins = <
>                 MX7D_PAD_SAI2_TX_SYNC__ECSPI3_MISO      0x17059
>         .........
>
> I did a deep feasibility analysis these days on the migrating to generic
> pinctrl binding you referred. It is regrettably that it may not be so suitable
> for IMX to fully migrate right now.
>
> Generic binding only supports parsing strings for pins and function
> from dts right now, that means we need back to the old way of hardcoding
> all register bits in driver which we actually chose to move out since
> the commit "e164153 pinctrl: imx: move hard-coding data into device tree".
>
> And IMX doesn't have PAD GROUP in HW, each pad theoretically can be muxed
> as 8 single functions.
> e.g.
> For IOMUXC_SW_MUX_CTL_PAD_CSI_DATA07
> MUX_MODE MUX Mode Select Field.
> Select 1 of 8 iomux modes to be used for pad: CSI_DATA07.
> 000 ALT0 — Select signal CSI1_DATA09.
> 001 ALT1 — Select signal ESAI_TX3_RX2.
> 010 ALT2 — Select signal I2C4_SDA.
> 011 ALT3 — Select signal KPP_ROW7.
> 100 ALT4 — Select signal UART6_CTS_B.
> 101 ALT5 — Select signal GPIO1_IO21.
> 110 ALT6 — Select signal EIM_DATA16.
> 111 ALT7 — Select signal DCIC1_OUT.
> By converting to generic binding, we need construct a huge function/group map
> in driver for hundreds of pads for each SoC. That is a bit fear to us.
>
> Current IMX method only generates the used function/groups dynamically
> by parsing board dts which is much a lighter way.
>
> But of course, we want to fix the raw config value issue as well which is
> un-maintainable and un-readable as you pointed out.
>
> I think there might be two options for us to go:
> One option is using macro definitions instead of raw data as pinctrl-single
> users as OMAP.
> e.g.
> art2_pins: pinmux_uart2_pins {
>         pinctrl-single,pins = <
>                 OMAP4_IOPAD(0x118, PIN_INPUT_PULLUP | MUX_MODE0)
>  /* uart2_cts.uart2_cts */
>         ........
>
> For this way, no driver changes required, just replace the raw data by macro
> in device tree. But i wonder this may not be your original intention although
> it's the easiest way for us.
> Anyway, if you agree, we probably would prefer this way.
>
> Another option is partially convert to generic binding for only pad
> configuration
> part. e.g.
> pinctrl_usdhc1: usdhc1grp {
>         fsl,pins = <
>                 MX7D_PAD_SD1_CMD__SD1_CMD
>                 MX7D_PAD_SD1_CLK__SD1_CLK
>                 MX7D_PAD_SD1_DATA0__SD1_DATA0
>                 ...
>         >;
>         bias-pull-up = <47KOHM>;
>         drive-strength = <130OHM>;
>         slew-rate = <FAST>;
>         speed = <50MHZ>;
>         ....
> };
> (Some of the property still not supported or not the same as generic
> binding right now)
>
> This way requires no big drivers changes and only extend the driver
> to support the generic pinctrl dt configuration properties.
>
> And it also fix the raw data issues and looks like more applicable than the full
> migration.
>
> So would you be willing to accept it?
>
> Another potential benefits for this way is that we may can use PCONFDUMP
> to dump a more readable data from pinctrl debug system.
>
>> Yours,
>> Linus Walleij
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> Regards
> Dong Aisheng
--
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
Linus Walleij March 14, 2017, 1:54 p.m. UTC | #13 
On Thu, Feb 16, 2017 at 8:51 AM, Dong Aisheng <dongas86@gmail.com> wrote:
> On Tue, Dec 27, 2016 at 8:59 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

>> I would consider augmenting the code in the driver to handle the generic
>> bindings *in addition* to the old legacy bindings, and use those over these
>> random custom bits.
(...)
> Generic binding only supports parsing strings for pins and function
> from dts right now, that means we need back to the old way of hardcoding
> all register bits in driver which we actually chose to move out since
> the commit "e164153 pinctrl: imx: move hard-coding data into device tree".

Aha I see.

> One option is using macro definitions instead of raw data as pinctrl-single
> users as OMAP.
> e.g.
> art2_pins: pinmux_uart2_pins {
>         pinctrl-single,pins = <
>                 OMAP4_IOPAD(0x118, PIN_INPUT_PULLUP | MUX_MODE0)
>  /* uart2_cts.uart2_cts */
>         ........
>
> For this way, no driver changes required, just replace the raw data by macro
> in device tree. But i wonder this may not be your original intention although
> it's the easiest way for us.
> Anyway, if you agree, we probably would prefer this way.

That make it look better than it looks today, but...

> Another option is partially convert to generic binding for only pad
> configuration
> part. e.g.
> pinctrl_usdhc1: usdhc1grp {
>         fsl,pins = <
>                 MX7D_PAD_SD1_CMD__SD1_CMD
>                 MX7D_PAD_SD1_CLK__SD1_CLK
>                 MX7D_PAD_SD1_DATA0__SD1_DATA0
>                 ...
>         >;
>         bias-pull-up = <47KOHM>;
>         drive-strength = <130OHM>;
>         slew-rate = <FAST>;
>         speed = <50MHZ>;
>         ....
> };
> (Some of the property still not supported or not the same as generic
> binding right now)

This looks totally awesome, so if you could do this, it would be even
better.

> This way requires no big drivers changes and only extend the driver
> to support the generic pinctrl dt configuration properties.

Yep, you should be able to support both the old and the new
way of configuring the pins this way I guess.

> And it also fix the raw data issues and looks like more applicable than the full
> migration.
>
> So would you be willing to accept it?

Anything that move us closer to the generic bindings will be accepted.

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
Dong Aisheng March 15, 2017, 2:05 p.m. UTC | #14 
Hi Linus,

On Tue, Mar 14, 2017 at 9:54 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Feb 16, 2017 at 8:51 AM, Dong Aisheng <dongas86@gmail.com> wrote:
>> On Tue, Dec 27, 2016 at 8:59 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>
>>> I would consider augmenting the code in the driver to handle the generic
>>> bindings *in addition* to the old legacy bindings, and use those over these
>>> random custom bits.
> (...)
>> Generic binding only supports parsing strings for pins and function
>> from dts right now, that means we need back to the old way of hardcoding
>> all register bits in driver which we actually chose to move out since
>> the commit "e164153 pinctrl: imx: move hard-coding data into device tree".
>
> Aha I see.
>
>> One option is using macro definitions instead of raw data as pinctrl-single
>> users as OMAP.
>> e.g.
>> art2_pins: pinmux_uart2_pins {
>>         pinctrl-single,pins = <
>>                 OMAP4_IOPAD(0x118, PIN_INPUT_PULLUP | MUX_MODE0)
>>  /* uart2_cts.uart2_cts */
>>         ........
>>
>> For this way, no driver changes required, just replace the raw data by macro
>> in device tree. But i wonder this may not be your original intention although
>> it's the easiest way for us.
>> Anyway, if you agree, we probably would prefer this way.
>
> That make it look better than it looks today, but...
>
>> Another option is partially convert to generic binding for only pad
>> configuration
>> part. e.g.
>> pinctrl_usdhc1: usdhc1grp {
>>         fsl,pins = <
>>                 MX7D_PAD_SD1_CMD__SD1_CMD
>>                 MX7D_PAD_SD1_CLK__SD1_CLK
>>                 MX7D_PAD_SD1_DATA0__SD1_DATA0
>>                 ...
>>         >;
>>         bias-pull-up = <47KOHM>;
>>         drive-strength = <130OHM>;
>>         slew-rate = <FAST>;
>>         speed = <50MHZ>;
>>         ....
>> };
>> (Some of the property still not supported or not the same as generic
>> binding right now)
>
> This looks totally awesome, so if you could do this, it would be even
> better.
>
>> This way requires no big drivers changes and only extend the driver
>> to support the generic pinctrl dt configuration properties.
>
> Yep, you should be able to support both the old and the new
> way of configuring the pins this way I guess.
>
>> And it also fix the raw data issues and looks like more applicable than the full
>> migration.
>>
>> So would you be willing to accept it?
>
> Anything that move us closer to the generic bindings will be accepted.
>

It's good to get an alignment with you.
We will start to do that.

Thanks

Regards
Dong Aisheng
--
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 mbox

Patch

diff --git a/Documentation/devicetree/bindings/pinctrl/fsl,imx6sll-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/fsl,imx6sll-pinctrl.txt
new file mode 100644
index 0000000..9561c1a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/fsl,imx6sll-pinctrl.txt
@@ -0,0 +1,37 @@ 
+* Freescale i.MX6 SLL IOMUX Controller
+
+Please refer to fsl,imx-pinctrl.txt in this directory for common binding part
+and usage.
+
+Required properties:
+- compatible: "fsl,imx6sll-iomuxc"
+- fsl,pins: each entry consists of 6 integers and represents the mux and config
+  setting for one pin.  The first 5 integers <mux_reg conf_reg input_reg mux_val
+  input_val> are specified using a PIN_FUNC_ID macro, which can be found in
+  imx6sll-pinfunc.h under device tree source folder.  The last integer CONFIG is
+  the pad setting value like pull-up on this pin.  Please refer to i.MX6SLL
+  Reference Manual for detailed CONFIG settings.
+
+CONFIG bits definition:
+PAD_CTL_LVE			(1 << 22)
+PAD_CTL_HYS                     (1 << 16)
+PAD_CTL_PUS_100K_DOWN           (0 << 14)
+PAD_CTL_PUS_47K_UP              (1 << 14)
+PAD_CTL_PUS_100K_UP             (2 << 14)
+PAD_CTL_PUS_22K_UP              (3 << 14)
+PAD_CTL_PUE                     (1 << 13)
+PAD_CTL_PKE                     (1 << 12)
+PAD_CTL_ODE                     (1 << 11)
+PAD_CTL_SPEED_LOW               (0 << 6)
+PAD_CTL_SPEED_MED               (1 << 6)
+PAD_CTL_SPEED_HIGH              (3 << 6)
+PAD_CTL_DSE_DISABLE             (0 << 3)
+PAD_CTL_DSE_260ohm              (1 << 3)
+PAD_CTL_DSE_130ohm              (2 << 3)
+PAD_CTL_DSE_87ohm               (3 << 3)
+PAD_CTL_DSE_65ohm               (4 << 3)
+PAD_CTL_DSE_52ohm               (5 << 3)
+PAD_CTL_DSE_43ohm               (6 << 3)
+PAD_CTL_DSE_37ohm               (7 << 3)
+PAD_CTL_SRE_FAST                (1 << 0)
+PAD_CTL_SRE_SLOW                (0 << 0)