| Message ID | 20231024-topic-pcf85363_hiz_output-v1-2-50908aff0e52@wolfvision.net |
|---|---|
| State | New |
| Headers | show |
| Series | rtc: pcf85363: add support for high-impedance output | expand |
Hello, I'm sorry I never replied to your previous thread... On 25/10/2023 18:21:55+0200, Javier Carrasco wrote: > The "hiz-output" property models the RTC output as a high-impedance > (hi-Z) output. > > This property is optional and if it is not defined, the output will > either act as an output clock (default mode) or as an interrupt > depending on the configuration set by other properties. > > Two modes are defined in case the high-impedance is used: "enabled" and > "sleep". The former disables the RTC output completely while the latter > keeps the RTC output disabled until the system enters in sleep mode. > This option is especially relevant if the output clock is used to feed a > PMU, a PMIC or any other device required to run when the rest of the > system is down. For the sake of completeness, a "disabled" mode has been > added, which acts as if the property was not defined. > > Document "hiz-output" as a non-required property. > > Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> > --- > Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml > index 52aa3e2091e9..4b27a9154191 100644 > --- a/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml > +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml > @@ -36,6 +36,19 @@ properties: > enum: [6000, 7000, 12500] > default: 7000 > > + hiz-output: > + description: > + Use enabled if the output should stay in high-impedance. This > + mode will mask the output as an interrupt source. > + Use sleep if the otuput should be only active in sleep mode. > + This mode is compatible with any other output configuration. > + The disabled value acts as if the property was not defined. > + enum: > + - enabled > + - sleep > + - disabled > + default: disabled > + If instead of using a custom property, you consider this as what it actually is: pinmuxing, then everything else comes for free. With pinctrl, you can define different states for runtime and sleep and they will get applied automatically instead of open coding in the driver. Also, how you define this property means that everyone currently using this RTC is going to have a new warning that they should just ignore.
Hi Alexandre, On 26.10.23 00:23, Alexandre Belloni wrote: > Hello, > > I'm sorry I never replied to your previous thread... > > On 25/10/2023 18:21:55+0200, Javier Carrasco wrote: >> The "hiz-output" property models the RTC output as a high-impedance >> (hi-Z) output. >> >> This property is optional and if it is not defined, the output will >> either act as an output clock (default mode) or as an interrupt >> depending on the configuration set by other properties. >> >> Two modes are defined in case the high-impedance is used: "enabled" and >> "sleep". The former disables the RTC output completely while the latter >> keeps the RTC output disabled until the system enters in sleep mode. >> This option is especially relevant if the output clock is used to feed a >> PMU, a PMIC or any other device required to run when the rest of the >> system is down. For the sake of completeness, a "disabled" mode has been >> added, which acts as if the property was not defined. >> >> Document "hiz-output" as a non-required property. >> >> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> >> --- >> Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml >> index 52aa3e2091e9..4b27a9154191 100644 >> --- a/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml >> +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml >> @@ -36,6 +36,19 @@ properties: >> enum: [6000, 7000, 12500] >> default: 7000 >> >> + hiz-output: >> + description: >> + Use enabled if the output should stay in high-impedance. This >> + mode will mask the output as an interrupt source. >> + Use sleep if the otuput should be only active in sleep mode. >> + This mode is compatible with any other output configuration. >> + The disabled value acts as if the property was not defined. >> + enum: >> + - enabled >> + - sleep >> + - disabled >> + default: disabled >> + > > If instead of using a custom property, you consider this as what it > actually is: pinmuxing, then everything else comes for free. With > pinctrl, you can define different states for runtime and sleep and they > will get applied automatically instead of open coding in the driver. > > Also, how you define this property means that everyone currently using > this RTC is going to have a new warning that they should just ignore. > > Thanks for your reply. The warning can only be triggered if the property is defined, so in principle no one could have that warning yet. Only the ones who actually define it and use an invalid value would get the warning. On the other hand I did not consider your approach, which might make this patch irrelevant. So I will have a look at it to make sure that it achieves the same results. Thanks again and best regards, Javier Carrasco
Hi, On 26.10.23 00:30, Javier Carrasco wrote: > Hi Alexandre, > > On 26.10.23 00:23, Alexandre Belloni wrote: >> Hello, >> >> I'm sorry I never replied to your previous thread... >> >> On 25/10/2023 18:21:55+0200, Javier Carrasco wrote: >>> The "hiz-output" property models the RTC output as a high-impedance >>> (hi-Z) output. >>> >>> This property is optional and if it is not defined, the output will >>> either act as an output clock (default mode) or as an interrupt >>> depending on the configuration set by other properties. >>> >>> Two modes are defined in case the high-impedance is used: "enabled" and >>> "sleep". The former disables the RTC output completely while the latter >>> keeps the RTC output disabled until the system enters in sleep mode. >>> This option is especially relevant if the output clock is used to feed a >>> PMU, a PMIC or any other device required to run when the rest of the >>> system is down. For the sake of completeness, a "disabled" mode has been >>> added, which acts as if the property was not defined. >>> >>> Document "hiz-output" as a non-required property. >>> >>> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> >>> --- >>> Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml | 14 ++++++++++++++ >>> 1 file changed, 14 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml >>> index 52aa3e2091e9..4b27a9154191 100644 >>> --- a/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml >>> +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml >>> @@ -36,6 +36,19 @@ properties: >>> enum: [6000, 7000, 12500] >>> default: 7000 >>> >>> + hiz-output: >>> + description: >>> + Use enabled if the output should stay in high-impedance. This >>> + mode will mask the output as an interrupt source. >>> + Use sleep if the otuput should be only active in sleep mode. >>> + This mode is compatible with any other output configuration. >>> + The disabled value acts as if the property was not defined. >>> + enum: >>> + - enabled >>> + - sleep >>> + - disabled >>> + default: disabled >>> + >> >> If instead of using a custom property, you consider this as what it >> actually is: pinmuxing, then everything else comes for free. With >> pinctrl, you can define different states for runtime and sleep and they >> will get applied automatically instead of open coding in the driver. I am not sure if your solution would cover all my needs: 1.- With pinctrl I can model the SoC pins, right? That would not stop the RTC output though, so the 32 kHz signal would be generated anyways even though the SoC would ignore it. That is one of the things I want to avoid. 2.- What happens if the RTC output is a clock for an external device that is only required when the SoC is in sleep mode? In that case I would like the RTC driver to control the output with the modes it provides. >> >> Also, how you define this property means that everyone currently using >> this RTC is going to have a new warning that they should just ignore. >> >> > Thanks for your reply. The warning can only be triggered if the property > is defined, so in principle no one could have that warning yet. Only the > ones who actually define it and use an invalid value would get the warning. > > On the other hand I did not consider your approach, which might make > this patch irrelevant. So I will have a look at it to make sure that it > achieves the same results. > > Thanks again and best regards, > Javier Carrasco >
On 26/10/2023 01:23:21+0200, Javier Carrasco wrote: > >>> + hiz-output: > >>> + description: > >>> + Use enabled if the output should stay in high-impedance. This > >>> + mode will mask the output as an interrupt source. > >>> + Use sleep if the otuput should be only active in sleep mode. > >>> + This mode is compatible with any other output configuration. > >>> + The disabled value acts as if the property was not defined. > >>> + enum: > >>> + - enabled > >>> + - sleep > >>> + - disabled > >>> + default: disabled > >>> + > >> > >> If instead of using a custom property, you consider this as what it > >> actually is: pinmuxing, then everything else comes for free. With > >> pinctrl, you can define different states for runtime and sleep and they > >> will get applied automatically instead of open coding in the driver. > > I am not sure if your solution would cover all my needs: > > 1.- With pinctrl I can model the SoC pins, right? That would not stop > the RTC output though, so the 32 kHz signal would be generated anyways > even though the SoC would ignore it. That is one of the things I want to > avoid. > No, you would model the INTA pin. > 2.- What happens if the RTC output is a clock for an external device > that is only required when the SoC is in sleep mode? In that case I > would like the RTC driver to control the output with the modes it provides. Even if I doubt this is a valid use case, this would be possible as long as the external device node has the correct pinctrl-* properties. > >> > >> Also, how you define this property means that everyone currently using > >> this RTC is going to have a new warning that they should just ignore. > >> > >> > > Thanks for your reply. The warning can only be triggered if the property > > is defined, so in principle no one could have that warning yet. Only the > > ones who actually define it and use an invalid value would get the warning. > > > > On the other hand I did not consider your approach, which might make > > this patch irrelevant. So I will have a look at it to make sure that it > > achieves the same results. > > > > Thanks again and best regards, > > Javier Carrasco > >
On 26.10.23 02:50, Alexandre Belloni wrote: > On 26/10/2023 01:23:21+0200, Javier Carrasco wrote: >>>>> + hiz-output: >>>>> + description: >>>>> + Use enabled if the output should stay in high-impedance. This >>>>> + mode will mask the output as an interrupt source. >>>>> + Use sleep if the otuput should be only active in sleep mode. >>>>> + This mode is compatible with any other output configuration. >>>>> + The disabled value acts as if the property was not defined. >>>>> + enum: >>>>> + - enabled >>>>> + - sleep >>>>> + - disabled >>>>> + default: disabled >>>>> + >>>> >>>> If instead of using a custom property, you consider this as what it >>>> actually is: pinmuxing, then everything else comes for free. With >>>> pinctrl, you can define different states for runtime and sleep and they >>>> will get applied automatically instead of open coding in the driver. >> >> I am not sure if your solution would cover all my needs: >> >> 1.- With pinctrl I can model the SoC pins, right? That would not stop >> the RTC output though, so the 32 kHz signal would be generated anyways >> even though the SoC would ignore it. That is one of the things I want to >> avoid. >> > > No, you would model the INTA pin. I am sorry for insisting on this topic, but if I get you right, I would be modeling an interrupt pin (INTA) to keep it from generating a clock signal when the RTC itself offers a high-impedance mode i.e. avoiding to use the RTC feature. Is that not a misuse of the INTA pin in the first place? If there was no other option, that would be an easy fix, but why would we not implement the hi-Z mode when it is available? If I see a pinctrl-* modeling an interrupt pin, it is not obvious that I am doing that to stop the clock signal and I would have to clarify it explicitly, especially if I am not interested in the interrupt. I would rather implement and document the hi-Z mode the RTC offers instead of using another mode like INTA which actually can trigger interrupts. If the implementation must be different is of course another topic. > >> 2.- What happens if the RTC output is a clock for an external device >> that is only required when the SoC is in sleep mode? In that case I >> would like the RTC driver to control the output with the modes it provides. > > Even if I doubt this is a valid use case, this would be possible as long > as the external device node has the correct pinctrl-* properties. > > >>>> >>>> Also, how you define this property means that everyone currently using >>>> this RTC is going to have a new warning that they should just ignore. >>>> >>>> >>> Thanks for your reply. The warning can only be triggered if the property >>> is defined, so in principle no one could have that warning yet. Only the >>> ones who actually define it and use an invalid value would get the warning. >>> >>> On the other hand I did not consider your approach, which might make >>> this patch irrelevant. So I will have a look at it to make sure that it >>> achieves the same results. >>> >>> Thanks again and best regards, >>> Javier Carrasco >>> >
On 26/10/2023 11:41:47+0200, Javier Carrasco wrote: > > On 26.10.23 02:50, Alexandre Belloni wrote: > > On 26/10/2023 01:23:21+0200, Javier Carrasco wrote: > >>>>> + hiz-output: > >>>>> + description: > >>>>> + Use enabled if the output should stay in high-impedance. This > >>>>> + mode will mask the output as an interrupt source. > >>>>> + Use sleep if the otuput should be only active in sleep mode. > >>>>> + This mode is compatible with any other output configuration. > >>>>> + The disabled value acts as if the property was not defined. > >>>>> + enum: > >>>>> + - enabled > >>>>> + - sleep > >>>>> + - disabled > >>>>> + default: disabled > >>>>> + > >>>> > >>>> If instead of using a custom property, you consider this as what it > >>>> actually is: pinmuxing, then everything else comes for free. With > >>>> pinctrl, you can define different states for runtime and sleep and they > >>>> will get applied automatically instead of open coding in the driver. > >> > >> I am not sure if your solution would cover all my needs: > >> > >> 1.- With pinctrl I can model the SoC pins, right? That would not stop > >> the RTC output though, so the 32 kHz signal would be generated anyways > >> even though the SoC would ignore it. That is one of the things I want to > >> avoid. > >> > > > > No, you would model the INTA pin. > I am sorry for insisting on this topic, but if I get you right, I would > be modeling an interrupt pin (INTA) to keep it from generating a clock > signal when the RTC itself offers a high-impedance mode i.e. avoiding to > use the RTC feature. > > Is that not a misuse of the INTA pin in the first place? If there was no > other option, that would be an easy fix, but why would we not implement > the hi-Z mode when it is available? If I see a pinctrl-* modeling an > interrupt pin, it is not obvious that I am doing that to stop the clock > signal and I would have to clarify it explicitly, especially if I am not > interested in the interrupt. > > I would rather implement and document the hi-Z mode the RTC offers > instead of using another mode like INTA which actually can trigger > interrupts. If the implementation must be different is of course another > topic. There is a pin named INTA, it can mux 4 different functions: - clock output - battery mode indication - interrupt - HiZ with pinmuxing, you can select which function you want for the pin. I'm not sure what is misused there. Can you explain what is your actual use case? I'm starting to understand that what you want is simply disable clock out because you are not using the interrupt. If we assume we are never going to use battery mode, then we could simply used the CCF for this like the other RTC drivers.
On 26.10.23 11:56, Alexandre Belloni wrote: > On 26/10/2023 11:41:47+0200, Javier Carrasco wrote: >> >> On 26.10.23 02:50, Alexandre Belloni wrote: >>> On 26/10/2023 01:23:21+0200, Javier Carrasco wrote: >>>>>>> + hiz-output: >>>>>>> + description: >>>>>>> + Use enabled if the output should stay in high-impedance. This >>>>>>> + mode will mask the output as an interrupt source. >>>>>>> + Use sleep if the otuput should be only active in sleep mode. >>>>>>> + This mode is compatible with any other output configuration. >>>>>>> + The disabled value acts as if the property was not defined. >>>>>>> + enum: >>>>>>> + - enabled >>>>>>> + - sleep >>>>>>> + - disabled >>>>>>> + default: disabled >>>>>>> + >>>>>> >>>>>> If instead of using a custom property, you consider this as what it >>>>>> actually is: pinmuxing, then everything else comes for free. With >>>>>> pinctrl, you can define different states for runtime and sleep and they >>>>>> will get applied automatically instead of open coding in the driver. >>>> >>>> I am not sure if your solution would cover all my needs: >>>> >>>> 1.- With pinctrl I can model the SoC pins, right? That would not stop >>>> the RTC output though, so the 32 kHz signal would be generated anyways >>>> even though the SoC would ignore it. That is one of the things I want to >>>> avoid. >>>> >>> >>> No, you would model the INTA pin. >> I am sorry for insisting on this topic, but if I get you right, I would >> be modeling an interrupt pin (INTA) to keep it from generating a clock >> signal when the RTC itself offers a high-impedance mode i.e. avoiding to >> use the RTC feature. >> >> Is that not a misuse of the INTA pin in the first place? If there was no >> other option, that would be an easy fix, but why would we not implement >> the hi-Z mode when it is available? If I see a pinctrl-* modeling an >> interrupt pin, it is not obvious that I am doing that to stop the clock >> signal and I would have to clarify it explicitly, especially if I am not >> interested in the interrupt. >> >> I would rather implement and document the hi-Z mode the RTC offers >> instead of using another mode like INTA which actually can trigger >> interrupts. If the implementation must be different is of course another >> topic. > > There is a pin named INTA, it can mux 4 different functions: > > - clock output > - battery mode indication > - interrupt > - HiZ > > with pinmuxing, you can select which function you want for the pin. I'm > not sure what is misused there. > > Can you explain what is your actual use case? I'm starting to understand > that what you want is simply disable clock out because you are not using > the interrupt. > > If we assume we are never going to use battery mode, then we could > simply used the CCF for this like the other RTC drivers. > I want to model the INTA pin as a clock source that only should run in sleep mode because its clock is only used in that mode. Therefore I want the pin to stay in hi-Z during normal operation. I do not want to get any interrupts from the INTA pin and the battery mode indication is not relevant for me either. I do not know the CCF mechanism in other RTCs though, but I think that the hi-Z mode accomplishes exactly what I described.The assumption about the battery mode is therefore beyond my knowledge, but my first reaction is that we already have the hi-Z for that. So in the end I just need a mechanism to configure INTA as hi-Z, which the driver still does not support. There is another application where the clock output is not required even though it is physically connected, so hi-Z is again an interesting mode and the battery mode would be available if it ever becomes relevant for anyone.
On 26/10/2023 12:13:23+0200, Javier Carrasco wrote: > I want to model the INTA pin as a clock source that only should run in > sleep mode because its clock is only used in that mode. Therefore I want > the pin to stay in hi-Z during normal operation. Can you disclose what is the user of the clock, do you have a driver for this device? > > I do not want to get any interrupts from the INTA pin and the battery > mode indication is not relevant for me either. I do not know the CCF > mechanism in other RTCs though, but I think that the hi-Z mode > accomplishes exactly what I described.The assumption about the battery > mode is therefore beyond my knowledge, but my first reaction is that we > already have the hi-Z for that. > > So in the end I just need a mechanism to configure INTA as hi-Z, which > the driver still does not support. There is another application where > the clock output is not required even though it is physically connected, > so hi-Z is again an interesting mode and the battery mode would be > available if it ever becomes relevant for anyone. >
On 26.10.23 12:21, Alexandre Belloni wrote: > On 26/10/2023 12:13:23+0200, Javier Carrasco wrote: >> I want to model the INTA pin as a clock source that only should run in >> sleep mode because its clock is only used in that mode. Therefore I want >> the pin to stay in hi-Z during normal operation. > > Can you disclose what is the user of the clock, do you have a driver for > this device? > It is a Rockchip PMU through its CLK32K_IN pin. I can't disclose the exact model (yet) though, but the use case is that the PMU can run with the RTC clock connected to that pin in low-power modes. That pin is configured in the proper mode and maybe it could be configured differently with pinctrl in "default" mode, but the RTC INTA would still output the clock, which is what I want to avoid in this particular case. I just want to stop the clock at the RTC end i.e. write PIN_IO_INTA_HIZ into the PIN_IO_INTAPM. >> >> I do not want to get any interrupts from the INTA pin and the battery >> mode indication is not relevant for me either. I do not know the CCF >> mechanism in other RTCs though, but I think that the hi-Z mode >> accomplishes exactly what I described.The assumption about the battery >> mode is therefore beyond my knowledge, but my first reaction is that we >> already have the hi-Z for that. >> >> So in the end I just need a mechanism to configure INTA as hi-Z, which >> the driver still does not support. There is another application where >> the clock output is not required even though it is physically connected, >> so hi-Z is again an interesting mode and the battery mode would be >> available if it ever becomes relevant for anyone. >> >
Hello Alexandre, On 26.10.23 11:56, Alexandre Belloni wrote: > There is a pin named INTA, it can mux 4 different functions: > > - clock output > - battery mode indication > - interrupt > - HiZ > > with pinmuxing, you can select which function you want for the pin. I'm > not sure what is misused there. > > Can you explain what is your actual use case? I'm starting to understand > that what you want is simply disable clock out because you are not using > the interrupt. > > If we assume we are never going to use battery mode, then we could > simply used the CCF for this like the other RTC drivers. > Could you please point me to an RTC driver that uses the CCF for a similar application? I am not sure what you mean with the battery mode to make use of the HiZ, and I would like to push forward with this functionality. Thank you. Best regards, Javier Carrasco
diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml b/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml index 52aa3e2091e9..4b27a9154191 100644 --- a/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml @@ -36,6 +36,19 @@ properties: enum: [6000, 7000, 12500] default: 7000 + hiz-output: + description: + Use enabled if the output should stay in high-impedance. This + mode will mask the output as an interrupt source. + Use sleep if the otuput should be only active in sleep mode. + This mode is compatible with any other output configuration. + The disabled value acts as if the property was not defined. + enum: + - enabled + - sleep + - disabled + default: disabled + start-year: true wakeup-source: true @@ -56,5 +69,6 @@ examples: reg = <0x51>; #clock-cells = <0>; quartz-load-femtofarads = <12500>; + hiz-output = "sleep"; }; };
The "hiz-output" property models the RTC output as a high-impedance (hi-Z) output. This property is optional and if it is not defined, the output will either act as an output clock (default mode) or as an interrupt depending on the configuration set by other properties. Two modes are defined in case the high-impedance is used: "enabled" and "sleep". The former disables the RTC output completely while the latter keeps the RTC output disabled until the system enters in sleep mode. This option is especially relevant if the output clock is used to feed a PMU, a PMIC or any other device required to run when the rest of the system is down. For the sake of completeness, a "disabled" mode has been added, which acts as if the property was not defined. Document "hiz-output" as a non-required property. Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> --- Documentation/devicetree/bindings/rtc/nxp,pcf85363.yaml | 14 ++++++++++++++ 1 file changed, 14 insertions(+)