[PATCH v2] gpio: octeon_gpio: Add GPIO controller driver for Octeon

[Stefan Roese]

On 22.07.20 13:47, Daniel Schwierzeck wrote:
>> Hi Daniel,
>>
>> On 21.07.20 16:59, Daniel Schwierzeck wrote:
>>>> Hi Daniel,
>>>>
>>>> On 18.07.20 15:25, Daniel Schwierzeck wrote:
>>>>>> From: Suneel Garapati <sgarapati at marvell.com>
>>> ...
>>>>> found another issue:
>>>>>
>>>>> drivers/gpio/octeon_gpio.c: In function 'octeon_gpio_probe':
>>>>> drivers/gpio/octeon_gpio.c:189:16: warning: implicit declaration of
>>>>> function 'dm_pci_map_bar'; did you mean 'pci_map_bar'? [-Wimplicit-
>>>>> function-declaration]
>>>>>      189 |   priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
>>>>>          |                ^~~~~~~~~~~~~~
>>>>>          |                pci_map_bar
>>>>> drivers/gpio/octeon_gpio.c:189:14: warning: assignment to 'void *' from
>>>>> 'int' makes pointer from integer without a cast [-Wint-conversion]
>>>>>      189 |   priv->base = dm_pci_map_bar(dev, PCI_BASE_ADDRESS_0,
>>>>>          |              ^
>>>>
>>>> Ah, I did not see this in my local builds, as I have enabled DM_PCI
>>>> here and forgot to add this dependency.
>>>>
>>>>> due to the dependency on DM_PCI you need to express this in Kconfig
>>>>> with "depends on DM_PCI ...". Alternatively you need to wrap the PCI
>>>>> specific code with a "#ifdef CONFIG_DM_PCI" as the DM_PCI specific
>>>>> function prototypes in pci.h are also wrapped with "#ifdef
>>>>> CONFIG_DM_PCI". The former option will build and link DM PCI code which
>>>>> is not used and therefore bloats the U-Boot binary.
>>>>>
>>>>> I guess the choice mainly depends on whether you'll add a PCI host
>>>>> controller driver for Octeon MIPS64 in the future and can live with the
>>>>> extra but unused PCI code until then.
>>>>
>>>> We can definitely live with the temporarily unused PCI code. I don't
>>>> want to add these #ifdefs again, which I removed explicitly upon Simon's
>>>> request.
>>>>
>>>> To solve this, I would prefer to add a "select DM_PCI & PCI" to
>>>> arch/mips/Kconfig for Octeon, as this PCI probing construct is not
>>>> only used in this GPIO driver, but also in many other drivers shared
>>>> between MIPS Octeon and the also upcoming ARM64 Octeon TX/TX2 support,
>>>> like I2C, SPI etc. Here all devices are PCI based hence the PCI probing
>>>> dependency.
>>>>
>>>> Is this okay with you? Or should I stay with the dependency in the
>>>> drivers Kconfig file?
>>>>
>>>
>>> I would also prefer a "select DM_PCI". I just struggle a bit with the
>>> "& PCI" part. IMHO DM_PCI should not depend on PCI because DM_PCI
>>> should be a hidden option which is selected by the SoC dependent on the
>>> SoC specific drivers and whether them support DM. It doesn't make sense
>>> to let the user choose all those DM_* options. Most other DM_* options
>>> already work like this except that them are not hidden.
>>>
>>> BTW: When you prepare a patch, you can also "select DM_I2C" and
>>> "DM_SPI" which have the same issues with the PCI dependency.
>>
>> Sure. But selecting only "DM_PCI" and not "PCI" leads to these issues:
>>
>> WARNING: unmet direct dependencies detected for DM_PCI
>>     Depends on [n]: PCI [=n] && DM [=y]
>>     Selected by [y]:
>>     - ARCH_OCTEON [=y] && <choice>
>>
>> It doesn't make sense to have DM_PCI enabled and PCI not. Same happens
>> with DM_SPI BTW.
> 
> exactly that is my problem. The dependency doesn't make sense and
> should be removed IMHO. Options like DM_PCI should be interpreted as
> "this platform with its drivers support PCI witrh DM". It doesn't nake
> sense to let an user choose this option. I just wanted to point out
> this inconsistency. Maybe Simon has an opinion about that.
> 
>>
>> How would you like me to solve this? Enable PCI (and SPI etc) in the
>> board defconfig file? This will lead to problems with git-bisecting
>> though, as the Kconfig change depends on the defconfig change and
>> vice versa. I'll squash the Kconfig and defconfig updates in one
>> patch because of this.
> 
> with the current state it's either "select DM_PCI & PCI" as you
> suggested or adding CONFIG_DM_PCI=y and CONFIG_PCI=y to your defconfig.
> The choice mainly depends on whether a subsystem driver like GPIO using
> DM_PCI is optional for the user or is always required.

Since most subsystem drivers like GPIO etc are optional, I would like to
move to enabling PCI & DM_PCI via defconfig. I'll remove the selection
of DM_PCI from arch/mips/Kconfig then to make it more consistant.

> Anyway all Octeon drivers using DM_PCI API needs a "depends on DM_PCI"
> so that you can't enable those drivers without enabling DM_PCI.
> Otherwise an user will get build errors.

I've updated the Kconfig file(s) with this dependency in the next
patchset version.

Thanks,
Stefan