[U-Boot] [PATCH] pinctrl: renesas: Fix linker error when PINCTRL_PFC=n

[Marek Vasut]

On 4/3/19 4:01 PM, Marek Vasut wrote:
> On 4/3/19 2:30 PM, Dirk Behme wrote:
>> On 03.04.2019 14:11, Marek Vasut wrote:
>>> On 4/2/19 7:02 PM, Eugeniu Rosca wrote:
>>>> On Tue, Apr 02, 2019 at 06:02:46PM +0200, Marek Vasut wrote:
>>>>> On 4/2/19 5:40 PM, Eugeniu Rosca wrote:
>>>>>> On Tue, Apr 02, 2019 at 05:28:43PM +0200, Marek Vasut wrote:
>>>>>>> On 4/2/19 5:17 PM, Dirk Behme wrote:
>>>>>>>> On 02.04.19 15:34, Marek Vasut wrote:
>>>>>>>>> On 4/2/19 3:18 PM, Eugeniu Rosca wrote:
>>>>>>>>>> With CONFIG_PINCTRL_PFC=n, aarch64-linux-gnu-ld reports:
>>>>>>>>>>
>>>>>>>>>> -----8<-----
>>>>>>>>>>     LD      u-boot
>>>>>>>>>> drivers/gpio/built-in.o: In function `rcar_gpio_request':
>>>>>>>>>> drivers/gpio/gpio-rcar.c:128: undefined reference to
>>>>>>>>>> `sh_pfc_config_mux_for_gpio'
>>>>>>>>>> -----8<-----
>>>> [..]
>>>>>>>>> Does CONFIG_PINCTRL_PFC=n produce a bootable binary ?
>>>>>>>>
>>>>>>>> Why not? Main memory, boot device and UART are configured before
>>>>>>>> U-Boot,
>>>>>>>> no?
>>>>>>>
>>>>>>> It depends on what is running before U-Boot, so not necessarily.
>>>>>>>
>>>>>>> And speaking of boot device, consider the case where the system runs
>>>>>>> from eMMC and uses the HS200/HS400 modes, which need to switch bus
>>>>>>> mode
>>>>>>> using the pinmux driver.
>>>>>>>
>>>>>>> Is there a real-world use case where you would want to disable the
>>>>>>> pinmux driver ? And what is the benefit of that, except that it would
>>>>>>> cause all kinds of weird problems.
>>>>>>
>>>>>> My H3ULCB-KF boots just fine [1] with CONFIG_PINCTRL_PFC=n, but I
>>>>>> personally don't have any use-case which I need to fulfill on a
>>>>>> Renesas reference design by disabling PFC.
>>>>>
>>>>> And the eMMC and SDHI both work fine too in HS400/SDR104 modes ?
>>>>> They cannot, since you cannot switch the pinmux properties of the bus.
>>>>> What about the errors in the log below, they don't look quite fine.
>>>>>
>>>>>> Rather, the motivation here is to ensure U-Boot builds fine with as
>>>>>> many randconfig results as possible, which is a standard practice in
>>>>>> Linux. I personally favor my solution, but I am also open minded if
>>>>>> the linker error is avoided by introducing a direct/reverse dependency
>>>>>> between PFC and another relevant R-Car3 Kconfig symbol.
>>>>>
>>>>> I am fine with fixing randconfig build errors. My question here is
>>>>> whether it makes sense to allow U-Boot build without PFC support,
>>>>> since that would cause all kinds of problems. I am banking toward
>>>>> playing it safe and not allowing such an option at all. Thoughts ?
>>>>
>>>> It looks like in Linux, PINCTRL is a fundamental feature selected
>>>> (i.e. *cannot* be disabled by users) by ARCH_RENESAS since v4.5 commit
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=26a7e06dfee9
>>>>
>>>> ("arm64: renesas: r8a7795: Add Renesas R8A7795 SoC support").
>>>>
>>>> So, demanding a PFC-free U-Boot doesn't look reasonable to me.
>>>
>>> That's sensible.
>>>
>>>> Should PINCTRL be selected by CONFIG_RCAR_GEN3 as it is done in Linux?
>>>> One caveat is that PINCTRL currently depends on DM, so R-Car3 U-Boot
>>>> would become dependent on DM too, i.e. users won't have the option of
>>>> a legacy U-Boot anymore.
>>>
>>> Non-DM operation is not supported anyway, the direction is toward DM/DT
>>> support. Ultimately, it should be possible to have a single U-Boot
>>> binary and just exchange the DT to support different boards.
>>>
>>> My concern is with the size of the PFC tables, they are massive, sparse
>>> and keep growing, but that's a different topic.
>>>
>>> That said, what about making the GPIO driver depend on PFC driver and
>>> then have Gen3 select PFC by default in Kconfig ?
>>
>>
>> Of course, you can add such a dependency in Kconfig. But that's not the
>> question here and won't fix the issue:
> 
> What is the question then ?
> 
>> It won't fix the issue that we have code encapsulated with a CONFIG_*
>> option and a caller which is not encapsulated with this.
>>
>> To fix this with your proposal, you need to merge CONFIG_PINCTRL_PFC and
>> CONFIG_RCAR_GPIO to *one* CONFIG_RCAR_PINCTRL_PFC_GPIO (or whatever) to
>> ensure that both, the function definition *and* the caller are
>> encapsulated by the *same* CONFIG switch. But this sounds somehow quite
>> strange to me ...
> 
> I don't think I understand this part. If the GPIO driver depends on the
> PFC driver in Kconfig, then you can either have
> - both compiled in
> - neither PFC nor GPIO driver
> - only the PFC driver
> and all three options provide working result. Did I miss something ?
> 
> We can add this patch too, but I'd like to see the Kconfig fix alongside
> it. Note that the patch should use #if CONFIG_IS_ENABLED(PINCTRL_PFC) .

I was thinking about this patch further and I think the best way forward
would be to extend the pinmux/pinctrl API with a callback to set a pin
as GPIO and then just call that API from the GPIO driver. That would be
the generic solution and would make this whole
sh_pfc_config_mux_for_gpio() go away altogether.

-- 
Best regards,
Marek Vasut