[PATCH v2] gpio: octeon_gpio: Add GPIO controller driver for Octeon
Daniel Schwierzeck
daniel.schwierzeck at gmail.com
Wed Jul 22 13:47:50 CEST 2020
> 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.
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
More information about the U-Boot
mailing list