| Message ID | 20211012134027.684712-13-kernel@esmil.dk (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Basic StarFive JH7100 RISC-V SoC support | expand |
On Tue, Oct 12, 2021 at 4:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > Add a combined pinctrl and gpio driver for the StarFive JH7100 SoC. > > For each "GPIO" there are two registers for configuring the output and > output enable signals which may come from other peripherals. Among these > are two special signals that are constant 0 and constant 1 respectively. > Controlling the GPIOs from software is done by choosing one of these > signals. In other words the same registers are used for both pinmuxing > and controlling the GPIOs, which makes it easier to combine the pinctrl > and gpio driver in one. > > I wrote the pinconf and pinmux parts, but the gpio part of the code is > based on the gpio driver in the vendor tree written by Huan Feng with > cleanups and fixes by Drew and me. s/gpio/GPIO/g ... > +config PINCTRL_STARFIVE > + bool "Pinctrl and GPIO driver for the StarFive JH7100 SoC" Why not module? > + depends on SOC_STARFIVE || COMPILE_TEST > + depends on OF Do you really need this taking into account... > + default SOC_STARFIVE > + select GENERIC_PINCTRL_GROUPS > + select GENERIC_PINMUX_FUNCTIONS > + select GENERIC_PINCONF > + select GPIOLIB > + select GPIOLIB_IRQCHIP > + select OF_GPIO ...this one? ... bits.h ? > +#include <linux/clk.h> > +#include <linux/gpio/driver.h> > +#include <linux/io.h> > +#include <linux/module.h> mod_devicetable.h ? > +#include <linux/pinctrl/pinctrl.h> > +#include <linux/pinctrl/pinmux.h> Can you move these as a group after generic linux/* ones? > +#include <linux/platform_device.h> > +#include <linux/reset.h> > +#include <linux/spinlock.h> ... > +/* > + * refer to Section 12. GPIO Registers in JH7100 datasheet: Be consistent in your style, here for example missed capitalization. > + * https://github.com/starfive-tech/StarLight_Docs Is it possible to have the datasheet to be provided as Datasheet: tag in the commit message? > + */ ... > +/* > + * Global enable for GPIO interrupts, offset: 0x0, field: GPIOEN > + * set to 1 if GPIO interrupts are enabled, set to 0 to disable > + */ > +#define IRQ_GLOBAL_EN 0x0 s/0x0/0x00/g ... > +/* > + * Interrupt Type for GPIO[31:0], offset: 0x10, field: GPIOS_0 > + * set to 1 if edge-triggered, set to 0 for level-triggered > + */ > +#define IRQ_TYPE_LOW 0x10 > + > +/* > + * Interrupt Type for GPIO[63:32], offset: 0x14, field: GPIOS_1 > + */ > +#define IRQ_TYPE_HIGH 0x14 As I reviewed below, the IRQ is represented by a few registers in a row, no need to define low and high separately. Ditto for the rest register pairs. ... > +/* > + * Interrupt Status after Masking GPIO[31:0], offset: 0x40, field: GPIOMIS_0 > + * status of edge-triggered or level-triggered after masking > + * value of 1 means edge or level was detected, value of 0 menas not detected menas?! > + */ ... > +/* > + * Data Value of GPIO for GPIO[31:0], offest: 0x48, field: GPIODIN_0 offest?! > + * dynamically reflects value on the GPIO pin > + */ Please, run a spellchecker. ... > +#define IO_PADSHARE_SEL 0x1a0 Okay, make all registers to be fixed width, i.e. 0x000 for IRQ global enabling and so on. ... > +#define PAD_SLEW_RATE_MASK 0xe00U GENMASK() > +#define PAD_BIAS_STRONG_PULL_UP 0x100U > +#define PAD_INPUT_ENABLE 0x080U > +#define PAD_INPUT_SCHMITT_ENABLE 0x040U > +#define PAD_BIAS_DISABLE 0x020U > +#define PAD_BIAS_PULL_DOWN 0x010U All above seems like BIT(_something_). > +#define PAD_BIAS_MASK 0x130U > +#define PAD_DRIVE_STRENGTH_MASK 0x007U GENMASK() ... > +#ifdef CONFIG_DEBUG_FS __maybe_unused ? > +#else > +#define starfive_pin_dbg_show NULL > +#endif ... > + dout = readl_relaxed(reg); readl_relaxed(reg + 0x00) > + reg += 4; > + doen = readl_relaxed(reg); readl_relaxed(reg + 0x04); ... > + seq_printf(s, "dout=%u%s doen=%u%s", > + dout & 0xffU, (dout & 0x80000000U) ? "r" : "", > + doen & 0xffU, (doen & 0x80000000U) ? "r" : ""); GENMASK() BIT() ... > + for_each_child_of_node(np, child) { > + const __be32 *pinmux_list; > + const __be32 *pins_list; > + int pinmux_size; > + int pins_size; > + > + pinmux_list = of_get_property(child, "pinmux", &pinmux_size); > + pins_list = of_get_property(child, "pins", &pins_size); > + if (pinmux_list && pins_list) { > + dev_err(dev, "invalid pinctrl group %pOFn.%pOFn: %s\n", > + np, child, "both pinmux and pins set"); > + of_node_put(child); > + return -EINVAL; > + } > + > + if (pinmux_list && pinmux_size > 0) { > + nmaps += 2; > + } else if (pins_list && pins_size > 0) { > + nmaps += 1; > + } else { > + dev_err(dev, "invalid pinctrl group %pOFn.%pOFn: %s\n", > + np, child, "neither pinmux nor pins set"); > + of_node_put(child); > + return -EINVAL; > + } > + ngroups += 1; > + } This entire loop seems like 1) it should be based on something from pin control core; 2) it's using some low level APIs instead of better ones like of_property_read_uXX(); 3) smells like unoptimized NIH. ... > + if ((list = of_get_property(child, "pinmux", &npins))) { Why not of_property_read_...() ? ... > + u32 v = be32_to_cpu(*list++); My gosh! ... > + for (i = 0; i < npins; i++) > + pins[i] = be32_to_cpu(*list++); Ditto. Even for this we have something in byteorder headers. Summary, make sure you use much better _existing_ APIs instead of the above crap. ... > +free_pinmux: > + devm_kfree(dev, pinmux); > +free_pins: > + devm_kfree(dev, pins); > +free_grpname: > + devm_kfree(dev, grpname); What the heck?! > +free_pgnames: > + devm_kfree(dev, pgnames); Ditto. ... > +out: Useless label. > + return ret; ... > + for (i = 0; i < group->num_pins; i++) { > + unsigned int gpio = starfive_pin_to_gpio(sfp, group->pins[i]); > + void __iomem *reg_dout; > + void __iomem *reg_doen; > + void __iomem *reg_din; > + u32 v, dout, doen, din; > + unsigned long flags; > + if (dev_WARN_ONCE(dev, gpio >= MAX_GPIO, What?! > + "%s: invalid gpiomux pin", group->name)) > + continue; > + > + v = pinmux[i]; > + dout = ((v & BIT(7)) << (31 - 7)) | ((v >> 24) & 0xffU); > + doen = ((v & BIT(6)) << (31 - 6)) | ((v >> 16) & 0xffU); > + din = (v >> 8) & 0xffU; What is this voodoo for? > + if (din != 0xff) > + reg_din = sfp->base + GPIO_IN_OFFSET + 4 * din; > + else > + reg_din = NULL; This looks like you maybe use gpio-regmap instead? ... > + void __iomem *reg = sfp->padctl + 4 * (pin / 2); > + u32 value = readl_relaxed(reg); > + > + if (pin & 1U) > + value >>= 16; > + return value; u8 shift = 16 * (pin % 2); return readl_relaxed() >> shift; ? Something similar for below code. ... > +#ifdef CONFIG_DEBUG_FS > +static const struct pin_config_item > +starfive_pinconf_custom_conf_items[ARRAY_SIZE(starfive_pinconf_custom_params)] = { Instead of using ARAY_SIZE() here, use static_assert(). __maybe_unused? > + PCONFDUMP(PIN_CONFIG_STARFIVE_STRONG_PULL_UP, "input bias strong pull-up", NULL, false), > +}; > +#else > +#define starfive_pinconf_custom_conf_items NULL > +#endif ... > +static const unsigned char starfive_drive_strength[] = { > + 14, 21, 28, 35, 42, 49, 56, 63, Why table? Can you simply use the formula?! > +}; ... > + if (unlikely(!group)) Why unlikely() Must be justified here and everywhere where you are using it. > + return -EINVAL; > + > + return starfive_pinconf_get(pctldev, group->pins[0], config); > +} ... > + case PIN_CONFIG_BIAS_DISABLE: > + mask |= PAD_BIAS_MASK; Use it... > + value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE; ...here. Ditto for the similar cases in this function and elsewhere. After done this, you will see how you can simplify and deduplicate the switch-cases. ... > +#ifdef CONFIG_DEBUG_FS __maybe_unused ? > +#else > +#define starfive_pinconf_dbg_show NULL > +#endif ... > + if (gpio < 32) { > + value = readl_relaxed(sfp->base + GPIO_DIN_LOW); > + value = (value >> gpio) & 1U; Drop > + } else { > + value = readl_relaxed(sfp->base + GPIO_DIN_HIGH); > + value = (value >> (gpio - 32)) & 1U; Drop > + } > + return value; return !!(value & BIT(gpio % 32)); ... > + if (arg == 0) > + return -ENOTSUPP; Shouldn't we return something else and pin control core will change it to something else if needed? > + if (arg == 0) > + return -ENOTSUPP; Ditto. > + default: > + return -ENOTSUPP; ... > + if (gpio < 0 || gpio >= MAX_GPIO) > + return; > + > + if (gpio < 32) { > + ie = sfp->base + IRQ_ENABLE_LOW; > + mask = BIT(gpio); > + } else { > + ie = sfp->base + IRQ_ENABLE_HIGH; > + mask = BIT(gpio - 32); > + } See below. And update all occurrences of these lines accordingly and everywhere. Also for IRQ may use helper functions if needed (but I don't believe the high and low register have stride more than 4). ... > + if (gpio < 0 || gpio >= MAX_GPIO) > + return -EINVAL; How is it possible to be ever triggered? ... > + if (gpio < 32) { > + base = sfp->base; > + mask = BIT(gpio); > + } else { > + base = sfp->base + 4; > + mask = BIT(gpio - 32); > + } base = sfp_base + 4 * (gpio / 32); mask = BIT(gpio % 32); ... > + irq_set_handler_locked(d, handle_edge_irq); > + irq_set_handler_locked(d, handle_edge_irq); Dup. ... > + irq_set_handler_locked(d, handle_edge_irq); > + irq_set_handler_locked(d, handle_level_irq); > + irq_set_handler_locked(d, handle_level_irq); Ditto. ... > + irq_set_handler_locked(d, handle_bad_irq); Why is this here? Move it to ->probe(). ... > + clk = devm_clk_get(dev, NULL); > + if (IS_ERR(clk)) { > + ret = PTR_ERR(clk); > + dev_err(dev, "could not get clock: %d\n", ret); Thank you for spamming logs with this noise. > + return ret; Hint: return dev_err_probe(). Ditto for the rest in this function. > + } ... > + ret = clk_prepare_enable(clk); > + if (ret) { > + reset_control_deassert(rst); Use devm_add_action_or_reset(). > + dev_err(dev, "could not enable clock: %d\n", ret); > + return ret; > + } ... > + if (!of_property_read_u32(dev->of_node, "starfive,signal-group", &value)) { Can be refactored without conditional. Also, why not to use device_property_read_u32()? > + if (value <= 6) > + writel(value, sfp->padctl + IO_PADSHARE_SEL); > + else > + dev_err(dev, "invalid signal group %u\n", value); Why _err if you not bail out here? > + } ... > + value = readl(sfp->padctl + IO_PADSHARE_SEL); > + switch (value) { > + case 0: > + sfp->gpios.pin_base = 0x10000; Magic number! > + goto done; > + case 1: > + sfp->gpios.pin_base = PAD_GPIO(0); > + break; > + case 2: > + sfp->gpios.pin_base = PAD_FUNC_SHARE(72); > + break; > + case 3: > + sfp->gpios.pin_base = PAD_FUNC_SHARE(70); > + break; > + case 4: case 5: case 6: > + sfp->gpios.pin_base = PAD_FUNC_SHARE(0); > + break; > + default: > + dev_err(dev, "invalid signal group %u\n", value); > + return -EINVAL; > + } ... > + sfp->gc.of_node = dev->of_node; Isn't GPIO library do this for you? ... > + starfive_irq_chip.parent_device = dev; Ditto? ... > + sfp->gc.irq.parents = > + devm_kcalloc(dev, 1, sizeof(*sfp->gc.irq.parents), GFP_KERNEL); 1 -> sfp->gc.irq.num_parents And hence move below line up. > + if (!sfp->gc.irq.parents) > + return -ENOMEM; > + sfp->gc.irq.num_parents = 1; ... > + dev_info(dev, "StarFive GPIO chip registered %d GPIOs\n", sfp->gc.ngpio); Redundant noise. ... > +static const struct of_device_id starfive_of_match[] = { > + { .compatible = "starfive,jh7100-pinctrl" }, > + { /* sentinel */ }, No comma needed for terminator entry. > +};
On Tue, 12 Oct 2021 at 19:03, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Oct 12, 2021 at 4:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > > > Add a combined pinctrl and gpio driver for the StarFive JH7100 SoC. > > > > For each "GPIO" there are two registers for configuring the output and > > output enable signals which may come from other peripherals. Among these > > are two special signals that are constant 0 and constant 1 respectively. > > Controlling the GPIOs from software is done by choosing one of these > > signals. In other words the same registers are used for both pinmuxing > > and controlling the GPIOs, which makes it easier to combine the pinctrl > > and gpio driver in one. > > > > I wrote the pinconf and pinmux parts, but the gpio part of the code is > > based on the gpio driver in the vendor tree written by Huan Feng with > > cleanups and fixes by Drew and me. > > s/gpio/GPIO/g > > ... > > > +config PINCTRL_STARFIVE > > > + bool "Pinctrl and GPIO driver for the StarFive JH7100 SoC" > > Why not module? > > > + depends on SOC_STARFIVE || COMPILE_TEST > > > + depends on OF > > Do you really need this taking into account... > > > + default SOC_STARFIVE > > + select GENERIC_PINCTRL_GROUPS > > + select GENERIC_PINMUX_FUNCTIONS > > + select GENERIC_PINCONF > > + select GPIOLIB > > + select GPIOLIB_IRQCHIP > > > + select OF_GPIO > > ...this one? > > ... > > bits.h ? > > > +#include <linux/clk.h> > > +#include <linux/gpio/driver.h> > > +#include <linux/io.h> > > +#include <linux/module.h> > > > mod_devicetable.h ? > > > +#include <linux/pinctrl/pinctrl.h> > > +#include <linux/pinctrl/pinmux.h> > > Can you move these as a group after generic linux/* ones? > > > +#include <linux/platform_device.h> > > +#include <linux/reset.h> > > +#include <linux/spinlock.h> > > ... > > > +/* > > + * refer to Section 12. GPIO Registers in JH7100 datasheet: > > Be consistent in your style, here for example missed capitalization. > > > + * https://github.com/starfive-tech/StarLight_Docs > > Is it possible to have the datasheet to be provided as Datasheet: tag > in the commit message? Oh neat. Didn't know about that. I'll also check with StarFive if this is the best address for it. > > + */ > > ... > > > +/* > > + * Global enable for GPIO interrupts, offset: 0x0, field: GPIOEN > > + * set to 1 if GPIO interrupts are enabled, set to 0 to disable > > + */ > > +#define IRQ_GLOBAL_EN 0x0 > > s/0x0/0x00/g > > ... > > > +/* > > + * Interrupt Type for GPIO[31:0], offset: 0x10, field: GPIOS_0 > > + * set to 1 if edge-triggered, set to 0 for level-triggered > > + */ > > +#define IRQ_TYPE_LOW 0x10 > > + > > +/* > > + * Interrupt Type for GPIO[63:32], offset: 0x14, field: GPIOS_1 > > + */ > > +#define IRQ_TYPE_HIGH 0x14 > > As I reviewed below, the IRQ is represented by a few registers in a > row, no need to define low and high separately. Ditto for the rest > register pairs. > > ... > > > +/* > > + * Interrupt Status after Masking GPIO[31:0], offset: 0x40, field: GPIOMIS_0 > > + * status of edge-triggered or level-triggered after masking > > + * value of 1 means edge or level was detected, value of 0 menas not detected > > menas?! > > > + */ > > ... > > > +/* > > + * Data Value of GPIO for GPIO[31:0], offest: 0x48, field: GPIODIN_0 > > offest?! > > > + * dynamically reflects value on the GPIO pin > > + */ > > Please, run a spellchecker. > > ... > > > +#define IO_PADSHARE_SEL 0x1a0 > > Okay, make all registers to be fixed width, i.e. 0x000 for IRQ global > enabling and so on. > > ... > > > +#define PAD_SLEW_RATE_MASK 0xe00U > > GENMASK() > > > +#define PAD_BIAS_STRONG_PULL_UP 0x100U > > +#define PAD_INPUT_ENABLE 0x080U > > +#define PAD_INPUT_SCHMITT_ENABLE 0x040U > > +#define PAD_BIAS_DISABLE 0x020U > > +#define PAD_BIAS_PULL_DOWN 0x010U > > All above seems like BIT(_something_). > > > +#define PAD_BIAS_MASK 0x130U > > +#define PAD_DRIVE_STRENGTH_MASK 0x007U > > GENMASK() > > ... > > > +#ifdef CONFIG_DEBUG_FS > > __maybe_unused ? This is the same as the clock driver. If we don't have the ifdef here we'll need it when populating the ops structure later or live with the code bloat when debugfs is not enabled. Other driver don't seem to do that. > > +#else > > +#define starfive_pin_dbg_show NULL > > +#endif > > ... > > > + dout = readl_relaxed(reg); > > readl_relaxed(reg + 0x00) > > > + reg += 4; > > > + doen = readl_relaxed(reg); > > readl_relaxed(reg + 0x04); > > ... > > > + seq_printf(s, "dout=%u%s doen=%u%s", > > + dout & 0xffU, (dout & 0x80000000U) ? "r" : "", > > + doen & 0xffU, (doen & 0x80000000U) ? "r" : ""); > > GENMASK() > BIT() > > ... > > > + for_each_child_of_node(np, child) { > > + const __be32 *pinmux_list; > > + const __be32 *pins_list; > > + int pinmux_size; > > + int pins_size; > > + > > + pinmux_list = of_get_property(child, "pinmux", &pinmux_size); > > + pins_list = of_get_property(child, "pins", &pins_size); > > + if (pinmux_list && pins_list) { > > + dev_err(dev, "invalid pinctrl group %pOFn.%pOFn: %s\n", > > + np, child, "both pinmux and pins set"); > > + of_node_put(child); > > + return -EINVAL; > > + } > > + > > + if (pinmux_list && pinmux_size > 0) { > > + nmaps += 2; > > + } else if (pins_list && pins_size > 0) { > > + nmaps += 1; > > + } else { > > + dev_err(dev, "invalid pinctrl group %pOFn.%pOFn: %s\n", > > + np, child, "neither pinmux nor pins set"); > > + of_node_put(child); > > + return -EINVAL; > > + } > > + ngroups += 1; > > + } > > This entire loop seems like > 1) it should be based on something from pin control core; > 2) it's using some low level APIs instead of better ones like > of_property_read_uXX(); > 3) smells like unoptimized NIH. > > ... > > > + if ((list = of_get_property(child, "pinmux", &npins))) { > > Why not of_property_read_...() ? > > ... > > > + u32 v = be32_to_cpu(*list++); > > My gosh! > > ... > > > + for (i = 0; i < npins; i++) > > + pins[i] = be32_to_cpu(*list++); > > Ditto. > Even for this we have something in byteorder headers. > > Summary, make sure you use much better _existing_ APIs instead of the > above crap. > > ... > > > +free_pinmux: > > + devm_kfree(dev, pinmux); > > +free_pins: > > + devm_kfree(dev, pins); > > +free_grpname: > > + devm_kfree(dev, grpname); > > What the heck?! Just to be clear. You mean we don't need to explicitly free them because they're tied to the device right? I don't think the device will go away just because a single device tree entry can't be parsed, so on such errors this garbage would be left behind. You can still argue we shouldn't optimize for broken device trees, I just want to make it at conscious decision. > > +free_pgnames: > > + devm_kfree(dev, pgnames); > > Ditto. > > ... > > > +out: > > Useless label. Hmm.. my compiler disagrees. > > + return ret; > > ... > > > + for (i = 0; i < group->num_pins; i++) { > > + unsigned int gpio = starfive_pin_to_gpio(sfp, group->pins[i]); > > + void __iomem *reg_dout; > > + void __iomem *reg_doen; > > + void __iomem *reg_din; > > + u32 v, dout, doen, din; > > + unsigned long flags; > > > + if (dev_WARN_ONCE(dev, gpio >= MAX_GPIO, > > What?! > > > + "%s: invalid gpiomux pin", group->name)) > > + continue; > > + > > + v = pinmux[i]; > > + dout = ((v & BIT(7)) << (31 - 7)) | ((v >> 24) & 0xffU); > > + doen = ((v & BIT(6)) << (31 - 6)) | ((v >> 16) & 0xffU); > > + din = (v >> 8) & 0xffU; > > What is this voodoo for? In order to do pinmux we need the following pieces of information from the device tree for each pin ("GPIO" they call it): output signal: 0-133 + 1bit reverse flag output enable signal: 0-133 + 1bit reverse flag optional input signal: 0-74 + special "none" value, right now 0xff gpio number: 0-63 As the code is now all that info is packed into a u32 for each pin using the GPIOMUX macro defined in the dt-binding header added in patch 10. There is also a diagram for how this packing is done. The above voodoo is for unpacking that. I'd very much like to hear if you have a better solution for how to convey that information from the device tree to here. > > + if (din != 0xff) > > + reg_din = sfp->base + GPIO_IN_OFFSET + 4 * din; > > + else > > + reg_din = NULL; > > This looks like you maybe use gpio-regmap instead? This was discussed at length when Drew sent in the GPIO part of this code: https://lore.kernel.org/linux-riscv/20210701002037.912625-1-drew@beagleboard.org/ The conclusion was that because pinmux and controlling the pins from software in GPIO mode uses the same registers it is better to do a combined driver like this that can share the lock among other things. > ... > > > + void __iomem *reg = sfp->padctl + 4 * (pin / 2); > > + u32 value = readl_relaxed(reg); > > + > > + if (pin & 1U) > > + value >>= 16; > > + return value; > > u8 shift = 16 * (pin % 2); > > return readl_relaxed() >> shift; > > ? > > Something similar for below code. > > ... > > > +#ifdef CONFIG_DEBUG_FS > > +static const struct pin_config_item > > +starfive_pinconf_custom_conf_items[ARRAY_SIZE(starfive_pinconf_custom_params)] = { > > Instead of using ARAY_SIZE() here, use static_assert(). > > __maybe_unused? As above. > > + PCONFDUMP(PIN_CONFIG_STARFIVE_STRONG_PULL_UP, "input bias strong pull-up", NULL, false), > > +}; > > +#else > > +#define starfive_pinconf_custom_conf_items NULL > > +#endif > > ... > > > +static const unsigned char starfive_drive_strength[] = { > > + 14, 21, 28, 35, 42, 49, 56, 63, > > Why table? Can you simply use the formula?! Heh, yeah. So these are rounded values from a table and I never noticed that after rounding they follow a nice arithmetic progression. It'll probably still be nice to have an explanation in the comments about the formula then. > > +}; > > ... > > > + if (unlikely(!group)) > > Why unlikely() Must be justified here and everywhere where you are using it. > > > + return -EINVAL; > > + > > + return starfive_pinconf_get(pctldev, group->pins[0], config); > > +} > > ... > > > + case PIN_CONFIG_BIAS_DISABLE: > > > + mask |= PAD_BIAS_MASK; > > Use it... > > > + value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE; > > ...here. Ditto for the similar cases in this function and elsewhere. > > After done this, you will see how you can simplify and deduplicate the > switch-cases. > > ... > > > +#ifdef CONFIG_DEBUG_FS > > __maybe_unused ? > > > +#else > > +#define starfive_pinconf_dbg_show NULL > > +#endif > > ... > > > + if (gpio < 32) { > > + value = readl_relaxed(sfp->base + GPIO_DIN_LOW); > > > + value = (value >> gpio) & 1U; > > Drop > > > + } else { > > + value = readl_relaxed(sfp->base + GPIO_DIN_HIGH); > > > + value = (value >> (gpio - 32)) & 1U; > > Drop > > > + } > > > + return value; > > return !!(value & BIT(gpio % 32)); > > ... > > > + if (arg == 0) > > > + return -ENOTSUPP; > > Shouldn't we return something else and pin control core will change it > to something else if needed? The documentation include/linux/pinctrl/pinconf.h has rules for when to return -ENOTSUPP and -EINVAL. > > + if (arg == 0) > > + return -ENOTSUPP; > > Ditto. > > > + default: > > + return -ENOTSUPP; > > ... > > > + if (gpio < 0 || gpio >= MAX_GPIO) > > + return; > > + > > + if (gpio < 32) { > > + ie = sfp->base + IRQ_ENABLE_LOW; > > + mask = BIT(gpio); > > + } else { > > + ie = sfp->base + IRQ_ENABLE_HIGH; > > + mask = BIT(gpio - 32); > > + } > > See below. And update all occurrences of these lines accordingly and > everywhere. Also for IRQ may use helper functions if needed (but I > don't believe the high and low register have stride more than 4). > > ... > > > + if (gpio < 0 || gpio >= MAX_GPIO) > > + return -EINVAL; > > How is it possible to be ever triggered? > > ... > > > + if (gpio < 32) { > > + base = sfp->base; > > + mask = BIT(gpio); > > + } else { > > + base = sfp->base + 4; > > + mask = BIT(gpio - 32); > > + } > > base = sfp_base + 4 * (gpio / 32); > mask = BIT(gpio % 32); > > ... > > > + irq_set_handler_locked(d, handle_edge_irq); > > > + irq_set_handler_locked(d, handle_edge_irq); > > Dup. > > ... > > > + irq_set_handler_locked(d, handle_edge_irq); > > > + irq_set_handler_locked(d, handle_level_irq); > > > + irq_set_handler_locked(d, handle_level_irq); > > Ditto. > > ... > > > + irq_set_handler_locked(d, handle_bad_irq); > > Why is this here? Move it to ->probe(). My thinking was that if something tries to set a an unsupported irq type, we should make sure the caller doesn't get spurious interrupts because we left the handler at its old value. > ... > > > + clk = devm_clk_get(dev, NULL); > > + if (IS_ERR(clk)) { > > + ret = PTR_ERR(clk); > > > + dev_err(dev, "could not get clock: %d\n", ret); > > Thank you for spamming logs with this noise. > > > + return ret; > > Hint: return dev_err_probe(). Ditto for the rest in this function. > > > + } > > ... > > > + ret = clk_prepare_enable(clk); > > + if (ret) { > > > + reset_control_deassert(rst); > > Use devm_add_action_or_reset(). > > > + dev_err(dev, "could not enable clock: %d\n", ret); > > + return ret; > > + } > > ... > > > + if (!of_property_read_u32(dev->of_node, "starfive,signal-group", &value)) { > > Can be refactored without conditional. Also, why not to use > device_property_read_u32()? > > > + if (value <= 6) > > + writel(value, sfp->padctl + IO_PADSHARE_SEL); > > + else > > > + dev_err(dev, "invalid signal group %u\n", value); > > Why _err if you not bail out here? My thinking was that if the device tree specifies an invalid signal group we should just leave the setting alone and not break booting, but still be loud about it. Maybe that's too lenient and it's better to crash and burn immediately if someone does that. > > + } > > ... > > > + value = readl(sfp->padctl + IO_PADSHARE_SEL); > > + switch (value) { > > + case 0: > > + sfp->gpios.pin_base = 0x10000; > > Magic number! > > > + goto done; > > + case 1: > > + sfp->gpios.pin_base = PAD_GPIO(0); > > + break; > > + case 2: > > + sfp->gpios.pin_base = PAD_FUNC_SHARE(72); > > + break; > > + case 3: > > + sfp->gpios.pin_base = PAD_FUNC_SHARE(70); > > + break; > > + case 4: case 5: case 6: > > + sfp->gpios.pin_base = PAD_FUNC_SHARE(0); > > + break; > > + default: > > + dev_err(dev, "invalid signal group %u\n", value); > > + return -EINVAL; > > + } > > ... > > > + sfp->gc.of_node = dev->of_node; > > Isn't GPIO library do this for you? > > ... > > > + starfive_irq_chip.parent_device = dev; > > Ditto? > > ... > > > + sfp->gc.irq.parents = > > + devm_kcalloc(dev, 1, sizeof(*sfp->gc.irq.parents), GFP_KERNEL); > > 1 -> sfp->gc.irq.num_parents > And hence move below line up. > > > + if (!sfp->gc.irq.parents) > > + return -ENOMEM; > > > + sfp->gc.irq.num_parents = 1; > > ... > > > + dev_info(dev, "StarFive GPIO chip registered %d GPIOs\n", sfp->gc.ngpio); > > Redundant noise. > > ... > > > +static const struct of_device_id starfive_of_match[] = { > > + { .compatible = "starfive,jh7100-pinctrl" }, > > > + { /* sentinel */ }, > > No comma needed for terminator entry. > > > +}; > > -- > With Best Regards, > Andy Shevchenko
On Wed, 13 Oct 2021 at 18:59, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Oct 13, 2021 at 06:38:14PM +0200, Emil Renner Berthing wrote: > > On Tue, 12 Oct 2021 at 19:03, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Tue, Oct 12, 2021 at 4:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > ... > > > > > +free_pinmux: > > > > + devm_kfree(dev, pinmux); > > > > +free_pins: > > > > + devm_kfree(dev, pins); > > > > +free_grpname: > > > > + devm_kfree(dev, grpname); > > > > > > What the heck?! > > > > Just to be clear. You mean we don't need to explicitly free them > > because they're tied to the device right? I don't think the device > > will go away just because a single device tree entry can't be parsed, > > so on such errors this garbage would be left behind. You can still > > argue we shouldn't optimize for broken device trees, I just want to > > make it at conscious decision. > > If you are using devm_kfree() it is quite likely shows either of the following > issues: > > * you mustn't use devm_*() in the first place due to object lifetime; > * you shouldn't use devm_kfree() since this is the whole point of devm. Hmm.. it seems like other drivers that dynamically builds the groups and functions either also uses devm_kcalloc/devm_kfree like fx. pinctrl-single or implements their own code to clean up groups and functions when unloaded. There are no group destroy or function destroy callbacks. I like devm_kcalloc/devm_kfree version better since it's less code to write. > > > > +free_pgnames: > > > > + devm_kfree(dev, pgnames); > > > > > > Ditto. > > ... > > > > > +out: > > > > > > Useless label. > > > > Hmm.. my compiler disagrees. > > The comment implies that you return directly instead of using `goto out;`. > > > > > + return ret; > > ... > > > > > + v = pinmux[i]; > > > > + dout = ((v & BIT(7)) << (31 - 7)) | ((v >> 24) & 0xffU); > > > > + doen = ((v & BIT(6)) << (31 - 6)) | ((v >> 16) & 0xffU); > > > > + din = (v >> 8) & 0xffU; > > > > > > What is this voodoo for? > > > > In order to do pinmux we need the following pieces of information from > > the device tree for each pin ("GPIO" they call it): > > > > output signal: 0-133 + 1bit reverse flag > > output enable signal: 0-133 + 1bit reverse flag > > optional input signal: 0-74 + special "none" value, right now 0xff > > gpio number: 0-63 > > > > As the code is now all that info is packed into a u32 for each pin > > using the GPIOMUX macro defined in the dt-binding header added in > > patch 10. There is also a diagram for how this packing is done. The > > above voodoo is for unpacking that. > > > > I'd very much like to hear if you have a better solution for how to > > convey that information from the device tree to here. > > At very least this code should have something like above in the comment. Will add! > ... > > > > > + if (din != 0xff) > > > > + reg_din = sfp->base + GPIO_IN_OFFSET + 4 * din; > > > > + else > > > > + reg_din = NULL; > > > > > > This looks like you maybe use gpio-regmap instead? > > > > This was discussed at length when Drew sent in the GPIO part of this code: > > https://lore.kernel.org/linux-riscv/20210701002037.912625-1-drew@beagleboard.org/ > > The conclusion was that because pinmux and controlling the pins from > > software in GPIO mode uses the same registers it is better to do a > > combined driver like this that can share the lock among other things. > > And what does prevent exactly to base the GPIO part on gpio-regmap? Other reasons are that gpio-regmap doesn't implement the .set_config and .add_pin_ranges callbacks. add_pin_ranges is needed because the 64 GPIOs map to different pin numbers depending on the chosen "signal group". > ... > > > > > +static const unsigned char starfive_drive_strength[] = { > > > > + 14, 21, 28, 35, 42, 49, 56, 63, > > > > > > Why table? Can you simply use the formula?! > > > > Heh, yeah. So these are rounded values from a table and I never > > noticed that after rounding they follow a nice arithmetic progression. > > It'll probably still be nice to have an explanation in the comments > > about the formula then. > > Yup! > > > > > +}; > > ... > > > > > + irq_set_handler_locked(d, handle_bad_irq); > > > > > > Why is this here? Move it to ->probe(). > > > > My thinking was that if something tries to set a an unsupported irq > > type, we should make sure the caller doesn't get spurious interrupts > > because we left the handler at its old value. > > You already assigned to this handler in the ->probe(), what's this then? But userspace could fx. first request IRQ_TYPE_EDGE_BOTH through the gpio api and later load a driver that might request an unsupported irq type right? Or am I missing something. > ... > > > > > + if (value <= 6) > > > > + writel(value, sfp->padctl + IO_PADSHARE_SEL); > > > > + else > > > > > > > + dev_err(dev, "invalid signal group %u\n", value); > > > > > > Why _err if you not bail out here? > > > > My thinking was that if the device tree specifies an invalid signal > > group we should just leave the setting alone and not break booting, > > but still be loud about it. Maybe that's too lenient and it's better > > to crash and burn immediately if someone does that. > > Here is inconsistency between level of the message and following action. > There are (rare!) cases when it's justified, but I believe it's not the > case here. You have two choices or justify why you have to use error > level without stopping process. > > ... > > All uncommented stuff you agreed on, correct? Yes, thank you! (.. or at least I'll get back to you if something comes up ;) > > -- > With Best Regards, > Andy Shevchenko > > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Wed, Oct 13, 2021 at 6:58 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Oct 13, 2021 at 06:38:14PM +0200, Emil Renner Berthing wrote: > > On Tue, 12 Oct 2021 at 19:03, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Tue, Oct 12, 2021 at 4:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > > > + v = pinmux[i]; > > > > + dout = ((v & BIT(7)) << (31 - 7)) | ((v >> 24) & 0xffU); > > > > + doen = ((v & BIT(6)) << (31 - 6)) | ((v >> 16) & 0xffU); > > > > + din = (v >> 8) & 0xffU; > > > > > > What is this voodoo for? > > > > In order to do pinmux we need the following pieces of information from > > the device tree for each pin ("GPIO" they call it): > > > > output signal: 0-133 + 1bit reverse flag > > output enable signal: 0-133 + 1bit reverse flag > > optional input signal: 0-74 + special "none" value, right now 0xff > > gpio number: 0-63 > > > > As the code is now all that info is packed into a u32 for each pin > > using the GPIOMUX macro defined in the dt-binding header added in > > patch 10. There is also a diagram for how this packing is done. The > > above voodoo is for unpacking that. > > > > I'd very much like to hear if you have a better solution for how to > > convey that information from the device tree to here. > > At very least this code should have something like above in the comment. And perhaps introduce some helper macros to access the fields? Gr{oetje,eeting}s, Geert
Hi Emil,
I love your patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on clk/clk-next pza/reset/next linus/master v5.15-rc5 next-20211013]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Emil-Renner-Berthing/Basic-StarFive-JH7100-RISC-V-SoC-support/20211012-220348
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: microblaze-randconfig-r033-20211013 (attached as .config)
compiler: microblaze-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/39e70136443b16615b0b83a448d6c5106d68cef7
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Emil-Renner-Berthing/Basic-StarFive-JH7100-RISC-V-SoC-support/20211012-220348
git checkout 39e70136443b16615b0b83a448d6c5106d68cef7
# save the attached .config to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=microblaze SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>, old ones prefixed by <<):
>> WARNING: modpost: vmlinux.o(.data+0x8a2c8): Section mismatch in reference from the variable starfive_pinctrl_driver to the function .init.text:starfive_probe()
The variable starfive_pinctrl_driver references
the function __init starfive_probe()
If the reference is valid then annotate the
variable with or __refdata (see linux/init.h) or name the variable:
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Wed, Oct 13, 2021 at 06:38:14PM +0200, Emil Renner Berthing wrote: > On Tue, 12 Oct 2021 at 19:03, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Tue, Oct 12, 2021 at 4:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: ... > > > +free_pinmux: > > > + devm_kfree(dev, pinmux); > > > +free_pins: > > > + devm_kfree(dev, pins); > > > +free_grpname: > > > + devm_kfree(dev, grpname); > > > > What the heck?! > > Just to be clear. You mean we don't need to explicitly free them > because they're tied to the device right? I don't think the device > will go away just because a single device tree entry can't be parsed, > so on such errors this garbage would be left behind. You can still > argue we shouldn't optimize for broken device trees, I just want to > make it at conscious decision. If you are using devm_kfree() it is quite likely shows either of the following issues: * you mustn't use devm_*() in the first place due to object lifetime; * you shouldn't use devm_kfree() since this is the whole point of devm. > > > +free_pgnames: > > > + devm_kfree(dev, pgnames); > > > > Ditto. ... > > > +out: > > > > Useless label. > > Hmm.. my compiler disagrees. The comment implies that you return directly instead of using `goto out;`. > > > + return ret; ... > > > + v = pinmux[i]; > > > + dout = ((v & BIT(7)) << (31 - 7)) | ((v >> 24) & 0xffU); > > > + doen = ((v & BIT(6)) << (31 - 6)) | ((v >> 16) & 0xffU); > > > + din = (v >> 8) & 0xffU; > > > > What is this voodoo for? > > In order to do pinmux we need the following pieces of information from > the device tree for each pin ("GPIO" they call it): > > output signal: 0-133 + 1bit reverse flag > output enable signal: 0-133 + 1bit reverse flag > optional input signal: 0-74 + special "none" value, right now 0xff > gpio number: 0-63 > > As the code is now all that info is packed into a u32 for each pin > using the GPIOMUX macro defined in the dt-binding header added in > patch 10. There is also a diagram for how this packing is done. The > above voodoo is for unpacking that. > > I'd very much like to hear if you have a better solution for how to > convey that information from the device tree to here. At very least this code should have something like above in the comment. ... > > > + if (din != 0xff) > > > + reg_din = sfp->base + GPIO_IN_OFFSET + 4 * din; > > > + else > > > + reg_din = NULL; > > > > This looks like you maybe use gpio-regmap instead? > > This was discussed at length when Drew sent in the GPIO part of this code: > https://lore.kernel.org/linux-riscv/20210701002037.912625-1-drew@beagleboard.org/ > The conclusion was that because pinmux and controlling the pins from > software in GPIO mode uses the same registers it is better to do a > combined driver like this that can share the lock among other things. And what does prevent exactly to base the GPIO part on gpio-regmap? ... > > > +static const unsigned char starfive_drive_strength[] = { > > > + 14, 21, 28, 35, 42, 49, 56, 63, > > > > Why table? Can you simply use the formula?! > > Heh, yeah. So these are rounded values from a table and I never > noticed that after rounding they follow a nice arithmetic progression. > It'll probably still be nice to have an explanation in the comments > about the formula then. Yup! > > > +}; ... > > > + irq_set_handler_locked(d, handle_bad_irq); > > > > Why is this here? Move it to ->probe(). > > My thinking was that if something tries to set a an unsupported irq > type, we should make sure the caller doesn't get spurious interrupts > because we left the handler at its old value. You already assigned to this handler in the ->probe(), what's this then? ... > > > + if (value <= 6) > > > + writel(value, sfp->padctl + IO_PADSHARE_SEL); > > > + else > > > > > + dev_err(dev, "invalid signal group %u\n", value); > > > > Why _err if you not bail out here? > > My thinking was that if the device tree specifies an invalid signal > group we should just leave the setting alone and not break booting, > but still be loud about it. Maybe that's too lenient and it's better > to crash and burn immediately if someone does that. Here is inconsistency between level of the message and following action. There are (rare!) cases when it's justified, but I believe it's not the case here. You have two choices or justify why you have to use error level without stopping process. ... All uncommented stuff you agreed on, correct?
On Tue, 12 Oct 2021 at 19:03, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Oct 12, 2021 at 4:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > Add a combined pinctrl and gpio driver for the StarFive JH7100 SoC. > > > > For each "GPIO" there are two registers for configuring the output and > > output enable signals which may come from other peripherals. Among these > > are two special signals that are constant 0 and constant 1 respectively. > > Controlling the GPIOs from software is done by choosing one of these > > signals. In other words the same registers are used for both pinmuxing > > and controlling the GPIOs, which makes it easier to combine the pinctrl > > and gpio driver in one. > > > > I wrote the pinconf and pinmux parts, but the gpio part of the code is > > based on the gpio driver in the vendor tree written by Huan Feng with > > cleanups and fixes by Drew and me. > > s/gpio/GPIO/g > > ... > > > +config PINCTRL_STARFIVE > > > + bool "Pinctrl and GPIO driver for the StarFive JH7100 SoC" > > Why not module? > > > + depends on SOC_STARFIVE || COMPILE_TEST > > > + depends on OF > > Do you really need this taking into account... > > > + default SOC_STARFIVE > > + select GENERIC_PINCTRL_GROUPS > > + select GENERIC_PINMUX_FUNCTIONS > > + select GENERIC_PINCONF > > + select GPIOLIB > > + select GPIOLIB_IRQCHIP > > > + select OF_GPIO > > ...this one? > > ... > > bits.h ? > > > +#include <linux/clk.h> > > +#include <linux/gpio/driver.h> > > +#include <linux/io.h> > > +#include <linux/module.h> > > > mod_devicetable.h ? > > > +#include <linux/pinctrl/pinctrl.h> > > +#include <linux/pinctrl/pinmux.h> > > Can you move these as a group after generic linux/* ones? > > > +#include <linux/platform_device.h> > > +#include <linux/reset.h> > > +#include <linux/spinlock.h> > > ... > > > +/* > > + * refer to Section 12. GPIO Registers in JH7100 datasheet: > > Be consistent in your style, here for example missed capitalization. > > > + * https://github.com/starfive-tech/StarLight_Docs > > Is it possible to have the datasheet to be provided as Datasheet: tag > in the commit message? > > > + */ > > ... > > > +/* > > + * Global enable for GPIO interrupts, offset: 0x0, field: GPIOEN > > + * set to 1 if GPIO interrupts are enabled, set to 0 to disable > > + */ > > +#define IRQ_GLOBAL_EN 0x0 > > s/0x0/0x00/g > > ... > > > +/* > > + * Interrupt Type for GPIO[31:0], offset: 0x10, field: GPIOS_0 > > + * set to 1 if edge-triggered, set to 0 for level-triggered > > + */ > > +#define IRQ_TYPE_LOW 0x10 > > + > > +/* > > + * Interrupt Type for GPIO[63:32], offset: 0x14, field: GPIOS_1 > > + */ > > +#define IRQ_TYPE_HIGH 0x14 > > As I reviewed below, the IRQ is represented by a few registers in a > row, no need to define low and high separately. Ditto for the rest > register pairs. > > ... > > > +/* > > + * Interrupt Status after Masking GPIO[31:0], offset: 0x40, field: GPIOMIS_0 > > + * status of edge-triggered or level-triggered after masking > > + * value of 1 means edge or level was detected, value of 0 menas not detected > > menas?! > > > + */ > > ... > > > +/* > > + * Data Value of GPIO for GPIO[31:0], offest: 0x48, field: GPIODIN_0 > > offest?! > > > + * dynamically reflects value on the GPIO pin > > + */ > > Please, run a spellchecker. > > ... > > > +#define IO_PADSHARE_SEL 0x1a0 > > Okay, make all registers to be fixed width, i.e. 0x000 for IRQ global > enabling and so on. > > ... > > > +#define PAD_SLEW_RATE_MASK 0xe00U > > GENMASK() > > > +#define PAD_BIAS_STRONG_PULL_UP 0x100U > > +#define PAD_INPUT_ENABLE 0x080U > > +#define PAD_INPUT_SCHMITT_ENABLE 0x040U > > +#define PAD_BIAS_DISABLE 0x020U > > +#define PAD_BIAS_PULL_DOWN 0x010U > > All above seems like BIT(_something_). > > > +#define PAD_BIAS_MASK 0x130U > > +#define PAD_DRIVE_STRENGTH_MASK 0x007U > > GENMASK() > > ... > > > +#ifdef CONFIG_DEBUG_FS > > __maybe_unused ? > > > +#else > > +#define starfive_pin_dbg_show NULL > > +#endif > > ... > > > + dout = readl_relaxed(reg); > > readl_relaxed(reg + 0x00) > > > + reg += 4; > > > + doen = readl_relaxed(reg); > > readl_relaxed(reg + 0x04); > > ... > > > + seq_printf(s, "dout=%u%s doen=%u%s", > > + dout & 0xffU, (dout & 0x80000000U) ? "r" : "", > > + doen & 0xffU, (doen & 0x80000000U) ? "r" : ""); > > GENMASK() > BIT() > > ... > > > + for_each_child_of_node(np, child) { > > + const __be32 *pinmux_list; > > + const __be32 *pins_list; > > + int pinmux_size; > > + int pins_size; > > + > > + pinmux_list = of_get_property(child, "pinmux", &pinmux_size); > > + pins_list = of_get_property(child, "pins", &pins_size); > > + if (pinmux_list && pins_list) { > > + dev_err(dev, "invalid pinctrl group %pOFn.%pOFn: %s\n", > > + np, child, "both pinmux and pins set"); > > + of_node_put(child); > > + return -EINVAL; > > + } > > + > > + if (pinmux_list && pinmux_size > 0) { > > + nmaps += 2; > > + } else if (pins_list && pins_size > 0) { > > + nmaps += 1; > > + } else { > > + dev_err(dev, "invalid pinctrl group %pOFn.%pOFn: %s\n", > > + np, child, "neither pinmux nor pins set"); > > + of_node_put(child); > > + return -EINVAL; > > + } > > + ngroups += 1; > > + } > > This entire loop seems like > 1) it should be based on something from pin control core; > 2) it's using some low level APIs instead of better ones like > of_property_read_uXX(); > 3) smells like unoptimized NIH. > > ... > > > + if ((list = of_get_property(child, "pinmux", &npins))) { > > Why not of_property_read_...() ? > > ... > > > + u32 v = be32_to_cpu(*list++); > > My gosh! > > ... > > > + for (i = 0; i < npins; i++) > > + pins[i] = be32_to_cpu(*list++); > > Ditto. > Even for this we have something in byteorder headers. > > Summary, make sure you use much better _existing_ APIs instead of the > above crap. > > ... > > > +free_pinmux: > > + devm_kfree(dev, pinmux); > > +free_pins: > > + devm_kfree(dev, pins); > > +free_grpname: > > + devm_kfree(dev, grpname); > > What the heck?! > > > +free_pgnames: > > + devm_kfree(dev, pgnames); > > Ditto. > > ... > > > +out: > > Useless label. > > > + return ret; > > ... > > > + for (i = 0; i < group->num_pins; i++) { > > + unsigned int gpio = starfive_pin_to_gpio(sfp, group->pins[i]); > > + void __iomem *reg_dout; > > + void __iomem *reg_doen; > > + void __iomem *reg_din; > > + u32 v, dout, doen, din; > > + unsigned long flags; > > > + if (dev_WARN_ONCE(dev, gpio >= MAX_GPIO, > > What?! > > > + "%s: invalid gpiomux pin", group->name)) > > + continue; > > + > > + v = pinmux[i]; > > + dout = ((v & BIT(7)) << (31 - 7)) | ((v >> 24) & 0xffU); > > + doen = ((v & BIT(6)) << (31 - 6)) | ((v >> 16) & 0xffU); > > + din = (v >> 8) & 0xffU; > > What is this voodoo for? > > > + if (din != 0xff) > > + reg_din = sfp->base + GPIO_IN_OFFSET + 4 * din; > > + else > > + reg_din = NULL; > > This looks like you maybe use gpio-regmap instead? > > ... > > > + void __iomem *reg = sfp->padctl + 4 * (pin / 2); > > + u32 value = readl_relaxed(reg); > > + > > + if (pin & 1U) > > + value >>= 16; > > + return value; > > u8 shift = 16 * (pin % 2); > > return readl_relaxed() >> shift; > > ? > > Something similar for below code. > > ... > > > +#ifdef CONFIG_DEBUG_FS > > +static const struct pin_config_item > > +starfive_pinconf_custom_conf_items[ARRAY_SIZE(starfive_pinconf_custom_params)] = { > > Instead of using ARAY_SIZE() here, use static_assert(). > > __maybe_unused? > > > + PCONFDUMP(PIN_CONFIG_STARFIVE_STRONG_PULL_UP, "input bias strong pull-up", NULL, false), > > +}; > > +#else > > +#define starfive_pinconf_custom_conf_items NULL > > +#endif > > ... > > > +static const unsigned char starfive_drive_strength[] = { > > + 14, 21, 28, 35, 42, 49, 56, 63, > > Why table? Can you simply use the formula?! > > > +}; > > ... > > > + if (unlikely(!group)) > > Why unlikely() Must be justified here and everywhere where you are using it. > > > + return -EINVAL; > > + > > + return starfive_pinconf_get(pctldev, group->pins[0], config); > > +} > > ... > > > + case PIN_CONFIG_BIAS_DISABLE: > > > + mask |= PAD_BIAS_MASK; > > Use it... > > > + value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE; > > ...here. Ditto for the similar cases in this function and elsewhere. I don't follow. How do you want me to use mask? If I did value = (value & ~mask) | PAD_BIAS_DISABLE; then I'd wipe the previous configuration. Eg. suppose the first config is the drive strength and second disables bias. Then on the 2nd loop mask = PAD_DRIVE_STRENGTH_MASK | PAD_BIAS_MASK and the drive strength value would be wiped. > After done this, you will see how you can simplify and deduplicate the > switch-cases. > > ... > > > +#ifdef CONFIG_DEBUG_FS > > __maybe_unused ? > > > +#else > > +#define starfive_pinconf_dbg_show NULL > > +#endif > > ... > > > + if (gpio < 32) { > > + value = readl_relaxed(sfp->base + GPIO_DIN_LOW); > > > + value = (value >> gpio) & 1U; > > Drop > > > + } else { > > + value = readl_relaxed(sfp->base + GPIO_DIN_HIGH); > > > + value = (value >> (gpio - 32)) & 1U; > > Drop > > > + } > > > + return value; > > return !!(value & BIT(gpio % 32)); > > ... > > > + if (arg == 0) > > > + return -ENOTSUPP; > > Shouldn't we return something else and pin control core will change it > to something else if needed? > > > + if (arg == 0) > > + return -ENOTSUPP; > > Ditto. > > > + default: > > + return -ENOTSUPP; > > ... > > > + if (gpio < 0 || gpio >= MAX_GPIO) > > + return; > > + > > + if (gpio < 32) { > > + ie = sfp->base + IRQ_ENABLE_LOW; > > + mask = BIT(gpio); > > + } else { > > + ie = sfp->base + IRQ_ENABLE_HIGH; > > + mask = BIT(gpio - 32); > > + } > > See below. And update all occurrences of these lines accordingly and > everywhere. Also for IRQ may use helper functions if needed (but I > don't believe the high and low register have stride more than 4). > > ... > > > + if (gpio < 0 || gpio >= MAX_GPIO) > > + return -EINVAL; > > How is it possible to be ever triggered? > > ... > > > + if (gpio < 32) { > > + base = sfp->base; > > + mask = BIT(gpio); > > + } else { > > + base = sfp->base + 4; > > + mask = BIT(gpio - 32); > > + } > > base = sfp_base + 4 * (gpio / 32); > mask = BIT(gpio % 32); > > ... > > > + irq_set_handler_locked(d, handle_edge_irq); > > > + irq_set_handler_locked(d, handle_edge_irq); > > Dup. > > ... > > > + irq_set_handler_locked(d, handle_edge_irq); > > > + irq_set_handler_locked(d, handle_level_irq); > > > + irq_set_handler_locked(d, handle_level_irq); > > Ditto. > > ... > > > + irq_set_handler_locked(d, handle_bad_irq); > > Why is this here? Move it to ->probe(). > > ... > > > + clk = devm_clk_get(dev, NULL); > > + if (IS_ERR(clk)) { > > + ret = PTR_ERR(clk); > > > + dev_err(dev, "could not get clock: %d\n", ret); > > Thank you for spamming logs with this noise. > > > + return ret; > > Hint: return dev_err_probe(). Ditto for the rest in this function. > > > + } > > ... > > > + ret = clk_prepare_enable(clk); > > + if (ret) { > > > + reset_control_deassert(rst); > > Use devm_add_action_or_reset(). I don't see how that is better. Then I'd first need to call that and check for errors, but just on the line below enabling the clock the reset line is deasserted anyway, so then the action isn't needed any longer. So that 3 lines of code for devm_add_action_or_reset + lingering unneeded action or code to remove it again vs. just the line above. > > + dev_err(dev, "could not enable clock: %d\n", ret); > > + return ret; > > + } > > ... > > > + if (!of_property_read_u32(dev->of_node, "starfive,signal-group", &value)) { > > Can be refactored without conditional. Also, why not to use > device_property_read_u32()? > > > + if (value <= 6) > > + writel(value, sfp->padctl + IO_PADSHARE_SEL); > > + else > > > + dev_err(dev, "invalid signal group %u\n", value); > > Why _err if you not bail out here? > > > + } > > ... > > > + value = readl(sfp->padctl + IO_PADSHARE_SEL); > > + switch (value) { > > + case 0: > > + sfp->gpios.pin_base = 0x10000; > > Magic number! > > > + goto done; > > + case 1: > > + sfp->gpios.pin_base = PAD_GPIO(0); > > + break; > > + case 2: > > + sfp->gpios.pin_base = PAD_FUNC_SHARE(72); > > + break; > > + case 3: > > + sfp->gpios.pin_base = PAD_FUNC_SHARE(70); > > + break; > > + case 4: case 5: case 6: > > + sfp->gpios.pin_base = PAD_FUNC_SHARE(0); > > + break; > > + default: > > + dev_err(dev, "invalid signal group %u\n", value); > > + return -EINVAL; > > + } > > ... > > > + sfp->gc.of_node = dev->of_node; > > Isn't GPIO library do this for you? If it does I can't find it. > > ... > > > + starfive_irq_chip.parent_device = dev; > > Ditto? > > ... > > > + sfp->gc.irq.parents = > > + devm_kcalloc(dev, 1, sizeof(*sfp->gc.irq.parents), GFP_KERNEL); > > 1 -> sfp->gc.irq.num_parents > And hence move below line up. > > > + if (!sfp->gc.irq.parents) > > + return -ENOMEM; > > > + sfp->gc.irq.num_parents = 1; > > ... > > > + dev_info(dev, "StarFive GPIO chip registered %d GPIOs\n", sfp->gc.ngpio); > > Redundant noise. > > ... > > > +static const struct of_device_id starfive_of_match[] = { > > + { .compatible = "starfive,jh7100-pinctrl" }, > > > + { /* sentinel */ }, > > No comma needed for terminator entry. > > > +}; > > -- > With Best Regards, > Andy Shevchenko
On Mon, Oct 18, 2021 at 6:35 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > On Tue, 12 Oct 2021 at 19:03, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Tue, Oct 12, 2021 at 4:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: When answering, cut down your message to the point, please! It's a bit annoying to remove overquoting... ... > > > + case PIN_CONFIG_BIAS_DISABLE: > > > > > + mask |= PAD_BIAS_MASK; > > > > Use it... > > > > > + value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE; > > > > ...here. Ditto for the similar cases in this function and elsewhere. > > I don't follow. How do you want me to use mask? If I did value = > (value & ~mask) | PAD_BIAS_DISABLE; then I'd wipe the previous > configuration. Eg. suppose the first config is the drive strength and > second disables bias. Then on the 2nd loop mask = > PAD_DRIVE_STRENGTH_MASK | PAD_BIAS_MASK and the drive strength value > would be wiped. Collect masks and new values in temporary variables and apply them once after the loop is done, no? ... > > > + ret = clk_prepare_enable(clk); > > > + if (ret) { > > > > > + reset_control_deassert(rst); > > > > Use devm_add_action_or_reset(). > > I don't see how that is better. Pity. The rule of thumb is to either try to use devm_*() everywhere in the probe, or don't use it at all. Above is the more-or-less standard pattern where devn_add_action_or_reset() is being used in the entire kernel. > Then I'd first need to call that and > check for errors, but just on the line below enabling the clock the > reset line is deasserted anyway, so then the action isn't needed any > longer. So that 3 lines of code for devm_add_action_or_reset + > lingering unneeded action or code to remove it again vs. just the line > above. Then don't use devm_*() at all. What's the point? ... > > > + sfp->gc.of_node = dev->of_node; > > > > Isn't GPIO library do this for you? > > If it does I can't find it. Heh... `man git grep` Hint: `git grep -n 'of_node = .*of_node' -- drivers/gpio/gpiolib*`
On Mon, 18 Oct 2021 at 17:48, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Oct 18, 2021 at 6:35 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > On Tue, 12 Oct 2021 at 19:03, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Tue, Oct 12, 2021 at 4:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > When answering, cut down your message to the point, please! It's a bit > annoying to remove overquoting... > > ... > > > > > + case PIN_CONFIG_BIAS_DISABLE: > > > > > > > + mask |= PAD_BIAS_MASK; > > > > > > Use it... > > > > > > > + value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE; > > > > > > ...here. Ditto for the similar cases in this function and elsewhere. > > > > I don't follow. How do you want me to use mask? If I did value = > > (value & ~mask) | PAD_BIAS_DISABLE; then I'd wipe the previous > > configuration. Eg. suppose the first config is the drive strength and > > second disables bias. Then on the 2nd loop mask = > > PAD_DRIVE_STRENGTH_MASK | PAD_BIAS_MASK and the drive strength value > > would be wiped. > > Collect masks and new values in temporary variables and apply them > once after the loop is done, no? But that's exactly what the code does. It merges all the config options into a single mask and value so we only need to do rmw on the register once. > ... > > > > > + ret = clk_prepare_enable(clk); > > > > + if (ret) { > > > > > > > + reset_control_deassert(rst); > > > > > > Use devm_add_action_or_reset(). > > > > I don't see how that is better. > > Pity. The rule of thumb is to either try to use devm_*() everywhere in > the probe, or don't use it at all. Above is the more-or-less standard > pattern where devn_add_action_or_reset() is being used in the entire > kernel. > > > Then I'd first need to call that and > > check for errors, but just on the line below enabling the clock the > > reset line is deasserted anyway, so then the action isn't needed any > > longer. So that 3 lines of code for devm_add_action_or_reset + > > lingering unneeded action or code to remove it again vs. just the line > > above. > > Then don't use devm_*() at all. What's the point? I'm confused. So you wan't an unneeded action to linger because the probe function temporarily asserts reset for 3 lines of code? > ... > > > > > + sfp->gc.of_node = dev->of_node; > > > > > > Isn't GPIO library do this for you? > > > > If it does I can't find it. > > Heh... `man git grep` > Hint: `git grep -n 'of_node = .*of_node' -- drivers/gpio/gpiolib*` That's exactly what I did. > -- > With Best Regards, > Andy Shevchenko
On Mon, Oct 18, 2021 at 6:56 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > On Mon, 18 Oct 2021 at 17:48, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Oct 18, 2021 at 6:35 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > > On Tue, 12 Oct 2021 at 19:03, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Tue, Oct 12, 2021 at 4:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: ... > > > > > + case PIN_CONFIG_BIAS_DISABLE: > > > > > > > > > + mask |= PAD_BIAS_MASK; > > > > > > > > Use it... > > > > > > > > > + value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE; > > > > > > > > ...here. Ditto for the similar cases in this function and elsewhere. > > > > > > I don't follow. How do you want me to use mask? If I did value = > > > (value & ~mask) | PAD_BIAS_DISABLE; then I'd wipe the previous > > > configuration. Eg. suppose the first config is the drive strength and > > > second disables bias. Then on the 2nd loop mask = > > > PAD_DRIVE_STRENGTH_MASK | PAD_BIAS_MASK and the drive strength value > > > would be wiped. > > > > Collect masks and new values in temporary variables and apply them > > once after the loop is done, no? > > But that's exactly what the code does. It merges all the config > options into a single mask and value so we only need to do rmw on the > register once. Then masking the value makes no sense. What you should have is simply as mask |= FOO; value |= BAR; ... > > > > > + ret = clk_prepare_enable(clk); > > > > > + if (ret) { > > > > > > > > > + reset_control_deassert(rst); > > > > > > > > Use devm_add_action_or_reset(). > > > > > > I don't see how that is better. > > > > Pity. The rule of thumb is to either try to use devm_*() everywhere in > > the probe, or don't use it at all. Above is the more-or-less standard > > pattern where devn_add_action_or_reset() is being used in the entire > > kernel. > > > > > Then I'd first need to call that and > > > check for errors, but just on the line below enabling the clock the > > > reset line is deasserted anyway, so then the action isn't needed any > > > longer. So that 3 lines of code for devm_add_action_or_reset + > > > lingering unneeded action or code to remove it again vs. just the line > > > above. > > > > Then don't use devm_*() at all. What's the point? > > I'm confused. So you wan't an unneeded action to linger because the > probe function temporarily asserts reset for 3 lines of code? I;m talking about clk_prepare_enable(). ... > > > > > + sfp->gc.of_node = dev->of_node; > > > > > > > > Isn't GPIO library do this for you? > > > > > > If it does I can't find it. > > > > Heh... `man git grep` > > Hint: `git grep -n 'of_node = .*of_node' -- drivers/gpio/gpiolib*` > > That's exactly what I did. Now look at the result and find the correct place where it's done. Btw, all hits are in the very same function.
On Mon, Oct 18, 2021 at 7:23 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Oct 18, 2021 at 6:56 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > On Mon, 18 Oct 2021 at 17:48, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Mon, Oct 18, 2021 at 6:35 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > > > On Tue, 12 Oct 2021 at 19:03, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > On Tue, Oct 12, 2021 at 4:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: ... > > > > > > + ret = clk_prepare_enable(clk); > > > > > > + if (ret) { > > > > > > > > > > > + reset_control_deassert(rst); > > > > > > > > > > Use devm_add_action_or_reset(). > > > > > > > > I don't see how that is better. > > > > > > Pity. The rule of thumb is to either try to use devm_*() everywhere in > > > the probe, or don't use it at all. Above is the more-or-less standard > > > pattern where devn_add_action_or_reset() is being used in the entire > > > kernel. > > > > > > > Then I'd first need to call that and > > > > check for errors, but just on the line below enabling the clock the > > > > reset line is deasserted anyway, so then the action isn't needed any > > > > longer. So that 3 lines of code for devm_add_action_or_reset + > > > > lingering unneeded action or code to remove it again vs. just the line > > > > above. > > > > > > Then don't use devm_*() at all. What's the point? > > > > I'm confused. So you wan't an unneeded action to linger because the > > probe function temporarily asserts reset for 3 lines of code? > > I;m talking about clk_prepare_enable(). Having a second look I found even problematic error paths because of mixing devm_*() with non-devm_*() calls, which only assures me that your ->probe() error path is broken and should be revisited.
On Mon, 18 Oct 2021 at 18:24, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Oct 18, 2021 at 6:56 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > On Mon, 18 Oct 2021 at 17:48, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Mon, Oct 18, 2021 at 6:35 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > > > On Tue, 12 Oct 2021 at 19:03, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > On Tue, Oct 12, 2021 at 4:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > ... > > > > > > > + case PIN_CONFIG_BIAS_DISABLE: > > > > > > > > > > > + mask |= PAD_BIAS_MASK; > > > > > > > > > > Use it... > > > > > > > > > > > + value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE; > > > > > > > > > > ...here. Ditto for the similar cases in this function and elsewhere. > > > > > > > > I don't follow. How do you want me to use mask? If I did value = > > > > (value & ~mask) | PAD_BIAS_DISABLE; then I'd wipe the previous > > > > configuration. Eg. suppose the first config is the drive strength and > > > > second disables bias. Then on the 2nd loop mask = > > > > PAD_DRIVE_STRENGTH_MASK | PAD_BIAS_MASK and the drive strength value > > > > would be wiped. > > > > > > Collect masks and new values in temporary variables and apply them > > > once after the loop is done, no? > > > > But that's exactly what the code does. It merges all the config > > options into a single mask and value so we only need to do rmw on the > > register once. > > Then masking the value makes no sense. > What you should have is simply as > > mask |= FOO; > value |= BAR; Yeah, but then we could get into weird states if the device tree specifies both bias-disable and bias-pull-up by mistake. This code is written so that only the last valid state is chosen. > ... > > > > > > > + ret = clk_prepare_enable(clk); > > > > > > + if (ret) { > > > > > > > > > > > + reset_control_deassert(rst); > > > > > > > > > > Use devm_add_action_or_reset(). > > > > > > > > I don't see how that is better. > > > > > > Pity. The rule of thumb is to either try to use devm_*() everywhere in > > > the probe, or don't use it at all. Above is the more-or-less standard > > > pattern where devn_add_action_or_reset() is being used in the entire > > > kernel. > > > > > > > Then I'd first need to call that and > > > > check for errors, but just on the line below enabling the clock the > > > > reset line is deasserted anyway, so then the action isn't needed any > > > > longer. So that 3 lines of code for devm_add_action_or_reset + > > > > lingering unneeded action or code to remove it again vs. just the line > > > > above. > > > > > > Then don't use devm_*() at all. What's the point? > > > > I'm confused. So you wan't an unneeded action to linger because the > > probe function temporarily asserts reset for 3 lines of code? > > I;m talking about clk_prepare_enable(). Ok, you wrote your comment under the reset_control_deassert call. How would devm_add_action_or_reset for clk_prepare_enable work? > ... > > > > > > > + sfp->gc.of_node = dev->of_node; > > > > > > > > > > Isn't GPIO library do this for you? > > > > > > > > If it does I can't find it. > > > > > > Heh... `man git grep` > > > Hint: `git grep -n 'of_node = .*of_node' -- drivers/gpio/gpiolib*` > > > > That's exactly what I did. > > Now look at the result and find the correct place where it's done. > Btw, all hits are in the very same function. > > -- > With Best Regards, > Andy Shevchenko
On Mon, 18 Oct 2021 at 18:29, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Oct 18, 2021 at 7:23 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Mon, Oct 18, 2021 at 6:56 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > > On Mon, 18 Oct 2021 at 17:48, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Mon, Oct 18, 2021 at 6:35 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > > > > On Tue, 12 Oct 2021 at 19:03, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > On Tue, Oct 12, 2021 at 4:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > ... > > > > > > > > + ret = clk_prepare_enable(clk); > > > > > > > + if (ret) { > > > > > > > > > > > > > + reset_control_deassert(rst); > > > > > > > > > > > > Use devm_add_action_or_reset(). > > > > > > > > > > I don't see how that is better. > > > > > > > > Pity. The rule of thumb is to either try to use devm_*() everywhere in > > > > the probe, or don't use it at all. Above is the more-or-less standard > > > > pattern where devn_add_action_or_reset() is being used in the entire > > > > kernel. > > > > > > > > > Then I'd first need to call that and > > > > > check for errors, but just on the line below enabling the clock the > > > > > reset line is deasserted anyway, so then the action isn't needed any > > > > > longer. So that 3 lines of code for devm_add_action_or_reset + > > > > > lingering unneeded action or code to remove it again vs. just the line > > > > > above. > > > > > > > > Then don't use devm_*() at all. What's the point? > > > > > > I'm confused. So you wan't an unneeded action to linger because the > > > probe function temporarily asserts reset for 3 lines of code? > > > > I;m talking about clk_prepare_enable(). > > Having a second look I found even problematic error paths because of > mixing devm_*() with non-devm_*() calls, which only assures me that > your ->probe() error path is broken and should be revisited. So do you want to expand on that now or should I send v2 first?
On Mon, Oct 18, 2021 at 06:35:10PM +0200, Emil Renner Berthing wrote: > On Mon, 18 Oct 2021 at 18:24, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Oct 18, 2021 at 6:56 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > > On Mon, 18 Oct 2021 at 17:48, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Mon, Oct 18, 2021 at 6:35 PM Emil Renner Berthing <kernel@esmil.dk> wrote: > > > > > On Tue, 12 Oct 2021 at 19:03, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > On Tue, Oct 12, 2021 at 4:43 PM Emil Renner Berthing <kernel@esmil.dk> wrote: ... > > > > > > > + case PIN_CONFIG_BIAS_DISABLE: > > > > > > > > > > > > > + mask |= PAD_BIAS_MASK; > > > > > > > > > > > > Use it... > > > > > > > > > > > > > + value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE; > > > > > > > > > > > > ...here. Ditto for the similar cases in this function and elsewhere. > > > > > > > > > > I don't follow. How do you want me to use mask? If I did value = > > > > > (value & ~mask) | PAD_BIAS_DISABLE; then I'd wipe the previous > > > > > configuration. Eg. suppose the first config is the drive strength and > > > > > second disables bias. Then on the 2nd loop mask = > > > > > PAD_DRIVE_STRENGTH_MASK | PAD_BIAS_MASK and the drive strength value > > > > > would be wiped. > > > > > > > > Collect masks and new values in temporary variables and apply them > > > > once after the loop is done, no? > > > > > > But that's exactly what the code does. It merges all the config > > > options into a single mask and value so we only need to do rmw on the > > > register once. > > > > Then masking the value makes no sense. > > What you should have is simply as > > > > mask |= FOO; > > value |= BAR; > > Yeah, but then we could get into weird states if the device tree > specifies both bias-disable and bias-pull-up by mistake. This code is > written so that only the last valid state is chosen. But shouldn't it be disallowed by: 1) DTC validator (Rob?) 2) GPIO / pin control (Linus, Bart?) ? ... > > > > > > > + ret = clk_prepare_enable(clk); > > > > > > > + if (ret) { > > > > > > > > > > > > > + reset_control_deassert(rst); > > > > > > > > > > > > Use devm_add_action_or_reset(). > > > > > > > > > > I don't see how that is better. > > > > > > > > Pity. The rule of thumb is to either try to use devm_*() everywhere in > > > > the probe, or don't use it at all. Above is the more-or-less standard > > > > pattern where devn_add_action_or_reset() is being used in the entire > > > > kernel. > > > > > > > > > Then I'd first need to call that and > > > > > check for errors, but just on the line below enabling the clock the > > > > > reset line is deasserted anyway, so then the action isn't needed any > > > > > longer. So that 3 lines of code for devm_add_action_or_reset + > > > > > lingering unneeded action or code to remove it again vs. just the line > > > > > above. > > > > > > > > Then don't use devm_*() at all. What's the point? > > > > > > I'm confused. So you wan't an unneeded action to linger because the > > > probe function temporarily asserts reset for 3 lines of code? > > > > I;m talking about clk_prepare_enable(). > > Ok, you wrote your comment under the reset_control_deassert call. How > would devm_add_action_or_reset for clk_prepare_enable work? It seems both are needed to be converted, otherwise _everything_ after reset_assert() should not be devm_*(). TL;DR: the rule is Allowed: devm_*() followed by non-devm_*() NOT allowed: devm_*() followed by non-devm_*() followed by devm_*() Of course, you may try to work the latter one, but it diminishes the whole idea behind it, that's why I told that may be not using devm_*() is the correct approach here and that what you meant (?). The example how to use above mentioned API, just grep for it. # See [1] for the sources of the used script $ gl4func.sh devm_add_action_or_reset clk_prepare_enable | wc -l 101 [1]: https://github.com/andy-shev/home-bin-tools/blob/master/gl4func.sh
On Mon, Oct 18, 2021 at 07:02:43PM +0200, Emil Renner Berthing wrote: > On Mon, 18 Oct 2021 at 18:29, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Oct 18, 2021 at 7:23 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: ... > > Having a second look I found even problematic error paths because of > > mixing devm_*() with non-devm_*() calls, which only assures me that > > your ->probe() error path is broken and should be revisited. > > So do you want to expand on that now or should I send v2 first? Here is not enough context anymore to point out. I expect one to have done their homework anyway.
diff --git a/MAINTAINERS b/MAINTAINERS index f7883377895e..4a34a8a9c987 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17854,6 +17854,14 @@ F: Documentation/devicetree/bindings/clock/starfive,jh7100-clkgen.yaml F: drivers/clk/starfive/clk-starfive-jh7100.c F: include/dt-bindings/clock/starfive-jh7100.h +STARFIVE JH7100 PINCTRL DRIVER +M: Emil Renner Berthing <kernel@esmil.dk> +L: linux-gpio@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/pinctrl/starfive,jh7100-pinctrl.yaml +F: drivers/pinctrl/pinctrl-starfive.c +F: include/dt-bindings/pinctrl/pinctrl-starfive.h + STARFIVE JH7100 RESET CONTROLLER DRIVER M: Emil Renner Berthing <kernel@esmil.dk> S: Maintained diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index 31921108e456..2f3d37b075c5 100644 --- a/drivers/pinctrl/Kconfig +++ b/drivers/pinctrl/Kconfig @@ -265,6 +265,23 @@ config PINCTRL_ST select PINCONF select GPIOLIB_IRQCHIP +config PINCTRL_STARFIVE + bool "Pinctrl and GPIO driver for the StarFive JH7100 SoC" + depends on SOC_STARFIVE || COMPILE_TEST + depends on OF + default SOC_STARFIVE + select GENERIC_PINCTRL_GROUPS + select GENERIC_PINMUX_FUNCTIONS + select GENERIC_PINCONF + select GPIOLIB + select GPIOLIB_IRQCHIP + select OF_GPIO + help + Say yes here to support pin control on the StarFive JH7100 SoC. + This also provides an interface to the GPIO pins not used by other + peripherals supporting inputs, outputs, configuring pull-up/pull-down + and interrupts on input changes. + config PINCTRL_STMFX tristate "STMicroelectronics STMFX GPIO expander pinctrl driver" depends on I2C diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index 200073bcc2c1..9c258047f11c 100644 --- a/drivers/pinctrl/Makefile +++ b/drivers/pinctrl/Makefile @@ -38,6 +38,7 @@ obj-$(CONFIG_PINCTRL_LANTIQ) += pinctrl-lantiq.o obj-$(CONFIG_PINCTRL_LPC18XX) += pinctrl-lpc18xx.o obj-$(CONFIG_PINCTRL_TB10X) += pinctrl-tb10x.o obj-$(CONFIG_PINCTRL_ST) += pinctrl-st.o +obj-$(CONFIG_PINCTRL_STARFIVE) += pinctrl-starfive.o obj-$(CONFIG_PINCTRL_STMFX) += pinctrl-stmfx.o obj-$(CONFIG_PINCTRL_ZYNQ) += pinctrl-zynq.o obj-$(CONFIG_PINCTRL_ZYNQMP) += pinctrl-zynqmp.o diff --git a/drivers/pinctrl/pinctrl-starfive.c b/drivers/pinctrl/pinctrl-starfive.c new file mode 100644 index 000000000000..e3b5a2f53fe1 --- /dev/null +++ b/drivers/pinctrl/pinctrl-starfive.c @@ -0,0 +1,1439 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Pinctrl / GPIO driver for StarFive JH7100 SoC + * + * Copyright (C) 2020 Shanghai StarFive Technology Co., Ltd. + * Copyright (C) 2021 Emil Renner Berthing <kernel@esmil.dk> + */ + +#include <linux/clk.h> +#include <linux/gpio/driver.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/pinctrl/pinctrl.h> +#include <linux/pinctrl/pinmux.h> +#include <linux/platform_device.h> +#include <linux/reset.h> +#include <linux/spinlock.h> + +#include <dt-bindings/pinctrl/pinctrl-starfive.h> + +#include "core.h" +#include "pinctrl-utils.h" +#include "pinmux.h" +#include "pinconf.h" + +#define DRIVER_NAME "pinctrl-starfive" + +/* + * refer to Section 12. GPIO Registers in JH7100 datasheet: + * https://github.com/starfive-tech/StarLight_Docs + */ +#define MAX_GPIO 64 + +/* + * Global enable for GPIO interrupts, offset: 0x0, field: GPIOEN + * set to 1 if GPIO interrupts are enabled, set to 0 to disable + */ +#define IRQ_GLOBAL_EN 0x0 + +/* + * Interrupt Type for GPIO[31:0], offset: 0x10, field: GPIOS_0 + * set to 1 if edge-triggered, set to 0 for level-triggered + */ +#define IRQ_TYPE_LOW 0x10 + +/* + * Interrupt Type for GPIO[63:32], offset: 0x14, field: GPIOS_1 + */ +#define IRQ_TYPE_HIGH 0x14 + +/* + * Edge-Triggered Interrupt Type for GPIO[31:0], offset: 0x18, field: GPIOIBE_0 + * set to 1 if both positive and negative edge, set to 0 if single edge + */ +#define IRQ_EDGE_BOTH_LOW 0x18 + +/* + * Edge-Triggered Interrupt Type for GPIO[63:32], offset: 0x1c, field: GPIOIBE_1 + */ +#define IRQ_EDGE_BOTH_HIGH 0x1c + +/* + * Interrupt Trigger Polarity for GPIO[31:0], offset: 0x20, field: GPIOEV_0 + * for edge-triggered on single edge, set to 1 for rising edge, 0 for falling edge + * for edge-triggered on both edges, this field is ignored + * for level-triggered, set to 1 for high level, 0 for low level + */ +#define IRQ_POLARITY_LOW 0x20 + +/* + * Interrupt Trigger Polarity for GPIO[63:32], offset: 0x24, field: GPIOEV_1 + */ +#define IRQ_POLARITY_HIGH 0x24 + +/* + * Interrupt Enable for GPIO[31:0], offset: 0x28, field: GPIOIE_0 + * set to 1 to enable (unmask) the interrupt, set to 0 to disable (mask) + */ +#define IRQ_ENABLE_LOW 0x28 + +/* + * Interrupt Mask for GPIO[63:32], offset: 0x2c, field: GPIOIE_1 + */ +#define IRQ_ENABLE_HIGH 0x2c + +/* + * Clear Edge-Triggered Interrupts GPIO[31:0], offset: 0x30, field: GPIOC_0 + * set to 1 to clear edge-triggered interrupt + */ +#define IRQ_CLEAR_EDGE_LOW 0x30 + +/* + * Clear Edge-Triggered Interrupts GPIO[63:32], offset: 0x34, field: GPIOC_1 + */ +#define IRQ_CLEAR_EDGE_HIGH 0x34 + +/* + * Edge-Triggered Interrupt Status GPIO[31:0], offset: 0x38, field: GPIORIS_0 + * value of 1 means edge detected, value of 0 means no edge detected + */ +#define IRQ_EDGE_STATUS_LOW 0x38 + +/* + * Edge-Triggered Interrupt Status GPIO[63:32], offset: 0x3C, field: GPIORIS_1 + */ +#define IRQ_EDGE_STATUS_HIGH 0x3c + +/* + * Interrupt Status after Masking GPIO[31:0], offset: 0x40, field: GPIOMIS_0 + * status of edge-triggered or level-triggered after masking + * value of 1 means edge or level was detected, value of 0 menas not detected + */ +#define IRQ_MASKED_STATUS_LOW 0x40 + +/* + * Interrupt Status after Masking GPIO[63:32], offset: 0x44, field: GPIOMIS_1 + */ +#define IRQ_MASKED_STATUS_HIGH 0x44 + +/* + * Data Value of GPIO for GPIO[31:0], offest: 0x48, field: GPIODIN_0 + * dynamically reflects value on the GPIO pin + */ +#define GPIO_DIN_LOW 0x48 + +/* + * Data Value of GPIO for GPIO[63:32], offest: 0x4C, field: GPIODIN_1 + */ +#define GPIO_DIN_HIGH 0x4c + +/* + * From datasheet section 12.2, there are 64 output data config registers which + * are 4 bytes wide. There are 64 output enable config registers which are 4 + * bytes wide too. Output data and output enable registers for a given GPIO pad + * are contiguous. Thus GPIO0_DOUT_CFG is 0x50 and GPIO0_DOEN_CFG is 0x54 while + * GPIO1_DOUT_CFG is 0x58 and GPIO1_DOEN_CFG is 0x5C. The stride between GPIO + * GPIO pads is effectively 8, thus: GPIOn_DOUT_CFG is 0x50+8n + */ +#define GPIO_N_DOUT_CFG 0x50 + +/* + * GPIO0_DOEN_CFG is 0x54, GPIOn_DOEN_CFG is 0x54+8n + */ +#define GPIO_N_DOEN_CFG 0x54 + +/* + * From Section 12.3, there are 75 input signal configuration registers which + * are 4 bytes wide starting with GPI_CPU_JTAG_TCK_CFG at 0x250 and ending with + * GPI_USB_OVER_CURRENT_CFG 0x378 + */ +#define GPIO_IN_OFFSET 0x250 + +/* + * From Section 11, IO_PADSHARE_SEL register can be programmed to select one of + * pre-defined multiplexed signal groups on PAD_FUNC_SHARE and PAD_GPIO pads. + * This is a global setting. Per Table 11-1, setting IO_PADSHARE_SEL to 6 would + * result in GPIO[63:0] being mapped to PAD_FUNC_SHARE[63:0] + */ +#define IO_PADSHARE_SEL 0x1a0 + +#define PAD_SLEW_RATE_MASK 0xe00U +#define PAD_SLEW_RATE_POS 9 +#define PAD_BIAS_STRONG_PULL_UP 0x100U +#define PAD_INPUT_ENABLE 0x080U +#define PAD_INPUT_SCHMITT_ENABLE 0x040U +#define PAD_BIAS_DISABLE 0x020U +#define PAD_BIAS_PULL_DOWN 0x010U +#define PAD_BIAS_MASK 0x130U +#define PAD_DRIVE_STRENGTH_MASK 0x007U +#define PAD_DRIVE_STRENGTH_POS 0 + +struct starfive_pinctrl { + struct gpio_chip gc; + struct pinctrl_gpio_range gpios; + raw_spinlock_t lock; + void __iomem *base; + void __iomem *padctl; + struct pinctrl_dev *pctl; +}; + +static struct device *starfive_dev(const struct starfive_pinctrl *sfp) +{ + return sfp->gc.parent; +} + +static unsigned int starfive_pin_to_gpio(const struct starfive_pinctrl *sfp, + unsigned int pin) +{ + return pin - sfp->gpios.pin_base; +} + +static unsigned int starfive_gpio_to_pin(const struct starfive_pinctrl *sfp, + unsigned int gpio) +{ + return sfp->gpios.pin_base + gpio; +} + +static struct starfive_pinctrl *starfive_from_gc(struct gpio_chip *gc) +{ + return container_of(gc, struct starfive_pinctrl, gc); +} + +static struct starfive_pinctrl *starfive_from_irq_data(struct irq_data *d) +{ + return starfive_from_gc(irq_data_get_irq_chip_data(d)); +} + +static struct starfive_pinctrl *starfive_from_irq_desc(struct irq_desc *desc) +{ + return starfive_from_gc(irq_desc_get_handler_data(desc)); +} + +static const struct pinctrl_pin_desc starfive_pins[] = { + PINCTRL_PIN(PAD_GPIO(0), "GPIO[0]"), + PINCTRL_PIN(PAD_GPIO(1), "GPIO[1]"), + PINCTRL_PIN(PAD_GPIO(2), "GPIO[2]"), + PINCTRL_PIN(PAD_GPIO(3), "GPIO[3]"), + PINCTRL_PIN(PAD_GPIO(4), "GPIO[4]"), + PINCTRL_PIN(PAD_GPIO(5), "GPIO[5]"), + PINCTRL_PIN(PAD_GPIO(6), "GPIO[6]"), + PINCTRL_PIN(PAD_GPIO(7), "GPIO[7]"), + PINCTRL_PIN(PAD_GPIO(8), "GPIO[8]"), + PINCTRL_PIN(PAD_GPIO(9), "GPIO[9]"), + PINCTRL_PIN(PAD_GPIO(10), "GPIO[10]"), + PINCTRL_PIN(PAD_GPIO(11), "GPIO[11]"), + PINCTRL_PIN(PAD_GPIO(12), "GPIO[12]"), + PINCTRL_PIN(PAD_GPIO(13), "GPIO[13]"), + PINCTRL_PIN(PAD_GPIO(14), "GPIO[14]"), + PINCTRL_PIN(PAD_GPIO(15), "GPIO[15]"), + PINCTRL_PIN(PAD_GPIO(16), "GPIO[16]"), + PINCTRL_PIN(PAD_GPIO(17), "GPIO[17]"), + PINCTRL_PIN(PAD_GPIO(18), "GPIO[18]"), + PINCTRL_PIN(PAD_GPIO(19), "GPIO[19]"), + PINCTRL_PIN(PAD_GPIO(20), "GPIO[20]"), + PINCTRL_PIN(PAD_GPIO(21), "GPIO[21]"), + PINCTRL_PIN(PAD_GPIO(22), "GPIO[22]"), + PINCTRL_PIN(PAD_GPIO(23), "GPIO[23]"), + PINCTRL_PIN(PAD_GPIO(24), "GPIO[24]"), + PINCTRL_PIN(PAD_GPIO(25), "GPIO[25]"), + PINCTRL_PIN(PAD_GPIO(26), "GPIO[26]"), + PINCTRL_PIN(PAD_GPIO(27), "GPIO[27]"), + PINCTRL_PIN(PAD_GPIO(28), "GPIO[28]"), + PINCTRL_PIN(PAD_GPIO(29), "GPIO[29]"), + PINCTRL_PIN(PAD_GPIO(30), "GPIO[30]"), + PINCTRL_PIN(PAD_GPIO(31), "GPIO[31]"), + PINCTRL_PIN(PAD_GPIO(32), "GPIO[32]"), + PINCTRL_PIN(PAD_GPIO(33), "GPIO[33]"), + PINCTRL_PIN(PAD_GPIO(34), "GPIO[34]"), + PINCTRL_PIN(PAD_GPIO(35), "GPIO[35]"), + PINCTRL_PIN(PAD_GPIO(36), "GPIO[36]"), + PINCTRL_PIN(PAD_GPIO(37), "GPIO[37]"), + PINCTRL_PIN(PAD_GPIO(38), "GPIO[38]"), + PINCTRL_PIN(PAD_GPIO(39), "GPIO[39]"), + PINCTRL_PIN(PAD_GPIO(40), "GPIO[40]"), + PINCTRL_PIN(PAD_GPIO(41), "GPIO[41]"), + PINCTRL_PIN(PAD_GPIO(42), "GPIO[42]"), + PINCTRL_PIN(PAD_GPIO(43), "GPIO[43]"), + PINCTRL_PIN(PAD_GPIO(44), "GPIO[44]"), + PINCTRL_PIN(PAD_GPIO(45), "GPIO[45]"), + PINCTRL_PIN(PAD_GPIO(46), "GPIO[46]"), + PINCTRL_PIN(PAD_GPIO(47), "GPIO[47]"), + PINCTRL_PIN(PAD_GPIO(48), "GPIO[48]"), + PINCTRL_PIN(PAD_GPIO(49), "GPIO[49]"), + PINCTRL_PIN(PAD_GPIO(50), "GPIO[50]"), + PINCTRL_PIN(PAD_GPIO(51), "GPIO[51]"), + PINCTRL_PIN(PAD_GPIO(52), "GPIO[52]"), + PINCTRL_PIN(PAD_GPIO(53), "GPIO[53]"), + PINCTRL_PIN(PAD_GPIO(54), "GPIO[54]"), + PINCTRL_PIN(PAD_GPIO(55), "GPIO[55]"), + PINCTRL_PIN(PAD_GPIO(56), "GPIO[56]"), + PINCTRL_PIN(PAD_GPIO(57), "GPIO[57]"), + PINCTRL_PIN(PAD_GPIO(58), "GPIO[58]"), + PINCTRL_PIN(PAD_GPIO(59), "GPIO[59]"), + PINCTRL_PIN(PAD_GPIO(60), "GPIO[60]"), + PINCTRL_PIN(PAD_GPIO(61), "GPIO[61]"), + PINCTRL_PIN(PAD_GPIO(62), "GPIO[62]"), + PINCTRL_PIN(PAD_GPIO(63), "GPIO[63]"), + PINCTRL_PIN(PAD_FUNC_SHARE(0), "FUNC_SHARE[0]"), + PINCTRL_PIN(PAD_FUNC_SHARE(1), "FUNC_SHARE[1]"), + PINCTRL_PIN(PAD_FUNC_SHARE(2), "FUNC_SHARE[2]"), + PINCTRL_PIN(PAD_FUNC_SHARE(3), "FUNC_SHARE[3]"), + PINCTRL_PIN(PAD_FUNC_SHARE(4), "FUNC_SHARE[4]"), + PINCTRL_PIN(PAD_FUNC_SHARE(5), "FUNC_SHARE[5]"), + PINCTRL_PIN(PAD_FUNC_SHARE(6), "FUNC_SHARE[6]"), + PINCTRL_PIN(PAD_FUNC_SHARE(7), "FUNC_SHARE[7]"), + PINCTRL_PIN(PAD_FUNC_SHARE(8), "FUNC_SHARE[8]"), + PINCTRL_PIN(PAD_FUNC_SHARE(9), "FUNC_SHARE[9]"), + PINCTRL_PIN(PAD_FUNC_SHARE(10), "FUNC_SHARE[10]"), + PINCTRL_PIN(PAD_FUNC_SHARE(11), "FUNC_SHARE[11]"), + PINCTRL_PIN(PAD_FUNC_SHARE(12), "FUNC_SHARE[12]"), + PINCTRL_PIN(PAD_FUNC_SHARE(13), "FUNC_SHARE[13]"), + PINCTRL_PIN(PAD_FUNC_SHARE(14), "FUNC_SHARE[14]"), + PINCTRL_PIN(PAD_FUNC_SHARE(15), "FUNC_SHARE[15]"), + PINCTRL_PIN(PAD_FUNC_SHARE(16), "FUNC_SHARE[16]"), + PINCTRL_PIN(PAD_FUNC_SHARE(17), "FUNC_SHARE[17]"), + PINCTRL_PIN(PAD_FUNC_SHARE(18), "FUNC_SHARE[18]"), + PINCTRL_PIN(PAD_FUNC_SHARE(19), "FUNC_SHARE[19]"), + PINCTRL_PIN(PAD_FUNC_SHARE(20), "FUNC_SHARE[20]"), + PINCTRL_PIN(PAD_FUNC_SHARE(21), "FUNC_SHARE[21]"), + PINCTRL_PIN(PAD_FUNC_SHARE(22), "FUNC_SHARE[22]"), + PINCTRL_PIN(PAD_FUNC_SHARE(23), "FUNC_SHARE[23]"), + PINCTRL_PIN(PAD_FUNC_SHARE(24), "FUNC_SHARE[24]"), + PINCTRL_PIN(PAD_FUNC_SHARE(25), "FUNC_SHARE[25]"), + PINCTRL_PIN(PAD_FUNC_SHARE(26), "FUNC_SHARE[26]"), + PINCTRL_PIN(PAD_FUNC_SHARE(27), "FUNC_SHARE[27]"), + PINCTRL_PIN(PAD_FUNC_SHARE(28), "FUNC_SHARE[28]"), + PINCTRL_PIN(PAD_FUNC_SHARE(29), "FUNC_SHARE[29]"), + PINCTRL_PIN(PAD_FUNC_SHARE(30), "FUNC_SHARE[30]"), + PINCTRL_PIN(PAD_FUNC_SHARE(31), "FUNC_SHARE[31]"), + PINCTRL_PIN(PAD_FUNC_SHARE(32), "FUNC_SHARE[32]"), + PINCTRL_PIN(PAD_FUNC_SHARE(33), "FUNC_SHARE[33]"), + PINCTRL_PIN(PAD_FUNC_SHARE(34), "FUNC_SHARE[34]"), + PINCTRL_PIN(PAD_FUNC_SHARE(35), "FUNC_SHARE[35]"), + PINCTRL_PIN(PAD_FUNC_SHARE(36), "FUNC_SHARE[36]"), + PINCTRL_PIN(PAD_FUNC_SHARE(37), "FUNC_SHARE[37]"), + PINCTRL_PIN(PAD_FUNC_SHARE(38), "FUNC_SHARE[38]"), + PINCTRL_PIN(PAD_FUNC_SHARE(39), "FUNC_SHARE[39]"), + PINCTRL_PIN(PAD_FUNC_SHARE(40), "FUNC_SHARE[40]"), + PINCTRL_PIN(PAD_FUNC_SHARE(41), "FUNC_SHARE[41]"), + PINCTRL_PIN(PAD_FUNC_SHARE(42), "FUNC_SHARE[42]"), + PINCTRL_PIN(PAD_FUNC_SHARE(43), "FUNC_SHARE[43]"), + PINCTRL_PIN(PAD_FUNC_SHARE(44), "FUNC_SHARE[44]"), + PINCTRL_PIN(PAD_FUNC_SHARE(45), "FUNC_SHARE[45]"), + PINCTRL_PIN(PAD_FUNC_SHARE(46), "FUNC_SHARE[46]"), + PINCTRL_PIN(PAD_FUNC_SHARE(47), "FUNC_SHARE[47]"), + PINCTRL_PIN(PAD_FUNC_SHARE(48), "FUNC_SHARE[48]"), + PINCTRL_PIN(PAD_FUNC_SHARE(49), "FUNC_SHARE[49]"), + PINCTRL_PIN(PAD_FUNC_SHARE(50), "FUNC_SHARE[50]"), + PINCTRL_PIN(PAD_FUNC_SHARE(51), "FUNC_SHARE[51]"), + PINCTRL_PIN(PAD_FUNC_SHARE(52), "FUNC_SHARE[52]"), + PINCTRL_PIN(PAD_FUNC_SHARE(53), "FUNC_SHARE[53]"), + PINCTRL_PIN(PAD_FUNC_SHARE(54), "FUNC_SHARE[54]"), + PINCTRL_PIN(PAD_FUNC_SHARE(55), "FUNC_SHARE[55]"), + PINCTRL_PIN(PAD_FUNC_SHARE(56), "FUNC_SHARE[56]"), + PINCTRL_PIN(PAD_FUNC_SHARE(57), "FUNC_SHARE[57]"), + PINCTRL_PIN(PAD_FUNC_SHARE(58), "FUNC_SHARE[58]"), + PINCTRL_PIN(PAD_FUNC_SHARE(59), "FUNC_SHARE[59]"), + PINCTRL_PIN(PAD_FUNC_SHARE(60), "FUNC_SHARE[60]"), + PINCTRL_PIN(PAD_FUNC_SHARE(61), "FUNC_SHARE[61]"), + PINCTRL_PIN(PAD_FUNC_SHARE(62), "FUNC_SHARE[62]"), + PINCTRL_PIN(PAD_FUNC_SHARE(63), "FUNC_SHARE[63]"), + PINCTRL_PIN(PAD_FUNC_SHARE(64), "FUNC_SHARE[64]"), + PINCTRL_PIN(PAD_FUNC_SHARE(65), "FUNC_SHARE[65]"), + PINCTRL_PIN(PAD_FUNC_SHARE(66), "FUNC_SHARE[66]"), + PINCTRL_PIN(PAD_FUNC_SHARE(67), "FUNC_SHARE[67]"), + PINCTRL_PIN(PAD_FUNC_SHARE(68), "FUNC_SHARE[68]"), + PINCTRL_PIN(PAD_FUNC_SHARE(69), "FUNC_SHARE[69]"), + PINCTRL_PIN(PAD_FUNC_SHARE(70), "FUNC_SHARE[70]"), + PINCTRL_PIN(PAD_FUNC_SHARE(71), "FUNC_SHARE[71]"), + PINCTRL_PIN(PAD_FUNC_SHARE(72), "FUNC_SHARE[72]"), + PINCTRL_PIN(PAD_FUNC_SHARE(73), "FUNC_SHARE[73]"), + PINCTRL_PIN(PAD_FUNC_SHARE(74), "FUNC_SHARE[74]"), + PINCTRL_PIN(PAD_FUNC_SHARE(75), "FUNC_SHARE[75]"), + PINCTRL_PIN(PAD_FUNC_SHARE(76), "FUNC_SHARE[76]"), + PINCTRL_PIN(PAD_FUNC_SHARE(77), "FUNC_SHARE[77]"), + PINCTRL_PIN(PAD_FUNC_SHARE(78), "FUNC_SHARE[78]"), + PINCTRL_PIN(PAD_FUNC_SHARE(79), "FUNC_SHARE[79]"), + PINCTRL_PIN(PAD_FUNC_SHARE(80), "FUNC_SHARE[80]"), + PINCTRL_PIN(PAD_FUNC_SHARE(81), "FUNC_SHARE[81]"), + PINCTRL_PIN(PAD_FUNC_SHARE(82), "FUNC_SHARE[82]"), + PINCTRL_PIN(PAD_FUNC_SHARE(83), "FUNC_SHARE[83]"), + PINCTRL_PIN(PAD_FUNC_SHARE(84), "FUNC_SHARE[84]"), + PINCTRL_PIN(PAD_FUNC_SHARE(85), "FUNC_SHARE[85]"), + PINCTRL_PIN(PAD_FUNC_SHARE(86), "FUNC_SHARE[86]"), + PINCTRL_PIN(PAD_FUNC_SHARE(87), "FUNC_SHARE[87]"), + PINCTRL_PIN(PAD_FUNC_SHARE(88), "FUNC_SHARE[88]"), + PINCTRL_PIN(PAD_FUNC_SHARE(89), "FUNC_SHARE[89]"), + PINCTRL_PIN(PAD_FUNC_SHARE(90), "FUNC_SHARE[90]"), + PINCTRL_PIN(PAD_FUNC_SHARE(91), "FUNC_SHARE[91]"), + PINCTRL_PIN(PAD_FUNC_SHARE(92), "FUNC_SHARE[92]"), + PINCTRL_PIN(PAD_FUNC_SHARE(93), "FUNC_SHARE[93]"), + PINCTRL_PIN(PAD_FUNC_SHARE(94), "FUNC_SHARE[94]"), + PINCTRL_PIN(PAD_FUNC_SHARE(95), "FUNC_SHARE[95]"), + PINCTRL_PIN(PAD_FUNC_SHARE(96), "FUNC_SHARE[96]"), + PINCTRL_PIN(PAD_FUNC_SHARE(97), "FUNC_SHARE[97]"), + PINCTRL_PIN(PAD_FUNC_SHARE(98), "FUNC_SHARE[98]"), + PINCTRL_PIN(PAD_FUNC_SHARE(99), "FUNC_SHARE[99]"), + PINCTRL_PIN(PAD_FUNC_SHARE(100), "FUNC_SHARE[100]"), + PINCTRL_PIN(PAD_FUNC_SHARE(101), "FUNC_SHARE[101]"), + PINCTRL_PIN(PAD_FUNC_SHARE(102), "FUNC_SHARE[102]"), + PINCTRL_PIN(PAD_FUNC_SHARE(103), "FUNC_SHARE[103]"), + PINCTRL_PIN(PAD_FUNC_SHARE(104), "FUNC_SHARE[104]"), + PINCTRL_PIN(PAD_FUNC_SHARE(105), "FUNC_SHARE[105]"), + PINCTRL_PIN(PAD_FUNC_SHARE(106), "FUNC_SHARE[106]"), + PINCTRL_PIN(PAD_FUNC_SHARE(107), "FUNC_SHARE[107]"), + PINCTRL_PIN(PAD_FUNC_SHARE(108), "FUNC_SHARE[108]"), + PINCTRL_PIN(PAD_FUNC_SHARE(109), "FUNC_SHARE[109]"), + PINCTRL_PIN(PAD_FUNC_SHARE(110), "FUNC_SHARE[110]"), + PINCTRL_PIN(PAD_FUNC_SHARE(111), "FUNC_SHARE[111]"), + PINCTRL_PIN(PAD_FUNC_SHARE(112), "FUNC_SHARE[112]"), + PINCTRL_PIN(PAD_FUNC_SHARE(113), "FUNC_SHARE[113]"), + PINCTRL_PIN(PAD_FUNC_SHARE(114), "FUNC_SHARE[114]"), + PINCTRL_PIN(PAD_FUNC_SHARE(115), "FUNC_SHARE[115]"), + PINCTRL_PIN(PAD_FUNC_SHARE(116), "FUNC_SHARE[116]"), + PINCTRL_PIN(PAD_FUNC_SHARE(117), "FUNC_SHARE[117]"), + PINCTRL_PIN(PAD_FUNC_SHARE(118), "FUNC_SHARE[118]"), + PINCTRL_PIN(PAD_FUNC_SHARE(119), "FUNC_SHARE[119]"), + PINCTRL_PIN(PAD_FUNC_SHARE(120), "FUNC_SHARE[120]"), + PINCTRL_PIN(PAD_FUNC_SHARE(121), "FUNC_SHARE[121]"), + PINCTRL_PIN(PAD_FUNC_SHARE(122), "FUNC_SHARE[122]"), + PINCTRL_PIN(PAD_FUNC_SHARE(123), "FUNC_SHARE[123]"), + PINCTRL_PIN(PAD_FUNC_SHARE(124), "FUNC_SHARE[124]"), + PINCTRL_PIN(PAD_FUNC_SHARE(125), "FUNC_SHARE[125]"), + PINCTRL_PIN(PAD_FUNC_SHARE(126), "FUNC_SHARE[126]"), + PINCTRL_PIN(PAD_FUNC_SHARE(127), "FUNC_SHARE[127]"), + PINCTRL_PIN(PAD_FUNC_SHARE(128), "FUNC_SHARE[128]"), + PINCTRL_PIN(PAD_FUNC_SHARE(129), "FUNC_SHARE[129]"), + PINCTRL_PIN(PAD_FUNC_SHARE(130), "FUNC_SHARE[130]"), + PINCTRL_PIN(PAD_FUNC_SHARE(131), "FUNC_SHARE[131]"), + PINCTRL_PIN(PAD_FUNC_SHARE(132), "FUNC_SHARE[132]"), + PINCTRL_PIN(PAD_FUNC_SHARE(133), "FUNC_SHARE[133]"), + PINCTRL_PIN(PAD_FUNC_SHARE(134), "FUNC_SHARE[134]"), + PINCTRL_PIN(PAD_FUNC_SHARE(135), "FUNC_SHARE[135]"), + PINCTRL_PIN(PAD_FUNC_SHARE(136), "FUNC_SHARE[136]"), + PINCTRL_PIN(PAD_FUNC_SHARE(137), "FUNC_SHARE[137]"), + PINCTRL_PIN(PAD_FUNC_SHARE(138), "FUNC_SHARE[138]"), + PINCTRL_PIN(PAD_FUNC_SHARE(139), "FUNC_SHARE[139]"), + PINCTRL_PIN(PAD_FUNC_SHARE(140), "FUNC_SHARE[140]"), + PINCTRL_PIN(PAD_FUNC_SHARE(141), "FUNC_SHARE[141]"), +}; + +#ifdef CONFIG_DEBUG_FS +static void starfive_pin_dbg_show(struct pinctrl_dev *pctldev, + struct seq_file *s, + unsigned int pin) +{ + struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev); + unsigned int gpio = starfive_pin_to_gpio(sfp, pin); + void __iomem *reg; + u32 dout, doen; + + if (gpio >= MAX_GPIO) + return; + + reg = sfp->base + GPIO_N_DOUT_CFG + 8 * gpio; + dout = readl_relaxed(reg); + reg += 4; + doen = readl_relaxed(reg); + + seq_printf(s, "dout=%u%s doen=%u%s", + dout & 0xffU, (dout & 0x80000000U) ? "r" : "", + doen & 0xffU, (doen & 0x80000000U) ? "r" : ""); +} +#else +#define starfive_pin_dbg_show NULL +#endif + +static int starfive_dt_node_to_map(struct pinctrl_dev *pctldev, + struct device_node *np, + struct pinctrl_map **maps, + unsigned int *num_maps) +{ + struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev); + struct device *dev = starfive_dev(sfp); + const char **pgnames; + struct pinctrl_map *map; + struct device_node *child; + const char *grpname; + int *pins; + u32 *pinmux; + int nmaps; + int ngroups; + int ret; + + nmaps = 0; + ngroups = 0; + for_each_child_of_node(np, child) { + const __be32 *pinmux_list; + const __be32 *pins_list; + int pinmux_size; + int pins_size; + + pinmux_list = of_get_property(child, "pinmux", &pinmux_size); + pins_list = of_get_property(child, "pins", &pins_size); + if (pinmux_list && pins_list) { + dev_err(dev, "invalid pinctrl group %pOFn.%pOFn: %s\n", + np, child, "both pinmux and pins set"); + of_node_put(child); + return -EINVAL; + } + + if (pinmux_list && pinmux_size > 0) { + nmaps += 2; + } else if (pins_list && pins_size > 0) { + nmaps += 1; + } else { + dev_err(dev, "invalid pinctrl group %pOFn.%pOFn: %s\n", + np, child, "neither pinmux nor pins set"); + of_node_put(child); + return -EINVAL; + } + ngroups += 1; + } + + ret = -ENOMEM; + pgnames = devm_kcalloc(dev, ngroups, sizeof(*pgnames), GFP_KERNEL); + if (!pgnames) + goto out; + + map = kcalloc(nmaps, sizeof(*map), GFP_KERNEL); + if (!map) + goto free_pgnames; + + nmaps = 0; + ngroups = 0; + for_each_child_of_node(np, child) { + const __be32 *list; + int npins; + int i; + + ret = -ENOMEM; + grpname = devm_kasprintf(dev, GFP_KERNEL, "%s.%s", np->name, child->name); + if (!grpname) + goto put_child; + + pgnames[ngroups++] = grpname; + + if ((list = of_get_property(child, "pinmux", &npins))) { + npins /= sizeof(*list); + + pins = devm_kcalloc(dev, npins, sizeof(*pins), GFP_KERNEL); + if (!pins) + goto free_grpname; + + pinmux = devm_kcalloc(dev, npins, sizeof(*pinmux), GFP_KERNEL); + if (!pinmux) + goto free_pins; + + for (i = 0; i < npins; i++) { + u32 v = be32_to_cpu(*list++); + + pins[i] = starfive_gpio_to_pin(sfp, v & (MAX_GPIO - 1)); + pinmux[i] = v; + } + + map[nmaps].type = PIN_MAP_TYPE_MUX_GROUP; + map[nmaps].data.mux.function = np->name; + map[nmaps].data.mux.group = grpname; + nmaps += 1; + } else if ((list = of_get_property(child, "pins", &npins))) { + npins /= sizeof(*list); + + pins = devm_kcalloc(dev, npins, sizeof(*pins), GFP_KERNEL); + if (!pins) + goto free_grpname; + + pinmux = NULL; + + for (i = 0; i < npins; i++) + pins[i] = be32_to_cpu(*list++); + } else { + ret = -EINVAL; + goto free_grpname; + } + + ret = pinctrl_generic_add_group(pctldev, grpname, pins, npins, pinmux); + if (ret < 0) { + dev_err(dev, "error adding group %pOFn.%pOFn: %d\n", + np, child, ret); + goto free_pinmux; + } + + ret = pinconf_generic_parse_dt_config(child, pctldev, + &map[nmaps].data.configs.configs, + &map[nmaps].data.configs.num_configs); + if (ret) { + dev_err(dev, "invalid pinctrl group %pOFn.%pOFn: %s\n", + np, child, "error parsing pin config"); + goto put_child; + } + + /* don't create a map if there are no pinconf settings */ + if (map[nmaps].data.configs.num_configs == 0) + continue; + + map[nmaps].type = PIN_MAP_TYPE_CONFIGS_GROUP; + map[nmaps].data.configs.group_or_pin = grpname; + nmaps += 1; + } + + ret = pinmux_generic_add_function(pctldev, np->name, pgnames, ngroups, NULL); + if (ret < 0) { + dev_err(dev, "error adding function %pOFn: %d\n", np, ret); + goto free_map; + } + + *maps = map; + *num_maps = nmaps; + return 0; + +free_pinmux: + devm_kfree(dev, pinmux); +free_pins: + devm_kfree(dev, pins); +free_grpname: + devm_kfree(dev, grpname); +put_child: + of_node_put(child); +free_map: + pinctrl_utils_free_map(pctldev, map, nmaps); +free_pgnames: + devm_kfree(dev, pgnames); +out: + return ret; +} + +static const struct pinctrl_ops starfive_pinctrl_ops = { + .get_groups_count = pinctrl_generic_get_group_count, + .get_group_name = pinctrl_generic_get_group_name, + .get_group_pins = pinctrl_generic_get_group_pins, + .pin_dbg_show = starfive_pin_dbg_show, + .dt_node_to_map = starfive_dt_node_to_map, + .dt_free_map = pinctrl_utils_free_map, +}; + +static int starfive_set_mux(struct pinctrl_dev *pctldev, + unsigned int fsel, unsigned int gsel) +{ + struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev); + struct device *dev = starfive_dev(sfp); + const struct group_desc *group; + const u32 *pinmux; + unsigned int i; + + group = pinctrl_generic_get_group(pctldev, gsel); + if (unlikely(!group)) + return -EINVAL; + + pinmux = group->data; + for (i = 0; i < group->num_pins; i++) { + unsigned int gpio = starfive_pin_to_gpio(sfp, group->pins[i]); + void __iomem *reg_dout; + void __iomem *reg_doen; + void __iomem *reg_din; + u32 v, dout, doen, din; + unsigned long flags; + + if (dev_WARN_ONCE(dev, gpio >= MAX_GPIO, + "%s: invalid gpiomux pin", group->name)) + continue; + + v = pinmux[i]; + dout = ((v & BIT(7)) << (31 - 7)) | ((v >> 24) & 0xffU); + doen = ((v & BIT(6)) << (31 - 6)) | ((v >> 16) & 0xffU); + din = (v >> 8) & 0xffU; + + dev_dbg(dev, "GPIO%u: dout=0x%x doen=0x%x din=0x%x\n", + gpio, dout, doen, din); + + reg_dout = sfp->base + GPIO_N_DOUT_CFG + 8 * gpio; + reg_doen = sfp->base + GPIO_N_DOEN_CFG + 8 * gpio; + if (din != 0xff) + reg_din = sfp->base + GPIO_IN_OFFSET + 4 * din; + else + reg_din = NULL; + + raw_spin_lock_irqsave(&sfp->lock, flags); + writel_relaxed(dout, reg_dout); + writel_relaxed(doen, reg_doen); + if (reg_din) + writel_relaxed(gpio + 2, reg_din); + raw_spin_unlock_irqrestore(&sfp->lock, flags); + } + + return 0; +} + +static const struct pinmux_ops starfive_pinmux_ops = { + .get_functions_count = pinmux_generic_get_function_count, + .get_function_name = pinmux_generic_get_function_name, + .get_function_groups = pinmux_generic_get_function_groups, + .set_mux = starfive_set_mux, + .strict = true, +}; + +static u16 starfive_padctl_get(struct starfive_pinctrl *sfp, + unsigned int pin) +{ + void __iomem *reg = sfp->padctl + 4 * (pin / 2); + u32 value = readl_relaxed(reg); + + if (pin & 1U) + value >>= 16; + return value; +} + +static void starfive_padctl_rmw(struct starfive_pinctrl *sfp, + unsigned int pin, + u16 _mask, u16 _value) +{ + void __iomem *reg = sfp->padctl + 4 * (pin / 2); + u32 mask = _mask; + u32 value = _value; + unsigned long flags; + + dev_dbg(starfive_dev(sfp), + "padctl_rmw(%u, 0x%03x, 0x%03x)\n", pin, mask, value); + + if (pin & 1U) { + value <<= 16; + mask <<= 16; + } + + raw_spin_lock_irqsave(&sfp->lock, flags); + value |= readl_relaxed(reg) & ~mask; + writel_relaxed(value, reg); + raw_spin_unlock_irqrestore(&sfp->lock, flags); +} + +#define PIN_CONFIG_STARFIVE_STRONG_PULL_UP (PIN_CONFIG_END + 1) + +static const struct pinconf_generic_params starfive_pinconf_custom_params[] = { + { "starfive,strong-pull-up", PIN_CONFIG_STARFIVE_STRONG_PULL_UP, 1 }, +}; + +#ifdef CONFIG_DEBUG_FS +static const struct pin_config_item +starfive_pinconf_custom_conf_items[ARRAY_SIZE(starfive_pinconf_custom_params)] = { + PCONFDUMP(PIN_CONFIG_STARFIVE_STRONG_PULL_UP, "input bias strong pull-up", NULL, false), +}; +#else +#define starfive_pinconf_custom_conf_items NULL +#endif + +static const unsigned char starfive_drive_strength[] = { + 14, 21, 28, 35, 42, 49, 56, 63, +}; + +static int starfive_pinconf_get(struct pinctrl_dev *pctldev, + unsigned int pin, unsigned long *config) +{ + struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev); + u16 value = starfive_padctl_get(sfp, pin); + int param = pinconf_to_config_param(*config); + u32 arg; + bool enabled; + + switch (param) { + case PIN_CONFIG_BIAS_DISABLE: + enabled = value & PAD_BIAS_DISABLE; + arg = 0; + break; + case PIN_CONFIG_BIAS_PULL_DOWN: + enabled = value & PAD_BIAS_PULL_DOWN; + arg = 1; + break; + case PIN_CONFIG_BIAS_PULL_UP: + enabled = !(value & PAD_BIAS_MASK); + arg = 1; + break; + case PIN_CONFIG_DRIVE_STRENGTH: + enabled = value & PAD_DRIVE_STRENGTH_MASK; + arg = starfive_drive_strength[value & PAD_DRIVE_STRENGTH_MASK]; + break; + case PIN_CONFIG_INPUT_ENABLE: + enabled = value & PAD_INPUT_ENABLE; + arg = enabled; + break; + case PIN_CONFIG_INPUT_SCHMITT_ENABLE: + enabled = value & PAD_INPUT_SCHMITT_ENABLE; + arg = enabled; + break; + case PIN_CONFIG_SLEW_RATE: + enabled = value & PAD_SLEW_RATE_MASK; + arg = (value & PAD_SLEW_RATE_MASK) >> PAD_SLEW_RATE_POS; + break; + case PIN_CONFIG_STARFIVE_STRONG_PULL_UP: + enabled = value & PAD_BIAS_STRONG_PULL_UP; + arg = enabled; + break; + default: + return -ENOTSUPP; + } + + *config = pinconf_to_config_packed(param, arg); + return enabled ? 0 : -EINVAL; +} + +static int starfive_pinconf_group_get(struct pinctrl_dev *pctldev, + unsigned int gsel, unsigned long *config) +{ + const struct group_desc *group; + + group = pinctrl_generic_get_group(pctldev, gsel); + if (unlikely(!group)) + return -EINVAL; + + return starfive_pinconf_get(pctldev, group->pins[0], config); +} + +static int starfive_pinconf_group_set(struct pinctrl_dev *pctldev, + unsigned int gsel, + unsigned long *configs, + unsigned int num_configs) +{ + struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev); + const struct group_desc *group; + u16 mask, value; + int i; + + group = pinctrl_generic_get_group(pctldev, gsel); + if (unlikely(!group)) + return -EINVAL; + + mask = 0; + value = 0; + for (i = 0; i < num_configs; i++) { + int param = pinconf_to_config_param(configs[i]); + u32 arg = pinconf_to_config_argument(configs[i]); + u16 ds; + + switch (param) { + case PIN_CONFIG_BIAS_DISABLE: + mask |= PAD_BIAS_MASK; + value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE; + break; + case PIN_CONFIG_BIAS_PULL_DOWN: + if (arg == 0) + return -ENOTSUPP; + mask |= PAD_BIAS_MASK; + value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_PULL_DOWN; + break; + case PIN_CONFIG_BIAS_PULL_UP: + if (arg == 0) + return -ENOTSUPP; + mask |= PAD_BIAS_MASK; + value = value & ~PAD_BIAS_MASK; + break; + case PIN_CONFIG_DRIVE_STRENGTH: + for (ds = 0; ds < PAD_DRIVE_STRENGTH_MASK; ds++) { + if (arg < starfive_drive_strength[ds + 1]) + break; + } + mask |= PAD_DRIVE_STRENGTH_MASK; + value = (value & ~PAD_DRIVE_STRENGTH_MASK) | ds; + break; + case PIN_CONFIG_INPUT_ENABLE: + mask |= PAD_INPUT_ENABLE; + if (arg) + value |= PAD_INPUT_ENABLE; + else + value &= ~PAD_INPUT_ENABLE; + break; + case PIN_CONFIG_INPUT_SCHMITT_ENABLE: + mask |= PAD_INPUT_SCHMITT_ENABLE; + if (arg) + value |= PAD_INPUT_SCHMITT_ENABLE; + else + value &= ~PAD_INPUT_SCHMITT_ENABLE; + break; + case PIN_CONFIG_SLEW_RATE: + mask |= PAD_SLEW_RATE_MASK; + value = (value & ~PAD_SLEW_RATE_MASK) | + ((arg << PAD_SLEW_RATE_POS) & PAD_SLEW_RATE_MASK); + break; + case PIN_CONFIG_STARFIVE_STRONG_PULL_UP: + if (arg) { + mask |= PAD_BIAS_MASK; + value = (value & ~PAD_BIAS_MASK) | + PAD_BIAS_STRONG_PULL_UP; + } else { + mask |= PAD_BIAS_STRONG_PULL_UP; + value = value & ~PAD_BIAS_STRONG_PULL_UP; + } + break; + default: + return -ENOTSUPP; + } + } + + for (i = 0; i < group->num_pins; i++) + starfive_padctl_rmw(sfp, group->pins[i], mask, value); + + return 0; +} + +#ifdef CONFIG_DEBUG_FS +static void starfive_pinconf_dbg_show(struct pinctrl_dev *pctldev, + struct seq_file *s, unsigned int pin) +{ + struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev); + u16 value = starfive_padctl_get(sfp, pin); + + seq_printf(s, " (0x%03x)", value); +} +#else +#define starfive_pinconf_dbg_show NULL +#endif + +static const struct pinconf_ops starfive_pinconf_ops = { + .pin_config_get = starfive_pinconf_get, + .pin_config_group_get = starfive_pinconf_group_get, + .pin_config_group_set = starfive_pinconf_group_set, + .pin_config_dbg_show = starfive_pinconf_dbg_show, + .is_generic = true, +}; + +static struct pinctrl_desc starfive_desc = { + .name = DRIVER_NAME, + .pins = starfive_pins, + .npins = ARRAY_SIZE(starfive_pins), + .pctlops = &starfive_pinctrl_ops, + .pmxops = &starfive_pinmux_ops, + .confops = &starfive_pinconf_ops, + .owner = THIS_MODULE, + .num_custom_params = ARRAY_SIZE(starfive_pinconf_custom_params), + .custom_params = starfive_pinconf_custom_params, + .custom_conf_items = starfive_pinconf_custom_conf_items, +}; + +static int starfive_gpio_request(struct gpio_chip *gc, unsigned int gpio) +{ + return pinctrl_gpio_request(gc->base + gpio); +} + +static void starfive_gpio_free(struct gpio_chip *gc, unsigned int gpio) +{ + pinctrl_gpio_free(gc->base + gpio); +} + +static int starfive_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio) +{ + struct starfive_pinctrl *sfp = starfive_from_gc(gc); + + if (gpio >= MAX_GPIO) + return -EINVAL; + + /* return GPIO_LINE_DIRECTION_OUT (0) only if doen == GPO_ENABLE (0) */ + return readl_relaxed(sfp->base + GPIO_N_DOEN_CFG + 8 * gpio) != GPO_ENABLE; +} + +static int starfive_gpio_direction_input(struct gpio_chip *gc, + unsigned int gpio) +{ + struct starfive_pinctrl *sfp = starfive_from_gc(gc); + unsigned long flags; + + if (gpio >= MAX_GPIO) + return -EINVAL; + + /* enable input and schmitt trigger */ + starfive_padctl_rmw(sfp, starfive_gpio_to_pin(sfp, gpio), + PAD_INPUT_ENABLE | PAD_INPUT_SCHMITT_ENABLE, + PAD_INPUT_ENABLE | PAD_INPUT_SCHMITT_ENABLE); + + raw_spin_lock_irqsave(&sfp->lock, flags); + writel_relaxed(GPO_DISABLE, sfp->base + GPIO_N_DOEN_CFG + 8 * gpio); + raw_spin_unlock_irqrestore(&sfp->lock, flags); + + return 0; +} + +static int starfive_gpio_direction_output(struct gpio_chip *gc, + unsigned int gpio, int value) +{ + struct starfive_pinctrl *sfp = starfive_from_gc(gc); + unsigned long flags; + + if (gpio >= MAX_GPIO) + return -EINVAL; + + raw_spin_lock_irqsave(&sfp->lock, flags); + writel_relaxed(value, sfp->base + GPIO_N_DOUT_CFG + 8 * gpio); + writel_relaxed(GPO_ENABLE, sfp->base + GPIO_N_DOEN_CFG + 8 * gpio); + raw_spin_unlock_irqrestore(&sfp->lock, flags); + + /* disable input, schmitt trigger and bias */ + starfive_padctl_rmw(sfp, starfive_gpio_to_pin(sfp, gpio), + PAD_BIAS_MASK | PAD_INPUT_ENABLE | PAD_INPUT_SCHMITT_ENABLE, + PAD_BIAS_DISABLE); + + return 0; +} + +static int starfive_gpio_get(struct gpio_chip *gc, unsigned int gpio) +{ + struct starfive_pinctrl *sfp = starfive_from_gc(gc); + u32 value; + + if (gpio >= MAX_GPIO) + return -EINVAL; + + if (gpio < 32) { + value = readl_relaxed(sfp->base + GPIO_DIN_LOW); + value = (value >> gpio) & 1U; + } else { + value = readl_relaxed(sfp->base + GPIO_DIN_HIGH); + value = (value >> (gpio - 32)) & 1U; + } + + return value; +} + +static void starfive_gpio_set(struct gpio_chip *gc, unsigned int gpio, + int value) +{ + struct starfive_pinctrl *sfp = starfive_from_gc(gc); + unsigned long flags; + + if (gpio >= MAX_GPIO) + return; + + raw_spin_lock_irqsave(&sfp->lock, flags); + writel_relaxed(value, sfp->base + GPIO_N_DOUT_CFG + 8 * gpio); + raw_spin_unlock_irqrestore(&sfp->lock, flags); +} + +static int starfive_gpio_set_config(struct gpio_chip *gc, unsigned int gpio, + unsigned long config) +{ + struct starfive_pinctrl *sfp = starfive_from_gc(gc); + u32 arg = pinconf_to_config_argument(config); + u16 mask; + u16 value; + + switch (pinconf_to_config_param(config)) { + case PIN_CONFIG_BIAS_DISABLE: + mask = PAD_BIAS_MASK; + value = PAD_BIAS_DISABLE; + break; + case PIN_CONFIG_BIAS_PULL_DOWN: + if (arg == 0) + return -ENOTSUPP; + mask = PAD_BIAS_MASK; + value = PAD_BIAS_PULL_DOWN; + break; + case PIN_CONFIG_BIAS_PULL_UP: + if (arg == 0) + return -ENOTSUPP; + mask = PAD_BIAS_MASK; + value = 0; + break; + case PIN_CONFIG_DRIVE_PUSH_PULL: + return 0; + case PIN_CONFIG_INPUT_ENABLE: + mask = PAD_INPUT_ENABLE; + value = arg ? PAD_INPUT_ENABLE : 0; + break; + case PIN_CONFIG_INPUT_SCHMITT_ENABLE: + mask = PAD_INPUT_SCHMITT_ENABLE; + value = arg ? PAD_INPUT_SCHMITT_ENABLE : 0; + break; + default: + return -ENOTSUPP; + }; + + starfive_padctl_rmw(sfp, starfive_gpio_to_pin(sfp, gpio), mask, value); + return 0; +} + +static int starfive_gpio_add_pin_ranges(struct gpio_chip *gc) +{ + struct starfive_pinctrl *sfp = starfive_from_gc(gc); + + sfp->gpios.name = sfp->gc.label; + sfp->gpios.base = sfp->gc.base; + /* + * sfp->gpios.pin_base depends on the chosen signal group + * and is set in starfive_probe() + */ + sfp->gpios.npins = MAX_GPIO; + sfp->gpios.gc = &sfp->gc; + pinctrl_add_gpio_range(sfp->pctl, &sfp->gpios); + return 0; +} + +static void starfive_irq_ack(struct irq_data *d) +{ + struct starfive_pinctrl *sfp = starfive_from_irq_data(d); + irq_hw_number_t gpio = irqd_to_hwirq(d); + unsigned long flags; + void __iomem *ic; + u32 mask; + + if (gpio < 0 || gpio >= MAX_GPIO) + return; + + if (gpio < 32) { + ic = sfp->base + IRQ_CLEAR_EDGE_LOW; + mask = BIT(gpio); + } else { + ic = sfp->base + IRQ_CLEAR_EDGE_HIGH; + mask = BIT(gpio - 32); + } + + raw_spin_lock_irqsave(&sfp->lock, flags); + writel_relaxed(mask, ic); + raw_spin_unlock_irqrestore(&sfp->lock, flags); +} + +static void starfive_irq_mask(struct irq_data *d) +{ + struct starfive_pinctrl *sfp = starfive_from_irq_data(d); + irq_hw_number_t gpio = irqd_to_hwirq(d); + unsigned long flags; + void __iomem *ie; + u32 mask, value; + + if (gpio < 0 || gpio >= MAX_GPIO) + return; + + if (gpio < 32) { + ie = sfp->base + IRQ_ENABLE_LOW; + mask = BIT(gpio); + } else { + ie = sfp->base + IRQ_ENABLE_HIGH; + mask = BIT(gpio - 32); + } + + raw_spin_lock_irqsave(&sfp->lock, flags); + value = readl_relaxed(ie); + value &= ~mask; + writel_relaxed(value, ie); + raw_spin_unlock_irqrestore(&sfp->lock, flags); +} + +static void starfive_irq_mask_ack(struct irq_data *d) +{ + struct starfive_pinctrl *sfp = starfive_from_irq_data(d); + irq_hw_number_t gpio = irqd_to_hwirq(d); + unsigned long flags; + void __iomem *ie; + void __iomem *ic; + u32 mask, value; + + if (gpio < 0 || gpio >= MAX_GPIO) + return; + + if (gpio < 32) { + ie = sfp->base + IRQ_ENABLE_LOW; + ic = sfp->base + IRQ_CLEAR_EDGE_LOW; + mask = BIT(gpio); + } else { + ie = sfp->base + IRQ_ENABLE_HIGH; + ic = sfp->base + IRQ_CLEAR_EDGE_HIGH; + mask = BIT(gpio - 32); + } + + raw_spin_lock_irqsave(&sfp->lock, flags); + value = readl_relaxed(ie); + value &= ~mask; + writel_relaxed(value, ie); + writel_relaxed(mask, ic); + raw_spin_unlock_irqrestore(&sfp->lock, flags); +} + +static void starfive_irq_unmask(struct irq_data *d) +{ + struct starfive_pinctrl *sfp = starfive_from_irq_data(d); + irq_hw_number_t gpio = irqd_to_hwirq(d); + unsigned long flags; + void __iomem *ie; + u32 mask, value; + + if (gpio < 0 || gpio >= MAX_GPIO) + return; + + if (gpio < 32) { + ie = sfp->base + IRQ_ENABLE_LOW; + mask = BIT(gpio); + } else { + ie = sfp->base + IRQ_ENABLE_HIGH; + mask = BIT(gpio - 32); + } + + raw_spin_lock_irqsave(&sfp->lock, flags); + value = readl_relaxed(ie); + value |= mask; + writel_relaxed(value, ie); + raw_spin_unlock_irqrestore(&sfp->lock, flags); +} + +static int starfive_irq_set_type(struct irq_data *d, unsigned int trigger) +{ + struct starfive_pinctrl *sfp = starfive_from_irq_data(d); + irq_hw_number_t gpio = irqd_to_hwirq(d); + unsigned long flags; + void __iomem *base; + u32 mask, irq_type, edge_both, polarity; + + if (gpio < 0 || gpio >= MAX_GPIO) + return -EINVAL; + + if (gpio < 32) { + base = sfp->base; + mask = BIT(gpio); + } else { + base = sfp->base + 4; + mask = BIT(gpio - 32); + } + + switch (trigger) { + case IRQ_TYPE_EDGE_RISING: + irq_set_handler_locked(d, handle_edge_irq); + irq_type = mask; /* 1: edge triggered */ + edge_both = 0; /* 0: single edge */ + polarity = mask; /* 1: rising edge */ + break; + case IRQ_TYPE_EDGE_FALLING: + irq_set_handler_locked(d, handle_edge_irq); + irq_type = mask; /* 1: edge triggered */ + edge_both = 0; /* 0: single edge */ + polarity = 0; /* 0: falling edge */ + break; + case IRQ_TYPE_EDGE_BOTH: + irq_set_handler_locked(d, handle_edge_irq); + irq_type = mask; /* 1: edge triggered */ + edge_both = mask; /* 1: both edges */ + polarity = 0; /* 0: ignored */ + break; + case IRQ_TYPE_LEVEL_HIGH: + irq_set_handler_locked(d, handle_level_irq); + irq_type = 0; /* 0: level trigged */ + edge_both = 0; /* 0: ignored */ + polarity = mask; /* 1: high level */ + break; + case IRQ_TYPE_LEVEL_LOW: + irq_set_handler_locked(d, handle_level_irq); + irq_type = 0; /* 0: level triggered */ + edge_both = 0; /* 0: ignored */ + polarity = 0; /* 0: low level */ + break; + default: + irq_set_handler_locked(d, handle_bad_irq); + return -ENOTSUPP; + } + + raw_spin_lock_irqsave(&sfp->lock, flags); + irq_type |= readl_relaxed(base + IRQ_TYPE_LOW) & ~mask; + writel_relaxed(irq_type, base + IRQ_TYPE_LOW); + edge_both |= readl_relaxed(base + IRQ_EDGE_BOTH_LOW) & ~mask; + writel_relaxed(edge_both, base + IRQ_EDGE_BOTH_LOW); + polarity |= readl_relaxed(base + IRQ_POLARITY_LOW) & ~mask; + writel_relaxed(polarity, base + IRQ_POLARITY_LOW); + raw_spin_unlock_irqrestore(&sfp->lock, flags); + return 0; +} + +static struct irq_chip starfive_irq_chip = { + .irq_ack = starfive_irq_ack, + .irq_mask = starfive_irq_mask, + .irq_mask_ack = starfive_irq_mask_ack, + .irq_unmask = starfive_irq_unmask, + .irq_set_type = starfive_irq_set_type, + .flags = IRQCHIP_SET_TYPE_MASKED, +}; + +static void starfive_gpio_irq_handler(struct irq_desc *desc) +{ + struct starfive_pinctrl *sfp = starfive_from_irq_desc(desc); + struct irq_chip *chip = irq_desc_get_chip(desc); + unsigned long mis; + unsigned int pin; + + chained_irq_enter(chip, desc); + + mis = readl_relaxed(sfp->base + IRQ_MASKED_STATUS_LOW); + for_each_set_bit(pin, &mis, 32) + generic_handle_domain_irq(sfp->gc.irq.domain, pin); + + mis = readl_relaxed(sfp->base + IRQ_MASKED_STATUS_HIGH); + for_each_set_bit(pin, &mis, 32) + generic_handle_domain_irq(sfp->gc.irq.domain, pin + 32); + + chained_irq_exit(chip, desc); +} + +static int starfive_gpio_init_hw(struct gpio_chip *gc) +{ + struct starfive_pinctrl *sfp = starfive_from_gc(gc); + + /* mask all GPIO interrupts */ + writel(0, sfp->base + IRQ_ENABLE_LOW); + writel(0, sfp->base + IRQ_ENABLE_HIGH); + /* clear edge interrupt flags */ + writel(~0U, sfp->base + IRQ_CLEAR_EDGE_LOW); + writel(~0U, sfp->base + IRQ_CLEAR_EDGE_HIGH); + /* enable GPIO interrupts */ + writel(1, sfp->base + IRQ_GLOBAL_EN); + return 0; +} + +static int __init starfive_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct starfive_pinctrl *sfp; + struct clk *clk; + struct reset_control *rst; + u32 value; + int ret; + + sfp = devm_kzalloc(dev, sizeof(*sfp), GFP_KERNEL); + if (!sfp) + return -ENOMEM; + + sfp->base = devm_platform_ioremap_resource_byname(pdev, "gpio"); + if (IS_ERR(sfp->base)) + return PTR_ERR(sfp->base); + + sfp->padctl = devm_platform_ioremap_resource_byname(pdev, "padctl"); + if (IS_ERR(sfp->padctl)) + return PTR_ERR(sfp->padctl); + + clk = devm_clk_get(dev, NULL); + if (IS_ERR(clk)) { + ret = PTR_ERR(clk); + dev_err(dev, "could not get clock: %d\n", ret); + return ret; + } + + rst = devm_reset_control_get_exclusive(dev, NULL); + if (IS_ERR(rst)) { + ret = PTR_ERR(rst); + dev_err(dev, "could not get reset: %d\n", ret); + return ret; + } + + ret = reset_control_assert(rst); + if (ret) { + dev_err(dev, "could not assert reset: %d\n", ret); + return ret; + } + + ret = clk_prepare_enable(clk); + if (ret) { + reset_control_deassert(rst); + dev_err(dev, "could not enable clock: %d\n", ret); + return ret; + } + + ret = reset_control_deassert(rst); + if (ret) { + dev_err(dev, "could not deassert reset: %d\n", ret); + return ret; + } + + platform_set_drvdata(pdev, sfp); + sfp->gc.parent = dev; + raw_spin_lock_init(&sfp->lock); + + ret = devm_pinctrl_register_and_init(dev, &starfive_desc, sfp, &sfp->pctl); + if (ret) { + dev_err(dev, "could not register pinctrl driver: %d\n", ret); + return ret; + } + + if (!of_property_read_u32(dev->of_node, "starfive,signal-group", &value)) { + if (value <= 6) + writel(value, sfp->padctl + IO_PADSHARE_SEL); + else + dev_err(dev, "invalid signal group %u\n", value); + } + + value = readl(sfp->padctl + IO_PADSHARE_SEL); + switch (value) { + case 0: + sfp->gpios.pin_base = 0x10000; + goto done; + case 1: + sfp->gpios.pin_base = PAD_GPIO(0); + break; + case 2: + sfp->gpios.pin_base = PAD_FUNC_SHARE(72); + break; + case 3: + sfp->gpios.pin_base = PAD_FUNC_SHARE(70); + break; + case 4: case 5: case 6: + sfp->gpios.pin_base = PAD_FUNC_SHARE(0); + break; + default: + dev_err(dev, "invalid signal group %u\n", value); + return -EINVAL; + } + + sfp->gc.label = dev_name(dev); + sfp->gc.of_node = dev->of_node; + sfp->gc.owner = THIS_MODULE; + sfp->gc.request = starfive_gpio_request; + sfp->gc.free = starfive_gpio_free; + sfp->gc.get_direction = starfive_gpio_get_direction; + sfp->gc.direction_input = starfive_gpio_direction_input; + sfp->gc.direction_output = starfive_gpio_direction_output; + sfp->gc.get = starfive_gpio_get; + sfp->gc.set = starfive_gpio_set; + sfp->gc.set_config = starfive_gpio_set_config; + sfp->gc.add_pin_ranges = starfive_gpio_add_pin_ranges; + sfp->gc.base = -1; + sfp->gc.ngpio = MAX_GPIO; + + starfive_irq_chip.parent_device = dev; + starfive_irq_chip.name = sfp->gc.label; + + sfp->gc.irq.chip = &starfive_irq_chip; + sfp->gc.irq.parent_handler = starfive_gpio_irq_handler; + sfp->gc.irq.parents = + devm_kcalloc(dev, 1, sizeof(*sfp->gc.irq.parents), GFP_KERNEL); + if (!sfp->gc.irq.parents) + return -ENOMEM; + sfp->gc.irq.num_parents = 1; + sfp->gc.irq.default_type = IRQ_TYPE_NONE; + sfp->gc.irq.handler = handle_bad_irq; + sfp->gc.irq.init_hw = starfive_gpio_init_hw; + + ret = platform_get_irq(pdev, 0); + if (ret < 0) + return ret; + sfp->gc.irq.parents[0] = ret; + + ret = devm_gpiochip_add_data(dev, &sfp->gc, sfp); + if (ret) { + dev_err(dev, "could not register gpiochip: %d\n", ret); + return ret; + } + + dev_info(dev, "StarFive GPIO chip registered %d GPIOs\n", sfp->gc.ngpio); +done: + return pinctrl_enable(sfp->pctl); +} + +static const struct of_device_id starfive_of_match[] = { + { .compatible = "starfive,jh7100-pinctrl" }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, starfive_of_match); + +static struct platform_driver starfive_pinctrl_driver = { + .probe = starfive_probe, + .driver = { + .name = DRIVER_NAME, + .of_match_table = starfive_of_match, + }, +}; +module_platform_driver(starfive_pinctrl_driver); + +MODULE_DESCRIPTION("Pinctrl driver for StarFive SoCs"); +MODULE_AUTHOR("Emil Renner Berthing <kernel@esmil.dk>"); +MODULE_LICENSE("GPL v2");