On Wed, Feb 15, 2023 at 7:13 AM Alexandre Belloni
<alexandre.belloni@xxxxxxxxxxx> wrote:
>
> On 01/02/2023 17:16:12+0800, Binbin Zhou wrote:
> > Hi Alexandre:
> >
> > Sorry for the late reply.
> >
> > On Tue, Jan 24, 2023 at 7:37 AM Alexandre Belloni
> > <alexandre.belloni@xxxxxxxxxxx> wrote:
> > >
> > > Hello,
> > >
> > >
> > > On 09/01/2023 09:35:12+0800, Binbin Zhou wrote:
> > > > This RTC module is integrated into the Loongson-2K SoC and the LS7A
> > > > bridge chip. This version is almost entirely rewritten to make use of
> > > > current kernel API, and it supports both ACPI and DT.
> > > >
> > > > This driver is shared by MIPS-based Loongson-3A4000 system (use FDT) and
> > > > LoongArch-based Loongson-3A5000 system (use ACPI).
> > > >
> > >
> > > checkpatch.pl --strict complains, please fix the warnings and checks
> >
> > Got.
> > >
> > >
> > > > +#ifdef CONFIG_ACPI
> > > > +static u32 ls2x_acpi_fix_handler(void *id)
> > > > +{
> > > > + int ret;
> > > > + struct ls2x_rtc_priv *priv = (struct ls2x_rtc_priv *)id;
> > > > +
> > > > + spin_lock(&priv->rtc_reglock);
> > > > +
> > > > + /* Disable acpi rtc enabled */
> > > > + ret = readl(priv->acpi_base + PM1_EN_REG) & ~RTC_EN;
> > > > + writel(ret, priv->acpi_base + PM1_EN_REG);
> > > > +
> > > > + /* Clear acpi rtc interrupt Status */
> > > > + writel(RTC_STS, priv->acpi_base + PM1_STS_REG);
> > > > +
> > > > + spin_unlock(&priv->rtc_reglock);
> > > > +
> > > > + /*
> > > > + * The TOY_MATCH0_REG should be cleared 0 here,
> > > > + * otherwise the interrupt cannot be cleared.
> > > > + * Because the match condition is still satisfied
> > > > + */
> > > > + ret = regmap_write(priv->regmap, TOY_MATCH0_REG, 0);
> > > > + if (ret < 0)
> > > > + return ret;
> > >
> > > How is this an ACPI related issue? I guess the same would happen on
> > > !ACPI.
> >
> > I just assumed that the function would only be called under CONFIG_ACPI.
> > >
> > > > +
> > > > + rtc_update_irq(priv->rtcdev, 1, RTC_AF | RTC_IRQF);
> > >
> > > This is not useful, at that time, userspace has had no chance to open
> > > the RTC device file as it is not created yet.
> > >
> > > > + return 0;
> > > > +}
> > > > +#endif
> > > > +
> > > > +static inline void ls2x_rtc_regs_to_time(struct ls2x_rtc_regs *regs,
> > >
> > > Those static inline functions seem to be used only once, you should just
> > > put the code in the proper location.
> >
> > I will do it in the next version.
> > >
> > > > + struct rtc_time *tm)
> > > > +{
> > > > + tm->tm_year = regs->reg1;
> > > > + tm->tm_sec = FIELD_GET(TOY_SEC, regs->reg0);
> > > > + tm->tm_min = FIELD_GET(TOY_MIN, regs->reg0);
> > > > + tm->tm_hour = FIELD_GET(TOY_HOUR, regs->reg0);
> > > > + tm->tm_mday = FIELD_GET(TOY_DAY, regs->reg0);
> > > > + tm->tm_mon = FIELD_GET(TOY_MON, regs->reg0) - 1;
> > > > +}
> > > > +
> > > > +static inline void ls2x_rtc_time_to_regs(struct rtc_time *tm,
> > > > + struct ls2x_rtc_regs *regs)
> > > > +{
> > > > + regs->reg0 = FIELD_PREP(TOY_SEC, tm->tm_sec);
> > > > + regs->reg0 |= FIELD_PREP(TOY_MIN, tm->tm_min);
> > > > + regs->reg0 |= FIELD_PREP(TOY_HOUR, tm->tm_hour);
> > > > + regs->reg0 |= FIELD_PREP(TOY_DAY, tm->tm_mday);
> > > > + regs->reg0 |= FIELD_PREP(TOY_MON, tm->tm_mon + 1);
> > > > + regs->reg1 = tm->tm_year;
> > > > +}
> > > > +
> > > > +static inline void ls2x_rtc_alarm_regs_to_time(struct ls2x_rtc_regs *regs,
> > > > + struct rtc_time *tm)
> > > > +{
> > > > + tm->tm_sec = FIELD_GET(TOY_MATCH_SEC, regs->reg0);
> > > > + tm->tm_min = FIELD_GET(TOY_MATCH_MIN, regs->reg0);
> > > > + tm->tm_hour = FIELD_GET(TOY_MATCH_HOUR, regs->reg0);
> > > > + tm->tm_mday = FIELD_GET(TOY_MATCH_DAY, regs->reg0);
> > > > + tm->tm_mon = FIELD_GET(TOY_MATCH_MON, regs->reg0) - 1;
> > > > + /*
> > > > + * The rtc SYS_TOYMATCH0/YEAR bit field is only 6 bits, so it means 63
> > > > + * years at most. Therefore, The RTC alarm years can be set from 1900
> > > > + * to 1963. This causes the initialization of alarm fail during call
> > > > + * __rtc_read_alarm.
> > > > + * We add 64 years offset to ls2x_rtc_read_alarm. After adding the
> > > > + * offset, the RTC alarm clock can be set from 1964 to 2027.
> > > > + */
> > > > + tm->tm_year = FIELD_GET(TOY_MATCH_YEAR, regs->reg0) + 64;
> > >
> > > This is not symmetric with ls2x_rtc_time_to_alarm_regs, how can it work?
> >
> > This is to avoid an "invalid alarm value" at boot time, which of
> > course should not be a good solution.
> > When the alarm value is read at boot time, "year" is not yet set to
> > the proper value so the year is always set to 1900.
>
> Why isn't it set at boot time? Isn't the register persisting after a
> reboot?
> Setting a bogus alarm is not a solution.
>
Hi, Alexandre:
Sorry, I seem to have misled the issue.
This is a hardware bug, as we know from the datasheet, the year field
in the TOY_MATCH register has only 6 bits (bit[31:26]), so the maximum
value is 63. For example, 2023 can only be read here as 1959,
resulting in an invalid alarm.
The current workaround: after reading the year field in
ls2x_rtc_read_alarm(), manually add 64 or a multiple of 64 (equivalent
to completing the high bits), which also ensures that the reading and
writing is consistent.
> >
> > How about just "tm->tm_year = -1", as the alarm is now only read when booting?
> >
> > >
> > > > +}
> > > > +
> > > > +static inline void ls2x_rtc_time_to_alarm_regs(struct rtc_time *tm,
> > > > + struct ls2x_rtc_regs *regs)
> > > > +{
> > > > + regs->reg0 = FIELD_PREP(TOY_MATCH_SEC, tm->tm_sec);
> > > > + regs->reg0 |= FIELD_PREP(TOY_MATCH_MIN, tm->tm_min);
> > > > + regs->reg0 |= FIELD_PREP(TOY_MATCH_HOUR, tm->tm_hour);
> > > > + regs->reg0 |= FIELD_PREP(TOY_MATCH_DAY, tm->tm_mday);
> > > > + regs->reg0 |= FIELD_PREP(TOY_MATCH_MON, tm->tm_mon + 1);
> > > > + regs->reg0 |= FIELD_PREP(TOY_MATCH_YEAR, tm->tm_year);
> > > > +}
> > > > +
> > > > +static int ls2x_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > > > +{
> > > > + int ret;
> > > > + struct ls2x_rtc_regs regs;
> > > > + struct ls2x_rtc_priv *priv = dev_get_drvdata(dev);
> > > > +
> > > > + ret = regmap_read(priv->regmap, TOY_READ1_REG, ®s.reg1);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + ret = regmap_read(priv->regmap, TOY_READ0_REG, ®s.reg0);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > >
> > > I never got a reply to my question:
> > >
> > > "
> > > I'm actually wondering why you read first TOY_READ1_REG then
> > > TOY_READ0_REG. ls1x does the opposite and the ls1c datasheet I found
> > > doesn't mention any latching happening. So unless latching is done on
> > > TOY_READ1_REG, you could use regmap_bulk_read and simply avoid struct
> > > ls2x_rtc_regs.
> > > If there is no latching, you may need to read TOY_READ0_REG at least
> > > twice. Because TOY_READ1_REG only contains the year, it is an issue only
> > > on 31 of December and it will not be easy to reproduce.
> > > "
> > >
> >
> > The LS7A and Loongson-2K datasheets also do not mention any latching
> > happening. Reading TOY_READ1_REG first is probably just a matter of
> > habit.
> > I tried using regmap_bulk_xxx() and it also reads and writes time
> > properly. In the next version I will rewrite this part of the code.
> >
> > Example:
> >
> > #define LS2X_NUM_TIME_REGS 2
> >
> > u32 rtc_data[LS2X_NUM_TIME_REGS];
> > struct ls2x_rtc_priv *priv = dev_get_drvdata(dev);
> >
> > ret = regmap_bulk_read(priv->regmap, TOY_READ0_REG, rtc_data,
> > LS2X_NUM_TIME_REGS);
> >
>
> Doing a bulk read doesn't guarantee the atomicity of the operation. You
> really must check whether a register changed while reading the other
> one.
>
How about protecting with mutex?
> >
> > >
> > > > + ls2x_rtc_regs_to_time(®s, tm);
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int ls2x_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > > > +{
> > > > + int ret;
> > > > + struct ls2x_rtc_regs regs;
> > > > + struct ls2x_rtc_priv *priv = dev_get_drvdata(dev);
> > > > +
> > > > + ls2x_rtc_time_to_regs(tm, ®s);
> > > > +
> > > > + ret = regmap_write(priv->regmap, TOY_WRITE0_REG, regs.reg0);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + return regmap_write(priv->regmap, TOY_WRITE1_REG, regs.reg1);
> > > > +}
> > > > +
> > > > +static int ls2x_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> > > > +{
> > > > + int ret;
> > > > + struct ls2x_rtc_regs regs;
> > > > + struct ls2x_rtc_priv *priv = dev_get_drvdata(dev);
> > > > +
> > > > + ret = regmap_read(priv->regmap, TOY_MATCH0_REG, ®s.reg0);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + ls2x_rtc_alarm_regs_to_time(®s, &alrm->time);
> > > > + alrm->enabled = !!(readl(priv->acpi_base + PM1_EN_REG) & RTC_EN);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int ls2x_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
> > > > +{
> > > > + u32 val;
> > > > + struct ls2x_rtc_priv *priv = dev_get_drvdata(dev);
> > > > +
> > > > + spin_lock(&priv->rtc_reglock);
> > > > + val = readl(priv->acpi_base + PM1_EN_REG);
> > > > +
> > > > + /* Enalbe RTC alarm */
> > > Typo
> > >
> > > > + writel((enabled ? val | RTC_EN : val & ~RTC_EN),
> > > > + priv->acpi_base + PM1_EN_REG);
> > > > + spin_unlock(&priv->rtc_reglock);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int ls2x_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> > > > +{
> > > > + int ret;
> > > > + struct ls2x_rtc_regs regs;
> > > > + struct ls2x_rtc_priv *priv = dev_get_drvdata(dev);
> > > > +
> > > > + ls2x_rtc_time_to_alarm_regs(&alrm->time, ®s);
> > > > +
> > > > + ret = regmap_write(priv->regmap, TOY_MATCH0_REG, regs.reg0);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + return ls2x_rtc_alarm_irq_enable(dev, alrm->enabled);
> > > > +}
> > > > +
> > > > +static const struct rtc_class_ops ls2x_rtc_ops = {
> > > > + .read_time = ls2x_rtc_read_time,
> > > > + .set_time = ls2x_rtc_set_time,
> > > > + .read_alarm = ls2x_rtc_read_alarm,
> > > > + .set_alarm = ls2x_rtc_set_alarm,
> > > > + .alarm_irq_enable = ls2x_rtc_alarm_irq_enable,
> > > > +};
> > > > +
> > > > +static int ls2x_enable_rtc(struct ls2x_rtc_priv *priv)
> > > > +{
> > > > + u32 val;
> > > > + int ret;
> > > > +
> > > > + ret = regmap_read(priv->regmap, RTC_CTRL_REG, &val);
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + return regmap_write(priv->regmap, RTC_CTRL_REG,
> > > > + val | TOY_ENABLE | OSC_ENABLE);
> > > > +}
> > > > +
> > > > +static int ls2x_rtc_probe(struct platform_device *pdev)
> > > > +{
> > > > + int ret;
> > > > + void __iomem *regs;
> > > > + struct ls2x_rtc_priv *priv;
> > > > + struct device *dev = &pdev->dev;
> > > > +
> > > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > > + if (!priv)
> > > > + return -ENOMEM;
> > > > +
> > > > + priv->irq = platform_get_irq(pdev, 0);
> > > > + if (priv->irq < 0)
> > > > + return dev_err_probe(dev, priv->irq, "platform_get_irq failed\n");
> > > > +
> > > > + platform_set_drvdata(pdev, priv);
> > > > +
> > > > + regs = devm_platform_ioremap_resource(pdev, 0);
> > > > + if (IS_ERR(regs))
> > > > + return dev_err_probe(dev, PTR_ERR(regs),
> > > > + "devm_platform_ioremap_resource failed\n");
> > > > +
> > > > + priv->regmap = devm_regmap_init_mmio(dev, regs,
> > > > + &ls2x_rtc_regmap_config);
> > > > + if (IS_ERR(priv->regmap))
> > > > + return dev_err_probe(dev, PTR_ERR(priv->regmap),
> > > > + "devm_regmap_init_mmio failed\n");
> > > > +
> > > > + priv->rtcdev = devm_rtc_allocate_device(dev);
> > > > + if (IS_ERR(priv->rtcdev))
> > > > + return dev_err_probe(dev, PTR_ERR(priv->rtcdev),
> > > > + "devm_rtc_allocate_device failed\n");
> > > > +
> > > > + /* Due to hardware erratum, all years multiple of 4 are considered
> > > > + * leap year, so only years 2000 through 2099 are usable.
> > > > + *
> > > > + * Previous out-of-tree versions of this driver wrote tm_year directly
> > > > + * into the year register, so epoch 2000 must be used to preserve
> > > > + * semantics on shipped systems.
> > > > + */
> > > > + priv->rtcdev->range_min = RTC_TIMESTAMP_BEGIN_2000;
> > > > + priv->rtcdev->range_max = RTC_TIMESTAMP_END_2099;
> > > > + priv->rtcdev->ops = &ls2x_rtc_ops;
> > > > + priv->acpi_base = regs - PM_RTC_OFFSET;
> > > > + spin_lock_init(&priv->rtc_reglock);
> > > > + clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, priv->rtcdev->features);
> > >
> > > Why?
> >
> > If you don't clear UIE, on the Loongson-2k, the shutdown will turn
> > into a reboot after setting up the wakealarm.
> >
>
> Why? I don't get how UIE can have this effect.
>
In fact, we need to clear the RTC wakeup interrupt manually.
I will fix this in the next release and remove clear_bit().
> > >
> > > > +
> > > > +#ifdef CONFIG_ACPI
> > > > + if (!acpi_disabled)
> > > > + acpi_install_fixed_event_handler(ACPI_EVENT_RTC,
> > > > + ls2x_acpi_fix_handler, priv);
> > > > +#endif
> > > > +
> > > > + ret = ls2x_enable_rtc(priv);
> > > > + if (ret < 0)
> > > > + return dev_err_probe(dev, ret, "ls2x_enable_rtc failed\n");
> > >
> > > This should not be done in probe but on the first set_time. This then
> > > allows you to know whether the time has been set and is valid in
> > > read_time. Please add the check.
> >
> > Actually I don't quite understand the point you are making, the
> > function just enables the TOY counter, would there be any particular
> > problem with calling it in the probe function?
> >
>
> Yes, you are losing an important bit of information which is that if
> TOY_ENABLE is not set, then you know the time has not been set. Else,
> you allow reading an known invalid time.
>
I will enable the RTC in set_time() and check in read_time() that it
has been enabled
Thanks.
Binbin
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
