| Message ID | 20220624141853.7417-2-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | clk/resets: baikal-t1: Add DDR/PCIe resets and xGMAC/SATA fixes | expand |
On 6/24/22 17:18, Serge Semin wrote: > Most likely due to copy-paste mistake the device managed version of the > denoted reset control getter has been implemented with invalid semantic, > which can be immediately spotted by having "WARN_ON(shared && acquired)" > warning in the system log as soon as the method is called. Anyway let's > fix it by altering the boolean arguments passed to the > __devm_reset_control_bulk_get() method from > - shared = true, optional = false, acquired = true > to > + shared = false, optional = true, acquired = true > That's what they were supposed to be in the first place (see the non-devm > version of the same method: reset_control_bulk_get_optional_exclusive()). > > Fixes: 48d71395896d ("reset: Add reset_control_bulk API") > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > Changelog v4: > - This is a new patch added on v4 lap of the series. > --- > include/linux/reset.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/reset.h b/include/linux/reset.h > index 8a21b5756c3e..514ddf003efc 100644 > --- a/include/linux/reset.h > +++ b/include/linux/reset.h > @@ -731,7 +731,7 @@ static inline int __must_check > devm_reset_control_bulk_get_optional_exclusive(struct device *dev, int num_rstcs, > struct reset_control_bulk_data *rstcs) > { > - return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, true, false, true); > + return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, false, true, true); > } > > /** Good catch, Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> Although, this patch should be sent as standalone since it's unrelated to the rest of the clk patches.
On Mon, Jun 27, 2022 at 01:35:28AM +0300, Dmitry Osipenko wrote: > On 6/24/22 17:18, Serge Semin wrote: > > Most likely due to copy-paste mistake the device managed version of the > > denoted reset control getter has been implemented with invalid semantic, > > which can be immediately spotted by having "WARN_ON(shared && acquired)" > > warning in the system log as soon as the method is called. Anyway let's > > fix it by altering the boolean arguments passed to the > > __devm_reset_control_bulk_get() method from > > - shared = true, optional = false, acquired = true > > to > > + shared = false, optional = true, acquired = true > > That's what they were supposed to be in the first place (see the non-devm > > version of the same method: reset_control_bulk_get_optional_exclusive()). > > > > Fixes: 48d71395896d ("reset: Add reset_control_bulk API") > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > --- > > > > Changelog v4: > > - This is a new patch added on v4 lap of the series. > > --- > > include/linux/reset.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/linux/reset.h b/include/linux/reset.h > > index 8a21b5756c3e..514ddf003efc 100644 > > --- a/include/linux/reset.h > > +++ b/include/linux/reset.h > > @@ -731,7 +731,7 @@ static inline int __must_check > > devm_reset_control_bulk_get_optional_exclusive(struct device *dev, int num_rstcs, > > struct reset_control_bulk_data *rstcs) > > { > > - return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, true, false, true); > > + return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, false, true, true); > > } > > > > /** > > Good catch, > > Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> Thanks. > > Although, this patch should be sent as standalone since it's unrelated > to the rest of the clk patches. Mainly you are right, but I've discovered the problem in the framework of this patchset development. So it was more suitable for me to submit the patch in the framework of it. There is an implicit context too. Due to the patchset containing the reset-control-related patches I has been trying to attract the Philipp' attention for about four months now, but with no luck. Having the kernel API fix-patch in the series was supposed to implicitly raise the patchset valuability for him. I don't know what else to do, but Stephen said he wanted to see what Philipp thought of the reset-part of the series. -Sergey > > -- > Best regards, > Dmitry
Hi Serge, On Fr, 2022-06-24 at 17:18 +0300, Serge Semin wrote: > Most likely due to copy-paste mistake the device managed version of the > denoted reset control getter has been implemented with invalid semantic, > which can be immediately spotted by having "WARN_ON(shared && acquired)" > warning in the system log as soon as the method is called. Anyway let's > fix it by altering the boolean arguments passed to the > __devm_reset_control_bulk_get() method from > - shared = true, optional = false, acquired = true > to > + shared = false, optional = true, acquired = true > That's what they were supposed to be in the first place (see the non-devm > version of the same method: reset_control_bulk_get_optional_exclusive()). > > Fixes: 48d71395896d ("reset: Add reset_control_bulk API") > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > Changelog v4: > - This is a new patch added on v4 lap of the series. > --- > include/linux/reset.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/reset.h b/include/linux/reset.h > index 8a21b5756c3e..514ddf003efc 100644 > --- a/include/linux/reset.h > +++ b/include/linux/reset.h > @@ -731,7 +731,7 @@ static inline int __must_check > devm_reset_control_bulk_get_optional_exclusive(struct device *dev, int num_rstcs, > struct reset_control_bulk_data *rstcs) > { > - return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, true, false, true); > + return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, false, true, true); > } You are right, thank you. I'll take this as a sign that the internal functions should probably switch from collecting ever more boolean arguments to a flags parameter, similar to enum gpiod_flags. I'll pick this up separately into the reset/fixes branch. regards Philipp
On Wed, Jun 29, 2022 at 04:02:43PM +0200, Philipp Zabel wrote: > Hi Serge, > > On Fr, 2022-06-24 at 17:18 +0300, Serge Semin wrote: > > Most likely due to copy-paste mistake the device managed version of the > > denoted reset control getter has been implemented with invalid semantic, > > which can be immediately spotted by having "WARN_ON(shared && acquired)" > > warning in the system log as soon as the method is called. Anyway let's > > fix it by altering the boolean arguments passed to the > > __devm_reset_control_bulk_get() method from > > - shared = true, optional = false, acquired = true > > to > > + shared = false, optional = true, acquired = true > > That's what they were supposed to be in the first place (see the non-devm > > version of the same method: reset_control_bulk_get_optional_exclusive()). > > > > Fixes: 48d71395896d ("reset: Add reset_control_bulk API") > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > --- > > > > Changelog v4: > > - This is a new patch added on v4 lap of the series. > > --- > > include/linux/reset.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/linux/reset.h b/include/linux/reset.h > > index 8a21b5756c3e..514ddf003efc 100644 > > --- a/include/linux/reset.h > > +++ b/include/linux/reset.h > > @@ -731,7 +731,7 @@ static inline int __must_check > > devm_reset_control_bulk_get_optional_exclusive(struct device *dev, int num_rstcs, > > struct reset_control_bulk_data *rstcs) > > { > > - return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, true, false, true); > > + return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, false, true, true); > > } > > You are right, thank you. I'll take this as a sign that the internal > functions should probably switch from collecting ever more boolean > arguments to a flags parameter, similar to enum gpiod_flags. > > I'll pick this up separately into the reset/fixes branch. Ok. Thanks. Meanwhile could you please take a look at the rest of the reset-related patches of the series? They concern the reset-control functionality updates of the Baikal-T1 CCU common-clock driver. [PATCH RESEND v5 6/8] clk: baikal-t1: Move reset-controls code into a dedicated module Link: https://lore.kernel.org/linux-clk/20220624141853.7417-7-Sergey.Semin@baikalelectronics.ru/ [PATCH RESEND v5 7/8] clk: baikal-t1: Add DDR/PCIe directly controlled resets support Link: https://lore.kernel.org/linux-clk/20220624141853.7417-8-Sergey.Semin@baikalelectronics.ru/ The functionality has been placed in the clocks subsystem because the reset-control is tightly coupled with the clock-control part of CCU. It's a normal approach for the SoC CCU/RCU devices, which normally have both clocks and reset controls intermixed. Anyway the main goal of the series was to add the directly controlled resets of the DDR and PCIe SoC sub-domains provided in the framework of the patch 7/8. Please see the patches log for details. -Sergey > > regards > Philipp
diff --git a/include/linux/reset.h b/include/linux/reset.h index 8a21b5756c3e..514ddf003efc 100644 --- a/include/linux/reset.h +++ b/include/linux/reset.h @@ -731,7 +731,7 @@ static inline int __must_check devm_reset_control_bulk_get_optional_exclusive(struct device *dev, int num_rstcs, struct reset_control_bulk_data *rstcs) { - return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, true, false, true); + return __devm_reset_control_bulk_get(dev, num_rstcs, rstcs, false, true, true); } /**
Most likely due to copy-paste mistake the device managed version of the denoted reset control getter has been implemented with invalid semantic, which can be immediately spotted by having "WARN_ON(shared && acquired)" warning in the system log as soon as the method is called. Anyway let's fix it by altering the boolean arguments passed to the __devm_reset_control_bulk_get() method from - shared = true, optional = false, acquired = true to + shared = false, optional = true, acquired = true That's what they were supposed to be in the first place (see the non-devm version of the same method: reset_control_bulk_get_optional_exclusive()). Fixes: 48d71395896d ("reset: Add reset_control_bulk API") Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> --- Changelog v4: - This is a new patch added on v4 lap of the series. --- include/linux/reset.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)