Re: [PATCH v1 17/30] mmc: sdhci-tegra: Support OPP and core voltage scaling

[Viresh Kumar]

On 09-11-20, 08:44, Dmitry Osipenko wrote:
> 09.11.2020 08:35, Viresh Kumar пишет:
> > On 09-11-20, 08:19, Dmitry Osipenko wrote:
> >> Thanks, I made it in a different way by simply adding helpers to the
> >> pm_opp.h which use devm_add_action_or_reset(). This doesn't require to
> >> add new kernel symbols.
> > 
> > I will prefer to add it in core.c itself, and yes
> > devm_add_action_or_reset() looks better. But I am still not sure for
> > which helpers do we need the devm_*() variants, as this is only useful
> > for non-CPU devices. But if we have users that we can add right now,
> > why not.
> 
> All current non-CPU drivers (devfreq, mmc, memory, etc) can benefit from it.
> 
> For Tegra drivers we need these variants:
> 
> devm_pm_opp_set_supported_hw()
> devm_pm_opp_set_regulators() [if we won't use GENPD]
> devm_pm_opp_set_clkname()
> devm_pm_opp_of_add_table()

I tried to look earlier for the stuff already merged in and didn't
find a lot of stuff where the devm_* could be used, maybe I missed
some of it.

Frank, would you like to refresh your series based on suggestions from
Dmitry and make other drivers adapt to the new APIs ?

-- 
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 17/30] mmc: sdhci-tegra: Support OPP and core voltage scaling

[Dmitry Osipenko]

09.11.2020 08:35, Viresh Kumar пишет:
> On 09-11-20, 08:19, Dmitry Osipenko wrote:
>> Thanks, I made it in a different way by simply adding helpers to the
>> pm_opp.h which use devm_add_action_or_reset(). This doesn't require to
>> add new kernel symbols.
> 
> I will prefer to add it in core.c itself, and yes
> devm_add_action_or_reset() looks better. But I am still not sure for
> which helpers do we need the devm_*() variants, as this is only useful
> for non-CPU devices. But if we have users that we can add right now,
> why not.

All current non-CPU drivers (devfreq, mmc, memory, etc) can benefit from it.

For Tegra drivers we need these variants:

devm_pm_opp_set_supported_hw()
devm_pm_opp_set_regulators() [if we won't use GENPD]
devm_pm_opp_set_clkname()
devm_pm_opp_of_add_table()
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 17/30] mmc: sdhci-tegra: Support OPP and core voltage scaling

[Viresh Kumar]

On 09-11-20, 08:19, Dmitry Osipenko wrote:
> Thanks, I made it in a different way by simply adding helpers to the
> pm_opp.h which use devm_add_action_or_reset(). This doesn't require to
> add new kernel symbols.

I will prefer to add it in core.c itself, and yes
devm_add_action_or_reset() looks better. But I am still not sure for
which helpers do we need the devm_*() variants, as this is only useful
for non-CPU devices. But if we have users that we can add right now,
why not.

> static inline int devm_pm_opp_of_add_table(struct device *dev)
> {
>   int err;
> 
>   err = dev_pm_opp_of_add_table(dev);
>   if (err)
>   return err;
> 
>   err = devm_add_action_or_reset(dev, (void*)dev_pm_opp_remove_table,
>  dev);
>   if (err)
>   return err;
> 
>   return 0;
> }

-- 
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 17/30] mmc: sdhci-tegra: Support OPP and core voltage scaling

[Dmitry Osipenko]

09.11.2020 08:10, Viresh Kumar пишет:
> On 09-11-20, 08:08, Dmitry Osipenko wrote:
>> 09.11.2020 08:00, Viresh Kumar пишет:
>>> On 06-11-20, 21:41, Frank Lee wrote:
 On Fri, Nov 6, 2020 at 9:18 PM Dmitry Osipenko  wrote:
>
> 06.11.2020 09:15, Viresh Kumar пишет:
>> Setting regulators for count as 0 doesn't sound good to me.
>>
>> But, I understand that you don't want to have that if (have_regulator)
>> check, and it is a fair request. What I will instead do is, allow all
>> dev_pm_opp_put*() API to start accepting a NULL pointer for the OPP
>> table and fail silently. And so you won't be required to have this
>> unwanted check. But you will be required to save the pointer returned
>> back by dev_pm_opp_set_regulators(), which is the right thing to do
>> anyways.
>
> Perhaps even a better variant could be to add a devm versions of the OPP
> API functions, then drivers won't need to care about storing the
> opp_table pointer if it's unused by drivers.

 I think so. The consumer may not be so concerned about the status of
 these OPP tables.
 If the driver needs to manage the release, it needs to add a pointer
 to their driver global structure.

 Maybe it's worth having these devm interfaces for opp.
>>>
>>> Sure if there are enough users of this, I am all for it. I was fine
>>> with the patches you sent, just that there were not a lot of users of
>>> it and so I pushed them back. If we find that we have more users of it
>>> now, we can surely get that back.
>>>
>>
>> There was already attempt to add the devm? Could you please give me a
>> link to the patches?
>>
>> I already prepared a patch which adds the devm helpers. It helps to keep
>> code cleaner and readable.
> 
> https://lore.kernel.org/lkml/20201012135517.19468-1-fr...@allwinnertech.com/
> 

Thanks, I made it in a different way by simply adding helpers to the
pm_opp.h which use devm_add_action_or_reset(). This doesn't require to
add new kernel symbols.

static inline int devm_pm_opp_of_add_table(struct device *dev)
{
int err;

err = dev_pm_opp_of_add_table(dev);
if (err)
return err;

err = devm_add_action_or_reset(dev, (void*)dev_pm_opp_remove_table,
   dev);
if (err)
return err;

return 0;
}
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 00/30] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs

[Viresh Kumar]

On 09-11-20, 08:10, Dmitry Osipenko wrote:
> 09.11.2020 07:47, Dmitry Osipenko пишет:
> > 09.11.2020 07:43, Viresh Kumar пишет:
> >> On 08-11-20, 15:19, Dmitry Osipenko wrote:
> >>> I took a detailed look at the GENPD and tried to implement it. Here is
> >>> what was found:
> >>>
> >>> 1. GENPD framework doesn't aggregate performance requests from the
> >>> attached devices. This means that if deviceA requests performance state
> >>> 10 and then deviceB requests state 3, then framework will set domain's
> >>> state to 3 instead of 10.
> >>
> >> It does. Look at _genpd_reeval_performance_state().
> >>
> > 
> > Thanks, I probably had a bug in the quick prototype and then overlooked
> > that function.
> > 
> 
> If a non-hardware device-tree node is okay to have for the domain, then
> I can try again.
> 
> What I also haven't mentioned is that GENPD adds some extra complexity
> to some drivers (3d, video decoder) because we will need to handle both
> new GENPD and legacy Tegra specific pre-genpd era domains.
> 
> I'm also not exactly sure how the topology of domains should look like
> because Tegra has a power-controller (PMC) which manages power rail of a
> few hardware units. Perhaps it should be
> 
>   device -> PMC domain -> CORE domain
> 
> but not exactly sure for now.

I am also confused on if it should be a domain or regulator, but that
is for Ulf to tell :)

-- 
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 17/30] mmc: sdhci-tegra: Support OPP and core voltage scaling

[Viresh Kumar]

On 09-11-20, 08:08, Dmitry Osipenko wrote:
> 09.11.2020 08:00, Viresh Kumar пишет:
> > On 06-11-20, 21:41, Frank Lee wrote:
> >> On Fri, Nov 6, 2020 at 9:18 PM Dmitry Osipenko  wrote:
> >>>
> >>> 06.11.2020 09:15, Viresh Kumar пишет:
>  Setting regulators for count as 0 doesn't sound good to me.
> 
>  But, I understand that you don't want to have that if (have_regulator)
>  check, and it is a fair request. What I will instead do is, allow all
>  dev_pm_opp_put*() API to start accepting a NULL pointer for the OPP
>  table and fail silently. And so you won't be required to have this
>  unwanted check. But you will be required to save the pointer returned
>  back by dev_pm_opp_set_regulators(), which is the right thing to do
>  anyways.
> >>>
> >>> Perhaps even a better variant could be to add a devm versions of the OPP
> >>> API functions, then drivers won't need to care about storing the
> >>> opp_table pointer if it's unused by drivers.
> >>
> >> I think so. The consumer may not be so concerned about the status of
> >> these OPP tables.
> >> If the driver needs to manage the release, it needs to add a pointer
> >> to their driver global structure.
> >>
> >> Maybe it's worth having these devm interfaces for opp.
> > 
> > Sure if there are enough users of this, I am all for it. I was fine
> > with the patches you sent, just that there were not a lot of users of
> > it and so I pushed them back. If we find that we have more users of it
> > now, we can surely get that back.
> > 
> 
> There was already attempt to add the devm? Could you please give me a
> link to the patches?
> 
> I already prepared a patch which adds the devm helpers. It helps to keep
> code cleaner and readable.

https://lore.kernel.org/lkml/20201012135517.19468-1-fr...@allwinnertech.com/

-- 
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 00/30] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs

[Dmitry Osipenko]

09.11.2020 07:47, Dmitry Osipenko пишет:
> 09.11.2020 07:43, Viresh Kumar пишет:
>> On 08-11-20, 15:19, Dmitry Osipenko wrote:
>>> I took a detailed look at the GENPD and tried to implement it. Here is
>>> what was found:
>>>
>>> 1. GENPD framework doesn't aggregate performance requests from the
>>> attached devices. This means that if deviceA requests performance state
>>> 10 and then deviceB requests state 3, then framework will set domain's
>>> state to 3 instead of 10.
>>
>> It does. Look at _genpd_reeval_performance_state().
>>
> 
> Thanks, I probably had a bug in the quick prototype and then overlooked
> that function.
> 

If a non-hardware device-tree node is okay to have for the domain, then
I can try again.

What I also haven't mentioned is that GENPD adds some extra complexity
to some drivers (3d, video decoder) because we will need to handle both
new GENPD and legacy Tegra specific pre-genpd era domains.

I'm also not exactly sure how the topology of domains should look like
because Tegra has a power-controller (PMC) which manages power rail of a
few hardware units. Perhaps it should be

  device -> PMC domain -> CORE domain

but not exactly sure for now.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 17/30] mmc: sdhci-tegra: Support OPP and core voltage scaling

[Dmitry Osipenko]

09.11.2020 08:00, Viresh Kumar пишет:
> On 06-11-20, 21:41, Frank Lee wrote:
>> On Fri, Nov 6, 2020 at 9:18 PM Dmitry Osipenko  wrote:
>>>
>>> 06.11.2020 09:15, Viresh Kumar пишет:
 Setting regulators for count as 0 doesn't sound good to me.

 But, I understand that you don't want to have that if (have_regulator)
 check, and it is a fair request. What I will instead do is, allow all
 dev_pm_opp_put*() API to start accepting a NULL pointer for the OPP
 table and fail silently. And so you won't be required to have this
 unwanted check. But you will be required to save the pointer returned
 back by dev_pm_opp_set_regulators(), which is the right thing to do
 anyways.
>>>
>>> Perhaps even a better variant could be to add a devm versions of the OPP
>>> API functions, then drivers won't need to care about storing the
>>> opp_table pointer if it's unused by drivers.
>>
>> I think so. The consumer may not be so concerned about the status of
>> these OPP tables.
>> If the driver needs to manage the release, it needs to add a pointer
>> to their driver global structure.
>>
>> Maybe it's worth having these devm interfaces for opp.
> 
> Sure if there are enough users of this, I am all for it. I was fine
> with the patches you sent, just that there were not a lot of users of
> it and so I pushed them back. If we find that we have more users of it
> now, we can surely get that back.
> 

There was already attempt to add the devm? Could you please give me a
link to the patches?

I already prepared a patch which adds the devm helpers. It helps to keep
code cleaner and readable.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 17/30] mmc: sdhci-tegra: Support OPP and core voltage scaling

[Viresh Kumar]

On 06-11-20, 21:41, Frank Lee wrote:
> On Fri, Nov 6, 2020 at 9:18 PM Dmitry Osipenko  wrote:
> >
> > 06.11.2020 09:15, Viresh Kumar пишет:
> > > Setting regulators for count as 0 doesn't sound good to me.
> > >
> > > But, I understand that you don't want to have that if (have_regulator)
> > > check, and it is a fair request. What I will instead do is, allow all
> > > dev_pm_opp_put*() API to start accepting a NULL pointer for the OPP
> > > table and fail silently. And so you won't be required to have this
> > > unwanted check. But you will be required to save the pointer returned
> > > back by dev_pm_opp_set_regulators(), which is the right thing to do
> > > anyways.
> >
> > Perhaps even a better variant could be to add a devm versions of the OPP
> > API functions, then drivers won't need to care about storing the
> > opp_table pointer if it's unused by drivers.
> 
> I think so. The consumer may not be so concerned about the status of
> these OPP tables.
> If the driver needs to manage the release, it needs to add a pointer
> to their driver global structure.
> 
> Maybe it's worth having these devm interfaces for opp.

Sure if there are enough users of this, I am all for it. I was fine
with the patches you sent, just that there were not a lot of users of
it and so I pushed them back. If we find that we have more users of it
now, we can surely get that back.

-- 
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 00/30] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs

[Dmitry Osipenko]

09.11.2020 07:43, Viresh Kumar пишет:
> On 08-11-20, 15:19, Dmitry Osipenko wrote:
>> I took a detailed look at the GENPD and tried to implement it. Here is
>> what was found:
>>
>> 1. GENPD framework doesn't aggregate performance requests from the
>> attached devices. This means that if deviceA requests performance state
>> 10 and then deviceB requests state 3, then framework will set domain's
>> state to 3 instead of 10.
> 
> It does. Look at _genpd_reeval_performance_state().
> 

Thanks, I probably had a bug in the quick prototype and then overlooked
that function.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 00/30] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs

[Viresh Kumar]

On 08-11-20, 15:19, Dmitry Osipenko wrote:
> I took a detailed look at the GENPD and tried to implement it. Here is
> what was found:
> 
> 1. GENPD framework doesn't aggregate performance requests from the
> attached devices. This means that if deviceA requests performance state
> 10 and then deviceB requests state 3, then framework will set domain's
> state to 3 instead of 10.

It does. Look at _genpd_reeval_performance_state().

-- 
viresh
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Formación Bonificable (Último plazo de inscripción 2020)

[FOESCO]

Buenos días



Os informamos que se encuentra abierto el plazo de inscripción para la "ÚLTIMA 
CONVOCATORIA 2020" de Cursos Bonificables para empleados en activo y en ERTE.

Los cursos son 100% Bonificables con cargo al Crédito de Formación 2020, si 
vuestra empresa todavía dispone de Crédito de Formación 2020 esta es la última 
oportunidad para poder consumirlo.


Deseáis que os mandemos la información?


Quedamos a la espera de vuestra respuesta.


Saludos cordiales.


Alex Pons
Director FOESCO.

FOESCO Formación Estatal Continua.
Entidad Organizadora: B200592AA
www.foesco.com
e-mail: cur...@foesco.net
Tel: 910 323 794
(Horario de 9h a 15h y de 17h a 20h de Lunes a Viernes)

FOESCO ofrece formación a empresas y trabajadores en activo a través de cursos 
bonificados por la Fundación Estatal para la Formación en el Empleo (antiguo 
FORCEM) que gestiona las acciones formativas de FORMACIÓN CONTINUA para 
trabajadores y se rige por la ley 30/2015 de 9 de Septiembre.

Antes de imprimir este e-mail piense bien si es necesario hacerlo. Before 
printing this e-mail please think twice if you really need it. FOESCO Tfno: 910 
382 880 Email: cur...@foesco.com. La información transmitida en este mensaje 
está dirigida solamente a las personas o entidades que figuran en el 
encabezamiento y contiene información confidencial, por lo que, si usted lo 
recibiera por error, por favor destrúyalo sin copiarlo, usarlo ni distribuirlo, 
comunicándolo inmediatamente al emisor del mensaje. De conformidad con lo 
dispuesto en el Reglamento Europeo del 2016/679, del 27 de Abril de 2016, 
FOESCO le informa que los datos por usted suministrados serán tratados con las 
medidas de seguridad conformes a la normativa vigente que se requiere. Dichos 
datos serán empleados con fines de gestión. Para el ejercicio de sus derechos 
de transparencia, información, acceso, rectificación, supresión o derecho al 
olvido, limitación del tratamiento , portabilidad de datos y oposición de sus 
datos de carácter personal deberá dirigirse a la dirección del Responsable del 
tratamiento a C/ LAGUNA DEL MARQUESADO Nº10, 28021, MADRID, "PULSANDO AQUI" 
 y "ENVIAR" o a traves de la 
dirección de correo electrónico: ba...@foesco.com 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1 00/30] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs

[Dmitry Osipenko]

05.11.2020 18:22, Dmitry Osipenko пишет:
> 05.11.2020 12:45, Ulf Hansson пишет:
> ...
>> I need some more time to review this, but just a quick check found a
>> few potential issues...
> 
> Thank you for starting the review! I'm pretty sure it will take a couple
> revisions until all the questions will be resolved :)
> 
>> The "core-supply", that you specify as a regulator for each
>> controller's device node, is not the way we describe power domains.
>> Instead, it seems like you should register a power-domain provider
>> (with the help of genpd) and implement the ->set_performance_state()
>> callback for it. Each device node should then be hooked up to this
>> power-domain, rather than to a "core-supply". For DT bindings, please
>> have a look at Documentation/devicetree/bindings/power/power-domain.yaml
>> and Documentation/devicetree/bindings/power/power_domain.txt.
>>
>> In regards to the "sync state" problem (preventing to change
>> performance states until all consumers have been attached), this can
>> then be managed by the genpd provider driver instead.
> 
> I'll need to take a closer look at GENPD, thank you for the suggestion.
> 
> Sounds like a software GENPD driver which manages clocks and voltages
> could be a good idea, but it also could be an unnecessary
> over-engineering. Let's see..
> 

Hello Ulf and all,

I took a detailed look at the GENPD and tried to implement it. Here is
what was found:

1. GENPD framework doesn't aggregate performance requests from the
attached devices. This means that if deviceA requests performance state
10 and then deviceB requests state 3, then framework will set domain's
state to 3 instead of 10.

https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/base/power/domain.c#L376

2. GENPD framework has a sync() callback in the genpd.domain structure,
but this callback isn't allowed to be used by the GENPD implementation.
The GENPD framework always overrides that callback for its own needs.
Hence GENPD doesn't allow to solve the bootstrapping
state-synchronization problem in a nice way.

https://elixir.bootlin.com/linux/v5.10-rc2/source/drivers/base/power/domain.c#L2606

3. Tegra doesn't have a dedicated hardware power-controller for the core
domain, instead there is only an external voltage regulator. Hence we
will need to create a phony device-tree node for the virtual power
domain, which is probably a wrong thing to do.

===

Perhaps it should be possible to create some hacks to work around
bullets 2 and 3 in order to achieve what we need for DVFS on Tegra, but
bullet 1 isn't solvable without changing how the GENPD core works.

Altogether, the GENPD in its current form is a wrong abstraction for a
system-wide DVFS in a case where multiple devices share power domain and
this domain is a voltage regulator. The regulator framework is the
correct abstraction in this case for today.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel