From: Michael Tretter <m.tretter@pengutronix.de>
To: Stephen Boyd <sboyd@kernel.org>
Cc: devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-clk@vger.kernel.org, rajanv@xilinx.com, tejasp@xilinx.com,
dshah@xilinx.com, rvisaval@xilinx.com, michals@xilinx.com,
kernel@pengutronix.de, robh+dt@kernel.org,
mturquette@baylibre.com
Subject: Re: [PATCH 08/12] soc: xilinx: vcu: implement clock provider for output clocks
Date: Tue, 15 Dec 2020 12:38:09 +0100 [thread overview]
Message-ID: <20201215113809.GA23407@pengutronix.de> (raw)
In-Reply-To: <160783893475.1580929.17041767429276672732@swboyd.mtv.corp.google.com>
On Sat, 12 Dec 2020 21:55:34 -0800, Stephen Boyd wrote:
> Quoting Michael Tretter (2020-11-15 23:55:28)
> > diff --git a/drivers/soc/xilinx/xlnx_vcu.c b/drivers/soc/xilinx/xlnx_vcu.c
> > index 725e646aa726..cedc8b7859f7 100644
> > --- a/drivers/soc/xilinx/xlnx_vcu.c
> > +++ b/drivers/soc/xilinx/xlnx_vcu.c
> > @@ -545,6 +512,146 @@ static int xvcu_set_pll(struct xvcu_device *xvcu)
> > return xvcu_pll_enable(xvcu);
> > }
> >
> > +static struct clk_hw *xvcu_clk_hw_register_leaf(struct device *dev,
> > + const char *name,
> > + const char * const *parent_names,
> > + u8 num_parents,
> > + struct clk_hw *parent_default,
> > + void __iomem *reg,
> > + spinlock_t *lock)
> > +{
> > + unsigned long flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT;
> > + u8 divider_flags = CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO |
>
> Why u8?
__clk_hw_register_divider expects u8 as divider_flags.
>
> > + CLK_DIVIDER_ROUND_CLOSEST;
> > + struct clk_hw *mux = NULL;
> > + struct clk_hw *divider = NULL;
> > + struct clk_hw *gate = NULL;
> > + char *name_mux;
> > + char *name_div;
> > + int err;
> > +
> > + name_mux = devm_kasprintf(dev, GFP_KERNEL, "%s%s", name, "_mux");
> > + if (!name_mux) {
> > + err = -ENOMEM;
> > + goto err;
> > + }
> > + mux = clk_hw_register_mux(dev, name_mux, parent_names, num_parents,
> > + flags, reg, 0, 1, 0, lock);
> > + if (IS_ERR(mux)) {
> > + err = PTR_ERR(divider);
> > + goto err;
> > + }
> > + clk_hw_set_parent(mux, parent_default);
>
> Can this be done from assigned-clock-parents binding?
Could be done, if the driver provides at least the PLL and the mux in addition
to the actual output clocks. Otherwise, it is not possible to reference the
PLL post divider and the mux from the device tree. I wanted to avoid to expose
the complexity to the device tree. Would you prefer, if all clocks are
provided instead of only the output clocks?
>
> > +
> > + name_div = devm_kasprintf(dev, GFP_KERNEL, "%s%s", name, "_div");
> > + if (!name_div) {
> > + err = -ENOMEM;
> > + goto err;
> > + }
> > + divider = clk_hw_register_divider_parent_hw(dev, name_div, mux, flags,
> > + reg, 4, 6, divider_flags,
> > + lock);
> > + if (IS_ERR(divider)) {
> > + err = PTR_ERR(divider);
> > + goto err;
> > + }
> > +
> > + gate = clk_hw_register_gate_parent_hw(dev, name, divider,
> > + CLK_SET_RATE_PARENT, reg, 12, 0,
> > + lock);
> > + if (IS_ERR(gate)) {
> > + err = PTR_ERR(gate);
> > + goto err;
> > + }
> > +
> > + return gate;
> > +
> > +err:
> > + if (!IS_ERR_OR_NULL(gate))
>
> Would be nicer to have some more goto labels and skip the IS_ERR_OR_NULL
> checks.
Ack.
>
> > + clk_hw_unregister_gate(gate);
> > + if (!IS_ERR_OR_NULL(divider))
> > + clk_hw_unregister_divider(divider);
> > + if (!IS_ERR_OR_NULL(mux))
> > + clk_hw_unregister_divider(mux);
> > +
> > + return ERR_PTR(err);
> > +}
> > +
> > +static void xvcu_clk_hw_unregister_leaf(struct clk_hw *hw)
> > +{
> > + struct clk_hw *gate = hw;
> > + struct clk_hw *divider;
> > + struct clk_hw *mux;
> > +
> > + if (!gate)
> > + return;
> > +
> > + divider = clk_hw_get_parent(gate);
> > + clk_hw_unregister_gate(gate);
> > + if (!divider)
> > + return;
> > +
> > + mux = clk_hw_get_parent(divider);
> > + clk_hw_unregister_mux(mux);
> > + if (!divider)
> > + return;
> > +
> > + clk_hw_unregister_divider(divider);
> > +}
> > +
> > +static DEFINE_SPINLOCK(venc_core_lock);
> > +static DEFINE_SPINLOCK(venc_mcu_lock);
>
> Any reason to not allocate these spinlocks too?
I will change this.
>
> > +
> > +static int xvcu_register_clock_provider(struct xvcu_device *xvcu)
> > +{
> > + struct device *dev = xvcu->dev;
> > + const char *parent_names[2];
> > + struct clk_hw *parent_default;
> > + struct clk_hw_onecell_data *data;
> > + struct clk_hw **hws;
> > + void __iomem *reg_base = xvcu->vcu_slcr_ba;
> > +
> > + data = devm_kzalloc(dev, struct_size(data, hws, CLK_XVCU_NUM_CLOCKS), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > + data->num = CLK_XVCU_NUM_CLOCKS;
> > + hws = data->hws;
> > +
> > + xvcu->clk_data = data;
> > +
> > + parent_default = xvcu->pll;
> > + parent_names[0] = "dummy";
>
> What is "dummy"?
According to the register reference [0], the output clocks can be driven by an
external clock instead of the PLL, but the VCU Product Guide [1] does not
mention any ports for actually driving the clock. From my understanding of the
IP core, this is a clock mux which has a not-connected first parent. Maybe
someone at Xilinx can clarify, what is happening here.
[0] https://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers.html
[1] https://www.xilinx.com/support/documentation-navigation/see-all-versions.html?xlnxproducttypes=IP%20Cores&xlnxipcoresname=v-vcu
What would be a better way to handle this?
>
> > + parent_names[1] = clk_hw_get_name(parent_default);
>
> Can we use the new way of specifying clk parents from DT or by using
> direct pointers instead of using string names? The idea is to get rid of
> clk_hw_get_name() eventually.
It should be possible to use the direct pointers, but this really depends on
how the "dummy" clock is handled.
Thanks,
Michael
>
> > +
> > + hws[CLK_XVCU_ENC_CORE] =
> > + xvcu_clk_hw_register_leaf(dev, "venc_core_clk",
> > + parent_names,
> > + ARRAY_SIZE(parent_names),
> > + parent_default,
> > + reg_base + VCU_ENC_CORE_CTRL,
> > + &venc_core_lock);
> > + hws[CLK_XVCU_ENC_MCU] =
> > + xvcu_clk_hw_register_leaf(dev, "venc_mcu_clk",
> > + parent_names,
> > + ARRAY_SIZE(parent_names),
> > + parent_default,
> > + reg_base + VCU_ENC_MCU_CTRL,
> > + &venc_mcu_lock);
> > +
>
next prev parent reply other threads:[~2020-12-15 11:39 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-16 7:55 [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michael Tretter
2020-11-16 7:55 ` [PATCH 01/12] ARM: dts: define indexes for output clocks Michael Tretter
2020-12-02 14:33 ` Michal Simek
2020-12-07 19:21 ` Rob Herring
2020-12-13 5:44 ` Stephen Boyd
2020-11-16 7:55 ` [PATCH 02/12] clk: divider: fix initialization with parent_hw Michael Tretter
2020-12-02 14:28 ` Michal Simek
2020-12-13 5:42 ` Stephen Boyd
2020-11-16 7:55 ` [PATCH 03/12] soc: xilinx: vcu: drop coreclk from struct xlnx_vcu Michael Tretter
2020-11-16 7:55 ` [PATCH 04/12] soc: xilinx: vcu: add helper to wait for PLL locked Michael Tretter
2020-11-16 7:55 ` [PATCH 05/12] soc: xilinx: vcu: add helpers for configuring PLL Michael Tretter
2020-11-16 7:55 ` [PATCH 06/12] soc: xilinx: vcu: implement PLL disable Michael Tretter
2020-11-16 7:55 ` [PATCH 07/12] soc: xilinx: vcu: register PLL as fixed rate clock Michael Tretter
2020-12-02 14:41 ` Michal Simek
2020-11-16 7:55 ` [PATCH 08/12] soc: xilinx: vcu: implement clock provider for output clocks Michael Tretter
2020-12-02 14:49 ` Michal Simek
2020-12-13 5:55 ` Stephen Boyd
2020-12-15 11:38 ` Michael Tretter [this message]
2020-12-16 1:09 ` Stephen Boyd
2020-12-21 9:18 ` Michael Tretter
2020-11-16 7:55 ` [PATCH 09/12] soc: xilinx: vcu: make pll post divider explicit Michael Tretter
2020-12-02 14:51 ` Michal Simek
2020-11-16 7:55 ` [PATCH 10/12] soc: xilinx: vcu: make the PLL configurable Michael Tretter
2020-12-02 14:54 ` Michal Simek
2020-11-16 7:55 ` [PATCH 11/12] soc: xilinx: vcu: remove calculation of PLL configuration Michael Tretter
2020-11-16 7:55 ` [PATCH 12/12] soc: xilinx: vcu: use bitfields for register definition Michael Tretter
2020-12-13 5:47 ` Stephen Boyd
2020-12-03 7:46 ` [PATCH 00/12] soc: xilinx: vcu: Convert driver to clock provider Michal Simek
2020-12-03 9:00 ` Michael Tretter
2020-12-03 9:14 ` Michal Simek
2020-12-13 5:50 ` Stephen Boyd
2020-12-15 11:56 ` Michael Tretter
2020-12-16 2:37 ` Stephen Boyd
2020-12-21 9:19 ` Michael Tretter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201215113809.GA23407@pengutronix.de \
--to=m.tretter@pengutronix.de \
--cc=devicetree@vger.kernel.org \
--cc=dshah@xilinx.com \
--cc=kernel@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=michals@xilinx.com \
--cc=mturquette@baylibre.com \
--cc=rajanv@xilinx.com \
--cc=robh+dt@kernel.org \
--cc=rvisaval@xilinx.com \
--cc=sboyd@kernel.org \
--cc=tejasp@xilinx.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).