| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 6 Dec 2012 09:28:36 +0800
From: Chao Xie <xiechao.mail@...il.com>
To: Haojian Zhuang <haojian.zhuang@...il.com>
Cc: Chao Xie <chao.xie@...vell.com>,
Linux Russell <linux@....linux.org.uk>,
Alessandro Zummo <a.zummo@...ertech.it>,
"rtc-linux@...glegroups.com" <rtc-linux@...glegroups.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
Robert Jarzmik <robert.jarzmik@...e.fr>
Subject: Re: [V2 PATCH 2/2] rtc: pxa: add pxa95x rtc support
On Wed, Dec 5, 2012 at 3:04 PM, Haojian Zhuang <haojian.zhuang@...il.com> wrote:
> On Wed, Dec 5, 2012 at 2:49 PM, Chao Xie <chao.xie@...vell.com> wrote:
>> the pxa95x rtc need access PBSR register before write to
>> RTTR, RCNR, RDCR, and RYCR registers.
>>
>> Signed-off-by: Chao Xie <chao.xie@...vell.com>
>> ---
>> drivers/rtc/rtc-pxa.c | 100 +++++++++++++++++++++++++++++++++++++++++-------
>> 1 files changed, 85 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-pxa.c b/drivers/rtc/rtc-pxa.c
>> index f771b2e..aee23cb 100644
>> --- a/drivers/rtc/rtc-pxa.c
>> +++ b/drivers/rtc/rtc-pxa.c
>> @@ -29,6 +29,7 @@
>> #include <linux/slab.h>
>> #include <linux/of.h>
>> #include <linux/of_device.h>
>> +#include <linux/delay.h>
>>
>> #include <mach/hardware.h>
>>
>> @@ -77,14 +78,28 @@
>> #define RTCPICR 0x34
>> #define PIAR 0x38
>>
>> +#define PSBR_RTC 0x00
>> +
>> #define rtc_readl(pxa_rtc, reg) \
>> __raw_readl((pxa_rtc)->base + (reg))
>> #define rtc_writel(pxa_rtc, reg, value) \
>> __raw_writel((value), (pxa_rtc)->base + (reg))
>> +#define rtc_readl_psbr(pxa_rtc, reg) \
>> + __raw_readl((pxa_rtc)->base_psbr + (reg))
>> +#define rtc_writel_psbr(pxa_rtc, reg, value) \
>> + __raw_writel((value), (pxa_rtc)->base_psbr + (reg))
>> +
>> +enum {
>> + RTC_PXA27X,
>> + RTC_PXA95X,
>> +};
>>
>> struct pxa_rtc {
>> struct resource *ress;
>> + struct resource *ress_psbr;
>> + unsigned int id;
>> void __iomem *base;
>> + void __iomem *base_psbr;
>> int irq_1Hz;
>> int irq_Alrm;
>> struct rtc_device *rtc;
>> @@ -242,9 +257,26 @@ static int pxa_rtc_set_time(struct device *dev, struct rtc_time *tm)
>> {
>> struct pxa_rtc *pxa_rtc = dev_get_drvdata(dev);
>>
>> + /*
>> + * sequence to wirte pxa rtc register RCNR RDCR RYCR is
>> + * 1. set PSBR[RWE] bit, take 2x32-khz to complete
>> + * 2. write to RTC register,take 2x32-khz to complete
>> + * 3. clear PSBR[RWE] bit,take 2x32-khz to complete
>> + */
>> + if (pxa_rtc->id == RTC_PXA95X) {
>> + rtc_writel_psbr(pxa_rtc, PSBR_RTC, 0x01);
>> + udelay(100);
>> + }
>
> How about define PSBP operation as a new clock, rtc interface clock.
> Then you could put
> the enable/disable into clock framework. And you only need to check
> whether the interface
> clock is NULL or not at here. If it's available, you can call
> clk_prepare_enable().
>
>> +
>> rtc_writel(pxa_rtc, RYCR, ryxr_calc(tm));
>> rtc_writel(pxa_rtc, RDCR, rdxr_calc(tm));
>>
>> + if (pxa_rtc->id == RTC_PXA95X) {
>> + udelay(100);
>> + rtc_writel_psbr(pxa_rtc, PSBR_RTC, 0x00);
>> + udelay(100);
>> + }
>> +
>> return 0;
>> }
>>
>> @@ -310,6 +342,20 @@ static const struct rtc_class_ops pxa_rtc_ops = {
>> .proc = pxa_rtc_proc,
>> };
>>
>> +static struct of_device_id pxa_rtc_dt_ids[] = {
>> + { .compatible = "marvell,pxa-rtc", .data = (void *)RTC_PXA27X },
>> + { .compatible = "marvell,pxa95x-rtc", .data = (void *)RTC_PXA95X },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, pxa_rtc_dt_ids);
>> +
>> +static const struct platform_device_id pxa_rtc_id_table[] = {
>> + { "pxa-rtc", RTC_PXA27X },
>> + { "pxa95x-rtc", RTC_PXA95X },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(platform, pxa_rtc_id_table);
>> +
>> static int __init pxa_rtc_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> @@ -324,13 +370,34 @@ static int __init pxa_rtc_probe(struct platform_device *pdev)
>> spin_lock_init(&pxa_rtc->lock);
>> platform_set_drvdata(pdev, pxa_rtc);
>>
>> + if (pdev->dev.of_node) {
>> + const struct of_device_id *of_id =
>> + of_match_device(pxa_rtc_dt_ids, &pdev->dev);
>> +
>> + pxa_rtc->id = (unsigned int)(of_id->data);
>> + } else {
>> + const struct platform_device_id *id =
>> + platform_get_device_id(pdev);
>> +
>> + pxa_rtc->id = id->driver_data;
>> + }
>> +
>> ret = -ENXIO;
>> pxa_rtc->ress = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> if (!pxa_rtc->ress) {
>> - dev_err(dev, "No I/O memory resource defined\n");
>> + dev_err(dev, "No I/O memory resource(id=0) defined\n");
>> goto err_ress;
>> }
>>
>> + if (pxa_rtc->id == RTC_PXA95X) {
>> + pxa_rtc->ress_psbr =
>> + platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> + if (!pxa_rtc->ress_psbr) {
>> + dev_err(dev, "No I/O memory resource(id=1) defined\n");
>> + goto err_ress;
>> + }
>> + }
>> +
>> pxa_rtc->irq_1Hz = platform_get_irq(pdev, 0);
>> if (pxa_rtc->irq_1Hz < 0) {
>> dev_err(dev, "No 1Hz IRQ resource defined\n");
>> @@ -347,7 +414,17 @@ static int __init pxa_rtc_probe(struct platform_device *pdev)
>> resource_size(pxa_rtc->ress));
>> if (!pxa_rtc->base) {
>> dev_err(&pdev->dev, "Unable to map pxa RTC I/O memory\n");
>> - goto err_map;
>> + goto err_map_base;
>> + }
>> +
>> + if (pxa_rtc->id == RTC_PXA95X) {
>> + pxa_rtc->base_psbr = ioremap(pxa_rtc->ress_psbr->start,
>> + resource_size(pxa_rtc->ress_psbr));
>> + if (!pxa_rtc->base_psbr) {
>> + dev_err(&pdev->dev,
>> + "Unable to map pxa RTC PSBR I/O memory\n");
>> + goto err_map_base_psbr;
>> + }
>> }
>>
>> /*
>> @@ -376,9 +453,12 @@ static int __init pxa_rtc_probe(struct platform_device *pdev)
>> return 0;
>>
>> err_rtc_reg:
>> + if (pxa_rtc->id == RTC_PXA95X)
>> + iounmap(pxa_rtc->base_psbr);
>> +err_map_base_psbr:
>> iounmap(pxa_rtc->base);
>> +err_map_base:
>> err_ress:
>> -err_map:
>> kfree(pxa_rtc);
>> return ret;
>> }
>> @@ -398,14 +478,6 @@ static int __exit pxa_rtc_remove(struct platform_device *pdev)
>> return 0;
>> }
>>
>> -#ifdef CONFIG_OF
>> -static struct of_device_id pxa_rtc_dt_ids[] = {
>> - { .compatible = "marvell,pxa-rtc" },
>> - {}
>> -};
>> -MODULE_DEVICE_TABLE(of, pxa_rtc_dt_ids);
>> -#endif
>> -
>> #ifdef CONFIG_PM
>> static int pxa_rtc_suspend(struct device *dev)
>> {
>> @@ -440,14 +512,12 @@ static struct platform_driver pxa_rtc_driver = {
>> .pm = &pxa_rtc_pm_ops,
>> #endif
>> },
>> + .id_table = pxa_rtc_id_table,
>> };
>>
>> static int __init pxa_rtc_init(void)
>> {
>> - if (cpu_is_pxa27x() || cpu_is_pxa3xx())
>> - return platform_driver_probe(&pxa_rtc_driver, pxa_rtc_probe);
>> -
>> - return -ENODEV;
>> + return platform_driver_probe(&pxa_rtc_driver, pxa_rtc_probe);
>> }
>>
>> static void __exit pxa_rtc_exit(void)
>> --
>> 1.7.4.1
>>
I do not think so.
First, from solicon, PBSR is not a clock. it only prorect RDCR, RYCR,
RNCR. There are still some other registers that do not need its
protection.
Second, in fact, the RTC has its own clock which is similar with
sa1100-rtc. define PBSR operation in the clock will finally make the
driver to handle two clocks. It will make the driver harder to handle
it, and even impact the clock device tree support for devices.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists