From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Roese Date: Thu, 23 Jul 2020 12:07:56 +0200 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: <5b9192dd-eea3-94f9-d2d0-4b4dfd01a546@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>> ... >>>>> 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. > >> >> 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