Re: [RESEND] rtc: hym8563: try multiple times to init device

[Robin Murphy]

On 2022-08-21 13:26, Frank Wunderlich wrote:
> From: Peter Geis <pgwipeout@xxxxxxxxx>
>
> RTC sometimes does not respond the first time in init.
> Try multiple times to get a response.
>
> Signed-off-by: Peter Geis <pgwipeout@xxxxxxxxx>
> Signed-off-by: Frank Wunderlich <frank-w@xxxxxxxxxxxxxxx>
> ---
> discussion from v1
> https://patchwork.kernel.org/project/linux-rockchip/patch/20220608161150.58919-2-linux@xxxxxxxxx/
>
> On Fri, Jul 8, 2022 at 12:18 PM Robin Murphy <robin.murphy@xxxxxxx> wrote:
> > FWIW, given that HYM8563 is fairly common on RK3288 boards - I can't say
> > I've ever noticed an issue with mine, for instance - it seems dubious
> > that this would be a general issue of the chip itself. Are you sure it's
> > not a SoC or board-level issue with the I2C bus being in a funny initial
> > state, timings being marginal, or suchlike?
>
> Peter Geis <pgwipeout@xxxxxxxxx>:
> I don't think this is an SoC issue since this is the first instance
> I've encountered it. Mind you we don't have the reset lines hooked up
> at all for the Rockchip i2c driver, so it's possible that's the case,
> but I'd imagine it would be observed more broadly if that was the
> case. I've tried pushing the timings out pretty far as well as bumping
> up the drive strength to no change. It seems to occur only with the
> hym rtc used on this board. I suspect it's a new variant of the hym
> that has slightly different behavior.

Sure, if it's documented somewhere that Hayou (or if the BPI-R2 Pro 
schematic is to be believed, AnalogTek) decided to innovate a new 
"sometimes doesn't work" feature for a chip that's been in production 
for a decade or more, and that 2 retries at 20ms intervals is what's 
recommended, then I'm open to believing that this isn't a complete hack. 
Or at least if someone can say they've scoped the pins and confirmed 
that nothing looks suspect at the protocol level when this happens that 
could explain it.

Otherwise, I'll remain unconvinced that it isn't a coincidence that this 
has shown up while bringing up a new board with a new SoC, and hacking a 
mature common driver to bodge around an issue that isn't fully 
understood, and could very conceivably lie elsewhere, is not the right 
answer. Especially when it involves a board vendor... let's say, whose 
reputation proceeds them.

Since I'm not above wasting 20 minutes of my time to prove a point, for 
starters the schematic seems to imply that it's using a variant of RK809 
where LDO4, used as the I/O supply for i2c3, is off by default, so on 
the face of it it could be something as stupidly simple as the RTC probe 
racing with the PMIC or I/O domain probe. Sure, the DT claims it's 
already on at boot, but *is* it? Maybe that was true with some 
downstream bootloader, but do we know that's what you're using to boot 
mainline? Maybe this something so obvious that you've already confirmed 
and taken it for granted, but the patch as presented doesn't give me the 
confidence to rule *anything* out.

Thanks,
Robin.

> ---
>   drivers/rtc/rtc-hym8563.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/rtc/rtc-hym8563.c b/drivers/rtc/rtc-hym8563.c
> index cc710d682121..d9d0b6615a07 100644
> --- a/drivers/rtc/rtc-hym8563.c
> +++ b/drivers/rtc/rtc-hym8563.c
> @@ -13,6 +13,7 @@
>   #include <linux/clk-provider.h>
>   #include <linux/i2c.h>
>   #include <linux/bcd.h>
> +#include <linux/delay.h>
>   #include <linux/rtc.h>
>   
>   #define HYM8563_CTL1		0x00
> @@ -438,10 +439,16 @@ static irqreturn_t hym8563_irq(int irq, void *dev_id)
>   
>   static int hym8563_init_device(struct i2c_client *client)
>   {
> -	int ret;
> +	int ret, i;
>   
>   	/* Clear stop flag if present */
> -	ret = i2c_smbus_write_byte_data(client, HYM8563_CTL1, 0);
> +	for (i = 0; i < 3; i++) {
> +		ret = i2c_smbus_write_byte_data(client, HYM8563_CTL1, 0);
> +		if (ret == 0)
> +			break;
> +		msleep(20);
> +	}
> +
>   	if (ret < 0)
>   		return ret;
>