* Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
From: Andy Shevchenko @ 2024-04-02 16:39 UTC (permalink / raw)
To: Cristian Marussi
Cc: Peng Fan, Peng Fan (OSS), Sudeep Holla, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Linus Walleij, Dan Carpenter,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, Oleksii Moisieiev
In-Reply-To: <ZgwrKnx3hb59OG77@pluto>
On Tue, Apr 2, 2024 at 6:58 PM Cristian Marussi
<cristian.marussi@arm.com> wrote:
> On Tue, Apr 02, 2024 at 04:06:06PM +0300, Andy Shevchenko wrote:
> > On Tue, Apr 2, 2024 at 10:48 AM Cristian Marussi
> > <cristian.marussi@arm.com> wrote:
> > > On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote:
> > > > > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti:
...
> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/scmi_protocol.h>
> > > > > > +#include <linux/slab.h>
> > > > >
> > > > > This is semi-random list of headers. Please, follow IWYU principle (include
> > > > > what you use). There are a lot of inclusions I see missing (just in the context of
> > > > > this page I see bits.h, types.h, and asm/byteorder.h).
> > > >
> > > > Is there any documentation about this requirement?
> > > > Some headers are already included by others.
> >
> > The documentation here is called "a common sense".
> > The C language is built like this and we expect that nobody will
> > invest into the dependency hell that we have already, that's why IWYU
> > principle, please follow it.
>
> Yes, but given that we have a growing number of SCMI protocols there is a
> common local protocols.h header to group all includes needed by any
> protocols: the idea behind this (and the devm_ saga down below) was to ease
> development of protocols, since there are lots of them and growing, given
> the SCMI spec is extensible.
Yes, and what you are effectively suggesting is: "Technical debt? Oh,
fine, we do not care!" This is not good. I'm in a long term of
cleaning up the dependency hell in the kernel (my main focus is
kernel.h for now) and I am talking from my experience. I do not like
what people are doing in 95% of the code, that's why I really want to
stop the bad practices as soon as possible.
Last to add, but not least is that your code may be used as an example
for others, hence we really have to do our best in order to avoid bad
design, practices, and cargo cults. If this requires more refactoring
of the existing code, then do it sooner than later.
...
> > > Andy made (mostly) the same remarks on this same patch ~1-year ago on
> > > this same patch while it was posted by Oleksii.
> > >
> > > And I told that time that most of the remarks around devm_ usage were
> > > wrong due to how the SCMI core handles protocol initialization (using a
> > > devres group transparently).
> > >
> > > This is what I answered that time.
> > >
> > > https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t
> > >
> > > I wont repeat myself, but, in a nutshell the memory allocation like it
> > > is now is fine: a bit happens via devm_ at protocol initialization, the
> > > other is doe via explicit kmalloc at runtime and freed via kfree at
> > > remove time (if needed...i.e. checking the present flag of some structs)
> >
> > This sounds like a mess. devm_ is expected to be used only for the
> > ->probe() stage, otherwise you may consider cleanup.h (__free() macro)
> > to have automatic free at the paths where memory is not needed.
>
> Indeed, this protocol_init code is called by the SCMI core once for all when
> an SCMI driver tries at first to use this specific protocol by 'getting' its
> protocol_ops, so it is indeed called inside the probe chain of the driver:
> at this point you *can* decide to use devres to allocate memory and be assured
> that if the init fails, or when the driver cease to use this protocol (calling
> its remove()) and no other driver is using it, all the stuff that have been
> allocated related to this protocol will be released by the core for you.
> (using an internal devres group)
>
> Without this you should handle manually all the deallocation manually on
> the init error-paths AND also provide all the cleanup explicitly when
> the protocol is no more used by any driver (multiple users of the same
> protocol instance are possible)...for all protocols.
Yes. Is it a problem?
> This is/was handy since, till now, all the SCMI querying and resources
> allocation happened anyway all at once at init time...
>
> ...the mess, as you kindly called it, derives from the fact that this specific
> protocol is the first and only one that does NOT allocate all that it needs
> during the initialization (to minimize needless allocs for a lot of possibly
> unused resources) and this lazy-initialization phase, done after init at runtime,
> must be handled manually since it cannot be managed by the devres group that is
> open/clsoed around init by the SCMI core.
>
> I dont like particularly this split allocation but it has a reason and any
> other solution seems more messy to me at the moment.
>
> And I dont feel like changing all the SCMI protocol initialziation core code
> (that address a lot more under the hood) is a desirable solution to address a
> non-existent problem really.
>
> > And the function naming doesn't suggest that you have a probe-remove
> > pair. Moreover, if the init-deinit part is called in the probe-remove,
> > the devm_ must not be mixed with non-devm ones, as it breaks the order
> > and leads to subtle mistakes.
>
> Initialization order is enforced by SCMI core like this:
>
> @driver_probe->get_protocol_ops()
> @core/get_protocol_ops
> -> devres_group_open()
> -> protocol_init->devm_*()
> -> devres_group_close()
> -> driver_probing
>
> @runtime optional explicit_lazy_kmallocs inside the protocol
>
> @driver_remove->put_protocol_ops()
> @core/put_protocol_ops()
> -> protocol_denit->optional_explicit_kfree_of_the_above
> -> devres_group_release()
> -> driver_removing
>
> ... dont think there's an ordering problem.
The mess with devm_ vs. non-devm is quite easy to achieve. You are
probably out of the control of what the protocol driver wants to do in
the init. Is the usage of devm (which APIs and in which order can be
used) WRT SCMI documented somewhere?
Misuse of devm is a common issue, I'm not surprised it will hit your
subsystem one day with such an approach.
> ...note that the ph->dev provided in the protocol_init and used by devm_
> is NOT the dev of the SCMI driver probe/remove that uses the get_protocol_ops,
> it is an internal SCMI device associated with the core SCMI stack probing and
> allocations, within which a devres group for the specific protocol is created
> when that specific protocol is initialized...protocols are not fully
> fledged drivers are just bits of the SCMI stack that are initialized when needed
> (and possibly also loaded when needed for vendor protocols) and
> de-initialzed when no more SCMI driver users exist for that protocol.
P.S. I guess from now on it's your call, but this code and in case
other drivers use similar, is badly written. I hope some documentation
exists to at least justify all this mess and explaining why there no
and (what's really important) will never be a problem.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v7 4/4] pinctrl: Implementation of the generic scmi-pinctrl driver
From: Cristian Marussi @ 2024-04-02 16:40 UTC (permalink / raw)
To: Dan Carpenter
Cc: Andy Shevchenko, Peng Fan (OSS), Sudeep Holla, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Linus Walleij,
linux-arm-kernel, linux-kernel, devicetree, linux-gpio, Peng Fan,
Oleksii Moisieiev
In-Reply-To: <c5bdf039-c43b-4611-9f0b-81585e296206@moroto.mountain>
On Tue, Apr 02, 2024 at 05:09:34PM +0300, Dan Carpenter wrote:
> On Tue, Apr 02, 2024 at 04:22:45PM +0300, Andy Shevchenko wrote:
> > On Tue, Apr 02, 2024 at 10:22:24AM +0800, Peng Fan (OSS) wrote:
> > > +static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
> > > + struct pinctrl_desc *desc)
> > > +{
> > > + struct pinctrl_pin_desc *pins;
> > > + unsigned int npins;
> > > + int ret, i;
> > > +
> > > + npins = pinctrl_ops->count_get(pmx->ph, PIN_TYPE);
> > > + /*
> > > + * npins will never be zero, the scmi pinctrl driver has bailed out
> > > + * if npins is zero.
> > > + */
> >
> > This is fragile, but at least it is documented.
> >
>
> It was never clear to me where the crash would happen if npins was zero.
> Does some part of pinctrl internals assume we have at least one pin?
Dont think there were any possible crashes since at the protoocl layer
(not here) kcalloc returns ZERO_SIZE_PTR into pinfo->pins for a zero-bytes
allocation BUT it is indeed never accessed since any attempt to access a
pin will be considerd invalid (any u32 index >= (nr_pins=0))...
...but what is the point of loading protocol and drivers with zero pins ?
You can have zero grouos and zero functions, but zero pins ?
Thanks,
Cristian
^ permalink raw reply
* Re: [PATCH v2 12/15] dt-bindings: thermal: mediatek: Add LVTS thermal controller definition for MT8188
From: Rob Herring @ 2024-04-02 16:23 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Nicolas Pitre, Daniel Lezcano, AngeloGioacchino Del Regno,
devicetree, linux-pm, linux-mediatek
In-Reply-To: <20240402032729.2736685-13-nico@fluxnic.net>
On Mon, 01 Apr 2024 23:25:46 -0400, Nicolas Pitre wrote:
> From: Nicolas Pitre <npitre@baylibre.com>
>
> Add LVTS thermal controller definition for MT8188.
>
> Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
> ---
> .../bindings/thermal/mediatek,lvts-thermal.yaml | 4 ++++
> .../dt-bindings/thermal/mediatek,lvts-thermal.h | 16 ++++++++++++++++
> 2 files changed, 20 insertions(+)
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v2 1/3] dt-bindings: leds: Add Silergy SY7802 flash LED
From: Rob Herring @ 2024-04-02 16:22 UTC (permalink / raw)
To: André Apitzsch
Cc: phone-devel, devicetree, linux-hardening, linux-leds,
~postmarketos/upstreaming, Gustavo A. R. Silva, Konrad Dybcio,
Lee Jones, Conor Dooley, Krzysztof Kozlowski, Bjorn Andersson,
linux-kernel, linux-arm-msm, Kees Cook, Pavel Machek
In-Reply-To: <20240401-sy7802-v2-1-1138190a7448@apitzsch.eu>
On Mon, 01 Apr 2024 23:23:55 +0200, André Apitzsch wrote:
> Document Silergy SY7802 flash LED driver devicetree bindings.
>
> Signed-off-by: André Apitzsch <git@apitzsch.eu>
> ---
> .../devicetree/bindings/leds/silergy,sy7802.yaml | 100 +++++++++++++++++++++
> 1 file changed, 100 insertions(+)
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH] arm64: dts: ti: k3-j722s: Disable ethernet ports by default
From: Nishanth Menon @ 2024-04-02 16:19 UTC (permalink / raw)
To: Michael Walle
Cc: Vignesh Raghavendra, Tero Kristo, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-arm-kernel, devicetree,
linux-kernel
In-Reply-To: <D09RSRPKV9L9.2TS01DA84TEKK@kernel.org>
On 18:16-20240402, Michael Walle wrote:
> Hi,
>
> > This is better at arch/arm64/boot/dts/ti/k3-am62p-main.dtsi.
>
> I'm fine with that, but be aware that we'll also change the am62p
> SoC dtsi in that case.
This change is valid to me (sigh.. it's been a whack-a-mole as you can expect).
62p and j722s/am67 are too closely related anyways.. but, this will need
to percolate consistently to all k3 SoCs.. (different problem).
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
^ permalink raw reply
* Re: [PATCH v1 6/6] clk: meson: a1: add Amlogic A1 CPU clock controller driver
From: Jerome Brunet @ 2024-04-02 16:17 UTC (permalink / raw)
To: Dmitry Rokosov
Cc: Jerome Brunet, neil.armstrong, mturquette, sboyd, robh+dt,
krzysztof.kozlowski+dt, khilman, martin.blumenstingl, kernel,
rockosov, linux-amlogic, linux-clk, devicetree, linux-kernel,
linux-arm-kernel
In-Reply-To: <20240402161110.v7t72s6t7m3bzifi@CAB-WSD-L081021>
On Tue 02 Apr 2024 at 19:11, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
> On Tue, Apr 02, 2024 at 04:11:12PM +0200, Jerome Brunet wrote:
>>
>> On Tue 02 Apr 2024 at 14:05, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
>>
>> > Hello Jerome,
>> >
>> > On Tue, Apr 02, 2024 at 11:35:49AM +0200, Jerome Brunet wrote:
>> >>
>> >> On Fri 29 Mar 2024 at 23:58, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
>> >>
>> >> > The CPU clock controller plays a general role in the Amlogic A1 SoC
>> >> > family by generating CPU clocks. As an APB slave module, it offers the
>> >> > capability to inherit the CPU clock from two sources: the internal fixed
>> >> > clock known as 'cpu fixed clock' and the external input provided by the
>> >> > A1 PLL clock controller, referred to as 'syspll'.
>> >> >
>> >> > It is important for the driver to handle cpu_clk rate switching
>> >> > effectively by transitioning to the CPU fixed clock to avoid any
>> >> > potential execution freezes.
>> >> >
>> >> > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
>> >> > ---
>> >> > drivers/clk/meson/Kconfig | 10 ++
>> >> > drivers/clk/meson/Makefile | 1 +
>> >> > drivers/clk/meson/a1-cpu.c | 324 +++++++++++++++++++++++++++++++++++++
>> >> > drivers/clk/meson/a1-cpu.h | 16 ++
>> >> > 4 files changed, 351 insertions(+)
>> >> > create mode 100644 drivers/clk/meson/a1-cpu.c
>> >> > create mode 100644 drivers/clk/meson/a1-cpu.h
>> >> >
>> >> > diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>> >> > index 80c4a18c83d2..148d4495eee3 100644
>> >> > --- a/drivers/clk/meson/Kconfig
>> >> > +++ b/drivers/clk/meson/Kconfig
>> >> > @@ -111,6 +111,16 @@ config COMMON_CLK_AXG_AUDIO
>> >> > Support for the audio clock controller on AmLogic A113D devices,
>> >> > aka axg, Say Y if you want audio subsystem to work.
>> >> >
>> >> > +config COMMON_CLK_A1_CPU
>> >> > + tristate "Amlogic A1 SoC CPU controller support"
>> >> > + depends on ARM64
>> >> > + select COMMON_CLK_MESON_REGMAP
>> >> > + select COMMON_CLK_MESON_CLKC_UTILS
>> >> > + help
>> >> > + Support for the CPU clock controller on Amlogic A113L based
>> >> > + device, A1 SoC Family. Say Y if you want A1 CPU clock controller
>> >> > + to work.
>> >> > +
>> >> > config COMMON_CLK_A1_PLL
>> >> > tristate "Amlogic A1 SoC PLL controller support"
>> >> > depends on ARM64
>> >> > diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>> >> > index 4968fc7ad555..2a06eb0303d6 100644
>> >> > --- a/drivers/clk/meson/Makefile
>> >> > +++ b/drivers/clk/meson/Makefile
>> >> > @@ -18,6 +18,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_AUDIO_RSTC) += meson-audio-rstc.o
>> >> >
>> >> > obj-$(CONFIG_COMMON_CLK_AXG) += axg.o axg-aoclk.o
>> >> > obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o
>> >> > +obj-$(CONFIG_COMMON_CLK_A1_CPU) += a1-cpu.o
>> >> > obj-$(CONFIG_COMMON_CLK_A1_PLL) += a1-pll.o
>> >> > obj-$(CONFIG_COMMON_CLK_A1_PERIPHERALS) += a1-peripherals.o
>> >> > obj-$(CONFIG_COMMON_CLK_A1_AUDIO) += a1-audio.o
>> >> > diff --git a/drivers/clk/meson/a1-cpu.c b/drivers/clk/meson/a1-cpu.c
>> >> > new file mode 100644
>> >> > index 000000000000..5f5d8ae112e5
>> >> > --- /dev/null
>> >> > +++ b/drivers/clk/meson/a1-cpu.c
>> >> > @@ -0,0 +1,324 @@
>> >> > +// SPDX-License-Identifier: GPL-2.0+
>> >> > +/*
>> >> > + * Amlogic A1 SoC family CPU Clock Controller driver.
>> >> > + *
>> >> > + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
>> >> > + * Author: Dmitry Rokosov <ddrokosov@salutedevices.com>
>> >> > + */
>> >> > +
>> >> > +#include <linux/clk.h>
>> >> > +#include <linux/clk-provider.h>
>> >> > +#include <linux/mod_devicetable.h>
>> >> > +#include <linux/platform_device.h>
>> >> > +#include "a1-cpu.h"
>> >> > +#include "clk-regmap.h"
>> >> > +#include "meson-clkc-utils.h"
>> >> > +
>> >> > +#include <dt-bindings/clock/amlogic,a1-cpu-clkc.h>
>> >> > +
>> >> > +static u32 cpu_fsource_sel_table[] = { 0, 1, 2 };
>> >> > +static const struct clk_parent_data cpu_fsource_sel_parents[] = {
>> >> > + { .fw_name = "xtal" },
>> >> > + { .fw_name = "fclk_div2" },
>> >> > + { .fw_name = "fclk_div3" },
>> >> > +};
>> >> > +
>> >> > +static struct clk_regmap cpu_fsource_sel0 = {
>> >> > + .data = &(struct clk_regmap_mux_data) {
>> >> > + .offset = CPUCTRL_CLK_CTRL0,
>> >> > + .mask = 0x3,
>> >> > + .shift = 0,
>> >> > + .table = cpu_fsource_sel_table,
>> >> > + },
>> >> > + .hw.init = &(struct clk_init_data) {
>> >> > + .name = "cpu_fsource_sel0",
>> >> > + .ops = &clk_regmap_mux_ops,
>> >> > + .parent_data = cpu_fsource_sel_parents,
>> >> > + .num_parents = ARRAY_SIZE(cpu_fsource_sel_parents),
>> >> > + .flags = CLK_SET_RATE_PARENT,
>> >> > + },
>> >> > +};
>> >> > +
>> >> > +static struct clk_regmap cpu_fsource_div0 = {
>> >> > + .data = &(struct clk_regmap_div_data) {
>> >> > + .offset = CPUCTRL_CLK_CTRL0,
>> >> > + .shift = 4,
>> >> > + .width = 6,
>> >> > + },
>> >> > + .hw.init = &(struct clk_init_data) {
>> >> > + .name = "cpu_fsource_div0",
>> >> > + .ops = &clk_regmap_divider_ops,
>> >> > + .parent_hws = (const struct clk_hw *[]) {
>> >> > + &cpu_fsource_sel0.hw
>> >> > + },
>> >> > + .num_parents = 1,
>> >> > + .flags = CLK_SET_RATE_PARENT,
>> >> > + },
>> >> > +};
>> >> > +
>> >> > +static struct clk_regmap cpu_fsel0 = {
>> >> > + .data = &(struct clk_regmap_mux_data) {
>> >> > + .offset = CPUCTRL_CLK_CTRL0,
>> >> > + .mask = 0x1,
>> >> > + .shift = 2,
>> >> > + },
>> >> > + .hw.init = &(struct clk_init_data) {
>> >> > + .name = "cpu_fsel0",
>> >> > + .ops = &clk_regmap_mux_ops,
>> >> > + .parent_hws = (const struct clk_hw *[]) {
>> >> > + &cpu_fsource_sel0.hw,
>> >> > + &cpu_fsource_div0.hw,
>> >> > + },
>> >> > + .num_parents = 2,
>> >> > + .flags = CLK_SET_RATE_PARENT,
>> >> > + },
>> >> > +};
>> >> > +
>> >> > +static struct clk_regmap cpu_fsource_sel1 = {
>> >> > + .data = &(struct clk_regmap_mux_data) {
>> >> > + .offset = CPUCTRL_CLK_CTRL0,
>> >> > + .mask = 0x3,
>> >> > + .shift = 16,
>> >> > + .table = cpu_fsource_sel_table,
>> >> > + },
>> >> > + .hw.init = &(struct clk_init_data) {
>> >> > + .name = "cpu_fsource_sel1",
>> >> > + .ops = &clk_regmap_mux_ops,
>> >> > + .parent_data = cpu_fsource_sel_parents,
>> >> > + .num_parents = ARRAY_SIZE(cpu_fsource_sel_parents),
>> >> > + .flags = CLK_SET_RATE_PARENT,
>> >> > + },
>> >> > +};
>> >> > +
>> >> > +static struct clk_regmap cpu_fsource_div1 = {
>> >> > + .data = &(struct clk_regmap_div_data) {
>> >> > + .offset = CPUCTRL_CLK_CTRL0,
>> >> > + .shift = 20,
>> >> > + .width = 6,
>> >> > + },
>> >> > + .hw.init = &(struct clk_init_data) {
>> >> > + .name = "cpu_fsource_div1",
>> >> > + .ops = &clk_regmap_divider_ops,
>> >> > + .parent_hws = (const struct clk_hw *[]) {
>> >> > + &cpu_fsource_sel1.hw
>> >> > + },
>> >> > + .num_parents = 1,
>> >> > + .flags = CLK_SET_RATE_PARENT,
>> >> > + },
>> >> > +};
>> >> > +
>> >> > +static struct clk_regmap cpu_fsel1 = {
>> >> > + .data = &(struct clk_regmap_mux_data) {
>> >> > + .offset = CPUCTRL_CLK_CTRL0,
>> >> > + .mask = 0x1,
>> >> > + .shift = 18,
>> >> > + },
>> >> > + .hw.init = &(struct clk_init_data) {
>> >> > + .name = "cpu_fsel1",
>> >> > + .ops = &clk_regmap_mux_ops,
>> >> > + .parent_hws = (const struct clk_hw *[]) {
>> >> > + &cpu_fsource_sel1.hw,
>> >> > + &cpu_fsource_div1.hw,
>> >> > + },
>> >> > + .num_parents = 2,
>> >> > + .flags = CLK_SET_RATE_PARENT,
>> >> > + },
>> >> > +};
>> >> > +
>> >> > +static struct clk_regmap cpu_fclk = {
>> >> > + .data = &(struct clk_regmap_mux_data) {
>> >> > + .offset = CPUCTRL_CLK_CTRL0,
>> >> > + .mask = 0x1,
>> >> > + .shift = 10,
>> >> > + },
>> >> > + .hw.init = &(struct clk_init_data) {
>> >> > + .name = "cpu_fclk",
>> >> > + .ops = &clk_regmap_mux_ops,
>> >> > + .parent_hws = (const struct clk_hw *[]) {
>> >> > + &cpu_fsel0.hw,
>> >> > + &cpu_fsel1.hw,
>> >> > + },
>> >> > + .num_parents = 2,
>> >> > + .flags = CLK_SET_RATE_PARENT,
>> >> > + },
>> >> > +};
>> >> > +
>> >> > +static struct clk_regmap cpu_clk = {
>> >> > + .data = &(struct clk_regmap_mux_data) {
>> >> > + .offset = CPUCTRL_CLK_CTRL0,
>> >> > + .mask = 0x1,
>> >> > + .shift = 11,
>> >> > + },
>> >> > + .hw.init = &(struct clk_init_data) {
>> >> > + .name = "cpu_clk",
>> >> > + .ops = &clk_regmap_mux_ops,
>> >> > + .parent_data = (const struct clk_parent_data []) {
>> >> > + { .hw = &cpu_fclk.hw },
>> >> > + { .fw_name = "sys_pll", },
>> >> > + },
>> >> > + .num_parents = 2,
>> >> > + .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>> >> > + },
>> >> > +};
>> >> > +
>> >> > +/* Array of all clocks registered by this provider */
>> >> > +static struct clk_hw *a1_cpu_hw_clks[] = {
>> >> > + [CLKID_CPU_FSOURCE_SEL0] = &cpu_fsource_sel0.hw,
>> >> > + [CLKID_CPU_FSOURCE_DIV0] = &cpu_fsource_div0.hw,
>> >> > + [CLKID_CPU_FSEL0] = &cpu_fsel0.hw,
>> >> > + [CLKID_CPU_FSOURCE_SEL1] = &cpu_fsource_sel1.hw,
>> >> > + [CLKID_CPU_FSOURCE_DIV1] = &cpu_fsource_div1.hw,
>> >> > + [CLKID_CPU_FSEL1] = &cpu_fsel1.hw,
>> >> > + [CLKID_CPU_FCLK] = &cpu_fclk.hw,
>> >> > + [CLKID_CPU_CLK] = &cpu_clk.hw,
>> >> > +};
>> >> > +
>> >> > +static struct clk_regmap *const a1_cpu_regmaps[] = {
>> >> > + &cpu_fsource_sel0,
>> >> > + &cpu_fsource_div0,
>> >> > + &cpu_fsel0,
>> >> > + &cpu_fsource_sel1,
>> >> > + &cpu_fsource_div1,
>> >> > + &cpu_fsel1,
>> >> > + &cpu_fclk,
>> >> > + &cpu_clk,
>> >> > +};
>> >> > +
>> >> > +static struct regmap_config a1_cpu_regmap_cfg = {
>> >> > + .reg_bits = 32,
>> >> > + .val_bits = 32,
>> >> > + .reg_stride = 4,
>> >> > + .max_register = CPUCTRL_CLK_CTRL1,
>> >> > +};
>> >> > +
>> >> > +static struct meson_clk_hw_data a1_cpu_clks = {
>> >> > + .hws = a1_cpu_hw_clks,
>> >> > + .num = ARRAY_SIZE(a1_cpu_hw_clks),
>> >> > +};
>> >> > +
>> >> > +struct a1_cpu_clk_nb_data {
>> >> > + const struct clk_ops *mux_ops;
>> >>
>> >> That's fishy ...
>> >>
>> >> > + struct clk_hw *cpu_clk;
>> >> > + struct notifier_block nb;
>> >> > + u8 parent;
>> >> > +};
>> >> > +
>> >> > +#define MESON_A1_CPU_CLK_GET_PARENT(nbd) \
>> >> > + ((nbd)->mux_ops->get_parent((nbd)->cpu_clk))
>> >> > +#define MESON_A1_CPU_CLK_SET_PARENT(nbd, index) \
>> >> > + ((nbd)->mux_ops->set_parent((nbd)->cpu_clk, index))
>> >>
>> >> ... Directly going for the mux ops ??!?? No way !
>> >>
>> >> We have a framework to handle the clocks, the whole point is to use it,
>> >> not bypass it !
>> >>
>> >
>> > I suppose you understand my approach, which is quite similar to what is
>> > happening in the Mediatek driver:
>> >
>> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/mediatek/clk-mux.c#L295
>> >
>> > Initially, I attempted to set the parent using the clk_set_parent() API.
>> > However, I encountered a problem with recursive calling of the
>> > notifier_block. This issue arises because the parent triggers
>> > notifications for its children, leading to repeated calls to the
>> > notifier_block.
>> >
>> > I find it puzzling why I cannot call an internal function or callback
>> > within the internal driver context. After all, the notifier block is
>> > just a part of the set_rate() flow. From a global Clock Control
>> > Framework perspective, the context should not change.
>>
>> I don't care what MTK is doing TBH. Forcefully calling ops on a clock,
>> hoping they are going to match with the clock type is wrong.
>>
>> There is a clk_hw API. Please it.
>>
>
> Yes, if sys_pll has a notifier_block instead of cpu_clk, there will be
> no problem with the clk_hw API.
>
> I will rework that point.
>
>> >
>> >> > +
>> >> > +static int meson_a1_cpu_clk_notifier_cb(struct notifier_block *nb,
>> >> > + unsigned long event, void *data)
>> >> > +{
>> >> > + struct a1_cpu_clk_nb_data *nbd;
>> >> > + int ret = 0;
>> >> > +
>> >> > + nbd = container_of(nb, struct a1_cpu_clk_nb_data, nb);
>> >> > +
>> >> > + switch (event) {
>> >> > + case PRE_RATE_CHANGE:
>> >> > + nbd->parent = MESON_A1_CPU_CLK_GET_PARENT(nbd);
>> >> > + /* Fallback to the CPU fixed clock */
>> >> > + ret = MESON_A1_CPU_CLK_SET_PARENT(nbd, 0);
>> >> > + /* Wait for clock propagation */
>> >> > + udelay(100);
>> >> > + break;
>> >> > +
>> >> > + case POST_RATE_CHANGE:
>> >> > + case ABORT_RATE_CHANGE:
>> >> > + /* Back to the original parent clock */
>> >> > + ret = MESON_A1_CPU_CLK_SET_PARENT(nbd, nbd->parent);
>> >> > + /* Wait for clock propagation */
>> >> > + udelay(100);
>> >> > + break;
>> >> > +
>> >> > + default:
>> >> > + pr_warn("Unknown event %lu for %s notifier block\n",
>> >> > + event, clk_hw_get_name(nbd->cpu_clk));
>> >> > + break;
>> >> > + }
>> >> > +
>> >> > + return notifier_from_errno(ret);
>> >> > +}
>> >> > +
>> >> > +static struct a1_cpu_clk_nb_data a1_cpu_clk_nb_data = {
>> >> > + .mux_ops = &clk_regmap_mux_ops,
>> >> > + .cpu_clk = &cpu_clk.hw,
>> >> > + .nb.notifier_call = meson_a1_cpu_clk_notifier_cb,
>> >> > +};
>> >> > +
>> >> > +static int meson_a1_dvfs_setup(struct platform_device *pdev)
>> >> > +{
>> >> > + struct device *dev = &pdev->dev;
>> >> > + struct clk *notifier_clk;
>> >> > + int ret;
>> >> > +
>> >> > + /* Setup clock notifier for cpu_clk */
>> >> > + notifier_clk = devm_clk_hw_get_clk(dev, &cpu_clk.hw, "dvfs");
>> >> > + if (IS_ERR(notifier_clk))
>> >> > + return dev_err_probe(dev, PTR_ERR(notifier_clk),
>> >> > + "can't get cpu_clk as notifier clock\n");
>> >> > +
>> >> > + ret = devm_clk_notifier_register(dev, notifier_clk,
>> >> > + &a1_cpu_clk_nb_data.nb);
>> >> > + if (ret)
>> >> > + return dev_err_probe(dev, ret,
>> >> > + "can't register cpu_clk notifier\n");
>> >> > +
>> >> > + return ret;
>> >> > +}
>> >> > +
>> >> > +static int meson_a1_cpu_probe(struct platform_device *pdev)
>> >> > +{
>> >> > + struct device *dev = &pdev->dev;
>> >> > + void __iomem *base;
>> >> > + struct regmap *map;
>> >> > + int clkid, i, err;
>> >> > +
>> >> > + base = devm_platform_ioremap_resource(pdev, 0);
>> >> > + if (IS_ERR(base))
>> >> > + return dev_err_probe(dev, PTR_ERR(base),
>> >> > + "can't ioremap resource\n");
>> >> > +
>> >> > + map = devm_regmap_init_mmio(dev, base, &a1_cpu_regmap_cfg);
>> >> > + if (IS_ERR(map))
>> >> > + return dev_err_probe(dev, PTR_ERR(map),
>> >> > + "can't init regmap mmio region\n");
>> >> > +
>> >> > + /* Populate regmap for the regmap backed clocks */
>> >> > + for (i = 0; i < ARRAY_SIZE(a1_cpu_regmaps); i++)
>> >> > + a1_cpu_regmaps[i]->map = map;
>> >> > +
>> >> > + for (clkid = 0; clkid < a1_cpu_clks.num; clkid++) {
>> >> > + err = devm_clk_hw_register(dev, a1_cpu_clks.hws[clkid]);
>> >> > + if (err)
>> >> > + return dev_err_probe(dev, err,
>> >> > + "clock[%d] registration failed\n",
>> >> > + clkid);
>> >> > + }
>> >> > +
>> >> > + err = devm_of_clk_add_hw_provider(dev, meson_clk_hw_get, &a1_cpu_clks);
>> >> > + if (err)
>> >> > + return dev_err_probe(dev, err, "can't add clk hw provider\n");
>> >>
>> >> I wonder if there is a window of opportunity to poke the syspll without
>> >> your notifier here. That being said, the situation would be similar on g12.
>> >>
>> >
>> > Yes, I have taken into account what you did in the G12A CPU clock
>> > relations. My thoughts were that it might not be applicable for the A1
>> > case. This is because the sys_pll should be located in a different
>> > driver from a logical perspective. Consequently, we cannot configure the
>> > sys_pll notifier block to manage the cpu_clk from a different driver.
>> > However, if I were to move the sys_pll clock object to the A1 CPU clock
>> > controller, I believe the g12a sys_pll notifier approach would work.
>> >
>>
>> I fail to see the connection between the number of device and the
>> approach you took.
>>
>>
>> >> > +
>> >> > + return meson_a1_dvfs_setup(pdev);
>> >>
>> >>
>> >>
>> >> > +}
>> >> > +
>> >> > +static const struct of_device_id a1_cpu_clkc_match_table[] = {
>> >> > + { .compatible = "amlogic,a1-cpu-clkc", },
>> >> > + {}
>> >> > +};
>> >> > +MODULE_DEVICE_TABLE(of, a1_cpu_clkc_match_table);
>> >> > +
>> >> > +static struct platform_driver a1_cpu_clkc_driver = {
>> >> > + .probe = meson_a1_cpu_probe,
>> >> > + .driver = {
>> >> > + .name = "a1-cpu-clkc",
>> >> > + .of_match_table = a1_cpu_clkc_match_table,
>> >> > + },
>> >> > +};
>> >> > +
>> >> > +module_platform_driver(a1_cpu_clkc_driver);
>> >> > +MODULE_AUTHOR("Dmitry Rokosov <ddrokosov@salutedevices.com>");
>> >> > +MODULE_LICENSE("GPL");
>> >> > diff --git a/drivers/clk/meson/a1-cpu.h b/drivers/clk/meson/a1-cpu.h
>> >> > new file mode 100644
>> >> > index 000000000000..e9af4117e26f
>> >> > --- /dev/null
>> >> > +++ b/drivers/clk/meson/a1-cpu.h
>> >>
>> >> There is not point putting the definition here in a header
>> >> These are clearly not going to be shared with another driver.
>> >>
>> >> Please drop this file
>> >>
>> >
>> > The same approach was applied to the Peripherals and PLL A1 drivers.
>> > Honestly, I am not a fan of having different file organization within a
>> > single logical code folder.
>> >
>> > Please refer to:
>> >
>> > drivers/clk/meson/a1-peripherals.h
>> > drivers/clk/meson/a1-pll.h
>>
>> I understand. There was a time where it made sense, some definition
>> could have been shared back then. This is no longer the case and it
>> would probably welcome a rework.
>>
>> ... and again, just pointing to another code does really invalidate my
>> point.
>>
>> >
>> >> > @@ -0,0 +1,16 @@
>> >> > +/* SPDX-License-Identifier: GPL-2.0+ */
>> >> > +/*
>> >> > + * Amlogic A1 CPU Clock Controller internals
>> >> > + *
>> >> > + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
>> >> > + * Author: Dmitry Rokosov <ddrokosov@salutedevices.com>
>> >> > + */
>> >> > +
>> >> > +#ifndef __A1_CPU_H
>> >> > +#define __A1_CPU_H
>> >> > +
>> >> > +/* cpu clock controller register offset */
>> >> > +#define CPUCTRL_CLK_CTRL0 0x80
>> >> > +#define CPUCTRL_CLK_CTRL1 0x84
>> >>
>> >> You are claiming the registers from 0x00 to 0x84 (included), but only
>> >> using these 2 registers ? What is the rest ? Are you sure there is only
>> >> clocks in there ?
>> >>
>> >
>> > Yes, unfortunately, the register map for this IP is not described in the
>> > A1 Datasheet. The only available information about it can be found in
>> > the vendor clock driver, which provides details for only two registers
>> > used to configure the CPU clock.
>> >
>> > From vendor kernel dtsi:
>> >
>> > clkc: clock-controller {
>> > compatible = "amlogic,a1-clkc";
>> > #clock-cells = <1>;
>> > reg = <0x0 0xfe000800 0x0 0x100>,
>> > <0x0 0xfe007c00 0x0 0x21c>,
>> > <0x0 0xfd000000 0x0 0x88>; <==== CPU clock regmap
>> > reg-names = "basic", "pll",
>> > "cpu_clk";
>> > clocks = <&xtal>;
>> > clock-names = "core";
>> > status = "okay";
>> > };
>> >
>> > From vendor clkc driver:
>> >
>> > /*
>> > * CPU clok register offset
>> > * APB_BASE: APB1_BASE_ADDR = 0xfd000000
>> > */
>> >
>> > #define CPUCTRL_CLK_CTRL0 0x80
>> > #define CPUCTRL_CLK_CTRL1 0x84
>>
>> If you had clock in 0x0 and 0x80, then I would agree claiming 0x0-0x88
>> is reasonable, even if you don't really know what is in between. It
>> would be a fair assumption.
>>
>> It is not the case here.
>> For all we know it could resets, power domains, etc ...
>>
>
> However, we do not have any information indicating that the gap from
> 0x00-0x80 contains additional registers. It is possible that there are
> internal registers, but they are not mentioned in the vendor datasheet
> or driver. Therefore, in this case, I personally prefer to rely on the
> vendor mapping.
I understand your preference. My request remains.
--
Jerome
^ permalink raw reply
* Re: [PATCH v3] clk: starfive: pll: Fix lower rate of CPUfreq by setting PLL0 rate to 1.5GHz
From: Krzysztof Kozlowski @ 2024-04-02 16:18 UTC (permalink / raw)
To: Xingyu Wu, Michael Turquette, Stephen Boyd, Conor Dooley,
Emil Renner Berthing, Rob Herring, Krzysztof Kozlowski
Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Hal Feng, linux-kernel,
linux-clk, linux-riscv, devicetree
In-Reply-To: <20240402090920.11627-1-xingyu.wu@starfivetech.com>
On 02/04/2024 11:09, Xingyu Wu wrote:
> CPUfreq supports 4 cpu frequency loads on 375/500/750/1500MHz.
> But now PLL0 rate is 1GHz and the cpu frequency loads become
> 333/500/500/1000MHz in fact.
>
> So PLL0 rate should be default set to 1.5GHz. But setting the
> PLL0 rate need certain steps:
>
> 1. Change the parent of cpu_root clock to OSC clock.
> 2. Change the divider of cpu_core if PLL0 rate is higher than
> 1.25GHz before CPUfreq boot.
> 3. Change the parent of cpu_root clock back to PLL0 clock.
>
> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
> Fixes: e2c510d6d630 ("riscv: dts: starfive: Add cpu scaling for JH7110 SoC")
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> ---
>
> Hi Stephen and Emil,
>
> This patch fixes the issue about lower rate of CPUfreq[1] by setting PLL0
> rate to 1.5GHz.
>
> In order not to affect the cpu operation, setting the PLL0 rate need
> certain steps. The cpu_root's parent clock should be changed first. And
> the divider of the cpu_core clock should be set to 2 so they won't crash
> when setting 1.5GHz without voltage regulation. Due to PLL driver boot
> earlier than SYSCRG driver, cpu_core and cpu_root clocks are using by
> ioremap().
>
> [1]: https://github.com/starfive-tech/VisionFive2/issues/55
>
> Previous patch link:
> v2: https://lore.kernel.org/all/20230821152915.208366-1-xingyu.wu@starfivetech.com/
> v1: https://lore.kernel.org/all/20230811033631.160912-1-xingyu.wu@starfivetech.com/
>
> Thanks,
> Xingyu Wu
> ---
> .../jh7110-starfive-visionfive-2.dtsi | 5 +
> .../clk/starfive/clk-starfive-jh7110-pll.c | 102 ++++++++++++++++++
Please do not mix DTS and driver code. That's not really portable. DTS
is being exported and used in other projects.
...
>
> @@ -458,6 +535,8 @@ static int jh7110_pll_probe(struct platform_device *pdev)
> struct jh7110_pll_priv *priv;
> unsigned int idx;
> int ret;
> + struct device_node *np;
> + struct resource res;
>
> priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> if (!priv)
> @@ -489,6 +568,29 @@ static int jh7110_pll_probe(struct platform_device *pdev)
> return ret;
> }
>
> + priv->is_first_set = true;
> + np = of_find_compatible_node(NULL, NULL, "starfive,jh7110-syscrg");
Your drivers should not do it. It's fragile, hides true link/dependency.
Please use phandles.
> + if (!np) {
> + ret = PTR_ERR(np);
> + dev_err(priv->dev, "failed to get syscrg node\n");
> + goto np_put;
> + }
> +
> + ret = of_address_to_resource(np, 0, &res);
> + if (ret) {
> + dev_err(priv->dev, "failed to get syscrg resource\n");
> + goto np_put;
> + }
> +
> + priv->syscrg_base = ioremap(res.start, resource_size(&res));
> + if (!priv->syscrg_base)
> + ret = -ENOMEM;
Why are you mapping other device's IO? How are you going to ensure
synced access to registers?
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH] arm64: dts: ti: k3-j722s: Disable ethernet ports by default
From: Michael Walle @ 2024-04-02 16:16 UTC (permalink / raw)
To: Nishanth Menon
Cc: Vignesh Raghavendra, Tero Kristo, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-arm-kernel, devicetree,
linux-kernel
In-Reply-To: <20240402161306.tkg35heqlwqxoaua@another>
[-- Attachment #1: Type: text/plain, Size: 176 bytes --]
Hi,
> This is better at arch/arm64/boot/dts/ti/k3-am62p-main.dtsi.
I'm fine with that, but be aware that we'll also change the am62p
SoC dtsi in that case.
-michael
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]
^ permalink raw reply
* Re: [PATCH v5 0/5] PCI: dwc: Add common pme_turn_off message by using outbound iATU
From: Frank Li @ 2024-04-02 16:15 UTC (permalink / raw)
To: Bjorn Helgaas, Jingoo Han, Gustavo Pimentel,
Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, imx
Cc: linux-pci, linux-kernel, devicetree, Yoshihiro Shimoda,
Serge Semin
In-Reply-To: <20240319-pme_msg-v5-0-af9ffe57f432@nxp.com>
On Tue, Mar 19, 2024 at 12:07:10PM -0400, Frank Li wrote:
> Involve an new and common mathod to send pme_turn_off() message. Previously
> pme_turn_off() implement by platform related special register to trigge
> it.
>
> But Yoshihiro give good idea by using iATU to send out message. Previously
> Yoshihiro provide patches to raise INTx message by dummy write to outbound
> iATU.
>
> Use similar mathod to send out pme_turn_off message.
>
> Previous two patches is picked from Yoshihiro' big patch serialise.
> PCI: dwc: Change arguments of dw_pcie_prog_outbound_atu()
> PCI: Add INTx Mechanism Messages macros
>
> PCI: Add PME_TURN_OFF message macro
> dt-bindings: PCI: dwc: Add 'msg" register region, Add "msg" region to use
> to map PCI msg.
>
> PCI: dwc: Add common pme_turn_off message method
> Using common pme_turn_off() message if platform have not define their.
>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Changes in v5:
> - Default disable allocate TLP message memory windows. If driver need use
> this feature, need set use_atu_msg = true before call dw_host_init().
Mani, lorenzo and bjorn
Any comments about these patches? I already set this feature default as
false. It is common for all dwc platform.
After this merge, imx6 and layerscape many customer PME code can be
removed.
Frank
>
> - Link to v4: https://lore.kernel.org/r/20240213-pme_msg-v4-0-e2acd4d7a292@nxp.com
>
> Changes in v4:
> - Remove dt-binding patch. Needn't change any dts file and binding doc.
> Reserve a region at end of first IORESOURCE_MEM window by call
> request_resource(). So PCIe stack will not use this reserve region to any
> PCIe devices.
> I tested it by reserve at begin of IORESOURCE_MEM window. PCIe stack
> will skip it as expection.
>
> Fixed a issue, forget set iATU index when sent PME_turn_off.
>
> - Link to v3: https://lore.kernel.org/r/20240202-pme_msg-v3-0-ff2af57a02ad@nxp.com
>
> Changes in v3:
> - fix 'MSG"
> - Add pcie spec ref in head file
> - using function name dw_pci_pme_turn_off()
> - Using PCIE_ prefix macro
> - Link to v2: https://lore.kernel.org/r/20240201-pme_msg-v2-0-6767052fe6a4@nxp.com
>
> Changes in v2:
> - Add my sign off at PCI: dwc: Add outbound MSG TLPs support
> - Add Bjorn review tag at Add INTx Mechanism Messages macros
> - using PME_Turn_Off match PCIe spec
> - ref to pcie spec v6.1
> - using section number.
>
> - Link to v1: https://lore.kernel.org/r/20240130-pme_msg-v1-0-d52b0add5c7c@nxp.com
>
> ---
> Frank Li (2):
> PCI: Add PCIE_MSG_CODE_PME_TURN_OFF message macro
> PCI: dwc: Add common send PME_Turn_Off message method
>
> Yoshihiro Shimoda (3):
> PCI: Add INTx Mechanism Messages macros
> PCI: dwc: Consolidate args of dw_pcie_prog_outbound_atu() into a structure
> PCI: dwc: Add outbound MSG TLPs support
>
> drivers/pci/controller/dwc/pcie-designware-ep.c | 21 ++--
> drivers/pci/controller/dwc/pcie-designware-host.c | 146 +++++++++++++++++++---
> drivers/pci/controller/dwc/pcie-designware.c | 54 ++++----
> drivers/pci/controller/dwc/pcie-designware.h | 22 +++-
> drivers/pci/pci.h | 20 +++
> 5 files changed, 199 insertions(+), 64 deletions(-)
> ---
> base-commit: e08fc59eee9991afa467d406d684d46d543299a9
> change-id: 20240130-pme_msg-dd2d81ee9886
>
> Best regards,
> ---
> Frank Li <Frank.Li@nxp.com>
>
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: phy: qmp-ufs: Fix PHY clocks for SC7180
From: Rob Herring @ 2024-04-02 16:15 UTC (permalink / raw)
To: Danila Tikhonov
Cc: davidwronek, manivannan.sadhasivam, krzysztof.kozlowski+dt,
linux-arm-msm, konrad.dybcio, linux-phy, conor+dt, kishon,
devicetree, andersson, vkoul, linux-kernel,
cros-qcom-dts-watchers
In-Reply-To: <20240401182240.55282-2-danila@jiaxyga.com>
On Mon, 01 Apr 2024 21:22:39 +0300, Danila Tikhonov wrote:
> QMP UFS PHY used in SC7180 requires 3 clocks:
>
> * ref - 19.2MHz reference clock from RPMh
> * ref_aux - Auxiliary reference clock from GCC
> * qref - QREF clock from GCC
>
> This change obviously breaks the ABI, but it is inevitable since the
> clock topology needs to be accurately described in the binding.
>
> Signed-off-by: Danila Tikhonov <danila@jiaxyga.com>
> ---
> .../devicetree/bindings/phy/qcom,sc8280xp-qmp-ufs-phy.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [RFC PATCH 5/6] watchdog: ROHM BD96801 PMIC WDG driver
From: Krzysztof Kozlowski @ 2024-04-02 16:15 UTC (permalink / raw)
To: Matti Vaittinen, Matti Vaittinen
Cc: Lee Jones, Wim Van Sebroeck, Guenter Roeck, devicetree,
linux-kernel, linux-watchdog
In-Reply-To: <f8e743a6c49607de0dd7a27778383477e051b130.1712058690.git.mazziesaccount@gmail.com>
On 02/04/2024 15:11, Matti Vaittinen wrote:
> Introduce driver for WDG block on ROHM BD96801 scalable PMIC.
>
> This driver only supports watchdog with I2C feeding and delayed
> response detection. Whether the watchdog toggles PRSTB pin or
> just causes an interrupt can be configured via device-tree.
>
> The BD96801 PMIC HW supports also window watchdog (too early
...
> +
> +static struct platform_driver bd96801_wdt = {
> + .driver = {
> + .name = "bd96801-wdt"
> + },
> + .probe = bd96801_wdt_probe,
> +};
> +module_platform_driver(bd96801_wdt);
> +
> +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
> +MODULE_DESCRIPTION("BD96801 watchdog driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:bd96801-wdt");
Same comment on alias. Please use ID table.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 2/2] arm64: dts: ti: k3-am642-phyboard-electra-rdk: Increase CAN max bitrate
From: Nishanth Menon @ 2024-04-02 16:14 UTC (permalink / raw)
To: Nathan Morrisson
Cc: vigneshr, kristo, robh, krzysztof.kozlowski+dt, conor+dt,
linux-arm-kernel, devicetree, linux-kernel, upstream, w.egorov
In-Reply-To: <20240402161203.q34gyqfaoewvjbky@unburned>
On 11:12-20240402, Nishanth Menon wrote:
> On 09:08-20240402, Nathan Morrisson wrote:
> > The phyBOARD-Electra has two TCAN1044VDD CAN transceivers which
> > support CAN FD at 8 Mbps.
> >
> > Increase the maximum bitrate to 8 Mbps.
> >
> > Signed-off-by: Nathan Morrisson <nmorrisson@phytec.com>
> > ---
> > arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts b/arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts
> > index 8237b8c815b8..522699ec65e8 100644
> > --- a/arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts
> > +++ b/arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts
> > @@ -42,7 +42,7 @@ can_tc1: can-phy0 {
> > pinctrl-names = "default";
> > pinctrl-0 = <&can_tc1_pins_default>;
> > #phy-cells = <0>;
> > - max-bitrate = <5000000>;
> > + max-bitrate = <8000000>;
> > standby-gpios = <&main_gpio0 32 GPIO_ACTIVE_HIGH>;
> > };
> >
> > @@ -51,7 +51,7 @@ can_tc2: can-phy1 {
> > pinctrl-names = "default";
> > pinctrl-0 = <&can_tc2_pins_default>;
> > #phy-cells = <0>;
> > - max-bitrate = <5000000>;
> > + max-bitrate = <8000000>;
> > standby-gpios = <&main_gpio0 35 GPIO_ACTIVE_HIGH>;
> > };
> >
> > --
> > 2.25.1
> >
>
> This is better at arch/arm64/boot/dts/ti/k3-am62p-main.dtsi.
>
Woops.. wrong mail thread. :( Apologies on the spam.
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
^ permalink raw reply
* Re: [PATCH] arm64: dts: ti: k3-j722s: Disable ethernet ports by default
From: Nishanth Menon @ 2024-04-02 16:13 UTC (permalink / raw)
To: Michael Walle
Cc: Vignesh Raghavendra, Tero Kristo, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-arm-kernel, devicetree,
linux-kernel
In-Reply-To: <20240402151802.3803708-1-mwalle@kernel.org>
On 17:18-20240402, Michael Walle wrote:
> Device tree best practice is to disable any external interface in the
> dtsi and just enable them if needed in the device tree. Thus, disable
> both ethernet ports by default and just enable the one used by the EVM
> in its device tree.
>
> There is no functional change.
>
> Signed-off-by: Michael Walle <mwalle@kernel.org>
> ---
> This should also be true for all the other SoCs. But I don't wanted to
> touch all the (older) device trees. j722s is pretty new, so there we
> should get it right.
> ---
> arch/arm64/boot/dts/ti/k3-j722s-evm.dts | 5 +----
> arch/arm64/boot/dts/ti/k3-j722s.dtsi | 8 ++++++++
> 2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/ti/k3-j722s-evm.dts b/arch/arm64/boot/dts/ti/k3-j722s-evm.dts
> index d045dc7dde0c..afe7f68e6a4b 100644
> --- a/arch/arm64/boot/dts/ti/k3-j722s-evm.dts
> +++ b/arch/arm64/boot/dts/ti/k3-j722s-evm.dts
> @@ -224,14 +224,11 @@ cpsw3g_phy0: ethernet-phy@0 {
> };
>
> &cpsw_port1 {
> + status = "okay";
> phy-mode = "rgmii-rxid";
> phy-handle = <&cpsw3g_phy0>;
> };
>
> -&cpsw_port2 {
> - status = "disabled";
> -};
> -
> &main_gpio1 {
> status = "okay";
> };
> diff --git a/arch/arm64/boot/dts/ti/k3-j722s.dtsi b/arch/arm64/boot/dts/ti/k3-j722s.dtsi
> index c75744edb143..d0451e6e7496 100644
> --- a/arch/arm64/boot/dts/ti/k3-j722s.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-j722s.dtsi
> @@ -83,6 +83,14 @@ &inta_main_dmss {
> ti,interrupt-ranges = <7 71 21>;
> };
>
> +&cpsw_port1 {
> + status = "disabled";
> +};
> +
> +&cpsw_port2 {
> + status = "disabled";
> +};
> +
> &oc_sram {
> reg = <0x00 0x70000000 0x00 0x40000>;
> ranges = <0x00 0x00 0x70000000 0x40000>;
> --
> 2.39.2
>
Meant to respond to this thread:
This is better at arch/arm64/boot/dts/ti/k3-am62p-main.dtsi.
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
^ permalink raw reply
* Re: [PATCH v6 1/4] dt-bindings: clock: add i.MX95 clock header
From: Rob Herring @ 2024-04-02 16:13 UTC (permalink / raw)
To: Peng Fan (OSS)
Cc: linux-clk, Krzysztof Kozlowski, Shawn Guo, Sascha Hauer, Peng Fan,
Abel Vesa, Pengutronix Kernel Team, devicetree, linux-arm-kernel,
Michael Turquette, Stephen Boyd, Fabio Estevam, imx, linux-kernel,
Conor Dooley
In-Reply-To: <20240401-imx95-blk-ctl-v6-1-84d4eca1e759@nxp.com>
On Mon, 01 Apr 2024 21:28:15 +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> Add clock header for i.MX95 BLK CTL modules
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> include/dt-bindings/clock/nxp,imx95-clock.h | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v2 2/2] ASoC: dt-bindings: fsl-asoc-card: convert to YAML
From: Rob Herring @ 2024-04-02 16:12 UTC (permalink / raw)
To: Shengjiu Wang
Cc: festevam, shengjiu.wang, shawnguo, broonie, linux-kernel, robh+dt,
linux-arm-kernel, krzysztof.kozlowski+dt, conor+dt, imx, s.hauer,
lgirdwood, linux-sound, kernel, devicetree
In-Reply-To: <1711976056-19884-3-git-send-email-shengjiu.wang@nxp.com>
On Mon, 01 Apr 2024 20:54:16 +0800, Shengjiu Wang wrote:
> Convert the fsl-asoc-card binding to YAML.
>
> When testing dtbs_check, found below compatible strings
> are not listed in document:
>
> fsl,imx-sgtl5000
> fsl,imx53-cpuvo-sgtl5000
> fsl,imx51-babbage-sgtl5000
> fsl,imx53-m53evk-sgtl5000
> fsl,imx53-qsb-sgtl5000
> fsl,imx53-voipac-sgtl5000
> fsl,imx6-armadeus-sgtl5000
> fsl,imx6-rex-sgtl5000
> fsl,imx6-sabreauto-cs42888
> fsl,imx6-wandboard-sgtl5000
> fsl,imx6dl-nit6xlite-sgtl5000
> fsl,imx6q-ba16-sgtl5000
> fsl,imx6q-nitrogen6_max-sgtl5000
> fsl,imx6q-nitrogen6_som2-sgtl5000
> fsl,imx6q-nitrogen6x-sgtl5000
> fsl,imx6q-sabrelite-sgtl5000
> fsl,imx6q-sabresd-wm8962
> fsl,imx6q-udoo-ac97
> fsl,imx6q-ventana-sgtl5000
> fsl,imx6sl-evk-wm8962
> fsl,imx6sx-sdb-mqs
> fsl,imx6sx-sdb-wm8962
> fsl,imx7d-evk-wm8960
> karo,tx53-audio-sgtl5000
> tq,imx53-mba53-sgtl5000
>
> So add them in yaml file to pass the test.
>
> Also correct the 'dai-format' to 'format' in document.
>
> For 'audio-routing', the items are not listed. Because
> this fsl-asoc-card is generic driver, which supports several
> codecs, if list all the items, there will be a long list.
>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
> .../bindings/sound/fsl-asoc-card.txt | 117 -----------
> .../bindings/sound/fsl-asoc-card.yaml | 195 ++++++++++++++++++
> 2 files changed, 195 insertions(+), 117 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
> create mode 100644 Documentation/devicetree/bindings/sound/fsl-asoc-card.yaml
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH 2/2] arm64: dts: ti: k3-am642-phyboard-electra-rdk: Increase CAN max bitrate
From: Nishanth Menon @ 2024-04-02 16:12 UTC (permalink / raw)
To: Nathan Morrisson
Cc: vigneshr, kristo, robh, krzysztof.kozlowski+dt, conor+dt,
linux-arm-kernel, devicetree, linux-kernel, upstream, w.egorov
In-Reply-To: <20240402160825.1516036-3-nmorrisson@phytec.com>
On 09:08-20240402, Nathan Morrisson wrote:
> The phyBOARD-Electra has two TCAN1044VDD CAN transceivers which
> support CAN FD at 8 Mbps.
>
> Increase the maximum bitrate to 8 Mbps.
>
> Signed-off-by: Nathan Morrisson <nmorrisson@phytec.com>
> ---
> arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts b/arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts
> index 8237b8c815b8..522699ec65e8 100644
> --- a/arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts
> +++ b/arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts
> @@ -42,7 +42,7 @@ can_tc1: can-phy0 {
> pinctrl-names = "default";
> pinctrl-0 = <&can_tc1_pins_default>;
> #phy-cells = <0>;
> - max-bitrate = <5000000>;
> + max-bitrate = <8000000>;
> standby-gpios = <&main_gpio0 32 GPIO_ACTIVE_HIGH>;
> };
>
> @@ -51,7 +51,7 @@ can_tc2: can-phy1 {
> pinctrl-names = "default";
> pinctrl-0 = <&can_tc2_pins_default>;
> #phy-cells = <0>;
> - max-bitrate = <5000000>;
> + max-bitrate = <8000000>;
> standby-gpios = <&main_gpio0 35 GPIO_ACTIVE_HIGH>;
> };
>
> --
> 2.25.1
>
This is better at arch/arm64/boot/dts/ti/k3-am62p-main.dtsi.
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
^ permalink raw reply
* Re: [PATCH v1 6/6] clk: meson: a1: add Amlogic A1 CPU clock controller driver
From: Dmitry Rokosov @ 2024-04-02 16:11 UTC (permalink / raw)
To: Jerome Brunet
Cc: neil.armstrong, mturquette, sboyd, robh+dt,
krzysztof.kozlowski+dt, khilman, martin.blumenstingl, kernel,
rockosov, linux-amlogic, linux-clk, devicetree, linux-kernel,
linux-arm-kernel
In-Reply-To: <1jr0fnj11b.fsf@starbuckisacylon.baylibre.com>
On Tue, Apr 02, 2024 at 04:11:12PM +0200, Jerome Brunet wrote:
>
> On Tue 02 Apr 2024 at 14:05, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
>
> > Hello Jerome,
> >
> > On Tue, Apr 02, 2024 at 11:35:49AM +0200, Jerome Brunet wrote:
> >>
> >> On Fri 29 Mar 2024 at 23:58, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
> >>
> >> > The CPU clock controller plays a general role in the Amlogic A1 SoC
> >> > family by generating CPU clocks. As an APB slave module, it offers the
> >> > capability to inherit the CPU clock from two sources: the internal fixed
> >> > clock known as 'cpu fixed clock' and the external input provided by the
> >> > A1 PLL clock controller, referred to as 'syspll'.
> >> >
> >> > It is important for the driver to handle cpu_clk rate switching
> >> > effectively by transitioning to the CPU fixed clock to avoid any
> >> > potential execution freezes.
> >> >
> >> > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> >> > ---
> >> > drivers/clk/meson/Kconfig | 10 ++
> >> > drivers/clk/meson/Makefile | 1 +
> >> > drivers/clk/meson/a1-cpu.c | 324 +++++++++++++++++++++++++++++++++++++
> >> > drivers/clk/meson/a1-cpu.h | 16 ++
> >> > 4 files changed, 351 insertions(+)
> >> > create mode 100644 drivers/clk/meson/a1-cpu.c
> >> > create mode 100644 drivers/clk/meson/a1-cpu.h
> >> >
> >> > diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> >> > index 80c4a18c83d2..148d4495eee3 100644
> >> > --- a/drivers/clk/meson/Kconfig
> >> > +++ b/drivers/clk/meson/Kconfig
> >> > @@ -111,6 +111,16 @@ config COMMON_CLK_AXG_AUDIO
> >> > Support for the audio clock controller on AmLogic A113D devices,
> >> > aka axg, Say Y if you want audio subsystem to work.
> >> >
> >> > +config COMMON_CLK_A1_CPU
> >> > + tristate "Amlogic A1 SoC CPU controller support"
> >> > + depends on ARM64
> >> > + select COMMON_CLK_MESON_REGMAP
> >> > + select COMMON_CLK_MESON_CLKC_UTILS
> >> > + help
> >> > + Support for the CPU clock controller on Amlogic A113L based
> >> > + device, A1 SoC Family. Say Y if you want A1 CPU clock controller
> >> > + to work.
> >> > +
> >> > config COMMON_CLK_A1_PLL
> >> > tristate "Amlogic A1 SoC PLL controller support"
> >> > depends on ARM64
> >> > diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> >> > index 4968fc7ad555..2a06eb0303d6 100644
> >> > --- a/drivers/clk/meson/Makefile
> >> > +++ b/drivers/clk/meson/Makefile
> >> > @@ -18,6 +18,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_AUDIO_RSTC) += meson-audio-rstc.o
> >> >
> >> > obj-$(CONFIG_COMMON_CLK_AXG) += axg.o axg-aoclk.o
> >> > obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o
> >> > +obj-$(CONFIG_COMMON_CLK_A1_CPU) += a1-cpu.o
> >> > obj-$(CONFIG_COMMON_CLK_A1_PLL) += a1-pll.o
> >> > obj-$(CONFIG_COMMON_CLK_A1_PERIPHERALS) += a1-peripherals.o
> >> > obj-$(CONFIG_COMMON_CLK_A1_AUDIO) += a1-audio.o
> >> > diff --git a/drivers/clk/meson/a1-cpu.c b/drivers/clk/meson/a1-cpu.c
> >> > new file mode 100644
> >> > index 000000000000..5f5d8ae112e5
> >> > --- /dev/null
> >> > +++ b/drivers/clk/meson/a1-cpu.c
> >> > @@ -0,0 +1,324 @@
> >> > +// SPDX-License-Identifier: GPL-2.0+
> >> > +/*
> >> > + * Amlogic A1 SoC family CPU Clock Controller driver.
> >> > + *
> >> > + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
> >> > + * Author: Dmitry Rokosov <ddrokosov@salutedevices.com>
> >> > + */
> >> > +
> >> > +#include <linux/clk.h>
> >> > +#include <linux/clk-provider.h>
> >> > +#include <linux/mod_devicetable.h>
> >> > +#include <linux/platform_device.h>
> >> > +#include "a1-cpu.h"
> >> > +#include "clk-regmap.h"
> >> > +#include "meson-clkc-utils.h"
> >> > +
> >> > +#include <dt-bindings/clock/amlogic,a1-cpu-clkc.h>
> >> > +
> >> > +static u32 cpu_fsource_sel_table[] = { 0, 1, 2 };
> >> > +static const struct clk_parent_data cpu_fsource_sel_parents[] = {
> >> > + { .fw_name = "xtal" },
> >> > + { .fw_name = "fclk_div2" },
> >> > + { .fw_name = "fclk_div3" },
> >> > +};
> >> > +
> >> > +static struct clk_regmap cpu_fsource_sel0 = {
> >> > + .data = &(struct clk_regmap_mux_data) {
> >> > + .offset = CPUCTRL_CLK_CTRL0,
> >> > + .mask = 0x3,
> >> > + .shift = 0,
> >> > + .table = cpu_fsource_sel_table,
> >> > + },
> >> > + .hw.init = &(struct clk_init_data) {
> >> > + .name = "cpu_fsource_sel0",
> >> > + .ops = &clk_regmap_mux_ops,
> >> > + .parent_data = cpu_fsource_sel_parents,
> >> > + .num_parents = ARRAY_SIZE(cpu_fsource_sel_parents),
> >> > + .flags = CLK_SET_RATE_PARENT,
> >> > + },
> >> > +};
> >> > +
> >> > +static struct clk_regmap cpu_fsource_div0 = {
> >> > + .data = &(struct clk_regmap_div_data) {
> >> > + .offset = CPUCTRL_CLK_CTRL0,
> >> > + .shift = 4,
> >> > + .width = 6,
> >> > + },
> >> > + .hw.init = &(struct clk_init_data) {
> >> > + .name = "cpu_fsource_div0",
> >> > + .ops = &clk_regmap_divider_ops,
> >> > + .parent_hws = (const struct clk_hw *[]) {
> >> > + &cpu_fsource_sel0.hw
> >> > + },
> >> > + .num_parents = 1,
> >> > + .flags = CLK_SET_RATE_PARENT,
> >> > + },
> >> > +};
> >> > +
> >> > +static struct clk_regmap cpu_fsel0 = {
> >> > + .data = &(struct clk_regmap_mux_data) {
> >> > + .offset = CPUCTRL_CLK_CTRL0,
> >> > + .mask = 0x1,
> >> > + .shift = 2,
> >> > + },
> >> > + .hw.init = &(struct clk_init_data) {
> >> > + .name = "cpu_fsel0",
> >> > + .ops = &clk_regmap_mux_ops,
> >> > + .parent_hws = (const struct clk_hw *[]) {
> >> > + &cpu_fsource_sel0.hw,
> >> > + &cpu_fsource_div0.hw,
> >> > + },
> >> > + .num_parents = 2,
> >> > + .flags = CLK_SET_RATE_PARENT,
> >> > + },
> >> > +};
> >> > +
> >> > +static struct clk_regmap cpu_fsource_sel1 = {
> >> > + .data = &(struct clk_regmap_mux_data) {
> >> > + .offset = CPUCTRL_CLK_CTRL0,
> >> > + .mask = 0x3,
> >> > + .shift = 16,
> >> > + .table = cpu_fsource_sel_table,
> >> > + },
> >> > + .hw.init = &(struct clk_init_data) {
> >> > + .name = "cpu_fsource_sel1",
> >> > + .ops = &clk_regmap_mux_ops,
> >> > + .parent_data = cpu_fsource_sel_parents,
> >> > + .num_parents = ARRAY_SIZE(cpu_fsource_sel_parents),
> >> > + .flags = CLK_SET_RATE_PARENT,
> >> > + },
> >> > +};
> >> > +
> >> > +static struct clk_regmap cpu_fsource_div1 = {
> >> > + .data = &(struct clk_regmap_div_data) {
> >> > + .offset = CPUCTRL_CLK_CTRL0,
> >> > + .shift = 20,
> >> > + .width = 6,
> >> > + },
> >> > + .hw.init = &(struct clk_init_data) {
> >> > + .name = "cpu_fsource_div1",
> >> > + .ops = &clk_regmap_divider_ops,
> >> > + .parent_hws = (const struct clk_hw *[]) {
> >> > + &cpu_fsource_sel1.hw
> >> > + },
> >> > + .num_parents = 1,
> >> > + .flags = CLK_SET_RATE_PARENT,
> >> > + },
> >> > +};
> >> > +
> >> > +static struct clk_regmap cpu_fsel1 = {
> >> > + .data = &(struct clk_regmap_mux_data) {
> >> > + .offset = CPUCTRL_CLK_CTRL0,
> >> > + .mask = 0x1,
> >> > + .shift = 18,
> >> > + },
> >> > + .hw.init = &(struct clk_init_data) {
> >> > + .name = "cpu_fsel1",
> >> > + .ops = &clk_regmap_mux_ops,
> >> > + .parent_hws = (const struct clk_hw *[]) {
> >> > + &cpu_fsource_sel1.hw,
> >> > + &cpu_fsource_div1.hw,
> >> > + },
> >> > + .num_parents = 2,
> >> > + .flags = CLK_SET_RATE_PARENT,
> >> > + },
> >> > +};
> >> > +
> >> > +static struct clk_regmap cpu_fclk = {
> >> > + .data = &(struct clk_regmap_mux_data) {
> >> > + .offset = CPUCTRL_CLK_CTRL0,
> >> > + .mask = 0x1,
> >> > + .shift = 10,
> >> > + },
> >> > + .hw.init = &(struct clk_init_data) {
> >> > + .name = "cpu_fclk",
> >> > + .ops = &clk_regmap_mux_ops,
> >> > + .parent_hws = (const struct clk_hw *[]) {
> >> > + &cpu_fsel0.hw,
> >> > + &cpu_fsel1.hw,
> >> > + },
> >> > + .num_parents = 2,
> >> > + .flags = CLK_SET_RATE_PARENT,
> >> > + },
> >> > +};
> >> > +
> >> > +static struct clk_regmap cpu_clk = {
> >> > + .data = &(struct clk_regmap_mux_data) {
> >> > + .offset = CPUCTRL_CLK_CTRL0,
> >> > + .mask = 0x1,
> >> > + .shift = 11,
> >> > + },
> >> > + .hw.init = &(struct clk_init_data) {
> >> > + .name = "cpu_clk",
> >> > + .ops = &clk_regmap_mux_ops,
> >> > + .parent_data = (const struct clk_parent_data []) {
> >> > + { .hw = &cpu_fclk.hw },
> >> > + { .fw_name = "sys_pll", },
> >> > + },
> >> > + .num_parents = 2,
> >> > + .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
> >> > + },
> >> > +};
> >> > +
> >> > +/* Array of all clocks registered by this provider */
> >> > +static struct clk_hw *a1_cpu_hw_clks[] = {
> >> > + [CLKID_CPU_FSOURCE_SEL0] = &cpu_fsource_sel0.hw,
> >> > + [CLKID_CPU_FSOURCE_DIV0] = &cpu_fsource_div0.hw,
> >> > + [CLKID_CPU_FSEL0] = &cpu_fsel0.hw,
> >> > + [CLKID_CPU_FSOURCE_SEL1] = &cpu_fsource_sel1.hw,
> >> > + [CLKID_CPU_FSOURCE_DIV1] = &cpu_fsource_div1.hw,
> >> > + [CLKID_CPU_FSEL1] = &cpu_fsel1.hw,
> >> > + [CLKID_CPU_FCLK] = &cpu_fclk.hw,
> >> > + [CLKID_CPU_CLK] = &cpu_clk.hw,
> >> > +};
> >> > +
> >> > +static struct clk_regmap *const a1_cpu_regmaps[] = {
> >> > + &cpu_fsource_sel0,
> >> > + &cpu_fsource_div0,
> >> > + &cpu_fsel0,
> >> > + &cpu_fsource_sel1,
> >> > + &cpu_fsource_div1,
> >> > + &cpu_fsel1,
> >> > + &cpu_fclk,
> >> > + &cpu_clk,
> >> > +};
> >> > +
> >> > +static struct regmap_config a1_cpu_regmap_cfg = {
> >> > + .reg_bits = 32,
> >> > + .val_bits = 32,
> >> > + .reg_stride = 4,
> >> > + .max_register = CPUCTRL_CLK_CTRL1,
> >> > +};
> >> > +
> >> > +static struct meson_clk_hw_data a1_cpu_clks = {
> >> > + .hws = a1_cpu_hw_clks,
> >> > + .num = ARRAY_SIZE(a1_cpu_hw_clks),
> >> > +};
> >> > +
> >> > +struct a1_cpu_clk_nb_data {
> >> > + const struct clk_ops *mux_ops;
> >>
> >> That's fishy ...
> >>
> >> > + struct clk_hw *cpu_clk;
> >> > + struct notifier_block nb;
> >> > + u8 parent;
> >> > +};
> >> > +
> >> > +#define MESON_A1_CPU_CLK_GET_PARENT(nbd) \
> >> > + ((nbd)->mux_ops->get_parent((nbd)->cpu_clk))
> >> > +#define MESON_A1_CPU_CLK_SET_PARENT(nbd, index) \
> >> > + ((nbd)->mux_ops->set_parent((nbd)->cpu_clk, index))
> >>
> >> ... Directly going for the mux ops ??!?? No way !
> >>
> >> We have a framework to handle the clocks, the whole point is to use it,
> >> not bypass it !
> >>
> >
> > I suppose you understand my approach, which is quite similar to what is
> > happening in the Mediatek driver:
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/mediatek/clk-mux.c#L295
> >
> > Initially, I attempted to set the parent using the clk_set_parent() API.
> > However, I encountered a problem with recursive calling of the
> > notifier_block. This issue arises because the parent triggers
> > notifications for its children, leading to repeated calls to the
> > notifier_block.
> >
> > I find it puzzling why I cannot call an internal function or callback
> > within the internal driver context. After all, the notifier block is
> > just a part of the set_rate() flow. From a global Clock Control
> > Framework perspective, the context should not change.
>
> I don't care what MTK is doing TBH. Forcefully calling ops on a clock,
> hoping they are going to match with the clock type is wrong.
>
> There is a clk_hw API. Please it.
>
Yes, if sys_pll has a notifier_block instead of cpu_clk, there will be
no problem with the clk_hw API.
I will rework that point.
> >
> >> > +
> >> > +static int meson_a1_cpu_clk_notifier_cb(struct notifier_block *nb,
> >> > + unsigned long event, void *data)
> >> > +{
> >> > + struct a1_cpu_clk_nb_data *nbd;
> >> > + int ret = 0;
> >> > +
> >> > + nbd = container_of(nb, struct a1_cpu_clk_nb_data, nb);
> >> > +
> >> > + switch (event) {
> >> > + case PRE_RATE_CHANGE:
> >> > + nbd->parent = MESON_A1_CPU_CLK_GET_PARENT(nbd);
> >> > + /* Fallback to the CPU fixed clock */
> >> > + ret = MESON_A1_CPU_CLK_SET_PARENT(nbd, 0);
> >> > + /* Wait for clock propagation */
> >> > + udelay(100);
> >> > + break;
> >> > +
> >> > + case POST_RATE_CHANGE:
> >> > + case ABORT_RATE_CHANGE:
> >> > + /* Back to the original parent clock */
> >> > + ret = MESON_A1_CPU_CLK_SET_PARENT(nbd, nbd->parent);
> >> > + /* Wait for clock propagation */
> >> > + udelay(100);
> >> > + break;
> >> > +
> >> > + default:
> >> > + pr_warn("Unknown event %lu for %s notifier block\n",
> >> > + event, clk_hw_get_name(nbd->cpu_clk));
> >> > + break;
> >> > + }
> >> > +
> >> > + return notifier_from_errno(ret);
> >> > +}
> >> > +
> >> > +static struct a1_cpu_clk_nb_data a1_cpu_clk_nb_data = {
> >> > + .mux_ops = &clk_regmap_mux_ops,
> >> > + .cpu_clk = &cpu_clk.hw,
> >> > + .nb.notifier_call = meson_a1_cpu_clk_notifier_cb,
> >> > +};
> >> > +
> >> > +static int meson_a1_dvfs_setup(struct platform_device *pdev)
> >> > +{
> >> > + struct device *dev = &pdev->dev;
> >> > + struct clk *notifier_clk;
> >> > + int ret;
> >> > +
> >> > + /* Setup clock notifier for cpu_clk */
> >> > + notifier_clk = devm_clk_hw_get_clk(dev, &cpu_clk.hw, "dvfs");
> >> > + if (IS_ERR(notifier_clk))
> >> > + return dev_err_probe(dev, PTR_ERR(notifier_clk),
> >> > + "can't get cpu_clk as notifier clock\n");
> >> > +
> >> > + ret = devm_clk_notifier_register(dev, notifier_clk,
> >> > + &a1_cpu_clk_nb_data.nb);
> >> > + if (ret)
> >> > + return dev_err_probe(dev, ret,
> >> > + "can't register cpu_clk notifier\n");
> >> > +
> >> > + return ret;
> >> > +}
> >> > +
> >> > +static int meson_a1_cpu_probe(struct platform_device *pdev)
> >> > +{
> >> > + struct device *dev = &pdev->dev;
> >> > + void __iomem *base;
> >> > + struct regmap *map;
> >> > + int clkid, i, err;
> >> > +
> >> > + base = devm_platform_ioremap_resource(pdev, 0);
> >> > + if (IS_ERR(base))
> >> > + return dev_err_probe(dev, PTR_ERR(base),
> >> > + "can't ioremap resource\n");
> >> > +
> >> > + map = devm_regmap_init_mmio(dev, base, &a1_cpu_regmap_cfg);
> >> > + if (IS_ERR(map))
> >> > + return dev_err_probe(dev, PTR_ERR(map),
> >> > + "can't init regmap mmio region\n");
> >> > +
> >> > + /* Populate regmap for the regmap backed clocks */
> >> > + for (i = 0; i < ARRAY_SIZE(a1_cpu_regmaps); i++)
> >> > + a1_cpu_regmaps[i]->map = map;
> >> > +
> >> > + for (clkid = 0; clkid < a1_cpu_clks.num; clkid++) {
> >> > + err = devm_clk_hw_register(dev, a1_cpu_clks.hws[clkid]);
> >> > + if (err)
> >> > + return dev_err_probe(dev, err,
> >> > + "clock[%d] registration failed\n",
> >> > + clkid);
> >> > + }
> >> > +
> >> > + err = devm_of_clk_add_hw_provider(dev, meson_clk_hw_get, &a1_cpu_clks);
> >> > + if (err)
> >> > + return dev_err_probe(dev, err, "can't add clk hw provider\n");
> >>
> >> I wonder if there is a window of opportunity to poke the syspll without
> >> your notifier here. That being said, the situation would be similar on g12.
> >>
> >
> > Yes, I have taken into account what you did in the G12A CPU clock
> > relations. My thoughts were that it might not be applicable for the A1
> > case. This is because the sys_pll should be located in a different
> > driver from a logical perspective. Consequently, we cannot configure the
> > sys_pll notifier block to manage the cpu_clk from a different driver.
> > However, if I were to move the sys_pll clock object to the A1 CPU clock
> > controller, I believe the g12a sys_pll notifier approach would work.
> >
>
> I fail to see the connection between the number of device and the
> approach you took.
>
>
> >> > +
> >> > + return meson_a1_dvfs_setup(pdev);
> >>
> >>
> >>
> >> > +}
> >> > +
> >> > +static const struct of_device_id a1_cpu_clkc_match_table[] = {
> >> > + { .compatible = "amlogic,a1-cpu-clkc", },
> >> > + {}
> >> > +};
> >> > +MODULE_DEVICE_TABLE(of, a1_cpu_clkc_match_table);
> >> > +
> >> > +static struct platform_driver a1_cpu_clkc_driver = {
> >> > + .probe = meson_a1_cpu_probe,
> >> > + .driver = {
> >> > + .name = "a1-cpu-clkc",
> >> > + .of_match_table = a1_cpu_clkc_match_table,
> >> > + },
> >> > +};
> >> > +
> >> > +module_platform_driver(a1_cpu_clkc_driver);
> >> > +MODULE_AUTHOR("Dmitry Rokosov <ddrokosov@salutedevices.com>");
> >> > +MODULE_LICENSE("GPL");
> >> > diff --git a/drivers/clk/meson/a1-cpu.h b/drivers/clk/meson/a1-cpu.h
> >> > new file mode 100644
> >> > index 000000000000..e9af4117e26f
> >> > --- /dev/null
> >> > +++ b/drivers/clk/meson/a1-cpu.h
> >>
> >> There is not point putting the definition here in a header
> >> These are clearly not going to be shared with another driver.
> >>
> >> Please drop this file
> >>
> >
> > The same approach was applied to the Peripherals and PLL A1 drivers.
> > Honestly, I am not a fan of having different file organization within a
> > single logical code folder.
> >
> > Please refer to:
> >
> > drivers/clk/meson/a1-peripherals.h
> > drivers/clk/meson/a1-pll.h
>
> I understand. There was a time where it made sense, some definition
> could have been shared back then. This is no longer the case and it
> would probably welcome a rework.
>
> ... and again, just pointing to another code does really invalidate my
> point.
>
> >
> >> > @@ -0,0 +1,16 @@
> >> > +/* SPDX-License-Identifier: GPL-2.0+ */
> >> > +/*
> >> > + * Amlogic A1 CPU Clock Controller internals
> >> > + *
> >> > + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
> >> > + * Author: Dmitry Rokosov <ddrokosov@salutedevices.com>
> >> > + */
> >> > +
> >> > +#ifndef __A1_CPU_H
> >> > +#define __A1_CPU_H
> >> > +
> >> > +/* cpu clock controller register offset */
> >> > +#define CPUCTRL_CLK_CTRL0 0x80
> >> > +#define CPUCTRL_CLK_CTRL1 0x84
> >>
> >> You are claiming the registers from 0x00 to 0x84 (included), but only
> >> using these 2 registers ? What is the rest ? Are you sure there is only
> >> clocks in there ?
> >>
> >
> > Yes, unfortunately, the register map for this IP is not described in the
> > A1 Datasheet. The only available information about it can be found in
> > the vendor clock driver, which provides details for only two registers
> > used to configure the CPU clock.
> >
> > From vendor kernel dtsi:
> >
> > clkc: clock-controller {
> > compatible = "amlogic,a1-clkc";
> > #clock-cells = <1>;
> > reg = <0x0 0xfe000800 0x0 0x100>,
> > <0x0 0xfe007c00 0x0 0x21c>,
> > <0x0 0xfd000000 0x0 0x88>; <==== CPU clock regmap
> > reg-names = "basic", "pll",
> > "cpu_clk";
> > clocks = <&xtal>;
> > clock-names = "core";
> > status = "okay";
> > };
> >
> > From vendor clkc driver:
> >
> > /*
> > * CPU clok register offset
> > * APB_BASE: APB1_BASE_ADDR = 0xfd000000
> > */
> >
> > #define CPUCTRL_CLK_CTRL0 0x80
> > #define CPUCTRL_CLK_CTRL1 0x84
>
> If you had clock in 0x0 and 0x80, then I would agree claiming 0x0-0x88
> is reasonable, even if you don't really know what is in between. It
> would be a fair assumption.
>
> It is not the case here.
> For all we know it could resets, power domains, etc ...
>
However, we do not have any information indicating that the gap from
0x00-0x80 contains additional registers. It is possible that there are
internal registers, but they are not mentioned in the vendor datasheet
or driver. Therefore, in this case, I personally prefer to rely on the
vendor mapping.
--
Thank you,
Dmitry
^ permalink raw reply
* [PATCH 2/2] arm64: dts: ti: k3-am642-phyboard-electra-rdk: Increase CAN max bitrate
From: Nathan Morrisson @ 2024-04-02 16:08 UTC (permalink / raw)
To: nm, vigneshr, kristo, robh, krzysztof.kozlowski+dt, conor+dt
Cc: linux-arm-kernel, devicetree, linux-kernel, upstream, w.egorov
In-Reply-To: <20240402160825.1516036-1-nmorrisson@phytec.com>
The phyBOARD-Electra has two TCAN1044VDD CAN transceivers which
support CAN FD at 8 Mbps.
Increase the maximum bitrate to 8 Mbps.
Signed-off-by: Nathan Morrisson <nmorrisson@phytec.com>
---
arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts b/arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts
index 8237b8c815b8..522699ec65e8 100644
--- a/arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts
+++ b/arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts
@@ -42,7 +42,7 @@ can_tc1: can-phy0 {
pinctrl-names = "default";
pinctrl-0 = <&can_tc1_pins_default>;
#phy-cells = <0>;
- max-bitrate = <5000000>;
+ max-bitrate = <8000000>;
standby-gpios = <&main_gpio0 32 GPIO_ACTIVE_HIGH>;
};
@@ -51,7 +51,7 @@ can_tc2: can-phy1 {
pinctrl-names = "default";
pinctrl-0 = <&can_tc2_pins_default>;
#phy-cells = <0>;
- max-bitrate = <5000000>;
+ max-bitrate = <8000000>;
standby-gpios = <&main_gpio0 35 GPIO_ACTIVE_HIGH>;
};
--
2.25.1
^ permalink raw reply related
* [PATCH 0/2] Increase CAN max bitrate for phyCORE-AM62x and phyCORE-am64x
From: Nathan Morrisson @ 2024-04-02 16:08 UTC (permalink / raw)
To: nm, vigneshr, kristo, robh, krzysztof.kozlowski+dt, conor+dt
Cc: linux-arm-kernel, devicetree, linux-kernel, upstream, w.egorov
Nathan Morrisson (2):
arm64: dts: ti: k3-am625-phyboard-lyra-rdk: Increase CAN max bitrate
arm64: dts: ti: k3-am642-phyboard-electra-rdk: Increase CAN max
bitrate
arch/arm64/boot/dts/ti/k3-am625-phyboard-lyra-rdk.dts | 2 +-
arch/arm64/boot/dts/ti/k3-am642-phyboard-electra-rdk.dts | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
--
2.25.1
^ permalink raw reply
* [PATCH 1/2] arm64: dts: ti: k3-am625-phyboard-lyra-rdk: Increase CAN max bitrate
From: Nathan Morrisson @ 2024-04-02 16:08 UTC (permalink / raw)
To: nm, vigneshr, kristo, robh, krzysztof.kozlowski+dt, conor+dt
Cc: linux-arm-kernel, devicetree, linux-kernel, upstream, w.egorov
In-Reply-To: <20240402160825.1516036-1-nmorrisson@phytec.com>
The phyBOARD-Lyra has one TCAN1044VDD CAN transceiver which supports
CAN FD at 8 Mbps.
Increase the maximum bitrate to 8 Mbps.
Signed-off-by: Nathan Morrisson <nmorrisson@phytec.com>
---
arch/arm64/boot/dts/ti/k3-am625-phyboard-lyra-rdk.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/ti/k3-am625-phyboard-lyra-rdk.dts b/arch/arm64/boot/dts/ti/k3-am625-phyboard-lyra-rdk.dts
index a83a90497857..e225d76d02c8 100644
--- a/arch/arm64/boot/dts/ti/k3-am625-phyboard-lyra-rdk.dts
+++ b/arch/arm64/boot/dts/ti/k3-am625-phyboard-lyra-rdk.dts
@@ -31,7 +31,7 @@ aliases {
can_tc1: can-phy0 {
compatible = "ti,tcan1042";
#phy-cells = <0>;
- max-bitrate = <5000000>;
+ max-bitrate = <8000000>;
standby-gpios = <&gpio_exp 1 GPIO_ACTIVE_HIGH>;
};
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v2 1/2] dt-bindings: clock: qcom: Update SM8150 videocc bindings
From: Rob Herring @ 2024-04-02 16:05 UTC (permalink / raw)
To: Satya Priya Kakitapalli
Cc: Bjorn Andersson, Konrad Dybcio, Krzysztof Kozlowski, Conor Dooley,
Michael Turquette, Stephen Boyd, Dmitry Baryshkov, Ajit Pandey,
Imran Shaik, Taniya Das, Jagadeesh Kona, linux-arm-msm,
devicetree, linux-kernel, linux-clk
In-Reply-To: <20240401-videocc-sm8150-dt-node-v2-1-3b87cd2add96@quicinc.com>
On Mon, Apr 01, 2024 at 04:44:23PM +0530, Satya Priya Kakitapalli wrote:
> Update the clocks list for SM8150 to add both AHB and XO clocks,
> as it needs both of them.
I read this as you are adding 2 clocks, but it is really just 1 you are
adding (iface).
This should have more detail on why breaking the ABI is okay here.
>
> Fixes: 35d26e9292e2 ("dt-bindings: clock: Add YAML schemas for the QCOM VIDEOCC clock bindings")
> Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@quicinc.com>
> ---
> .../devicetree/bindings/clock/qcom,videocc.yaml | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/clock/qcom,videocc.yaml b/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
> index 6999e36ace1b..68bac801adb0 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
> @@ -75,7 +75,6 @@ allOf:
> enum:
> - qcom,sc7180-videocc
> - qcom,sdm845-videocc
> - - qcom,sm8150-videocc
> then:
> properties:
> clocks:
> @@ -101,6 +100,22 @@ allOf:
> - const: bi_tcxo
> - const: bi_tcxo_ao
>
> + - if:
> + properties:
> + compatible:
> + enum:
> + - qcom,sm8150-videocc
> + then:
> + properties:
> + clocks:
> + items:
> + - description: AHB
> + - description: Board XO source
> + clock-names:
> + items:
> + - const: iface
> + - const: bi_tcxo
> +
> - if:
> properties:
> compatible:
>
> --
> 2.25.1
>
^ permalink raw reply
* Re: [PATCH net-next 2/3] net: stmmac: add support for RZ/N1 GMAC
From: Romain Gantois @ 2024-04-02 16:04 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Romain Gantois, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm, Alexandre Torgue, Jose Abreu,
Maxime Coquelin, Clément Léger, Thomas Petazzoni,
netdev, devicetree, linux-kernel, linux-renesas-soc, linux-stm32,
linux-arm-kernel
In-Reply-To: <ZgwoygldsA1V8fs9@shell.armlinux.org.uk>
Hello Russell,
On Tue, 2 Apr 2024, Russell King (Oracle) wrote:
> > I'm afraid that this fails at one of the most basic principles of kernel
> > multi-threaded programming. stmmac_dvr_probe() as part of its work calls
> > register_netdev() which publishes to userspace the network device.
> >
> > Everything that is required must be setup _prior_ to publication to
> > userspace to avoid races, because as soon as the network device is
> > published, userspace can decide to bring that interface up. If one
> > hasn't finished the initialisation, the interface can be brought up
> > before that initialisation is complete.
...
>
> I'm not going to say that the two patches threaded to this email are
> "sane" but at least it avoids the problem. socfpga still has issues
> with initialisation done after register_netdev() though.
Thanks a lot for providing a fix to this issue, introducing new pcs_init/exit()
hooks seems like the best solution at this time, I'll make sure to integrate
those patches in the v2 for this series.
Thanks,
--
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH V5] PCI: Add support for preserving boot configuration
From: Rob Herring @ 2024-04-02 16:01 UTC (permalink / raw)
To: Vidya Sagar
Cc: lpieralisi, mmaddireddy, linux-kernel, will, jonathanh, kthota,
frowand.list, kw, linux-arm-kernel, lenb, devicetree, sagar.tv,
rafael, bhelgaas, linux-pci, treding, linux-acpi
In-Reply-To: <20240401075031.3337211-1-vidyas@nvidia.com>
On Mon, 01 Apr 2024 13:20:31 +0530, Vidya Sagar wrote:
> Add support for preserving the boot configuration done by the
> platform firmware per host bridge basis, based on the presence of
> 'linux,pci-probe-only' property in the respective PCI host bridge
> device-tree node. It also unifies the ACPI and DT based boot flows
> in this regard.
>
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> V5:
> * Addressed Rob's review comments
>
> V4:
> * Addressed Bjorn's review comments
>
> V3:
> * Unified ACPI and DT flows as part of addressing Bjorn's review comments
>
> V2:
> * Addressed issues reported by kernel test robot <lkp@intel.com>
>
> drivers/acpi/pci_root.c | 12 -----
> drivers/pci/controller/pci-host-common.c | 4 --
> drivers/pci/of.c | 57 +++++++++++++++++++-----
> drivers/pci/probe.c | 46 ++++++++++++++-----
> include/linux/of_pci.h | 6 +++
> 5 files changed, 88 insertions(+), 37 deletions(-)
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH 3/3] arm64: dts: msm8996: add fastrpc nodes
From: Dmitry Baryshkov @ 2024-04-02 15:58 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Bjorn Andersson, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Mathieu Poirier, Sibi Sankar, linux-arm-msm, devicetree,
linux-remoteproc, linux-kernel, Srinivas Kandagatla
In-Reply-To: <d9ba1e11-44ea-4c1f-ab33-56a8bf57ab63@linaro.org>
On Tue, 2 Apr 2024 at 17:47, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 31.03.2024 11:10 PM, Dmitry Baryshkov wrote:
> > From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> >
> > The ADSP provides fastrpc/compute capabilities. Enable support for the
> > fastrpc on this DSP.
> >
> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > arch/arm64/boot/dts/qcom/msm8996.dtsi | 57 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 57 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> > index 7ae499fa7d91..cf7ab01f3af6 100644
> > --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
> > @@ -3545,6 +3545,63 @@ q6routing: routing {
> > };
> > };
> > };
> > +
> > + fastrpc {
> > + compatible = "qcom,fastrpc";
> > + qcom,smd-channels = "fastrpcsmd-apps-dsp";
> > + label = "adsp";
> > + qcom,non-secure-domain;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + cb@8 {
> > + compatible = "qcom,fastrpc-compute-cb";
> > + reg = <8>;
> > + iommus = <&lpass_q6_smmu 8>;
> > + };
> > +
> > + cb@9 {
> > + compatible = "qcom,fastrpc-compute-cb";
> > + reg = <9>;
> > + iommus = <&lpass_q6_smmu 9>;
> > + };
> > +
> > + cb@10 {
> > + compatible = "qcom,fastrpc-compute-cb";
> > + reg = <10>;
> > + iommus = <&lpass_q6_smmu 10>;
> > + };
> > +
> > + cb@11 {
> > + compatible = "qcom,fastrpc-compute-cb";
> > + reg = <11>;
> > + iommus = <&lpass_q6_smmu 11>;
> > + };
> > +
> > + cb@12 {
> > + compatible = "qcom,fastrpc-compute-cb";
> > + reg = <12>;
> > + iommus = <&lpass_q6_smmu 12>;
> > + };
> > +
> > + cb@5 {
> > + compatible = "qcom,fastrpc-compute-cb";
> > + reg = <5>;
>
> No need to copy downstream's creative alphabetical-but-not-numerical
> sorting..
Ack, I'll fix the order.
> The entries look OK though.. although, any reason we have
> such a weird binding including faux child nodes and not just an array
> of iommus? Is the only way to discover the fastrpc nodes' properties
> such as qcom,non-secure-domain or vmid belonging through hardcoding?
No idea here. This is how fastrpc nodes are defined on all existing
platforms. Maybe Srini knows the story and the reason behind the
bindings??
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
From: Cristian Marussi @ 2024-04-02 15:58 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Peng Fan, Peng Fan (OSS), Sudeep Holla, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Linus Walleij, Dan Carpenter,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, Oleksii Moisieiev
In-Reply-To: <CAHp75VdAaTeQ_Ag3gd0s9UfT=kAT2hwibeJ9-YFXJx4z=R3e+g@mail.gmail.com>
On Tue, Apr 02, 2024 at 04:06:06PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 2, 2024 at 10:48 AM Cristian Marussi
> <cristian.marussi@arm.com> wrote:
> > On Sun, Mar 31, 2024 at 01:44:28PM +0000, Peng Fan wrote:
> > > > Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti:
>
> ...
>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/scmi_protocol.h>
> > > > > +#include <linux/slab.h>
> > > >
> > > > This is semi-random list of headers. Please, follow IWYU principle (include
> > > > what you use). There are a lot of inclusions I see missing (just in the context of
> > > > this page I see bits.h, types.h, and asm/byteorder.h).
> > >
> > > Is there any documentation about this requirement?
> > > Some headers are already included by others.
>
> The documentation here is called "a common sense".
> The C language is built like this and we expect that nobody will
> invest into the dependency hell that we have already, that's why IWYU
> principle, please follow it.
>
Yes, but given that we have a growing number of SCMI protocols there is a
common local protocols.h header to group all includes needed by any
protocols: the idea behind this (and the devm_ saga down below) was to ease
development of protocols, since there are lots of them and growing, given
the SCMI spec is extensible.
> > Andy made (mostly) the same remarks on this same patch ~1-year ago on
> > this same patch while it was posted by Oleksii.
> >
> > And I told that time that most of the remarks around devm_ usage were
> > wrong due to how the SCMI core handles protocol initialization (using a
> > devres group transparently).
> >
> > This is what I answered that time.
> >
> > https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t
> >
> > I wont repeat myself, but, in a nutshell the memory allocation like it
> > is now is fine: a bit happens via devm_ at protocol initialization, the
> > other is doe via explicit kmalloc at runtime and freed via kfree at
> > remove time (if needed...i.e. checking the present flag of some structs)
>
> This sounds like a mess. devm_ is expected to be used only for the
> ->probe() stage, otherwise you may consider cleanup.h (__free() macro)
> to have automatic free at the paths where memory is not needed.
>
Indeed, this protocol_init code is called by the SCMI core once for all when
an SCMI driver tries at first to use this specific protocol by 'getting' its
protocol_ops, so it is indeed called inside the probe chain of the driver:
at this point you *can* decide to use devres to allocate memory and be assured
that if the init fails, or when the driver cease to use this protocol (calling
its remove()) and no other driver is using it, all the stuff that have been
allocated related to this protocol will be released by the core for you.
(using an internal devres group)
Without this you should handle manually all the deallocation manually on
the init error-paths AND also provide all the cleanup explicitly when
the protocol is no more used by any driver (multiple users of the same
protocol instance are possible)...for all protocols.
This is/was handy since, till now, all the SCMI querying and resources
allocation happened anyway all at once at init time...
...the mess, as you kindly called it, derives from the fact that this specific
protocol is the first and only one that does NOT allocate all that it needs
during the initialization (to minimize needless allocs for a lot of possibly
unused resources) and this lazy-initialization phase, done after init at runtime,
must be handled manually since it cannot be managed by the devres group that is
open/clsoed around init by the SCMI core.
I dont like particularly this split allocation but it has a reason and any
other solution seems more messy to me at the moment.
And I dont feel like changing all the SCMI protocol initialziation core code
(that address a lot more under the hood) is a desirable solution to address a
non-existent problem really.
> And the function naming doesn't suggest that you have a probe-remove
> pair. Moreover, if the init-deinit part is called in the probe-remove,
> the devm_ must not be mixed with non-devm ones, as it breaks the order
> and leads to subtle mistakes.
>
Initialization order is enforced by SCMI core like this:
@driver_probe->get_protocol_ops()
@core/get_protocol_ops
-> devres_group_open()
-> protocol_init->devm_*()
-> devres_group_close()
-> driver_probing
@runtime optional explicit_lazy_kmallocs inside the protocol
@driver_remove->put_protocol_ops()
@core/put_protocol_ops()
-> protocol_denit->optional_explicit_kfree_of_the_above
-> devres_group_release()
-> driver_removing
... dont think there's an ordering problem.
...note that the ph->dev provided in the protocol_init and used by devm_
is NOT the dev of the SCMI driver probe/remove that uses the get_protocol_ops,
it is an internal SCMI device associated with the core SCMI stack probing and
allocations, within which a devres group for the specific protocol is created
when that specific protocol is initialized...protocols are not fully
fledged drivers are just bits of the SCMI stack that are initialized when needed
(and possibly also loaded when needed for vendor protocols) and
de-initialzed when no more SCMI driver users exist for that protocol.
Thanks,
Cristian
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox