[PATCH v2] gpio: octeon_gpio: Add GPIO controller driver for Octeon

[Daniel Schwierzeck]

> 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