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 <sgarap...@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
