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

Tue Jul 21 13:03:16 CEST 2020

Hi Daniel,

On 18.07.20 15:25, Daniel Schwierzeck wrote:
> 
>> From: Suneel Garapati <sgarapati at marvell.com>
>>
>> Add support for GPIO controllers found on Octeon II/III and Octeon TX
>> TX2 SoC platforms.
>>
>> Signed-off-by: Aaron Williams <awilliams at marvell.com>
>> Signed-off-by: Suneel Garapati <sgarapati at marvell.com>
>> Signed-off-by: Stefan Roese <sr at denx.de>
>> Cc: Simon Glass <sjg at chromium.org>
>> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
>> Cc: Aaron Williams <awilliams at marvell.com>
>> Cc: Chandrakala Chavva <cchavva at marvell.com>
>> ---
>> v2 (Stefan):
>> - Removed #ifdef's for Octeon vs OcteonTX/TX2 completely
>>    The differentiation is now made via driver data / compatible
>>    string
>>
>> RFC -> v1 (Stefan)
>> - Separated this patch from the OcteonTX/TX2 RFC patch series into a
>>    single patch. This is useful, as the upcoming MIPS Octeon support will
>>    use this GPIO driver.
>> - Added MIPS Octeon II/III support (big endian). Rename driver and its
>>    function names from "octeontx" to "octeon" to better match all Octeon
>>    platforms.
>> - Moved from union to defines / bitmasks. This makes the driver usage
>>    on little- and big-endian platforms much easier.
>> - Used clrbits_64() instead of clrbits_le64() and friends to support
>>    usage on little- and big-endian systems
>> - Removed dev->req_seq assignment
>> - Enhanced Kconfig text
>> - Rewrote GPIO_BIT macro
>> - Dropped many macros to calculate the registers offsets and implemented
>>    simple functions for this (easier to read)
>> - Used GENMASK_ULL and FIELD_GET helpers
>> - Minor cosmetic changes (dropped brackets etc)
>> - Reword commit text and subject
>>
>>   drivers/gpio/Kconfig       |  10 ++
>>   drivers/gpio/Makefile      |   1 +
>>   drivers/gpio/octeon_gpio.c | 253 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 264 insertions(+)
>>   create mode 100644 drivers/gpio/octeon_gpio.c
>>
>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>> index d87f6cc105..451896f400 100644
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -316,6 +316,16 @@ config PIC32_GPIO
>>   	help
>>   	  Say yes here to support Microchip PIC32 GPIOs.
>>   
>> +config OCTEON_GPIO
>> +	bool "Octeon II/III/TX/TX2 GPIO driver"
>> +	depends on DM_GPIO && (ARCH_OCTEON || ARCH_OCTEONTX || ARCH_OCTEONTX2)
>> +	default y
>> +	help
>> +	  Add support for the Marvell Octeon GPIO driver. This is used with
>> +	  various Octeon parts such as Octeon II/III and OcteonTX/TX2.
>> +	  Octeon II/III has 32 GPIOs (count defined via DT) and OcteonTX/TX2
>> +	  has 64 GPIOs (count defined via internal register).
>> +
> 
> 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?

>>   config STM32_GPIO
>>   	bool "ST STM32 GPIO driver"
>>   	depends on DM_GPIO && (ARCH_STM32 || ARCH_STM32MP)
>> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
>> index 7638259007..eb6364adb4 100644
>> --- a/drivers/gpio/Makefile
>> +++ b/drivers/gpio/Makefile
>> @@ -58,6 +58,7 @@ obj-$(CONFIG_HIKEY_GPIO)	+= hi6220_gpio.o
>>   obj-$(CONFIG_HSDK_CREG_GPIO)	+= hsdk-creg-gpio.o
>>   obj-$(CONFIG_IMX_RGPIO2P)	+= imx_rgpio2p.o
>>   obj-$(CONFIG_PIC32_GPIO)	+= pic32_gpio.o
>> +obj-$(CONFIG_OCTEON_GPIO)	+= octeon_gpio.o
>>   obj-$(CONFIG_MVEBU_GPIO)	+= mvebu_gpio.o
>>   obj-$(CONFIG_MSM_GPIO)		+= msm_gpio.o
>>   obj-$(CONFIG_$(SPL_)PCF8575_GPIO)	+= pcf8575_gpio.o
>> diff --git a/drivers/gpio/octeon_gpio.c b/drivers/gpio/octeon_gpio.c
>> new file mode 100644
>> index 0000000000..d7ac9a1910
>> --- /dev/null
>> +++ b/drivers/gpio/octeon_gpio.c
>> @@ -0,0 +1,253 @@
>> +// SPDX-License-Identifier:    GPL-2.0
>> +/*
>> + * Copyright (C) 2018 Marvell International Ltd.
>> + *
>> + * (C) Copyright 2011
>> + * eInfochips Ltd. <www.einfochips.com>
>> + * Written-by: Ajay Bhargav <ajay.bhargav at einfochips.com>
>> + *
>> + * (C) Copyright 2010
>> + * Marvell Semiconductor <www.marvell.com>
>> + */
>> +
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <errno.h>
>> +#include <fdtdec.h>
>> +#include <pci_ids.h>
>> +#include <asm/bitops.h>
>> +#include <asm/gpio.h>
>> +#include <asm/io.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/compat.h>
>> +#include <dt-bindings/gpio/gpio.h>
> 
> don't use common.h anymore. Also some headers are unused. I can build
> the driver with:
> 
> #include <dm.h>
> #include <pci.h>
> #include <pci_ids.h>
> #include <asm/gpio.h>
> #include <asm/io.h>
> #include <linux/bitfield.h>
> #include <linux/compat.h>
> #include <dt-bindings/gpio/gpio.h>

Yes, thanks. I'll change this in v3.

Thanks,
Stefan