All of lore.kernel.org
 help / color / mirror / Atom feed
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
To: Jian Hu <jian.hu@amlogic.com>
Cc: Jerome Brunet <jbrunet@baylibre.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Kevin Hilman <khilman@baylibre.com>,
	Rob Herring <robh@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Qiufang Dai <qiufang.dai@amlogic.com>,
	Jianxin Pan <jianxin.pan@amlogic.com>,
	Victor Wan <victor.wan@amlogic.com>,
	Chandle Zou <chandle.zou@amlogic.com>,
	linux-clk@vger.kernel.org, linux-amlogic@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 5/5] clk: meson: a1: add support for Amlogic A1 Peripheral clock driver
Date: Fri, 27 Dec 2019 18:22:52 +0100	[thread overview]
Message-ID: <CAFBinCB_0+k6rGxChpB77rPUrb-0mzxt_nQWXbiztCJnJq8XnQ@mail.gmail.com> (raw)
In-Reply-To: <20191227094606.143637-6-jian.hu@amlogic.com>

 Hi Jian,

my comments and questions below
please keep in mind that I don't have access to the A1 datasheets, so
I may ask stupid questions :)

On Fri, Dec 27, 2019 at 10:47 AM Jian Hu <jian.hu@amlogic.com> wrote:
[...]
> +/* PLLs clock in gates, its parent is xtal */
yes. doesn't the code below describe exactly this (what is so special
about it that we need an extra comment)?

[...]
> +static const struct clk_parent_data sys_clk_parents[] = {
> +       { .fw_name = "xtal" },
> +       { .fw_name = "fclk_div2"},
> +       { .fw_name = "fclk_div3"},
> +       { .fw_name = "fclk_div5"},
the last three values are missing a space before "}"

[...]
> +       .hw.init = &(struct clk_init_data){
> +               .name = "sys_clk",
> +               .ops = &clk_regmap_mux_ro_ops,
> +               .parent_hws = (const struct clk_hw *[]) {
> +                       &a1_sys_a.hw, &a1_sys_b.hw,
> +               },
> +               .num_parents = 2,
> +               /*
> +                * This clock is used by APB bus which setted in Romcode
like in the PLL clkc patch:
- setted -> "is set"
- Romcode == boot ROM ?

[...]
> +static struct clk_regmap a1_rtc_32k_sel = {
> +       .data = &(struct clk_regmap_mux_data) {
> +               .offset = RTC_CTRL,
> +               .mask = 0x3,
> +               .shift = 0,
> +               .flags = CLK_MUX_ROUND_CLOSEST,
CLK_MUX_ROUND_CLOSEST means the common clock framework will also
accept rates greater than 32kHz.
is that fine for this case?

[...]
> +/*
> + * the second parent is sys_pll_div16, it will complete in the CPU clock,
I was confused by this but I assume you mean the parent with index 2?

> + * the forth parent is the clock measurement source, it relies on
> + * the clock measurement register configuration.
...and parent with index 4 here

[...]
> +static struct clk_regmap a1_pwm_a = {
> +       .data = &(struct clk_regmap_gate_data){
> +               .offset = PWM_CLK_AB_CTRL,
> +               .bit_idx = 8,
> +       },
> +       .hw.init = &(struct clk_init_data) {
> +               .name = "pwm_a",
> +               .ops = &clk_regmap_gate_ops,
> +               .parent_hws = (const struct clk_hw *[]) {
> +                       &a1_pwm_a_div.hw
> +               },
> +               .num_parents = 1,
> +               /*
> +                * The CPU working voltage is controlled by pwm_a
> +                * in BL2 firmware. add the CLK_IS_CRITICAL flag
> +                * to avoid changing at runtime.
on G12A and G12B Linux has to manage the CPU voltage regulator
can you confirm that for the A1 SoC this is really done by BL2? (I'm
wondering since A1 is newer than G12)

> +/*
> + * spicc clk
> + *    div2   |\         |\       _____
> + *  ---------| |---DIV--| |     |     |    spicc out
> + *  ---------| |        | |-----|GATE |---------
> + *     ..... |/         | /     |_____|
> + *  --------------------|/
> + *                 24M
does that "div2" stand for fclk_div2?

[...]
> +static const struct meson_eeclkc_data a1_periphs_data = {
> +               .regmap_clks = a1_periphs_regmaps,
> +               .regmap_clk_num = ARRAY_SIZE(a1_periphs_regmaps),
> +               .hw_onecell_data = &a1_periphs_hw_onecell_data,
> +};
same comment as for the PLL clkc: please drop this and use the
variables directly inside _probe to get rid of the struct
meson_eeclkc_data (so I won't be confused about "EE clocks" on A1,
while according to your description there's no "EE" domain)


Martin

WARNING: multiple messages have this Message-ID
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
To: Jian Hu <jian.hu@amlogic.com>
Cc: Rob Herring <robh@kernel.org>,
	Victor Wan <victor.wan@amlogic.com>,
	Jianxin Pan <jianxin.pan@amlogic.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Kevin Hilman <khilman@baylibre.com>,
	Michael Turquette <mturquette@baylibre.com>,
	linux-kernel@vger.kernel.org,
	Qiufang Dai <qiufang.dai@amlogic.com>,
	Chandle Zou <chandle.zou@amlogic.com>,
	linux-amlogic@lists.infradead.org, linux-clk@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Jerome Brunet <jbrunet@baylibre.com>
Subject: Re: [PATCH v5 5/5] clk: meson: a1: add support for Amlogic A1 Peripheral clock driver
Date: Fri, 27 Dec 2019 18:22:52 +0100	[thread overview]
Message-ID: <CAFBinCB_0+k6rGxChpB77rPUrb-0mzxt_nQWXbiztCJnJq8XnQ@mail.gmail.com> (raw)
In-Reply-To: <20191227094606.143637-6-jian.hu@amlogic.com>

 Hi Jian,

my comments and questions below
please keep in mind that I don't have access to the A1 datasheets, so
I may ask stupid questions :)

On Fri, Dec 27, 2019 at 10:47 AM Jian Hu <jian.hu@amlogic.com> wrote:
[...]
> +/* PLLs clock in gates, its parent is xtal */
yes. doesn't the code below describe exactly this (what is so special
about it that we need an extra comment)?

[...]
> +static const struct clk_parent_data sys_clk_parents[] = {
> +       { .fw_name = "xtal" },
> +       { .fw_name = "fclk_div2"},
> +       { .fw_name = "fclk_div3"},
> +       { .fw_name = "fclk_div5"},
the last three values are missing a space before "}"

[...]
> +       .hw.init = &(struct clk_init_data){
> +               .name = "sys_clk",
> +               .ops = &clk_regmap_mux_ro_ops,
> +               .parent_hws = (const struct clk_hw *[]) {
> +                       &a1_sys_a.hw, &a1_sys_b.hw,
> +               },
> +               .num_parents = 2,
> +               /*
> +                * This clock is used by APB bus which setted in Romcode
like in the PLL clkc patch:
- setted -> "is set"
- Romcode == boot ROM ?

[...]
> +static struct clk_regmap a1_rtc_32k_sel = {
> +       .data = &(struct clk_regmap_mux_data) {
> +               .offset = RTC_CTRL,
> +               .mask = 0x3,
> +               .shift = 0,
> +               .flags = CLK_MUX_ROUND_CLOSEST,
CLK_MUX_ROUND_CLOSEST means the common clock framework will also
accept rates greater than 32kHz.
is that fine for this case?

[...]
> +/*
> + * the second parent is sys_pll_div16, it will complete in the CPU clock,
I was confused by this but I assume you mean the parent with index 2?

> + * the forth parent is the clock measurement source, it relies on
> + * the clock measurement register configuration.
...and parent with index 4 here

[...]
> +static struct clk_regmap a1_pwm_a = {
> +       .data = &(struct clk_regmap_gate_data){
> +               .offset = PWM_CLK_AB_CTRL,
> +               .bit_idx = 8,
> +       },
> +       .hw.init = &(struct clk_init_data) {
> +               .name = "pwm_a",
> +               .ops = &clk_regmap_gate_ops,
> +               .parent_hws = (const struct clk_hw *[]) {
> +                       &a1_pwm_a_div.hw
> +               },
> +               .num_parents = 1,
> +               /*
> +                * The CPU working voltage is controlled by pwm_a
> +                * in BL2 firmware. add the CLK_IS_CRITICAL flag
> +                * to avoid changing at runtime.
on G12A and G12B Linux has to manage the CPU voltage regulator
can you confirm that for the A1 SoC this is really done by BL2? (I'm
wondering since A1 is newer than G12)

> +/*
> + * spicc clk
> + *    div2   |\         |\       _____
> + *  ---------| |---DIV--| |     |     |    spicc out
> + *  ---------| |        | |-----|GATE |---------
> + *     ..... |/         | /     |_____|
> + *  --------------------|/
> + *                 24M
does that "div2" stand for fclk_div2?

[...]
> +static const struct meson_eeclkc_data a1_periphs_data = {
> +               .regmap_clks = a1_periphs_regmaps,
> +               .regmap_clk_num = ARRAY_SIZE(a1_periphs_regmaps),
> +               .hw_onecell_data = &a1_periphs_hw_onecell_data,
> +};
same comment as for the PLL clkc: please drop this and use the
variables directly inside _probe to get rid of the struct
meson_eeclkc_data (so I won't be confused about "EE clocks" on A1,
while according to your description there's no "EE" domain)


Martin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
To: Jian Hu <jian.hu@amlogic.com>
Cc: Rob Herring <robh@kernel.org>,
	Victor Wan <victor.wan@amlogic.com>,
	Jianxin Pan <jianxin.pan@amlogic.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Kevin Hilman <khilman@baylibre.com>,
	Michael Turquette <mturquette@baylibre.com>,
	linux-kernel@vger.kernel.org,
	Qiufang Dai <qiufang.dai@amlogic.com>,
	Chandle Zou <chandle.zou@amlogic.com>,
	linux-amlogic@lists.infradead.org, linux-clk@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Jerome Brunet <jbrunet@baylibre.com>
Subject: Re: [PATCH v5 5/5] clk: meson: a1: add support for Amlogic A1 Peripheral clock driver
Date: Fri, 27 Dec 2019 18:22:52 +0100	[thread overview]
Message-ID: <CAFBinCB_0+k6rGxChpB77rPUrb-0mzxt_nQWXbiztCJnJq8XnQ@mail.gmail.com> (raw)
In-Reply-To: <20191227094606.143637-6-jian.hu@amlogic.com>

 Hi Jian,

my comments and questions below
please keep in mind that I don't have access to the A1 datasheets, so
I may ask stupid questions :)

On Fri, Dec 27, 2019 at 10:47 AM Jian Hu <jian.hu@amlogic.com> wrote:
[...]
> +/* PLLs clock in gates, its parent is xtal */
yes. doesn't the code below describe exactly this (what is so special
about it that we need an extra comment)?

[...]
> +static const struct clk_parent_data sys_clk_parents[] = {
> +       { .fw_name = "xtal" },
> +       { .fw_name = "fclk_div2"},
> +       { .fw_name = "fclk_div3"},
> +       { .fw_name = "fclk_div5"},
the last three values are missing a space before "}"

[...]
> +       .hw.init = &(struct clk_init_data){
> +               .name = "sys_clk",
> +               .ops = &clk_regmap_mux_ro_ops,
> +               .parent_hws = (const struct clk_hw *[]) {
> +                       &a1_sys_a.hw, &a1_sys_b.hw,
> +               },
> +               .num_parents = 2,
> +               /*
> +                * This clock is used by APB bus which setted in Romcode
like in the PLL clkc patch:
- setted -> "is set"
- Romcode == boot ROM ?

[...]
> +static struct clk_regmap a1_rtc_32k_sel = {
> +       .data = &(struct clk_regmap_mux_data) {
> +               .offset = RTC_CTRL,
> +               .mask = 0x3,
> +               .shift = 0,
> +               .flags = CLK_MUX_ROUND_CLOSEST,
CLK_MUX_ROUND_CLOSEST means the common clock framework will also
accept rates greater than 32kHz.
is that fine for this case?

[...]
> +/*
> + * the second parent is sys_pll_div16, it will complete in the CPU clock,
I was confused by this but I assume you mean the parent with index 2?

> + * the forth parent is the clock measurement source, it relies on
> + * the clock measurement register configuration.
...and parent with index 4 here

[...]
> +static struct clk_regmap a1_pwm_a = {
> +       .data = &(struct clk_regmap_gate_data){
> +               .offset = PWM_CLK_AB_CTRL,
> +               .bit_idx = 8,
> +       },
> +       .hw.init = &(struct clk_init_data) {
> +               .name = "pwm_a",
> +               .ops = &clk_regmap_gate_ops,
> +               .parent_hws = (const struct clk_hw *[]) {
> +                       &a1_pwm_a_div.hw
> +               },
> +               .num_parents = 1,
> +               /*
> +                * The CPU working voltage is controlled by pwm_a
> +                * in BL2 firmware. add the CLK_IS_CRITICAL flag
> +                * to avoid changing at runtime.
on G12A and G12B Linux has to manage the CPU voltage regulator
can you confirm that for the A1 SoC this is really done by BL2? (I'm
wondering since A1 is newer than G12)

> +/*
> + * spicc clk
> + *    div2   |\         |\       _____
> + *  ---------| |---DIV--| |     |     |    spicc out
> + *  ---------| |        | |-----|GATE |---------
> + *     ..... |/         | /     |_____|
> + *  --------------------|/
> + *                 24M
does that "div2" stand for fclk_div2?

[...]
> +static const struct meson_eeclkc_data a1_periphs_data = {
> +               .regmap_clks = a1_periphs_regmaps,
> +               .regmap_clk_num = ARRAY_SIZE(a1_periphs_regmaps),
> +               .hw_onecell_data = &a1_periphs_hw_onecell_data,
> +};
same comment as for the PLL clkc: please drop this and use the
variables directly inside _probe to get rid of the struct
meson_eeclkc_data (so I won't be confused about "EE clocks" on A1,
while according to your description there's no "EE" domain)


Martin

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

  reply	other threads:[~2019-12-27 17:23 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-27  9:46 [PATCH v5 0/5] add Amlogic A1 clock controller driver Jian Hu
2019-12-27  9:46 ` Jian Hu
2019-12-27  9:46 ` Jian Hu
2019-12-27  9:46 ` [PATCH v5 1/5] dt-bindings: clock: meson: add A1 PLL clock controller bindings Jian Hu
2019-12-27  9:46   ` Jian Hu
2019-12-27  9:46   ` Jian Hu
2020-01-03 22:29   ` Rob Herring
2020-01-03 22:29     ` Rob Herring
2020-01-03 22:29     ` Rob Herring
2020-01-10 15:36   ` Jerome Brunet
2020-01-10 15:36     ` Jerome Brunet
2020-01-10 15:36     ` Jerome Brunet
2020-01-16  5:50     ` Jian Hu
2020-01-16  5:50       ` Jian Hu
2020-01-16  5:50       ` Jian Hu
2019-12-27  9:46 ` [PATCH v5 2/5] clk: meson: add support for A1 PLL clock ops Jian Hu
2019-12-27  9:46   ` Jian Hu
2019-12-27  9:46   ` Jian Hu
2019-12-27 16:53   ` Martin Blumenstingl
2019-12-27 16:53     ` Martin Blumenstingl
2019-12-27 16:53     ` Martin Blumenstingl
2020-01-09  6:55     ` Jian Hu
2020-01-09  6:55       ` Jian Hu
2020-01-09  6:55       ` Jian Hu
2019-12-27  9:46 ` [PATCH v5 3/5] clk: meson: a1: add support for Amlogic A1 PLL clock driver Jian Hu
2019-12-27  9:46   ` Jian Hu
2019-12-27  9:46   ` Jian Hu
2019-12-27 17:04   ` Martin Blumenstingl
2019-12-27 17:04     ` Martin Blumenstingl
2019-12-27 17:04     ` Martin Blumenstingl
2020-01-09  7:55     ` Jian Hu
2020-01-09  7:55       ` Jian Hu
2020-01-09  7:55       ` Jian Hu
2019-12-27  9:46 ` [PATCH v5 4/5] dt-bindings: clock: meson: add A1 peripheral clock controller bindings Jian Hu
2019-12-27  9:46   ` Jian Hu
2019-12-27  9:46   ` Jian Hu
2020-01-10 15:38   ` Jerome Brunet
2020-01-10 15:38     ` Jerome Brunet
2020-01-10 15:38     ` Jerome Brunet
2020-01-16  6:19     ` Jian Hu
2020-01-16  6:19       ` Jian Hu
2020-01-16  6:19       ` Jian Hu
2019-12-27  9:46 ` [PATCH v5 5/5] clk: meson: a1: add support for Amlogic A1 Peripheral clock driver Jian Hu
2019-12-27  9:46   ` Jian Hu
2019-12-27  9:46   ` Jian Hu
2019-12-27 17:22   ` Martin Blumenstingl [this message]
2019-12-27 17:22     ` Martin Blumenstingl
2019-12-27 17:22     ` Martin Blumenstingl
2020-01-09 11:11     ` Jian Hu
2020-01-09 11:11       ` Jian Hu
2020-01-09 11:11       ` Jian Hu

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=CAFBinCB_0+k6rGxChpB77rPUrb-0mzxt_nQWXbiztCJnJq8XnQ@mail.gmail.com \
    --to=martin.blumenstingl@googlemail.com \
    --cc=chandle.zou@amlogic.com \
    --cc=jbrunet@baylibre.com \
    --cc=jian.hu@amlogic.com \
    --cc=jianxin.pan@amlogic.com \
    --cc=khilman@baylibre.com \
    --cc=linux-amlogic@lists.infradead.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mturquette@baylibre.com \
    --cc=narmstrong@baylibre.com \
    --cc=qiufang.dai@amlogic.com \
    --cc=robh@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=victor.wan@amlogic.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.