Re: [PATCH] Revert "ARM: dts: porter: Enable SCIF_CLK frequency and pins"
On Wed, Apr 06, 2016 at 09:32:06PM +0200, Sjoerd Simons wrote:
> This reverts commit 19417bd9c5112f58ea63e97ba72edabd5e1cc0fe as
> according to
> http://elinux.org/File:R-CarM2-KOELSCH_PORTER-B_PORTER_C_Comparison.pdf
> the external oscilator for SCIF_CLK is not mounted on the porter boards.
>
> Signed-off-by: Sjoerd Simons
Thanks, I will queue up the following as a fix for v4.6 based
on v4.6-rc1.
>From e885767418bff0c591cc1e45babdd25d91b4a795 Mon Sep 17 00:00:00 2001
From: Sjoerd Simons
Date: Wed, 6 Apr 2016 21:32:06 +0200
Subject: [PATCH] Revert "ARM: dts: porter: Enable SCIF_CLK frequency and pins"
This reverts commit 19417bd9c511 ("ARM: dts: porter: Enable SCIF_CLK
frequency and pins") as according to
http://elinux.org/File:R-CarM2-KOELSCH_PORTER-B_PORTER_C_Comparison.pdf
the external oscillator for SCIF_CLK is not mounted on the porter boards.
Signed-off-by: Sjoerd Simons
Acked-by: Geert Uytterhoeven
Signed-off-by: Simon Horman
---
arch/arm/boot/dts/r8a7791-porter.dts | 13 -
1 file changed, 13 deletions(-)
diff --git a/arch/arm/boot/dts/r8a7791-porter.dts
b/arch/arm/boot/dts/r8a7791-porter.dts
index 6c08314427d6..329a9dcc88e9 100644
--- a/arch/arm/boot/dts/r8a7791-porter.dts
+++ b/arch/arm/boot/dts/r8a7791-porter.dts
@@ -143,19 +143,11 @@
};
&pfc {
- pinctrl-0 = <&scif_clk_pins>;
- pinctrl-names = "default";
-
scif0_pins: serial0 {
renesas,groups = "scif0_data_d";
renesas,function = "scif0";
};
- scif_clk_pins: scif_clk {
- renesas,groups = "scif_clk";
- renesas,function = "scif_clk";
- };
-
ether_pins: ether {
renesas,groups = "eth_link", "eth_mdio", "eth_rmii";
renesas,function = "eth";
@@ -229,11 +221,6 @@
status = "okay";
};
-&scif_clk {
- clock-frequency = <14745600>;
- status = "okay";
-};
-
ðer {
pinctrl-0 = <ðer_pins &phy1_pins>;
pinctrl-names = "default";
--
2.1.4
Re: [PATCH v2] clk: let clk_disable() return immediately if clk is NULL or error
On Thu, Apr 07, 2016 at 05:33:28PM -0700, Stephen Boyd wrote:
> On 04/05, Masahiro Yamada wrote:
> > The clk_disable() in the common clock framework (drivers/clk/clk.c)
> > returns immediately if a given clk is NULL or an error pointer. It
> > allows clock consumers to call clk_disable() without IS_ERR_OR_NULL
> > checking if drivers are only used with the common clock framework.
> >
> > Unfortunately, NULL/error checking is missing from some of non-common
> > clk_disable() implementations. This prevents us from completely
> > dropping NULL/error checking from callers. Let's make it tree-wide
> > consistent by adding IS_ERR_OR_NULL(clk) to all callees.
> >
> > Signed-off-by: Masahiro Yamada
> > Acked-by: Greg Ungerer
> > Acked-by: Wan Zongshun
> > ---
> >
> > Stephen,
> >
> > This patch has been unapplied for a long time.
> >
> > Please let me know if there is something wrong with this patch.
> >
>
> I'm mostly confused why we wouldn't want to encourage people to
> call clk_disable or unprepare on a clk that's an error pointer.
> Typically an error pointer should be dealt with, instead of
> silently ignored, so why wasn't it dealt with by passing it up
> the probe() path?
While your argument makes perfect sense, Many clk_disable implementations
are already doing similar checks, for example:
arch/arm/mach-davinci/clock.c:
void clk_disable(struct clk *clk)
{
unsigned long flags;
if (clk == NULL || IS_ERR(clk))
return;
[...]
arch/arm/mach-omap1/clock.c
void clk_disable(struct clk *clk)
{
unsigned long flags;
if (clk == NULL || IS_ERR(clk))
return;
[...]
arch/avr32/mach-at32ap/clock.c
void clk_disable(struct clk *clk)
{
unsigned long flags;
if (IS_ERR_OR_NULL(clk))
return;
[...]
arch/mips/lantiq/clk.c:
static inline int clk_good(struct clk *clk)
{
return clk && !IS_ERR(clk);
}
[...]
void clk_disable(struct clk *clk)
{
if (unlikely(!clk_good(clk)))
return;
if (clk->disable)
[...]
So should we go and weed out these checks?
Ralf
Re: [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks
On Thu, 2016-04-07 at 16:21 -0700, Stephen Boyd wrote:
> On 04/06, Sjoerd Simons wrote:
> >
> > On Wed, 2016-04-06 at 15:11 +0200, Geert Uytterhoeven wrote:
> > >
> > > CC Mike, Stephen, linux-clk (this time with the new Mike)
> > >
> > > On Wed, Apr 6, 2016 at 2:52 PM, Sjoerd Simons
> > > wrote:
> > > >
> > > >
> > > > clk_get on a disabled clock node will return EPROBE_DEFER,
> > > > which
> > > > can
> > > > cause drivers to be deferred forever if such clocks are
> > > > referenced
> > > > in
> > > > their clocks property.
> > > Is this a side effect of commit 3e5dd6f6e690048d ("clk: Ignore
> > > disabled DT
> > > clock providers")?
> > Yes it seems so. Reverting that patch means that i can drop this
> > one
> > and get the expected behaviour again.
> The DT is broken then? Is it possible to mark these status =
> "okay" so that things work again?
> >
> >
> > Though even so I'm not sure what the convention is for clocks like
> > these, the r8a7791.dtsi is inconsistent, as some are disabled while
> > others (e.g. the audio clocks) are 0hz. Would be good to get some
> > input
> > on that regardless.
> >
> What's the question here?
So the question is how to model unconnected external clocks in device-
tree.
The dtsi we're loooking at has (in pseudo dt):
device {
clock-names = "internal", "external";
clocks = <&internal, &external>
};
external {
compatible = "fixed-clock";
clock-frequency = <12345>;
status = "disabled";
};
Before 3e5dd6f6e690048d ("clk: Ignore disabled DT clock providers")
this apparently worked. Afterwards drivers getting all the clocks would
fail to probe with -EPROBE_DEFER.
Judging by your comment I assume this way of modelling it is broken
(and the behaviour caused by the patch is correct)?
And as a follow-up, is modelling unconnected clocks as enabled with a
frequency of 0hz as my proposed patch does seen as the right way of
doing things?
--
Sjoerd Simons
Collabora Ltd.
Re: [PATCH v2] clk: let clk_disable() return immediately if clk is NULL or error
2016-04-08 19:06 GMT+09:00 Ralf Baechle :
> On Thu, Apr 07, 2016 at 05:33:28PM -0700, Stephen Boyd wrote:
>
>> On 04/05, Masahiro Yamada wrote:
>> > The clk_disable() in the common clock framework (drivers/clk/clk.c)
>> > returns immediately if a given clk is NULL or an error pointer. It
>> > allows clock consumers to call clk_disable() without IS_ERR_OR_NULL
>> > checking if drivers are only used with the common clock framework.
>> >
>> > Unfortunately, NULL/error checking is missing from some of non-common
>> > clk_disable() implementations. This prevents us from completely
>> > dropping NULL/error checking from callers. Let's make it tree-wide
>> > consistent by adding IS_ERR_OR_NULL(clk) to all callees.
>> >
>> > Signed-off-by: Masahiro Yamada
>> > Acked-by: Greg Ungerer
>> > Acked-by: Wan Zongshun
>> > ---
>> >
>> > Stephen,
>> >
>> > This patch has been unapplied for a long time.
>> >
>> > Please let me know if there is something wrong with this patch.
>> >
>>
>> I'm mostly confused why we wouldn't want to encourage people to
>> call clk_disable or unprepare on a clk that's an error pointer.
>> Typically an error pointer should be dealt with, instead of
>> silently ignored, so why wasn't it dealt with by passing it up
>> the probe() path?
>
> While your argument makes perfect sense, Many clk_disable implementations
> are already doing similar checks, for example:
>
> arch/arm/mach-davinci/clock.c:
>
> void clk_disable(struct clk *clk)
> {
> unsigned long flags;
>
> if (clk == NULL || IS_ERR(clk))
> return;
> [...]
>
> arch/arm/mach-omap1/clock.c
>
> void clk_disable(struct clk *clk)
> {
> unsigned long flags;
>
> if (clk == NULL || IS_ERR(clk))
> return;
> [...]
>
> arch/avr32/mach-at32ap/clock.c
>
> void clk_disable(struct clk *clk)
> {
> unsigned long flags;
>
> if (IS_ERR_OR_NULL(clk))
> return;
> [...]
>
> arch/mips/lantiq/clk.c:
>
> static inline int clk_good(struct clk *clk)
> {
> return clk && !IS_ERR(clk);
> }
>
> [...]
>
> void clk_disable(struct clk *clk)
> {
> if (unlikely(!clk_good(clk)))
> return;
>
> if (clk->disable)
> [...]
>
> So should we go and weed out these checks?
>
> Ralf
Please help me understand your thought clearly.
[1] Should calling clk_unprepare/disable() with a NULL pointer be allowed?
[2] Should calling clk_unprepare/disable() with an error pointer be allowed?
--
Best Regards
Masahiro Yamada
[PATCH RESEND] net: ethernet: renesas: ravb_main: test clock rate to avoid division by 0
From: Wolfram Sang The clk API may return 0 on clk_get_rate, so we should check the result before using it as a divisor. Signed-off-by: Wolfram Sang Acked-by: Sergei Shtylyov --- The original patch was marked as "NOT APPLICABLE" in patchwork. I reviewed again and can't see why. So, in case this remains true, please explain. drivers/net/ethernet/renesas/ravb_main.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 4e1a7dba7c4abb..791930b63991dc 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -1691,6 +1691,9 @@ static int ravb_set_gti(struct net_device *ndev) rate = clk_get_rate(clk); clk_put(clk); + if (!rate) + return -EINVAL; + inc = 10ULL << 20; do_div(inc, rate); -- 2.7.0
RE: [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks
Hi,
On 07 April 2016 20:14, Sergei Shtylyov wrote:
> On 04/07/2016 10:00 AM, Sjoerd Simons wrote:
>
> > Hey Sergei,
> >
> > Thanks for your review.
>
> You're welcome. :-)
>
> > On Thu, 2016-04-07 at 02:15 +0300, Sergei Shtylyov wrote:
> >> On 04/06/2016 03:52 PM, Sjoerd Simons wrote:
> >>
> >>>
> >>> clk_get on a disabled clock node will return EPROBE_DEFER, which
> >>> can
> >>> cause drivers to be deferred forever if such clocks are referenced
> >>> in
> >>> their clocks property.
> >>>
> >>> Update the various disabled external clock nodes to default to a
> >>> frequency of 0, but don't disable them to prevent this.
> >>>
> >>> Signed-off-by: Sjoerd Simons
> >>>
> >>> ---
> >>>
> >>>arch/arm/boot/dts/r8a7791-koelsch.dts | 1 +
> >>>arch/arm/boot/dts/r8a7791-porter.dts | 1 +
> >>>arch/arm/boot/dts/r8a7791.dtsi| 5 +
> >>>3 files changed, 3 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts
> >>> b/arch/arm/boot/dts/r8a7791-koelsch.dts
> >>> index 1adf877..da59c28 100644
> >>> --- a/arch/arm/boot/dts/r8a7791-koelsch.dts
> >>> +++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
> >>> @@ -660,6 +660,7 @@
> >>>};
> >>>
> >>>&pcie_bus_clk {
> >>> + clock-frequency = <1>;
>
> >> Hmmm, looking at the Koelsch schematics, I don't see this clock.
> >> :-/
> >
> > I don't have the schematics so i was simply keeping the current state.
>
> I understand. I was just surprised by what checking the code against the
> schematics revealed.
>
> > I've added Phil Edworthy to the list as he was the one originally
>
> I should've CC'ed Phil myself...
>
> > enable the bus clk for Koelsh according to git. Hopefully he can
> > clarify :)
>
> Let's hope he'd reply...
>
> >>> status = "okay";
> >>>};
> >>>
> >>> diff --git a/arch/arm/boot/dts/r8a7791-porter.dts
> >>> b/arch/arm/boot/dts/r8a7791-porter.dts
> >>> index 9554d13..19b257e 100644
> >>> --- a/arch/arm/boot/dts/r8a7791-porter.dts
> >>> +++ b/arch/arm/boot/dts/r8a7791-porter.dts
> >>> @@ -413,6 +413,7 @@
> >>>};
> >>>
> >>>&pcie_bus_clk {
> >>> + clock-frequency = <1>;
> >>> status = "okay";
> >>>};
> >>>
> >> Again, looking at the Porter schematics, I don't see this clock
> >> either. :-/
> >
> > You were the one enabling this clock for Porter ;) I don't have PCIE
>
> Yes, of course. I don't remember the gory details already -- I might have
> blindly copied what was in the Koelsch's .dts.
>
> > hardware to test with on my porter board, might be worth if you could
> > disable this clock and see if PCI-E still fucntions as expected (maybe
> > in practise it just happens to prefer the internal clock?) ?
>
> I know the PCIe driver does require this clock in order to function -- it
> would be the same eternal -EPROBE_DEFER condition you'd already described;
> see drivers/pci/host/pcie-rcar.c yourself...
>
> >>> diff --git a/arch/arm/boot/dts/r8a7791.dtsi
> >>> b/arch/arm/boot/dts/r8a7791.dtsi
> >>> index 8693888..676df63 100644
> >>> --- a/arch/arm/boot/dts/r8a7791.dtsi
> >>> +++ b/arch/arm/boot/dts/r8a7791.dtsi
> >>> @@ -1104,8 +1104,7 @@
> >>> pcie_bus_clk: pcie_bus {
> >>> compatible = "fixed-clock";
> >>> #clock-cells = <0>;
> >>> - clock-frequency = <1>;
> >>> - status = "disabled";
> >>> + clock-frequency = <0>;
> >> If the clock has a good default frequency, I don't think you need
> >> to
> >> remove it. Otherwise you missed USB_EXTAL which is 48 MHz (and can be
> >> overridden).
> >
> > I did that as it was by default disabled, so if i do enable it but
> > don't drop the default frequency to 0 board swithout that clock will
> > suddenly have it added to their dtb.
>
> Zero frequency is hardly any better then the default...
>
> > For the usb external clock I didn't touch it as it was already enabled
> > by default with a proper frequency, so it wouldn't hit the issue i was
> > trying to fix here. But i agree, both looking at other Renesas dtsis
> > and how all other external clocks are done in this dtsi, this node is
> > odd.
>
> It's not that odd given that the R-Car gen2 manual specifically says it
> should be 48 MHz.
> Looking into the manual again, the PCIe bus clock must be the onne
> presented on the CICREF[PN]1_SATA/PCIe input signals and it indeed should be
> 100 MHz... So Phil was correct.
PCIe always requires a 100MHz reference clock. On Koelsch, this clock is
generated by U6 and always on. Some boards may need additional hardware
to be setup in order to output the clock, hence the DT node for the clock.
I hope that clarifies the situation!
Phil
