Re: [PATCH v3 1/2] dt-bindings: Add binding for Renesas 8T49N241

From: Alex Helms
Date: Mon Jul 12 2021 - 14:10:51 EST

On 7/12/2021 9:28 AM, Rob Herring wrote:
> On Wed, Jul 07, 2021 at 11:26:58AM -0700, Alex Helms wrote:
>> Renesas 8T49N241 has 4 outputs, 1 integral and 3 fractional dividers.
>> The 8T49N241 accepts up to two differential or single-ended input clocks
>> and a fundamental-mode crystal input. The internal PLL can lock to either
>> of the input reference clocks or to the crystal to behave as a frequency
>> synthesizer.
>>
>> Signed-off-by: Alex Helms <alexander.helms.jy@xxxxxxxxxxx>
>> ---
>> .../bindings/clock/renesas,8t49n241.yaml | 188 ++++++++++++++++++
>> MAINTAINERS | 6 +
>> 2 files changed, 194 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/clock/renesas,8t49n241.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/clock/renesas,8t49n241.yaml b/Documentation/devicetree/bindings/clock/renesas,8t49n241.yaml
>> new file mode 100644
>> index 000000000..4e26b3f11
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/renesas,8t49n241.yaml
>> @@ -0,0 +1,188 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fclock%2Frenesas%2C8t49n241.yaml%23&amp;data=04%7C01%7Calexander.helms.jy%40renesas.com%7Cb047bd6bed2448f744e708d945521084%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C1%7C637617041032675498%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=RYKX4RbZlT0YS3ka1oZ79%2BFTvUKtLUoEFkqMG6hLjYE%3D&amp;reserved=0
>> +$schema: https://jpn01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23&amp;data=04%7C01%7Calexander.helms.jy%40renesas.com%7Cb047bd6bed2448f744e708d945521084%7C53d82571da1947e49cb4625a166a4a2a%7C0%7C1%7C637617041032675498%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=qG%2F%2Bc%2BiaSBDT0pIP%2Bvk7jjplus1UZJrehllaRtsDGKI%3D&amp;reserved=0
>> +
>> +title: Binding for Renesas 8T49N241 Universal Frequency Translator
>> +
>> +description: |
>> + The 8T49N241 has one fractional-feedback PLL that can be used as a
>> + jitter attenuator and frequency translator. It is equipped with one
>> + integer and three fractional output dividers, allowing the generation
>> + of up to four different output frequencies, ranging from 8kHz to 1GHz.
>> + These frequencies are completely independent of each other, the input
>> + reference frequencies and the crystal reference frequency. The device
>> + places virtually no constraints on input to output frequency conversion,
>> + supporting all FEC rates, including the new revision of ITU-T
>> + Recommendation G.709 (2009), most with 0ppm conversion error.
>> + The outputs may select among LVPECL, LVDS, HCSL or LVCMOS output levels.
>> +
>> + The driver can read a full register map from the DT, and will use that
>> + register map to initialize the attached part (via I2C) when the system
>> + boots. Any configuration not supported by the common clock framework
>> + must be done via the full register map, including optimized settings.
>> +
>> + The 8T49N241 accepts up to two differential or single-ended input clocks
>> + and a fundamental-mode crystal input. The internal PLL can lock to either
>> + of the input reference clocks or just to the crystal to behave as a
>> + frequency synthesizer. The PLL can use the second input for redundant
>> + backup of the primary input reference, but in this case, both input clock
>> + references must be related in frequency.
>> +
>> + All outputs are currently assumed to be LVDS, unless overridden in the
>> + full register map in the DT.
>> +
>> +maintainers:
>> + - Alex Helms <alexander.helms.jy@xxxxxxxxxxx>
>> + - David Cater <david.cater.jc@xxxxxxxxxxx>
>> +
>> +properties:
>> + compatible:
>> + enum:
>> + - 8t49n241
>
> Needs a vendor prefix.
>

Will fix in next patch version.

>> +
>> + reg:
>> + description: I2C device address
>> + enum: [ 0x7c, 0x6c, 0x7d, 0x6d, 0x7e, 0x6e, 0x7f, 0x6f ]
>> +
>> + '#clock-cells':
>> + const: 1
>> +
>> + clock-names:
>> + description: Name of the input clock
>
> Drop. That's every 'clock-names'.
>

Will remove in next patch version.

>> + minItems: 1
>> + maxItems: 3
>> + items:
>> + enum: [ input-xtal, input-clk0, input-clk1 ]
>
> 'input-' is redundant.
>

Will remove in next patch version.

>> +
>> + clocks:
>> + minItems: 1
>> + maxItems: 3
>> +
>> + settings:
>> + description: Optional, list of space separated ASCII numbers in hex.
>
> Huh?
>
>> + This list is the entire register map of the product and must contain
>> + 791 items.
>
> What is this for?
>
> Seems suspect, but would need a vendor prefix and type at a minimum.
>

The description could be better, I'll improve it in the next patch version.
More info about it is in the main description for the device tree at the
top of the schema. The user can provide the entire register map to be
written to the device during initialization. This provides the user a way
to fully customize the device without limitations. This is typically used
by users who have spent time optimizing the device for performance and
want to use those exact settings.

>> +
>> +required:
>> + - compatible
>> + - reg
>> + - '#clock-cells'
>> + - clocks
>> + - clock-names
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + /* 25MHz reference clock */
>> + input_clk0: input_clk0 {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <25000000>;
>> + };
>> +
>> + i2c@0 {
>> + reg = <0x0 0x100>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + renesas8t49n241_1: clock-generator@6c {
>> + compatible = "renesas,8t49n241";
>> + reg = <0x6c>;
>> + #clock-cells = <1>;
>> +
>> + clocks = <&input_clk0>;
>> + clock-names = "input-clk0";
>> + };
>> + };
>> +
>> + /* Consumer referencing the 8T49N241 Q1 */
>> + consumer {
>> + /* ... */
>> + clocks = <&renesas8t49n241_1 1>;
>> + /* ... */
>> + };
>> + - |
>> + /* 40MHz crystal */
>> + input_xtal: input_xtal {
>> + compatible = "fixed-clock";
>> + #clock-cells = <0>;
>> + clock-frequency = <40000000>;
>> + };
>> +
>> + i2c@0 {
>> + reg = <0x0 0x100>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + renesas8t49n241_2: clock-generator@6c {
>> + compatible = "renesas,8t49n241";
>> + reg = <0x6c>;
>> + #clock-cells = <1>;
>> +
>> + clocks = <&input_xtal>;
>> + clock-names = "input-xtal";
>> +
>> + settings=[
>> + 09 50 00 60 67 C5 6C FF 03 00 30 00 00 01 00 00
>> + 01 07 00 00 07 00 00 77 6D 06 00 00 00 00 00 FF
>> + FF FF FF 00 3F 00 2A 00 16 33 33 00 01 00 00 D0
>> + 00 00 00 00 00 00 00 00 00 04 00 00 00 02 00 00
>> + 00 00 00 00 00 00 00 17 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 D7 0A 2B 20 00 00 00 0B
>> + 00 00 00 00 00 00 00 00 00 00 27 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + C3 00 08 01 00 00 00 00 00 00 00 00 00 30 00 00
>> + 00 0A 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> + 00 00 00 00 85 00 00 9C 01 D4 02 71 07 00 00 00
>> + 00 83 00 10 02 08 8C
>> + ];
>> + };
>> + };
>> +
>> + /* Consumer referencing the 8T49N241 Q1 */
>> + consumer {
>> + /* ... */
>> + clocks = <&renesas8t49n241_2 1>;
>> + /* ... */
>> + };
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 0cce91cd5..882d79ead 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -15575,6 +15575,12 @@ F: include/linux/rpmsg/
>> F: include/uapi/linux/rpmsg.h
>> F: samples/rpmsg/
>>
>> +RENESAS 8T49N24X DRIVER
>> +M: Alex Helms <alexander.helms.jy@xxxxxxxxxxx>
>> +M: David Cater <david.cater.jc@xxxxxxxxxxx>
>> +S: Odd Fixes
>> +F: Documentation/devicetree/bindings/clock/renesas,8t49n241.yaml
>> +
>> RENESAS CLOCK DRIVERS
>> M: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>> L: linux-renesas-soc@xxxxxxxxxxxxxxx
>> --
>> 2.30.2
>>
>>