| Message ID | 1461699585-6649-2-git-send-email-eric@anholt.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> On 26.04.2016, at 21:39, Eric Anholt <eric@anholt.net> wrote: > > If the firmware had set up a clock to source from PLLC, go along with > it. But if we're looking for a new parent, we don't want to switch it > to PLLC because the firmware will force PLLC (and thus the AXI bus > clock) to different frequencies during over-temp/under-voltage, > without notification to Linux. > > On my system, this moves the Linux-enabled HDMI state machine and DSI1 > escape clock over to plld_per from pllc_per. EMMC still ends up on > pllc_per, because the firmware had set it up to use that. > > Signed-off-by: Eric Anholt <eric@anholt.net> > Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks") > — I guess this patch looks to me as if it is a policy inside the kernel, which is AFAIK frowned upon. I am looking into making "assigned-clock-parents” inside the dt work with the driver. Could look something like this: i2s: i2s@7e203000 { assigned-clock-parents = <&cprman BCM2835_PLLD_PER>, <&clk_osc>; assigned-clocks = <&cprman BCM2835_CLOCK_PCM>, <&cprman BCM2835_CLOCK_PCM>; }; (not sure if that works really - the same clock in assigned-clocks looks suspicious) This would move the policy out of the kernel into the device-tree, which - i guess is a better solution. Martin
On 30.04.2016 11:28, Martin Sperl wrote: >> On 26.04.2016, at 21:39, Eric Anholt <eric@anholt.net> wrote: >> >> If the firmware had set up a clock to source from PLLC, go along with >> it. But if we're looking for a new parent, we don't want to switch it >> to PLLC because the firmware will force PLLC (and thus the AXI bus >> clock) to different frequencies during over-temp/under-voltage, >> without notification to Linux. >> >> On my system, this moves the Linux-enabled HDMI state machine and DSI1 >> escape clock over to plld_per from pllc_per. EMMC still ends up on >> pllc_per, because the firmware had set it up to use that. >> >> Signed-off-by: Eric Anholt <eric@anholt.net> >> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks") >> — > I guess this patch looks to me as if it is a policy inside the kernel, > which is AFAIK frowned upon. > > I am looking into making "assigned-clock-parents” inside the dt > work with the driver. > > Could look something like this: > i2s: i2s@7e203000 { > assigned-clock-parents = <&cprman BCM2835_PLLD_PER>, <&clk_osc>; > assigned-clocks = <&cprman BCM2835_CLOCK_PCM>, <&cprman BCM2835_CLOCK_PCM>; > }; > (not sure if that works really - the same clock in assigned-clocks looks suspicious) > > This would move the policy out of the kernel into the device-tree, > which - i guess is a better solution. So after some more investigation it seems that we can not really use those assigned-clock-parents properties for our purpose of filtering the parent clocks, as it: a) requires also assigned-clocks to be set (this may be OK) b) it does not allow to define a list of clocks to get used - it will just set the parent of the assigned-clock - if we take the example shown above, it would call clk_set_parent 2 times for the PCM clock - once with PLLD_PER and once with clk_osc. So I start to wonder if it would not be better to use an approach like this: cprman: cprman@7e101000 { ... brcm,clock-flags = <flags for PCM>, <flags for PWM>; brcm,clock-index = <BCM2835_CLOCK_PCM>, <BCM2835_CLOCK_PWM>; } the flags would be a bitfield that select the parent clocks. So it could look like this: cprman: cprman@7e101000 { ... brcm,clock-flags = (BIT(BCM2835_PER_PARENT_OSC) | BIT(BCM2835_PER_PARENT_PLLD_PER)), ...; brcm,clock-index = <BCM2835_CLOCK_PCM>, <BCM2835_CLOCK_PWM>; } BCM2835_PER_PARENT_PLLD_PER and BCM2835_PER_PARENT_OSC would then be defined in include/dt-bindings/clock/bcm2835.h In addition this would also allow us to add other flags to enable higher order MASH clock dividers - we currently only allow simple fractional dividers - we could even force the use of integer dividers if there comes a need. This would really allow us to define the parents freely and if the firmware ever changes its behavior with regards to PLLC, then we can easily change the device-tree. Is this approach acceptable - maybe in a variation? Thanks, Martin
Martin Sperl <kernel@martin.sperl.org> writes: >> On 26.04.2016, at 21:39, Eric Anholt <eric@anholt.net> wrote: >> >> If the firmware had set up a clock to source from PLLC, go along with >> it. But if we're looking for a new parent, we don't want to switch it >> to PLLC because the firmware will force PLLC (and thus the AXI bus >> clock) to different frequencies during over-temp/under-voltage, >> without notification to Linux. >> >> On my system, this moves the Linux-enabled HDMI state machine and DSI1 >> escape clock over to plld_per from pllc_per. EMMC still ends up on >> pllc_per, because the firmware had set it up to use that. >> >> Signed-off-by: Eric Anholt <eric@anholt.net> >> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks") >> — > > I guess this patch looks to me as if it is a policy inside the kernel, > which is AFAIK frowned upon. Can you come up with a use for putting peripherals on PLLC ever, such that we need choice?
> On 02.05.2016, at 17:29, Eric Anholt <eric@anholt.net> wrote: > > Martin Sperl <kernel@martin.sperl.org> writes: > >>> On 26.04.2016, at 21:39, Eric Anholt <eric@anholt.net> wrote: >>> >>> If the firmware had set up a clock to source from PLLC, go along with >>> it. But if we're looking for a new parent, we don't want to switch it >>> to PLLC because the firmware will force PLLC (and thus the AXI bus >>> clock) to different frequencies during over-temp/under-voltage, >>> without notification to Linux. >>> >>> On my system, this moves the Linux-enabled HDMI state machine and DSI1 >>> escape clock over to plld_per from pllc_per. EMMC still ends up on >>> pllc_per, because the firmware had set it up to use that. >>> >>> Signed-off-by: Eric Anholt <eric@anholt.net> >>> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks") >>> — >> >> I guess this patch looks to me as if it is a policy inside the kernel, >> which is AFAIK frowned upon. > > Can you come up with a use for putting peripherals on PLLC ever, such > that we need choice? For PLLC not right now, but with clk_notifier_register drivers could work around those clock changes (assuming we get that information from the firmware somehow - or if we could move this decision into the kernel: even better). But I can come up with a scenario that would make use of the pllh_aux under some circumstances - e.g when requesting 290039Hz on clock gp0/1/2. Similarly: if we ever enable the testdebugX clocks these become immediate candidates for parent-clocks as well which can result in more headache. Being able to define which clocks to use at least give the dts author a means also to control clock selection if he wants to enable the testdebug clocks. Another similar situation can occur with plla_per - under some circumstances it may get used unexpectedly. Martin
Martin Sperl <kernel@martin.sperl.org> writes: >> On 02.05.2016, at 17:29, Eric Anholt <eric@anholt.net> wrote: >> >> Martin Sperl <kernel@martin.sperl.org> writes: >> >>>> On 26.04.2016, at 21:39, Eric Anholt <eric@anholt.net> wrote: >>>> >>>> If the firmware had set up a clock to source from PLLC, go along with >>>> it. But if we're looking for a new parent, we don't want to switch it >>>> to PLLC because the firmware will force PLLC (and thus the AXI bus >>>> clock) to different frequencies during over-temp/under-voltage, >>>> without notification to Linux. >>>> >>>> On my system, this moves the Linux-enabled HDMI state machine and DSI1 >>>> escape clock over to plld_per from pllc_per. EMMC still ends up on >>>> pllc_per, because the firmware had set it up to use that. >>>> >>>> Signed-off-by: Eric Anholt <eric@anholt.net> >>>> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks") >>>> — >>> >>> I guess this patch looks to me as if it is a policy inside the kernel, >>> which is AFAIK frowned upon. >> >> Can you come up with a use for putting peripherals on PLLC ever, such >> that we need choice? > > For PLLC not right now, but with clk_notifier_register drivers could > work around those clock changes (assuming we get that information > from the firmware somehow - or if we could move this decision into the > kernel: even better). Why would you want to automatically choose an unstable clock instead of the stable clock we have available? > But I can come up with a scenario that would make use of the pllh_aux > under some circumstances - e.g when requesting 290039Hz on clock gp0/1/2. > > Similarly: if we ever enable the testdebugX clocks these become immediate > candidates for parent-clocks as well which can result in more headache. How are you planning to make use of the testdebug inputs? As far as I know, those are for bit-banging your clocks during hardware bringup debugging. They wouldn't be clocks you'd automatically choose. > Being able to define which clocks to use at least give the dts author > a means also to control clock selection if he wants to enable the > testdebug clocks. If you were to clock-assigned-parents something to PLLC, this code won't override that.
diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c index 14f3066194ac..870c5745d649 100644 --- a/drivers/clk/bcm/clk-bcm2835.c +++ b/drivers/clk/bcm/clk-bcm2835.c @@ -1035,16 +1035,28 @@ static int bcm2835_clock_set_rate(struct clk_hw *hw, return 0; } +static bool +bcm2835_clk_is_pllc(struct clk_hw *hw) +{ + if (!hw) + return false; + + return strncmp(clk_hw_get_name(hw), "pllc", 4) == 0; +} + static int bcm2835_clock_determine_rate(struct clk_hw *hw, struct clk_rate_request *req) { struct bcm2835_clock *clock = bcm2835_clock_from_hw(hw); struct clk_hw *parent, *best_parent = NULL; + bool current_parent_is_pllc; unsigned long rate, best_rate = 0; unsigned long prate, best_prate = 0; size_t i; u32 div; + current_parent_is_pllc = bcm2835_clk_is_pllc(clk_hw_get_parent(hw)); + /* * Select parent clock that results in the closest but lower rate */ @@ -1052,6 +1064,17 @@ static int bcm2835_clock_determine_rate(struct clk_hw *hw, parent = clk_hw_get_parent_by_index(hw, i); if (!parent) continue; + + /* + * Don't choose a PLLC-derived clock as our parent + * unless it had been manually set that way. PLLC's + * frequency gets adjusted by the firmware due to + * over-temp or under-voltage conditions, without + * prior notification to our clock consumer. + */ + if (bcm2835_clk_is_pllc(parent) && !current_parent_is_pllc) + continue; + prate = clk_hw_get_rate(parent); div = bcm2835_clock_choose_div(hw, req->rate, prate, true); rate = bcm2835_clock_rate_from_divisor(clock, prate, div);
If the firmware had set up a clock to source from PLLC, go along with it. But if we're looking for a new parent, we don't want to switch it to PLLC because the firmware will force PLLC (and thus the AXI bus clock) to different frequencies during over-temp/under-voltage, without notification to Linux. On my system, this moves the Linux-enabled HDMI state machine and DSI1 escape clock over to plld_per from pllc_per. EMMC still ends up on pllc_per, because the firmware had set it up to use that. Signed-off-by: Eric Anholt <eric@anholt.net> Fixes: 41691b8862e2 ("clk: bcm2835: Add support for programming the audio domain clocks") --- drivers/clk/bcm/clk-bcm2835.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)