Re: [PATCH 12/32] MAINTAINERS: update lego,ev3-battery.yaml reference
On 4/1/21 7:17 AM, Mauro Carvalho Chehab wrote:
Changeset 3004e581d92a ("dt-bindings: power: supply: lego-ev3-battery: Convert to DT
schema format")
renamed: Documentation/devicetree/bindings/power/supply/lego_ev3_battery.txt
to: Documentation/devicetree/bindings/power/supply/lego,ev3-battery.yaml.
Update its cross-reference accordingly.
Fixes: 3004e581d92a ("dt-bindings: power: supply: lego-ev3-battery: Convert to DT
schema format")
Signed-off-by: Mauro Carvalho Chehab
---
Reviewed-by: David Lechner
Re: [PATCH v3 0/1] drm/tiny: add support for Waveshare 2inch LCD module
On 3/30/21 3:08 AM, Carlis wrote: From: Xuezhi Zhang This adds a new module for the ST7789V controller with parameters for the Waveshare 2inch LCD module. Signed-off-by: Xuezhi Zhang --- v2:change compatible value. v3:change author name. --- MAINTAINERS| 8 + drivers/gpu/drm/tiny/Kconfig | 14 ++ drivers/gpu/drm/tiny/Makefile | 1 + drivers/gpu/drm/tiny/st7789v.c | 269 + 4 files changed, 292 insertions(+) create mode 100644 drivers/gpu/drm/tiny/st7789v.c diff --git a/MAINTAINERS b/MAINTAINERS index d92f85ca831d..df25e8e0deb1 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -5769,6 +5769,14 @@ T: git git://anongit.freedesktop.org/drm/drm-misc F:Documentation/devicetree/bindings/display/sitronix,st7735r.yaml F:drivers/gpu/drm/tiny/st7735r.c +DRM DRIVER FOR SITRONIX ST7789V PANELS +M: David Lechner I should not be added here. I don't have one of these displays. +M: Xuezhi Zhang +S: Maintained +T: git git://anongit.freedesktop.org/drm/drm-misc +F: Documentation/devicetree/bindings/display/sitronix,st7789v-dbi.yaml +F: drivers/gpu/drm/tiny/st7789v.c + DRM DRIVER FOR SONY ACX424AKP PANELS M:Linus Walleij S:Maintained
Re: [PATCHv2 19/38] dt-bindings: power: supply: lego-ev3-battery: Convert to DT schema format
On 3/17/21 8:48 AM, Sebastian Reichel wrote: Convert the binding to DT schema format. Cc: David Lechner Signed-off-by: Sebastian Reichel --- Reviewed-by: David Lechner
Re: [PATCH 19/38] dt-bindings: power: supply: lego-ev3-battery: Convert to DT schema format
On 3/12/21 9:43 AM, Sebastian Reichel wrote: Convert the binding to DT schema format. Cc: David Lechner Signed-off-by: Sebastian Reichel --- Reviewed-by: David Lechner
Re: [PATCH v8 20/22] counter: Implement events_queue_size sysfs attribute
On 2/25/21 6:03 PM, William Breathitt Gray wrote:
On Sun, Feb 21, 2021 at 03:51:40PM +, Jonathan Cameron wrote:
On Thu, 18 Feb 2021 19:32:16 +0900
William Breathitt Gray wrote:
On Sun, Feb 14, 2021 at 06:11:46PM +, Jonathan Cameron wrote:
On Fri, 12 Feb 2021 21:13:44 +0900
William Breathitt Gray wrote:
The events_queue_size sysfs attribute provides a way for users to
dynamically configure the Counter events queue size for the Counter
character device interface. The size is in number of struct
counter_event data structures. The number of elements will be rounded-up
to a power of 2 due to a requirement of the kfifo_alloc function called
during reallocation of the queue.
Cc: Oleksij Rempel
Signed-off-by: William Breathitt Gray
---
Documentation/ABI/testing/sysfs-bus-counter | 8 +++
drivers/counter/counter-chrdev.c| 23 +++
drivers/counter/counter-chrdev.h| 2 ++
drivers/counter/counter-sysfs.c | 25 +
4 files changed, 58 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-bus-counter
b/Documentation/ABI/testing/sysfs-bus-counter
index 847e96f19d19..f6cb2a8b08a7 100644
--- a/Documentation/ABI/testing/sysfs-bus-counter
+++ b/Documentation/ABI/testing/sysfs-bus-counter
@@ -212,6 +212,14 @@ Description:
both edges:
Any state transition.
+What: /sys/bus/counter/devices/counterX/events_queue_size
+KernelVersion: 5.13
+Contact: linux-...@vger.kernel.org
+Description:
+ Size of the Counter events queue in number of struct
+ counter_event data structures. The number of elements will be
+ rounded-up to a power of 2.
+
What: /sys/bus/counter/devices/counterX/name
KernelVersion:5.2
Contact: linux-...@vger.kernel.org
diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
index 16f02df7f73d..53eea894e13f 100644
--- a/drivers/counter/counter-chrdev.c
+++ b/drivers/counter/counter-chrdev.c
@@ -375,6 +375,29 @@ void counter_chrdev_remove(struct counter_device *const
counter)
cdev_del(>chrdev);
}
+int counter_chrdev_realloc_queue(struct counter_device *const counter,
+size_t queue_size)
+{
+ int err;
+ DECLARE_KFIFO_PTR(events, struct counter_event);
+ unsigned long flags;
+
+ /* Allocate new events queue */
+ err = kfifo_alloc(, queue_size, GFP_ATOMIC);
Is there any potential for losing events?
We take the events_list_lock down below so we're safe against missing an
event, but past events currently unread in the queue will be lost.
Shortening the size of the queue is inherently a destructive process if
we have more events in the current queue than can fit in the new queue.
Because we a liable to lose some events in such a case, I think it's
best to keep the behavior of this reallocation consistent and have it
provide a fresh empty queue every time, as opposed to sometimes dropping
events and sometimes not.
I also suspect an actual user would be setting the size of their queue
to the required amount before they begin watching events, rather than
adjusting it sporadically during a live operation.
Absolutely agree. As such I wonder if you are better off enforcing this
behaviour? If the cdev is open for reading, don't allow the fifo to be
resized.
Jonathan
I can't really think of a good reason not to, so let's enforce it: if
the cdev is open, then we'll return an EINVAL if the user attempts to
resize the queue.
What is a good way to check for this condition? Should I just call
kref_read() and see if it's greater than 1? For example, in
counter_chrdev_realloc_queue():
if (kref_read(>dev.kobj.kref) > 1)
return -EINVAL;
William Breathitt Gray
Wouldn't EBUSY make more sense?
Re: [PATCH v8 16/22] counter: Move counter enums to uapi header
On 2/12/21 6:13 AM, William Breathitt Gray wrote: This is in preparation for a subsequent patch implementing a character device interface for the Counter subsystem. Signed-off-by: William Breathitt Gray --- Reviewed-by: David Lechner
Re: [PATCH v8 13/22] counter: Internalize sysfs interface code
On 2/12/21 6:13 AM, William Breathitt Gray wrote: This is a reimplementation of the Generic Counter driver interface. There are no modifications to the Counter subsystem userspace interface, so existing userspace applications should continue to run seamlessly. The purpose of this patch is to internalize the sysfs interface code among the various counter drivers into a shared module. Counter drivers pass and take data natively (i.e. u8, u64, etc.) and the shared counter module handles the translation between the sysfs interface and the device drivers. This guarantees a standard userspace interface for all counter drivers, and helps generalize the Generic Counter driver ABI in order to support the Generic Counter chrdev interface (introduced in a subsequent patch) without significant changes to the existing counter drivers. Note, Counter device registration is the same as before: drivers populate a struct counter_device with components and callbacks, then pass the structure to the devm_counter_register function. However, what's different now is how the Counter subsystem code handles this registration internally. Whereas before callbacks would interact directly with sysfs data, this interaction is now abstracted and instead callbacks interact with native C data types. The counter_comp structure forms the basis for Counter extensions. The counter-sysfs.c file contains the code to parse through the counter_device structure and register the requested components and extensions. Attributes are created and populated based on type, with respective translation functions to handle the mapping between sysfs and the counter driver callbacks. The translation performed for each attribute is straightforward: the attribute type and data is parsed from the counter_attribute structure, the respective counter driver read/write callback is called, and sysfs I/O is handled before or after the driver read/write function is called. Cc: Syed Nayyar Waris Cc: Patrick Havelange Cc: Kamel Bouhara Cc: Fabrice Gasnier Cc: Maxime Coquelin Cc: Alexandre Torgue Cc: David Lechner Cc: Dan Carpenter Signed-off-by: William Breathitt Gray --- For TI eQEP bits... Reviewed-by: David Lechner Tested-by: David Lechner
Re: [PATCH v8 12/22] counter: Rename counter_count_function to counter_function
On 2/14/21 11:13 AM, Jonathan Cameron wrote: On Fri, 12 Feb 2021 21:13:36 +0900 William Breathitt Gray wrote: The phrase "Counter Count function" is verbose and unintentionally implies that function is a Count extension. This patch adjusts the Counter subsystem code to use the more direct "Counter function" phrase to make the intent of this code clearer. The phrase "Count action" is adjusted herein as well for the same reason. Cc: Syed Nayyar Waris Cc: Patrick Havelange Cc: Kamel Bouhara Cc: Fabrice Gasnier Cc: Maxime Coquelin Cc: Alexandre Torgue Cc: David Lechner Signed-off-by: William Breathitt Gray I agree this makes sense in the counter core code, but in the drivers it may not be quite so obvious we are talking about a counter function given some of the bits of hardware do a number of other things. I guess up to the various driver maintainers on whether they think the new meaning is clear enough! Jonathan TBH, I think "counter count function" makes more sense to me.
Re: [PATCH v8 11/22] counter: Rename counter_signal_value to counter_signal_level
On 2/12/21 6:13 AM, William Breathitt Gray wrote: Signal values will always be levels so let's be explicit it about it to make the intent of the code clear. Cc: Syed Nayyar Waris Cc: Kamel Bouhara Signed-off-by: William Breathitt Gray --- Reviewed-by: David Lechner
Re: [PATCH v8 10/22] counter: Standardize to ERANGE for limit exceeded errors
On 2/12/21 6:13 AM, William Breathitt Gray wrote: ERANGE is a semantically better error code to return when an argument value falls outside the supported limit range of a device. Cc: Syed Nayyar Waris Cc: Fabrice Gasnier Cc: Maxime Coquelin Cc: Alexandre Torgue Signed-off-by: William Breathitt Gray --- Reviewed-by: David Lechner (I agree with William's assessment that this use of ERANGE is consistent with other uses in the kernel.)
Re: [PATCH v8 09/22] counter: Return error code on invalid modes
On 2/12/21 6:13 AM, William Breathitt Gray wrote: Only a select set of modes (function, action, etc.) are valid for a given device configuration. This patch ensures that invalid modes result in a return -EINVAL. Such a situation should never occur in reality, but it's good to define a default switch cases for the sake of making the intent of the code clear. Cc: Syed Nayyar Waris Cc: Kamel Bouhara Cc: Fabrice Gasnier Cc: Maxime Coquelin Cc: Alexandre Torgue Cc: David Lechner Signed-off-by: William Breathitt Gray --- Reviewed-by: David Lechner (In response to Jonathan's comment, I think this is fine rather than adding more churn to change all of the breaks to returns - but will keep that in mind for future changes.)
Re: [PATCH v8 03/22] counter: 104-quad-8: Return error when invalid mode during ceiling_write
On 2/12/21 6:13 AM, William Breathitt Gray wrote:
The 104-QUAD-8 only has two count modes where a ceiling value makes
sense: Range Limit and Modulo-N. Outside of these two modes, setting a
ceiling value is an invalid operation -- so let's report it as such by
returning -EINVAL.
Fixes: fc069262261c ("counter: 104-quad-8: Add lock guards - generic interface")
Cc: Syed Nayyar Waris
Signed-off-by: William Breathitt Gray
---
drivers/counter/104-quad-8.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
index 9691f8612be8..f0608b21196a 100644
--- a/drivers/counter/104-quad-8.c
+++ b/drivers/counter/104-quad-8.c
@@ -714,13 +714,14 @@ static ssize_t quad8_count_ceiling_write(struct
counter_device *counter,
switch (priv->count_mode[count->id]) {
case 1:
case 3:
+ mutex_unlock(>lock);
quad8_preset_register_set(priv, count->id, ceiling);
- break;
+ return len;
}
mutex_unlock(>lock);
- return len;
+ return -EINVAL;
}
static ssize_t quad8_count_preset_enable_read(struct counter_device *counter,
What happens if the register is written to when in the wrong mode?
If it doesn't hurt anything, I would suggest always writing the register
instead so that users don't have to know worry about the fact that the mode
has to be set first.
Re: [PATCH v8 02/22] docs: counter: Fix spelling
On 2/12/21 6:13 AM, William Breathitt Gray wrote: "Miscellaneous" is the correct spelling. Signed-off-by: William Breathitt Gray --- Reviewed-by: David Lechner
Re: [PATCH v8 01/22] docs: counter: Consolidate Counter sysfs attributes documentation
On 2/12/21 6:13 AM, William Breathitt Gray wrote: Duplicate ABIs are not valid, so let's consolidate these sysfs attributes into the main sysfs-bus-counter documentation file. Cc: Patrick Havelange Signed-off-by: William Breathitt Gray --- Reviewed-by: David Lechner
Re: [PATCH v7 5/5] counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8
On 2/11/21 5:56 PM, William Breathitt Gray wrote: On Wed, Dec 30, 2020 at 11:36:45AM -0600, David Lechner wrote: On 12/25/20 6:15 PM, William Breathitt Gray wrote: diff --git a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 index eac32180c40d..0ecba24d43aa 100644 --- a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 +++ b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 @@ -1,3 +1,28 @@ +What: /sys/bus/counter/devices/counterX/countY/irq_trigger Do we really need this sysfs attribute? Shouldn't interrupts be configured _only_ by the chrdev interface? I think this attribute can go away because we can implicitly figure out the correct IRQ configuration from the struct counter_watch data when a user executes a COUNTER_ADD_WATCH_IOCTL ioctl command. However, I need some help deciding on an appropriate behavior for conflicting counter_watch configurations. Let me give some context first. The 104-QUAD-8 features 8 channels (essentially 8 independent physical counters on the device). Each channel can independently issue an event, but any particular channel can only be set to a single kind of event (COUNTER_EVENT_INDEX, COUNTER_EVENT_OVERFLOW, etc.). The purpose of the irq_trigger sysfs attribute I introduced in this patch is to allow the user to select the event configuration they want for a particular channel. We can theoretically figure this out implicitly from the struct counter_watch request, so this sysfs attribute may not be necessary. However, how do we handle the case where a user executes two COUNTER_ADD_WATCH_IOCTL ioctl commands for the same channel but with different event selections? I'm considering three possible behaviors: * Fail the second ioctl call; event selection of the first struct counter_watch takes precedence and thus second is incompatible. * Issue a dev_warn() indicating that the second struct counter_watch event selection will now be the event configuration for that channel. * Don't notify the user, just silently reconfigure for the second struct counter_watch event selection. I'm suspecting the first behavior I listed here (ioctl returning failed) is the most appropriate as a user is explicitly made known of this particular device's inability to support more than one type of event per channel. What do you think? I agree that it should return an error instead of adding the watch. I'm pretty sure that is how I implemented the TI eQEP driver already.
Re: [PATCH] remoteproc: pru: future-proof PRU ID matching
On 1/28/21 4:55 PM, Suman Anna wrote:
Hi David,
On 1/15/21 6:53 PM, Suman Anna wrote:
On 1/4/21 3:18 PM, David Lechner wrote:
static int pru_rproc_probe(struct platform_device *pdev)
@@ -825,20 +808,28 @@ static int pru_rproc_remove(struct platform_device *pdev)
static const struct pru_private_data pru_data = {
.type = PRU_TYPE_PRU,
+ .pru0_iram_offset = 0x4000,
+ .pru1_iram_offset = 0x8000,
The offsets for the PRU cores are actually 0x34000 and 0x38000 respectively from
the base of the PRUSS on non-Davinci SoCs.
If we were to use this static data approach, then we might as well continue to
use the current address masking logic with the appropriate masks for Davinci
(0x38000 and 0x3C000, not true offsets but as masks they would work). Davinci
PRUSS is the only one with its differences being the first PRUSS IP, and I would
prefer to keep the logic aligned to the IPs on all the recent SoCs on 3
different TI SoC families (OMAP, Keystone 2 and K3).
Let me know what you think.
I'm not too picky as long as it works. :-)
If keeping the static data to a minimum is really important, I suppose we could
introduce a new type = PRU_TYPE_PRU_V1 for these PRUSSs instead. It sounds like
this information might be useful elsewhere anyway.
Re: [PATCH 1/2] dt-bindings: soc: ti: ti,pruss: add ti,am1806-pruss
On 1/15/21 10:45 AM, Suman Anna wrote: + Sekhar and Bartosz Hi David, On 1/4/21 12:30 PM, David Lechner wrote: This adds a "ti,am1806-pruss" compatible type for the PRUSS found in TI AM18xx/OMAP-L138 SoCs. Signed-off-by: David Lechner --- Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml index 037c51b2f972..a6ed23fdbc00 100644 --- a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml +++ b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml @@ -61,6 +61,7 @@ properties: compatible: enum: + - ti,am1806-pruss # for AM18xx/OMAP-L138 SoC family Almost all the drivers for these SoCs use the prefix "ti,da850-xxx" for the compatibles. Can we switch to using those instead of ti,am1806? I wasn't sure which chips exactly are "DA850". If someone can tell me, I can look at the docs to see if they have a PRUSS.
Re: [PATCH 2/2] soc: ti: pruss: add support for AM18XX/OMAP-L138 PRUSS
On 1/15/21 6:52 PM, Suman Anna wrote:
Hi David,
On 1/4/21 12:30 PM, David Lechner wrote:
This adds support for the PRUSS found in AM18XX/OMAP-L138. This PRUSS
doesn't have a CFG register, so that is made optional as selected by
the device tree compatible string.
ARCH_DAVINCI is added in the Kconfig so that the driver can be selected
on that platform.
Signed-off-by: David Lechner
---
drivers/soc/ti/Kconfig | 2 +-
drivers/soc/ti/pruss.c | 76 --
2 files changed, 45 insertions(+), 33 deletions(-)
diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig
index 7e2fb1c16af1..7a692a21480a 100644
--- a/drivers/soc/ti/Kconfig
+++ b/drivers/soc/ti/Kconfig
@@ -85,7 +85,7 @@ config TI_K3_SOCINFO
config TI_PRUSS
tristate "TI PRU-ICSS Subsystem Platform drivers"
- depends on SOC_AM33XX || SOC_AM43XX || SOC_DRA7XX || ARCH_KEYSTONE ||
ARCH_K3
+ depends on ARCH_DAVINCI || SOC_AM33XX || SOC_AM43XX || SOC_DRA7XX ||
ARCH_KEYSTONE || ARCH_K3
select MFD_SYSCON
help
TI PRU-ICSS Subsystem platform specific support.
diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
index 5d6e7132a5c4..bfaf3ff74b01 100644
--- a/drivers/soc/ti/pruss.c
+++ b/drivers/soc/ti/pruss.c
@@ -24,10 +24,12 @@
* struct pruss_private_data - PRUSS driver private data
* @has_no_sharedram: flag to indicate the absence of PRUSS Shared Data RAM
* @has_core_mux_clock: flag to indicate the presence of PRUSS core clock
+ * @has_cfg: flag to indicate the presence of PRUSS CFG registers
I recommend to change this to a negative flag as the Davinci platforms are the
only ones that don't have CFG (being the very first SoCs with a PRUSS IP)
sub-module.
Negative flags hurt my brain. :-)
I was actually thinking about submitting a patch to convert
has_no_sharedram to a positive flag as well. But I understand
the sentiment of only setting the flag true for the odd case
rather than the usual case.
*/
struct pruss_private_data {
bool has_no_sharedram;
bool has_core_mux_clock;
+ bool has_cfg;
};
static void pruss_of_free_clk_provider(void *data)
@@ -239,42 +241,44 @@ static int pruss_probe(struct platform_device *pdev)
goto rpm_disable;
}
And use it here to skip all the cfg code parsing. All the below delta is just
for the additional indentation for the flag. If you don't like goto's in
non-error paths, then we can refactor the CFG parse code into a separate
function.
Refactoring to a separate function sounds good to me.
Re: [PATCH] clocksource: davinci: move pr_fmt() before the includes
On 1/11/21 8:08 AM, Bartosz Golaszewski wrote: From: Bartosz Golaszewski We no longer need to undef pr_fmt if we define our own before including any headers. Signed-off-by: Bartosz Golaszewski --- Acked-by: David Lechner
Re: [PATCH] irqchip: Simplify the TI_PRUSS_INTC Kconfig
On 1/8/21 10:29 AM, Suman Anna wrote: The TI PRUSS INTC irqchip driver handles the local interrupt controller which is a child device of it's parent PRUSS/ICSSG device. The driver was upstreamed in parallel with the PRUSS platform driver, and was configurable independently previously. The PRUSS interrupt controller is an integral part of the overall PRUSS software architecture, and is not useful at all by itself. Simplify the TI_PRUSS_INTC Kconfig dependencies by making it silent and selected automatically when the TI_PRUSS platform driver is enabled. Signed-off-by: Suman Anna --- Reviewed-by: David Lechner
Re: [PATCH] irqchip/irq-pruss-intc: implement set_type() callback
On 1/5/21 2:47 AM, Marc Zyngier wrote:
On 2021-01-04 18:36, David Lechner wrote:
This implements the irqchip set_type() callback for the TI PRUSS
interrupt controller. This is needed for cases where an event needs
to be active low.
According to the technical reference manual, the polarity should always
be set to high, however in practice, the polarity needs to be set low
for the McASP Tx/Rx system event in conjunction with soft UART PRU
firmware for TI AM18XX SoCs, otherwise it doesn't work.
I remember asking about this when I reviewed the patch series, and
was told that there was no need to handle anything *but* level high.
As a consequence, the DT binding doesn't have any way to express
the trigger configuration.
Now this? What is going to drive the configuration?
I made it work by setting IRQF_TRIGGER_LOW in devm_request_irq().
I can try to see if there is a way to change the (10 year old)
firmware instead.
Hopefully someone from TI can shed more light on the subject?
Signed-off-by: David Lechner
---
drivers/irqchip/irq-pruss-intc.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
index 5409016e6ca0..f882af8a7ded 100644
--- a/drivers/irqchip/irq-pruss-intc.c
+++ b/drivers/irqchip/irq-pruss-intc.c
@@ -334,6 +334,32 @@ static void pruss_intc_irq_unmask(struct irq_data *data)
pruss_intc_write_reg(intc, PRU_INTC_EISR, hwirq);
}
+static int pruss_intc_irq_set_type(struct irq_data *data, unsigned int type)
+{
+ struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
+ u32 reg, bit, val;
+
+ if (type & IRQ_TYPE_LEVEL_MASK) {
+ /* polarity register */
+ reg = PRU_INTC_SIPR(data->hwirq / 32);
+ bit = BIT(data->hwirq % 32);
+ val = pruss_intc_read_reg(intc, reg);
+
+ /*
+ * This check also ensures that IRQ_TYPE_DEFAULT will result
+ * in setting the level to high.
+ */
+ if (type & IRQ_TYPE_LEVEL_HIGH)
+ val |= bit;
+ else
+ val &= ~bit;
+
+ pruss_intc_write_reg(intc, reg, val);
RMW of a shared register without locking?
+ }
+
+ return 0;
What happens when this is passed an edge configuration? It should at
least return an error.
This is probably a moot point if we are not going to allow
changing the type, but there is another register for selecting
edge or level. But the manual says similarly says it should
always be set to level.
+}
+
static int pruss_intc_irq_reqres(struct irq_data *data)
{
if (!try_module_get(THIS_MODULE))
@@ -389,6 +415,7 @@ static struct irq_chip pruss_irqchip = {
.irq_ack = pruss_intc_irq_ack,
.irq_mask = pruss_intc_irq_mask,
.irq_unmask = pruss_intc_irq_unmask,
+ .irq_set_type = pruss_intc_irq_set_type,
.irq_request_resources = pruss_intc_irq_reqres,
.irq_release_resources = pruss_intc_irq_relres,
.irq_get_irqchip_state = pruss_intc_irq_get_irqchip_state,
Thanks,
M.
[PATCH] remoteproc: pru: future-proof PRU ID matching
Currently, to determine the ID (0 or 1) of a PRU core, the last 19 bits
of the physical address of the cores IRAM are compared to known values.
However, the PRUs on TI AM18XX have IRAM at 0x01c38000 and 0x01c3c000
respectively. The former conflicts with PRU1_IRAM_ADDR_MASK which could
cause PRU0 to be detected as PRU1. (The latter also conflicts with
TX_PRU1_IRAM_ADDR_MASK but it would still be correctly detected as
PRU1.)
This fixes the problem by moving the address matching offset values to
the device-specific data. This way the compatible string does half of
the work of narrowing down the addresses to two possibilities instead
of checking the address against all possible PRU types. This also lets
us narrow down the scope of the match from 19 bits to 16 bits for all
PRU types.
After this, the TI AM18XX PRUs will be able to be added without running
into the problems stated above.
We can also drop the local ret variable while touching this code.
Signed-off-by: David Lechner
---
drivers/remoteproc/pru_rproc.c | 49 ++
1 file changed, 20 insertions(+), 29 deletions(-)
diff --git a/drivers/remoteproc/pru_rproc.c b/drivers/remoteproc/pru_rproc.c
index 2667919d76b3..94ce48df2f48 100644
--- a/drivers/remoteproc/pru_rproc.c
+++ b/drivers/remoteproc/pru_rproc.c
@@ -46,15 +46,6 @@
#define PRU_DEBUG_GPREG(x) (0x + (x) * 4)
#define PRU_DEBUG_CT_REG(x)(0x0080 + (x) * 4)
-/* PRU/RTU/Tx_PRU Core IRAM address masks */
-#define PRU_IRAM_ADDR_MASK 0x3
-#define PRU0_IRAM_ADDR_MASK0x34000
-#define PRU1_IRAM_ADDR_MASK0x38000
-#define RTU0_IRAM_ADDR_MASK0x4000
-#define RTU1_IRAM_ADDR_MASK0x6000
-#define TX_PRU0_IRAM_ADDR_MASK 0xa000
-#define TX_PRU1_IRAM_ADDR_MASK 0xc000
-
/* PRU device addresses for various type of PRU RAMs */
#define PRU_IRAM_DA0 /* Instruction RAM */
#define PRU_PDRAM_DA 0 /* Primary Data RAM */
@@ -96,10 +87,14 @@ enum pru_type {
/**
* struct pru_private_data - device data for a PRU core
* @type: type of the PRU core (PRU, RTU, Tx_PRU)
+ * @pru0_iram_offset: used to identify PRU core 0
+ * @pru1_iram_offset: used to identify PRU core 1
* @is_k3: flag used to identify the need for special load handling
*/
struct pru_private_data {
enum pru_type type;
+ u16 pru0_iram_offset;
+ u16 pru1_iram_offset;
unsigned int is_k3 : 1;
};
@@ -693,33 +688,21 @@ static int pru_rproc_parse_fw(struct rproc *rproc, const
struct firmware *fw)
}
/*
- * Compute PRU id based on the IRAM addresses. The PRU IRAMs are
+ * Compute PRU id based on the last 16 bits of IRAM addresses. The PRU IRAMs
are
* always at a particular offset within the PRUSS address space.
*/
static int pru_rproc_set_id(struct pru_rproc *pru)
{
- int ret = 0;
-
- switch (pru->mem_regions[PRU_IOMEM_IRAM].pa & PRU_IRAM_ADDR_MASK) {
- case TX_PRU0_IRAM_ADDR_MASK:
- fallthrough;
- case RTU0_IRAM_ADDR_MASK:
- fallthrough;
- case PRU0_IRAM_ADDR_MASK:
+ u16 offset = pru->mem_regions[PRU_IOMEM_IRAM].pa;
+
+ if (offset == pru->data->pru0_iram_offset)
pru->id = 0;
- break;
- case TX_PRU1_IRAM_ADDR_MASK:
- fallthrough;
- case RTU1_IRAM_ADDR_MASK:
- fallthrough;
- case PRU1_IRAM_ADDR_MASK:
+ else if (offset == pru->data->pru1_iram_offset)
pru->id = 1;
- break;
- default:
- ret = -EINVAL;
- }
+ else
+ return -EINVAL;
- return ret;
+ return 0;
}
static int pru_rproc_probe(struct platform_device *pdev)
@@ -825,20 +808,28 @@ static int pru_rproc_remove(struct platform_device *pdev)
static const struct pru_private_data pru_data = {
.type = PRU_TYPE_PRU,
+ .pru0_iram_offset = 0x4000,
+ .pru1_iram_offset = 0x8000,
};
static const struct pru_private_data k3_pru_data = {
.type = PRU_TYPE_PRU,
+ .pru0_iram_offset = 0x4000,
+ .pru1_iram_offset = 0x8000,
.is_k3 = 1,
};
static const struct pru_private_data k3_rtu_data = {
.type = PRU_TYPE_RTU,
+ .pru0_iram_offset = 0x4000,
+ .pru1_iram_offset = 0x6000,
.is_k3 = 1,
};
static const struct pru_private_data k3_tx_pru_data = {
.type = PRU_TYPE_TX_PRU,
+ .pru0_iram_offset = 0xa000,
+ .pru1_iram_offset = 0xc000,
.is_k3 = 1,
};
--
2.25.1
Re: [PATCH v2 2/5] remoteproc: pru: Add APIs to get and put the PRU cores
@@ -706,14 +824,14 @@ static int pru_rproc_set_id(struct pru_rproc *pru) case RTU0_IRAM_ADDR_MASK: fallthrough; case PRU0_IRAM_ADDR_MASK: - pru->id = 0; + pru->id = PRUSS_PRU0; break; case TX_PRU1_IRAM_ADDR_MASK: fallthrough; case RTU1_IRAM_ADDR_MASK: fallthrough; case PRU1_IRAM_ADDR_MASK: - pru->id = 1; + pru->id = PRUSS_PRU1; break; default: ret = -EINVAL; There is a similar opportunity for using PRUSS_PRU1 in pru_d_da_to_va()
Re: [PATCH v2 0/5] Introduce PRU remoteproc consumer API
Please see the individual patches for exact changes in each patch, following is
the only change from v1:
- Change the 'prus' property name to 'ti,prus' as suggested by Rob Herring,
which influences patch #1 and patch #2
It looks like "soc: ti: pruss: Add pruss_{request, release}_mem_region() API"
was also dropped in v2. Was this intentional?
Re: [PATCH 1/5] dt-bindings: remoteproc: Add PRU consumer bindings
Also, I think you can get rid of 'ti,pruss-gp-mux-sel'. Can't it just be an arg cell in 'ti,prus' entries? Rob +1 for using cells instead of a separate property. FYI, we will have a similar issue with the PRUSSEVTSEL signal for the interrupt controller on the AM18XX. I am still of the opinion (described in more detail at [1]) that using a cell for this makes for both better device tree bindings and easier driver implementation. So I am interested to see what the resolution is here. [1]: https://patchwork.kernel.org/project/linux-arm-kernel/patch/20190708035243.12170-5-s-a...@ti.com/
[PATCH] irqchip/irq-pruss-intc: implement set_type() callback
This implements the irqchip set_type() callback for the TI PRUSS
interrupt controller. This is needed for cases where an event needs
to be active low.
According to the technical reference manual, the polarity should always
be set to high, however in practice, the polarity needs to be set low
for the McASP Tx/Rx system event in conjunction with soft UART PRU
firmware for TI AM18XX SoCs, otherwise it doesn't work.
Signed-off-by: David Lechner
---
drivers/irqchip/irq-pruss-intc.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
index 5409016e6ca0..f882af8a7ded 100644
--- a/drivers/irqchip/irq-pruss-intc.c
+++ b/drivers/irqchip/irq-pruss-intc.c
@@ -334,6 +334,32 @@ static void pruss_intc_irq_unmask(struct irq_data *data)
pruss_intc_write_reg(intc, PRU_INTC_EISR, hwirq);
}
+static int pruss_intc_irq_set_type(struct irq_data *data, unsigned int type)
+{
+ struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
+ u32 reg, bit, val;
+
+ if (type & IRQ_TYPE_LEVEL_MASK) {
+ /* polarity register */
+ reg = PRU_INTC_SIPR(data->hwirq / 32);
+ bit = BIT(data->hwirq % 32);
+ val = pruss_intc_read_reg(intc, reg);
+
+ /*
+* This check also ensures that IRQ_TYPE_DEFAULT will result
+* in setting the level to high.
+*/
+ if (type & IRQ_TYPE_LEVEL_HIGH)
+ val |= bit;
+ else
+ val &= ~bit;
+
+ pruss_intc_write_reg(intc, reg, val);
+ }
+
+ return 0;
+}
+
static int pruss_intc_irq_reqres(struct irq_data *data)
{
if (!try_module_get(THIS_MODULE))
@@ -389,6 +415,7 @@ static struct irq_chip pruss_irqchip = {
.irq_ack= pruss_intc_irq_ack,
.irq_mask = pruss_intc_irq_mask,
.irq_unmask = pruss_intc_irq_unmask,
+ .irq_set_type = pruss_intc_irq_set_type,
.irq_request_resources = pruss_intc_irq_reqres,
.irq_release_resources = pruss_intc_irq_relres,
.irq_get_irqchip_state = pruss_intc_irq_get_irqchip_state,
--
2.25.1
[PATCH 2/2] soc: ti: pruss: add support for AM18XX/OMAP-L138 PRUSS
This adds support for the PRUSS found in AM18XX/OMAP-L138. This PRUSS
doesn't have a CFG register, so that is made optional as selected by
the device tree compatible string.
ARCH_DAVINCI is added in the Kconfig so that the driver can be selected
on that platform.
Signed-off-by: David Lechner
---
drivers/soc/ti/Kconfig | 2 +-
drivers/soc/ti/pruss.c | 76 --
2 files changed, 45 insertions(+), 33 deletions(-)
diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig
index 7e2fb1c16af1..7a692a21480a 100644
--- a/drivers/soc/ti/Kconfig
+++ b/drivers/soc/ti/Kconfig
@@ -85,7 +85,7 @@ config TI_K3_SOCINFO
config TI_PRUSS
tristate "TI PRU-ICSS Subsystem Platform drivers"
- depends on SOC_AM33XX || SOC_AM43XX || SOC_DRA7XX || ARCH_KEYSTONE ||
ARCH_K3
+ depends on ARCH_DAVINCI || SOC_AM33XX || SOC_AM43XX || SOC_DRA7XX ||
ARCH_KEYSTONE || ARCH_K3
select MFD_SYSCON
help
TI PRU-ICSS Subsystem platform specific support.
diff --git a/drivers/soc/ti/pruss.c b/drivers/soc/ti/pruss.c
index 5d6e7132a5c4..bfaf3ff74b01 100644
--- a/drivers/soc/ti/pruss.c
+++ b/drivers/soc/ti/pruss.c
@@ -24,10 +24,12 @@
* struct pruss_private_data - PRUSS driver private data
* @has_no_sharedram: flag to indicate the absence of PRUSS Shared Data RAM
* @has_core_mux_clock: flag to indicate the presence of PRUSS core clock
+ * @has_cfg: flag to indicate the presence of PRUSS CFG registers
*/
struct pruss_private_data {
bool has_no_sharedram;
bool has_core_mux_clock;
+ bool has_cfg;
};
static void pruss_of_free_clk_provider(void *data)
@@ -239,42 +241,44 @@ static int pruss_probe(struct platform_device *pdev)
goto rpm_disable;
}
- child = of_get_child_by_name(np, "cfg");
- if (!child) {
- dev_err(dev, "%pOF is missing its 'cfg' node\n", child);
- ret = -ENODEV;
- goto rpm_put;
- }
+ if (data->has_cfg) {
+ child = of_get_child_by_name(np, "cfg");
+ if (!child) {
+ dev_err(dev, "%pOF is missing its 'cfg' node\n", child);
+ ret = -ENODEV;
+ goto rpm_put;
+ }
- if (of_address_to_resource(child, 0, )) {
- ret = -ENOMEM;
- goto node_put;
- }
+ if (of_address_to_resource(child, 0, )) {
+ ret = -ENOMEM;
+ goto node_put;
+ }
- pruss->cfg_base = devm_ioremap(dev, res.start, resource_size());
- if (!pruss->cfg_base) {
- ret = -ENOMEM;
- goto node_put;
- }
+ pruss->cfg_base = devm_ioremap(dev, res.start,
resource_size());
+ if (!pruss->cfg_base) {
+ ret = -ENOMEM;
+ goto node_put;
+ }
- regmap_conf.name = kasprintf(GFP_KERNEL, "%pOFn@%llx", child,
-(u64)res.start);
- regmap_conf.max_register = resource_size() - 4;
-
- pruss->cfg_regmap = devm_regmap_init_mmio(dev, pruss->cfg_base,
- _conf);
- kfree(regmap_conf.name);
- if (IS_ERR(pruss->cfg_regmap)) {
- dev_err(dev, "regmap_init_mmio failed for cfg, ret = %ld\n",
- PTR_ERR(pruss->cfg_regmap));
- ret = PTR_ERR(pruss->cfg_regmap);
- goto node_put;
- }
+ regmap_conf.name = kasprintf(GFP_KERNEL, "%pOFn@%llx", child,
+(u64)res.start);
+ regmap_conf.max_register = resource_size() - 4;
+
+ pruss->cfg_regmap = devm_regmap_init_mmio(dev, pruss->cfg_base,
+ _conf);
+ kfree(regmap_conf.name);
+ if (IS_ERR(pruss->cfg_regmap)) {
+ dev_err(dev, "regmap_init_mmio failed for cfg, ret =
%ld\n",
+ PTR_ERR(pruss->cfg_regmap));
+ ret = PTR_ERR(pruss->cfg_regmap);
+ goto node_put;
+ }
- ret = pruss_clk_init(pruss, child);
- if (ret) {
- dev_err(dev, "failed to setup coreclk-mux\n");
- goto node_put;
+ ret = pruss_clk_init(pruss, child);
+ if (ret) {
+ dev_err(dev, "failed to setup coreclk-mux\n");
+ goto node_put;
+ }
}
ret = devm_of_platform_populate(dev);
@@ -309,19 +313,27 @@ static int pruss_remove(struct platform_device *pdev)
}
/* instance-specific driver private data */
+stat
[PATCH 0/2] Add support for TI AM18XX/OMAP-L138 PRUSS
This is the first step for adding support for the PRUSS on TI AM18XX/OMAP-L138 SoCs. This series adds support in the top-level PRUSS driver. (Patches for the interrupt controller and individual PRUs are independent of this change and will be submitted separately.) David Lechner (2): dt-bindings: soc: ti: ti,pruss: add ti,am1806-pruss soc: ti: pruss: add support for AM18XX/OMAP-L138 PRUSS .../devicetree/bindings/soc/ti/ti,pruss.yaml | 2 + drivers/soc/ti/Kconfig| 2 +- drivers/soc/ti/pruss.c| 76 +++ 3 files changed, 47 insertions(+), 33 deletions(-) -- 2.25.1
[PATCH 1/2] dt-bindings: soc: ti: ti,pruss: add ti,am1806-pruss
This adds a "ti,am1806-pruss" compatible type for the PRUSS found in TI AM18xx/OMAP-L138 SoCs. Signed-off-by: David Lechner --- Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml index 037c51b2f972..a6ed23fdbc00 100644 --- a/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml +++ b/Documentation/devicetree/bindings/soc/ti/ti,pruss.yaml @@ -61,6 +61,7 @@ properties: compatible: enum: + - ti,am1806-pruss # for AM18xx/OMAP-L138 SoC family - ti,am3356-pruss # for AM335x SoC family - ti,am4376-pruss0 # for AM437x SoC family and PRUSS unit 0 - ti,am4376-pruss1 # for AM437x SoC family and PRUSS unit 1 @@ -321,6 +322,7 @@ if: compatible: contains: enum: + - ti,am1806-pruss - ti,k2g-pruss - ti,am654-icssg - ti,j721e-icssg -- 2.25.1
[PATCH] ARM: dts: da850: add MMD SDIO interrupts
This adds the MMC SDIO interrupts to the MMC nodes in the device tree
for TI DA850/AM18XX/OMAP-L138.
The missing interrupts were causing the following error message to be
printed:
davinci_mmc 1c4.mmc: IRQ index 1 not found
Signed-off-by: David Lechner
---
arch/arm/boot/dts/da850.dtsi | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 7cf31b6e48b7..d2c609e4da5b 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -537,7 +537,7 @@ mmc0: mmc@4 {
reg = <0x4 0x1000>;
cap-sd-highspeed;
cap-mmc-highspeed;
- interrupts = <16>;
+ interrupts = <16>, <17>;
dmas = < 16 0>, < 17 0>;
dma-names = "rx", "tx";
clocks = < 5>;
@@ -567,7 +567,7 @@ mmc1: mmc@21b000 {
reg = <0x21b000 0x1000>;
cap-sd-highspeed;
cap-mmc-highspeed;
- interrupts = <72>;
+ interrupts = <72>, <73>;
dmas = < 28 0>, < 29 0>;
dma-names = "rx", "tx";
clocks = < 18>;
--
2.25.1
Re: [PATCH v7 0/5] Introduce the Counter character device interface
On 12/25/20 6:15 PM, William Breathitt Gray wrote: Changes in v7: - Implemented u32 enums; enum types can now be used directly for callbacks and values - Fixed refcount underflow bug - Refactored all err check to check for err < 0; this should help prevent future oversights on valid positive return valids - Use mutex instead of raw_spin_lock in counter_chrdev_read(); kifo_to_user() can now safely sleep - Renamed COUNTER_COMPONENT_DUMMY to COUNTER_COMPONENT_NONE to make purpose more obvious - Introduced a watch_validate() callback - Consolidated the functionality to clear the watches to the counter_clear_watches() function - Reimplemented counter_push_event() as a void function; on error, errno is returned via struct counter_event so that it can be handled by userspace (because interrupt handlers can't do much for this) - Renamed the events_config() callback to events_configure(); "events_config" could be confused as a read callback when this is actually intended to configure the device for the requested events - Reimplemented 104-QUAD-8 driver to use events_configure() and watch_validate() callbacks; irq_trigger_enable sysfs attribute removed because events_configure() now serves this purpose The changes for this revision were much simpler compared to the previous revisions. I don't have any further questions for this patchset, so it's really just a search for possible bugs or regressions now. Please test and verify this patchset on your systems, and ACK appropriately. I'll wait for the next round to give my Reviewed-By, Tested-By but I've rebased my WIP TI eQEP changes[1] on this and I haven't ran into any problems yet. [1]: https://github.com/dlech/linux/commits/bone-counter
Re: [PATCH v7 1/5] counter: Internalize sysfs interface code
On 12/25/20 6:15 PM, William Breathitt Gray wrote:
diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
index a60aee1a1a29..6c058b93dc98 100644
--- a/drivers/counter/ti-eqep.c
+++ b/drivers/counter/ti-eqep.c
-static ssize_t ti_eqep_position_floor_write(struct counter_device *counter,
- struct counter_count *count,
- void *ext_priv, const char *buf,
- size_t len)
+static int ti_eqep_position_floor_write(struct counter_device *counter,
+ struct counter_count *count, u64 floor)
{
struct ti_eqep_cnt *priv = counter->priv;
- int err;
- u32 res;
- err = kstrtouint(buf, 0, );
- if (err < 0)
- return err;
+ if (floor != (u32)floor)
+ return -ERANGE;
- regmap_write(priv->regmap32, QPOSINIT, res);
+ regmap_write(priv->regmap32, QPOSINIT, floor);
- return len;
+ return 0;
}
This will conflict with 2ba7b50893de "counter:ti-eqep: remove floor"
(in Jonathan's fixes-togreg branch) which removes these functions.
Re: [PATCH v7 3/5] counter: Add character device interface
On 12/25/20 6:15 PM, William Breathitt Gray wrote:
diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
new file mode 100644
index ..7585dc9db19d
--- /dev/null
+++ b/include/uapi/linux/counter.h
@@ -0,0 +1,123 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Userspace ABI for Counter character devices
+ * Copyright (C) 2020 William Breathitt Gray
+ */
+#ifndef _UAPI_COUNTER_H_
+#define _UAPI_COUNTER_H_
+
+#include
+#include
+
+/* Component type definitions */
+enum counter_component_type {
+ COUNTER_COMPONENT_NONE,
+ COUNTER_COMPONENT_SIGNAL,
+ COUNTER_COMPONENT_COUNT,
+ COUNTER_COMPONENT_FUNCTION,
+ COUNTER_COMPONENT_SYNAPSE_ACTION,
+ COUNTER_COMPONENT_EXTENSION,
+};
+
+/* Component scope definitions */
+enum counter_scope {
Do we need COUNTER_SCOPE_NONE to go with COUNTER_COMPONENT_NONE?
+ COUNTER_SCOPE_DEVICE,
+ COUNTER_SCOPE_SIGNAL,
+ COUNTER_SCOPE_COUNT,
+};
+
+/**
+ * struct counter_component - Counter component identification
+ * @type: component type (Count, extension, etc.)
Instead of "Count, extension, etc.", it could be more helpful
to say one of enum counter_component_type.
+ * @scope: component scope (Device, Count, or Signal)
Same here. @scope must be one of enum counter_scope.
+ * @parent: parent component identification number
+ * @id: component identification number
It could be helpful to say that these id numbers match
the X/Y/Z in the sysfs paths as described in the sysfs
ABI.
+ */
+struct counter_component {
+ __u8 type;
+ __u8 scope;
+ __u8 parent;
+ __u8 id;
+};
+
+/* Event type definitions */
+enum counter_event_type {
+ COUNTER_EVENT_OVERFLOW,
+ COUNTER_EVENT_UNDERFLOW,
+ COUNTER_EVENT_OVERFLOW_UNDERFLOW,
+ COUNTER_EVENT_THRESHOLD,
+ COUNTER_EVENT_INDEX,
+};
+
+/**
+ * struct counter_watch - Counter component watch configuration
+ * @component: component to watch when event triggers
+ * @event: event that triggers
It would be helpful to say that @event must be one of
enum counter_event_type.
+ * @channel: event channel
It would be useful to say that @channel should be 0 unless
a component has more than one event of the same type.
+ */
+struct counter_watch {
+ struct counter_component component;
+ __u8 event;
+ __u8 channel;
+};
+
+/* ioctl commands */
+#define COUNTER_CLEAR_WATCHES_IOCTL _IO(0x3E, 0x00)
+#define COUNTER_ADD_WATCH_IOCTL _IOW(0x3E, 0x01, struct counter_watch)
+#define COUNTER_LOAD_WATCHES_IOCTL _IO(0x3E, 0x02)
+
+/**
+ * struct counter_event - Counter event data
+ * @timestamp: best estimate of time of event occurrence, in nanoseconds
+ * @value: component value
+ * @watch: component watch configuration
+ * @errno: system error number
+ */
+struct counter_event {
+ __aligned_u64 timestamp;
+ __aligned_u64 value;
+ struct counter_watch watch;
+ __u8 errno;
There are error codes larger than 255. Probably better
make this __u32.
+};
+
+/* Count direction values */
+enum counter_count_direction {
+ COUNTER_COUNT_DIRECTION_FORWARD,
+ COUNTER_COUNT_DIRECTION_BACKWARD,
+};
+
+/* Count mode values */
+enum counter_count_mode {
+ COUNTER_COUNT_MODE_NORMAL,
+ COUNTER_COUNT_MODE_RANGE_LIMIT,
+ COUNTER_COUNT_MODE_NON_RECYCLE,
+ COUNTER_COUNT_MODE_MODULO_N,
+};
+
+/* Count function values */
+enum counter_function {
+ COUNTER_FUNCTION_INCREASE,
+ COUNTER_FUNCTION_DECREASE,
+ COUNTER_FUNCTION_PULSE_DIRECTION,
+ COUNTER_FUNCTION_QUADRATURE_X1_A,
+ COUNTER_FUNCTION_QUADRATURE_X1_B,
+ COUNTER_FUNCTION_QUADRATURE_X2_A,
+ COUNTER_FUNCTION_QUADRATURE_X2_B,
+ COUNTER_FUNCTION_QUADRATURE_X4,
+};
+
+/* Signal values */
+enum counter_signal_level {
+ COUNTER_SIGNAL_LEVEL_LOW,
+ COUNTER_SIGNAL_LEVEL_HIGH,
+};
+
+/* Action mode values */
+enum counter_synapse_action {
+ COUNTER_SYNAPSE_ACTION_NONE,
+ COUNTER_SYNAPSE_ACTION_RISING_EDGE,
+ COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
+ COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
+};
+
+#endif /* _UAPI_COUNTER_H_ */
Re: [PATCH v7 4/5] docs: counter: Document character device interface
On 12/25/20 6:15 PM, William Breathitt Gray wrote: +Userspace +- +Userspace applications can configure Counter events via ioctl operations +on the Counter character device node. There following ioctl codes are +supported and provided by the `linux/counter.h` userspace header file: + +* COUNTER_CLEAR_WATCHES_IOCTL: + Clear all Counter watches from all events + +* COUNTER_ADD_WATCH_IOCTL: + Add a Counter watch for the specified event + +* COUNTER_LOAD_WATCHES_IOCTL: + Activates the Counter watches added earlier Would it make more sense to call the last and first ones COUNTER_ENABLE_EVENTS_IOCTL and COUNTER_DISABLE_EVENTS_IOCTL? In any case, more explanation would be helpful. * COUNTER_ADD_WATCH_IOCTL: Queues a Counter watch for the specified event. The queued watches will not be applied until COUNTER_ENABLE_EVENTS_IOCTL is called. * COUNTER_ENABLE_EVENTS_IOCTL: Enables monitoring the events specified by the Counter watches that were queued by COUNTER_ADD_WATCH_IOCTL. If events are already enabled, the new set of watches replaces the old one. Calling this ioctl also has the effect of clearing the queue of watches added by COUNTER_ADD_WATCH_IOCTL. * COUNTER_DISABLE_EVENTS_IOCTL: Stops monitoring the previously enabled events. + +To configure events to gather Counter data, users first populate a +`struct counter_watch` with the relevant event id, event channel id, and +the information for the desired Counter component from which to read, +and then pass it via the `COUNTER_ADD_WATCH_IOCTL` ioctl command. for restructured text, two backticks are needed for ``code`` formatting
Re: [PATCH v7 5/5] counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8
On 12/25/20 6:15 PM, William Breathitt Gray wrote: diff --git a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 index eac32180c40d..0ecba24d43aa 100644 --- a/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 +++ b/Documentation/ABI/testing/sysfs-bus-counter-104-quad-8 @@ -1,3 +1,28 @@ +What: /sys/bus/counter/devices/counterX/countY/irq_trigger Do we really need this sysfs attribute? Shouldn't interrupts be configured _only_ by the chrdev interface?
Re: [PATCH v6 1/5] counter: Internalize sysfs interface code
On 12/20/20 4:11 PM, William Breathitt Gray wrote:
On Sun, Dec 13, 2020 at 05:15:00PM -0600, David Lechner wrote:
On 11/22/20 2:29 PM, William Breathitt Gray wrote:
14 files changed, 1806 insertions(+), 2546 deletions(-)
It would be really nice if we could break this down into smaller
pieces and start getting it merged. It is really tough to keep
reviewing this much code in one patch over and over again.
Yes, this is a pretty massive patch. I could break this across the
individual files affected to make it simpler to review, but in the end
all those patches would need to end up squashed together before merge
again (for the sake of git bisect), so the effort feels somewhat moot.
Luckily, I don't think there will be much change in the next revision
since it's looking like it'll mainly be a few bug fixes; hopefully this
coming version 7 will be the final revision before merge.
Here are some initial findings from testing:
+static void counter_device_release(struct device *dev)
+{
+ struct counter_device *const counter = dev_get_drvdata(dev);
+
+ counter_chrdev_remove(counter);
+ ida_simple_remove(_ida, counter->id);
+}
I got the following error after `modprobe -r ti-eqep`:
[ 1186.045766] [ cut here ]
[ 1186.050647] WARNING: CPU: 0 PID: 2625 at lib/refcount.c:28
counter_device_release+0x10/0x24 [counter]
[ 1186.059976] refcount_t: underflow; use-after-free.
[ 1186.064790] Modules linked in: aes_arm_bs(+) crypto_simd cryptd ccm
usb_f_mass_storage usb_f_acm u_serial usb_f_ecm rfcomm usb_f_rndis u_ether
libcomposite aes_arm aes_generic cmac bnep wl18xx wlcore mac80211 libarc4
sha256_generic libsha256 sha256_arm cfg80211 ti_am335x_adc kfifo_buf
omap_aes_driver omap_crypto omap_sham crypto_engine pm33xx ti_emif_sram
hci_uart omap_rng btbcm rng_core ti_eqep(-) counter bluetooth c_can_platform
c_can ecdh_generic bmp280_spi ecc can_dev libaes bmp280_i2c bmp280 industrialio
omap_mailbox musb_dsps wlcore_sdio musb_hdrc udc_core usbcore wkup_m3_ipc at24
omap_wdt phy_am335x watchdog phy_am335x_control ti_am335x_tscadc phy_generic
wkup_m3_rproc usb_common cppi41 rtc_omap leds_gpio led_class cpufreq_dt
pwm_tiehrpwm autofs4
[ 1186.132376] CPU: 0 PID: 2625 Comm: modprobe Not tainted
5.10.0-rc7bone-counter+ #23
[ 1186.140070] Hardware name: Generic AM33XX (Flattened Device Tree)
[ 1186.146225] [] (unwind_backtrace) from []
(show_stack+0x10/0x14)
[ 1186.154017] [] (show_stack) from []
(dump_stack+0xc4/0xe4)
[ 1186.161285] [] (dump_stack) from [] (__warn+0xd8/0x100)
[ 1186.168284] [] (__warn) from []
(warn_slowpath_fmt+0x94/0xbc)
[ 1186.175814] [] (warn_slowpath_fmt) from []
(counter_device_release+0x10/0x24 [counter])
[ 1186.185632] [] (counter_device_release [counter]) from
[] (device_release+0x30/0xa4)
[ 1186.195163] [] (device_release) from []
(kobject_put+0x94/0x104)
[ 1186.202944] [] (kobject_put) from []
(kobject_put+0x94/0x104)
[ 1186.210472] [] (kobject_put) from []
(ti_eqep_remove+0x10/0x30 [ti_eqep])
[ 1186.219047] [] (ti_eqep_remove [ti_eqep]) from []
(platform_drv_remove+0x24/0x3c)
[ 1186.228313] [] (platform_drv_remove) from []
(device_release_driver_internal+0xfc/0x1d0)
[ 1186.238187] [] (device_release_driver_internal) from []
(driver_detach+0x58/0xa8)
[ 1186.247456] [] (driver_detach) from []
(bus_remove_driver+0x4c/0xa0)
[ 1186.255594] [] (bus_remove_driver) from []
(sys_delete_module+0x180/0x264)
[ 1186.264250] [] (sys_delete_module) from []
(ret_fast_syscall+0x0/0x54)
[ 1186.272551] Exception stack(0xd247ffa8 to 0xd247fff0)
[ 1186.277629] ffa0: 004fb478 004fb478 004fb4b4 0800
b3bfcf00
[ 1186.285847] ffc0: 004fb478 004fb478 004fb478 0081 be974900
be974a55 004fb478
[ 1186.294062] ffe0: 004f8f5c be97352c 004ddd97 b6d11d68
[ 1186.299253] ---[ end trace e1c61dea091f1078 ]---
I noticed that I'm calling counter_chrdev_remove() twice: once in
counter_unregister(), and again in counter_device_release(). I suspect
this is what's causing the refcount to underflow. I'll test and verify
that this is the culprit.
In fact, I don't think I need to define a counter_device_release()
callback at all, would I? These cleanup function calls could be moved to
counter_unregister() instead.
As long as a user program keeps a chrdev open, it holds a reference to
the device, so I think it needs to be the other way around. (Unless it
is impossible to call counter_unregister() before all references have
been released - but I don't think that is the case - not 100% sure.)
Re: [PATCH v6 0/5] Introduce the Counter character device interface
On 12/20/20 3:44 PM, William Breathitt Gray wrote:
On Sun, Dec 13, 2020 at 05:15:14PM -0600, David Lechner wrote:
On 11/22/20 2:29 PM, William Breathitt Gray wrote:
1. Should standard Counter component data types be defined as u8 or u32?
Many standard Counter component types such COUNTER_COMP_SIGNAL_LEVEL
have standard values defined (e.g. COUNTER_SIGNAL_LEVEL_LOW and
COUNTER_SIGNAL_LEVEL_HIGH). These values are currently handled by the
Counter subsystem code as u8 data types.
If u32 is used for these values instead, C enum structures could be
used by driver authors to implicitly cast these values via the driver
callback parameters.
This question is primarily addressed to David Lechner. I'm somewhat
confused about how this setup would look in device drivers. I've gone
ahead and refactored the code to support u32 enums, and pushed it to
a separate branch on my repository called counter_chrdev_v6_u32_enum:
https://gitlab.com/vilhelmgray/iio/-/tree/counter_chrdev_v6_u32_enum
Please check it out and let me know what you think. Is this the
support you had in mind? I'm curious to see an example of how would
your driver callback functions would look in this case. If everything
works out fine, then I'll submit this branch as v7 of this patchset.
I haven't had time to look at this in depth, but just superficially looking
at it, it is mostly there. The driver callback would just use the enum type
in place of u32. For example:
static int ti_eqep_function_write(struct counter_device *counter,
struct counter_count *count,
enum counter_function function)
and the COUNTER_FUNCTION_* constants would be defined as:
enum counter_function {
COUNTER_FUNCTION_INCREASE,
...
};
instead of using #define macros.
One advantage I see to using u8, at least in the user API data structures,
is that it increases the number of events that fit in the kfifo buffer by
a significant factor.
And that is not to say that we couldn't do both: have the user API structs
use u8 for enum values and still use u32/strong enum types internally in
the callback functions.
I'm including David Laight because he initially opposed enums in favor
of fixed size types when we discussed this in an earlier revision:
https://lkml.org/lkml/2020/5/3/159
However, there have been significant changes to this patchset so the
context now is different than those earlier discussions (i.e. we're no
longer discussing ioctl calls).
I think reimplementing these constants as enums as described could work.
If we do so, should the enum constants be given specific values? For
example:
enum counter_function {
COUNTER_FUNCTION_INCREASE = 0,
COUNTER_FUNCTION_DECREASE = 1,
...
};
I would say no on the explicit values since they don't have
any significant meaning.
Re: [PATCH] counter:ti-eqep: remove floor
On 12/14/20 5:46 AM, William Breathitt Gray wrote:
On Sun, Dec 13, 2020 at 06:09:27PM -0600, David Lechner wrote:
The hardware doesn't support this. QPOSINIT is an initialization value
that is triggered by other things. When the counter overflows, it
always wraps around to zero.
Fixes: f213729f6796 "counter: new TI eQEP driver"
Signed-off-by: David Lechner
Is the QPOSINIT preprocessor define needed at all anymore, or should it also be
removed?
There are already many more defines for registers that are not
used, so I didn't remove it.
Acked-by: William Breathitt Gray
---
drivers/counter/ti-eqep.c | 35 ---
1 file changed, 35 deletions(-)
diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
index a60aee1a1a29..65df9ef5b5bc 100644
--- a/drivers/counter/ti-eqep.c
+++ b/drivers/counter/ti-eqep.c
@@ -235,36 +235,6 @@ static ssize_t ti_eqep_position_ceiling_write(struct
counter_device *counter,
return len;
}
-static ssize_t ti_eqep_position_floor_read(struct counter_device *counter,
- struct counter_count *count,
- void *ext_priv, char *buf)
-{
- struct ti_eqep_cnt *priv = counter->priv;
- u32 qposinit;
-
- regmap_read(priv->regmap32, QPOSINIT, );
-
- return sprintf(buf, "%u\n", qposinit);
-}
-
-static ssize_t ti_eqep_position_floor_write(struct counter_device *counter,
- struct counter_count *count,
- void *ext_priv, const char *buf,
- size_t len)
-{
- struct ti_eqep_cnt *priv = counter->priv;
- int err;
- u32 res;
-
- err = kstrtouint(buf, 0, );
- if (err < 0)
- return err;
-
- regmap_write(priv->regmap32, QPOSINIT, res);
-
- return len;
-}
-
static ssize_t ti_eqep_position_enable_read(struct counter_device *counter,
struct counter_count *count,
void *ext_priv, char *buf)
@@ -301,11 +271,6 @@ static struct counter_count_ext ti_eqep_position_ext[] = {
.read = ti_eqep_position_ceiling_read,
.write = ti_eqep_position_ceiling_write,
},
- {
- .name = "floor",
- .read = ti_eqep_position_floor_read,
- .write = ti_eqep_position_floor_write,
- },
{
.name = "enable",
.read = ti_eqep_position_enable_read,
--
2.25.1
[PATCH] counter:ti-eqep: remove floor
The hardware doesn't support this. QPOSINIT is an initialization value
that is triggered by other things. When the counter overflows, it
always wraps around to zero.
Fixes: f213729f6796 "counter: new TI eQEP driver"
Signed-off-by: David Lechner
---
drivers/counter/ti-eqep.c | 35 ---
1 file changed, 35 deletions(-)
diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
index a60aee1a1a29..65df9ef5b5bc 100644
--- a/drivers/counter/ti-eqep.c
+++ b/drivers/counter/ti-eqep.c
@@ -235,36 +235,6 @@ static ssize_t ti_eqep_position_ceiling_write(struct
counter_device *counter,
return len;
}
-static ssize_t ti_eqep_position_floor_read(struct counter_device *counter,
- struct counter_count *count,
- void *ext_priv, char *buf)
-{
- struct ti_eqep_cnt *priv = counter->priv;
- u32 qposinit;
-
- regmap_read(priv->regmap32, QPOSINIT, );
-
- return sprintf(buf, "%u\n", qposinit);
-}
-
-static ssize_t ti_eqep_position_floor_write(struct counter_device *counter,
- struct counter_count *count,
- void *ext_priv, const char *buf,
- size_t len)
-{
- struct ti_eqep_cnt *priv = counter->priv;
- int err;
- u32 res;
-
- err = kstrtouint(buf, 0, );
- if (err < 0)
- return err;
-
- regmap_write(priv->regmap32, QPOSINIT, res);
-
- return len;
-}
-
static ssize_t ti_eqep_position_enable_read(struct counter_device *counter,
struct counter_count *count,
void *ext_priv, char *buf)
@@ -301,11 +271,6 @@ static struct counter_count_ext ti_eqep_position_ext[] = {
.read = ti_eqep_position_ceiling_read,
.write = ti_eqep_position_ceiling_write,
},
- {
- .name = "floor",
- .read = ti_eqep_position_floor_read,
- .write = ti_eqep_position_floor_write,
- },
{
.name = "enable",
.read = ti_eqep_position_enable_read,
--
2.25.1
Re: [PATCH v6 3/5] counter: Add character device interface
On 11/22/20 2:29 PM, William Breathitt Gray wrote:
This patch introduces a character device interface for the Counter
subsystem. Device data is exposed through standard character device read
operations. Device data is gathered when a Counter event is pushed by
the respective Counter device driver. Configuration is handled via ioctl
operations on the respective Counter character device node.
Cc: David Lechner
Cc: Gwendal Grignou
Cc: Dan Carpenter
Signed-off-by: William Breathitt Gray
---
diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
new file mode 100644
index ..96fa7fbeef92
--- /dev/null
+++ b/drivers/counter/counter-chrdev.c
+static ssize_t counter_chrdev_read(struct file *filp, char __user *buf,
+ size_t len, loff_t *f_ps)
+{
+ struct counter_device *const counter = filp->private_data;
+ int err;
+ unsigned long flags;
+ unsigned int copied;
+
+ if (len < sizeof(struct counter_event))
+ return -EINVAL;
+
+ do {
+ if (kfifo_is_empty(>events)) {
+ if (filp->f_flags & O_NONBLOCK)
+ return -EAGAIN;
+
+ err = wait_event_interruptible(counter->events_wait,
+ !kfifo_is_empty(>events));
+ if (err)
+ return err;
+ }
+
+ raw_spin_lock_irqsave(>events_lock, flags);
As mentioned in my previous review, I think it would be fine to use a
mutex here instead of disabling interrupts. In fact, I think copy_to_user()
might sleep, in which case we really don't want to call this with
interrupts disabled.
+ err = kfifo_to_user(>events, buf, len, );
+ raw_spin_unlock_irqrestore(>events_lock, flags);
+ if (err)
+ return err;
+ } while (!copied);
+
+ return copied;
+}
+
+static int counter_add_watch(struct counter_device *const counter,
+const unsigned long arg)
+{
+ void __user *const uwatch = (void __user *)arg;
+ struct counter_watch watch;
+ struct counter_comp_node comp_node = {0};
+ size_t parent, id;
+ struct counter_comp *ext;
+ size_t num_ext;
+
+ if (copy_from_user(, uwatch, sizeof(watch)))
+ return -EFAULT;
+
+ /* Dummy components can skip evaluation */
+ if (watch.component.type == COUNTER_COMPONENT_DUMMY)
I think "none" would be a better name than "dummy". Then it just
naturally makes sense why we would skip the evaluation.
+ goto dummy_component;
+
+ parent = watch.component.parent;
+
+ /* Configure parent component info for comp node */
+ switch (watch.component.scope) {
+ case COUNTER_SCOPE_DEVICE:
+ ext = counter->ext;
+ num_ext = counter->num_ext;
+ break;
+ case COUNTER_SCOPE_SIGNAL:
+ if (parent >= counter->num_signals)
+ return -EINVAL;
+ parent = array_index_nospec(parent, counter->num_signals);
+
+ comp_node.parent = counter->signals + parent;
+
+ ext = counter->signals[parent].ext;
+ num_ext = counter->signals[parent].num_ext;
+ break;
+ case COUNTER_SCOPE_COUNT:
+ if (parent >= counter->num_counts)
+ return -EINVAL;
+ parent = array_index_nospec(parent, counter->num_counts);
+
+ comp_node.parent = counter->counts + parent;
+
+ ext = counter->counts[parent].ext;
+ num_ext = counter->counts[parent].num_ext;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ id = watch.component.id;
+
+ /* Configure component info for comp node */
+ switch (watch.component.type) {
+ case COUNTER_COMPONENT_SIGNAL:
+ if (watch.component.scope != COUNTER_SCOPE_SIGNAL)
+ return -EINVAL;
+
+ comp_node.comp.type = COUNTER_COMP_SIGNAL_LEVEL;
+ comp_node.comp.signal_u8_read = counter->ops->signal_read;
+ break;
+ case COUNTER_COMPONENT_COUNT:
+ if (watch.component.scope != COUNTER_SCOPE_COUNT)
+ return -EINVAL;
+
+ comp_node.comp.type = COUNTER_COMP_U64;
+ comp_node.comp.count_u64_read = counter->ops->count_read;
+ break;
+ case COUNTER_COMPONENT_FUNCTION:
+ if (watch.component.scope != COUNTER_SCOPE_COUNT)
+ return -EINVAL;
+
+ comp_node.comp.type = COUNTER_COMP_FUNCTION;
+ comp_node.comp.count_u8
Re: [PATCH v6 0/5] Introduce the Counter character device interface
On 11/22/20 2:29 PM, William Breathitt Gray wrote:
1. Should standard Counter component data types be defined as u8 or u32?
Many standard Counter component types such COUNTER_COMP_SIGNAL_LEVEL
have standard values defined (e.g. COUNTER_SIGNAL_LEVEL_LOW and
COUNTER_SIGNAL_LEVEL_HIGH). These values are currently handled by the
Counter subsystem code as u8 data types.
If u32 is used for these values instead, C enum structures could be
used by driver authors to implicitly cast these values via the driver
callback parameters.
This question is primarily addressed to David Lechner. I'm somewhat
confused about how this setup would look in device drivers. I've gone
ahead and refactored the code to support u32 enums, and pushed it to
a separate branch on my repository called counter_chrdev_v6_u32_enum:
https://gitlab.com/vilhelmgray/iio/-/tree/counter_chrdev_v6_u32_enum
Please check it out and let me know what you think. Is this the
support you had in mind? I'm curious to see an example of how would
your driver callback functions would look in this case. If everything
works out fine, then I'll submit this branch as v7 of this patchset.
I haven't had time to look at this in depth, but just superficially looking
at it, it is mostly there. The driver callback would just use the enum type
in place of u32. For example:
static int ti_eqep_function_write(struct counter_device *counter,
struct counter_count *count,
enum counter_function function)
and the COUNTER_FUNCTION_* constants would be defined as:
enum counter_function {
COUNTER_FUNCTION_INCREASE,
...
};
instead of using #define macros.
One advantage I see to using u8, at least in the user API data structures,
is that it increases the number of events that fit in the kfifo buffer by
a significant factor.
And that is not to say that we couldn't do both: have the user API structs
use u8 for enum values and still use u32/strong enum types internally in
the callback functions.
2. How should we handle "raw" timestamps?
Ahmad Fatoum brought up the possibility of returning "raw" timestamps
similar to what the network stack offers (see the network stack
SOF_TIMESTAMPING_{RAW,SYS}_HARDWARE support).
I'm not very familiar with the networking stack code, but if I
understand correctly the SOF_TIMESTAMPING_RAW_HARDWARE timestamps are
values returned from the device. If so, I suspect we would be able to
support these "raw" timestamps by defining them as Counter Extensions
and returning them in struct counter_event elements similar to the
other Extension values.
Is nanosecond resolution good enough? In the TI eQEP driver I considered
returning the raw timer value, but quickly realized that it would not be
very nice to expect the user code to know the clock rate of the timer. It
was very easy to get the clock rate in the kernel and just convert the
timer value to nanoseconds before returning it to userspace.
So if there is some specialized case where it can be solved no other way
besides using raw timestamps, then sure, include it. Otherwise I think we
should stick with nanoseconds for time values when possible.
Re: [PATCH v6 1/5] counter: Internalize sysfs interface code
On 11/22/20 2:29 PM, William Breathitt Gray wrote:
14 files changed, 1806 insertions(+), 2546 deletions(-)
It would be really nice if we could break this down into smaller
pieces and start getting it merged. It is really tough to keep
reviewing this much code in one patch over and over again.
Here are some initial findings from testing:
+static void counter_device_release(struct device *dev)
+{
+ struct counter_device *const counter = dev_get_drvdata(dev);
+
+ counter_chrdev_remove(counter);
+ ida_simple_remove(_ida, counter->id);
+}
I got the following error after `modprobe -r ti-eqep`:
[ 1186.045766] [ cut here ]
[ 1186.050647] WARNING: CPU: 0 PID: 2625 at lib/refcount.c:28
counter_device_release+0x10/0x24 [counter]
[ 1186.059976] refcount_t: underflow; use-after-free.
[ 1186.064790] Modules linked in: aes_arm_bs(+) crypto_simd cryptd ccm
usb_f_mass_storage usb_f_acm u_serial usb_f_ecm rfcomm usb_f_rndis u_ether
libcomposite aes_arm aes_generic cmac bnep wl18xx wlcore mac80211 libarc4
sha256_generic libsha256 sha256_arm cfg80211 ti_am335x_adc kfifo_buf
omap_aes_driver omap_crypto omap_sham crypto_engine pm33xx ti_emif_sram
hci_uart omap_rng btbcm rng_core ti_eqep(-) counter bluetooth c_can_platform
c_can ecdh_generic bmp280_spi ecc can_dev libaes bmp280_i2c bmp280 industrialio
omap_mailbox musb_dsps wlcore_sdio musb_hdrc udc_core usbcore wkup_m3_ipc at24
omap_wdt phy_am335x watchdog phy_am335x_control ti_am335x_tscadc phy_generic
wkup_m3_rproc usb_common cppi41 rtc_omap leds_gpio led_class cpufreq_dt
pwm_tiehrpwm autofs4
[ 1186.132376] CPU: 0 PID: 2625 Comm: modprobe Not tainted
5.10.0-rc7bone-counter+ #23
[ 1186.140070] Hardware name: Generic AM33XX (Flattened Device Tree)
[ 1186.146225] [] (unwind_backtrace) from []
(show_stack+0x10/0x14)
[ 1186.154017] [] (show_stack) from []
(dump_stack+0xc4/0xe4)
[ 1186.161285] [] (dump_stack) from [] (__warn+0xd8/0x100)
[ 1186.168284] [] (__warn) from []
(warn_slowpath_fmt+0x94/0xbc)
[ 1186.175814] [] (warn_slowpath_fmt) from []
(counter_device_release+0x10/0x24 [counter])
[ 1186.185632] [] (counter_device_release [counter]) from
[] (device_release+0x30/0xa4)
[ 1186.195163] [] (device_release) from []
(kobject_put+0x94/0x104)
[ 1186.202944] [] (kobject_put) from []
(kobject_put+0x94/0x104)
[ 1186.210472] [] (kobject_put) from []
(ti_eqep_remove+0x10/0x30 [ti_eqep])
[ 1186.219047] [] (ti_eqep_remove [ti_eqep]) from []
(platform_drv_remove+0x24/0x3c)
[ 1186.228313] [] (platform_drv_remove) from []
(device_release_driver_internal+0xfc/0x1d0)
[ 1186.238187] [] (device_release_driver_internal) from []
(driver_detach+0x58/0xa8)
[ 1186.247456] [] (driver_detach) from []
(bus_remove_driver+0x4c/0xa0)
[ 1186.255594] [] (bus_remove_driver) from []
(sys_delete_module+0x180/0x264)
[ 1186.264250] [] (sys_delete_module) from []
(ret_fast_syscall+0x0/0x54)
[ 1186.272551] Exception stack(0xd247ffa8 to 0xd247fff0)
[ 1186.277629] ffa0: 004fb478 004fb478 004fb4b4 0800
b3bfcf00
[ 1186.285847] ffc0: 004fb478 004fb478 004fb478 0081 be974900
be974a55 004fb478
[ 1186.294062] ffe0: 004f8f5c be97352c 004ddd97 b6d11d68
[ 1186.299253] ---[ end trace e1c61dea091f1078 ]---
+static ssize_t counter_comp_u8_store(struct device *dev,
+struct device_attribute *attr,
+const char *buf, size_t len)
+{
+ const struct counter_attribute *const a = to_counter_attribute(attr);
+ struct counter_device *const counter = dev_get_drvdata(dev);
+ struct counter_count *const count = a->parent;
+ struct counter_synapse *const synapse = a->comp.priv;
+ const struct counter_available *const avail = a->comp.priv;
+ int err;
+ bool bool_data;
+ int idx;
+ u8 data;
+
+ switch (a->comp.type) {
+ case COUNTER_COMP_BOOL:
+ err = kstrtobool(buf, _data);
+ data = bool_data;
+ break;
+ case COUNTER_COMP_FUNCTION:
+ err = find_in_string_array(, count->functions_list,
+ count->num_functions, buf,
+ counter_function_str);
+ break;
+ case COUNTER_COMP_SYNAPSE_ACTION:
+ err = find_in_string_array(, synapse->actions_list,
+ synapse->num_actions, buf,
+ counter_synapse_action_str);
+ break;
+ case COUNTER_COMP_ENUM:
+ idx = __sysfs_match_string(avail->strs, avail->num_items, buf);
+ if (idx < 0)
+ return idx;
+ data = idx;
+ break;
+ case COUNTER_COMP_COUNT_MODE:
+ err = find_in_string_array(, avail->enums,
+ avail->num_items, buf,
+
Re: [PATCH 0/3] Enable eQEP counter driver on BeagleBone Blue
On 11/16/20 5:36 AM, Tony Lindgren wrote: * David Lechner [201013 00:13]: This series adds device tree nodes for the eQEP portion of the PWMSS on AM33xx and enables it on BeagleBone Blue. I actually submitted these a year ago, but it looks like these patches never got applied with the actual eQEP driver when it was merged. Sorry if I dropped these earlier, I guess I though you're reposting the series and untagged them. No worries, I forgot about them too. :-) For reference, there was some previous discussion about the clocks in "ARM: dts: am33xx: Add nodes for eQEP". [1] [1]: https://lore.kernel.org/linux-omap/20190723145100.gs5...@atomide.com/ I have also included a new patch to enable the eQEP driver in the defconfig. Great, thanks applying these into omap-for-v5.11/dt and defconfig branches. Regards, Tony
[PATCH v2] counter/ti-eqep: Fix regmap max_register
The values given were the offset of the register after the last
register instead of the actual last register in each range. Fix
by using the correct last register of each range.
Fixes: f213729f6796 ("counter: new TI eQEP driver")
Signed-off-by: David Lechner
Acked-by: William Breathitt Gray
---
v2 changes:
* add Fixes: tag
* picked up Acked-by:
drivers/counter/ti-eqep.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
index 1ff07faef27f..5d6470968d2c 100644
--- a/drivers/counter/ti-eqep.c
+++ b/drivers/counter/ti-eqep.c
@@ -368,7 +368,7 @@ static const struct regmap_config ti_eqep_regmap32_config =
{
.reg_bits = 32,
.val_bits = 32,
.reg_stride = 4,
- .max_register = 0x24,
+ .max_register = QUPRD,
};
static const struct regmap_config ti_eqep_regmap16_config = {
@@ -376,7 +376,7 @@ static const struct regmap_config ti_eqep_regmap16_config =
{
.reg_bits = 16,
.val_bits = 16,
.reg_stride = 2,
- .max_register = 0x1e,
+ .max_register = QCPRDLAT,
};
static int ti_eqep_probe(struct platform_device *pdev)
--
2.25.1
Re: [PATCH v5 3/5] counter: Add character device interface
diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c index e66ed99dd5ea..cefef61f170d 100644 --- a/drivers/counter/counter-sysfs.c +++ b/drivers/counter/counter-sysfs.c Not sure why sysfs changes are in the chrdev patch. Are these changes related somehow? Sorry, I forgot to explain this in the cover letter. The changes here are only useful for the character device interface. These changes introduce the extensionZ_name and extensionZ_width sysfs attributes. In the character device interface, extensions are selected by their id number, and the value returned depends on the type of data. The new sysfs attributes introduced here allow users to match the id of an extension with its name, as well as the bit width of the value returned so that the user knows whether to use the value_u8 or value_u64 union member in struct counter_event. Are we sure that all value types will always be CPU-endian unsigned integers? Or should we make an enum to describe the data type instead of just the width? It should be safe to assume that the character device interface will only ever return CPU-endian unsigned integers. The device driver should handle the conversion of any strange endianness from the device before the character device interface, while userspace is the one responsible for interpreting the meaning of count in the context of the application. Let's create a scenario for the sake of example. Suppose we want to use a counter device to track the vertical position of a component moved by a linear actuator. The operator considers some vertical position as the horizon, where anything above would be a positive position and anything below a negative position. The counter device stores its count in big-endian format; but the system CPU expects little-endian. The flow of data for this scenario would look like the following (where BE = big-endian, LE = little-endian): +--+ +---+ ++ | Raw data | - BE -> | Device driver | -> LE -> | chrdev | - u64 -> +--+ +---+ ++ At this point, the userspace application would read the unsigned integer from the character device and determine how to interpret the position -- whether the count be converted to a signed value to represent a negative physical position. Whether or not a position should be considered negative is dependent on the user application and context. Because the character device does not know the context of the user application, it should only provide unsigned integers in order to ensure a standard interface for counter devices; userspace will be responsible for interpreting those counts to something meaningful for the context of their applications. William Breathitt Gray Sounds reasonable to me.
Re: [PATCH v5 3/5] counter: Add character device interface
On 10/25/20 8:18 AM, William Breathitt Gray wrote:
On Tue, Oct 20, 2020 at 11:06:42AM -0500, David Lechner wrote:
On 10/18/20 11:58 AM, William Breathitt Gray wrote:
On Wed, Oct 14, 2020 at 05:40:44PM -0500, David Lechner wrote:
On 9/26/20 9:18 PM, William Breathitt Gray wrote:
+static ssize_t counter_chrdev_read(struct file *filp, char __user *buf,
+ size_t len, loff_t *f_ps)
+{
+ struct counter_device *const counter = filp->private_data;
+ int err;
+ unsigned long flags;
+ unsigned int copied;
+
+ if (len < sizeof(struct counter_event))
+ return -EINVAL;
+
+ do {
+ if (kfifo_is_empty(>events)) {
+ if (filp->f_flags & O_NONBLOCK)
+ return -EAGAIN;
+
+ err = wait_event_interruptible(counter->events_wait,
+ !kfifo_is_empty(>events));
+ if (err)
+ return err;
+ }
+
+ raw_spin_lock_irqsave(>events_lock, flags);
+ err = kfifo_to_user(>events, buf, len, );
+ raw_spin_unlock_irqrestore(>events_lock, flags);
+ if (err)
+ return err;
+ } while (!copied);
+
+ return copied;
+}
All other uses of kfifo_to_user() I saw use a mutex instead of spin
lock. I don't see a reason for disabling interrupts here.
The Counter character device interface is special in this case because
counter->events could be accessed from an interrupt context. This is
possible if counter_push_event() is called for an interrupt (as is the
case for the 104_quad_8 driver). In this case, we can't use mutex
because we can't sleep in an interrupt context, so our only option is to
use spin lock.
The way I understand it, locking is only needed for concurrent readers
and locking between reader and writer is not needed.
You're right, it does say in the kfifo.h comments that with only one
concurrent reader and one current write, we don't need extra locking to
use these macros. Because we only have one kfifo_to_user() operating on
counter->events, does that mean we don't need locking at all here for
the counter_chrdev_read() function?
William Breathitt Gray
Even if we have the policy that only one file handle to the chrdev
can be open at a time, it is still possible that the it could be
read from multiple threads. So it I think it makes sense to keep
it just to be safe.
Re: [PATCH v5 3/5] counter: Add character device interface
On 10/18/20 11:58 AM, William Breathitt Gray wrote:
On Wed, Oct 14, 2020 at 05:40:44PM -0500, David Lechner wrote:
On 9/26/20 9:18 PM, William Breathitt Gray wrote:
+static ssize_t counter_chrdev_read(struct file *filp, char __user *buf,
+ size_t len, loff_t *f_ps)
+{
+ struct counter_device *const counter = filp->private_data;
+ int err;
+ unsigned long flags;
+ unsigned int copied;
+
+ if (len < sizeof(struct counter_event))
+ return -EINVAL;
+
+ do {
+ if (kfifo_is_empty(>events)) {
+ if (filp->f_flags & O_NONBLOCK)
+ return -EAGAIN;
+
+ err = wait_event_interruptible(counter->events_wait,
+ !kfifo_is_empty(>events));
+ if (err)
+ return err;
+ }
+
+ raw_spin_lock_irqsave(>events_lock, flags);
+ err = kfifo_to_user(>events, buf, len, );
+ raw_spin_unlock_irqrestore(>events_lock, flags);
+ if (err)
+ return err;
+ } while (!copied);
+
+ return copied;
+}
All other uses of kfifo_to_user() I saw use a mutex instead of spin
lock. I don't see a reason for disabling interrupts here.
The Counter character device interface is special in this case because
counter->events could be accessed from an interrupt context. This is
possible if counter_push_event() is called for an interrupt (as is the
case for the 104_quad_8 driver). In this case, we can't use mutex
because we can't sleep in an interrupt context, so our only option is to
use spin lock.
The way I understand it, locking is only needed for concurrent readers
and locking between reader and writer is not needed.
Re: [PATCH v5 3/5] counter: Add character device interface
+static long counter_chrdev_ioctl(struct file *filp, unsigned int cmd,
+unsigned long arg)
+{
+ struct counter_device *const counter = filp->private_data;
+ raw_spinlock_t *const events_lock = >events_lock;
+ unsigned long flags;
+ struct list_head *const events_list = >events_list;
+ struct list_head *const next_events_list = >next_events_list;
+
+ switch (cmd) {
+ case COUNTER_CLEAR_WATCHES_IOCTL:
+ raw_spin_lock_irqsave(events_lock, flags);
+ counter_events_list_free(events_list);
+ raw_spin_unlock_irqrestore(events_lock, flags);
+ counter_events_list_free(next_events_list);
I think this ioctl is doing too much. If we have to use it for both
stopping events and clearing the list accumulated by
COUNTER_SET_WATCH_IOCTL, then we have a race condition of no events
after clearing watches during the time we are adding new ones and
until we load the new ones.
It would probably make more sense to call this ioctl
COUNTER_STOP_WATCHES_IOCTL and move counter_events_list_free(
next_events_list) to the end of COUNTER_LOAD_WATCHES_IOCTL.
I don't think we will necessarily have a race condition here.
COUNTER_CLEAR_WATCHES_IOCTL is intended to just clear the watches; e.g.
bring us back to a clear state when some sort of job has completely
finished and the user is no longer going to watch events for a while
(maybe they're adjusting the conveyor for the next job or some similar
operation).
I think the scenario you're concerned about is when you need to swap
watches in the middle of a job without losing events. In this case, you
wouldn't need to use COUNTER_CLEAR_WATCHES_IOCTL at all. Instead, you
would just set up the watches via COUNTER_SET_WATCH_IOCTL, and then use
COUNTER_LOAD_WATCHES_IOCTL to perform the swap; after
COUNTER_LOAD_WATCHES_IOCTL completes, next_events_list is empty (thanks
to list_replace_init()) and you're ready for the next set of watches.
Got it. I think I missed the fact that list_replace_init() clears
next_events_list.
+
+static int counter_chrdev_release(struct inode *inode, struct file *filp)
+{
+ struct counter_device *const counter = filp->private_data;
+ unsigned long flags;
+
+ put_device(>dev);
put_device() should be at the end of the function in case it is the last
reference.
put_device() shouldn't affect the counter_device events members, so I
don't think there's a difference in this case if it's called at the
beginning or end of the counter_chrdev_release function.
It isn't possible the some memory allocated with devm_kalloc() could be
be referenced after calling put_device() now or in the future?
+}
+EXPORT_SYMBOL_GPL(counter_push_event);
diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c
index e66ed99dd5ea..cefef61f170d 100644
--- a/drivers/counter/counter-sysfs.c
+++ b/drivers/counter/counter-sysfs.c
Not sure why sysfs changes are in the chrdev patch. Are these
changes related somehow?
Sorry, I forgot to explain this in the cover letter. The changes here
are only useful for the character device interface. These changes
introduce the extensionZ_name and extensionZ_width sysfs attributes.
In the character device interface, extensions are selected by their id
number, and the value returned depends on the type of data. The new
sysfs attributes introduced here allow users to match the id of an
extension with its name, as well as the bit width of the value returned
so that the user knows whether to use the value_u8 or value_u64 union
member in struct counter_event.
Are we sure that all value types will always be CPU-endian unsigned
integers? Or should we make an enum to describe the data type instead
of just the width?
Re: [PATCH v5 1/5] counter: Internalize sysfs interface code
On 10/18/20 9:49 AM, William Breathitt Gray wrote:
On Mon, Oct 12, 2020 at 09:15:00PM -0500, David Lechner wrote:
On 9/26/20 9:18 PM, William Breathitt Gray wrote:
This is a reimplementation of the Generic Counter driver interface.
I'll follow up if I find any problems while testing but here are some
comments I had from looking over the patch.
diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
new file mode 100644
index ..987c6e8277eb
--- /dev/null
+++ b/drivers/counter/counter-core.c
+/**
+ * counter_register - register Counter to the system
+ * @counter: pointer to Counter to register
+ *
+ * This function registers a Counter to the system. A sysfs "counter" directory
+ * will be created and populated with sysfs attributes correlating with the
+ * Counter Signals, Synapses, and Counts respectively.
+ */
+int counter_register(struct counter_device *const counter)
+{
+ struct device *const dev = >dev;
+ int err;
+
+ /* Acquire unique ID */
+ counter->id = ida_simple_get(_ida, 0, 0, GFP_KERNEL);
+ if (counter->id < 0)
+ return counter->id;
+
+ /* Configure device structure for Counter */
+ dev->type = _device_type;
+ dev->bus = _bus_type;
+ if (counter->parent) {
+ dev->parent = counter->parent;
+ dev->of_node = counter->parent->of_node;
+ }
+ dev_set_name(dev, "counter%d", counter->id);
+ device_initialize(dev);> + dev_set_drvdata(dev, counter);
+
+ /* Add Counter sysfs attributes */
+ err = counter_sysfs_add(counter);
+ if (err)
+ goto err_free_id;
+
+ /* Add device to system */
+ err = device_add(dev);
+ if (err) {
+ put_device(dev);
+ goto err_free_id;
+ }
+
+ return 0;
+
+err_free_id:
+ /* get_device/put_device combo used to free managed resources */
+ get_device(dev);
+ put_device(dev);
I've never seen this in a driver before, so it makes me think this is
not the "right way" to do this. After device_initialize() is called, we
already should have a reference to dev, so only device_put() is needed.
I do admit this feels very hacky, but I'm not sure how to handle this
more elegantly. The problem is that device_initialize() is not enough by
itself -- it just initializes the structure, while device_add()
increments the reference count via a call to get_device().
add_device() also releases this reference by calling put_device() in all
return paths. The reference count is initialized to 1 in device_initialize().
device_initialize > kobject_init > kobject_init_internal > kref_init
So let's assume counter_sysfs_add() fails and we go to err_free_id
before device_add() is called; if we ignore get_device(), the reference
count will be 0 at this point. We then call put_device(), which calls
kobject_put(), kref_put(), and eventually refcount_dec_and_test().
The refcount_dec_and_test() function returns 0 at this point because the
reference count can't be decremented further (refcount is already 0), so
release() is never called and we end up leaking all the memory we
allocated in counter_sysfs_add().
Is my logic correct here?
Not quite. I think you missed a few things I mentioned above. So we never
have a ref count of 0 until the very last call to put_device().
[PATCH] counter/ti-eqep: Fix regmap max_register
The values given were the offset of the register after the last
register instead of the actual last register in each range. Fix
by using the correct last register of each range.
Signed-off-by: David Lechner
---
drivers/counter/ti-eqep.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
index fe2c6bb22133..e60aec225541 100644
--- a/drivers/counter/ti-eqep.c
+++ b/drivers/counter/ti-eqep.c
@@ -798,7 +798,7 @@ static const struct regmap_config ti_eqep_regmap32_config =
{
.reg_bits = 32,
.val_bits = 32,
.reg_stride = 4,
- .max_register = 0x24,
+ .max_register = QUPRD,
};
static const struct regmap_config ti_eqep_regmap16_config = {
@@ -806,7 +806,7 @@ static const struct regmap_config ti_eqep_regmap16_config =
{
.reg_bits = 16,
.val_bits = 16,
.reg_stride = 2,
- .max_register = 0x1e,
+ .max_register = QCPRDLAT,
};
static int ti_eqep_probe(struct platform_device *pdev)
--
2.25.1
Re: [PATCH v5 1/5] counter: Internalize sysfs interface code
On 9/26/20 9:18 PM, William Breathitt Gray wrote:
+static ssize_t counter_comp_u8_store(struct device *dev,
+struct device_attribute *attr,
+const char *buf, size_t len)
+{
+ const struct counter_attribute *const a = to_counter_attribute(attr);
+ struct counter_device *const counter = dev_get_drvdata(dev);
+ struct counter_count *const count = a->parent;
+ struct counter_synapse *const synapse = a->comp.priv;
+ const struct counter_available *const avail = a->comp.priv;
+ int err;
+ bool bool_data;
+ u8 data;
+
+ switch (a->comp.type) {
+ case COUNTER_COMP_BOOL:
+ err = kstrtobool(buf, _data);
+ data = bool_data;
+ break;
+ case COUNTER_COMP_FUNCTION:
+ err = find_in_string_array(, count->functions_list,
+ count->num_functions, buf,
+ counter_function_str);
+ break;
+ case COUNTER_COMP_SYNAPSE_ACTION:
+ err = find_in_string_array(, synapse->actions_list,
+ synapse->num_actions, buf,
+ counter_synapse_action_str);
+ break;
+ case COUNTER_COMP_ENUM:
+ err = __sysfs_match_string(avail->strs, avail->num_items, buf);
+ data = err;
+ break;
+ case COUNTER_COMP_COUNT_MODE:
+ err = find_in_string_array(, avail->enums,
+ avail->num_items, buf,
+ counter_count_mode_str);
+ break;
+ default:
+ err = kstrtou8(buf, 0, );
+ break;
+ }
In this function, return values are not always errors. So it would make
sense to call the err variable ret instead and check for ret < 0 below.
Setting enums to a value with index > 0 fails currently because of this.
+ if (err)
+ return err;
+
+ switch (a->scope) {
+ case COUNTER_SCOPE_DEVICE:
+ err = a->comp.device_u8_write(counter, data);
+ break;
+ case COUNTER_SCOPE_SIGNAL:
+ err = a->comp.signal_u8_write(counter, a->parent, data);
+ break;
+ case COUNTER_SCOPE_COUNT:
+ if (a->comp.type == COUNTER_COMP_SYNAPSE_ACTION)
+ err = a->comp.action_write(counter, count, synapse,
+ data);
+ else
+ err = a->comp.count_u8_write(counter, count, data);
+ break;
+ }
+ if (err)
+ return err;
+
+ return len;
+}
Re: [PATCH v5 3/5] counter: Add character device interface
On 9/26/20 9:18 PM, William Breathitt Gray wrote:
+static ssize_t counter_chrdev_read(struct file *filp, char __user *buf,
+ size_t len, loff_t *f_ps)
+{
+ struct counter_device *const counter = filp->private_data;
+ int err;
+ unsigned long flags;
+ unsigned int copied;
+
+ if (len < sizeof(struct counter_event))
+ return -EINVAL;
+
+ do {
+ if (kfifo_is_empty(>events)) {
+ if (filp->f_flags & O_NONBLOCK)
+ return -EAGAIN;
+
+ err = wait_event_interruptible(counter->events_wait,
+ !kfifo_is_empty(>events));
+ if (err)
+ return err;
+ }
+
+ raw_spin_lock_irqsave(>events_lock, flags);
+ err = kfifo_to_user(>events, buf, len, );
+ raw_spin_unlock_irqrestore(>events_lock, flags);
+ if (err)
+ return err;
+ } while (!copied);
+
+ return copied;
+}
All other uses of kfifo_to_user() I saw use a mutex instead of spin
lock. I don't see a reason for disabling interrupts here.
Example:
static ssize_t iio_event_chrdev_read(struct file *filep,
char __user *buf,
size_t count,
loff_t *f_ps)
{
struct iio_dev *indio_dev = filep->private_data;
struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
struct iio_event_interface *ev_int = iio_dev_opaque->event_interface;
unsigned int copied;
int ret;
if (!indio_dev->info)
return -ENODEV;
if (count < sizeof(struct iio_event_data))
return -EINVAL;
do {
if (kfifo_is_empty(_int->det_events)) {
if (filep->f_flags & O_NONBLOCK)
return -EAGAIN;
ret = wait_event_interruptible(ev_int->wait,
!kfifo_is_empty(_int->det_events) ||
indio_dev->info == NULL);
if (ret)
return ret;
if (indio_dev->info == NULL)
return -ENODEV;
}
if (mutex_lock_interruptible(_int->read_lock))
return -ERESTARTSYS;
ret = kfifo_to_user(_int->det_events, buf, count, );
mutex_unlock(_int->read_lock);
if (ret)
return ret;
/*
* If we couldn't read anything from the fifo (a different
* thread might have been faster) we either return -EAGAIN if
* the file descriptor is non-blocking, otherwise we go back to
* sleep and wait for more data to arrive.
*/
if (copied == 0 && (filep->f_flags & O_NONBLOCK))
return -EAGAIN;
} while (copied == 0);
return copied;
}
Re: [PATCH v5 3/5] counter: Add character device interface
On 10/14/20 2:05 PM, William Breathitt Gray wrote:
On Wed, Oct 14, 2020 at 12:43:08PM -0500, David Lechner wrote:
On 9/26/20 9:18 PM, William Breathitt Gray wrote:
diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
new file mode 100644
index ..2be3846e4105
--- /dev/null
+++ b/drivers/counter/counter-chrdev.c
+/**
+ * counter_push_event - queue event for userspace reading
+ * @counter: pointer to Counter structure
+ * @event: triggered event
+ * @channel: event channel
+ *
+ * Note: If no one is watching for the respective event, it is silently
+ * discarded.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int counter_push_event(struct counter_device *const counter, const u8 event,
+ const u8 channel)
+{
+ struct counter_event ev = {0};
+ unsigned int copied = 0;
+ unsigned long flags;
+ struct counter_event_node *event_node;
+ struct counter_comp_node *comp_node;
+ int err;
+
+ ev.timestamp = ktime_get_ns();
+ ev.watch.event = event;
+ ev.watch.channel = channel;
+
+ raw_spin_lock_irqsave(>events_lock, flags);
+
+ /* Search for event in the list */
+ list_for_each_entry(event_node, >events_list, l)
+ if (event_node->event == event &&
+ event_node->channel == channel)
+ break;
+
+ /* If event is not in the list */
+ if (_node->l == >events_list)
+ goto exit_early;
+
+ /* Read and queue relevant comp for userspace */
+ list_for_each_entry(comp_node, _node->comp_list, l) {
+ err = counter_get_data(counter, comp_node, _u8);
Currently all counter devices are memory mapped devices so calling
counter_get_data() here with interrupts disabled is probably OK, but
if any counter drivers are added that use I2C/SPI/etc. that will take
a long time to read, it would cause problems leaving interrupts
disabled here.
Brainstorming: Would it make sense to separate the event from the
component value being read? As I mentioned in one of my previous
reviews, I think there are some cases where we would just want to
know when an event happened and not read any additional data anyway.
In the case of a slow communication bus, this would also let us
queue the event in the kfifo and notify poll right away and then
defer the reads in a workqueue for later.
I don't see any problems with reporting just an event without any
component value attached (e.g. userspace could handle the component
reads via sysfs at a later point). We would just need a way to inform
userspace that the struct counter_component in the struct counter_watch
returned should be ignored.
Perhaps we can add an additional member to struct counter_watch
indicating whether it's an empty watch; or alternatively, add a new
component scope define to differentiate between an actual component and
an empty one (e.g. COUNTER_SCOPE_EVENT). What do you think?
William Breathitt Gray
I made the same suggestion in one of my other replies - except
I called it COUNTER_SCOPE_NONE.
Re: [PATCH v5 3/5] counter: Add character device interface
On 9/26/20 9:18 PM, William Breathitt Gray wrote:
diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
new file mode 100644
index ..2be3846e4105
--- /dev/null
+++ b/drivers/counter/counter-chrdev.c
+/**
+ * counter_push_event - queue event for userspace reading
+ * @counter: pointer to Counter structure
+ * @event: triggered event
+ * @channel: event channel
+ *
+ * Note: If no one is watching for the respective event, it is silently
+ * discarded.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int counter_push_event(struct counter_device *const counter, const u8 event,
+ const u8 channel)
+{
+ struct counter_event ev = {0};
+ unsigned int copied = 0;
+ unsigned long flags;
+ struct counter_event_node *event_node;
+ struct counter_comp_node *comp_node;
+ int err;
+
+ ev.timestamp = ktime_get_ns();
+ ev.watch.event = event;
+ ev.watch.channel = channel;
+
+ raw_spin_lock_irqsave(>events_lock, flags);
+
+ /* Search for event in the list */
+ list_for_each_entry(event_node, >events_list, l)
+ if (event_node->event == event &&
+ event_node->channel == channel)
+ break;
+
+ /* If event is not in the list */
+ if (_node->l == >events_list)
+ goto exit_early;
+
+ /* Read and queue relevant comp for userspace */
+ list_for_each_entry(comp_node, _node->comp_list, l) {
+ err = counter_get_data(counter, comp_node, _u8);
Currently all counter devices are memory mapped devices so calling
counter_get_data() here with interrupts disabled is probably OK, but
if any counter drivers are added that use I2C/SPI/etc. that will take
a long time to read, it would cause problems leaving interrupts
disabled here.
Brainstorming: Would it make sense to separate the event from the
component value being read? As I mentioned in one of my previous
reviews, I think there are some cases where we would just want to
know when an event happened and not read any additional data anyway.
In the case of a slow communication bus, this would also let us
queue the event in the kfifo and notify poll right away and then
defer the reads in a workqueue for later.
+ if (err)
+ goto err_counter_get_data;
+
+ ev.watch.component = comp_node->component;
+
+ copied += kfifo_put(>events, ev);
+ }
+
+ if (copied)
+ wake_up_poll(>events_wait, EPOLLIN);
+
+exit_early:
+ raw_spin_unlock_irqrestore(>events_lock, flags);
+
+ return 0;
+
+err_counter_get_data:
+ raw_spin_unlock_irqrestore(>events_lock, flags);
+ return err;
+}
Re: [PATCH v5 3/5] counter: Add character device interface
On 9/26/20 9:18 PM, William Breathitt Gray wrote:
This patch introduces a character device interface for the Counter
subsystem. Device data is exposed through standard character device read
operations. Device data is gathered when a Counter event is pushed by
the respective Counter device driver. Configuration is handled via ioctl
operations on the respective Counter character device node.
Probably don't need to duplicate the full documentation in the commit
message.
diff --git a/drivers/counter/counter-chrdev.c b/drivers/counter/counter-chrdev.c
new file mode 100644
index ..2be3846e4105
--- /dev/null
+++ b/drivers/counter/counter-chrdev.c
+
+static int counter_set_event_node(struct counter_device *const counter,
+ struct counter_watch *const watch,
+ const struct counter_comp_node *const cfg)
+{
+ struct counter_event_node *event_node;
+ int err;
+ struct counter_comp_node *comp_node;
+
+ /* Search for event in the list */
+ list_for_each_entry(event_node, >next_events_list, l)
+ if (event_node->event == watch->event &&
+ event_node->channel == watch->channel)
+ break;
+
+ /* If event is not already in the list */
+ if (_node->l == >next_events_list) {
+ /* Allocate new event node */
+ event_node = kmalloc(sizeof(*event_node), GFP_ATOMIC);
+ if (!event_node)
+ return -ENOMEM;
+
+ /* Configure event node and add to the list */
+ event_node->event = watch->event;
+ event_node->channel = watch->channel;
+ INIT_LIST_HEAD(_node->comp_list);
+ list_add(_node->l, >next_events_list);
+ }
+
+ /* Check if component watch has already been set before */
+ list_for_each_entry(comp_node, _node->comp_list, l)
+ if (comp_node->parent == cfg->parent &&
+ comp_node->comp.count_u8_read == cfg->comp.count_u8_read)
+ return -EINVAL;
There are already enough things that could cause EINVAL, we could
probably skip this duplicate check to make troubleshooting easier.
+
+ /* Allocate component node */
+ comp_node = kmalloc(sizeof(*comp_node), GFP_ATOMIC);
+ if (!comp_node) {
+ err = -ENOMEM;
+ goto err_comp_node;
Since there is only one goto, we could just handle the error and
return here instead.
+ }
+ *comp_node = *cfg;
+
+ /* Add component node to event node */
+ list_add_tail(_node->l, _node->comp_list);
+
+ return 0;
+
+err_comp_node:
A comment explaining the list_empty() check would be nice.
It makes sense if you think about it, but it is not super
obvious.
+ if (list_empty(_node->comp_list)) {
+ list_del(_node->l);
+ kfree(event_node);
+ }
+ return err;
+}
+
+static int counter_set_watch(struct counter_device *const counter,
+const unsigned long arg)
+{
+ void __user *const uwatch = (void __user *)arg;
+ struct counter_watch watch;
+ struct counter_comp_node comp_node;
+ size_t parent, id;
+ struct counter_comp *ext;
+ size_t num_ext;
+
+ if (copy_from_user(, uwatch, sizeof(watch)))
+ return -EFAULT;
+ parent = watch.component.parent;
+ id = watch.component.id;
+
+ /* Configure parent component info for comp node */
+ switch (watch.component.scope) {
+ case COUNTER_SCOPE_DEVICE:
+ comp_node.parent = NULL;
+
+ ext = counter->ext;
+ num_ext = counter->num_ext;
+ break;
+ case COUNTER_SCOPE_SIGNAL:
+ if (counter->num_signals < parent + 1)
I think it would be more conventional this way:
if (parent >= counter->num_signals)
+ return -EINVAL;
+
+ comp_node.parent = counter->signals + parent;
+
+ ext = counter->signals[parent].ext;
+ num_ext = counter->signals[parent].num_ext;
+ break;
+ case COUNTER_SCOPE_COUNT:
+ if (counter->num_counts < parent + 1)
Same here.
+ return -EINVAL;
+
+ comp_node.parent = counter->counts + parent;
+
+ ext = counter->counts[parent].ext;
+ num_ext = counter->counts[parent].num_ext;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ /* Configure component info for comp node */
+ switch (watch.component.type) {
+ case COUNTER_COMPONENT_SIGNAL:
+ if (watch.component.scope != COUNTER_SCOPE_SIGNAL)
+ return -EINVAL;
+
+ comp_node.comp.type = COUNTER_COMP_SIGNAL_LEVEL;
+ comp_node.comp.signal_u8_read =
Re: [PATCH v5 5/5] counter: 104-quad-8: Add IRQ support for the ACCES 104-QUAD-8
On 9/26/20 9:18 PM, William Breathitt Gray wrote:
+static irqreturn_t quad8_irq_handler(int irq, void *quad8iio)
+{
+ struct quad8_iio *const priv = quad8iio;
+ const unsigned long base = priv->base;
+ unsigned long irq_status;
+ unsigned long channel;
+ u8 event;
+ int err;
+
+ irq_status = inb(base + QUAD8_REG_INTERRUPT_STATUS);
+ if (!irq_status)
+ return IRQ_NONE;
+
+ for_each_set_bit(channel, _status, QUAD8_NUM_COUNTERS) {
+ switch (priv->irq_trigger[channel]) {
+ case 0:
+ event = COUNTER_EVENT_OVERFLOW;
+ break;
+ case 1:
+ event = COUNTER_EVENT_THRESHOLD;
+ break;
+ case 2:
+ event = COUNTER_EVENT_OVERFLOW_UNDERFLOW;
+ break;
+ case 3:
+ event = COUNTER_EVENT_INDEX;
+ break;
+ default:
+ /* We should never reach here */
+ return -EINVAL;
This is not a valid return value for an IRQ handler. Maybe WARN_ONCE instead?
+ }
+ err = counter_push_event(>counter, event, channel);
+ if (err)
+ return err;
Same here. Otherwise, I think we could end up with interrupts in an endless
loop since the interrupt would never be cleared.
+ }
+
+ /* Clear pending interrupts on device */
+ outb(QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC, base + QUAD8_REG_CHAN_OP);
+
+ return IRQ_HANDLED;
+}
+
Re: [PATCH v5 4/5] docs: counter: Document character device interface
On 10/13/20 1:58 PM, William Breathitt Gray wrote:
On Mon, Oct 12, 2020 at 12:04:10PM -0500, David Lechner wrote:
On 10/8/20 7:28 AM, William Breathitt Gray wrote:
On Thu, Oct 08, 2020 at 10:09:09AM +0200, Pavel Machek wrote:
Hi!
+int main(void)
+{
+struct pollfd pfd = { .events = POLLIN };
+struct counter_event event_data[2];
+
+pfd.fd = open("/dev/counter0", O_RDWR);
+
+ioctl(pfd.fd, COUNTER_SET_WATCH_IOCTL, watches);
+ioctl(pfd.fd, COUNTER_SET_WATCH_IOCTL, watches + 1);
+ioctl(pfd.fd, COUNTER_LOAD_WATCHES_IOCTL);
+
+for (;;) {
+poll(, 1, -1);
Why do poll, when you are doing blocking read?
+read(pfd.fd, event_data, sizeof(event_data));
Does your new chrdev always guarantee returning complete buffer?
If so, should it behave like that?
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
I suppose you're right: a poll() should be redundant now with this
version of the character device implementation because buffers will
always return complete; so a blocking read() should achieve the same
behavior that a poll() with read() would.
I'll give some more time for additional feedback to come in for this
version of the patchset, and then likely remove support for poll() in
the v6 submission.
William Breathitt Gray
I hope that you mean that you will just remove it from the example
and not from the chardev. Otherwise it won't be possible to
integrate this with an event loop.
Would you elaborate a bit further on this? My thought process is that
because users must set the Counter Events they want to watch, and only
those Counter Events show up in the character device node, a blocking
read() would effectively behave the same as poll() with read(); if none
of the Counter Events occur, the read() just blocks until one does, thus
making the use of a poll() call redundant.
William Breathitt Gray
If the counter device was the only file descriptor being read, then yes
it wouldn't matter. But if we are using this in combination with other
file descriptors, then it is common to poll all of the file descriptors
using a single syscall to see which one is ready to read rather than
doing a non-blocking read on all of the file descriptors, which would
result in many unnecessary syscalls.
Re: [PATCH v5 1/5] counter: Internalize sysfs interface code
On 9/26/20 9:18 PM, William Breathitt Gray wrote:
This is a reimplementation of the Generic Counter driver interface.
I'll follow up if I find any problems while testing but here are some
comments I had from looking over the patch.
diff --git a/drivers/counter/counter-core.c b/drivers/counter/counter-core.c
new file mode 100644
index ..987c6e8277eb
--- /dev/null
+++ b/drivers/counter/counter-core.c
+/**
+ * counter_register - register Counter to the system
+ * @counter: pointer to Counter to register
+ *
+ * This function registers a Counter to the system. A sysfs "counter" directory
+ * will be created and populated with sysfs attributes correlating with the
+ * Counter Signals, Synapses, and Counts respectively.
+ */
+int counter_register(struct counter_device *const counter)
+{
+ struct device *const dev = >dev;
+ int err;
+
+ /* Acquire unique ID */
+ counter->id = ida_simple_get(_ida, 0, 0, GFP_KERNEL);
+ if (counter->id < 0)
+ return counter->id;
+
+ /* Configure device structure for Counter */
+ dev->type = _device_type;
+ dev->bus = _bus_type;
+ if (counter->parent) {
+ dev->parent = counter->parent;
+ dev->of_node = counter->parent->of_node;
+ }
+ dev_set_name(dev, "counter%d", counter->id);
+ device_initialize(dev);> + dev_set_drvdata(dev, counter);
+
+ /* Add Counter sysfs attributes */
+ err = counter_sysfs_add(counter);
+ if (err)
+ goto err_free_id;
+
+ /* Add device to system */
+ err = device_add(dev);
+ if (err) {
+ put_device(dev);
+ goto err_free_id;
+ }
+
+ return 0;
+
+err_free_id:
+ /* get_device/put_device combo used to free managed resources */
+ get_device(dev);
+ put_device(dev);
I've never seen this in a driver before, so it makes me think this is
not the "right way" to do this. After device_initialize() is called, we
already should have a reference to dev, so only device_put() is needed.
+ ida_simple_remove(_ida, counter->id);
In the case of error after device_initialize() is called, won't this
result in ida_simple_remove() being called twice, once here and once in
the release callback?
+ return err;
+}
+EXPORT_SYMBOL_GPL(counter_register);
+
+/**
+ * counter_unregister - unregister Counter from the system
+ * @counter: pointer to Counter to unregister
+ *
+ * The Counter is unregistered from the system; all allocated memory is freed.
+ */
+void counter_unregister(struct counter_device *const counter)
+{
+ if (!counter)
+ return;
+
+ device_unregister(>dev);
+}
+EXPORT_SYMBOL_GPL(counter_unregister);
+
+static void devm_counter_unreg(struct device *dev, void *res)
To be consistent, it would be nice to spell out unregister.
+{
+ counter_unregister(*(struct counter_device **)res);
+}
+
diff --git a/drivers/counter/counter-sysfs.c b/drivers/counter/counter-sysfs.c
new file mode 100644
index ..e66ed99dd5ea
--- /dev/null
+++ b/drivers/counter/counter-sysfs.c
+/**
+ * counter_sysfs_add - Adds Counter sysfs attributes to the device structure
+ * @counter: Pointer to the Counter device structure
+ *
+ * Counter sysfs attributes are created and added to the respective device
+ * structure for later registration to the system. Resource-managed memory
+ * allocation is performed by this function, and this memory should be freed
+ * when no longer needed (automatically by a device_unregister call, or
+ * manually by a devres_release_all call).
+ */
+int counter_sysfs_add(struct counter_device *const counter)
+{
+ struct device *const dev = >dev;
+ const size_t num_groups = counter->num_signals + counter->num_counts +
+ 1;
It is OK to go past 80 columns, especially for just for a few characters.
+ struct counter_attribute_group *groups;
+ size_t i, j;
+ int err;
+ struct attribute_group *group;
+ struct counter_attribute *p;
+
+ /* Allocate space for attribute groups (signals, counts, and ext) */
+ groups = devm_kcalloc(dev, num_groups, sizeof(*groups), GFP_KERNEL);
+ if (!groups)
+ return -ENOMEM;
+
+ /* Initialize attribute lists */
+ for (i = 0; i < num_groups; i++)
+ INIT_LIST_HEAD([i].attr_list);
+
+ /* Register Counter device attributes */
+ err = counter_device_register(counter, groups);
This function name is a bit misleading. At first I though we were registering
a new counter device (struct device). Maybe counter_sysfs_create_attrs()
would be a better name? (I wouldn't mind having all functions in this
file having a "counter_sysfs_" prefix for that matter.)
diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
index 1ff07faef27f..938085dead80 100644
--- a/drivers/counter/ti-eqep.c
+++
Re: [PATCH v5 0/5] Introduce the Counter character device interface
On 9/26/20 9:18 PM, William Breathitt Gray wrote: The following are some questions I have about this patchset: 1. Should standard Counter component data types be defined as u8 or u32? Many standard Counter component types such COUNTER_COMP_SIGNAL_LEVEL have standard values defined (e.g. COUNTER_SIGNAL_LEVEL_LOW and COUNTER_SIGNAL_LEVEL_HIGH). These values are currently handled by the Counter subsystem code as u8 data types. If u32 is used for these values instead, C enum structures could be used by driver authors to implicit cast these values via the driver callback parameters; userspace would still use u32 with no issue. In theory this can work because GCC will treat enums are having a 32-bit size; but I worry about the possibility of build targets that have -fshort-enums enabled, resulting in enums having a size less than 32 bits. Would this be a problem? We shouldn't have to worry about userspace programs using -fshort-enums since that would break all kernel interfaces that use enums, not just these - so no one should be using that compiler flag. So I am in favor of using strongly typed enums with u32 as the "generic" enum member type. 2. Should I have reserved members in the userspace structures? The structures in include/uapi/linux/counter.h are available to userspace applications. Should I reserve space in these structures for future additions and usage? Will endianess and packing be a concern here? Since there doesn't seem to be a large number of counter devices this probably isn't critical. Are there any aspects of counter devices in general that couldn't be described with the proposed structures? For example, could there be components more than two levels deep (i.e. it would need id, parent id and grandparent id to describe fully)?
[PATCH 3/3] ARM: omap2plus_defconfig: Enable TI eQEP counter driver
This enables the TI eQEP counter driver that is used by BeagleBone Blue. Signed-off-by: David Lechner --- arch/arm/configs/omap2plus_defconfig | 2 ++ 1 file changed, 2 insertions(+) diff --git a/arch/arm/configs/omap2plus_defconfig b/arch/arm/configs/omap2plus_defconfig index fe383f5a92fb..71b1a8f4c241 100644 --- a/arch/arm/configs/omap2plus_defconfig +++ b/arch/arm/configs/omap2plus_defconfig @@ -524,6 +524,8 @@ CONFIG_PHY_DM816X_USB=m CONFIG_OMAP_USB2=m CONFIG_TI_PIPE3=y CONFIG_TWL4030_USB=m +CONFIG_COUNTER=m +CONFIG_TI_EQEP=m CONFIG_EXT2_FS=y CONFIG_EXT3_FS=y CONFIG_EXT4_FS_SECURITY=y -- 2.25.1
[PATCH 1/3] ARM: dts: am33xx: Add nodes for eQEP
This adds new nodes for the Texas Instruments Enhanced Quadrature
Encoder Pulse (eQEP) module in the PWM subsystem on AM33XX.
Signed-off-by: David Lechner
---
arch/arm/boot/dts/am33xx-l4.dtsi | 27 +++
1 file changed, 27 insertions(+)
diff --git a/arch/arm/boot/dts/am33xx-l4.dtsi b/arch/arm/boot/dts/am33xx-l4.dtsi
index b88d0caa4b2d..17910268df82 100644
--- a/arch/arm/boot/dts/am33xx-l4.dtsi
+++ b/arch/arm/boot/dts/am33xx-l4.dtsi
@@ -1924,6 +1924,15 @@ ecap0: ecap@100 {
status = "disabled";
};
+ eqep0: counter@180 {
+ compatible = "ti,am3352-eqep";
+ reg = <0x180 0x80>;
+ clocks = <_gclk>;
+ clock-names = "sysclkout";
+ interrupts = <79>;
+ status = "disabled";
+ };
+
ehrpwm0: pwm@200 {
compatible = "ti,am3352-ehrpwm",
"ti,am33xx-ehrpwm";
@@ -1976,6 +1985,15 @@ ecap1: ecap@100 {
status = "disabled";
};
+ eqep1: counter@180 {
+ compatible = "ti,am3352-eqep";
+ reg = <0x180 0x80>;
+ clocks = <_gclk>;
+ clock-names = "sysclkout";
+ interrupts = <88>;
+ status = "disabled";
+ };
+
ehrpwm1: pwm@200 {
compatible = "ti,am3352-ehrpwm",
"ti,am33xx-ehrpwm";
@@ -2028,6 +2046,15 @@ ecap2: ecap@100 {
status = "disabled";
};
+ eqep2: counter@180 {
+ compatible = "ti,am3352-eqep";
+ reg = <0x180 0x80>;
+ clocks = <_gclk>;
+ clock-names = "sysclkout";
+ interrupts = <89>;
+ status = "disabled";
+ };
+
ehrpwm2: pwm@200 {
compatible = "ti,am3352-ehrpwm",
"ti,am33xx-ehrpwm";
--
2.25.1
[PATCH 0/3] Enable eQEP counter driver on BeagleBone Blue
This series adds device tree nodes for the eQEP portion of the PWMSS on AM33xx and enables it on BeagleBone Blue. I actually submitted these a year ago, but it looks like these patches never got applied with the actual eQEP driver when it was merged. For reference, there was some previous discussion about the clocks in "ARM: dts: am33xx: Add nodes for eQEP". [1] [1]: https://lore.kernel.org/linux-omap/20190723145100.gs5...@atomide.com/ I have also included a new patch to enable the eQEP driver in the defconfig. David Lechner (3): ARM: dts: am33xx: Add nodes for eQEP ARM: dts: am335x-boneblue: Enable eQEP ARM: omap2plus_defconfig: Enable TI eQEP counter driver arch/arm/boot/dts/am335x-boneblue.dts | 54 +++ arch/arm/boot/dts/am33xx-l4.dtsi | 27 ++ arch/arm/configs/omap2plus_defconfig | 2 + 3 files changed, 83 insertions(+) -- 2.25.1
[PATCH 2/3] ARM: dts: am335x-boneblue: Enable eQEP
This enables the Enhanced Quadrature Encoder Pulse (eQEP) module for
connectors E1, E2 and E3 on BeagleBone Blue.
Signed-off-by: David Lechner
---
arch/arm/boot/dts/am335x-boneblue.dts | 54 +++
1 file changed, 54 insertions(+)
diff --git a/arch/arm/boot/dts/am335x-boneblue.dts
b/arch/arm/boot/dts/am335x-boneblue.dts
index c696d57cf364..69acaf4ea0f3 100644
--- a/arch/arm/boot/dts/am335x-boneblue.dts
+++ b/arch/arm/boot/dts/am335x-boneblue.dts
@@ -241,6 +241,30 @@ AM33XX_PADCONF(AM335X_PIN_UART0_CTSN, PIN_OUTPUT,
MUX_MODE2) /* (E18) uart0_cts
AM33XX_PADCONF(AM335X_PIN_MII1_RXD0, PIN_OUTPUT,
MUX_MODE7) /* (M16) gmii1_rxd0.gpio2[21] */
>;
};
+
+ /* E1 */
+ eqep0_pins: pinmux_eqep0_pins {
+ pinctrl-single,pins = <
+ AM33XX_PADCONF(AM335X_PIN_MCASP0_AXR0, PIN_INPUT,
MUX_MODE1)/* (B12) mcasp0_aclkr.eQEP0A_in */
+ AM33XX_PADCONF(AM335X_PIN_MCASP0_FSR, PIN_INPUT,
MUX_MODE1) /* (C13) mcasp0_fsr.eQEP0B_in */
+ >;
+ };
+
+ /* E2 */
+ eqep1_pins: pinmux_eqep1_pins {
+ pinctrl-single,pins = <
+ AM33XX_PADCONF(AM335X_PIN_LCD_DATA12, PIN_INPUT,
MUX_MODE2) /* (V2) lcd_data12.eQEP1A_in */
+ AM33XX_PADCONF(AM335X_PIN_LCD_DATA13, PIN_INPUT,
MUX_MODE2) /* (V3) lcd_data13.eQEP1B_in */
+ >;
+ };
+
+ /* E3 */
+ eqep2_pins: pinmux_eqep2_pins {
+ pinctrl-single,pins = <
+ AM33XX_PADCONF(AM335X_PIN_GPMC_AD12, PIN_INPUT,
MUX_MODE4) /* (T12) gpmc_ad12.eQEP2A_in */
+ AM33XX_PADCONF(AM335X_PIN_GPMC_AD13, PIN_INPUT,
MUX_MODE4) /* (R12) gpmc_ad13.eQEP2B_in */
+ >;
+ };
};
{
@@ -419,3 +443,33 @@ ls_buf_en {
line-name = "LS_BUF_EN";
};
};
+
+ {
+ status = "okay";
+};
+
+ {
+ pinctrl-names = "default";
+ pinctrl-0 = <_pins>;
+ status = "okay";
+};
+
+ {
+ status = "okay";
+};
+
+ {
+ pinctrl-names = "default";
+ pinctrl-0 = <_pins>;
+ status = "okay";
+};
+
+ {
+ status = "okay";
+};
+
+ {
+ pinctrl-names = "default";
+ pinctrl-0 = <_pins>;
+ status = "okay";
+};
--
2.25.1
Re: [PATCH v5 4/5] docs: counter: Document character device interface
On 10/8/20 7:28 AM, William Breathitt Gray wrote:
On Thu, Oct 08, 2020 at 10:09:09AM +0200, Pavel Machek wrote:
Hi!
+int main(void)
+{
+struct pollfd pfd = { .events = POLLIN };
+struct counter_event event_data[2];
+
+pfd.fd = open("/dev/counter0", O_RDWR);
+
+ioctl(pfd.fd, COUNTER_SET_WATCH_IOCTL, watches);
+ioctl(pfd.fd, COUNTER_SET_WATCH_IOCTL, watches + 1);
+ioctl(pfd.fd, COUNTER_LOAD_WATCHES_IOCTL);
+
+for (;;) {
+poll(, 1, -1);
Why do poll, when you are doing blocking read?
+read(pfd.fd, event_data, sizeof(event_data));
Does your new chrdev always guarantee returning complete buffer?
If so, should it behave like that?
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
I suppose you're right: a poll() should be redundant now with this
version of the character device implementation because buffers will
always return complete; so a blocking read() should achieve the same
behavior that a poll() with read() would.
I'll give some more time for additional feedback to come in for this
version of the patchset, and then likely remove support for poll() in
the v6 submission.
William Breathitt Gray
I hope that you mean that you will just remove it from the example
and not from the chardev. Otherwise it won't be possible to
integrate this with an event loop.
[tip: irq/core] irqchip/irq-pruss-intc: Implement irq_{get, set}_irqchip_state ops
The following commit has been merged into the irq/core branch of tip:
Commit-ID: b1026e8a95e430e615579f14f0f73c94f9690468
Gitweb:
https://git.kernel.org/tip/b1026e8a95e430e615579f14f0f73c94f9690468
Author:David Lechner
AuthorDate:Wed, 16 Sep 2020 18:36:37 +02:00
Committer: Marc Zyngier
CommitterDate: Thu, 17 Sep 2020 12:20:32 +01:00
irqchip/irq-pruss-intc: Implement irq_{get, set}_irqchip_state ops
This implements the irq_get_irqchip_state and irq_set_irqchip_state
callbacks for the TI PRUSS INTC driver. The set callback can be used
by drivers to "kick" a PRU by injecting a PRU system event.
Co-developed-by: Suman Anna
Co-developed-by: Grzegorz Jaszczyk
Signed-off-by: Suman Anna
Signed-off-by: David Lechner
Signed-off-by: Grzegorz Jaszczyk
Reviewed-by: Lee Jones
Signed-off-by: Marc Zyngier
---
drivers/irqchip/irq-pruss-intc.c | 40 +++-
1 file changed, 40 insertions(+)
diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
index e7ba358..bfe529a 100644
--- a/drivers/irqchip/irq-pruss-intc.c
+++ b/drivers/irqchip/irq-pruss-intc.c
@@ -12,6 +12,7 @@
* Copyright (C) 2019 David Lechner
*/
+#include
#include
#include
#include
@@ -323,6 +324,43 @@ static void pruss_intc_irq_relres(struct irq_data *data)
module_put(THIS_MODULE);
}
+static int pruss_intc_irq_get_irqchip_state(struct irq_data *data,
+ enum irqchip_irq_state which,
+ bool *state)
+{
+ struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
+ u32 reg, mask, srsr;
+
+ if (which != IRQCHIP_STATE_PENDING)
+ return -EINVAL;
+
+ reg = PRU_INTC_SRSR(data->hwirq / 32);
+ mask = BIT(data->hwirq % 32);
+
+ srsr = pruss_intc_read_reg(intc, reg);
+
+ *state = !!(srsr & mask);
+
+ return 0;
+}
+
+static int pruss_intc_irq_set_irqchip_state(struct irq_data *data,
+ enum irqchip_irq_state which,
+ bool state)
+{
+ struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
+
+ if (which != IRQCHIP_STATE_PENDING)
+ return -EINVAL;
+
+ if (state)
+ pruss_intc_write_reg(intc, PRU_INTC_SISR, data->hwirq);
+ else
+ pruss_intc_write_reg(intc, PRU_INTC_SICR, data->hwirq);
+
+ return 0;
+}
+
static struct irq_chip pruss_irqchip = {
.name = "pruss-intc",
.irq_ack= pruss_intc_irq_ack,
@@ -330,6 +368,8 @@ static struct irq_chip pruss_irqchip = {
.irq_unmask = pruss_intc_irq_unmask,
.irq_request_resources = pruss_intc_irq_reqres,
.irq_release_resources = pruss_intc_irq_relres,
+ .irq_get_irqchip_state = pruss_intc_irq_get_irqchip_state,
+ .irq_set_irqchip_state = pruss_intc_irq_set_irqchip_state,
};
static int pruss_intc_validate_mapping(struct pruss_intc *intc, int event,
Re: [PATCH 15/18] counter: use semicolons rather than commas to separate statements
On 9/27/20 2:12 PM, Julia Lawall wrote: Replace commas with semicolons. What is done is essentially described by the following Coccinelle semantic patch (http://coccinelle.lip6.fr/): // @@ expression e1,e2; @@ e1 -, +; e2 ... when any // Signed-off-by: Julia Lawall --- Reviewed-by: David Lechner
Re: [PATCH 09/10] clk: davinci: Add missing kerneldoc
On 9/2/20 10:03 AM, Krzysztof Kozlowski wrote: Add missing kerneldoc to fix compile warning: drivers/clk/davinci/da8xx-cfgchip.c:578: warning: Function parameter or member 'dev' not described in 'da8xx_cfgchip_register_usb1_clk48' Signed-off-by: Krzysztof Kozlowski --- Reviewed-by: David Lechner
Re: [PATCH 10/10] clk: davinci: Simplify with dev_err_probe()
On 9/2/20 10:03 AM, Krzysztof Kozlowski wrote: Common pattern of handling deferred probe can be simplified with dev_err_probe(). Less code and the error value gets printed. Signed-off-by: Krzysztof Kozlowski --- Reviewed-by: David Lechner
Re: [PATCH 5/5] power: supply: lego_ev3: Simplify with dev_err_probe()
On 8/26/20 9:48 AM, Krzysztof Kozlowski wrote: Common pattern of handling deferred probe can be simplified with dev_err_probe(). Less code and also it prints the error value. Signed-off-by: Krzysztof Kozlowski --- Reviewed-by: David Lechner
Re: [PATCH] power: supply: Add dependency to lego-ev3-battery Kconfig options
On 8/17/20 12:43 PM, Alex Dewar wrote: On Wed, Aug 12, 2020 at 02:12:57PM -0500, David Lechner wrote: On 8/12/20 2:02 PM, Alex Dewar wrote: On Wed, Aug 12, 2020 at 10:24:30AM -0500, David Lechner wrote: On 8/12/20 8:37 AM, Alex Dewar wrote: On Tue, Aug 11, 2020 at 09:24:10AM -0500, David Lechner wrote: On 8/9/20 1:54 PM, Alex Dewar wrote: This battery appears only to be used by a single board (DA850), so it makes sense to add this to the Kconfig file so that users don't build the module unnecessarily. It currently seems to be built for the x86 Arch Linux kernel where it's probably not doing much good. It would probably also make sense to add "default n" since it only applies to one board in the entire arch. Ah ok. That makes sense. Would you like me to send a follow-on patch for this? You can just send a v2 patch that includes the change below and the additional change. I've just had a look at the documentation[1] and it seems that as there's no "default y" there it'll default to n anyway. Have I got that right? [1] https://www.kernel.org/doc/html/latest/kbuild/kconfig-language.html#menu-attributes Yes, that seems right. That makes me wonder why this would have been enabled in the Arch Linux kernel for x86 then. Not sure, maybe the Arch devs like Lego? ;-) Are you happy to give an Acked-by for this anyhoo? Yup. Acked-by: David Lechner @Sebastian, are you happy to pick up this patch? Best, Alex
Re: [PATCH] power: supply: Add dependency to lego-ev3-battery Kconfig options
On 8/12/20 2:02 PM, Alex Dewar wrote: On Wed, Aug 12, 2020 at 10:24:30AM -0500, David Lechner wrote: On 8/12/20 8:37 AM, Alex Dewar wrote: On Tue, Aug 11, 2020 at 09:24:10AM -0500, David Lechner wrote: On 8/9/20 1:54 PM, Alex Dewar wrote: This battery appears only to be used by a single board (DA850), so it makes sense to add this to the Kconfig file so that users don't build the module unnecessarily. It currently seems to be built for the x86 Arch Linux kernel where it's probably not doing much good. It would probably also make sense to add "default n" since it only applies to one board in the entire arch. Ah ok. That makes sense. Would you like me to send a follow-on patch for this? You can just send a v2 patch that includes the change below and the additional change. I've just had a look at the documentation[1] and it seems that as there's no "default y" there it'll default to n anyway. Have I got that right? [1] https://www.kernel.org/doc/html/latest/kbuild/kconfig-language.html#menu-attributes Yes, that seems right. That makes me wonder why this would have been enabled in the Arch Linux kernel for x86 then.
Re: [PATCH] clk: davinci: Use the correct size when allocating memory
On 8/9/20 9:49 AM, Christophe JAILLET wrote:
'sizeof(*pllen)' should be used in place of 'sizeof(*pllout)' to avoid a
small over-allocation.
Fixes: 2d1726915159 ("clk: davinci: New driver for davinci PLL clocks")
Signed-off-by: Christophe JAILLET
---
Reviewed-by: David Lechner
Re: [PATCH] power: supply: Add dependency to lego-ev3-battery Kconfig options
On 8/12/20 8:37 AM, Alex Dewar wrote: On Tue, Aug 11, 2020 at 09:24:10AM -0500, David Lechner wrote: On 8/9/20 1:54 PM, Alex Dewar wrote: This battery appears only to be used by a single board (DA850), so it makes sense to add this to the Kconfig file so that users don't build the module unnecessarily. It currently seems to be built for the x86 Arch Linux kernel where it's probably not doing much good. It would probably also make sense to add "default n" since it only applies to one board in the entire arch. Ah ok. That makes sense. Would you like me to send a follow-on patch for this? You can just send a v2 patch that includes the change below and the additional change. Alex BATTERY_LEGO_EV3 is already explicitly set to "m" in the appropriate defconfig file, so I don't think it would break anything. Signed-off-by: Alex Dewar --- drivers/power/supply/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig index faf2830aa1527..9f76e2f47ac6d 100644 --- a/drivers/power/supply/Kconfig +++ b/drivers/power/supply/Kconfig @@ -164,7 +164,7 @@ config BATTERY_DS2782 config BATTERY_LEGO_EV3 tristate "LEGO MINDSTORMS EV3 battery" - depends on OF && IIO && GPIOLIB + depends on OF && IIO && GPIOLIB && (ARCH_DAVINCI_DA850 || COMPILE_TEST) help Say Y here to enable support for the LEGO MINDSTORMS EV3 battery.
Re: [PATCH] power: supply: Add dependency to lego-ev3-battery Kconfig options
On 8/9/20 1:54 PM, Alex Dewar wrote: This battery appears only to be used by a single board (DA850), so it makes sense to add this to the Kconfig file so that users don't build the module unnecessarily. It currently seems to be built for the x86 Arch Linux kernel where it's probably not doing much good. It would probably also make sense to add "default n" since it only applies to one board in the entire arch. BATTERY_LEGO_EV3 is already explicitly set to "m" in the appropriate defconfig file, so I don't think it would break anything. Signed-off-by: Alex Dewar --- drivers/power/supply/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig index faf2830aa1527..9f76e2f47ac6d 100644 --- a/drivers/power/supply/Kconfig +++ b/drivers/power/supply/Kconfig @@ -164,7 +164,7 @@ config BATTERY_DS2782 config BATTERY_LEGO_EV3 tristate "LEGO MINDSTORMS EV3 battery" - depends on OF && IIO && GPIOLIB + depends on OF && IIO && GPIOLIB && (ARCH_DAVINCI_DA850 || COMPILE_TEST) help Say Y here to enable support for the LEGO MINDSTORMS EV3 battery.
Re: [PATCH v4 3/5] counter: Add character device interface
On 8/9/20 9:51 AM, William Breathitt Gray wrote: On Tue, Jul 28, 2020 at 07:20:03PM -0500, David Lechner wrote: On 7/21/20 2:35 PM, William Breathitt Gray wrote: This patch introduces a character device interface for the Counter subsystem. Device data is exposed through standard character device read operations. Device data is gathered when a Counter event is pushed by the respective Counter device driver. Configuration is handled via ioctl operations on the respective Counter character device node. This sounds similar to triggers and buffers in the iio subsystem. And I can see how it might be useful in some cases. But I think it would not give the desired results when performance is important. Thinking through a few cases here... Suppose there was a new counter device that used the I2C bus. This would either have to be periodically polled for events or it might have a separate GPIO line to notify the MCU. In any case, with the proposed implementation, there would be a separate I2C transaction for each data point for that event. So none of the data for that event would actually be from the same point in time. And with I2C, this time difference could be significant. With the TI eQEP I have been working with, there are special latched registers for some events. To make use of these with events, we would have add extensions for each one we want to use (and expose it in sysfs). But really, the fact that we are using a latched register should be an implementation detail in the driver and not something userspace should have to know about. So, I'm wondering if it would make sense to keep things simpler and have events like the input subsystem where the event value is directly tied to the event. It would probably be rare for an event to have more than one or two values. And error events probably would not have a value at all. For example, with the TI eQEP, there is a unit timer time out event. This latches the position count, the timer count and the timer period. To translate this to an event data structure, the latched time would be the event timestamp and the position count would be the event value. The timer period should already be known since we would have configured the timer ourselves. There is also a count event that works similarly. In this case, the latched time would be the event timestamp and the latched timer period would be the event value. We would know the count already since we get an event for each count (and a separate direction change event if the direction changes). There are use-cases where it would be useful to have the extension reads occur as close to the event trigger as possible (e.g. multiple-axes positioning with boundary sensor flags) so I don't think this functionality should be completely abadoned, but I think your argument for standard event types makes sense. We could treat those extensions reads as an optional feature that can be enabled and configured by ioctls. However, the use-case you are concerned with, we can redesign Counter events to return specific data based on the specific event type. For example, we could have a COUNTER_EVENT_INDEX which occurs when an Index signal edge is detected, and the return data is the Count value for that channel; we can also have a COUNTER_EVENT_TIMEOUT which occurs when a unit timer times out, and returns the data you mentioned you are interested in seeing. These Counter event types would be standard, so user applications wouldn't need to know driver/device implementation details, but instead just follow the API to get the data they expect for that particular event type. Would this kind of design work for your needs? Yes. After trying (and failing) to implement my suggestions here, I came to the conclusion that it was not sufficient to only have one value per event. And it doesn't seem as obvious as I initially thought which should be the "standard" value for an event in some cases. When `counter_push_event(counter, 1)` is called for example, it will go down the list for Event 1 and execute the read callbacks for Signal 0, Signal 0 Extension 0, and Extension 4 -- the data returned for each is pushed to a kfifo as a `struct counter_event`, which userspace can retrieve via a standard read operation on the respective character device node. Userspace - Userspace applications can configure Counter events via ioctl operations on the Counter character device node. There following ioctl codes are supported and provided by the `linux/counter.h` userspace header file: * COUNTER_CLEAR_WATCHES_IOCTL: Clear all Counter watches from all events * COUNTER_SET_WATCH_IOCTL: Set a Counter watch on the specified event To configure events to gather Counter data, users first populate a `struct counter_watch` with the relevant event id and the information for the desired Counter component from which to read, and then pass it via the `COUNTER_SET_WATCH_IOCTL` ioctl command. Userspace applications can then execu
Re: [PATCH v4 1/5] counter: Internalize sysfs interface code
CPMAC ETHERNET DRIVER
M: Florian Fainelli
diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
index 78766b6ec271..0f20920073d6 100644
--- a/drivers/counter/104-quad-8.c
+++ b/drivers/counter/104-quad-8.c
@@ -621,7 +621,7 @@ static const struct iio_chan_spec quad8_channels[] = {
};
static int quad8_signal_read(struct counter_device *counter,
- struct counter_signal *signal, enum counter_signal_value *val)
+struct counter_signal *signal, u8 *val)
I'm not a fan of replacing enum types with u8 everywhere in this patch.
But if we have to for technical reasons (e.g. causes compiler error if
we don't) then it would be helpful to add comments giving the enum type
everywhere like this instance where u8 is actually an enum value.
If we use u32 as the generic type for enums instead of u8, I think the
compiler will happlily let us use enum type and u32 interchangeably and
not complain.
I switched to fixed-width types after the suggestion by David Laight:
https://lkml.org/lkml/2020/5/3/159. I'll CC David Laight just in case he
wants to chime in again.
Enum types would be nice for making the valid values explicit, but there
is one benefit I have appreciated from the move to fixed-width types:
there has been a significant reduction of duplicate code; before, we had
a different read function for each different enum type, but now we use a
single function to handle them all.
Yes, what I was trying to explain is that by using u32 instead of u8, I
think we can actually do both.
The function pointers in struct counter_device *counter would use u32 as a
generic enum value in the declaration, but then the actual implementations
could still use the proper enum type.
Oh, I see what you mean now. So for example:
int (*signal_read)(struct counter_device *counter,
struct counter_signal *signal, u8 *val)
This will become instead:
int (*signal_read)(struct counter_device *counter,
struct counter_signal *signal, u32 *val)
Then in the driver callback implementation we use the enum type we need:
enum counter_signal_level signal_level = COUNTER_SIGNAL_HIGH;
...
*val = signal_level;
Is that what you have in mind?
Yes.
Additionally, if we have...
int (*x_write)(struct counter_device *counter,
..., u32 val)
We should be able to define the implementation as:
static int my_driver_x_write(struct counter_device *counter,
..., enum some_type val)
{
...
}
Not sure if it works if val is a pointer though. Little-
endian systems would probably be fine, but maybe not big-
endian combined with -fshort-enums compiler flag.
int (*x_read)(struct counter_device *counter,
..., u32 *val)
static int my_driver_x_read(struct counter_device *counter,
..., enum some_type *val)
{
...
}
Re: [PATCH v4 1/5] counter: Internalize sysfs interface code
On 8/2/20 4:04 PM, William Breathitt Gray wrote:
On Tue, Jul 28, 2020 at 05:45:53PM -0500, David Lechner wrote:
On 7/21/20 2:35 PM, William Breathitt Gray wrote:
This is a reimplementation of the Generic Counter driver interface.
...
-F: include/linux/counter_enum.h
+F: include/uapi/linux/counter.h
Seems odd to be introducing a uapi header here since this patch doesn't
make any changes to userspace.
These defines are needed by userspace for the character device
interface, but I see your point that at this point in the patchset they
don't need to be exposed yet.
I could create temporary include/linux/counter_types.h to house these
defines, and then later move them to include/uapi/linux/counter.h in the
character device interface introduction patch. Do you think I should do
so?
Since this patch is independent of the chardev changes and probably ready
to merge after one more round of review, I would say it probably makes
sense to just leave them in counter.h for now and move them to uapi when
the chardev interface is finalized. This way, we can just merge this patch
as soon as it is ready.
CPMAC ETHERNET DRIVER
M: Florian Fainelli
diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
index 78766b6ec271..0f20920073d6 100644
--- a/drivers/counter/104-quad-8.c
+++ b/drivers/counter/104-quad-8.c
@@ -621,7 +621,7 @@ static const struct iio_chan_spec quad8_channels[] = {
};
static int quad8_signal_read(struct counter_device *counter,
- struct counter_signal *signal, enum counter_signal_value *val)
+struct counter_signal *signal, u8 *val)
I'm not a fan of replacing enum types with u8 everywhere in this patch.
But if we have to for technical reasons (e.g. causes compiler error if
we don't) then it would be helpful to add comments giving the enum type
everywhere like this instance where u8 is actually an enum value.
If we use u32 as the generic type for enums instead of u8, I think the
compiler will happlily let us use enum type and u32 interchangeably and
not complain.
I switched to fixed-width types after the suggestion by David Laight:
https://lkml.org/lkml/2020/5/3/159. I'll CC David Laight just in case he
wants to chime in again.
Enum types would be nice for making the valid values explicit, but there
is one benefit I have appreciated from the move to fixed-width types:
there has been a significant reduction of duplicate code; before, we had
a different read function for each different enum type, but now we use a
single function to handle them all.
Yes, what I was trying to explain is that by using u32 instead of u8, I
think we can actually do both.
The function pointers in struct counter_device *counter would use u32 as a
generic enum value in the declaration, but then the actual implementations
could still use the proper enum type.
+ device_del(>dev);
+ counter_sysfs_free(counter);
Should sysfs be freed before deleting device? I think sysfs might be
using dev still.
I think it's the other way around isn't it? The Counter sysfs memory
should stay alive for the lifetime of the device. Once the device is
deleted, there's nothing left to access those struct attributes, so that
memory can now be freed. Correct me if my reasoning is wrong here.
I think you are right. I was thinking that device_del() would free
memory, but it doesn't. It also looks like other drivers call
device_put() after this, so maybe needed here too?
+static ssize_t counter_data_u8_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ const struct counter_attribute *const a = to_counter_attribute(attr);
+ struct counter_device *const counter = dev_get_drvdata(dev);
+ const struct counter_available *const avail = a->data.priv;
+ int err;
+ u8 data;
+
+ switch (a->type) {
I don't understand the use of the word "owner" here. What is being "owned"?
Perhaps "component" would be a better choice?
I wasn't too set on calling this "owner" either, but I'm not sure if
"component" would make sense either because I wouldn't label a device
attribute as belonging to any particular component (in fact it's quite
the opposite).
Perhaps the word "scope" would be better. What do you think? Or would
that be too vague as well.
"scope" makes sense to me.
-/**
- * struct counter_signal_ext - Counter Signal extensions
- * @name: attribute name
- * @read: read callback for this attribute; may be NULL
- * @write: write callback for this attribute; may be NULL
- * @priv: data private to the driver
- */
-struct counter_signal_ext {
+enum counter_data_type {
+ COUNTER_DATA_TYPE_U8,
+ COUNTER_DATA_TYPE_U64,
+ COUNTER_DATA_TYPE_BOOL,
+ COUNTER_DATA_TYPE_SIGNAL,
Does this mean signal name?
This repre
Re: [PATCH v4 4/5] irqchip/irq-pruss-intc: Implement irq_{get,set}_irqchip_state ops
On 7/31/20 7:28 AM, Grzegorz Jaszczyk wrote:
On Wed, 29 Jul 2020 at 21:23, David Lechner wrote:
On 7/28/20 4:18 AM, Grzegorz Jaszczyk wrote:
From: David Lechner
This implements the irq_get_irqchip_state and irq_set_irqchip_state
callbacks for the TI PRUSS INTC driver. The set callback can be used
by drivers to "kick" a PRU by injecting a PRU system event.
Example:
We could improve this example by showing a device tree node of a
firmware-defined device implemented in the PRU:
/* Software-defined UART in PRU */
pru_uart: serial@ {
compatible = "ti,pru-uart";
...
interrupt-parent = <_intc>;
/* PRU system event 31, channel 0, host event 0 */
interrupts = <31 0 0>, ...;
interrupt-names = "kick", ...;
...
},
Then driver would request the IRQ during probe:
data->kick_irq = of_irq_get_byname(dev, "kick");
if (data->kick_irq < 0)
...
And later the driver would use the IRQ to kick the PRU:
irq_set_irqchip_state(data->kick_irq, IRQCHIP_STATE_PENDING, true);
We could but I am not sure if this kind of complex example should land
in the commit log.
Marc could you please comment how you want to see this?
Thank you,
Grzegorz
Based on the discussion in the device tree binding patch, the expanded
example I gave is incorrect, so we can just drop it.
Re: [PATCH v4 1/5] dt-bindings: irqchip: Add PRU-ICSS interrupt controller bindings
On 7/31/20 6:48 AM, Grzegorz Jaszczyk wrote: On Wed, 29 Jul 2020 at 19:34, David Lechner wrote: It is not clear what the meaning of each cell is. Looking at later patches, it looks like the first cell is the PRU system event number, the second cell is the channel and the third cell is the host event number. Ok, how about updating above description like this: Client users shall use the PRU System event number (the interrupt source that the client is interested in) [cell 1], PRU channel [cell 2] and PRU host_intr (target) [cell 3] as the value of the interrupts property in their node. The system events can be mapped to some output host interrupts through 2 levels of many-to-one mapping i.e. events to channel mapping and channels to host interrupts so through this property entire mapping is provided. Cell 3 is host_intr0-7? How would we map to other host events?
Re: [PATCH v4 3/5] counter: Add character device interface
On 7/28/20 7:20 PM, David Lechner wrote: On 7/21/20 2:35 PM, William Breathitt Gray wrote: This patch introduces a character device interface for the Counter subsystem. Device data is exposed through standard character device read operations. Device data is gathered when a Counter event is pushed by the respective Counter device driver. Configuration is handled via ioctl operations on the respective Counter character device node. This sounds similar to triggers and buffers in the iio subsystem. And I can see how it might be useful in some cases. But I think it would not give the desired results when performance is important. By the way, I really appreciate the work you have done here. When reviewing code, it is easy to point out what is wrong or we don't like and to not mention all the parts that are good. And there is a lot of really good work here already. I've been working on this all week to try out some of my suggestions and I'm not getting very far. This is a very difficult problem to solve! I just wanted to mention this since I responded to this patch series already but I am still learning and trying things. So I may have more/ different feedback in the future and I may decide some of my suggestions are not so good. :-) And one more thing, there was a nice talk at the Embedded Linux Conference last month about lessons learned from designing a userspace API for the GPIO subsystem [1]. Unfortunately, there is no video yet, but the slides might have some helpful ideas about mistakes to avoid. [1]: https://elinux.org/ELC_2020_Presentations
Re: [PATCH v4 5/5] irqchip/irq-pruss-intc: Add support for ICSSG INTC on K3 SoCs
On 7/28/20 4:18 AM, Grzegorz Jaszczyk wrote: From: Suman Anna The K3 AM65x and J721E SoCs have the next generation of the PRU-ICSS IP, commonly called ICSSG. The PRUSS INTC present within the ICSSG supports more System Events (160 vs 64), more Interrupt Channels and Host Interrupts (20 vs 10) compared to the previous generation PRUSS INTC instances. The first 2 and the last 10 of these host interrupt lines are used by the PRU and other auxiliary cores and sub-modules within the ICSSG, with 8 host interrupts connected to MPU. The host interrupts 5, 6, 7 are also connected to the other ICSSG instances within the SoC and can be partitioned as per system integration through the board dts files. Enhance the PRUSS INTC driver to add support for this ICSSG INTC instance. Signed-off-by: Suman Anna Signed-off-by: Grzegorz Jaszczyk --- There is not much left in this patch. Might as well squash this into "irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts".
Re: [PATCH v4 4/5] irqchip/irq-pruss-intc: Implement irq_{get,set}_irqchip_state ops
On 7/28/20 4:18 AM, Grzegorz Jaszczyk wrote:
From: David Lechner
This implements the irq_get_irqchip_state and irq_set_irqchip_state
callbacks for the TI PRUSS INTC driver. The set callback can be used
by drivers to "kick" a PRU by injecting a PRU system event.
Example:
We could improve this example by showing a device tree node of a
firmware-defined device implemented in the PRU:
/* Software-defined UART in PRU */
pru_uart: serial@ {
compatible = "ti,pru-uart";
...
interrupt-parent = <_intc>;
/* PRU system event 31, channel 0, host event 0 */
interrupts = <31 0 0>, ...;
interrupt-names = "kick", ...;
...
},
Then driver would request the IRQ during probe:
data->kick_irq = of_irq_get_byname(dev, "kick");
if (data->kick_irq < 0)
...
And later the driver would use the IRQ to kick the PRU:
irq_set_irqchip_state(data->kick_irq, IRQCHIP_STATE_PENDING, true);
irq_set_irqchip_state(irq, IRQCHIP_STATE_PENDING, true);
Signed-off-by: David Lechner
Signed-off-by: Suman Anna
Signed-off-by: Grzegorz Jaszczyk
Reviewed-by: Lee Jones
---
Re: [PATCH v4 3/5] irqchip/irq-pruss-intc: Add logic for handling reserved interrupts
On 7/28/20 4:18 AM, Grzegorz Jaszczyk wrote:
From: Suman Anna
The PRUSS INTC has a fixed number of output interrupt lines that are
connected to a number of processors or other PRUSS instances or other
devices (like DMA) on the SoC. The output interrupt lines 2 through 9
are usually connected to the main Arm host processor and are referred
to as host interrupts 0 through 7 from ARM/MPU perspective.
All of these 8 host interrupts are not always exclusively connected
to the Arm interrupt controller. Some SoCs have some interrupt lines
not connected to the Arm interrupt controller at all, while a few others
have the interrupt lines connected to multiple processors in which they
need to be partitioned as per SoC integration needs. For example, AM437x
and 66AK2G SoCs have 2 PRUSS instances each and have the host interrupt 5
connected to the other PRUSS, while AM335x has host interrupt 0 shared
between MPU and TSC_ADC and host interrupts 6 & 7 shared between MPU and
a DMA controller.
Add logic to the PRUSS INTC driver to ignore both these shared and
invalid interrupts.
If a person wanted to use DMA with a PRU what will handle the mapping
of a PRU event to host interrupt 6 or 7 if they are being ignored here?
Signed-off-by: Suman Anna
Signed-off-by: Grzegorz Jaszczyk
---
v3->v4:
- Due to changes in DT bindings which converts irqs-reserved
property from uint8-array to bitmask requested by Rob introduce
relevant changes in the driver.
- Merge the irqs-reserved and irqs-shared to one property since they
can be handled by one logic (relevant change was introduced to DT
binding).
- Update commit message.
v2->v3:
- Extra checks for (intc->irqs[i]) in error/remove path was moved from
"irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS
interrupts" to this patch
v1->v2:
- https://patchwork.kernel.org/patch/11069757/
---
drivers/irqchip/irq-pruss-intc.c | 29 -
1 file changed, 24 insertions(+), 5 deletions(-)
diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
index 45b966a..cf9a59b 100644
--- a/drivers/irqchip/irq-pruss-intc.c
+++ b/drivers/irqchip/irq-pruss-intc.c
@@ -474,7 +474,7 @@ static int pruss_intc_probe(struct platform_device *pdev)
struct pruss_intc *intc;
struct pruss_host_irq_data *host_data[MAX_NUM_HOST_IRQS] = { NULL };
int i, irq, ret;
- u8 max_system_events;
+ u8 max_system_events, invalid_intr = 0;
data = of_device_get_match_data(dev);
if (!data)
@@ -496,6 +496,16 @@ static int pruss_intc_probe(struct platform_device *pdev)
return PTR_ERR(intc->base);
}
+ ret = of_property_read_u8(dev->of_node, "ti,irqs-reserved",
+ _intr);
Why not make the variable name match the property name?
+
+ /*
+* The irqs-reserved is used only for some SoC's therefore not having
+* this property is still valid
+*/
+ if (ret < 0 && ret != -EINVAL)
+ return ret;
+
pruss_intc_init(intc);
mutex_init(>lock);
@@ -506,6 +516,9 @@ static int pruss_intc_probe(struct platform_device *pdev)
return -ENOMEM;
for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
+ if (invalid_intr & BIT(i))
+ continue;
+
irq = platform_get_irq_byname(pdev, irq_names[i]);
if (irq <= 0) {
dev_err(dev, "platform_get_irq_byname failed for %s :
%d\n",
@@ -533,8 +546,11 @@ static int pruss_intc_probe(struct platform_device *pdev)
return 0;
fail_irq:
- while (--i >= 0)
- irq_set_chained_handler_and_data(intc->irqs[i], NULL, NULL);
+ while (--i >= 0) {
+ if (intc->irqs[i])
+ irq_set_chained_handler_and_data(intc->irqs[i], NULL,
+NULL);
+ }
irq_domain_remove(intc->domain);
@@ -548,8 +564,11 @@ static int pruss_intc_remove(struct platform_device *pdev)
unsigned int hwirq;
int i;
- for (i = 0; i < MAX_NUM_HOST_IRQS; i++)
- irq_set_chained_handler_and_data(intc->irqs[i], NULL, NULL);
+ for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
+ if (intc->irqs[i])
+ irq_set_chained_handler_and_data(intc->irqs[i], NULL,
+NULL);
+ }
for (hwirq = 0; hwirq < max_system_events; hwirq++)
irq_dispose_mapping(irq_find_mapping(intc->domain, hwirq));
Re: [PATCH v4 2/5] irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts
On 7/28/20 4:18 AM, Grzegorz Jaszczyk wrote: The Programmable Real-Time Unit Subsystem (PRUSS) contains a local interrupt controller (INTC) that can handle various system input events and post interrupts back to the device-level initiators. The INTC can support upto 64 input events with individual control configuration and hardware prioritization. These events are mapped onto 10 output interrupt lines through two levels of many-to-one mapping support. Different interrupt lines are routed to the individual PRU cores or to the host CPU, or to other devices on the SoC. Some of these events are sourced from peripherals or other sub-modules within that PRUSS, while a few others are sourced from SoC-level peripherals/devices. The PRUSS INTC platform driver manages this PRUSS interrupt controller and implements an irqchip driver to provide a Linux standard way for the PRU client users to enable/disable/ack/re-trigger a PRUSS system event. The system events to interrupt channels and output interrupts relies on the mapping configuration provided either through the PRU firmware blob (for interrupts routed to PRU cores) or via the PRU application's device tree node (for interrupt routed to the main CPU). In the first case the mappings will be programmed on PRU remoteproc driver demand (via irq_create_fwspec_mapping) during the boot of a PRU core and cleaned up after the PRU core is stopped. Reference counting is used to allow multiple system events to share a single channel and to allow multiple channels to share a single host event. The PRUSS INTC module is reference counted during the interrupt setup phase through the irqchip's irq_request_resources() and irq_release_resources() ops. This restricts the module from being removed as long as there are active interrupt users. The driver currently supports and can be built for OMAP architecture based AM335x, AM437x and AM57xx SoCs; Keystone2 architecture based 66AK2G SoCs and Davinci architecture based OMAP-L13x/AM18x/DA850 SoCs. All of these SoCs support 64 system events, 10 interrupt channels and 10 output interrupt lines per PRUSS INTC with a few SoC integration differences. NOTE: Each PRU-ICSS's INTC on AM57xx SoCs is preceded by a Crossbar that enables multiple external events to be routed to a specific number of input interrupt events. Any non-default external interrupt event directed towards PRUSS needs this crossbar to be setup properly. Signed-off-by: Suman Anna Signed-off-by: Andrew F. Davis Signed-off-by: Roger Quadros Signed-off-by: Grzegorz Jaszczyk --- It looks like this patch also includes code that I wrote [1] so: Signed-off-by: David Lechner [1]: https://lore.kernel.org/lkml/124b03b8-f8e7-682b-8767-13a739329...@lechnology.com/ diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c new file mode 100644 index 000..45b966a --- /dev/null +++ b/drivers/irqchip/irq-pruss-intc.c @@ -0,0 +1,591 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * PRU-ICSS INTC IRQChip driver for various TI SoCs + * + * Copyright (C) 2016-2020 Texas Instruments Incorporated - http://www.ti.com/ + * Andrew F. Davis + * Suman Anna Please add: + * Copyright (C) 2019 David Lechner + */ + +#include +#include +#include +#include +#include +#include + +/* + * Number of host interrupts reaching the main MPU sub-system. Note that this + * is not the same as the total number of host interrupts supported by the PRUSS + * INTC instance + */ +#define MAX_NUM_HOST_IRQS 8 + +/* minimum starting host interrupt number for MPU */ +#define MIN_PRU_HOST_INT 2 nit: "First" might be a better word choice than "minimum" + +/* PRU_ICSS_INTC registers */ +#define PRU_INTC_REVID 0x +#define PRU_INTC_CR0x0004 +#define PRU_INTC_GER 0x0010 +#define PRU_INTC_GNLR 0x001c +#define PRU_INTC_SISR 0x0020 +#define PRU_INTC_SICR 0x0024 +#define PRU_INTC_EISR 0x0028 +#define PRU_INTC_EICR 0x002c +#define PRU_INTC_HIEISR0x0034 +#define PRU_INTC_HIDISR0x0038 +#define PRU_INTC_GPIR 0x0080 +#define PRU_INTC_SRSR(x) (0x0200 + (x) * 4) +#define PRU_INTC_SECR(x) (0x0280 + (x) * 4) +#define PRU_INTC_ESR(x)(0x0300 + (x) * 4) +#define PRU_INTC_ECR(x)(0x0380 + (x) * 4) +#define PRU_INTC_CMR(x)(0x0400 + (x) * 4) +#define PRU_INTC_HMR(x)(0x0800 + (x) * 4) +#define PRU_INTC_HIPIR(x) (0x0900 + (x) * 4) +#define PRU_INTC_SIPR(x) (0x0d00 + (x) * 4) +#define PRU_INTC_SITR(x) (0x0d80 + (x) * 4) +#define PRU_INTC_HINLR(x) (0x1100 + (x) * 4) +#define PRU_INTC_HIER 0x1500 + +/* CMR register bit-field macros */ +#define CMR_EVT_MAP_MASK 0xf +#define CMR_EVT_MAP_BITS 8 +#define CMR_EVT_PER_REG4 + +/* HMR register bit-field macros */ +#define HMR_CH_MAP_MASK0xf +#de
Re: [PATCH v4 1/5] dt-bindings: irqchip: Add PRU-ICSS interrupt controller bindings
On 7/28/20 4:18 AM, Grzegorz Jaszczyk wrote: From: Suman Anna The Programmable Real-Time Unit and Industrial Communication Subsystem (PRU-ICSS or simply PRUSS) contains an interrupt controller (INTC) that can handle various system input events and post interrupts back to the device-level initiators. The INTC can support upto 64 input events on nit: "up to" is two separate words most SoCs with individual control configuration and h/w prioritization. These events are mapped onto 10 output interrupt lines through two levels of many-to-one mapping support. Different interrupt lines are routed to the individual PRU cores or to the host CPU or to other PRUSS instances. The K3 AM65x and J721E SoCs have the next generation of the PRU-ICSS IP, commonly called ICSSG. The ICSSG interrupt controller on K3 SoCs provide a higher number of host interrupts (20 vs 10) and can handle an increased number of input events (160 vs 64) from various SoC interrupt sources. Add the bindings document for these interrupt controllers on all the applicable SoCs. It covers the OMAP architecture SoCs - AM33xx, AM437x and AM57xx; the Keystone 2 architecture based 66AK2G SoC; the Davinci architecture based OMAPL138 SoCs, and the K3 architecture based AM65x and J721E SoCs. Signed-off-by: Suman Anna Signed-off-by: Andrew F. Davis Signed-off-by: Roger Quadros Signed-off-by: Grzegorz Jaszczyk --- v3->v4: - Drop allOf references to interrupt-controller.yaml and interrupts.yaml. - Drop items descriptions and use only maxItems: 1 as suggested by Rob. - Convert irqs-reserved property from uint8-array to bitmask. - Minor descriptions updates. - Change interrupt-cells to 3 in order to provide 2-level mapping description for interrupts routed to the main CPU (as Marc requested). - Merge the irqs-reserved and irqs-shared to one property since they can be handled by one logic. - Drop reviewed-by due to introduced changes. - Add another example illustrating irqs-reserved property usage. v2->v3: - Convert dt-binding to YAML v1->v2: - https://patchwork.kernel.org/patch/11069767/ --- .../interrupt-controller/ti,pruss-intc.yaml| 157 + 1 file changed, 157 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ti,pruss-intc.yaml diff --git a/Documentation/devicetree/bindings/interrupt-controller/ti,pruss-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/ti,pruss-intc.yaml new file mode 100644 index 000..7336b11 --- /dev/null +++ b/Documentation/devicetree/bindings/interrupt-controller/ti,pruss-intc.yaml @@ -0,0 +1,157 @@ +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/interrupt-controller/ti,pruss-intc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: TI PRU-ICSS Local Interrupt Controller + +maintainers: + - Suman Anna + +description: | + Each PRU-ICSS has a single interrupt controller instance that is common + to all the PRU cores. Most interrupt controllers can route 64 input events + which are then mapped to 10 possible output interrupts through two levels + of mapping. The input events can be triggered by either the PRUs and/or + various other PRUSS internal and external peripherals. The first 2 output + interrupts (0, 1) are fed exclusively to the internal PRU cores, with the + remaining 8 (2 through 9) connected to external interrupt controllers + including the MPU and/or other PRUSS instances, DSPs or devices. + + The property "ti,irqs-reserved" is used for denoting the connection + differences on the output interrupts 2 through 9. If this property is not + defined, it implies that all the PRUSS INTC output interrupts 2 through 9 + (host_intr0 through host_intr7) are connected exclusively to the Arm interrupt + controller. + + The K3 family of SoCs can handle 160 input events that can be mapped to 20 + different possible output interrupts. The additional output interrupts (10 + through 19) are connected to new sub-modules within the ICSSG instances. + + This interrupt-controller node should be defined as a child node of the + corresponding PRUSS node. The node should be named "interrupt-controller". + +properties: + compatible: +enum: + - ti,pruss-intc + - ti,icssg-intc +description: | + Use "ti,pruss-intc" for OMAP-L13x/AM18x/DA850 SoCs, + AM335x family of SoCs, + AM437x family of SoCs, + AM57xx family of SoCs + 66AK2G family of SoCs + Use "ti,icssg-intc" for K3 AM65x & J721E family of SoCs + + reg: +maxItems: 1 + + interrupts: +minItems: 1 +maxItems: 8 +description: | + All the interrupts generated towards the main host processor in the SoC. + A shared interrupt can be skipped if the desired destination and usage is + by a different processor/device.
Re: [PATCH v4 3/5] counter: Add character device interface
.component.owner_id = 1,
.component.type = COUNTER_COMPONENT_TYPE_COUNT,
},
};
int main(void)
{
struct pollfd pfd = { .events = POLLIN };
struct counter_event event_data[2];
pfd.fd = open("/dev/counter0", O_RDWR);
ioctl(pfd.fd, COUNTER_SET_WATCH_IOCTL, watches);
ioctl(pfd.fd, COUNTER_SET_WATCH_IOCTL, watches + 1);
What enables events? If an event is enabled for each of these ioctls,
then we have a race condition where events events from the first watch
can start to be queued before the second watch is added. So we would
have to flush the chardev first before polling, otherwise the assumption
that event_data[0] is owner_id=0 and event_data[1] is owner_id=1 is
not true.
This is also racy if we want to clear watches and set up new watches
at runtime. There would be a period of time where there were no watches
and we could miss events.
With my suggested changes of having fixed values per event and generic
events, we could just have a single ioctl to enable and disable events.
This would probably need to take an array of event descriptors as an
argument where event descriptors contain the component type/id and the
event to enable.
for (;;) {
poll(, 1, -1);
read(pfd.fd, event_data, sizeof(event_data));
printf("Timestamp 0: %llu\nCount 0: %llu\n"
"Timestamp 1: %llu\nCount 1: %llu\n",
(unsigned long long)event_data[0].timestamp,
(unsigned long long)event_data[0].value_u64,
(unsigned long long)event_data[1].timestamp,
(unsigned long long)event_data[1].value_u64);
}
return 0;
}
Cc: David Lechner
Cc: Gwendal Grignou
Signed-off-by: William Breathitt Gray
---
Re: [PATCH][next] clk: davinci: Use fallthrough pseudo-keyword
On 7/27/20 3:11 PM, Gustavo A. R. Silva wrote: Replace the existing /* fall through */ comments and its variants with the new pseudo-keyword macro fallthrough[1]. [1] https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through Signed-off-by: Gustavo A. R. Silva Reviewed-by: David Lechner
Re: [PATCH v3 3/4] counter: Add character device interface
On 6/27/20 1:17 PM, William Breathitt Gray wrote:
On Mon, Jun 22, 2020 at 09:08:48AM -0500, David Lechner wrote:
On 6/21/20 2:53 PM, William Breathitt Gray wrote:
For example, in the dual-axes positioning table scenario, a user
application would likely want to know the exact X and Y position at the
time of a given event -- that means an event should provide two Count
values (and possibly associated device flags) when it occurs. I'm not
sure yet how the struct counter_event should be defined in order to
support this; we will need to indicate the format of data as well as
provide the data itself. Perhaps, we can handle this by providing an
unique id field so that only a single datum (e.g. a single count value)
is provided via the value field, but subsequent struct counter_event
items share the same id so that the user knows that a particular datum
is part of a larger group of data for a specific event.
The timestamp could act as the "id" to correlate multiple values of a
single event.
Okay, I see how that can work. So the /dev/counterX character nodes
would return a stream of data structures that look something like this:
struct counter_event {
/**
* Best approximation of when event occurred in nanoseconds.
* Same timestamp value indicates data is part of same event.
*/
struct timeval time;
/**
* Type of event that triggered. This would correlate with the
* IRQ set up for the device.
*/
__u16 type;
/**
* Type of data represented by the value member. This enables
* the user to extract the right datatype from the value field.
*/
__u16 code;
/** The value recorded when the event fired. */
__u64 value;
};
In fact, this data structure looks a lot like struct input_event; would
it make sense to use that for this? I suppose we can't because we need
to support 64-bit value for our use cases.
Yes, since counter is its own subsystem, it makes sense to have its own
data types.
Userspace also requires a way to enable the events and configure them to
report the data it wants. So perhaps the following sysfs attributes
would accomplish such:
* /sys/bus/devices/counterX/eventY_enable:
Users can enable/disable event Y.
* /sys/bus/devices/counterX/eventY_config:
Data to get when event Y is triggered (e.g. Counts, extensions, etc.).
This is one of the questions I had too that I don't have a good answer to.
If we want to allow the use case of multiple "consumers" for a single
chardev where each consumer wants different events (e.g program X opens
/dev/counter0 and wants only events A and B while program Y opens the
same /dev/counter0 and wants only events A and C) then it would make
sense to use an ioctl for configuration of events so that each open
file descriptor could be configured differently.
But if we only want to allow one user of a counter, then configuring
via sysfs as you have suggested is probably fine.
I think I might make sense to us an ioctl in any case though if we are
going to use the same code values to configure the event.
Here's another concern for latency-sensitive applications: should we
handle writing data to the devices? While we have real-life examples of
latency-sensitive read operations, I'm not sure if a user will ever need
to write to a counter device within some realtime critical deadline --
I think write operations are primarily done for the purpose of
configuring the device for operation rather than during it. So perhaps
we don't need to worry about this use case because users can write data
via the existing sysfs interface.
Agreed.
Re: [PATCH v3 3/4] counter: Add character device interface
On 6/21/20 2:53 PM, William Breathitt Gray wrote: Synapses simply indicate a change in a Count value Ah, ok. I understand now that synapse is the wrong term for things like the change in direction event or error events. For example, in the dual-axes positioning table scenario, a user application would likely want to know the exact X and Y position at the time of a given event -- that means an event should provide two Count values (and possibly associated device flags) when it occurs. I'm not sure yet how the struct counter_event should be defined in order to support this; we will need to indicate the format of data as well as provide the data itself. Perhaps, we can handle this by providing an unique id field so that only a single datum (e.g. a single count value) is provided via the value field, but subsequent struct counter_event items share the same id so that the user knows that a particular datum is part of a larger group of data for a specific event. The timestamp could act as the "id" to correlate multiple values of a single event.
Re: [PATCH v3 3/4] counter: Add character device interface
On 6/16/20 8:40 PM, William Breathitt Gray wrote:
This patch introduces a character device interface for the Counter
subsystem. Device control is exposed through standard character device
read and write operations.
A /sys/bus/counter/devices/counterX/chrdev_format sysfs attribute is
introduced to expose the character device data format:
* Users may write to this sysfs attribute to select the components they
want to interface -- the layout can be determined as well from the
order. For example:
# echo "C0 C3 C2" > /sys/bus/counter/devices/counter0/chrdev_format
This would select Counts 0, 3, and 2 (in that order) to be available
in the /dev/counter0 node as a contiguous memory region.
You can select extensions in a similar fashion:
# echo "C4E2 S1E0" > /sys/bus/counter/devices/counter0/chrdev_format
This would select extension 2 from Count 4, and extension 0 from
Signal 1.
* Users may read from this chrdev_format sysfs attribute in order to see
the currently configured format of the character device.
* Users may perform read/write operations on the /dev/counterX node
directly; the layout of the data is what they user has configured via
the chrdev_format sysfs attribute. For example:
# echo "C0 C1 S0 S1" > /sys/bus/counter/devices/counter0/chrdev_format
Yields the following /dev/counter0 memory layout:
+-+--+--+--+
| Byte 0 - Byte 7 | Byte 8 - Byte 15 | Byte 16 | Byte 17 |
+-+--+--+--+
| Count 0 | Count 1 | Signal 0 | Signal 2 |
+-+--+--+--+
The number of bytes allotted for each component or extension is
determined by its respective data type: u8 will have 1 byte allotted,
u64 will have 8 bytes allotted, etc.
Instead of the proposed character device, I would really rather have one
that gives past events instead of the current state.
I have thought about some of the suggestions from previous version of
this patch series and I'm starting to think something similar to the
input and gpio subsystems would work fairly well.
There would have to be a fixed size event data structure:
struct counter_event {
/** Best approximation of when event occurred in nanoseconds. */
__u64 timestamp;
/**
* Description of the synapse that triggered the event and the
* signal/counter that the value represents.
*/
__u64 descriptor;
/** The signal/counter value recorded when the synapse fired. */
__u64 value;
};
The descriptor field would actually probably be a union of __u64 and a
struct with its own fields to describe the synapse and signal or count.
If a synapse involves more than one signal or count, then there would
be multiple events with identical timestamps.
Userspace programs should be able to enable only the events/synapses they
are interested in and then the poll() the character device to wait for
events in an efficient way instead of having to constantly read - which
could still miss events.
---
Real world use case - measuring the speed of a motor:
At low speeds it is more accurate to measure the time difference between
count events. In this case we would want events from two synapses. One
triggered by the rising and falling edges of signal A and one triggered
by the direction signal. The magnitude of the speed is calculated by
taking the difference in timestamps between signal A events and the +/-
sign is determined by the direction signal.
At high speeds a different configuration is needed. Assuming the counter
has a timer clock signal a synapse would be configured to fire every 10
or 20 milliseconds. This would trigger an event that contains the count.
The speed is calculated as the difference in counts divided by the fixed
time interval.
Some applications may need to do both and be able to change the
configuration at runtime. It may start out in the low speed configuration,
but as the speed increases, events triggered by the change in count will
need to be disabled to prevent being overwhelmed with too many count
events. But if the speed drops low again, the count events will need to
be turned back on.
---
Regarding the implementation, the character device can be backed by a
kfifo. Interrupts from the counter hardware push events to the kfifo
and reading from the character device drains the kfifo.
drivers/gpio/gpiolib.c could be a good example to follow.
If we only want to allow one consumer to open the chardev at a time,
then enabling/disabling events via sysfs would probably be fine since
we are already sort of doing that anyway to enable/disable counters.
But if we need to allow multiple consumers per chardev that would each
want different events, then configuring via ioctl would be required so
that per-file descriptor configuration could be done (each call to open()
would create a
Re: [PATCH v2 0/4] Introduce the Counter character device interface
On 5/31/20 12:14 PM, William Breathitt Gray wrote: Yielding the following /dev/counter0 memory layout: ++-++---+ | Byte 0 | Byte 1 - Byte 8 | Byte 9 | Byte 10 - Byte 17 | ++-++---+ | Boundary 0 | Count 0 | Boundary 1 | Count 1 | ++-++---+ A potential pitfall with this sort of packing is that some platforms do not support unaligned access, so data would have to be "unpacked" before it could be used.
Re: [PATCH 0/4] Introduce the Counter character device interface
On 4/29/20 1:11 PM, William Breathitt Gray wrote: Over the past couple years we have noticed some shortcomings with the Counter sysfs interface. Although useful in the majority of situations, there are certain use-cases where interacting through sysfs attributes can become cumbersome and inefficient. A desire to support more advanced functionality such as timestamps, multi-axis positioning tables, and other such latency-sensitive applications, has motivated a reevaluation of the Counter subsystem. I believe a character device interface will be helpful for this more niche area of counter device use. Nice to see some progress being made. :-) Interaction with Counter character devices occurs via ioctl commands. This allows userspace applications to access and set counter data using native C datatypes rather than working through string translations. For most aspects of the counter subsystem, this is not an issue since configuring a counter is not a time-sensitive operation. Instead of ioctls, I was expecting to just be able to read the character device and receive counter events or poll to wait for events similar to how the input subsystem works or how buffers work in the iio subsystem. I'm afraid I don't really see much use in having ioctls that do exactly what sysfs already does. And my intuition tells me that the extra work needed to maintain it will probably cost more than any benefit gained. (Maybe other have a different experience that leads to a different conclusion?)
Re: [PATCH v4 2/2] docs: driver-api: generic-counter: Update Count and Signal data types
On 10/6/19 11:03 AM, William Breathitt Gray wrote: Count data is now always represented as an unsigned integer, while Signal data is either SIGNAL_LOW or SIGNAL_HIGH. Signed-off-by: William Breathitt Gray --- Documentation/driver-api/generic-counter.rst | 22 +++- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/Documentation/driver-api/generic-counter.rst b/Documentation/driver-api/generic-counter.rst index 8382f01a53e3..161652fc1025 100644 --- a/Documentation/driver-api/generic-counter.rst +++ b/Documentation/driver-api/generic-counter.rst @@ -39,10 +39,7 @@ There are three core components to a counter: COUNT - A Count represents the count data for a set of Signals. The Generic -Counter interface provides the following available count data types: - -* COUNT_POSITION: - Unsigned integer value representing position. +Counter interface represents the count data as an unsigned integer. The previous patch looks like it is using unsigned long instead of unsigned integer.
Re: [PATCH v4 1/2] counter: Simplify the count_read and count_write callbacks
On 10/6/19 11:03 AM, William Breathitt Gray wrote: The count_read and count_write callbacks are simplified to pass val as unsigned long rather than as an opaque data structure. The opaque counter_count_read_value and counter_count_write_value structures, counter_count_value_type enum, and relevant counter_count_read_value_set and counter_count_write_value_get functions, are removed as they are no longer used. Cc: David Lechner Cc: Patrick Havelange Acked-by: Fabrice Gasnier Signed-off-by: William Breathitt Gray --- Acked-by: David Lechner
Re: [PATCH v3 0/6] counter: new TI eQEP driver
On 9/1/19 5:58 PM, David Lechner wrote: This series adds device tree bindings and a new counter driver for the Texas Instruments Enhanced Quadrature Encoder Pulse (eQEP). ... David Lechner (6): bus/ti-pwmss: move TI PWMSS driver from PWM to bus subsystem dt-bindings: counter: new bindings for TI eQEP counter: new TI eQEP driver ARM: dts: am33xx: Add nodes for eQEP ARM: dts: am335x-boneblue: Enable eQEP ARM: dts: am335x-boneblue: Use of am335x-osd335x-common.dtsi In case anyone notices, this series only has 5 patches, not 6. "ARM: dts: am335x-boneblue: Use of am335x-osd335x-common.dtsi" is unrelated and was submitted separately.
Re: [PATCH] counter: fix devm_platform_ioremap_resource.cocci warnings
On 8/9/19 4:41 AM, Julia Lawall wrote:
From: kbuild test robot
Use devm_platform_ioremap_resource helper which wraps
platform_get_resource() and devm_ioremap_resource() together.
Generated by: scripts/coccinelle/api/devm_platform_ioremap_resource.cocci
Fixes: 78958c294246 ("counter: new TI eQEP driver")
Signed-off-by: kbuild test robot
Signed-off-by: Julia Lawall
---
Included this change in v2 of the "counter: new TI eQEP driver" series.
Thanks.
[PATCH v3 2/6] dt-bindings: counter: new bindings for TI eQEP
This documents device tree binding for the Texas Instruments Enhanced
Quadrature Encoder Pulse (eQEP) Module found in various TI SoCs.
Signed-off-by: David Lechner
---
v3 changes:
- fixed style issues
- fixed generic node name
- (was suggested to drop descriptions since there is only one interrupt and one
clock, but I opted to keep them anyway)
v2 changes:
- convert to .yaml format
- rename clock to "sysclkout"
.../devicetree/bindings/counter/ti-eqep.yaml | 50 +++
1 file changed, 50 insertions(+)
create mode 100644 Documentation/devicetree/bindings/counter/ti-eqep.yaml
diff --git a/Documentation/devicetree/bindings/counter/ti-eqep.yaml
b/Documentation/devicetree/bindings/counter/ti-eqep.yaml
new file mode 100644
index ..85f1ff83afe7
--- /dev/null
+++ b/Documentation/devicetree/bindings/counter/ti-eqep.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/counter/ti-eqep.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments Enhanced Quadrature Encoder Pulse (eQEP) Module
+
+maintainers:
+ - David Lechner
+
+properties:
+ compatible:
+const: ti,am3352-eqep
+
+ reg:
+maxItems: 1
+
+ interrupts:
+description: The eQEP event interrupt
+maxItems: 1
+
+ clocks:
+description: The clock that determines the SYSCLKOUT rate for the eQEP
+ peripheral.
+maxItems: 1
+
+ clock-names:
+const: sysclkout
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - clock-names
+
+additionalProperties: false
+
+examples:
+ - |
+eqep0: counter@180 {
+compatible = "ti,am3352-eqep";
+reg = <0x180 0x80>;
+clocks = <_gclk>;
+clock-names = "sysclkout";
+interrupts = <79>;
+};
+
+...
--
2.17.1
[PATCH v3 1/6] bus/ti-pwmss: move TI PWMSS driver from PWM to bus subsystem
The TI PWMSS driver is a simple bus driver for providing power
power management for the PWM peripherals on TI AM33xx SoCs, namely
eCAP, eHRPWM and eQEP. The eQEP is a counter rather than a PWM, so
it does not make sense to have the bus driver in the PWM subsystem
since the PWMSS is not exclusive to PWM devices.
Signed-off-by: David Lechner
---
v3 changes:
- none
v2 changes:
- new patch
drivers/bus/Kconfig | 9 +
drivers/bus/Makefile | 1 +
drivers/{pwm/pwm-tipwmss.c => bus/ti-pwmss.c} | 0
drivers/pwm/Kconfig | 9 -
drivers/pwm/Makefile | 1 -
5 files changed, 10 insertions(+), 10 deletions(-)
rename drivers/{pwm/pwm-tipwmss.c => bus/ti-pwmss.c} (100%)
diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 1851112ccc29..4eeb15839ce0 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -140,6 +140,15 @@ config TEGRA_GMI
Driver for the Tegra Generic Memory Interface bus which can be used
to attach devices such as NOR, UART, FPGA and more.
+config TI_PWMSS
+ bool
+ default y if (ARCH_OMAP2PLUS) && (PWM_TIECAP || PWM_TIEHRPWM)
+ help
+ PWM Subsystem driver support for AM33xx SOC.
+
+ PWM submodules require PWM config space access from submodule
+ drivers and require common parent driver support.
+
config TI_SYSC
bool "TI sysc interconnect target module driver"
depends on ARCH_OMAP2PLUS
diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index ca300b1914ce..a2d13cf4a877 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_SUNXI_RSB) += sunxi-rsb.o
obj-$(CONFIG_SIMPLE_PM_BUS)+= simple-pm-bus.o
obj-$(CONFIG_TEGRA_ACONNECT) += tegra-aconnect.o
obj-$(CONFIG_TEGRA_GMI)+= tegra-gmi.o
+obj-$(CONFIG_TI_PWMSS) += ti-pwmss.o
obj-$(CONFIG_TI_SYSC) += ti-sysc.o
obj-$(CONFIG_TS_NBUS) += ts-nbus.o
obj-$(CONFIG_UNIPHIER_SYSTEM_BUS) += uniphier-system-bus.o
diff --git a/drivers/pwm/pwm-tipwmss.c b/drivers/bus/ti-pwmss.c
similarity index 100%
rename from drivers/pwm/pwm-tipwmss.c
rename to drivers/bus/ti-pwmss.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index a7e57516959e..300396564769 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -497,15 +497,6 @@ config PWM_TIEHRPWM
To compile this driver as a module, choose M here: the module
will be called pwm-tiehrpwm.
-config PWM_TIPWMSS
- bool
- default y if (ARCH_OMAP2PLUS) && (PWM_TIECAP || PWM_TIEHRPWM)
- help
- PWM Subsystem driver support for AM33xx SOC.
-
- PWM submodules require PWM config space access from submodule
- drivers and require common parent driver support.
-
config PWM_TWL
tristate "TWL4030/6030 PWM support"
depends on TWL4030_CORE
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 76b555b51887..f67eb6e9294d 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -49,7 +49,6 @@ obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o
obj-$(CONFIG_PWM_TEGRA)+= pwm-tegra.o
obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
-obj-$(CONFIG_PWM_TIPWMSS) += pwm-tipwmss.o
obj-$(CONFIG_PWM_TWL) += pwm-twl.o
obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.o
obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o
--
2.17.1
[PATCH v3 0/6] counter: new TI eQEP driver
This series adds device tree bindings and a new counter driver for the Texas
Instruments Enhanced Quadrature Encoder Pulse (eQEP).
As mentioned in one of the commit messages, to start with, the driver only
supports reading the current counter value and setting the min/max values.
Other features can be added as the counter subsystem gains support for them.
v3 changes:
- Minor changes to device tree bindings (style and generic node name)
- Drop action in initializer
- Fix ordering of pm runtime disable
v2 changes:
- New patch to move TI PWMSS driver from drivers/pwm/ to drivers/bus/
- Device tree bindings converted to .yaml format
- Device tree clock renamed from "fck" to "sysclkout"
- Dropped unused index and strobe signals from counter driver
- Added synapses and actions to counter driver
- Fixed base in of kstrtouint()
- Clarifications in commit messages
This series has been tested on a BeagleBone Blue with the following script:
#!/usr/bin/env python3
from os import path
from time import sleep
COUNTER_PATH = '/sys/bus/counter/devices'
COUNTERS = ['counter0', 'counter1', 'counter2']
COUNT0 = 'count0'
COUNT = 'count'
FUNCTION = 'function'
CEILING = 'ceiling'
FLOOR = 'floor'
ENABLE = 'enable'
cnts = []
for c in COUNTERS:
function_path = path.join(COUNTER_PATH, c, COUNT0, FUNCTION)
with open(function_path, 'w') as f:
f.write('quadrature x4')
floor_path = path.join(COUNTER_PATH, c, COUNT0, FLOOR)
with open(floor_path, 'w') as f:
f.write(str(0))
ceiling_path = path.join(COUNTER_PATH, c, COUNT0, CEILING)
with open(ceiling_path, 'w') as f:
f.write(str(0x))
enable_path = path.join(COUNTER_PATH, c, COUNT0, ENABLE)
with open(enable_path, 'w') as f:
f.write('1')
cnt_path = path.join(COUNTER_PATH, c, COUNT0, COUNT)
cnts.append(open(cnt_path, 'r'))
while True:
for c in cnts:
c.seek(0)
val = int(c.read())
if val >= 0x8000:
val -= 0x1
print(val, end=' ')
print()
sleep(1)
David Lechner (6):
bus/ti-pwmss: move TI PWMSS driver from PWM to bus subsystem
dt-bindings: counter: new bindings for TI eQEP
counter: new TI eQEP driver
ARM: dts: am33xx: Add nodes for eQEP
ARM: dts: am335x-boneblue: Enable eQEP
ARM: dts: am335x-boneblue: Use of am335x-osd335x-common.dtsi
.../devicetree/bindings/counter/ti-eqep.yaml | 50 ++
MAINTAINERS | 6 +
arch/arm/boot/dts/am335x-boneblue.dts | 146 +++---
arch/arm/boot/dts/am33xx-l4.dtsi | 27 +
drivers/bus/Kconfig | 9 +
drivers/bus/Makefile | 1 +
drivers/{pwm/pwm-tipwmss.c => bus/ti-pwmss.c} | 0
drivers/counter/Kconfig | 11 +
drivers/counter/Makefile | 1 +
drivers/counter/ti-eqep.c | 473 ++
drivers/pwm/Kconfig | 9 -
drivers/pwm/Makefile | 1 -
12 files changed, 634 insertions(+), 100 deletions(-)
create mode 100644 Documentation/devicetree/bindings/counter/ti-eqep.yaml
rename drivers/{pwm/pwm-tipwmss.c => bus/ti-pwmss.c} (100%)
create mode 100644 drivers/counter/ti-eqep.c
--
2.17.1
[PATCH v3 5/6] ARM: dts: am335x-boneblue: Enable eQEP
This enables the Enhanced Quadrature Encoder Pulse (eQEP) module for
connectors E1, E2 and E3 on BeagleBone Blue.
Signed-off-by: David Lechner
---
v3 changes:
- none
v2 changes:
- none
arch/arm/boot/dts/am335x-boneblue.dts | 54 +++
1 file changed, 54 insertions(+)
diff --git a/arch/arm/boot/dts/am335x-boneblue.dts
b/arch/arm/boot/dts/am335x-boneblue.dts
index 0257576d5d16..df3978ce061c 100644
--- a/arch/arm/boot/dts/am335x-boneblue.dts
+++ b/arch/arm/boot/dts/am335x-boneblue.dts
@@ -258,6 +258,30 @@
AM33XX_PADCONF(AM335X_PIN_MII1_RXD0, PIN_OUTPUT,
MUX_MODE7) /* (M16) gmii1_rxd0.gpio2[21] */
>;
};
+
+ /* E1 */
+ eqep0_pins: pinmux_eqep0_pins {
+ pinctrl-single,pins = <
+ AM33XX_PADCONF(AM335X_PIN_MCASP0_AXR0, PIN_INPUT,
MUX_MODE1)/* (B12) mcasp0_aclkr.eQEP0A_in */
+ AM33XX_PADCONF(AM335X_PIN_MCASP0_FSR, PIN_INPUT,
MUX_MODE1) /* (C13) mcasp0_fsr.eQEP0B_in */
+ >;
+ };
+
+ /* E2 */
+ eqep1_pins: pinmux_eqep1_pins {
+ pinctrl-single,pins = <
+ AM33XX_PADCONF(AM335X_PIN_LCD_DATA12, PIN_INPUT,
MUX_MODE2) /* (V2) lcd_data12.eQEP1A_in */
+ AM33XX_PADCONF(AM335X_PIN_LCD_DATA13, PIN_INPUT,
MUX_MODE2) /* (V3) lcd_data13.eQEP1B_in */
+ >;
+ };
+
+ /* E3 */
+ eqep2_pins: pinmux_eqep2_pins {
+ pinctrl-single,pins = <
+ AM33XX_PADCONF(AM335X_PIN_GPMC_AD12, PIN_INPUT,
MUX_MODE4) /* (T12) gpmc_ad12.eQEP2A_in */
+ AM33XX_PADCONF(AM335X_PIN_GPMC_AD13, PIN_INPUT,
MUX_MODE4) /* (R12) gpmc_ad13.eQEP2B_in */
+ >;
+ };
};
{
@@ -530,3 +554,33 @@
line-name = "LS_BUF_EN";
};
};
+
+ {
+ status = "okay";
+};
+
+ {
+ status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <_pins>;
+};
+
+ {
+ status = "okay";
+};
+
+ {
+ status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <_pins>;
+};
+
+ {
+ status = "okay";
+};
+
+ {
+ status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <_pins>;
+};
--
2.17.1
