| Message ID | fe6ab23a-5f0d-a317-14d9-f538c3ad4052@grinn-global.com |
|---|---|
| State | Not Applicable |
| Headers | show |
* Marcin Niestroj <m.niestroj@grinn-global.com> [160426 00:40]: > In below code we check reg resource size in order to know if we > should update RTC_PMIC or it is pinctrl responsibility. It's a little > hack, but we make sure, that every device with not modified device-tree > will work as previously. If we want to add support for ext_wakeup, we > just change reg resouce size in device-tree in order to not overlap > requested memory regions in rtc-omap and pinctrl. > > I used am335x-chilisom instead of dra7 and updated > pinctrl-single,funcion-mask so the EXT_WAKEUP_STATUS is always cleared > after boot. > > So the question now is: is this acceptable? Do we want to continue with > this approach? Well the concern I have here is that we not use pinctrl-single as a separate driver if any of the registers are shared with the RTC driver. If the registers are shared, the pinctrl functionality should be implemented in the RTC driver. The pinctrl driver can also implement a chained IRQ later on for consumer device drivers to use with the wakeirq API. Maybe eventually we'll have real chained interrupt for the RTC to use from Linux side of the C-M3 driver. Regards, Tony
Hi, Sorry for such delay in response. Please see my comments/questions on using pinctrl below. On 26.04.2016 17:39, Tony Lindgren wrote: > * Marcin Niestroj <m.niestroj@grinn-global.com> [160426 00:40]: >> In below code we check reg resource size in order to know if we >> should update RTC_PMIC or it is pinctrl responsibility. It's a little >> hack, but we make sure, that every device with not modified device-tree >> will work as previously. If we want to add support for ext_wakeup, we >> just change reg resouce size in device-tree in order to not overlap >> requested memory regions in rtc-omap and pinctrl. >> >> I used am335x-chilisom instead of dra7 and updated >> pinctrl-single,funcion-mask so the EXT_WAKEUP_STATUS is always cleared >> after boot. >> >> So the question now is: is this acceptable? Do we want to continue with >> this approach? > > Well the concern I have here is that we not use pinctrl-single > as a separate driver if any of the registers are shared with > the RTC driver. If the registers are shared, the pinctrl > functionality should be implemented in the RTC driver. We can use pinctrl generic params for: * enable wakeup inputs - with 'input-enable' * input debounce enable/configuration - with 'input-debounce' However I don't see any generic way for setting wakeup input polarity. So I guess we should add some driver specific devicetree binding to handle hat. I also didn't find any other driver that implements it. In case of pinctrl-single we have just written custom register values to set polarity. So the question is: how should we proceed? Is pinctrl still and option? If yes, what is your idea about configuring input polarity? > > The pinctrl driver can also implement a chained IRQ later > on for consumer device drivers to use with the wakeirq API. > Maybe eventually we'll have real chained interrupt for the > RTC to use from Linux side of the C-M3 driver. > > Regards, > > Tony >
* Marcin Niestroj <m.niestroj@grinn-global.com> [160615 01:40]: > Hi, > > Sorry for such delay in response. Please see my comments/questions on > using pinctrl below. > > On 26.04.2016 17:39, Tony Lindgren wrote: > > * Marcin Niestroj <m.niestroj@grinn-global.com> [160426 00:40]: > > > In below code we check reg resource size in order to know if we > > > should update RTC_PMIC or it is pinctrl responsibility. It's a little > > > hack, but we make sure, that every device with not modified device-tree > > > will work as previously. If we want to add support for ext_wakeup, we > > > just change reg resouce size in device-tree in order to not overlap > > > requested memory regions in rtc-omap and pinctrl. > > > > > > I used am335x-chilisom instead of dra7 and updated > > > pinctrl-single,funcion-mask so the EXT_WAKEUP_STATUS is always cleared > > > after boot. > > > > > > So the question now is: is this acceptable? Do we want to continue with > > > this approach? > > > > Well the concern I have here is that we not use pinctrl-single > > as a separate driver if any of the registers are shared with > > the RTC driver. If the registers are shared, the pinctrl > > functionality should be implemented in the RTC driver. > > We can use pinctrl generic params for: > * enable wakeup inputs - with 'input-enable' > * input debounce enable/configuration - with 'input-debounce' > > However I don't see any generic way for setting wakeup input polarity. > So I guess we should add some driver specific devicetree binding to > handle hat. I also didn't find any other driver that implements it. > > In case of pinctrl-single we have just written custom register values to > set polarity. > > So the question is: how should we proceed? Is pinctrl still and option? > If yes, what is your idea about configuring input polarity? At least I don't have any better ideas. You can define the values in the driver specific binding, I'd use something like GPIO_ACTIVE_LOW even though it's a broken GPIO and broken interrupt :) Regards, Tony
diff --git a/arch/arm/boot/dts/am335x-chilisom.dtsi b/arch/arm/boot/dts/am335x-chilisom.dtsi index bc0880d..525ed4f 100644 --- a/arch/arm/boot/dts/am335x-chilisom.dtsi +++ b/arch/arm/boot/dts/am335x-chilisom.dtsi @@ -124,6 +124,31 @@ &rtc { system-power-controller; + + reg = <0x44e3e000 0x98>; + #address-cells = <1>; + #size-cells = <1>; + ranges; + + rtc_pinmux: pinmux@0x44e3e098 { + compatible = "pinctrl-single"; + reg = <0x44e3e098 0x8>; + #address-cells = <1>; + #size-cells = <1>; + + pinctrl-single,register-width = <32>; + pinctrl-single,function-mask = <0x10fff>; + + pinctrl-names = "default"; + pinctrl-0 = <&rtc_pins_default>; + + rtc_pins_default: rtc_pins_default { + pinctrl-single,pins = < + 0x0 0x00010011 + 0x4 0x00000000 + >; + }; + }; }; /* NAND Flash */ diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c index ec2e9c5..db2512c 100644 --- a/drivers/rtc/rtc-omap.c +++ b/drivers/rtc/rtc-omap.c @@ -134,6 +134,7 @@ struct omap_rtc_device_type { struct omap_rtc { struct rtc_device *rtc; void __iomem *base; + resource_size_t res_size; struct clk *clk; int irq_alarm; int irq_timer; @@ -428,8 +429,11 @@ static void omap_rtc_power_off(void) rtc->type->unlock(rtc); /* enable pmic_power_en control */ - val = rtc_readl(rtc, OMAP_RTC_PMIC_REG); - rtc_writel(rtc, OMAP_RTC_PMIC_REG, val | OMAP_RTC_PMIC_POWER_EN_EN); + if (rtc->res_size > OMAP_RTC_PMIC_REG) { + val = rtc_readl(rtc, OMAP_RTC_PMIC_REG); + rtc_writel(rtc, OMAP_RTC_PMIC_REG, + val | OMAP_RTC_PMIC_POWER_EN_EN); + } /* set alarm two seconds from now */ omap_rtc_read_time_raw(rtc, &tm); @@ -567,6 +571,7 @@ static int omap_rtc_probe(struct platform_device *pdev) clk_prepare_enable(rtc->clk); res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + rtc->res_size = resource_size(res); rtc->base = devm_ioremap_resource(&pdev->dev, res); if (IS_ERR(rtc->base)) return PTR_ERR(rtc->base); @@ -681,6 +686,9 @@ static int omap_rtc_probe(struct platform_device *pdev) } } + /* handle pinmux nodes which configure ext_wakeup inputs */ + of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); + return 0; err: