Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dts: ti: cc13xx_cc26xx: devicetree sysclk alignment #59965

Merged

Conversation

ghost
Copy link

@ghost ghost commented Jul 3, 2023

This change introduces the "_rtc_timer" suffix for the system tick timer driver "compatible" property and aligns naming conventions with the actual CC13/26xx SoC series product policy.

This frees up the "_rtc" namespace to introduce additional APIs based on the same peripheral in the future:

rtc: rtc@... {
  compatible = "ti,cc13xx-cc26xx-rtc";
  ...

  timer {
    compatible = "ti,cc13xx-cc26xx-rtc-timer";
    ...
  };

  counter {
    compatible = "ti,cc13xx-cc26xx-rtc-counter";
    ...
  };

  pps {
    compatible = "ti,cc13xx-cc26xx-rtc-pps";
    ...
  };
};

Or alternatively an MFD pattern with similar requirements.

To enable this change, some duplicate devicetree nodes were removed and inconsistencies in the naming and include hierarchy of the cc13xx_cc26xx devicetree and RTC system clock driver fixed.

@zephyrbot zephyrbot added area: Kernel area: Timer Timer platform: TI SimpleLink Texas Instruments SimpleLink MCU area: Devicetree Binding PR modifies or adds a Device Tree binding labels Jul 3, 2023
@ghost
Copy link
Author

ghost commented Jul 3, 2023

@cfriedt @niflostancu You'll probably also want to review this.

Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible that this may be usable as a RTC in spite of the fact that it's missing separate year, month, day of month, etc registers.

In particular, the fact that it has support for compensation indicates to me that it might actually work.

I wouldn't mind hearing @vaishnavachath's thoughts here.

@ghost
Copy link
Author

ghost commented Jul 4, 2023

It's possible that this may be usable as a RTC in spite of the fact that it's missing separate year, month, day of month, etc registers.

@cfriedt True, the hardware peripheral has the capability for system_timer.h, rtc.h, counter.h and PPS for clock synchronization.

Got some help from Brix on Discord, I guess the correct solution would then be rtc: rtc@... following the "peripheral-name-from-datasheet: generic-function@address" pattern? The same is probably true for the compat-String which was fine as it represents hardware, not the driver software. Will update the PR accordingly.

@ghost
Copy link
Author

ghost commented Jul 4, 2023

@cfriedt Update: As only one driver can be attached per dt node using the _rtc_timer suffix for the compatible property is correct as we'd clutter the namespace otherwise. Do you agree?

Just updated the PR accordingly.

@ghost ghost force-pushed the fix/cc13xx_cc26xx_device_hierarchy branch 3 times, most recently from c7e431e to 0eed9f9 Compare July 4, 2023 13:38
@ghost ghost requested a review from cfriedt July 4, 2023 13:41
@ghost ghost force-pushed the fix/cc13xx_cc26xx_device_hierarchy branch from 0eed9f9 to ff62820 Compare July 4, 2023 13:43
Copy link
Collaborator

@vaishnavachath vaishnavachath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fgrandel can we split the changes to two separate commits? one for the binding/DT update and another for the rename/include fixes?

dts/arm/ti/cc13xx_cc26xx.dtsi Show resolved Hide resolved
dts/arm/ti/cc13xx_cc26xx.dtsi Show resolved Hide resolved
@ghost ghost force-pushed the fix/cc13xx_cc26xx_device_hierarchy branch from ff62820 to 8c9ccab Compare July 6, 2023 14:09
@ghost
Copy link
Author

ghost commented Jul 6, 2023

an we split the changes to two separate commits? one for the binding/DT update and another for the rename/include fixes?

@vaishnavachath Yes, of course. Makes sense. Done.

@ghost ghost requested a review from vaishnavachath July 6, 2023 23:03
@ghost
Copy link
Author

ghost commented Jul 6, 2023

Echo client/server regression test result on real hardware to verify that nothing fundamental broke:

IPv6 recv      78248    sent    78327   drop    6       forwarded       0
IPv6 ND recv   992      sent    1001    drop    0
IPv6 MLD recv  0        sent    3       drop    0
IP vhlerr      0        hblener 0       lblener 0
IP fragerr     0        chkerr  0       protoer 0
UDP recv       77250    sent    77322   drop    0
UDP chkerr     0
Bytes received 10916465
Bytes sent     10920976
Processing err 6

Removes duplicate code and inconsistencies in the naming of the
cc13xx_cc26xx devicetree and RTC driver hierarchy and alignes it with
the actual TI product series naming hierarchy.

Signed-off-by: Florian Grandel <fgrandel@code-for-humans.de>
This change introduces the "_rtc_timer" suffix for the system tick timer
driver "compatible" property and aligns naming conventions with the
actual CC13/26xx SoC series product policy.

This frees up the "_rtc" namespace to introduce additional APIs based on
the same peripheral in the future (not part of this PR):

rtc: rtc@... {
  compatible = "ti,cc13xx-cc26xx-rtc";
  ...

  timer {
    compatible = "ti,cc13xx-cc26xx-rtc-timer";
    ...
  };

  counter {
    compatible = "ti,cc13xx-cc26xx-rtc-counter";
    ...
  };

  pps {
    compatible = "ti,cc13xx-cc26xx-rtc-pps";
    ...
  };
};

Or alternatively an MFD pattern with similar requirements.

Fixing the namespacing now makes sense standalone as it reduces the
chance of custom drivers being broken in the future.

Redundant extension of the mandatory system clock devicetree node is
replaced with a single `status = "okay"` which seems to be the more
sensible default to avoid user error when defining custom boards.
Knowledgeable users can still override this if really needed.

Signed-off-by: Florian Grandel <fgrandel@code-for-humans.de>
@ghost ghost force-pushed the fix/cc13xx_cc26xx_device_hierarchy branch from 8c9ccab to e89c6ea Compare July 7, 2023 16:22
@ghost
Copy link
Author

ghost commented Jul 7, 2023

@cfriedt I think this one is ready for re-reviewing now.

@cfriedt cfriedt merged commit 75c83ed into zephyrproject-rtos:main Jul 7, 2023
@kartben
Copy link
Collaborator

kartben commented Jul 8, 2023

I'm obviously late here, but shouldn't these changes be added to the release notes? Renaming the compatible and Kconfig options may break things for people.
Also, it would probably make sense to rename ti,cc13xx-cc26xx-rtc.yaml to ti,cc13xx-cc26xx-rtc-timer.yaml?

@ghost
Copy link
Author

ghost commented Jul 8, 2023

I'm obviously late here,

@kartben It is never too late. :-) See #60177

Thanks for catching these obvious issues!

@ghost ghost deleted the fix/cc13xx_cc26xx_device_hierarchy branch July 8, 2023 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree Binding PR modifies or adds a Device Tree binding area: Kernel area: Timer Timer platform: TI SimpleLink Texas Instruments SimpleLink MCU
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants