From: Ran Bi <[email protected]>
This add support for the MediaTek MT6358 RTC. Driver using
compatible data to store different RTC_WRTGR address offset.
Signed-off-by: Ran Bi <[email protected]>
Signed-off-by: Hsin-Hsiung Wang <[email protected]>
---
drivers/rtc/rtc-mt6397.c | 25 +++++++++++++++++--------
include/linux/mfd/mt6397/rtc.h | 16 +++++++++++++++-
2 files changed, 32 insertions(+), 9 deletions(-)
diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
index 5249fc9..a90735e1 100644
--- a/drivers/rtc/rtc-mt6397.c
+++ b/drivers/rtc/rtc-mt6397.c
@@ -9,18 +9,31 @@
#include <linux/mfd/mt6397/core.h>
#include <linux/module.h>
#include <linux/mutex.h>
+#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/regmap.h>
#include <linux/rtc.h>
#include <linux/mfd/mt6397/rtc.h>
#include <linux/mod_devicetable.h>
+static const struct of_device_id mt6397_rtc_of_match[] = {
+ { .compatible = "mediatek,mt6323-rtc",
+ .data = (void *)&mt6397_rtc_data, },
+ { .compatible = "mediatek,mt6358-rtc",
+ .data = (void *)&mt6358_rtc_data, },
+ { .compatible = "mediatek,mt6397-rtc",
+ .data = (void *)&mt6397_rtc_data, },
+ {}
+};
+MODULE_DEVICE_TABLE(of, mt6397_rtc_of_match);
+
static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
{
int ret;
u32 data;
- ret = regmap_write(rtc->regmap, rtc->addr_base + RTC_WRTGR, 1);
+ ret = regmap_write(rtc->regmap,
+ rtc->addr_base + rtc->data->wrtgr, 1);
if (ret < 0)
return ret;
@@ -258,6 +271,9 @@ static int mtk_rtc_probe(struct platform_device *pdev)
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
rtc->addr_base = res->start;
+ rtc->data = (struct mtk_rtc_data *)
+ of_device_get_match_data(&pdev->dev);
+
rtc->irq = platform_get_irq(pdev, 0);
if (rtc->irq < 0)
return rtc->irq;
@@ -322,13 +338,6 @@ static int mt6397_rtc_resume(struct device *dev)
static SIMPLE_DEV_PM_OPS(mt6397_pm_ops, mt6397_rtc_suspend,
mt6397_rtc_resume);
-static const struct of_device_id mt6397_rtc_of_match[] = {
- { .compatible = "mediatek,mt6323-rtc", },
- { .compatible = "mediatek,mt6397-rtc", },
- { }
-};
-MODULE_DEVICE_TABLE(of, mt6397_rtc_of_match);
-
static struct platform_driver mtk_rtc_driver = {
.driver = {
.name = "mt6397-rtc",
diff --git a/include/linux/mfd/mt6397/rtc.h b/include/linux/mfd/mt6397/rtc.h
index f84b916..fffe34a 100644
--- a/include/linux/mfd/mt6397/rtc.h
+++ b/include/linux/mfd/mt6397/rtc.h
@@ -18,7 +18,8 @@
#define RTC_BBPU_CBUSY BIT(6)
#define RTC_BBPU_KEY (0x43 << 8)
-#define RTC_WRTGR 0x003c
+#define RTC_WRTGR_MT6358 0x3a
+#define RTC_WRTGR_MT6397 0x3c
#define RTC_IRQ_STA 0x0002
#define RTC_IRQ_STA_AL BIT(0)
@@ -57,6 +58,10 @@
#define MTK_RTC_POLL_DELAY_US 10
#define MTK_RTC_POLL_TIMEOUT (jiffies_to_usecs(HZ))
+struct mtk_rtc_data {
+ u32 wrtgr;
+};
+
struct mt6397_rtc {
struct device *dev;
struct rtc_device *rtc_dev;
@@ -66,6 +71,15 @@ struct mt6397_rtc {
struct regmap *regmap;
int irq;
u32 addr_base;
+ const struct mtk_rtc_data *data;
+};
+
+static const struct mtk_rtc_data mt6358_rtc_data = {
+ .wrtgr = RTC_WRTGR_MT6358,
+};
+
+static const struct mtk_rtc_data mt6397_rtc_data = {
+ .wrtgr = RTC_WRTGR_MT6397,
};
#endif /* _LINUX_MFD_MT6397_RTC_H_ */
--
2.6.4
On Mon, 2020-02-03 at 19:40 +0800, Hsin-Hsiung Wang wrote:
> From: Ran Bi <[email protected]>
>
> This add support for the MediaTek MT6358 RTC. Driver using
> compatible data to store different RTC_WRTGR address offset.
>
> Signed-off-by: Ran Bi <[email protected]>
> Signed-off-by: Hsin-Hsiung Wang <[email protected]>
> ---
> drivers/rtc/rtc-mt6397.c | 25 +++++++++++++++++--------
> include/linux/mfd/mt6397/rtc.h | 16 +++++++++++++++-
> 2 files changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> index 5249fc9..a90735e1 100644
> --- a/drivers/rtc/rtc-mt6397.c
> +++ b/drivers/rtc/rtc-mt6397.c
> @@ -9,18 +9,31 @@
> #include <linux/mfd/mt6397/core.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> +#include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/regmap.h>
> #include <linux/rtc.h>
> #include <linux/mfd/mt6397/rtc.h>
> #include <linux/mod_devicetable.h>
>
> +static const struct of_device_id mt6397_rtc_of_match[] = {
> + { .compatible = "mediatek,mt6323-rtc",
> + .data = (void *)&mt6397_rtc_data, },
> + { .compatible = "mediatek,mt6358-rtc",
> + .data = (void *)&mt6358_rtc_data, },
> + { .compatible = "mediatek,mt6397-rtc",
> + .data = (void *)&mt6397_rtc_data, },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, mt6397_rtc_of_match);
> +
> static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
> {
> int ret;
> u32 data;
>
> - ret = regmap_write(rtc->regmap, rtc->addr_base + RTC_WRTGR, 1);
> + ret = regmap_write(rtc->regmap,
> + rtc->addr_base + rtc->data->wrtgr, 1);
nit: fit in one line.
<...>
> diff --git a/include/linux/mfd/mt6397/rtc.h b/include/linux/mfd/mt6397/rtc.h
> index f84b916..fffe34a 100644
> --- a/include/linux/mfd/mt6397/rtc.h
> +++ b/include/linux/mfd/mt6397/rtc.h
> @@ -18,7 +18,8 @@
> #define RTC_BBPU_CBUSY BIT(6)
> #define RTC_BBPU_KEY (0x43 << 8)
>
> -#define RTC_WRTGR 0x003c
> +#define RTC_WRTGR_MT6358 0x3a
> +#define RTC_WRTGR_MT6397 0x3c
>
> #define RTC_IRQ_STA 0x0002
> #define RTC_IRQ_STA_AL BIT(0)
> @@ -57,6 +58,10 @@
> #define MTK_RTC_POLL_DELAY_US 10
> #define MTK_RTC_POLL_TIMEOUT (jiffies_to_usecs(HZ))
>
> +struct mtk_rtc_data {
> + u32 wrtgr;
> +};
> +
> struct mt6397_rtc {
> struct device *dev;
> struct rtc_device *rtc_dev;
> @@ -66,6 +71,15 @@ struct mt6397_rtc {
> struct regmap *regmap;
> int irq;
> u32 addr_base;
> + const struct mtk_rtc_data *data;
> +};
> +
> +static const struct mtk_rtc_data mt6358_rtc_data = {
> + .wrtgr = RTC_WRTGR_MT6358,
> +};
> +
> +static const struct mtk_rtc_data mt6397_rtc_data = {
> + .wrtgr = RTC_WRTGR_MT6397,
> };
Hi,
Putting these in header file doesn't looks right to me.
Who need this? can you move them back to rtc-mt6397.c?
Joe.C
On Tue, 2020-02-04 at 00:50 +0800, Yingjoe Chen wrote:
> > diff --git a/include/linux/mfd/mt6397/rtc.h b/include/linux/mfd/mt6397/rtc.h
> > index f84b916..fffe34a 100644
> > --- a/include/linux/mfd/mt6397/rtc.h
> > +++ b/include/linux/mfd/mt6397/rtc.h
> > @@ -18,7 +18,8 @@
> > #define RTC_BBPU_CBUSY BIT(6)
> > #define RTC_BBPU_KEY (0x43 << 8)
> >
> > -#define RTC_WRTGR 0x003c
> > +#define RTC_WRTGR_MT6358 0x3a
> > +#define RTC_WRTGR_MT6397 0x3c
> >
> > #define RTC_IRQ_STA 0x0002
> > #define RTC_IRQ_STA_AL BIT(0)
> > @@ -57,6 +58,10 @@
> > #define MTK_RTC_POLL_DELAY_US 10
> > #define MTK_RTC_POLL_TIMEOUT (jiffies_to_usecs(HZ))
> >
> > +struct mtk_rtc_data {
> > + u32 wrtgr;
> > +};
> > +
> > struct mt6397_rtc {
> > struct device *dev;
> > struct rtc_device *rtc_dev;
> > @@ -66,6 +71,15 @@ struct mt6397_rtc {
> > struct regmap *regmap;
> > int irq;
> > u32 addr_base;
> > + const struct mtk_rtc_data *data;
> > +};
> > +
> > +static const struct mtk_rtc_data mt6358_rtc_data = {
> > + .wrtgr = RTC_WRTGR_MT6358,
> > +};
> > +
> > +static const struct mtk_rtc_data mt6397_rtc_data = {
> > + .wrtgr = RTC_WRTGR_MT6397,
> > };
>
> Hi,
>
> Putting these in header file doesn't looks right to me.
> Who need this? can you move them back to rtc-mt6397.c?
>
> Joe.C
>
This could also effect kernel/drivers/power/reset/mt6323-poweroff.c
which using same region of RTC registers.
There are 2 ways of modification:
1. kernel/drivers/rtc/rtc-mt6397.c implement do_pwroff function and
export to mt6323-poweroff.c
2. Just modify mt6323-poweroff.c file to compatible this patch. I mean
using RTC_WRTGR_MT6397 to replace RTC_WRTGR. Or modify mt6323-poweroff.c
like rtc-mt6397.c
Hi,
On Mon, Feb 17, 2020 at 11:52 AM Ran Bi <[email protected]> wrote:
>
> On Tue, 2020-02-04 at 00:50 +0800, Yingjoe Chen wrote:
> > > diff --git a/include/linux/mfd/mt6397/rtc.h b/include/linux/mfd/mt6397/rtc.h
> > > index f84b916..fffe34a 100644
> > > --- a/include/linux/mfd/mt6397/rtc.h
> > > +++ b/include/linux/mfd/mt6397/rtc.h
> > > @@ -18,7 +18,8 @@
> > > #define RTC_BBPU_CBUSY BIT(6)
> > > #define RTC_BBPU_KEY (0x43 << 8)
> > >
> > > -#define RTC_WRTGR 0x003c
> > > +#define RTC_WRTGR_MT6358 0x3a
> > > +#define RTC_WRTGR_MT6397 0x3c
> > >
> > > #define RTC_IRQ_STA 0x0002
> > > #define RTC_IRQ_STA_AL BIT(0)
> > > @@ -57,6 +58,10 @@
> > > #define MTK_RTC_POLL_DELAY_US 10
> > > #define MTK_RTC_POLL_TIMEOUT (jiffies_to_usecs(HZ))
> > >
> > > +struct mtk_rtc_data {
> > > + u32 wrtgr;
> > > +};
> > > +
> > > struct mt6397_rtc {
> > > struct device *dev;
> > > struct rtc_device *rtc_dev;
> > > @@ -66,6 +71,15 @@ struct mt6397_rtc {
> > > struct regmap *regmap;
> > > int irq;
> > > u32 addr_base;
> > > + const struct mtk_rtc_data *data;
> > > +};
> > > +
> > > +static const struct mtk_rtc_data mt6358_rtc_data = {
> > > + .wrtgr = RTC_WRTGR_MT6358,
> > > +};
> > > +
> > > +static const struct mtk_rtc_data mt6397_rtc_data = {
> > > + .wrtgr = RTC_WRTGR_MT6397,
> > > };
> >
> > Hi,
> >
> > Putting these in header file doesn't looks right to me.
> > Who need this? can you move them back to rtc-mt6397.c?
> > Joe.C
> >
>
> This could also effect kernel/drivers/power/reset/mt6323-poweroff.c
> which using same region of RTC registers.
> There are 2 ways of modification:
> 1. kernel/drivers/rtc/rtc-mt6397.c implement do_pwroff function and
> export to mt6323-poweroff.c
> 2. Just modify mt6323-poweroff.c file to compatible this patch. I mean
> using RTC_WRTGR_MT6397 to replace RTC_WRTGR. Or modify mt6323-poweroff.c
> like rtc-mt6397.c
Oh, I see, so basically both rtc-mt6397.c and mt6323-poweroff.c need
to know at what offset RTC_WRTGR actually is. Correct?
Is there any plan to have mt6323-poweroff.c support any of the other
PMICs (not just MT6323?)?
a. If not, I'd just add:
#define RTC_WRTGR_MT6323 RTC_WRTGR_MT6397
in rtc.h, for added clarity, use that in mt6323-poweroff.c
(s/RTC_WRTGR/RTC_WRTGR_MT6323/), and be done with it.
Actually, even if there's a plan, you can go ahead with this simpler
solution for now, and fix later when the issue comes up.
b. If you ever want to support multiple PMICs with mt6323-poweroff.c,
you'd need that offset for 2 different sub-devices under the same mfd,
so the matching logic belongs in the main mfd device, not in
rtc/poweroff driver.
So I'd move the matching logic in drivers/mfd/mt6397-core.c, and add
rtc_wrtgr offset (or a full _data structure) to `struct mt6397_chip`,
or, probably better, add a IORESOURCE_REG to the matching resources to
specify the offset (that's what drivers/mfd/88pm860x-core.c seems to
be doing, for example).
And then mt6323-poweroff.c should probably be renamed to mt6397-poweroff.c.
(actually, looking at this, I'm even questioning if mt6323-poweroff.c
should even exist, and if you should just fold it into rtc-mt6397.c?
Since they use the same registers?)
Hope this makes sense?
Best,
Hi,
On Wed, 2020-03-04 at 20:59 +0800, Nicolas Boichat wrote:
> Hi,
>
> On Mon, Feb 17, 2020 at 11:52 AM Ran Bi <[email protected]> wrote:
> >
> > On Tue, 2020-02-04 at 00:50 +0800, Yingjoe Chen wrote:
> > > > diff --git a/include/linux/mfd/mt6397/rtc.h b/include/linux/mfd/mt6397/rtc.h
> > > > index f84b916..fffe34a 100644
> > > > --- a/include/linux/mfd/mt6397/rtc.h
> > > > +++ b/include/linux/mfd/mt6397/rtc.h
> > > > @@ -18,7 +18,8 @@
> > > > #define RTC_BBPU_CBUSY BIT(6)
> > > > #define RTC_BBPU_KEY (0x43 << 8)
> > > >
> > > > -#define RTC_WRTGR 0x003c
> > > > +#define RTC_WRTGR_MT6358 0x3a
> > > > +#define RTC_WRTGR_MT6397 0x3c
> > > >
> > > > #define RTC_IRQ_STA 0x0002
> > > > #define RTC_IRQ_STA_AL BIT(0)
> > > > @@ -57,6 +58,10 @@
> > > > #define MTK_RTC_POLL_DELAY_US 10
> > > > #define MTK_RTC_POLL_TIMEOUT (jiffies_to_usecs(HZ))
> > > >
> > > > +struct mtk_rtc_data {
> > > > + u32 wrtgr;
> > > > +};
> > > > +
> > > > struct mt6397_rtc {
> > > > struct device *dev;
> > > > struct rtc_device *rtc_dev;
> > > > @@ -66,6 +71,15 @@ struct mt6397_rtc {
> > > > struct regmap *regmap;
> > > > int irq;
> > > > u32 addr_base;
> > > > + const struct mtk_rtc_data *data;
> > > > +};
> > > > +
> > > > +static const struct mtk_rtc_data mt6358_rtc_data = {
> > > > + .wrtgr = RTC_WRTGR_MT6358,
> > > > +};
> > > > +
> > > > +static const struct mtk_rtc_data mt6397_rtc_data = {
> > > > + .wrtgr = RTC_WRTGR_MT6397,
> > > > };
> > >
> > > Hi,
> > >
> > > Putting these in header file doesn't looks right to me.
> > > Who need this? can you move them back to rtc-mt6397.c?
> > > Joe.C
> > >
> >
> > This could also effect kernel/drivers/power/reset/mt6323-poweroff.c
> > which using same region of RTC registers.
> > There are 2 ways of modification:
> > 1. kernel/drivers/rtc/rtc-mt6397.c implement do_pwroff function and
> > export to mt6323-poweroff.c
> > 2. Just modify mt6323-poweroff.c file to compatible this patch. I mean
> > using RTC_WRTGR_MT6397 to replace RTC_WRTGR. Or modify mt6323-poweroff.c
> > like rtc-mt6397.c
>
> Oh, I see, so basically both rtc-mt6397.c and mt6323-poweroff.c need
> to know at what offset RTC_WRTGR actually is. Correct?
>
Yes, you are right both drivers need to know RTC_WRTGR offset. Offsets
of other registers are the same.
> Is there any plan to have mt6323-poweroff.c support any of the other
> PMICs (not just MT6323?)?
>
Currently, we don't have a plan to let mt6323-poweroff.c support other
PMICs. Because other PMICs like mt6397 and mt6358 could using
arm-trust-firmware PSCI power off flow instead. mt6323-poweroff.c was
prepared for platform without arm-trust-firmware.
> a. If not, I'd just add:
> #define RTC_WRTGR_MT6323 RTC_WRTGR_MT6397
> in rtc.h, for added clarity, use that in mt6323-poweroff.c
> (s/RTC_WRTGR/RTC_WRTGR_MT6323/), and be done with it.
>
I would just change RTC_WRTGR to RTC_WRTGR_MT6397 in mt6323-poweroff.c
at next patchset.
> Actually, even if there's a plan, you can go ahead with this simpler
> solution for now, and fix later when the issue comes up.
>
> b. If you ever want to support multiple PMICs with mt6323-poweroff.c,
> you'd need that offset for 2 different sub-devices under the same mfd,
> so the matching logic belongs in the main mfd device, not in
> rtc/poweroff driver.
>
> So I'd move the matching logic in drivers/mfd/mt6397-core.c, and add
> rtc_wrtgr offset (or a full _data structure) to `struct mt6397_chip`,
> or, probably better, add a IORESOURCE_REG to the matching resources to
> specify the offset (that's what drivers/mfd/88pm860x-core.c seems to
> be doing, for example).
>
> And then mt6323-poweroff.c should probably be renamed to mt6397-poweroff.c.
>
> (actually, looking at this, I'm even questioning if mt6323-poweroff.c
> should even exist, and if you should just fold it into rtc-mt6397.c?
> Since they use the same registers?)
>
mt6323-poweroff.c which hijack pm_power_off pointer is only for platform
without arm-trust-firmware. This is the reason I am considering
mt6323-poweroff.c should not be folded into rtc-mt6397.c.
> Hope this makes sense?
>
> Best,
Thanks for your suggestions.
Best,
On Thu, 2020-03-05 at 15:37 +0800, Ran Bi wrote:
> Hi,
>
> On Wed, 2020-03-04 at 20:59 +0800, Nicolas Boichat wrote:
> > Hi,
> >
> > On Mon, Feb 17, 2020 at 11:52 AM Ran Bi <[email protected]> wrote:
> > >
> > > On Tue, 2020-02-04 at 00:50 +0800, Yingjoe Chen wrote:
<....>
> > > >
> > > > Putting these in header file doesn't looks right to me.
> > > > Who need this? can you move them back to rtc-mt6397.c?
> > > > Joe.C
> > > >
> > >
> > > This could also effect kernel/drivers/power/reset/mt6323-poweroff.c
> > > which using same region of RTC registers.
> > > There are 2 ways of modification:
> > > 1. kernel/drivers/rtc/rtc-mt6397.c implement do_pwroff function and
> > > export to mt6323-poweroff.c
> > > 2. Just modify mt6323-poweroff.c file to compatible this patch. I mean
> > > using RTC_WRTGR_MT6397 to replace RTC_WRTGR. Or modify mt6323-poweroff.c
> > > like rtc-mt6397.c
> >
> > Oh, I see, so basically both rtc-mt6397.c and mt6323-poweroff.c need
> > to know at what offset RTC_WRTGR actually is. Correct?
> >
>
> Yes, you are right both drivers need to know RTC_WRTGR offset. Offsets
> of other registers are the same.
>
> > Is there any plan to have mt6323-poweroff.c support any of the other
> > PMICs (not just MT6323?)?
> >
>
> Currently, we don't have a plan to let mt6323-poweroff.c support other
> PMICs. Because other PMICs like mt6397 and mt6358 could using
> arm-trust-firmware PSCI power off flow instead. mt6323-poweroff.c was
> prepared for platform without arm-trust-firmware.
This depends on SoC instead of PMIC.
We will need mt6323-poweroff.c for soc with armv7 CPU, because we won't
have ATF on them. I'm not aware of new plan for this.
> > a. If not, I'd just add:
> > #define RTC_WRTGR_MT6323 RTC_WRTGR_MT6397
> > in rtc.h, for added clarity, use that in mt6323-poweroff.c
> > (s/RTC_WRTGR/RTC_WRTGR_MT6323/), and be done with it.
> >
>
> I would just change RTC_WRTGR to RTC_WRTGR_MT6397 in mt6323-poweroff.c
> at next patchset.
>
> > Actually, even if there's a plan, you can go ahead with this simpler
> > solution for now, and fix later when the issue comes up.
> >
> > b. If you ever want to support multiple PMICs with mt6323-poweroff.c,
> > you'd need that offset for 2 different sub-devices under the same mfd,
> > so the matching logic belongs in the main mfd device, not in
> > rtc/poweroff driver.
> >
> > So I'd move the matching logic in drivers/mfd/mt6397-core.c, and add
> > rtc_wrtgr offset (or a full _data structure) to `struct mt6397_chip`,
> > or, probably better, add a IORESOURCE_REG to the matching resources to
> > specify the offset (that's what drivers/mfd/88pm860x-core.c seems to
> > be doing, for example).
> >
> > And then mt6323-poweroff.c should probably be renamed to mt6397-poweroff.c.
> >
> > (actually, looking at this, I'm even questioning if mt6323-poweroff.c
> > should even exist, and if you should just fold it into rtc-mt6397.c?
> > Since they use the same registers?)
> >
>
> mt6323-poweroff.c which hijack pm_power_off pointer is only for platform
> without arm-trust-firmware. This is the reason I am considering
> mt6323-poweroff.c should not be folded into rtc-mt6397.c.
Using/sharing same set of registers from different drivers is not good:
- WRTGR is a special register to 'commit' previous changes. If 2 drivers
are running at the same time, it is possible to commit incomplete update
and cause unexpected result. It is easier to control this from same
driver.
- It is easy to overlook the register is access by others and lead to
bugs/build fails when doing driver update, eg, this patchset.
- The trigger code is duplicate in mt6323-poweroff.c, can just call
mtk_rtc_write_trigger.
So I agree with Nicolas, mt6323-poweroff should be folded into
rtc-mt6397.c. We should be able to disable pm_power_off hijacking for
platform with armV8 CPU. Maybe we can keep "mediatek,mt6323-pwrc"
compatible in mt6323-poweroff.c for this.
I'm ok with implement a. as suggested by Nicolas for now.
Joe.C