From: Ivan Mikhaylov <fr0st61te@gmail.com> To: fercerpav@gmail.com Cc: a.zummo@towertech.it, alexandre.belloni@bootlin.com, i.mikhaylov@yadro.com, linux-kernel@vger.kernel.org, openbmc@lists.ozlabs.org Subject: Re: [PATCH 1/2] rtc: pch-rtc: add RTC driver for Intel Series PCH Date: Fri, 20 Aug 2021 15:34:10 +0300 [thread overview] Message-ID: <20210820123410.46539-1-fr0st61te@gmail.com> (raw) In-Reply-To: <20210814225242.GY15173@home.paul.comp> 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.
WARNING: multiple messages have this Message-ID (diff)
From: Ivan Mikhaylov <fr0st61te@gmail.com> To: fercerpav@gmail.com Cc: i.mikhaylov@yadro.com, a.zummo@towertech.it, alexandre.belloni@bootlin.com, linux-kernel@vger.kernel.org, openbmc@lists.ozlabs.org Subject: Re: [PATCH 1/2] rtc: pch-rtc: add RTC driver for Intel Series PCH Date: Fri, 20 Aug 2021 15:34:10 +0300 [thread overview] Message-ID: <20210820123410.46539-1-fr0st61te@gmail.com> (raw) In-Reply-To: <20210814225242.GY15173@home.paul.comp> 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.
next prev parent reply other threads:[~2021-08-20 12:34 UTC|newest] Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-08-10 15:44 [PATCH 0/2] rtc: pch-rtc: add Intel Series PCH built-in read-only RTC Ivan Mikhaylov 2021-08-10 15:44 ` Ivan Mikhaylov 2021-08-10 15:44 ` [PATCH 1/2] rtc: pch-rtc: add RTC driver for Intel Series PCH Ivan Mikhaylov 2021-08-10 15:44 ` Ivan Mikhaylov 2021-08-14 22:52 ` Paul Fertser 2021-08-20 12:34 ` Ivan Mikhaylov [this message] 2021-08-20 12:34 ` Ivan Mikhaylov 2021-09-25 22:24 ` Alexandre Belloni 2021-09-25 22:24 ` Alexandre Belloni 2021-08-10 15:44 ` [PATCH 2/2] dt-bindings: rtc: provide RTC PCH device tree binding doc Ivan Mikhaylov 2021-08-10 15:44 ` Ivan Mikhaylov 2021-08-14 22:42 ` [PATCH 0/2] rtc: pch-rtc: add Intel Series PCH built-in read-only RTC Paul Fertser 2021-08-14 23:22 ` Alexandre Belloni 2021-08-17 18:04 ` Milton Miller II 2021-08-17 18:04 ` Milton Miller II 2021-08-17 20:05 ` Alexandre Belloni 2021-08-17 20:05 ` Alexandre Belloni 2021-08-30 11:56 ` Ivan Mikhaylov 2021-08-30 11:56 ` Ivan Mikhaylov 2021-09-14 23:52 ` Ivan Mikhaylov 2021-09-14 23:52 ` Ivan Mikhaylov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210820123410.46539-1-fr0st61te@gmail.com \ --to=fr0st61te@gmail.com \ --cc=a.zummo@towertech.it \ --cc=alexandre.belloni@bootlin.com \ --cc=fercerpav@gmail.com \ --cc=i.mikhaylov@yadro.com \ --cc=linux-kernel@vger.kernel.org \ --cc=openbmc@lists.ozlabs.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.