mbox series

[v5,0/5] ARM: Add GPIO support

Message ID 20230705194544.100370-1-nick.hawkins@hpe.com
Headers show
Series ARM: Add GPIO support | expand

Message

Hawkins, Nick July 5, 2023, 7:45 p.m. UTC
From: Nick Hawkins <nick.hawkins@hpe.com>

The GXP SoC supports GPIO on multiple interfaces. The interfaces are
CPLD and Host. The GPIOs are a combination of both physical and virtual
I/O across the interfaces. The gpio-gxp-pl driver covers the CPLD which
takes physical I/O from the board and shares it with GXP via a proprietary
interface that maps the I/O onto a specific register area of the GXP.
The CPLD interface supports interrupts.

Notes:

Based on previous feedback the gpio-gxp.c driver has been discarded in
favor of it going into a separate larger patchset. This leaves behind
only the gpio-gxp-pl.c driver.

After exploring the recommendation of using regmap_gpio it does not seem
like a good fit. Some of the GPIOs are a combination of several bits in
a byte where others are not contiguous blocks of GPIOs.

The gxp-fan-ctrl driver in HWMON no longer will report fan presence
or fan failure states as these GPIOs providing this information will be
consumed by the host. It will be the hosts function to keep track of
fan presence and status. There was an excellent suggestion to have linux
handle the entire thermal management of the system however the HPE
OpenBMC developers prefer to handle this inside OpenBMC stack instead.

---

Changes since v4:
 *Removed gpio-gxp.c patch as it requires a much larger patchset based
  on feedback.
 *Modified MAINTAINERS Removing gpio-gxp.c and adding missing
  gpio-gxp-pl.c
 *Modified hpe,gxp-gpio.yaml by removing hpe,gxp-gpio compatible
  reference for now in favor of adding it later with separate patchset.
 *Modified cover letter to explain that although there is a suggestion
  to have the kernel handle all thermal matters the HPE OpenBMC developers
  prefer to handle it there instead.
 *Modified cover letter to explain gpio-gxp.c removal

Changes since v3:
 *Added called with debugfs to read server id
 *Added reviewed-by: tags to hwmon fan driver and fan yaml
 *Changed maxItems to be 4 instead of 6 on reg and reg-names in gpio
  yaml
 *Moved gpio-gxp-pl.c to be in a separate patch/commit.
 *Moved regmap_config out of function in both gpio drivers to turn into
  static const
 *Removed unnecesary variables and redundant conditionals
 *Modified regmap_read switch statements to calculate offset and mask
  then read at end
 *Removed use of -EOPNOTSUPP in favor of -ENOTSUPP
 *Removed redundant casting
 *Switched generic_handle_irq for generic_handle_domain_irq
 *Used GENMASK where applicable
 *Used bitmap_xor and for_each_bit_set in PL PSU interrupt
 *Made GPIO chip const and marked as a template (in the name)
 *Made irq_chip const and immutable
 *Corrected check on devm_gpiochip_add_data
 *Removed dev_err_probe on platform_get_irq
 *Changed return 0 to devm_request_irq

Changes since v2:
 *Removed shared fan variables between HWMON and GPIO based on feedback
 *Removed reporting fan presence and failure from hwmon gxp-fan-ctrl
  driver
 *Removed GPIO dependency from gxp-fan-ctrl driver
 *Changed description and title for hpe,gxp-gpio binding
 *Corrected indention on example for hpe,gxp-gpio binding
 *Removed additional example from hpe,gxp-gpio binding

Changes since v1:
 *Removed ARM device tree changes and defconfig changes to reduce
  patchset size
 *Removed GXP PSU changes to reduce patchset size
 *Corrected hpe,gxp-gpio YAML file based on feedback
 *Created new gpio-gxp-pl file to reduce complexity
 *Separated code into two files to keep size down: gpio-gxp.c and
  gpio-gxp-pl.c
 *Fixed Kconfig indentation as well as add new entry for gpio-gxp-pl
 *Removed use of linux/of.h and linux/of_device.h
 *Added mod_devicetable.h and property.h
 *Fixed indentation of defines and uses consistent number of digits
 *Corrected defines with improper GPIO_ namespace.
 *For masks now use BIT()
 *Added comment for PLREG offsets
 *Move gpio_chip to be first in structure
 *Calculate offset for high and low byte GPIO reads instead of having
  H(High) and L(Low) letters added to the variables.
 *Removed repeditive use of "? 1 : 0"
 *Switched to handle_bad_irq()
 *Removed improper bailout on gpiochip_add_data
 *Used GENMASK to arm interrupts
 *Removed use of of_match_device
 *fixed sizeof in devm_kzalloc
 *Added COMPILE_TEST to Kconfig
 *Added dev_err_probe where applicable
 *Removed unecessary parent and compatible checks

Nick Hawkins (5):
  dt-bindings: gpio: Add HPE GXP GPIO
  gpio: gxp: Add HPE GXP GPIO PL
  dt-bindings: hwmon: hpe,gxp-fan-ctrl: remove fn2 and pl registers
  hwmon: (gxp_fan_ctrl) Provide fan info via gpio
  MAINTAINERS: hpe: Add GPIO

 .../bindings/gpio/hpe,gxp-gpio.yaml           |  71 +++
 .../bindings/hwmon/hpe,gxp-fan-ctrl.yaml      |  16 +-
 MAINTAINERS                                   |   2 +
 drivers/gpio/Kconfig                          |   9 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-gxp-pl.c                    | 582 ++++++++++++++++++
 drivers/hwmon/Kconfig                         |   2 +-
 drivers/hwmon/gxp-fan-ctrl.c                  | 106 +---
 8 files changed, 671 insertions(+), 118 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml
 create mode 100644 drivers/gpio/gpio-gxp-pl.c

Comments

Krzysztof Kozlowski July 6, 2023, 6:22 a.m. UTC | #1
On 05/07/2023 21:45, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> Provide access to the register regions and interrupt for GPIO. The
> driver under the hpe,gxp-gpio-pl will provide GPIO information from the
> CPLD interface. The CPLD interface represents all physical GPIOs. The
> GPIO interface with the CPLD allows use of interrupts.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> 
> ---
> 
> v5:
>  *Removed use of gpio-gxp in favor of just supporting
>   hpe,gxp-gpio-pl for now as the full gpio-gxp will
>   require a much larger patchset

Bindings describe hardware, not drivers, and should be rather complete.


>  *Modified commit description to reflect removal of
>   hpe,gxp-gpio
> v4:
>  *Fix min and max values for regs
> v3:
>  *Remove extra example in examples
>  *Actually fixed indentation on example - Aligned
>   GPIO line names with " above.
> v2:
>  *Put binding patch before the driver in the series
>  *Improved patch description
>  *Removed oneOf and items in compatible definition
>  *Moved additionalProperties definition to correct spot in file
>  *Fixed indentation on example
>  *Improved description in .yaml
> ---
>  .../bindings/gpio/hpe,gxp-gpio.yaml           | 71 +++++++++++++++++++
>  1 file changed, 71 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml b/Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml
> new file mode 100644
> index 000000000000..799643c1a0c2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml
> @@ -0,0 +1,71 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/hpe,gxp-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HPE GXP gpio controllers

GPIO
> +
> +maintainers:
> +  - Nick Hawkins <nick.hawkins@hpe.com>
> +
> +description:
> +  Interruptable GPIO drivers for the HPE GXP that covers multiple interfaces

"drivers" as Linux drivers? If so, then drop and rephrase to describe
hardware.

> +  of both physical and virtual GPIO pins.
> +
> +properties:
> +  compatible:
> +    const: hpe,gxp-gpio-pl> +
> +  reg:
> +    items:
> +      - description: pl base gpio
> +      - description: pl interrupt gpio
> +
> +  reg-names:
> +    items:
> +      - const: base
> +      - const: interrupt
> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  gpio-line-names:
> +    maxItems: 80
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - gpio-controller
> +  - "#gpio-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +        gpio@51000300 {

Wrong indentation. Use 2 or 4 (preferred) spaces, not 8.

> +          compatible = "hpe,gxp-gpio-pl";
> +          reg = <0x51000300 0x7f>, <0x51000380 0x20>;
> +          reg-names = "base", "interrupt";
> +          gpio-controller;
> +          #gpio-cells = <2>;
> +          interrupt-parent = <&vic0>;
> +          interrupts = <24>;
> +          gpio-line-names =
> +          "IOP_LED1", "IOP_LED2", "IOP_LED3", "IOP_LED4", "IOP_LED5", "IOP_LED6", "IOP_LED7", "IOP_LED8",

And this is even worse.

> +          "FAN1_INST", "FAN2_INST", "FAN3_INST", "FAN4_INST", "FAN5_INST", "FAN6_INST", "FAN7_INST",
> +          "FAN8_INST", "FAN1_FAIL", "FAN2_FAIL", "FAN3_FAIL", "FAN4_FAIL", "FAN5_FAIL", "FAN6_FAIL",
> +          "FAN7_FAIL", "FAN8_FAIL", "FAN1_ID", "FAN2_ID", "FAN3_ID", "FAN4_ID", "FAN5_ID", "FAN6_ID",
> +          "FAN7_ID", "FAN8_ID", "IDENTIFY", "HEALTH_RED", "HEALTH_AMBER", "POWER_BUTTON", "UID_PRESS",
> +          "SLP", "NMI_BUTTON", "RESET_BUTTON", "SIO_S5", "SO_ON_CONTROL", "PSU1_INST", "PSU2_INST",
> +          "PSU3_INST", "PSU4_INST", "PSU5_INST", "PSU6_INST", "PSU7_INST", "PSU8_INST", "PSU1_AC",
> +          "PSU2_AC", "PSU3_AC", "PSU4_AC", "PSU5_AC", "PSU6_AC", "PSU7_AC", "PSU8_AC", "PSU1_DC",
> +          "PSU2_DC", "PSU3_DC", "PSU4_DC", "PSU5_DC", "PSU6_DC", "PSU7_DC", "PSU8_DC", "", "", "", "",
> +          "", "", "", "", "", "", "", "", "", "";
> +        };

Best regards,
Krzysztof
Hawkins, Nick July 6, 2023, 2:12 p.m. UTC | #2
Greetings Krzysztof,

Thank you for the feedback. I see that due to a patch conflict I
reintroduced some of the alignment issues you had me fix in
a previous version. This was a mistake and I will correct this.

> > v5:
> > *Removed use of gpio-gxp in favor of just supporting
> > hpe,gxp-gpio-pl for now as the full gpio-gxp will
> > require a much larger patchset

> Bindings describe hardware, not drivers, and should be rather complete.

This patch is intended to still cover the hardware interface between our
BMC and our CPLD which gathers GPIO for us. The part of the binding I
removed was a completely separate interface with different mechanisms
for reading GPIOs. With that said I could keep these two interfaces
separate in yaml files: Having a yaml for hpe,gxp-gpio and another for
hpe,gxp-gpio-pl. Would this be a better approach?

Thank you,

-Nick Hawkins
Rob Herring (Arm) July 6, 2023, 7:05 p.m. UTC | #3
On Thu, Jul 06, 2023 at 02:12:12PM +0000, Hawkins, Nick wrote:
> Greetings Krzysztof,
> 
> Thank you for the feedback. I see that due to a patch conflict I
> reintroduced some of the alignment issues you had me fix in
> a previous version. This was a mistake and I will correct this.
> 
> > > v5:
> > > *Removed use of gpio-gxp in favor of just supporting
> > > hpe,gxp-gpio-pl for now as the full gpio-gxp will
> > > require a much larger patchset
> 
> > Bindings describe hardware, not drivers, and should be rather complete.
> 
> This patch is intended to still cover the hardware interface between our
> BMC and our CPLD which gathers GPIO for us. The part of the binding I
> removed was a completely separate interface with different mechanisms
> for reading GPIOs. With that said I could keep these two interfaces
> separate in yaml files: Having a yaml for hpe,gxp-gpio and another for
> hpe,gxp-gpio-pl. Would this be a better approach?

If they are independent (and it sounds like they are), then yes.

Rob
Linus Walleij July 16, 2023, 9:12 p.m. UTC | #4
On Wed, Jul 5, 2023 at 9:49 PM <nick.hawkins@hpe.com> wrote:

> From: Nick Hawkins <nick.hawkins@hpe.com>
>
> The GXP SoC supports GPIO on multiple interfaces. The interfaces are
> CPLD and Host. The GPIOs are a combination of both physical and virtual
> I/O across the interfaces. The gpio-gxp-pl driver covers the CPLD which
> takes physical I/O from the board and shares it with GXP via a proprietary
> interface that maps the I/O onto a specific register area of the GXP.
> The CPLD interface supports interrupts.
>
> Notes:
>
> Based on previous feedback the gpio-gxp.c driver has been discarded in
> favor of it going into a separate larger patchset. This leaves behind
> only the gpio-gxp-pl.c driver.

The kernel certainly looks better after this change than before, so
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij