Re: [PATCH] clk: Dont return error when assigned-clocks is empty or missing
On Tue, Aug 29, 2023 at 09:18:53AM +0200, Michal Simek wrote:
>
>
> On 8/28/23 17:50, Tom Rini wrote:
> > On Mon, Aug 28, 2023 at 12:25:30PM +0200, Michal Simek wrote:
> > >
> > >
> > > On 8/25/23 16:39, Tom Rini wrote:
> > > > On Fri, Aug 25, 2023 at 09:15:09AM +0200, Michal Simek wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On 7/11/23 11:51, Ashok Reddy Soma wrote:
> > > > > > There is a chance that assigned-clock-rates is given and
> > > > > > assigned-clocks
> > > > > > could be empty. Dont return error in that case, because the probe
> > > > > > of the
> > > > > > corresponding driver will not be called at all if this fails.
> > > > > > Better to continue to look for it and return 0.
> > > > > >
> > > > > > Signed-off-by: Ashok Reddy Soma
> > > > > > ---
> > > > > >
> > > > > > drivers/clk/clk-uclass.c | 8 +++-
> > > > > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> > > > > > index dc3e9d6a26..f186fcbcdb 100644
> > > > > > --- a/drivers/clk/clk-uclass.c
> > > > > > +++ b/drivers/clk/clk-uclass.c
> > > > > > @@ -329,7 +329,13 @@ static int clk_set_default_rates(struct
> > > > > > udevice *dev,
> > > > > > dev_dbg(dev,
> > > > > > "could not get assigned clock
> > > > > > %d (err = %d)\n",
> > > > > > index, ret);
> > > > > > - continue;
> > > > > > + /* Skip if it is empty */
> > > > > > + if (ret == -ENOENT) {
> > > > > > + ret = 0;
> > > > > > + continue;
> > > > > > + }
> > > > > > +
> > > > > > + return ret;
> > > > > > }
> > > > > > /* This is clk provider device trying to
> > > > > > program itself
> > > > >
> > > > > What's your take on this one? I didn't get reply from Sean.
> > > >
> > > > I guess, what's the validated upstream dts where this is the case?
> > > >
> > >
> > > It was found by incorrect DT. It means I don't think there is any DT which
> > > contains this issue.
> > > But that being said we can extend current clock tests to cover this case.
> > > Please look below.
> >
> > Well, if the DT is invalid (and yes, we can't easily run the validation
> > suite in U-Boot today), I'd rather go with a debug we can optimize out
> > so that the next person with an invalid DT can more quickly find their
> > problem and we don't work-around it instead. Or am I missing something?
>
> There is already dev_dbg print above. It means when user enable DEBUG it
> gets information about it.
>
> The thing is that it just sync behavior with the linux kernel.
> You can look at
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-conf.c?h=v6.5#n90
>
> There is also one more example like this.
> clk-test4 {
> compatible = "sandbox,clk-test";
> assigned-clock-rates = <654>, <321>;
> assigned-clocks = <_sandbox 1>;
> };
>
> But if you prefer to fail in all these cases I am also fine with it.
Ah, OK. Yes, I guess matching the kernel here is a good idea then. And
yes, updating the tests next.
Reviewed-by: Tom Rini
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH] clk: Dont return error when assigned-clocks is empty or missing
On 8/28/23 17:50, Tom Rini wrote:
On Mon, Aug 28, 2023 at 12:25:30PM +0200, Michal Simek wrote:
On 8/25/23 16:39, Tom Rini wrote:
On Fri, Aug 25, 2023 at 09:15:09AM +0200, Michal Simek wrote:
Hi Tom,
On 7/11/23 11:51, Ashok Reddy Soma wrote:
There is a chance that assigned-clock-rates is given and assigned-clocks
could be empty. Dont return error in that case, because the probe of the
corresponding driver will not be called at all if this fails.
Better to continue to look for it and return 0.
Signed-off-by: Ashok Reddy Soma
---
drivers/clk/clk-uclass.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index dc3e9d6a26..f186fcbcdb 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -329,7 +329,13 @@ static int clk_set_default_rates(struct udevice *dev,
dev_dbg(dev,
"could not get assigned clock %d (err = %d)\n",
index, ret);
- continue;
+ /* Skip if it is empty */
+ if (ret == -ENOENT) {
+ ret = 0;
+ continue;
+ }
+
+ return ret;
}
/* This is clk provider device trying to program itself
What's your take on this one? I didn't get reply from Sean.
I guess, what's the validated upstream dts where this is the case?
It was found by incorrect DT. It means I don't think there is any DT which
contains this issue.
But that being said we can extend current clock tests to cover this case.
Please look below.
Well, if the DT is invalid (and yes, we can't easily run the validation
suite in U-Boot today), I'd rather go with a debug we can optimize out
so that the next person with an invalid DT can more quickly find their
problem and we don't work-around it instead. Or am I missing something?
There is already dev_dbg print above. It means when user enable DEBUG it gets
information about it.
The thing is that it just sync behavior with the linux kernel.
You can look at
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-conf.c?h=v6.5#n90
There is also one more example like this.
clk-test4 {
compatible = "sandbox,clk-test";
assigned-clock-rates = <654>, <321>;
assigned-clocks = <_sandbox 1>;
};
But if you prefer to fail in all these cases I am also fine with it.
Thanks,
Michal
Re: [PATCH] clk: Dont return error when assigned-clocks is empty or missing
On Mon, Aug 28, 2023 at 12:25:30PM +0200, Michal Simek wrote:
>
>
> On 8/25/23 16:39, Tom Rini wrote:
> > On Fri, Aug 25, 2023 at 09:15:09AM +0200, Michal Simek wrote:
> > > Hi Tom,
> > >
> > > On 7/11/23 11:51, Ashok Reddy Soma wrote:
> > > > There is a chance that assigned-clock-rates is given and assigned-clocks
> > > > could be empty. Dont return error in that case, because the probe of the
> > > > corresponding driver will not be called at all if this fails.
> > > > Better to continue to look for it and return 0.
> > > >
> > > > Signed-off-by: Ashok Reddy Soma
> > > > ---
> > > >
> > > >drivers/clk/clk-uclass.c | 8 +++-
> > > >1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> > > > index dc3e9d6a26..f186fcbcdb 100644
> > > > --- a/drivers/clk/clk-uclass.c
> > > > +++ b/drivers/clk/clk-uclass.c
> > > > @@ -329,7 +329,13 @@ static int clk_set_default_rates(struct udevice
> > > > *dev,
> > > > dev_dbg(dev,
> > > > "could not get assigned clock %d (err =
> > > > %d)\n",
> > > > index, ret);
> > > > - continue;
> > > > + /* Skip if it is empty */
> > > > + if (ret == -ENOENT) {
> > > > + ret = 0;
> > > > + continue;
> > > > + }
> > > > +
> > > > + return ret;
> > > > }
> > > > /* This is clk provider device trying to program itself
> > >
> > > What's your take on this one? I didn't get reply from Sean.
> >
> > I guess, what's the validated upstream dts where this is the case?
> >
>
> It was found by incorrect DT. It means I don't think there is any DT which
> contains this issue.
> But that being said we can extend current clock tests to cover this case.
> Please look below.
Well, if the DT is invalid (and yes, we can't easily run the validation
suite in U-Boot today), I'd rather go with a debug we can optimize out
so that the next person with an invalid DT can more quickly find their
problem and we don't work-around it instead. Or am I missing something?
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH] clk: Dont return error when assigned-clocks is empty or missing
On 8/25/23 16:39, Tom Rini wrote:
On Fri, Aug 25, 2023 at 09:15:09AM +0200, Michal Simek wrote:
Hi Tom,
On 7/11/23 11:51, Ashok Reddy Soma wrote:
There is a chance that assigned-clock-rates is given and assigned-clocks
could be empty. Dont return error in that case, because the probe of the
corresponding driver will not be called at all if this fails.
Better to continue to look for it and return 0.
Signed-off-by: Ashok Reddy Soma
---
drivers/clk/clk-uclass.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index dc3e9d6a26..f186fcbcdb 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -329,7 +329,13 @@ static int clk_set_default_rates(struct udevice *dev,
dev_dbg(dev,
"could not get assigned clock %d (err = %d)\n",
index, ret);
- continue;
+ /* Skip if it is empty */
+ if (ret == -ENOENT) {
+ ret = 0;
+ continue;
+ }
+
+ return ret;
}
/* This is clk provider device trying to program itself
What's your take on this one? I didn't get reply from Sean.
I guess, what's the validated upstream dts where this is the case?
It was found by incorrect DT. It means I don't think there is any DT which
contains this issue.
But that being said we can extend current clock tests to cover this case.
Please look below.
Thanks,
Michal
diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index ff9f9222e6f9..0f9bedc0065c 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -589,6 +589,16 @@
clock-names = "fixed", "i2c", "spi", "uart2", "uart1";
};
+ clk-test2 {
+ compatible = "sandbox,clk-test";
+ assigned-clock-rates = <321>;
+ };
+
+ clk-test3 {
+ compatible = "sandbox,clk-test";
+ assigned-clocks = <_sandbox 1>;
+ };
+
ccf: clk-ccf {
compatible = "sandbox,clk-ccf";
};
diff --git a/test/dm/clk.c b/test/dm/clk.c
index f48de05436bf..71ae32eaff50 100644
--- a/test/dm/clk.c
+++ b/test/dm/clk.c
@@ -36,6 +36,12 @@ static int dm_test_clk_base(struct unit_test_state *uts)
ut_asserteq(clk_is_match(_method1, _method2), true);
ut_asserteq(clk_method1.id, clk_method2.id);
+ ut_assertok(uclass_get_device_by_name(UCLASS_MISC, "clk-test2", ));
+ ut_assertok(clk_set_defaults(dev, CLK_DEFAULTS_PRE));
+
+ ut_assertok(uclass_get_device_by_name(UCLASS_MISC, "clk-test3", ));
+ ut_assertok(clk_set_defaults(dev, CLK_DEFAULTS_PRE));
+
return 0;
}
Re: [PATCH] clk: Dont return error when assigned-clocks is empty or missing
On Fri, Aug 25, 2023 at 09:15:09AM +0200, Michal Simek wrote:
> Hi Tom,
>
> On 7/11/23 11:51, Ashok Reddy Soma wrote:
> > There is a chance that assigned-clock-rates is given and assigned-clocks
> > could be empty. Dont return error in that case, because the probe of the
> > corresponding driver will not be called at all if this fails.
> > Better to continue to look for it and return 0.
> >
> > Signed-off-by: Ashok Reddy Soma
> > ---
> >
> > drivers/clk/clk-uclass.c | 8 +++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> > index dc3e9d6a26..f186fcbcdb 100644
> > --- a/drivers/clk/clk-uclass.c
> > +++ b/drivers/clk/clk-uclass.c
> > @@ -329,7 +329,13 @@ static int clk_set_default_rates(struct udevice *dev,
> > dev_dbg(dev,
> > "could not get assigned clock %d (err = %d)\n",
> > index, ret);
> > - continue;
> > + /* Skip if it is empty */
> > + if (ret == -ENOENT) {
> > + ret = 0;
> > + continue;
> > + }
> > +
> > + return ret;
> > }
> > /* This is clk provider device trying to program itself
>
> What's your take on this one? I didn't get reply from Sean.
I guess, what's the validated upstream dts where this is the case?
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH] clk: Dont return error when assigned-clocks is empty or missing
Hi Tom,
On 7/11/23 11:51, Ashok Reddy Soma wrote:
There is a chance that assigned-clock-rates is given and assigned-clocks
could be empty. Dont return error in that case, because the probe of the
corresponding driver will not be called at all if this fails.
Better to continue to look for it and return 0.
Signed-off-by: Ashok Reddy Soma
---
drivers/clk/clk-uclass.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index dc3e9d6a26..f186fcbcdb 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -329,7 +329,13 @@ static int clk_set_default_rates(struct udevice *dev,
dev_dbg(dev,
"could not get assigned clock %d (err = %d)\n",
index, ret);
- continue;
+ /* Skip if it is empty */
+ if (ret == -ENOENT) {
+ ret = 0;
+ continue;
+ }
+
+ return ret;
}
/* This is clk provider device trying to program itself
What's your take on this one? I didn't get reply from Sean.
Thanks,
Michal
Re: [PATCH] clk: Dont return error when assigned-clocks is empty or missing
Hi Sean, On 7/11/23 16:55, Michal Simek wrote: On 7/11/23 16:28, Sean Anderson wrote: On 7/11/23 10:20, Michal Simek wrote: Hi Sean, On 7/11/23 15:40, Sean Anderson wrote: On 7/11/23 05:51, Ashok Reddy Soma wrote: There is a chance that assigned-clock-rates is given and assigned-clocks could be empty. Dont return error in that case, because the probe of the corresponding driver will not be called at all if this fails. Better to continue to look for it and return 0. No, this is an error in the device tree. assigned-clock-rates depends on assigned-clocks, so you must provide the latter if the former is present. We were also checking it and in the Linux kernel it is handle like this. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-conf.c#n95 It means you can have rate assigned but not assigned-clocks property. And yes in working case both should be present to work properly. What is the use-case for this? It will not pass schema checking [1] anyway. --Sean [1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/clock/clock.yaml If you check your DT against schema. No doubt how correct behavior should be. It is just aligning behavior with Linux kernel if user messes up DT. I see that you are listed as clock maintainer. Are you fine with this alignment patch or do you want to change something or reject it? Thanks, Michal
Re: [PATCH] clk: Dont return error when assigned-clocks is empty or missing
On 7/11/23 16:28, Sean Anderson wrote: On 7/11/23 10:20, Michal Simek wrote: Hi Sean, On 7/11/23 15:40, Sean Anderson wrote: On 7/11/23 05:51, Ashok Reddy Soma wrote: There is a chance that assigned-clock-rates is given and assigned-clocks could be empty. Dont return error in that case, because the probe of the corresponding driver will not be called at all if this fails. Better to continue to look for it and return 0. No, this is an error in the device tree. assigned-clock-rates depends on assigned-clocks, so you must provide the latter if the former is present. We were also checking it and in the Linux kernel it is handle like this. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-conf.c#n95 It means you can have rate assigned but not assigned-clocks property. And yes in working case both should be present to work properly. What is the use-case for this? It will not pass schema checking [1] anyway. --Sean [1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/clock/clock.yaml If you check your DT against schema. No doubt how correct behavior should be. It is just aligning behavior with Linux kernel if user messes up DT. Thanks, Michal
Re: [PATCH] clk: Dont return error when assigned-clocks is empty or missing
On 7/11/23 10:20, Michal Simek wrote: Hi Sean, On 7/11/23 15:40, Sean Anderson wrote: On 7/11/23 05:51, Ashok Reddy Soma wrote: There is a chance that assigned-clock-rates is given and assigned-clocks could be empty. Dont return error in that case, because the probe of the corresponding driver will not be called at all if this fails. Better to continue to look for it and return 0. No, this is an error in the device tree. assigned-clock-rates depends on assigned-clocks, so you must provide the latter if the former is present. We were also checking it and in the Linux kernel it is handle like this. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-conf.c#n95 It means you can have rate assigned but not assigned-clocks property. And yes in working case both should be present to work properly. What is the use-case for this? It will not pass schema checking [1] anyway. --Sean [1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/clock/clock.yaml
Re: [PATCH] clk: Dont return error when assigned-clocks is empty or missing
Hi Sean, On 7/11/23 15:40, Sean Anderson wrote: On 7/11/23 05:51, Ashok Reddy Soma wrote: There is a chance that assigned-clock-rates is given and assigned-clocks could be empty. Dont return error in that case, because the probe of the corresponding driver will not be called at all if this fails. Better to continue to look for it and return 0. No, this is an error in the device tree. assigned-clock-rates depends on assigned-clocks, so you must provide the latter if the former is present. We were also checking it and in the Linux kernel it is handle like this. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-conf.c#n95 It means you can have rate assigned but not assigned-clocks property. And yes in working case both should be present to work properly. Thanks, Michal
Re: [PATCH] clk: Dont return error when assigned-clocks is empty or missing
On 7/11/23 05:51, Ashok Reddy Soma wrote:
There is a chance that assigned-clock-rates is given and assigned-clocks
could be empty. Dont return error in that case, because the probe of the
corresponding driver will not be called at all if this fails.
Better to continue to look for it and return 0.
No, this is an error in the device tree. assigned-clock-rates depends on
assigned-clocks, so you must provide the latter if the former is present.
--Sean
Signed-off-by: Ashok Reddy Soma
---
drivers/clk/clk-uclass.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index dc3e9d6a26..f186fcbcdb 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -329,7 +329,13 @@ static int clk_set_default_rates(struct udevice *dev,
dev_dbg(dev,
"could not get assigned clock %d (err = %d)\n",
index, ret);
- continue;
+ /* Skip if it is empty */
+ if (ret == -ENOENT) {
+ ret = 0;
+ continue;
+ }
+
+ return ret;
}
/* This is clk provider device trying to program itself
[PATCH] clk: Dont return error when assigned-clocks is empty or missing
There is a chance that assigned-clock-rates is given and assigned-clocks
could be empty. Dont return error in that case, because the probe of the
corresponding driver will not be called at all if this fails.
Better to continue to look for it and return 0.
Signed-off-by: Ashok Reddy Soma
---
drivers/clk/clk-uclass.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index dc3e9d6a26..f186fcbcdb 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -329,7 +329,13 @@ static int clk_set_default_rates(struct udevice *dev,
dev_dbg(dev,
"could not get assigned clock %d (err = %d)\n",
index, ret);
- continue;
+ /* Skip if it is empty */
+ if (ret == -ENOENT) {
+ ret = 0;
+ continue;
+ }
+
+ return ret;
}
/* This is clk provider device trying to program itself
--
2.17.1
