* [PATCH 1/5] rtc: ds1307: Keep DT based RTC configuration from breaking DS1337/41
2016-06-07 20:01 [PATCH 0/5] Fix problems with DS1307 RTC driver Trent Piepho
@ 2016-06-07 20:03 ` Trent Piepho
2016-06-07 20:04 ` [PATCH 2/5] rtc: ds1307: Remove unused and unneeded driver code Trent Piepho
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Trent Piepho @ 2016-06-07 20:03 UTC (permalink / raw)
To: barebox; +Cc: Andrey Smirnov
The DS1337 and DS1341 use a different configuration register layout
than the other supported ds1307 RTC chips. The code that does the
DT based configuration doesn't support this layout and will
incorrectly program them.
This disables DT configuration for DS1337 and DS1341.
Signed-off-by: Trent Piepho <tpiepho@kymetacorp.com>
---
drivers/rtc/rtc-ds1307.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 73d88ba..86cba6e 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -382,7 +382,8 @@ read_rtc:
}
/* Configure clock using OF data if available */
- if (IS_ENABLED(CONFIG_OFDEVICE) && np) {
+ if (IS_ENABLED(CONFIG_OFDEVICE) && np &&
+ ds1307->type != ds_1337 && ds1307->type != ds_1341) {
u8 control = ds1307->regs[DS1307_REG_CONTROL];
u32 rate = 0;
--
2.7.0.25.gfc10eb5.dirty
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 2/5] rtc: ds1307: Remove unused and unneeded driver code
2016-06-07 20:01 [PATCH 0/5] Fix problems with DS1307 RTC driver Trent Piepho
2016-06-07 20:03 ` [PATCH 1/5] rtc: ds1307: Keep DT based RTC configuration from breaking DS1337/41 Trent Piepho
@ 2016-06-07 20:04 ` Trent Piepho
2016-06-07 20:06 ` [PATCH 3/5] rtc: ds1307: Clean up of chip variant support Trent Piepho
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Trent Piepho @ 2016-06-07 20:04 UTC (permalink / raw)
To: barebox; +Cc: Andrey Smirnov
There is a bunch of unused stuff in this driver from the Linux Kernel
import.
Get rid of offset in driver state, was always zero.
Get rid of flags, was always zero and never used.
Get rid of pointers to read/write block data functions as they always
point to the single set present in the current code.
Get rid of reg cache. It was never used as a cache and wasn't always
kept up to date with register modifications. When ds1337 support was
added it it didn't put the new registers in the correctly location.
Add some macros for register range starts and ends, rather than magic
0s and 7s in various functions.
Decrease stack buffer in read/write block functions from 255 bytes to
16, keyed to register definition, as that's all that's ever needed.
Get rid of switch statement that had only default case.
Signed-off-by: Trent Piepho <tpiepho@kymetacorp.com>
---
drivers/rtc/rtc-ds1307.c | 117 ++++++++++++++++++++---------------------------
1 file changed, 50 insertions(+), 67 deletions(-)
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 86cba6e..5ed64bf 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -54,6 +54,10 @@ enum ds_type {
# define DS1337_BIT_CENTURY 0x80 /* in REG_MONTH */
#define DS1307_REG_YEAR 0x06 /* 00-99 */
+/* Register block for above time registers */
+#define DS13xx_REG_TIME_START DS1307_REG_SECS
+#define DS13xx_REG_TIME_COUNT (DS1307_REG_YEAR - DS13xx_REG_TIME_START + 1)
+
/*
* Other registers (control, status, alarms, trickle charge, NVRAM, etc)
* start at 7, and they differ a LOT. Only control and status matter for
@@ -91,19 +95,14 @@ enum ds_type {
# define DS1341_BIT_ECLK 0x04
# define DS1337_BIT_A2I 0x02
# define DS1337_BIT_A1I 0x01
+/* Most registers for any device */
+#define DS13xx_REG_COUNT 16
struct ds1307 {
struct rtc_device rtc;
- u8 offset; /* register's offset */
- u8 regs[11];
enum ds_type type;
- unsigned long flags;
struct i2c_client *client;
- s32 (*read_block_data)(const struct i2c_client *client, u8 command,
- u8 length, u8 *values);
- s32 (*write_block_data)(const struct i2c_client *client, u8 command,
- u8 length, const u8 *values);
};
static struct platform_device_id ds1307_id[] = {
@@ -137,7 +136,7 @@ static s32 ds1307_read_block_data_once(const struct i2c_client *client,
static s32 ds1307_read_block_data(const struct i2c_client *client, u8 command,
u8 length, u8 *values)
{
- u8 oldvalues[255];
+ u8 oldvalues[DS13xx_REG_COUNT];
s32 ret;
int tries = 0;
@@ -163,7 +162,7 @@ static s32 ds1307_read_block_data(const struct i2c_client *client, u8 command,
static s32 ds1307_write_block_data(const struct i2c_client *client, u8 command,
u8 length, const u8 *values)
{
- u8 currvalues[255];
+ u8 currvalues[DS13xx_REG_COUNT];
int tries = 0;
dev_dbg(&client->dev, "ds1307_write_block_data (length=%d)\n", length);
@@ -198,29 +197,30 @@ static int ds1307_get_time(struct rtc_device *rtcdev, struct rtc_time *t)
{
struct device_d *dev = rtcdev->dev;
struct ds1307 *ds1307 = to_ds1307_priv(rtcdev);
+ u8 buf[DS13xx_REG_TIME_COUNT];
int tmp;
/* read the RTC date and time registers all at once */
- tmp = ds1307->read_block_data(ds1307->client,
- ds1307->offset, 7, ds1307->regs);
- if (tmp != 7) {
+ tmp = ds1307_read_block_data(ds1307->client, DS13xx_REG_TIME_START,
+ DS13xx_REG_TIME_COUNT, buf);
+ if (tmp != DS13xx_REG_TIME_COUNT) {
dev_err(dev, "%s error %d\n", "read", tmp);
return -EIO;
}
- dev_dbg(dev, "%s: %7ph\n", "read", ds1307->regs);
+ dev_dbg(dev, "%s: %7ph\n", "read", buf);
- t->tm_sec = bcd2bin(ds1307->regs[DS1307_REG_SECS] & 0x7f);
- t->tm_min = bcd2bin(ds1307->regs[DS1307_REG_MIN] & 0x7f);
- tmp = ds1307->regs[DS1307_REG_HOUR] & 0x3f;
+ t->tm_sec = bcd2bin(buf[DS1307_REG_SECS] & 0x7f);
+ t->tm_min = bcd2bin(buf[DS1307_REG_MIN] & 0x7f);
+ tmp = buf[DS1307_REG_HOUR] & 0x3f;
t->tm_hour = bcd2bin(tmp);
- t->tm_wday = bcd2bin(ds1307->regs[DS1307_REG_WDAY] & 0x07) - 1;
- t->tm_mday = bcd2bin(ds1307->regs[DS1307_REG_MDAY] & 0x3f);
- tmp = ds1307->regs[DS1307_REG_MONTH] & 0x1f;
+ t->tm_wday = bcd2bin(buf[DS1307_REG_WDAY] & 0x07) - 1;
+ t->tm_mday = bcd2bin(buf[DS1307_REG_MDAY] & 0x3f);
+ tmp = buf[DS1307_REG_MONTH] & 0x1f;
t->tm_mon = bcd2bin(tmp) - 1;
/* assume 20YY not 19YY, and ignore DS1337_BIT_CENTURY */
- t->tm_year = bcd2bin(ds1307->regs[DS1307_REG_YEAR]) + 100;
+ t->tm_year = bcd2bin(buf[DS1307_REG_YEAR]) + 100;
dev_dbg(dev, "%s secs=%d, mins=%d, "
"hours=%d, mday=%d, mon=%d, year=%d, wday=%d\n",
@@ -238,7 +238,7 @@ static int ds1307_set_time(struct rtc_device *rtcdev, struct rtc_time *t)
struct ds1307 *ds1307 = to_ds1307_priv(rtcdev);
int result;
int tmp;
- u8 *buf = ds1307->regs;
+ u8 buf[DS13xx_REG_TIME_COUNT];
dev_dbg(dev, "%s secs=%d, mins=%d, "
"hours=%d, mday=%d, mon=%d, year=%d, wday=%d\n",
@@ -269,8 +269,8 @@ static int ds1307_set_time(struct rtc_device *rtcdev, struct rtc_time *t)
dev_dbg(dev, "%s: %7ph\n", "write", buf);
- result = ds1307->write_block_data(ds1307->client,
- ds1307->offset, 7, buf);
+ result = ds1307_write_block_data(ds1307->client, DS13xx_REG_TIME_START,
+ DS13xx_REG_TIME_COUNT, buf);
if (result < 0) {
dev_err(dev, "%s error %d\n", "write", result);
return result;
@@ -289,7 +289,7 @@ static int ds1307_probe(struct device_d *dev)
struct ds1307 *ds1307;
int err = -ENODEV;
int tmp;
- unsigned char *buf;
+ unsigned char buf[DS13xx_REG_COUNT];
unsigned long driver_data;
const struct device_node *np = dev->device_node;
@@ -302,18 +302,12 @@ static int ds1307_probe(struct device_d *dev)
ds1307->client = client;
ds1307->type = driver_data;
- buf = ds1307->regs;
-
- ds1307->read_block_data = ds1307_read_block_data;
- ds1307->write_block_data = ds1307_write_block_data;
-
-
switch (ds1307->type) {
case ds_1337:
case ds_1341:
/* get registers that the "rtc" read below won't read... */
- tmp = ds1307->read_block_data(ds1307->client,
- DS1337_REG_CONTROL, 2, buf);
+ tmp = ds1307_read_block_data(ds1307->client,
+ DS1337_REG_CONTROL, 2, buf);
if (tmp != 2) {
dev_dbg(&client->dev, "read error %d\n", tmp);
@@ -322,8 +316,8 @@ static int ds1307_probe(struct device_d *dev)
}
/* oscillator off? turn it on, so clock can tick. */
- if (ds1307->regs[0] & DS1337_BIT_nEOSC)
- ds1307->regs[0] &= ~DS1337_BIT_nEOSC;
+ if (buf[0] & DS1337_BIT_nEOSC)
+ buf[0] &= ~DS1337_BIT_nEOSC;
/*
@@ -334,37 +328,37 @@ static int ds1307_probe(struct device_d *dev)
wave generation), but disabling each individual
alarm interrupt source
*/
- ds1307->regs[0] |= DS1337_BIT_INTCN;
- ds1307->regs[0] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);
+ buf[0] |= DS1337_BIT_INTCN;
+ buf[0] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);
i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
- ds1307->regs[0]);
+ buf[0]);
if (ds1307->type == ds_1341) {
/*
* For the above to be true, DS1341 also has to have
* ECLK bit set to 0
*/
- ds1307->regs[1] &= ~DS1341_BIT_ECLK;
+ buf[1] &= ~DS1341_BIT_ECLK;
/*
* Let's set additionale RTC bits to
* facilitate maximum power saving.
*/
- ds1307->regs[0] |= DS1341_BIT_DOSF;
- ds1307->regs[0] &= ~DS1341_BIT_EGFIL;
+ buf[0] |= DS1341_BIT_DOSF;
+ buf[0] &= ~DS1341_BIT_EGFIL;
i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
- ds1307->regs[0]);
+ buf[0]);
i2c_smbus_write_byte_data(client, DS1337_REG_STATUS,
- ds1307->regs[1]);
+ buf[1]);
}
/* oscillator fault? clear flag, and warn */
- if (ds1307->regs[1] & DS1337_BIT_OSF) {
+ if (buf[1] & DS1337_BIT_OSF) {
i2c_smbus_write_byte_data(client, DS1337_REG_STATUS,
- ds1307->regs[1] & ~DS1337_BIT_OSF);
+ buf[1] & ~DS1337_BIT_OSF);
dev_warn(&client->dev, "SET TIME!\n");
}
@@ -374,7 +368,7 @@ static int ds1307_probe(struct device_d *dev)
read_rtc:
/* read RTC registers */
- tmp = ds1307->read_block_data(client, ds1307->offset, 8, buf);
+ tmp = ds1307_read_block_data(client, 0, 8, buf);
if (tmp != 8) {
dev_dbg(&client->dev, "read error %d\n", tmp);
err = -EIO;
@@ -384,7 +378,7 @@ read_rtc:
/* Configure clock using OF data if available */
if (IS_ENABLED(CONFIG_OFDEVICE) && np &&
ds1307->type != ds_1337 && ds1307->type != ds_1341) {
- u8 control = ds1307->regs[DS1307_REG_CONTROL];
+ u8 control = buf[DS1307_REG_CONTROL];
u32 rate = 0;
if (of_property_read_bool(np, "ext-clock-input"))
@@ -415,18 +409,14 @@ read_rtc:
}
dev_dbg(&client->dev, "control reg: 0x%02x\n", control);
- if (ds1307->regs[DS1307_REG_CONTROL] != control) {
+ if (buf[DS1307_REG_CONTROL] != control) {
i2c_smbus_write_byte_data(client, DS1307_REG_CONTROL, control);
- ds1307->regs[DS1307_REG_CONTROL] = control;
+ buf[DS1307_REG_CONTROL] = control;
}
}
- /*
- * minimal sanity checking; some chips (like DS1340) don't
- * specify the extra bits as must-be-zero, but there are
- * still a few values that are clearly out-of-range.
- */
- tmp = ds1307->regs[DS1307_REG_SECS];
+ /* Check for clock halted conditions and start clock */
+ tmp = buf[DS1307_REG_SECS];
switch (ds1307->type) {
case ds_1307:
/* clock halted? turn it on, so clock can tick. */
@@ -442,9 +432,9 @@ read_rtc:
i2c_smbus_write_byte_data(client, DS1307_REG_SECS, 0);
/* oscillator fault? clear flag, and warn */
- if (ds1307->regs[DS1307_REG_CONTROL] & DS1338_BIT_OSF) {
+ if (buf[DS1307_REG_CONTROL] & DS1338_BIT_OSF) {
i2c_smbus_write_byte_data(client, DS1307_REG_CONTROL,
- ds1307->regs[DS1307_REG_CONTROL]
+ buf[DS1307_REG_CONTROL]
& ~DS1338_BIT_OSF);
dev_warn(&client->dev, "SET TIME!\n");
goto read_rtc;
@@ -454,12 +444,7 @@ read_rtc:
break;
}
- tmp = ds1307->regs[DS1307_REG_HOUR];
- switch (ds1307->type) {
- default:
- if (!(tmp & DS1307_BIT_12HR))
- break;
-
+ if (buf[DS1307_REG_HOUR] & DS1307_BIT_12HR) {
/*
* Be sure we're in 24 hour mode. Multi-master systems
* take note...
@@ -467,11 +452,9 @@ read_rtc:
tmp = bcd2bin(tmp & 0x1f);
if (tmp == 12)
tmp = 0;
- if (ds1307->regs[DS1307_REG_HOUR] & DS1307_BIT_PM)
+ if (buf[DS1307_REG_HOUR] & DS1307_BIT_PM)
tmp += 12;
- i2c_smbus_write_byte_data(client,
- ds1307->offset + DS1307_REG_HOUR,
- bin2bcd(tmp));
+ i2c_smbus_write_byte_data(client, DS1307_REG_HOUR, bin2bcd(tmp));
}
ds1307->rtc.ops = &ds13xx_rtc_ops;
@@ -486,7 +469,7 @@ exit:
}
static struct driver_d ds1307_driver = {
- .name = "rtc-ds1307",
+ .name = "rtc-ds1307",
.probe = ds1307_probe,
.id_table = ds1307_id,
};
--
2.7.0.25.gfc10eb5.dirty
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 3/5] rtc: ds1307: Clean up of chip variant support
2016-06-07 20:01 [PATCH 0/5] Fix problems with DS1307 RTC driver Trent Piepho
2016-06-07 20:03 ` [PATCH 1/5] rtc: ds1307: Keep DT based RTC configuration from breaking DS1337/41 Trent Piepho
2016-06-07 20:04 ` [PATCH 2/5] rtc: ds1307: Remove unused and unneeded driver code Trent Piepho
@ 2016-06-07 20:06 ` Trent Piepho
2016-06-12 23:54 ` Andrey Smirnov
2016-06-07 20:09 ` [PATCH 4/5] rtc: ds1307: Allow configuring DS1337 type chips Trent Piepho
2016-06-07 20:09 ` [PATCH 5/5] rtc: ds1307: Limit clock starting retries Trent Piepho
4 siblings, 1 reply; 9+ messages in thread
From: Trent Piepho @ 2016-06-07 20:06 UTC (permalink / raw)
To: barebox; +Cc: Andrey Smirnov
Gets rid of multiple case statements and chip type checks by
attempting to use a more unified approach to chip differences. Flag
bits are added to the state struct for specific differences.
Combines the checks for OSF and CH bits into a single block for all
chips.
Add some flags the indicate chip features. Use these instead of a
bunch of case statements in different places.
Do a single read of chips registers in probe instead of multiple ones
and place register in matching location in the buffer. Fix bug where
DOSF bit was set in incorrect buffer location.
Signed-off-by: Trent Piepho <tpiepho@kymetacorp.com>
---
drivers/rtc/rtc-ds1307.c | 159 +++++++++++++++++++++++++++--------------------
1 file changed, 90 insertions(+), 69 deletions(-)
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 5ed64bf..1526273 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -98,10 +98,26 @@ enum ds_type {
/* Most registers for any device */
#define DS13xx_REG_COUNT 16
-
struct ds1307 {
struct rtc_device rtc;
enum ds_type type;
+ /*
+ * Flags to code chip differences that this code handles.
+ * has_alarms: Chip has alarms, also signifies:
+ * Control register has a different address and format
+ * Seconds register does not have a 'CH' bit
+ * Month register has century bit
+ * osf: Oscillator Stop Flag location
+ * 0 = none
+ * 1..3 = DS13(38|40|37)_BIT_OSF
+ */
+ unsigned has_alarms:1;
+#define OSF_NONE 0
+#define OSF_1338 1
+#define OSF_1340 2
+#define OSF_1337 3
+ unsigned osf:2;
+
struct i2c_client *client;
};
@@ -257,16 +273,10 @@ static int ds1307_set_time(struct rtc_device *rtcdev, struct rtc_time *t)
tmp = t->tm_year - 100;
buf[DS1307_REG_YEAR] = bin2bcd(tmp);
- switch (ds1307->type) {
- case ds_1337:
- case ds_1341:
+ if (ds1307->has_alarms) {
buf[DS1307_REG_MONTH] |= DS1337_BIT_CENTURY;
- break;
- default:
- break;
}
-
dev_dbg(dev, "%s: %7ph\n", "write", buf);
result = ds1307_write_block_data(ds1307->client, DS13xx_REG_TIME_START,
@@ -289,6 +299,8 @@ static int ds1307_probe(struct device_d *dev)
struct ds1307 *ds1307;
int err = -ENODEV;
int tmp;
+ int fault;
+ int nreg;
unsigned char buf[DS13xx_REG_COUNT];
unsigned long driver_data;
const struct device_node *np = dev->device_node;
@@ -303,23 +315,35 @@ static int ds1307_probe(struct device_d *dev)
ds1307->type = driver_data;
switch (ds1307->type) {
+ case ds_1307:
+ nreg = DS1307_REG_CONTROL + 1;
+ break;
+ case ds_1338:
+ nreg = DS1307_REG_CONTROL + 1;
+ ds1307->osf = OSF_1338;
+ break;
case ds_1337:
case ds_1341:
- /* get registers that the "rtc" read below won't read... */
- tmp = ds1307_read_block_data(ds1307->client,
- DS1337_REG_CONTROL, 2, buf);
-
- if (tmp != 2) {
- dev_dbg(&client->dev, "read error %d\n", tmp);
- err = -EIO;
- goto exit;
- }
-
- /* oscillator off? turn it on, so clock can tick. */
- if (buf[0] & DS1337_BIT_nEOSC)
- buf[0] &= ~DS1337_BIT_nEOSC;
+ nreg = DS1337_REG_STATUS + 1;
+ ds1307->osf = OSF_1337;
+ ds1307->has_alarms = 1;
+ break;
+ default:
+ dev_err(&client->dev, "Unknown device type %lu\n", driver_data);
+ err = -ENODEV;
+ goto exit;
+ }
+read_rtc:
+ /* read RTC registers */
+ tmp = ds1307_read_block_data(client, 0, nreg, buf);
+ if (tmp != nreg) {
+ dev_dbg(&client->dev, "read error %d\n", tmp);
+ err = -EIO;
+ goto exit;
+ }
+ if (ds1307->has_alarms) {
/*
Make sure no alarm interrupts or square wave signals
are produced by the chip while we are in
@@ -328,56 +352,36 @@ static int ds1307_probe(struct device_d *dev)
wave generation), but disabling each individual
alarm interrupt source
*/
- buf[0] |= DS1337_BIT_INTCN;
- buf[0] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);
+ buf[DS1337_REG_CONTROL] |= DS1337_BIT_INTCN;
+ buf[DS1337_REG_CONTROL] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);
i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
- buf[0]);
+ buf[DS1337_REG_CONTROL]);
if (ds1307->type == ds_1341) {
/*
* For the above to be true, DS1341 also has to have
* ECLK bit set to 0
*/
- buf[1] &= ~DS1341_BIT_ECLK;
+ buf[DS1337_REG_STATUS] &= ~DS1341_BIT_ECLK;
/*
* Let's set additionale RTC bits to
* facilitate maximum power saving.
*/
- buf[0] |= DS1341_BIT_DOSF;
- buf[0] &= ~DS1341_BIT_EGFIL;
+ buf[DS1337_REG_STATUS] |= DS1341_BIT_DOSF;
+ buf[DS1337_REG_CONTROL] &= ~DS1341_BIT_EGFIL;
i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
- buf[0]);
+ buf[DS1337_REG_CONTROL]);
i2c_smbus_write_byte_data(client, DS1337_REG_STATUS,
- buf[1]);
+ buf[DS1337_REG_STATUS]);
}
-
- /* oscillator fault? clear flag, and warn */
- if (buf[1] & DS1337_BIT_OSF) {
- i2c_smbus_write_byte_data(client, DS1337_REG_STATUS,
- buf[1] & ~DS1337_BIT_OSF);
- dev_warn(&client->dev, "SET TIME!\n");
- }
-
- default:
- break;
- }
-
-read_rtc:
- /* read RTC registers */
- tmp = ds1307_read_block_data(client, 0, 8, buf);
- if (tmp != 8) {
- dev_dbg(&client->dev, "read error %d\n", tmp);
- err = -EIO;
- goto exit;
}
/* Configure clock using OF data if available */
- if (IS_ENABLED(CONFIG_OFDEVICE) && np &&
- ds1307->type != ds_1337 && ds1307->type != ds_1341) {
+ if (IS_ENABLED(CONFIG_OFDEVICE) && np && !ds1307->has_alarms) {
u8 control = buf[DS1307_REG_CONTROL];
u32 rate = 0;
@@ -416,32 +420,49 @@ read_rtc:
}
/* Check for clock halted conditions and start clock */
- tmp = buf[DS1307_REG_SECS];
- switch (ds1307->type) {
- case ds_1307:
- /* clock halted? turn it on, so clock can tick. */
- if (tmp & DS1307_BIT_CH) {
- i2c_smbus_write_byte_data(client, DS1307_REG_SECS, 0);
- dev_warn(&client->dev, "SET TIME!\n");
- goto read_rtc;
+ fault = 0;
+
+ /* clock halted? turn it on, so clock can tick. */
+ if (ds1307->has_alarms) {
+ if (buf[DS1337_REG_CONTROL] & DS1337_BIT_nEOSC) {
+ buf[DS1337_REG_CONTROL] &= ~DS1337_BIT_nEOSC;
+ i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
+ buf[DS1337_REG_CONTROL]);
+ fault = 1;
}
- break;
- case ds_1338:
- /* clock halted? turn it on, so clock can tick. */
- if (tmp & DS1307_BIT_CH)
- i2c_smbus_write_byte_data(client, DS1307_REG_SECS, 0);
+ } else {
+ if (buf[DS1307_REG_SECS] & DS1307_BIT_CH) {
+ buf[DS1307_REG_SECS] = 0;
+ i2c_smbus_write_byte_data(client, DS1307_REG_SECS,
+ buf[DS1307_REG_SECS]);
+ fault = 1;
+ }
+ }
- /* oscillator fault? clear flag, and warn */
+ /* Oscillator fault? clear flag and print warning */
+ switch (ds1307->osf) {
+ case OSF_1338:
if (buf[DS1307_REG_CONTROL] & DS1338_BIT_OSF) {
+ buf[DS1307_REG_CONTROL] &= ~DS1338_BIT_OSF;
i2c_smbus_write_byte_data(client, DS1307_REG_CONTROL,
- buf[DS1307_REG_CONTROL]
- & ~DS1338_BIT_OSF);
- dev_warn(&client->dev, "SET TIME!\n");
- goto read_rtc;
+ buf[DS1307_REG_CONTROL]);
+ fault = 1;
}
break;
- default:
+ case OSF_1337:
+ if (buf[DS1337_REG_STATUS] & DS1337_BIT_OSF) {
+ buf[DS1337_REG_STATUS] &= ~DS1337_BIT_OSF;
+ i2c_smbus_write_byte_data(client, DS1337_REG_STATUS,
+ buf[DS1337_REG_STATUS]);
+ fault = 1;
+ }
break;
+ default: ;
+ }
+
+ if (fault) {
+ dev_warn(&client->dev, "SET TIME!\n");
+ goto read_rtc;
}
if (buf[DS1307_REG_HOUR] & DS1307_BIT_12HR) {
--
2.7.0.25.gfc10eb5.dirty
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 3/5] rtc: ds1307: Clean up of chip variant support
2016-06-07 20:06 ` [PATCH 3/5] rtc: ds1307: Clean up of chip variant support Trent Piepho
@ 2016-06-12 23:54 ` Andrey Smirnov
0 siblings, 0 replies; 9+ messages in thread
From: Andrey Smirnov @ 2016-06-12 23:54 UTC (permalink / raw)
To: Trent Piepho; +Cc: barebox
On Tue, Jun 7, 2016 at 1:06 PM, Trent Piepho <tpiepho@kymetacorp.com> wrote:
> Gets rid of multiple case statements and chip type checks by
> attempting to use a more unified approach to chip differences. Flag
> bits are added to the state struct for specific differences.
>
> Combines the checks for OSF and CH bits into a single block for all
> chips.
>
> Add some flags the indicate chip features. Use these instead of a
> bunch of case statements in different places.
>
> Do a single read of chips registers in probe instead of multiple ones
> and place register in matching location in the buffer. Fix bug where
> DOSF bit was set in incorrect buffer location.
>
> Signed-off-by: Trent Piepho <tpiepho@kymetacorp.com>
> ---
> drivers/rtc/rtc-ds1307.c | 159 +++++++++++++++++++++++++++--------------------
> 1 file changed, 90 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index 5ed64bf..1526273 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -98,10 +98,26 @@ enum ds_type {
> /* Most registers for any device */
> #define DS13xx_REG_COUNT 16
>
> -
> struct ds1307 {
> struct rtc_device rtc;
> enum ds_type type;
> + /*
> + * Flags to code chip differences that this code handles.
> + * has_alarms: Chip has alarms, also signifies:
> + * Control register has a different address and format
> + * Seconds register does not have a 'CH' bit
> + * Month register has century bit
> + * osf: Oscillator Stop Flag location
> + * 0 = none
> + * 1..3 = DS13(38|40|37)_BIT_OSF
> + */
> + unsigned has_alarms:1;
> +#define OSF_NONE 0
> +#define OSF_1338 1
> +#define OSF_1340 2
> +#define OSF_1337 3
> + unsigned osf:2;
> +
> struct i2c_client *client;
> };
>
> @@ -257,16 +273,10 @@ static int ds1307_set_time(struct rtc_device *rtcdev, struct rtc_time *t)
> tmp = t->tm_year - 100;
> buf[DS1307_REG_YEAR] = bin2bcd(tmp);
>
> - switch (ds1307->type) {
> - case ds_1337:
> - case ds_1341:
> + if (ds1307->has_alarms) {
> buf[DS1307_REG_MONTH] |= DS1337_BIT_CENTURY;
> - break;
> - default:
> - break;
> }
>
> -
> dev_dbg(dev, "%s: %7ph\n", "write", buf);
>
> result = ds1307_write_block_data(ds1307->client, DS13xx_REG_TIME_START,
> @@ -289,6 +299,8 @@ static int ds1307_probe(struct device_d *dev)
> struct ds1307 *ds1307;
> int err = -ENODEV;
> int tmp;
> + int fault;
> + int nreg;
> unsigned char buf[DS13xx_REG_COUNT];
> unsigned long driver_data;
> const struct device_node *np = dev->device_node;
> @@ -303,23 +315,35 @@ static int ds1307_probe(struct device_d *dev)
> ds1307->type = driver_data;
>
> switch (ds1307->type) {
> + case ds_1307:
> + nreg = DS1307_REG_CONTROL + 1;
> + break;
> + case ds_1338:
> + nreg = DS1307_REG_CONTROL + 1;
> + ds1307->osf = OSF_1338;
> + break;
> case ds_1337:
> case ds_1341:
> - /* get registers that the "rtc" read below won't read... */
> - tmp = ds1307_read_block_data(ds1307->client,
> - DS1337_REG_CONTROL, 2, buf);
> -
> - if (tmp != 2) {
> - dev_dbg(&client->dev, "read error %d\n", tmp);
> - err = -EIO;
> - goto exit;
> - }
> -
> - /* oscillator off? turn it on, so clock can tick. */
> - if (buf[0] & DS1337_BIT_nEOSC)
> - buf[0] &= ~DS1337_BIT_nEOSC;
> + nreg = DS1337_REG_STATUS + 1;
> + ds1307->osf = OSF_1337;
> + ds1307->has_alarms = 1;
> + break;
> + default:
> + dev_err(&client->dev, "Unknown device type %lu\n", driver_data);
> + err = -ENODEV;
> + goto exit;
> + }
>
> +read_rtc:
> + /* read RTC registers */
> + tmp = ds1307_read_block_data(client, 0, nreg, buf);
> + if (tmp != nreg) {
> + dev_dbg(&client->dev, "read error %d\n", tmp);
> + err = -EIO;
> + goto exit;
> + }
>
> + if (ds1307->has_alarms) {
> /*
> Make sure no alarm interrupts or square wave signals
> are produced by the chip while we are in
> @@ -328,56 +352,36 @@ static int ds1307_probe(struct device_d *dev)
> wave generation), but disabling each individual
> alarm interrupt source
> */
> - buf[0] |= DS1337_BIT_INTCN;
> - buf[0] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);
> + buf[DS1337_REG_CONTROL] |= DS1337_BIT_INTCN;
> + buf[DS1337_REG_CONTROL] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);
>
> i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
> - buf[0]);
> + buf[DS1337_REG_CONTROL]);
>
> if (ds1307->type == ds_1341) {
> /*
> * For the above to be true, DS1341 also has to have
> * ECLK bit set to 0
> */
> - buf[1] &= ~DS1341_BIT_ECLK;
> + buf[DS1337_REG_STATUS] &= ~DS1341_BIT_ECLK;
>
> /*
> * Let's set additionale RTC bits to
> * facilitate maximum power saving.
> */
> - buf[0] |= DS1341_BIT_DOSF;
> - buf[0] &= ~DS1341_BIT_EGFIL;
> + buf[DS1337_REG_STATUS] |= DS1341_BIT_DOSF;
> + buf[DS1337_REG_CONTROL] &= ~DS1341_BIT_EGFIL;
>
> i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
> - buf[0]);
> + buf[DS1337_REG_CONTROL]);
> i2c_smbus_write_byte_data(client, DS1337_REG_STATUS,
> - buf[1]);
> + buf[DS1337_REG_STATUS]);
> }
>
> -
> - /* oscillator fault? clear flag, and warn */
> - if (buf[1] & DS1337_BIT_OSF) {
> - i2c_smbus_write_byte_data(client, DS1337_REG_STATUS,
> - buf[1] & ~DS1337_BIT_OSF);
> - dev_warn(&client->dev, "SET TIME!\n");
> - }
> -
> - default:
> - break;
> - }
> -
> -read_rtc:
> - /* read RTC registers */
> - tmp = ds1307_read_block_data(client, 0, 8, buf);
> - if (tmp != 8) {
> - dev_dbg(&client->dev, "read error %d\n", tmp);
> - err = -EIO;
> - goto exit;
> }
>
> /* Configure clock using OF data if available */
> - if (IS_ENABLED(CONFIG_OFDEVICE) && np &&
> - ds1307->type != ds_1337 && ds1307->type != ds_1341) {
> + if (IS_ENABLED(CONFIG_OFDEVICE) && np && !ds1307->has_alarms) {
> u8 control = buf[DS1307_REG_CONTROL];
> u32 rate = 0;
>
> @@ -416,32 +420,49 @@ read_rtc:
> }
>
> /* Check for clock halted conditions and start clock */
> - tmp = buf[DS1307_REG_SECS];
> - switch (ds1307->type) {
> - case ds_1307:
> - /* clock halted? turn it on, so clock can tick. */
> - if (tmp & DS1307_BIT_CH) {
> - i2c_smbus_write_byte_data(client, DS1307_REG_SECS, 0);
> - dev_warn(&client->dev, "SET TIME!\n");
> - goto read_rtc;
> + fault = 0;
> +
> + /* clock halted? turn it on, so clock can tick. */
> + if (ds1307->has_alarms) {
> + if (buf[DS1337_REG_CONTROL] & DS1337_BIT_nEOSC) {
> + buf[DS1337_REG_CONTROL] &= ~DS1337_BIT_nEOSC;
> + i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
> + buf[DS1337_REG_CONTROL]);
> + fault = 1;
> }
> - break;
> - case ds_1338:
> - /* clock halted? turn it on, so clock can tick. */
> - if (tmp & DS1307_BIT_CH)
> - i2c_smbus_write_byte_data(client, DS1307_REG_SECS, 0);
> + } else {
> + if (buf[DS1307_REG_SECS] & DS1307_BIT_CH) {
> + buf[DS1307_REG_SECS] = 0;
> + i2c_smbus_write_byte_data(client, DS1307_REG_SECS,
> + buf[DS1307_REG_SECS]);
> + fault = 1;
> + }
> + }
>
> - /* oscillator fault? clear flag, and warn */
> + /* Oscillator fault? clear flag and print warning */
> + switch (ds1307->osf) {
> + case OSF_1338:
> if (buf[DS1307_REG_CONTROL] & DS1338_BIT_OSF) {
> + buf[DS1307_REG_CONTROL] &= ~DS1338_BIT_OSF;
> i2c_smbus_write_byte_data(client, DS1307_REG_CONTROL,
> - buf[DS1307_REG_CONTROL]
> - & ~DS1338_BIT_OSF);
> - dev_warn(&client->dev, "SET TIME!\n");
> - goto read_rtc;
> + buf[DS1307_REG_CONTROL]);
> + fault = 1;
> }
> break;
> - default:
> + case OSF_1337:
> + if (buf[DS1337_REG_STATUS] & DS1337_BIT_OSF) {
> + buf[DS1337_REG_STATUS] &= ~DS1337_BIT_OSF;
> + i2c_smbus_write_byte_data(client, DS1337_REG_STATUS,
> + buf[DS1337_REG_STATUS]);
> + fault = 1;
> + }
> break;
> + default: ;
> + }
> +
> + if (fault) {
> + dev_warn(&client->dev, "SET TIME!\n");
> + goto read_rtc;
> }
Now that you only have only one "goto" place, wouldn't it make things
more readable if you convert this to a loop? This should also make the
diff of 5/5 slightly smaller.
>
> if (buf[DS1307_REG_HOUR] & DS1307_BIT_12HR) {
> --
> 2.7.0.25.gfc10eb5.dirty
>
>
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 4/5] rtc: ds1307: Allow configuring DS1337 type chips
2016-06-07 20:01 [PATCH 0/5] Fix problems with DS1307 RTC driver Trent Piepho
` (2 preceding siblings ...)
2016-06-07 20:06 ` [PATCH 3/5] rtc: ds1307: Clean up of chip variant support Trent Piepho
@ 2016-06-07 20:09 ` Trent Piepho
2016-06-12 22:29 ` Andrey Smirnov
2016-06-07 20:09 ` [PATCH 5/5] rtc: ds1307: Limit clock starting retries Trent Piepho
4 siblings, 1 reply; 9+ messages in thread
From: Trent Piepho @ 2016-06-07 20:09 UTC (permalink / raw)
To: barebox; +Cc: Andrey Smirnov
Move OF configuration code into new function which can be used by both
style of chips.
Extend the DT bindings to allow configuring both input and output
rate, as the DS1337/41 type chips have this ability.
Signed-off-by: Trent Piepho <tpiepho@kymetacorp.com>
---
.../devicetree/bindings/rtc/dallas,ds1307.rst | 18 +-
drivers/rtc/rtc-ds1307.c | 188 ++++++++++++---------
2 files changed, 124 insertions(+), 82 deletions(-)
diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1307.rst b/Documentation/devicetree/bindings/rtc/dallas,ds1307.rst
index 52787a8..b9e91f1 100644
--- a/Documentation/devicetree/bindings/rtc/dallas,ds1307.rst
+++ b/Documentation/devicetree/bindings/rtc/dallas,ds1307.rst
@@ -2,22 +2,28 @@ Dallas DS1307 I2C Serial Real-Time Clock
========================================
Required properties:
-* ``compatible``: ``dallas,ds1307``, ``dallas,ds1308``, ``dallas,ds1338``
+* ``compatible``: ``dallas,ds1307``, ``dallas,ds1308``, ``dallas,ds1337``,
+ ``dallas,ds1338``, ``dallas,ds1341``
"maxim" can be used in place of "dallas"
* ``reg``: I2C address for chip
Optional properties:
* ``ext-clock-input``: Enable external clock input pin
-* ``ext-clock-output``: Enable square wave output. The above two
- properties are mutually exclusive
+* ``ext-clock-output``: Enable square wave output. On some chips both
+ the above properties can not be enabled at once.
* ``ext-clock-bb``: Enable external clock on battery power
-* ``ext-clock-rate``: Expected/Generated rate on external clock pin
- in Hz. Allowable values are 1, 50, 60, 4096, 8192, and 32768 Hz.
+* ``ext-clock-input-rate``: Expected rate on external clock input pin
+ in Hz. Allowable values are 1, 50, 60, 4096, 8192, and 32768 Hz.
Not all values are valid for all configurations.
+* ``ext-clock-output-rate``: Rate to generate on square wave output pin
+ in Hz. Allowable values are 1, 50, 60, 4096, 8192, and 32768 Hz.
+ Not all values are valid for all configurations. Some chips do not allow
+ setting both input and output rate. In this case, only the correct property
+ should be set.
The default is ext-clock-input, ext-clock-output, and ext-clock-bb
-disabled and ext-clock-rate of 1 Hz.
+disabled and ext-clock-input/output-rate of 1 Hz.
Example::
ds1307: rtc@68 {
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 1526273..b772ca3 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -92,6 +92,8 @@ enum ds_type {
#define DS1337_REG_STATUS 0x0f
# define DS1337_BIT_OSF 0x80
# define DS1341_BIT_DOSF 0x40
+# define DS1341_BIT_CLKSEL2 0x10
+# define DS1341_BIT_CLKSEL1 0x08
# define DS1341_BIT_ECLK 0x04
# define DS1337_BIT_A2I 0x02
# define DS1337_BIT_A1I 0x01
@@ -293,6 +295,70 @@ static const struct rtc_class_ops ds13xx_rtc_ops = {
.set_time = ds1307_set_time,
};
+
+/* Convert rate in Hz to config bits */
+static u8 parse_rate(u32 rate)
+{
+ switch (rate) {
+ default:
+ case 1: return 0;
+ case 50: return DS1307_BIT_RS0;
+ case 60: return DS1307_BIT_RS1;
+ case 4096: return DS1307_BIT_RS0;
+ case 8192: return DS1307_BIT_RS1;
+ case 32768: return DS1307_BIT_RS0|DS1307_BIT_RS1;
+ }
+}
+
+/* Produces a config word with DT settings. This word must be
+ * translated into chip specific bits. The low 8-bit are basically
+ * identical to the ds1307/1308/1338 control reg to make this
+ * translation simpler.
+ *
+ * Word uses these bits:
+ * DS1308_BIT_ECLK enable external clock input
+ * DS1307_BIT_SQWE enable square wave output
+ * DS1308_BIT_BBCLK enable square wave on battery
+ * DS1307_BIT_RS1/0 square wave output frequency
+ * DS1307_BIT_RS1/0 << 8 external clock input frequency
+ *
+ * The rate select bits will be zero if not specified in the device
+ * tree.
+ */
+static u16 ds1307_config(const struct device_node *np)
+{
+ u16 control = 0;
+ u32 rate;
+
+ if (of_property_read_bool(np, "ext-clock-input"))
+ control |= DS1308_BIT_ECLK;
+
+ if (of_property_read_bool(np, "ext-clock-output"))
+ control |= DS1307_BIT_SQWE;
+
+ if (of_property_read_bool(np, "ext-clock-bb"))
+ control |= DS1308_BIT_BBCLK;
+
+ rate = 0;
+ of_property_read_u32(np, "ext-clock-input-rate", &rate);
+ control |= parse_rate(rate);
+
+ rate = 0;
+ of_property_read_u32(np, "ext-clock-output-rate", &rate);
+ control |= parse_rate(rate) << 8;
+
+ return control;
+}
+
+static void write_if_changed(struct ds1307 *ds1307, u8 *buf,
+ int reg, u8 value)
+{
+ if (buf[reg] != value) {
+ buf[reg] = value;
+ i2c_smbus_write_byte_data(ds1307->client, reg, buf[reg]);
+ }
+}
+
static int ds1307_probe(struct device_d *dev)
{
struct i2c_client *client = to_i2c_client(dev);
@@ -343,82 +409,6 @@ read_rtc:
goto exit;
}
- if (ds1307->has_alarms) {
- /*
- Make sure no alarm interrupts or square wave signals
- are produced by the chip while we are in
- bootloader. We do this by configuring the RTC to
- generate alarm interrupts (thus disabling square
- wave generation), but disabling each individual
- alarm interrupt source
- */
- buf[DS1337_REG_CONTROL] |= DS1337_BIT_INTCN;
- buf[DS1337_REG_CONTROL] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);
-
- i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
- buf[DS1337_REG_CONTROL]);
-
- if (ds1307->type == ds_1341) {
- /*
- * For the above to be true, DS1341 also has to have
- * ECLK bit set to 0
- */
- buf[DS1337_REG_STATUS] &= ~DS1341_BIT_ECLK;
-
- /*
- * Let's set additionale RTC bits to
- * facilitate maximum power saving.
- */
- buf[DS1337_REG_STATUS] |= DS1341_BIT_DOSF;
- buf[DS1337_REG_CONTROL] &= ~DS1341_BIT_EGFIL;
-
- i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
- buf[DS1337_REG_CONTROL]);
- i2c_smbus_write_byte_data(client, DS1337_REG_STATUS,
- buf[DS1337_REG_STATUS]);
- }
-
- }
-
- /* Configure clock using OF data if available */
- if (IS_ENABLED(CONFIG_OFDEVICE) && np && !ds1307->has_alarms) {
- u8 control = buf[DS1307_REG_CONTROL];
- u32 rate = 0;
-
- if (of_property_read_bool(np, "ext-clock-input"))
- control |= DS1308_BIT_ECLK;
- else
- control &= ~DS1308_BIT_ECLK;
-
- if (of_property_read_bool(np, "ext-clock-output"))
- control |= DS1307_BIT_SQWE;
- else
- control &= ~DS1307_BIT_SQWE;
-
- if (of_property_read_bool(np, "ext-clock-bb"))
- control |= DS1308_BIT_BBCLK;
- else
- control &= ~DS1308_BIT_BBCLK;
-
- control &= ~(DS1307_BIT_RS1 | DS1307_BIT_RS0);
- of_property_read_u32(np, "ext-clock-rate", &rate);
- switch (rate) {
- default:
- case 1: control |= 0; break;
- case 50: control |= 1; break;
- case 60: control |= 2; break;
- case 4096: control |= 1; break;
- case 8192: control |= 2; break;
- case 32768: control |= 3; break;
- }
- dev_dbg(&client->dev, "control reg: 0x%02x\n", control);
-
- if (buf[DS1307_REG_CONTROL] != control) {
- i2c_smbus_write_byte_data(client, DS1307_REG_CONTROL, control);
- buf[DS1307_REG_CONTROL] = control;
- }
- }
-
/* Check for clock halted conditions and start clock */
fault = 0;
@@ -465,6 +455,52 @@ read_rtc:
goto read_rtc;
}
+ /* Configure clock using OF data if available */
+ if (IS_ENABLED(CONFIG_OFDEVICE) && np) {
+ u16 config = ds1307_config(np);
+ if (ds1307->has_alarms) {
+ u8 creg = buf[DS1337_REG_CONTROL];
+ u8 sreg = buf[DS1337_REG_STATUS];
+
+ /* Translate bits to the DS1337_REG_CONTROL format */
+ creg &= ~(DS1337_BIT_RS2 | DS1337_BIT_RS1);
+ creg |= (config & 0x003) << 3;
+ sreg &= ~(DS1341_BIT_CLKSEL2 | DS1341_BIT_CLKSEL1);
+ sreg |= (config & 0x300) >> 5;
+ creg &= ~(DS1337_BIT_INTCN | DS1341_BIT_ECLK);
+ creg |= config & DS1307_BIT_SQWE ? 0 : DS1337_BIT_INTCN;
+ creg |= config & DS1308_BIT_ECLK ? DS1341_BIT_ECLK : 0;
+
+ /* Make sure no alarm interrupts are triggered by
+ * disabling alarms. Square wave signals are already
+ * disabled unless the DT turned them on above. */
+ creg &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);
+
+ /*
+ * Let's set additional RTC bits to
+ * facilitate maximum power saving.
+ */
+ if (ds1307->type == ds_1341) {
+ buf[DS1337_REG_STATUS] |= DS1341_BIT_DOSF;
+ buf[DS1337_REG_CONTROL] &= ~DS1341_BIT_EGFIL;
+ }
+
+ write_if_changed(ds1307, buf, DS1337_REG_CONTROL, creg);
+ write_if_changed(ds1307, buf, DS1337_REG_STATUS, sreg);
+ } else {
+ u8 reg = buf[DS1307_REG_CONTROL];
+
+ /* Combine input and output rate bits */
+ config |= (config >> 8);
+ reg &= ~(DS1308_BIT_ECLK | DS1307_BIT_SQWE |
+ DS1308_BIT_BBCLK |
+ DS1307_BIT_RS1 | DS1307_BIT_RS0);
+ reg |= config;
+
+ write_if_changed(ds1307, buf, DS1307_REG_CONTROL, reg);
+ }
+ }
+
if (buf[DS1307_REG_HOUR] & DS1307_BIT_12HR) {
/*
* Be sure we're in 24 hour mode. Multi-master systems
--
2.7.0.25.gfc10eb5.dirty
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 4/5] rtc: ds1307: Allow configuring DS1337 type chips
2016-06-07 20:09 ` [PATCH 4/5] rtc: ds1307: Allow configuring DS1337 type chips Trent Piepho
@ 2016-06-12 22:29 ` Andrey Smirnov
0 siblings, 0 replies; 9+ messages in thread
From: Andrey Smirnov @ 2016-06-12 22:29 UTC (permalink / raw)
To: Trent Piepho; +Cc: barebox
On Tue, Jun 7, 2016 at 1:09 PM, Trent Piepho <tpiepho@kymetacorp.com> wrote:
> Move OF configuration code into new function which can be used by both
> style of chips.
>
> Extend the DT bindings to allow configuring both input and output
> rate, as the DS1337/41 type chips have this ability.
>
> Signed-off-by: Trent Piepho <tpiepho@kymetacorp.com>
> ---
> .../devicetree/bindings/rtc/dallas,ds1307.rst | 18 +-
> drivers/rtc/rtc-ds1307.c | 188 ++++++++++++---------
> 2 files changed, 124 insertions(+), 82 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/rtc/dallas,ds1307.rst b/Documentation/devicetree/bindings/rtc/dallas,ds1307.rst
> index 52787a8..b9e91f1 100644
> --- a/Documentation/devicetree/bindings/rtc/dallas,ds1307.rst
> +++ b/Documentation/devicetree/bindings/rtc/dallas,ds1307.rst
> @@ -2,22 +2,28 @@ Dallas DS1307 I2C Serial Real-Time Clock
> ========================================
>
> Required properties:
> -* ``compatible``: ``dallas,ds1307``, ``dallas,ds1308``, ``dallas,ds1338``
> +* ``compatible``: ``dallas,ds1307``, ``dallas,ds1308``, ``dallas,ds1337``,
> + ``dallas,ds1338``, ``dallas,ds1341``
> "maxim" can be used in place of "dallas"
>
> * ``reg``: I2C address for chip
>
> Optional properties:
> * ``ext-clock-input``: Enable external clock input pin
> -* ``ext-clock-output``: Enable square wave output. The above two
> - properties are mutually exclusive
> +* ``ext-clock-output``: Enable square wave output. On some chips both
> + the above properties can not be enabled at once.
> * ``ext-clock-bb``: Enable external clock on battery power
> -* ``ext-clock-rate``: Expected/Generated rate on external clock pin
> - in Hz. Allowable values are 1, 50, 60, 4096, 8192, and 32768 Hz.
There seem to be a trailing white-space at the end of the line above,
which prevented me from applying the first hunk of the patch since
there was no trailing white-space in the latest "next" source tree.
> +* ``ext-clock-input-rate``: Expected rate on external clock input pin
> + in Hz. Allowable values are 1, 50, 60, 4096, 8192, and 32768 Hz.
> Not all values are valid for all configurations.
> +* ``ext-clock-output-rate``: Rate to generate on square wave output pin
> + in Hz. Allowable values are 1, 50, 60, 4096, 8192, and 32768 Hz.
> + Not all values are valid for all configurations. Some chips do not allow
> + setting both input and output rate. In this case, only the correct property
> + should be set.
>
> The default is ext-clock-input, ext-clock-output, and ext-clock-bb
> -disabled and ext-clock-rate of 1 Hz.
> +disabled and ext-clock-input/output-rate of 1 Hz.
>
> Example::
> ds1307: rtc@68 {
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index 1526273..b772ca3 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -92,6 +92,8 @@ enum ds_type {
> #define DS1337_REG_STATUS 0x0f
> # define DS1337_BIT_OSF 0x80
> # define DS1341_BIT_DOSF 0x40
> +# define DS1341_BIT_CLKSEL2 0x10
> +# define DS1341_BIT_CLKSEL1 0x08
> # define DS1341_BIT_ECLK 0x04
> # define DS1337_BIT_A2I 0x02
> # define DS1337_BIT_A1I 0x01
> @@ -293,6 +295,70 @@ static const struct rtc_class_ops ds13xx_rtc_ops = {
> .set_time = ds1307_set_time,
> };
>
> +
> +/* Convert rate in Hz to config bits */
> +static u8 parse_rate(u32 rate)
> +{
> + switch (rate) {
> + default:
> + case 1: return 0;
> + case 50: return DS1307_BIT_RS0;
> + case 60: return DS1307_BIT_RS1;
> + case 4096: return DS1307_BIT_RS0;
> + case 8192: return DS1307_BIT_RS1;
> + case 32768: return DS1307_BIT_RS0|DS1307_BIT_RS1;
> + }
> +}
> +
> +/* Produces a config word with DT settings. This word must be
> + * translated into chip specific bits. The low 8-bit are basically
> + * identical to the ds1307/1308/1338 control reg to make this
> + * translation simpler.
> + *
> + * Word uses these bits:
> + * DS1308_BIT_ECLK enable external clock input
> + * DS1307_BIT_SQWE enable square wave output
> + * DS1308_BIT_BBCLK enable square wave on battery
> + * DS1307_BIT_RS1/0 square wave output frequency
> + * DS1307_BIT_RS1/0 << 8 external clock input frequency
> + *
> + * The rate select bits will be zero if not specified in the device
> + * tree.
> + */
> +static u16 ds1307_config(const struct device_node *np)
> +{
> + u16 control = 0;
> + u32 rate;
> +
> + if (of_property_read_bool(np, "ext-clock-input"))
> + control |= DS1308_BIT_ECLK;
> +
> + if (of_property_read_bool(np, "ext-clock-output"))
> + control |= DS1307_BIT_SQWE;
> +
> + if (of_property_read_bool(np, "ext-clock-bb"))
> + control |= DS1308_BIT_BBCLK;
> +
> + rate = 0;
> + of_property_read_u32(np, "ext-clock-input-rate", &rate);
> + control |= parse_rate(rate);
> +
> + rate = 0;
> + of_property_read_u32(np, "ext-clock-output-rate", &rate);
> + control |= parse_rate(rate) << 8;
> +
> + return control;
> +}
> +
> +static void write_if_changed(struct ds1307 *ds1307, u8 *buf,
> + int reg, u8 value)
> +{
> + if (buf[reg] != value) {
> + buf[reg] = value;
> + i2c_smbus_write_byte_data(ds1307->client, reg, buf[reg]);
> + }
> +}
> +
> static int ds1307_probe(struct device_d *dev)
> {
> struct i2c_client *client = to_i2c_client(dev);
> @@ -343,82 +409,6 @@ read_rtc:
> goto exit;
> }
>
> - if (ds1307->has_alarms) {
> - /*
> - Make sure no alarm interrupts or square wave signals
> - are produced by the chip while we are in
> - bootloader. We do this by configuring the RTC to
> - generate alarm interrupts (thus disabling square
> - wave generation), but disabling each individual
> - alarm interrupt source
> - */
> - buf[DS1337_REG_CONTROL] |= DS1337_BIT_INTCN;
> - buf[DS1337_REG_CONTROL] &= ~(DS1337_BIT_A2IE | DS1337_BIT_A1IE);
> -
> - i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
> - buf[DS1337_REG_CONTROL]);
> -
> - if (ds1307->type == ds_1341) {
> - /*
> - * For the above to be true, DS1341 also has to have
> - * ECLK bit set to 0
> - */
> - buf[DS1337_REG_STATUS] &= ~DS1341_BIT_ECLK;
> -
> - /*
> - * Let's set additionale RTC bits to
> - * facilitate maximum power saving.
> - */
> - buf[DS1337_REG_STATUS] |= DS1341_BIT_DOSF;
> - buf[DS1337_REG_CONTROL] &= ~DS1341_BIT_EGFIL;
> -
> - i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
> - buf[DS1337_REG_CONTROL]);
> - i2c_smbus_write_byte_data(client, DS1337_REG_STATUS,
> - buf[DS1337_REG_STATUS]);
> - }
> -
> - }
> -
> - /* Configure clock using OF data if available */
> - if (IS_ENABLED(CONFIG_OFDEVICE) && np && !ds1307->has_alarms) {
> - u8 control = buf[DS1307_REG_CONTROL];
> - u32 rate = 0;
> -
> - if (of_property_read_bool(np, "ext-clock-input"))
> - control |= DS1308_BIT_ECLK;
> - else
> - control &= ~DS1308_BIT_ECLK;
> -
> - if (of_property_read_bool(np, "ext-clock-output"))
> - control |= DS1307_BIT_SQWE;
> - else
> - control &= ~DS1307_BIT_SQWE;
> -
> - if (of_property_read_bool(np, "ext-clock-bb"))
> - control |= DS1308_BIT_BBCLK;
> - else
> - control &= ~DS1308_BIT_BBCLK;
> -
> - control &= ~(DS1307_BIT_RS1 | DS1307_BIT_RS0);
> - of_property_read_u32(np, "ext-clock-rate", &rate);
> - switch (rate) {
> - default:
> - case 1: control |= 0; break;
> - case 50: control |= 1; break;
> - case 60: control |= 2; break;
> - case 4096: control |= 1; break;
> - case 8192: control |= 2; break;
> - case 32768: control |= 3; break;
> - }
> - dev_dbg(&client->dev, "control reg: 0x%02x\n", control);
> -
> - if (buf[DS1307_REG_CONTROL] != control) {
> - i2c_smbus_write_byte_data(client, DS1307_REG_CONTROL, control);
> - buf[DS1307_REG_CONTROL] = control;
> - }
> - }
> -
> /* Check for clock halted conditions and start clock */
> fault = 0;
>
> @@ -465,6 +455,52 @@ read_rtc:
> goto read_rtc;
> }
>
> + /* Configure clock using OF data if available */
> + if (IS_ENABLED(CONFIG_OFDEVICE) && np) {
> + u16 config = ds1307_config(np);
> + if (ds1307->has_alarms) {
> + u8 creg = buf[DS1337_REG_CONTROL];
> + u8 sreg = buf[DS1337_REG_STATUS];
> +
> + /* Translate bits to the DS1337_REG_CONTROL format */
> + creg &= ~(DS1337_BIT_RS2 | DS1337_BIT_RS1);
> + creg |= (config & 0x003) << 3;
If you swap masking and shifting you should be able to use named
constants instead of magic numbers.
> + sreg &= ~(DS1341_BIT_CLKSEL2 | DS1341_BIT_CLKSEL1);
> + sreg |= (config & 0x300) >> 5;
Ditto.
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 5/5] rtc: ds1307: Limit clock starting retries
2016-06-07 20:01 [PATCH 0/5] Fix problems with DS1307 RTC driver Trent Piepho
` (3 preceding siblings ...)
2016-06-07 20:09 ` [PATCH 4/5] rtc: ds1307: Allow configuring DS1337 type chips Trent Piepho
@ 2016-06-07 20:09 ` Trent Piepho
2016-06-12 23:51 ` Andrey Smirnov
4 siblings, 1 reply; 9+ messages in thread
From: Trent Piepho @ 2016-06-07 20:09 UTC (permalink / raw)
To: barebox; +Cc: Andrey Smirnov
If the driver detects the clock is stopped, via clock halted control
bit or oscillator stopped status flag, it will try to start the clock
and then reread the control registers and retry the probe process.
It will continue to retry until the clock starts, possibly forever if
the clock crystal is not installed. While the driver's dogged
determination to start the clock is admirable, at some point you have
to say enough is enough, this clock won't go, and get one with more
important things, like booting.
This limits the number of tries to start the clock to three.
Signed-off-by: Trent Piepho <tpiepho@kymetacorp.com>
---
drivers/rtc/rtc-ds1307.c | 101 ++++++++++++++++++++++++-----------------------
1 file changed, 51 insertions(+), 50 deletions(-)
diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index b772ca3..34f24ed 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -365,7 +365,7 @@ static int ds1307_probe(struct device_d *dev)
struct ds1307 *ds1307;
int err = -ENODEV;
int tmp;
- int fault;
+ int fault, retries;
int nreg;
unsigned char buf[DS13xx_REG_COUNT];
unsigned long driver_data;
@@ -400,60 +400,61 @@ static int ds1307_probe(struct device_d *dev)
goto exit;
}
-read_rtc:
- /* read RTC registers */
- tmp = ds1307_read_block_data(client, 0, nreg, buf);
- if (tmp != nreg) {
- dev_dbg(&client->dev, "read error %d\n", tmp);
- err = -EIO;
- goto exit;
- }
-
- /* Check for clock halted conditions and start clock */
- fault = 0;
-
- /* clock halted? turn it on, so clock can tick. */
- if (ds1307->has_alarms) {
- if (buf[DS1337_REG_CONTROL] & DS1337_BIT_nEOSC) {
- buf[DS1337_REG_CONTROL] &= ~DS1337_BIT_nEOSC;
- i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
- buf[DS1337_REG_CONTROL]);
- fault = 1;
- }
- } else {
- if (buf[DS1307_REG_SECS] & DS1307_BIT_CH) {
- buf[DS1307_REG_SECS] = 0;
- i2c_smbus_write_byte_data(client, DS1307_REG_SECS,
- buf[DS1307_REG_SECS]);
- fault = 1;
+ retries = 3;
+ do {
+ /* read RTC registers */
+ tmp = ds1307_read_block_data(client, 0, nreg, buf);
+ if (tmp != nreg) {
+ dev_dbg(&client->dev, "read error %d\n", tmp);
+ err = -EIO;
+ goto exit;
}
- }
- /* Oscillator fault? clear flag and print warning */
- switch (ds1307->osf) {
- case OSF_1338:
- if (buf[DS1307_REG_CONTROL] & DS1338_BIT_OSF) {
- buf[DS1307_REG_CONTROL] &= ~DS1338_BIT_OSF;
- i2c_smbus_write_byte_data(client, DS1307_REG_CONTROL,
- buf[DS1307_REG_CONTROL]);
- fault = 1;
+ fault = 0;
+ /* clock halted? turn it on, so clock can tick. */
+ if (ds1307->has_alarms) {
+ if (buf[DS1337_REG_CONTROL] & DS1337_BIT_nEOSC) {
+ buf[DS1337_REG_CONTROL] &= ~DS1337_BIT_nEOSC;
+ i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
+ buf[DS1337_REG_CONTROL]);
+ fault = 1;
+ }
+ } else {
+ if (buf[DS1307_REG_SECS] & DS1307_BIT_CH) {
+ buf[DS1307_REG_SECS] = 0;
+ i2c_smbus_write_byte_data(client, DS1307_REG_SECS,
+ buf[DS1307_REG_SECS]);
+ fault = 1;
+ }
}
- break;
- case OSF_1337:
- if (buf[DS1337_REG_STATUS] & DS1337_BIT_OSF) {
- buf[DS1337_REG_STATUS] &= ~DS1337_BIT_OSF;
- i2c_smbus_write_byte_data(client, DS1337_REG_STATUS,
- buf[DS1337_REG_STATUS]);
- fault = 1;
+
+ /* Oscillator fault? clear flag and print warning */
+ switch (ds1307->osf) {
+ case OSF_1338:
+ if (buf[DS1307_REG_CONTROL] & DS1338_BIT_OSF) {
+ buf[DS1307_REG_CONTROL] &= ~DS1338_BIT_OSF;
+ i2c_smbus_write_byte_data(client, DS1307_REG_CONTROL,
+ buf[DS1307_REG_CONTROL]);
+ fault = 1;
+ }
+ break;
+ case OSF_1337:
+ if (buf[DS1337_REG_STATUS] & DS1337_BIT_OSF) {
+ buf[DS1337_REG_STATUS] &= ~DS1337_BIT_OSF;
+ i2c_smbus_write_byte_data(client, DS1337_REG_STATUS,
+ buf[DS1337_REG_STATUS]);
+ fault = 1;
+ }
+ break;
+ default: ;
}
- break;
- default: ;
- }
- if (fault) {
- dev_warn(&client->dev, "SET TIME!\n");
- goto read_rtc;
- }
+ if (fault)
+ dev_warn(&client->dev, "SET TIME!\n");
+ } while (fault && retries--);
+
+ if (fault)
+ dev_err(&client->dev, "Failed to start clock, placing in correct once per day mode!\n");
/* Configure clock using OF data if available */
if (IS_ENABLED(CONFIG_OFDEVICE) && np) {
--
2.7.0.25.gfc10eb5.dirty
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 5/5] rtc: ds1307: Limit clock starting retries
2016-06-07 20:09 ` [PATCH 5/5] rtc: ds1307: Limit clock starting retries Trent Piepho
@ 2016-06-12 23:51 ` Andrey Smirnov
0 siblings, 0 replies; 9+ messages in thread
From: Andrey Smirnov @ 2016-06-12 23:51 UTC (permalink / raw)
To: Trent Piepho; +Cc: barebox
On Tue, Jun 7, 2016 at 1:09 PM, Trent Piepho <tpiepho@kymetacorp.com> wrote:
> If the driver detects the clock is stopped, via clock halted control
> bit or oscillator stopped status flag, it will try to start the clock
> and then reread the control registers and retry the probe process.
>
> It will continue to retry until the clock starts, possibly forever if
> the clock crystal is not installed. While the driver's dogged
> determination to start the clock is admirable, at some point you have
> to say enough is enough, this clock won't go, and get one with more
Typo? -----------------------------------------------------------------^
> important things, like booting.
>
> This limits the number of tries to start the clock to three.
This is definitely a good change. Might I suggest a slightly different
implementation? Something to the effect of:
------------------------8<----------------------------------------------------
for (retries = 0; retries < 3; retries++) {
clock_halted = oscillator_failed = false;
/* read RTC registers */
tmp = ds1307_read_block_data(client, 0, nreg, buf);
if (tmp != nreg) {
dev_dbg(&client->dev, "read error %d\n", tmp);
err = -EIO;
goto exit;
}
/* clock halted? turn it on, so clock can tick. */
if (ds1307->has_alarms) {
if (buf[DS1337_REG_CONTROL] & DS1337_BIT_nEOSC) {
buf[DS1337_REG_CONTROL] &= ~DS1337_BIT_nEOSC;
i2c_smbus_write_byte_data(client,
DS1337_REG_CONTROL,
buf[DS1337_REG_CONTROL]);
clock_halted = true;
}
} else {
if (buf[DS1307_REG_SECS] & DS1307_BIT_CH) {
buf[DS1307_REG_SECS] = 0;
i2c_smbus_write_byte_data(client,
DS1307_REG_SECS,
buf[DS1307_REG_SECS]);
clock_halted = true;
}
}
if (clock_halted) {
dev_warn(&client->dev,
"chip's oscillator is disabled. "
"Re-enabling it.\n");
continue;
}
/* Oscillator fault? clear flag and print warning */
switch (ds1307->osf) {
case OSF_1338:
if (buf[DS1307_REG_CONTROL] & DS1338_BIT_OSF) {
buf[DS1307_REG_CONTROL] &= ~DS1338_BIT_OSF;
i2c_smbus_write_byte_data(client,
DS1307_REG_CONTROL,
buf[DS1307_REG_CONTROL]);
oscillator_failed = true;
}
break;
case OSF_1337:
if (buf[DS1337_REG_STATUS] & DS1337_BIT_OSF) {
buf[DS1337_REG_STATUS] &= ~DS1337_BIT_OSF;
i2c_smbus_write_byte_data(client,
DS1337_REG_STATUS,
buf[DS1337_REG_STATUS]);
oscillator_failed = true;
}
break;
default: ;
}
if (oscillator_failed) {
dev_warn(&client->dev,
"chip's oscillator failed. "
"Clearing and re-reading status.\n");
continue;
}
break;
};
if (oscillator_failed || clock_halted) {
dev_warn(&client->dev,
"RTC's time information is incorrect. "
"Please set time\n")
}
if (!retries)
dev_err(&client->dev,
"Failed to start clock, placing in correct once per
day mode!\n");
------------------------>8----------------------------------------------------
Note, however that this would restore original error handling behavior
-- which you changed in 3/5 -- where all registers are re-read after
each type of failure is detected.
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply [flat|nested] 9+ messages in thread