| Message ID | 20231221083622.3445726-1-yuklin.soo@starfivetech.com |
|---|---|
| Headers | show |
| Series | Add Pinctrl driver for Starfive JH8100 SoC | expand |
Hi Alex, thanks for your patch! On Thu, Dec 21, 2023 at 9:36 AM Alex Soo <yuklin.soo@starfivetech.com> wrote: > pinctrl: starfive: jh8100: add pinctrl driver for sys_east domain > pinctrl: starfive: jh8100: add pinctrl driver for sys_west domain > pinctrl: starfive: jh8100: add pinctrl driver for sys_gmac domain > pinctrl: starfive: jh8100: add pinctrl driver for AON domain To my eye it looks like a lot of code is duplicated between the four subdrivers. The pattern from other pin controllers is to create a file with all the common code and then subdrivers for each sub-pincontroller that have their own probe but calls into the library. C.f. drivers/pinctrl/qcom/pinctrl-apq8064.c: static int apq8064_pinctrl_probe(struct platform_device *pdev) { return msm_pinctrl_probe(pdev, &apq8064_pinctrl); } And that function is in drivers/pinctrl/qcom/pinctrl-msm.c and you find great inspiration in the qcom Kconfig and Makefile and drivers/pinctrl/qcom/pinctrl-msm.h that you can copypaste to pull this off. Maybe you should start with a patch that extract the common stuff from the existing jh7100/jh7110 drivers and then reuse that for jh8100? Yours, Linus Walleij
> -----Original Message----- > From: Linus Walleij <linus.walleij@linaro.org> > Sent: Thursday, December 21, 2023 9:57 PM > To: Yuklin Soo <yuklin.soo@starfivetech.com> > Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Hal Feng > <hal.feng@starfivetech.com>; Leyfoon Tan <leyfoon.tan@starfivetech.com>; > Jianlong Huang <jianlong.huang@starfivetech.com>; Emil Renner Berthing > <kernel@esmil.dk>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Drew Fustini <drew@beagleboard.org>; linux-gpio@vger.kernel.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org; linux- > riscv@lists.infradead.org; Paul Walmsley <paul.walmsley@sifive.com>; Palmer > Dabbelt <palmer@dabbelt.com>; Albert Ou <aou@eecs.berkeley.edu> > Subject: Re: [RFC PATCH 1/6] dt-bindings: pinctrl: starfive: add JH8100 pinctrl > bindings > > Hi Alex, > > thanks for your patch! > > On Thu, Dec 21, 2023 at 9:36 AM Alex Soo <yuklin.soo@starfivetech.com> > wrote: > > > Add dt-binding documentation and header file for JH8100 pinctrl > > driver. > > > > Signed-off-by: Alex Soo <yuklin.soo@starfivetech.com> > > Reviewed-by: Ley Foon Tan <leyfoon.tan@starfivetech.com> > (...) > > +title: StarFive JH8100 AON Pin Controller > > If AON means "always-on" then spell that out in the title, the world has too > many opaque TLAs. > > title: StarFive JH8100 AON (always-on) Pin Controller This title will be updated as shown above to clarify the meaning of acronym AON. > > (...) > > + properties: > > + pinmux: > > + description: | > > + The list of GPIOs and their mux settings or function select. > > + The GPIOMUX and PINMUX macros are used to configure the > > + I/O multiplexing and function selection respectively. > > + > > + bias-disable: true > > + > > + bias-pull-up: > > + type: boolean > > + > > + bias-pull-down: > > + type: boolean > > + > > + drive-strength: > > + enum: [ 2, 4, 8, 12 ] > > Milliamperes? Then spell that out in a description: Yes, the unit is milliamperes, “drive-strength:” will be changed to “drive-strength: Drive strength in mA” > > > + Voltage regulator supply the actual voltage to the GPIO bank while > > + the syscon register configuration in bit [1:0] indicates the current voltage > setting. > > + > > + +------+------+-------------------+ > > + | Bit1 | Bit0 | Reference Voltage | > > + +------+------+-------------------+ > > + | 0 | 0 | 3.3 V | > > + +------+------+-------------------+ > > + | 1 | x | 1.8 V | > > + +------+------+-------------------+ > > + > > + To increase the device voltage, set bit [1:0] to the new operating > > + state first before raising the actual voltage to the higher operating point. > > + > > + To decrease the device voltage, hold bit [1:0] to the current > > + operating state until the actual voltage has stabilized at the > > + lower operating point before changing the setting. > > + > > + Alternatively, a device voltage change can always be initiated by > > + first setting syscon register bit [1:0] = 0, the safe 3.3V startup > > + condition, before changing the device voltage. Then once the actual > > + voltage is changed and has stabilized at the new operating point, bit [1:0] > can be reset as appropriate. > > Actually: where is this information even used? Each pin controller in JH8100 SoC has a set of regmap-based syscon registers to serve different purposes in respective pinctrl domain. In AON (always-on) pinctrl, there are: - syscon registers to indicate the I/O voltage level of eMMC, SD0, RGPIOs, and XSPI. - syscon registers to control the GMAC settings. The former will be used in the SD/eMMC device driver to indicate the change in voltage supply during voltage switching in the initialization process. The syscon register configuration provides information to indicate the device I/O voltage level. The device driver must make sure these syscon registers are configured properly. In case if setting syscon register to indicate device I/O voltage as 1.8V, but the actual voltage supply is 3.3V. The pads used for the device I/O interface will get damaged. > > > + slew-rate: > > + maximum: 1 > > Milliseconds? Write unit in description please. The slew-rate is a single bit in the IO Pad configuration register. Bit value 0 means slow and 1 means fast. slew-rate: maximum: 1 will be changed to slew-rate: enum: [ 0, 1 ] default: 0 description: | 0: slow (half frequency) 1: fast Is that okay? > > Yours, > Linus Walleij
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Thursday, December 21, 2023 11:45 PM > To: Linus Walleij <linus.walleij@linaro.org>; Yuklin Soo > <yuklin.soo@starfivetech.com> > Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Hal Feng > <hal.feng@starfivetech.com>; Leyfoon Tan <leyfoon.tan@starfivetech.com>; > Jianlong Huang <jianlong.huang@starfivetech.com>; Emil Renner Berthing > <kernel@esmil.dk>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Drew Fustini <drew@beagleboard.org>; linux-gpio@vger.kernel.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org; linux- > riscv@lists.infradead.org; Paul Walmsley <paul.walmsley@sifive.com>; Palmer > Dabbelt <palmer@dabbelt.com>; Albert Ou <aou@eecs.berkeley.edu> > Subject: Re: [RFC PATCH 1/6] dt-bindings: pinctrl: starfive: add JH8100 pinctrl > bindings > > On 21/12/2023 14:57, Linus Walleij wrote: > >> + drive-strength: > >> + enum: [ 2, 4, 8, 12 ] > > > > Milliamperes? Then spell that out in a description: > > Or just use drive-strength-microamp Suggest changing “drive-strength:” to “drive-strength: Drive strength in mA” since the unit is in mA. > > Best regards, > Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Friday, December 22, 2023 12:20 AM > To: Yuklin Soo <yuklin.soo@starfivetech.com>; Linus Walleij > <linus.walleij@linaro.org>; Bartosz Golaszewski > <bartosz.golaszewski@linaro.org>; Hal Feng <hal.feng@starfivetech.com>; > Leyfoon Tan <leyfoon.tan@starfivetech.com>; Jianlong Huang > <jianlong.huang@starfivetech.com>; Emil Renner Berthing > <kernel@esmil.dk>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Drew Fustini <drew@beagleboard.org> > Cc: linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org; linux-riscv@lists.infradead.org; Paul Walmsley > <paul.walmsley@sifive.com>; Palmer Dabbelt <palmer@dabbelt.com>; > Albert Ou <aou@eecs.berkeley.edu> > Subject: Re: [RFC PATCH 0/6] Add Pinctrl driver for Starfive JH8100 SoC > > On 21/12/2023 09:36, Alex Soo wrote: > > Starfive JH8100 SoC consists of 4 pinctrl domains - sys_east, > > sys_west, sys_gmac, and aon. This patch series adds pinctrl drivers > > for these 4 pinctrl domains and this patch series is depending on the > > JH8100 base patch series in [1] and [2]. > > The relevant dt-binding documentation for each pinctrl domain has been > > updated accordingly. > > Please explain why this is RFC. Every patch is RFC, so what is special about > here? Usually this means work is not finished and should not be merged, > neither reviewed. If you spelled out here the reasons, it would be easier for > us to understand whether we should complain about broken and non- > building code or not. This JH8100 SoC pinctrl patch is dependent on the following: - Initial device tree support and dt-bindings for JH8100 SoC https://lore.kernel.org/lkml/20231214-platonic-unhearing-27e2ec3d8f75@spud/ - Clock & Reset Support for JH8100 SoC https://lore.kernel.org/lkml/20231206115000.295825-1-jeeheng.sia@starfivetech.com/ Refer to the first link, there is maintainer feedback that if our evaluation platform is FPGA-based (since actual silicon is still unavailable), they are not keen on merging the patches, and things like pinctrl or clock drivers should first be submitted as “not to be merged”, in other words, as RFC patches. > > Best regards, > Krzysztof
On Thu, Jan 04, 2024 at 03:12:31AM +0000, Yuklin Soo wrote: > > > > -----Original Message----- > > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Sent: Thursday, December 21, 2023 11:45 PM > > To: Linus Walleij <linus.walleij@linaro.org>; Yuklin Soo > > <yuklin.soo@starfivetech.com> > > Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Hal Feng > > <hal.feng@starfivetech.com>; Leyfoon Tan <leyfoon.tan@starfivetech.com>; > > Jianlong Huang <jianlong.huang@starfivetech.com>; Emil Renner Berthing > > <kernel@esmil.dk>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski > > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > > Drew Fustini <drew@beagleboard.org>; linux-gpio@vger.kernel.org; linux- > > kernel@vger.kernel.org; devicetree@vger.kernel.org; linux- > > riscv@lists.infradead.org; Paul Walmsley <paul.walmsley@sifive.com>; Palmer > > Dabbelt <palmer@dabbelt.com>; Albert Ou <aou@eecs.berkeley.edu> > > Subject: Re: [RFC PATCH 1/6] dt-bindings: pinctrl: starfive: add JH8100 pinctrl > > bindings > > > > On 21/12/2023 14:57, Linus Walleij wrote: > > >> + drive-strength: > > >> + enum: [ 2, 4, 8, 12 ] > > > > > > Milliamperes? Then spell that out in a description: > > > > Or just use drive-strength-microamp > > Suggest changing “drive-strength:” to “drive-strength: Drive strength in mA” since the unit is in mA. Just call the property "drive-strength-microamp". We have existing users of that property. Cheers, Conor.
Hi Alex, thanks for your patch! On Thu, Dec 21, 2023 at 9:36 AM Alex Soo <yuklin.soo@starfivetech.com> wrote: > Add pinctrl driver for sys_east domain. This commit message is wrong, it also contains the main driver for jh8100. Please add some proper subject and commit message. > Signed-off-by: Alex Soo <yuklin.soo@starfivetech.com> > Reviewed-by: Ley Foon Tan <leyfoon.tan@starfivetech.com> (...) > +#define pin_to_hwirq(sfp) (((sfp)->wakeup_gpio) - ((sfp)->gc.base)) Please do not reference gc.base like this, it is a gpio internal detail. Also, turn this into a static inline function, the macro is hard to read. > +/* pad control bits */ > +#define JH8100_PADCFG_POS BIT(7) > +#define JH8100_PADCFG_SMT BIT(6) > +#define JH8100_PADCFG_SLEW BIT(5) > +#define JH8100_PADCFG_PD BIT(4) > +#define JH8100_PADCFG_PU BIT(3) > +#define JH8100_PADCFG_BIAS (JH8100_PADCFG_PD | JH8100_PADCFG_PU) JH8100_PADCFG_BIAS_MASK > +#define JH8100_PADCFG_DS_MASK GENMASK(2, 1) > +#define JH8100_PADCFG_DS_2MA (0U << 1) > +#define JH8100_PADCFG_DS_4MA BIT(1) > +#define JH8100_PADCFG_DS_8MA (2U << 1) > +#define JH8100_PADCFG_DS_12MA (3U << 1) Please use (1U << 1) for 4MA, this looks weird otherwise. > +static const struct pinconf_ops jh8100_pinconf_ops = { > + .pin_config_get = jh8100_pinconf_get, > + .pin_config_group_get = jh8100_pinconf_group_get, > + .pin_config_group_set = jh8100_pinconf_group_set, > + .pin_config_dbg_show = jh8100_pinconf_dbg_show, > + .is_generic = true, > +}; > + > +static int jh8100_gpio_request(struct gpio_chip *gc, unsigned int gpio) > +{ > + return pinctrl_gpio_request(gc, gpio); > +} > + > +static void jh8100_gpio_free(struct gpio_chip *gc, unsigned int gpio) > +{ > + pinctrl_gpio_free(gc, gpio); > +} Skip one level of indirection, just add pinctrl_gpio_request/free directly into the vtable. > +static int jh8100_gpio_set_config(struct gpio_chip *gc, > + unsigned int gpio, unsigned long config) > +{ > + struct jh8100_pinctrl *sfp = container_of(gc, > + struct jh8100_pinctrl, gc); > + u32 arg = pinconf_to_config_argument(config); Please don't reimplement .set_config, just call into the pinctrl backend using .set_config = gpiochip_generic_config > +static int jh8100_gpio_add_pin_ranges(struct gpio_chip *gc) > +{ > + struct jh8100_pinctrl *sfp = container_of(gc, > + struct jh8100_pinctrl, gc); > + > + sfp->gpios.name = sfp->gc.label; > + sfp->gpios.base = sfp->gc.base; > + sfp->gpios.pin_base = 0; > + sfp->gpios.npins = sfp->gc.ngpio; > + sfp->gpios.gc = &sfp->gc; > + pinctrl_add_gpio_range(sfp->pctl, &sfp->gpios); > + return 0; > +} Why are you not putting the ranges into the device tree where the GPIO core will add them for you? > + if (info->irq_reg) { > + jh8100_irq_chip.name = sfp->gc.label; That's not immutable. The struct should be const. You have to use .irq_print_chip in the irq_chip. > + gpio_irq_chip_set_chip(&sfp->gc.irq, &jh8100_irq_chip); Use the convention: struct gpio_irq_chip *girq; girq = &chip->irq; gpio_irq_chip_set_chip(girq, &nmk_irq_chip); ... and use girq-> in the rest of the assignments. > + dev_info(dev, "StarFive GPIO chip registered %d GPIOs\n", sfp->gc.ngpio); StarFive JH8100 (be precise) Yours, Linus Walleij