From: Tomer Maimon <tmaimon77@gmail.com>
To: Stephen Boyd <sboyd@kernel.org>
Cc: benjaminfair@google.com, avifishman70@gmail.com,
venture@google.com, mturquette@baylibre.com,
linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org,
tali.perry1@gmail.com, joel@jms.id.au, openbmc@lists.ozlabs.org
Subject: Re: [PATCH v20] clk: npcm8xx: add clock controller
Date: Wed, 29 Nov 2023 11:47:12 +0200 [thread overview]
Message-ID: <CAP6Zq1ie_meX9Kuz3C8KBkYWxjLBDtimk3PS9=zYOhrGxFikBg@mail.gmail.com> (raw)
In-Reply-To: <7d529b2b9a16f238f533f1c03b4261b2.sboyd@kernel.org>
Hi Stephen,
Thanks for your comments and sorry for the late reply.
On Sat, 7 Oct 2023 at 02:50, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Tomer Maimon (2023-09-28 15:40:51)
> > diff --git a/drivers/clk/clk-npcm8xx.c b/drivers/clk/clk-npcm8xx.c
> > new file mode 100644
> > index 000000000000..e575a8676ca3
> > --- /dev/null
> > +++ b/drivers/clk/clk-npcm8xx.c
> > @@ -0,0 +1,547 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> [...]
> > +
> > +/* configurable dividers: */
> > +static const struct npcm8xx_clk_div_data npcm8xx_divs[] = {
> > + { NPCM8XX_CLKDIV1, 28, 3, NPCM8XX_CLK_S_ADC,
> > + { .name = NPCM8XX_CLK_S_PRE_ADC, .index = -1 },
> > + CLK_DIVIDER_READ_ONLY | CLK_DIVIDER_POWER_OF_TWO, 0, NPCM8XX_CLK_ADC },
>
> Please format this some other way. I assume one line means one clk, but
> here it is actually three lines. Perhaps something like this?
Ready in V21
>
> > + {
> > + NPCM8XX_CLKDIV1, 28, 3, NPCM8XX_CLK_S_ADC,
> > + { .name = NPCM8XX_CLK_S_PRE_ADC, .index = -1 },
> > + CLK_DIVIDER_READ_ONLY | CLK_DIVIDER_POWER_OF_TWO, 0, NPCM8XX_CLK_ADC
> > + },
>
> Please stop using the .name member of struct clk_parent_data. That
> member is only there to support drivers that are migrating from a
> binding that didn't specify the parents of clks that are outside of the
> clk controller with the clocks property in their DT node. I see that the
> dts exists upstream, but luckily we don't have a driver merged, so we're
> free to change the binding to specify clks external to the node. The
> .fw_name member will match a 'clock-names' element for the registering
> driver's node. The .index member will match the index in the 'clocks'
> property. Neither of those properties exist in the nuvoton,npcm845-clk
> DT binding, so neither of those members shall be present. This means
> that either the binding needs to be updated, or the clk_parent_data
> structure should be replaced with clk_hw pointers to describe parents.
> I'm going to guess that there aren't any external clk parents, so to
> keep things simple this driver should change to use direct clk_hw
> pointers to describe topology.
Ready in V21
>
> > + { NPCM8XX_CLKDIV1, 26, 2, NPCM8XX_CLK_S_AHB, { .hw = &hw_pre_clk },
> > + CLK_DIVIDER_READ_ONLY, CLK_IS_CRITICAL, NPCM8XX_CLK_AHB },
> > + { NPCM8XX_CLKDIV1, 21, 5, NPCM8XX_CLK_S_PRE_ADC,
> > + { .hw = &npcm8xx_muxes[6].hw }, CLK_DIVIDER_READ_ONLY, 0, -1 },
> > + { NPCM8XX_CLKDIV1, 16, 5, NPCM8XX_CLK_S_UART,
> > + { .hw = &npcm8xx_muxes[3].hw }, 0, 0, NPCM8XX_CLK_UART },
> > + { NPCM8XX_CLKDIV1, 11, 5, NPCM8XX_CLK_S_MMC,
> > + { .hw = &npcm8xx_muxes[2].hw }, CLK_DIVIDER_READ_ONLY, 0,
> > + NPCM8XX_CLK_MMC },
> > + { NPCM8XX_CLKDIV1, 6, 5, NPCM8XX_CLK_S_SPI3,
> > + { .fw_name = NPCM8XX_CLK_S_AHB, .name = NPCM8XX_CLK_S_AHB }, 0, 0,
> > + NPCM8XX_CLK_SPI3 },
> > + { NPCM8XX_CLKDIV1, 2, 4, NPCM8XX_CLK_S_PCI,
> > + { .hw = &npcm8xx_muxes[7].hw }, CLK_DIVIDER_READ_ONLY, 0,
> > + NPCM8XX_CLK_PCI },
>
> BTW, I looked at the dts file upstream (nuvoton-common-npcm8xx.dtsi).
> The reset and clock controller start at the same address, which can only
> mean that they're actually the same device. The two nodes should be
unfortunately, It is two services (reset and clock) that are handled
in the same memory space.
> combined into one node and one driver should match that compatible so
> that one IO mapping is made for the entire clock and reset contoller
> register space. If you want, that driver can make two auxiliary device
> drivers for the reset and clk parts of the io space and then those two
> drivers can reside in drivers/reset and drivers/clk. I don't know where
> the driver goes that matches the compatible node though, probably in
> drivers/soc. This allows us to properly model the logical components
> that make up the device in hardware (clks and resets) while also
> allowing any device specific things for that entire register space to
> live in the soc driver. For example, if some power domain needs to be
> enabled to access that register space it would be attached to the soc
> driver.
Sorry I didn't understand, do you mean to have one driver that handles
the clock and the reset modules and will sis under driver/soc
or one driver that handles the reset and clock IO space?
What about using regmap to handle the clock and the reset? for this,
the NPCM clock driver will use a unique clock setting like it is done
in Tegra clk.
https://elixir.bootlin.com/linux/v6.7-rc2/source/drivers/clk/tegra/clk-divider.c
instead of using clk_divide and clk_mux default services.
Thanks,
Tomer
prev parent reply other threads:[~2023-11-29 9:48 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-28 22:40 [PATCH v20] clk: npcm8xx: add clock controller Tomer Maimon
2023-10-06 23:50 ` Stephen Boyd
2023-11-29 9:47 ` Tomer Maimon [this message]
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='CAP6Zq1ie_meX9Kuz3C8KBkYWxjLBDtimk3PS9=zYOhrGxFikBg@mail.gmail.com' \
--to=tmaimon77@gmail.com \
--cc=avifishman70@gmail.com \
--cc=benjaminfair@google.com \
--cc=joel@jms.id.au \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mturquette@baylibre.com \
--cc=openbmc@lists.ozlabs.org \
--cc=sboyd@kernel.org \
--cc=tali.perry1@gmail.com \
--cc=venture@google.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 read-only IMAP folder(s) and NNTP newsgroup(s).