Re: [U-Boot] [PATCH 55/60] i2c: tegra: move pinmux setup to board files

[Simon Glass]

Hi Stephen,

On 27 April 2016 at 11:16, Stephen Warren  wrote:
> On 04/27/2016 10:58 AM, Simon Glass wrote:
>>
>> Hi Stephen,
>>
>> On 27 April 2016 at 10:24, Stephen Warren  wrote:
>>>
>>> On 04/27/2016 09:12 AM, Simon Glass wrote:


 Hi Stephen,

 On 19 April 2016 at 14:59, Stephen Warren  wrote:
>
>
> From: Stephen Warren 
>
> Remove funcmux calls from the Tegra I2C driver. Knowledge of pinmux
> setup must come from either board files or DT; it should not be
> embedded
> into board-agnostic driver code. The DT pinmux bindings do not allow
> drivers to derive funcmux-style information, since the DT bindings are
> pin-based whereas funcmux is controller-based, so there's no good way
> to
> call the existing funcmux APIs from drivers. Converting drivers to use
> a
> new (as yet non-existent in U-Boot) API that pulls pinmux information
> from
> DT isn't useful for Tegra, since Tegra's DT files don't contain any
> per-device pinmux tables, so this would simply be extra code that has
> no
> effect; doesn't actually set up the pinmux. We are left with moving the
> pinmux setup functionality into board files. In theory the board files
> could be converted later to use DT, but that would be a separate
> change.
>
> Signed-off-by: Stephen Warren 
> ---
>board/avionic-design/common/tamonten.c  |  5 +
>board/nvidia/seaboard/seaboard.c|  3 +++
>board/nvidia/whistler/whistler.c|  1 +
>board/toradex/colibri_t20/colibri_t20.c |  3 +++
>drivers/i2c/tegra_i2c.c | 19 ---
>5 files changed, 12 insertions(+), 19 deletions(-)



 This should use driver model, which handles pinmux automatically if
 you have a pinctrl driver.
>>>
>>>
>>>
>>> Can you define "automatic"? I don't understand exactly which benefit
>>> you're
>>> describing there.
>>
>>
>> When you probe a device, its pinmux is set up automatically, so the
>> explicit funcmux calls can go away.
>
>
> So the device here would be the pin controller device itself, not individual
> I2C/SPI/SD/... devices.
>
> That's because the Tegra HW does not support[1] dynamic or partial pinmux
> configuration. As such, we need to program the entire pinmux at once early
> during boot, not piece-by-piece as the individual U-Boot devices that use
> individual pins are probed. This is why for example the kernel DT contains a
> single pinmux table that the pin controller driver sets up at boot (with
> identical configuration to what U-Boot sets up, so it's a no-op), rather
> than splitting it into per-device chunks.

Is this the big table that I see in device tree files?

pinmux@0,7868 {
pinctrl-names = "default";
pinctrl-0 = <_default>;

pinmux_default: common {
clk_32k_out_pa0 {
nvidia,pins = "clk_32k_out_pa0";
nvidia,pull = ;
nvidia,tristate = ;
nvidia,enable-input = ;
};
...

>
> As such, there isn't any need for, say, an I2C device probe to call into
> pinmux at all; the pinmux would already have been set up entirely during
> early boot, and hence no I2C-specific actions would be needed later.

OK. As you know I don't like this monolithic pinmux stuff - at least
in early boot it is useful to enable (say) just the UART. Anyway I
still think a driver is useful and will provide a better base for the
future.

>
> So I'm not sure what benefit conversion to DM pinctrl has here. Sure it
> means things get set up the same way as with any other pinctrl device or
> SoC, but this is early SoC-specific configuration, without any interaction
> with common or driver code besides being implemented via some standard
> core->board callbacks/hooks. It seems reasonable to just program the pinmux
> directly using SoC-specific APIs rather than having to add a layer of
> abstraction on top of it just so we can route it through DM. In other words,
> what's already done by this patch series.
>
> Besides, I believe the programming happens before a DM pinctrl device would
> be ready, doesn't it, given it happens from tegra_board_early_init_f()? Or,
> would we be able to fully probe a DM device at that point? The UART console
> setup is even earlier, in SPL pre-T210, where I don't think we even have DM
> enabled.

DM is enabled pretty early, before board_early_init_f(). But if you
hit a problem we can look at it. There is no point in setting up
pinmux before driver model, since it is the drivers that use it.

Worst case, how about creating a pinctrl driver where everything
happens in the probe() method? Ideally you could do the minimum
necessary before relocation, and then the full table load afterwards.

>
> [1] Yes, the HW registers can in practice be programmed bit-by-bit, simply
> because there are a number of registers and the SoC doesn't have a way to
> 

Re: [U-Boot] [PATCH 55/60] i2c: tegra: move pinmux setup to board files

[Stephen Warren]

On 04/27/2016 10:58 AM, Simon Glass wrote:

Hi Stephen,

On 27 April 2016 at 10:24, Stephen Warren  wrote:

On 04/27/2016 09:12 AM, Simon Glass wrote:


Hi Stephen,

On 19 April 2016 at 14:59, Stephen Warren  wrote:


From: Stephen Warren 

Remove funcmux calls from the Tegra I2C driver. Knowledge of pinmux
setup must come from either board files or DT; it should not be embedded
into board-agnostic driver code. The DT pinmux bindings do not allow
drivers to derive funcmux-style information, since the DT bindings are
pin-based whereas funcmux is controller-based, so there's no good way to
call the existing funcmux APIs from drivers. Converting drivers to use a
new (as yet non-existent in U-Boot) API that pulls pinmux information
from
DT isn't useful for Tegra, since Tegra's DT files don't contain any
per-device pinmux tables, so this would simply be extra code that has no
effect; doesn't actually set up the pinmux. We are left with moving the
pinmux setup functionality into board files. In theory the board files
could be converted later to use DT, but that would be a separate change.

Signed-off-by: Stephen Warren 
---
   board/avionic-design/common/tamonten.c  |  5 +
   board/nvidia/seaboard/seaboard.c|  3 +++
   board/nvidia/whistler/whistler.c|  1 +
   board/toradex/colibri_t20/colibri_t20.c |  3 +++
   drivers/i2c/tegra_i2c.c | 19 ---
   5 files changed, 12 insertions(+), 19 deletions(-)



This should use driver model, which handles pinmux automatically if
you have a pinctrl driver.



Can you define "automatic"? I don't understand exactly which benefit you're
describing there.


When you probe a device, its pinmux is set up automatically, so the
explicit funcmux calls can go away.


So the device here would be the pin controller device itself, not 
individual I2C/SPI/SD/... devices.


That's because the Tegra HW does not support[1] dynamic or partial 
pinmux configuration. As such, we need to program the entire pinmux at 
once early during boot, not piece-by-piece as the individual U-Boot 
devices that use individual pins are probed. This is why for example the 
kernel DT contains a single pinmux table that the pin controller driver 
sets up at boot (with identical configuration to what U-Boot sets up, so 
it's a no-op), rather than splitting it into per-device chunks.


As such, there isn't any need for, say, an I2C device probe to call into 
pinmux at all; the pinmux would already have been set up entirely during 
early boot, and hence no I2C-specific actions would be needed later.


So I'm not sure what benefit conversion to DM pinctrl has here. Sure it 
means things get set up the same way as with any other pinctrl device or 
SoC, but this is early SoC-specific configuration, without any 
interaction with common or driver code besides being implemented via 
some standard core->board callbacks/hooks. It seems reasonable to just 
program the pinmux directly using SoC-specific APIs rather than having 
to add a layer of abstraction on top of it just so we can route it 
through DM. In other words, what's already done by this patch series.


Besides, I believe the programming happens before a DM pinctrl device 
would be ready, doesn't it, given it happens from 
tegra_board_early_init_f()? Or, would we be able to fully probe a DM 
device at that point? The UART console setup is even earlier, in SPL 
pre-T210, where I don't think we even have DM enabled.


[1] Yes, the HW registers can in practice be programmed bit-by-bit, 
simply because there are a number of registers and the SoC doesn't have 
a way to physically force SW to write to each of those registers. 
"Support" here refers to what the ASIC team will guarantee will work 
correctly without causing glitches or similar issues. There are a few 
limited exceptions, e.g. console UART muxing on its own has been at 
least partially thought out (although there are still conflicts in some 
cases on older chips), and IO controllers that may contain boot media 
are generally OK to mux on their own. However, for anything else, i.e. 
the majority of cases, the supported model is to program everything 
up-front in one go, and not change it later. Sticking to that general 
model in absolutely all cases removes special cases and simplifies the code.

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 55/60] i2c: tegra: move pinmux setup to board files

[Simon Glass]

Hi Stephen,

On 27 April 2016 at 10:24, Stephen Warren  wrote:
> On 04/27/2016 09:12 AM, Simon Glass wrote:
>>
>> Hi Stephen,
>>
>> On 19 April 2016 at 14:59, Stephen Warren  wrote:
>>>
>>> From: Stephen Warren 
>>>
>>> Remove funcmux calls from the Tegra I2C driver. Knowledge of pinmux
>>> setup must come from either board files or DT; it should not be embedded
>>> into board-agnostic driver code. The DT pinmux bindings do not allow
>>> drivers to derive funcmux-style information, since the DT bindings are
>>> pin-based whereas funcmux is controller-based, so there's no good way to
>>> call the existing funcmux APIs from drivers. Converting drivers to use a
>>> new (as yet non-existent in U-Boot) API that pulls pinmux information
>>> from
>>> DT isn't useful for Tegra, since Tegra's DT files don't contain any
>>> per-device pinmux tables, so this would simply be extra code that has no
>>> effect; doesn't actually set up the pinmux. We are left with moving the
>>> pinmux setup functionality into board files. In theory the board files
>>> could be converted later to use DT, but that would be a separate change.
>>>
>>> Signed-off-by: Stephen Warren 
>>> ---
>>>   board/avionic-design/common/tamonten.c  |  5 +
>>>   board/nvidia/seaboard/seaboard.c|  3 +++
>>>   board/nvidia/whistler/whistler.c|  1 +
>>>   board/toradex/colibri_t20/colibri_t20.c |  3 +++
>>>   drivers/i2c/tegra_i2c.c | 19 ---
>>>   5 files changed, 12 insertions(+), 19 deletions(-)
>>
>>
>> This should use driver model, which handles pinmux automatically if
>> you have a pinctrl driver.
>
>
> Can you define "automatic"? I don't understand exactly which benefit you're
> describing there.

When you probe a device, its pinmux is set up automatically, so the
explicit funcmux calls can go away.

>
> I'd rather keep this series as simple cleanup of existing code, and handle
> any large-scale DM conversions separately later. That said, as full
> disclosure, I'm certainly not signing up for any more work on pinctrl,
> especially if it involves reading the pinmux tables from DT, as I see no
> benefit in that. Note that in future chips the boot ROM will handle 100% of
> static pinmux setup so there won't be a U-Boot driver for those SoCs going
> forward. I'd rather not invest any more than minimal effort in something
> that's going away.

There's no need for it to read from DT, and it doesn't have to be
comprehensive but I think you should create a pinctrl driver and put
the funcmux stuff in that (along with processing your big tables of
pinmux stuff).

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 55/60] i2c: tegra: move pinmux setup to board files

[Stephen Warren]

On 04/27/2016 09:12 AM, Simon Glass wrote:

Hi Stephen,

On 19 April 2016 at 14:59, Stephen Warren  wrote:

From: Stephen Warren 

Remove funcmux calls from the Tegra I2C driver. Knowledge of pinmux
setup must come from either board files or DT; it should not be embedded
into board-agnostic driver code. The DT pinmux bindings do not allow
drivers to derive funcmux-style information, since the DT bindings are
pin-based whereas funcmux is controller-based, so there's no good way to
call the existing funcmux APIs from drivers. Converting drivers to use a
new (as yet non-existent in U-Boot) API that pulls pinmux information from
DT isn't useful for Tegra, since Tegra's DT files don't contain any
per-device pinmux tables, so this would simply be extra code that has no
effect; doesn't actually set up the pinmux. We are left with moving the
pinmux setup functionality into board files. In theory the board files
could be converted later to use DT, but that would be a separate change.

Signed-off-by: Stephen Warren 
---
  board/avionic-design/common/tamonten.c  |  5 +
  board/nvidia/seaboard/seaboard.c|  3 +++
  board/nvidia/whistler/whistler.c|  1 +
  board/toradex/colibri_t20/colibri_t20.c |  3 +++
  drivers/i2c/tegra_i2c.c | 19 ---
  5 files changed, 12 insertions(+), 19 deletions(-)


This should use driver model, which handles pinmux automatically if
you have a pinctrl driver.


Can you define "automatic"? I don't understand exactly which benefit 
you're describing there.


I'd rather keep this series as simple cleanup of existing code, and 
handle any large-scale DM conversions separately later. That said, as 
full disclosure, I'm certainly not signing up for any more work on 
pinctrl, especially if it involves reading the pinmux tables from DT, as 
I see no benefit in that. Note that in future chips the boot ROM will 
handle 100% of static pinmux setup so there won't be a U-Boot driver for 
those SoCs going forward. I'd rather not invest any more than minimal 
effort in something that's going away.

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 55/60] i2c: tegra: move pinmux setup to board files

[Simon Glass]

Hi Stephen,

On 19 April 2016 at 14:59, Stephen Warren  wrote:
> From: Stephen Warren 
>
> Remove funcmux calls from the Tegra I2C driver. Knowledge of pinmux
> setup must come from either board files or DT; it should not be embedded
> into board-agnostic driver code. The DT pinmux bindings do not allow
> drivers to derive funcmux-style information, since the DT bindings are
> pin-based whereas funcmux is controller-based, so there's no good way to
> call the existing funcmux APIs from drivers. Converting drivers to use a
> new (as yet non-existent in U-Boot) API that pulls pinmux information from
> DT isn't useful for Tegra, since Tegra's DT files don't contain any
> per-device pinmux tables, so this would simply be extra code that has no
> effect; doesn't actually set up the pinmux. We are left with moving the
> pinmux setup functionality into board files. In theory the board files
> could be converted later to use DT, but that would be a separate change.
>
> Signed-off-by: Stephen Warren 
> ---
>  board/avionic-design/common/tamonten.c  |  5 +
>  board/nvidia/seaboard/seaboard.c|  3 +++
>  board/nvidia/whistler/whistler.c|  1 +
>  board/toradex/colibri_t20/colibri_t20.c |  3 +++
>  drivers/i2c/tegra_i2c.c | 19 ---
>  5 files changed, 12 insertions(+), 19 deletions(-)

This should use driver model, which handles pinmux automatically if
you have a pinctrl driver.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot