| Message ID | 20231004003125.2289613-1-dmitry.baryshkov@linaro.org |
|---|---|
| Headers | show |
| Series | clk: qcom: provide alternative 'parked' RCG | expand |
On 07/10/2023 00:45, Konrad Dybcio wrote: > On 4.10.2023 14:52, Bryan O'Donoghue wrote: >> On 04/10/2023 13:08, Dmitry Baryshkov wrote: >>> On Wed, 4 Oct 2023 at 12:27, Bryan O'Donoghue >>> <bryan.odonoghue@linaro.org> wrote: >>>> >>>> On 04/10/2023 01:31, Dmitry Baryshkov wrote: >>>>> clk_rcg2_shared_ops implements support for the case of the RCG which >>>>> must not be completely turned off. However its design has one major >>>>> drawback: it doesn't allow us to properly implement the is_enabled >>>>> callback, which causes different kinds of misbehaviour from the CCF. >>>>> >>>>> Follow the idea behind clk_regmap_phy_mux_ops and implement the new >>>>> clk_rcg2_parked_ops. It also targets the clocks which must not be fully >>>>> switched off (and shared most of the implementation with >>>>> clk_rcg2_shared_ops). The major difference is that it requires that the >>>>> parent map doesn't conain the safe (parked) clock source. Instead if the >>>>> CFG_REG register points to the safe source, the clock is considered to >>>>> be disabled. >>>> >>>> Why not have a new bit in .flags ? >>>> >>>> Instead of lying about the clock being off, mark the clock as "parked", >>>> or "safe parked" or whatever term we choose for it ? >>> >>> The main problem with adding flags doesn't fully scale. From the CCF >>> perspective, what should be the difference between parked and disabled >>> clocks? How should it treat the parked one? >> >> Exactly the same as a disabled clock, except you get a "parked" instead of a "disabled" when looking up its state and you don't have to > The thing is that currently there's only the notion of "enabled" > or "not enabled".. Introducing a third state here would be the > jump from boolean to quantum logic! > > I think that abstracting this information away from Linux is not > an issue.. These clocks "can't be any more off", or the SoC will > explode badly and Linux will be unusable.. > > Think of it like CPUs with a hypervisor, you shut them down, but > the physical number crunchers on the host CPU may not actually > get cut off from their power source, but there's no reason for > the VM to know that. That's probably what happens on our little > virtualized snapdragons anyway.. > > Konrad So not a state but a flag. 1. The clock tree we declare _should_ be a fair and complete description of the hardware clock tree. 2. If we remove XO from some trees with the only indication of differentiation being the callback you bind the tree to you need someone reading the code both know about parking, derive that information from reading the callback, which means you require that person to read the code in detail and understand it. That's alot of tribal knowledge we are storing up there. 3. A different approach is to add a new CLK_DISABLE_PARKS_TO_XO - or whatever name makes sense. a) The clock tree declared in the gcc, camcc, dispcc, videocc or is correct and aligned with the documentation and silicon. Right away this avoids patches sent to 'fixup' incomplete trees. b) When you look at a clock struct clk_branch_gcc.clk.hw.init.flags there is a big dirty CLK_DOES_THIS_THING flag which doesn't require a developer to have tribal knowledge about how we've hacked up clock parking. My basic point here is the declaration of a parked clock should be obvious, easy to understand and not lend itself to "helpful" patches to "fix" the clock tree. Also consider precedent. When you want to quickly get your clock controller up and running - you generally open existing upstream stuff to clone and own as much as possible. A BIT_DIRTY_FLAG transmits more information than a small callback with esoteric logic buried inside of the disable path. I agree with your point on a new state but similarly I think the callback buries too much information away. IMO the top level clock declaration - rather like the DT should as closely as possible declare an accurate clock tree. If we need to do special stuff to an individual tree, then CLK_FLAG it. Are qcom clocks really the only clocks in the world that need to park to XO on the disable path ? Alternatively continue on with the callback but make the name more instructive not "park" since we are dealing with people who have English as a second language, third language. English is my first language but still a "parked" clock means little to me except that like you and Dmitry I work with qcom stuff so I understand it. "disable_park_xo->clk_disable" or something - even still I think removing XO from the clock tree is asking for trouble. Start from the principle that gcc/camcc/dispcc clock trees should be complete and work from there. That's my 0.02 anyway. --- bod
On 07/10/2023 13:08, Bryan O'Donoghue wrote: > On 07/10/2023 00:45, Konrad Dybcio wrote: >> On 4.10.2023 14:52, Bryan O'Donoghue wrote: >>> On 04/10/2023 13:08, Dmitry Baryshkov wrote: >>>> On Wed, 4 Oct 2023 at 12:27, Bryan O'Donoghue >>>> <bryan.odonoghue@linaro.org> wrote: >>>>> >>>>> On 04/10/2023 01:31, Dmitry Baryshkov wrote: >>>>>> clk_rcg2_shared_ops implements support for the case of the RCG which >>>>>> must not be completely turned off. However its design has one major >>>>>> drawback: it doesn't allow us to properly implement the is_enabled >>>>>> callback, which causes different kinds of misbehaviour from the CCF. >>>>>> >>>>>> Follow the idea behind clk_regmap_phy_mux_ops and implement the new >>>>>> clk_rcg2_parked_ops. It also targets the clocks which must not be >>>>>> fully >>>>>> switched off (and shared most of the implementation with >>>>>> clk_rcg2_shared_ops). The major difference is that it requires >>>>>> that the >>>>>> parent map doesn't conain the safe (parked) clock source. Instead >>>>>> if the >>>>>> CFG_REG register points to the safe source, the clock is >>>>>> considered to >>>>>> be disabled. >>>>> >>>>> Why not have a new bit in .flags ? >>>>> >>>>> Instead of lying about the clock being off, mark the clock as >>>>> "parked", >>>>> or "safe parked" or whatever term we choose for it ? >>>> >>>> The main problem with adding flags doesn't fully scale. From the CCF >>>> perspective, what should be the difference between parked and disabled >>>> clocks? How should it treat the parked one? >>> >>> Exactly the same as a disabled clock, except you get a "parked" >>> instead of a "disabled" when looking up its state and you don't have to >> The thing is that currently there's only the notion of "enabled" >> or "not enabled".. Introducing a third state here would be the >> jump from boolean to quantum logic! >> >> I think that abstracting this information away from Linux is not >> an issue.. These clocks "can't be any more off", or the SoC will >> explode badly and Linux will be unusable.. >> >> Think of it like CPUs with a hypervisor, you shut them down, but >> the physical number crunchers on the host CPU may not actually >> get cut off from their power source, but there's no reason for >> the VM to know that. That's probably what happens on our little >> virtualized snapdragons anyway.. >> >> Konrad > > So not a state but a flag. > > 1. The clock tree we declare _should_ be a fair and complete description > of the hardware clock tree. Yes and no. We already have clocks not present in the tree for different reasons: being handled by the RPM(h), being critical to the platform integrity, being useless for Linux, etc. > > 2. If we remove XO from some trees with the only indication of > differentiation being the callback you bind the tree to you need > someone reading the code both know about parking, derive that > information from reading the callback, which means you require that > person to read the code in detail and understand it. > > That's alot of tribal knowledge we are storing up there. I think adding a huge comment should help. Because otherwise you sound like 'we should not expect kernel developers to read the code', which is not true. > > 3. A different approach is to add a new CLK_DISABLE_PARKS_TO_XO - or > whatever name makes sense. > > a) The clock tree declared in the gcc, camcc, dispcc, videocc or > is correct and aligned with the documentation and silicon. > Right away this avoids patches sent to 'fixup' incomplete trees. > > b) When you look at a clock struct clk_branch_gcc.clk.hw.init.flags > there is a big dirty CLK_DOES_THIS_THING flag which doesn't > require a developer to have tribal knowledge about how we've > hacked up clock parking. But the problem is that this flag is not generic at all. I think we will be damned and prosecuted if we try adding anything about PARK_TO_XO to <linux/clk-provider.h>. And also there is always a question on the state integrity: if the clock is parented with the XO on the driver bootstrap, should CCF treat it as 'parked' or as 'enabled, clocked by XO'? > > My basic point here is the declaration of a parked clock should be > obvious, easy to understand and not lend itself to "helpful" patches to > "fix" the clock tree. We already tried doing that... And it failed. For the PHY PIPE clocks we ended up doing exactly the same thing, because it simplified the code _a_lot_. > Also consider precedent. When you want to quickly get your clock > controller up and running - you generally open existing upstream stuff > to clone and own as much as possible. A BIT_DIRTY_FLAG transmits more > information than a small callback with esoteric logic buried inside of > the disable path. > > I agree with your point on a new state but similarly I think the > callback buries too much information away. IMO the top level clock > declaration - rather like the DT should as closely as possible declare > an accurate clock tree. > > If we need to do special stuff to an individual tree, then CLK_FLAG it. > Are qcom clocks really the only clocks in the world that need to park to > XO on the disable path ? > > Alternatively continue on with the callback but make the name more > instructive not "park" since we are dealing with people who have English > as a second language, third language. English is my first language but > still a "parked" clock means little to me except that like you and > Dmitry I work with qcom stuff so I understand it. > > "disable_park_xo->clk_disable" or something - even still I think > removing XO from the clock tree is asking for trouble. clk_rcg2_disable_parks_to_tcxo_ops ? Slightly ugly but I'm fine with that. > > Start from the principle that gcc/camcc/dispcc clock trees should be > complete and work from there. > > That's my 0.02 anyway. >
On 10/7/23 01:43, Konrad Dybcio wrote: > On 4.10.2023 02:31, Dmitry Baryshkov wrote: >> clk_rcg2_shared_ops implements support for the case of the RCG which >> must not be completely turned off. However its design has one major >> drawback: it doesn't allow us to properly implement the is_enabled >> callback, which causes different kinds of misbehaviour from the CCF. >> >> Follow the idea behind clk_regmap_phy_mux_ops and implement the new >> clk_rcg2_parked_ops. It also targets the clocks which must not be fully >> switched off (and shared most of the implementation with >> clk_rcg2_shared_ops). The major difference is that it requires that the >> parent map doesn't conain the safe (parked) clock source. Instead if the >> CFG_REG register points to the safe source, the clock is considered to >> be disabled. >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- > Would the intention here be to replace all usages of _shared_? ? Konrad
On Thu, 26 Oct 2023 at 21:57, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > On 10/7/23 01:43, Konrad Dybcio wrote: > > On 4.10.2023 02:31, Dmitry Baryshkov wrote: > >> clk_rcg2_shared_ops implements support for the case of the RCG which > >> must not be completely turned off. However its design has one major > >> drawback: it doesn't allow us to properly implement the is_enabled > >> callback, which causes different kinds of misbehaviour from the CCF. > >> > >> Follow the idea behind clk_regmap_phy_mux_ops and implement the new > >> clk_rcg2_parked_ops. It also targets the clocks which must not be fully > >> switched off (and shared most of the implementation with > >> clk_rcg2_shared_ops). The major difference is that it requires that the > >> parent map doesn't conain the safe (parked) clock source. Instead if the > >> CFG_REG register points to the safe source, the clock is considered to > >> be disabled. > >> > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >> --- > > Would the intention here be to replace all usages of _shared_? Yes
On 10/26/23 22:47, Dmitry Baryshkov wrote: > On Thu, 26 Oct 2023 at 21:57, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: >> >> >> >> On 10/7/23 01:43, Konrad Dybcio wrote: >>> On 4.10.2023 02:31, Dmitry Baryshkov wrote: >>>> clk_rcg2_shared_ops implements support for the case of the RCG which >>>> must not be completely turned off. However its design has one major >>>> drawback: it doesn't allow us to properly implement the is_enabled >>>> callback, which causes different kinds of misbehaviour from the CCF. >>>> >>>> Follow the idea behind clk_regmap_phy_mux_ops and implement the new >>>> clk_rcg2_parked_ops. It also targets the clocks which must not be fully >>>> switched off (and shared most of the implementation with >>>> clk_rcg2_shared_ops). The major difference is that it requires that the >>>> parent map doesn't conain the safe (parked) clock source. Instead if the >>>> CFG_REG register points to the safe source, the clock is considered to >>>> be disabled. >>>> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>>> --- >>> Would the intention here be to replace all usages of _shared_? > > Yes Then I suppose an immediate followup question would be: "why introduce new ops instead of replacing the existing ones in the patchset?". Konrad
On Thu, 26 Oct 2023 at 23:49, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > > > > On 10/26/23 22:47, Dmitry Baryshkov wrote: > > On Thu, 26 Oct 2023 at 21:57, Konrad Dybcio <konrad.dybcio@linaro.org> wrote: > >> > >> > >> > >> On 10/7/23 01:43, Konrad Dybcio wrote: > >>> On 4.10.2023 02:31, Dmitry Baryshkov wrote: > >>>> clk_rcg2_shared_ops implements support for the case of the RCG which > >>>> must not be completely turned off. However its design has one major > >>>> drawback: it doesn't allow us to properly implement the is_enabled > >>>> callback, which causes different kinds of misbehaviour from the CCF. > >>>> > >>>> Follow the idea behind clk_regmap_phy_mux_ops and implement the new > >>>> clk_rcg2_parked_ops. It also targets the clocks which must not be fully > >>>> switched off (and shared most of the implementation with > >>>> clk_rcg2_shared_ops). The major difference is that it requires that the > >>>> parent map doesn't conain the safe (parked) clock source. Instead if the > >>>> CFG_REG register points to the safe source, the clock is considered to > >>>> be disabled. > >>>> > >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >>>> --- > >>> Would the intention here be to replace all usages of _shared_? > > > > Yes > Then I suppose an immediate followup question would be: "why > introduce new ops instead of replacing the existing ones in the > patchset?". Because using this ops requires doing more than just replacing the name. it also requires dropping the XO source from the parent maps. So I'd prefer to perform this migration on a driver-by-driver basis. Otherwise it might be very easy to introduce a mistake somewhere.
On Wed, Oct 04, 2023 at 03:31:24AM +0300, Dmitry Baryshkov wrote: > clk_rcg2_shared_ops implements support for the case of the RCG which > must not be completely turned off. However its design has one major > drawback: it doesn't allow us to properly implement the is_enabled > callback, which causes different kinds of misbehaviour from the CCF. I have some behaviors in mind when reading this, others might not. Please give some specific behavior(s) here. Thanks, Bjorn > > Follow the idea behind clk_regmap_phy_mux_ops and implement the new > clk_rcg2_parked_ops. It also targets the clocks which must not be fully > switched off (and shared most of the implementation with > clk_rcg2_shared_ops). The major difference is that it requires that the > parent map doesn't conain the safe (parked) clock source. Instead if the > CFG_REG register points to the safe source, the clock is considered to > be disabled. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/clk/qcom/clk-rcg.h | 1 + > drivers/clk/qcom/clk-rcg2.c | 34 ++++++++++++++++++++++++++++++++++ > 2 files changed, 35 insertions(+) > > diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h > index e6d84c8c7989..9fbbf1251564 100644 > --- a/drivers/clk/qcom/clk-rcg.h > +++ b/drivers/clk/qcom/clk-rcg.h > @@ -176,6 +176,7 @@ extern const struct clk_ops clk_byte2_ops; > extern const struct clk_ops clk_pixel_ops; > extern const struct clk_ops clk_gfx3d_ops; > extern const struct clk_ops clk_rcg2_shared_ops; > +extern const struct clk_ops clk_rcg2_parked_ops; > extern const struct clk_ops clk_dp_ops; > > struct clk_rcg_dfs_data { > diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c > index 5183c74b074f..3f52abf0025e 100644 > --- a/drivers/clk/qcom/clk-rcg2.c > +++ b/drivers/clk/qcom/clk-rcg2.c > @@ -5,6 +5,7 @@ > > #include <linux/kernel.h> > #include <linux/bitops.h> > +#include <linux/bitfield.h> > #include <linux/err.h> > #include <linux/bug.h> > #include <linux/export.h> > @@ -1150,6 +1151,39 @@ const struct clk_ops clk_rcg2_shared_ops = { > }; > EXPORT_SYMBOL_GPL(clk_rcg2_shared_ops); > > +static int clk_rcg2_park_is_enabled(struct clk_hw *hw) > +{ > + struct clk_rcg2 *rcg = to_clk_rcg2(hw); > + u32 cfg; > + int ret; > + > + if (!clk_rcg2_is_enabled(hw)) > + return false; > + > + ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &cfg); > + if (ret) > + return ret; > + > + return FIELD_GET(CFG_SRC_SEL_MASK, cfg) != rcg->safe_src_index; > +} > + > +/* > + * Unlike clk_rcg2_shared_ops, the safe_src_index aka XO must NOT be present in > + * parent_map. This allows us to implement proper is_enabled callback. > + */ > +const struct clk_ops clk_rcg2_parked_ops = { > + .is_enabled = clk_rcg2_park_is_enabled, > + .enable = clk_rcg2_shared_enable, > + .disable = clk_rcg2_shared_disable, > + .get_parent = clk_rcg2_shared_get_parent, > + .set_parent = clk_rcg2_shared_set_parent, > + .recalc_rate = clk_rcg2_shared_recalc_rate, > + .determine_rate = clk_rcg2_determine_rate, > + .set_rate = clk_rcg2_shared_set_rate, > + .set_rate_and_parent = clk_rcg2_shared_set_rate_and_parent, > +}; > +EXPORT_SYMBOL_GPL(clk_rcg2_parked_ops); > + > /* Common APIs to be used for DFS based RCGR */ > static void clk_rcg2_dfs_populate_freq(struct clk_hw *hw, unsigned int l, > struct freq_tbl *f) > -- > 2.39.2 >
On Fri, 27 Oct 2023 at 01:56, Bjorn Andersson <quic_bjorande@quicinc.com> wrote: > > On Wed, Oct 04, 2023 at 03:31:24AM +0300, Dmitry Baryshkov wrote: > > clk_rcg2_shared_ops implements support for the case of the RCG which > > must not be completely turned off. However its design has one major > > drawback: it doesn't allow us to properly implement the is_enabled > > callback, which causes different kinds of misbehaviour from the CCF. > > I have some behaviors in mind when reading this, others might not. > Please give some specific behavior(s) here. Bjorn (and other interested parties). At this RFC stage it would be really nice to check whether the patch idea is worth the trouble and if it fixes the issue. > > Thanks, > Bjorn > > > > > Follow the idea behind clk_regmap_phy_mux_ops and implement the new > > clk_rcg2_parked_ops. It also targets the clocks which must not be fully > > switched off (and shared most of the implementation with > > clk_rcg2_shared_ops). The major difference is that it requires that the > > parent map doesn't conain the safe (parked) clock source. Instead if the > > CFG_REG register points to the safe source, the clock is considered to > > be disabled. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/clk/qcom/clk-rcg.h | 1 + > > drivers/clk/qcom/clk-rcg2.c | 34 ++++++++++++++++++++++++++++++++++ > > 2 files changed, 35 insertions(+) > > > > diff --git a/drivers/clk/qcom/clk-rcg.h b/drivers/clk/qcom/clk-rcg.h > > index e6d84c8c7989..9fbbf1251564 100644 > > --- a/drivers/clk/qcom/clk-rcg.h > > +++ b/drivers/clk/qcom/clk-rcg.h > > @@ -176,6 +176,7 @@ extern const struct clk_ops clk_byte2_ops; > > extern const struct clk_ops clk_pixel_ops; > > extern const struct clk_ops clk_gfx3d_ops; > > extern const struct clk_ops clk_rcg2_shared_ops; > > +extern const struct clk_ops clk_rcg2_parked_ops; > > extern const struct clk_ops clk_dp_ops; > > > > struct clk_rcg_dfs_data { > > diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c > > index 5183c74b074f..3f52abf0025e 100644 > > --- a/drivers/clk/qcom/clk-rcg2.c > > +++ b/drivers/clk/qcom/clk-rcg2.c > > @@ -5,6 +5,7 @@ > > > > #include <linux/kernel.h> > > #include <linux/bitops.h> > > +#include <linux/bitfield.h> > > #include <linux/err.h> > > #include <linux/bug.h> > > #include <linux/export.h> > > @@ -1150,6 +1151,39 @@ const struct clk_ops clk_rcg2_shared_ops = { > > }; > > EXPORT_SYMBOL_GPL(clk_rcg2_shared_ops); > > > > +static int clk_rcg2_park_is_enabled(struct clk_hw *hw) > > +{ > > + struct clk_rcg2 *rcg = to_clk_rcg2(hw); > > + u32 cfg; > > + int ret; > > + > > + if (!clk_rcg2_is_enabled(hw)) > > + return false; > > + > > + ret = regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &cfg); > > + if (ret) > > + return ret; > > + > > + return FIELD_GET(CFG_SRC_SEL_MASK, cfg) != rcg->safe_src_index; > > +} > > + > > +/* > > + * Unlike clk_rcg2_shared_ops, the safe_src_index aka XO must NOT be present in > > + * parent_map. This allows us to implement proper is_enabled callback. > > + */ > > +const struct clk_ops clk_rcg2_parked_ops = { > > + .is_enabled = clk_rcg2_park_is_enabled, > > + .enable = clk_rcg2_shared_enable, > > + .disable = clk_rcg2_shared_disable, > > + .get_parent = clk_rcg2_shared_get_parent, > > + .set_parent = clk_rcg2_shared_set_parent, > > + .recalc_rate = clk_rcg2_shared_recalc_rate, > > + .determine_rate = clk_rcg2_determine_rate, > > + .set_rate = clk_rcg2_shared_set_rate, > > + .set_rate_and_parent = clk_rcg2_shared_set_rate_and_parent, > > +}; > > +EXPORT_SYMBOL_GPL(clk_rcg2_parked_ops); > > + > > /* Common APIs to be used for DFS based RCGR */ > > static void clk_rcg2_dfs_populate_freq(struct clk_hw *hw, unsigned int l, > > struct freq_tbl *f) > > -- > > 2.39.2 > >