Message-Id: <20230613073316.1984750-1-bbara93@gmail.com>
Date:   Tue, 13 Jun 2023 09:33:16 +0200
From:   Benjamin Bara <bbara93@...il.com>
To:     mazziesaccount@...il.com
Cc:     DLG-Adam.Ward.opensource@...renesas.com, bbara93@...il.com,
        benjamin.bara@...data.com, broonie@...nel.org, lgirdwood@...il.com,
        linux-kernel@...r.kernel.org, support.opensource@...semi.com
Subject: Re: [PATCH RFC v3 1/5] regulator: move monitor handling into own function

Hi Matti!

On Wed, 24 May 2023 at 09:28, Matti Vaittinen <mazziesaccount@...il.com> wrote:
> I am thinking that maybe the default should still be to not touch the
> monitoring unless explicitly requested.

Got it - I will bring back the "mon_disable_reg_disabled" property. With
this property, the current behaviour will be in-place.

> My thinking is that the hardware should by default be able to handle
> the voltage change / enable / disable etc while monitoring is enabled.
> Hardware which requires explicit monitoring disabling sounds (to me)
> like a 'design problem' and disabling the monitoring sounds (to me)
> like a workaround. I wouldn't make this workaround default.
> Furthermore, monitoring is a safety feature, and as such core should
> not autonomously disable it (unless such behaviour is requested).

I totally agree with you here. However, there are regulators that
require such workarounds (e.g. bd718x7 and da9063) and that's the reason
for this series.

> I was thinking of a case where regulator state is not readable - I'm
> not 100% sure how core thinks of their state.

AFAIK, they would be considered as always on. But as Mark said, we could
add this as a requirement for having protection.

> Another case could be a regulator which is not registered to the core
> but shares monitoring with some other regulator. 

I think this case should be handled by the driver anyways. Activating a
shared protection on one regulator, without activating it on the other
regulator should be considered as an error in my opinion.

> If I didn't misread the code, the differences here are that existing
> "ideology" is to a) only touch the monitoring (enable/disable) when
> explicitly requested for a given level and b) knowing that all
> monitors that are requested to be enabled are enabled at the end of
> the probe.
>
> In my eyes change a) is problematic. For example, if a board using
> BD9576 wants to have protection disabled via device-tree (let's assume
> there is a board where we know that some disturbance to voltages will
> occur under specific conditions) - it is very valid to complain
> disabling protection is not supported. 

Yes, I think so too. I would not give the BD9576 any new "workaround
property", which would lead to the behaviour which is currently
implemented, meaning the monitoring is not touched after initialization.

> Go fix your board design message needs to be given because protection
> can't be disabled. This is very different from case where we just try
> disabling monitoring because regulator is turned off. In latter case
> with BD9576 the failure to disable protection should just be silently
> ignored. When we use same callbacks for both the initial configuration
> and the runtime enable/disable/voltage-change handling the driver has
> no way knowing if this is an error or not.

Got it. I am aware now that there are PMICs which do not allow to turn
off the monitor, therefore the default behaviour will be the same as
now. For now, only the da9063 (invalid state when monitoring a disabled
monitor) and the bd718x7 (invalid state when monitoring an enabled
regulator that switches to a higher voltage) are affected by the new
properties. The others which currently have {O,U}VP (max597x, bd9576)
should stay the same as now.

> Therefore, I will switch back to only do it when the monitor
> configuration for enable/disable/voltage-change should be done via
> separate driver callback - that would allow driver to separate these
> use-cases. If this was change I wrote, I might try creating separate
> driver callbacks for
> enable/disable/voltage_change_start/voltage_change_done which get the
> initial monitor configuration (as was read from device-tree) as an
> argument. Do you think that could give the flexibility to handle all
> different hardware quirks?

I think it would, yes. But I also think that it will lead to a lot of
duplicate code. However, instead of a simple "enable/disable" property,
we could reuse the "type of protection" too, to create some kind of
matrix. Example: Instead of setting mon_disable_reg_set_higher to 1 for
the bd718x7, we could set it to REGULATOR_MONITOR_OVER_VOLTAGE, meaning
just this protection should be disabled while switching to the higher
voltage. What do you think about that?

> The change b) does also have consequences. Some PMICs like the BD9576
> do use same IRQ for indicating either ERROR or WARNING level problem.
> Whether to use WARNING or ERROR is selected at star-up when the
> device-tree flags are read. Eg, the .set_<XXX>_protection callbacks
> store the severity information (WARNING or ERROR) and complain if both
> are tried to be used. With the current approach we know the validity
> of this configuration is checked right when regulator is registered,
> not later at runtime when regulator is enabled.

Not sure about that, but I think it would fail to register the
regulator? In this case, later it would not be able to enable it because
it is not registered, right?

> Another example regarding design that uses the knowledge that all
> requested monitors are enabled when regulator is registered is BD96801
> - which is not upstream (although I've had patches in my outbox for an
> year already waiting for permission from the HQ to actually send
> them... Don't ask...). This PMIC can configure fatality of the fault
> monitoring. This driver checks that all regulators did agree on
> whether to use PROTECTION or ERROR/WARNING level monitoring at the end
> of the probe - and toggles the IRQ fatality accordingly. I truly
> believe that out-of-tree drivers must not mandate upstream design -
> but I equally believe that we may see similar HW designs in upstream
> and considering this now makes sense :) Yes, in order to paper over b)
> a driver can for sure go and parse all the monitoring properties from
> device-tree itself and decide things based on that - but it might be
> quite a lot of duplicated code.

>From my point of view, the behaviour will stay exactly the same! If they
don't agree on the same level, the probe should actually fail and the
regulators should not be registered. 

> To sum up my view - I do definitely like the idea that core supports
> toggling the monitors for duration of enable/disable/voltage-change as
> this is needed by some real world ICs.
>
> I, however, think drivers should be able to separate the "set the
> default monitoring config" request from the "change config to
> something we use for duration of this operation" - because the best
> monitoring config that is required for an operation may not be a
> "disable all". Hence, we should leave it for the driver to decide what
> config to set for the duration of an
> enable/disable/voltage_set-operation.
>
> Furthermore, I believe the default should be "don't touch the
> monitoring" and not to try disable/enable it w/o explicit request.

Yes, I will definitely keep that in mind and implement it like that in
the next version.

> Again, thank you for working on this and including me in the
> discussion :)

Thanks for your valuable feedback!

Best regards,
Benjamin

Powered by blists - more mailing lists