| Message ID | 20230606014234.29491-1-nick.hawkins@hpe.com |
|---|---|
| Headers | show |
| Series | ARM: Add GPIO support | expand |
Mon, Jun 05, 2023 at 08:42:31PM -0500, nick.hawkins@hpe.com kirjoitti: > From: Nick Hawkins <nick.hawkins@hpe.com> > > The GXP SoC supports GPIO on multiple interfaces. The interfaces are > CPLD and Host. The GPIOs is a combination of both physical and virtual > I/O across the interfaces. The gpio-gxp driver specifically covers the > CSM(physical), FN2(virtual), and VUHC(virtual) which are the host. The > gpio-gxp-pl driver covers the CPLD which takes physical I/O from the > board and shares it with GXP via a propriety interface that maps the I/O > onto a specific register area of the GXP. The drivers both support > interrupts but from different interrupt parents. Thank you. This needs some work to be done. See my comments below. ... > +/* Remember last PSU config */ Would be nice to add why this is a global variable. > +u8 psu_presence; ... > +struct gxp_gpio_drvdata { > + struct regmap *base; > + struct regmap *interrupt; > + struct gpio_chip chip; Making this the first member might save a few bytes of code. > + int irq; > +}; > +static struct regmap *gxp_gpio_init_regmap(struct platform_device *pdev, > + char *reg_name, u8 bits) > +{ > + struct regmap_config regmap_config = { > + .reg_bits = 32, > + .reg_stride = 4, > + .val_bits = 32, > + }; Move it out and make static const. > + void __iomem *base; > + if (bits == 8) { > + regmap_config.reg_bits = 8; > + regmap_config.reg_stride = 1; > + regmap_config.val_bits = 8; > + regmap_config.max_register = 0xff; > + } Just make two regmap configs and choose them based on the bits. > + base = devm_platform_ioremap_resource_byname(pdev, reg_name); > + if (IS_ERR(base)) > + return ERR_CAST(base); > + > + regmap_config.name = reg_name; > + > + return devm_regmap_init_mmio(&pdev->dev, base, ®map_config); Why are you not using gpio-regmap? > +} ... > +static int gxp_gpio_pl_get(struct gpio_chip *chip, unsigned int offset) > +{ > + struct gxp_gpio_drvdata *drvdata = dev_get_drvdata(chip->parent); > + unsigned int val; > + int ret = 0; Unneeded. > + switch (offset) { > + case IOP_LED1 ... IOP_LED8: > + regmap_read(drvdata->base, PLREG_IOP_LED, &val); > + ret = (val & BIT(offset)) ? 1 : 0; > + break; > + case FAN1_INST ...FAN8_INST: > + regmap_read(drvdata->base, PLREG_FAN_INST, &val); > + ret = (val & BIT((offset - FAN1_INST))) ? 1 : 0; > + break; > + case FAN1_FAIL ... FAN8_FAIL: > + regmap_read(drvdata->base, PLREG_FAN_FAIL, &val); > + ret = (val & BIT((offset - FAN1_FAIL))) ? 1 : 0; > + break; > + case PWR_BTN_INT ... SLP_INT: > + regmap_read(drvdata->base, PLREG_INT_GRP5_FLAG, &val); > + /* Active low */ > + ret = (val & BIT((offset - PWR_BTN_INT) + 16)) ? 0 : 1; > + break; > + case PSU1_INST ... PSU8_INST: > + regmap_read(drvdata->base, PLREG_PSU_INST, &val); > + psu_presence = (u8)val; > + ret = (psu_presence & BIT((offset - PSU1_INST))) ? 1 : 0; > + break; > + case PSU1_AC ... PSU8_AC: > + regmap_read(drvdata->base, PLREG_PSU_AC, &val); > + ret = (val & BIT((offset - PSU1_AC))) ? 1 : 0; > + break; > + case PSU1_DC ... PSU8_DC: > + regmap_read(drvdata->base, PLREG_PSU_DC, &val); > + ret = (val & BIT((offset - PSU1_DC))) ? 1 : 0; > + break; > + default: > + break; > + } Obviously what needs to be done in a switch case is to assing offset adjustment and register name. Then here you read and return the value based on that. Same approach may be used in other switch-cases in the driver. > + return ret; > +} ... > +static int gxp_gpio_pl_get_direction(struct gpio_chip *chip, unsigned int offset) > +{ > + int ret = GXP_GPIO_DIR_IN; Useless variable. Return directly. > + switch (offset) { > + case IOP_LED1 ... IOP_LED8: > + case LED_IDENTIFY ... LED_HEALTH_AMBER: > + case ACM_FORCE_OFF: > + case ACM_REQ_N: > + ret = GXP_GPIO_DIR_OUT; > + break; > + default: > + break; > + } > + > + return ret; > +} ... > +static int gxp_gpio_pl_direction_input(struct gpio_chip *chip, > + unsigned int offset) > +{ > + int ret = -EOPNOTSUPP; Ditto. Also note, GPIOLIB uses ENOTSUPP. > + switch (offset) { > + case 8 ... 55: > + ret = GXP_GPIO_DIR_OUT; > + break; > + case 59 ... 65: > + ret = GXP_GPIO_DIR_OUT; > + break; > + default: > + break; > + } > + > + return ret; > +} Same comments as above may be applied to other function implementations in the driver. ... > +static irqreturn_t gxp_gpio_pl_irq_handle(int irq, void *_drvdata) > +{ > + struct gxp_gpio_drvdata *drvdata = (struct gxp_gpio_drvdata *)_drvdata; Redundant casting. > + unsigned int val, girq, i; > + > + /* Check group 5 interrupts */ > + > + regmap_read(drvdata->base, PLREG_INT_GRP5_FLAG, &val); No error check? So it will spit a spurious interrupts here and there? > + if (val) { Redundant conditional. > + for_each_set_bit(i, (unsigned long *)&val, 3) { This casting is red flag. It has 3 issues. So, no. Just make the copy in the unsigned long type. > + girq = irq_find_mapping(drvdata->chip.irq.domain, > + i + PWR_BTN_INT); > + generic_handle_irq(girq); generic_handle_domain_irq() > + } > + > + /* Clear latched interrupt */ > + regmap_update_bits(drvdata->interrupt, PLREG_INT_GRP5_FLAG, > + 0xFF, 0xFF); GENMASK() > + regmap_update_bits(drvdata->interrupt, PLREG_INT_GRP5_BASE, > + BIT(0) | BIT(2), 0); > + } > + > + /* Check group 6 interrupts */ > + > + regmap_read(drvdata->base, PLREG_INT_GRP6_FLAG, &val); > + > + if (val & BIT(2)) { > + u8 old_psu = psu_presence; > + > + regmap_read(drvdata->base, PLREG_PSU_INST, &val); > + psu_presence = (u8)val; > + > + if (old_psu != psu_presence) { > + /* Identify all bits which differs */ > + u8 current_val = psu_presence; > + u8 old_val = old_psu; > + for (i = 0 ; i < 8 ; i++) { > + if ((current_val & 0x1) != (old_val & 0x1)) { Make them unsigned long, use bitmap_xor() or just ^ followed by for_each_set_bit(). > + /* PSU state has changed */ > + girq = irq_find_mapping(drvdata->chip.irq.domain, > + i + PSU1_INST); > + if (girq) > + generic_handle_irq(girq); See above. > + } > + current_val = current_val >> 1; > + old_val = old_val >> 1; No need with the above suggestion. > + } > + } > + } > + > + /* Clear latched interrupt */ > + regmap_update_bits(drvdata->interrupt, PLREG_INT_GRP6_FLAG, > + 0xFF, 0xFF); GENMASK() > + regmap_update_bits(drvdata->interrupt, PLREG_INT_GRP6_BASE, > + BIT(2), 0); > + > + return IRQ_HANDLED; > +} > + > +static struct gpio_chip plreg_chip = { Make it const and mark as a template (in the name). > + .label = "gxp_gpio_plreg", > + .owner = THIS_MODULE, > + .get = gxp_gpio_pl_get, > + .set = gxp_gpio_pl_set, > + .get_direction = gxp_gpio_pl_get_direction, > + .direction_input = gxp_gpio_pl_direction_input, > + .direction_output = gxp_gpio_pl_direction_output, > + .base = -1, > +}; > + > +static struct irq_chip gxp_plreg_irqchip = { Make it const and immutable. > + .name = "gxp_plreg", > + .irq_ack = gxp_gpio_pl_irq_ack, > + .irq_mask = gxp_gpio_pl_irq_mask, > + .irq_unmask = gxp_gpio_pl_irq_unmask, > + .irq_set_type = gxp_gpio_pl_set_type, > +}; ... > + psu_presence = (u8)val; Why casting? ... > + ret = devm_gpiochip_add_data(&pdev->dev, &drvdata->chip, drvdata); > + if (ret < 0) What is the meaning of positive returned value? Why do we not care about it? > + dev_err_probe(&pdev->dev, ret, "Could not register gpiochip for plreg\n"); ... > + ret = platform_get_irq(pdev, 0); > + if (ret < 0) > + return dev_err_probe(&pdev->dev, ret, "Get irq from platform fail\n"); No need to repeat what the API already messages to the user. ... > + ret = devm_request_irq(&pdev->dev, drvdata->irq, gxp_gpio_pl_irq_handle, > + IRQF_SHARED, "gxp-pl", drvdata); > + if (ret < 0) > + return ret; > + > + return 0; return devm_request_irq(...); > +} ... > +static int gxp_gpio_probe(struct platform_device *pdev) > +{ > + int ret; > + struct gxp_gpio_drvdata *drvdata; > + > + /* Initialize global vars */ > + psu_presence = 0; > + drvdata = devm_kzalloc(&pdev->dev, sizeof(*drvdata), GFP_KERNEL); > + if (!drvdata) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, drvdata); > + ret = gxp_gpio_pl_init(pdev); > + > + return ret; return gxp_...; But why that function is separate? > +} ... > +++ b/drivers/gpio/gpio-gxp.c Take all above comments and apply here as well. ... Split this to two patches, one per driver. It makes easier reviews and flexible handling of the driver maintenance.
On Mon, Jun 05, 2023 at 08:42:33PM -0500, nick.hawkins@hpe.com wrote: > From: Nick Hawkins <nick.hawkins@hpe.com> > > The fan driver now is independent of the fan plreg GPIO information. > Therefore there will no longer be presence or fail information available > from the driver. Part of the changes includes removing a system power check > as the GPIO driver needs it to report power state to host. > > Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com> For my reference: Reviewed-by: Guenter Roeck <linux@roeck-us.net> Let me know if you want me to apply this patch now or if I should wait for the gpio patches to be accepted. Guenter
Hi Andy, Thanks for the feedback. I have a question below: > > + base = devm_platform_ioremap_resource_byname(pdev, reg_name); > > + if (IS_ERR(base)) > > + return ERR_CAST(base); > > + > > + regmap_config.name = reg_name; > > + > > + return devm_regmap_init_mmio(&pdev->dev, base, ®map_config); > Why are you not using gpio-regmap? Is there are good example or previous commit you would recommend looking at that shows how to convert from regmap to gpio-regmap? Later in the code I am using regmap_read and regmap_update_bits with large differences in offset registers, and not so much a contiguous block. Thank you for the review and assistance! -Nick Hawkins
On Tue, Jun 6, 2023 at 9:51 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote: ... > > Why are you not using gpio-regmap? > > Is there are good example or previous commit you would recommend > looking at that shows how to convert from regmap to gpio-regmap? > Later in the code I am using regmap_read and regmap_update_bits > with large differences in offset registers, and not so much a > contiguous block. I don't know how good these are, but that's what we have currently as most prominent use of gpio-regmap 1) (ongoing) https://lore.kernel.org/linux-gpio/20230606092107.764621-6-jiawenwu@trustnetic.com/ 2) (in the repo) https://elixir.bootlin.com/linux/v6.4-rc4/source/drivers/gpio/gpio-i8255.c 3) (in the repo) https://elixir.bootlin.com/linux/v6.4-rc4/source/drivers/gpio/gpio-104-idi-48.c 2) & 3) were converted, so you may see by executing respective `git log -p -- drivers/gpio/...`. -- With Best Regards, Andy Shevchenko
> > > Why are you not using gpio-regmap? > > > > Is there are good example or previous commit you would recommend > > looking at that shows how to convert from regmap to gpio-regmap? > > Later in the code I am using regmap_read and regmap_update_bits > > with large differences in offset registers, and not so much a > > contiguous block. > I don't know how good these are, but that's what we have currently as > most prominent use of gpio-regmap > 1) (ongoing) https://lore.kernel.org/linux-gpio/20230606092107.764621-6-jiawenwu@trustnetic.com <mailto:20230606092107.764621-6-jiawenwu@trustnetic.com>/ > 2) (in the repo) > https://elixir.bootlin.com/linux/v6.4-rc4/source/drivers/gpio/gpio-i8255.c <https://elixir.bootlin.com/linux/v6.4-rc4/source/drivers/gpio/gpio-i8255.c> > 3) (in the repo) > https://elixir.bootlin.com/linux/v6.4-rc4/source/drivers/gpio/gpio-104-idi-48.c <https://elixir.bootlin.com/linux/v6.4-rc4/source/drivers/gpio/gpio-104-idi-48.c> > 2) & 3) were converted, so you may see by executing respective `git > log -p -- drivers/gpio/...`. Greetings Andy, Thank you for those links, I have observed the gpio_regmap code they have implemented in that case. It appears that the regmap code is opening the entire range of memory to be read. For my particular purpose I am not wanting to expose all the 0-0xff byte range of the GPIOs. In my case is it still necessary to use the gpio_regmap code? If gpio_regmap is required, how do I create a direct correlation between a specific gpio-line and a register offset? For example, in gpio-gxp-pl.c. Gpio-line at offset 0 (IOPLED) is at register 0x04. The gpio-line at offset 8 (FAN_INST) is at register 0x27. Additionally, is it required to remove gpio_chip if gpio_regmap is used? Thank you for the assistance, -Nick Hawkins
> > The fan driver now is independent of the fan plreg GPIO information. > > Therefore there will no longer be presence or fail information available > > from the driver. Part of the changes includes removing a system power check > > as the GPIO driver needs it to report power state to host. > > > > Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com <mailto:nick.hawkins@hpe.com>> > For my reference: > Reviewed-by: Guenter Roeck <linux@roeck-us.net <mailto:linux@roeck-us.net>> > Let me know if you want me to apply this patch now or if I should wait > for the gpio patches to be accepted. Greetings Guenter, Please wait until GPIO patches are accepted. Thanks! -Nick Hawkins
On Wed, Jun 7, 2023 at 7:07 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote: ... > > > > Why are you not using gpio-regmap? > > > > > > Is there are good example or previous commit you would recommend > > > looking at that shows how to convert from regmap to gpio-regmap? > > > Later in the code I am using regmap_read and regmap_update_bits > > > with large differences in offset registers, and not so much a > > > contiguous block. > > > > I don't know how good these are, but that's what we have currently as > > most prominent use of gpio-regmap > > > > 1) (ongoing) https://lore.kernel.org/linux-gpio/20230606092107.764621-6-jiawenwu@trustnetic.com <mailto:20230606092107.764621-6-jiawenwu@trustnetic.com>/ > > 2) (in the repo) > > https://elixir.bootlin.com/linux/v6.4-rc4/source/drivers/gpio/gpio-i8255.c <https://elixir.bootlin.com/linux/v6.4-rc4/source/drivers/gpio/gpio-i8255.c> > > 3) (in the repo) > > https://elixir.bootlin.com/linux/v6.4-rc4/source/drivers/gpio/gpio-104-idi-48.c <https://elixir.bootlin.com/linux/v6.4-rc4/source/drivers/gpio/gpio-104-idi-48.c> > > > > 2) & 3) were converted, so you may see by executing respective `git > > log -p -- drivers/gpio/...`. > > Greetings Andy, > > Thank you for those links, I have observed the gpio_regmap code > they have implemented in that case. It appears that the regmap > code is opening the entire range of memory to be read. For my > particular purpose I am not wanting to expose all the 0-0xff byte > range of the GPIOs. This is also supported by regmap (and regmap has caches for the sparse registers as well). > In my case is it still necessary to use the > gpio_regmap code? It does care about things the average GPIO controller driver needs to repeat. So at least you may try and see how it will look. > If gpio_regmap is required, how do I create a direct correlation > between a specific gpio-line and a register offset? For example, in > gpio-gxp-pl.c. Gpio-line at offset 0 (IOPLED) is at register 0x04. The > gpio-line at offset 8 (FAN_INST) is at register 0x27. You may remap registers. See, for example, gpio-pca953x, where some of the registers (with high bit set) are actually virtual rather than real offsets. Similar idea can be used in your case. > Additionally, is it required to remove gpio_chip if gpio_regmap is > used? I don't think gpio_chip will be needed.
> It does care about things the average GPIO controller driver needs to > repeat. So at least you may try and see how it will look. > > If gpio_regmap is required, how do I create a direct correlation > > between a specific gpio-line and a register offset? For example, in > > gpio-gxp-pl.c. Gpio-line at offset 0 (IOPLED) is at register 0x04. The > > gpio-line at offset 8 (FAN_INST) is at register 0x27. > You may remap registers. See, for example, gpio-pca953x, where some of > the registers (with high bit set) are actually virtual rather than > real offsets. Similar idea can be used in your case. Greetings Andy, Is there any documents available describing how regmap_gpio populates the GPIO lines? Does it automatically go through and add lines for each successful regmap_read and bits per byte? I have taken your advice and used the additional readable and writeable on regmap_config to limit the number of accessible registers. static const struct regmap_config gxp_regmap_config = { .reg_bits = 8, .reg_stride = 1, .val_bits = 8, .readable_reg = gxp_readable_register, .writeable_reg = gxp_writeable_register, .max_register = 0x80, .name = "gxp-gpio-pl", }; static const struct regmap_config gxp_int_regmap_config = { .reg_bits = 8, .reg_stride = 1, .val_bits = 8, .readable_reg = gxp_read_write_int_register, .writeable_reg = gxp_read_write_int_register, .max_register = 0x7f .name = "gxp-gpio-pl-int" }; Thanks, -Nick Hawkins
On Wed, Jun 7, 2023 at 11:45 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote: > > > It does care about things the average GPIO controller driver needs to > > repeat. So at least you may try and see how it will look. ... > Is there any documents available describing how regmap_gpio > populates the GPIO lines? Does it automatically go through and add lines > for each successful regmap_read and bits per byte? Nope, it assumes one bit per register or something different if xlate callback is defined. This is my understanding. That said, it might be that this is a limitation which does not allow you to switch to that library. ... > static const struct regmap_config gxp_int_regmap_config = { > .reg_bits = 8, > .reg_stride = 1, AFAIU 0 is the same as 1, so this can be dropped. > .val_bits = 8, > .readable_reg = gxp_read_write_int_register, > .writeable_reg = gxp_read_write_int_register, > .max_register = 0x7f > .name = "gxp-gpio-pl-int" > };
> > Is there any documents available describing how regmap_gpio > > populates the GPIO lines? Does it automatically go through and add lines > > for each successful regmap_read and bits per byte? > Nope, it assumes one bit per register or something different if xlate > callback is defined. This is my understanding. That said, it might be > that this is a limitation which does not allow you to switch to that > library. Greetings Andy, Thank you for this feedback. After exploring the gpio_regmap it seems it does not fit my needs. Some of the GPIOs are a combination of several bits in a byte. For instance the Health LED or Identify LED have more than 2 states. If acceptable I believe the gxp-gpio-pl.c file should not use the gpio_regmap. Thanks, -Nick Hawkins
On Thu, Jun 8, 2023 at 5:58 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote: > > > Is there any documents available describing how regmap_gpio > > > populates the GPIO lines? Does it automatically go through and add lines > > > for each successful regmap_read and bits per byte? > > > Nope, it assumes one bit per register or something different if xlate > > callback is defined. This is my understanding. That said, it might be > > that this is a limitation which does not allow you to switch to that > > library. > > Thank you for this feedback. After exploring the gpio_regmap it seems > it does not fit my needs. Some of the GPIOs are a combination of > several bits in a byte. For instance the Health LED or Identify LED have > more than 2 states. If acceptable I believe the gxp-gpio-pl.c file should > not use the gpio_regmap. Yes, just mention this reasoning in the cover letter.
From: Nick Hawkins <nick.hawkins@hpe.com> The GXP SoC supports GPIO on multiple interfaces. The interfaces are CPLD and Host. The GPIOs is a combination of both physical and virtual I/O across the interfaces. The gpio-gxp driver specifically covers the CSM(physical), FN2(virtual), and VUHC(virtual) which are the host. The gpio-gxp-pl driver covers the CPLD which takes physical I/O from the board and shares it with GXP via a propriety interface that maps the I/O onto a specific register area of the GXP. The drivers both support interrupts but from different interrupt parents. 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. --- 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 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 | 139 ++++ .../bindings/hwmon/hpe,gxp-fan-ctrl.yaml | 16 +- MAINTAINERS | 2 + drivers/gpio/Kconfig | 18 + drivers/gpio/Makefile | 2 + drivers/gpio/gpio-gxp-pl.c | 519 ++++++++++++++ drivers/gpio/gpio-gxp.c | 637 ++++++++++++++++++ drivers/hwmon/Kconfig | 2 +- drivers/hwmon/gxp-fan-ctrl.c | 108 +-- 9 files changed, 1324 insertions(+), 119 deletions(-) create mode 100644 Documentation/devicetree/bindings/gpio/hpe,gxp-gpio.yaml create mode 100644 drivers/gpio/gpio-gxp-pl.c create mode 100644 drivers/gpio/gpio-gxp.c