Re: [PATCH v2 1/7] dt-bindings: misc: Add the Lantiq PEF2466 E1/T1/J1 framer

Hi Krzysztof

On Fri, 17 Mar 2023 09:54:07 +0100 Krzysztof Kozlowski krzysztof.kozlowski@linaro.org wrote:

On 16/03/2023 13:27, Herve Codina wrote:

...

The Lantiq PEF2256 is a framer and line interface component designed to fulfill all required interfacing between an analog E1/T1/J1 line and the digital PCM system highway/H.100 bus.

Signed-off-by: Herve Codina herve.codina@bootlin.com

.../bindings/misc/lantiq,pef2256.yaml | 190 ++++++++++++++++++ 1 file changed, 190 insertions(+) create mode 100644 Documentation/devicetree/bindings/misc/lantiq,pef2256.yaml

diff --git a/Documentation/devicetree/bindings/misc/lantiq,pef2256.yaml b/Documentation/devicetree/bindings/misc/lantiq,pef2256.yaml new file mode 100644 index 000000000000..1ba788d06a14 --- /dev/null +++ b/Documentation/devicetree/bindings/misc/lantiq,pef2256.yaml @@ -0,0 +1,190 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/misc/lantiq,pef2256.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#

+title: Lantiq PEF2256

+maintainers:

• • Herve Codina herve.codina@bootlin.com

+description:

• The Lantiq PEF2256, also known as Infineon PEF2256 or FALC256, is a framer and
• line interface component designed to fulfill all required interfacing between
• an analog E1/T1/J1 line and the digital PCM system highway/H.100 bus.

+properties:

• compatible:
• const: lantiq,pef2256
• reg:
• maxItems: 1
• clocks:
• items:

•  - description: Master clock
  

• clock-names:
• items:

•  - const: mclk
  

• interrupts:
• maxItems: 1
• reset-gpios:
• description:

•  GPIO used to reset the device.
  

• maxItems: 1
• pinctrl:
• allOf:

•  - $ref: "/schemas/pinctrl/pinctrl.yaml#"  
  

Drop quotes. Drop allOf, no need for it.

additionalProperties: false

Will be dropped and added in v3

...

• patternProperties:

•  '-pins$':
  

•    type: object
  

•    allOf:
  

•      - $ref: "/schemas/pinctrl/pincfg-node.yaml#"  
  

Drop quotes. Drop allOf, no need for it.

additionalProperties: false

Will be dropped and added in v3

...

•    properties:
  

•      pins:
  

•        enum: [ RPA, RPB, RPC, RPD, XPA, XPB, XPC, XPD ]
  

•      function:
  

•        enum: [ SYPR, RFM, RFMB, RSIGM, RSIG, DLR, FREEZE, RFSP, LOS,
  

•                SYPX, XFMS, XSIG, TCLK, XMFB, XSIGM, DLX, XCLK, XLT,
  

•                GPI, GPOH, GPOL ]
  

•    required:
  

•      - pins
  

•      - function
  

• lantiq,line-interface:
• $ref: /schemas/types.yaml#/definitions/string
• enum: [e1, t1j1]
• default: e1
• description: |

•  The line interface type
  

•    - e1: E1 line
  

•    - t1j1: T1/J1 line
  

• lantiq,sysclk-rate-hz:
• enum: [2048000, 4096000, 8192000, 16384000]
• default: 2048000
• description:

•  Clock rate (Hz) on the system highway.  
  

I am pretty sure we have discussions on sysclk for other drivers. First, why you cannot use assigned-clock-rates? Or clk_get_rate() if this is about being consumer?

Second, there is already system-clock-frequency property, so use it.

Indeed, I will added the related clocks in the 'clocks' property and use clk_get_rate() in the driver.

...

• lantiq,data-rate-bps:
• $ref: /schemas/types.yaml#/definitions/uint32
• enum: [2048000, 4096000, 8192000, 16384000]
• default: 2048000
• description:

•  Data rate (bit per seconds) on the system highway.  
  

Why do you need it? How is it different from clock? Do you expect some DDR here?

This is needed to set the data position on the data line. If the data line clock (sysclk-rate-hz) is greater than 'data-rate-bps', the device interleaves some holes between data in the full frame.

The exact position of the data and the holes is defined by 'channel-phase'

So, two information are needed: - The number of slots available (deduced from 'lantiq,data-rate-bps') - The slot to use in the available slots ('lantiq,channel-phase" property

lantiq,data-rate-bps is not a clock but a property used to set the frame physical setting. ie the correct data position in the frame.

...

• lantiq,clock-falling-edge:
• $ref: /schemas/types.yaml#/definitions/flag
• description:

•  Data is sent on falling edge of the clock (and received on the rising
  

•  edge). If 'clock-falling-edge' is not present, data is sent on the
  

•  rising edge (and received on the falling edge).
  

• lantiq,channel-phase:
• $ref: /schemas/types.yaml#/definitions/uint32
• enum: [0, 1, 2, 3, 4, 5, 6, 7]
• default: 0
• description:

•  The pef2256 delivers a full frame (32 8bit time-slots in E1 and 24 8bit
  

•  time-slots 8 8bit signaling in E1/J1) every 125us. This lead to a data
  

•  rate of 2048000 bit/s. When lantiq,data-rate-bps is more than 2048000
  

•  bit/s, the data (all 32 8bit) present in the frame are interleave with
  

•  unused time-slots. The lantiq,channel-phase property allows to set the
  

•  correct alignment of the interleave mechanism.
  

•  For instance, suppose lantiq,data-rate-bps = 8192000 (ie 4*2048000), and
  

•  lantiq,channel-phase = 2, the interleave schema with unused time-slots
  

•  (nu) and used time-slots (XX) for TSi is
  

•    nu nu XX nu nu nu XX nu nu nu XX nu
  

•    <-- TSi --> <- TSi+1 -> <- TSi+2 ->
  

•  With lantiq,data-rate-bps = 8192000, and lantiq,channel-phase = 1, the
  

•  interleave schema is
  

•    nu XX nu nu nu XX nu nu nu XX nu nu
  

•    <-- TSi --> <- TSi+1 -> <- TSi+2 ->
  

•  With lantiq,data-rate-bps = 4096000 (ie 2*2048000), and
  

•  lantiq,channel-phase = 1, the interleave schema is
  

•    nu    XX    nu    XX    nu    XX
  

•    <-- TSi --> <- TSi+1 -> <- TSi+2 ->
  

• lantiq,subordinate:
• $ref: /schemas/types.yaml#/definitions/flag
• description:

•  If present, the pef2256 works in subordinate mode. In this mode it
  

•  synchronizes on line interface clock signals. Otherwise, it synchronizes
  

•  on internal clocks.
  

+allOf:

• • if:

•  properties:
  

•    lantiq,line-interface:
  

•      contains:
  

•        const: e1
  

• then:

•  properties:
  

•    lantiq,frame-format:  
  

Do not define properties in if:then, but in top-level. Disallow them or customize for the specific cases in if:then

Will be changed in v3.

...

•      $ref: /schemas/types.yaml#/definitions/string
  

•      enum: [doubleframe, crc4-multiframe, auto-multiframe]
  

•      default: doubleframe
  

•      description: |
  

•        The E1 line interface frame format
  

•          - doubleframe: Doubleframe format
  

•          - crc4-multiframe: CRC4 multiframe format
  

•          - auto-multiframe: CRC4 multiframe format with interworking
  

•                             capabilities (ITU-T G.706 Annex B)
  

• else:

•  # T1/J1 line
  

•  properties:
  

•    lantiq,frame-format:  
  

Same problem - definitions go to top level.

Will be changed in v3

...

•      $ref: /schemas/types.yaml#/definitions/string
  

•      enum: [4frame, 12frame, 24frame, 72frame]
  

•      default: 12frame
  

•      description: |
  

•        The T1/J1 line interface frame format
  

•          - 4frame: 4-frame multiframe format (F4)
  

•          - 12frame: 12-frame multiframe format (F12, D3/4)
  

•          - 24frame: 24-frame multiframe format (ESF)
  

•          - 72frame: 72-frame multiframe format (F72, remote switch mode)
  

+required:

• • compatible
• • reg
• • clocks
• • clock-names
• • interrupts

+additionalProperties: false

+examples:

• • |
• #include <dt-bindings/gpio/gpio.h>
• pef2256@2000000 {

Figure out some generic node name.

What do you think about 'framer' ?

...

•  compatible = "lantiq,pef2256";
  

•  reg = <0x2000000 0xFF>;  
  

Lowercase hex

Will be changed in v3.

...

•  interrupts = <8 1>;  
  

if 1 is interrupt flag, use proper defines.

Will be replaced by 'interrupts = <8 IRQ_TYPE_LEVEL_LOW>' I will also change the interrupt-parent to interrupt-parent = <&intc> to avoid mentioning PIC and avoid any misunderstanding as the PIC we use does not follow the standard IRQ_TYPE_* flags.

...

•  interrupt-parent = <&PIC>;
  

•  clocks = <&clk_mclk>;
  

•  clock-names = "mclk";
  

•  reset-gpios = <&gpio 11 GPIO_ACTIVE_LOW>;
  

•  lantiq,sysclk-rate-hz = <8192000>;
  

•  lantiq,data-rate-bps = <4096000>;
  

•  pinctrl {
  

•    pef2256_rpa_sypr: rpa-pins {
  

•      pins = "RPA";
  

•      function = "SYPR";
  

•    };
  

•    pef2256_xpa_sypx: xpa-pins {
  

•      pins = "XPA";
  

•      function = "SYPX";
  

•    };
  

•  };
  

• };

Best regards, Krzysztof

Thanks for the review.

Best regards, Hervé