| Message ID | 20201102120115.29993-3-nicola.mazzucato@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | CPUFreq: Add support for cpu performance dependencies | expand |
On 02-11-20, 12:01, Nicola Mazzucato wrote: > The opp binding now allows to have an empty opp table and shared-opp to > merely describe a hw connection among devices (f/v lines). > > When initialising an opp table, allow such case by: > - treating some errors as warnings > - do not mark empty tables as shared > - don't fail on empty table > > Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com> > --- > drivers/opp/of.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/opp/of.c b/drivers/opp/of.c > index 874b58756220..b0230490bb31 100644 > --- a/drivers/opp/of.c > +++ b/drivers/opp/of.c > @@ -157,6 +157,11 @@ static void _opp_table_free_required_tables(struct opp_table *opp_table) > /* > * Populate all devices and opp tables which are part of "required-opps" list. > * Checking only the first OPP node should be enough. > + * > + * Corner case: empty opp table and opp-shared found. In this case we set > + * unconditionally the opp table access to exclusive, as the opp-shared property > + * is used purely to describe hw connections. Such information will be retrieved > + * via dev_pm_opp_of_get_sharing_cpus(). > */ > static void _opp_table_alloc_required_tables(struct opp_table *opp_table, > struct device *dev, > @@ -169,7 +174,9 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table, > /* Traversing the first OPP node is all we need */ > np = of_get_next_available_child(opp_np, NULL); > if (!np) { > - dev_err(dev, "Empty OPP table\n"); > + dev_warn(dev, "Empty OPP table\n"); > + > + opp_table->shared_opp = OPP_TABLE_ACCESS_EXCLUSIVE; I am not sure I understand the reasoning behind this. > return; > } > > @@ -377,7 +384,9 @@ int dev_pm_opp_of_find_icc_paths(struct device *dev, > struct icc_path **paths; > > ret = _bandwidth_supported(dev, opp_table); > - if (ret <= 0) > + if (ret == -EINVAL) > + return 0; /* Empty OPP table is a valid corner-case, let's not fail */ > + else if (ret <= 0) > return ret; > > ret = 0; > -- > 2.27.0
Hi Viresh, thanks for looking into this. On 11/3/20 5:01 AM, Viresh Kumar wrote: > On 02-11-20, 12:01, Nicola Mazzucato wrote: >> The opp binding now allows to have an empty opp table and shared-opp to >> merely describe a hw connection among devices (f/v lines). >> >> When initialising an opp table, allow such case by: >> - treating some errors as warnings >> - do not mark empty tables as shared >> - don't fail on empty table >> >> Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com> >> --- >> drivers/opp/of.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/opp/of.c b/drivers/opp/of.c >> index 874b58756220..b0230490bb31 100644 >> --- a/drivers/opp/of.c >> +++ b/drivers/opp/of.c >> @@ -157,6 +157,11 @@ static void _opp_table_free_required_tables(struct opp_table *opp_table) >> /* >> * Populate all devices and opp tables which are part of "required-opps" list. >> * Checking only the first OPP node should be enough. >> + * >> + * Corner case: empty opp table and opp-shared found. In this case we set >> + * unconditionally the opp table access to exclusive, as the opp-shared property >> + * is used purely to describe hw connections. Such information will be retrieved >> + * via dev_pm_opp_of_get_sharing_cpus(). >> */ >> static void _opp_table_alloc_required_tables(struct opp_table *opp_table, >> struct device *dev, >> @@ -169,7 +174,9 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table, >> /* Traversing the first OPP node is all we need */ >> np = of_get_next_available_child(opp_np, NULL); >> if (!np) { >> - dev_err(dev, "Empty OPP table\n"); >> + dev_warn(dev, "Empty OPP table\n"); >> + >> + opp_table->shared_opp = OPP_TABLE_ACCESS_EXCLUSIVE; > > I am not sure I understand the reasoning behind this. Initially I thought to place a comment right there but I ended up with an explanation of this case at the top of this function (the corner-case). It probably also needs more details.. Basically, on this case - empty opp table & opp-shared - we limit the scope of opp-shared to *only* tell us about hw description, and not marking the opp points as shared, since they are not present in DT. It would be the equivalent of describing that devices share clock/voltage lines, but we can't tell anything about opp points cause they are not there (in DT). OTOH If we don't set shared_opp to OPP_TABLE_ACCESS_EXCLUSIVE for that specific case, we won't be able to add opps for the remaining cpus as the opp core will find the opps as duplicated. This is a corner case, really. Please let me know if it's not clear. Many thanks Nicola > >> return; >> } >> >> @@ -377,7 +384,9 @@ int dev_pm_opp_of_find_icc_paths(struct device *dev, >> struct icc_path **paths; >> >> ret = _bandwidth_supported(dev, opp_table); >> - if (ret <= 0) >> + if (ret == -EINVAL) >> + return 0; /* Empty OPP table is a valid corner-case, let's not fail */ >> + else if (ret <= 0) >> return ret; >> >> ret = 0; >> -- >> 2.27.0 >
On 04-11-20, 17:54, Nicola Mazzucato wrote: > Initially I thought to place a comment right there but I ended up with an > explanation of this case at the top of this function (the corner-case). It > probably also needs more details.. I read it earlier as well but yeah, that wasn't good enough for me to understand what you are doing. > Basically, on this case - empty opp table & opp-shared - we limit the scope of > opp-shared to *only* tell us about hw description, and not marking the opp > points as shared, since they are not present in DT. It doesn't matter where the OPP table entries are coming from. The OPP table should be marked shared if it is found to be shared. > It would be the equivalent > of describing that devices share clock/voltage lines, but we can't tell anything > about opp points cause they are not there (in DT). Its okay, even then we should set the right flags here. It is really confusing that we blindly set it as exclusive, even though it may be shared. > OTOH If we don't set shared_opp to OPP_TABLE_ACCESS_EXCLUSIVE for that specific > case, we won't be able to add opps for the remaining cpus as the opp core > will find the opps as duplicated. This is a corner case, really. Hmm, I am not sure where you fail and how but this should be set to OPP_TABLE_ACCESS_EXCLUSIVE only if the OPP table isn't shared. else it should be OPP_TABLE_ACCESS_SHARED.
Hi Viresh, sorry for being late in replying. On 11/5/20 4:41 AM, Viresh Kumar wrote: > On 04-11-20, 17:54, Nicola Mazzucato wrote: >> Initially I thought to place a comment right there but I ended up with an >> explanation of this case at the top of this function (the corner-case). It >> probably also needs more details.. > > I read it earlier as well but yeah, that wasn't good enough for me to > understand what you are doing. > >> Basically, on this case - empty opp table & opp-shared - we limit the scope of >> opp-shared to *only* tell us about hw description, and not marking the opp >> points as shared, since they are not present in DT. > > It doesn't matter where the OPP table entries are coming from. The OPP > table should be marked shared if it is found to be shared. > >> It would be the equivalent >> of describing that devices share clock/voltage lines, but we can't tell anything >> about opp points cause they are not there (in DT). > > Its okay, even then we should set the right flags here. It is really > confusing that we blindly set it as exclusive, even though it may be > shared. > >> OTOH If we don't set shared_opp to OPP_TABLE_ACCESS_EXCLUSIVE for that specific >> case, we won't be able to add opps for the remaining cpus as the opp core >> will find the opps as duplicated. This is a corner case, really. > > Hmm, I am not sure where you fail and how but this should be set to > OPP_TABLE_ACCESS_EXCLUSIVE only if the OPP table isn't shared. else it > should be OPP_TABLE_ACCESS_SHARED. > Thanks for providing more details around the meaning of opp-shared, much appreciated. I had some time to play a bit more, and yes, there is no need to set shared_opp to OPP_TABLE_ACCESS_EXCLUSIVE. A minimal change in the driver sequence would suffice. I will remove that in the V4. Many thanks, Nicola
diff --git a/drivers/opp/of.c b/drivers/opp/of.c index 874b58756220..b0230490bb31 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -157,6 +157,11 @@ static void _opp_table_free_required_tables(struct opp_table *opp_table) /* * Populate all devices and opp tables which are part of "required-opps" list. * Checking only the first OPP node should be enough. + * + * Corner case: empty opp table and opp-shared found. In this case we set + * unconditionally the opp table access to exclusive, as the opp-shared property + * is used purely to describe hw connections. Such information will be retrieved + * via dev_pm_opp_of_get_sharing_cpus(). */ static void _opp_table_alloc_required_tables(struct opp_table *opp_table, struct device *dev, @@ -169,7 +174,9 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table, /* Traversing the first OPP node is all we need */ np = of_get_next_available_child(opp_np, NULL); if (!np) { - dev_err(dev, "Empty OPP table\n"); + dev_warn(dev, "Empty OPP table\n"); + + opp_table->shared_opp = OPP_TABLE_ACCESS_EXCLUSIVE; return; } @@ -377,7 +384,9 @@ int dev_pm_opp_of_find_icc_paths(struct device *dev, struct icc_path **paths; ret = _bandwidth_supported(dev, opp_table); - if (ret <= 0) + if (ret == -EINVAL) + return 0; /* Empty OPP table is a valid corner-case, let's not fail */ + else if (ret <= 0) return ret; ret = 0;
The opp binding now allows to have an empty opp table and shared-opp to merely describe a hw connection among devices (f/v lines). When initialising an opp table, allow such case by: - treating some errors as warnings - do not mark empty tables as shared - don't fail on empty table Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com> --- drivers/opp/of.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)