Re: [PATCH 2/3] rtc: support for the Amlogic on-chip RTC

[Krzysztof Kozlowski <krzk@kernel.org>]

On 23/08/2024 11:19, Xianwei Zhao via B4 Relay wrote:
> From: Yiting Deng <yiting.deng@amlogic.com>
> 
> Support for the on-chip RTC found in some of Amlogic's SoCs such as the
> A113L2 and A113X2.
> 
> Signed-off-by: Yiting Deng <yiting.deng@amlogic.com>
> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
> ---
>  drivers/rtc/Kconfig       |  12 +
>  drivers/rtc/Makefile      |   1 +
>  drivers/rtc/rtc-amlogic.c | 589 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 602 insertions(+)
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 2a95b05982ad..0dd042701c3b 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -2043,4 +2043,16 @@ config RTC_DRV_SSD202D
>  	  This driver can also be built as a module, if so, the module
>  	  will be called "rtc-ssd20xd".
>  
> +config RTC_DRV_AMLOGIC
> +	tristate "Amlogic RTC"
> +	depends on ARCH_MESON || COMPILE_TEST

So the third driver? What is wrong with existing ones? And why this one
is named so differently?

> +	select REGMAP_MMIO
> +	default y
> +	help
> +	  If you say yes here you get support for the RTC block on the
> +	  Amlogic A113L2(A4) and A113X2(A5) SoCs.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called "rtc-amlogic".
> +
>  endif # RTC_CLASS
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 3004e372f25f..d4a56ddb4246 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_RTC_DRV_ABB5ZES3)	+= rtc-ab-b5ze-s3.o
>  obj-$(CONFIG_RTC_DRV_ABEOZ9)	+= rtc-ab-eoz9.o
>  obj-$(CONFIG_RTC_DRV_ABX80X)	+= rtc-abx80x.o
>  obj-$(CONFIG_RTC_DRV_AC100)	+= rtc-ac100.o
> +obj-$(CONFIG_RTC_DRV_AMLOGIC)	+= rtc-amlogic.o
>  obj-$(CONFIG_RTC_DRV_ARMADA38X)	+= rtc-armada38x.o
>  obj-$(CONFIG_RTC_DRV_AS3722)	+= rtc-as3722.o
>  obj-$(CONFIG_RTC_DRV_ASM9260)	+= rtc-asm9260.o


> +
> +static int aml_rtc_adjust_sec(struct device *dev, u32 match_counter,
> +			      int ops, int enable)
> +{
> +	struct aml_rtc_data *rtc = dev_get_drvdata(dev->parent);
> +	u32 reg_val;
> +
> +	if (!FIELD_FIT(RTC_MATCH_COUNTER, match_counter)) {
> +		pr_err("%s: invalid match_counter\n", __func__);

No, do not use pr_, but driver ones.


> +		return -EINVAL;
> +	}
> +
> +	reg_val = FIELD_PREP(RTC_SEC_ADJUST_CTRL, ops)
> +		  | FIELD_PREP(RTC_MATCH_COUNTER, match_counter)
> +		  | FIELD_PREP(RTC_ADJ_VALID, enable);
> +	/* Set sec_adjust_ctrl, match_counter and Valid adjust */
> +	regmap_write(rtc->map, RTC_SEC_ADJUST_REG, reg_val);
> +
> +	return 0;
> +}
> +
> +static int aml_rtc_set_calibration(struct device *dev, u32 calibration)
> +{
> +	int cal_ops, enable, match_counter;
> +	int ret;
> +
> +	match_counter = FIELD_GET(RTC_MATCH_COUNTER, calibration);
> +	cal_ops = FIELD_GET(RTC_SEC_ADJUST_CTRL, calibration);
> +	enable = FIELD_GET(RTC_ADJ_VALID, calibration);
> +
> +	ret = aml_rtc_adjust_sec(dev, match_counter, cal_ops, enable);
> +	dev_dbg(dev, "%s: Success to store RTC calibration attribute\n",
> +		__func__);
> +
> +	return ret;
> +}
> +
> +static int aml_rtc_get_calibration(struct device *dev, u32 *calibration)
> +{
> +	struct aml_rtc_data *rtc = dev_get_drvdata(dev->parent);
> +	u32 reg_val;
> +
> +	regmap_read(rtc->map, RTC_SEC_ADJUST_REG, &reg_val);
> +	*calibration = FIELD_GET(RTC_SEC_ADJUST_CTRL | RTC_MATCH_COUNTER, reg_val);
> +	/* BIT is only UL defined，and GENMASK has no type, its' donot used together */
> +	*calibration |= FIELD_PREP(RTC_ADJ_VALID, FIELD_GET(RTC_ADJ_VALID, reg_val));
> +
> +	return 0;
> +}
> +
> +/**
> + * The calibration value transferred from buf takes bit[18:0] to represent
> + * match_counter, while takes bit[20:19] to represent sec_adjust_ctrl, bit[23]
> + * to represent adj_valid. enable/disable sec_adjust_ctrl and match_counter.
> + * @buf: Separate buf to match_counter, sec_adjust_ctrl and adj_valid
> + *	 match_counter: bit[18:0], value is 0 ~ 0x7fff
> + *	 sec_adjust_ctrl: bit[20:19], value is 0 ~ 2. 3 to insert a second once every
> + *	 match_counter+1 seconds, 2 to swallow a second once every match_counter+1 seconds
> + *	 0 or 1 to no adjustment
> + *	 adj_valid: bit[23], value is 0 or 1, 0 to disable sec_adjust_ctrl and
> + *	 match_counter, and 1 to enable them.

Messed kerneldoc. You have warning shere. Fix it.

> + */
> +static ssize_t rtc_calibration_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	int retval;
> +	int calibration = 0;
> +
> +	if (sscanf(buf, " %i ", &calibration) != 1) {
> +		dev_err(dev, "Failed to store RTC calibration attribute\n");

Where is the ABI documented?

> +		return -EINVAL;
> +	}
> +	retval = aml_rtc_set_calibration(dev, calibration);
> +
> +	return retval ? retval : count;
> +}
> +
> +static ssize_t rtc_calibration_show(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	int  retval = 0;
> +	u32  calibration = 0;
> +
> +	retval = aml_rtc_get_calibration(dev, &calibration);
> +	if (retval < 0) {
> +		dev_err(dev, "Failed to read RTC calibration attribute\n");
> +		sprintf(buf, "0\n");
> +		return retval;
> +	}
> +
> +	return sprintf(buf, "0x%x\n", calibration);
> +}
> +static DEVICE_ATTR_RW(rtc_calibration);

Document the ABI.

> +
> +static int rtc_set_div256_adjust(struct device *dev, u32 *value)
> +{
> +	struct aml_rtc_data *rtc = dev_get_drvdata(dev->parent);
> +	u32 div256_adj;
> +
> +	div256_adj = FIELD_PREP(RTC_DIV256_ADJ_DSR | RTC_DIV256_ADJ_VAL, *value);
> +	/*
> +	 * AO_RTC_SEC_ADJUST_REG bit 24 insert/remove(1/0) a div256 cycle,
> +	 * bit 25 valid/invalid(1/0) div256_adj_val
> +	 */
> +	regmap_write_bits(rtc->map, RTC_SEC_ADJUST_REG,
> +			  RTC_DIV256_ADJ_DSR | RTC_DIV256_ADJ_VAL, div256_adj);
> +	/* rtc need about 30ms to adjust its time after written */
> +	mdelay(30);
> +
> +	return 0;
> +}
> +
> +static int rtc_get_div256_adjust(struct device *dev, u32 *value)
> +{
> +	struct aml_rtc_data *rtc = dev_get_drvdata(dev->parent);
> +	u32 reg_val;
> +
> +	regmap_read(rtc->map, RTC_SEC_ADJUST_REG, &reg_val);
> +	*value = FIELD_GET(RTC_DIV256_ADJ_DSR | RTC_DIV256_ADJ_VAL, reg_val);
> +
> +	return 0;
> +}
> +
> +/**
> + * div256 adjust function is controlled using bit[24] and bit[25].
> + * transferred buf takes bit[0] to represent div256 adj val, bit[1]
> + * to represent div256 adj enable/disable. div256 cycle means that adjust
> + * 1/32768/256 once by written once, it's val is equal to 1/128s
> + * @buf: 3: enable div256 adjust and insert a div256 cycle
> + *	 2: enable div256 adjust and remove a div256 cycle
> + *	 1 or 0: disable div256 adjust

Again incorrect kerneldoc.

> + */
> +static ssize_t rtc_div256_adjust_store(struct device *dev,
> +				       struct device_attribute *attr,
> +				       const char *buf, size_t count)
> +{
> +	int retval;
> +	u32 value = 0;
> +
> +	if (sscanf(buf, " %i ", &value) != 1) {
> +		dev_err(dev, "Failed to store RTC div256 adjust attribute\n");
> +		return -EINVAL;
> +	}
> +	retval = rtc_set_div256_adjust(dev, &value);
> +
> +	return retval ? retval : count;
> +}
> +
> +static ssize_t rtc_div256_adjust_show(struct device *dev,
> +				      struct device_attribute *attr, char *buf)
> +{
> +	int retval = 0;
> +	u32 value = 0;
> +
> +	retval = rtc_get_div256_adjust(dev, &value);
> +	if (retval < 0) {
> +		dev_err(dev, "Failed to read RTC div256 adjust attribute\n");
> +		sprintf(buf, "0\n");
> +		return retval;
> +	}
> +
> +	return sprintf(buf, "0x%x\n", value);
> +}
> +static DEVICE_ATTR_RW(rtc_div256_adjust);
> +
> +static struct attribute *aml_rtc_attrs[] = {
> +	&dev_attr_rtc_calibration.attr,
> +	&dev_attr_rtc_div256_adjust.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group aml_rtc_sysfs_files = {
> +	.attrs	= aml_rtc_attrs,
> +};
> +
> +static void aml_rtc_init(struct device *dev, struct aml_rtc_data *rtc)
> +{
> +	u32 reg_val;
> +	u32 rtc_enable;
> +
> +	regmap_read(rtc->map, RTC_CTRL, &reg_val);
> +	rtc_enable = FIELD_GET(RTC_ENABLE, reg_val);
> +	if (!rtc_enable) {
> +		if (clk_get_rate(rtc->sclk) == OSC_24M) {
> +			/* select 24M oscillator */
> +			regmap_update_bits(rtc->map, RTC_CTRL, RTC_OSC_SEL, RTC_OSC_SEL);
> +
> +			/*
> +			 * Set RTC oscillator to freq_out to freq_in/((N0*M0+N1*M1)/(M0+M1))
> +			 * Enable clock_in gate of oscillator 24MHz
> +			 * Set N0 to 733, N1 to 732
> +			 */
> +			reg_val = FIELD_PREP(RTC_OSCIN_IN_EN, 1)
> +				  | FIELD_PREP(RTC_OSCIN_OUT_CFG, 1)
> +				  | FIELD_PREP(RTC_OSCIN_OUT_N0M0, RTC_OSCIN_OUT_32K_N0)
> +				  | FIELD_PREP(RTC_OSCIN_OUT_N1M1, RTC_OSCIN_OUT_32K_N1);
> +			regmap_write_bits(rtc->map, RTC_OSCIN_CTRL0, RTC_OSCIN_IN_EN
> +					  | RTC_OSCIN_OUT_CFG | RTC_OSCIN_OUT_N0M0
> +					  | RTC_OSCIN_OUT_N1M1, reg_val);
> +
> +			/* Set M0 to 2, M1 to 3, so freq_out = 32768 Hz*/
> +			reg_val = FIELD_PREP(RTC_OSCIN_OUT_N0M0, RTC_OSCIN_OUT_32K_M0)
> +				  | FIELD_PREP(RTC_OSCIN_OUT_N1M1, RTC_OSCIN_OUT_32K_M1);
> +			regmap_write_bits(rtc->map, RTC_OSCIN_CTRL1, RTC_OSCIN_OUT_N0M0
> +					  | RTC_OSCIN_OUT_N1M1, reg_val);
> +		} else {
> +			/* select 32K oscillator */
> +			regmap_write_bits(rtc->map, RTC_CTRL, RTC_OSC_SEL, 0);
> +		}
> +		/* Enable RTC */
> +		regmap_write_bits(rtc->map, RTC_CTRL, RTC_ENABLE, RTC_ENABLE);
> +		usleep_range(100, 200);
> +	}
> +	regmap_write_bits(rtc->map, RTC_INT_MASK,
> +			  RTC_ALRM0_IRQ_MSK, RTC_ALRM0_IRQ_MSK);
> +	regmap_write_bits(rtc->map, RTC_CTRL, RTC_ALRM0_EN, 0);
> +}
> +
> +static int aml_rtc_probe(struct platform_device *pdev)
> +{
> +	struct aml_rtc_data *rtc;
> +	void __iomem *base;
> +	struct clk *core_clk;
> +	int ret = 0;
> +
> +	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
> +	if (!rtc)
> +		return -ENOMEM;
> +
> +	rtc->config = of_device_get_match_data(&pdev->dev);
> +	if (!rtc->config)
> +		return -ENODEV;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base)) {
> +		dev_err(&pdev->dev, "resource ioremap failed\n");
> +		return PTR_ERR(base);
> +	}
> +
> +	rtc->map = devm_regmap_init_mmio(&pdev->dev, base, &aml_rtc_regmap_config);
> +	if (IS_ERR(rtc->map)) {
> +		dev_err(&pdev->dev, "regmap init failed\n");
> +		return PTR_ERR(rtc->map);
> +	}
> +
> +	rtc->irq = platform_get_irq(pdev, 0);
> +	if (rtc->irq < 0)
> +		return rtc->irq;
> +
> +	rtc->sclk = devm_clk_get(&pdev->dev, "rtc_osc");

Clock name should be: "osc"

> +	if (IS_ERR(rtc->sclk))
> +		return PTR_ERR(rtc->sclk);
> +	if (clk_get_rate(rtc->sclk) != OSC_32K && clk_get_rate(rtc->sclk) != OSC_24M) {
> +		dev_err(&pdev->dev, "Invalid clock configuration\n");
> +		return -EINVAL;
> +	}
> +
> +	core_clk = devm_clk_get(&pdev->dev, "rtc_sys_clk");

Clock name: "sys"

> +	if (IS_ERR(core_clk)) {
> +		dev_err(&pdev->dev, "failed to get rtc sys clk\n");

Syntax is return dev_err_probe.

> +		return PTR_ERR(core_clk);
> +	}
> +	clk_prepare_enable(core_clk);
> +
> +	aml_rtc_init(&pdev->dev, rtc);
> +
> +	device_init_wakeup(&pdev->dev, 1);
> +	platform_set_drvdata(pdev, rtc);
> +
> +	rtc->rtc_dev = devm_rtc_allocate_device(&pdev->dev);
> +	if (IS_ERR(rtc->rtc_dev))
> +		return PTR_ERR(rtc->rtc_dev);
> +
> +	ret = devm_request_irq(&pdev->dev, rtc->irq, aml_rtc_handler,
> +			       IRQF_ONESHOT, "aml-rtc alarm", rtc);
> +	if (ret) {
> +		dev_err(&pdev->dev, "IRQ%d request failed, ret = %d\n",
> +			rtc->irq, ret);

Your code is buggy. You leave with prepared clock.

Use devm_clk_get_enabled where applicable.

> +		return ret;
> +	}
> +
> +	rtc->rtc_dev->ops = &aml_rtc_ops;
> +	rtc->rtc_dev->range_min = 0;
> +	rtc->rtc_dev->range_max = U32_MAX;
> +
> +	ret = rtc_add_group(rtc->rtc_dev, &aml_rtc_sysfs_files);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to create sysfs group: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return devm_rtc_register_device(rtc->rtc_dev);
> +}
> +
> +static int aml_rtc_suspend(struct device *dev)
> +{
> +	struct aml_rtc_data *rtc = dev_get_drvdata(dev);
> +
> +	if (device_may_wakeup(dev))
> +		enable_irq_wake(rtc->irq);
> +
> +	return 0;
> +}
> +
> +static int aml_rtc_resume(struct device *dev)
> +{
> +	struct aml_rtc_data *rtc = dev_get_drvdata(dev);
> +
> +	if (device_may_wakeup(dev))
> +		disable_irq_wake(rtc->irq);
> +
> +	return 0;
> +}
> +

Where is the remove to cleanup?

> +static SIMPLE_DEV_PM_OPS(aml_rtc_pm_ops,
> +			 aml_rtc_suspend, aml_rtc_resume);
> +
> +static const struct aml_rtc_config a5_rtc_config = {
> +};
> +
> +static const struct aml_rtc_config a4_rtc_config = {
> +	.gray_stored = true,
> +};
> +
> +static const struct of_device_id aml_rtc_device_id[] = {
> +	{
> +		.compatible = "amlogic,a4-rtc",
> +		.data = &a4_rtc_config,
> +	},
> +	{
> +		.compatible = "amlogic,a5-rtc",
> +		.data = &a5_rtc_config,
> +	},
> +};
> +MODULE_DEVICE_TABLE(of, aml_rtc_device_id);
> +
> +static struct platform_driver aml_rtc_driver = {
> +	.probe = aml_rtc_probe,
> +	.driver = {
> +		.name = "aml-rtc",
> +		.of_match_table = of_match_ptr(aml_rtc_device_id),

Drop of_match_ptr. You have a warning here.

> +		.pm = &aml_rtc_pm_ops,
> +	},
Best regards,
Krzysztof


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic