From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Tue, 28 Jul 2020 12:58:35 -0600 Subject: [PATCH v2] gpio: octeon_gpio: Add GPIO controller driver for Octeon In-Reply-To: <88ab0fb216fc00177f03331cdf0b6a3ef61a95b5.camel@gmail.com> References: <20200526121932.4121-1-sr@denx.de> <187a977eaafb9384b53e21b6d2ea6d3f152c769b.camel@gmail.com> <206e2fc1-3284-6520-c9ba-a52191501a78@denx.de> <88ab0fb216fc00177f03331cdf0b6a3ef61a95b5.camel@gmail.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Daniel, On Wed, 22 Jul 2020 at 05: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 > > > ... > > > > > 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] && > > > > 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. Yes that's right. We have it as an option since supporting DM_PCI can be board-by-board at present. Once migration is finished, we can drop DM_PCI. [..] Regards, Simon