Re: [V2 PATCH 2/2] rtc: pxa: add pxa95x rtc support

[Chao Xie]

On Wed, Dec 5, 2012 at 3:04 PM, Haojian Zhuang <haojian.zhuang@xxxxxxxxx> wrote:
> On Wed, Dec 5, 2012 at 2:49 PM, Chao Xie <chao.xie@xxxxxxxxxxx> wrote:
>> the pxa95x rtc need access PBSR register before write to
>> RTTR, RCNR, RDCR, and RYCR registers.
>>
>> Signed-off-by: Chao Xie <chao.xie@xxxxxxxxxxx>
>> ---
>>  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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/