[PATCH 0/2] rtc: pch-rtc: add Intel Series PCH built-in read-only RTC

[PATCH 0/2] rtc: pch-rtc: add Intel Series PCH built-in read-only RTC

[Ivan Mikhaylov]

Add RTC driver with dt binding tree document. Also this driver adds one sysfs
attribute for host power control which I think is odd for RTC driver.
Need I cut it off and use I2C_SLAVE_FORCE? I2C_SLAVE_FORCE is not good
way too from my point of view. Is there any better approach?

Ivan Mikhaylov (2):
  rtc: pch-rtc: add RTC driver for Intel Series PCH
  dt-bindings: rtc: provide RTC PCH device tree binding doc

 .../bindings/rtc/intel,pch-rtc.yaml           |  39 +++++
 drivers/rtc/Kconfig                           |  10 ++
 drivers/rtc/Makefile                          |   1 +
 drivers/rtc/rtc-pch.c                         | 148 ++++++++++++++++++
 4 files changed, 198 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/intel,pch-rtc.yaml
 create mode 100644 drivers/rtc/rtc-pch.c

-- 
2.31.1

[PATCH 1/2] rtc: pch-rtc: add RTC driver for Intel Series PCH

[Ivan Mikhaylov]

Add the Intel Series PCH built-in read-only RTC. This driver is not for
in-system use on x86 but rather is for external access over I2C from a BMC.

Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com>
---
 drivers/rtc/Kconfig   |  10 +++
 drivers/rtc/Makefile  |   1 +
 drivers/rtc/rtc-pch.c | 148 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 159 insertions(+)
 create mode 100644 drivers/rtc/rtc-pch.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 12153d5801ce..4f31bcbf0736 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -555,6 +555,16 @@ config RTC_DRV_PALMAS
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-palma.
 
+config RTC_DRV_PCH
+	tristate "PCH RTC driver"
+	help
+	  If you say yes here you get support for the Intel Series PCH
+	  built-in read-only RTC. This driver is not for in-system use on x86,
+	  but rather is for external access over I2C from a BMC.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called rtc-pch.
+
 config RTC_DRV_TPS6586X
 	tristate "TI TPS6586X RTC driver"
 	depends on MFD_TPS6586X
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 2dd0dd956b0e..cb2804433d87 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -121,6 +121,7 @@ obj-$(CONFIG_RTC_DRV_PCF8523)	+= rtc-pcf8523.o
 obj-$(CONFIG_RTC_DRV_PCF85363)	+= rtc-pcf85363.o
 obj-$(CONFIG_RTC_DRV_PCF8563)	+= rtc-pcf8563.o
 obj-$(CONFIG_RTC_DRV_PCF8583)	+= rtc-pcf8583.o
+obj-$(CONFIG_RTC_DRV_PCH)	+= rtc-pch.o
 obj-$(CONFIG_RTC_DRV_PIC32)	+= rtc-pic32.o
 obj-$(CONFIG_RTC_DRV_PL030)	+= rtc-pl030.o
 obj-$(CONFIG_RTC_DRV_PL031)	+= rtc-pl031.o
diff --git a/drivers/rtc/rtc-pch.c b/drivers/rtc/rtc-pch.c
new file mode 100644
index 000000000000..a2918257bf39
--- /dev/null
+++ b/drivers/rtc/rtc-pch.c
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * I2C read-only RTC driver for PCH with additional sysfs attribute for host
+ * power control.
+ *
+ * Copyright (C) 2021 YADRO
+ */
+
+#include <linux/i2c.h>
+#include <linux/slab.h>
+#include <linux/rtc.h>
+#include <linux/bcd.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#define PCH_REG_FORCE_OFF		0x00
+#define PCH_REG_SC			0x09
+#define PCH_REG_MN			0x0a
+#define PCH_REG_HR			0x0b
+#define PCH_REG_DW			0x0c
+#define PCH_REG_DM			0x0d
+#define PCH_REG_MO			0x0e
+#define PCH_REG_YR			0x0f
+
+#define NUM_TIME_REGS   (PCH_REG_YR - PCH_REG_SC + 1)
+
+struct pch {
+	struct rtc_device *rtc;
+	struct regmap *regmap;
+};
+
+static int pch_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct pch *pch = i2c_get_clientdata(client);
+	unsigned char rtc_data[NUM_TIME_REGS] = {0};
+	int rc;
+
+	rc = regmap_bulk_read(pch->regmap, PCH_REG_SC, rtc_data, NUM_TIME_REGS);
+	if (rc < 0) {
+		dev_err(dev, "fail to read time reg(%d)\n", rc);
+		return rc;
+	}
+
+	tm->tm_sec	= bcd2bin(rtc_data[0]);
+	tm->tm_min	= bcd2bin(rtc_data[1]);
+	tm->tm_hour	= bcd2bin(rtc_data[2]);
+	tm->tm_wday	= rtc_data[3];
+	tm->tm_mday	= bcd2bin(rtc_data[4]);
+	tm->tm_mon	= bcd2bin(rtc_data[5]) - 1;
+	tm->tm_year	= bcd2bin(rtc_data[6]) + 100;
+
+	return 0;
+}
+
+static ssize_t force_off_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct pch *pch = i2c_get_clientdata(client);
+	unsigned long val;
+	int rc;
+
+	if (kstrtoul(buf, 10, &val))
+		return -EINVAL;
+
+	if (val) {
+		/* 0x02 host force off */
+		rc = regmap_write(pch->regmap, PCH_REG_FORCE_OFF, 0x2);
+		if (rc < 0) {
+			dev_err(dev, "Fail to read time reg(%d)\n", rc);
+			return rc;
+		}
+	}
+
+	return 0;
+}
+static DEVICE_ATTR_WO(force_off);
+
+static const struct rtc_class_ops pch_rtc_ops = {
+	.read_time = pch_rtc_read_time,
+};
+
+static const struct regmap_config pch_rtc_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.use_single_read = true,
+};
+
+static int pch_rtc_probe(struct i2c_client *client)
+{
+	struct pch *pch;
+	int rc;
+
+	pch = devm_kzalloc(&client->dev, sizeof(*pch), GFP_KERNEL);
+	if (!pch)
+		return -ENOMEM;
+
+	pch->regmap = devm_regmap_init_i2c(client, &pch_rtc_regmap_config);
+	if (IS_ERR(pch->regmap)) {
+		dev_err(&client->dev, "regmap_init failed\n");
+		return PTR_ERR(pch->regmap);
+	}
+
+	i2c_set_clientdata(client, pch);
+
+	pch->rtc = devm_rtc_device_register(&client->dev, "pch-rtc",
+					    &pch_rtc_ops, THIS_MODULE);
+	if (IS_ERR(pch->rtc)) {
+		dev_err(&client->dev, "rtc device register failed\n");
+		return PTR_ERR(pch->rtc);
+	}
+
+	rc = sysfs_create_file(&client->dev.kobj, &dev_attr_force_off.attr);
+	if (rc) {
+		dev_err(&client->dev, "couldn't create sysfs attr : %i\n", rc);
+		return rc;
+	}
+
+	return 0;
+}
+
+static int pch_rtc_remove(struct i2c_client *client)
+{
+	sysfs_remove_file(&client->dev.kobj, &dev_attr_force_off.attr);
+	return 0;
+}
+
+static const struct of_device_id pch_rtc_of_match[] = {
+	{ .compatible = "intel,pch-rtc", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pch_rtc_of_match);
+
+static struct i2c_driver pch_rtc_driver = {
+	.driver		= {
+		.name	= "pch-rtc",
+		.of_match_table = pch_rtc_of_match,
+	},
+	.probe_new	= pch_rtc_probe,
+	.remove		= pch_rtc_remove,
+};
+module_i2c_driver(pch_rtc_driver);
+
+MODULE_DESCRIPTION("RTC PCH driver");
+MODULE_AUTHOR("Ivan Mikhaylov <i.mikhaylov@yadro.com>");
+MODULE_LICENSE("GPL");
-- 
2.31.1

[PATCH 2/2] dt-bindings: rtc: provide RTC PCH device tree binding doc

[Ivan Mikhaylov]

Add I2C Intel Series PCH built-in read-only RTC driver documentation.

Signed-off-by: Ivan Mikhaylov <i.mikhaylov@yadro.com>
---
 .../bindings/rtc/intel,pch-rtc.yaml           | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/intel,pch-rtc.yaml

diff --git a/Documentation/devicetree/bindings/rtc/intel,pch-rtc.yaml b/Documentation/devicetree/bindings/rtc/intel,pch-rtc.yaml
new file mode 100644
index 000000000000..f49867257f93
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/intel,pch-rtc.yaml
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rtc/intel,pch-rtc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intel Series PCH built-in read-only RTC
+
+maintainers:
+  - Ivan Mikhaylov <i.mikhaylov@yadro.com>
+
+allOf:
+  - $ref: rtc.yaml#
+
+properties:
+  compatible:
+    enum:
+      - intel,pch-rtc
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        rtc@0 {
+            compatible = "intel,pch-rtc";
+            reg = <0x44>;
+        };
+    };
-- 
2.31.1

Re: [PATCH 0/2] rtc: pch-rtc: add Intel Series PCH built-in read-only RTC

[Paul Fertser]

On Tue, Aug 10, 2021 at 06:44:34PM +0300, Ivan Mikhaylov wrote:
> Add RTC driver with dt binding tree document. Also this driver adds one sysfs
> attribute for host power control which I think is odd for RTC driver.
> Need I cut it off and use I2C_SLAVE_FORCE? I2C_SLAVE_FORCE is not good
> way too from my point of view. Is there any better approach?

Reading the C620 datasheet I see this interface also allows other
commands (wake up, watchdog feeding, reboot etc.) and reading statuses
(e.g Intruder Detect, POWER_OK_BAD).

I think if there's any plan to use anything other but RTC via this
interface then the driver should be registered as an MFD.

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

Re: [PATCH 1/2] rtc: pch-rtc: add RTC driver for Intel Series PCH

[Paul Fertser]

On Tue, Aug 10, 2021 at 06:44:35PM +0300, Ivan Mikhaylov wrote:
> +config RTC_DRV_PCH
> +	tristate "PCH RTC driver"
> +	help
> +	  If you say yes here you get support for the Intel Series PCH

I'm afraid this is really lacking some specification of devices that
are supported. Is it really everything that Intel currently calls PCH?

> +static int pch_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct pch *pch = i2c_get_clientdata(client);
> +	unsigned char rtc_data[NUM_TIME_REGS] = {0};
> +	int rc;
> +
> +	rc = regmap_bulk_read(pch->regmap, PCH_REG_SC, rtc_data, NUM_TIME_REGS);
> +	if (rc < 0) {
> +		dev_err(dev, "fail to read time reg(%d)\n", rc);
> +		return rc;
> +	}

Citing 26.7.2.3 from C620 (Lewisburg/Purley) datasheet:

"The PCH SMBus slave interface only supports Byte Read operation. The
external SMBus master will read the RTC time bytes one after
another. It is software’s responsibility to check and manage the
possible time rollover when subsequent time bytes are read.

For example, assuming the RTC time is 11 hours: 59 minutes: 59
seconds. When the external SMBus master reads the hour as 11, then
proceeds to read the minute, it is possible that the rollover happens
between the reads and the minute is read as 0. This results in 11
hours: 0 minutes instead of the correct time of 12 hours: 0 minutes.
Unless it is certain that rollover will not occur, software is
required to detect the possible time rollover by reading multiple
times such that the read time bytes can be adjusted accordingly if
needed."

Should this be taken additional care of somehow?

> +static ssize_t force_off_store(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct pch *pch = i2c_get_clientdata(client);
> +	unsigned long val;
> +	int rc;
> +
> +	if (kstrtoul(buf, 10, &val))
> +		return -EINVAL;
> +
> +	if (val) {
> +		/* 0x02 host force off */

I wonder why you write "host force off" while the C620 datasheet calls
it "Unconditional Power Down", does your PCH manual use different
naming?

In any case this doesn't belong to an RTC driver, as previously noted.

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

Re: [PATCH 0/2] rtc: pch-rtc: add Intel Series PCH built-in read-only RTC

[Alexandre Belloni]

On 15/08/2021 01:42:15+0300, Paul Fertser wrote:
> On Tue, Aug 10, 2021 at 06:44:34PM +0300, Ivan Mikhaylov wrote:
> > Add RTC driver with dt binding tree document. Also this driver adds one sysfs
> > attribute for host power control which I think is odd for RTC driver.
> > Need I cut it off and use I2C_SLAVE_FORCE? I2C_SLAVE_FORCE is not good
> > way too from my point of view. Is there any better approach?
> 
> Reading the C620 datasheet I see this interface also allows other
> commands (wake up, watchdog feeding, reboot etc.) and reading statuses
> (e.g Intruder Detect, POWER_OK_BAD).
> 
> I think if there's any plan to use anything other but RTC via this
> interface then the driver should be registered as an MFD.
> 

This is not the current thinking, if everything is integrated, then
there is no issue registering a watchdog from the RTC driver. I'll let
you check with Lee...

However, I'm not sure what is the correct interface for poweroff/reboot
control.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

RE: [PATCH 0/2] rtc: pch-rtc: add Intel Series PCH built-in read-only RTC

[Milton Miller II]

On Aug 16, 2021, Alexandre Belloni wrote:
>On 15/08/2021 01:42:15+0300, Paul Fertser wrote:
>> On Tue, Aug 10, 2021 at 06:44:34PM +0300, Ivan Mikhaylov wrote:
>> > Add RTC driver with dt binding tree document. Also this driver
>adds one sysfs
>> > attribute for host power control which I think is odd for RTC
>driver.
>> > Need I cut it off and use I2C_SLAVE_FORCE? I2C_SLAVE_FORCE is not
>good
>> > way too from my point of view. Is there any better approach?
>> 
>> Reading the C620 datasheet I see this interface also allows other
>> commands (wake up, watchdog feeding, reboot etc.) and reading
>statuses
>> (e.g Intruder Detect, POWER_OK_BAD).
>> 
>> I think if there's any plan to use anything other but RTC via this
>> interface then the driver should be registered as an MFD.
>> 
>
>This is not the current thinking, if everything is integrated, then
>there is no issue registering a watchdog from the RTC driver. I'll
>let
>you check with Lee...

I think the current statement is "if they are truly disjoint 
hardware controls" then an MFD might suffice, but if they require 
software cordination the new auxillary bus seems to be desired.

>>However, I'm not sure what is the correct interface for
>poweroff/reboot
>control.

While there is a gpio interface to a simple regulator switch,
the project to date has been asserting direct or indirect 
gpios etc to control the host.   If these are events to 
trigger a change in state and not a direct state change
that some controller trys to follow, maybe a message delivery 
model?   (this is not to reboot or cycle the bmc).

milton

Re: [PATCH 0/2] rtc: pch-rtc: add Intel Series PCH built-in read-only RTC

[Alexandre Belloni]

On 17/08/2021 18:04:09+0000, Milton Miller II wrote:
> 
> On Aug 16, 2021, Alexandre Belloni wrote:
> >On 15/08/2021 01:42:15+0300, Paul Fertser wrote:
> >> On Tue, Aug 10, 2021 at 06:44:34PM +0300, Ivan Mikhaylov wrote:
> >> > Add RTC driver with dt binding tree document. Also this driver
> >adds one sysfs
> >> > attribute for host power control which I think is odd for RTC
> >driver.
> >> > Need I cut it off and use I2C_SLAVE_FORCE? I2C_SLAVE_FORCE is not
> >good
> >> > way too from my point of view. Is there any better approach?
> >> 
> >> Reading the C620 datasheet I see this interface also allows other
> >> commands (wake up, watchdog feeding, reboot etc.) and reading
> >statuses
> >> (e.g Intruder Detect, POWER_OK_BAD).
> >> 
> >> I think if there's any plan to use anything other but RTC via this
> >> interface then the driver should be registered as an MFD.
> >> 
> >
> >This is not the current thinking, if everything is integrated, then
> >there is no issue registering a watchdog from the RTC driver. I'll
> >let
> >you check with Lee...
> 
> I think the current statement is "if they are truly disjoint 
> hardware controls" then an MFD might suffice, but if they require 
> software cordination the new auxillary bus seems to be desired.
> 

Honestly, the auxiliary bus doesn't provide anything that you can't do
by registering a device in multiple subsystem from a single driver.
(Lee Jones, Mark Brown and I did complain at the time that this was yet
another back channel for misuses).

> >>However, I'm not sure what is the correct interface for
> >poweroff/reboot
> >control.
> 
> While there is a gpio interface to a simple regulator switch,
> the project to date has been asserting direct or indirect 
> gpios etc to control the host.   If these are events to 
> trigger a change in state and not a direct state change
> that some controller trys to follow, maybe a message delivery 
> model?   (this is not to reboot or cycle the bmc).
> 
> milton

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 1/2] rtc: pch-rtc: add RTC driver for Intel Series PCH

[Ivan Mikhaylov]

On Tue, Aug 15, 2021 at 01:52:42PM +0300, Paul Fertser wrote:
> On Tue, Aug 10, 2021 at 06:44:35PM +0300, Ivan Mikhaylov wrote:
> > +config RTC_DRV_PCH
> > +	tristate "PCH RTC driver"
> > +	help
> > +	  If you say yes here you get support for the Intel Series PCH
>
> I'm afraid this is really lacking some specification of devices that
> are supported. Is it really everything that Intel currently calls PCH?

Yes, from infromation that I know.

> > +static int pch_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct pch *pch = i2c_get_clientdata(client);
> > +	unsigned char rtc_data[NUM_TIME_REGS] = {0};
> > +	int rc;
> > +
> > +	rc = regmap_bulk_read(pch->regmap, PCH_REG_SC, rtc_data, NUM_TIME_REGS);
> > +	if (rc < 0) {
> > +		dev_err(dev, "fail to read time reg(%d)\n", rc);
> > +		return rc;
> > +	}
>
> Citing 26.7.2.3 from C620 (Lewisburg/Purley) datasheet:
> 
> "The PCH SMBus slave interface only supports Byte Read operation. The
> external SMBus master will read the RTC time bytes one after
> another. It is software’s responsibility to check and manage the
> possible time rollover when subsequent time bytes are read.
> 
> For example, assuming the RTC time is 11 hours: 59 minutes: 59
> seconds. When the external SMBus master reads the hour as 11, then
> proceeds to read the minute, it is possible that the rollover happens
> between the reads and the minute is read as 0. This results in 11
> hours: 0 minutes instead of the correct time of 12 hours: 0 minutes.
> Unless it is certain that rollover will not occur, software is
> required to detect the possible time rollover by reading multiple
> times such that the read time bytes can be adjusted accordingly if
> needed."
> 
> Should this be taken additional care of somehow?

1. .use_single_read in regmap_config.
2. Maybe that is the right place for rollover check.

> > +static ssize_t force_off_store(struct device *dev,
> > +			       struct device_attribute *attr,
> > +			       const char *buf, size_t count)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct pch *pch = i2c_get_clientdata(client);
> > +	unsigned long val;
> > +	int rc;
> > +
> > +	if (kstrtoul(buf, 10, &val))
> > +		return -EINVAL;
> > +
> > +	if (val) {
> > +		/* 0x02 host force off */
> 
> I wonder why you write "host force off" while the C620 datasheet calls
> it "Unconditional Power Down", does your PCH manual use different
> naming?

It just a synonym, does the same. I can change it but first it's need to
be decided if attribute would go or not.

Thanks.

Re: [PATCH 0/2] rtc: pch-rtc: add Intel Series PCH built-in read-only RTC

[Ivan Mikhaylov]

On Tue, 2021-08-17 at 22:05 +0200, Alexandre Belloni wrote:
> On 17/08/2021 18:04:09+0000, Milton Miller II wrote:
> > 
> > On Aug 16, 2021, Alexandre Belloni wrote:
> > > On 15/08/2021 01:42:15+0300, Paul Fertser wrote:
> > > > On Tue, Aug 10, 2021 at 06:44:34PM +0300, Ivan Mikhaylov wrote:
> > > > > Add RTC driver with dt binding tree document. Also this driver
> > > adds one sysfs
> > > > > attribute for host power control which I think is odd for RTC
> > > driver.
> > > > > Need I cut it off and use I2C_SLAVE_FORCE? I2C_SLAVE_FORCE is not
> > > good
> > > > > way too from my point of view. Is there any better approach?
> > > > 
> > > > Reading the C620 datasheet I see this interface also allows other
> > > > commands (wake up, watchdog feeding, reboot etc.) and reading
> > > statuses
> > > > (e.g Intruder Detect, POWER_OK_BAD).
> > > > 
> > > > I think if there's any plan to use anything other but RTC via this
> > > > interface then the driver should be registered as an MFD.
> > > > 
> > > 
> > > This is not the current thinking, if everything is integrated, then
> > > there is no issue registering a watchdog from the RTC driver. I'll
> > > let
> > > you check with Lee...
> > 
> > I think the current statement is "if they are truly disjoint 
> > hardware controls" then an MFD might suffice, but if they require 
> > software cordination the new auxillary bus seems to be desired.
> > 
> 
> Honestly, the auxiliary bus doesn't provide anything that you can't do
> by registering a device in multiple subsystem from a single driver.
> (Lee Jones, Mark Brown and I did complain at the time that this was yet
> another back channel for misuses).
> 
> > > > However, I'm not sure what is the correct interface for
> > > poweroff/reboot
> > > control.
> > 
> > While there is a gpio interface to a simple regulator switch,
> > the project to date has been asserting direct or indirect 
> > gpios etc to control the host.   If these are events to 
> > trigger a change in state and not a direct state change
> > that some controller trys to follow, maybe a message delivery 
> > model?   (this is not to reboot or cycle the bmc).
> > 
> > milton
> 

Alexandre, gentle reminder about this one series. I can get rid off from sysfs
attribute and put it like RO rtc without any additional things for now as
starter.

Thanks.

Re: [PATCH 0/2] rtc: pch-rtc: add Intel Series PCH built-in read-only RTC

[Ivan Mikhaylov]

On Mon, 2021-08-30 at 14:56 +0300, Ivan Mikhaylov wrote:
> On Tue, 2021-08-17 at 22:05 +0200, Alexandre Belloni wrote:
> > On 17/08/2021 18:04:09+0000, Milton Miller II wrote:
> > > 
> > > On Aug 16, 2021, Alexandre Belloni wrote:
> > > > On 15/08/2021 01:42:15+0300, Paul Fertser wrote:
> > > > > On Tue, Aug 10, 2021 at 06:44:34PM +0300, Ivan Mikhaylov wrote:
> > > > > > Add RTC driver with dt binding tree document. Also this driver
> > > > adds one sysfs
> > > > > > attribute for host power control which I think is odd for RTC
> > > > driver.
> > > > > > Need I cut it off and use I2C_SLAVE_FORCE? I2C_SLAVE_FORCE is not
> > > > good
> > > > > > way too from my point of view. Is there any better approach?
> > > > > 
> > > > > Reading the C620 datasheet I see this interface also allows other
> > > > > commands (wake up, watchdog feeding, reboot etc.) and reading
> > > > statuses
> > > > > (e.g Intruder Detect, POWER_OK_BAD).
> > > > > 
> > > > > I think if there's any plan to use anything other but RTC via this
> > > > > interface then the driver should be registered as an MFD.
> > > > > 
> > > > 
> > > > This is not the current thinking, if everything is integrated, then
> > > > there is no issue registering a watchdog from the RTC driver. I'll
> > > > let
> > > > you check with Lee...
> > > 
> > > I think the current statement is "if they are truly disjoint 
> > > hardware controls" then an MFD might suffice, but if they require 
> > > software cordination the new auxillary bus seems to be desired.
> > > 
> > 
> > Honestly, the auxiliary bus doesn't provide anything that you can't do
> > by registering a device in multiple subsystem from a single driver.
> > (Lee Jones, Mark Brown and I did complain at the time that this was yet
> > another back channel for misuses).
> > 
> > > > > However, I'm not sure what is the correct interface for
> > > > poweroff/reboot
> > > > control.
> > > 
> > > While there is a gpio interface to a simple regulator switch,
> > > the project to date has been asserting direct or indirect 
> > > gpios etc to control the host.   If these are events to 
> > > trigger a change in state and not a direct state change
> > > that some controller trys to follow, maybe a message delivery 
> > > model?   (this is not to reboot or cycle the bmc).
> > > 
> > > milton
> > 
> 
> Alexandre, gentle reminder about this one series. I can get rid off from sysfs
> attribute and put it like RO rtc without any additional things for now as
> starter.
> 
> Thanks.
> 

ping

Thanks.

Re: [PATCH 1/2] rtc: pch-rtc: add RTC driver for Intel Series PCH

[Alexandre Belloni]

On 20/08/2021 15:34:10+0300, Ivan Mikhaylov wrote:
> On Tue, Aug 15, 2021 at 01:52:42PM +0300, Paul Fertser wrote:
> > On Tue, Aug 10, 2021 at 06:44:35PM +0300, Ivan Mikhaylov wrote:
> > > +config RTC_DRV_PCH
> > > +	tristate "PCH RTC driver"
> > > +	help
> > > +	  If you say yes here you get support for the Intel Series PCH
> >
> > I'm afraid this is really lacking some specification of devices that
> > are supported. Is it really everything that Intel currently calls PCH?
> 
> Yes, from infromation that I know.
> 
> > > +static int pch_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > > +{
> > > +	struct i2c_client *client = to_i2c_client(dev);
> > > +	struct pch *pch = i2c_get_clientdata(client);
> > > +	unsigned char rtc_data[NUM_TIME_REGS] = {0};
> > > +	int rc;
> > > +
> > > +	rc = regmap_bulk_read(pch->regmap, PCH_REG_SC, rtc_data, NUM_TIME_REGS);
> > > +	if (rc < 0) {
> > > +		dev_err(dev, "fail to read time reg(%d)\n", rc);
> > > +		return rc;
> > > +	}
> >
> > Citing 26.7.2.3 from C620 (Lewisburg/Purley) datasheet:
> > 
> > "The PCH SMBus slave interface only supports Byte Read operation. The
> > external SMBus master will read the RTC time bytes one after
> > another. It is software’s responsibility to check and manage the
> > possible time rollover when subsequent time bytes are read.
> > 
> > For example, assuming the RTC time is 11 hours: 59 minutes: 59
> > seconds. When the external SMBus master reads the hour as 11, then
> > proceeds to read the minute, it is possible that the rollover happens
> > between the reads and the minute is read as 0. This results in 11
> > hours: 0 minutes instead of the correct time of 12 hours: 0 minutes.
> > Unless it is certain that rollover will not occur, software is
> > required to detect the possible time rollover by reading multiple
> > times such that the read time bytes can be adjusted accordingly if
> > needed."
> > 
> > Should this be taken additional care of somehow?
> 
> 1. .use_single_read in regmap_config.
> 2. Maybe that is the right place for rollover check.
> 
> > > +static ssize_t force_off_store(struct device *dev,
> > > +			       struct device_attribute *attr,
> > > +			       const char *buf, size_t count)
> > > +{
> > > +	struct i2c_client *client = to_i2c_client(dev);
> > > +	struct pch *pch = i2c_get_clientdata(client);
> > > +	unsigned long val;
> > > +	int rc;
> > > +
> > > +	if (kstrtoul(buf, 10, &val))
> > > +		return -EINVAL;
> > > +
> > > +	if (val) {
> > > +		/* 0x02 host force off */
> > 
> > I wonder why you write "host force off" while the C620 datasheet calls
> > it "Unconditional Power Down", does your PCH manual use different
> > naming?
> 
> It just a synonym, does the same. I can change it but first it's need to
> be decided if attribute would go or not.
> 

I would prefer if this was added later on in a later patch, with the
proper ABI documentation so that the ABI can be discussed.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com