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

[Alexandre Belloni]

On 27/10/2021 19:05:27+0200, Emmanuel Gil Peyrot wrote:
> > 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.
> 

I think this is appropriate, that would have been my suggestion.

> > > +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).
> 

Ok, fine by me.

> > > +#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!
> 

This is feature creep pushed to the max, I would take your driver even
if you don't do 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.
> 

This can also come in a second patch. I'll try to boot up my own
(platinum edition) GC, I'm wondering what is the battery status :)


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com