| Message ID | 20190816024636.34738-1-biwen.li@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] rtc: pcf85363/pcf85263: fix error that failed to run hwclock -w | expand |
On 16/08/2019 10:46:36+0800, Biwen Li wrote: > Issue: > - # hwclock -w > hwclock: RTC_SET_TIME: Invalid argument > > Why: > - Relative patch: https://lkml.org/lkml/2019/4/3/55 , this patch > will always check for unwritable registers, it will compare reg > with max_register in regmap_writeable. > > - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but DT_100THS > is 0, max_regiter is 0x2f, then reg will be equal to 0x30, > '0x30 < 0x2f' is false,so regmap_writeable will return false. > > - Root cause: the buf[] was written to a wrong place in the file > drivers/rtc/rtc-pcf85363.c > This is not true, the RTC wraps the register accesses properly and this is probably something that should be handled by regmap_writable.
On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > > On 16/08/2019 10:46:36+0800, Biwen Li wrote: > > Issue: > > - # hwclock -w > > hwclock: RTC_SET_TIME: Invalid argument > > > > Why: > > - Relative patch: https://lkml.org/lkml/2019/4/3/55 , this patch > > will always check for unwritable registers, it will compare reg > > with max_register in regmap_writeable. > > > > - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but DT_100THS > > is 0, max_regiter is 0x2f, then reg will be equal to 0x30, > > '0x30 < 0x2f' is false,so regmap_writeable will return false. > > > > - Root cause: the buf[] was written to a wrong place in the file > > drivers/rtc/rtc-pcf85363.c > > > > This is not true, the RTC wraps the register accesses properly and this This performance hack probably deserve some explanation in the code comment. :) > is probably something that should be handled by regmap_writable. The address wrapping is specific to this RTC chip. Is it also commonly used by other I2C devices? I'm not sure if regmap_writable should handle the wrapping case if it is too special. Regards, Leo
On 16/08/2019 10:50:49-0500, Li Yang wrote: > On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni > <alexandre.belloni@bootlin.com> wrote: > > > > On 16/08/2019 10:46:36+0800, Biwen Li wrote: > > > Issue: > > > - # hwclock -w > > > hwclock: RTC_SET_TIME: Invalid argument > > > > > > Why: > > > - Relative patch: https://lkml.org/lkml/2019/4/3/55 , this patch > > > will always check for unwritable registers, it will compare reg > > > with max_register in regmap_writeable. > > > > > > - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but DT_100THS > > > is 0, max_regiter is 0x2f, then reg will be equal to 0x30, > > > '0x30 < 0x2f' is false,so regmap_writeable will return false. > > > > > > - Root cause: the buf[] was written to a wrong place in the file > > > drivers/rtc/rtc-pcf85363.c > > > > > > > This is not true, the RTC wraps the register accesses properly and this > > This performance hack probably deserve some explanation in the code comment. :) > > > is probably something that should be handled by regmap_writable. > > The address wrapping is specific to this RTC chip. Is it also > commonly used by other I2C devices? I'm not sure if regmap_writable > should handle the wrapping case if it is too special. > Most of the i2c RTCs do address wrapping which is sometimes the only way to properly set the time.
On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > > On 16/08/2019 10:50:49-0500, Li Yang wrote: > > On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni > > <alexandre.belloni@bootlin.com> wrote: > > > > > > On 16/08/2019 10:46:36+0800, Biwen Li wrote: > > > > Issue: > > > > - # hwclock -w > > > > hwclock: RTC_SET_TIME: Invalid argument > > > > > > > > Why: > > > > - Relative patch: https://lkml.org/lkml/2019/4/3/55 , this patch > > > > will always check for unwritable registers, it will compare reg > > > > with max_register in regmap_writeable. > > > > > > > > - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but DT_100THS > > > > is 0, max_regiter is 0x2f, then reg will be equal to 0x30, > > > > '0x30 < 0x2f' is false,so regmap_writeable will return false. > > > > > > > > - Root cause: the buf[] was written to a wrong place in the file > > > > drivers/rtc/rtc-pcf85363.c > > > > > > > > > > This is not true, the RTC wraps the register accesses properly and this > > > > This performance hack probably deserve some explanation in the code comment. :) > > > > > is probably something that should be handled by regmap_writable. > > > > The address wrapping is specific to this RTC chip. Is it also > > commonly used by other I2C devices? I'm not sure if regmap_writable > > should handle the wrapping case if it is too special. > > > > Most of the i2c RTCs do address wrapping which is sometimes the only way > to properly set the time. Adding Mark and Nandor to the loop. Regards, Leo
On Fri, Aug 16, 2019 at 02:40:47PM -0500, Li Yang wrote: > On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni > > Most of the i2c RTCs do address wrapping which is sometimes the only way > > to properly set the time. > Adding Mark and Nandor to the loop. Is there a specific question or something here?
On Tue, Aug 20, 2019 at 1:24 PM Mark Brown <broonie@kernel.org> wrote: > > On Fri, Aug 16, 2019 at 02:40:47PM -0500, Li Yang wrote: > > On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni > > > > Most of the i2c RTCs do address wrapping which is sometimes the only way > > > to properly set the time. > > > Adding Mark and Nandor to the loop. > > Is there a specific question or something here? Some of the RTC hardware has the capability of address wrapping which means if you access a continuous address range across a certain boundary(could be the boundary of a regmap region) the hardware actually wrap the access to a lower address. But the address violation check of regmap rejects such access. According to Alexcandre, the address wrapping is essential to the functionality of some RTC devices and can improve performance for some others. We are wondering if it is reasonable to have regmap support this address wrapping. Regards, Leo
On 8/16/19 10:40 PM, Li Yang wrote: > On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni > <alexandre.belloni@bootlin.com> wrote: >> >> On 16/08/2019 10:50:49-0500, Li Yang wrote: >>> On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni >>> <alexandre.belloni@bootlin.com> wrote: >>>> >>>> On 16/08/2019 10:46:36+0800, Biwen Li wrote: >>>>> Issue: >>>>> - # hwclock -w >>>>> hwclock: RTC_SET_TIME: Invalid argument >>>>> >>>>> Why: >>>>> - Relative patch: https://lkml.org/lkml/2019/4/3/55 , this patch >>>>> will always check for unwritable registers, it will compare reg >>>>> with max_register in regmap_writeable. >>>>> >>>>> - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but DT_100THS >>>>> is 0, max_regiter is 0x2f, then reg will be equal to 0x30, >>>>> '0x30 < 0x2f' is false,so regmap_writeable will return false. >>>>> >>>>> - Root cause: the buf[] was written to a wrong place in the file >>>>> drivers/rtc/rtc-pcf85363.c >>>>> >>>> >>>> This is not true, the RTC wraps the register accesses properly and this >>> >>> This performance hack probably deserve some explanation in the code comment. :) >>> >>>> is probably something that should be handled by regmap_writable. >>> >>> The address wrapping is specific to this RTC chip. Is it also >>> commonly used by other I2C devices? I'm not sure if regmap_writable >>> should handle the wrapping case if it is too special. >>> >> >> Most of the i2c RTCs do address wrapping which is sometimes the only way >> to properly set the time. > > Adding Mark and Nandor to the loop. > > Regards, > Leo > Hi, `regmap` provides couple of ways to validate the registers: max_register, callback function and write table. All of these are optional, so it gives you the freedom to customize it as needed. In this situation probably you could: 1. Avoid using the wrapping feature of pcf85363 (you can just provide separate calls for stop, reset and time confguration). In this way the `max_register` validation method will work fine. 2. Replace `max_register` method validation with `callback function` validation method, were you could make your own validation. Regards, Nandor
On Tue, Aug 20, 2019 at 01:33:14PM -0500, Li Yang wrote: > Some of the RTC hardware has the capability of address wrapping which > means if you access a continuous address range across a certain > boundary(could be the boundary of a regmap region) the hardware > actually wrap the access to a lower address. But the address > violation check of regmap rejects such access. According to > Alexcandre, the address wrapping is essential to the functionality of It's *essential*? Will innovation never cease? > some RTC devices and can improve performance for some others. We are > wondering if it is reasonable to have regmap support this address > wrapping. I guess, I don't see any particular reason why not unless the patches are horrible or get in the way of other stuff.
On 21/08/2019 12:21:42+0100, Mark Brown wrote: > On Tue, Aug 20, 2019 at 01:33:14PM -0500, Li Yang wrote: > > > Some of the RTC hardware has the capability of address wrapping which > > means if you access a continuous address range across a certain > > boundary(could be the boundary of a regmap region) the hardware > > actually wrap the access to a lower address. But the address > > violation check of regmap rejects such access. According to > > Alexcandre, the address wrapping is essential to the functionality of > > It's *essential*? Will innovation never cease? > To be clear, for some RTCs, its is the only way to accurately set the time.
On Wed, Aug 21, 2019 at 01:24:13PM +0200, Alexandre Belloni wrote: > On 21/08/2019 12:21:42+0100, Mark Brown wrote: > > On Tue, Aug 20, 2019 at 01:33:14PM -0500, Li Yang wrote: > > > violation check of regmap rejects such access. According to > > > Alexcandre, the address wrapping is essential to the functionality of > > It's *essential*? Will innovation never cease? > To be clear, for some RTCs, its is the only way to accurately set the > time. What's the mechanism here? It's a very strange thing to require.
On 21/08/2019 12:30:29+0100, Mark Brown wrote: > On Wed, Aug 21, 2019 at 01:24:13PM +0200, Alexandre Belloni wrote: > > On 21/08/2019 12:21:42+0100, Mark Brown wrote: > > > On Tue, Aug 20, 2019 at 01:33:14PM -0500, Li Yang wrote: > > > > > violation check of regmap rejects such access. According to > > > > Alexcandre, the address wrapping is essential to the functionality of > > > > It's *essential*? Will innovation never cease? > > > To be clear, for some RTCs, its is the only way to accurately set the > > time. > > What's the mechanism here? It's a very strange thing to require. The clock control is on the first register, then you have sec, min, hour, day, mon, year. To be able to set the clock accurately, you need to first disable the clock, then set the time and date and finally reenable the clock in the first register. This should be done in a single i2c write.
On Wed, Aug 21, 2019 at 01:38:56PM +0200, Alexandre Belloni wrote: > On 21/08/2019 12:30:29+0100, Mark Brown wrote: > > What's the mechanism here? It's a very strange thing to require. > The clock control is on the first register, then you have sec, min, > hour, day, mon, year. > To be able to set the clock accurately, you need to first disable the > clock, then set the time and date and finally reenable the clock in the > first register. This should be done in a single i2c write. Ugh, right. And of course it would be silly to put the enable register last or done some sort of strobe thing.
> > On 8/16/19 10:40 PM, Li Yang wrote: > > On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni > > <alexandre.belloni@bootlin.com> wrote: > >> > >> On 16/08/2019 10:50:49-0500, Li Yang wrote: > >>> On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni > >>> <alexandre.belloni@bootlin.com> wrote: > >>>> > >>>> On 16/08/2019 10:46:36+0800, Biwen Li wrote: > >>>>> Issue: > >>>>> - # hwclock -w > >>>>> hwclock: RTC_SET_TIME: Invalid argument > >>>>> > >>>>> Why: > >>>>> - Relative patch: > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org > %2Flkml%2F2019%2F4%2F3%2F55&data=02%7C01%7Cbiwen.li%40nxp. > com%7Cff8cebc3f1034ae3fa9608d725ff9e5e%7C686ea1d3bc2b4c6fa92cd99 > c5c301635%7C0%7C0%7C637019652111923736&sdata=spY6e22YOkOF > 3%2BF7crSM0M6xPmOhgULDqMZLQw%2BAmdI%3D&reserved=0 , this > patch > >>>>> will always check for unwritable registers, it will compare reg > >>>>> with max_register in regmap_writeable. > >>>>> > >>>>> - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but > DT_100THS > >>>>> is 0, max_regiter is 0x2f, then reg will be equal to 0x30, > >>>>> '0x30 < 0x2f' is false,so regmap_writeable will return false. > >>>>> > >>>>> - Root cause: the buf[] was written to a wrong place in the file > >>>>> drivers/rtc/rtc-pcf85363.c > >>>>> > >>>> > >>>> This is not true, the RTC wraps the register accesses properly and > >>>> this > >>> > >>> This performance hack probably deserve some explanation in the code > >>> comment. :) > >>> > >>>> is probably something that should be handled by regmap_writable. > >>> > >>> The address wrapping is specific to this RTC chip. Is it also > >>> commonly used by other I2C devices? I'm not sure if regmap_writable > >>> should handle the wrapping case if it is too special. > >>> > >> > >> Most of the i2c RTCs do address wrapping which is sometimes the only > >> way to properly set the time. > > > > Adding Mark and Nandor to the loop. > > > > Regards, > > Leo > > > > Hi, > `regmap` provides couple of ways to validate the registers: > max_register, callback function and write table. All of these are optional, so it > gives you the freedom to customize it as needed. > > In this situation probably you could: > 1. Avoid using the wrapping feature of pcf85363 (you can just provide > separate calls for stop, reset and time confguration). In this way the > `max_register` validation method will work fine. Yes, I use this way. Path as follows: Stop and reset - > set time > stop > 2. Replace `max_register` method validation with `callback function` > validation method, were you could make your own validation. It is not work, show the code in as follows: bool regmap_writeable(struct regmap *map, unsigned int reg) { if (map->max_register && reg > map->max_register) return false; Callback function (writeable_reg) will not be called. if (map->writeable_reg) return map->writeable_reg(map->dev, reg); if (map->wr_table) return regmap_check_range_table(map, reg, map->wr_table); return true; } > > > Regards, > Nandor >
On 8/26/19 7:29 AM, Biwen Li wrote: >> >> On 8/16/19 10:40 PM, Li Yang wrote: >>> On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni >>> <alexandre.belloni@bootlin.com> wrote: >>>> >>>> On 16/08/2019 10:50:49-0500, Li Yang wrote: >>>>> On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni >>>>> <alexandre.belloni@bootlin.com> wrote: >>>>>> >>>>>> On 16/08/2019 10:46:36+0800, Biwen Li wrote: >>>>>>> Issue: >>>>>>> - # hwclock -w >>>>>>> hwclock: RTC_SET_TIME: Invalid argument >>>>>>> >>>>>>> Why: >>>>>>> - Relative patch: >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org >> %2Flkml%2F2019%2F4%2F3%2F55&data=02%7C01%7Cbiwen.li%40nxp. >> com%7Cff8cebc3f1034ae3fa9608d725ff9e5e%7C686ea1d3bc2b4c6fa92cd99 >> c5c301635%7C0%7C0%7C637019652111923736&sdata=spY6e22YOkOF >> 3%2BF7crSM0M6xPmOhgULDqMZLQw%2BAmdI%3D&reserved=0 , this >> patch >>>>>>> will always check for unwritable registers, it will compare reg >>>>>>> with max_register in regmap_writeable. >>>>>>> >>>>>>> - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but >> DT_100THS >>>>>>> is 0, max_regiter is 0x2f, then reg will be equal to 0x30, >>>>>>> '0x30 < 0x2f' is false,so regmap_writeable will return false. >>>>>>> >>>>>>> - Root cause: the buf[] was written to a wrong place in the file >>>>>>> drivers/rtc/rtc-pcf85363.c >>>>>>> >>>>>> >>>>>> This is not true, the RTC wraps the register accesses properly and >>>>>> this >>>>> >>>>> This performance hack probably deserve some explanation in the code >>>>> comment. :) >>>>> >>>>>> is probably something that should be handled by regmap_writable. >>>>> >>>>> The address wrapping is specific to this RTC chip. Is it also >>>>> commonly used by other I2C devices? I'm not sure if regmap_writable >>>>> should handle the wrapping case if it is too special. >>>>> >>>> >>>> Most of the i2c RTCs do address wrapping which is sometimes the only >>>> way to properly set the time. >>> >>> Adding Mark and Nandor to the loop. >>> >>> Regards, >>> Leo >>> >> >> Hi, >> `regmap` provides couple of ways to validate the registers: >> max_register, callback function and write table. All of these are optional, so it >> gives you the freedom to customize it as needed. >> >> In this situation probably you could: >> 1. Avoid using the wrapping feature of pcf85363 (you can just provide >> separate calls for stop, reset and time confguration). In this way the >> `max_register` validation method will work fine. > Yes, I use this way. Path as follows: > Stop and reset - > set time > stop > Some of the concerns regarding this method was that it might not be precise enough. That because you need 2 I2C operations (one for stop and one for time configuration). Not sure about your case if this is a problem or not. >> 2. Replace `max_register` method validation with `callback function` >> validation method, were you could make your own validation. > It is not work, show the code in as follows: > > bool regmap_writeable(struct regmap *map, unsigned int reg) > { > if (map->max_register && reg > map->max_register) > return false; > Callback function (writeable_reg) will not be called. > if (map->writeable_reg) > return map->writeable_reg(map->dev, reg); Hi Li, If you *replace* the `max_register` method with `callback function` it should work. The code above will use every method *if provided*. In other words if `map->max_register` is 0 will go to the next step and check `map->writeable_reg`. Right? Regards, Nandor
> > On 8/26/19 7:29 AM, Biwen Li wrote: > >> > >> On 8/16/19 10:40 PM, Li Yang wrote: > >>> On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni > >>> <alexandre.belloni@bootlin.com> wrote: > >>>> > >>>> On 16/08/2019 10:50:49-0500, Li Yang wrote: > >>>>> On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni > >>>>> <alexandre.belloni@bootlin.com> wrote: > >>>>>> > >>>>>> On 16/08/2019 10:46:36+0800, Biwen Li wrote: > >>>>>>> Issue: > >>>>>>> - # hwclock -w > >>>>>>> hwclock: RTC_SET_TIME: Invalid argument > >>>>>>> > >>>>>>> Why: > >>>>>>> - Relative patch: > >> https://lkml.org > >> %2Flkml%2F2019%2F4%2F3%2F55&data=02%7C01%7Cbiwen.li%40n > xp. > >> > com%7Cff8cebc3f1034ae3fa9608d725ff9e5e%7C686ea1d3bc2b4c6fa92cd99 > >> > c5c301635%7C0%7C0%7C637019652111923736&sdata=spY6e22YOkOF > >> 3%2BF7crSM0M6xPmOhgULDqMZLQw%2BAmdI%3D&reserved=0 , > this patch > >>>>>>> will always check for unwritable registers, it will compare reg > >>>>>>> with max_register in regmap_writeable. > >>>>>>> > >>>>>>> - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but > >> DT_100THS > >>>>>>> is 0, max_regiter is 0x2f, then reg will be equal to 0x30, > >>>>>>> '0x30 < 0x2f' is false,so regmap_writeable will return false. > >>>>>>> > >>>>>>> - Root cause: the buf[] was written to a wrong place in the file > >>>>>>> drivers/rtc/rtc-pcf85363.c > >>>>>>> > >>>>>> > >>>>>> This is not true, the RTC wraps the register accesses properly > >>>>>> and this > >>>>> > >>>>> This performance hack probably deserve some explanation in the > >>>>> code comment. :) > >>>>> > >>>>>> is probably something that should be handled by regmap_writable. > >>>>> > >>>>> The address wrapping is specific to this RTC chip. Is it also > >>>>> commonly used by other I2C devices? I'm not sure if > >>>>> regmap_writable should handle the wrapping case if it is too special. > >>>>> > >>>> > >>>> Most of the i2c RTCs do address wrapping which is sometimes the > >>>> only way to properly set the time. > >>> > >>> Adding Mark and Nandor to the loop. > >>> > >>> Regards, > >>> Leo > >>> > >> > >> Hi, > >> `regmap` provides couple of ways to validate the registers: > >> max_register, callback function and write table. All of these are > >> optional, so it gives you the freedom to customize it as needed. > >> > >> In this situation probably you could: > >> 1. Avoid using the wrapping feature of pcf85363 (you can just > >> provide separate calls for stop, reset and time confguration). In > >> this way the `max_register` validation method will work fine. > > Yes, I use this way. Path as follows: > > Stop and reset - > set time > stop > > > > Some of the concerns regarding this method was that it might not be precise > enough. That because you need 2 I2C operations (one for stop and one for time > configuration). Not sure about your case if this is a problem or not. Ok, got it, thanks. > > >> 2. Replace `max_register` method validation with `callback > >> function` validation method, were you could make your own validation. > > It is not work, show the code in as follows: > > > > bool regmap_writeable(struct regmap *map, unsigned int reg) { > > if (map->max_register && reg > map->max_register) > > return false; > > Callback function (writeable_reg) will not be called. > > if (map->writeable_reg) > > return map->writeable_reg(map->dev, reg); > > Hi Li, > If you *replace* the `max_register` method with `callback function` it > should work. The code above will use every method *if provided*. In other > words if `map->max_register` is 0 will go to the next step and check > `map->writeable_reg`. Right? Yes, you are right. Thanks. > > > > Regards, > Nandor
Hi, On 26/08/2019 09:49:49+0000, Biwen Li wrote: > > > > On 8/26/19 7:29 AM, Biwen Li wrote: > > >> > > >> On 8/16/19 10:40 PM, Li Yang wrote: > > >>> On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni > > >>> <alexandre.belloni@bootlin.com> wrote: > > >>>> > > >>>> On 16/08/2019 10:50:49-0500, Li Yang wrote: > > >>>>> On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni > > >>>>> <alexandre.belloni@bootlin.com> wrote: > > >>>>>> > > >>>>>> On 16/08/2019 10:46:36+0800, Biwen Li wrote: > > >>>>>>> Issue: > > >>>>>>> - # hwclock -w > > >>>>>>> hwclock: RTC_SET_TIME: Invalid argument > > >>>>>>> > > >>>>>>> Why: > > >>>>>>> - Relative patch: > > >> https://lkml.org > > >> %2Flkml%2F2019%2F4%2F3%2F55&data=02%7C01%7Cbiwen.li%40n > > xp. > > >> > > com%7Cff8cebc3f1034ae3fa9608d725ff9e5e%7C686ea1d3bc2b4c6fa92cd99 > > >> > > c5c301635%7C0%7C0%7C637019652111923736&sdata=spY6e22YOkOF > > >> 3%2BF7crSM0M6xPmOhgULDqMZLQw%2BAmdI%3D&reserved=0 , > > this patch > > >>>>>>> will always check for unwritable registers, it will compare reg > > >>>>>>> with max_register in regmap_writeable. > > >>>>>>> > > >>>>>>> - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but > > >> DT_100THS > > >>>>>>> is 0, max_regiter is 0x2f, then reg will be equal to 0x30, > > >>>>>>> '0x30 < 0x2f' is false,so regmap_writeable will return false. > > >>>>>>> > > >>>>>>> - Root cause: the buf[] was written to a wrong place in the file > > >>>>>>> drivers/rtc/rtc-pcf85363.c > > >>>>>>> > > >>>>>> > > >>>>>> This is not true, the RTC wraps the register accesses properly > > >>>>>> and this > > >>>>> > > >>>>> This performance hack probably deserve some explanation in the > > >>>>> code comment. :) > > >>>>> > > >>>>>> is probably something that should be handled by regmap_writable. > > >>>>> > > >>>>> The address wrapping is specific to this RTC chip. Is it also > > >>>>> commonly used by other I2C devices? I'm not sure if > > >>>>> regmap_writable should handle the wrapping case if it is too special. > > >>>>> > > >>>> > > >>>> Most of the i2c RTCs do address wrapping which is sometimes the > > >>>> only way to properly set the time. > > >>> > > >>> Adding Mark and Nandor to the loop. > > >>> > > >>> Regards, > > >>> Leo > > >>> > > >> > > >> Hi, > > >> `regmap` provides couple of ways to validate the registers: > > >> max_register, callback function and write table. All of these are > > >> optional, so it gives you the freedom to customize it as needed. > > >> > > >> In this situation probably you could: > > >> 1. Avoid using the wrapping feature of pcf85363 (you can just > > >> provide separate calls for stop, reset and time confguration). In > > >> this way the `max_register` validation method will work fine. > > > Yes, I use this way. Path as follows: > > > Stop and reset - > set time > stop > > > > > > > Some of the concerns regarding this method was that it might not be precise > > enough. That because you need 2 I2C operations (one for stop and one for time > > configuration). Not sure about your case if this is a problem or not. > Ok, got it, thanks. To be clear, for this RTC it is fine to separate both writes. Want I want is a corrected commit message with a proper reference to 8b9f9d4dc511309918c4f6793bae7387c0c638af instead of a link to lkml.org and a proper explanation.
> > Hi, > > On 26/08/2019 09:49:49+0000, Biwen Li wrote: > > > > > > On 8/26/19 7:29 AM, Biwen Li wrote: > > > >> > > > >> On 8/16/19 10:40 PM, Li Yang wrote: > > > >>> On Fri, Aug 16, 2019 at 11:30 AM Alexandre Belloni > > > >>> <alexandre.belloni@bootlin.com> wrote: > > > >>>> > > > >>>> On 16/08/2019 10:50:49-0500, Li Yang wrote: > > > >>>>> On Fri, Aug 16, 2019 at 3:05 AM Alexandre Belloni > > > >>>>> <alexandre.belloni@bootlin.com> wrote: > > > >>>>>> > > > >>>>>> On 16/08/2019 10:46:36+0800, Biwen Li wrote: > > > >>>>>>> Issue: > > > >>>>>>> - # hwclock -w > > > >>>>>>> hwclock: RTC_SET_TIME: Invalid argument > > > >>>>>>> > > > >>>>>>> Why: > > > >>>>>>> - Relative patch: > > > >> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2 > > > >> > Flkml.org&data=02%7C01%7Cbiwen.li%40nxp.com%7C03141ff7858343 > 3 > > > >> > 20be408d72a0d1e10%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0% > 7C63 > > > >> > 7024108138794294&sdata=QrALkFN6heF%2B7S73FQ9c%2FyKNRHyBuL > %2B6 > > > >> %2B4PDM9hYRyM%3D&reserved=0 > > > >> %2Flkml%2F2019%2F4%2F3%2F55&data=02%7C01%7Cbiwen.li% > 40n > > > xp. > > > >> > > > > com%7Cff8cebc3f1034ae3fa9608d725ff9e5e%7C686ea1d3bc2b4c6fa92cd99 > > > >> > > > > c5c301635%7C0%7C0%7C637019652111923736&sdata=spY6e22YOkOF > > > >> > 3%2BF7crSM0M6xPmOhgULDqMZLQw%2BAmdI%3D&reserved=0 , > > > this patch > > > >>>>>>> will always check for unwritable registers, it will compare > reg > > > >>>>>>> with max_register in regmap_writeable. > > > >>>>>>> > > > >>>>>>> - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, > > > >>>>>>> but > > > >> DT_100THS > > > >>>>>>> is 0, max_regiter is 0x2f, then reg will be equal to 0x30, > > > >>>>>>> '0x30 < 0x2f' is false,so regmap_writeable will return > false. > > > >>>>>>> > > > >>>>>>> - Root cause: the buf[] was written to a wrong place in the > file > > > >>>>>>> drivers/rtc/rtc-pcf85363.c > > > >>>>>>> > > > >>>>>> > > > >>>>>> This is not true, the RTC wraps the register accesses > > > >>>>>> properly and this > > > >>>>> > > > >>>>> This performance hack probably deserve some explanation in the > > > >>>>> code comment. :) > > > >>>>> > > > >>>>>> is probably something that should be handled by regmap_writable. > > > >>>>> > > > >>>>> The address wrapping is specific to this RTC chip. Is it also > > > >>>>> commonly used by other I2C devices? I'm not sure if > > > >>>>> regmap_writable should handle the wrapping case if it is too special. > > > >>>>> > > > >>>> > > > >>>> Most of the i2c RTCs do address wrapping which is sometimes the > > > >>>> only way to properly set the time. > > > >>> > > > >>> Adding Mark and Nandor to the loop. > > > >>> > > > >>> Regards, > > > >>> Leo > > > >>> > > > >> > > > >> Hi, > > > >> `regmap` provides couple of ways to validate the registers: > > > >> max_register, callback function and write table. All of these are > > > >> optional, so it gives you the freedom to customize it as needed. > > > >> > > > >> In this situation probably you could: > > > >> 1. Avoid using the wrapping feature of pcf85363 (you can just > > > >> provide separate calls for stop, reset and time confguration). In > > > >> this way the `max_register` validation method will work fine. > > > > Yes, I use this way. Path as follows: > > > > Stop and reset - > set time > stop > > > > > > > > > > Some of the concerns regarding this method was that it might not be > > > precise enough. That because you need 2 I2C operations (one for stop > > > and one for time configuration). Not sure about your case if this is a problem > or not. > > Ok, got it, thanks. > > To be clear, for this RTC it is fine to separate both writes. Want I want is a > corrected commit message with a proper reference to > 8b9f9d4dc511309918c4f6793bae7387c0c638af instead of a link to lkml.org > and a proper explanation. Ok, got it, thanks.I will replace link to lkml.org with 8b9f9d4dc511309918c4f6793bae7387c0c638af and add a proper explanation to the commit message in v4. > > -- > Alexandre Belloni, Bootlin > Embedded Linux and Kernel engineering > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootlin. > com&data=02%7C01%7Cbiwen.li%40nxp.com%7C03141ff7858343320b > e408d72a0d1e10%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6 > 37024108138794294&sdata=XnAxJmOkh1VVA9ed%2FLr%2BbvWbVpLD > bwLjJrdaFidRtDk%3D&reserved=0
diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c index a075e77617dc..3450d615974d 100644 --- a/drivers/rtc/rtc-pcf85363.c +++ b/drivers/rtc/rtc-pcf85363.c @@ -166,7 +166,12 @@ static int pcf85363_rtc_set_time(struct device *dev, struct rtc_time *tm) buf[DT_YEARS] = bin2bcd(tm->tm_year % 100); ret = regmap_bulk_write(pcf85363->regmap, CTRL_STOP_EN, - tmp, sizeof(tmp)); + tmp, 2); + if (ret) + return ret; + + ret = regmap_bulk_write(pcf85363->regmap, DT_100THS, + buf, sizeof(tmp) - 2); if (ret) return ret;
Issue: - # hwclock -w hwclock: RTC_SET_TIME: Invalid argument Why: - Relative patch: https://lkml.org/lkml/2019/4/3/55 , this patch will always check for unwritable registers, it will compare reg with max_register in regmap_writeable. - In drivers/rtc/rtc-pcf85363.c, CTRL_STOP_EN is 0x2e, but DT_100THS is 0, max_regiter is 0x2f, then reg will be equal to 0x30, '0x30 < 0x2f' is false,so regmap_writeable will return false. - Root cause: the buf[] was written to a wrong place in the file drivers/rtc/rtc-pcf85363.c How: - Split of writing regs to two parts, first part writes control registers about stop_enable and resets, second part writes RTC time and date registers. Signed-off-by: Biwen Li <biwen.li@nxp.com> --- Change in v2: - add Why and How into commit message. drivers/rtc/rtc-pcf85363.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)