> 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. 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'll combine all these changes including the GPIO driver update, all > the dts updates and a newly introduced simple clk driver in a new > patchset shortly. > > Thanks, > Stefan -- - Daniel
