RE: [PATCH v1 1/1] gpio: add driver for Mellanox BlueField GPIO controller
Thank you for the feedback. Regarding the suggested use of regmap mmio, all of
the registers being accessed here are 64-bit and the
regmap_update_bits/regmap_read/regmap_write calls aren't very convenient to use
in this case. All other comments have been addressed in v2.
Regards,
Shravan Kumar Ramani
-Original Message-
From: Bartosz Golaszewski
Sent: Friday, February 15, 2019 3:48 AM
To: Shravan Ramani
Cc: Linus Walleij ; linux-gpio
; LKML
Subject: Re: [PATCH v1 1/1] gpio: add driver for Mellanox BlueField GPIO
controller
czw., 14 lut 2019 o 22:30 Shravan Kumar Ramani
napisał(a):
>
> This patch adds support for the GPIO controller used by Mellanox
> BlueField SOCs.
>
> Reviewed-by: David Woods
> Signed-off-by: Shravan Kumar Ramani
> ---
> drivers/gpio/Kconfig | 7 ++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-mlxbf.c | 287
> ++
> 3 files changed, 295 insertions(+)
> create mode 100644 drivers/gpio/gpio-mlxbf.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index
> b5a2845..2bebe92 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1292,6 +1292,13 @@ config GPIO_MERRIFIELD
> help
> Say Y here to support Intel Merrifield GPIO.
>
> +config GPIO_MLXBF
> + tristate "Mellanox BlueField SoC GPIO"
> + depends on (MELLANOX_PLATFORM && ARM64 && ACPI) || COMPILE_TEST
> + select GPIO_GENERIC
You're not really depending on it.
> + help
> + Say Y here if you want GPIO support on Mellanox BlueField SoC.
> +
> config GPIO_ML_IOH
> tristate "OKI SEMICONDUCTOR ML7213 IOH GPIO support"
> depends on X86 || COMPILE_TEST diff --git
> a/drivers/gpio/Makefile b/drivers/gpio/Makefile index 37628f8..8d54279
> 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -83,6 +83,7 @@ obj-$(CONFIG_GPIO_MENZ127)+= gpio-menz127.o
> obj-$(CONFIG_GPIO_MERRIFIELD) += gpio-merrifield.o
> obj-$(CONFIG_GPIO_MC33880) += gpio-mc33880.o
> obj-$(CONFIG_GPIO_MC9S08DZ60) += gpio-mc9s08dz60.o
> +obj-$(CONFIG_GPIO_MLXBF) += gpio-mlxbf.o
> obj-$(CONFIG_GPIO_ML_IOH) += gpio-ml-ioh.o
> obj-$(CONFIG_GPIO_MM_LANTIQ) += gpio-mm-lantiq.o
> obj-$(CONFIG_GPIO_MOCKUP) += gpio-mockup.o
> diff --git a/drivers/gpio/gpio-mlxbf.c b/drivers/gpio/gpio-mlxbf.c new
> file mode 100644 index 000..e5c50db
> --- /dev/null
> +++ b/drivers/gpio/gpio-mlxbf.c
> @@ -0,0 +1,287 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/* Number of pins on BlueField */
> +#define MLXBF_GPIO_NR 54
> +
> +/* Pad Electrical Controls. */
> +#define GPIO_PAD_CONTROL__FIRST_WORD 0x0700 #define
> +GPIO_PAD_CONTROL_1__FIRST_WORD 0x0708 #define
> +GPIO_PAD_CONTROL_2__FIRST_WORD 0x0710 #define
> +GPIO_PAD_CONTROL_3__FIRST_WORD 0x0718
> +
> +#define GPIO_PIN_DIR_I 0x1040
> +#define GPIO_PIN_DIR_O 0x1048
> +#define GPIO_PIN_STATE 0x1000
> +#define GPIO_SCRATCHPAD 0x20
> +
> +#ifdef CONFIG_PM
> +struct bluefield_context_save_regs {
> + u64 gpio_scratchpad;
> + u64 gpio_pad_control[MLXBF_GPIO_NR];
> + u64 gpio_pin_dir_i;
> + u64 gpio_pin_dir_o;
> +};
> +#endif
> +
> +/* Device state structure. */
> +struct gpio_state {
> + struct list_head node;
Why do you need that? You're not using it anywhere AFAICT.
> + struct platform_device *pdev;
You're not using this either.
> +
> + struct gpio_chip gc;
> +
> + /* Must hold this lock to modify shared data. */
> + spinlock_t lock;
> +
> + /* Memory Address */
> + void __iomem *dc_base;
> +
> +#ifdef CONFIG_PM
> + struct bluefield_context_save_regs csave_regs; #endif };
> +
> +/* GPIO driver set input pins */
> +static int gpio_bf_set_input(struct gpio_chip *chip, unsigned int
> +offset) {
> + struct gpio_state *gs = gpiochip_get_data(chip);
> + u64 in;
> + u64 out;
> +
> + if (offset > MLXBF_GPIO_NR - 1) {
> + dev_err(chip->parent, "gpio input pins: Invalid offset %d\n",
> + offset);
> + return -EINVAL;
> + }
You don't need that, the subsystem makes sure this doesn't happen.
Same elsewhere.
> +
> + out = readq(gs->dc_base + GPIO_PIN_DIR_O);
> + in = readq(gs->dc_base + GP
Re: [PATCH v1 1/1] gpio: add driver for Mellanox BlueField GPIO controller
czw., 14 lut 2019 o 22:30 Shravan Kumar Ramani
napisał(a):
>
> This patch adds support for the GPIO controller used by Mellanox
> BlueField SOCs.
>
> Reviewed-by: David Woods
> Signed-off-by: Shravan Kumar Ramani
> ---
> drivers/gpio/Kconfig | 7 ++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-mlxbf.c | 287
> ++
> 3 files changed, 295 insertions(+)
> create mode 100644 drivers/gpio/gpio-mlxbf.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index b5a2845..2bebe92 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1292,6 +1292,13 @@ config GPIO_MERRIFIELD
> help
> Say Y here to support Intel Merrifield GPIO.
>
> +config GPIO_MLXBF
> + tristate "Mellanox BlueField SoC GPIO"
> + depends on (MELLANOX_PLATFORM && ARM64 && ACPI) || COMPILE_TEST
> + select GPIO_GENERIC
You're not really depending on it.
> + help
> + Say Y here if you want GPIO support on Mellanox BlueField SoC.
> +
> config GPIO_ML_IOH
> tristate "OKI SEMICONDUCTOR ML7213 IOH GPIO support"
> depends on X86 || COMPILE_TEST
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 37628f8..8d54279 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -83,6 +83,7 @@ obj-$(CONFIG_GPIO_MENZ127)+= gpio-menz127.o
> obj-$(CONFIG_GPIO_MERRIFIELD) += gpio-merrifield.o
> obj-$(CONFIG_GPIO_MC33880) += gpio-mc33880.o
> obj-$(CONFIG_GPIO_MC9S08DZ60) += gpio-mc9s08dz60.o
> +obj-$(CONFIG_GPIO_MLXBF) += gpio-mlxbf.o
> obj-$(CONFIG_GPIO_ML_IOH) += gpio-ml-ioh.o
> obj-$(CONFIG_GPIO_MM_LANTIQ) += gpio-mm-lantiq.o
> obj-$(CONFIG_GPIO_MOCKUP) += gpio-mockup.o
> diff --git a/drivers/gpio/gpio-mlxbf.c b/drivers/gpio/gpio-mlxbf.c
> new file mode 100644
> index 000..e5c50db
> --- /dev/null
> +++ b/drivers/gpio/gpio-mlxbf.c
> @@ -0,0 +1,287 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/* Number of pins on BlueField */
> +#define MLXBF_GPIO_NR 54
> +
> +/* Pad Electrical Controls. */
> +#define GPIO_PAD_CONTROL__FIRST_WORD 0x0700
> +#define GPIO_PAD_CONTROL_1__FIRST_WORD 0x0708
> +#define GPIO_PAD_CONTROL_2__FIRST_WORD 0x0710
> +#define GPIO_PAD_CONTROL_3__FIRST_WORD 0x0718
> +
> +#define GPIO_PIN_DIR_I 0x1040
> +#define GPIO_PIN_DIR_O 0x1048
> +#define GPIO_PIN_STATE 0x1000
> +#define GPIO_SCRATCHPAD 0x20
> +
> +#ifdef CONFIG_PM
> +struct bluefield_context_save_regs {
> + u64 gpio_scratchpad;
> + u64 gpio_pad_control[MLXBF_GPIO_NR];
> + u64 gpio_pin_dir_i;
> + u64 gpio_pin_dir_o;
> +};
> +#endif
> +
> +/* Device state structure. */
> +struct gpio_state {
> + struct list_head node;
Why do you need that? You're not using it anywhere AFAICT.
> + struct platform_device *pdev;
You're not using this either.
> +
> + struct gpio_chip gc;
> +
> + /* Must hold this lock to modify shared data. */
> + spinlock_t lock;
> +
> + /* Memory Address */
> + void __iomem *dc_base;
> +
> +#ifdef CONFIG_PM
> + struct bluefield_context_save_regs csave_regs;
> +#endif
> +};
> +
> +/* GPIO driver set input pins */
> +static int gpio_bf_set_input(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct gpio_state *gs = gpiochip_get_data(chip);
> + u64 in;
> + u64 out;
> +
> + if (offset > MLXBF_GPIO_NR - 1) {
> + dev_err(chip->parent, "gpio input pins: Invalid offset %d\n",
> + offset);
> + return -EINVAL;
> + }
You don't need that, the subsystem makes sure this doesn't happen.
Same elsewhere.
> +
> + out = readq(gs->dc_base + GPIO_PIN_DIR_O);
> + in = readq(gs->dc_base + GPIO_PIN_DIR_I);
> +
> + writeq(out & ~BIT(offset), gs->dc_base + GPIO_PIN_DIR_O);
> + writeq(in | BIT(offset), gs->dc_base + GPIO_PIN_DIR_I);
> +
If you used the mmio regmap in this driver you could avoid both the
locking (regmap uses a spinlock if fast_io is set to true) and manual
bit updating here. You would do something like:
regmap_update_bits(map, gs->dc_base + GPIO_PIN_DIR_O, out &
BIT(offset), ~BIT(offset)).
> + return 0;
> +}
> +
> +/* GPIO driver set output pins */
> +static int gpio_bf_set_output(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct gpio_state *gs = gpiochip_get_data(chip);
> + u64 in;
> + u64 out;
> +
> + if (offset > MLXBF_GPIO_NR - 1) {
> + dev_err(chip->parent, "gpio output pins: Invalid offset %d\n",
> + offset);
> + return -EINVAL;
> + }
> +
> + out = readq(gs->dc_base + GPIO_PIN_DIR_O);
> + in = readq(gs->dc_base + GPIO_PIN_DIR_I);
> +
> + writeq(out |
