[PATCH v2 01/10] Documentation: Add device tree binding for Goldfish RTC driver

[PATCH v2 01/10] Documentation: Add device tree binding for Goldfish RTC driver

[Aleksandar Markovic]

From: Aleksandar Markovic <aleksandar.markovic@imgtec.com>

Add documentation for DT binding of Goldfish RTC driver. The compatible
string used by OS for binding the driver is "google,goldfish-rtc".

Signed-off-by: Miodrag Dinic <miodrag.dinic@imgtec.com>
Signed-off-by: Goran Ferenc <goran.ferenc@imgtec.com>
Signed-off-by: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
---
 .../devicetree/bindings/rtc/google,goldfish-rtc.txt     | 17 +++++++++++++++++
 MAINTAINERS                                             |  5 +++++
 2 files changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/google,goldfish-rtc.txt

diff --git a/Documentation/devicetree/bindings/rtc/google,goldfish-rtc.txt b/Documentation/devicetree/bindings/rtc/google,goldfish-rtc.txt
new file mode 100644
index 0000000..634312d
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/google,goldfish-rtc.txt
@@ -0,0 +1,17 @@
+Android Goldfish RTC
+
+Android Goldfish RTC device used by Android emulator.
+
+Required properties:
+
+- compatible : should contain "google,goldfish-rtc"
+- reg        : <registers mapping>
+- interrupts : <interrupt mapping>
+
+Example:
+
+	goldfish_timer@9020000 {
+		compatible = "google,goldfish-rtc";
+		reg = <0x9020000 0x1000>;
+		interrupts = <0x3>;
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index 09b5ab6..519cdef 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -841,6 +841,11 @@ S:	Supported
 F:	drivers/android/
 F:	drivers/staging/android/
 
+ANDROID GOLDFISH RTC DRIVER
+M:	Miodrag Dinic <miodrag.dinic@imgtec.com>
+S:	Supported
+F:	Documentation/devicetree/bindings/rtc/google,goldfish-rtc.txt
+
 ANDROID ION DRIVER
 M:	Laura Abbott <labbott@redhat.com>
 M:	Sumit Semwal <sumit.semwal@linaro.org>
-- 
2.7.4

[PATCH v2 02/10] MIPS: ranchu: Add Goldfish RTC driver

[Aleksandar Markovic]

From: Aleksandar Markovic <aleksandar.markovic@imgtec.com>

Add device driver for a virtual Goldfish RTC clock.

The driver can be built only if CONFIG_MIPS and CONFIG_GOLDFISH are
set. The compatible string used by OS for binding the driver is
defined as "google,goldfish-rtc".

Signed-off-by: Miodrag Dinic <miodrag.dinic@imgtec.com>
Signed-off-by: Goran Ferenc <goran.ferenc@imgtec.com>
Signed-off-by: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
---
 MAINTAINERS                |   1 +
 drivers/rtc/Kconfig        |   6 ++
 drivers/rtc/Makefile       |   1 +
 drivers/rtc/rtc-goldfish.c | 136 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 144 insertions(+)
 create mode 100644 drivers/rtc/rtc-goldfish.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 519cdef..26a1267 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -845,6 +845,7 @@ ANDROID GOLDFISH RTC DRIVER
 M:	Miodrag Dinic <miodrag.dinic@imgtec.com>
 S:	Supported
 F:	Documentation/devicetree/bindings/rtc/google,goldfish-rtc.txt
+F:	drivers/rtc/rtc-goldfish.c
 
 ANDROID ION DRIVER
 M:	Laura Abbott <labbott@redhat.com>
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 8d3b957..510c5b7 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1753,5 +1753,11 @@ config RTC_DRV_HID_SENSOR_TIME
 	  If this driver is compiled as a module, it will be named
 	  rtc-hid-sensor-time.
 
+config RTC_DRV_GOLDFISH
+	tristate "Goldfish Real Time Clock"
+	depends on MIPS
+	depends on GOLDFISH
+	help
+	  Say yes here to build support for the Goldfish RTC.
 
 endif # RTC_CLASS
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 13857d2..dfc38f5 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -168,3 +168,4 @@ obj-$(CONFIG_RTC_DRV_WM8350)	+= rtc-wm8350.o
 obj-$(CONFIG_RTC_DRV_X1205)	+= rtc-x1205.o
 obj-$(CONFIG_RTC_DRV_XGENE)	+= rtc-xgene.o
 obj-$(CONFIG_RTC_DRV_ZYNQMP)	+= rtc-zynqmp.o
+obj-$(CONFIG_RTC_DRV_GOLDFISH)	+= rtc-goldfish.o
diff --git a/drivers/rtc/rtc-goldfish.c b/drivers/rtc/rtc-goldfish.c
new file mode 100644
index 0000000..e206a98
--- /dev/null
+++ b/drivers/rtc/rtc-goldfish.c
@@ -0,0 +1,136 @@
+/* drivers/rtc/rtc-goldfish.c
+ *
+ * Copyright (C) 2007 Google, Inc.
+ * Copyright (C) 2017 Imagination Technologies Ltd.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/rtc.h>
+
+#define TIMER_TIME_LOW         0x00	/* get low bits of current time  */
+					/*   and update TIMER_TIME_HIGH  */
+#define TIMER_TIME_HIGH        0x04	/* get high bits of time at last */
+					/*   TIMER_TIME_LOW read         */
+#define TIMER_ALARM_LOW        0x08	/* set low bits of alarm and     */
+					/*   activate it                 */
+#define TIMER_ALARM_HIGH       0x0c	/* set high bits of next alarm   */
+#define TIMER_CLEAR_INTERRUPT  0x10
+#define TIMER_CLEAR_ALARM      0x14
+
+struct goldfish_rtc {
+	void __iomem *base;
+	u32 irq;
+	struct rtc_device *rtc;
+};
+
+static irqreturn_t goldfish_rtc_interrupt(int irq, void *dev_id)
+{
+	struct goldfish_rtc	*qrtc = dev_id;
+	unsigned long		events = 0;
+	void __iomem *base = qrtc->base;
+
+	writel(1, base + TIMER_CLEAR_INTERRUPT);
+	events = RTC_IRQF | RTC_AF;
+
+	rtc_update_irq(qrtc->rtc, 1, events);
+
+	return IRQ_HANDLED;
+}
+
+static int goldfish_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	u64 time;
+	u64 time_low;
+	u64 time_high;
+	u64 time_high_prev;
+
+	struct goldfish_rtc *qrtc =
+			platform_get_drvdata(to_platform_device(dev));
+	void __iomem *base = qrtc->base;
+
+	time_high = readl(base + TIMER_TIME_HIGH);
+	do {
+		time_high_prev = time_high;
+		time_low = readl(base + TIMER_TIME_LOW);
+		time_high = readl(base + TIMER_TIME_HIGH);
+	} while (time_high != time_high_prev);
+	time = (time_high << 32) | time_low;
+
+	do_div(time, NSEC_PER_SEC);
+
+	rtc_time_to_tm(time, tm);
+
+	return 0;
+}
+
+static const struct rtc_class_ops goldfish_rtc_ops = {
+	.read_time	= goldfish_rtc_read_time,
+};
+
+static int goldfish_rtc_probe(struct platform_device *pdev)
+{
+	struct resource *r;
+	struct goldfish_rtc *qrtc;
+	unsigned long rtc_dev_len;
+	unsigned long rtc_dev_addr;
+	int err;
+
+	qrtc = devm_kzalloc(&pdev->dev, sizeof(*qrtc), GFP_KERNEL);
+	if (qrtc == NULL)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, qrtc);
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (r == NULL)
+		return -ENODEV;
+
+	rtc_dev_addr = r->start;
+	rtc_dev_len = resource_size(r);
+	qrtc->base = devm_ioremap(&pdev->dev, rtc_dev_addr, rtc_dev_len);
+	if (IS_ERR(qrtc->base))
+		return -ENODEV;
+
+	qrtc->irq = platform_get_irq(pdev, 0);
+	if (qrtc->irq < 0)
+		return -ENODEV;
+
+	qrtc->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
+					&goldfish_rtc_ops, THIS_MODULE);
+	if (IS_ERR(qrtc->rtc))
+		return PTR_ERR(qrtc->rtc);
+
+	err = devm_request_irq(&pdev->dev, qrtc->irq, goldfish_rtc_interrupt,
+		0, pdev->name, qrtc);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static const struct of_device_id goldfish_rtc_of_match[] = {
+	{ .compatible = "google,goldfish-rtc", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, goldfish_rtc_of_match);
+
+static struct platform_driver goldfish_rtc = {
+	.probe = goldfish_rtc_probe,
+	.driver = {
+		.name = "goldfish_rtc",
+		.of_match_table = goldfish_rtc_of_match,
+	}
+};
+
+module_platform_driver(goldfish_rtc);
-- 
2.7.4

Re: [PATCH v2 02/10] MIPS: ranchu: Add Goldfish RTC driver

[Alexandre Belloni]

Hi,

The subject doesn't fit the subsystem style, this needs to be changed.

On 28/06/2017 at 17:46:55 +0200, Aleksandar Markovic wrote:
> From: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
> 
> Add device driver for a virtual Goldfish RTC clock.
> 
> The driver can be built only if CONFIG_MIPS and CONFIG_GOLDFISH are
> set. The compatible string used by OS for binding the driver is
> defined as "google,goldfish-rtc".
> 

Is it really MIPS specific? I would expect the same driver to work on
the ARM based emulator too.

> +config RTC_DRV_GOLDFISH
> +	tristate "Goldfish Real Time Clock"
> +	depends on MIPS
> +	depends on GOLDFISH

This should be made buildable with COMPILE_TEST to extend coverage.

> +	help
> +	  Say yes here to build support for the Goldfish RTC.

Please, don't expect anybody to actually know what is goldfish can you
add a sentence or two?

> +static irqreturn_t goldfish_rtc_interrupt(int irq, void *dev_id)
> +{
> +	struct goldfish_rtc	*qrtc = dev_id;
> +	unsigned long		events = 0;
> +	void __iomem *base = qrtc->base;
> +
> +	writel(1, base + TIMER_CLEAR_INTERRUPT);
> +	events = RTC_IRQF | RTC_AF;
> +
> +	rtc_update_irq(qrtc->rtc, 1, events);

I'd say that events is not needed you can pass the flags directly to
rtc_update_irq

> +static int goldfish_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	u64 time;
> +	u64 time_low;
> +	u64 time_high;
> +	u64 time_high_prev;
> +
> +	struct goldfish_rtc *qrtc =
> +			platform_get_drvdata(to_platform_device(dev));
> +	void __iomem *base = qrtc->base;
> +
> +	time_high = readl(base + TIMER_TIME_HIGH);
> +	do {
> +		time_high_prev = time_high;
> +		time_low = readl(base + TIMER_TIME_LOW);
> +		time_high = readl(base + TIMER_TIME_HIGH);
> +	} while (time_high != time_high_prev);

I'm not sure why you need that loop as the comments for TIMER_TIME_LOW
and TIMER_TIME_HIGH indicate that TIMER_TIME_HIGH is latched when
TIMER_TIME_LOW is read. Note that the original driver from google
doesn't do that.

> +	time = (time_high << 32) | time_low;
> +
> +	do_div(time, NSEC_PER_SEC);
> +
> +	rtc_time_to_tm(time, tm);
> +
> +	return 0;
> +}
> +
> +static const struct rtc_class_ops goldfish_rtc_ops = {
> +	.read_time	= goldfish_rtc_read_time,
> +};
> +
> +static int goldfish_rtc_probe(struct platform_device *pdev)
> +{
> +	struct resource *r;
> +	struct goldfish_rtc *qrtc;
> +	unsigned long rtc_dev_len;
> +	unsigned long rtc_dev_addr;
> +	int err;
> +
> +	qrtc = devm_kzalloc(&pdev->dev, sizeof(*qrtc), GFP_KERNEL);
> +	if (qrtc == NULL)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, qrtc);
> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (r == NULL)
> +		return -ENODEV;
> +
> +	rtc_dev_addr = r->start;
> +	rtc_dev_len = resource_size(r);
> +	qrtc->base = devm_ioremap(&pdev->dev, rtc_dev_addr, rtc_dev_len);

devm_ioremap_resource ?

> +	if (IS_ERR(qrtc->base))
> +		return -ENODEV;
> +
> +	qrtc->irq = platform_get_irq(pdev, 0);
> +	if (qrtc->irq < 0)
> +		return -ENODEV;
> +

Is the irq so important that you have to fail here even if the driver
doesn't support any alarm?

> +	qrtc->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
> +					&goldfish_rtc_ops, THIS_MODULE);
> +	if (IS_ERR(qrtc->rtc))
> +		return PTR_ERR(qrtc->rtc);
> +
> +	err = devm_request_irq(&pdev->dev, qrtc->irq, goldfish_rtc_interrupt,
> +		0, pdev->name, qrtc);
> +	if (err)
> +		return err;

Ditto.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

RE: [PATCH v2 02/10] MIPS: ranchu: Add Goldfish RTC driver

[Miodrag Dinic]

cc-ing Jin Quian, Bo Hu & Lingfeng Yang from Google

Hi Alexandre,

thank you for your comments, answers are inline:

> 
> On 28/06/2017 at 17:46:55 +0200, Aleksandar Markovic wrote:
> > From: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
> > 
> > Add device driver for a virtual Goldfish RTC clock.
> > 
> > The driver can be built only if CONFIG_MIPS and CONFIG_GOLDFISH are
> > set. The compatible string used by OS for binding the driver is
> > defined as "google,goldfish-rtc".
> > 
> 
> Is it really MIPS specific? I would expect the same driver to work on
> the ARM based emulator too.

This driver can be made to work for ARM/Intel emulator but it is currently
used only by MIPS emulator, so I would prefer to keep it guarded with "MIPS".
If ARM or Intel decide to use this driver for their emulators it can be easily
enabled.

> > +config RTC_DRV_GOLDFISH
> > +     tristate "Goldfish Real Time Clock"
> > +     depends on MIPS
> > +     depends on GOLDFISH
> 
> This should be made buildable with COMPILE_TEST to extend coverage.

It will be included in the next version.

> > +     help
> > +       Say yes here to build support for the Goldfish RTC.
> 
> Please, don't expect anybody to actually know what is goldfish can you
> add a sentence or two?

It will be better documented in the next version. Thank you.

> > +static irqreturn_t goldfish_rtc_interrupt(int irq, void *dev_id)
> > +{
> > +     struct goldfish_rtc     *qrtc = dev_id;
> > +     unsigned long           events = 0;
> > +     void __iomem *base = qrtc->base;
> > +
> > +     writel(1, base + TIMER_CLEAR_INTERRUPT);
> > +     events = RTC_IRQF | RTC_AF;
> > +
> > +     rtc_update_irq(qrtc->rtc, 1, events);
> 
> I'd say that events is not needed you can pass the flags directly to
> rtc_update_irq

It will be corrected in the next version.

> > +static int goldfish_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +     u64 time;
> > +     u64 time_low;
> > +     u64 time_high;
> > +     u64 time_high_prev;
> > +
> > +     struct goldfish_rtc *qrtc =
> > +                     platform_get_drvdata(to_platform_device(dev));
> > +     void __iomem *base = qrtc->base;
> > +
> > +     time_high = readl(base + TIMER_TIME_HIGH);
> > +     do {
> > +             time_high_prev = time_high;
> > +             time_low = readl(base + TIMER_TIME_LOW);
> > +             time_high = readl(base + TIMER_TIME_HIGH);
> > +     } while (time_high != time_high_prev);
> 
> I'm not sure why you need that loop as the comments for TIMER_TIME_LOW
> and TIMER_TIME_HIGH indicate that TIMER_TIME_HIGH is latched when
> TIMER_TIME_LOW is read. Note that the original driver from google
> doesn't do that.

This is the way how other HW drivers are doing it, so we used this
approach to make it more in-line with other HW, and it also does not
make any assumptions regarding TIMER_TIME_HIGH is latched or not.
This is the relevant part of code on the RTC device side which emulates these reads:

static uint64_t goldfish_timer_read(void *opaque, hwaddr offset, unsigned size)
{
    struct timer_state *s = (struct timer_state *)opaque;
    switch(offset) {
        case TIMER_TIME_LOW:
            s->now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
            return s->now_ns;
        case TIMER_TIME_HIGH:
            return s->now_ns >> 32;
        default:
            cpu_abort(current_cpu,
                      "goldfish_timer_read: Bad offset %" HWADDR_PRIx "\n",
                      offset);
            return 0;
    }
}

So theoretically speaking, we could imagine the situation that the kernel pre-empts after the
first TIMER_TIME_LOW read and another request for reading the time gets processed, so
the previous call would end-up having stale TIMER_TIME_LOW value.
This is however very unlikely to happen, but one extra read in the loop doesn't hurt and
actually makes the code less prone to error.

> > +     time = (time_high << 32) | time_low;
> > +
> > +     do_div(time, NSEC_PER_SEC);
> > +
> > +     rtc_time_to_tm(time, tm);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct rtc_class_ops goldfish_rtc_ops = {
> > +     .read_time      = goldfish_rtc_read_time,
> > +};
> > +
> > +static int goldfish_rtc_probe(struct platform_device *pdev)
> > +{
> > +     struct resource *r;
> > +     struct goldfish_rtc *qrtc;
> > +     unsigned long rtc_dev_len;
> > +     unsigned long rtc_dev_addr;
> > +     int err;
> > +
> > +     qrtc = devm_kzalloc(&pdev->dev, sizeof(*qrtc), GFP_KERNEL);
> > +     if (qrtc == NULL)
> > +             return -ENOMEM;
> > +
> > +     platform_set_drvdata(pdev, qrtc);
> > +
> > +     r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +     if (r == NULL)
> > +             return -ENODEV;
> > +
> > +     rtc_dev_addr = r->start;
> > +     rtc_dev_len = resource_size(r);
> > +     qrtc->base = devm_ioremap(&pdev->dev, rtc_dev_addr, rtc_dev_len);
> 
> devm_ioremap_resource ?

Thanks, it will be fixed in next version to use devm_ioremap_resource().

> > +     if (IS_ERR(qrtc->base))
> > +             return -ENODEV;
> > +
> > +     qrtc->irq = platform_get_irq(pdev, 0);
> > +     if (qrtc->irq < 0)
> > +             return -ENODEV;
> > +
> 
> Is the irq so important that you have to fail here even if the driver
> doesn't support any alarm?

Well currently it does not support alarm features, but we are considering
to implement it in some other iteration. Maybe we will introduce it in the next version
if not we will remove the IRQ handling. Thanks.

> > +     qrtc->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
> > +                                     &goldfish_rtc_ops, THIS_MODULE);
> > +     if (IS_ERR(qrtc->rtc))
> > +             return PTR_ERR(qrtc->rtc);
> > +
> > +     err = devm_request_irq(&pdev->dev, qrtc->irq, goldfish_rtc_interrupt,
> > +             0, pdev->name, qrtc);
> > +     if (err)
> > +             return err;
> 
> Ditto.

Kind regards,
Miodrag

________________________________________
From: Alexandre Belloni [alexandre.belloni@free-electrons.com]
Sent: Wednesday, July 05, 2017 11:56 PM
To: Aleksandar Markovic
Cc: linux-mips@linux-mips.org; Aleksandar Markovic; Miodrag Dinic; Goran Ferenc; Alessandro Zummo; David S. Miller; Douglas Leung; Greg Kroah-Hartman; James Hogan; linux-kernel@vger.kernel.org; linux-rtc@vger.kernel.org; Martin K. Petersen; Mauro Carvalho Chehab; Paul Burton; Petar Jovanovic; Raghu Gandham
Subject: Re: [PATCH v2 02/10] MIPS: ranchu: Add Goldfish RTC driver

Hi,

The subject doesn't fit the subsystem style, this needs to be changed.

On 28/06/2017 at 17:46:55 +0200, Aleksandar Markovic wrote:
> From: Aleksandar Markovic <aleksandar.markovic@imgtec.com>
>
> Add device driver for a virtual Goldfish RTC clock.
>
> The driver can be built only if CONFIG_MIPS and CONFIG_GOLDFISH are
> set. The compatible string used by OS for binding the driver is
> defined as "google,goldfish-rtc".
>

Is it really MIPS specific? I would expect the same driver to work on
the ARM based emulator too.

> +config RTC_DRV_GOLDFISH
> +     tristate "Goldfish Real Time Clock"
> +     depends on MIPS
> +     depends on GOLDFISH

This should be made buildable with COMPILE_TEST to extend coverage.

> +     help
> +       Say yes here to build support for the Goldfish RTC.

Please, don't expect anybody to actually know what is goldfish can you
add a sentence or two?

> +static irqreturn_t goldfish_rtc_interrupt(int irq, void *dev_id)
> +{
> +     struct goldfish_rtc     *qrtc = dev_id;
> +     unsigned long           events = 0;
> +     void __iomem *base = qrtc->base;
> +
> +     writel(1, base + TIMER_CLEAR_INTERRUPT);
> +     events = RTC_IRQF | RTC_AF;
> +
> +     rtc_update_irq(qrtc->rtc, 1, events);

I'd say that events is not needed you can pass the flags directly to
rtc_update_irq

> +static int goldfish_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +     u64 time;
> +     u64 time_low;
> +     u64 time_high;
> +     u64 time_high_prev;
> +
> +     struct goldfish_rtc *qrtc =
> +                     platform_get_drvdata(to_platform_device(dev));
> +     void __iomem *base = qrtc->base;
> +
> +     time_high = readl(base + TIMER_TIME_HIGH);
> +     do {
> +             time_high_prev = time_high;
> +             time_low = readl(base + TIMER_TIME_LOW);
> +             time_high = readl(base + TIMER_TIME_HIGH);
> +     } while (time_high != time_high_prev);

I'm not sure why you need that loop as the comments for TIMER_TIME_LOW
and TIMER_TIME_HIGH indicate that TIMER_TIME_HIGH is latched when
TIMER_TIME_LOW is read. Note that the original driver from google
doesn't do that.

> +     time = (time_high << 32) | time_low;
> +
> +     do_div(time, NSEC_PER_SEC);
> +
> +     rtc_time_to_tm(time, tm);
> +
> +     return 0;
> +}
> +
> +static const struct rtc_class_ops goldfish_rtc_ops = {
> +     .read_time      = goldfish_rtc_read_time,
> +};
> +
> +static int goldfish_rtc_probe(struct platform_device *pdev)
> +{
> +     struct resource *r;
> +     struct goldfish_rtc *qrtc;
> +     unsigned long rtc_dev_len;
> +     unsigned long rtc_dev_addr;
> +     int err;
> +
> +     qrtc = devm_kzalloc(&pdev->dev, sizeof(*qrtc), GFP_KERNEL);
> +     if (qrtc == NULL)
> +             return -ENOMEM;
> +
> +     platform_set_drvdata(pdev, qrtc);
> +
> +     r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +     if (r == NULL)
> +             return -ENODEV;
> +
> +     rtc_dev_addr = r->start;
> +     rtc_dev_len = resource_size(r);
> +     qrtc->base = devm_ioremap(&pdev->dev, rtc_dev_addr, rtc_dev_len);

devm_ioremap_resource ?

> +     if (IS_ERR(qrtc->base))
> +             return -ENODEV;
> +
> +     qrtc->irq = platform_get_irq(pdev, 0);
> +     if (qrtc->irq < 0)
> +             return -ENODEV;
> +

Is the irq so important that you have to fail here even if the driver
doesn't support any alarm?

> +     qrtc->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
> +                                     &goldfish_rtc_ops, THIS_MODULE);
> +     if (IS_ERR(qrtc->rtc))
> +             return PTR_ERR(qrtc->rtc);
> +
> +     err = devm_request_irq(&pdev->dev, qrtc->irq, goldfish_rtc_interrupt,
> +             0, pdev->name, qrtc);
> +     if (err)
> +             return err;

Ditto.


--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 02/10] MIPS: ranchu: Add Goldfish RTC driver

[Alexandre Belloni]

On 06/07/2017 at 13:25:09 +0000, Miodrag Dinic wrote:
> > > +static int goldfish_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > > +{
> > > +     u64 time;
> > > +     u64 time_low;
> > > +     u64 time_high;
> > > +     u64 time_high_prev;
> > > +
> > > +     struct goldfish_rtc *qrtc =
> > > +                     platform_get_drvdata(to_platform_device(dev));
> > > +     void __iomem *base = qrtc->base;
> > > +
> > > +     time_high = readl(base + TIMER_TIME_HIGH);
> > > +     do {
> > > +             time_high_prev = time_high;
> > > +             time_low = readl(base + TIMER_TIME_LOW);
> > > +             time_high = readl(base + TIMER_TIME_HIGH);
> > > +     } while (time_high != time_high_prev);
> > 
> > I'm not sure why you need that loop as the comments for TIMER_TIME_LOW
> > and TIMER_TIME_HIGH indicate that TIMER_TIME_HIGH is latched when
> > TIMER_TIME_LOW is read. Note that the original driver from google
> > doesn't do that.
> 
> This is the way how other HW drivers are doing it, so we used this
> approach to make it more in-line with other HW, and it also does not
> make any assumptions regarding TIMER_TIME_HIGH is latched or not.
> This is the relevant part of code on the RTC device side which emulates these reads:
> 
> static uint64_t goldfish_timer_read(void *opaque, hwaddr offset, unsigned size)
> {
>     struct timer_state *s = (struct timer_state *)opaque;
>     switch(offset) {
>         case TIMER_TIME_LOW:
>             s->now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>             return s->now_ns;
>         case TIMER_TIME_HIGH:
>             return s->now_ns >> 32;
>         default:
>             cpu_abort(current_cpu,
>                       "goldfish_timer_read: Bad offset %" HWADDR_PRIx "\n",
>                       offset);
>             return 0;
>     }
> }
> 
> So theoretically speaking, we could imagine the situation that the kernel pre-empts after the
> first TIMER_TIME_LOW read and another request for reading the time gets processed, so
> the previous call would end-up having stale TIMER_TIME_LOW value.
> This is however very unlikely to happen, but one extra read in the loop doesn't hurt and
> actually makes the code less prone to error.
> 

Every call to the RTC callbacks are protected by a mutex so this will
never happen.

Most of the RTCs are actually latching the time on the first register
read and don't require specific handling. I'd prefer to keep the driver
simple.


> > > +     qrtc->irq = platform_get_irq(pdev, 0);
> > > +     if (qrtc->irq < 0)
> > > +             return -ENODEV;
> > > +
> > 
> > Is the irq so important that you have to fail here even if the driver
> > doesn't support any alarm?
> 
> Well currently it does not support alarm features, but we are considering
> to implement it in some other iteration. Maybe we will introduce it in the next version
> if not we will remove the IRQ handling. Thanks.
> 

I'd say that you should probably leave out the whole IRQ handling until
you really handle alarms in the driver or do you have a way to generate
alarms (and so interrupts) without using the driver?


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com