| Message ID | 515CF985.6050009@interlog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/04/2013 05:54 AM, Douglas Gilbert : > Some members of the at91 SoCs use the Real Time Timer (RTT) > and the General Purpose Backup Registers (GPBR) to implement > a real time clock (RTC). The AT91SAM9G20 is one example. > > Attached is a patch to add DT support to rtc-at91sam9.c . > The patch is against lk 3.9.0-rc5 . > > Below is a snippet of DT code for the 'G20 that was observed > to work with this patch: > > ahb { > apb { > > rtc { > compatible = "atmel,at91sam9-rtc"; The compatible string has to be formed by the name of the first SoC compatible with this IP. It turns to be the at91sam9260. The second part of the string should be a name that reflects the nature of the peripheral. For this binding, I would like to mention the "RTT" in the compatibility string (because other drivers can use other RTT with other uses). What do you think about: "atmel,at91sam9260-rtt-as-rtc"? or something shorter? > /* RTTC followed by GPBR (backup registers) */ > reg = <0xfffffd20 0x10>, <0xfffffd50 0x10>; > interrupts = <1 4 7>; > status = "okay"; Last, but not least, when we add a DT binding, it is a requirement to add the corresponding documentation in the Documentation/devicetree/bindings/rtc/ directory. > }; > } > }; > > Signed-off-by: Douglas Gilbert <dgilbert@interlog.com> Thanks, best regards,
On Wed, Apr 03, 2013 at 11:54:45PM -0400, Douglas Gilbert wrote: > diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c > index 39cfd2e..69e9389 100644 > --- a/drivers/rtc/rtc-at91sam9.c > +++ b/drivers/rtc/rtc-at91sam9.c > @@ -20,6 +20,8 @@ > #include <linux/ioctl.h> > #include <linux/slab.h> > #include <linux/platform_data/atmel.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > > #include <mach/at91_rtt.h> > #include <mach/cpu.h> > @@ -71,6 +73,17 @@ struct sam9_rtc { > #define gpbr_writel(rtc, val) \ > __raw_writel((val), (rtc)->gpbr) > > +#ifdef CONFIG_OF > +static const struct of_device_id at91sam9_rtc_dt_ids[] = { > + { .compatible = "atmel,at91sam9-rtc" }, This should probably be "atmel,at91sam9260-rtt". > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, at91sam9_rtc_dt_ids); > +#else > +#define at91sam9_rtc_dt_ids NULL > +#endif /* CONFIG_OF */ You don't need the else-clause when you use of_match_ptr below. > + > + > /* > * Read current time and date in RTC > */ > @@ -470,6 +483,7 @@ static struct platform_driver at91_rtc_driver = { > .driver = { > .name = "rtc-at91sam9", > .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(at91sam9_rtc_dt_ids), > }, > }; Thanks, Johan
On 13-04-04 04:11 AM, Nicolas Ferre wrote: > On 04/04/2013 05:54 AM, Douglas Gilbert : >> Some members of the at91 SoCs use the Real Time Timer (RTT) >> and the General Purpose Backup Registers (GPBR) to implement >> a real time clock (RTC). The AT91SAM9G20 is one example. >> >> Attached is a patch to add DT support to rtc-at91sam9.c . >> The patch is against lk 3.9.0-rc5 . >> >> Below is a snippet of DT code for the 'G20 that was observed >> to work with this patch: >> >> ahb { >> apb { >> >> rtc { >> compatible = "atmel,at91sam9-rtc"; > > The compatible string has to be formed by the name of the first SoC > compatible with this IP. It turns to be the at91sam9260. > The second part of the string should be a name that reflects the nature > of the peripheral. For this binding, I would like to mention the "RTT" > in the compatibility string (because other drivers can use other RTT > with other uses). > > What do you think about: > "atmel,at91sam9260-rtt-as-rtc"? or something shorter? Hi Nicolas, Johan Hovold suggested: atmel,at91sam9260-rtt I notice (in the G20 doco) that the acronym RTTC is also used for the rtt registers. What do you want? > >> /* RTTC followed by GPBR (backup registers) */ >> reg = <0xfffffd20 0x10>, <0xfffffd50 0x10>; >> interrupts = <1 4 7>; >> status = "okay"; > > Last, but not least, when we add a DT binding, it is a requirement to > add the corresponding documentation in the > Documentation/devicetree/bindings/rtc/ directory. I have been underwhelmed by the accuracy and the organisation of information in that documentation. And the examples are often misleading given the actual hierarchy of real dtsi/dts config files. Give me working examples any day. You could (and should) test what I gave on a g20ek board. Also I note there is no "bindings" documentation for rtc-at91rm9200.c :-) After you, sir .... Doug Gilbert
On 04/04/2013 03:25 PM, Douglas Gilbert : > On 13-04-04 04:11 AM, Nicolas Ferre wrote: >> On 04/04/2013 05:54 AM, Douglas Gilbert : >>> Some members of the at91 SoCs use the Real Time Timer (RTT) >>> and the General Purpose Backup Registers (GPBR) to implement >>> a real time clock (RTC). The AT91SAM9G20 is one example. >>> >>> Attached is a patch to add DT support to rtc-at91sam9.c . >>> The patch is against lk 3.9.0-rc5 . >>> >>> Below is a snippet of DT code for the 'G20 that was observed >>> to work with this patch: >>> >>> ahb { >>> apb { >>> >>> rtc { >>> compatible = "atmel,at91sam9-rtc"; >> >> The compatible string has to be formed by the name of the first SoC >> compatible with this IP. It turns to be the at91sam9260. >> The second part of the string should be a name that reflects the nature >> of the peripheral. For this binding, I would like to mention the "RTT" >> in the compatibility string (because other drivers can use other RTT >> with other uses). >> >> What do you think about: >> "atmel,at91sam9260-rtt-as-rtc"? or something shorter? > > Hi Nicolas, > Johan Hovold suggested: > atmel,at91sam9260-rtt Yes, but I fear this could bring confusion if someone is building a RTT driver that is not targeted at acting as a RTC... > I notice (in the G20 doco) that the acronym RTTC is also used The "C" at the end stands for "Controller" (not very useful convention in my opinion). > for the rtt registers. What do you want? I found xxx-rtt-as-rtc or xxx-rtt-rtc but not completely satisfied with any of them... >>> /* RTTC followed by GPBR (backup registers) */ >>> reg = <0xfffffd20 0x10>, <0xfffffd50 0x10>; >>> interrupts = <1 4 7>; >>> status = "okay"; >> >> Last, but not least, when we add a DT binding, it is a requirement to >> add the corresponding documentation in the >> Documentation/devicetree/bindings/rtc/ directory. > > I have been underwhelmed by the accuracy and the organisation > of information in that documentation. And the examples are > often misleading given the actual hierarchy of real dtsi/dts > config files. Oh, really? I will have a look one of those days... > Give me working examples any day. You could (and should) test > what I gave on a g20ek board. > > Also I note there is no "bindings" documentation for > rtc-at91rm9200.c :-) After you, sir .... Already submitted, my dear ;-) Here: http://ozlabs.org/~akpm/mmots/broken-out/drivers-rtc-rtc-at91rm9200c-add-dt-support.patch Bye,
On 13-04-04 10:08 AM, Nicolas Ferre wrote: > On 04/04/2013 03:25 PM, Douglas Gilbert : >> On 13-04-04 04:11 AM, Nicolas Ferre wrote: >>> On 04/04/2013 05:54 AM, Douglas Gilbert : >>>> Some members of the at91 SoCs use the Real Time Timer (RTT) >>>> and the General Purpose Backup Registers (GPBR) to implement >>>> a real time clock (RTC). The AT91SAM9G20 is one example. >>>> >>>> Attached is a patch to add DT support to rtc-at91sam9.c . >>>> The patch is against lk 3.9.0-rc5 . >>>> >>>> Below is a snippet of DT code for the 'G20 that was observed >>>> to work with this patch: >>>> >>>> ahb { >>>> apb { >>>> >>>> rtc { >>>> compatible = "atmel,at91sam9-rtc"; >>> >>> The compatible string has to be formed by the name of the first SoC >>> compatible with this IP. It turns to be the at91sam9260. >>> The second part of the string should be a name that reflects the nature >>> of the peripheral. For this binding, I would like to mention the "RTT" >>> in the compatibility string (because other drivers can use other RTT >>> with other uses). >>> >>> What do you think about: >>> "atmel,at91sam9260-rtt-as-rtc"? or something shorter? >> >> Hi Nicolas, >> Johan Hovold suggested: >> atmel,at91sam9260-rtt > > Yes, but I fear this could bring confusion if someone is building a RTT > driver that is not targeted at acting as a RTC... > >> I notice (in the G20 doco) that the acronym RTTC is also used > > The "C" at the end stands for "Controller" (not very useful convention > in my opinion). > >> for the rtt registers. What do you want? > > I found xxx-rtt-as-rtc or xxx-rtt-rtc but not completely satisfied with > any of them... > >>>> /* RTTC followed by GPBR (backup registers) */ >>>> reg = <0xfffffd20 0x10>, <0xfffffd50 0x10>; >>>> interrupts = <1 4 7>; >>>> status = "okay"; >>> >>> Last, but not least, when we add a DT binding, it is a requirement to >>> add the corresponding documentation in the >>> Documentation/devicetree/bindings/rtc/ directory. >> >> I have been underwhelmed by the accuracy and the organisation >> of information in that documentation. And the examples are >> often misleading given the actual hierarchy of real dtsi/dts >> config files. > > Oh, really? I will have a look one of those days... > > >> Give me working examples any day. You could (and should) test >> what I gave on a g20ek board. >> >> Also I note there is no "bindings" documentation for >> rtc-at91rm9200.c :-) After you, sir .... > > Already submitted, my dear ;-) > Here: > http://ozlabs.org/~akpm/mmots/broken-out/drivers-rtc-rtc-at91rm9200c-add-dt-support.patch Good. On a more serious note, looking at a dts file for working hardware is useful _but_ it is not clear how a kernel should be configured to fully utilize that dts file. Without a defconfig a user may need to resort to several searches like this: find <kernel_src> -name '*.c' -exec grep "atmel,at91sam9x5-rtc" {} \; -print which is non-obvious to the novice. After a line like that, the Makefile in that directory may need to be checked to find out which CONFIG_ option needs to be set to get the associated driver built in (or a module?). So is there a policy _not_ to place "compat" strings in Kconfig files where they might be searchable? The "bindings" documentation and DT incantations are all for nought if the associated driver is not configured. Looking at the dmesg output is futile. Doug Gilbert
On Thu, Apr 04, 2013 at 04:08:04PM +0200, Nicolas Ferre wrote: > On 04/04/2013 03:25 PM, Douglas Gilbert : > > On 13-04-04 04:11 AM, Nicolas Ferre wrote: > >> On 04/04/2013 05:54 AM, Douglas Gilbert : > >>> Some members of the at91 SoCs use the Real Time Timer (RTT) > >>> and the General Purpose Backup Registers (GPBR) to implement > >>> a real time clock (RTC). The AT91SAM9G20 is one example. > >>> > >>> Attached is a patch to add DT support to rtc-at91sam9.c . > >>> The patch is against lk 3.9.0-rc5 . > >>> > >>> Below is a snippet of DT code for the 'G20 that was observed > >>> to work with this patch: > >>> > >>> ahb { > >>> apb { > >>> > >>> rtc { > >>> compatible = "atmel,at91sam9-rtc"; > >> > >> The compatible string has to be formed by the name of the first SoC > >> compatible with this IP. It turns to be the at91sam9260. > >> The second part of the string should be a name that reflects the nature > >> of the peripheral. For this binding, I would like to mention the "RTT" > >> in the compatibility string (because other drivers can use other RTT > >> with other uses). > >> > >> What do you think about: > >> "atmel,at91sam9260-rtt-as-rtc"? or something shorter? > > > > Hi Nicolas, > > Johan Hovold suggested: > > atmel,at91sam9260-rtt > > Yes, but I fear this could bring confusion if someone is building a RTT > driver that is not targeted at acting as a RTC... But isn't the compatible property supposed to describe the peripheral in a completely platform-agnostic way? In fact, it seems it should be on the form compatible = "atmel,at91sam9g45-rtt", "atmel,at91sam9260-rtt"; where an exact description (the actual SoC-IP) is followed by a more generic compatible description (e.g. the first SoC that introduced the IP that is still compatible) (see [1]). This policy seems not have been followed for at91 though, as only the second (first compatible SoC) string is generally used. Either way, this still leaves open the question of which driver gets bound to a device if there are more than one driver that matches the compatible property. I guess this could be solved with a property set or overridden by the board description. For example: at91sam9g45.dtsi: rtt@fffffd20 { compatible = "atmel,at91sam9g45-rtt", "atmel,at91sam9260-rtt"; reg = <0xfffffd20 0x10>; interrupts = <1 4 7>; status = "disabled"; }; at91sam9m10g45ek.dts: rtt@fffffd20 { atmel,at91-rtt-as-rtc; status = "okay"; }; If the device-specific property atmel,at91-rtt-as-rtc is missing, the rtc-at91sam9 driver probe should fail so that any other matching driver is probed. Of course, this will only work if the other matching rtt-drivers also add device-specific properties so that they can be disabled as well (should the probe order be reversed), for instance: atmel,at91-rtt-as-<xxx>; > >>> /* RTTC followed by GPBR (backup registers) */ > >>> reg = <0xfffffd20 0x10>, <0xfffffd50 0x10>; > >>> interrupts = <1 4 7>; > >>> status = "okay"; Which backup register (GPBR) the RTC-time base should be stored in is currently configurable using CONFIG_RTC_DRV_AT91SAM9_GPBR (and the address arguably not even a property of the rtt) so the register address should in any case not be set in the rtt-node of the dtsi. Instead I think we need the arch code to map the backup-registers and thus to add a new gpbr-node: gpbr@fffffd60 { compatible = "atmel,at91sam9g45-gpbr", "atmel,at91sam9260-gpbr"; reg = <0xfffffd60 0x10>; }; We could then use CONFIG_RTC_DRV_AT91SAM9_GPBR for non-DT as before, and add a new property (or simply replace the boolean) to the rtt-nodes in the board descriptions: rtt@fffffd20 { atmel,at91-rtt-as-rtc; atmel,at91-rtt-as-rtc-gpbr = <n>; status = "okay"; }; to select which of the four (sam9263 has 20) backup registers to use. I'm responding to this mail with an RFC-implementing the GPBR and RTT-as-RTC DT support as outlined above. Finally, for the record: There are currently no other rtt-drivers, but I have posted a patch series a while ago to fix hanged boot due to early spurious RTT (or RTC) interrupts (see [2]). The series uses "atmel,at91sam9260-rtt" to detect the SoCs that are affected by the problem, and I guess that is an example of why the compatible string should merely describe the peripheral (and not its use). Thanks, Johan [1] http://www.devicetree.org/Device_Tree_Usage#Understanding_the_compatible_Property [2] http://marc.info/?l=linux-arm-kernel&m=136302554825979&w=2
On 17:12 Sun 07 Apr , Johan Hovold wrote: > > Add General Purpose Backup Register (GPBR) support. > > Most at91 SoCs have at least four 32-bit General Purpose Backup > Registers (GPBR) powered by backup-power (VDDBU). One such register is > currently used by rtc-at91sam9 driver to store the RTC time base. > > Make sure the registers are mapped by arch setup code and add generic > accessors for in-kernel use. > > This is a step in adding device-tree support to the rtc-at91sam9 driver. > This is a regression we loose the tracking of what request and use the GPBR Best Regsards, J. > Signed-off-by: Johan Hovold <jhovold@gmail.com> > --- > arch/arm/mach-at91/at91sam9260.c | 1 + > arch/arm/mach-at91/at91sam9260_devices.c | 15 +++-------- > arch/arm/mach-at91/at91sam9261.c | 1 + > arch/arm/mach-at91/at91sam9261_devices.c | 15 +++-------- > arch/arm/mach-at91/at91sam9263.c | 1 + > arch/arm/mach-at91/at91sam9263_devices.c | 18 +++---------- > arch/arm/mach-at91/at91sam9g45.c | 1 + > arch/arm/mach-at91/at91sam9g45_devices.c | 15 +++-------- > arch/arm/mach-at91/at91sam9rl.c | 1 + > arch/arm/mach-at91/at91sam9rl_devices.c | 15 +++-------- > arch/arm/mach-at91/generic.h | 3 +++ > arch/arm/mach-at91/include/mach/at91_gpbr.h | 29 ++++++++++++++++++++ > arch/arm/mach-at91/setup.c | 10 +++++++ > drivers/rtc/rtc-at91sam9.c | 41 +++++++++-------------------- > 14 files changed, 76 insertions(+), 90 deletions(-) > create mode 100644 arch/arm/mach-at91/include/mach/at91_gpbr.h > > diff --git a/arch/arm/mach-at91/at91sam9260.c b/arch/arm/mach-at91/at91sam9260.c > index b67cd53..984b0d5 100644 > --- a/arch/arm/mach-at91/at91sam9260.c > +++ b/arch/arm/mach-at91/at91sam9260.c > @@ -334,6 +334,7 @@ static void __init at91sam9260_map_io(void) > > static void __init at91sam9260_ioremap_registers(void) > { > + at91_ioremap_gpbr(AT91SAM9260_BASE_GPBR, 0x10); > at91_ioremap_shdwc(AT91SAM9260_BASE_SHDWC); > at91_ioremap_rstc(AT91SAM9260_BASE_RSTC); > at91_ioremap_ramc(0, AT91SAM9260_BASE_SDRAMC, 512); > diff --git a/arch/arm/mach-at91/at91sam9260_devices.c b/arch/arm/mach-at91/at91sam9260_devices.c > index eda8d16..10c71a8 100644 > --- a/arch/arm/mach-at91/at91sam9260_devices.c > +++ b/arch/arm/mach-at91/at91sam9260_devices.c > @@ -649,8 +649,6 @@ static struct resource rtt_resources[] = { > .end = AT91SAM9260_BASE_RTT + SZ_16 - 1, > .flags = IORESOURCE_MEM, > }, { > - .flags = IORESOURCE_MEM, > - }, { > .flags = IORESOURCE_IRQ, > }, > }; > @@ -666,16 +664,9 @@ static struct platform_device at91sam9260_rtt_device = { > static void __init at91_add_device_rtt_rtc(void) > { > at91sam9260_rtt_device.name = "rtc-at91sam9"; > - /* > - * The second resource is needed: > - * GPBR will serve as the storage for RTC time offset > - */ > - at91sam9260_rtt_device.num_resources = 3; > - rtt_resources[1].start = AT91SAM9260_BASE_GPBR + > - 4 * CONFIG_RTC_DRV_AT91SAM9_GPBR; > - rtt_resources[1].end = rtt_resources[1].start + 3; > - rtt_resources[2].start = NR_IRQS_LEGACY + AT91_ID_SYS; > - rtt_resources[2].end = NR_IRQS_LEGACY + AT91_ID_SYS; > + at91sam9260_rtt_device.num_resources = 2; > + rtt_resources[1].start = NR_IRQS_LEGACY + AT91_ID_SYS; > + rtt_resources[1].end = NR_IRQS_LEGACY + AT91_ID_SYS; > } > #else > static void __init at91_add_device_rtt_rtc(void) > diff --git a/arch/arm/mach-at91/at91sam9261.c b/arch/arm/mach-at91/at91sam9261.c > index 0204f4c..b234c5d 100644 > --- a/arch/arm/mach-at91/at91sam9261.c > +++ b/arch/arm/mach-at91/at91sam9261.c > @@ -278,6 +278,7 @@ static void __init at91sam9261_map_io(void) > > static void __init at91sam9261_ioremap_registers(void) > { > + at91_ioremap_gpbr(AT91SAM9261_BASE_GPBR, 0x10); > at91_ioremap_shdwc(AT91SAM9261_BASE_SHDWC); > at91_ioremap_rstc(AT91SAM9261_BASE_RSTC); > at91_ioremap_ramc(0, AT91SAM9261_BASE_SDRAMC, 512); > diff --git a/arch/arm/mach-at91/at91sam9261_devices.c b/arch/arm/mach-at91/at91sam9261_devices.c > index 629ea5f..e8ff5e7 100644 > --- a/arch/arm/mach-at91/at91sam9261_devices.c > +++ b/arch/arm/mach-at91/at91sam9261_devices.c > @@ -618,8 +618,6 @@ static struct resource rtt_resources[] = { > .end = AT91SAM9261_BASE_RTT + SZ_16 - 1, > .flags = IORESOURCE_MEM, > }, { > - .flags = IORESOURCE_MEM, > - }, { > .flags = IORESOURCE_IRQ, > } > }; > @@ -634,16 +632,9 @@ static struct platform_device at91sam9261_rtt_device = { > static void __init at91_add_device_rtt_rtc(void) > { > at91sam9261_rtt_device.name = "rtc-at91sam9"; > - /* > - * The second resource is needed: > - * GPBR will serve as the storage for RTC time offset > - */ > - at91sam9261_rtt_device.num_resources = 3; > - rtt_resources[1].start = AT91SAM9261_BASE_GPBR + > - 4 * CONFIG_RTC_DRV_AT91SAM9_GPBR; > - rtt_resources[1].end = rtt_resources[1].start + 3; > - rtt_resources[2].start = NR_IRQS_LEGACY + AT91_ID_SYS; > - rtt_resources[2].end = NR_IRQS_LEGACY + AT91_ID_SYS; > + at91sam9261_rtt_device.num_resources = 2; > + rtt_resources[1].start = NR_IRQS_LEGACY + AT91_ID_SYS; > + rtt_resources[1].end = NR_IRQS_LEGACY + AT91_ID_SYS; > } > #else > static void __init at91_add_device_rtt_rtc(void) > diff --git a/arch/arm/mach-at91/at91sam9263.c b/arch/arm/mach-at91/at91sam9263.c > index c0cbb81..42d09f5 100644 > --- a/arch/arm/mach-at91/at91sam9263.c > +++ b/arch/arm/mach-at91/at91sam9263.c > @@ -315,6 +315,7 @@ static void __init at91sam9263_map_io(void) > > static void __init at91sam9263_ioremap_registers(void) > { > + at91_ioremap_gpbr(AT91SAM9263_BASE_GPBR, 0x50); > at91_ioremap_shdwc(AT91SAM9263_BASE_SHDWC); > at91_ioremap_rstc(AT91SAM9263_BASE_RSTC); > at91_ioremap_ramc(0, AT91SAM9263_BASE_SDRAMC0, 512); > diff --git a/arch/arm/mach-at91/at91sam9263_devices.c b/arch/arm/mach-at91/at91sam9263_devices.c > index 858c8aa..374fe56 100644 > --- a/arch/arm/mach-at91/at91sam9263_devices.c > +++ b/arch/arm/mach-at91/at91sam9263_devices.c > @@ -1014,8 +1014,6 @@ static struct resource rtt0_resources[] = { > .end = AT91SAM9263_BASE_RTT0 + SZ_16 - 1, > .flags = IORESOURCE_MEM, > }, { > - .flags = IORESOURCE_MEM, > - }, { > .flags = IORESOURCE_IRQ, > } > }; > @@ -1032,8 +1030,6 @@ static struct resource rtt1_resources[] = { > .end = AT91SAM9263_BASE_RTT1 + SZ_16 - 1, > .flags = IORESOURCE_MEM, > }, { > - .flags = IORESOURCE_MEM, > - }, { > .flags = IORESOURCE_IRQ, > } > }; > @@ -1052,18 +1048,14 @@ static void __init at91_add_device_rtt_rtc(void) > > switch (CONFIG_RTC_DRV_AT91SAM9_RTT) { > case 0: > - /* > - * The second resource is needed only for the chosen RTT: > - * GPBR will serve as the storage for RTC time offset > - */ > - at91sam9263_rtt0_device.num_resources = 3; > + at91sam9263_rtt0_device.num_resources = 2; > at91sam9263_rtt1_device.num_resources = 1; > pdev = &at91sam9263_rtt0_device; > r = rtt0_resources; > break; > case 1: > at91sam9263_rtt0_device.num_resources = 1; > - at91sam9263_rtt1_device.num_resources = 3; > + at91sam9263_rtt1_device.num_resources = 2; > pdev = &at91sam9263_rtt1_device; > r = rtt1_resources; > break; > @@ -1074,10 +1066,8 @@ static void __init at91_add_device_rtt_rtc(void) > } > > pdev->name = "rtc-at91sam9"; > - r[1].start = AT91SAM9263_BASE_GPBR + 4 * CONFIG_RTC_DRV_AT91SAM9_GPBR; > - r[1].end = r[1].start + 3; > - r[2].start = NR_IRQS_LEGACY + AT91_ID_SYS; > - r[2].end = NR_IRQS_LEGACY + AT91_ID_SYS; > + r[1].start = NR_IRQS_LEGACY + AT91_ID_SYS; > + r[1].end = NR_IRQS_LEGACY + AT91_ID_SYS; > } > #else > static void __init at91_add_device_rtt_rtc(void) > diff --git a/arch/arm/mach-at91/at91sam9g45.c b/arch/arm/mach-at91/at91sam9g45.c > index b4968aa..4b9547f 100644 > --- a/arch/arm/mach-at91/at91sam9g45.c > +++ b/arch/arm/mach-at91/at91sam9g45.c > @@ -361,6 +361,7 @@ static void __init at91sam9g45_map_io(void) > > static void __init at91sam9g45_ioremap_registers(void) > { > + at91_ioremap_gpbr(AT91SAM9G45_BASE_GPBR, 0x10); > at91_ioremap_shdwc(AT91SAM9G45_BASE_SHDWC); > at91_ioremap_rstc(AT91SAM9G45_BASE_RSTC); > at91_ioremap_ramc(0, AT91SAM9G45_BASE_DDRSDRC1, 512); > diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c > index fe626d4..65d700b 100644 > --- a/arch/arm/mach-at91/at91sam9g45_devices.c > +++ b/arch/arm/mach-at91/at91sam9g45_devices.c > @@ -1290,8 +1290,6 @@ static struct resource rtt_resources[] = { > .end = AT91SAM9G45_BASE_RTT + SZ_16 - 1, > .flags = IORESOURCE_MEM, > }, { > - .flags = IORESOURCE_MEM, > - }, { > .flags = IORESOURCE_IRQ, > } > }; > @@ -1306,16 +1304,9 @@ static struct platform_device at91sam9g45_rtt_device = { > static void __init at91_add_device_rtt_rtc(void) > { > at91sam9g45_rtt_device.name = "rtc-at91sam9"; > - /* > - * The second resource is needed: > - * GPBR will serve as the storage for RTC time offset > - */ > - at91sam9g45_rtt_device.num_resources = 3; > - rtt_resources[1].start = AT91SAM9G45_BASE_GPBR + > - 4 * CONFIG_RTC_DRV_AT91SAM9_GPBR; > - rtt_resources[1].end = rtt_resources[1].start + 3; > - rtt_resources[2].start = NR_IRQS_LEGACY + AT91_ID_SYS; > - rtt_resources[2].end = NR_IRQS_LEGACY + AT91_ID_SYS; > + at91sam9g45_rtt_device.num_resources = 2; > + rtt_resources[1].start = NR_IRQS_LEGACY + AT91_ID_SYS; > + rtt_resources[1].end = NR_IRQS_LEGACY + AT91_ID_SYS; > } > #else > static void __init at91_add_device_rtt_rtc(void) > diff --git a/arch/arm/mach-at91/at91sam9rl.c b/arch/arm/mach-at91/at91sam9rl.c > index 3de3e04..bf7e555 100644 > --- a/arch/arm/mach-at91/at91sam9rl.c > +++ b/arch/arm/mach-at91/at91sam9rl.c > @@ -281,6 +281,7 @@ static void __init at91sam9rl_map_io(void) > > static void __init at91sam9rl_ioremap_registers(void) > { > + at91_ioremap_gpbr(AT91SAM9RL_BASE_GPBR, 0x10); > at91_ioremap_shdwc(AT91SAM9RL_BASE_SHDWC); > at91_ioremap_rstc(AT91SAM9RL_BASE_RSTC); > at91_ioremap_ramc(0, AT91SAM9RL_BASE_SDRAMC, 512); > diff --git a/arch/arm/mach-at91/at91sam9rl_devices.c b/arch/arm/mach-at91/at91sam9rl_devices.c > index 352468f..533948c 100644 > --- a/arch/arm/mach-at91/at91sam9rl_devices.c > +++ b/arch/arm/mach-at91/at91sam9rl_devices.c > @@ -687,8 +687,6 @@ static struct resource rtt_resources[] = { > .end = AT91SAM9RL_BASE_RTT + SZ_16 - 1, > .flags = IORESOURCE_MEM, > }, { > - .flags = IORESOURCE_MEM, > - }, { > .flags = IORESOURCE_IRQ, > } > }; > @@ -703,16 +701,9 @@ static struct platform_device at91sam9rl_rtt_device = { > static void __init at91_add_device_rtt_rtc(void) > { > at91sam9rl_rtt_device.name = "rtc-at91sam9"; > - /* > - * The second resource is needed: > - * GPBR will serve as the storage for RTC time offset > - */ > - at91sam9rl_rtt_device.num_resources = 3; > - rtt_resources[1].start = AT91SAM9RL_BASE_GPBR + > - 4 * CONFIG_RTC_DRV_AT91SAM9_GPBR; > - rtt_resources[1].end = rtt_resources[1].start + 3; > - rtt_resources[2].start = NR_IRQS_LEGACY + AT91_ID_SYS; > - rtt_resources[2].end = NR_IRQS_LEGACY + AT91_ID_SYS; > + at91sam9rl_rtt_device.num_resources = 2; > + rtt_resources[1].start = NR_IRQS_LEGACY + AT91_ID_SYS; > + rtt_resources[1].end = NR_IRQS_LEGACY + AT91_ID_SYS; > } > #else > static void __init at91_add_device_rtt_rtc(void) > diff --git a/arch/arm/mach-at91/generic.h b/arch/arm/mach-at91/generic.h > index fc593d6..6d444cc 100644 > --- a/arch/arm/mach-at91/generic.h > +++ b/arch/arm/mach-at91/generic.h > @@ -59,6 +59,9 @@ extern void at91_irq_resume(void); > /* idle */ > extern void at91sam9_idle(void); > > +/* backup registers */ > +extern void at91_ioremap_gpbr(u32 base_addr, u32 size); > + > /* reset */ > extern void at91_ioremap_rstc(u32 base_addr); > extern void at91sam9_alt_restart(char, const char *); > diff --git a/arch/arm/mach-at91/include/mach/at91_gpbr.h b/arch/arm/mach-at91/include/mach/at91_gpbr.h > new file mode 100644 > index 0000000..f4b4895 > --- /dev/null > +++ b/arch/arm/mach-at91/include/mach/at91_gpbr.h > @@ -0,0 +1,29 @@ > +/* > + * arch/arm/mach-at91/include/mach/at91_gpbr.h > + * > + * Copyright (C) 2013 Johan Hovold <jhovold@gmail.com> > + * > + * General Purpose Backup Registers (GPBR) > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#ifndef AT91_GPBR_H > +#define AT91_GPBR_H > + > +extern void __iomem *at91_gpbr_base; > + > +static inline u32 at91_gpbr_read(unsigned nr) > +{ > + return __raw_readl(at91_gpbr_base + ((nr) << 2)); > +} > + > +static inline void at91_gpbr_write(unsigned nr, u32 value) > +{ > + __raw_writel(value, at91_gpbr_base + ((nr) << 2)); > +} > + > +#endif /* AT91_GPBR_H */ > diff --git a/arch/arm/mach-at91/setup.c b/arch/arm/mach-at91/setup.c > index 9e7c1e1..b01f4aa 100644 > --- a/arch/arm/mach-at91/setup.c > +++ b/arch/arm/mach-at91/setup.c > @@ -18,6 +18,7 @@ > #include <mach/hardware.h> > #include <mach/cpu.h> > #include <mach/at91_dbgu.h> > +#include <mach/at91_gpbr.h> > #include <mach/at91_pmc.h> > > #include "at91_shdwc.h" > @@ -277,6 +278,15 @@ void __init at91_map_io(void) > at91_boot_soc.map_io(); > } > > +void __iomem *at91_gpbr_base; > + > +void __init at91_ioremap_gpbr(u32 base_addr, u32 size) > +{ > + at91_gpbr_base = ioremap(base_addr, size); > + if (!at91_gpbr_base) > + panic("AT91: Failed to ioremap gpbr registers\n"); > +} > + > void __iomem *at91_shdwc_base = NULL; > > static void at91sam9_poweroff(void) > diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c > index 39cfd2e..983667b 100644 > --- a/drivers/rtc/rtc-at91sam9.c > +++ b/drivers/rtc/rtc-at91sam9.c > @@ -21,6 +21,7 @@ > #include <linux/slab.h> > #include <linux/platform_data/atmel.h> > > +#include <mach/at91_gpbr.h> > #include <mach/at91_rtt.h> > #include <mach/cpu.h> > > @@ -57,7 +58,7 @@ struct sam9_rtc { > void __iomem *rtt; > struct rtc_device *rtcdev; > u32 imr; > - void __iomem *gpbr; > + unsigned gpbr; > int irq; > }; > > @@ -66,11 +67,6 @@ struct sam9_rtc { > #define rtt_writel(rtc, field, val) \ > __raw_writel((val), (rtc)->rtt + AT91_RTT_ ## field) > > -#define gpbr_readl(rtc) \ > - __raw_readl((rtc)->gpbr) > -#define gpbr_writel(rtc, val) \ > - __raw_writel((val), (rtc)->gpbr) > - > /* > * Read current time and date in RTC > */ > @@ -81,7 +77,7 @@ static int at91_rtc_readtime(struct device *dev, struct rtc_time *tm) > u32 offset; > > /* read current time offset */ > - offset = gpbr_readl(rtc); > + offset = at91_gpbr_read(rtc->gpbr); > if (offset == 0) > return -EILSEQ; > > @@ -124,11 +120,11 @@ static int at91_rtc_settime(struct device *dev, struct rtc_time *tm) > rtt_writel(rtc, MR, mr & ~(AT91_RTT_ALMIEN | AT91_RTT_RTTINCIEN)); > > /* read current time offset */ > - offset = gpbr_readl(rtc); > + offset = at91_gpbr_read(rtc->gpbr); > > /* store the new base time in a battery backup register */ > secs += 1; > - gpbr_writel(rtc, secs); > + at91_gpbr_write(rtc->gpbr, secs); > > /* adjust the alarm time for the new base */ > alarm = rtt_readl(rtc, AR); > @@ -160,7 +156,7 @@ static int at91_rtc_readalarm(struct device *dev, struct rtc_wkalrm *alrm) > u32 alarm = rtt_readl(rtc, AR); > u32 offset; > > - offset = gpbr_readl(rtc); > + offset = at91_gpbr_read(rtc->gpbr); > if (offset == 0) > return -EILSEQ; > > @@ -192,7 +188,7 @@ static int at91_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm) > if (err != 0) > return err; > > - offset = gpbr_readl(rtc); > + offset = at91_gpbr_read(rtc->gpbr); > if (offset == 0) { > /* time is not set */ > return -EILSEQ; > @@ -291,17 +287,14 @@ static const struct rtc_class_ops at91_rtc_ops = { > */ > static int at91_rtc_probe(struct platform_device *pdev) > { > - struct resource *r, *r_gpbr; > + struct resource *r; > struct sam9_rtc *rtc; > int ret, irq; > u32 mr; > > r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - r_gpbr = platform_get_resource(pdev, IORESOURCE_MEM, 1); > - if (!r || !r_gpbr) { > - dev_err(&pdev->dev, "need 2 ressources\n"); > - return -ENODEV; > - } > + if (!r) > + return -ENXIO; > > irq = platform_get_irq(pdev, 0); > if (irq < 0) { > @@ -327,19 +320,14 @@ static int at91_rtc_probe(struct platform_device *pdev) > goto fail; > } > > - rtc->gpbr = ioremap(r_gpbr->start, resource_size(r_gpbr)); > - if (!rtc->gpbr) { > - dev_err(&pdev->dev, "failed to map gpbr registers, aborting.\n"); > - ret = -ENOMEM; > - goto fail_gpbr; > - } > + rtc->gpbr = CONFIG_RTC_DRV_AT91SAM9_GPBR; > > mr = rtt_readl(rtc, MR); > > /* unless RTT is counting at 1 Hz, re-initialize it */ > if ((mr & AT91_RTT_RTPRES) != AT91_SLOW_CLOCK) { > mr = AT91_RTT_RTTRST | (AT91_SLOW_CLOCK & AT91_RTT_RTPRES); > - gpbr_writel(rtc, 0); > + at91_gpbr_write(rtc->gpbr, 0); > } > > /* disable all interrupts (same as on shutdown path) */ > @@ -368,15 +356,13 @@ static int at91_rtc_probe(struct platform_device *pdev) > * clock, discrete RTC, etc > */ > > - if (gpbr_readl(rtc) == 0) > + if (at91_gpbr_read(rtc->gpbr) == 0) > dev_warn(&pdev->dev, "%s: SET TIME!\n", > dev_name(&rtc->rtcdev->dev)); > > return 0; > > fail_register: > - iounmap(rtc->gpbr); > -fail_gpbr: > iounmap(rtc->rtt); > fail: > platform_set_drvdata(pdev, NULL); > @@ -398,7 +384,6 @@ static int at91_rtc_remove(struct platform_device *pdev) > > rtc_device_unregister(rtc->rtcdev); > > - iounmap(rtc->gpbr); > iounmap(rtc->rtt); > platform_set_drvdata(pdev, NULL); > kfree(rtc); > -- > 1.8.1.5 >
On 17:12 Sun 07 Apr , Johan Hovold wrote: > Add arch device-tree setup code and DT-nodes for the General Purpose > Backup Registers (GPBR). > > Note that all AT91 SoCs but at91rm9200 have these backup-powered > registers. > > Signed-off-by: Johan Hovold <jhovold@gmail.com> > --- > .../devicetree/bindings/arm/atmel-at91.txt | 4 ++++ > arch/arm/boot/dts/at91sam9260.dtsi | 5 +++++ > arch/arm/boot/dts/at91sam9263.dtsi | 5 +++++ > arch/arm/boot/dts/at91sam9g45.dtsi | 5 +++++ > arch/arm/boot/dts/at91sam9n12.dtsi | 5 +++++ > arch/arm/boot/dts/at91sam9x5.dtsi | 5 +++++ > arch/arm/mach-at91/setup.c | 21 +++++++++++++++++++++ > 7 files changed, 50 insertions(+) > > diff --git a/Documentation/devicetree/bindings/arm/atmel-at91.txt b/Documentation/devicetree/bindings/arm/atmel-at91.txt > index 1196290..fe64d0a 100644 > --- a/Documentation/devicetree/bindings/arm/atmel-at91.txt > +++ b/Documentation/devicetree/bindings/arm/atmel-at91.txt > @@ -1,6 +1,10 @@ > Atmel AT91 device tree bindings. > ================================ > > +General Purpose Backup Registers (GPBR) required properties: > +- compatible: Should be "atmel,at91sam9260-gpbr" > +- reg: Should contain register location and length > + > PIT Timer required properties: > - compatible: Should be "atmel,at91sam9260-pit" > - reg: Should contain registers location and length > diff --git a/arch/arm/boot/dts/at91sam9260.dtsi b/arch/arm/boot/dts/at91sam9260.dtsi > index cb7bcc5..48ef04d 100644 > --- a/arch/arm/boot/dts/at91sam9260.dtsi > +++ b/arch/arm/boot/dts/at91sam9260.dtsi > @@ -87,6 +87,11 @@ > interrupts = <1 4 7>; > }; > > + gpbr@fffffd50 { > + compatible = "atmel,at91sam9260-gpbr"; > + reg = <0xfffffd50 0x10>; > + }; > + > tcb0: timer@fffa0000 { > compatible = "atmel,at91rm9200-tcb"; > reg = <0xfffa0000 0x100>; > diff --git a/arch/arm/boot/dts/at91sam9263.dtsi b/arch/arm/boot/dts/at91sam9263.dtsi > index 271d4de..e9676ba 100644 > --- a/arch/arm/boot/dts/at91sam9263.dtsi > +++ b/arch/arm/boot/dts/at91sam9263.dtsi > @@ -75,6 +75,11 @@ > interrupts = <1 4 7>; > }; > > + gpbr@fffffd60 { > + compatible = "atmel,at91sam9260-gpbr"; > + reg = <0xfffffd60 0x50>; > + }; > + > tcb0: timer@fff7c000 { > compatible = "atmel,at91rm9200-tcb"; > reg = <0xfff7c000 0x100>; > diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi > index 6b1d4ca..d5e37ce 100644 > --- a/arch/arm/boot/dts/at91sam9g45.dtsi > +++ b/arch/arm/boot/dts/at91sam9g45.dtsi > @@ -92,6 +92,11 @@ > reg = <0xfffffd10 0x10>; > }; > > + gpbr@fffffd60 { > + compatible = "atmel,at91sam9260-gpbr"; > + reg = <0xfffffd60 0x10>; > + }; > + > tcb0: timer@fff7c000 { > compatible = "atmel,at91rm9200-tcb"; > reg = <0xfff7c000 0x100>; > diff --git a/arch/arm/boot/dts/at91sam9n12.dtsi b/arch/arm/boot/dts/at91sam9n12.dtsi > index 4801717..ac122a6 100644 > --- a/arch/arm/boot/dts/at91sam9n12.dtsi > +++ b/arch/arm/boot/dts/at91sam9n12.dtsi > @@ -85,6 +85,11 @@ > reg = <0xfffffe10 0x10>; > }; > > + gpbr@fffffe60 { > + compatible = "atmel,at91sam9260-gpbr"; > + reg = <0xfffffe60 0x10>; > + }; > + > mmc0: mmc@f0008000 { > compatible = "atmel,hsmci"; > reg = <0xf0008000 0x600>; > diff --git a/arch/arm/boot/dts/at91sam9x5.dtsi b/arch/arm/boot/dts/at91sam9x5.dtsi > index c461e11..13d069e 100644 > --- a/arch/arm/boot/dts/at91sam9x5.dtsi > +++ b/arch/arm/boot/dts/at91sam9x5.dtsi > @@ -88,6 +88,11 @@ > interrupts = <1 4 7>; > }; > > + gpbr@fffffe60 { > + compatible = "atmel,at91sam9260-gpbr"; > + reg = <0xfffffe60 0x10>; > + }; > + > tcb0: timer@f8008000 { > compatible = "atmel,at91sam9x5-tcb"; > reg = <0xf8008000 0x100>; > diff --git a/arch/arm/mach-at91/setup.c b/arch/arm/mach-at91/setup.c > index b01f4aa..a525390 100644 > --- a/arch/arm/mach-at91/setup.c > +++ b/arch/arm/mach-at91/setup.c > @@ -374,6 +374,26 @@ static void at91_dt_ramc(void) > of_node_put(np); > } > > +static struct of_device_id gpbr_ids[] = { > + { .compatible = "atmel,at91sam9260-gpbr", }, > + { /* sentinel */ } > +}; > + > +static void at91_dt_gpbr(void) > +{ > + struct device_node *np; > + > + np = of_find_matching_node(NULL, gpbr_ids); > + if (!np) > + panic("AT91: failed to find compatible gpbr node in dtb\n"); > + > + at91_gpbr_base = of_iomap(np, 0); > + if (!at91_gpbr_base) > + panic("AT91: Failed to map gpbr registers\n"); > + > + of_node_put(np); > +} binding ok this NO no global pointer unless no choice we need a framework Best Regards, J. > + > static struct of_device_id shdwc_ids[] = { > { .compatible = "atmel,at91sam9260-shdwc", }, > { .compatible = "atmel,at91sam9rl-shdwc", }, > @@ -468,6 +488,7 @@ void __init at91_dt_initialize(void) > at91_dt_rstc(); > at91_dt_ramc(); > at91_dt_shdwc(); > + at91_dt_gpbr(); > > /* Init clock subsystem */ > at91_dt_clock_init(); > -- > 1.8.1.5 >
On 17:12 Sun 07 Apr , Johan Hovold wrote: > Add device-tree support. > > The AT91 RTT can be used as an RTC if the atmel,at91-rtt-as-rtc-gpbr > property is present and set to the general-purpose backup register to > use to store the RTC time base. > > Signed-off-by: Johan Hovold <jhovold@gmail.com> > --- > .../devicetree/bindings/rtc/rtc-at91sam9.txt | 19 ++++++++++++ > drivers/rtc/rtc-at91sam9.c | 36 +++++++++++++++++++++- > 2 files changed, 54 insertions(+), 1 deletion(-) > create mode 100644 Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt > > diff --git a/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt b/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt > new file mode 100644 > index 0000000..0f54988 > --- /dev/null > +++ b/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt > @@ -0,0 +1,19 @@ > +Atmel AT91 RTT as RTC > +===================== > + > +Required properties: > +- compatible: Should be "atmel,at91sam9260-rtt" > +- reg: Should contain register location and length > +- interrupts: Should contain interrupt for the RTT which is the IRQ line > + shared across all System Controller members. > +- atmel,rtt-as-rtc-gpbr: Should contain the backup-register to use to store > + the RTC time base > + > +Example: > + > +rtt@fffffd20 { > + compatible = "atmel,at91sam9g45-rtt", "atmel,at91sam9260-rtt"; > + reg = <0xfffffd20 0x10>; > + interrupts = <1 4 7>; > + atmel,at91-rtt-as-rtc-gpbr = <0>; no you miss the point of the DT you need to describe the hardware no a particular use of it the RTT is a general purpose timer backuped that we use in linux as a RTC with a gpbr to store the time you need 2 binding on for the RTT one the RTT-rtc > + }; > diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c > index 983667b..7ccce19 100644 > --- a/drivers/rtc/rtc-at91sam9.c > +++ b/drivers/rtc/rtc-at91sam9.c > @@ -13,6 +13,7 @@ > > #include <linux/module.h> > #include <linux/kernel.h> > +#include <linux/of.h> > #include <linux/platform_device.h> > #include <linux/time.h> > #include <linux/rtc.h> > @@ -282,6 +283,29 @@ static const struct rtc_class_ops at91_rtc_ops = { > .alarm_irq_enable = at91_rtc_alarm_irq_enable, > }; > > + > +#ifdef CONFIG_OF > +static struct of_device_id at91_rtc_of_match[] = { > + { .compatible = "atmel,at91sam9260-rtt", }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, at91_rtc_of_match); > +#endif > + > +static int at91_rtc_parse_dt(struct device_node *node, unsigned *gpbr) > +{ > + u32 val; > + int res; > + > + res = of_property_read_u32(node, "atmel,at91-rtt-as-rtc-gpbr", &val); > + if (!res) > + return -ENODEV; > + > + *gpbr = val; > + > + return 0; > +} > + > /* > * Initialize and install RTC driver > */ > @@ -291,6 +315,15 @@ static int at91_rtc_probe(struct platform_device *pdev) > struct sam9_rtc *rtc; > int ret, irq; > u32 mr; > + unsigned gpbr; > + > + if (pdev->dev.of_node) { > + ret = at91_rtc_parse_dt(pdev->dev.of_node, &gpbr); > + if (ret) > + return ret; > + } else { > + gpbr = CONFIG_RTC_DRV_AT91SAM9_GPBR; > + } > > r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (!r) > @@ -320,7 +353,7 @@ static int at91_rtc_probe(struct platform_device *pdev) > goto fail; > } > > - rtc->gpbr = CONFIG_RTC_DRV_AT91SAM9_GPBR; > + rtc->gpbr = gpbr; > > mr = rtt_readl(rtc, MR); > > @@ -455,6 +488,7 @@ static struct platform_driver at91_rtc_driver = { > .driver = { > .name = "rtc-at91sam9", > .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(at91_rtc_of_match), > }, > }; > > -- > 1.8.1.5 >
On Mon, Apr 08, 2013 at 09:33:29AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > On 17:12 Sun 07 Apr , Johan Hovold wrote: > > > > Add General Purpose Backup Register (GPBR) support. > > > > Most at91 SoCs have at least four 32-bit General Purpose Backup > > Registers (GPBR) powered by backup-power (VDDBU). One such register is > > currently used by rtc-at91sam9 driver to store the RTC time base. > > > > Make sure the registers are mapped by arch setup code and add generic > > accessors for in-kernel use. > > > > This is a step in adding device-tree support to the rtc-at91sam9 driver. > > > This is a regression > > we loose the tracking of what request and use the GPBR Implementing the GPBR accessors using a simple global resource was one quick way forward to decouple and generalise the GPBR. Of course, this could be turned into a more elaborate framework or driver where registers are requested and released by other drivers and possibly also by user-space. Thanks, Johan
On Mon, Apr 08, 2013 at 09:38:07AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > On 17:12 Sun 07 Apr , Johan Hovold wrote: > > Add device-tree support. > > > > The AT91 RTT can be used as an RTC if the atmel,at91-rtt-as-rtc-gpbr > > property is present and set to the general-purpose backup register to > > use to store the RTC time base. > > > > Signed-off-by: Johan Hovold <jhovold@gmail.com> > > --- > > .../devicetree/bindings/rtc/rtc-at91sam9.txt | 19 ++++++++++++ > > drivers/rtc/rtc-at91sam9.c | 36 +++++++++++++++++++++- > > 2 files changed, 54 insertions(+), 1 deletion(-) > > create mode 100644 Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt > > > > diff --git a/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt b/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt > > new file mode 100644 > > index 0000000..0f54988 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt > > @@ -0,0 +1,19 @@ > > +Atmel AT91 RTT as RTC > > +===================== > > + > > +Required properties: > > +- compatible: Should be "atmel,at91sam9260-rtt" > > +- reg: Should contain register location and length > > +- interrupts: Should contain interrupt for the RTT which is the IRQ line > > + shared across all System Controller members. > > +- atmel,rtt-as-rtc-gpbr: Should contain the backup-register to use to store > > + the RTC time base > > + > > +Example: > > + > > +rtt@fffffd20 { > > + compatible = "atmel,at91sam9g45-rtt", "atmel,at91sam9260-rtt"; > > + reg = <0xfffffd20 0x10>; > > + interrupts = <1 4 7>; > > + atmel,at91-rtt-as-rtc-gpbr = <0>; > no you miss the point of the DT > > you need to describe the hardware no a particular use of it That was what I was trying to achieve by adding the two use-neutral rtt and gpbr-nodes. But then the question is how would you influence which out of two rtt-drivers to use? Adding a property as above in the final board descriptions seemed preferable to adding rtt-as-rtc to the compatible string of the rtt as that would mean describing use rather than just hardware. > the RTT is a general purpose timer backuped that we use in linux as a > RTC with a gpbr to store the time > > you need 2 binding on for the RTT one the RTT-rtc As in adding some virtual hardware-node which uses the rtt and gpbr as resources? Thanks, Johan
On 04/08/2013 11:00 AM, Johan Hovold : > On Mon, Apr 08, 2013 at 09:38:07AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: >> On 17:12 Sun 07 Apr , Johan Hovold wrote: >>> Add device-tree support. >>> >>> The AT91 RTT can be used as an RTC if the atmel,at91-rtt-as-rtc-gpbr >>> property is present and set to the general-purpose backup register to >>> use to store the RTC time base. >>> >>> Signed-off-by: Johan Hovold <jhovold@gmail.com> >>> --- >>> .../devicetree/bindings/rtc/rtc-at91sam9.txt | 19 ++++++++++++ >>> drivers/rtc/rtc-at91sam9.c | 36 +++++++++++++++++++++- >>> 2 files changed, 54 insertions(+), 1 deletion(-) >>> create mode 100644 Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt >>> >>> diff --git a/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt b/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt >>> new file mode 100644 >>> index 0000000..0f54988 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt >>> @@ -0,0 +1,19 @@ >>> +Atmel AT91 RTT as RTC >>> +===================== >>> + >>> +Required properties: >>> +- compatible: Should be "atmel,at91sam9260-rtt" >>> +- reg: Should contain register location and length >>> +- interrupts: Should contain interrupt for the RTT which is the IRQ line >>> + shared across all System Controller members. >>> +- atmel,rtt-as-rtc-gpbr: Should contain the backup-register to use to store >>> + the RTC time base >>> + >>> +Example: >>> + >>> +rtt@fffffd20 { >>> + compatible = "atmel,at91sam9g45-rtt", "atmel,at91sam9260-rtt"; No, there is no visible difference between the sam9g45 RTT and the sam9260 one. So the most precise compatibility string is still sam9260. If one day we feel the need for a advanced feature that exists on a more recent SoC, we have the possibility to add it at that time... >>> + reg = <0xfffffd20 0x10>; >>> + interrupts = <1 4 7>; >>> + atmel,at91-rtt-as-rtc-gpbr = <0>; >> no you miss the point of the DT >> >> you need to describe the hardware no a particular use of it > > That was what I was trying to achieve by adding the two use-neutral > rtt and gpbr-nodes. But then the question is how would you influence > which out of two rtt-drivers to use? > > Adding a property as above in the final board descriptions seemed > preferable to adding rtt-as-rtc to the compatible string of the rtt as > that would mean describing use rather than just hardware. Well, re-reading the Device_Tree_Usage page, I found this sentence: " Understanding the compatible Property Every node in the tree that represents a device is required to have the compatible property. compatible is the key an operating system uses to decide which device driver to bind to a device. " or ePARP: " The compatible property value consists of one or more strings that define the specific programming model for the device. " We have the notion of link between hardware and software in this *compatible* sting, even if the *node* itself is about hardware description. >> the RTT is a general purpose timer backuped that we use in linux as a >> RTC with a gpbr to store the time >> >> you need 2 binding on for the RTT one the RTT-rtc > > As in adding some virtual hardware-node which uses the rtt and gpbr as > resources? So, why not simply having a compatibility string that collects the uses of this RTT node: compatible = "atmel,at91sam9260-rtt-as-rtc", "atmel,at91sam9260-rtt"; And then "decide which device driver to bind to [the RTT] device"... If the rtt-as-rtc driver is not selected, the device can still be used as a simple "rtt". The board .dts can overload a compatibility string according to the use, etc. Then the way do describe which GPBR to use has still to be discussed. But for the RTT itself, I would keep it simple like that. Best regards,
On 11:57 Mon 08 Apr , Nicolas Ferre wrote: > On 04/08/2013 11:00 AM, Johan Hovold : > > On Mon, Apr 08, 2013 at 09:38:07AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > >> On 17:12 Sun 07 Apr , Johan Hovold wrote: > >>> Add device-tree support. > >>> > >>> The AT91 RTT can be used as an RTC if the atmel,at91-rtt-as-rtc-gpbr > >>> property is present and set to the general-purpose backup register to > >>> use to store the RTC time base. > >>> > >>> Signed-off-by: Johan Hovold <jhovold@gmail.com> > >>> --- > >>> .../devicetree/bindings/rtc/rtc-at91sam9.txt | 19 ++++++++++++ > >>> drivers/rtc/rtc-at91sam9.c | 36 +++++++++++++++++++++- > >>> 2 files changed, 54 insertions(+), 1 deletion(-) > >>> create mode 100644 Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt > >>> > >>> diff --git a/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt b/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt > >>> new file mode 100644 > >>> index 0000000..0f54988 > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt > >>> @@ -0,0 +1,19 @@ > >>> +Atmel AT91 RTT as RTC > >>> +===================== > >>> + > >>> +Required properties: > >>> +- compatible: Should be "atmel,at91sam9260-rtt" > >>> +- reg: Should contain register location and length > >>> +- interrupts: Should contain interrupt for the RTT which is the IRQ line > >>> + shared across all System Controller members. > >>> +- atmel,rtt-as-rtc-gpbr: Should contain the backup-register to use to store > >>> + the RTC time base > >>> + > >>> +Example: > >>> + > >>> +rtt@fffffd20 { > >>> + compatible = "atmel,at91sam9g45-rtt", "atmel,at91sam9260-rtt"; > > No, there is no visible difference between the sam9g45 RTT and the > sam9260 one. So the most precise compatibility string is still sam9260. > If one day we feel the need for a advanced feature that exists on a more > recent SoC, we have the possibility to add it at that time... > > >>> + reg = <0xfffffd20 0x10>; > >>> + interrupts = <1 4 7>; > >>> + atmel,at91-rtt-as-rtc-gpbr = <0>; > >> no you miss the point of the DT > >> > >> you need to describe the hardware no a particular use of it > > > > That was what I was trying to achieve by adding the two use-neutral > > rtt and gpbr-nodes. But then the question is how would you influence > > which out of two rtt-drivers to use? > > > > Adding a property as above in the final board descriptions seemed > > preferable to adding rtt-as-rtc to the compatible string of the rtt as > > that would mean describing use rather than just hardware. > > Well, re-reading the Device_Tree_Usage page, I found this sentence: > " > Understanding the compatible Property > > Every node in the tree that represents a device is required to have the > compatible property. compatible is the key an operating system uses to > decide which device driver to bind to a device. > " > or ePARP: > > " > The compatible property value consists of one or more strings that > define the specific programming model for the device. > " > > We have the notion of link between hardware and software in this > *compatible* sting, even if the *node* itself is about hardware description. > > >> the RTT is a general purpose timer backuped that we use in linux as a > >> RTC with a gpbr to store the time > >> > >> you need 2 binding on for the RTT one the RTT-rtc > > > > As in adding some virtual hardware-node which uses the rtt and gpbr as > > resources? > > So, why not simply having a compatibility string that collects the uses > of this RTT node: > > compatible = "atmel,at91sam9260-rtt-as-rtc", "atmel,at91sam9260-rtt"; > > And then "decide which device driver to bind to [the RTT] device"... > If the rtt-as-rtc driver is not selected, the device can still be used > as a simple "rtt". The board .dts can overload a compatibility string > according to the use, etc. > > Then the way do describe which GPBR to use has still to be discussed. > But for the RTT itself, I would keep it simple like that. no as infact the rtc-at91sam9 should not even exist as this is much more generic we use a backped register and a timer to emulate a RTC this can be unsed by any one and I can use any backuped timer we need to have frameworks where the gpbr are tracked and the rtt for you describe the resources rtt0: rtt@fffffd20 { compatible = "atmel,at91sam9260-rtt"; reg = <0xfffffd20 0x10>; interrupts = <1 4 7>; }; rtc-timer { compatible = "linux,rtc-timer"; timer = <&rtt0>; backuped-register = <&gpbr 0>; }; this need to SoC implemetation generic
On 10:46 Mon 08 Apr , Johan Hovold wrote: > On Mon, Apr 08, 2013 at 09:33:29AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > On 17:12 Sun 07 Apr , Johan Hovold wrote: > > > > > > Add General Purpose Backup Register (GPBR) support. > > > > > > Most at91 SoCs have at least four 32-bit General Purpose Backup > > > Registers (GPBR) powered by backup-power (VDDBU). One such register is > > > currently used by rtc-at91sam9 driver to store the RTC time base. > > > > > > Make sure the registers are mapped by arch setup code and add generic > > > accessors for in-kernel use. > > > > > > This is a step in adding device-tree support to the rtc-at91sam9 driver. > > > > > This is a regression > > > > we loose the tracking of what request and use the GPBR > > Implementing the GPBR accessors using a simple global resource was one > quick way forward to decouple and generalise the GPBR. Of course, this > could be turned into a more elaborate framework or driver where > registers are requested and released by other drivers and possibly also > by user-space. yes must as we used on SAMA5 to pass the boot source from the firstage of the boot loader to the other software
On Mon, Apr 08, 2013 at 11:57:06AM +0200, Nicolas Ferre wrote: > On 04/08/2013 11:00 AM, Johan Hovold : > > On Mon, Apr 08, 2013 at 09:38:07AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > >> On 17:12 Sun 07 Apr , Johan Hovold wrote: > >>> Add device-tree support. > >>> > >>> The AT91 RTT can be used as an RTC if the atmel,at91-rtt-as-rtc-gpbr > >>> property is present and set to the general-purpose backup register to > >>> use to store the RTC time base. > >>> > >>> Signed-off-by: Johan Hovold <jhovold@gmail.com> > >>> --- > >>> .../devicetree/bindings/rtc/rtc-at91sam9.txt | 19 ++++++++++++ > >>> drivers/rtc/rtc-at91sam9.c | 36 +++++++++++++++++++++- > >>> 2 files changed, 54 insertions(+), 1 deletion(-) > >>> create mode 100644 Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt > >>> > >>> diff --git > >>> a/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt > >>> b/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt > >>> new > >>> file mode 100644 > >>> index 0000000..0f54988 > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt > >>> @@ -0,0 +1,19 @@ > >>> +Atmel AT91 RTT as RTC > >>> +===================== > >>> + > >>> +Required properties: > >>> +- compatible: Should be "atmel,at91sam9260-rtt" > >>> +- reg: Should contain register location and length > >>> +- interrupts: Should contain interrupt for the RTT which is the IRQ line > >>> + shared across all System Controller members. > >>> +- atmel,rtt-as-rtc-gpbr: Should contain the backup-register to use to store > >>> + the RTC time base > >>> + > >>> +Example: > >>> + > >>> +rtt@fffffd20 { > >>> + compatible = "atmel,at91sam9g45-rtt", "atmel,at91sam9260-rtt"; > > No, there is no visible difference between the sam9g45 RTT and the > sam9260 one. So the most precise compatibility string is still sam9260. > If one day we feel the need for a advanced feature that exists on a more > recent SoC, we have the possibility to add it at that time... Yes, this should be just "atmel,at91sam9260-rtt" to follow the current practise in AT91. However, as I mentioned in an earlier mail one could interpret "The first string in the list specifies the exact device that the node represents in the form "<manufacturer>,<model>". The following strings represent other devices that the device is compatible with. For example, the Freescale MPC8349 System on Chip (SoC) has a serial device which implements the National Semiconductor ns16550 register interface. The compatible property for the MPC8349 serial device should therefore be: compatible = "fsl,mpc8349-uart", "ns16550". In this case, fsl,mpc8349-uart specifies the exact device, and ns16550 states that it is register-level compatible with a National Semiconductor 16550 UART." http://www.devicetree.org/Device_Tree_Usage#Understanding_the_compatible_Property to mean that the compatible property should always be exact SoC-IP followed by the first (most generic) compatible one. > >>> + reg = <0xfffffd20 0x10>; > >>> + interrupts = <1 4 7>; > >>> + atmel,at91-rtt-as-rtc-gpbr = <0>; > >> no you miss the point of the DT > >> > >> you need to describe the hardware no a particular use of it > > > > That was what I was trying to achieve by adding the two use-neutral > > rtt and gpbr-nodes. But then the question is how would you influence > > which out of two rtt-drivers to use? > > > > Adding a property as above in the final board descriptions seemed > > preferable to adding rtt-as-rtc to the compatible string of the rtt as > > that would mean describing use rather than just hardware. > > Well, re-reading the Device_Tree_Usage page, I found this sentence: > " > Understanding the compatible Property > > Every node in the tree that represents a device is required to have the > compatible property. compatible is the key an operating system uses to > decide which device driver to bind to a device. > " > or ePARP: > > " > The compatible property value consists of one or more strings that > define the specific programming model for the device. > " > > We have the notion of link between hardware and software in this > *compatible* sting, even if the *node* itself is about hardware description. > > >> the RTT is a general purpose timer backuped that we use in linux as a > >> RTC with a gpbr to store the time > >> > >> you need 2 binding on for the RTT one the RTT-rtc > > > > As in adding some virtual hardware-node which uses the rtt and gpbr as > > resources? > > So, why not simply having a compatibility string that collects the uses > of this RTT node: > > compatible = "atmel,at91sam9260-rtt-as-rtc", "atmel,at91sam9260-rtt"; > > And then "decide which device driver to bind to [the RTT] device"... > If the rtt-as-rtc driver is not selected, the device can still be used > as a simple "rtt". The board .dts can overload a compatibility string > according to the use, etc. > > Then the way do describe which GPBR to use has still to be discussed. > But for the RTT itself, I would keep it simple like that. Yes, this would also work but it just appears to me that it would violate the above mentioned description of the compatible property: "The first string in the list specifies the exact device that the node represents in the form "<manufacturer>,<model>". The following strings represent other devices that the device is compatible with." Thanks, Johan
On 04/08/2013 12:03 PM, Jean-Christophe PLAGNIOL-VILLARD : > On 11:57 Mon 08 Apr , Nicolas Ferre wrote: >> On 04/08/2013 11:00 AM, Johan Hovold : >>> On Mon, Apr 08, 2013 at 09:38:07AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: >>>> On 17:12 Sun 07 Apr , Johan Hovold wrote: >>>>> Add device-tree support. >>>>> >>>>> The AT91 RTT can be used as an RTC if the atmel,at91-rtt-as-rtc-gpbr >>>>> property is present and set to the general-purpose backup register to >>>>> use to store the RTC time base. >>>>> >>>>> Signed-off-by: Johan Hovold <jhovold@gmail.com> >>>>> --- >>>>> .../devicetree/bindings/rtc/rtc-at91sam9.txt | 19 ++++++++++++ >>>>> drivers/rtc/rtc-at91sam9.c | 36 +++++++++++++++++++++- >>>>> 2 files changed, 54 insertions(+), 1 deletion(-) >>>>> create mode 100644 Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt b/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt >>>>> new file mode 100644 >>>>> index 0000000..0f54988 >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt >>>>> @@ -0,0 +1,19 @@ >>>>> +Atmel AT91 RTT as RTC >>>>> +===================== >>>>> + >>>>> +Required properties: >>>>> +- compatible: Should be "atmel,at91sam9260-rtt" >>>>> +- reg: Should contain register location and length >>>>> +- interrupts: Should contain interrupt for the RTT which is the IRQ line >>>>> + shared across all System Controller members. >>>>> +- atmel,rtt-as-rtc-gpbr: Should contain the backup-register to use to store >>>>> + the RTC time base >>>>> + >>>>> +Example: >>>>> + >>>>> +rtt@fffffd20 { >>>>> + compatible = "atmel,at91sam9g45-rtt", "atmel,at91sam9260-rtt"; >> >> No, there is no visible difference between the sam9g45 RTT and the >> sam9260 one. So the most precise compatibility string is still sam9260. >> If one day we feel the need for a advanced feature that exists on a more >> recent SoC, we have the possibility to add it at that time... >> >>>>> + reg = <0xfffffd20 0x10>; >>>>> + interrupts = <1 4 7>; >>>>> + atmel,at91-rtt-as-rtc-gpbr = <0>; >>>> no you miss the point of the DT >>>> >>>> you need to describe the hardware no a particular use of it >>> >>> That was what I was trying to achieve by adding the two use-neutral >>> rtt and gpbr-nodes. But then the question is how would you influence >>> which out of two rtt-drivers to use? >>> >>> Adding a property as above in the final board descriptions seemed >>> preferable to adding rtt-as-rtc to the compatible string of the rtt as >>> that would mean describing use rather than just hardware. >> >> Well, re-reading the Device_Tree_Usage page, I found this sentence: >> " >> Understanding the compatible Property >> >> Every node in the tree that represents a device is required to have the >> compatible property. compatible is the key an operating system uses to >> decide which device driver to bind to a device. >> " >> or ePARP: >> >> " >> The compatible property value consists of one or more strings that >> define the specific programming model for the device. >> " >> >> We have the notion of link between hardware and software in this >> *compatible* sting, even if the *node* itself is about hardware description. >> >>>> the RTT is a general purpose timer backuped that we use in linux as a >>>> RTC with a gpbr to store the time >>>> >>>> you need 2 binding on for the RTT one the RTT-rtc >>> >>> As in adding some virtual hardware-node which uses the rtt and gpbr as >>> resources? >> >> So, why not simply having a compatibility string that collects the uses >> of this RTT node: >> >> compatible = "atmel,at91sam9260-rtt-as-rtc", "atmel,at91sam9260-rtt"; >> >> And then "decide which device driver to bind to [the RTT] device"... >> If the rtt-as-rtc driver is not selected, the device can still be used >> as a simple "rtt". The board .dts can overload a compatibility string >> according to the use, etc. >> >> Then the way do describe which GPBR to use has still to be discussed. >> But for the RTT itself, I would keep it simple like that. > > no as infact the rtc-at91sam9 should not even exist > as this is much more generic > > we use a backped register and a timer to emulate a RTC this can be unsed by > any one > > and I can use any backuped timer > > we need to have frameworks Is it possible to focus on the current problem instead of calling for the creation for Yet Another Framework? > where the gpbr are tracked and the rtt > > for you describe the resources > > rtt0: rtt@fffffd20 { > compatible = "atmel,at91sam9260-rtt"; > reg = <0xfffffd20 0x10>; > interrupts = <1 4 7>; > }; > > rtc-timer { > compatible = "linux,rtc-timer"; > timer = <&rtt0>; > backuped-register = <&gpbr 0>; > }; > > this need to SoC implemetation generic OMG, so well, we will have to re-write the whole rtc-at91sam9 now??!! Oh yes, I see, we do not have more urgent things to do... Please stop trying re-inventing things that will do everything but solving the real issues: why not agree on a sort term solution about this rtt/rtc thing: I have *never* heard complains about this precise driver. And we must concentrate now on problem solving. Bye,
On Mon, Apr 08, 2013 at 12:03:01PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > On 11:57 Mon 08 Apr , Nicolas Ferre wrote: > > On 04/08/2013 11:00 AM, Johan Hovold : > > > On Mon, Apr 08, 2013 at 09:38:07AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > >> On 17:12 Sun 07 Apr , Johan Hovold wrote: [...] > > >> the RTT is a general purpose timer backuped that we use in linux as a > > >> RTC with a gpbr to store the time > > >> > > >> you need 2 binding on for the RTT one the RTT-rtc > > > > > > As in adding some virtual hardware-node which uses the rtt and gpbr as > > > resources? > > > > So, why not simply having a compatibility string that collects the uses > > of this RTT node: > > > > compatible = "atmel,at91sam9260-rtt-as-rtc", "atmel,at91sam9260-rtt"; > > > > And then "decide which device driver to bind to [the RTT] device"... > > If the rtt-as-rtc driver is not selected, the device can still be used > > as a simple "rtt". The board .dts can overload a compatibility string > > according to the use, etc. > > > > Then the way do describe which GPBR to use has still to be discussed. > > But for the RTT itself, I would keep it simple like that. > > no as infact the rtc-at91sam9 should not even exist > as this is much more generic > > we use a backped register and a timer to emulate a RTC this can be unsed by > any one > > and I can use any backuped timer > > we need to have frameworks > where the gpbr are tracked and the rtt > > for you describe the resources > > rtt0: rtt@fffffd20 { > compatible = "atmel,at91sam9260-rtt"; > reg = <0xfffffd20 0x10>; > interrupts = <1 4 7>; > }; > > rtc-timer { > compatible = "linux,rtc-timer"; > timer = <&rtt0>; > backuped-register = <&gpbr 0>; > }; > > this need to SoC implemetation generic This would indeed be a neat solution, albeit one that requires a lot work to be realised as both a backup-register and backed-up-timer framework would need to be implemented. Thanks, Johan
On 12:42 Mon 08 Apr , Nicolas Ferre wrote: > On 04/08/2013 12:03 PM, Jean-Christophe PLAGNIOL-VILLARD : > > On 11:57 Mon 08 Apr , Nicolas Ferre wrote: > >> On 04/08/2013 11:00 AM, Johan Hovold : > >>> On Mon, Apr 08, 2013 at 09:38:07AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > >>>> On 17:12 Sun 07 Apr , Johan Hovold wrote: > >>>>> Add device-tree support. > >>>>> > >>>>> The AT91 RTT can be used as an RTC if the atmel,at91-rtt-as-rtc-gpbr > >>>>> property is present and set to the general-purpose backup register to > >>>>> use to store the RTC time base. > >>>>> > >>>>> Signed-off-by: Johan Hovold <jhovold@gmail.com> > >>>>> --- > >>>>> .../devicetree/bindings/rtc/rtc-at91sam9.txt | 19 ++++++++++++ > >>>>> drivers/rtc/rtc-at91sam9.c | 36 +++++++++++++++++++++- > >>>>> 2 files changed, 54 insertions(+), 1 deletion(-) > >>>>> create mode 100644 Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt > >>>>> > >>>>> diff --git a/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt b/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt > >>>>> new file mode 100644 > >>>>> index 0000000..0f54988 > >>>>> --- /dev/null > >>>>> +++ b/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt > >>>>> @@ -0,0 +1,19 @@ > >>>>> +Atmel AT91 RTT as RTC > >>>>> +===================== > >>>>> + > >>>>> +Required properties: > >>>>> +- compatible: Should be "atmel,at91sam9260-rtt" > >>>>> +- reg: Should contain register location and length > >>>>> +- interrupts: Should contain interrupt for the RTT which is the IRQ line > >>>>> + shared across all System Controller members. > >>>>> +- atmel,rtt-as-rtc-gpbr: Should contain the backup-register to use to store > >>>>> + the RTC time base > >>>>> + > >>>>> +Example: > >>>>> + > >>>>> +rtt@fffffd20 { > >>>>> + compatible = "atmel,at91sam9g45-rtt", "atmel,at91sam9260-rtt"; > >> > >> No, there is no visible difference between the sam9g45 RTT and the > >> sam9260 one. So the most precise compatibility string is still sam9260. > >> If one day we feel the need for a advanced feature that exists on a more > >> recent SoC, we have the possibility to add it at that time... > >> > >>>>> + reg = <0xfffffd20 0x10>; > >>>>> + interrupts = <1 4 7>; > >>>>> + atmel,at91-rtt-as-rtc-gpbr = <0>; > >>>> no you miss the point of the DT > >>>> > >>>> you need to describe the hardware no a particular use of it > >>> > >>> That was what I was trying to achieve by adding the two use-neutral > >>> rtt and gpbr-nodes. But then the question is how would you influence > >>> which out of two rtt-drivers to use? > >>> > >>> Adding a property as above in the final board descriptions seemed > >>> preferable to adding rtt-as-rtc to the compatible string of the rtt as > >>> that would mean describing use rather than just hardware. > >> > >> Well, re-reading the Device_Tree_Usage page, I found this sentence: > >> " > >> Understanding the compatible Property > >> > >> Every node in the tree that represents a device is required to have the > >> compatible property. compatible is the key an operating system uses to > >> decide which device driver to bind to a device. > >> " > >> or ePARP: > >> > >> " > >> The compatible property value consists of one or more strings that > >> define the specific programming model for the device. > >> " > >> > >> We have the notion of link between hardware and software in this > >> *compatible* sting, even if the *node* itself is about hardware description. > >> > >>>> the RTT is a general purpose timer backuped that we use in linux as a > >>>> RTC with a gpbr to store the time > >>>> > >>>> you need 2 binding on for the RTT one the RTT-rtc > >>> > >>> As in adding some virtual hardware-node which uses the rtt and gpbr as > >>> resources? > >> > >> So, why not simply having a compatibility string that collects the uses > >> of this RTT node: > >> > >> compatible = "atmel,at91sam9260-rtt-as-rtc", "atmel,at91sam9260-rtt"; > >> > >> And then "decide which device driver to bind to [the RTT] device"... > >> If the rtt-as-rtc driver is not selected, the device can still be used > >> as a simple "rtt". The board .dts can overload a compatibility string > >> according to the use, etc. > >> > >> Then the way do describe which GPBR to use has still to be discussed. > >> But for the RTT itself, I would keep it simple like that. > > > > no as infact the rtc-at91sam9 should not even exist > > as this is much more generic > > > > we use a backped register and a timer to emulate a RTC this can be unsed by > > any one > > > > and I can use any backuped timer > > > > we need to have frameworks > > Is it possible to focus on the current problem instead of calling for > the creation for Yet Another Framework? > > > where the gpbr are tracked and the rtt > > > > for you describe the resources > > > > rtt0: rtt@fffffd20 { > > compatible = "atmel,at91sam9260-rtt"; > > reg = <0xfffffd20 0x10>; > > interrupts = <1 4 7>; > > }; > > > > rtc-timer { > > compatible = "linux,rtc-timer"; > > timer = <&rtt0>; > > backuped-register = <&gpbr 0>; > > }; > > > > this need to SoC implemetation generic > > OMG, so well, we will have to re-write the whole rtc-at91sam9 now??!! Oh > yes, I see, we do not have more urgent things to do... > > Please stop trying re-inventing things that will do everything but > solving the real issues: why not agree on a sort term solution about > this rtt/rtc thing: I have *never* heard complains about this precise > driver. And we must concentrate now on problem solving. no the current driver is badly design this need to be cleanup switch to DT does mean do the stuff CLEAN no quirk hack we did the same for pinctrl/dma/fb. This need to be done correctly DT is to describe hardware not software implentation Best Regards, J.
On 12:48 Mon 08 Apr , Johan Hovold wrote: > On Mon, Apr 08, 2013 at 12:03:01PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > On 11:57 Mon 08 Apr , Nicolas Ferre wrote: > > > On 04/08/2013 11:00 AM, Johan Hovold : > > > > On Mon, Apr 08, 2013 at 09:38:07AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > > >> On 17:12 Sun 07 Apr , Johan Hovold wrote: > > [...] > > > > >> the RTT is a general purpose timer backuped that we use in linux as a > > > >> RTC with a gpbr to store the time > > > >> > > > >> you need 2 binding on for the RTT one the RTT-rtc > > > > > > > > As in adding some virtual hardware-node which uses the rtt and gpbr as > > > > resources? > > > > > > So, why not simply having a compatibility string that collects the uses > > > of this RTT node: > > > > > > compatible = "atmel,at91sam9260-rtt-as-rtc", "atmel,at91sam9260-rtt"; > > > > > > And then "decide which device driver to bind to [the RTT] device"... > > > If the rtt-as-rtc driver is not selected, the device can still be used > > > as a simple "rtt". The board .dts can overload a compatibility string > > > according to the use, etc. > > > > > > Then the way do describe which GPBR to use has still to be discussed. > > > But for the RTT itself, I would keep it simple like that. > > > > no as infact the rtc-at91sam9 should not even exist > > as this is much more generic > > > > we use a backped register and a timer to emulate a RTC this can be unsed by > > any one > > > > and I can use any backuped timer > > > > we need to have frameworks > > where the gpbr are tracked and the rtt > > > > for you describe the resources > > > > rtt0: rtt@fffffd20 { > > compatible = "atmel,at91sam9260-rtt"; > > reg = <0xfffffd20 0x10>; > > interrupts = <1 4 7>; > > }; > > > > rtc-timer { > > compatible = "linux,rtc-timer"; > > timer = <&rtt0>; > > backuped-register = <&gpbr 0>; > > }; > > > > this need to SoC implemetation generic > > This would indeed be a neat solution, albeit one that requires a lot > work to be realised as both a backup-register and backed-up-timer > framework would need to be implemented. not that much for backuped register or rtt a simple list with spinlock will be enougth and after just handle the phandle it's not that complex but if we want to describe the hardware correctly this is needed And this will not be AT91 specific but SoC generic Ensure the GPBR are requested once is mandatory for any bug detection a global ioremap is really dirty and need to be used when only NO other choice Best Regards, J. > > Bye, > -- > Nicolas Ferre > > Thanks, > Johan
On 12:38 Mon 08 Apr , Johan Hovold wrote: > On Mon, Apr 08, 2013 at 11:57:06AM +0200, Nicolas Ferre wrote: > > On 04/08/2013 11:00 AM, Johan Hovold : > > > On Mon, Apr 08, 2013 at 09:38:07AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > >> On 17:12 Sun 07 Apr , Johan Hovold wrote: > > >>> Add device-tree support. > > >>> > > >>> The AT91 RTT can be used as an RTC if the atmel,at91-rtt-as-rtc-gpbr > > >>> property is present and set to the general-purpose backup register to > > >>> use to store the RTC time base. > > >>> > > >>> Signed-off-by: Johan Hovold <jhovold@gmail.com> > > >>> --- > > >>> .../devicetree/bindings/rtc/rtc-at91sam9.txt | 19 ++++++++++++ > > >>> drivers/rtc/rtc-at91sam9.c | 36 +++++++++++++++++++++- > > >>> 2 files changed, 54 insertions(+), 1 deletion(-) > > >>> create mode 100644 Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt > >>> > > >>> diff --git > > >>> a/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt > > >>> b/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt > >>> new > > >>> file mode 100644 > > >>> index 0000000..0f54988 > > >>> --- /dev/null > > >>> +++ b/Documentation/devicetree/bindings/rtc/rtc-at91sam9.txt > > >>> @@ -0,0 +1,19 @@ > > >>> +Atmel AT91 RTT as RTC > > >>> +===================== > > >>> + > > >>> +Required properties: > > >>> +- compatible: Should be "atmel,at91sam9260-rtt" > > >>> +- reg: Should contain register location and length > > >>> +- interrupts: Should contain interrupt for the RTT which is the IRQ line > > >>> + shared across all System Controller members. > > >>> +- atmel,rtt-as-rtc-gpbr: Should contain the backup-register to use to store > > >>> + the RTC time base > > >>> + > > >>> +Example: > > >>> + > > >>> +rtt@fffffd20 { > > >>> + compatible = "atmel,at91sam9g45-rtt", "atmel,at91sam9260-rtt"; > > > > No, there is no visible difference between the sam9g45 RTT and the > > sam9260 one. So the most precise compatibility string is still sam9260. > > If one day we feel the need for a advanced feature that exists on a more > > recent SoC, we have the possibility to add it at that time... > > Yes, this should be just "atmel,at91sam9260-rtt" to follow the current > practise in AT91. However, as I mentioned in an earlier mail one could > interpret > > "The first string in the list specifies the exact device that > the node represents in the form "<manufacturer>,<model>". The > following strings represent other devices that the device is > compatible with. > > For example, the Freescale MPC8349 System on Chip (SoC) has a > serial device which implements the National Semiconductor > ns16550 register interface. The compatible property for the > MPC8349 serial device should therefore be: compatible = > "fsl,mpc8349-uart", "ns16550". In this case, fsl,mpc8349-uart > specifies the exact device, and ns16550 states that it is > register-level compatible with a National Semiconductor 16550 > UART." > > http://www.devicetree.org/Device_Tree_Usage#Understanding_the_compatible_Property > > to mean that the compatible property should always be exact SoC-IP > followed by the first (most generic) compatible one. here you describe compatible IP not drivers implementation they are both usuart IP that are compatible at IP level. Best Regards, J.
On Mon, Apr 08, 2013 at 12:04:17PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > On 10:46 Mon 08 Apr , Johan Hovold wrote: > > On Mon, Apr 08, 2013 at 09:33:29AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > > On 17:12 Sun 07 Apr , Johan Hovold wrote: > > > > > > > > Add General Purpose Backup Register (GPBR) support. > > > > > > > > Most at91 SoCs have at least four 32-bit General Purpose Backup > > > > Registers (GPBR) powered by backup-power (VDDBU). One such register is > > > > currently used by rtc-at91sam9 driver to store the RTC time base. > > > > > > > > Make sure the registers are mapped by arch setup code and add generic > > > > accessors for in-kernel use. > > > > > > > > This is a step in adding device-tree support to the rtc-at91sam9 driver. > > > > > > > This is a regression > > > > > > we loose the tracking of what request and use the GPBR > > > > Implementing the GPBR accessors using a simple global resource was one > > quick way forward to decouple and generalise the GPBR. Of course, this > > could be turned into a more elaborate framework or driver where > > registers are requested and released by other drivers and possibly also > > by user-space. > yes must as we used on SAMA5 to pass the boot source from the firstage > of the boot loader to the other software Could you please elaborate on this; how is the GPBR used, how many registers, and when is it accessed? Is there already code in at91/at91-3.10-soc or are you referring to a userspace consumer? Thanks, Johan
On Mon, Apr 08, 2013 at 01:11:13PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > On 12:38 Mon 08 Apr , Johan Hovold wrote: > > On Mon, Apr 08, 2013 at 11:57:06AM +0200, Nicolas Ferre wrote: > > > On 04/08/2013 11:00 AM, Johan Hovold : > > > > On Mon, Apr 08, 2013 at 09:38:07AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > > >> On 17:12 Sun 07 Apr , Johan Hovold wrote: [...] > > > >>> +rtt@fffffd20 { > > > >>> + compatible = "atmel,at91sam9g45-rtt", "atmel,at91sam9260-rtt"; > > > > > > No, there is no visible difference between the sam9g45 RTT and the > > > sam9260 one. So the most precise compatibility string is still sam9260. > > > If one day we feel the need for a advanced feature that exists on a more > > > recent SoC, we have the possibility to add it at that time... > > > > Yes, this should be just "atmel,at91sam9260-rtt" to follow the current > > practise in AT91. However, as I mentioned in an earlier mail one could > > interpret > > > > "The first string in the list specifies the exact device that > > the node represents in the form "<manufacturer>,<model>". The > > following strings represent other devices that the device is > > compatible with. > > > > For example, the Freescale MPC8349 System on Chip (SoC) has a > > serial device which implements the National Semiconductor > > ns16550 register interface. The compatible property for the > > MPC8349 serial device should therefore be: compatible = > > "fsl,mpc8349-uart", "ns16550". In this case, fsl,mpc8349-uart > > specifies the exact device, and ns16550 states that it is > > register-level compatible with a National Semiconductor 16550 > > UART." > > > > http://www.devicetree.org/Device_Tree_Usage#Understanding_the_compatible_Property > > > > to mean that the compatible property should always be exact SoC-IP > > followed by the first (most generic) compatible one. > > here you describe compatible IP not drivers implementation I'm referring to the IP and not drivers. > they are both usuart IP that are compatible at IP level. Fair enough, I see how this can be considered to be in accordance with the above quotation. Johan
On 14:39 Thu 11 Apr , Johan Hovold wrote: > On Mon, Apr 08, 2013 at 12:04:17PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > On 10:46 Mon 08 Apr , Johan Hovold wrote: > > > On Mon, Apr 08, 2013 at 09:33:29AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote: > > > > On 17:12 Sun 07 Apr , Johan Hovold wrote: > > > > > > > > > > Add General Purpose Backup Register (GPBR) support. > > > > > > > > > > Most at91 SoCs have at least four 32-bit General Purpose Backup > > > > > Registers (GPBR) powered by backup-power (VDDBU). One such register is > > > > > currently used by rtc-at91sam9 driver to store the RTC time base. > > > > > > > > > > Make sure the registers are mapped by arch setup code and add generic > > > > > accessors for in-kernel use. > > > > > > > > > > This is a step in adding device-tree support to the rtc-at91sam9 driver. > > > > > > > > > This is a regression > > > > > > > > we loose the tracking of what request and use the GPBR > > > > > > Implementing the GPBR accessors using a simple global resource was one > > > quick way forward to decouple and generalise the GPBR. Of course, this > > > could be turned into a more elaborate framework or driver where > > > registers are requested and released by other drivers and possibly also > > > by user-space. > > yes must as we used on SAMA5 to pass the boot source from the firstage > > of the boot loader to the other software > > Could you please elaborate on this; how is the GPBR used, how many > registers, and when is it accessed? The second stage bootloader store the content of the boot mode passs via r4 from the Rom-Code in the 2nd gpbr > > Is there already code in at91/at91-3.10-soc or are you referring to > a userspace consumer? no user you can see the code in barebox you can see the code of barebox here http://git.linupdate.org/git?p=barebox.git;a=shortlog;h=refs/heads/delivery/sama5-release-bootmode Best Regards, J. > > Thanks, > Johan
diff --git a/drivers/rtc/rtc-at91sam9.c b/drivers/rtc/rtc-at91sam9.c index 39cfd2e..69e9389 100644 --- a/drivers/rtc/rtc-at91sam9.c +++ b/drivers/rtc/rtc-at91sam9.c @@ -20,6 +20,8 @@ #include <linux/ioctl.h> #include <linux/slab.h> #include <linux/platform_data/atmel.h> +#include <linux/of.h> +#include <linux/of_device.h> #include <mach/at91_rtt.h> #include <mach/cpu.h> @@ -71,6 +73,17 @@ struct sam9_rtc { #define gpbr_writel(rtc, val) \ __raw_writel((val), (rtc)->gpbr) +#ifdef CONFIG_OF +static const struct of_device_id at91sam9_rtc_dt_ids[] = { + { .compatible = "atmel,at91sam9-rtc" }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, at91sam9_rtc_dt_ids); +#else +#define at91sam9_rtc_dt_ids NULL +#endif /* CONFIG_OF */ + + /* * Read current time and date in RTC */ @@ -470,6 +483,7 @@ static struct platform_driver at91_rtc_driver = { .driver = { .name = "rtc-at91sam9", .owner = THIS_MODULE, + .of_match_table = of_match_ptr(at91sam9_rtc_dt_ids), }, };
Some members of the at91 SoCs use the Real Time Timer (RTT) and the General Purpose Backup Registers (GPBR) to implement a real time clock (RTC). The AT91SAM9G20 is one example. Attached is a patch to add DT support to rtc-at91sam9.c . The patch is against lk 3.9.0-rc5 . Below is a snippet of DT code for the 'G20 that was observed to work with this patch: ahb { apb { rtc { compatible = "atmel,at91sam9-rtc"; /* RTTC followed by GPBR (backup registers) */ reg = <0xfffffd20 0x10>, <0xfffffd50 0x10>; interrupts = <1 4 7>; status = "okay"; }; } }; Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>