Re: [PATCH] rtc: nintendo: Add a RTC driver for the GameCube, Wii and Wii U

[Emmanuel Gil Peyrot]

On Wed, Oct 27, 2021 at 06:45:54PM +0200, Alexandre Belloni wrote:
> Hello Emmanuel,
> 
> Thanks for the patch!

Hi, and thanks for your review!

> 
> On 15/10/2021 00:05:24+0200, Emmanuel Gil Peyrot wrote:
> > These three consoles share a device, the MX23L4005, which contains a
> > clock and 64 bytes of SRAM storage, and is exposed on the EXI bus
> > (similar to SPI) on channel 0, device 1.  This driver allows it to be
> > used as a Linux RTC device, where time can be read and set.
> > 
> > The hardware also exposes two timers, one which shuts down the console
> > and one which powers it on, but these aren’t supported currently.
> > 
> > On the Wii U, the counter bias is stored in a XML file, /config/rtc.xml,
> > encrypted in the SLC (eMMC storage), using a proprietary filesystem.  In
> > order to avoid having to implement all that, this driver assumes a
> > bootloader will parse this XML file and write the bias into the SRAM, at
> > the same location the other two consoles have it.
> > 
> > Signed-off-by: Emmanuel Gil Peyrot <linkmauve@xxxxxxxxxxxx>
> > ---
> >  drivers/rtc/Kconfig        |  10 ++
> >  drivers/rtc/Makefile       |   1 +
> >  drivers/rtc/rtc-nintendo.c | 305 +++++++++++++++++++++++++++++++++++++
> 
> I'm not convinced this is a good name, seeing that the switch will
> certainly not use this driver (neither is the snes mini).

Other subsystem maintainers have requested this to be changed, so I
reflected it here too.  For instance hid requested a Wii U-specific
driver to be merged with the Switch one.

Would rtc-gamecube be fine then?  So far I have only tested on Wii U,
but this driver is expected to support all three generations of
GameCube, Wii and Wii U.

> 
> >  3 files changed, 316 insertions(+)
> >  create mode 100644 drivers/rtc/rtc-nintendo.c
> > 
> > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> > index 914497abeef9..f2a15429e4b1 100644
> > --- a/drivers/rtc/Kconfig
> > +++ b/drivers/rtc/Kconfig
> > @@ -1214,6 +1214,16 @@ config RTC_DRV_V3020
> >  	  This driver can also be built as a module. If so, the module
> >  	  will be called rtc-v3020.
> >  
> > +config RTC_DRV_NINTENDO
> > +	tristate "Nintendo GameCube, Wii and Wii U RTC"
> > +	depends on GAMECUBE || WII || WIIU || COMPILE_TEST
> 
> CONFIG_WIIU doesn't seem to exist or am I missing something?

Indeed, we haven’t submitted the platform yet, only some of the self-
contained drivers.  I will remove it for now, and will add it again once
CONFIG_WIIU is added.

> 
> > +	help
> > +	  If you say yes here you will get support for the RTC subsystem
> > +	  of the Nintendo GameCube, Wii and Wii U.
> > +
> > +	  This driver can also be built as a module. If so, the module
> > +	  will be called "rtc-nintendo".
> > +
> >  config RTC_DRV_WM831X
> >  	tristate "Wolfson Microelectronics WM831x RTC"
> >  	depends on MFD_WM831X
> > diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> > index 2dd0dd956b0e..0c7c0e7d9637 100644
> > --- a/drivers/rtc/Makefile
> > +++ b/drivers/rtc/Makefile
> > @@ -108,6 +108,7 @@ obj-$(CONFIG_RTC_DRV_MT7622)	+= rtc-mt7622.o
> >  obj-$(CONFIG_RTC_DRV_MV)	+= rtc-mv.o
> >  obj-$(CONFIG_RTC_DRV_MXC)	+= rtc-mxc.o
> >  obj-$(CONFIG_RTC_DRV_MXC_V2)	+= rtc-mxc_v2.o
> > +obj-$(CONFIG_RTC_DRV_NINTENDO)	+= rtc-nintendo.o
> >  obj-$(CONFIG_RTC_DRV_NTXEC)	+= rtc-ntxec.o
> >  obj-$(CONFIG_RTC_DRV_OMAP)	+= rtc-omap.o
> >  obj-$(CONFIG_RTC_DRV_OPAL)	+= rtc-opal.o
> > diff --git a/drivers/rtc/rtc-nintendo.c b/drivers/rtc/rtc-nintendo.c
> > new file mode 100644
> > index 000000000000..11bea40ca13d
> > --- /dev/null
> > +++ b/drivers/rtc/rtc-nintendo.c
> 
> You have a bunch of checkpatch warnings, can you fix those?

Will do, sorry about that.

> 
> > @@ -0,0 +1,305 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Nintendo GameCube, Wii and Wii U RTC driver
> > + *
> > + * This driver is for the MX23L4005, more specifically its real-time clock and
> > + * SRAM storage.  The value returned by the RTC counter must be added with the
> > + * offset stored in a bias register in SRAM (on the GameCube and Wii) or in
> > + * /config/rtc.xml (on the Wii U).  The latter being very impractical to access
> > + * from Linux, this driver assumes the bootloader has read it and stored it in
> > + * SRAM like for the other two consoles.
> > + *
> > + * This device sits on a bus named EXI (which is similar to SPI), channel 0,
> > + * device 1.  This driver assumes no other user of the EXI bus, which is
> > + * currently the case but would have to be reworked to add support for other
> > + * GameCube hardware exposed on this bus.
> > + *
> > + * Note that this device is very much not Y2k38 aware.
> > + *
> > + * References:
> > + * - https://wiiubrew.org/wiki/Hardware/RTC
> > + * - https://wiibrew.org/wiki/MX23L4005
> > + *
> > + * Copyright (C) 2018 rw-r-r-0644
> > + * Copyright (C) 2021 Emmanuel Gil Peyrot <linkmauve@xxxxxxxxxxxx>
> > + *
> > + * Based on rtc-gcn.c
> > + * Copyright (C) 2004-2009 The GameCube Linux Team
> > + * Copyright (C) 2005,2008,2009 Albert Herranz
> > + * Based on gamecube_time.c from Torben Nielsen.
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/rtc.h>
> > +#include <linux/time.h>
> > +
> > +/* EXI registers */
> > +#define EXICSR	0
> > +#define EXICR	12
> > +#define EXIDATA	16
> > +
> > +/* EXI register values */
> > +#define EXICSR_DEV		0x380
> > +	#define EXICSR_DEV1	0x100
> > +#define EXICSR_CLK		0x070
> > +	#define EXICSR_CLK_1MHZ	0x000
> > +	#define EXICSR_CLK_2MHZ	0x010
> > +	#define EXICSR_CLK_4MHZ	0x020
> > +	#define EXICSR_CLK_8MHZ	0x030
> > +	#define EXICSR_CLK_16MHZ 0x040
> > +	#define EXICSR_CLK_32MHZ 0x050
> > +#define EXICSR_INT		0x008
> > +	#define EXICSR_INTSET	0x008
> > +
> > +#define EXICR_TSTART		0x001
> > +#define EXICR_TRSMODE		0x002
> > +	#define EXICR_TRSMODE_IMM 0x000
> > +#define EXICR_TRSTYPE		0x00C
> > +	#define EXICR_TRSTYPE_R	0x000
> > +	#define EXICR_TRSTYPE_W	0x004
> > +#define EXICR_TLEN		0x030
> > +	#define EXICR_TLEN32	0x030
> > +
> > +/* EXI registers values to access the RTC */
> > +#define RTC_EXICSR	(EXICSR_DEV1 | EXICSR_CLK_8MHZ | EXICSR_INTSET)
> > +#define RTC_EXICR_W	(EXICR_TSTART | EXICR_TRSMODE_IMM | EXICR_TRSTYPE_W | EXICR_TLEN32)
> > +#define RTC_EXICR_R	(EXICR_TSTART | EXICR_TRSMODE_IMM | EXICR_TRSTYPE_R | EXICR_TLEN32)
> > +#define RTC_EXIDATA_W	0x80000000
> > +
> > +/* RTC registers */
> > +#define RTC_COUNTER	0x20000000
> > +#define RTC_SRAM	0x20000100
> > +#define RTC_SRAM_BIAS	0x20000400
> > +#define RTC_SNAPSHOT	0x20400000
> > +#define RTC_ONTMR	0x21000000
> > +#define RTC_OFFTMR	0x21000100
> > +#define RTC_TEST0	0x21000400
> > +#define RTC_TEST1	0x21000500
> > +#define RTC_TEST2	0x21000600
> > +#define RTC_TEST3	0x21000700
> > +#define RTC_CONTROL0	0x21000C00
> > +#define RTC_CONTROL1	0x21000D00
> > +
> > +struct nintendo_rtc_drvdata {
> > +	void __iomem *iob;
> > +	u32 rtc_bias;
> > +};
> > +
> > +static u32 exi_read(void __iomem *iob, u32 reg)
> > +{
> > +	u32 data;
> > +
> > +	/* The spin loops here loop about 15~16 times each, so there is no need
> > +	 * to use a more expensive sleep method. */
> > +
> > +	/* Write register offset */
> > +	iowrite32be(RTC_EXICSR, iob + EXICSR);
> > +	iowrite32be(reg, iob + EXIDATA);
> > +	iowrite32be(RTC_EXICR_W, iob + EXICR);
> > +	while (!(ioread32be(iob + EXICSR) & EXICSR_INTSET))
> > +		cpu_relax();
> > +
> > +	/* Read data */
> > +	iowrite32be(RTC_EXICSR, iob + EXICSR);
> > +	iowrite32be(RTC_EXICR_R, iob + EXICR);
> > +	while (!(ioread32be(iob + EXICSR) & EXICSR_INTSET))
> > +		cpu_relax();
> > +	data = ioread32be(iob + EXIDATA);
> > +
> > +	/* Clear channel parameters */
> > +	iowrite32be(0, iob + EXICSR);
> > +
> > +	return data;
> > +}
> > +
> > +static void exi_write(void __iomem *iob, u32 reg, u32 data)
> > +{
> > +	/* The spin loops here loop about 15~16 times each, so there is no need
> > +	 * to use a more expensive sleep method. */
> > +
> > +	/* Write register offset */
> > +	iowrite32be(RTC_EXICSR, iob + EXICSR);
> > +	iowrite32be(reg | RTC_EXIDATA_W, iob + EXIDATA);
> > +	iowrite32be(RTC_EXICR_W, iob + EXICR);
> > +	while (!(ioread32be(iob + EXICSR) & EXICSR_INTSET))
> > +		cpu_relax();
> > +
> > +	/* Write data */
> > +	iowrite32be(RTC_EXICSR, iob + EXICSR);
> > +	iowrite32be(data, iob + EXIDATA);
> > +	iowrite32be(RTC_EXICR_W, iob + EXICR);
> > +	while (!(ioread32be(iob + EXICSR) & EXICSR_INTSET))
> > +		cpu_relax();
> > +
> > +	/* Clear channel parameters */
> > +	iowrite32be(0, iob + EXICSR);
> > +}
> > +
> > +static int nintendo_rtc_read_time(struct device *dev, struct rtc_time *t)
> > +{
> > +	time64_t timestamp;
> > +	struct nintendo_rtc_drvdata *d = dev_get_drvdata(dev);
> > +
> > +	/* Add the counter and the bias to obtain the timestamp */
> > +	timestamp = exi_read(d->iob, RTC_COUNTER) + d->rtc_bias;
> > +	rtc_time64_to_tm(timestamp, t);
> > +
> > +	return 0;
> > +}
> > +
> > +static int nintendo_rtc_set_time(struct device *dev, struct rtc_time *t)
> > +{
> > +	time64_t timestamp;
> > +	struct nintendo_rtc_drvdata *d = dev_get_drvdata(dev);
> > +
> > +	/* Subtract the timestamp and the bias to obtain the counter value */
> > +	timestamp = rtc_tm_to_time64(t);
> > +	exi_write(d->iob, RTC_COUNTER, timestamp - d->rtc_bias);
> 
> As you are able to update RTC_COUNTER, I'm not sure why you actually
> need rtc_bias.

The proprietary firmware sets the counter based on the bias, so if we
want to get the correct time out of the box we have to sum them.  It
wouldn’t even be possible to set bias on all of the consoles from Linux,
for instance on the Wii U it is stored in an XML file, in a proprietary
filesystem, and encrypted (using keys for which the drivers are already
in mainline at least).

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct rtc_class_ops nintendo_rtc_ops = {
> > +	.read_time      = nintendo_rtc_read_time,
> > +	.set_time       = nintendo_rtc_set_time,
> > +};
> > +
> > +#ifdef DEBUG
> > +static void nintendo_rtc_dumpregs(void __iomem *iob)
> > +{
> > +	int i;
> > +	u32 sram_addr = RTC_SRAM;
> > +
> > +	printk("RTC_COUNTER:  %08X\n", exi_read(iob, RTC_COUNTER));
> > +	printk("RTC_SNAPSHOT: %08X\n", exi_read(iob, RTC_SNAPSHOT));
> > +	printk("RTC_ONTMR:    %08X\n", exi_read(iob, RTC_ONTMR));
> > +	printk("RTC_OFFTMR:   %08X\n", exi_read(iob, RTC_OFFTMR));
> > +	printk("RTC_TEST0:    %08X\n", exi_read(iob, RTC_TEST0));
> > +	printk("RTC_TEST1:    %08X\n", exi_read(iob, RTC_TEST1));
> > +	printk("RTC_TEST2:    %08X\n", exi_read(iob, RTC_TEST2));
> > +	printk("RTC_TEST3:    %08X\n", exi_read(iob, RTC_TEST3));
> > +	printk("RTC_CONTROL0: %08X\n", exi_read(iob, RTC_CONTROL0));
> > +	printk("RTC_CONTROL1: %08X\n", exi_read(iob, RTC_CONTROL1));
> > +	printk("RTC_SRAM:\n");
> > +	for(i = 0; i < 4; i++) {
> > +		printk("%08X %08X %08X %08X\n",
> > +		       exi_read(iob, sram_addr + 0x100 * 0),
> > +		       exi_read(iob, sram_addr + 0x100 * 1),
> > +		       exi_read(iob, sram_addr + 0x100 * 2),
> > +		       exi_read(iob, sram_addr + 0x100 * 3));
> > +		sram_addr += 0x400;
> 
> Something great to do would be to convert the driver to regmap, provding
> custom regmap_read and regmap_write functions to access the EXI bus.
> Then you'd get this directly in debugfs. And this could be split ou once
> other drivers need access to the bus (I guess power/reset at some
> point).

Will do, I wasn’t aware of regmap, thanks!

> 
> > +	}
> > +}
> > +#endif
> > +
> > +static int nintendo_rtc_read_offset_from_sram(struct nintendo_rtc_drvdata *d)
> > +{
> > +	struct device_node *np;
> > +	int ret;
> > +	struct resource res;
> > +	void __iomem *hw_srnprot;
> > +	u32 old;
> > +
> > +	np = of_find_compatible_node(NULL, NULL, "nintendo,latte-srnprot");
> > +	if (!np) {
> > +		pr_err("HW_SRNPROT not found\n");
> > +		return -1;
> > +	}
> > +
> > +	ret = of_address_to_resource(np, 0, &res);
> > +	if (ret) {
> > +		pr_err("no io memory range found\n");
> > +		return -1;
> > +	}
> > +
> > +	hw_srnprot = ioremap(res.start, resource_size(&res));
> > +	old = ioread32be(hw_srnprot);
> > +
> > +	/* TODO: figure out why we use this magic constant.  I obtained it by
> > +	 * reading the leftover value after boot, after IOSU already ran.
> > +	 *
> > +	 * On my Wii U, setting this register to 1 prevents the console from
> > +	 * rebooting properly, so wiiubrew.org must be missing something.
> > +	 *
> > +	 * See https://wiiubrew.org/wiki/Hardware/Latte_registers
> > +	 */
> > +	if (old != 0x7bf)
> > +		iowrite32be(0x7bf, hw_srnprot);
> > +
> > +#ifdef DEBUG
> > +	nintendo_rtc_dumpregs(d->iob);
> > +#endif
> > +
> > +	/* Get the offset from RTC SRAM.
> > +	 *
> > +	 * Its default location on the GameCube and on the Wii is in the SRAM,
> > +	 * while on the Wii U the bootloader needs to fill it with the contents
> > +	 * of /config/rtc.xml on the SLC (the eMMC).  We don’t do that from
> > +	 * Linux since it requires implementing a proprietary filesystem and do
> > +	 * file decryption, instead we require the bootloader to fill the same
> > +	 * SRAM address as on previous consoles.
> > +	 */
> > +	d->rtc_bias = exi_read(d->iob, RTC_SRAM_BIAS);
> > +
> > +	/* Reset SRAM access to how it was before, our job here is done. */
> > +	iowrite32be(old, hw_srnprot);
> > +	iounmap(hw_srnprot);
> > +
> > +	return 0;
> > +}
> > +
> > +static int nintendo_rtc_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct rtc_device *rtc;
> > +	struct resource	*res;
> > +	struct nintendo_rtc_drvdata *d;
> > +	int ret;
> > +
> > +	d = devm_kzalloc(dev, sizeof(struct nintendo_rtc_drvdata), GFP_KERNEL);
> > +	if (IS_ERR(d))
> > +		return PTR_ERR(d);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	d->iob = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(d->iob))
> > +		return PTR_ERR(d->iob);
> > +
> > +	ret = nintendo_rtc_read_offset_from_sram(d);
> > +	if (ret)
> > +		return ret;
> > +	dev_dbg(dev, "SRAM bias: 0x%x", d->rtc_bias);
> > +
> > +	dev_set_drvdata(dev, d);
> > +
> > +	rtc = devm_rtc_device_register(dev, dev_name(dev), &nintendo_rtc_ops,
> > +				       THIS_MODULE);
> > +	if (IS_ERR(rtc))
> > +		return PTR_ERR(rtc);
> 
> Please use devm_rtc_allocate_device and devm_rtc_register_device and set
> the RTC range properly. You claim it is not Y2038 aware but it seems to
> be a 32bit counter which means it will count up to year 2106 (the RTC
> value is necessarily unsigned) so I guess you should also fix that
> comment.

Hmm, indeed, I’ll fix that.

> 
> Ideally, you should also expose LOW_BATT from RTC_CONTROL0 using the
> VL_READ ioctl as I'm guessing many console will start to have a depleted
> battery.

I won’t be able to test that yet, but I’ll try to implement it as it is
documented.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id nintendo_rtc_of_match[] = {
> > +	{.compatible = "nintendo,latte-exi" },
> > +	{.compatible = "nintendo,hollywood-exi" },
> > +	{.compatible = "nintendo,flipper-exi" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, nintendo_rtc_of_match);
> > +
> > +static struct platform_driver nintendo_rtc_driver = {
> > +	.probe		= nintendo_rtc_probe,
> > +	.driver		= {
> > +		.name	= "rtc-nintendo",
> > +		.of_match_table	= nintendo_rtc_of_match,
> > +	},
> > +};
> > +module_platform_driver(nintendo_rtc_driver);
> > +
> > +MODULE_AUTHOR("Emmanuel Gil Peyrot <linkmauve@xxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Nintendo GameCube, Wii and Wii U RTC driver");
> > +MODULE_LICENSE("GPL");
> > -- 
> > 2.33.1
> > 
> 
> -- 
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

-- 
Emmanuel Gil Peyrot

Attachment: signature.asc
Description: PGP signature