drivers: rtc: rtc_mc146818: Added RTC driver for Motorola MC146818B #56334

akanisetti · 2023-03-29T18:25:31Z

Added RTC driver that supports Motorola MC146818B
Enabled RTC set/get time and alarm, alarm callback
and update callback.

Counter and RTC uses same hardware in case of
motorola mc146818, so they can't be used at a time.

Updated stand-alone mc146818 counter dts instances
to support RTC and counter with same compatible
string of "motorola,mc146818" on ia32, atom,
apollo_lake, elhart_lake and raptor_lake platforms.

Signed-off-by: Anisetti Avinash Krishna [email protected]

bjarki-andreasen · 2023-04-01T13:01:19Z

First of, happy to see drivers being written for the new API :)

Regarding the naming rtc_intel for the Motorola MC146818 RTC, I believe the naming should be rtc_mc146818.c.

Looking at the current driver base however, there is already a motorola,mc146818 devicetree compatible, this one is currently supported by the drivers/counter/counter_cmos.c. We can not reuse the devicetree compatible since it would break the boards currently using it, I propose we use the fact that NXP also produced this chip at some point (see https://www.nxp.com/docs/en/data-sheet/MC146818.pdf), and call the devicetree compatible nxp,mc146818

With this little trick, we can have the new driver and the old one coexist without breaking anything, and allow other boards to use the driver as well :)

Otherwise, we need to replace the old dts file and in-tree boards using it, which might be a good solution, but not something we should do within a single PR.

akanisetti · 2023-04-03T04:10:41Z

First of, happy to see drivers being written for the new API :)

Regarding the naming rtc_intel for the Motorola MC146818 RTC, I believe the naming should be rtc_mc146818.c.

Looking at the current driver base however, there is already a motorola,mc146818 devicetree compatible, this one is currently supported by the drivers/counter/counter_cmos.c. We can not reuse the devicetree compatible since it would break the boards currently using it, I propose we use the fact that NXP also produced this chip at some point (see https://www.nxp.com/docs/en/data-sheet/MC146818.pdf), and call the devicetree compatible nxp,mc146818

With this little trick, we can have the new driver and the old one coexist without breaking anything, and allow other boards to use the driver as well :)

Otherwise, we need to replace the old dts file and in-tree boards using it, which might be a good solution, but not something we should do within a single PR.

Yeah, rtc_mc146818.c seems to be a better name, I will add that change.
Regarding devicetree compatible, how about using motorola,mc146818-rtc, as motorola,mc146818 is a counter.

bjarki-andreasen · 2023-04-04T16:27:24Z

Yeah, rtc_mc146818.c seems to be a better name, I will add that change.
Regarding devicetree compatible, how about using motorola,mc146818-rtc, as motorola,mc146818 is a counter.

The motorola mc146818 is a real-time clock, right? The existing driver that is using that devicetree binding is just using the counter API, its not a counter on a hardware level as I understand it.

I agree with the naming, but I lack understanding of the existing counter node. The .dtsi looks kind of odd from my perspective. It seems there are two instances of the RTC hardware, one at address <0x70 0x0D>, and the one you have added at <0x71, 0x50>, is that accurate? I have no experience with raptor_lake, It would be great if you could help me understand it :)

akanisetti · 2023-04-05T03:41:24Z

Yeah, rtc_mc146818.c seems to be a better name, I will add that change.
Regarding devicetree compatible, how about using motorola,mc146818-rtc, as motorola,mc146818 is a counter.

The motorola mc146818 is a real-time clock, right? The existing driver that is using that devicetree binding is just using the counter API, its not a counter on a hardware level as I understand it.

I agree with the naming, but I lack understanding of the existing counter node. The .dtsi looks kind of odd from my perspective. It seems there are two instances of the RTC hardware, one at address <0x70 0x0D>, and the one you have added at <0x71, 0x50>, is that accurate? I have no experience with raptor_lake, It would be great if you could help me understand it :)

Motorola mc146818 is a real-time clock and used as counter. Raptor lake has only one instance of RTC, but if I give same reg address it is not accepting. So, I have given the "target" address instead of "index" for RTC instance.

bjarki-andreasen · 2023-04-05T06:20:36Z

Motorola mc146818 is a real-time clock and used as counter. Raptor lake has only one instance of RTC, but if I give same reg
address it is not accepting. So, I have given the "target" address instead of "index" for RTC instance.

In this case, I recommend we reuse the existing devicetree compatible:

Move the dts bindings file motorola,mc146818.yaml from the bindings/counter to the bindings/rtc folder
Update it to be based on the rtc-device binding

# Copyright (c) 2022, Intel Corporation
#
# SPDX-License-Identifier: Apache-2.0

description: Motorola MC146818 compatible Real Timer Clock

compatible: "motorola,mc146818"

-- include: base.yaml
++ include: rtc-device.yaml

Update the existing node in the dtsi for all in-tree boards using the Motorola MC146818. Notice that the node rtc now has two node labels, counter and rtc, to be backwards compatible with apps that use the device as a counter.

--		counter: counter@70 {
--			compatible = "motorola,mc146818";
--			reg = <0x70 0x0D 0x71 0x0D>;
--			status = "okay";
--		};

++		rtc: counter: rtc@70 {
++			compatible = "motorola,mc146818";
++			reg = <0x70 0x0D 0x71 0x0D>;
++			interrupts = <8 IRQ_TYPE_LOWEST_EDGE_RISING 3>;
++			interrupt-parent = <&intc>;
++			alarms-count = <1>;
++
++			status = "okay";
++		};

Add a rule to Kconfig to ensure only either driver (RTC or Counter) is included in the build. If the RTC subsystem is enabled CONFIG_RTC=y, the RTC driver in this PR is included, if counter is enabled CONFIG_COUNTER=y and the RTC isn't, the counter_cmos.c driver is included. This is acheived with the following changes

config COUNTER_CMOS
	bool "Counter driver for x86 CMOS/RTC clock"
++	default y if !RTC
	depends on DT_HAS_MOTOROLA_MC146818_ENABLED

config RTC_MOTOROLA_MC146818
	bool "RTC driver for x86 CMOS/RTC clock"
	depends on DT_HAS_MOTOROLA_MC146818_ENABLED

This solution keeps one device binding for the one device, which is appropriate, then allows the user to select the appropriate driver for it by manually including CONFIG_COUNTER, and or CONFIG_RTC, while keeping the dtsi backwards compatible :)

akanisetti · 2023-04-05T09:27:37Z

Motorola mc146818 is a real-time clock and used as counter. Raptor lake has only one instance of RTC, but if I give same reg
address it is not accepting. So, I have given the "target" address instead of "index" for RTC instance.

In this case, I recommend we reuse the existing devicetree compatible:

Move the dts bindings file motorola,mc146818.yaml from the bindings/counter to the bindings/rtc folder

Update it to be based on the rtc-device binding
# Copyright (c) 2022, Intel Corporation
#
# SPDX-License-Identifier: Apache-2.0

description: Motorola MC146818 compatible Real Timer Clock

compatible: "motorola,mc146818"

-- include: base.yaml
++ include: rtc-device.yaml
Update the existing node in the dtsi for all in-tree boards using the Motorola MC146818. Notice that the node rtc now has two node labels, counter and rtc, to be backwards compatible with apps that use the device as a counter.
--		counter: counter@70 {
--			compatible = "motorola,mc146818";
--			reg = <0x70 0x0D 0x71 0x0D>;
--			status = "okay";
--		};

++		rtc: counter: rtc@70 {
++			compatible = "motorola,mc146818";
++			reg = <0x70 0x0D 0x71 0x0D>;
++			interrupts = <8 IRQ_TYPE_LOWEST_EDGE_RISING 3>;
++			interrupt-parent = <&intc>;
++			alarms-count = <1>;
++
++			status = "okay";
++		};
Add a rule to Kconfig to ensure only either driver (RTC or Counter) is included in the build. If the RTC subsystem is enabled CONFIG_RTC=y, the RTC driver in this PR is included, if counter is enabled CONFIG_COUNTER=y and the RTC isn't, the counter_cmos.c driver is included. This is acheived with the following changes
config COUNTER_CMOS
	bool "Counter driver for x86 CMOS/RTC clock"
++	default y if !RTC
	depends on DT_HAS_MOTOROLA_MC146818_ENABLED
config RTC_MOTOROLA_MC146818
	bool "RTC driver for x86 CMOS/RTC clock"
	depends on DT_HAS_MOTOROLA_MC146818_ENABLED
This solution keeps one device binding for the one device, which is appropriate, then allows the user to select the appropriate driver for it by manually including CONFIG_COUNTER, and or CONFIG_RTC, while keeping the dtsi backwards compatible :)

Thank you for these changes, but for backward compatibility I may need to mark alarm-count property in rtc-device.yaml as "required: false". If I change it to false in mc146818.yaml it is throwing an error. Could you please suggest any better method to change this without disturbing rtc-device.yaml. Thanks

bjarki-andreasen · 2023-04-05T11:16:02Z

You should be able to have the alarms-count <1>; property in the .dtsi file without affecting the counter drivers use of the same devicetree compatible :)

Like the interrupt property, if the included driver is not actively using it through one of the DT macros, it will simply be ignored.

akanisetti · 2023-04-05T11:52:26Z

You should be able to have the alarms-count <1>; property in the .dtsi file without affecting the counter drivers use of the same devicetree compatible :)

Like the interrupt property, if the included driver is not actively using it through one of the DT macros, it will simply be ignored.

I checked and confirmed that counter is not used on raptor lake, so instead of disturbing counter yaml, I just removed counter instance from dtsi file and added rtc only.
Regarding adding alarm-count property, I have to add it all the counter instances wherever they were defined.

dts/bindings/rtc/intel,rtc.yaml

bjarki-andreasen · 2023-04-05T12:34:06Z

I checked and confirmed that counter is not used on raptor lake, so instead of disturbing counter yaml, I just removed counter instance from dtsi file and added rtc only. Regarding adding alarm-count property, I have to add it all the counter instances wherever they were defined.

This is a breaking change for potential downstream boards currently using the cmos_counter.c driver. If we are breaking it, we should probably go all the way and replace the cmos_counter.c driver entirely, for the other 4 in-tree overlays (apollo_lake, atom, elkhart, ia32) using it as well :)

I think this is the most proper change to align the devices and drivers, no motorola,mc146818 and motorola,mc146818-rtc, just the first one, using the better fit RTC driver :)

akanisetti · 2023-04-05T12:47:33Z

I don't think this will break downstream boards, I am not disturbing counter of older boards. They will still use counter_cmos.c.

bjarki-andreasen · 2023-04-05T15:36:39Z

I don't think this will break downstream boards, I am not disturbing counter of older boards. They will still use counter_cmos.c.

In that case I agree with just removing the counter node from the new board :)

jhedberg · 2023-04-20T15:27:38Z

drivers/rtc/Kconfig.mc146818

+config RTC_MOTOROLA_MC146818
+	bool "RTC driver for x86 CMOS/RTC clock"
+	default y
+	depends on DT_HAS_MOTOROLA_MC146818_ENABLED && SOC_RAPTOR_LAKE


Why did you add the SoC dependency here? I don’t think that’s the right thing to do. What are you trying to solve with that?

Should there instead be default y if !COUNTER_CMOS?

Why did you add the SoC dependency here? I don’t think that’s the right thing to do. What are you trying to solve with that?

If this is not added, while running rtc tests for other boards with just counter instance and compatible "motorola,mc146818" status okay, is building rtc driver without instance and failing.

Other way is that if we have counter instance with compatible "motorola,mc164818" in dts then we can add RTC instance for those boards also.

Well, whatever the solution, it can't be that we restrict using the driver to only one specific SoC.

If this is not added, while running rtc tests for other boards with just counter instance and compatible "motorola,mc146818" status okay, is building rtc driver without instance and failing.

Would one solution for this be to introduce a supported: rtc property to the board yaml files, and then we enable RTC tests only if the board indicates support for RTC? Any comments from @bjarki-trackunit ?

Would one solution for this be to introduce a supported: rtc property to the board yaml files, and then we enable RTC tests only if the board indicates support for RTC? Any comments from @bjarki-trackunit ?

Actually, isn't this already the case? I see depends_on: rtc in the testcase.yaml files of RTC tests, so they should only get enabled if the board declares supported: rtc. @akanisetti what were the problematic boards exactly?

depends_on: rtc is not there for test/drivers/rtc/rtc_api_helpers/tetscase.yaml file. It is failing for all x86 boards which doesn't have an rtc instance in dts but motorola,mc146818 compatible is okay status. Build of rpl_api_helpers test application is failing on qemu_x86 on CI.

jhedberg · 2023-04-24T09:27:42Z

@akanisetti looks like tests/drivers/rtc/rtc_api_helpers/testcase.yaml has a missing dependency on rtc

akanisetti · 2023-04-24T09:29:46Z

@akanisetti looks like tests/drivers/rtc/rtc_api_helpers/testcase.yaml has a missing dependency on rtc

Yes.

bjarki-andreasen · 2023-04-24T15:18:32Z

The tests/drivers/rtc/rtc_api_helpers/testcase.yaml tests and validates the rtc_time_to_tm helper, it is not dependent on the board supporting RTC. It is a a check to validate the compatibility between the struct rtc_time and struct tm, which is aimed at different architectures, like 64-bit, 32-bit etc.

The tests should build and run without error based on this test run https://github.com/zephyrproject-rtos/zephyr/actions/runs/4756340536/jobs/8451830119?pr=57087 from a previous PR. Not sure why the test can not be built for this PR...

akanisetti · 2023-04-24T17:24:26Z

The tests/drivers/rtc/rtc_api_helpers/testcase.yaml tests and validates the rtc_time_to_tm helper, it is not dependent on the board supporting RTC. It is a a check to validate the compatibility between the struct rtc_time and struct tm, which is aimed at different architectures, like 64-bit, 32-bit etc.

The tests should build and run without error based on this test run https://github.com/zephyrproject-rtos/zephyr/actions/runs/4756340536/jobs/8451830119?pr=57087 from a previous PR. Not sure why the test can not be built for this PR...

It is failing for the boards which has counter instance in dts as following,

            counter: counter@70 {
		compatible = "motorola,mc146818";
		reg = <0x70 0x0D 0x71 0x0D>;
		status = "okay";
	};

when "tests/drivers/rtc/rtc_api_helpers" is tested, it is enabling CONFIG_RTC and from dts motorola,mc146818 is enabled. So this counter@70 instance is trying to build with rtc_motorola_mc146818.c driver file and is failing to get interrupt details from dts.

Solution could be modifying all cmos_counter instances to support both rtc and counter as it is done in rpl_crb dts. As same hardware and same register address in all those instances.

bjarki-andreasen · 2023-04-24T21:34:28Z

Solution could be modifying all cmos_counter instances to support both rtc and counter as it is done in rpl_crb dts. As same hardware and same register address in all those instances.

I believe this is the best solution :)

jhedberg · 2023-04-26T13:05:16Z

dts/x86/intel/elkhart_lake.dtsi

@@ -689,11 +689,15 @@
 			status = "okay";
 		};

-		counter: counter@70 {
+		rtc: counter: rtc@70 {


Since you're making this change, would it make sense to add supported: rtc to the board definition of ehl_crb too (as well as an alias to the board dts)? Same for the ia32 change and any boards that use that? It can of course be done as a follow-up PR as well, but I feel like it'd be good to be consistent with this, i.e. if the dts has an rtc node label, then boards using it should declare supported: rtc.

Sure, will add these changes.

I think by adding supported: rtc for ia32 I am enabling rtc tests on qemu_x86 which will fail. I should remove supported: rtc or instead remove rtc/counter instance from ia32.dtsi file as it doesn't work in qemu anyway. Please provide your suggestions. @jhedberg @bjarki-trackunit

You should remove supported: rtc from the qemu_x86.yaml file :) The x86/acrn board also uses the ia32.dtsi. I don't know it it uses the counter node either, but I think it is safest to keep the rtc/counter node in the ia32.dtsi file.

Added RTC driver that supports Motorola MC146818B Enabled RTC set/get time and alarm, alarm callback and update callback. Counter and RTC uses same hardware in case of Motorola MC146818, so they can't be used at a time. Updated stand-alone mc146818 counter dts instances to support rtc and counter with same compatible string of "motorola,mc146818" on ia32, atom, apollo_lake, elhart_lake and raptor_lake platforms. Signed-off-by: Anisetti Avinash Krishna <[email protected]>

bjarki-andreasen · 2023-04-27T14:30:07Z

Nice work @akanisetti :)

akanisetti · 2023-04-27T14:32:43Z

Thank you for the support @bjarki-trackunit and @jhedberg

akanisetti requested review from nashif, carlescufi, galak, MaureenHelm and jhedberg as code owners March 29, 2023 18:25

akanisetti marked this pull request as draft March 29, 2023 18:25

zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding area: Documentation platform: X86 x86 and x86-64 labels Mar 29, 2023

zephyrbot requested review from aasthagr, kartben, laurenmurphyx64 and tbursztyka March 29, 2023 18:26

akanisetti force-pushed the rtc_intel branch from 9d2fd70 to 0bd15e7 Compare March 29, 2023 18:37

jhedberg requested a review from bjarki-andreasen March 29, 2023 19:15

akanisetti force-pushed the rtc_intel branch from 0bd15e7 to 998ba04 Compare April 5, 2023 10:16

jhedberg reviewed Apr 5, 2023

View reviewed changes

dts/bindings/rtc/intel,rtc.yaml Outdated Show resolved Hide resolved

akanisetti force-pushed the rtc_intel branch from 998ba04 to 9bf88d2 Compare April 5, 2023 12:10

akanisetti marked this pull request as ready for review April 5, 2023 18:02

akanisetti dismissed jhedberg’s stale review via a5403ec April 20, 2023 14:40

akanisetti force-pushed the rtc_intel branch from 8758803 to a5403ec Compare April 20, 2023 14:40

jhedberg reviewed Apr 20, 2023

View reviewed changes

akanisetti force-pushed the rtc_intel branch from a5403ec to 5e4fd5d Compare April 24, 2023 08:19

akanisetti force-pushed the rtc_intel branch 2 times, most recently from 49ae941 to b4c3c78 Compare April 26, 2023 11:46

jhedberg reviewed Apr 26, 2023

View reviewed changes

akanisetti force-pushed the rtc_intel branch 2 times, most recently from e0a3ae8 to f2a2754 Compare April 27, 2023 03:05

akanisetti changed the title ~~drivers: rtc: rtc_intel: Added RTC driver support for Motorola MC146818B~~ drivers: rtc: rtc_mc146818: Added RTC driver support for Motorola MC146818B Apr 27, 2023

akanisetti force-pushed the rtc_intel branch from f2a2754 to bcd1ea2 Compare April 27, 2023 03:31

akanisetti changed the title ~~drivers: rtc: rtc_mc146818: Added RTC driver support for Motorola MC146818B~~ drivers: rtc: rtc_mc146818: Added RTC driver for Motorola MC146818B Apr 27, 2023

akanisetti requested review from jhedberg and bjarki-andreasen April 27, 2023 04:32

jhedberg previously approved these changes Apr 27, 2023

View reviewed changes

akanisetti dismissed jhedberg’s stale review via 7b494d3 April 27, 2023 08:58

akanisetti force-pushed the rtc_intel branch from bcd1ea2 to 7b494d3 Compare April 27, 2023 08:58

jhedberg approved these changes Apr 27, 2023

View reviewed changes

bjarki-andreasen approved these changes Apr 27, 2023

View reviewed changes

carlescufi merged commit bfeb504 into zephyrproject-rtos:main Apr 27, 2023

cfriedt mentioned this pull request Jun 20, 2023

drivers: rtc: mc146818: driver fails multiple tests from rtc_api testsuite #59448

Closed

akanisetti deleted the rtc_intel branch September 5, 2023 04:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: rtc: rtc_mc146818: Added RTC driver for Motorola MC146818B #56334

drivers: rtc: rtc_mc146818: Added RTC driver for Motorola MC146818B #56334

akanisetti commented Mar 29, 2023 •

edited

Loading

bjarki-andreasen commented Apr 1, 2023 •

edited

Loading

akanisetti commented Apr 3, 2023

bjarki-andreasen commented Apr 4, 2023 •

edited

Loading

akanisetti commented Apr 5, 2023

bjarki-andreasen commented Apr 5, 2023 •

edited

Loading

akanisetti commented Apr 5, 2023

bjarki-andreasen commented Apr 5, 2023

akanisetti commented Apr 5, 2023

bjarki-andreasen commented Apr 5, 2023 •

edited

Loading

akanisetti commented Apr 5, 2023

bjarki-andreasen commented Apr 5, 2023

jhedberg Apr 20, 2023

jhedberg Apr 20, 2023

akanisetti Apr 20, 2023

akanisetti Apr 20, 2023

jhedberg Apr 21, 2023

jhedberg Apr 21, 2023

akanisetti Apr 21, 2023

jhedberg commented Apr 24, 2023

akanisetti commented Apr 24, 2023

bjarki-andreasen commented Apr 24, 2023

akanisetti commented Apr 24, 2023

bjarki-andreasen commented Apr 24, 2023

jhedberg Apr 26, 2023

akanisetti Apr 26, 2023

akanisetti Apr 26, 2023

bjarki-andreasen Apr 26, 2023 •

edited

Loading

bjarki-andreasen commented Apr 27, 2023

akanisetti commented Apr 27, 2023

drivers: rtc: rtc_mc146818: Added RTC driver for Motorola MC146818B #56334

drivers: rtc: rtc_mc146818: Added RTC driver for Motorola MC146818B #56334

Conversation

akanisetti commented Mar 29, 2023 • edited Loading

bjarki-andreasen commented Apr 1, 2023 • edited Loading

akanisetti commented Apr 3, 2023

bjarki-andreasen commented Apr 4, 2023 • edited Loading

akanisetti commented Apr 5, 2023

bjarki-andreasen commented Apr 5, 2023 • edited Loading

akanisetti commented Apr 5, 2023

bjarki-andreasen commented Apr 5, 2023

akanisetti commented Apr 5, 2023

bjarki-andreasen commented Apr 5, 2023 • edited Loading

akanisetti commented Apr 5, 2023

bjarki-andreasen commented Apr 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhedberg commented Apr 24, 2023

akanisetti commented Apr 24, 2023

bjarki-andreasen commented Apr 24, 2023

akanisetti commented Apr 24, 2023

bjarki-andreasen commented Apr 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjarki-andreasen Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

bjarki-andreasen commented Apr 27, 2023

akanisetti commented Apr 27, 2023

akanisetti commented Mar 29, 2023 •

edited

Loading

bjarki-andreasen commented Apr 1, 2023 •

edited

Loading

bjarki-andreasen commented Apr 4, 2023 •

edited

Loading

bjarki-andreasen commented Apr 5, 2023 •

edited

Loading

bjarki-andreasen commented Apr 5, 2023 •

edited

Loading

bjarki-andreasen Apr 26, 2023 •

edited

Loading