State of pinctrl and exynos5250?

State of pinctrl and exynos5250?

[Doug Anderson]

Thomas and Tomasz,

I'm trying to get my head wrapped around the state of pinctrl for
exynos5250.  I see various patches that have floated around at times
but it doesn't look like anything has landed.  It would be really nice
to get this resolved since I think it blocks getting eint support
landed for exynos5250 and that blocks getting many peripherals working
on the ARM Chromebook.

I'm still a little new to the world on pinctrl so hopefully nothing
below is too stupidly wrong...

---

Patches that seem to be relevant (NOTE: all of these need
"pinctrl-exynos5250" fixed to "exynos5250-pinctrl"):

pinctrl: exynos: add exynos5250 SoC specific data
- https://patchwork.kernel.org/patch/1871901/

gpio: samsung: skip gpiolib registration if pinctrl support is enabled
for exynos5250
- https://patchwork.kernel.org/patch/1871911/

arm: exynos: skip wakeup interrupt registration for exynos5250 if
pinctrl is enabled
- https://patchwork.kernel.org/patch/1871921/

ARM: dts: add pinctrl nodes for Exynos5250 SoC
- https://patchwork.kernel.org/patch/1871991/
--> NOTE: Appears to be missing pinctrl_1 and pinctrl_2.  That
prevents me from compiling with i2c arbitration patches landed since I
need gpf0.
--> NOTE: Appears (IIRC) to have incorrect interrupt for pinctrl_3.  I
believe that 45 is pinctrl_1.  Maybe 3 is 47?
--> NIT NOTE: It appears sd0_busN is strangely specified.
Specifically sd0_bus4 includes the pins for sd0_bus1 but sd0_bus8
doesn't.

---

I've got all of the above patches "fixed up" in my local tree
(including adding really basic support for pinctrl_1 and pinctrl_2).
...but my machine doesn't boot all the way.  I think that's because
many of the peripherals don't yet understand pinctrl.  Specifically I
get delightful looking error messages at bootup that look like:

[    0.440000] _gpio_request: gpio-36 (i2c-bus) status -16
[    0.445000] s3c-i2c s3c2440-i2c.0: gpio [36] request failed

I then replaced some of the "muxing via GPIO" with "muxing via
pinctrl" for i2c parts, like:
-               gpios = <&gpb3 0 2 3 0>,
-                       <&gpb3 1 2 3 0>;
+               pinctrl-0 = <&i2c0_bus>;
+               pinctrl-names = "default";

...and I got rid of those errors, but it looks like we're missing
pinctrl support for the ever-important "dw_mmc".

[    0.910000] Synopsys Designware Multimedia Card Interface Driver
[    0.915000] dwmmc_exynos dw_mmc.0: Using internal DMA controller.
[    0.920000] dwmmc_exynos dw_mmc.0: DW MMC controller at irq 107, 32
bit host data width, 128 deep fifo
[    0.930000] of_get_named_gpio_flags exited with status 40
[    0.935000] of_get_named_gpio_flags: can't parse gpios property
[    0.940000] dwmmc_exynos dw_mmc.0: invalid gpio: -2
[    0.945000] dwmmc_exynos: probe of dw_mmc.0 failed with error -22

We also need it for "spi-s3c64xx.c" but that's less of a critical component.
[    0.405000] of_get_named_gpio_flags exited with status 18
[    0.410000] /spi@12d30000: could not get #gpio-cells for
/pinctrl@11400000/i2c0-bus
[    0.415000] of_get_named_gpio_flags: can't parse gpios property
[    0.420000] s3c64xx-spi exynos4210-spi.1: invalid gpio[1]: -22
[    0.425000] s3c64xx-spi: probe of exynos4210-spi.1 failed with error -16

Those are the drivers that have their muxing state defined using the
old hacky samsung gpio descriptor.


Hmmm, it also looks like I need to update other "gpio" references too
(gpio-keys, i2c-arbitrator, hpd-gpio) since the GPIO specifier has
changed now that it's based on pinmux.  I guess you either need to
move a whole architecture (all of exynos5250) at once since you can't
mix and match.


IMHO that means we've got the following work ahead of us:
1. Add pinctrl support to dw_mmc-exynos (with backward compability for
GPIO specifier)
2. Add pinctrl support to spi-s3c64xx.c (with backward compability for
GPIO specifier)
3. In one fell swoop convert all exynos5 dts / dtsi files over to use
pinctrl since things will be broken with a "half" conversion.

Assuming I'm understanding #3 above, I guess that means that I "NAK"
the "ARM: dts: add pinctrl nodes for Exynos5250 SoC" patch since it
doesn't include fixup for all of the exynos5250 platforms (smdk5250
and snow) out there.

---

I'd appreciate any thoughts that you have.  I'd also appreciate a
timeframe for when this work will be picked back up, since the old
patches are 2.5 months old now...  I may be able to help a little
(especially with bits relevant to the ARM Chromebook) but I'm not sure
I can do all of the heavy lifting...  ...and I'd hate to rebase/repost
Thomas's patches myself without checking...

NOTE: I've actually hacked something together that seems to get dw_mmc
happy with pinmuxing and I can boot the system, so I guess that proves
that the above can't all be wrong.  ;)


Thanks much and sorry for the super long email!  :)

-Doug

Re: State of pinctrl and exynos5250?

[Tomasz Figa]

Hello Doug,

On Friday 01 of March 2013 16:19:39 Doug Anderson wrote:
> Thomas and Tomasz,
> 
> I'm trying to get my head wrapped around the state of pinctrl for
> exynos5250.  I see various patches that have floated around at times
> but it doesn't look like anything has landed.  It would be really nice
> to get this resolved since I think it blocks getting eint support
> landed for exynos5250 and that blocks getting many peripherals working
> on the ARM Chromebook.

It's great that finally someone noticed the problem (despite my comments 
to patches adding more and more cruft using the GPIO specifier hack).

> I'm still a little new to the world on pinctrl so hopefully nothing
> below is too stupidly wrong...

It's always better to ask than to get something completely wrong.
 
> Patches that seem to be relevant (NOTE: all of these need
> "pinctrl-exynos5250" fixed to "exynos5250-pinctrl"):

> pinctrl: exynos: add exynos5250 SoC specific data
> - https://patchwork.kernel.org/patch/1871901/
> 
> gpio: samsung: skip gpiolib registration if pinctrl support is enabled
> for exynos5250
> - https://patchwork.kernel.org/patch/1871911/
> 
> arm: exynos: skip wakeup interrupt registration for exynos5250 if
> pinctrl is enabled
> - https://patchwork.kernel.org/patch/1871921/

The 4 patches above are already merged in Kgene's for-next-next (for 3.10) 
branch.

> ARM: dts: add pinctrl nodes for Exynos5250 SoC
> - https://patchwork.kernel.org/patch/1871991/

This one is not merged yet. Since I do not know much about Exynos 5250, I 
could not verify any hardware-specific details in the patch, just the 
general correctness of the patch.

> --> NOTE: Appears to be missing pinctrl_1 and pinctrl_2.  That
> prevents me from compiling with i2c arbitration patches landed since I
> need gpf0.

Indeed. I would prefer adding all of them at once.

> --> NOTE: Appears (IIRC) to have incorrect interrupt for pinctrl_3.  I
> believe that 45 is pinctrl_1.  Maybe 3 is 47?

I can't verify this, unfortunately.

> --> NIT NOTE: It appears sd0_busN is strangely specified.
> Specifically sd0_bus4 includes the pins for sd0_bus1 but sd0_bus8
> doesn't.

Yes, it is at least somewhat inconsitent. For now this is not really a 
problem, but eventually it will have to be sanitized. However, note that 
although sd0_bus8 has the same samsung,pin-function as sd0_bus4, this is 
no longer the case for sd2_bus8 and sd2_bus4.

I would suggest making all the three separate from each other, so 1-bit 
bus node would just specify sd0_bus1, 4-bit would specify sd0_bus1 and 
sd0_bus4 and 8-bit all the three.
 
> ---
> 
> I've got all of the above patches "fixed up" in my local tree
> (including adding really basic support for pinctrl_1 and pinctrl_2).
> ...but my machine doesn't boot all the way.  I think that's because
> many of the peripherals don't yet understand pinctrl.  Specifically I
> get delightful looking error messages at bootup that look like:
> 
> [    0.440000] _gpio_request: gpio-36 (i2c-bus) status -16
> [    0.445000] s3c-i2c s3c2440-i2c.0: gpio [36] request failed
> 
> I then replaced some of the "muxing via GPIO" with "muxing via
> pinctrl" for i2c parts, like:
> -               gpios = <&gpb3 0 2 3 0>,
> -                       <&gpb3 1 2 3 0>;
> +               pinctrl-0 = <&i2c0_bus>;
> +               pinctrl-names = "default";
> 
> ...and I got rid of those errors, but it looks like we're missing
> pinctrl support for the ever-important "dw_mmc".
> 
> [    0.910000] Synopsys Designware Multimedia Card Interface Driver
> [    0.915000] dwmmc_exynos dw_mmc.0: Using internal DMA controller.
> [    0.920000] dwmmc_exynos dw_mmc.0: DW MMC controller at irq 107, 32
> bit host data width, 128 deep fifo
> [    0.930000] of_get_named_gpio_flags exited with status 40
> [    0.935000] of_get_named_gpio_flags: can't parse gpios property
> [    0.940000] dwmmc_exynos dw_mmc.0: invalid gpio: -2
> [    0.945000] dwmmc_exynos: probe of dw_mmc.0 failed with error -22
> 
> We also need it for "spi-s3c64xx.c" but that's less of a critical
> component. [    0.405000] of_get_named_gpio_flags exited with status 18
> [    0.410000] /spi@12d30000: could not get #gpio-cells for
> /pinctrl@11400000/i2c0-bus
> [    0.415000] of_get_named_gpio_flags: can't parse gpios property
> [    0.420000] s3c64xx-spi exynos4210-spi.1: invalid gpio[1]: -22
> [    0.425000] s3c64xx-spi: probe of exynos4210-spi.1 failed with error
> -16
> 
> Those are the drivers that have their muxing state defined using the
> old hacky samsung gpio descriptor.
> 
> 
> Hmmm, it also looks like I need to update other "gpio" references too
> (gpio-keys, i2c-arbitrator, hpd-gpio) since the GPIO specifier has
> changed now that it's based on pinmux.  I guess you either need to
> move a whole architecture (all of exynos5250) at once since you can't
> mix and match.

Correct.

> 
> IMHO that means we've got the following work ahead of us:
> 1. Add pinctrl support to dw_mmc-exynos (with backward compability for
> GPIO specifier)
> 2. Add pinctrl support to spi-s3c64xx.c (with backward compability for
> GPIO specifier)

There is a patch that should be already merged that makes the driver core 
set default pinctrl state for a device before entering driver's probe 
callback.

In case when the driver doesn't require more states than just the default 
one you don't have to add pinctrl support to the driver at all, just 
specify appropriate pinctrl properties in device tree (with only one state 
listed in pinctrl-names called "default").

However in some drivers this might be prevented by legacy pin 
configuration code which would fail.

> 3. In one fell swoop convert all exynos5 dts / dtsi files over to use
> pinctrl since things will be broken with a "half" conversion.
> 
> Assuming I'm understanding #3 above, I guess that means that I "NAK"
> the "ARM: dts: add pinctrl nodes for Exynos5250 SoC" patch since it
> doesn't include fixup for all of the exynos5250 platforms (smdk5250
> and snow) out there.

Yes, this seems reasonable.

> 
> ---
> 
> I'd appreciate any thoughts that you have.  I'd also appreciate a
> timeframe for when this work will be picked back up, since the old
> patches are 2.5 months old now...  I may be able to help a little
> (especially with bits relevant to the ARM Chromebook) but I'm not sure
> I can do all of the heavy lifting...  ...and I'd hate to rebase/repost
> Thomas's patches myself without checking...

AFAIK, Thomas had been doing something in this matter, but I haven't seen 
any progress since the discussion by the way of the patches you linked.

> NOTE: I've actually hacked something together that seems to get dw_mmc
> happy with pinmuxing and I can boot the system, so I guess that proves
> that the above can't all be wrong.  ;)

Good. It would be great if we could finally move all the DT-enabled 
Samsung architectures to pin control and drop the hacky GPIO DT support.

Best regards,
Tomasz

Re: State of pinctrl and exynos5250?

[Thomas Abraham]

Hi Doug,

On 2 March 2013 17:18, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hello Doug,
>
> On Friday 01 of March 2013 16:19:39 Doug Anderson wrote:
>> Thomas and Tomasz,
>>
>> I'm trying to get my head wrapped around the state of pinctrl for
>> exynos5250.  I see various patches that have floated around at times
>> but it doesn't look like anything has landed.  It would be really nice
>> to get this resolved since I think it blocks getting eint support
>> landed for exynos5250 and that blocks getting many peripherals working
>> on the ARM Chromebook.
>
> It's great that finally someone noticed the problem (despite my comments
> to patches adding more and more cruft using the GPIO specifier hack).
>
>> I'm still a little new to the world on pinctrl so hopefully nothing
>> below is too stupidly wrong...
>
> It's always better to ask than to get something completely wrong.
>
>> Patches that seem to be relevant (NOTE: all of these need
>> "pinctrl-exynos5250" fixed to "exynos5250-pinctrl"):
>
>> pinctrl: exynos: add exynos5250 SoC specific data
>> - https://patchwork.kernel.org/patch/1871901/
>>
>> gpio: samsung: skip gpiolib registration if pinctrl support is enabled
>> for exynos5250
>> - https://patchwork.kernel.org/patch/1871911/
>>
>> arm: exynos: skip wakeup interrupt registration for exynos5250 if
>> pinctrl is enabled
>> - https://patchwork.kernel.org/patch/1871921/
>
> The 4 patches above are already merged in Kgene's for-next-next (for 3.10)
> branch.
>
>> ARM: dts: add pinctrl nodes for Exynos5250 SoC
>> - https://patchwork.kernel.org/patch/1871991/
>
> This one is not merged yet. Since I do not know much about Exynos 5250, I
> could not verify any hardware-specific details in the patch, just the
> general correctness of the patch.

This patch was not merged since individual drivers needed modification
without which it would break existing Exynos5250 based platforms.

>
>> --> NOTE: Appears to be missing pinctrl_1 and pinctrl_2.  That
>> prevents me from compiling with i2c arbitration patches landed since I
>> need gpf0.
>
> Indeed. I would prefer adding all of them at once.

Ok. I will repost this patch again with pinctrl_1 and pinctrl_2
included. I had not included this in the earlier patch since I was not
sure of the best pin grouping for the camera and c2c interface.

>
>> --> NOTE: Appears (IIRC) to have incorrect interrupt for pinctrl_3.  I
>> believe that 45 is pinctrl_1.  Maybe 3 is 47?
>
> I can't verify this, unfortunately.

The documentation does not state this clearly. I will recheck on this
and correct as needed.

>
>> --> NIT NOTE: It appears sd0_busN is strangely specified.
>> Specifically sd0_bus4 includes the pins for sd0_bus1 but sd0_bus8
>> doesn't.
>
> Yes, it is at least somewhat inconsitent. For now this is not really a
> problem, but eventually it will have to be sanitized. However, note that
> although sd0_bus8 has the same samsung,pin-function as sd0_bus4, this is
> no longer the case for sd2_bus8 and sd2_bus4.
>
> I would suggest making all the three separate from each other, so 1-bit
> bus node would just specify sd0_bus1, 4-bit would specify sd0_bus1 and
> sd0_bus4 and 8-bit all the three.

Ok. I will do this change in the next version of the patch.

>
>> ---
>>
>> I've got all of the above patches "fixed up" in my local tree
>> (including adding really basic support for pinctrl_1 and pinctrl_2).
>> ...but my machine doesn't boot all the way.  I think that's because
>> many of the peripherals don't yet understand pinctrl.  Specifically I
>> get delightful looking error messages at bootup that look like:
>>
>> [    0.440000] _gpio_request: gpio-36 (i2c-bus) status -16
>> [    0.445000] s3c-i2c s3c2440-i2c.0: gpio [36] request failed
>>
>> I then replaced some of the "muxing via GPIO" with "muxing via
>> pinctrl" for i2c parts, like:
>> -               gpios = <&gpb3 0 2 3 0>,
>> -                       <&gpb3 1 2 3 0>;
>> +               pinctrl-0 = <&i2c0_bus>;
>> +               pinctrl-names = "default";
>>
>> ...and I got rid of those errors, but it looks like we're missing
>> pinctrl support for the ever-important "dw_mmc".
>>
>> [    0.910000] Synopsys Designware Multimedia Card Interface Driver
>> [    0.915000] dwmmc_exynos dw_mmc.0: Using internal DMA controller.
>> [    0.920000] dwmmc_exynos dw_mmc.0: DW MMC controller at irq 107, 32
>> bit host data width, 128 deep fifo
>> [    0.930000] of_get_named_gpio_flags exited with status 40
>> [    0.935000] of_get_named_gpio_flags: can't parse gpios property
>> [    0.940000] dwmmc_exynos dw_mmc.0: invalid gpio: -2
>> [    0.945000] dwmmc_exynos: probe of dw_mmc.0 failed with error -22
>>
>> We also need it for "spi-s3c64xx.c" but that's less of a critical
>> component. [    0.405000] of_get_named_gpio_flags exited with status 18
>> [    0.410000] /spi@12d30000: could not get #gpio-cells for
>> /pinctrl@11400000/i2c0-bus
>> [    0.415000] of_get_named_gpio_flags: can't parse gpios property
>> [    0.420000] s3c64xx-spi exynos4210-spi.1: invalid gpio[1]: -22
>> [    0.425000] s3c64xx-spi: probe of exynos4210-spi.1 failed with error
>> -16
>>
>> Those are the drivers that have their muxing state defined using the
>> old hacky samsung gpio descriptor.
>>
>>
>> Hmmm, it also looks like I need to update other "gpio" references too
>> (gpio-keys, i2c-arbitrator, hpd-gpio) since the GPIO specifier has
>> changed now that it's based on pinmux.  I guess you either need to
>> move a whole architecture (all of exynos5250) at once since you can't
>> mix and match.
>
> Correct.
>
>>
>> IMHO that means we've got the following work ahead of us:
>> 1. Add pinctrl support to dw_mmc-exynos (with backward compability for
>> GPIO specifier)
>> 2. Add pinctrl support to spi-s3c64xx.c (with backward compability for
>> GPIO specifier)
>
> There is a patch that should be already merged that makes the driver core
> set default pinctrl state for a device before entering driver's probe
> callback.
>
> In case when the driver doesn't require more states than just the default
> one you don't have to add pinctrl support to the driver at all, just
> specify appropriate pinctrl properties in device tree (with only one state
> listed in pinctrl-names called "default").
>
> However in some drivers this might be prevented by legacy pin
> configuration code which would fail.
>
>> 3. In one fell swoop convert all exynos5 dts / dtsi files over to use
>> pinctrl since things will be broken with a "half" conversion.
>>
>> Assuming I'm understanding #3 above, I guess that means that I "NAK"
>> the "ARM: dts: add pinctrl nodes for Exynos5250 SoC" patch since it
>> doesn't include fixup for all of the exynos5250 platforms (smdk5250
>> and snow) out there.
>
> Yes, this seems reasonable.
>
>>
>> ---
>>
>> I'd appreciate any thoughts that you have.  I'd also appreciate a
>> timeframe for when this work will be picked back up, since the old
>> patches are 2.5 months old now...  I may be able to help a little
>> (especially with bits relevant to the ARM Chromebook) but I'm not sure
>> I can do all of the heavy lifting...  ...and I'd hate to rebase/repost
>> Thomas's patches myself without checking...
>
> AFAIK, Thomas had been doing something in this matter, but I haven't seen
> any progress since the discussion by the way of the patches you linked.

I have posted i2c pinctrl support patch based on LinusW's pin grab by
device core patch. If that is acceptable, I can post pinctrl support
patches for other other controllers as well.

>
>> NOTE: I've actually hacked something together that seems to get dw_mmc
>> happy with pinmuxing and I can boot the system, so I guess that proves
>> that the above can't all be wrong.  ;)
>
> Good. It would be great if we could finally move all the DT-enabled
> Samsung architectures to pin control and drop the hacky GPIO DT support.
>
> Best regards,
> Tomasz
>

Thanks,
Thomas.

Re: State of pinctrl and exynos5250?

[Doug Anderson]

Tomasz,

Thanks for your response.


On Sat, Mar 2, 2013 at 3:48 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>
> The 4 patches above are already merged in Kgene's for-next-next (for 3.10)
> branch.

Excellent.  I see them now.  I haven't yet seen them show up in
linux-next (which is where I tend to look first), though...

>> ARM: dts: add pinctrl nodes for Exynos5250 SoC
>> - https://patchwork.kernel.org/patch/1871991/
>
> This one is not merged yet. Since I do not know much about Exynos 5250, I
> could not verify any hardware-specific details in the patch, just the
> general correctness of the patch.

OK, good.  That means you guys came to the same conclusions that I did.  :)

>> IMHO that means we've got the following work ahead of us:
>> 1. Add pinctrl support to dw_mmc-exynos (with backward compability for
>> GPIO specifier)
>> 2. Add pinctrl support to spi-s3c64xx.c (with backward compability for
>> GPIO specifier)
>
> There is a patch that should be already merged that makes the driver core
> set default pinctrl state for a device before entering driver's probe
> callback.
>
> In case when the driver doesn't require more states than just the default
> one you don't have to add pinctrl support to the driver at all, just
> specify appropriate pinctrl properties in device tree (with only one state
> listed in pinctrl-names called "default").
>
> However in some drivers this might be prevented by legacy pin
> configuration code which would fail.

Nice to know about.  In the very least I'm pretty sure that we'll need
a patch to make the GPIO settings optional.  Looking at
dw_mci_exynos_setup_bus it's a fatal error if the GPIOs are missing.
The dw_mmc stuff will also be interesting since we'll need to figure
out if the muxing needs to be specified on a per-slot level.  I think
that most of the automatic stuff won't work in that case.

It sounds like Thomas is planning on taking this on?

-Doug

Re: State of pinctrl and exynos5250?

[Thomas Abraham]

On 6 March 2013 04:59, Doug Anderson <dianders@chromium.org> wrote:
> Tomasz,
>
> Thanks for your response.
>
>
> On Sat, Mar 2, 2013 at 3:48 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>
>> The 4 patches above are already merged in Kgene's for-next-next (for 3.10)
>> branch.
>
> Excellent.  I see them now.  I haven't yet seen them show up in
> linux-next (which is where I tend to look first), though...
>
>>> ARM: dts: add pinctrl nodes for Exynos5250 SoC
>>> - https://patchwork.kernel.org/patch/1871991/
>>
>> This one is not merged yet. Since I do not know much about Exynos 5250, I
>> could not verify any hardware-specific details in the patch, just the
>> general correctness of the patch.
>
> OK, good.  That means you guys came to the same conclusions that I did.  :)
>
>>> IMHO that means we've got the following work ahead of us:
>>> 1. Add pinctrl support to dw_mmc-exynos (with backward compability for
>>> GPIO specifier)
>>> 2. Add pinctrl support to spi-s3c64xx.c (with backward compability for
>>> GPIO specifier)
>>
>> There is a patch that should be already merged that makes the driver core
>> set default pinctrl state for a device before entering driver's probe
>> callback.
>>
>> In case when the driver doesn't require more states than just the default
>> one you don't have to add pinctrl support to the driver at all, just
>> specify appropriate pinctrl properties in device tree (with only one state
>> listed in pinctrl-names called "default").
>>
>> However in some drivers this might be prevented by legacy pin
>> configuration code which would fail.
>
> Nice to know about.  In the very least I'm pretty sure that we'll need
> a patch to make the GPIO settings optional.  Looking at
> dw_mci_exynos_setup_bus it's a fatal error if the GPIOs are missing.
> The dw_mmc stuff will also be interesting since we'll need to figure
> out if the muxing needs to be specified on a per-slot level.  I think
> that most of the automatic stuff won't work in that case.
>
> It sounds like Thomas is planning on taking this on?

Hi Doug,

Yes, I am currently held up with supporting default pin states at slot
level. One option that could be considered is to list out the default
pin states for all slots in the parent node of the slots. So it would
be something like below as an example.

        dwmmc2@12220000 {
               <....>

                pinctrl-names = "slot0-default", "slot1-default";
                pinctrl-0 = <&sd2_clk &sd2_cmd &sd2_cd &sd2_bus4>;
                pinctrl-1 = <&sd2_clk &sd2_cmd &sd2_cd &sd2_bus4 &sd2_bus8>;

                slot@0 {
                        reg = <0>;
                        bus-width = <4>;
                        disable-wp;
                };

                slot@1 {
                        reg = <0>;
                        bus-width = <8>;
                        disable-wp;
                };
        };

Then, during the initial slot setup stage, the driver, knowing the
current slot number that is being setup, can setup the pin state for
the particular slot using the devm_pinctrl_get_select API. pinctrl
subsystem would not setup the default pin state in this case.

Does this look like a hack? For now, I have prepared patches without
this approach, since Exynos5250 has just one slot, I have just listed
only one default pin state, which the pinctrl framework helps to setup
during driver bind.

Thanks,
Thomas.

>
> -Doug

Re: State of pinctrl and exynos5250?

[Doug Anderson]

Thomas,

On Mon, Mar 4, 2013 at 6:04 AM, Thomas Abraham
<thomas.abraham@linaro.org> wrote:
>
> Ok. I will repost this patch again with pinctrl_1 and pinctrl_2
> included. I had not included this in the earlier patch since I was not
> sure of the best pin grouping for the camera and c2c interface.

OK, thanks.

>>> --> NOTE: Appears (IIRC) to have incorrect interrupt for pinctrl_3.  I
>>> believe that 45 is pinctrl_1.  Maybe 3 is 47?
>>
>> I can't verify this, unfortunately.
>
> The documentation does not state this clearly. I will recheck on this
> and correct as needed.

OK.  Let me know if you can't find it.  I spent a bunch of time
digging in this area and wrote up my findings, but I certainly could
be wrong.  ;)

> I have posted i2c pinctrl support patch based on LinusW's pin grab by
> device core patch. If that is acceptable, I can post pinctrl support
> patches for other other controllers as well.

OK, I see that.  <https://patchwork.kernel.org/patch/2212731/>.  That
particular patch isn't strictly required, right?  Even without that
patch I can specify pinctrl for the i2c nodes and it will work
OK--this just removes the old gpio nodes.

One problem I see with taking this approach for DW_MMC and for SPI is
that it will make bisecting hard.  If you land a change to take out
GPIO stuff from dw_mmc and the spi driver then it will make the
transition difficult.  exynos5 boards will stop booting until the
device tree support is also landed.  ...and it would be an awfully big
patch to add all the device tree stuff in the same patch as the dw_mmc
and spi driver changes...


-Doug

RE: State of pinctrl and exynos5250?

[Kukjin Kim]

Doug Anderson wrote:
> 
> Tomasz,
> 
> Thanks for your response.
> 
> 
> On Sat, Mar 2, 2013 at 3:48 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> >
> > The 4 patches above are already merged in Kgene's for-next-next (for
> 3.10)
> > branch.
> 
> Excellent.  I see them now.  I haven't yet seen them show up in
> linux-next (which is where I tend to look first), though...
> 
Just note, I'm sorting them out. So you will see them in linux-next within a
couple of days.

BRs,

- Kukjin

Re: State of pinctrl and exynos5250?

[Thomas Abraham]

On 6 March 2013 05:20, Doug Anderson <dianders@chromium.org> wrote:
> Thomas,
>
> On Mon, Mar 4, 2013 at 6:04 AM, Thomas Abraham
> <thomas.abraham@linaro.org> wrote:
>>
>> Ok. I will repost this patch again with pinctrl_1 and pinctrl_2
>> included. I had not included this in the earlier patch since I was not
>> sure of the best pin grouping for the camera and c2c interface.
>
> OK, thanks.

I have added pinctrl_1 and pinctrl_2 now.

>
>>>> --> NOTE: Appears (IIRC) to have incorrect interrupt for pinctrl_3.  I
>>>> believe that 45 is pinctrl_1.  Maybe 3 is 47?
>>>
>>> I can't verify this, unfortunately.
>>
>> The documentation does not state this clearly. I will recheck on this
>> and correct as needed.
>
> OK.  Let me know if you can't find it.  I spent a bunch of time
> digging in this area and wrote up my findings, but I certainly could
> be wrong.  ;)

So now I have: pinctrl_0 is SPI[46], pinctrl_1 is SPI[45], pinctrl_2
is SPI[50] and pinctrl_3 is SPI[47]. I am yet to test this and
confirm. If you feel these are wrong, could you please let me know.

>
>> I have posted i2c pinctrl support patch based on LinusW's pin grab by
>> device core patch. If that is acceptable, I can post pinctrl support
>> patches for other other controllers as well.
>
> OK, I see that.  <https://patchwork.kernel.org/patch/2212731/>.  That
> particular patch isn't strictly required, right?  Even without that
> patch I can specify pinctrl for the i2c nodes and it will work
> OK--this just removes the old gpio nodes.
>
> One problem I see with taking this approach for DW_MMC and for SPI is
> that it will make bisecting hard.  If you land a change to take out
> GPIO stuff from dw_mmc and the spi driver then it will make the
> transition difficult.  exynos5 boards will stop booting until the
> device tree support is also landed.  ...and it would be an awfully big
> patch to add all the device tree stuff in the same patch as the dw_mmc
> and spi driver changes...

Right, I did not pay attention to the bisecting problem. What I have
now is, a set of individual patches that migrate i2c, sdhci, dwmmc,
i2s, spi and keypad drivers to pinctrl based pin-configuration. In
addition to that, two patches that add default pin-state information
for Exynos4 and Exynos5 platforms. Since there are multiple drivers
involved, is it okay to keep them as separate patches?

Thanks,
Thomas.

>
>
> -Doug

Re: State of pinctrl and exynos5250?

[Doug Anderson]

Thomas,

On Tue, Mar 5, 2013 at 3:49 PM, Thomas Abraham
<thomas.abraham@linaro.org> wrote:
> Yes, I am currently held up with supporting default pin states at slot
> level. One option that could be considered is to list out the default
> pin states for all slots in the parent node of the slots. So it would
> be something like below as an example.
>
>         dwmmc2@12220000 {
>                <....>
>
>                 pinctrl-names = "slot0-default", "slot1-default";
>                 pinctrl-0 = <&sd2_clk &sd2_cmd &sd2_cd &sd2_bus4>;
>                 pinctrl-1 = <&sd2_clk &sd2_cmd &sd2_cd &sd2_bus4 &sd2_bus8>;
>
>                 slot@0 {
>                         reg = <0>;
>                         bus-width = <4>;
>                         disable-wp;
>                 };
>
>                 slot@1 {
>                         reg = <0>;
>                         bus-width = <8>;
>                         disable-wp;
>                 };
>         };
>
> Then, during the initial slot setup stage, the driver, knowing the
> current slot number that is being setup, can setup the pin state for
> the particular slot using the devm_pinctrl_get_select API. pinctrl
> subsystem would not setup the default pin state in this case.
>
> Does this look like a hack?

That feels like a hack to me.  The right place for the slot data is
under the slot node, not in the parent.  ...but I would certainly
defer to any experts that might have a different opinion.


> For now, I have prepared patches without
> this approach, since Exynos5250 has just one slot, I have just listed
> only one default pin state, which the pinctrl framework helps to setup
> during driver bind.

That actually doesn't seem awful to me.  It's basically saying that we
are describing pinmux at a device level and not a slot level.  It
would make it a pain to dynamically adjust pinmux on a slot-by-slot
basis, but is that a terrible thing?

In the two-slot case you could just do:

  pinctrl-0 = <&sd1_clk &sd1_cmd &sd1_cd &sd2_bus4 &sd2_clk &sd2_cmd
&sd1_cd &sd1_bus4>;
  pinctrl-names = "default";

In other words the default state just sets up pin muxing for all the
whole controller (AKA all slots).

...but maybe someone will yell at me for that suggestion.  It's really
hard to justify all this extra work for the multiple slot case when
there are no current users that I know of...


-Doug

Re: State of pinctrl and exynos5250?

[Doug Anderson]

Thomas,

On Tue, Mar 5, 2013 at 4:01 PM, Thomas Abraham
<thomas.abraham@linaro.org> wrote:
> So now I have: pinctrl_0 is SPI[46], pinctrl_1 is SPI[45], pinctrl_2
> is SPI[50] and pinctrl_3 is SPI[47]. I am yet to test this and
> confirm. If you feel these are wrong, could you please let me know.

Yes, that matches my findings / guesses.  ;)


> Right, I did not pay attention to the bisecting problem. What I have
> now is, a set of individual patches that migrate i2c, sdhci, dwmmc,
> i2s, spi and keypad drivers to pinctrl based pin-configuration. In
> addition to that, two patches that add default pin-state information
> for Exynos4 and Exynos5 platforms. Since there are multiple drivers
> involved, is it okay to keep them as separate patches?

My own personal opinion is that it's worth keeping bisect working and
also a good idea to keep the patches to the individual drivers
separate.  That would mean adding support for either pinctrl or gpio
in the short term (like Linus did for i2c).  Once everything is moved
over then killing gpio support would be OK by me.

I'd feel most strongly about not killing dw_mmc since that is such a
critical driver for booting.  i2s/spi/keypad are a little less
critical and I'd be more OK with them breaking as long as it was clear
in the commit message that breakage is expected.

I'm a bit of a latecomer to this conversation, though, so I'm not sure
my ack / nak holds much weight.  If someone else thinks it's more
important to kill the old GPIO stuff (or more important to avoid
useless churn) then their opinion ought to hold.

-Doug

Re: State of pinctrl and exynos5250?

[Linus Walleij]

On Wed, Mar 6, 2013 at 12:49 AM, Thomas Abraham
<thomas.abraham@linaro.org> wrote:

> Yes, I am currently held up with supporting default pin states at slot
> level. One option that could be considered is to list out the default
> pin states for all slots in the parent node of the slots. So it would
> be something like below as an example.
>
>         dwmmc2@12220000 {
>                <....>
>
>                 pinctrl-names = "slot0-default", "slot1-default";
>                 pinctrl-0 = <&sd2_clk &sd2_cmd &sd2_cd &sd2_bus4>;
>                 pinctrl-1 = <&sd2_clk &sd2_cmd &sd2_cd &sd2_bus4 &sd2_bus8>;
>
>                 slot@0 {
>                         reg = <0>;
>                         bus-width = <4>;
>                         disable-wp;
>                 };
>
>                 slot@1 {
>                         reg = <0>;
>                         bus-width = <8>;
>                         disable-wp;
>                 };
>         };

I don't quite understand the above. Is it such that there is one device,
with two slots, and in the device model you have represented this
two-slot device with a single struct device?

Have you considered just registering one device for each slot?

That would make things quite a lot simpler, just a single pinctrl
handle per device, right?

You need to pass this by Stephen Warren, I think he had a similar
case in the Tegra.

Yours,
Linus Walleij

Re: State of pinctrl and exynos5250?

[Doug Anderson]

Linus,

+dw_mmc folks and Stephen Warren : for context here, we are discussing
device tree bindings for pinmux for dw_mmc.  The issue at hand is
whether they belong under the slot node or under the top-level device
node.


On Wed, Mar 6, 2013 at 11:57 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> I don't quite understand the above. Is it such that there is one device,
> with two slots, and in the device model you have represented this
> two-slot device with a single struct device?

Yes, that's the issue.  That's dw_mmc that has been in the kernel for
a bit of time now (looks like Jan 2011) and has had a single struct
device for as long as I've been looking at it.

Relevant links for convenience:
* https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/dw_mmc.c
* https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mmc/synopsis-dw-mshc.txt
* https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/exynos5250.dtsi#n243
* https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/cros5250-common.dtsi#n92


> Have you considered just registering one device for each slot?
>
> That would make things quite a lot simpler, just a single pinctrl
> handle per device, right?

I don't know why the original decision was made to just have one
struct device.  ...that would be a pretty big code change at this
point, I think.

...I think the most important issue at hand is the device tree
bindings for pinmux on this device.  It sounds like you are in
agreement that the best thing is to have a pinmux specified per-slot.
If the code is a bit awkward now (due to not having a struct device
per slot) then that's just something we have to live with.


-Doug

Re: State of pinctrl and exynos5250?

[Stephen Warren]

On 03/07/2013 09:27 AM, Doug Anderson wrote:
> +the proper address for Will.  Sorry...
> 
> 
> On Thu, Mar 7, 2013 at 8:13 AM, Doug Anderson <dianders@chromium.org
> <mailto:dianders@chromium.org>> wrote:
> 
>     Linus,
> 
>     +dw_mmc folks and Stephen Warren : for context here, we are discussing
>     device tree bindings for pinmux for dw_mmc.  The issue at hand is
>     whether they belong under the slot node or under the top-level device
>     node.

There's no need for dynamic pin-muxing for MMC AFAIK, so I'd expect a
single pinctrl state "default" to exist that covers any/all requirements
of both slots' pinmux configuration.

>     On Wed, Mar 6, 2013 at 11:57 PM, Linus Walleij
>     <linus.walleij@linaro.org <mailto:linus.walleij@linaro.org>> wrote:
>     > I don't quite understand the above. Is it such that there is one
>     device,
>     > with two slots, and in the device model you have represented this
>     > two-slot device with a single struct device?
> 
>     Yes, that's the issue.  That's dw_mmc that has been in the kernel for
>     a bit of time now (looks like Jan 2011) and has had a single struct
>     device for as long as I've been looking at it.
> 
>     Relevant links for convenience:
>     *
>     https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/dw_mmc.c
>     *
>     https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mmc/synopsis-dw-mshc.txt
>     *
>     https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/exynos5250.dtsi#n243
>     *
>     https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/cros5250-common.dtsi#n92
> 
> 
>     > Have you considered just registering one device for each slot?
>     >
>     > That would make things quite a lot simpler, just a single pinctrl
>     > handle per device, right?
> 
>     I don't know why the original decision was made to just have one
>     struct device.  ...that would be a pretty big code change at this
>     point, I think.
> 
>     ...I think the most important issue at hand is the device tree
>     bindings for pinmux on this device.  It sounds like you are in
>     agreement that the best thing is to have a pinmux specified per-slot.
>     If the code is a bit awkward now (due to not having a struct device
>     per slot) then that's just something we have to live with.
> 
> 
>     -Doug
> 
> 

Re: State of pinctrl and exynos5250?

[Linus Walleij]

On Thu, Mar 7, 2013 at 5:13 PM, Doug Anderson <dianders@chromium.org> wrote:

> ...I think the most important issue at hand is the device tree
> bindings for pinmux on this device.  It sounds like you are in
> agreement that the best thing is to have a pinmux specified per-slot.

I don't know, Stephen's point to have one single pinctrl set-up for the
entire device seems perfectly valid, why do you need to specify it
per-slot at all?

Yours,
Linus Walleij

Re: State of pinctrl and exynos5250?

[Doug Anderson]

Linus,

On Thu, Mar 7, 2013 at 6:04 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Mar 7, 2013 at 5:13 PM, Doug Anderson <dianders@chromium.org> wrote:
>
>> ...I think the most important issue at hand is the device tree
>> bindings for pinmux on this device.  It sounds like you are in
>> agreement that the best thing is to have a pinmux specified per-slot.
>
> I don't know, Stephen's point to have one single pinctrl set-up for the
> entire device seems perfectly valid, why do you need to specify it
> per-slot at all?

OK by me.  It seemed more "pure" to specify it per-slot (in case we
ever did have a struct device per slot, it would make live easier).
...but it doesn't seem incorrect to have it once for the entire
device.

:)

-Doug

P.S. sorry for double mail for some people...