* [PATCH] clk: vc5: Abort clock configuration without upstream clock
@ 2018-12-15 0:55 Marek Vasut
2018-12-16 7:19 ` Laurent Pinchart
2019-01-09 18:54 ` Stephen Boyd
0 siblings, 2 replies; 7+ messages in thread
From: Marek Vasut @ 2018-12-15 0:55 UTC (permalink / raw)
To: linux-clk
Cc: linux-renesas-soc, Marek Vasut, Alexey Firago, Laurent Pinchart,
Stephen Boyd
In case the upstream clock are not set, which can happen in case the
VC5 has no valid upstream clock, the $src variable is used uninited
by regmap_update_bits(). Check for this condition and return -EINVAL
in such case.
Note that in case the VC5 has no valid upstream clock, the VC5 can
not operate correctly. That is a hardware property of the VC5. The
internal oscilator present in some VC5 models is also considered
upstream clock.
Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Alexey Firago <alexey_firago@mentor.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-renesas-soc@vger.kernel.org
---
NOTE: This is an updated version of:
https://patchwork.kernel.org/patch/10731699/
---
drivers/clk/clk-versaclock5.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
index 5b393e711e94..b10801506518 100644
--- a/drivers/clk/clk-versaclock5.c
+++ b/drivers/clk/clk-versaclock5.c
@@ -262,8 +262,10 @@ static int vc5_mux_set_parent(struct clk_hw *hw, u8 index)
if (vc5->clk_mux_ins == VC5_MUX_IN_XIN)
src = VC5_PRIM_SRC_SHDN_EN_XTAL;
- if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN)
+ else if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN)
src = VC5_PRIM_SRC_SHDN_EN_CLKIN;
+ else
+ return -EINVAL;
}
return regmap_update_bits(vc5->regmap, VC5_PRIM_SRC_SHDN, mask, src);
--
2.18.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] clk: vc5: Abort clock configuration without upstream clock
2018-12-15 0:55 [PATCH] clk: vc5: Abort clock configuration without upstream clock Marek Vasut
@ 2018-12-16 7:19 ` Laurent Pinchart
2018-12-16 17:14 ` Marek Vasut
2019-01-09 18:54 ` Stephen Boyd
1 sibling, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2018-12-16 7:19 UTC (permalink / raw)
To: Marek Vasut
Cc: linux-clk, linux-renesas-soc, Marek Vasut, Alexey Firago, Stephen Boyd
Hi Marek,
Thank you for the patch.
On Saturday, 15 December 2018 02:55:19 EET Marek Vasut wrote:
> In case the upstream clock are not set, which can happen in case the
> VC5 has no valid upstream clock, the $src variable is used uninited
> by regmap_update_bits(). Check for this condition and return -EINVAL
> in such case.
Note that the probe() function will fail in this case, so vc5_mux_set_parent()
won't be reached.
> Note that in case the VC5 has no valid upstream clock, the VC5 can
> not operate correctly. That is a hardware property of the VC5. The
> internal oscilator present in some VC5 models is also considered
> upstream clock.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Alexey Firago <alexey_firago@mentor.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-renesas-soc@vger.kernel.org
> ---
> NOTE: This is an updated version of:
> https://patchwork.kernel.org/patch/10731699/
> ---
> drivers/clk/clk-versaclock5.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
> index 5b393e711e94..b10801506518 100644
> --- a/drivers/clk/clk-versaclock5.c
> +++ b/drivers/clk/clk-versaclock5.c
> @@ -262,8 +262,10 @@ static int vc5_mux_set_parent(struct clk_hw *hw, u8
> index)
>
> if (vc5->clk_mux_ins == VC5_MUX_IN_XIN)
> src = VC5_PRIM_SRC_SHDN_EN_XTAL;
> - if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN)
> + else if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN)
> src = VC5_PRIM_SRC_SHDN_EN_CLKIN;
> + else
> + return -EINVAL;
> }
I'd rather go for Stephen's approach if the goal is just to silence a warning
for a condition that can't happen in practice.
> return regmap_update_bits(vc5->regmap, VC5_PRIM_SRC_SHDN, mask, src);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] clk: vc5: Abort clock configuration without upstream clock
2018-12-16 7:19 ` Laurent Pinchart
@ 2018-12-16 17:14 ` Marek Vasut
2018-12-28 22:30 ` Stephen Boyd
0 siblings, 1 reply; 7+ messages in thread
From: Marek Vasut @ 2018-12-16 17:14 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-clk, linux-renesas-soc, Marek Vasut, Alexey Firago, Stephen Boyd
On 12/16/2018 08:19 AM, Laurent Pinchart wrote:
> Hi Marek,
>
> Thank you for the patch.
>
> On Saturday, 15 December 2018 02:55:19 EET Marek Vasut wrote:
>> In case the upstream clock are not set, which can happen in case the
>> VC5 has no valid upstream clock, the $src variable is used uninited
>> by regmap_update_bits(). Check for this condition and return -EINVAL
>> in such case.
>
> Note that the probe() function will fail in this case, so vc5_mux_set_parent()
> won't be reached.
>
>> Note that in case the VC5 has no valid upstream clock, the VC5 can
>> not operate correctly. That is a hardware property of the VC5. The
>> internal oscilator present in some VC5 models is also considered
>> upstream clock.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>> Cc: Alexey Firago <alexey_firago@mentor.com>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Stephen Boyd <sboyd@kernel.org>
>> Cc: linux-renesas-soc@vger.kernel.org
>> ---
>> NOTE: This is an updated version of:
>> https://patchwork.kernel.org/patch/10731699/
>> ---
>> drivers/clk/clk-versaclock5.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
>> index 5b393e711e94..b10801506518 100644
>> --- a/drivers/clk/clk-versaclock5.c
>> +++ b/drivers/clk/clk-versaclock5.c
>> @@ -262,8 +262,10 @@ static int vc5_mux_set_parent(struct clk_hw *hw, u8
>> index)
>>
>> if (vc5->clk_mux_ins == VC5_MUX_IN_XIN)
>> src = VC5_PRIM_SRC_SHDN_EN_XTAL;
>> - if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN)
>> + else if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN)
>> src = VC5_PRIM_SRC_SHDN_EN_CLKIN;
>> + else
>> + return -EINVAL;
>> }
>
> I'd rather go for Stephen's approach if the goal is just to silence a warning
> for a condition that can't happen in practice.
Sure, probe will fail, but it's safer to handle the possibility that
probe() is broken and this code is reached by properly handling the
failure instead of doing something obviously wrong (like configuring the
hardware with value 0).
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] clk: vc5: Abort clock configuration without upstream clock
2018-12-16 17:14 ` Marek Vasut
@ 2018-12-28 22:30 ` Stephen Boyd
2018-12-29 16:34 ` Laurent Pinchart
0 siblings, 1 reply; 7+ messages in thread
From: Stephen Boyd @ 2018-12-28 22:30 UTC (permalink / raw)
To: Laurent Pinchart, Marek Vasut
Cc: linux-clk, linux-renesas-soc, Marek Vasut, Alexey Firago
Quoting Marek Vasut (2018-12-16 09:14:29)
> On 12/16/2018 08:19 AM, Laurent Pinchart wrote:
> > Hi Marek,
> >
> > Thank you for the patch.
> >
> > On Saturday, 15 December 2018 02:55:19 EET Marek Vasut wrote:
> >> In case the upstream clock are not set, which can happen in case the
> >> VC5 has no valid upstream clock, the $src variable is used uninited
> >> by regmap_update_bits(). Check for this condition and return -EINVAL
> >> in such case.
> >
> > Note that the probe() function will fail in this case, so vc5_mux_set_parent()
> > won't be reached.
> >
> >> Note that in case the VC5 has no valid upstream clock, the VC5 can
> >> not operate correctly. That is a hardware property of the VC5. The
> >> internal oscilator present in some VC5 models is also considered
> >> upstream clock.
> >>
> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >> Cc: Alexey Firago <alexey_firago@mentor.com>
> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Cc: Stephen Boyd <sboyd@kernel.org>
> >> Cc: linux-renesas-soc@vger.kernel.org
> >> ---
> >> NOTE: This is an updated version of:
> >> https://patchwork.kernel.org/patch/10731699/
> >> ---
> >> drivers/clk/clk-versaclock5.c | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
> >> index 5b393e711e94..b10801506518 100644
> >> --- a/drivers/clk/clk-versaclock5.c
> >> +++ b/drivers/clk/clk-versaclock5.c
> >> @@ -262,8 +262,10 @@ static int vc5_mux_set_parent(struct clk_hw *hw, u8
> >> index)
> >>
> >> if (vc5->clk_mux_ins == VC5_MUX_IN_XIN)
> >> src = VC5_PRIM_SRC_SHDN_EN_XTAL;
> >> - if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN)
> >> + else if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN)
> >> src = VC5_PRIM_SRC_SHDN_EN_CLKIN;
> >> + else
> >> + return -EINVAL;
> >> }
> >
> > I'd rather go for Stephen's approach if the goal is just to silence a warning
> > for a condition that can't happen in practice.
>
> Sure, probe will fail, but it's safer to handle the possibility that
> probe() is broken and this code is reached by properly handling the
> failure instead of doing something obviously wrong (like configuring the
> hardware with value 0).
I'm fine with this approach. Laurent?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] clk: vc5: Abort clock configuration without upstream clock
2018-12-28 22:30 ` Stephen Boyd
@ 2018-12-29 16:34 ` Laurent Pinchart
2019-01-09 18:49 ` Stephen Boyd
0 siblings, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2018-12-29 16:34 UTC (permalink / raw)
To: Stephen Boyd
Cc: Marek Vasut, linux-clk, linux-renesas-soc, Marek Vasut, Alexey Firago
Hi Stephen,
On Saturday, 29 December 2018 00:30:16 EET Stephen Boyd wrote:
> Quoting Marek Vasut (2018-12-16 09:14:29)
> > On 12/16/2018 08:19 AM, Laurent Pinchart wrote:
> > > On Saturday, 15 December 2018 02:55:19 EET Marek Vasut wrote:
> > >> In case the upstream clock are not set, which can happen in case the
> > >> VC5 has no valid upstream clock, the $src variable is used uninited
> > >> by regmap_update_bits(). Check for this condition and return -EINVAL
> > >> in such case.
> > >
> > > Note that the probe() function will fail in this case, so
> > > vc5_mux_set_parent() won't be reached.
> > >
> > >> Note that in case the VC5 has no valid upstream clock, the VC5 can
> > >> not operate correctly. That is a hardware property of the VC5. The
> > >> internal oscilator present in some VC5 models is also considered
> > >> upstream clock.
> > >>
> > >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> > >> Cc: Alexey Firago <alexey_firago@mentor.com>
> > >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >> Cc: Stephen Boyd <sboyd@kernel.org>
> > >> Cc: linux-renesas-soc@vger.kernel.org
> > >> ---
> > >>
> > >> NOTE: This is an updated version of:
> > >> https://patchwork.kernel.org/patch/10731699/
> > >>
> > >> ---
> > >>
> > >> drivers/clk/clk-versaclock5.c | 4 +++-
> > >> 1 file changed, 3 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/clk/clk-versaclock5.c
> > >> b/drivers/clk/clk-versaclock5.c
> > >> index 5b393e711e94..b10801506518 100644
> > >> --- a/drivers/clk/clk-versaclock5.c
> > >> +++ b/drivers/clk/clk-versaclock5.c
> > >> @@ -262,8 +262,10 @@ static int vc5_mux_set_parent(struct clk_hw *hw,
> > >> u8> index)
> > >>
> > >> if (vc5->clk_mux_ins == VC5_MUX_IN_XIN)
> > >> src = VC5_PRIM_SRC_SHDN_EN_XTAL;
> > >> - if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN)
> > >> + else if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN)
> > >> src = VC5_PRIM_SRC_SHDN_EN_CLKIN;
> > >> + else
> > >> + return -EINVAL;
> > >> }
> > >
> > > I'd rather go for Stephen's approach if the goal is just to silence a
> > > warning for a condition that can't happen in practice.
> >
> > Sure, probe will fail, but it's safer to handle the possibility that
> > probe() is broken and this code is reached by properly handling the
> > failure instead of doing something obviously wrong (like configuring the
> > hardware with value 0).
>
> I'm fine with this approach. Laurent?
So am I. I might add a comment in the vc5_mu_set_parent() function to explain
this can't happen.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] clk: vc5: Abort clock configuration without upstream clock
2018-12-29 16:34 ` Laurent Pinchart
@ 2019-01-09 18:49 ` Stephen Boyd
0 siblings, 0 replies; 7+ messages in thread
From: Stephen Boyd @ 2019-01-09 18:49 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Marek Vasut, linux-clk, linux-renesas-soc, Marek Vasut, Alexey Firago
Quoting Laurent Pinchart (2018-12-29 08:34:18)
> Hi Stephen,
>
> On Saturday, 29 December 2018 00:30:16 EET Stephen Boyd wrote:
> > Quoting Marek Vasut (2018-12-16 09:14:29)
> > > On 12/16/2018 08:19 AM, Laurent Pinchart wrote:
> > > > On Saturday, 15 December 2018 02:55:19 EET Marek Vasut wrote:
> > > >> In case the upstream clock are not set, which can happen in case the
> > > >> VC5 has no valid upstream clock, the $src variable is used uninited
> > > >> by regmap_update_bits(). Check for this condition and return -EINVAL
> > > >> in such case.
> > > >
> > > > Note that the probe() function will fail in this case, so
> > > > vc5_mux_set_parent() won't be reached.
> > > >
> > > >> Note that in case the VC5 has no valid upstream clock, the VC5 can
> > > >> not operate correctly. That is a hardware property of the VC5. The
> > > >> internal oscilator present in some VC5 models is also considered
> > > >> upstream clock.
> > > >>
> > > >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> > > >> Cc: Alexey Firago <alexey_firago@mentor.com>
> > > >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > >> Cc: Stephen Boyd <sboyd@kernel.org>
> > > >> Cc: linux-renesas-soc@vger.kernel.org
> > > >> ---
> > > >>
> > > >> NOTE: This is an updated version of:
> > > >> https://patchwork.kernel.org/patch/10731699/
> > > >>
> > > >> ---
> > > >>
> > > >> drivers/clk/clk-versaclock5.c | 4 +++-
> > > >> 1 file changed, 3 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/drivers/clk/clk-versaclock5.c
> > > >> b/drivers/clk/clk-versaclock5.c
> > > >> index 5b393e711e94..b10801506518 100644
> > > >> --- a/drivers/clk/clk-versaclock5.c
> > > >> +++ b/drivers/clk/clk-versaclock5.c
> > > >> @@ -262,8 +262,10 @@ static int vc5_mux_set_parent(struct clk_hw *hw,
> > > >> u8> index)
> > > >>
> > > >> if (vc5->clk_mux_ins == VC5_MUX_IN_XIN)
> > > >> src = VC5_PRIM_SRC_SHDN_EN_XTAL;
> > > >> - if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN)
> > > >> + else if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN)
> > > >> src = VC5_PRIM_SRC_SHDN_EN_CLKIN;
> > > >> + else
> > > >> + return -EINVAL;
> > > >> }
> > > >
> > > > I'd rather go for Stephen's approach if the goal is just to silence a
> > > > warning for a condition that can't happen in practice.
> > >
> > > Sure, probe will fail, but it's safer to handle the possibility that
> > > probe() is broken and this code is reached by properly handling the
> > > failure instead of doing something obviously wrong (like configuring the
> > > hardware with value 0).
> >
> > I'm fine with this approach. Laurent?
>
> So am I. I might add a comment in the vc5_mu_set_parent() function to explain
> this can't happen.
>
Ok. I can add a note and throw this into clk-fixes.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] clk: vc5: Abort clock configuration without upstream clock
2018-12-15 0:55 [PATCH] clk: vc5: Abort clock configuration without upstream clock Marek Vasut
2018-12-16 7:19 ` Laurent Pinchart
@ 2019-01-09 18:54 ` Stephen Boyd
1 sibling, 0 replies; 7+ messages in thread
From: Stephen Boyd @ 2019-01-09 18:54 UTC (permalink / raw)
To: Marek Vasut, linux-clk
Cc: linux-renesas-soc, Marek Vasut, Alexey Firago, Laurent Pinchart
Quoting Marek Vasut (2018-12-14 16:55:19)
> In case the upstream clock are not set, which can happen in case the
> VC5 has no valid upstream clock, the $src variable is used uninited
> by regmap_update_bits(). Check for this condition and return -EINVAL
> in such case.
>
> Note that in case the VC5 has no valid upstream clock, the VC5 can
> not operate correctly. That is a hardware property of the VC5. The
> internal oscilator present in some VC5 models is also considered
> upstream clock.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Alexey Firago <alexey_firago@mentor.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-renesas-soc@vger.kernel.org
> ---
Applied to clk-fixes
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-01-09 18:54 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-15 0:55 [PATCH] clk: vc5: Abort clock configuration without upstream clock Marek Vasut
2018-12-16 7:19 ` Laurent Pinchart
2018-12-16 17:14 ` Marek Vasut
2018-12-28 22:30 ` Stephen Boyd
2018-12-29 16:34 ` Laurent Pinchart
2019-01-09 18:49 ` Stephen Boyd
2019-01-09 18:54 ` Stephen Boyd
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).