[PATCH v6 00/13] Add pinctrl sysfs and RZ/G2L POEG support

[PATCH v6 00/13] Add pinctrl sysfs and RZ/G2L POEG support

[Biju Das]

The output pins of the general PWM timer (GPT) can be disabled by using
the port output enabling function for the GPT (POEG). Specifically, either
of the following ways can be used.

  * Input level detection of the GTETRGA to GTETRGD pins.
  * Output-disable request from the GPT.
  * Register setting(ie, by setting POEGGn.SSF to 1)

This patch series adds support for controlling output disable function using
sysfs in a generic way as described below.

|    A     |    |     B      |    |     C     |    |     D        |  | E |
|user space|--->|pinctrl core|<-->|SoC pinctrl|<-->|Output disable|--|PWM|
|          |    |            |    |           |    |              |  |   |

A executes command to configure a pin group for pin output disable operation
  echo "fname gname conf conf_val" > configure

B parses the command and identifies the binding device associated with that
  pin group

C matches the binding device against the device registered by D for
  configuration operation

D matches the pin group and configure the pins for Output disable operation

Both D and E are linked together by dt(i.e. PWM channel linked with Output
 disable Port)

eg: echo "gpt4-pins gpt4-pins 0 1" > configure ,will activate output 
    disable by the user.

As part of this added a simple sysfs interface to the generic pinctrl
framework.

  /sys/class/pinctrl/
  `-- output-disable/
      |-- configure         (w/o) ask the kernel to configure a pin group
				  for output disable operation.

Patch #1 to #5 crate a framework for registering pin output disable
device to pincontroller.

Patch #7 to patch #13 are just for testing only(GPT Output-disable
request to POEG)

When dead time error occurs or the GTIOCA pin output value is
the same as the GTIOCB pin output value, output protection is
required. GPT detects this condition and generates output disable
requests to POEG based on the settings in the output disable request
permission bits, such as GTINTAD.GRPDTE, GTINTAD.GRPABH,
GTINTAD.GRPABL. After the POEG receives output disable requests from
each channel and calculates external input using an OR operation, the
POEG generates output disable requests to GPT.

POEG handles output disable request and send an event to userspace.
Userspace clears the fault condition and request poeg to cancel
the output disable request.

Logs:
root@smarc-rzg2l:~# /poeg-generic.sh
[POEG]open
16
pwmchip0
Test case 1 user POEG control
 74:          0          0     GICv3 357 Level     10049400.poeg
Read at address  0x10048434 (0xffffb00b0434): 0x0200031B
Read at address  0x10048438 (0xffff911ef438): 0x03000000
Read at address  0x10049400 (0xffffa8dff400): 0x00000030
Test case 2 user GPT control both high
Read at address  0x10048434 (0xffff8537e434): 0x021B031B
Read at address  0x10048438 (0xffffbb73c438): 0x23000000
Read at address  0x10049400 (0xffffa5742400): 0x00000030
gpt ch:3, irq=2
gpt ch:3, irq=2
Read at address  0x10048434 (0xffffb0193434): 0x021B031B
Read at address  0x10048438 (0xffffae0b4438): 0x03000000
Read at address  0x10049400 (0xffff873f5400): 0x00000030
gpt ch:3, irq=2
 74:          6          0     GICv3 357 Level     10049400.poeg
Test case 3 user GPT control both low
Read at address  0x10048434 (0xffffbadb1434): 0x021B031B
Read at address  0x10048438 (0xffff88720438): 0x43000000
Read at address  0x10049400 (0xffff9d7a9400): 0x00000030
gpt ch:3, irq=4
gpt ch:3, irq=4
Read at address  0x10048434 (0xffff95d68434): 0x021B031B
Read at address  0x10048438 (0xffff82428438): 0x03000000
Read at address  0x10049400 (0xffff92716400): 0x00000030
gpt ch:3, irq=4
 74:         12          0     GICv3 357 Level     10049400.poeg
root@smarc-rzg2l:~#

v5->v6:
 * Dropped binding patch as it is accepted for 6.4.
 * Added sysfs support for configuring pin output disable 
   function in a generic way.
v4->v5:
 * Added Rb tag from Rob.
 * Updated kernel version in sysfs doc.
v3->v4:
 * Replaced companion->renesas,gpt for the phandle to gpt instance
 * Replaced renesas,id->renesas,poeg-id
 * Removed default from renesas,poeg-id as default for a required
   property doesn't make much sense.
 * Updated the example and required properties with above changes
v2->v3:
 * Removed Rb tag from Rob as there are some changes introduced.
 * Added companion property, so that poeg can link with gpt device
 * Documented renesas,id, as identifier for POEGG{A,B,C,D}.
 * Updated the binding example.
 * Added sysfs documentation for output_disable
 * PWM_RZG2L_GPT implies ARCH_RZG2L. So removed ARCH_RZG2L dependency
 * Used dev_get_drvdata to get device data
 * Replaced sprintf->sysfs_emit in show().
v1->v2:
 * Updated binding description.
 * Renamed the file poeg-rzg2l->rzg2l-poeg
 * Removed the macro POEGG as there is only single register and
   updated rzg2l_poeg_write() and rzg2l_poeg_read()
 * Updated error handling in probe()
REF->v1:
 * Modelled as pincontrol as most of its configuration is intended to be
   static and moved driver files from soc to pincontrol directory.
 * Updated reg size in dt binding example.
 * Updated Kconfig

REF:
https://lore.kernel.org/linux-renesas-soc/20220510151112.16249-1-biju.das.jz@bp.renesas.com/

Biju Das (13):
  pinctrl: core: Add pinctrl_get_device()
  pinctrl: Add poutdisops variable to struct pinctrl_desc
  pinctrl: Add sysfs support
  pinctrl: renesas: rzg2l: Add pin output disable support
  soc: renesas: Kconfig: Enable pin output disable for RZ/G2L SoC
  drivers: pinctrl: renesas: Add RZ/G2L POEG driver support
  pwm: rzg2l-gpt: Add support for output disable request from gpt
  pinctrl: renesas: rzg2l-poeg: Add support for GPT Output-Disable
    Request
  pwm: rzg2l-gpt: Add support for output disable when both output low
  pinctrl: renesas: rzg2l-poeg: output-disable request from GPT when
    both outputs are low.
  pwm: rzg2l-gpt: Add support for output disable on dead time error
  pinctrl: renesas: rzg2l-poeg: output-disable request from GPT on dead
    time error
  tools/poeg: Add test app for poeg

 Documentation/ABI/testing/sysfs-class-pinctrl |  32 ++
 drivers/pinctrl/Kconfig                       |   4 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/core.c                        |  52 ++
 drivers/pinctrl/output-disable.c              | 148 ++++++
 drivers/pinctrl/output-disable.h              |  32 ++
 drivers/pinctrl/renesas/Kconfig               |   2 +
 drivers/pinctrl/renesas/Makefile              |   2 +
 drivers/pinctrl/renesas/pinctrl-rzg2l.c       |  44 ++
 drivers/pinctrl/renesas/poeg/Kconfig          |  11 +
 drivers/pinctrl/renesas/poeg/Makefile         |   2 +
 drivers/pinctrl/renesas/poeg/rzg2l-poeg.c     | 486 ++++++++++++++++++
 drivers/pwm/pwm-rzg2l-gpt.c                   | 129 +++++
 drivers/soc/renesas/Kconfig                   |   1 +
 include/linux/pinctrl/consumer.h              |   9 +
 include/linux/pinctrl/output-disable.h        |  42 ++
 include/linux/pinctrl/pinctrl-rzg2l.h         |  35 ++
 include/linux/pinctrl/pinctrl.h               |   4 +
 include/linux/pwm/rzg2l-gpt.h                 |  44 ++
 tools/poeg/Build                              |   1 +
 tools/poeg/Makefile                           |  53 ++
 tools/poeg/poeg_app.c                         |  60 +++
 22 files changed, 1194 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-pinctrl
 create mode 100644 drivers/pinctrl/output-disable.c
 create mode 100644 drivers/pinctrl/output-disable.h
 create mode 100644 drivers/pinctrl/renesas/poeg/Kconfig
 create mode 100644 drivers/pinctrl/renesas/poeg/Makefile
 create mode 100644 drivers/pinctrl/renesas/poeg/rzg2l-poeg.c
 create mode 100644 include/linux/pinctrl/output-disable.h
 create mode 100644 include/linux/pinctrl/pinctrl-rzg2l.h
 create mode 100644 include/linux/pwm/rzg2l-gpt.h
 create mode 100644 tools/poeg/Build
 create mode 100644 tools/poeg/Makefile
 create mode 100644 tools/poeg/poeg_app.c

-- 
2.25.1

[PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()

[Biju Das]

Add pinctrl_get_device() to find a device handle associated with
a pincontrol group(i.e. by matching function name and group name for a
device). This device handle can then be used for finding match for the
pin output disable device that protects device against short circuits
on the pins.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v6:
 * New patch
Ref:
 https://lore.kernel.org/linux-renesas-soc/OS0PR01MB5922F5494D3C0862E15F3F8486B39@OS0PR01MB5922.jpnprd01.prod.outlook.com/T/#t
---
 drivers/pinctrl/core.c           | 49 ++++++++++++++++++++++++++++++++
 include/linux/pinctrl/consumer.h |  9 ++++++
 2 files changed, 58 insertions(+)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index d6e6c751255f..2ba222026db4 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -27,6 +27,7 @@
 #include <linux/pinctrl/devinfo.h>
 #include <linux/pinctrl/machine.h>
 #include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
 
 #ifdef CONFIG_GPIOLIB
 #include "../gpio/gpiolib.h"
@@ -1584,6 +1585,54 @@ int pinctrl_select_default_state(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(pinctrl_select_default_state);
 
+static bool pinctrl_get_device_match(struct pinctrl_setting *setting,
+				     const char *fname, const char *gname)
+{
+	struct pinctrl_dev *pctldev = setting->pctldev;
+	const struct pinmux_ops *pmxops = pctldev->desc->pmxops;
+	const struct pinctrl_ops *pctlops = pctldev->desc->pctlops;
+	const char *function = pmxops->get_function_name(pctldev,
+							 setting->data.mux.func);
+	const char *group = pctlops->get_group_name(pctldev,
+						    setting->data.mux.group);
+
+	if ((!strcmp(function, fname)) && (!strcmp(group, gname)))
+		return true;
+
+	return false;
+}
+
+/**
+ * pinctrl_get_device() - returns device associated with a pincontrol group
+ * @fname: function name
+ * @gname: group name
+ */
+struct device *pinctrl_get_device(const char *fname, const char *gname)
+{
+	struct pinctrl *p;
+	struct pinctrl_state *state;
+	struct pinctrl_setting *setting;
+
+	mutex_lock(&pinctrl_list_mutex);
+
+	list_for_each_entry(p, &pinctrl_list, node) {
+		list_for_each_entry(state, &p->states, node) {
+			list_for_each_entry(setting, &state->settings, node) {
+				if (setting->type == PIN_MAP_TYPE_MUX_GROUP &&
+				    pinctrl_get_device_match(setting, fname, gname)) {
+					mutex_unlock(&pinctrl_list_mutex);
+					return p->dev;
+				}
+			}
+		}
+	}
+
+	mutex_unlock(&pinctrl_list_mutex);
+
+	return ERR_PTR(-EINVAL);
+}
+EXPORT_SYMBOL_GPL(pinctrl_get_device);
+
 #ifdef CONFIG_PM
 
 /**
diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h
index 4729d54e8995..6ff8857c0a9c 100644
--- a/include/linux/pinctrl/consumer.h
+++ b/include/linux/pinctrl/consumer.h
@@ -42,6 +42,9 @@ extern struct pinctrl * __must_check devm_pinctrl_get(struct device *dev);
 extern void devm_pinctrl_put(struct pinctrl *p);
 extern int pinctrl_select_default_state(struct device *dev);
 
+extern struct device * __must_check pinctrl_get_device(const char *fname,
+						       const char *gname);
+
 #ifdef CONFIG_PM
 extern int pinctrl_pm_select_default_state(struct device *dev);
 extern int pinctrl_pm_select_sleep_state(struct device *dev);
@@ -142,6 +145,12 @@ static inline int pinctrl_pm_select_idle_state(struct device *dev)
 	return 0;
 }
 
+static inline struct device * __must_check pinctrl_get_device(const char *fname,
+							      const char *gname)
+{
+	return NULL;
+}
+
 #endif /* CONFIG_PINCTRL */
 
 static inline struct pinctrl * __must_check pinctrl_get_select(struct device *dev,
-- 
2.25.1

[PATCH v6 02/13] pinctrl: Add poutdisops variable to struct pinctrl_desc

[Biju Das]

Add pin output disable operations vtable to struct pinctrl_desc.
This is needed for configuring IPs that support pin output disable
function.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v6:
 * New patch
Ref:
 https://lore.kernel.org/linux-renesas-soc/OS0PR01MB5922F5494D3C0862E15F3F8486B39@OS0PR01MB5922.jpnprd01.prod.outlook.com/T/#t
---
 include/linux/pinctrl/output-disable.h | 42 ++++++++++++++++++++++++++
 include/linux/pinctrl/pinctrl.h        |  4 +++
 2 files changed, 46 insertions(+)
 create mode 100644 include/linux/pinctrl/output-disable.h

diff --git a/include/linux/pinctrl/output-disable.h b/include/linux/pinctrl/output-disable.h
new file mode 100644
index 000000000000..7f63dcc93e44
--- /dev/null
+++ b/include/linux/pinctrl/output-disable.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Interface the output disable portions of the pinctrl subsystem
+ *
+ * Copyright (C) 2023 Renesas Electronics Corporation
+ * This interface is used in the core to keep track of output_disable pins.
+ *
+ * Author: Biju Das <biju.das.jz@bp.renesas.com>
+ */
+#ifndef __LINUX_PINCTRL_OUTPUT_DISABLE_H
+#define __LINUX_PINCTRL_OUTPUT_DISABLE_H
+
+#include <linux/types.h>
+#include <linux/pinctrl/consumer.h>
+
+struct pinctrl_dev;
+struct seq_file;
+
+enum pin_output_disable_conf {
+	PINCTRL_OUTPUT_DISABLE_BY_USER,
+	PINCTRL_OUTPUT_DISABLE_BY_SOC_ON_PIN_OUTPUT_HIGH,
+	PINCTRL_OUTPUT_DISABLE_BY_SOC_ON_PIN_OUTPUT_LOW,
+	PINCTRL_OUTPUT_DISABLE_BY_SOC_ON_DEAD_TIME_ERROR,
+	PINCTRL_OUTPUT_DISABLE_BY_LEVEL_DETECTION,
+};
+
+/**
+ * struct pin_config_set - pin output disable config operations, to be
+ * implemented by pin configuration capable drivers.
+ * @pin_output_disable_config_set: for pin controllers that want to use the
+ *	generic interface, this flag tells the framework that it's generic.
+ */
+struct pin_output_disable_ops {
+	int (*pin_output_disable_config_set)(struct pinctrl_dev *pctldev,
+					     struct device *dev,
+					     const char *fname,
+					     const char *gname,
+					     enum pin_output_disable_conf conf,
+					     unsigned int conf_val);
+};
+
+#endif /* __LINUX_PINCTRL_OUTPUT_DISABLE_H */
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 4d252ea00ed1..3b5beee49039 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -25,6 +25,7 @@ struct pinconf_ops;
 struct pinctrl_dev;
 struct pinctrl_map;
 struct pinmux_ops;
+struct pin_output_disable_ops;
 
 /**
  * struct pingroup - provides information on pingroup
@@ -135,6 +136,8 @@ struct pinctrl_ops {
  * @pmxops: pinmux operations vtable, if you support pinmuxing in your driver
  * @confops: pin config operations vtable, if you support pin configuration in
  *	your driver
+ * @poutdisops: pin output disable operations vtable, if you support pin output
+ *	disable in your driver
  * @owner: module providing the pin controller, used for refcounting
  * @num_custom_params: Number of driver-specific custom parameters to be parsed
  *	from the hardware description
@@ -154,6 +157,7 @@ struct pinctrl_desc {
 	const struct pinctrl_ops *pctlops;
 	const struct pinmux_ops *pmxops;
 	const struct pinconf_ops *confops;
+	const struct pin_output_disable_ops *poutdisops;
 	struct module *owner;
 #ifdef CONFIG_GENERIC_PINCONF
 	unsigned int num_custom_params;
-- 
2.25.1

[PATCH v6 03/13] pinctrl: Add sysfs support

[Biju Das]

Add a simple sysfs interface to the generic pinctrl framework
for configuring pins for output disable operation.

/sys/class/pinctrl/
  `-- output-disable/
      |-- configure    (w/o) ask the kernel to configure a pin group
                             for output disable operation.

  echo "<group-name function-name 0 1>" > configure

The existing "pinmux-functions" debugfs file lists the pin functions
registered for the pin controller. For example:

  function 0: usb0, groups = [ usb0 ]
  function 1: usb1, groups = [ usb1 ]
  function 2: gpt4-pins, groups = [ gpt4-pins ]
  function 3: scif0, groups = [ scif0 ]
  function 4: scif2, groups = [ scif2 ]
  function 5: spi1, groups = [ spi1 ]

To configure gpt4-pins for output disable activation by user:

  echo "gpt4-pins gpt4-pins 0 1" > configure

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v6:
 * New patch
Ref:
 https://lore.kernel.org/linux-renesas-soc/OS0PR01MB5922F5494D3C0862E15F3F8486B39@OS0PR01MB5922.jpnprd01.prod.outlook.com/T/#t
---
 Documentation/ABI/testing/sysfs-class-pinctrl |  32 ++++
 drivers/pinctrl/Kconfig                       |   4 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/core.c                        |   3 +
 drivers/pinctrl/output-disable.c              | 148 ++++++++++++++++++
 drivers/pinctrl/output-disable.h              |  32 ++++
 6 files changed, 220 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-pinctrl
 create mode 100644 drivers/pinctrl/output-disable.c
 create mode 100644 drivers/pinctrl/output-disable.h

diff --git a/Documentation/ABI/testing/sysfs-class-pinctrl b/Documentation/ABI/testing/sysfs-class-pinctrl
new file mode 100644
index 000000000000..cdf47ea77c32
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-pinctrl
@@ -0,0 +1,32 @@
+What:		/sys/class/pinctrl
+Date:		March 2023
+KernelVersion:	6.4
+Contact:	Biju Das <biju.das.jz@bp.renesas.com>
+Description:
+		The pinctrl class sub-directory belongs to the Generic pincontrol
+		Framework and provides a sysfs interface for using pincontrol
+		configurations.
+
+What:		/sys/class/pinctrl/output-disable
+Date:		March 2023
+KernelVersion:	6.4
+Contact:	Biju Das <biju.das.jz@bp.renesas.com>
+Description:
+		A /sys/class/pinctrl/output-disable directory is created for
+		pin output disable function.
+
+What:		/sys/class/pinctrl/output-disable/configure
+Date:		March 2023
+KernelVersion:	6.4
+Contact:	Biju Das <biju.das.jz@bp.renesas.com>
+Description:
+		The configuration for pin output disable IP.
+
+		Write the following string to configure the pin output disable:
+
+		- "fname gname conf conf_val"
+
+		fname is function name,
+		gname is group name,
+		conf is configuration
+		conf_val is 0 or 1, to disable/enable the above conf.
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index dcb53c4a9584..3553d76c3577 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -21,6 +21,10 @@ config GENERIC_PINMUX_FUNCTIONS
 config PINCONF
 	bool "Support pin configuration controllers" if COMPILE_TEST
 
+config PIN_OUTPUT_DISABLE
+	bool
+	default y if SYSFS
+
 config GENERIC_PINCONF
 	bool
 	select PINCONF
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index d5939840bb2a..e923adaa2b9c 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -6,6 +6,7 @@ subdir-ccflags-$(CONFIG_DEBUG_PINCTRL)	+= -DDEBUG
 obj-y				+= core.o pinctrl-utils.o
 obj-$(CONFIG_PINMUX)		+= pinmux.o
 obj-$(CONFIG_PINCONF)		+= pinconf.o
+obj-$(CONFIG_PIN_OUTPUT_DISABLE) += output-disable.o
 obj-$(CONFIG_GENERIC_PINCONF)	+= pinconf-generic.o
 obj-$(CONFIG_OF)		+= devicetree.o
 
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 2ba222026db4..3fce50786754 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -36,6 +36,7 @@
 
 #include "core.h"
 #include "devicetree.h"
+#include "output-disable.h"
 #include "pinconf.h"
 #include "pinmux.h"
 
@@ -2171,6 +2172,7 @@ int pinctrl_enable(struct pinctrl_dev *pctldev)
 	mutex_unlock(&pinctrldev_list_mutex);
 
 	pinctrl_init_device_debugfs(pctldev);
+	output_disable_init_device_sysfs(pctldev);
 
 	return 0;
 }
@@ -2252,6 +2254,7 @@ void pinctrl_unregister(struct pinctrl_dev *pctldev)
 
 	mutex_lock(&pctldev->mutex);
 	pinctrl_remove_device_debugfs(pctldev);
+	output_disable_remove_device_sysfs(pctldev);
 	mutex_unlock(&pctldev->mutex);
 
 	if (!IS_ERR_OR_NULL(pctldev->p))
diff --git a/drivers/pinctrl/output-disable.c b/drivers/pinctrl/output-disable.c
new file mode 100644
index 000000000000..fb233b10b7df
--- /dev/null
+++ b/drivers/pinctrl/output-disable.c
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Generic pin output-disable sysfs interface for pinctrl
+ * Copyright (C) 2023  <biju.das.jz@bp.renesas.com>
+ */
+
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/types.h>
+
+#include <linux/pinctrl/consumer.h>
+#include <linux/pinctrl/output-disable.h>
+#include <linux/pinctrl/pinctrl.h>
+
+#include "core.h"
+#include "output-disable.h"
+
+#define OUTPUT_DISABLE_BUF_MAX 128
+
+static int output_disable_get_vars(char *src, char **dst)
+{
+	char *result = (char *)dst;
+
+	/* find a separator which is a spacelike character */
+	for (result = src; !isspace(*result); result++) {
+		if (*result == '\0')
+			return -EINVAL;
+	}
+	*result = '\0';
+
+	/* drop extra spaces between function and group names */
+	result = skip_spaces(result + 1);
+	if (*result == '\0')
+		return -EINVAL;
+
+	*dst = result;
+
+	return 0;
+}
+
+static ssize_t configure_store(struct device *parent,
+			       struct device_attribute *attr,
+			       const char *src_buf,
+			       size_t len)
+{
+	struct pinctrl_dev *pctldev = dev_get_drvdata(parent);
+	const struct pin_output_disable_ops *poutdisops = pctldev->desc->poutdisops;
+	char *buf, *gname, *fname, *conf, *conf_val;
+	int config, config_val;
+	struct device *dev;
+	int ret;
+
+	if (len > OUTPUT_DISABLE_BUF_MAX)
+		return -ENOMEM;
+
+	buf = kzalloc(OUTPUT_DISABLE_BUF_MAX, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	memcpy(buf, src_buf, len);
+	buf[len - 1] = '\0';
+
+	/* remove leading and trailing spaces of input buffer */
+	gname = strstrip(buf);
+	if (*gname == '\0') {
+		ret = -EINVAL;
+		goto exit_free_buf;
+	}
+
+	ret = output_disable_get_vars(gname, &fname);
+	if (ret < 0)
+		goto exit_free_buf;
+
+	ret = output_disable_get_vars(fname, &conf);
+	if (ret < 0)
+		goto exit_free_buf;
+
+	ret = output_disable_get_vars(conf, &conf_val);
+	if (ret < 0)
+		goto exit_free_buf;
+
+	ret = kstrtoint(conf, 0, &config);
+	if (ret)
+		goto exit_free_buf;
+
+	ret = kstrtoint(conf_val, 0, &config_val);
+	if (ret)
+		goto exit_free_buf;
+
+	dev = pinctrl_get_device(fname, gname);
+	if (IS_ERR(dev)) {
+		dev_err(pctldev->dev, "invalid function %s in map table\n", fname);
+		goto exit_free_buf;
+	}
+
+	if (poutdisops)
+		poutdisops->pin_output_disable_config_set(pctldev, dev,
+							  fname, gname,
+							  config, config_val);
+
+exit_free_buf:
+	kfree(buf);
+
+	return ret ? : len;
+}
+static DEVICE_ATTR_WO(configure);
+
+static struct attribute *pinctrl_attrs[] = {
+	&dev_attr_configure.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(pinctrl);
+
+static struct class pinctrl_class = {
+	.name = "pinctrl",
+	.owner = THIS_MODULE,
+	.dev_groups = pinctrl_groups,
+};
+
+void output_disable_init_device_sysfs(struct pinctrl_dev *pctldev)
+{
+	struct device *parent;
+
+	parent = device_create(&pinctrl_class, pctldev->dev, MKDEV(0, 0),
+			       pctldev, "output-disable");
+	if (IS_ERR(parent))
+		dev_err(pctldev->dev, "device_create failed for pinctrl sysfs\n");
+}
+
+void output_disable_remove_device_sysfs(struct pinctrl_dev *pctldev)
+{
+	device_destroy(&pinctrl_class, MKDEV(0, 0));
+}
+
+static int __init pinctrl_sysfs_init(void)
+{
+	return class_register(&pinctrl_class);
+}
+subsys_initcall(pinctrl_sysfs_init);
+
+static void __exit pinctrl_sysfs_exit(void)
+{
+	class_unregister(&pinctrl_class);
+}
+module_exit(pinctrl_sysfs_exit);
diff --git a/drivers/pinctrl/output-disable.h b/drivers/pinctrl/output-disable.h
new file mode 100644
index 000000000000..f933965599d2
--- /dev/null
+++ b/drivers/pinctrl/output-disable.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Internal interface between the core pin control system and the
+ * ouput-disable portions
+ *
+ * Copyright (C) 2023 Renesas Electronics Corporation
+ * Based on bits of pinmux.c
+ *
+ * Author: Biju Das <biju.das.jz@bp.renesas.com>
+ */
+
+#include <linux/types.h>
+
+struct dentry;
+struct seq_file;
+
+struct pinctrl_dev;
+
+#ifdef CONFIG_PIN_OUTPUT_DISABLE
+void output_disable_init_device_sysfs(struct pinctrl_dev *pctldev);
+void output_disable_remove_device_sysfs(struct pinctrl_dev *pctldev);
+
+#else
+static inline void output_disable_init_device_sysfs(struct pinctrl_dev *pctldev)
+{
+}
+
+static inline void output_disable_remove_device_sysfs(struct pinctrl_dev *pctldev)
+{
+}
+
+#endif
-- 
2.25.1

[PATCH v6 04/13] pinctrl: renesas: rzg2l: Add pin output disable support

[Biju Das]

Add pin output disable support for RZ/G2L alike SoCs by registering with
pincontrol core and provide callback mechanism for configuring pin output
disable device by matching with device registered with callback against
the device found by the core framework for a given function name and device
name.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v6:
 * New patch
Ref:
 https://lore.kernel.org/linux-renesas-soc/OS0PR01MB5922F5494D3C0862E15F3F8486B39@OS0PR01MB5922.jpnprd01.prod.outlook.com/T/#t
---
 drivers/pinctrl/renesas/pinctrl-rzg2l.c | 44 +++++++++++++++++++++++++
 include/linux/pinctrl/pinctrl-rzg2l.h   | 26 +++++++++++++++
 2 files changed, 70 insertions(+)
 create mode 100644 include/linux/pinctrl/pinctrl-rzg2l.h

diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index 04b31f0c6b34..137d085077d8 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -17,9 +17,11 @@
 #include <linux/spinlock.h>
 
 #include <linux/pinctrl/consumer.h>
+#include <linux/pinctrl/output-disable.h>
 #include <linux/pinctrl/pinconf-generic.h>
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinctrl-rzg2l.h>
 #include <linux/pinctrl/pinmux.h>
 
 #include <dt-bindings/pinctrl/rzg2l-pinctrl.h>
@@ -136,6 +138,10 @@ struct rzg2l_pinctrl_data {
 	unsigned int n_dedicated_pins;
 };
 
+static struct device *rzg2l_poeg_dev;
+static output_disable_cb rzg2l_poeg_cb;
+static void *rzg2l_poeg_context;
+
 struct rzg2l_pinctrl {
 	struct pinctrl_dev		*pctl;
 	struct pinctrl_desc		desc;
@@ -747,6 +753,28 @@ static int rzg2l_pinctrl_pinconf_group_get(struct pinctrl_dev *pctldev,
 	return 0;
 };
 
+static int rzg2l_pinctrl_output_disable_set(struct pinctrl_dev *pctldev,
+					    struct device *dev,
+					    const char *fname,
+					    const char *gname,
+					    enum pin_output_disable_conf conf,
+					    unsigned int conf_val)
+{
+	struct rzg2l_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+
+	if (!rzg2l_poeg_cb || !rzg2l_poeg_context) {
+		dev_err(pctrl->dev, "No callback function registered\n");
+		return -EINVAL;
+	}
+
+	if (!rzg2l_poeg_dev || rzg2l_poeg_dev != dev) {
+		dev_err(pctrl->dev, "Device match failed\n");
+		return -EINVAL;
+	}
+
+	return rzg2l_poeg_cb(rzg2l_poeg_context, fname, gname, conf, conf_val);
+}
+
 static const struct pinctrl_ops rzg2l_pinctrl_pctlops = {
 	.get_groups_count = pinctrl_generic_get_group_count,
 	.get_group_name = pinctrl_generic_get_group_name,
@@ -772,6 +800,10 @@ static const struct pinconf_ops rzg2l_pinctrl_confops = {
 	.pin_config_config_dbg_show = pinconf_generic_dump_config,
 };
 
+static const struct pin_output_disable_ops rzg2l_pinctrl_output_disable_fops = {
+	.pin_output_disable_config_set = rzg2l_pinctrl_output_disable_set,
+};
+
 static int rzg2l_gpio_request(struct gpio_chip *chip, unsigned int offset)
 {
 	struct rzg2l_pinctrl *pctrl = gpiochip_get_data(chip);
@@ -1320,6 +1352,17 @@ static void rzg2l_init_irq_valid_mask(struct gpio_chip *gc,
 	}
 }
 
+int rzg2l_output_disable_cb_register(struct device *dev, output_disable_cb cb,
+				     void *cb_context)
+{
+	rzg2l_poeg_dev = dev;
+	rzg2l_poeg_cb = cb;
+	rzg2l_poeg_context = cb_context;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(rzg2l_output_disable_cb_register);
+
 static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
 {
 	struct device_node *np = pctrl->dev->of_node;
@@ -1404,6 +1447,7 @@ static int rzg2l_pinctrl_register(struct rzg2l_pinctrl *pctrl)
 	pctrl->desc.pctlops = &rzg2l_pinctrl_pctlops;
 	pctrl->desc.pmxops = &rzg2l_pinctrl_pmxops;
 	pctrl->desc.confops = &rzg2l_pinctrl_confops;
+	pctrl->desc.poutdisops = &rzg2l_pinctrl_output_disable_fops;
 	pctrl->desc.owner = THIS_MODULE;
 
 	pins = devm_kcalloc(pctrl->dev, pctrl->desc.npins, sizeof(*pins), GFP_KERNEL);
diff --git a/include/linux/pinctrl/pinctrl-rzg2l.h b/include/linux/pinctrl/pinctrl-rzg2l.h
new file mode 100644
index 000000000000..a49b4c5f8908
--- /dev/null
+++ b/include/linux/pinctrl/pinctrl-rzg2l.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_PINCTRL_RZG2L_H__
+#define __LINUX_PINCTRL_RZG2L_H__
+
+#include <linux/pinctrl/output-disable.h>
+
+typedef int (*output_disable_cb) (void *context,
+				  const char *fname,
+				  const char *gname,
+				  enum pin_output_disable_conf conf,
+				  unsigned int conf_val);
+
+#if IS_ENABLED(CONFIG_PINCTRL_RZG2L)
+int rzg2l_output_disable_cb_register(struct device *dev,
+				     output_disable_cb cb,
+				     void *cb_context);
+#else
+static inline int rzg2l_output_disable_cb_register(struct device *dev,
+						   output_disable_cb cb,
+						   void *cb_context)
+{
+	return -EINVAL;
+}
+#endif
+
+#endif /* __LINUX_PINCTRL_RZG2L_H__ */
-- 
2.25.1

[PATCH v6 05/13] soc: renesas: Kconfig: Enable pin output disable for RZ/G2L SoC

[Biju Das]

Select PIN_OUTPUT_DISABLE config option if ARCH_RZG2L is enabled
so that pin output disable is enabled on RZ/G2L (and alike) SoCs.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v6:
 * New patch
Ref:
 https://lore.kernel.org/linux-renesas-soc/OS0PR01MB5922F5494D3C0862E15F3F8486B39@OS0PR01MB5922.jpnprd01.prod.outlook.com/T/#t
---
 drivers/soc/renesas/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soc/renesas/Kconfig b/drivers/soc/renesas/Kconfig
index 4e8b51ba2266..727ea6149631 100644
--- a/drivers/soc/renesas/Kconfig
+++ b/drivers/soc/renesas/Kconfig
@@ -44,6 +44,7 @@ config ARCH_RMOBILE
 
 config ARCH_RZG2L
 	bool
+	select PIN_OUTPUT_DISABLE
 	select PM
 	select PM_GENERIC_DOMAINS
 	select RENESAS_RZG2L_IRQC
-- 
2.25.1

[PATCH v6 06/13] drivers: pinctrl: renesas: Add RZ/G2L POEG driver support

[Biju Das]

The output pins of the RZ/G2L general PWM timer (GPT) can be disabled
by using the port output enabling function for the GPT (POEG).

Add basic support using s/w control through generic pincontrol sysfs to
enable/disable output from GPT by registering with RZ/G2L pincontrol
driver.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v5->v6:
 * Dropped sysfs and is handled in generic driver.
v4->v5:
 * Updated kernel version in sysfs doc.
v3->v4:
 * Updated commit description.
v2->v3:
 * Added sysfs documentation for output_disable
 * PWM_RZG2L_GPT implies ARCH_RZG2L. So removed ARCH_RZG2L dependency
 * Used dev_get_drvdata to get device data
 * Replaced sprintf->sysfs_emit in show().
v1->v2:
 * Renamed the file poeg-rzg2l->rzg2l-poeg
 * Removed the macro POEGG as there is only single register and
   updated rzg2l_poeg_write() and rzg2l_poeg_read()
 * Updated error handling in probe()
Ref->v1:
 * Moved driver files from soc to pincontrol directory
 * Updated KConfig
---
 drivers/pinctrl/renesas/Kconfig           |   2 +
 drivers/pinctrl/renesas/Makefile          |   2 +
 drivers/pinctrl/renesas/poeg/Kconfig      |  11 +
 drivers/pinctrl/renesas/poeg/Makefile     |   2 +
 drivers/pinctrl/renesas/poeg/rzg2l-poeg.c | 254 ++++++++++++++++++++++
 5 files changed, 271 insertions(+)
 create mode 100644 drivers/pinctrl/renesas/poeg/Kconfig
 create mode 100644 drivers/pinctrl/renesas/poeg/Makefile
 create mode 100644 drivers/pinctrl/renesas/poeg/rzg2l-poeg.c

diff --git a/drivers/pinctrl/renesas/Kconfig b/drivers/pinctrl/renesas/Kconfig
index 0903a0a41831..92bdc2e1e125 100644
--- a/drivers/pinctrl/renesas/Kconfig
+++ b/drivers/pinctrl/renesas/Kconfig
@@ -308,4 +308,6 @@ config PINCTRL_PFC_SHX3
 	bool "pin control support for SH-X3" if COMPILE_TEST
 	select PINCTRL_SH_FUNC_GPIO
 
+source "drivers/pinctrl/renesas/poeg/Kconfig"
+
 endmenu
diff --git a/drivers/pinctrl/renesas/Makefile b/drivers/pinctrl/renesas/Makefile
index 558b30ce0dec..de1bb592fbf3 100644
--- a/drivers/pinctrl/renesas/Makefile
+++ b/drivers/pinctrl/renesas/Makefile
@@ -52,6 +52,8 @@ obj-$(CONFIG_PINCTRL_RZG2L)	+= pinctrl-rzg2l.o
 obj-$(CONFIG_PINCTRL_RZN1)	+= pinctrl-rzn1.o
 obj-$(CONFIG_PINCTRL_RZV2M)	+= pinctrl-rzv2m.o
 
+obj-$(CONFIG_POEG_RZG2L)	+= poeg/
+
 ifeq ($(CONFIG_COMPILE_TEST),y)
 CFLAGS_pfc-sh7203.o	+= -I$(srctree)/arch/sh/include/cpu-sh2a
 CFLAGS_pfc-sh7264.o	+= -I$(srctree)/arch/sh/include/cpu-sh2a
diff --git a/drivers/pinctrl/renesas/poeg/Kconfig b/drivers/pinctrl/renesas/poeg/Kconfig
new file mode 100644
index 000000000000..306e8ae81cb2
--- /dev/null
+++ b/drivers/pinctrl/renesas/poeg/Kconfig
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0
+config POEG_RZG2L
+	tristate "Renesas RZ/G2L poeg support"
+	depends on PWM_RZG2L_GPT || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  This driver exposes the Port Output Enable for GPT(POEG) found
+	  in Renesas RZ/G2L alike SoCs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called rzg2l-poeg.
diff --git a/drivers/pinctrl/renesas/poeg/Makefile b/drivers/pinctrl/renesas/poeg/Makefile
new file mode 100644
index 000000000000..610bdd6182be
--- /dev/null
+++ b/drivers/pinctrl/renesas/poeg/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_POEG_RZG2L)	+= rzg2l-poeg.o
diff --git a/drivers/pinctrl/renesas/poeg/rzg2l-poeg.c b/drivers/pinctrl/renesas/poeg/rzg2l-poeg.c
new file mode 100644
index 000000000000..30f4352e257d
--- /dev/null
+++ b/drivers/pinctrl/renesas/poeg/rzg2l-poeg.c
@@ -0,0 +1,254 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/G2L Port Output Enable for GPT (POEG) driver
+ *
+ * Copyright (C) 2022 Renesas Electronics Corporation
+ */
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/pinctrl/pinctrl-rzg2l.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+
+#define POEGG_SSF	BIT(3)
+
+#define RZG2L_POEG_MAX_INDEX		3
+
+#define RZG2L_GPT_MAX_HW_CHANNELS	8
+#define RZG2L_GPT_INVALID_CHANNEL	0xff
+
+struct rzg2l_poeg_chip {
+	struct device *gpt_dev;
+	struct reset_control *rstc;
+	void __iomem *mmio;
+	u8 index;
+	u8 gpt_channels[RZG2L_GPT_MAX_HW_CHANNELS];
+};
+
+static const char * const rzg2l_gpt_pins[] = {
+	"gpt0-pins",
+	"gpt1-pins",
+	"gpt2-pins",
+	"gpt3-pins",
+	"gpt4-pins",
+	"gpt5-pins",
+	"gpt6-pins",
+	"gpt7-pins",
+};
+
+static void rzg2l_poeg_write(struct rzg2l_poeg_chip *chip, u32 data)
+{
+	iowrite32(data, chip->mmio);
+}
+
+static u32 rzg2l_poeg_read(struct rzg2l_poeg_chip *chip)
+{
+	return ioread32(chip->mmio);
+}
+
+static int rzg2l_poeg_output_disable_user(struct rzg2l_poeg_chip *chip,
+					  bool enable)
+{
+	u32 reg_val;
+
+	reg_val = rzg2l_poeg_read(chip);
+	if (enable)
+		reg_val |= POEGG_SSF;
+	else
+		reg_val &= ~POEGG_SSF;
+
+	rzg2l_poeg_write(chip, reg_val);
+
+	return 0;
+}
+
+static int rzg2l_poeg_cb(void *context, const char *fname, const char *gname,
+			 enum pin_output_disable_conf conf,
+			 unsigned int conf_val)
+{
+	bool pin_match = false;
+	int ret, i;
+
+	for (i = 0; i < RZG2L_GPT_MAX_HW_CHANNELS; i++) {
+		if ((!strcmp(rzg2l_gpt_pins[i], fname)) &&
+		    (!strcmp(rzg2l_gpt_pins[i], gname))) {
+			pin_match = true;
+			break;
+		}
+	}
+
+	if (!pin_match)
+		return -EINVAL;
+
+	switch (conf) {
+	case PINCTRL_OUTPUT_DISABLE_BY_USER:
+		ret = rzg2l_poeg_output_disable_user(context, !!conf_val);
+		break;
+	case PINCTRL_OUTPUT_DISABLE_BY_SOC_ON_PIN_OUTPUT_HIGH:
+	case PINCTRL_OUTPUT_DISABLE_BY_SOC_ON_PIN_OUTPUT_LOW:
+	case PINCTRL_OUTPUT_DISABLE_BY_SOC_ON_DEAD_TIME_ERROR:
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static bool rzg2l_poeg_get_linked_gpt_channels(struct platform_device *pdev,
+					       struct rzg2l_poeg_chip *chip,
+					       struct device_node *gpt_np,
+					       u8 poeg_id)
+{
+	struct of_phandle_args of_args;
+	bool ret = false;
+	unsigned int i;
+	u32 poeg_grp;
+	int cells;
+	int err;
+
+	cells = of_property_count_u32_elems(gpt_np, "renesas,poegs");
+	if (cells == -EINVAL)
+		return ret;
+
+	for (i = 0 ; i < RZG2L_GPT_MAX_HW_CHANNELS; i++)
+		chip->gpt_channels[i] = RZG2L_GPT_INVALID_CHANNEL;
+
+	cells >>= 1;
+	for (i = 0; i < cells; i++) {
+		err = of_parse_phandle_with_fixed_args(gpt_np,
+						       "renesas,poegs", 1, i,
+						       &of_args);
+		if (err) {
+			dev_err(&pdev->dev,
+				"Failed to parse 'renesas,poegs' property\n");
+			break;
+		}
+
+		if (of_args.args[0] >= RZG2L_GPT_MAX_HW_CHANNELS) {
+			dev_err(&pdev->dev, "Invalid channel %d >= %d\n",
+				of_args.args[0], RZG2L_GPT_MAX_HW_CHANNELS);
+			of_node_put(of_args.np);
+			break;
+		}
+
+		if (!of_property_read_u32(of_args.np, "renesas,poeg-id", &poeg_grp)) {
+			if (poeg_grp == poeg_id) {
+				chip->gpt_channels[poeg_grp] = poeg_id;
+				ret = true;
+			}
+		}
+
+		of_node_put(of_args.np);
+	}
+
+	return ret;
+}
+
+static const struct of_device_id rzg2l_poeg_of_table[] = {
+	{ .compatible = "renesas,rzg2l-poeg", },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, rzg2l_poeg_of_table);
+
+static void rzg2l_poeg_cleanup(void *data)
+{
+	struct rzg2l_poeg_chip *chip = data;
+
+	put_device(chip->gpt_dev);
+}
+
+static int rzg2l_poeg_probe(struct platform_device *pdev)
+{
+	struct platform_device *gpt_pdev = NULL;
+	struct rzg2l_poeg_chip *chip;
+	bool gpt_linked = false;
+	struct device_node *np;
+	u32 val;
+	int ret;
+
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	if (!of_property_read_u32(pdev->dev.of_node, "renesas,poeg-id", &val))
+		chip->index = val;
+
+	if (chip->index > RZG2L_POEG_MAX_INDEX)
+		return -EINVAL;
+
+	np = of_parse_phandle(pdev->dev.of_node, "renesas,gpt", 0);
+	if (np)
+		gpt_pdev = of_find_device_by_node(np);
+
+	gpt_linked = rzg2l_poeg_get_linked_gpt_channels(pdev, chip, np,
+							chip->index);
+	of_node_put(np);
+	if (!gpt_pdev)
+		return -ENODEV;
+
+	chip->gpt_dev = &gpt_pdev->dev;
+	ret = devm_add_action_or_reset(&pdev->dev,
+				       rzg2l_poeg_cleanup, chip);
+	if (ret < 0)
+		return ret;
+
+	chip->mmio = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(chip->mmio))
+		return PTR_ERR(chip->mmio);
+
+	if (gpt_linked)
+		rzg2l_output_disable_cb_register(chip->gpt_dev,
+						 rzg2l_poeg_cb, chip);
+
+	chip->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+	if (IS_ERR(chip->rstc))
+		return dev_err_probe(&pdev->dev, PTR_ERR(chip->rstc),
+				     "get reset failed\n");
+
+	ret = reset_control_deassert(chip->rstc);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, chip);
+	pm_runtime_enable(&pdev->dev);
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "pm_runtime_resume_and_get failed: %d\n", ret);
+		goto err_pm_disable;
+	}
+
+	return 0;
+
+err_pm_disable:
+	pm_runtime_disable(&pdev->dev);
+	reset_control_assert(chip->rstc);
+	return ret;
+}
+
+static int rzg2l_poeg_remove(struct platform_device *pdev)
+{
+	struct rzg2l_poeg_chip *chip = platform_get_drvdata(pdev);
+
+	pm_runtime_put(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	reset_control_assert(chip->rstc);
+
+	return 0;
+}
+
+static struct platform_driver rzg2l_poeg_driver = {
+	.driver = {
+		.name = "rzg2l-poeg",
+		.of_match_table = of_match_ptr(rzg2l_poeg_of_table),
+	},
+	.probe = rzg2l_poeg_probe,
+	.remove = rzg2l_poeg_remove,
+};
+module_platform_driver(rzg2l_poeg_driver);
+
+MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
+MODULE_DESCRIPTION("Renesas RZ/G2L POEG Driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

[DO NOT APPLY PATCH v6 07/13] pwm: rzg2l-gpt: Add support for output disable request from gpt

[Biju Das]

When dead time error occurs or the GTIOCA pin output value is
the same as the GTIOCB pin output value, output protection is
required. GPT detects this condition and generates output disable
requests to POEG based on the settings in the output disable request
permission bits, such as GTINTAD.GRPDTE, GTINTAD.GRPABH,
GTINTAD.GRPABL. After the POEG receives output disable requests from
each channel and calculates external input using an OR operation, the
POEG generates output disable requests to GPT.

This patch adds support for output disable request from gpt,
when same time output level is high.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/pwm/pwm-rzg2l-gpt.c   | 111 ++++++++++++++++++++++++++++++++++
 include/linux/pwm/rzg2l-gpt.h |  32 ++++++++++
 2 files changed, 143 insertions(+)
 create mode 100644 include/linux/pwm/rzg2l-gpt.h

diff --git a/drivers/pwm/pwm-rzg2l-gpt.c b/drivers/pwm/pwm-rzg2l-gpt.c
index 9f3e2f7635a8..2f138e95f752 100644
--- a/drivers/pwm/pwm-rzg2l-gpt.c
+++ b/drivers/pwm/pwm-rzg2l-gpt.c
@@ -25,6 +25,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/pwm.h>
+#include <linux/pwm/rzg2l-gpt.h>
 #include <linux/reset.h>
 #include <linux/time.h>
 
@@ -32,6 +33,7 @@
 #define RZG2L_GTUDDTYC		0x30
 #define RZG2L_GTIOR		0x34
 #define RZG2L_GTINTAD		0x38
+#define RZG2L_GTST		0x3c
 #define RZG2L_GTBER		0x40
 #define RZG2L_GTCNT		0x48
 #define RZG2L_GTCCRA		0x4c
@@ -72,6 +74,12 @@
 	(FIELD_PREP(RZG2L_GTIOR_GTIOB, RZG2L_INIT_OUT_LO_OUT_LO_END_TOGGLE) | RZG2L_GTIOR_OBE)
 
 #define RZG2L_GTINTAD_GRP_MASK			GENMASK(25, 24)
+#define RZG2L_GTINTAD_OUTPUT_DISABLE_SAME_LEVEL_HIGH	BIT(29)
+
+#define RZG2L_GTST_OABHF			BIT(29)
+#define RZG2L_GTST_OABLF			BIT(30)
+
+#define RZG2L_GTST_POEG_IRQ_MASK		GENMASK(30, 28)
 
 #define RZG2L_GTCCR(i) (0x4c + 4 * (i))
 
@@ -431,6 +439,109 @@ static DEFINE_RUNTIME_DEV_PM_OPS(rzg2l_gpt_pm_ops,
 				 rzg2l_gpt_pm_runtime_suspend,
 				 rzg2l_gpt_pm_runtime_resume, NULL);
 
+u32 rzg2l_gpt_poeg_disable_req_irq_status(void *dev, u8 grp)
+{
+	u8 bitpos = grp * RZG2L_MAX_HW_CHANNELS;
+	struct rzg2l_gpt_chip *rzg2l_gpt;
+	unsigned int i;
+	u32 val = 0;
+	u32 offs;
+	u32 reg;
+
+	rzg2l_gpt = dev_get_drvdata(dev);
+	for (i = 0; i < RZG2L_MAX_HW_CHANNELS; i++) {
+		val <<= 3;
+		if (!test_bit(bitpos + i, rzg2l_gpt->poeg_gpt_link))
+			continue;
+
+		offs = RZG2L_GET_CH_OFFS(i);
+		reg = rzg2l_gpt_read(rzg2l_gpt, offs + RZG2L_GTST);
+		val |= FIELD_GET(RZG2L_GTST_POEG_IRQ_MASK, reg);
+	}
+
+	return val;
+}
+EXPORT_SYMBOL_GPL(rzg2l_gpt_poeg_disable_req_irq_status);
+
+int rzg2l_gpt_poeg_disable_req_clr(void *dev, u8 grp)
+{
+	u8 bitpos = grp * RZG2L_MAX_HW_CHANNELS;
+	struct rzg2l_gpt_chip *rzg2l_gpt;
+	unsigned int i;
+	u32 offs;
+	u32 reg;
+
+	rzg2l_gpt = dev_get_drvdata(dev);
+	for (i = 0; i < RZG2L_MAX_HW_CHANNELS; i++) {
+		if (!test_bit(bitpos + i, rzg2l_gpt->poeg_gpt_link))
+			continue;
+
+		offs = RZG2L_GET_CH_OFFS(i);
+		reg = rzg2l_gpt_read(rzg2l_gpt, offs + RZG2L_GTST);
+
+		if (reg & (RZG2L_GTST_OABHF | RZG2L_GTST_OABLF))
+			rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTIOR,
+					 RZG2L_GTIOR_OBE, 0);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(rzg2l_gpt_poeg_disable_req_clr);
+
+int rzg2l_gpt_pin_reenable(void *dev, u8 grp)
+{
+	u8 bitpos = grp * RZG2L_MAX_HW_CHANNELS;
+	struct rzg2l_gpt_chip *rzg2l_gpt;
+	unsigned int i;
+	u32 offs;
+
+	rzg2l_gpt = dev_get_drvdata(dev);
+	for (i = 0; i < RZG2L_MAX_HW_CHANNELS; i++) {
+		if (!test_bit(bitpos + i, rzg2l_gpt->poeg_gpt_link))
+			continue;
+
+		offs = RZG2L_GET_CH_OFFS(i);
+		rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTIOR,
+				 RZG2L_GTIOR_OBE, RZG2L_GTIOR_OBE);
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(rzg2l_gpt_pin_reenable);
+
+static int rzg2l_gpt_poeg_disable_req_endisable(void *dev, u8 grp, int op, bool on)
+{
+	u8 bitpos = grp * RZG2L_MAX_HW_CHANNELS;
+	struct rzg2l_gpt_chip *rzg2l_gpt;
+	unsigned int i;
+	u32 offs;
+
+	rzg2l_gpt = dev_get_drvdata(dev);
+	pm_runtime_get_sync(dev);
+
+	for (i = 0; i < RZG2L_MAX_HW_CHANNELS; i++) {
+		if (!test_bit(bitpos + i, rzg2l_gpt->poeg_gpt_link))
+			continue;
+
+		offs = RZG2L_GET_CH_OFFS(i);
+		if (on)
+			rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTINTAD, op, op);
+		else
+			rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTINTAD, op, 0);
+	}
+
+	pm_runtime_put(dev);
+
+	return 0;
+}
+
+int rzg2l_gpt_poeg_disable_req_both_high(void *dev, u8 grp, bool on)
+{
+	int id = RZG2L_GTINTAD_OUTPUT_DISABLE_SAME_LEVEL_HIGH;
+
+	return rzg2l_gpt_poeg_disable_req_endisable(dev, grp, id, on);
+}
+EXPORT_SYMBOL_GPL(rzg2l_gpt_poeg_disable_req_both_high);
+
 static void rzg2l_gpt_reset_assert_pm_disable(void *data)
 {
 	struct rzg2l_gpt_chip *rzg2l_gpt = data;
diff --git a/include/linux/pwm/rzg2l-gpt.h b/include/linux/pwm/rzg2l-gpt.h
new file mode 100644
index 000000000000..0fc13ab57420
--- /dev/null
+++ b/include/linux/pwm/rzg2l-gpt.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_PWM_RZG2L_GPT_H__
+#define __LINUX_PWM_RZG2L_GPT_H__
+
+#if IS_ENABLED(CONFIG_PWM_RZG2L_GPT)
+u32 rzg2l_gpt_poeg_disable_req_irq_status(void *dev, u8 grp);
+int rzg2l_gpt_poeg_disable_req_clr(void *gpt_device, u8 grp);
+int rzg2l_gpt_pin_reenable(void *gpt_device, u8 grp);
+int rzg2l_gpt_poeg_disable_req_both_high(void *gpt_device, u8 grp, bool on);
+#else
+static inline u32 rzg2l_gpt_poeg_disable_req_irq_status(void *dev, u8 grp)
+{
+	return -ENODEV;
+}
+
+static inline int rzg2l_gpt_poeg_disable_req_clr(void *gpt_device, u8 grp)
+{
+	return -ENODEV;
+}
+
+static inline int rzg2l_gpt_pin_reenable(void *gpt_device, u8 grp)
+{
+	return -ENODEV;
+}
+
+static inline int rzg2l_gpt_poeg_disable_req_both_high(void *gpt_device, u8 grp, bool on)
+{
+	return -ENODEV;
+}
+#endif
+
+#endif /* __LINUX_PWM_RZG2L_GPT_H__ */
-- 
2.25.1

[DO NOT APPLY PATCH v6 08/13] pinctrl: renesas: rzg2l-poeg: Add support for GPT Output-Disable Request

[Biju Das]

This patch supports output-disable requests from GPT.

Added sysfs to enable/disable request from GPT when both outputs
are high.

When both outputs are high, gpt detects the condition and triggers
an interrupt to POEG. POEG handles the interrupt and send notification
to userspace. userspace handles the fault and issue a write call to
cancel the disable output request.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/pinctrl/renesas/poeg/rzg2l-poeg.c | 206 +++++++++++++++++++++-
 include/linux/pinctrl/pinctrl-rzg2l.h     |   9 +
 2 files changed, 212 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/renesas/poeg/rzg2l-poeg.c b/drivers/pinctrl/renesas/poeg/rzg2l-poeg.c
index 30f4352e257d..b6f01065c058 100644
--- a/drivers/pinctrl/renesas/poeg/rzg2l-poeg.c
+++ b/drivers/pinctrl/renesas/poeg/rzg2l-poeg.c
@@ -4,27 +4,45 @@
  *
  * Copyright (C) 2022 Renesas Electronics Corporation
  */
+#include <linux/cdev.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/kfifo.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/pinctrl/pinctrl-rzg2l.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/pwm/rzg2l-gpt.h>
+#include <linux/poll.h>
 #include <linux/reset.h>
+#include <linux/wait.h>
 
+#define POEGG_IOCE	BIT(5)
+#define POEGG_PIDE	BIT(4)
 #define POEGG_SSF	BIT(3)
+#define POEGG_IOCF	BIT(1)
+#define POEGG_PIDF	BIT(0)
 
 #define RZG2L_POEG_MAX_INDEX		3
 
 #define RZG2L_GPT_MAX_HW_CHANNELS	8
 #define RZG2L_GPT_INVALID_CHANNEL	0xff
 
+static struct class *poeg_class;
+static dev_t g_poeg_dev;
+static int minor_n;
+
 struct rzg2l_poeg_chip {
 	struct device *gpt_dev;
 	struct reset_control *rstc;
 	void __iomem *mmio;
 	u8 index;
+	DECLARE_BITMAP(gpt_irq, 3);
+	struct cdev poeg_cdev;
+	wait_queue_head_t events_wait;
+	DECLARE_KFIFO_PTR(events, struct poeg_event);
 	u8 gpt_channels[RZG2L_GPT_MAX_HW_CHANNELS];
 };
 
@@ -65,6 +83,20 @@ static int rzg2l_poeg_output_disable_user(struct rzg2l_poeg_chip *chip,
 	return 0;
 }
 
+static int rzg2l_poeg_output_disable_both_high(struct rzg2l_poeg_chip *chip,
+					       bool enable)
+{
+	if (enable)
+		set_bit(RZG2L_GPT_OABHF, chip->gpt_irq);
+	else
+		clear_bit(RZG2L_GPT_OABHF, chip->gpt_irq);
+
+	rzg2l_gpt_poeg_disable_req_both_high(chip->gpt_dev, chip->index,
+					     test_bit(RZG2L_GPT_OABHF, chip->gpt_irq));
+
+	return 0;
+}
+
 static int rzg2l_poeg_cb(void *context, const char *fname, const char *gname,
 			 enum pin_output_disable_conf conf,
 			 unsigned int conf_val)
@@ -88,6 +120,8 @@ static int rzg2l_poeg_cb(void *context, const char *fname, const char *gname,
 		ret = rzg2l_poeg_output_disable_user(context, !!conf_val);
 		break;
 	case PINCTRL_OUTPUT_DISABLE_BY_SOC_ON_PIN_OUTPUT_HIGH:
+		ret = rzg2l_poeg_output_disable_both_high(context, !!conf_val);
+		break;
 	case PINCTRL_OUTPUT_DISABLE_BY_SOC_ON_PIN_OUTPUT_LOW:
 	case PINCTRL_OUTPUT_DISABLE_BY_SOC_ON_DEAD_TIME_ERROR:
 	default:
@@ -97,6 +131,111 @@ static int rzg2l_poeg_cb(void *context, const char *fname, const char *gname,
 	return ret;
 }
 
+static irqreturn_t rzg2l_poeg_irq(int irq, void *ptr)
+{
+	struct rzg2l_poeg_chip *chip = ptr;
+	struct poeg_event ev;
+	u32 val;
+
+	val = rzg2l_gpt_poeg_disable_req_irq_status(chip->gpt_dev, chip->index);
+	ev.channel = chip->index;
+	ev.gpt_disable_irq_status = val;
+	kfifo_in(&chip->events, &ev, 1);
+	wake_up_poll(&chip->events_wait, EPOLLIN);
+
+	val = rzg2l_poeg_read(chip);
+	if (val & POEGG_IOCF)
+		val &= ~POEGG_IOCF;
+
+	if (val & POEGG_PIDF)
+		val &= ~POEGG_PIDF;
+
+	rzg2l_poeg_write(chip, val);
+	rzg2l_gpt_poeg_disable_req_clr(chip->gpt_dev, chip->index);
+
+	return IRQ_HANDLED;
+}
+
+static __poll_t rzg2l_poeg_chrdev_poll(struct file *filp,
+				       struct poll_table_struct *pollt)
+{
+	struct rzg2l_poeg_chip *const chip = filp->private_data;
+	__poll_t events = 0;
+
+	poll_wait(filp, &chip->events_wait, pollt);
+	if (!kfifo_is_empty(&chip->events))
+		events = EPOLLIN | EPOLLRDNORM;
+
+	return events;
+}
+
+static ssize_t rzg2l_poeg_chrdev_read(struct file *filp, char __user *buf,
+				      size_t len, loff_t *f_ps)
+{
+	struct rzg2l_poeg_chip *const chip = filp->private_data;
+	unsigned int copied;
+	int err;
+
+	if (len < sizeof(struct poeg_event))
+		return -EINVAL;
+
+	do {
+		if (kfifo_is_empty(&chip->events)) {
+			if (filp->f_flags & O_NONBLOCK)
+				return -EAGAIN;
+
+			err = wait_event_interruptible(chip->events_wait,
+						       !kfifo_is_empty(&chip->events));
+			if (err < 0)
+				return err;
+		}
+
+		err = kfifo_to_user(&chip->events, buf, len, &copied);
+		if (err < 0)
+			return err;
+	} while (!copied);
+
+	return copied;
+}
+
+static ssize_t rzg2l_poeg_chrdev_write(struct file *filp,
+				       const char __user *buf,
+				       size_t len, loff_t *f_ps)
+{
+	struct rzg2l_poeg_chip *const chip = filp->private_data;
+
+	rzg2l_gpt_pin_reenable(chip->gpt_dev, chip->index);
+
+	return len;
+}
+
+static int rzg2l_poeg_chrdev_open(struct inode *inode, struct file *filp)
+{
+	struct rzg2l_poeg_chip *const chip = container_of(inode->i_cdev,
+							  typeof(*chip),
+							  poeg_cdev);
+
+	filp->private_data = chip;
+
+	return nonseekable_open(inode, filp);
+}
+
+static int rzg2l_poeg_chrdev_release(struct inode *inode, struct file *filp)
+{
+	filp->private_data = NULL;
+
+	return 0;
+}
+
+static const struct file_operations poeg_fops = {
+	.owner = THIS_MODULE,
+	.read = rzg2l_poeg_chrdev_read,
+	.write = rzg2l_poeg_chrdev_write,
+	.poll = rzg2l_poeg_chrdev_poll,
+	.open = rzg2l_poeg_chrdev_open,
+	.release = rzg2l_poeg_chrdev_release,
+};
+
 static bool rzg2l_poeg_get_linked_gpt_channels(struct platform_device *pdev,
 					       struct rzg2l_poeg_chip *chip,
 					       struct device_node *gpt_np,
@@ -168,6 +307,7 @@ static int rzg2l_poeg_probe(struct platform_device *pdev)
 	struct device_node *np;
 	u32 val;
 	int ret;
+	int irq;
 
 	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
@@ -199,10 +339,20 @@ static int rzg2l_poeg_probe(struct platform_device *pdev)
 	if (IS_ERR(chip->mmio))
 		return PTR_ERR(chip->mmio);
 
-	if (gpt_linked)
+	if (gpt_linked) {
 		rzg2l_output_disable_cb_register(chip->gpt_dev,
 						 rzg2l_poeg_cb, chip);
 
+		irq = platform_get_irq(pdev, 0);
+		if (irq < 0)
+			return irq;
+
+		ret = devm_request_irq(&pdev->dev, irq, rzg2l_poeg_irq, 0,
+				       dev_name(&pdev->dev), chip);
+		if (ret < 0)
+			return dev_err_probe(&pdev->dev, ret, "cannot get irq\n");
+	}
+
 	chip->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
 	if (IS_ERR(chip->rstc))
 		return dev_err_probe(&pdev->dev, PTR_ERR(chip->rstc),
@@ -220,8 +370,24 @@ static int rzg2l_poeg_probe(struct platform_device *pdev)
 		goto err_pm_disable;
 	}
 
-	return 0;
+	if (gpt_linked)
+		rzg2l_poeg_write(chip, POEGG_IOCE | POEGG_PIDE);
+
+	init_waitqueue_head(&chip->events_wait);
+	cdev_init(&chip->poeg_cdev, &poeg_fops);
+	chip->poeg_cdev.owner = THIS_MODULE;
+	ret = cdev_add(&chip->poeg_cdev,  MKDEV(MAJOR(g_poeg_dev), minor_n), 1);
+	if (ret)
+		goto err_pm;
+
+	device_create(poeg_class, NULL, MKDEV(MAJOR(g_poeg_dev), minor_n),
+		      NULL, "poeg%d", minor_n);
+	minor_n++;
+
+	return kfifo_alloc(&chip->events, 64, GFP_KERNEL);
 
+err_pm:
+	pm_runtime_put(&pdev->dev);
 err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
 	reset_control_assert(chip->rstc);
@@ -232,6 +398,10 @@ static int rzg2l_poeg_remove(struct platform_device *pdev)
 {
 	struct rzg2l_poeg_chip *chip = platform_get_drvdata(pdev);
 
+	kfifo_free(&chip->events);
+	device_destroy(poeg_class,
+		       MKDEV(MAJOR(g_poeg_dev), MINOR(chip->poeg_cdev.dev)));
+	cdev_del(&chip->poeg_cdev);
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	reset_control_assert(chip->rstc);
@@ -247,7 +417,37 @@ static struct platform_driver rzg2l_poeg_driver = {
 	.probe = rzg2l_poeg_probe,
 	.remove = rzg2l_poeg_remove,
 };
-module_platform_driver(rzg2l_poeg_driver);
+
+static int rzg2l_poeg_device_init(void)
+{
+	int err;
+
+	err = alloc_chrdev_region(&g_poeg_dev, 0, 1, "poeg");
+	if (err)
+		goto out;
+
+	poeg_class = class_create(THIS_MODULE, "poeg");
+	if (IS_ERR(poeg_class)) {
+		err = PTR_ERR(poeg_class);
+		goto err_free_chrdev;
+	}
+
+	return platform_driver_register(&rzg2l_poeg_driver);
+
+err_free_chrdev:
+	unregister_chrdev_region(g_poeg_dev, 1);
+out:
+	return err;
+}
+
+static void rzg2l_poeg_device_exit(void)
+{
+	class_destroy(poeg_class);
+	unregister_chrdev_region(g_poeg_dev, 1);
+}
+
+module_init(rzg2l_poeg_device_init);
+module_exit(rzg2l_poeg_device_exit);
 
 MODULE_AUTHOR("Biju Das <biju.das.jz@bp.renesas.com>");
 MODULE_DESCRIPTION("Renesas RZ/G2L POEG Driver");
diff --git a/include/linux/pinctrl/pinctrl-rzg2l.h b/include/linux/pinctrl/pinctrl-rzg2l.h
index a49b4c5f8908..94d1b12d84c8 100644
--- a/include/linux/pinctrl/pinctrl-rzg2l.h
+++ b/include/linux/pinctrl/pinctrl-rzg2l.h
@@ -4,6 +4,15 @@
 
 #include <linux/pinctrl/output-disable.h>
 
+#define RZG2L_GPT_DTEF	0
+#define RZG2L_GPT_OABHF	1
+#define RZG2L_GPT_OABLF	2
+
+struct poeg_event {
+	__u32 gpt_disable_irq_status;
+	__u8 channel;
+};
+
 typedef int (*output_disable_cb) (void *context,
 				  const char *fname,
 				  const char *gname,
-- 
2.25.1

[DO NOT APPLY PATCH v6 09/13] pwm: rzg2l-gpt: Add support for output disable when both output low

[Biju Das]

This patch adds support for output disable request from gpt,
when same time output level is low.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/pwm/pwm-rzg2l-gpt.c   | 9 +++++++++
 include/linux/pwm/rzg2l-gpt.h | 6 ++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/pwm/pwm-rzg2l-gpt.c b/drivers/pwm/pwm-rzg2l-gpt.c
index 2f138e95f752..2291cc3cff39 100644
--- a/drivers/pwm/pwm-rzg2l-gpt.c
+++ b/drivers/pwm/pwm-rzg2l-gpt.c
@@ -75,6 +75,7 @@
 
 #define RZG2L_GTINTAD_GRP_MASK			GENMASK(25, 24)
 #define RZG2L_GTINTAD_OUTPUT_DISABLE_SAME_LEVEL_HIGH	BIT(29)
+#define RZG2L_GTINTAD_OUTPUT_DISABLE_SAME_LEVEL_LOW	BIT(30)
 
 #define RZG2L_GTST_OABHF			BIT(29)
 #define RZG2L_GTST_OABLF			BIT(30)
@@ -542,6 +543,14 @@ int rzg2l_gpt_poeg_disable_req_both_high(void *dev, u8 grp, bool on)
 }
 EXPORT_SYMBOL_GPL(rzg2l_gpt_poeg_disable_req_both_high);
 
+int rzg2l_gpt_poeg_disable_req_both_low(void *dev, u8 grp, bool on)
+{
+	int id = RZG2L_GTINTAD_OUTPUT_DISABLE_SAME_LEVEL_LOW;
+
+	return rzg2l_gpt_poeg_disable_req_endisable(dev, grp, id, on);
+}
+EXPORT_SYMBOL_GPL(rzg2l_gpt_poeg_disable_req_both_low);
+
 static void rzg2l_gpt_reset_assert_pm_disable(void *data)
 {
 	struct rzg2l_gpt_chip *rzg2l_gpt = data;
diff --git a/include/linux/pwm/rzg2l-gpt.h b/include/linux/pwm/rzg2l-gpt.h
index 0fc13ab57420..592bc2900c9e 100644
--- a/include/linux/pwm/rzg2l-gpt.h
+++ b/include/linux/pwm/rzg2l-gpt.h
@@ -7,6 +7,7 @@ u32 rzg2l_gpt_poeg_disable_req_irq_status(void *dev, u8 grp);
 int rzg2l_gpt_poeg_disable_req_clr(void *gpt_device, u8 grp);
 int rzg2l_gpt_pin_reenable(void *gpt_device, u8 grp);
 int rzg2l_gpt_poeg_disable_req_both_high(void *gpt_device, u8 grp, bool on);
+int rzg2l_gpt_poeg_disable_req_both_low(void *gpt_device, u8 grp, bool on);
 #else
 static inline u32 rzg2l_gpt_poeg_disable_req_irq_status(void *dev, u8 grp)
 {
@@ -27,6 +28,11 @@ static inline int rzg2l_gpt_poeg_disable_req_both_high(void *gpt_device, u8 grp,
 {
 	return -ENODEV;
 }
+
+static inline int rzg2l_gpt_poeg_disable_req_both_low(void *gpt_device, u8 grp, bool on)
+{
+	return -ENODEV;
+}
 #endif
 
 #endif /* __LINUX_PWM_RZG2L_GPT_H__ */
-- 
2.25.1

[DO NOT APPLY PATCH v6 10/13] pinctrl: renesas: rzg2l-poeg: output-disable request from GPT when both outputs are low.

[Biju Das]

This patch adds support fpr output-disable requests from GPT, when both
outputs are low.

Added sysfs to enable/disable for configuring GPT output disable request
when both outputs are low.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/pinctrl/renesas/poeg/rzg2l-poeg.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/pinctrl/renesas/poeg/rzg2l-poeg.c b/drivers/pinctrl/renesas/poeg/rzg2l-poeg.c
index b6f01065c058..bbca21557a70 100644
--- a/drivers/pinctrl/renesas/poeg/rzg2l-poeg.c
+++ b/drivers/pinctrl/renesas/poeg/rzg2l-poeg.c
@@ -97,6 +97,20 @@ static int rzg2l_poeg_output_disable_both_high(struct rzg2l_poeg_chip *chip,
 	return 0;
 }
 
+static int rzg2l_poeg_output_disable_both_low(struct rzg2l_poeg_chip *chip,
+					      bool enable)
+{
+	if (enable)
+		set_bit(RZG2L_GPT_OABLF, chip->gpt_irq);
+	else
+		clear_bit(RZG2L_GPT_OABLF, chip->gpt_irq);
+
+	rzg2l_gpt_poeg_disable_req_both_low(chip->gpt_dev, chip->index,
+					    test_bit(RZG2L_GPT_OABLF, chip->gpt_irq));
+
+	return 0;
+}
+
 static int rzg2l_poeg_cb(void *context, const char *fname, const char *gname,
 			 enum pin_output_disable_conf conf,
 			 unsigned int conf_val)
@@ -123,6 +137,8 @@ static int rzg2l_poeg_cb(void *context, const char *fname, const char *gname,
 		ret = rzg2l_poeg_output_disable_both_high(context, !!conf_val);
 		break;
 	case PINCTRL_OUTPUT_DISABLE_BY_SOC_ON_PIN_OUTPUT_LOW:
+		ret = rzg2l_poeg_output_disable_both_low(context, !!conf_val);
+		break;
 	case PINCTRL_OUTPUT_DISABLE_BY_SOC_ON_DEAD_TIME_ERROR:
 	default:
 		return -EINVAL;
-- 
2.25.1

[DO NOT APPLY PATCH v6 11/13] pwm: rzg2l-gpt: Add support for output disable on dead time error

[Biju Das]

This patch adds support for output disable request from gpt,
when dead time error occurred.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/pwm/pwm-rzg2l-gpt.c   | 9 +++++++++
 include/linux/pwm/rzg2l-gpt.h | 6 ++++++
 2 files changed, 15 insertions(+)

diff --git a/drivers/pwm/pwm-rzg2l-gpt.c b/drivers/pwm/pwm-rzg2l-gpt.c
index 2291cc3cff39..c88a5bf9e31d 100644
--- a/drivers/pwm/pwm-rzg2l-gpt.c
+++ b/drivers/pwm/pwm-rzg2l-gpt.c
@@ -74,6 +74,7 @@
 	(FIELD_PREP(RZG2L_GTIOR_GTIOB, RZG2L_INIT_OUT_LO_OUT_LO_END_TOGGLE) | RZG2L_GTIOR_OBE)
 
 #define RZG2L_GTINTAD_GRP_MASK			GENMASK(25, 24)
+#define RZG2L_GTINTAD_OUTPUT_DISABLE_DEADTIME_ERROR	BIT(28)
 #define RZG2L_GTINTAD_OUTPUT_DISABLE_SAME_LEVEL_HIGH	BIT(29)
 #define RZG2L_GTINTAD_OUTPUT_DISABLE_SAME_LEVEL_LOW	BIT(30)
 
@@ -551,6 +552,14 @@ int rzg2l_gpt_poeg_disable_req_both_low(void *dev, u8 grp, bool on)
 }
 EXPORT_SYMBOL_GPL(rzg2l_gpt_poeg_disable_req_both_low);
 
+int rzg2l_gpt_poeg_disable_req_deadtime_error(void *dev, u8 grp, bool on)
+{
+	int id = RZG2L_GTINTAD_OUTPUT_DISABLE_DEADTIME_ERROR;
+
+	return rzg2l_gpt_poeg_disable_req_endisable(dev, grp, id, on);
+}
+EXPORT_SYMBOL_GPL(rzg2l_gpt_poeg_disable_req_deadtime_error);
+
 static void rzg2l_gpt_reset_assert_pm_disable(void *data)
 {
 	struct rzg2l_gpt_chip *rzg2l_gpt = data;
diff --git a/include/linux/pwm/rzg2l-gpt.h b/include/linux/pwm/rzg2l-gpt.h
index 592bc2900c9e..8a004c690c2a 100644
--- a/include/linux/pwm/rzg2l-gpt.h
+++ b/include/linux/pwm/rzg2l-gpt.h
@@ -8,6 +8,7 @@ int rzg2l_gpt_poeg_disable_req_clr(void *gpt_device, u8 grp);
 int rzg2l_gpt_pin_reenable(void *gpt_device, u8 grp);
 int rzg2l_gpt_poeg_disable_req_both_high(void *gpt_device, u8 grp, bool on);
 int rzg2l_gpt_poeg_disable_req_both_low(void *gpt_device, u8 grp, bool on);
+int rzg2l_gpt_poeg_disable_req_deadtime_error(void *gpt_device, u8 grp, bool on);
 #else
 static inline u32 rzg2l_gpt_poeg_disable_req_irq_status(void *dev, u8 grp)
 {
@@ -33,6 +34,11 @@ static inline int rzg2l_gpt_poeg_disable_req_both_low(void *gpt_device, u8 grp,
 {
 	return -ENODEV;
 }
+
+static inline int rzg2l_gpt_poeg_disable_req_deadtime_err(void *gpt_device, u8 grp, bool on)
+{
+	return -ENODEV;
+}
 #endif
 
 #endif /* __LINUX_PWM_RZG2L_GPT_H__ */
-- 
2.25.1

[DO NOT APPLY PATCH v6 12/13] pinctrl: renesas: rzg2l-poeg: output-disable request from GPT on dead time error

[Biju Das]

This patch adds support for output-disable requests from GPT,
when dead time error occurs.

Added sysfs to enable/disable for configuring GPT output disable request
for dead time error.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/pinctrl/renesas/poeg/rzg2l-poeg.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/pinctrl/renesas/poeg/rzg2l-poeg.c b/drivers/pinctrl/renesas/poeg/rzg2l-poeg.c
index bbca21557a70..3d0801d8937a 100644
--- a/drivers/pinctrl/renesas/poeg/rzg2l-poeg.c
+++ b/drivers/pinctrl/renesas/poeg/rzg2l-poeg.c
@@ -111,6 +111,20 @@ static int rzg2l_poeg_output_disable_both_low(struct rzg2l_poeg_chip *chip,
 	return 0;
 }
 
+static int rzg2l_poeg_output_disable_deadtime_err(struct rzg2l_poeg_chip *chip,
+						  bool enable)
+{
+	if (enable)
+		set_bit(RZG2L_GPT_DTEF, chip->gpt_irq);
+	else
+		clear_bit(RZG2L_GPT_DTEF, chip->gpt_irq);
+
+	rzg2l_gpt_poeg_disable_req_deadtime_error(chip->gpt_dev, chip->index,
+						  test_bit(RZG2L_GPT_DTEF, chip->gpt_irq));
+
+	return 0;
+}
+
 static int rzg2l_poeg_cb(void *context, const char *fname, const char *gname,
 			 enum pin_output_disable_conf conf,
 			 unsigned int conf_val)
@@ -140,6 +154,8 @@ static int rzg2l_poeg_cb(void *context, const char *fname, const char *gname,
 		ret = rzg2l_poeg_output_disable_both_low(context, !!conf_val);
 		break;
 	case PINCTRL_OUTPUT_DISABLE_BY_SOC_ON_DEAD_TIME_ERROR:
+		ret = rzg2l_poeg_output_disable_deadtime_err(context, !!conf_val);
+		break;
 	default:
 		return -EINVAL;
 	}
-- 
2.25.1

[DO NOT APPLY PATCH v6 13/13] tools/poeg: Add test app for poeg

[Biju Das]

Add test app for poeg

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 tools/poeg/Build      |  1 +
 tools/poeg/Makefile   | 53 ++++++++++++++++++++++++++++++++++++++
 tools/poeg/poeg_app.c | 60 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 114 insertions(+)
 create mode 100644 tools/poeg/Build
 create mode 100644 tools/poeg/Makefile
 create mode 100644 tools/poeg/poeg_app.c

diff --git a/tools/poeg/Build b/tools/poeg/Build
new file mode 100644
index 000000000000..f960920a4afb
--- /dev/null
+++ b/tools/poeg/Build
@@ -0,0 +1 @@
+poeg_app-y += poeg_app.o
diff --git a/tools/poeg/Makefile b/tools/poeg/Makefile
new file mode 100644
index 000000000000..6946e6956215
--- /dev/null
+++ b/tools/poeg/Makefile
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: GPL-2.0
+include ../scripts/Makefile.include
+
+bindir ?= /usr/bin
+
+ifeq ($(srctree),)
+srctree := $(patsubst %/,%,$(dir $(CURDIR)))
+srctree := $(patsubst %/,%,$(dir $(srctree)))
+endif
+
+# Do not use make's built-in rules
+# (this improves performance and avoids hard-to-debug behaviour);
+MAKEFLAGS += -r
+
+override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
+
+ALL_TARGETS := poeg_app
+ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
+
+all: $(ALL_PROGRAMS)
+
+export srctree OUTPUT CC LD CFLAGS
+include $(srctree)/tools/build/Makefile.include
+
+#
+# We need the following to be outside of kernel tree
+#
+$(OUTPUT)include/linux/poeg.h: ../../include/linux/soc/renesas/rzg2l-poeg.h
+	mkdir -p $(OUTPUT)include/linux 2>&1 || true
+	ln -sf $(CURDIR)/../../include/linux/soc/renesas/rzg2l-poeg.h $@
+
+prepare: $(OUTPUT)include/linux/poeg.h
+
+COUNTER_EXAMPLE := $(OUTPUT)poeg_app.o
+$(COUNTER_EXAMPLE): prepare FORCE
+	$(Q)$(MAKE) $(build)=poeg_app
+$(OUTPUT)poeg_app: $(COUNTER_EXAMPLE)
+	$(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
+
+clean:
+	rm -f $(ALL_PROGRAMS)
+	rm -rf $(OUTPUT)include/linux/counter.h
+	find $(or $(OUTPUT),.) -name '*.o' -delete -o -name '\.*.d' -delete
+
+install: $(ALL_PROGRAMS)
+	install -d -m 755 $(DESTDIR)$(bindir);		\
+	for program in $(ALL_PROGRAMS); do		\
+		install $$program $(DESTDIR)$(bindir);	\
+	done
+
+FORCE:
+
+.PHONY: all install clean FORCE prepare
diff --git a/tools/poeg/poeg_app.c b/tools/poeg/poeg_app.c
new file mode 100644
index 000000000000..79cacb8c60c5
--- /dev/null
+++ b/tools/poeg/poeg_app.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * POEG - example userspace application
+ * Copyright (C) 2022 Biju Das
+ */
+#include <stdio.h>
+#include <stdlib.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <errno.h>
+#include <sys/ioctl.h>
+#include <linux/ioctl.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/time.h>
+#include <sys/types.h>
+#include <poll.h>
+
+#include <linux/poeg.h>
+
+int main(int argc, char *arg[])
+{
+	struct poeg_event event_data;
+	unsigned int val;
+	int ret, fd, i;
+
+	fd = open("/dev/poeg3", O_RDWR);
+	if (fd < 0)
+		perror("open");
+	else
+		printf("[POEG]open\n");
+
+	for (;;) {
+		ret = read(fd, &event_data, sizeof(event_data));
+		if (ret == -1) {
+			perror("Failed to read event data");
+			return 1;
+		}
+
+		val = event_data.gpt_disable_irq_status;
+		if (val) {
+			/* emulate fault clearing condition by adding delay */
+			sleep(2);
+			for (i = 0; i < 8; i++) {
+				if (val & 7) {
+					printf("gpt ch:%u, irq=%x\n", i, val & 7);
+					ret = write(fd, &event_data, sizeof(event_data));
+				}
+				val >>= 3;
+			}
+		}
+	}
+
+	if (close(fd) != 0)
+		perror("close");
+	else
+		printf("[POEG]close\n");
+
+	return 0;
+}
-- 
2.25.1

Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()

[andy.shevchenko]

Mon, Mar 06, 2023 at 09:00:02AM +0000, Biju Das kirjoitti:
> Add pinctrl_get_device() to find a device handle associated with
> a pincontrol group(i.e. by matching function name and group name for a
> device). This device handle can then be used for finding match for the
> pin output disable device that protects device against short circuits
> on the pins.

Not sure I understand the use case. Please, create a better commit message.

Also it is missing the explanation why there will be no collisions when looking
by the same pair of function and group name from different device.

(Always imagine that you have 2+ same IP blocks on the platform before doing
 any pin control core work. This will help you to design it properly. )

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 06/13] drivers: pinctrl: renesas: Add RZ/G2L POEG driver support

[andy.shevchenko]

Mon, Mar 06, 2023 at 09:00:07AM +0000, Biju Das kirjoitti:
> The output pins of the RZ/G2L general PWM timer (GPT) can be disabled
> by using the port output enabling function for the GPT (POEG).
> 
> Add basic support using s/w control through generic pincontrol sysfs to
> enable/disable output from GPT by registering with RZ/G2L pincontrol
> driver.

You have wrong Subject prefix.

...

> +static void rzg2l_poeg_write(struct rzg2l_poeg_chip *chip, u32 data)
> +{
> +	iowrite32(data, chip->mmio);
> +}
> +
> +static u32 rzg2l_poeg_read(struct rzg2l_poeg_chip *chip)
> +{
> +	return ioread32(chip->mmio);
> +}

Why not regmap MMIO?

...

> +static struct platform_driver rzg2l_poeg_driver = {
> +	.driver = {
> +		.name = "rzg2l-poeg",
> +		.of_match_table = of_match_ptr(rzg2l_poeg_of_table),

Why do you need of_match_ptr()?

> +	},
> +	.probe = rzg2l_poeg_probe,
> +	.remove = rzg2l_poeg_remove,
> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 03/13] pinctrl: Add sysfs support

[andy.shevchenko]

Mon, Mar 06, 2023 at 09:00:04AM +0000, Biju Das kirjoitti:
> Add a simple sysfs interface to the generic pinctrl framework
> for configuring pins for output disable operation.
> 
> /sys/class/pinctrl/
>   `-- output-disable/
>       |-- configure    (w/o) ask the kernel to configure a pin group
>                              for output disable operation.
> 
>   echo "<group-name function-name 0 1>" > configure
> 
> The existing "pinmux-functions" debugfs file lists the pin functions
> registered for the pin controller. For example:
> 
>   function 0: usb0, groups = [ usb0 ]
>   function 1: usb1, groups = [ usb1 ]
>   function 2: gpt4-pins, groups = [ gpt4-pins ]
>   function 3: scif0, groups = [ scif0 ]
>   function 4: scif2, groups = [ scif2 ]
>   function 5: spi1, groups = [ spi1 ]
> 
> To configure gpt4-pins for output disable activation by user:
> 
>   echo "gpt4-pins gpt4-pins 0 1" > configure

...

> +static struct attribute *pinctrl_attrs[] = {
> +	&dev_attr_configure.attr,
> +	NULL,

No comma for a terminator entry.

> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [DO NOT APPLY PATCH v6 10/13] pinctrl: renesas: rzg2l-poeg: output-disable request from GPT when both outputs are low.

[andy.shevchenko]

Mon, Mar 06, 2023 at 09:00:11AM +0000, Biju Das kirjoitti:
> This patch adds support fpr output-disable requests from GPT, when both
> outputs are low.
> 
> Added sysfs to enable/disable for configuring GPT output disable request
> when both outputs are low.

...

> +static int rzg2l_poeg_output_disable_both_low(struct rzg2l_poeg_chip *chip,
> +					      bool enable)
> +{
> +	if (enable)
> +		set_bit(RZG2L_GPT_OABLF, chip->gpt_irq);
> +	else
> +		clear_bit(RZG2L_GPT_OABLF, chip->gpt_irq);

JFYI: assign_bit()


> +	rzg2l_gpt_poeg_disable_req_both_low(chip->gpt_dev, chip->index,
> +					    test_bit(RZG2L_GPT_OABLF, chip->gpt_irq));
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()
> 
> Mon, Mar 06, 2023 at 09:00:02AM +0000, Biju Das kirjoitti:
> > Add pinctrl_get_device() to find a device handle associated with a
> > pincontrol group(i.e. by matching function name and group name for a
> > device). This device handle can then be used for finding match for the
> > pin output disable device that protects device against short circuits
> > on the pins.
> 
> Not sure I understand the use case. Please, create a better commit message.

OK, Basically pinmux_enable_setting allows exclusive access of pin to a device.
It won't allow multiple devices to claim a pin.

Maybe instead of find device handle, it should re worded as to find the
current owner of the pins associated with a given function and group name??

Or please let me know have better suggestion.

Then use this info for matching the device used for pin output
disable.

> 
> Also it is missing the explanation why there will be no collisions when
> looking by the same pair of function and group name from different device.

setting->data.mux will be unique for a pin. So there won't be a collision when
looking by the same pair of function and group name from different device.

> 
> (Always imagine that you have 2+ same IP blocks on the platform before doing
> any pin control core work. This will help you to design it properly. )

OK.

Cheers,
Biju

RE: [PATCH v6 06/13] drivers: pinctrl: renesas: Add RZ/G2L POEG driver support

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.

> -----Original Message-----
> From: andy.shevchenko@gmail.com <andy.shevchenko@gmail.com>
> Sent: Monday, March 6, 2023 11:36 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>; Philipp Zabel
> <p.zabel@pengutronix.de>; Geert Uytterhoeven <geert+renesas@glider.be>;
> Thierry Reding <thierry.reding@gmail.com>; Uwe Kleine-König <u.kleine-
> koenig@pengutronix.de>; linux-pwm@vger.kernel.org; linux-renesas-
> soc@vger.kernel.org; linux-gpio@vger.kernel.org; Chris Paterson
> <Chris.Paterson2@renesas.com>; Prabhakar Mahadev Lad <prabhakar.mahadev-
> lad.rj@bp.renesas.com>
> Subject: Re: [PATCH v6 06/13] drivers: pinctrl: renesas: Add RZ/G2L POEG
> driver support
> 
> Mon, Mar 06, 2023 at 09:00:07AM +0000, Biju Das kirjoitti:
> > The output pins of the RZ/G2L general PWM timer (GPT) can be disabled
> > by using the port output enabling function for the GPT (POEG).
> >
> > Add basic support using s/w control through generic pincontrol sysfs
> > to enable/disable output from GPT by registering with RZ/G2L
> > pincontrol driver.
> 
> You have wrong Subject prefix.

Oops. Will fix.

> 
> ...
> 
> > +static void rzg2l_poeg_write(struct rzg2l_poeg_chip *chip, u32 data)
> > +{
> > +	iowrite32(data, chip->mmio);
> > +}
> > +
> > +static u32 rzg2l_poeg_read(struct rzg2l_poeg_chip *chip) {
> > +	return ioread32(chip->mmio);
> > +}
> 
> Why not regmap MMIO?

Some drivers used iowrite32, some uses writel, some uses regmap. 

will use regmap for read/write,If the preference is regmap MMIO
as it comes with spinlock for MMIO access.

> 
> ...
> 
> > +static struct platform_driver rzg2l_poeg_driver = {
> > +	.driver = {
> > +		.name = "rzg2l-poeg",
> > +		.of_match_table = of_match_ptr(rzg2l_poeg_of_table),
> 
> Why do you need of_match_ptr()?


Not needed. Will remove it.

Cheers,
Biju

> 
> > +	},
> > +	.probe = rzg2l_poeg_probe,
> > +	.remove = rzg2l_poeg_remove,
> > +};
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

RE: [PATCH v6 03/13] pinctrl: Add sysfs support

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [PATCH v6 03/13] pinctrl: Add sysfs support
> 
> Mon, Mar 06, 2023 at 09:00:04AM +0000, Biju Das kirjoitti:
> > Add a simple sysfs interface to the generic pinctrl framework for
> > configuring pins for output disable operation.
> >
> > /sys/class/pinctrl/
> >   `-- output-disable/
> >       |-- configure    (w/o) ask the kernel to configure a pin group
> >                              for output disable operation.
> >
> >   echo "<group-name function-name 0 1>" > configure
> >
> > The existing "pinmux-functions" debugfs file lists the pin functions
> > registered for the pin controller. For example:
> >
> >   function 0: usb0, groups = [ usb0 ]
> >   function 1: usb1, groups = [ usb1 ]
> >   function 2: gpt4-pins, groups = [ gpt4-pins ]
> >   function 3: scif0, groups = [ scif0 ]
> >   function 4: scif2, groups = [ scif2 ]
> >   function 5: spi1, groups = [ spi1 ]
> >
> > To configure gpt4-pins for output disable activation by user:
> >
> >   echo "gpt4-pins gpt4-pins 0 1" > configure
> 
> ...
> 
> > +static struct attribute *pinctrl_attrs[] = {
> > +	&dev_attr_configure.attr,
> > +	NULL,
> 
> No comma for a terminator entry.

Agreed.

Cheers,
Biju

RE: [DO NOT APPLY PATCH v6 10/13] pinctrl: renesas: rzg2l-poeg: output-disable request from GPT when both outputs are low.

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [DO NOT APPLY PATCH v6 10/13] pinctrl: renesas: rzg2l-poeg:
> output-disable request from GPT when both outputs are low.
> 
> Mon, Mar 06, 2023 at 09:00:11AM +0000, Biju Das kirjoitti:
> > This patch adds support fpr output-disable requests from GPT, when
> > both outputs are low.
> >
> > Added sysfs to enable/disable for configuring GPT output disable
> > request when both outputs are low.
> 
> ...
> 
> > +static int rzg2l_poeg_output_disable_both_low(struct rzg2l_poeg_chip
> *chip,
> > +					      bool enable)
> > +{
> > +	if (enable)
> > +		set_bit(RZG2L_GPT_OABLF, chip->gpt_irq);
> > +	else
> > +		clear_bit(RZG2L_GPT_OABLF, chip->gpt_irq);
> 
> JFYI: assign_bit()

OK, will use assign_bit() and remove the above code.

Cheers,
Biju
> 
> 
> > +	rzg2l_gpt_poeg_disable_req_both_low(chip->gpt_dev, chip->index,
> > +					    test_bit(RZG2L_GPT_OABLF, chip-
> >gpt_irq));
> > +
> > +	return 0;
> > +}
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH v6 06/13] drivers: pinctrl: renesas: Add RZ/G2L POEG driver support

[Andy Shevchenko]

On Tue, Mar 7, 2023 at 10:53 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > From: andy.shevchenko@gmail.com <andy.shevchenko@gmail.com>
> > Sent: Monday, March 6, 2023 11:36 PM
> > Mon, Mar 06, 2023 at 09:00:07AM +0000, Biju Das kirjoitti:

...

> > > +static void rzg2l_poeg_write(struct rzg2l_poeg_chip *chip, u32 data)
> > > +{
> > > +   iowrite32(data, chip->mmio);
> > > +}
> > > +
> > > +static u32 rzg2l_poeg_read(struct rzg2l_poeg_chip *chip) {
> > > +   return ioread32(chip->mmio);
> > > +}
> >
> > Why not regmap MMIO?
>
> Some drivers used iowrite32, some uses writel, some uses regmap.
>
> will use regmap for read/write,If the preference is regmap MMIO
> as it comes with spinlock for MMIO access.

Lock can be disabled. It's up to the user of regmap.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v6 06/13] drivers: pinctrl: renesas: Add RZ/G2L POEG driver support

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [PATCH v6 06/13] drivers: pinctrl: renesas: Add RZ/G2L POEG
> driver support
> 
> On Tue, Mar 7, 2023 at 10:53 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > From: andy.shevchenko@gmail.com <andy.shevchenko@gmail.com>
> > > Sent: Monday, March 6, 2023 11:36 PM Mon, Mar 06, 2023 at 09:00:07AM
> > > +0000, Biju Das kirjoitti:
> 
> ...
> 
> > > > +static void rzg2l_poeg_write(struct rzg2l_poeg_chip *chip, u32
> > > > +data) {
> > > > +   iowrite32(data, chip->mmio);
> > > > +}
> > > > +
> > > > +static u32 rzg2l_poeg_read(struct rzg2l_poeg_chip *chip) {
> > > > +   return ioread32(chip->mmio);
> > > > +}
> > >
> > > Why not regmap MMIO?
> >
> > Some drivers used iowrite32, some uses writel, some uses regmap.
> >
> > will use regmap for read/write,If the preference is regmap MMIO as it
> > comes with spinlock for MMIO access.
> 
> Lock can be disabled. It's up to the user of regmap.

Ok, Just want to double check,
POEG has a single 32 bit register. So it worth to use regmap?
A simple readl/write is sufficient no??

Cheers,
Biju

Re: [PATCH v6 06/13] drivers: pinctrl: renesas: Add RZ/G2L POEG driver support

[Andy Shevchenko]

On Tue, Mar 7, 2023 at 12:10 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > On Tue, Mar 7, 2023 at 10:53 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > From: andy.shevchenko@gmail.com <andy.shevchenko@gmail.com>
> > > > Sent: Monday, March 6, 2023 11:36 PM Mon, Mar 06, 2023 at 09:00:07AM
> > > > +0000, Biju Das kirjoitti:

...

> > > > > +static void rzg2l_poeg_write(struct rzg2l_poeg_chip *chip, u32
> > > > > +data) {
> > > > > +   iowrite32(data, chip->mmio);
> > > > > +}
> > > > > +
> > > > > +static u32 rzg2l_poeg_read(struct rzg2l_poeg_chip *chip) {
> > > > > +   return ioread32(chip->mmio);
> > > > > +}
> > > >
> > > > Why not regmap MMIO?
> > >
> > > Some drivers used iowrite32, some uses writel, some uses regmap.
> > >
> > > will use regmap for read/write,If the preference is regmap MMIO as it
> > > comes with spinlock for MMIO access.
> >
> > Lock can be disabled. It's up to the user of regmap.
>
> Ok, Just want to double check,
> POEG has a single 32 bit register. So it worth to use regmap?
> A simple readl/write is sufficient no??

It can be. But can you explain why you used iowriteXX() / ioreadXX()
instead of writeX()/readX()?

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v6 06/13] drivers: pinctrl: renesas: Add RZ/G2L POEG driver support

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [PATCH v6 06/13] drivers: pinctrl: renesas: Add RZ/G2L POEG
> driver support
> 
> On Tue, Mar 7, 2023 at 12:10 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > On Tue, Mar 7, 2023 at 10:53 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > > > From: andy.shevchenko@gmail.com <andy.shevchenko@gmail.com>
> > > > > Sent: Monday, March 6, 2023 11:36 PM Mon, Mar 06, 2023 at
> > > > > 09:00:07AM
> > > > > +0000, Biju Das kirjoitti:
> 
> ...
> 
> > > > > > +static void rzg2l_poeg_write(struct rzg2l_poeg_chip *chip,
> > > > > > +u32
> > > > > > +data) {
> > > > > > +   iowrite32(data, chip->mmio); }
> > > > > > +
> > > > > > +static u32 rzg2l_poeg_read(struct rzg2l_poeg_chip *chip) {
> > > > > > +   return ioread32(chip->mmio); }
> > > > >
> > > > > Why not regmap MMIO?
> > > >
> > > > Some drivers used iowrite32, some uses writel, some uses regmap.
> > > >
> > > > will use regmap for read/write,If the preference is regmap MMIO as
> > > > it comes with spinlock for MMIO access.
> > >
> > > Lock can be disabled. It's up to the user of regmap.
> >
> > Ok, Just want to double check,
> > POEG has a single 32 bit register. So it worth to use regmap?
> > A simple readl/write is sufficient no??
> 
> It can be. But can you explain why you used iowriteXX() / ioreadXX() instead
> of writeX()/readX()?

It is a mistake from my side. I have referred RZ/G2L BSP poeg driver code[1],
initially and missed to change this.

[1]
https://github.com/renesas-rz/rz_linux-cip/blob/rz-5.10-cip17/drivers/pwm/poeg-rzg2l.c

Cheers,
Biju

Re: [PATCH v6 03/13] pinctrl: Add sysfs support

[Linus Walleij]

On Mon, Mar 6, 2023 at 10:00 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:

> Add a simple sysfs interface to the generic pinctrl framework
> for configuring pins for output disable operation.
>
> /sys/class/pinctrl/
>   `-- output-disable/
>       |-- configure    (w/o) ask the kernel to configure a pin group
>                              for output disable operation.
>
>   echo "<group-name function-name 0 1>" > configure
>
> The existing "pinmux-functions" debugfs file lists the pin functions
> registered for the pin controller. For example:
>
>   function 0: usb0, groups = [ usb0 ]
>   function 1: usb1, groups = [ usb1 ]
>   function 2: gpt4-pins, groups = [ gpt4-pins ]
>   function 3: scif0, groups = [ scif0 ]
>   function 4: scif2, groups = [ scif2 ]
>   function 5: spi1, groups = [ spi1 ]
>
> To configure gpt4-pins for output disable activation by user:
>
>   echo "gpt4-pins gpt4-pins 0 1" > configure
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>


You have to run things like this by Greg K-H (adde on To)

> +static struct class pinctrl_class = {
> +       .name = "pinctrl",
> +       .owner = THIS_MODULE,
> +       .dev_groups = pinctrl_groups,
> +};

Why are you adding a new device class?

IIRC Greg don't like them, we should probably just deal with the
devices directly on the bus where they are, which also implies
some topology search etc which is a feature.

Yours,
Linus Walleij

RE: [PATCH v6 03/13] pinctrl: Add sysfs support

[Biju Das]

Hi Linus Walleij,

Thanks for the feedback.

> Subject: Re: [PATCH v6 03/13] pinctrl: Add sysfs support
> 
> On Mon, Mar 6, 2023 at 10:00 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> 
> > Add a simple sysfs interface to the generic pinctrl framework for
> > configuring pins for output disable operation.
> >
> > /sys/class/pinctrl/
> >   `-- output-disable/
> >       |-- configure    (w/o) ask the kernel to configure a pin group
> >                              for output disable operation.
> >
> >   echo "<group-name function-name 0 1>" > configure
> >
> > The existing "pinmux-functions" debugfs file lists the pin functions
> > registered for the pin controller. For example:
> >
> >   function 0: usb0, groups = [ usb0 ]
> >   function 1: usb1, groups = [ usb1 ]
> >   function 2: gpt4-pins, groups = [ gpt4-pins ]
> >   function 3: scif0, groups = [ scif0 ]
> >   function 4: scif2, groups = [ scif2 ]
> >   function 5: spi1, groups = [ spi1 ]
> >
> > To configure gpt4-pins for output disable activation by user:
> >
> >   echo "gpt4-pins gpt4-pins 0 1" > configure
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> 
> You have to run things like this by Greg K-H (adde on To)
> 
> > +static struct class pinctrl_class = {
> > +       .name = "pinctrl",
> > +       .owner = THIS_MODULE,
> > +       .dev_groups = pinctrl_groups,
> > +};
> 
> Why are you adding a new device class?
> 
> IIRC Greg don't like them, we should probably just deal with the devices
> directly on the bus where they are, which also implies some topology search
> etc which is a feature.

I am ok for both, 

I thought adding new device class will be more generic and 
people can use simple sysfs[1] like pwm for output disable operation
rather than the SoC specific operation[2].

[1]
/sys/class/pinctrl/output-disable/configure

vs

[2]
/sys/devices/platform/soc/11030000.pinctrl/output_disable

Cheers,
Biju

Re: [PATCH v6 03/13] pinctrl: Add sysfs support

[Greg KH]

On Tue, Mar 07, 2023 at 02:41:16PM +0100, Linus Walleij wrote:
> On Mon, Mar 6, 2023 at 10:00 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> 
> > Add a simple sysfs interface to the generic pinctrl framework
> > for configuring pins for output disable operation.
> >
> > /sys/class/pinctrl/
> >   `-- output-disable/
> >       |-- configure    (w/o) ask the kernel to configure a pin group
> >                              for output disable operation.
> >
> >   echo "<group-name function-name 0 1>" > configure
> >
> > The existing "pinmux-functions" debugfs file lists the pin functions
> > registered for the pin controller. For example:
> >
> >   function 0: usb0, groups = [ usb0 ]
> >   function 1: usb1, groups = [ usb1 ]
> >   function 2: gpt4-pins, groups = [ gpt4-pins ]
> >   function 3: scif0, groups = [ scif0 ]
> >   function 4: scif2, groups = [ scif2 ]
> >   function 5: spi1, groups = [ spi1 ]
> >
> > To configure gpt4-pins for output disable activation by user:
> >
> >   echo "gpt4-pins gpt4-pins 0 1" > configure

That is not what sysfs is for (multiple values per file.)  Why not use
configfs for this as that is exactly what this is designed for?

thanks,

greg k-h

Re: [PATCH v6 03/13] pinctrl: Add sysfs support

[Greg KH]

On Tue, Mar 07, 2023 at 02:43:54PM +0000, Biju Das wrote:
> Hi Linus Walleij,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH v6 03/13] pinctrl: Add sysfs support
> > 
> > On Mon, Mar 6, 2023 at 10:00 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > 
> > > Add a simple sysfs interface to the generic pinctrl framework for
> > > configuring pins for output disable operation.
> > >
> > > /sys/class/pinctrl/
> > >   `-- output-disable/
> > >       |-- configure    (w/o) ask the kernel to configure a pin group
> > >                              for output disable operation.
> > >
> > >   echo "<group-name function-name 0 1>" > configure
> > >
> > > The existing "pinmux-functions" debugfs file lists the pin functions
> > > registered for the pin controller. For example:
> > >
> > >   function 0: usb0, groups = [ usb0 ]
> > >   function 1: usb1, groups = [ usb1 ]
> > >   function 2: gpt4-pins, groups = [ gpt4-pins ]
> > >   function 3: scif0, groups = [ scif0 ]
> > >   function 4: scif2, groups = [ scif2 ]
> > >   function 5: spi1, groups = [ spi1 ]
> > >
> > > To configure gpt4-pins for output disable activation by user:
> > >
> > >   echo "gpt4-pins gpt4-pins 0 1" > configure
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > 
> > 
> > You have to run things like this by Greg K-H (adde on To)
> > 
> > > +static struct class pinctrl_class = {
> > > +       .name = "pinctrl",
> > > +       .owner = THIS_MODULE,
> > > +       .dev_groups = pinctrl_groups,
> > > +};
> > 
> > Why are you adding a new device class?
> > 
> > IIRC Greg don't like them, we should probably just deal with the devices
> > directly on the bus where they are, which also implies some topology search
> > etc which is a feature.
> 
> I am ok for both, 
> 
> I thought adding new device class will be more generic and 
> people can use simple sysfs[1] like pwm for output disable operation
> rather than the SoC specific operation[2].
> 
> [1]
> /sys/class/pinctrl/output-disable/configure

That's fine, but you don't need a class for it, use configfs for
configuring things like this, that is what it is there for.

thanks,

greg k-h

Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()

[Andy Shevchenko]

On Tue, Mar 7, 2023 at 10:13 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()
> > Mon, Mar 06, 2023 at 09:00:02AM +0000, Biju Das kirjoitti:

...

> > > Add pinctrl_get_device() to find a device handle associated with a
> > > pincontrol group(i.e. by matching function name and group name for a
> > > device). This device handle can then be used for finding match for the
> > > pin output disable device that protects device against short circuits
> > > on the pins.
> >
> > Not sure I understand the use case. Please, create a better commit message.
>
> OK, Basically pinmux_enable_setting allows exclusive access of pin to a device.
> It won't allow multiple devices to claim a pin.

Which is correct. No? Show me the schematics of the real use case for that.

The owner of the pin is the host side. I can't imagine how the same
pin is shared inside the SoC. Is it broken hardware design?

...

> > Also it is missing the explanation why there will be no collisions when
> > looking by the same pair of function and group name from different device.
>
> setting->data.mux will be unique for a pin. So there won't be a collision when
> looking by the same pair of function and group name from different device.
>
> > (Always imagine that you have 2+ same IP blocks on the platform before doing
> > any pin control core work. This will help you to design it properly. )

Not sure how the pair function_name group_name makes the device unique.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()

[Linus Walleij]

On Tue, Mar 7, 2023 at 9:13 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Mon, Mar 06, 2023 at 09:00:02AM +0000, Biju Das kirjoitti:

> > > Add pinctrl_get_device() to find a device handle associated with a
> > > pincontrol group(i.e. by matching function name and group name for a
> > > device). This device handle can then be used for finding match for the
> > > pin output disable device that protects device against short circuits
> > > on the pins.
> >
> > Not sure I understand the use case. Please, create a better commit message.
>
> OK, Basically pinmux_enable_setting allows exclusive access of pin to a device.
> It won't allow multiple devices to claim a pin.

So what is the use case? Which two devices need to use the same
pin at the same time?

You can already:

1) Use the same pin with different devices at different times, because
   pin configs can be changed arbitrarily at runtime, see for example:
  drivers/i2c/muxes/i2c-demux-pinctrl.c

2) Mux a pin to a certain device *and* use it for GPIO at the same time,
  all that is needed is to set .strict to false in struct pinmux_ops.
  This should be false if e.g. the GPIO can be used to "sample" the
  output of a I2C block connected to the same pins, so the two
  functions (I2C and GPIO) are not electrically decoupled.

So do you really have a use case where two devices need to use the same
pin at the same time? I've seen much but I haven't seen this before!
Which two devices are that?

Yours,
Linus Walleij

RE: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()

[Biju Das]

Hi Linus and Andy,

> Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()
> 
> On Tue, Mar 7, 2023 at 9:13 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > Mon, Mar 06, 2023 at 09:00:02AM +0000, Biju Das kirjoitti:
> 
> > > > Add pinctrl_get_device() to find a device handle associated with a
> > > > pincontrol group(i.e. by matching function name and group name for
> > > > a device). This device handle can then be used for finding match
> > > > for the pin output disable device that protects device against
> > > > short circuits on the pins.
> > >
> > > Not sure I understand the use case. Please, create a better commit
> message.
> >
> > OK, Basically pinmux_enable_setting allows exclusive access of pin to a
> device.
> > It won't allow multiple devices to claim a pin.
> 
> So what is the use case? Which two devices need to use the same pin at the
> same time?

Andy asked a question Can a pin be used by multiple devices at same time
My answer is no. 

The reason is,

There will be a single owner claiming a pin at given time.
setting->data.mux will be unique for a pin. So there won't be a collision when
looking by the same pair of function and group name from different device.

> 
> You can already:
> 
> 1) Use the same pin with different devices at different times, because
>    pin configs can be changed arbitrarily at runtime, see for example:
>   drivers/i2c/muxes/i2c-demux-pinctrl.c

Agreed.

> 
> 2) Mux a pin to a certain device *and* use it for GPIO at the same time,
>   all that is needed is to set .strict to false in struct pinmux_ops.
>   This should be false if e.g. the GPIO can be used to "sample" the
>   output of a I2C block connected to the same pins, so the two
>   functions (I2C and GPIO) are not electrically decoupled.
> 
> So do you really have a use case where two devices need to use the same pin
> at the same time? I've seen much but I haven't seen this before!
> Which two devices are that?

I don't have a use case like that.

Cheers,
Biju

RE: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()

[Biju Das]

Hi Andy, 

Thanks for the feedback.

> Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()
> 
> On Tue, Mar 7, 2023 at 10:13 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add
> > > pinctrl_get_device() Mon, Mar 06, 2023 at 09:00:02AM +0000, Biju Das
> kirjoitti:
> 
> ...
> 
> > > > Add pinctrl_get_device() to find a device handle associated with a
> > > > pincontrol group(i.e. by matching function name and group name for
> > > > a device). This device handle can then be used for finding match
> > > > for the pin output disable device that protects device against
> > > > short circuits on the pins.
> > >
> > > Not sure I understand the use case. Please, create a better commit
> message.
> >
> > OK, Basically pinmux_enable_setting allows exclusive access of pin to a
> device.
> > It won't allow multiple devices to claim a pin.
> 
> Which is correct. No? Show me the schematics of the real use case for that.
> 
> The owner of the pin is the host side. I can't imagine how the same pin is
> shared inside the SoC. Is it broken hardware design?

We are discussing usage of 

echo "fname gname" and you asked a question whether multiple devices can
claim a pin at the same time

and my answer is no.

as setting->data.mux will be unique for a pin and will be claimed by
device during commit state.

Am I missing anything here??

Cheers,
Biju

> 
> ...
> 
> > > Also it is missing the explanation why there will be no collisions
> > > when looking by the same pair of function and group name from different
> device.
> >
> > setting->data.mux will be unique for a pin. So there won't be a
> > setting->collision when
> > looking by the same pair of function and group name from different device.
> >
> > > (Always imagine that you have 2+ same IP blocks on the platform
> > > before doing any pin control core work. This will help you to design
> > > it properly. )
> 
> Not sure how the pair function_name group_name makes the device unique.

Do you agree Device handle + function_name +  group_name make it unique.

For pin outdisable we are making use of this 3 combination.
See the details.


This patch series adds support for controlling output disable function using
sysfs in a generic way as described below.

|    A     |    |     B      |    |     C     |    |     D        |  | E |
|user space|--->|pinctrl core|<-->|SoC pinctrl|<-->|Output disable|--|PWM|
|          |    |            |    |           |    |              |  |   |

A executes command to configure a pin group for pin output disable operation
  echo "fname gname conf conf_val" > configure

B parses the command and identifies the binding device associated with that
  pin group

C matches the binding device against the device registered by D for
  configuration operation

D matches the pin group and configure the pins for Output disable operation

Both D and E are linked together by dt(i.e. PWM channel linked with Output
 disable Port)

Cheers,
Biju

Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()

[Andy Shevchenko]

On Thu, Mar 9, 2023 at 3:26 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()
> > On Tue, Mar 7, 2023 at 10:13 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add
> > > > pinctrl_get_device() Mon, Mar 06, 2023 at 09:00:02AM +0000, Biju Das
> > kirjoitti:

...

> > > > > Add pinctrl_get_device() to find a device handle associated with a
> > > > > pincontrol group(i.e. by matching function name and group name for
> > > > > a device). This device handle can then be used for finding match
> > > > > for the pin output disable device that protects device against
> > > > > short circuits on the pins.
> > > >
> > > > Not sure I understand the use case. Please, create a better commit
> > message.
> > >
> > > OK, Basically pinmux_enable_setting allows exclusive access of pin to a
> > device.
> > > It won't allow multiple devices to claim a pin.

This is a confusion you brought. You got us completely lost. Please,
try again from the clean sheet.

> > Which is correct. No? Show me the schematics of the real use case for that.
> >
> > The owner of the pin is the host side. I can't imagine how the same pin is
> > shared inside the SoC. Is it broken hardware design?
>
> We are discussing usage of
>
> echo "fname gname" and you asked a question whether multiple devices can
> claim a pin at the same time
>
> and my answer is no.

> as setting->data.mux will be unique for a pin and will be claimed by
> device during commit state.
>
> Am I missing anything here??

Yes. The same fname/gname can be in many *pin control* (provider) devices.

So, it does not uniquely define the pin control device.

...

> > > > Also it is missing the explanation why there will be no collisions
> > > > when looking by the same pair of function and group name from different
> > device.
> > >
> > > setting->data.mux will be unique for a pin. So there won't be a
> > > setting->collision when
> > > looking by the same pair of function and group name from different device.
> > >
> > > > (Always imagine that you have 2+ same IP blocks on the platform
> > > > before doing any pin control core work. This will help you to design
> > > > it properly. )
> >
> > Not sure how the pair function_name group_name makes the device unique.
>
> Do you agree Device handle + function_name +  group_name make it unique.

Yes.

> For pin outdisable we are making use of this 3 combination.
> See the details.
>
>
> This patch series adds support for controlling output disable function using
> sysfs in a generic way as described below.
>
> |    A     |    |     B      |    |     C     |    |     D        |  | E |
> |user space|--->|pinctrl core|<-->|SoC pinctrl|<-->|Output disable|--|PWM|
> |          |    |            |    |           |    |              |  |   |
>
> A executes command to configure a pin group for pin output disable operation
>   echo "fname gname conf conf_val" > configure
>
> B parses the command and identifies the binding device associated with that
>   pin group
>
> C matches the binding device against the device registered by D for
>   configuration operation
>
> D matches the pin group and configure the pins for Output disable operation
>
> Both D and E are linked together by dt(i.e. PWM channel linked with Output
>  disable Port)

Sounds like an overengineered hack to achieve something that I can't
read between the lines. Why is this so complicated interface and flow
are needed to begin with?


-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()

[Biju Das]

Hi Andy,

Thanks for the explanation.

> Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()
> 
> On Thu, Mar 9, 2023 at 3:26 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add
> > > pinctrl_get_device() On Tue, Mar 7, 2023 at 10:13 AM Biju Das
> <biju.das.jz@bp.renesas.com> wrote:
> > > > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add
> > > > > pinctrl_get_device() Mon, Mar 06, 2023 at 09:00:02AM +0000, Biju
> > > > > Das
> > > kirjoitti:
> 
> ...
> 
> > > > > > Add pinctrl_get_device() to find a device handle associated
> > > > > > with a pincontrol group(i.e. by matching function name and
> > > > > > group name for a device). This device handle can then be used
> > > > > > for finding match for the pin output disable device that
> > > > > > protects device against short circuits on the pins.
> > > > >
> > > > > Not sure I understand the use case. Please, create a better
> > > > > commit
> > > message.
> > > >
> > > > OK, Basically pinmux_enable_setting allows exclusive access of pin
> > > > to a
> > > device.
> > > > It won't allow multiple devices to claim a pin.
> 
> This is a confusion you brought. You got us completely lost. Please, try
> again from the clean sheet.
> 
> > > Which is correct. No? Show me the schematics of the real use case for
> that.
> > >
> > > The owner of the pin is the host side. I can't imagine how the same
> > > pin is shared inside the SoC. Is it broken hardware design?
> >
> > We are discussing usage of
> >
> > echo "fname gname" and you asked a question whether multiple devices
> > can claim a pin at the same time
> >
> > and my answer is no.
> 
> > as setting->data.mux will be unique for a pin and will be claimed by
> > device during commit state.
> >
> > Am I missing anything here??
> 
> Yes. The same fname/gname can be in many *pin control* (provider) devices.

I agree. 

I have used the code used for [1] getting *pin control* devices associated with function name
and group name.

[1]
cat /sys/kernel/debug/pinctrl/pinctrl-handles

if output of [1], can return multiple devices for a given fname/gname, then I am wrong.
Please correct me if that is the case.

So I was under the impression that [2], it will fail if multiple devices try to acquire a pin.

[2]
https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/pinmux.c#L132


> 
> So, it does not uniquely define the pin control device.

I agree.


> 
> ...
> 
> > > > > Also it is missing the explanation why there will be no
> > > > > collisions when looking by the same pair of function and group
> > > > > name from different
> > > device.
> > > >
> > > > setting->data.mux will be unique for a pin. So there won't be a
> > > > setting->collision when
> > > > looking by the same pair of function and group name from different
> device.
> > > >
> > > > > (Always imagine that you have 2+ same IP blocks on the platform
> > > > > before doing any pin control core work. This will help you to
> > > > > design it properly. )
> > >
> > > Not sure how the pair function_name group_name makes the device unique.
> >
> > Do you agree Device handle + function_name +  group_name make it unique.
> 
> Yes.
> 
> > For pin outdisable we are making use of this 3 combination.
> > See the details.
> >
> >
> > This patch series adds support for controlling output disable function
> > using sysfs in a generic way as described below.
> >
> > |    A     |    |     B      |    |     C     |    |     D        |  | E |
> > |user space|--->|pinctrl core|<-->|SoC pinctrl|<-->|Output disable|--|PWM|
> > |          |    |            |    |           |    |              |  |   |
> >
> > A executes command to configure a pin group for pin output disable
> operation
> >   echo "fname gname conf conf_val" > configure
> >
> > B parses the command and identifies the binding device associated with
> that
> >   pin group
> >
> > C matches the binding device against the device registered by D for
> >   configuration operation
> >
> > D matches the pin group and configure the pins for Output disable
> > operation
> >
> > Both D and E are linked together by dt(i.e. PWM channel linked with
> > Output  disable Port)
> 
> Sounds like an overengineered hack to achieve something that I can't read
> between the lines. Why is this so complicated interface and flow are needed
> to begin with?

It is very simple.

I am trying to give details like how a pwm pins used for pin output disable
is  configured from user space in a generic way.

My use case is,

I have an IP which detects short circuit between the output terminals and 
disable the output from pwm pins ,when it detects short circuit to protect from
system failure.

pwm-pins are involved in this operation.

From user space we need to configure the type of protection for this pins (eg: disable PWM output,
when both pwm outputs are high at same time).

For that, we need to find a provider device (which provides gpt-pins).

pinctrl_get_device() returns "current provider device" associated with fname/gname.
If " current provider device" == "pwm device" do the configuration.

Cheers,
Biju

RE: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()

[Biju Das]

Hi Andy,

> Subject: RE: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()
> 
> Hi Andy,
> 
> Thanks for the explanation.
> 
> > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()
> >
> > On Thu, Mar 9, 2023 at 3:26 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add
> > > > pinctrl_get_device() On Tue, Mar 7, 2023 at 10:13 AM Biju Das
> > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add
> > > > > > pinctrl_get_device() Mon, Mar 06, 2023 at 09:00:02AM +0000,
> > > > > > Biju Das
> > > > kirjoitti:
> >
> > ...
> >
> > > > > > > Add pinctrl_get_device() to find a device handle associated
> > > > > > > with a pincontrol group(i.e. by matching function name and
> > > > > > > group name for a device). This device handle can then be
> > > > > > > used for finding match for the pin output disable device
> > > > > > > that protects device against short circuits on the pins.
> > > > > >
> > > > > > Not sure I understand the use case. Please, create a better
> > > > > > commit
> > > > message.
> > > > >
> > > > > OK, Basically pinmux_enable_setting allows exclusive access of
> > > > > pin to a
> > > > device.
> > > > > It won't allow multiple devices to claim a pin.
> >
> > This is a confusion you brought. You got us completely lost. Please,
> > try again from the clean sheet.
> >
> > > > Which is correct. No? Show me the schematics of the real use case
> > > > for
> > that.
> > > >
> > > > The owner of the pin is the host side. I can't imagine how the
> > > > same pin is shared inside the SoC. Is it broken hardware design?
> > >
> > > We are discussing usage of
> > >
> > > echo "fname gname" and you asked a question whether multiple devices
> > > can claim a pin at the same time
> > >
> > > and my answer is no.
> >
> > > as setting->data.mux will be unique for a pin and will be claimed by
> > > device during commit state.
> > >
> > > Am I missing anything here??
> >
> > Yes. The same fname/gname can be in many *pin control* (provider) devices.
> 
> I agree.
> 
> I have used the code used for [1] getting *pin control* devices associated
> with function name and group name.
> 
> [1]
> cat /sys/kernel/debug/pinctrl/pinctrl-handles
> 
> if output of [1], can return multiple devices for a given fname/gname, then
> I am wrong.
> Please correct me if that is the case.
> 
> So I was under the impression that [2], it will fail if multiple devices try
> to acquire a pin.
> 
> [2]
> https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/pinmux.c#L132


I have done the below modifications in my dt and confirm that, pinctrl framework
doesn't allow 2 devices to claim pins  with a given function name and group name
at same time.

dt-changes:
&gpt {
+               gpt6-pins {
+                       pinmux = <RZG2L_PORT_PINMUX(44, 0, 5)>, /* GTIOC6A */
+                                <RZG2L_PORT_PINMUX(44, 1, 5)>; /* GTIOC6B */
+               };
+
+               gpt7-pins {
+                       pinmux = <RZG2L_PORT_PINMUX(44, 2, 5)>, /* GTIOC7A */
+                                <RZG2L_PORT_PINMUX(44, 3, 5)>; /* GTIOC7B */
+               };
};

&mtu {

+               gpt6-pins {
+                       pinmux = <RZG2L_PORT_PINMUX(44, 0, 4)>, /* MTIOC3A */
+                                <RZG2L_PORT_PINMUX(44, 1, 4)>, /* MTIOC3B */
+                                <RZG2L_PORT_PINMUX(44, 2, 4)>, /* MTIOC3C */
+                                <RZG2L_PORT_PINMUX(44, 3, 4)>; /* MTIOC3D */
                };
};

Initially MTU device is claiming the pins P44_0 and P44_1.

root@smarc-rzg2l:~# cat /sys/kernel/debug/pinctrl/pinctrl-handles
	device: 10001200.timer current state: default
  state: default
    type: MUX_GROUP controller pinctrl-rzg2l group: mtu3_zphase_clk (3) function: mtu3_zphase_clk (3)
    type: MUX_GROUP controller pinctrl-rzg2l group: mtu3_clk (4) function: mtu3_clk (4)
    type: MUX_GROUP controller pinctrl-rzg2l group: gpt6-pins (5) function: gpt6-pins (5)

When It tried to load gpt, I get the below erro,
[   15.024714] pinctrl-rzg2l 11030000.pinctrl: pin P44_0 already requested by 10001200.timer; cannot claim for 10048000.pwm
         Starting Update UTMP about System Runlevel Changes...
[   15.044828] pinctrl-rzg2l 11030000.pinctrl: pin-352 (10048000.pwm) status -22
[   15.056515] pinctrl-rzg2l 11030000.pinctrl: could not request pin 352 (P44_0) from group gpt6-pins  on device pinctrl-rzg2l
[   15.070224] pwm-rzg2l-gpt 10048000.pwm: Error applying setting, reverse things back

After unloading mtu modules and loading gpt modules, gpt module is claims the pin.

root@smarc-rzg2l:~# rmmod rz_mtu3_cnt
root@smarc-rzg2l:~# rmmod pwm_rz_mtu3
root@smarc-rzg2l:~# cd /sys/bus/platform/drivers/rz-mtu3/
root@smarc-rzg2l:/sys/bus/platform/drivers/rz-mtu3# echo 10001200.timer > unbind
root@smarc-rzg2l:/sys/bus/platform/drivers/rz-mtu3# rmmod rzg2l_poeg
root@smarc-rzg2l:/sys/bus/platform/drivers/rz-mtu3# rmmod pwm_rzg2l_gpt
root@smarc-rzg2l:/sys/bus/platform/drivers/rz-mtu3# cat /sys/kernel/debug/pinctrl/pinctrl-handles | grep gpt
root@smarc-rzg2l:/sys/bus/platform/drivers/rz-mtu3#

root@smarc-rzg2l:/sys/bus/platform/drivers/rz-mtu3# modprobe rzg2l_poeg
root@smarc-rzg2l:/sys/bus/platform/drivers/rz-mtu3# modprobe pwm_rzg2l_gpt
root@smarc-rzg2l:/sys/bus/platform/drivers/rz-mtu3# cat /sys/kernel/debug/pinctrl/pinctrl-handles | grep gpt
    type: MUX_GROUP controller pinctrl-rzg2l group: gpt6-pins (5) function: gpt6-pins (5)
    type: MUX_GROUP controller pinctrl-rzg2l group: gpt7-pins (11) function: gpt7-pins (11)

root@smarc-rzg2l:/sys/bus/platform/drivers/rz-mtu3# cat /sys/kernel/debug/pinctrl/pinctrl-handles
device: 10048000.pwm current state: default
  state: default
    type: MUX_GROUP controller pinctrl-rzg2l group: gpt6-pins (5) function: gpt6-pins (5)
    type: MUX_GROUP controller pinctrl-rzg2l group: gpt7-pins (11) function: gpt7-pins (11)


Since I have used same function name/ group name it gets a unique selector(5) and the functionality for
gpt is not working as the selector meant for MTU.

But if I replace "gpt6-pins" -> "mtu-pins" in MTU node, then the functionality works as expected as
the selector is different for mtu-pins and gpt-pins eventhough both uses same pins.

So I believe current implementation is based on /sys/kernel/debug/pinctrl/pinctrl-handles is
good unless I miss something here. Please correct me,if you think my observation is wrong.

Cheers,
Biju

Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()

[Linus Walleij]

On Thu, Mar 9, 2023 at 3:19 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:

> I have an IP which detects short circuit between the output terminals and
> disable the output from pwm pins ,when it detects short circuit to protect from
> system failure.
>
> pwm-pins are involved in this operation.
>
> From user space we need to configure the type of protection for this pins (eg: disable PWM output,
> when both pwm outputs are high at same time).

Why do you want to do this from user space?

It sounds like something the kernel should be doing.

The kernel has a PWM subsystem, and a pin control subsystem,
and we don't even have a userspace ABI for pin control.
Pin control is designed to avoid electrical disasters
and a driver can add further policy for sure.

If you want to add policy of different types to avoid electrical
disaster into the pin control driver, go ahead, just run it by
Geert so he's on board with the ideas.

> For that, we need to find a provider device (which provides gpt-pins).
>
> pinctrl_get_device() returns "current provider device" associated with fname/gname.
> If " current provider device" == "pwm device" do the configuration.

I don't understand this, sorry.

Yours,
Linus Walleij

RE: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()

[Biju Das]

Hi Linus W,

Thanks for feedback.

> Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()
> 
> On Thu, Mar 9, 2023 at 3:19 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> 
> > I have an IP which detects short circuit between the output terminals
> > and disable the output from pwm pins ,when it detects short circuit to
> > protect from system failure.
> >
> > pwm-pins are involved in this operation.
> >
> > From user space we need to configure the type of protection for this
> > pins (eg: disable PWM output, when both pwm outputs are high at same
> time).
> 
> Why do you want to do this from user space?

To take care of the below features provided by Port Output Enable for GPT(a.k.a PWM)
(POEG) IP. 

The output pins of the general PWM timer (GPT) can be disabled by
using the port output enabling function for the GPT (POEG). 
Specifically, either of the following ways can be used[1].

[1]

Use case 1)
● Input level detection of the GTETRGA to GTETRGD pins (i.e: detect short circuit in switching circuit
externally and use an external pin(GTETRGA to GTETRGD) to disable the output pins of PWM)

Use case 2)
● Output-disable request from the GPT (GPT detects short circuit in switching circuit internally and 
disable the output pins of PWM)

Use case 3)
● Register settings(Detect short circuit in switching circuit
externally and use internal register to disable the output pins of PWM)

The advantage of providing these options from user space is, it is flexible.
Runtime user can configure the use case he wants to use for his product.

+Rob, + Krzysztof Kozlowski

If we cannot do it in user space, then we need to make it as part of
Dt bindings and users will define the use case they need in DT.

Some of the failure conditions in switching circuits are like below

when you use PWM push-pull configuration you SHOULD have a dead time.
When + mosfet is turned off - mosfet can't be immediately turned on
because you will create a direct path (short circuit) between + and -
as parasitic capacitance will leave + mosfet turned on for a while .
This will destroy your mosfets.

Dead time is the delay measured from turning off the driver switch
connected to one rail of the power supply to the time the switch
connected to the other rail of the power supply is turned on. 
Switching devices like MOSFETs and IGBTs turn off after a delay 
when the gate drive is turned off. If the other switch on the half
bridge is turned on immediately, both upper and lower switches may be 
in a conductive region for a brief moment, shorting the power rail. 
In order to avoid this, a dead time is maintained between turning off
of one switch and turning on the other in a half bridge.

POEG IP provides the below protections,

1) When the GTIOCA pin and the GTIOCB pin(PWM pins) are driven to an active level simultaneously, the
PWM generates an output-disable request to the POEG. Through reception of these requests,
the POEG can control whether the GTIOCA and GTIOCB pins are output-disabled

2) PWM output pins can be set to be disabled when the PWM output pins detect a dead time error
or short circuit detection between the output terminals

> 
> It sounds like something the kernel should be doing.

If we cannot do the above use cases[1] in user space, then we need to make it as part of
Dt bindings and it will be fully taken care in kernel.

> 
> The kernel has a PWM subsystem, and a pin control subsystem, and we don't
> even have a userspace ABI for pin control.
> Pin control is designed to avoid electrical disasters and a driver can add
> further policy for sure.
> 
> If you want to add policy of different types to avoid electrical disaster
> into the pin control driver, go ahead, just run it by Geert so he's on board
> with the ideas.

OK. Geert Can you please provide valuable suggestion how to address this use cases[1]
As mentioned above?

Currently Pin control driver is used for identifying the pwm device by using
pwm gname/fname and configuring the various use cases as mentioned above[1]
for a system.

> 
> > For that, we need to find a provider device (which provides gpt-pins).
> >
> > pinctrl_get_device() returns "current provider device" associated with
> fname/gname.
> > If " current provider device" == "pwm device" do the configuration.
> 
> I don't understand this, sorry.

Andy mentioned, the same fname/gname can be used in many *pin control* (provider) devices.

But when checking the code(/sys/kernel/debug/pinctrl/pinctrl-handles), at a given time, 
there will be only one device claims Pins associated with a given fname/gname. 

Currently pinctrl_get_device() returns the current *pin control* (provider) device
that claimed pins associated with a given fname/gname.

Cheers,
Biju

Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()

[Linus Walleij]

On Tue, Mar 14, 2023 at 9:27 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:

> If we cannot do it in user space, then we need to make it as part of
> Dt bindings and users will define the use case they need in DT.

That sounds like a much better idea :)

The kernel is for protecting hardware from users after all, and it
seems you want to select one of these use cases and DT is
excellent for system config like this.

So I would say work ahead on this path.

Yours,
Linus Walleij

Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()

[Geert Uytterhoeven]

Hi Biju,

On Tue, Mar 14, 2023 at 9:27 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()
> > On Thu, Mar 9, 2023 at 3:19 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > I have an IP which detects short circuit between the output terminals
> > > and disable the output from pwm pins ,when it detects short circuit to
> > > protect from system failure.
> > >
> > > pwm-pins are involved in this operation.
> > >
> > > From user space we need to configure the type of protection for this
> > > pins (eg: disable PWM output, when both pwm outputs are high at same
> > time).
> >
> > Why do you want to do this from user space?
>
> To take care of the below features provided by Port Output Enable for GPT(a.k.a PWM)
> (POEG) IP.
>
> The output pins of the general PWM timer (GPT) can be disabled by
> using the port output enabling function for the GPT (POEG).
> Specifically, either of the following ways can be used[1].
>
> [1]
>
> Use case 1)
> ● Input level detection of the GTETRGA to GTETRGD pins (i.e: detect short circuit in switching circuit
> externally and use an external pin(GTETRGA to GTETRGD) to disable the output pins of PWM)
>
> Use case 2)
> ● Output-disable request from the GPT (GPT detects short circuit in switching circuit internally and
> disable the output pins of PWM)
>
> Use case 3)
> ● Register settings(Detect short circuit in switching circuit
> externally and use internal register to disable the output pins of PWM)
>
> The advantage of providing these options from user space is, it is flexible.
> Runtime user can configure the use case he wants to use for his product.
>
> +Rob, + Krzysztof Kozlowski
>
> If we cannot do it in user space, then we need to make it as part of
> Dt bindings and users will define the use case they need in DT.
>
> Some of the failure conditions in switching circuits are like below
>
> when you use PWM push-pull configuration you SHOULD have a dead time.
> When + mosfet is turned off - mosfet can't be immediately turned on
> because you will create a direct path (short circuit) between + and -
> as parasitic capacitance will leave + mosfet turned on for a while .
> This will destroy your mosfets.
>
> Dead time is the delay measured from turning off the driver switch
> connected to one rail of the power supply to the time the switch
> connected to the other rail of the power supply is turned on.
> Switching devices like MOSFETs and IGBTs turn off after a delay
> when the gate drive is turned off. If the other switch on the half
> bridge is turned on immediately, both upper and lower switches may be
> in a conductive region for a brief moment, shorting the power rail.
> In order to avoid this, a dead time is maintained between turning off
> of one switch and turning on the other in a half bridge.
>
> POEG IP provides the below protections,
>
> 1) When the GTIOCA pin and the GTIOCB pin(PWM pins) are driven to an active level simultaneously, the
> PWM generates an output-disable request to the POEG. Through reception of these requests,
> the POEG can control whether the GTIOCA and GTIOCB pins are output-disabled
>
> 2) PWM output pins can be set to be disabled when the PWM output pins detect a dead time error
> or short circuit detection between the output terminals
>
> >
> > It sounds like something the kernel should be doing.
>
> If we cannot do the above use cases[1] in user space, then we need to make it as part of
> Dt bindings and it will be fully taken care in kernel.
>
> >
> > The kernel has a PWM subsystem, and a pin control subsystem, and we don't
> > even have a userspace ABI for pin control.
> > Pin control is designed to avoid electrical disasters and a driver can add
> > further policy for sure.
> >
> > If you want to add policy of different types to avoid electrical disaster
> > into the pin control driver, go ahead, just run it by Geert so he's on board
> > with the ideas.
>
> OK. Geert Can you please provide valuable suggestion how to address this use cases[1]
> As mentioned above?

Is this configuration you need to do once, based on the hardware,
or do you need to change it at run-time?

Before, I was under the impression you needed to change the
configuration at run-time, hence the need for a sysfs API.
If configuration is static, DT is the way to go.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()
> 
> Hi Biju,
> 
> On Tue, Mar 14, 2023 at 9:27 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add
> > > pinctrl_get_device() On Thu, Mar 9, 2023 at 3:19 PM Biju Das
> <biju.das.jz@bp.renesas.com> wrote:
> > > > I have an IP which detects short circuit between the output
> > > > terminals and disable the output from pwm pins ,when it detects
> > > > short circuit to protect from system failure.
> > > >
> > > > pwm-pins are involved in this operation.
> > > >
> > > > From user space we need to configure the type of protection for
> > > > this pins (eg: disable PWM output, when both pwm outputs are high
> > > > at same
> > > time).
> > >
> > > Why do you want to do this from user space?
> >
> > To take care of the below features provided by Port Output Enable for
> > GPT(a.k.a PWM)
> > (POEG) IP.
> >
> > The output pins of the general PWM timer (GPT) can be disabled by
> > using the port output enabling function for the GPT (POEG).
> > Specifically, either of the following ways can be used[1].
> >
> > [1]
> >
> > Use case 1)
> > ● Input level detection of the GTETRGA to GTETRGD pins (i.e: detect
> > short circuit in switching circuit externally and use an external
> > pin(GTETRGA to GTETRGD) to disable the output pins of PWM)
> >
> > Use case 2)
> > ● Output-disable request from the GPT (GPT detects short circuit in
> > switching circuit internally and disable the output pins of PWM)
> >
> > Use case 3)
> > ● Register settings(Detect short circuit in switching circuit
> > externally and use internal register to disable the output pins of
> > PWM)
> >
> > The advantage of providing these options from user space is, it is
> flexible.
> > Runtime user can configure the use case he wants to use for his product.
> >
> > +Rob, + Krzysztof Kozlowski
> >
> > If we cannot do it in user space, then we need to make it as part of
> > Dt bindings and users will define the use case they need in DT.
> >
> > Some of the failure conditions in switching circuits are like below
> >
> > when you use PWM push-pull configuration you SHOULD have a dead time.
> > When + mosfet is turned off - mosfet can't be immediately turned on
> > because you will create a direct path (short circuit) between + and -
> > as parasitic capacitance will leave + mosfet turned on for a while .
> > This will destroy your mosfets.
> >
> > Dead time is the delay measured from turning off the driver switch
> > connected to one rail of the power supply to the time the switch
> > connected to the other rail of the power supply is turned on.
> > Switching devices like MOSFETs and IGBTs turn off after a delay when
> > the gate drive is turned off. If the other switch on the half bridge
> > is turned on immediately, both upper and lower switches may be in a
> > conductive region for a brief moment, shorting the power rail.
> > In order to avoid this, a dead time is maintained between turning off
> > of one switch and turning on the other in a half bridge.
> >
> > POEG IP provides the below protections,
> >
> > 1) When the GTIOCA pin and the GTIOCB pin(PWM pins) are driven to an
> > active level simultaneously, the PWM generates an output-disable
> > request to the POEG. Through reception of these requests, the POEG can
> > control whether the GTIOCA and GTIOCB pins are output-disabled
> >
> > 2) PWM output pins can be set to be disabled when the PWM output pins
> > detect a dead time error or short circuit detection between the output
> > terminals
> >
> > >
> > > It sounds like something the kernel should be doing.
> >
> > If we cannot do the above use cases[1] in user space, then we need to
> > make it as part of Dt bindings and it will be fully taken care in kernel.
> >
> > >
> > > The kernel has a PWM subsystem, and a pin control subsystem, and we
> > > don't even have a userspace ABI for pin control.
> > > Pin control is designed to avoid electrical disasters and a driver
> > > can add further policy for sure.
> > >
> > > If you want to add policy of different types to avoid electrical
> > > disaster into the pin control driver, go ahead, just run it by Geert
> > > so he's on board with the ideas.
> >
> > OK. Geert Can you please provide valuable suggestion how to address
> > this use cases[1] As mentioned above?
> 
> Is this configuration you need to do once, based on the hardware, or do you
> need to change it at run-time?

I think this configuration needed only once based on the hardware.

> 
> Before, I was under the impression you needed to change the configuration at
> run-time, hence the need for a sysfs API.
> If configuration is static, DT is the way to go.

At that time, I was not explored the possibility of creating poeg char device.

For eg: After the initial setting in DT, I guess with poeg char device we should be able to
achieve below use cases.

Use case 1)
 We can provide user space event indicating, Output-disable request from the GTETRGn pin occurred.
and provide some options (rd/wr file ops) to user space to clear the fault.

Use case 2)
 We can provide user space event indicating, Output-disable request from GPT disable request occurred.
 and provide some options(rd/wr file ops) to user space to clear the fault.

Use case 3)
 User space to control Output-disable through rd/wr file ops.

Please let me know is it ok or am I missing something here??

Cheers,
Biju

Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()

[Geert Uytterhoeven]

Hi Biju,

On Tue, Mar 14, 2023 at 12:33 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()
> > On Tue, Mar 14, 2023 at 9:27 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > Subject: Re: [PATCH v6 01/13] pinctrl: core: Add
> > > > pinctrl_get_device() On Thu, Mar 9, 2023 at 3:19 PM Biju Das
> > <biju.das.jz@bp.renesas.com> wrote:
> > > > > I have an IP which detects short circuit between the output
> > > > > terminals and disable the output from pwm pins ,when it detects
> > > > > short circuit to protect from system failure.
> > > > >
> > > > > pwm-pins are involved in this operation.
> > > > >
> > > > > From user space we need to configure the type of protection for
> > > > > this pins (eg: disable PWM output, when both pwm outputs are high
> > > > > at same
> > > > time).
> > > >
> > > > Why do you want to do this from user space?
> > >
> > > To take care of the below features provided by Port Output Enable for
> > > GPT(a.k.a PWM)
> > > (POEG) IP.
> > >
> > > The output pins of the general PWM timer (GPT) can be disabled by
> > > using the port output enabling function for the GPT (POEG).
> > > Specifically, either of the following ways can be used[1].
> > >
> > > [1]
> > >
> > > Use case 1)
> > > ● Input level detection of the GTETRGA to GTETRGD pins (i.e: detect
> > > short circuit in switching circuit externally and use an external
> > > pin(GTETRGA to GTETRGD) to disable the output pins of PWM)
> > >
> > > Use case 2)
> > > ● Output-disable request from the GPT (GPT detects short circuit in
> > > switching circuit internally and disable the output pins of PWM)
> > >
> > > Use case 3)
> > > ● Register settings(Detect short circuit in switching circuit
> > > externally and use internal register to disable the output pins of
> > > PWM)
> > >
> > > The advantage of providing these options from user space is, it is
> > flexible.
> > > Runtime user can configure the use case he wants to use for his product.
> > >
> > > +Rob, + Krzysztof Kozlowski
> > >
> > > If we cannot do it in user space, then we need to make it as part of
> > > Dt bindings and users will define the use case they need in DT.
> > >
> > > Some of the failure conditions in switching circuits are like below
> > >
> > > when you use PWM push-pull configuration you SHOULD have a dead time.
> > > When + mosfet is turned off - mosfet can't be immediately turned on
> > > because you will create a direct path (short circuit) between + and -
> > > as parasitic capacitance will leave + mosfet turned on for a while .
> > > This will destroy your mosfets.
> > >
> > > Dead time is the delay measured from turning off the driver switch
> > > connected to one rail of the power supply to the time the switch
> > > connected to the other rail of the power supply is turned on.
> > > Switching devices like MOSFETs and IGBTs turn off after a delay when
> > > the gate drive is turned off. If the other switch on the half bridge
> > > is turned on immediately, both upper and lower switches may be in a
> > > conductive region for a brief moment, shorting the power rail.
> > > In order to avoid this, a dead time is maintained between turning off
> > > of one switch and turning on the other in a half bridge.
> > >
> > > POEG IP provides the below protections,
> > >
> > > 1) When the GTIOCA pin and the GTIOCB pin(PWM pins) are driven to an
> > > active level simultaneously, the PWM generates an output-disable
> > > request to the POEG. Through reception of these requests, the POEG can
> > > control whether the GTIOCA and GTIOCB pins are output-disabled
> > >
> > > 2) PWM output pins can be set to be disabled when the PWM output pins
> > > detect a dead time error or short circuit detection between the output
> > > terminals
> > >
> > > >
> > > > It sounds like something the kernel should be doing.
> > >
> > > If we cannot do the above use cases[1] in user space, then we need to
> > > make it as part of Dt bindings and it will be fully taken care in kernel.
> > >
> > > >
> > > > The kernel has a PWM subsystem, and a pin control subsystem, and we
> > > > don't even have a userspace ABI for pin control.
> > > > Pin control is designed to avoid electrical disasters and a driver
> > > > can add further policy for sure.
> > > >
> > > > If you want to add policy of different types to avoid electrical
> > > > disaster into the pin control driver, go ahead, just run it by Geert
> > > > so he's on board with the ideas.
> > >
> > > OK. Geert Can you please provide valuable suggestion how to address
> > > this use cases[1] As mentioned above?
> >
> > Is this configuration you need to do once, based on the hardware, or do you
> > need to change it at run-time?
>
> I think this configuration needed only once based on the hardware.

OK, so using DT for that would be fine.

> > Before, I was under the impression you needed to change the configuration at
> > run-time, hence the need for a sysfs API.
> > If configuration is static, DT is the way to go.
>
> At that time, I was not explored the possibility of creating poeg char device.
>
> For eg: After the initial setting in DT, I guess with poeg char device we should be able to
> achieve below use cases.
>
> Use case 1)
>  We can provide user space event indicating, Output-disable request from the GTETRGn pin occurred.
> and provide some options (rd/wr file ops) to user space to clear the fault.
>
> Use case 2)
>  We can provide user space event indicating, Output-disable request from GPT disable request occurred.
>  and provide some options(rd/wr file ops) to user space to clear the fault.
>
> Use case 3)
>  User space to control Output-disable through rd/wr file ops.
>
> Please let me know is it ok or am I missing something here??

Using a char device for that sounds fine to me.

If you just needed to clear the fault, you could use a device property in sysfs.
But that would still leave us without a way to provide events to userspace.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v6 03/13] pinctrl: Add sysfs support

[Biju Das]

Hi Greg,

> Subject: Re: [PATCH v6 03/13] pinctrl: Add sysfs support
> 
> On Tue, Mar 07, 2023 at 02:43:54PM +0000, Biju Das wrote:
> > Hi Linus Walleij,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH v6 03/13] pinctrl: Add sysfs support
> > >
> > > On Mon, Mar 6, 2023 at 10:00 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > >
> > > > Add a simple sysfs interface to the generic pinctrl framework for
> > > > configuring pins for output disable operation.
> > > >
> > > > /sys/class/pinctrl/
> > > >   `-- output-disable/
> > > >       |-- configure    (w/o) ask the kernel to configure a pin group
> > > >                              for output disable operation.
> > > >
> > > >   echo "<group-name function-name 0 1>" > configure
> > > >
> > > > The existing "pinmux-functions" debugfs file lists the pin
> > > > functions registered for the pin controller. For example:
> > > >
> > > >   function 0: usb0, groups = [ usb0 ]
> > > >   function 1: usb1, groups = [ usb1 ]
> > > >   function 2: gpt4-pins, groups = [ gpt4-pins ]
> > > >   function 3: scif0, groups = [ scif0 ]
> > > >   function 4: scif2, groups = [ scif2 ]
> > > >   function 5: spi1, groups = [ spi1 ]
> > > >
> > > > To configure gpt4-pins for output disable activation by user:
> > > >
> > > >   echo "gpt4-pins gpt4-pins 0 1" > configure
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > >
> > >
> > > You have to run things like this by Greg K-H (adde on To)
> > >
> > > > +static struct class pinctrl_class = {
> > > > +       .name = "pinctrl",
> > > > +       .owner = THIS_MODULE,
> > > > +       .dev_groups = pinctrl_groups, };
> > >
> > > Why are you adding a new device class?
> > >
> > > IIRC Greg don't like them, we should probably just deal with the
> > > devices directly on the bus where they are, which also implies some
> > > topology search etc which is a feature.
> >
> > I am ok for both,
> >
> > I thought adding new device class will be more generic and people can
> > use simple sysfs[1] like pwm for output disable operation rather than
> > the SoC specific operation[2].
> >
> > [1]
> > /sys/class/pinctrl/output-disable/configure
> 
> That's fine, but you don't need a class for it, use configfs for configuring
> things like this, that is what it is there for.

As per discussion with Linus W and Geert, it is agreed to use configure this
settings in device tree as DT is the best place for system config like these use
cases.

Cheers,
Biju

RE: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()

[Biju Das]

Hi Linus Walleij,

Thanks for the feedback.

> <krzysztof.kozlowski+dt@linaro.org>
> Subject: Re: [PATCH v6 01/13] pinctrl: core: Add pinctrl_get_device()
> 
> On Tue, Mar 14, 2023 at 9:27 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> 
> > If we cannot do it in user space, then we need to make it as part of
> > Dt bindings and users will define the use case they need in DT.
> 
> That sounds like a much better idea :)
> 
> The kernel is for protecting hardware from users after all, and it seems you
> want to select one of these use cases and DT is excellent for system config
> like this.
> 
> So I would say work ahead on this path.

Agreed. Will send next version based on this approach.

Cheers,
Biju