Re: [PATCH v4 1/4] clk: qcom: branch: Add clk_branch2_mdio_ops

[Jie Luo]

On 8/18/2023 11:29 AM, Bjorn Andersson wrote:
> On Tue, Aug 15, 2023 at 04:52:02PM +0800, Luo Jie wrote:
> > Add the clk_branch2_mdio_ops for supporting clock controller
> > where the hardware register is accessed by MDIO bus, and the
> > spin clock can't be used because of sleep during the MDIO
>
> spin clock?
>
> I believe you're trying to say that the underlying access to the MDIO
> bus can not be done in non-sleepable context and we can therefor not use
> enable/disable to operate it?

Hi Bjorn,
Thanks for the review comments.
yes, the MDIO operation can't be done in non-sleepable context, and 
enable/disable clock is using spin lock, so i enable the clock in 
.prepare ops.

will fix this typo"spin clock" to "spin lock"

> > operation.
> >
> > The clock is enabled by the .prepare instead of .enable when
> > the clk_branch2_mdio_ops is used.
> >
> > Signed-off-by: Luo Jie <quic_luoj@xxxxxxxxxxx>
> > ---
> >   drivers/clk/qcom/clk-branch.c | 7 +++++++
> >   drivers/clk/qcom/clk-branch.h | 1 +
> >   2 files changed, 8 insertions(+)
> >
> > diff --git a/drivers/clk/qcom/clk-branch.c b/drivers/clk/qcom/clk-branch.c
> > index fc4735f74f0f..5e08c026ca4a 100644
> > --- a/drivers/clk/qcom/clk-branch.c
> > +++ b/drivers/clk/qcom/clk-branch.c
> > @@ -153,3 +153,10 @@ const struct clk_ops clk_branch_simple_ops = {
> >   	.is_enabled = clk_is_enabled_regmap,
> >   };
> >   EXPORT_SYMBOL_GPL(clk_branch_simple_ops);
> > +
> > +const struct clk_ops clk_branch2_mdio_ops = {
> > +	.prepare = clk_branch2_enable,
> > +	.unprepare = clk_branch2_disable,
>
> I see none of the clocks specify halt_check, which would imply that
> these two calls just turns into clk_enable_regmap() and
> clk_disable_regmap().
>
> So, isn't this then equivalent to clk_branch_simple_ops?
>
> Regards,
> Bjorn

Thanks Bjorn for pointing this, i will add the the halt_check in the 
next patch set, halt_check is applicable.

> > +	.is_prepared = clk_is_enabled_regmap,
> > +};
> > +EXPORT_SYMBOL_GPL(clk_branch2_mdio_ops);
> > diff --git a/drivers/clk/qcom/clk-branch.h b/drivers/clk/qcom/clk-branch.h
> > index 0cf800b9d08d..4b006e8eec5e 100644
> > --- a/drivers/clk/qcom/clk-branch.h
> > +++ b/drivers/clk/qcom/clk-branch.h
> > @@ -85,6 +85,7 @@ extern const struct clk_ops clk_branch_ops;
> >   extern const struct clk_ops clk_branch2_ops;
> >   extern const struct clk_ops clk_branch_simple_ops;
> >   extern const struct clk_ops clk_branch2_aon_ops;
> > +extern const struct clk_ops clk_branch2_mdio_ops;
> >   
> >   #define to_clk_branch(_hw) \
> >   	container_of(to_clk_regmap(_hw), struct clk_branch, clkr)
> > --
> > 2.17.1