| Message ID | 20210716144508.6058-2-viktor.prutyanov@phystech.edu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: rc: add support for Amlogic Meson IR blaster | expand |
Hi Viktor, On Fri, Jul 16, 2021 at 4:45 PM Viktor Prutyanov <viktor.prutyanov@phystech.edu> wrote: [...] > + amlogic,fifo-threshold: > + description: TX FIFO threshold > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 0 > + maximum: 127 I tried to make sense of this property but I don't understand it yet (even after reading the second patch in this series). A "FIFO" (in my own words) is some physical property of the IR transmitter in these Amlogic SoCs. So for one specific SoC there can only be one FIFO size, not a range (0..127). What about a value of 0: my understanding is that this means that there's no FIFO. Will this hardware still work in that case? From reading the driver code it seems to me that the FIFO size is 128. The driver can use fewer FIFO entries if it wants, but this must not affect the dt-bindings (as these describe the hardware - they don't "configure" the driver). If you look at arch/arm64/boot/dts/amlogic/meson-g12.dtsi you'll find toddr_a, toddr_b and toddr_c there: All three of them use identical circuitry internally, except that toddr_a has a bigger FIFO size than the other two. Using a FIFO size of 256 for toddr_a is not correct as it's an improper description of the toddr_a hardware (that said, the driver can still decide that it only wants to use 256 FIFO entries). Best regards, Martin
Hi Martin, (just reading this patch out of passing curiosity...) On 2021-07-17 20:57, Martin Blumenstingl wrote: > Hi Viktor, > > On Fri, Jul 16, 2021 at 4:45 PM Viktor Prutyanov > <viktor.prutyanov@phystech.edu> wrote: > [...] >> + amlogic,fifo-threshold: >> + description: TX FIFO threshold >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + minimum: 0 >> + maximum: 127 > I tried to make sense of this property but I don't understand it yet > (even after reading the second patch in this series). > A "FIFO" (in my own words) is some physical property of the IR > transmitter in these Amlogic SoCs. > So for one specific SoC there can only be one FIFO size, not a range (0..127). > > What about a value of 0: my understanding is that this means that > there's no FIFO. Will this hardware still work in that case? > > From reading the driver code it seems to me that the FIFO size is 128. > The driver can use fewer FIFO entries if it wants, but this must not > affect the dt-bindings (as these describe the hardware - they don't > "configure" the driver). > If you look at arch/arm64/boot/dts/amlogic/meson-g12.dtsi you'll find > toddr_a, toddr_b and toddr_c there: > All three of them use identical circuitry internally, except that > toddr_a has a bigger FIFO size than the other two. Using a FIFO size > of 256 for toddr_a is not correct as it's an improper description of > the toddr_a hardware (that said, the driver can still decide that it > only wants to use 256 FIFO entries). In general, a FIFO threshold is not about how much of the FIFO you use overall, but how often and/or urgently you tend to it. If the only thing that matters is minimising CPU overhead then the optimum choice is to wait until the FIFO is entirely full/empty before taking action to drain/refill it. However, that necessarily creates a pause in reception/transmission for the time it takes to respond to the empty/full interrupt, hence why many FIFOs also implement a threshold interrupt for cases when uninterrupted communication is more desirable than absolutely minimising interrupts. Typically those are set to fire at some point shortly *before* the FIFO becomes completely full/empty, to leave enough remaining buffer for communication to continue during that time window until the ISR actually gets to respond. That said, I'm also doubtful about this particular case. If the physical FIFO depth does actually vary between SoCs, that should be known by the driver and implicit in the compatible string, definitely not hidden in a tangential property. Otherwise, it's not apparent how this makes sense to configure statically on a per-SoC or per-board basis. If anything it would depend on the transmission rate of whatever IR protocol the user wishes to use at any given time AFAICS. If it's not sufficient for the driver to simply assume, say, an 80% threshold as "good enough", then presumably it has enough information about the clock rate and/or the parameters of the given Tx request to implement a slightly cleverer heuristic. If it's desirable to tweak the specific driver behaviour in cases where the user does know better, then by all means make that a module parameter, but it's not something which belongs in DT. Cheers, Robin.
diff --git a/Documentation/devicetree/bindings/media/amlogic,meson-ir-tx.yaml b/Documentation/devicetree/bindings/media/amlogic,meson-ir-tx.yaml new file mode 100644 index 000000000000..88655413495d --- /dev/null +++ b/Documentation/devicetree/bindings/media/amlogic,meson-ir-tx.yaml @@ -0,0 +1,67 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) + +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/media/amlogic,meson-ir-tx.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#" + +title: Amlogic Meson IR transmitter + +maintainers: + - Viktor Prutyanov <viktor.prutyanov@phystech.edu> + +description: | + Some Amlogic SoCs such as A311D and T950D4 have IR transmitter + (also called blaster) controller onboard. It is capable of + sending IR signals with arbitrary carrier frequency and duty cycle. + +properties: + compatible: + oneOf: + - const: amlogic,meson-ir-tx + - items: + - const: amlogic,meson-g12a-ir-tx + - const: amlogic,meson-ir-tx + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clocks: + maxItems: 2 + + clock-names: + items: + - const: sysclk + - const: xtal + + amlogic,fifo-threshold: + description: TX FIFO threshold + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 0 + maximum: 127 + +required: + - compatible + - reg + - interrupts + - clocks + - clock-names + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/clock/g12a-clkc.h> + + ir@ff80014c { + compatible = "amlogic,meson-g12a-ir-tx", "amlogic,meson-ir-tx"; + reg = <0xff80014c 0x10>; + interrupts = <0 198 IRQ_TYPE_EDGE_RISING>; + clocks = <&clkc CLKID_CLK81>, <&xtal>; + clock-names = "sysclk", "xtal"; + amlogic,fifo-threshold = <32>; + };
This patch adds binding documentation for the IR transmitter available in Amlogic Meson SoCs. Signed-off-by: Viktor Prutyanov <viktor.prutyanov@phystech.edu> --- changes in v2: - compatible = "amlogic,meson-g12a-irblaster" added - clocks, clock-names and mod-clock updated changes in v3: - mod-clock removed - max-fifo-level added changes in v4: - irblaster -> ir-tx renaming changes in v5: - max-fifo-level -> amlogic,fifo-threshold (fifo-threshold == 128 - max-fifo-level) - amlogic,fifo-threshold becomes uint32 in range [0; 127] no changes in v6 .../bindings/media/amlogic,meson-ir-tx.yaml | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/amlogic,meson-ir-tx.yaml