Message-ID: <b0ae635f-461f-be80-ebff-a548c9dd66af@ti.com>
Date:   Fri, 15 Apr 2022 13:33:14 +0530
From:   Vignesh Raghavendra <vigneshr@...com>
To:     Nishanth Menon <nm@...com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Alessandro Zummo <a.zummo@...ertech.it>
CC:     <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-rtc@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 2/2] rtc: Introduce ti-k3-rtc

Hi,

On 12/04/22 1:01 pm, Nishanth Menon wrote:
> +/**
> + * k3rtc_fence  - Ensure a register sync took place between the two domains
> + * @priv:      pointer to priv data
> + *
> + * Return: 0 if the sync took place, else returns -ETIMEDOUT
> + */
> +static int k3rtc_fence(struct ti_k3_rtc *priv)
> +{
> +	u32 timeout = priv->sync_timeout_us;
> +	u32 mask = K3RTC_RD_PEND_BIT | K3RTC_WR_PEND_BIT;
> +	u32 val = 0;
> +
> +	while (timeout--) {
> +		val = k3rtc_readl(priv, REG_K3RTC_SYNCPEND);
> +		if (!(val & mask))
> +			return 0;
> +		usleep_range(1, 2);
> +	}

readl_poll_timeout() ?

> +
> +	pr_err("RTC Fence timeout: 0x%08x\n", val);

Can we use dev_err()?  Provides better indication of the driver throwing
error.

> +	return -ETIMEDOUT;
> +}
> +
> +static inline int k3rtc_check_unlocked(struct ti_k3_rtc *priv)
> +{
> +	u32 val;
> +
> +	val = k3rtc_readl(priv, REG_K3RTC_GENERAL_CTL);
> +	return (val & K3RTC_UNLOCK_BIT) ? 0 : 1;
> +}
> +
> +static int k3rtc_unlock_rtc(struct ti_k3_rtc *priv)
> +{
> +	u32 timeout = priv->sync_timeout_us;
> +	int ret;
> +
> +	ret = k3rtc_check_unlocked(priv);
> +	if (!ret)
> +		return ret;
> +
> +	k3rtc_writel(priv, REG_K3RTC_KICK0, K3RTC_KICK0_UNLOCK_VALUE);
> +	k3rtc_writel(priv, REG_K3RTC_KICK1, K3RTC_KICK1_UNLOCK_VALUE);
> +
> +	/* Skip fence since we are going to check the unlock bit as fence */
> +	while (timeout--) {
> +		ret = k3rtc_check_unlocked(priv);
> +		if (!ret)
> +			return ret;
> +		usleep_range(1, 2);
> +	}

readl_poll_timeout() ?

> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int k3rtc_configure(struct device *dev)
> +{
> +	int ret;
> +	u32 ctl;
> +	struct ti_k3_rtc *priv = dev_get_drvdata(dev);
> +
> +	/*
> +	 * HWBUG: The compare statemachine is broken if the RTC module
> +	 * is NOT unlocked in under one second of boot - which is pretty long
> +	 * time from the perspective of Linux driver (module load, u-boot
> +	 * shell all can take much longer than this.
> +	 *
> +	 * In such occurrence, it is assumed that the RTC module is un-usable
> +	 */
> +	if (priv->soc->unlock_irq_erratum) {
> +		ret = k3rtc_check_unlocked(priv);
> +		/* If there is an error OR if we are locked, return error */
> +		if (ret) {
> +			dev_err(dev, HW_ERR "Erratum i2327 unlock QUIRK! Cannot operate!!\n");
> +			return -EFAULT;
> +		}
> +	} else {
> +		/* May Need to explicitly unlock first time */
> +		ret = k3rtc_unlock_rtc(priv);
> +		if (ret) {
> +			dev_err(dev, "Failed to unlock(%d)!\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	/* Enable Shadow register sync on 32k clk boundary */
> +	ctl = k3rtc_readl(priv, REG_K3RTC_GENERAL_CTL);
> +	ctl |= K3RTC_O32K_OSC_DEP_EN_BIT;
> +	k3rtc_writel(priv, REG_K3RTC_GENERAL_CTL, ctl);
> +
> +	/*
> +	 * Wait at least 2 clk sync time before proceeding further programming.
> +	 * This ensures that the 32k based sync is active.
> +	 */
> +	usleep_range(priv->sync_timeout_us, priv->sync_timeout_us + 5);
> +
> +	/* We need to ensure fence here to make sure sync here */
> +	ret = k3rtc_fence(priv);
> +	if (ret) {
> +		dev_err(dev, "Failed fence osc_dep enable(%d) - is 32k clk working?!\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	/* Lets just make sure we get consistent time value */
> +	ctl &= ~K3RTC_CNT_FMODE_MASK;
> +	/*
> +	 * FMODE setting: Reading lower seconds will freeze value on higher
> +	 * seconds. This also implies that we must *ALWAYS* read lower seconds
> +	 * prior to reading higher seconds
> +	 */
> +	ctl |= K3RTC_CNT_FMODE_S_CNT_VALUE;
> +	k3rtc_writel(priv, REG_K3RTC_GENERAL_CTL, ctl);
> +
> +	/* Clear any spurious IRQ sources if any */
> +	k3rtc_writel(priv, REG_K3RTC_IRQSTATUS_SYS,
> +		     K3RTC_EVENT_ON_OFF_BIT | K3RTC_EVENT_OFF_ON_BIT);
> +	/* Disable all IRQs */
> +	k3rtc_writel(priv, REG_K3RTC_IRQENABLE_CLR_SYS,
> +		     K3RTC_EVENT_ON_OFF_BIT | K3RTC_EVENT_OFF_ON_BIT);
> +
> +	/* And.. Let us Sync the writes in */
> +	ret = k3rtc_fence(priv);
> +	if (ret) {
> +		dev_err(dev, "Failed to fence(%d)!\n", ret);
> +		return ret;

nit: this can be dropped as next statement will return error code anyway

> +	}
> +
> +	return ret;
> +}
> +

[...]

> +
> +static const struct ti_k3_rtc_soc_data ti_k3_am62_data = {
> +	.unlock_irq_erratum = true,
> +};
> +
> +static const struct of_device_id ti_k3_rtc_of_match_table[] = {
> +	{.compatible = "ti,am62-rtc", .data = &ti_k3_am62_data},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, ti_k3_rtc_of_match_table);
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int ti_k3_rtc_suspend(struct device *dev)

 __maybe_unused preferred instead of #ifdef for better compile coverage
but upto you.

> +{
> +	struct ti_k3_rtc *priv = dev_get_drvdata(dev);
> +
> +	if (device_may_wakeup(dev))
> +		enable_irq_wake(priv->irq);
> +	return 0;
> +}
> +
> +static int ti_k3_rtc_resume(struct device *dev)
> +{
> +	struct ti_k3_rtc *priv = dev_get_drvdata(dev);
> +
> +	if (device_may_wakeup(dev))
> +		disable_irq_wake(priv->irq);
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(ti_k3_rtc_pm_ops, ti_k3_rtc_suspend, ti_k3_rtc_resume);
> +
> +static struct platform_driver ti_k3_rtc_driver = {
> +	.probe = ti_k3_rtc_probe,
> +	.driver = {
> +		   .name = "rtc-ti-k3",
> +		   .of_match_table = ti_k3_rtc_of_match_table,
> +		   .pm = &ti_k3_rtc_pm_ops,
> +		   },
Extra tab?

> +};
> +module_platform_driver(ti_k3_rtc_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("TI K3 RTC driver");
> +MODULE_AUTHOR("Nishanth Menon");
> +MODULE_ALIAS("platform:rtc-ti-k3");
> -- 2.31.1

Powered by blists - more mailing lists