This series adds support for the StarFive JH7100 RISC-V SoC. The SoC has
many devices that need non-coherent dma operations to work which isn't
upstream yet[1], so this just adds basic support to boot up, get a
serial console, blink an LED and reboot itself. Unlike the Allwinner D1
this chip doesn't use any extra pagetable bits, but instead the DDR RAM
appears twice in the memory map, with and without the cache.
The JH7100 is a test chip for the upcoming JH7110 and about 300 BeagleV
Starlight Beta boards were sent out with them as part of a now cancelled
BeagleBoard.org project. However StarFive has produced more of the
JH7100s and more boards will be available[2] to buy. I've seen pictures
of the new boards now, so hopefully before the end of the year.
This series is also available at
https://github.com/esmil/linux/commits/starlight-minimal
..but a more complete kernel including drivers for non-coherent
peripherals based on this series can be found at
https://github.com/starfive-tech/linux/tree/visionfive
[1]: https://lore.kernel.org/linux-riscv/[email protected]/
[2]: https://www.linkedin.com/pulse/starfive-release-open-source-single-board-platform-q3-2021-starfive/
/Emil
Changes since v2:
- Ahmad and Geert agreed to switch the license of the clock and reset dt
headers to GPL-2.0 OR MIT, so that both headers and device tree files
can all use the same license.
Bindings are still GPL-2.0-only OR BSD-2-Clause as recommended.
- Clock and reset drivers now set .suppress_bind_attrs = true and use
builtin_platform_driver_probe to make sure the probe function is only
called at init time so we can use __init and __initconst.
- The clock driver now uses devm_clk_hw_register and .parent_data when
registering clocks. This way we can use the dt clock indexes rather
than strings for parent lists and decrease the amount of static data
needed considerably.
- Various dt binding cleanups from Rob
- Reworked description in the pinctrl dt binding.
- Pinctrl driver now depends on CONFIG_OF again since it uses
pinconf_generic_parse_dt_config which is otherwise not defined.
- Pinctrl no longer devm_kfree's data that won't be referenced
if dt pinconf parsing fails before registering groups and function,
and other nits by Andy.
- The dw8250 quirk no longer needs a skip_clk_set_rate bit, but sets
port->set_termios to the function called after clk_set_rate.
Changes since v1:
- Let SOC_STARFIVE select RESET_CONTROLLER but drop SERIAL_8250_DW
- Add missing Signed-of-by to clock dt-binding header
- Use builtin_platform_driver macro for the clock driver, add explicit
comment to the determine_rate callback and other small nits from Andy
- Use reset-controller for node names in documentation and device tree
- Use readl_poll_timeout in reset driver to avoid hanging forever if a
driver leaves the associated clock gated and sort Kconfig and Makefile
entries properly.
- In the pinctrl driver align register names with documentation, remove
invalid __init tag from probe function, use of_property_* functions to
parse device tree, hoist pinmux unpacking into helper function to
better document what's going on, bail on invalid signal group in
device tree and fix many other nits from Andy.
- Refactor and rebase 8250_dw quirk on tty-next
Emil Renner Berthing (12):
RISC-V: Add StarFive SoC Kconfig option
dt-bindings: timer: Add StarFive JH7100 clint
dt-bindings: interrupt-controller: Add StarFive JH7100 plic
dt-bindings: reset: Add Starfive JH7100 reset bindings
reset: starfive-jh7100: Add StarFive JH7100 reset driver
dt-bindings: pinctrl: Add StarFive pinctrl definitions
dt-bindings: pinctrl: Add StarFive JH7100 bindings
pinctrl: starfive: Add pinctrl driver for StarFive SoCs
dt-bindings: serial: snps-dw-apb-uart: Add JH7100 uarts
serial: 8250_dw: Add StarFive JH7100 quirk
RISC-V: Add initial StarFive JH7100 device tree
RISC-V: Add BeagleV Starlight Beta device tree
Geert Uytterhoeven (4):
dt-bindings: clock: starfive: Add JH7100 clock definitions
dt-bindings: clock: starfive: Add JH7100 bindings
clk: starfive: Add JH7100 clock generator driver
dt-bindings: reset: Add StarFive JH7100 reset definitions
.../clock/starfive,jh7100-clkgen.yaml | 56 +
.../sifive,plic-1.0.0.yaml | 1 +
.../pinctrl/starfive,jh7100-pinctrl.yaml | 307 ++++
.../bindings/reset/starfive,jh7100-reset.yaml | 38 +
.../bindings/serial/snps-dw-apb-uart.yaml | 5 +
.../bindings/timer/sifive,clint.yaml | 1 +
MAINTAINERS | 22 +
arch/riscv/Kconfig.socs | 8 +
arch/riscv/boot/dts/Makefile | 1 +
arch/riscv/boot/dts/starfive/Makefile | 2 +
.../dts/starfive/jh7100-beaglev-starlight.dts | 164 ++
arch/riscv/boot/dts/starfive/jh7100.dtsi | 230 +++
drivers/clk/Kconfig | 1 +
drivers/clk/Makefile | 1 +
drivers/clk/starfive/Kconfig | 9 +
drivers/clk/starfive/Makefile | 3 +
drivers/clk/starfive/clk-starfive-jh7100.c | 689 +++++++++
drivers/pinctrl/Kconfig | 17 +
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-starfive.c | 1353 +++++++++++++++++
drivers/reset/Kconfig | 7 +
drivers/reset/Makefile | 1 +
drivers/reset/reset-starfive-jh7100.c | 178 +++
drivers/tty/serial/8250/8250_dw.c | 3 +
include/dt-bindings/clock/starfive-jh7100.h | 202 +++
.../dt-bindings/pinctrl/pinctrl-starfive.h | 275 ++++
include/dt-bindings/reset/starfive-jh7100.h | 126 ++
27 files changed, 3701 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/starfive,jh7100-clkgen.yaml
create mode 100644 Documentation/devicetree/bindings/pinctrl/starfive,jh7100-pinctrl.yaml
create mode 100644 Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
create mode 100644 arch/riscv/boot/dts/starfive/Makefile
create mode 100644 arch/riscv/boot/dts/starfive/jh7100-beaglev-starlight.dts
create mode 100644 arch/riscv/boot/dts/starfive/jh7100.dtsi
create mode 100644 drivers/clk/starfive/Kconfig
create mode 100644 drivers/clk/starfive/Makefile
create mode 100644 drivers/clk/starfive/clk-starfive-jh7100.c
create mode 100644 drivers/pinctrl/pinctrl-starfive.c
create mode 100644 drivers/reset/reset-starfive-jh7100.c
create mode 100644 include/dt-bindings/clock/starfive-jh7100.h
create mode 100644 include/dt-bindings/pinctrl/pinctrl-starfive.h
create mode 100644 include/dt-bindings/reset/starfive-jh7100.h
--
2.33.1
Add compatible string for StarFive JH7100 plic.
Signed-off-by: Emil Renner Berthing <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
.../bindings/interrupt-controller/sifive,plic-1.0.0.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
index 08d5a57ce00f..28b6b17fe4b2 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
@@ -45,6 +45,7 @@ properties:
items:
- enum:
- sifive,fu540-c000-plic
+ - starfive,jh7100-plic
- canaan,k210-plic
- const: sifive,plic-1.0.0
--
2.33.1
Add compatibles for the StarFive JH7100 uarts.
Signed-off-by: Emil Renner Berthing <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
.../devicetree/bindings/serial/snps-dw-apb-uart.yaml | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
index b49fda5e608f..12137fe80acf 100644
--- a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
+++ b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
@@ -40,6 +40,11 @@ properties:
- brcm,bcm11351-dw-apb-uart
- brcm,bcm21664-dw-apb-uart
- const: snps,dw-apb-uart
+ - items:
+ - enum:
+ - starfive,jh7100-hsuart
+ - starfive,jh7100-uart
+ - const: snps,dw-apb-uart
- const: snps,dw-apb-uart
reg:
--
2.33.1
Add a driver for the StarFive JH7100 reset controller.
Signed-off-by: Emil Renner Berthing <[email protected]>
---
MAINTAINERS | 7 +
drivers/reset/Kconfig | 7 +
drivers/reset/Makefile | 1 +
drivers/reset/reset-starfive-jh7100.c | 178 ++++++++++++++++++++++++++
4 files changed, 193 insertions(+)
create mode 100644 drivers/reset/reset-starfive-jh7100.c
diff --git a/MAINTAINERS b/MAINTAINERS
index ed49827dfb29..8274fa4b8430 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17866,6 +17866,13 @@ 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 RESET CONTROLLER DRIVER
+M: Emil Renner Berthing <[email protected]>
+S: Maintained
+F: Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
+F: drivers/reset/reset-starfive-jh7100.c
+F: include/dt-bindings/reset/starfive-jh7100.h
+
STATIC BRANCH/CALL
M: Peter Zijlstra <[email protected]>
M: Josh Poimboeuf <[email protected]>
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index b0056ae5d463..346e66ae690b 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -224,6 +224,13 @@ config RESET_SOCFPGA
This enables the reset driver for the SoCFPGA ARMv7 platforms. This
driver gets initialized early during platform init calls.
+config RESET_STARFIVE_JH7100
+ bool "StarFive JH7100 Reset Driver"
+ depends on SOC_STARFIVE || COMPILE_TEST
+ default SOC_STARFIVE
+ help
+ This enables the reset controller driver for the StarFive JH7100 SoC.
+
config RESET_SUNXI
bool "Allwinner SoCs Reset Driver" if COMPILE_TEST && !ARCH_SUNXI
default ARCH_SUNXI
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 21d46d8869ff..bd0a97be18b5 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_RESET_RZG2L_USBPHY_CTRL) += reset-rzg2l-usbphy-ctrl.o
obj-$(CONFIG_RESET_SCMI) += reset-scmi.o
obj-$(CONFIG_RESET_SIMPLE) += reset-simple.o
obj-$(CONFIG_RESET_SOCFPGA) += reset-socfpga.o
+obj-$(CONFIG_RESET_STARFIVE_JH7100) += reset-starfive-jh7100.o
obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
diff --git a/drivers/reset/reset-starfive-jh7100.c b/drivers/reset/reset-starfive-jh7100.c
new file mode 100644
index 000000000000..a3cbae933ec0
--- /dev/null
+++ b/drivers/reset/reset-starfive-jh7100.c
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Reset driver for the StarFive JH7100 SoC
+ *
+ * Copyright (C) 2021 Emil Renner Berthing <[email protected]>
+ */
+
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/spinlock.h>
+
+#include <dt-bindings/reset/starfive-jh7100.h>
+
+#define BIT_MASK32(x) BIT((x) % 32)
+
+/* register offsets */
+#define JH7100_RESET_ASSERT0 0x00
+#define JH7100_RESET_ASSERT1 0x04
+#define JH7100_RESET_ASSERT2 0x08
+#define JH7100_RESET_ASSERT3 0x0c
+#define JH7100_RESET_STATUS0 0x10
+#define JH7100_RESET_STATUS1 0x14
+#define JH7100_RESET_STATUS2 0x18
+#define JH7100_RESET_STATUS3 0x1c
+
+/*
+ * the registers work like a 32bit bitmap, so writing a 1 to the m'th bit of
+ * the n'th ASSERT register asserts line 32n + m, and writing a 0 deasserts the
+ * same line.
+ * most reset lines have their status inverted so a 0 in the STATUS register
+ * means the line is asserted and a 1 means it's deasserted. a few lines don't
+ * though, so store the expected value of the status registers when all lines
+ * are asserted.
+ */
+static const u32 jh7100_reset_asserted[4] = {
+ /* STATUS0 register */
+ BIT_MASK32(JH7100_RST_U74) |
+ BIT_MASK32(JH7100_RST_VP6_DRESET) |
+ BIT_MASK32(JH7100_RST_VP6_BRESET),
+ /* STATUS1 register */
+ BIT_MASK32(JH7100_RST_HIFI4_DRESET) |
+ BIT_MASK32(JH7100_RST_HIFI4_BRESET),
+ /* STATUS2 register */
+ BIT_MASK32(JH7100_RST_E24),
+ /* STATUS3 register */
+ 0,
+};
+
+struct jh7100_reset {
+ struct reset_controller_dev rcdev;
+ /* protect registers against concurrent read-modify-write */
+ spinlock_t lock;
+ void __iomem *base;
+};
+
+static inline struct jh7100_reset *
+jh7100_reset_from(struct reset_controller_dev *rcdev)
+{
+ return container_of(rcdev, struct jh7100_reset, rcdev);
+}
+
+static int jh7100_reset_update(struct reset_controller_dev *rcdev,
+ unsigned long id, bool assert)
+{
+ struct jh7100_reset *data = jh7100_reset_from(rcdev);
+ unsigned long offset = id / 32;
+ void __iomem *reg_assert = data->base + JH7100_RESET_ASSERT0 + 4 * offset;
+ void __iomem *reg_status = data->base + JH7100_RESET_STATUS0 + 4 * offset;
+ u32 mask = BIT_MASK32(id);
+ u32 done = jh7100_reset_asserted[offset] & mask;
+ unsigned long flags;
+ u32 value;
+ int ret;
+
+ if (!assert)
+ done ^= mask;
+
+ spin_lock_irqsave(&data->lock, flags);
+
+ value = readl(reg_assert);
+ if (assert)
+ value |= mask;
+ else
+ value &= ~mask;
+ writel(value, reg_assert);
+
+ /* if the associated clock is gated, deasserting might otherwise hang forever */
+ ret = readl_poll_timeout_atomic(reg_status, value, (value & mask) == done, 0, 1000);
+
+ spin_unlock_irqrestore(&data->lock, flags);
+ return ret;
+}
+
+static int jh7100_reset_assert(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+ dev_dbg(rcdev->dev, "assert(%lu)\n", id);
+ return jh7100_reset_update(rcdev, id, true);
+}
+
+static int jh7100_reset_deassert(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+ dev_dbg(rcdev->dev, "deassert(%lu)\n", id);
+ return jh7100_reset_update(rcdev, id, false);
+}
+
+static int jh7100_reset_reset(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+ int ret;
+
+ dev_dbg(rcdev->dev, "reset(%lu)\n", id);
+ ret = jh7100_reset_assert(rcdev, id);
+ if (ret)
+ return ret;
+
+ return jh7100_reset_deassert(rcdev, id);
+}
+
+static int jh7100_reset_status(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+ struct jh7100_reset *data = jh7100_reset_from(rcdev);
+ unsigned long offset = id / 32;
+ void __iomem *reg_status = data->base + JH7100_RESET_STATUS0 + 4 * offset;
+ u32 mask = BIT_MASK32(id);
+ int ret = !((readl(reg_status) ^ jh7100_reset_asserted[offset]) & mask);
+
+ dev_dbg(rcdev->dev, "status(%lu) = %d\n", id, ret);
+ return ret;
+}
+
+static const struct reset_control_ops jh7100_reset_ops = {
+ .assert = jh7100_reset_assert,
+ .deassert = jh7100_reset_deassert,
+ .reset = jh7100_reset_reset,
+ .status = jh7100_reset_status,
+};
+
+static int __init jh7100_reset_probe(struct platform_device *pdev)
+{
+ struct jh7100_reset *data;
+
+ data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(data->base))
+ return PTR_ERR(data->base);
+
+ data->rcdev.ops = &jh7100_reset_ops;
+ data->rcdev.owner = THIS_MODULE;
+ data->rcdev.nr_resets = JH7100_RSTN_END;
+ data->rcdev.dev = &pdev->dev;
+ data->rcdev.of_node = pdev->dev.of_node;
+ spin_lock_init(&data->lock);
+
+ return devm_reset_controller_register(&pdev->dev, &data->rcdev);
+}
+
+static const struct of_device_id jh7100_reset_dt_ids[] = {
+ { .compatible = "starfive,jh7100-reset" },
+ { /* sentinel */ }
+};
+
+static struct platform_driver jh7100_reset_driver = {
+ .driver = {
+ .name = "jh7100-reset",
+ .of_match_table = jh7100_reset_dt_ids,
+ .suppress_bind_attrs = true,
+ },
+};
+builtin_platform_driver_probe(jh7100_reset_driver, jh7100_reset_probe);
--
2.33.1
Add compatible string for the StarFive JH7100 clint.
Signed-off-by: Emil Renner Berthing <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
Documentation/devicetree/bindings/timer/sifive,clint.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/timer/sifive,clint.yaml b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
index a35952f48742..8d5f4687add9 100644
--- a/Documentation/devicetree/bindings/timer/sifive,clint.yaml
+++ b/Documentation/devicetree/bindings/timer/sifive,clint.yaml
@@ -25,6 +25,7 @@ properties:
items:
- enum:
- sifive,fu540-c000-clint
+ - starfive,jh7100-clint
- canaan,k210-clint
- const: sifive,clint0
--
2.33.1
Add a combined pinctrl and GPIO driver for the JH7100 RISC-V SoC by
StarFive Ltd. This is a test chip for their upcoming JH7110 SoC, which
is said to feature only minor changes to these pinctrl/GPIO parts.
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 pin muxing
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.
Datasheet: https://github.com/starfive-tech/JH7100_Docs/blob/main/JH7100%20Data%20Sheet%20V01.01.04-EN%20(4-21-2021).pdf
Co-developed-by: Huan Feng <[email protected]>
Signed-off-by: Huan Feng <[email protected]>
Signed-off-by: Emil Renner Berthing <[email protected]>
Co-developed-by: Drew Fustini <[email protected]>
Signed-off-by: Drew Fustini <[email protected]>
---
MAINTAINERS | 8 +
drivers/pinctrl/Kconfig | 17 +
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-starfive.c | 1353 ++++++++++++++++++++++++++++
4 files changed, 1379 insertions(+)
create mode 100644 drivers/pinctrl/pinctrl-starfive.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 8274fa4b8430..73c987480a44 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17866,6 +17866,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 <[email protected]>
+L: [email protected]
+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 <[email protected]>
S: Maintained
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 31921108e456..b298cf32804a 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -265,6 +265,23 @@ config PINCTRL_ST
select PINCONF
select GPIOLIB_IRQCHIP
+config PINCTRL_STARFIVE
+ tristate "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..15e5a672ffff
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-starfive.c
@@ -0,0 +1,1353 @@
+// 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 <[email protected]>
+ */
+
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/gpio/driver.h>
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/spinlock.h>
+
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.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 the JH7100 data sheet:
+ * https://github.com/starfive-tech/JH7100_Docs
+ */
+#define NR_GPIOS 64
+
+/*
+ * Global enable for GPIO interrupts. If bit 0 is set to 1 the GPIO interrupts
+ * are enabled. If set to 0 the GPIO interrupts are disabled.
+ */
+#define GPIOEN 0x000
+
+/*
+ * The following 32-bit registers come in pairs, but only the offset of the
+ * first register is defined. The first controls (interrupts for) GPIO 0-31 and
+ * the second GPIO 32-63.
+ */
+
+/*
+ * Interrupt Type. If set to 1 the interrupt is edge-triggered. If set to 0 the
+ * interrupt is level-triggered.
+ */
+#define GPIOIS 0x010
+
+/*
+ * Edge-Trigger Interrupt Type. If set to 1 the interrupt gets triggered on
+ * both positive and negative edges. If set to 0 the interrupt is triggered by a
+ * single edge.
+ */
+#define GPIOIBE 0x018
+
+/*
+ * Interrupt Trigger Polarity. If set to 1 the interrupt is triggered on a
+ * rising edge (edge-triggered) or high level (level-triggered). If set to 0 the
+ * interrupt is triggered on a falling edge (edge-triggered) or low level
+ * (level-triggered).
+ */
+#define GPIOIEV 0x020
+
+/*
+ * Interrupt Mask. If set to 1 the interrupt is enabled (unmasked). If set to 0
+ * the interrupt is disabled (masked). Note that the current documentation is
+ * wrong and says the exct opposite of this.
+ */
+#define GPIOIE 0x028
+
+/*
+ * Clear Edge-Triggered Interrupts. Write a 1 to clear the edge-triggered
+ * interrupt.
+ */
+#define GPIOIC 0x030
+
+/*
+ * Edge-Triggered Interrupt Status. A 1 means the configured edge was detected.
+ */
+#define GPIORIS 0x038
+
+/*
+ * Interrupt Status after Masking. A 1 means the configured edge or level was
+ * detected and not masked.
+ */
+#define GPIOMIS 0x040
+
+/*
+ * Data Value. Dynamically reflects the value of the GPIO pin. If 1 the pin is
+ * a digital 1 and if 0 the pin is a digital 0.
+ */
+#define GPIODIN 0x048
+
+/*
+ * From the data sheet section 12.2, there are 64 32-bit output data registers
+ * and 64 output enable registers. Output data and output enable registers for
+ * a given GPIO are contiguous. Eg. GPO0_DOUT_CFG is 0x50 and GPO0_DOEN_CFG is
+ * 0x54 while GPO1_DOUT_CFG is 0x58 and GPO1_DOEN_CFG is 0x5c. The stride
+ * between GPIO registers is effectively 8, thus: GPOn_DOUT_CFG is 0x50 + 8n
+ * and GPOn_DOEN_CFG is 0x54 + 8n.
+ */
+#define GPON_DOUT_CFG 0x050
+#define GPON_DOEN_CFG 0x054
+
+/*
+ * 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 GPI_CFG_OFFSET 0x250
+
+/*
+ * Pad Control Bits. There are 16 pad control bits for each pin located in 103
+ * 32-bit registers controlling PAD_GPIO[0] to PAD_GPIO[63] followed by
+ * PAD_FUNC_SHARE[0] to PAD_FUNC_SHARE[141]. Odd numbered pins use the upper 16
+ * bit of each register.
+ */
+#define PAD_SLEW_RATE_MASK GENMASK(11, 9)
+#define PAD_SLEW_RATE_POS 9
+#define PAD_BIAS_STRONG_PULL_UP BIT(8)
+#define PAD_INPUT_ENABLE BIT(7)
+#define PAD_INPUT_SCHMITT_ENABLE BIT(6)
+#define PAD_BIAS_DISABLE BIT(5)
+#define PAD_BIAS_PULL_DOWN BIT(4)
+#define PAD_BIAS_MASK \
+ (PAD_BIAS_STRONG_PULL_UP | \
+ PAD_BIAS_DISABLE | \
+ PAD_BIAS_PULL_DOWN)
+#define PAD_DRIVE_STRENGTH_MASK GENMASK(3, 0)
+#define PAD_DRIVE_STRENGTH_POS 0
+
+/*
+ * From Section 11, the IO_PADSHARE_SEL register can be programmed to select
+ * one of seven pre-defined multiplexed signal groups on PAD_FUNC_SHARE and
+ * PAD_GPIO pads. This is a global setting.
+ */
+#define IO_PADSHARE_SEL 0x1a0
+
+/*
+ * This just needs to be some number such that when
+ * sfp->gpio.pin_base = PAD_INVALID_GPIO then
+ * starfive_pin_to_gpio(sfp, validpin) is never a valid GPIO number.
+ * That is it should underflow and return something >= NR_GPIOS.
+ */
+#define PAD_INVALID_GPIO 0x10000
+
+/*
+ * The packed pinmux values from the device tree look like this:
+ *
+ * | 31 - 24 | 23 - 16 | 15 - 8 | 7 | 6 | 5 - 0 |
+ * | dout | doen | din | dout rev | doen rev | gpio nr |
+ *
+ * ..but the GPOn_DOUT_CFG and GPOn_DOEN_CFG registers look like this:
+ *
+ * | 31 | 30 - 8 | 7 - 0 |
+ * | dout/doen rev | unused | dout/doen |
+ */
+static unsigned int starfive_pinmux_to_gpio(u32 v)
+{
+ return v & (NR_GPIOS - 1);
+}
+
+static u32 starfive_pinmux_to_dout(u32 v)
+{
+ return ((v & BIT(7)) << (31 - 7)) | ((v >> 24) & GENMASK(7, 0));
+}
+
+static u32 starfive_pinmux_to_doen(u32 v)
+{
+ return ((v & BIT(6)) << (31 - 6)) | ((v >> 16) & GENMASK(7, 0));
+}
+
+static u32 starfive_pinmux_to_din(u32 v)
+{
+ return (v >> 8) & GENMASK(7, 0);
+}
+
+/*
+ * The maximum GPIO output current depends on the chosen drive strength:
+ *
+ * DS: 0 1 2 3 4 5 6 7
+ * mA: 14.2 21.2 28.2 35.2 42.2 49.1 56.0 62.8
+ *
+ * After rounding that is 7*DS + 14 mA
+ */
+static u32 starfive_drive_strength_to_max_mA(u16 ds)
+{
+ return 7 * ds + 14;
+}
+
+static u16 starfive_drive_strength_from_max_mA(u32 i)
+{
+ return (clamp(i, 14U, 63U) - 14) / 7;
+}
+
+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 inline struct device *starfive_dev(const struct starfive_pinctrl *sfp)
+{
+ return sfp->gc.parent;
+}
+
+static inline unsigned int starfive_pin_to_gpio(const struct starfive_pinctrl *sfp,
+ unsigned int pin)
+{
+ return pin - sfp->gpios.pin_base;
+}
+
+static inline 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_irq_data(struct irq_data *d)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+
+ return container_of(gc, struct starfive_pinctrl, gc);
+}
+
+static struct starfive_pinctrl *starfive_from_irq_desc(struct irq_desc *desc)
+{
+ struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+
+ return container_of(gc, struct starfive_pinctrl, gc);
+}
+
+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 >= NR_GPIOS)
+ return;
+
+ reg = sfp->base + GPON_DOUT_CFG + 8 * gpio;
+ dout = readl_relaxed(reg + 0x000);
+ doen = readl_relaxed(reg + 0x004);
+
+ seq_printf(s, "dout=%lu%s doen=%lu%s",
+ dout & GENMASK(7, 0), (dout & BIT(31)) ? "r" : "",
+ doen & GENMASK(7, 0), (doen & BIT(31)) ? "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) {
+ int npinmux = of_property_count_u32_elems(child, "pinmux");
+ int npins = of_property_count_u32_elems(child, "pins");
+
+ if (npinmux > 0 && npins > 0) {
+ dev_err(dev, "invalid pinctrl group %pOFn.%pOFn: both pinmux and pins set\n",
+ np, child);
+ of_node_put(child);
+ return -EINVAL;
+ }
+ if (npinmux == 0 && npins == 0) {
+ dev_err(dev, "invalid pinctrl group %pOFn.%pOFn: neither pinmux nor pins set\n",
+ np, child);
+ of_node_put(child);
+ return -EINVAL;
+ }
+
+ if (npinmux > 0)
+ nmaps += 2;
+ else
+ nmaps += 1;
+ ngroups += 1;
+ }
+
+ pgnames = devm_kcalloc(dev, ngroups, sizeof(*pgnames), GFP_KERNEL);
+ if (!pgnames)
+ return -ENOMEM;
+
+ map = kcalloc(nmaps, sizeof(*map), GFP_KERNEL);
+ if (!map)
+ return -ENOMEM;
+
+ nmaps = 0;
+ ngroups = 0;
+ for_each_child_of_node(np, child) {
+ int npins;
+ int i;
+
+ grpname = devm_kasprintf(dev, GFP_KERNEL, "%pOFn.%pOFn", np, child);
+ if (!grpname) {
+ ret = -ENOMEM;
+ goto put_child;
+ }
+
+ pgnames[ngroups++] = grpname;
+
+ if ((npins = of_property_count_u32_elems(child, "pinmux")) > 0) {
+ pins = devm_kcalloc(dev, npins, sizeof(*pins), GFP_KERNEL);
+ if (!pins) {
+ ret = -ENOMEM;
+ goto put_child;
+ }
+
+ pinmux = devm_kcalloc(dev, npins, sizeof(*pinmux), GFP_KERNEL);
+ if (!pinmux) {
+ ret = -ENOMEM;
+ goto put_child;
+ }
+
+ ret = of_property_read_u32_array(child, "pinmux", pinmux, npins);
+ if (ret)
+ goto put_child;
+
+ for (i = 0; i < npins; i++) {
+ unsigned int gpio = starfive_pinmux_to_gpio(pinmux[i]);
+
+ pins[i] = starfive_gpio_to_pin(sfp, gpio);
+ }
+
+ 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 ((npins = of_property_count_u32_elems(child, "pins")) > 0) {
+ pins = devm_kcalloc(dev, npins, sizeof(*pins), GFP_KERNEL);
+ if (!pins) {
+ ret = -ENOMEM;
+ goto put_child;
+ }
+
+ pinmux = NULL;
+
+ for (i = 0; i < npins; i++) {
+ u32 v;
+
+ ret = of_property_read_u32_index(child, "pins", i, &v);
+ if (ret)
+ goto put_child;
+ pins[i] = v;
+ }
+ } else {
+ ret = -EINVAL;
+ goto put_child;
+ }
+
+ ret = pinctrl_generic_add_group(pctldev, grpname, pins, npins, pinmux);
+ if (ret < 0) {
+ dev_err(dev, "error adding group %s: %d\n", grpname, ret);
+ goto put_child;
+ }
+
+ ret = pinconf_generic_parse_dt_config(child, pctldev,
+ &map[nmaps].data.configs.configs,
+ &map[nmaps].data.configs.num_configs);
+ if (ret) {
+ dev_err(dev, "error parsing pin config of group %s: %d\n",
+ grpname, ret);
+ 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 %s: %d\n", np->name, ret);
+ goto free_map;
+ }
+
+ *maps = map;
+ *num_maps = nmaps;
+ return 0;
+
+put_child:
+ of_node_put(child);
+free_map:
+ pinctrl_utils_free_map(pctldev, map, nmaps);
+ 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 (!group)
+ return -EINVAL;
+
+ pinmux = group->data;
+ for (i = 0; i < group->num_pins; i++) {
+ u32 v = pinmux[i];
+ unsigned int gpio = starfive_pinmux_to_gpio(v);
+ u32 dout = starfive_pinmux_to_dout(v);
+ u32 doen = starfive_pinmux_to_doen(v);
+ u32 din = starfive_pinmux_to_din(v);
+ void __iomem *reg_dout;
+ void __iomem *reg_doen;
+ void __iomem *reg_din;
+ unsigned long flags;
+
+ dev_dbg(dev, "GPIO%u: dout=0x%x doen=0x%x din=0x%x\n",
+ gpio, dout, doen, din);
+
+ reg_dout = sfp->base + GPON_DOUT_CFG + 8 * gpio;
+ reg_doen = sfp->base + GPON_DOEN_CFG + 8 * gpio;
+ if (din != GPI_NONE)
+ reg_din = sfp->base + GPI_CFG_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);
+ int shift = 16 * (pin % 2);
+
+ return readl_relaxed(reg) >> shift;
+}
+
+static void starfive_padctl_rmw(struct starfive_pinctrl *sfp,
+ unsigned int pin,
+ u16 _mask, u16 _value)
+{
+ void __iomem *reg = sfp->padctl + 4 * (pin / 2);
+ int shift = 16 * (pin % 2);
+ u32 mask = (u32)_mask << shift;
+ u32 value = (u32)_value << shift;
+ unsigned long flags;
+
+ dev_dbg(starfive_dev(sfp), "padctl_rmw(%u, 0x%03x, 0x%03x)\n", pin, _mask, _value);
+
+ 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[] = {
+ PCONFDUMP(PIN_CONFIG_STARFIVE_STRONG_PULL_UP, "input bias strong pull-up", NULL, false),
+};
+
+static_assert(ARRAY_SIZE(starfive_pinconf_custom_conf_items) ==
+ ARRAY_SIZE(starfive_pinconf_custom_params));
+#else
+#define starfive_pinconf_custom_conf_items NULL
+#endif
+
+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_to_max_mA(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 (!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 (!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]);
+
+ 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:
+ mask |= PAD_DRIVE_STRENGTH_MASK;
+ value = (value & ~PAD_DRIVE_STRENGTH_MASK) |
+ starfive_drive_strength_from_max_mA(arg);
+ 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 = container_of(gc, struct starfive_pinctrl, gc);
+ void __iomem *doen = sfp->base + GPON_DOEN_CFG + 8 * gpio;
+
+ /* return GPIO_LINE_DIRECTION_OUT (0) only if doen == GPO_ENABLE (0) */
+ return readl_relaxed(doen) != GPO_ENABLE;
+}
+
+static int starfive_gpio_direction_input(struct gpio_chip *gc,
+ unsigned int gpio)
+{
+ struct starfive_pinctrl *sfp = container_of(gc, struct starfive_pinctrl, gc);
+ void __iomem *doen = sfp->base + GPON_DOEN_CFG + 8 * gpio;
+ unsigned long flags;
+
+ /* 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, doen);
+ 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 = container_of(gc, struct starfive_pinctrl, gc);
+ void __iomem *dout = sfp->base + GPON_DOUT_CFG + 8 * gpio;
+ void __iomem *doen = sfp->base + GPON_DOEN_CFG + 8 * gpio;
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&sfp->lock, flags);
+ writel_relaxed(value, dout);
+ writel_relaxed(GPO_ENABLE, doen);
+ 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 = container_of(gc, struct starfive_pinctrl, gc);
+ void __iomem *din = sfp->base + GPIODIN + 4 * (gpio / 32);
+
+ return !!(readl_relaxed(din) & BIT(gpio % 32));
+}
+
+static void starfive_gpio_set(struct gpio_chip *gc, unsigned int gpio,
+ int value)
+{
+ struct starfive_pinctrl *sfp = container_of(gc, struct starfive_pinctrl, gc);
+ void __iomem *dout = sfp->base + GPON_DOUT_CFG + 8 * gpio;
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&sfp->lock, flags);
+ writel_relaxed(value, dout);
+ 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 = container_of(gc, struct starfive_pinctrl, 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 = container_of(gc, struct starfive_pinctrl, 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 = NR_GPIOS;
+ 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);
+ void __iomem *ic = sfp->base + GPIOIC + 4 * (gpio / 32);
+ u32 mask = BIT(gpio % 32);
+ unsigned long flags;
+
+ 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);
+ void __iomem *ie = sfp->base + GPIOIE + 4 * (gpio / 32);
+ u32 mask = BIT(gpio % 32);
+ unsigned long flags;
+ u32 value;
+
+ raw_spin_lock_irqsave(&sfp->lock, flags);
+ value = readl_relaxed(ie) & ~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);
+ void __iomem *ie = sfp->base + GPIOIE + 4 * (gpio / 32);
+ void __iomem *ic = sfp->base + GPIOIC + 4 * (gpio / 32);
+ u32 mask = BIT(gpio % 32);
+ unsigned long flags;
+ u32 value;
+
+ raw_spin_lock_irqsave(&sfp->lock, flags);
+ value = readl_relaxed(ie) & ~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);
+ void __iomem *ie = sfp->base + GPIOIE + 4 * (gpio / 32);
+ u32 mask = BIT(gpio % 32);
+ unsigned long flags;
+ u32 value;
+
+ raw_spin_lock_irqsave(&sfp->lock, flags);
+ value = readl_relaxed(ie) | 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);
+ void __iomem *base = sfp->base + 4 * (gpio / 32);
+ u32 mask = BIT(gpio % 32);
+ u32 irq_type, edge_both, polarity;
+ unsigned long flags;
+
+ if (trigger & IRQ_TYPE_EDGE_BOTH)
+ irq_set_handler_locked(d, handle_edge_irq);
+ else if (trigger & IRQ_TYPE_LEVEL_MASK)
+ irq_set_handler_locked(d, handle_level_irq);
+
+ switch (trigger) {
+ case IRQ_TYPE_EDGE_RISING:
+ irq_type = mask; /* 1: edge triggered */
+ edge_both = 0; /* 0: single edge */
+ polarity = mask; /* 1: rising edge */
+ break;
+ case IRQ_TYPE_EDGE_FALLING:
+ irq_type = mask; /* 1: edge triggered */
+ edge_both = 0; /* 0: single edge */
+ polarity = 0; /* 0: falling edge */
+ break;
+ case IRQ_TYPE_EDGE_BOTH:
+ irq_type = mask; /* 1: edge triggered */
+ edge_both = mask; /* 1: both edges */
+ polarity = 0; /* 0: ignored */
+ break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ irq_type = 0; /* 0: level triggered */
+ edge_both = 0; /* 0: ignored */
+ polarity = mask; /* 1: high level */
+ break;
+ case IRQ_TYPE_LEVEL_LOW:
+ 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 -EINVAL;
+ }
+
+ raw_spin_lock_irqsave(&sfp->lock, flags);
+ irq_type |= readl_relaxed(base + GPIOIS) & ~mask;
+ writel_relaxed(irq_type, base + GPIOIS);
+ edge_both |= readl_relaxed(base + GPIOIBE) & ~mask;
+ writel_relaxed(edge_both, base + GPIOIBE);
+ polarity |= readl_relaxed(base + GPIOIEV) & ~mask;
+ writel_relaxed(polarity, base + GPIOIEV);
+ 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 + GPIOMIS + 0);
+ for_each_set_bit(pin, &mis, 32)
+ generic_handle_domain_irq(sfp->gc.irq.domain, pin);
+
+ mis = readl_relaxed(sfp->base + GPIOMIS + 4);
+ 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 = container_of(gc, struct starfive_pinctrl, gc);
+
+ /* mask all GPIO interrupts */
+ writel(0, sfp->base + GPIOIE + 0);
+ writel(0, sfp->base + GPIOIE + 4);
+ /* clear edge interrupt flags */
+ writel(~0U, sfp->base + GPIOIC + 0);
+ writel(~0U, sfp->base + GPIOIC + 4);
+ /* enable GPIO interrupts */
+ writel(1, sfp->base + GPIOEN);
+ return 0;
+}
+
+static void starfive_disable_clock(void *data)
+{
+ clk_disable_unprepare(data);
+}
+
+static int 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))
+ return dev_err_probe(dev, PTR_ERR(clk), "could not get clock\n");
+
+ rst = devm_reset_control_get_exclusive(dev, NULL);
+ if (IS_ERR(rst))
+ return dev_err_probe(dev, PTR_ERR(rst), "could not get reset\n");
+
+ ret = clk_prepare_enable(clk);
+ if (ret)
+ return dev_err_probe(dev, ret, "could not enable clock\n");
+
+ ret = devm_add_action_or_reset(dev, starfive_disable_clock, clk);
+ if (ret)
+ return ret;
+
+ ret = reset_control_deassert(rst);
+ if (ret)
+ return dev_err_probe(dev, ret, "could not deassert resetd\n");
+
+ 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)
+ return dev_err_probe(dev, ret, "could not register pinctrl driver\n");
+
+ if (!of_property_read_u32(dev->of_node, "starfive,signal-group", &value)) {
+ if (value > 6)
+ return dev_err_probe(dev, -EINVAL, "invalid signal group %u\n", value);
+ writel(value, sfp->padctl + IO_PADSHARE_SEL);
+ }
+
+ value = readl(sfp->padctl + IO_PADSHARE_SEL);
+ switch (value) {
+ case 0:
+ sfp->gpios.pin_base = PAD_INVALID_GPIO;
+ 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:
+ return dev_err_probe(dev, -EINVAL, "invalid signal group %u\n", value);
+ }
+
+ sfp->gc.label = dev_name(dev);
+ 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 = NR_GPIOS;
+
+ 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.num_parents = 1;
+ sfp->gc.irq.parents = devm_kcalloc(dev, sfp->gc.irq.num_parents,
+ sizeof(*sfp->gc.irq.parents), GFP_KERNEL);
+ if (!sfp->gc.irq.parents)
+ return -ENOMEM;
+ 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)
+ return dev_err_probe(dev, ret, "could not register gpiochip\n");
+
+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 <[email protected]>");
+MODULE_LICENSE("GPL v2");
--
2.33.1
+Cc: Yury (bitmap expert)
On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <[email protected]> wrote:
>
> Add a driver for the StarFive JH7100 reset controller.
...
> +#define BIT_MASK32(x) BIT((x) % 32)
Possible namespace collision.
...
> +/*
> + * the registers work like a 32bit bitmap, so writing a 1 to the m'th bit of
> + * the n'th ASSERT register asserts line 32n + m, and writing a 0 deasserts the
> + * same line.
> + * most reset lines have their status inverted so a 0 in the STATUS register
> + * means the line is asserted and a 1 means it's deasserted. a few lines don't
> + * though, so store the expected value of the status registers when all lines
> + * are asserted.
> + */
Besides missing capitalization, if it sounds like bitmap, use bitmap.
I have checked DT definitions and it seems you don't even need the
BIT_MASK() macro,
> +static const u32 jh7100_reset_asserted[4] = {
> + /* STATUS0 register */
> + BIT_MASK32(JH7100_RST_U74) |
> + BIT_MASK32(JH7100_RST_VP6_DRESET) |
> + BIT_MASK32(JH7100_RST_VP6_BRESET),
> + /* STATUS1 register */
> + BIT_MASK32(JH7100_RST_HIFI4_DRESET) |
> + BIT_MASK32(JH7100_RST_HIFI4_BRESET),
> + /* STATUS2 register */
> + BIT_MASK32(JH7100_RST_E24),
> + /* STATUS3 register */
> + 0,
> +};
Yury, do we have any clever (clean) way to initialize a bitmap with
particular bits so that it will be a constant from the beginning? If
no, any suggestion what we can provide to such users?
...
> + dev_dbg(rcdev->dev, "reset(%lu)\n", id);
These debug messages are useless since one should use ftrace facility instead,
--
With Best Regards,
Andy Shevchenko
On Tue, 2 Nov 2021 at 20:43, Andy Shevchenko <[email protected]> wrote:
> +Cc: Yury (bitmap expert)
>
> On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <[email protected]> wrote:
> >
> > Add a driver for the StarFive JH7100 reset controller.
>
> ...
>
> > +#define BIT_MASK32(x) BIT((x) % 32)
>
> Possible namespace collision.
>
> ...
>
> > +/*
> > + * the registers work like a 32bit bitmap, so writing a 1 to the m'th bit of
> > + * the n'th ASSERT register asserts line 32n + m, and writing a 0 deasserts the
> > + * same line.
> > + * most reset lines have their status inverted so a 0 in the STATUS register
> > + * means the line is asserted and a 1 means it's deasserted. a few lines don't
> > + * though, so store the expected value of the status registers when all lines
> > + * are asserted.
> > + */
>
> Besides missing capitalization,
I'm confused. it was you who wanted all comments to capitalized the same..
64bi
if it sounds like bitmap, use bitmap.
> I have checked DT definitions and it seems you don't even need the
> BIT_MASK() macro,
>
> > +static const u32 jh7100_reset_asserted[4] = {
> > + /* STATUS0 register */
> > + BIT_MASK32(JH7100_RST_U74) |
> > + BIT_MASK32(JH7100_RST_VP6_DRESET) |
> > + BIT_MASK32(JH7100_RST_VP6_BRESET),
> > + /* STATUS1 register */
> > + BIT_MASK32(JH7100_RST_HIFI4_DRESET) |
> > + BIT_MASK32(JH7100_RST_HIFI4_BRESET),
> > + /* STATUS2 register */
> > + BIT_MASK32(JH7100_RST_E24),
> > + /* STATUS3 register */
> > + 0,
> > +};
>
> Yury, do we have any clever (clean) way to initialize a bitmap with
> particular bits so that it will be a constant from the beginning? If
> no, any suggestion what we can provide to such users?
The problem is, that even if we could initialize this without the
monstrosity in our last conversation a 64bit bitmap would still
produce worse code. As it is now it's simply a 32bit load and mask
with index and mask already calculated for the registers. In the
status callback the mask can even be folded into the register read
mask. With a 64bit bitmap you'd need to calculate new 64bit index and
masks, and then conditionally shift the bits into position.
If this reflection of the 32bit registers bothers you that much we
could alternatively do
static bool jh7100_reset_inverted(unsigned int idx)
{
switch (idx) {
case JH7100_RST_U74:
case JH7100_RST_VP6_DRESET:
..
return false;
}
return true;
}
It'd still produce worse code, but at least it would be readable.
/Emil
> ...
>
> > + dev_dbg(rcdev->dev, "reset(%lu)\n", id);
>
> These debug messages are useless since one should use ftrace facility instead,
>
> --
> With Best Regards,
> Andy Shevchenko
On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <[email protected]> wrote:
>
> Add a combined pinctrl and GPIO driver for the JH7100 RISC-V SoC by
> StarFive Ltd. This is a test chip for their upcoming JH7110 SoC, which
> is said to feature only minor changes to these pinctrl/GPIO parts.
>
> 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 pin muxing
> 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.
...
> + depends on OF
So this descreases test coverage.
Linus, can we provide a necessary stub so we may drop this dependency?
...
> +static inline struct device *starfive_dev(const struct starfive_pinctrl *sfp)
> +{
> + return sfp->gc.parent;
> +}
> +
This seems useless helper. You may do what it's doing just in place.
It will save 5 LOCs.
...
> +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 >= NR_GPIOS)
> + return;
Dead code?
> + reg = sfp->base + GPON_DOUT_CFG + 8 * gpio;
> + dout = readl_relaxed(reg + 0x000);
> + doen = readl_relaxed(reg + 0x004);
> +
> + seq_printf(s, "dout=%lu%s doen=%lu%s",
> + dout & GENMASK(7, 0), (dout & BIT(31)) ? "r" : "",
> + doen & GENMASK(7, 0), (doen & BIT(31)) ? "r" : "");
> +}
...
> + 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;
Reversed xmas tree order?
> + int nmaps;
> + int ngroups;
> + int ret;
...
> +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 (!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]);
> +
> + 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:
> + mask |= PAD_DRIVE_STRENGTH_MASK;
> + value = (value & ~PAD_DRIVE_STRENGTH_MASK) |
> + starfive_drive_strength_from_max_mA(arg);
> + 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;
> +}
...
> +static int starfive_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct starfive_pinctrl *sfp = container_of(gc, struct starfive_pinctrl, gc);
> + void __iomem *doen = sfp->base + GPON_DOEN_CFG + 8 * gpio;
> +
> + /* return GPIO_LINE_DIRECTION_OUT (0) only if doen == GPO_ENABLE (0) */
> + return readl_relaxed(doen) != GPO_ENABLE;
I believe the idea was to return the predefined values for the direction.
> +}
...
> +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);
> + void __iomem *base = sfp->base + 4 * (gpio / 32);
> + u32 mask = BIT(gpio % 32);
> + u32 irq_type, edge_both, polarity;
> + unsigned long flags;
> +
> + if (trigger & IRQ_TYPE_EDGE_BOTH)
> + irq_set_handler_locked(d, handle_edge_irq);
> + else if (trigger & IRQ_TYPE_LEVEL_MASK)
> + irq_set_handler_locked(d, handle_level_irq);
Usually we don't assign this twice, so it should be after the switch.
> + switch (trigger) {
> + case IRQ_TYPE_EDGE_RISING:
> + irq_type = mask; /* 1: edge triggered */
> + edge_both = 0; /* 0: single edge */
> + polarity = mask; /* 1: rising edge */
> + break;
> + case IRQ_TYPE_EDGE_FALLING:
> + irq_type = mask; /* 1: edge triggered */
> + edge_both = 0; /* 0: single edge */
> + polarity = 0; /* 0: falling edge */
> + break;
> + case IRQ_TYPE_EDGE_BOTH:
> + irq_type = mask; /* 1: edge triggered */
> + edge_both = mask; /* 1: both edges */
> + polarity = 0; /* 0: ignored */
> + break;
> + case IRQ_TYPE_LEVEL_HIGH:
> + irq_type = 0; /* 0: level triggered */
> + edge_both = 0; /* 0: ignored */
> + polarity = mask; /* 1: high level */
> + break;
> + case IRQ_TYPE_LEVEL_LOW:
> + 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);
Why? You have it already in ->probe(), what's the point?
> + return -EINVAL;
> + }
> +
> + raw_spin_lock_irqsave(&sfp->lock, flags);
> + irq_type |= readl_relaxed(base + GPIOIS) & ~mask;
> + writel_relaxed(irq_type, base + GPIOIS);
> + edge_both |= readl_relaxed(base + GPIOIBE) & ~mask;
> + writel_relaxed(edge_both, base + GPIOIBE);
> + polarity |= readl_relaxed(base + GPIOIEV) & ~mask;
> + writel_relaxed(polarity, base + GPIOIEV);
> + raw_spin_unlock_irqrestore(&sfp->lock, flags);
> + return 0;
> +}
...
> + ret = reset_control_deassert(rst);
> + if (ret)
> + return dev_err_probe(dev, ret, "could not deassert resetd\n");
> + ret = devm_pinctrl_register_and_init(dev, &starfive_desc, sfp, &sfp->pctl);
> + if (ret)
I don't see who will assert reset here.
> + return dev_err_probe(dev, ret, "could not register pinctrl driver\n");
...
> + switch (value) {
> + case 0:
> + sfp->gpios.pin_base = PAD_INVALID_GPIO;
> + 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:
Ditto.
> + return dev_err_probe(dev, -EINVAL, "invalid signal group %u\n", value);
> + }
...
> + ret = devm_gpiochip_add_data(dev, &sfp->gc, sfp);
> + if (ret)
Ditto.
> + return dev_err_probe(dev, ret, "could not register gpiochip\n");
> +
> +done:
> + return pinctrl_enable(sfp->pctl);
Ditto.
And better to use label name like following
out_pinctrl_enable:
--
With Best Regards,
Andy Shevchenko
On Tue, Nov 2, 2021 at 10:02 PM Andy Shevchenko
<[email protected]> wrote:
> On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <[email protected]> wrote:
...
> > +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 (!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]);
> > +
> > + 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:
> > + mask |= PAD_DRIVE_STRENGTH_MASK;
> > + value = (value & ~PAD_DRIVE_STRENGTH_MASK) |
> > + starfive_drive_strength_from_max_mA(arg);
> > + 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;
> > +}
Linus any comments on this code (sorry if I missed your reply)? The
idea behind above is to skip all settings from the same category and
apply only the last one, e.g. if we have "bias set to X", ..., "bias
disable", ..., "bias set to Y", the hardware will see only the last
operation, i.e. "bias set to Y". I think it may not be the best
approach (theoretically?) since the hardware definitely may behave
differently on the other side in case of such series of the
configurations (yes, I have seen some interesting implementations of
the touchpad / touchscreen GPIOs that may be affected).
--
With Best Regards,
Andy Shevchenko
On Tue, Nov 2, 2021 at 9:59 PM Emil Renner Berthing <[email protected]> wrote:
> On Tue, 2 Nov 2021 at 20:43, Andy Shevchenko <[email protected]> wrote:
> > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <[email protected]> wrote:
...
> > > +/*
> > > + * the registers work like a 32bit bitmap, so writing a 1 to the m'th bit of
> > > + * the n'th ASSERT register asserts line 32n + m, and writing a 0 deasserts the
> > > + * same line.
> > > + * most reset lines have their status inverted so a 0 in the STATUS register
> > > + * means the line is asserted and a 1 means it's deasserted. a few lines don't
> > > + * though, so store the expected value of the status registers when all lines
> > > + * are asserted.
> > > + */
> >
> > Besides missing capitalization,
>
> I'm confused. it was you who wanted all comments to capitalized the same..
Yes and there are two types of the comments, one-liners and
multi-line. In multi-line you usually use proper English grammar,
where capitalization means what it means. For the one-liners just
choose either small letters or capital letters to start them with.
> 64bi
Something is missing here.
> if it sounds like bitmap, use bitmap.
> > I have checked DT definitions and it seems you don't even need the
> > BIT_MASK() macro,
> >
> > > +static const u32 jh7100_reset_asserted[4] = {
> > > + /* STATUS0 register */
> > > + BIT_MASK32(JH7100_RST_U74) |
> > > + BIT_MASK32(JH7100_RST_VP6_DRESET) |
> > > + BIT_MASK32(JH7100_RST_VP6_BRESET),
> > > + /* STATUS1 register */
> > > + BIT_MASK32(JH7100_RST_HIFI4_DRESET) |
> > > + BIT_MASK32(JH7100_RST_HIFI4_BRESET),
> > > + /* STATUS2 register */
> > > + BIT_MASK32(JH7100_RST_E24),
> > > + /* STATUS3 register */
> > > + 0,
> > > +};
> >
> > Yury, do we have any clever (clean) way to initialize a bitmap with
> > particular bits so that it will be a constant from the beginning? If
> > no, any suggestion what we can provide to such users?
>
> The problem is, that even if we could initialize this without the
> monstrosity in our last conversation a 64bit bitmap would still
> produce worse code. As it is now it's simply a 32bit load and mask
> with index and mask already calculated for the registers. In the
> status callback the mask can even be folded into the register read
> mask. With a 64bit bitmap you'd need to calculate new 64bit index and
> masks, and then conditionally shift the bits into position.
Why? You may use 8 byte IO (writeq() / readq() or their relaxed versions), no?
...
> If this reflection of the 32bit registers bothers you that much
What bothers me is hidden endianess issues (yeah, here it might be
theoretical, but consider that somebody will look at your code and use
it as the best example ever).
--
With Best Regards,
Andy Shevchenko
On Tue, 2 Nov 2021 at 21:02, Andy Shevchenko <[email protected]> wrote:
> On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <[email protected]> wrote:
> > Add a combined pinctrl and GPIO driver for the JH7100 RISC-V SoC by
> > StarFive Ltd. This is a test chip for their upcoming JH7110 SoC, which
> > is said to feature only minor changes to these pinctrl/GPIO parts.
> >
> > 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 pin muxing
> > 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.
>
> ...
>
> > + depends on OF
>
> So this descreases test coverage.
> Linus, can we provide a necessary stub so we may drop this dependency?
>
> ...
>
> > +static inline struct device *starfive_dev(const struct starfive_pinctrl *sfp)
> > +{
> > + return sfp->gc.parent;
> > +}
> > +
>
> This seems useless helper. You may do what it's doing just in place.
> It will save 5 LOCs.
I don't mind removing it, I just think it's easier to read when we're
explicit that all we want is a dev pointer, and we don't suddenly need
to know the parent of the gpio chip in all the pinmux/pinconf
callbacks.
> > +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 >= NR_GPIOS)
> > + return;
>
> Dead code?
No, this function is called for all 206 configurable pins, but only 64
of them are gpio pins. Which ones depend on the signal group.
> > + reg = sfp->base + GPON_DOUT_CFG + 8 * gpio;
> > + dout = readl_relaxed(reg + 0x000);
> > + doen = readl_relaxed(reg + 0x004);
> > +
> > + seq_printf(s, "dout=%lu%s doen=%lu%s",
> > + dout & GENMASK(7, 0), (dout & BIT(31)) ? "r" : "",
> > + doen & GENMASK(7, 0), (doen & BIT(31)) ? "r" : "");
> > +}
>
> ...
>
> > + 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;
>
> Reversed xmas tree order?
>
> > +static int starfive_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
> > +{
> > + struct starfive_pinctrl *sfp = container_of(gc, struct starfive_pinctrl, gc);
> > + void __iomem *doen = sfp->base + GPON_DOEN_CFG + 8 * gpio;
> > +
> > + /* return GPIO_LINE_DIRECTION_OUT (0) only if doen == GPO_ENABLE (0) */
> > + return readl_relaxed(doen) != GPO_ENABLE;
>
> I believe the idea was to return the predefined values for the direction.
You mean this?
return readl_relaxed(doen) == GPO_ENABLE ? GPIO_LINE_DIRECTION_OUT :
GPIO_LINE_DIRECTION_IN;
> > +}
>
> ...
>
> > +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);
> > + void __iomem *base = sfp->base + 4 * (gpio / 32);
> > + u32 mask = BIT(gpio % 32);
> > + u32 irq_type, edge_both, polarity;
> > + unsigned long flags;
> > +
> > + if (trigger & IRQ_TYPE_EDGE_BOTH)
> > + irq_set_handler_locked(d, handle_edge_irq);
> > + else if (trigger & IRQ_TYPE_LEVEL_MASK)
> > + irq_set_handler_locked(d, handle_level_irq);
>
> Usually we don't assign this twice, so it should be after the switch.
>
> > + switch (trigger) {
> > + case IRQ_TYPE_EDGE_RISING:
> > + irq_type = mask; /* 1: edge triggered */
> > + edge_both = 0; /* 0: single edge */
> > + polarity = mask; /* 1: rising edge */
> > + break;
> > + case IRQ_TYPE_EDGE_FALLING:
> > + irq_type = mask; /* 1: edge triggered */
> > + edge_both = 0; /* 0: single edge */
> > + polarity = 0; /* 0: falling edge */
> > + break;
> > + case IRQ_TYPE_EDGE_BOTH:
> > + irq_type = mask; /* 1: edge triggered */
> > + edge_both = mask; /* 1: both edges */
> > + polarity = 0; /* 0: ignored */
> > + break;
> > + case IRQ_TYPE_LEVEL_HIGH:
> > + irq_type = 0; /* 0: level triggered */
> > + edge_both = 0; /* 0: ignored */
> > + polarity = mask; /* 1: high level */
> > + break;
> > + case IRQ_TYPE_LEVEL_LOW:
> > + 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);
>
> Why? You have it already in ->probe(), what's the point?
So last time you asked about this, I explained a situation where
userspace first grabs a GPIO, set the interrupt to edge triggered, and
then later loads a driver that requests an unsupported IRQ type. Then
I'd like to set the handler back to handle_bad_irq so we don't get
weird interrupts, but maybe now you know a reason why that doesn't
matter or can't happen?
> > + return -EINVAL;
> > + }
> > +
> > + raw_spin_lock_irqsave(&sfp->lock, flags);
> > + irq_type |= readl_relaxed(base + GPIOIS) & ~mask;
> > + writel_relaxed(irq_type, base + GPIOIS);
> > + edge_both |= readl_relaxed(base + GPIOIBE) & ~mask;
> > + writel_relaxed(edge_both, base + GPIOIBE);
> > + polarity |= readl_relaxed(base + GPIOIEV) & ~mask;
> > + writel_relaxed(polarity, base + GPIOIEV);
> > + raw_spin_unlock_irqrestore(&sfp->lock, flags);
> > + return 0;
> > +}
>
> ...
>
> > + ret = reset_control_deassert(rst);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "could not deassert resetd\n");
>
> > + ret = devm_pinctrl_register_and_init(dev, &starfive_desc, sfp, &sfp->pctl);
> > + if (ret)
>
> I don't see who will assert reset here.
No, so originally this driver would first assert and then deassert
reset. I decided against that because in all likelyhood earlier boot
stages would have set pinmux up for a serial port, and we don't want
to interrupt the serial debug output. The only reason I make sure the
reset line is deasserted is in case someone makes a really minimal
bootloader that just does the absolute minimal to load a Linux kernel
and doesn't even log any anything.
By the same token we also don't want to assert reset on error in case
it resets pin muxing for the the serial line that was supposed to log
the error.
>
> > + return dev_err_probe(dev, ret, "could not register pinctrl driver\n");
>
> ...
>
> > + switch (value) {
> > + case 0:
> > + sfp->gpios.pin_base = PAD_INVALID_GPIO;
> > + 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:
>
> Ditto.
>
> > + return dev_err_probe(dev, -EINVAL, "invalid signal group %u\n", value);
> > + }
>
> ...
>
> > + ret = devm_gpiochip_add_data(dev, &sfp->gc, sfp);
> > + if (ret)
>
> Ditto.
>
> > + return dev_err_probe(dev, ret, "could not register gpiochip\n");
> > +
> > +done:
> > + return pinctrl_enable(sfp->pctl);
>
> Ditto.
>
> And better to use label name like following
> out_pinctrl_enable:
Good idea.
On Tue, Nov 2, 2021 at 12:59 PM Emil Renner Berthing <[email protected]> wrote:
>
> On Tue, 2 Nov 2021 at 20:43, Andy Shevchenko <[email protected]> wrote:
> > +Cc: Yury (bitmap expert)
> >
> > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <[email protected]> wrote:
> > >
> > > Add a driver for the StarFive JH7100 reset controller.
> >
> > ...
> >
> > > +#define BIT_MASK32(x) BIT((x) % 32)
> >
> > Possible namespace collision.
> >
> > ...
> >
> > > +/*
> > > + * the registers work like a 32bit bitmap, so writing a 1 to the m'th bit of
> > > + * the n'th ASSERT register asserts line 32n + m, and writing a 0 deasserts the
> > > + * same line.
We don't have 32-bit bitmaps. Bitmaps are always arrays of unsigned longs. On a
64-bit system this '32-bit bitmap' may be broken due to endianness issues.
> > > + * most reset lines have their status inverted so a 0 in the STATUS register
> > > + * means the line is asserted and a 1 means it's deasserted. a few lines don't
> > > + * though, so store the expected value of the status registers when all lines
> > > + * are asserted.
> > > + */
> >
> > Besides missing capitalization,
>
> I'm confused. it was you who wanted all comments to capitalized the same..
> 64bi
> if it sounds like bitmap, use bitmap.
> > I have checked DT definitions and it seems you don't even need the
> > BIT_MASK() macro,
> >
> > > +static const u32 jh7100_reset_asserted[4] = {
> > > + /* STATUS0 register */
> > > + BIT_MASK32(JH7100_RST_U74) |
I think we have no BIT_MASK32() for a good reason. Natural alignment is
always preferable.
> > > + BIT_MASK32(JH7100_RST_VP6_DRESET) |
> > > + BIT_MASK32(JH7100_RST_VP6_BRESET),
> > > + /* STATUS1 register */
> > > + BIT_MASK32(JH7100_RST_HIFI4_DRESET) |
> > > + BIT_MASK32(JH7100_RST_HIFI4_BRESET),
> > > + /* STATUS2 register */
> > > + BIT_MASK32(JH7100_RST_E24),
> > > + /* STATUS3 register */
> > > + 0,
> > > +};
> >
> > Yury, do we have any clever (clean) way to initialize a bitmap with
> > particular bits so that it will be a constant from the beginning? If
> > no, any suggestion what we can provide to such users?
If you want your array to be a true bitmap, ie, all bitmap functions should
work with it correctly, you'd initialize it like this:
static const unsigned long jh7100_reset_asserted[] = {
BITMAP_FROM_U64(BIT_MASK(JH7100_RST_VP6_DRESET) |
BIT_MASK(JH7100_RST_VP6_BRESET) |
BIT_MASK(JH7100_RST_HIFI4_DRESET) |
BIT_MASK(JH7100_RST_HIFI4_BRESET)),
BITMAP_FROM_U64(BIT_MASK(JH7100_RST_E24)),
}
Look at lib/test_bitmap.c for example, and comment to BITMAP_FROM_U64()
for internal details.
On the other hand, if you hardware has tricky requirements for bit
positions, and they should depend on endianness and/or size of
long in a way not compatible with bitmaps, you probably know better
how to handle this. Just don't refer to your structure as a bitmap.
Thanks,
Yury
> The problem is, that even if we could initialize this without the
> monstrosity in our last conversation a 64bit bitmap would still
> produce worse code. As it is now it's simply a 32bit load and mask
> with index and mask already calculated for the registers. In the
> status callback the mask can even be folded into the register read
> mask. With a 64bit bitmap you'd need to calculate new 64bit index and
> masks, and then conditionally shift the bits into position.
>
> If this reflection of the 32bit registers bothers you that much we
> could alternatively do
>
> static bool jh7100_reset_inverted(unsigned int idx)
> {
> switch (idx) {
> case JH7100_RST_U74:
> case JH7100_RST_VP6_DRESET:
> ..
> return false;
> }
> return true;
> }
>
> It'd still produce worse code, but at least it would be readable.
>
> /Emil
>
> > ...
> >
> > > + dev_dbg(rcdev->dev, "reset(%lu)\n", id);
> >
> > These debug messages are useless since one should use ftrace facility instead,
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
On Tue, 2 Nov 2021 at 21:14, Andy Shevchenko <[email protected]> wrote:
> On Tue, Nov 2, 2021 at 9:59 PM Emil Renner Berthing <[email protected]> wrote:
> > On Tue, 2 Nov 2021 at 20:43, Andy Shevchenko <[email protected]> wrote:
> > > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <[email protected]> wrote:
>
> ...
>
> > > > +/*
> > > > + * the registers work like a 32bit bitmap, so writing a 1 to the m'th bit of
> > > > + * the n'th ASSERT register asserts line 32n + m, and writing a 0 deasserts the
> > > > + * same line.
> > > > + * most reset lines have their status inverted so a 0 in the STATUS register
> > > > + * means the line is asserted and a 1 means it's deasserted. a few lines don't
> > > > + * though, so store the expected value of the status registers when all lines
> > > > + * are asserted.
> > > > + */
> > >
> > > Besides missing capitalization,
> >
> > I'm confused. it was you who wanted all comments to capitalized the same..
>
> Yes and there are two types of the comments, one-liners and
> multi-line. In multi-line you usually use proper English grammar,
> where capitalization means what it means. For the one-liners just
> choose either small letters or capital letters to start them with.
That sounds reasonable, it was just that you complained about
inconsistent comments in the pinctrl driver that follows the above.
> > if it sounds like bitmap, use bitmap.
> > > I have checked DT definitions and it seems you don't even need the
> > > BIT_MASK() macro,
> > >
> > > > +static const u32 jh7100_reset_asserted[4] = {
> > > > + /* STATUS0 register */
> > > > + BIT_MASK32(JH7100_RST_U74) |
> > > > + BIT_MASK32(JH7100_RST_VP6_DRESET) |
> > > > + BIT_MASK32(JH7100_RST_VP6_BRESET),
> > > > + /* STATUS1 register */
> > > > + BIT_MASK32(JH7100_RST_HIFI4_DRESET) |
> > > > + BIT_MASK32(JH7100_RST_HIFI4_BRESET),
> > > > + /* STATUS2 register */
> > > > + BIT_MASK32(JH7100_RST_E24),
> > > > + /* STATUS3 register */
> > > > + 0,
> > > > +};
> > >
> > > Yury, do we have any clever (clean) way to initialize a bitmap with
> > > particular bits so that it will be a constant from the beginning? If
> > > no, any suggestion what we can provide to such users?
> >
> > The problem is, that even if we could initialize this without the
> > monstrosity in our last conversation a 64bit bitmap would still
> > produce worse code. As it is now it's simply a 32bit load and mask
> > with index and mask already calculated for the registers. In the
> > status callback the mask can even be folded into the register read
> > mask. With a 64bit bitmap you'd need to calculate new 64bit index and
> > masks, and then conditionally shift the bits into position.
>
> Why? You may use 8 byte IO (writeq() / readq() or their relaxed versions), no?
>
> ...
>
> > If this reflection of the 32bit registers bothers you that much
>
> What bothers me is hidden endianess issues (yeah, here it might be
> theoretical, but consider that somebody will look at your code and use
> it as the best example ever).
Wouldn't endian issues be a reason to make sure we read 32bit
registers with 32bit reads? Or do you expect a hypothetical big-endian
StarFive SoC to also change the order of the registers?
On Tue, Nov 2, 2021 at 10:35 PM Emil Renner Berthing <[email protected]> wrote:
>
> On Tue, 2 Nov 2021 at 21:02, Andy Shevchenko <[email protected]> wrote:
> > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <[email protected]> wrote:
...
> > > +static inline struct device *starfive_dev(const struct starfive_pinctrl *sfp)
> > > +{
> > > + return sfp->gc.parent;
> > > +}
> > > +
> >
> > This seems useless helper. You may do what it's doing just in place.
> > It will save 5 LOCs.
>
> I don't mind removing it, I just think it's easier to read when we're
> explicit that all we want is a dev pointer, and we don't suddenly need
> to know the parent of the gpio chip in all the pinmux/pinconf
> callbacks.
I don't really see the gain of it.
...
> > > +static int starfive_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
> > > +{
> > > + struct starfive_pinctrl *sfp = container_of(gc, struct starfive_pinctrl, gc);
> > > + void __iomem *doen = sfp->base + GPON_DOEN_CFG + 8 * gpio;
> > > +
> > > + /* return GPIO_LINE_DIRECTION_OUT (0) only if doen == GPO_ENABLE (0) */
> > > + return readl_relaxed(doen) != GPO_ENABLE;
> >
> > I believe the idea was to return the predefined values for the direction.
>
> You mean this?
> return readl_relaxed(doen) == GPO_ENABLE ? GPIO_LINE_DIRECTION_OUT :
> GPIO_LINE_DIRECTION_IN;
For example, or with if (...) return _OUT; return _IN;'
> > > +}
...
> > > + if (trigger & IRQ_TYPE_EDGE_BOTH)
> > > + irq_set_handler_locked(d, handle_edge_irq);
> > > + else if (trigger & IRQ_TYPE_LEVEL_MASK)
> > > + irq_set_handler_locked(d, handle_level_irq);
> >
> > Usually we don't assign this twice, so it should be after the switch.
> >
> > > + switch (trigger) {
> > > + default:
> >
> > > + irq_set_handler_locked(d, handle_bad_irq);
> >
> > Why? You have it already in ->probe(), what's the point?
>
> So last time you asked about this, I explained a situation where
> userspace first grabs a GPIO, set the interrupt to edge triggered, and
> then later loads a driver that requests an unsupported IRQ type.
I didn't get this scenario. Is it real?
> Then
> I'd like to set the handler back to handle_bad_irq so we don't get
> weird interrupts, but maybe now you know a reason why that doesn't
> matter or can't happen?
In ->probe() you set _default_ handler to bad(), what do you mean by
'set the handler back to bad()'? How is it otherwise if you free an
interrupt?
So, please elaborate with call traces what the scenario / use case you
are talking about. If it's true what you are saying, we have a
situation (plenty of GPIO drivers don't do what you are suggesting
here).
> > > + return -EINVAL;
> > > + }
...
> > > + ret = reset_control_deassert(rst);
> > > + if (ret)
> > > + return dev_err_probe(dev, ret, "could not deassert resetd\n");
> >
> > > + ret = devm_pinctrl_register_and_init(dev, &starfive_desc, sfp, &sfp->pctl);
> > > + if (ret)
> >
> > I don't see who will assert reset here.
>
> No, so originally this driver would first assert and then deassert
> reset. I decided against that because in all likelyhood earlier boot
> stages would have set pinmux up for a serial port, and we don't want
> to interrupt the serial debug output. The only reason I make sure the
> reset line is deasserted is in case someone makes a really minimal
> bootloader that just does the absolute minimal to load a Linux kernel
> and doesn't even log any anything.
>
> By the same token we also don't want to assert reset on error in case
> it resets pin muxing for the the serial line that was supposed to log
> the error.
Perhaps comment in the code explaining this?
--
With Best Regards,
Andy Shevchenko
On Wed, 3 Nov 2021 at 10:13, Andy Shevchenko <[email protected]> wrote:
> On Tue, Nov 2, 2021 at 10:35 PM Emil Renner Berthing <[email protected]> wrote:
> > On Tue, 2 Nov 2021 at 21:02, Andy Shevchenko <[email protected]> wrote:
> > > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <[email protected]> wrote:
>
> > > > + switch (trigger) {
>
> > > > + default:
> > >
> > > > + irq_set_handler_locked(d, handle_bad_irq);
> > >
> > > Why? You have it already in ->probe(), what's the point?
> >
> > So last time you asked about this, I explained a situation where
> > userspace first grabs a GPIO, set the interrupt to edge triggered, and
> > then later loads a driver that requests an unsupported IRQ type.
>
> I didn't get this scenario. Is it real?
No, it's totally made up, but I mean we even have tools like fuzzing
to help us find bugs that would never happen in real use cases.
> > Then
> > I'd like to set the handler back to handle_bad_irq so we don't get
> > weird interrupts, but maybe now you know a reason why that doesn't
> > matter or can't happen?
>
> In ->probe() you set _default_ handler to bad(), what do you mean by
> 'set the handler back to bad()'? How is it otherwise if you free an
> interrupt?
It might not be, but when not sure I thought it better to error on the
safe side.
> So, please elaborate with call traces what the scenario / use case you
> are talking about. If it's true what you are saying, we have a
> situation (plenty of GPIO drivers don't do what you are suggesting
> here).
>
> > > > + return -EINVAL;
> > > > + }
>
> ...
>
> > > > + ret = reset_control_deassert(rst);
> > > > + if (ret)
> > > > + return dev_err_probe(dev, ret, "could not deassert resetd\n");
> > >
> > > > + ret = devm_pinctrl_register_and_init(dev, &starfive_desc, sfp, &sfp->pctl);
> > > > + if (ret)
> > >
> > > I don't see who will assert reset here.
> >
> > No, so originally this driver would first assert and then deassert
> > reset. I decided against that because in all likelyhood earlier boot
> > stages would have set pinmux up for a serial port, and we don't want
> > to interrupt the serial debug output. The only reason I make sure the
> > reset line is deasserted is in case someone makes a really minimal
> > bootloader that just does the absolute minimal to load a Linux kernel
> > and doesn't even log any anything.
> >
> > By the same token we also don't want to assert reset on error in case
> > it resets pin muxing for the the serial line that was supposed to log
> > the error.
>
> Perhaps comment in the code explaining this?
Sure.
On Wed, Nov 03, 2021 at 01:35:23PM +0100, Emil Renner Berthing wrote:
> On Wed, 3 Nov 2021 at 10:13, Andy Shevchenko <[email protected]> wrote:
> > On Tue, Nov 2, 2021 at 10:35 PM Emil Renner Berthing <[email protected]> wrote:
> > > On Tue, 2 Nov 2021 at 21:02, Andy Shevchenko <[email protected]> wrote:
> > > > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <[email protected]> wrote:
...
> > > > > + irq_set_handler_locked(d, handle_bad_irq);
> > > >
> > > > Why? You have it already in ->probe(), what's the point?
> > >
> > > So last time you asked about this, I explained a situation where
> > > userspace first grabs a GPIO, set the interrupt to edge triggered, and
> > > then later loads a driver that requests an unsupported IRQ type.
> >
> > I didn't get this scenario. Is it real?
>
> No, it's totally made up, but I mean we even have tools like fuzzing
> to help us find bugs that would never happen in real use cases.
>
> > > Then
> > > I'd like to set the handler back to handle_bad_irq so we don't get
> > > weird interrupts, but maybe now you know a reason why that doesn't
> > > matter or can't happen?
> >
> > In ->probe() you set _default_ handler to bad(), what do you mean by
> > 'set the handler back to bad()'? How is it otherwise if you free an
> > interrupt?
>
> It might not be, but when not sure I thought it better to error on the
> safe side.
With a dead code?
I do not believe there is an issue since. like I said, there are plenty drivers
that don't do what you are suggesting here --> 99.99% you added a dead code.
> > So, please elaborate with call traces what the scenario / use case you
> > are talking about. If it's true what you are saying, we have a
> > situation (plenty of GPIO drivers don't do what you are suggesting
> > here).
--
With Best Regards,
Andy Shevchenko
On Tue, 2 Nov 2021 at 22:17, Emil Renner Berthing <[email protected]> wrote:
> On Tue, 2 Nov 2021 at 21:14, Andy Shevchenko <[email protected]> wrote:
> > On Tue, Nov 2, 2021 at 9:59 PM Emil Renner Berthing <[email protected]> wrote:
> > > On Tue, 2 Nov 2021 at 20:43, Andy Shevchenko <[email protected]> wrote:
> > > > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <[email protected]> wrote:
> >
> > ...
> >
> > > > > +/*
> > > > > + * the registers work like a 32bit bitmap, so writing a 1 to the m'th bit of
> > > > > + * the n'th ASSERT register asserts line 32n + m, and writing a 0 deasserts the
> > > > > + * same line.
> > > > > + * most reset lines have their status inverted so a 0 in the STATUS register
> > > > > + * means the line is asserted and a 1 means it's deasserted. a few lines don't
> > > > > + * though, so store the expected value of the status registers when all lines
> > > > > + * are asserted.
> > > > > + */
> > > >
> > > > Besides missing capitalization,
> > >
> > > I'm confused. it was you who wanted all comments to capitalized the same..
> >
> > Yes and there are two types of the comments, one-liners and
> > multi-line. In multi-line you usually use proper English grammar,
> > where capitalization means what it means. For the one-liners just
> > choose either small letters or capital letters to start them with.
>
> That sounds reasonable, it was just that you complained about
> inconsistent comments in the pinctrl driver that follows the above.
>
> > > if it sounds like bitmap, use bitmap.
> > > > I have checked DT definitions and it seems you don't even need the
> > > > BIT_MASK() macro,
> > > >
> > > > > +static const u32 jh7100_reset_asserted[4] = {
> > > > > + /* STATUS0 register */
> > > > > + BIT_MASK32(JH7100_RST_U74) |
> > > > > + BIT_MASK32(JH7100_RST_VP6_DRESET) |
> > > > > + BIT_MASK32(JH7100_RST_VP6_BRESET),
> > > > > + /* STATUS1 register */
> > > > > + BIT_MASK32(JH7100_RST_HIFI4_DRESET) |
> > > > > + BIT_MASK32(JH7100_RST_HIFI4_BRESET),
> > > > > + /* STATUS2 register */
> > > > > + BIT_MASK32(JH7100_RST_E24),
> > > > > + /* STATUS3 register */
> > > > > + 0,
> > > > > +};
> > > >
> > > > Yury, do we have any clever (clean) way to initialize a bitmap with
> > > > particular bits so that it will be a constant from the beginning? If
> > > > no, any suggestion what we can provide to such users?
> > >
> > > The problem is, that even if we could initialize this without the
> > > monstrosity in our last conversation a 64bit bitmap would still
> > > produce worse code. As it is now it's simply a 32bit load and mask
> > > with index and mask already calculated for the registers. In the
> > > status callback the mask can even be folded into the register read
> > > mask. With a 64bit bitmap you'd need to calculate new 64bit index and
> > > masks, and then conditionally shift the bits into position.
> >
> > Why? You may use 8 byte IO (writeq() / readq() or their relaxed versions), no?
> >
> > > If this reflection of the 32bit registers bothers you that much
> >
> > What bothers me is hidden endianess issues (yeah, here it might be
> > theoretical, but consider that somebody will look at your code and use
> > it as the best example ever).
>
> Wouldn't endian issues be a reason to make sure we read 32bit
> registers with 32bit reads? Or do you expect a hypothetical big-endian
> StarFive SoC to also change the order of the registers?
Hi Andy.
I'd really like to understand your reasoning here. As far as I can
tell reading 2 adjacent 32bit registers with a 64bit read as you're
proposing is exactly what would cause endian issues. Eg. on little
endian you'd get reg0 | reg1 << 32 whereas on big-endian you'd get
reg0 << 32 | reg1.
/Emil
On Thu, Nov 04, 2021 at 01:15:46PM +0100, Emil Renner Berthing wrote:
> On Tue, 2 Nov 2021 at 22:17, Emil Renner Berthing <[email protected]> wrote:
...
> I'd really like to understand your reasoning here. As far as I can
> tell reading 2 adjacent 32bit registers with a 64bit read as you're
> proposing is exactly what would cause endian issues. Eg. on little
> endian you'd get reg0 | reg1 << 32 whereas on big-endian you'd get
> reg0 << 32 | reg1.
Nope, it won't. The endianess is a property of both CPU and device.
The I/O accessors, such as readl()/writel() and iowrtieXX()/ioreadXX()
are _always_ LE.
So, writeq() will properly put bits to their places in case device is LE.
And most devices are LE (or should be). Of course there are cases, but then
you have to specify them explicitly.
My motive here is simple as that the device is definitely a set of a few
128-bit bitmaps (in registers) and using bitmap _is_ representing hardware
in the kernel. Using something else will deviate from that (maybe not too
far, but still...).
--
With Best Regards,
Andy Shevchenko
On Tue, Nov 2, 2021 at 9:02 PM Andy Shevchenko
<[email protected]> wrote:
> > + depends on OF
>
> So this descreases test coverage.
> Linus, can we provide a necessary stub so we may drop this dependency?
Hm it further selects OF_GPIO which depends on OF
so I don't know how that would work.
But does it decrease compile coverage a lot, even x86 has
optional OF support so I imagine it appears in x86
allyesconfig I suppose? Or am I wrong?
Yours,
Linus Walleij
On Tue, Nov 2, 2021 at 9:08 PM Andy Shevchenko
<[email protected]> wrote:
(...)
> > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <[email protected]> wrote:
>
> ...
>
> > > +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 (!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]);
> > > +
> > > + 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:
> > > + mask |= PAD_DRIVE_STRENGTH_MASK;
> > > + value = (value & ~PAD_DRIVE_STRENGTH_MASK) |
> > > + starfive_drive_strength_from_max_mA(arg);
> > > + 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;
> > > +}
>
> Linus any comments on this code (sorry if I missed your reply)? The
> idea behind above is to skip all settings from the same category and
> apply only the last one, e.g. if we have "bias set to X", ..., "bias
> disable", ..., "bias set to Y", the hardware will see only the last
> operation, i.e. "bias set to Y". I think it may not be the best
> approach (theoretically?) since the hardware definitely may behave
> differently on the other side in case of such series of the
> configurations (yes, I have seen some interesting implementations of
> the touchpad / touchscreen GPIOs that may be affected).
That sounds weird. I think we need to look at how other drivers
deal with this.
To me it seems more natural that
starfive_padctl_rmw(sfp, group->pins[i], mask, value);
would get called at the end of each iteration of the
for (i = 0; i < num_configs; i++) loop.
Yours,
Linus Walleij
On Tue, Nov 9, 2021 at 2:54 AM Linus Walleij <[email protected]> wrote:
> On Tue, Nov 2, 2021 at 9:02 PM Andy Shevchenko
> <[email protected]> wrote:
>
> > > + depends on OF
> >
> > So this descreases test coverage.
> > Linus, can we provide a necessary stub so we may drop this dependency?
>
> Hm it further selects OF_GPIO which depends on OF
> so I don't know how that would work.
>
> But does it decrease compile coverage a lot, even x86 has
> optional OF support so I imagine it appears in x86
> allyesconfig I suppose? Or am I wrong?
I believe so. At least in my environment I have OF enabled (I haven't
looked into what was the change to the config, though).
--
With Best Regards,
Andy Shevchenko
On Tue, 9 Nov 2021 at 02:01, Linus Walleij <[email protected]> wrote:
>
> On Tue, Nov 2, 2021 at 9:08 PM Andy Shevchenko
> <[email protected]> wrote:
> (...)
> > > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <[email protected]> wrote:
> >
> > ...
> >
> > > > +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 (!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]);
> > > > +
> > > > + 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:
> > > > + mask |= PAD_DRIVE_STRENGTH_MASK;
> > > > + value = (value & ~PAD_DRIVE_STRENGTH_MASK) |
> > > > + starfive_drive_strength_from_max_mA(arg);
> > > > + 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;
> > > > +}
> >
> > Linus any comments on this code (sorry if I missed your reply)? The
> > idea behind above is to skip all settings from the same category and
> > apply only the last one, e.g. if we have "bias set to X", ..., "bias
> > disable", ..., "bias set to Y", the hardware will see only the last
> > operation, i.e. "bias set to Y". I think it may not be the best
> > approach (theoretically?) since the hardware definitely may behave
> > differently on the other side in case of such series of the
> > configurations (yes, I have seen some interesting implementations of
> > the touchpad / touchscreen GPIOs that may be affected).
>
> That sounds weird. I think we need to look at how other drivers
> deal with this.
>
> To me it seems more natural that
> starfive_padctl_rmw(sfp, group->pins[i], mask, value);
> would get called at the end of each iteration of the
> for (i = 0; i < num_configs; i++) loop.
That would work, but when the loop is done the end result would be
exactly the same. The only difference is that the above would rapidly
"blink" the different states during the loop until it arrives at the
result. This would certainly be different, but it can never be the
intended behaviour and only a side-effect on how the pinctrl framework
works. The order the different states are blinked depends entirely on
how the pinctrl framework parses the device tree. I still think it
would be more natural to cleanly go to the end result without this
blinking.
/Emil
On Mon, 8 Nov 2021 at 10:18, Andy Shevchenko <[email protected]> wrote:
> On Thu, Nov 04, 2021 at 01:15:46PM +0100, Emil Renner Berthing wrote:
> > On Tue, 2 Nov 2021 at 22:17, Emil Renner Berthing <[email protected]> wrote:
>
> ...
>
> > I'd really like to understand your reasoning here. As far as I can
> > tell reading 2 adjacent 32bit registers with a 64bit read as you're
> > proposing is exactly what would cause endian issues. Eg. on little
> > endian you'd get reg0 | reg1 << 32 whereas on big-endian you'd get
> > reg0 << 32 | reg1.
>
> Nope, it won't. The endianess is a property of both CPU and device.
>
> The I/O accessors, such as readl()/writel() and iowrtieXX()/ioreadXX()
> are _always_ LE.
Aha! Thanks, that's the bit I was missing.
On Tue, Nov 9, 2021 at 11:21 AM Emil Renner Berthing <[email protected]> wrote:
> On Tue, 9 Nov 2021 at 02:01, Linus Walleij <[email protected]> wrote:
> > On Tue, Nov 2, 2021 at 9:08 PM Andy Shevchenko
> > <[email protected]> wrote:
...
> > > Linus any comments on this code (sorry if I missed your reply)? The
> > > idea behind above is to skip all settings from the same category and
> > > apply only the last one, e.g. if we have "bias set to X", ..., "bias
> > > disable", ..., "bias set to Y", the hardware will see only the last
> > > operation, i.e. "bias set to Y". I think it may not be the best
> > > approach (theoretically?) since the hardware definitely may behave
> > > differently on the other side in case of such series of the
> > > configurations (yes, I have seen some interesting implementations of
> > > the touchpad / touchscreen GPIOs that may be affected).
> >
> > That sounds weird. I think we need to look at how other drivers
> > deal with this.
> >
> > To me it seems more natural that
> > starfive_padctl_rmw(sfp, group->pins[i], mask, value);
> > would get called at the end of each iteration of the
> > for (i = 0; i < num_configs; i++) loop.
>
> That would work, but when the loop is done the end result would be
> exactly the same.
It seems we interpret the term "result" differently. The result when
we talking about GPIOs is the series of pin state changes incl.
configuration. This is how it should be recognized when programming
hardware.
> The only difference is that the above would rapidly
> "blink" the different states during the loop until it arrives at the
> result. This would certainly be different, but it can never be the
> intended behaviour and only a side-effect on how the pinctrl framework
> works.
Is it? That's what I'm trying to get an answer to. If you may
guarantee this (the keywords "intended behaviour" and "side effect"),
I wouldn't object.
> The order the different states are blinked depends entirely on
> how the pinctrl framework parses the device tree. I still think it
> would be more natural to cleanly go to the end result without this
> blinking.
--
With Best Regards,
Andy Shevchenko
On Tue, 9 Nov 2021 at 10:34, Andy Shevchenko <[email protected]> wrote:
> On Tue, Nov 9, 2021 at 11:21 AM Emil Renner Berthing <[email protected]> wrote:
> > On Tue, 9 Nov 2021 at 02:01, Linus Walleij <[email protected]> wrote:
> > > On Tue, Nov 2, 2021 at 9:08 PM Andy Shevchenko
> > > <[email protected]> wrote:
>
> ...
>
> > > > Linus any comments on this code (sorry if I missed your reply)? The
> > > > idea behind above is to skip all settings from the same category and
> > > > apply only the last one, e.g. if we have "bias set to X", ..., "bias
> > > > disable", ..., "bias set to Y", the hardware will see only the last
> > > > operation, i.e. "bias set to Y". I think it may not be the best
> > > > approach (theoretically?) since the hardware definitely may behave
> > > > differently on the other side in case of such series of the
> > > > configurations (yes, I have seen some interesting implementations of
> > > > the touchpad / touchscreen GPIOs that may be affected).
> > >
> > > That sounds weird. I think we need to look at how other drivers
> > > deal with this.
> > >
> > > To me it seems more natural that
> > > starfive_padctl_rmw(sfp, group->pins[i], mask, value);
> > > would get called at the end of each iteration of the
> > > for (i = 0; i < num_configs; i++) loop.
> >
> > That would work, but when the loop is done the end result would be
> > exactly the same.
>
> It seems we interpret the term "result" differently. The result when
> we talking about GPIOs is the series of pin state changes incl.
> configuration. This is how it should be recognized when programming
> hardware.
>
> > The only difference is that the above would rapidly
> > "blink" the different states during the loop until it arrives at the
> > result. This would certainly be different, but it can never be the
> > intended behaviour and only a side-effect on how the pinctrl framework
> > works.
>
> Is it? That's what I'm trying to get an answer to. If you may
> guarantee this (the keywords "intended behaviour" and "side effect"),
> I wouldn't object.
>
> > The order the different states are blinked depends entirely on
> > how the pinctrl framework parses the device tree. I still think it
> > would be more natural to cleanly go to the end result without this
> > blinking.
Hmm.. but if going through the different states is what you want, then
wouldn't you need the device tree to have an ordered list of the
states rather than just a single node and also a way to tune how long
time the different states are blinked?
On Tue, Nov 9, 2021 at 10:40 AM Emil Renner Berthing <[email protected]> wrote:
> On Tue, 9 Nov 2021 at 10:34, Andy Shevchenko <[email protected]> wrote:
> > > The order the different states are blinked depends entirely on
> > > how the pinctrl framework parses the device tree. I still think it
> > > would be more natural to cleanly go to the end result without this
> > > blinking.
>
> Hmm.. but if going through the different states is what you want, then
> wouldn't you need the device tree to have an ordered list of the
> states rather than just a single node and also a way to tune how long
> time the different states are blinked?
In a way you are correct that the DT is a functional language and it's
a bit lite a style sheet or prolog or something in that the end reduction
is what counts.
In this case, I would say something is weird if there are interim states,
the yaml validation should not allow you to set the same thing back
and forth in your DTS file.
Alas we are not perfect as in yaml validation isn't perfect either.
I can't see what the problem is really, just write proper DTS files
and there will not be any interim states, right? And if it is possible
to write DTS files that have states and sequence requirements,
these should be caught in validation. Should be.
Yours,
Linus Walleij
On Tue, 9 Nov 2021 at 21:29, Linus Walleij <[email protected]> wrote:
> On Tue, Nov 9, 2021 at 10:40 AM Emil Renner Berthing <[email protected]> wrote:
> > On Tue, 9 Nov 2021 at 10:34, Andy Shevchenko <[email protected]> wrote:
>
> > > > The order the different states are blinked depends entirely on
> > > > how the pinctrl framework parses the device tree. I still think it
> > > > would be more natural to cleanly go to the end result without this
> > > > blinking.
> >
> > Hmm.. but if going through the different states is what you want, then
> > wouldn't you need the device tree to have an ordered list of the
> > states rather than just a single node and also a way to tune how long
> > time the different states are blinked?
>
> In a way you are correct that the DT is a functional language and it's
> a bit lite a style sheet or prolog or something in that the end reduction
> is what counts.
>
> In this case, I would say something is weird if there are interim states,
> the yaml validation should not allow you to set the same thing back
> and forth in your DTS file.
Yes, exactly.
> Alas we are not perfect as in yaml validation isn't perfect either.
> I can't see what the problem is really, just write proper DTS files
> and there will not be any interim states, right?
No, I agree. I think it's only that Andy wasn't sure if these interim
states might be meaningful/useful.
> And if it is possible
> to write DTS files that have states and sequence requirements,
> these should be caught in validation. Should be.
>
> Yours,
> Linus Walleij
On Tue, Nov 09, 2021 at 10:04:24PM +0100, Emil Renner Berthing wrote:
> On Tue, 9 Nov 2021 at 21:29, Linus Walleij <[email protected]> wrote:
> > On Tue, Nov 9, 2021 at 10:40 AM Emil Renner Berthing <[email protected]> wrote:
...
> No, I agree. I think it's only that Andy wasn't sure if these interim
> states might be meaningful/useful.
Exactly. Because HW could behave differently.
> > And if it is possible
> > to write DTS files that have states and sequence requirements,
> > these should be caught in validation. Should be.
--
With Best Regards,
Andy Shevchenko
On Wed, 10 Nov 2021 at 09:05, Andy Shevchenko <[email protected]> wrote:
> On Tue, Nov 09, 2021 at 10:04:24PM +0100, Emil Renner Berthing wrote:
> > On Tue, 9 Nov 2021 at 21:29, Linus Walleij <[email protected]> wrote:
> > > On Tue, Nov 9, 2021 at 10:40 AM Emil Renner Berthing <[email protected]> wrote:
>
> ...
>
> > No, I agree. I think it's only that Andy wasn't sure if these interim
> > states might be meaningful/useful.
>
> Exactly. Because HW could behave differently.
Right. But I think we've now established that what is described in the
device tree is the state the pins should be in after the function has
been called, eg. only the reduction matters, and any interim states
would just be a byproduct of storing the state in the configs list.
> > > And if it is possible
> > > to write DTS files that have states and sequence requirements,
> > > these should be caught in validation. Should be.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Tue, Nov 2, 2021 at 1:55 PM Yury Norov <[email protected]> wrote:
>
> On Tue, Nov 2, 2021 at 12:59 PM Emil Renner Berthing <[email protected]> wrote:
> >
> > On Tue, 2 Nov 2021 at 20:43, Andy Shevchenko <[email protected]> wrote:
> > > +Cc: Yury (bitmap expert)
> > >
> > > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <[email protected]> wrote:
> > > >
> > > > Add a driver for the StarFive JH7100 reset controller.
> > >
> > > ...
> > >
> > > > +#define BIT_MASK32(x) BIT((x) % 32)
> > >
> > > Possible namespace collision.
> > >
> > > ...
> > >
> > > > +/*
> > > > + * the registers work like a 32bit bitmap, so writing a 1 to the m'th bit of
> > > > + * the n'th ASSERT register asserts line 32n + m, and writing a 0 deasserts the
> > > > + * same line.
>
> We don't have 32-bit bitmaps. Bitmaps are always arrays of unsigned longs. On a
> 64-bit system this '32-bit bitmap' may be broken due to endianness issues.
>
> > > > + * most reset lines have their status inverted so a 0 in the STATUS register
> > > > + * means the line is asserted and a 1 means it's deasserted. a few lines don't
> > > > + * though, so store the expected value of the status registers when all lines
> > > > + * are asserted.
> > > > + */
> > >
> > > Besides missing capitalization,
> >
> > I'm confused. it was you who wanted all comments to capitalized the same..
> > 64bi
> > if it sounds like bitmap, use bitmap.
> > > I have checked DT definitions and it seems you don't even need the
> > > BIT_MASK() macro,
> > >
> > > > +static const u32 jh7100_reset_asserted[4] = {
> > > > + /* STATUS0 register */
> > > > + BIT_MASK32(JH7100_RST_U74) |
>
> I think we have no BIT_MASK32() for a good reason. Natural alignment is
> always preferable.
>
> > > > + BIT_MASK32(JH7100_RST_VP6_DRESET) |
> > > > + BIT_MASK32(JH7100_RST_VP6_BRESET),
> > > > + /* STATUS1 register */
> > > > + BIT_MASK32(JH7100_RST_HIFI4_DRESET) |
> > > > + BIT_MASK32(JH7100_RST_HIFI4_BRESET),
> > > > + /* STATUS2 register */
> > > > + BIT_MASK32(JH7100_RST_E24),
> > > > + /* STATUS3 register */
> > > > + 0,
> > > > +};
> > >
> > > Yury, do we have any clever (clean) way to initialize a bitmap with
> > > particular bits so that it will be a constant from the beginning? If
> > > no, any suggestion what we can provide to such users?
>
> If you want your array to be a true bitmap, ie, all bitmap functions should
> work with it correctly, you'd initialize it like this:
>
> static const unsigned long jh7100_reset_asserted[] = {
> BITMAP_FROM_U64(BIT_MASK(JH7100_RST_VP6_DRESET) |
> BIT_MASK(JH7100_RST_VP6_BRESET) |
> BIT_MASK(JH7100_RST_HIFI4_DRESET) |
> BIT_MASK(JH7100_RST_HIFI4_BRESET)),
> BITMAP_FROM_U64(BIT_MASK(JH7100_RST_E24)),
> }
My bad, it should be BIT_ULL_MASK.
Thanks,
Yury