* Re: [PATCH v2 4/5] phy: qcom: qmp-pcie: Add Gen5 8-lanes mode for Glymur
From: Dmitry Baryshkov @ 2026-04-01 14:08 UTC (permalink / raw)
To: Qiang Yu
Cc: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Philipp Zabel, Bjorn Andersson, Konrad Dybcio,
linux-arm-msm, linux-phy, devicetree, linux-kernel
In-Reply-To: <acua8Me0zo3v/CBi@hu-qianyu-lv.qualcomm.com>
On Tue, Mar 31, 2026 at 02:59:12AM -0700, Qiang Yu wrote:
> On Tue, Mar 24, 2026 at 11:23:19PM +0200, Dmitry Baryshkov wrote:
> > On Mon, Mar 23, 2026 at 12:15:31AM -0700, Qiang Yu wrote:
> > > The third PCIe controller on Glymur SoC supports 8-lane operation via
> > > bifurcation of two PHYs (each requires separate power domian, resets and
> > > aux clk).
> > >
> > > Add dedicated reset/no_csr reset list ("phy_b", "phy_b_nocsr") and
> > > clock ("phy_b_aux") required for 8-lane operation. Introduce new
> > > glymur_qmp_gen5x8_pciephy_cfg configuration to enable PCIe Gen5 x8 mode.
> > >
> > > Signed-off-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
> > > ---
> > > drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 30 +++++++++++++++++++++++++++++-
> > > 1 file changed, 29 insertions(+), 1 deletion(-)
> > >
> > > @@ -4705,6 +4713,23 @@ static const struct qmp_phy_cfg glymur_qmp_gen4x2_pciephy_cfg = {
> > > .phy_status = PHYSTATUS_4_20,
> > > };
> > >
> > > +static const struct qmp_phy_cfg glymur_qmp_gen5x8_pciephy_cfg = {
> > > + .lanes = 8,
> > > +
> > > + .offsets = &qmp_pcie_offsets_v8_50,
> > > +
> > > + .reset_list = glymur_pciephy_reset_l,
> > > + .num_resets = ARRAY_SIZE(glymur_pciephy_reset_l),
> > > + .nocsr_reset_list = glymur_pciephy_nocsr_reset_l,
> > > + .num_nocsr_resets = ARRAY_SIZE(glymur_pciephy_nocsr_reset_l),
> >
> > Just for my understanding. If it was not the NOCSR case and had to
> > program the registers, would we have needed to program anything in the
> > PCIe3B space?
>
> The PCIe3B PHY registers need to be programmed.
> But we don't need to do it explicitly because there are also broadcast
> registers: writing to these registers will automatically write the same
> offset and value to both PHY ports simultaneously.
THanks.
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>
> - Qiang Yu
> >
> > > + .vreg_list = qmp_phy_vreg_l,
> > > + .num_vregs = ARRAY_SIZE(qmp_phy_vreg_l),
> > > +
> > > + .regs = pciephy_v8_50_regs_layout,
> > > +
> > > + .phy_status = PHYSTATUS_4_20,
> > > +};
> > > +
> > > static void qmp_pcie_init_port_b(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tbls *tbls)
> > > {
> > > const struct qmp_phy_cfg *cfg = qmp->cfg;
> > > @@ -5483,6 +5508,9 @@ static const struct of_device_id qmp_pcie_of_match_table[] = {
> > > }, {
> > > .compatible = "qcom,glymur-qmp-gen5x4-pcie-phy",
> > > .data = &glymur_qmp_gen5x4_pciephy_cfg,
> > > + }, {
> > > + .compatible = "qcom,glymur-qmp-gen5x8-pcie-phy",
> > > + .data = &glymur_qmp_gen5x8_pciephy_cfg,
> > > }, {
> > > .compatible = "qcom,ipq6018-qmp-pcie-phy",
> > > .data = &ipq6018_pciephy_cfg,
> > >
> > > --
> > > 2.34.1
> > >
> >
> > --
> > With best wishes
> > Dmitry
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH 4/6] arm64: dts: qcom: kaanapali-mtp: Enable bluetooth and Wifi
From: Dmitry Baryshkov @ 2026-04-01 14:07 UTC (permalink / raw)
To: Zijun Hu
Cc: Jingyi Wang, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, aiqun.yu, tingwei.zhang,
trilok.soni, yijie.yang, linux-arm-msm, devicetree, linux-kernel,
20260224-knp-dts-misc-v6-0-79d20dab8a60
In-Reply-To: <4a3887f7-9445-4d46-b250-5fb160c9795b@oss.qualcomm.com>
On Wed, Apr 01, 2026 at 09:34:16PM +0800, Zijun Hu wrote:
> On 4/1/2026 7:08 PM, Dmitry Baryshkov wrote:
> >>>>> - Is the pin wired in the hardware?
> >>>> pin SW_CTRL is wired in hardware.
> >>> Granted your three answers, it can and should be described in the DT.
> >>>
> >>>> i have below confusions about 'swctrl-gpios' of 'qcom,wcn7850-pmu'
> >>>> which WCN7850 pin is 'swctrl-gpios' mean for ?
> >>>> Why to introduce 'swctrl-gpios' ?
> >>>> what problem does it solve ?
> >>>> how to solve the problem ?
> >>> Please descibe the hardware in the DT. Problem solving belongs to the
> >>> driver.
> >> sorry for not agreeing with your points here.
> >>
> >> it is better to correct or remove 'swctrl-gpios' within DT binding spec at least
> >> for 'qcom,wcn7850-pmu' with below reasons:
> >>
> >> 1) provided that 'swctrl-gpios' is for pin SW_CTRL of datasheet, binding spec's
> >> both description and its expected usage are wrong.
> > Please correct it.
> >
> >> 2) its driver does not parse and use the property 'swctrl-gpios', moreover, the
> >> property have no user within upstream DT tree.
> > There is no "driver" in the "DT bindings"
> >
>
> 'its driver' i mean here is the driver which drives the device which is generated
> by this DT node 'qcom,wcn7850-pmu'.
> source code of the driver is drivers/power/sequencing/pwrseq-qcom-wcn.c
DT describes the hardware. The driver behaviour is not that relevant
here.
>
> >> 3) the property is not mandatory based on binding spec.
> > Which is expected, because on some platforms it might be not wired up
> > and on the other platforms the pin to which it is wired to might be
> > unknown (think about all the phones for which the community doesn't have
> > schematics).
> >
>
> got your points and will explain mine at below 2) together.
>
> >> 4) upstream DT tree have had many such usages as mine which just set default pin
> >> configuration and not specify 'swctrl-gpios' explicitly.
> > I don't understand this part.
> >
>
> For DT node 'qcom,wcn7850-pmu' of products identified by the following dts file at least:
>
> wcn7850-pmu {
> compatible = "qcom,wcn7850-pmu";
>
> pinctrl-names = "default"; // config SW_CTRL pin default settings, but
> pinctrl-0 = ....; // this DT node does not specify property 'swctrl-gpios'.
> ....
> }
>
>
> grep -l -r "qcom,wcn7850-pmu" arch/arm64/boot/dts/qcom/ | xargs grep -l -r "sw[_-]ctrl"
> arch/arm64/boot/dts/qcom/sm8550-hdk.dts
> arch/arm64/boot/dts/qcom/sm8650-qrd.dts
> arch/arm64/boot/dts/qcom/sm8750-mtp.dts
> arch/arm64/boot/dts/qcom/sm8650-ayaneo-pocket-s2.dts
> arch/arm64/boot/dts/qcom/sm8550-qrd.dts
> arch/arm64/boot/dts/qcom/sm8650-hdk.dts
So, let's fix them too.
> >> 5) kaanapali-mtp is originally preinstalled with android OS which supports some
> >> qualcomm specific feature which have not been supported by up-stream kernel.
> >> so kaanapali-mtp H/W has some wired pins which is not used by up-stream
> >> kernel sometimes
> > Again, what does that have to do with the hardware description?
>
> kaanapali-mtp hardware supports the feature pin SW_CTRL involved, but we can decide
> not to enable the feature based on requirements.
>
> any advise about how to correct DTS to not enable the feature SW_CTRL involved ?
You can enable or disable something in the driver. It doesn't change the
way the chip is wired (that's what DT describes).
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v5 2/3] arm64: dts: rockchip: refactor items from Orange Pi 5/b to prep for Pro
From: Jonas Karlman @ 2026-04-01 14:05 UTC (permalink / raw)
To: dennis@ausil.us
Cc: FUKAUMI Naoki, Hsun Lai, Chaoyi Chen, John Clark,
Michael Opdenacker, Quentin Schulz, Andrew Lunn, Chukun Pan,
Alexey Charkov, Peter Robinson, Michael Riesch, Mykola Kvach,
Jimmy Hon, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner
In-Reply-To: <20260401010707.2584962-3-dennis@ausil.us>
Hi Dennis,
On 4/1/2026 3:07 AM, dennis@ausil.us wrote:
> From: Dennis Gilmore <dennis@ausil.us>
>
> The Orange Pi 5 Pro uses the same SoC and base as the Orange Pi 5 and
> Orange Pi 5B but has had sound, USB, and leds wired up differently. The
> boards also use gmac for ethernet where thre Pro has a PCIe attached NIC
>
> Signed-off-by: Dennis Gilmore <dennis@ausil.us>
> ---
> .../boot/dts/rockchip/rk3588s-orangepi-5.dts | 184 ++++++++++++++++
> .../boot/dts/rockchip/rk3588s-orangepi-5.dtsi | 202 ++----------------
> .../boot/dts/rockchip/rk3588s-orangepi-5b.dts | 181 ++++++++++++++++
> 3 files changed, 378 insertions(+), 189 deletions(-)
This patch seem to reintroduce a lot of duplication for the 5 and 5b
.dts-files. Please reduce the added duplication caused by this patch.
Maybe it is better to create a rk3588s-orangepi-5-base.dtsi or similar
where everything that is shared for all three boards is moved. Or the 5
an 5b specific parts are moved into a rk3588s-orangepi-5-5b.dtsi or
similar. Then the change in 5 and 5b board .dts-files are kept to a
minimum.
Regards,
Jonas
^ permalink raw reply
* Re: [PATCH 10/16] clk: Add support for clock nexus dt bindings
From: Brian Masney @ 2026-04-01 14:04 UTC (permalink / raw)
To: Miquel Raynal
Cc: Michael Turquette, Stephen Boyd, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Thomas Gleixner, Olivia Mackall, Herbert Xu,
Jayesh Choudhary, David S. Miller, Christian Marangi,
Antoine Tenart, Geert Uytterhoeven, Magnus Damm, Thomas Petazzoni,
Pascal EBERHARD, Wolfram Sang, linux-clk, devicetree,
linux-kernel, linux-crypto, linux-renesas-soc, Herve Codina
In-Reply-To: <87y0j76p8o.fsf@bootlin.com>
Hi Miquel,
On Wed, Apr 01, 2026 at 10:47:51AM +0200, Miquel Raynal wrote:
> First, thanks for the whole review.
>
> On 30/03/2026 at 11:16:44 -04, Brian Masney <bmasney@redhat.com> wrote:
> >> - ret = of_parse_phandle_with_args(np, "clocks", "#clock-cells",
> >> - index, out_args);
> >> + ret = of_parse_phandle_with_args_map(np, "clocks", "clock",
> >> + index, out_args);
> >
> > Before I left my Reviewed-by, I should have double checked Sashiko. It
> > has several questions about this patch. The first is:
> >
> > Are there other places in the clock framework that need to transition to the
> > new map API to ensure assigned clocks work?
> >
> > For instance, assigned-clocks and assigned-clock-parents are parsed in
> > drivers/clk/clk-conf.c using of_parse_phandle_with_args(). If a device
> > specifies an assigned clock that routes through a nexus node, will it fail
> > to configure because the map is not traversed?
>
> The goal of the nexus node is to isolate what is behind. Are
> assigned-clocks et al. supposed to traverse a nexus node? I am tempted
> to say "no", but I'm open to discussing this ofc.
I agree that it's not needed as well, however I want to defer to
Stephen's expertise here. I mainly brought this up trying to help him
with reviews.
> > https://sashiko.dev/#/patchset/20260327-schneider-v7-0-rc1-crypto-v1-0-5e6ff7853994%40bootlin.com?patch=12563
>
> I have mixed feelings concerning Sashiko's feedback. I will go through
> that page nevertheless, there are interesting comments in there.
I have mixed feelings as well about the feedback from Sashiko. It finds
issues, however not all of the feedback has been helpful. On the whole,
I'm glad that it's available.
Brian
^ permalink raw reply
* Re: [PATCH v5 0/2] Add support for Texas Instruments INA4230 power monitor
From: Rob Herring @ 2026-04-01 14:03 UTC (permalink / raw)
To: Alexey Charkov
Cc: Guenter Roeck, Krzysztof Kozlowski, Conor Dooley, linux-hwmon,
devicetree, linux-kernel, Krzysztof Kozlowski
In-Reply-To: <CAKTNdwGcXcE25QiBTrZO6akMad+Lny5iPvAAAmUt6x2Hyzu5wg@mail.gmail.com>
On Tue, Mar 31, 2026 at 11:46 AM Alexey Charkov <alchark@flipper.net> wrote:
>
> On Tue, Mar 31, 2026 at 8:10 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On 3/31/26 08:52, Rob Herring wrote:
> > > On Mon, Mar 30, 2026 at 09:07:32AM -0700, Guenter Roeck wrote:
> > >> On 3/30/26 08:14, Alexey Charkov wrote:
> > >>> TI INA4230 is a 4-channel power monitor with I2C interface, similar in
> > >>> operation to INA3221 (3-channel) and INA219 (single-channel) but with
> > >>> a different register layout, different alerting mechanism and slightly
> > >>> different support for directly reading calculated current/power/energy
> > >>> values (pre-multiplied by the device itself and needing only to be scaled
> > >>> by the driver depending on its selected LSB unit values).
> > >>>
> > >>> In this initial implementation, the driver supports reading voltage,
> > >>> current, power and energy values, but does not yet support alerts, which
> > >>> can be added separately if needed. Also the overflows during hardware
> > >>> calculations are not yet handled, nor is the support for the device's
> > >>> internal 32-bit energy counter reset.
> > >>>
> > >>> An example device tree using this binding and driver is available at [1]
> > >>> (not currently upstreamed, as the device in question is in engineering
> > >>> phase and not yet publicly available)
> > >>>
> > >>> [1] https://github.com/flipperdevices/flipper-linux-kernel/blob/flipper-devel/arch/arm64/boot/dts/rockchip/rk3576-flipper-one-rev-f0b0c1.dts
> > >>>
> > >>> Signed-off-by: Alexey Charkov <alchark@flipper.net>
> > >>> ---
> > >>> Changes in v5:
> > >>> - Reworded per-channel subnodes description in the binding for clarity (Sashiko)
> > >>> - NB: Sashiko's suggestion to allow interrupts in the binding sounds premature,
> > >>> as the alerts mechanism is not implemented yet and there are no known users
> > >>> to test it. If anyone has hardware with the alert pins wired to an interrupt
> > >>> line - please shout and we can test/extend it together
> > >>
> > >> The bindings are supposed to be complete, even if not implemented, so I am not sure
> > >> if the DT maintainers will agree here. We'll see.
> > >
> > > Given ti,alert-polarity-active-high is added seems like the interrupt
> > > should be too. And the interrupt can specify the polarity, so is that
> > > property really needed? There's alway the possibility that you have some
> > > inverter on the board too and the interrupt polarity is not enough, but
> > > solve that problem when it actually exists.
> > >
> >
> > The alert pin can be attached to a board interrupt, or (more likely) it can
> > be attached to the I2C controller's alert pin. In the latter case there is
> > no interrupt property.
>
> Alright, I will add the interrupt property and keep the dedicated flag
> for alert polarity.
>
> Following the logic of binding completeness, should I add a flag for
> the single-shot mode too, even though I dropped that functionality
> from the driver in one of the prior iterations?
I don't remember what that was exactly, but that sounds like a user
selection which would be some sysfs or other runtime control rather
than in DT. Unless the h/w design dictates what mode should be used.
Rob
^ permalink raw reply
* Re: [PATCH 06/16] clk: tests: Add clk_parse_clkspec() Kunit testing
From: Brian Masney @ 2026-04-01 13:55 UTC (permalink / raw)
To: Miquel Raynal
Cc: Michael Turquette, Stephen Boyd, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Thomas Gleixner, Olivia Mackall, Herbert Xu,
Jayesh Choudhary, David S. Miller, Christian Marangi,
Antoine Tenart, Geert Uytterhoeven, Magnus Damm, Thomas Petazzoni,
Pascal EBERHARD, Wolfram Sang, linux-clk, devicetree,
linux-kernel, linux-crypto, linux-renesas-soc, Chen-Yu Tsai
In-Reply-To: <87mrzn6opj.fsf@bootlin.com>
Hi Miquel,
On Wed, Apr 01, 2026 at 10:59:20AM +0200, Miquel Raynal wrote:
> >> + of_node_put(ctx->prov1_np);
> >> +
> >> + /* Register provider 2 */
> >> + hw2 = kunit_kzalloc(test, sizeof(*hw2), GFP_KERNEL);
> >> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hw2);
> >> + hw2->init = &clk_parse_clkspec_2_init_data;
> >> +
> >> + ctx->prov2_np = of_find_compatible_node(NULL, NULL, "test,clock-provider2");
> >> + KUNIT_ASSERT_NOT_NULL(test, ctx->prov2_np);
> >> +
> >> + KUNIT_ASSERT_EQ(test, 0, of_clk_hw_register_kunit(test, ctx->prov2_np, hw2));
> >> + of_clk_add_hw_provider(ctx->prov2_np, kunit_clk_get, hw2);
> >> + of_node_put(ctx->prov2_np);
> >> +
> >> + ctx->cons_np = of_find_compatible_node(NULL, NULL, "test,clock-consumer");
> >> + KUNIT_ASSERT_NOT_NULL(test, ctx->cons_np);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void clk_parse_clkspec_exit(struct kunit *test)
> >> +{
> >> + struct clk_parse_clkspec_ctx *ctx = test->priv;
> >> +
> >> + of_node_put(ctx->prov1_np);
> >> + of_node_put(ctx->prov2_np);
> >
> > Is there a double free of prov1_np and prov2_np? If this is dropped from
> > the test exit, then they should't need to be in the ctx struct.
>
> These two calls increment the refcount on the node:
> - of_find_compatible_node()
> - of_clk_add_hw_provider()
>
> However this makes me realize maybe I should call of_clk_del_provider()
> in the exit() function. In any case, I believe keeping a reference over
> the nodes during the test is correct and if there is an of_node_put()
> call to remove, it should be the on in the _init().
Take a look at drivers/clk/clk_kunit_helpers.c.
of_clk_add_hw_provider_kunit() will call of_clk_del_provider() for you
via of_clk_del_provider_wrapper.
Brian
^ permalink raw reply
* Re: [PATCH v3 7/7] dt-bindings: PCI: intel,lgm-pcie: Add atu resource
From: Florian Eckert @ 2026-04-01 13:49 UTC (permalink / raw)
To: Rob Herring
Cc: linux-pci, Eckert.Florian, Rahul Tanwar, linux-kernel,
Lorenzo Pieralisi, Krzysztof Kozlowski, Sajid Dalvi,
Bjorn Helgaas, Manivannan Sadhasivam, Ajay Agarwal, devicetree,
Johan Hovold, Conor Dooley, ms, Krzysztof Wilczyński
In-Reply-To: <CAL_JsqL2JW6zecCJGhFiJYraSGRNJGRsZ+oWJ+=4JWY5GD9SPQ@mail.gmail.com>
On 2026-04-01 15:07, Rob Herring wrote:
> On Wed, Apr 1, 2026 at 5:37 AM Rob Herring (Arm) <robh@kernel.org>
> wrote:
>>
>>
>> On Wed, 01 Apr 2026 11:31:43 +0200, Florian Eckert wrote:
>> > The 'atu' information is already set in the dwc core, if it is specified
>> > in the devicetree. The driver uses its own default, if not set in the
>> > devicetree. This information is hardware specific and should therefore be
>> > maintained in the devicetree rather than in the source.
>> >
>> > To be backward compatibile, this field is not mandatory. If 'atu'
>> > resource is not specified in the devicetree, the driver’s default value
>> > is used.
>> >
>> > Old DTS entry for PCIe:
>> >
>> > reg = <0xd1000000 0x1000>,
>> > <0xd3000000 0x20000>,
>> > <0xd0c41000.0x1000>;
>> > reg-names = "dbi", "config", "app";
>> >
>> > New DTS entry for PCIe:
>> >
>> > reg = <0xd1000000 0x1000>,
>> > <0xd10c0000 0x1000>,
>> > <0xd3000000 0x20000>,
>> > <0xd0c41000.0x1000>;
>> > reg-names = "dbi", "atu", "config", "app";
>
> This is also wrong. But the diff of the example shows the old vs. new,
> so there's really no reason for any of this in the commit msg.
Got it. Thanks for pointing that out. I’ve already removed it for the
next cycle for the v4.
--
Florian
>> >
>> > Signed-off-by: Florian Eckert <fe@dev.tdt.de>
>> > ---
>> > Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml | 6 +++++-
>> > 1 file changed, 5 insertions(+), 1 deletion(-)
>> >
>>
>> My bot found errors running 'make dt_binding_check' on your patch:
>>
>> yamllint warnings/errors:
>>
>> dtschema/dtc warnings/errors:
>> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/intel-gw-pcie.example.dtb:
>> pcie@d0e00000 (intel,lgm-pcie): reg-names:1: 'config' was expected
>> from schema $id:
>> http://devicetree.org/schemas/pci/intel-gw-pcie.yaml
>> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/intel-gw-pcie.example.dtb:
>> pcie@d0e00000 (intel,lgm-pcie): reg-names:2: 'app' was expected
>> from schema $id:
>> http://devicetree.org/schemas/pci/intel-gw-pcie.yaml
>> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/intel-gw-pcie.example.dtb:
>> pcie@d0e00000 (intel,lgm-pcie): reg-names:3: 'atu' was expected
>> from schema $id:
>> http://devicetree.org/schemas/pci/intel-gw-pcie.yaml
>>
>> doc reference errors (make refcheckdocs):
>>
>> See
>> https://patchwork.kernel.org/project/devicetree/patch/20260401-pcie-intel-gw-v3-7-63b008c5b7b2@dev.tdt.de
>>
>> The base for the series is generally the latest rc1. A different
>> dependency
>> should be noted in *this* patch.
>>
>> If you already ran 'make dt_binding_check' and didn't see the above
>> error(s), then make sure 'yamllint' is installed and dt-schema is up
>> to
>> date:
>>
>> pip3 install dtschema --upgrade
>>
>> Please check and re-submit after running the above command yourself.
>> Note
>> that DT_SCHEMA_FILES can be set to your schema file to speed up
>> checking
>> your schema. However, it must be unset to test all examples with your
>> schema.
>>
>>
^ permalink raw reply
* Re: [PATCH v4 3/5] mfd: aaeon: Add SRG-IMX8P MCU driver
From: Thomas Perrot @ 2026-04-01 13:48 UTC (permalink / raw)
To: Lee Jones
Cc: thomas.perrot, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, Bartosz Golaszewski, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam,
Jérémie Dautheribes, Wim Van Sebroeck, Guenter Roeck,
devicetree, linux-kernel, linux-gpio, imx, linux-arm-kernel,
linux-watchdog, Thomas Petazzoni, Miquel Raynal
In-Reply-To: <20260331130848.GG3795166@google.com>
[-- Attachment #1: Type: text/plain, Size: 11931 bytes --]
Hello Lee,
Thank you for the review. Please find answers to your questions inline,
and the remaining items will be addressed in v5.
On Tue, 2026-03-31 at 14:08 +0100, Lee Jones wrote:
> On Tue, 24 Mar 2026, Thomas Perrot (Schneider Electric) wrote:
>
> > Add Multi-Function Device (MFD) driver for the Aaeon SRG-IMX8P
> > embedded controller. This driver provides the core I2C
> > communication
> > interface and registers child devices (GPIO and watchdog
> > controllers).
> >
> > The driver implements a custom regmap bus over I2C to match the
> > MCU's
> > fixed 3-byte command format [opcode, arg, value]. Register
> > addresses
> > are encoded as 16-bit values (opcode << 8 | arg) using the
> > AAEON_MCU_REG() macro defined in the shared header. The regmap
> > instance is shared with child drivers via dev_get_regmap().
> > Concurrent
> > I2C accesses from child drivers are serialized by regmap's built-in
> > locking.
> >
> > Co-developed-by: Jérémie Dautheribes (Schneider Electric)
> > <jeremie.dautheribes@bootlin.com>
> > Signed-off-by: Jérémie Dautheribes (Schneider Electric)
> > <jeremie.dautheribes@bootlin.com>
> > Signed-off-by: Thomas Perrot (Schneider Electric)
> > <thomas.perrot@bootlin.com>
> > ---
> > MAINTAINERS | 2 +
> > drivers/mfd/Kconfig | 10 +++
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/aaeon-mcu.c | 155
> > ++++++++++++++++++++++++++++++++++++++++++
> > include/linux/mfd/aaeon-mcu.h | 20 ++++++
> > 5 files changed, 188 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index
> > ea9d55f76f3509c7f6ba6d1bc86ca2e2e71aa954..f91b6a1826d04bef8a0f88221
> > f6c8e8a3652cd77 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -191,6 +191,8 @@ M: Thomas Perrot <thomas.perrot@bootlin.com>
> > R: Jérémie Dautheribes <jeremie.dautheribes@bootlin.com>
> > S: Maintained
> > F: Documentation/devicetree/bindings/mfd/aaeon,srg-imx8p-
> > mcu.yaml
> > +F: drivers/mfd/aaeon-mcu.c
> > +F: include/linux/mfd/aaeon-mcu.h
> >
> > AAEON UPBOARD FPGA MFD DRIVER
> > M: Thomas Richard <thomas.richard@bootlin.com>
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index
> > aace5766b38aa5e46e32a8a7b42eea238159fbcf..7a1ceedece899faad7a03a1fe
> > 7b1c91b72253c05 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1574,6 +1574,16 @@ config AB8500_CORE
> > the irq_chip parts for handling the Mixed Signal chip
> > events.
> > This chip embeds various other multimedia
> > functionalities as well.
> >
> > +config MFD_AAEON_MCU
> > + tristate "Aaeon SRG-IMX8P MCU Driver"
> > + depends on I2C || COMPILE_TEST
> > + select MFD_CORE
> > + help
> > + Select this option to enable support for the Aaeon SRG-
> > IMX8P
> > + onboard microcontroller (MCU). This driver provides the
> > core
> > + functionality to communicate with the MCU over I2C. The
> > MCU
> > + provides GPIO and watchdog functionality.
> > +
> > config MFD_DB8500_PRCMU
> > bool "ST-Ericsson DB8500 Power Reset Control Management
> > Unit"
> > depends on UX500_SOC_DB8500
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index
> > e75e8045c28afae975ac61d282b3b85af5440119..34db5b033584368b7a269b1ee
> > f12528a74baf8f5 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -8,6 +8,7 @@ obj-$(CONFIG_MFD_88PM860X) += 88pm860x.o
> > obj-$(CONFIG_MFD_88PM800) += 88pm800.o 88pm80x.o
> > obj-$(CONFIG_MFD_88PM805) += 88pm805.o 88pm80x.o
> > obj-$(CONFIG_MFD_88PM886_PMIC) += 88pm886.o
> > +obj-$(CONFIG_MFD_AAEON_MCU) += aaeon-mcu.o
> > obj-$(CONFIG_MFD_ACT8945A) += act8945a.o
> > obj-$(CONFIG_MFD_SM501) += sm501.o
> > obj-$(CONFIG_ARCH_BCM2835) += bcm2835-pm.o
> > diff --git a/drivers/mfd/aaeon-mcu.c b/drivers/mfd/aaeon-mcu.c
> > new file mode 100644
> > index
> > 0000000000000000000000000000000000000000..5a969890d201c027eb25c324b
> > 4d4d89b1f8c563e
> > --- /dev/null
> > +++ b/drivers/mfd/aaeon-mcu.c
> > @@ -0,0 +1,155 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Aaeon MCU driver
> > + *
> > + * Copyright (C) 2025 Bootlin
> > + * Author: Jérémie Dautheribes <jeremie.dautheribes@bootlin.com>
> > + * Author: Thomas Perrot <thomas.perrot@bootlin.com>
> > + */
>
> Consider updating the Copyright date - we're pretty deep into 2026 at
> this point.
>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +
> > +static const struct mfd_cell aaeon_mcu_devs[] = {
> > + {
> > + .name = "aaeon-mcu-wdt",
> > + },
> > + {
> > + .name = "aaeon-mcu-gpio",
> > + },
> > +};
>
> MFD_CELL_BASIC()
>
> > +/*
> > + * Custom regmap bus for the Aaeon MCU I2C protocol.
> > + *
> > + * The MCU uses a fixed 3-byte command format [opcode, arg, value]
> > followed
> > + * by a 1-byte response. It requires a STOP condition between the
> > command
> > + * write and the response read, so two separate i2c_transfer()
> > calls are
> > + * issued. The regmap lock serialises concurrent accesses from
> > the GPIO
> > + * and watchdog child drivers.
> > + *
> > + * Register addresses are encoded as a 16-bit big-endian value
> > where the
> > + * high byte is the opcode and the low byte is the argument,
> > matching the
> > + * wire layout produced by regmap for reg_bits=16.
> > + */
> > +
> > +static int aaeon_mcu_regmap_write(void *context, const void *data,
> > size_t count)
> > +{
> > + struct i2c_client *client = context;
> > + /* data = [opcode, arg, value] as formatted by regmap */
> > + struct i2c_msg write_msg = {
> > + .addr = client->addr,
> > + .flags = 0,
> > + .buf = (u8 *)data,
> > + .len = count,
> > + };
> > + u8 rsp;
> > + /* The MCU always sends a response byte after each
> > command; discard it. */
> > + struct i2c_msg rsp_msg = {
>
> Assuming 'rsp' means response, let's just write that out in full.
>
> Readability wins over brevity every time.
>
> > + .addr = client->addr,
> > + .flags = I2C_M_RD,
> > + .buf = &rsp,
> > + .len = 1,
> > + };
> > + int ret;
>
> Since some I2C host controllers might use DMA, should we ensure that
> the
> 'rsp' buffer is allocated in DMA-safe memory rather than on the stack
> to
> prevent potential cache-line corruption?
>
> Also allocation of structs during in declaration statements is rough!
>
> And adding that u8 in the middle is just rubbing it in.
>
> > + ret = i2c_transfer(client->adapter, &write_msg, 1);
> > + if (ret < 0)
> > + return ret;
> > + if (ret != 1)
> > + return -EIO;
> > +
> > + ret = i2c_transfer(client->adapter, &rsp_msg, 1);
> > + if (ret < 0)
> > + return ret;
> > + if (ret != 1)
> > + return -EIO;
> > +
> > + return 0;
> > +}
> > +
> > +static int aaeon_mcu_regmap_read(void *context, const void
> > *reg_buf,
> > + size_t reg_size, void *val_buf,
> > size_t val_size)
> > +{
> > + struct i2c_client *client = context;
> > + /*
> > + * reg_buf holds the 2-byte big-endian register address
> > [opcode, arg].
> > + * Append a trailing 0x00 to form the full 3-byte MCU
> > command.
> > + */
> > + u8 cmd[3] = { ((u8 *)reg_buf)[0], ((u8 *)reg_buf)[1], 0x00
> > };
> > + struct i2c_msg write_msg = {
> > + .addr = client->addr,
> > + .flags = 0,
> > + .buf = cmd,
> > + .len = sizeof(cmd),
> > + };
> > + struct i2c_msg read_msg = {
> > + .addr = client->addr,
> > + .flags = I2C_M_RD,
> > + .buf = val_buf,
> > + .len = val_size,
> > + };
> > + int ret;
> > +
> > + ret = i2c_transfer(client->adapter, &write_msg, 1);
> > + if (ret < 0)
> > + return ret;
> > + if (ret != 1)
> > + return -EIO;
> > +
> > + ret = i2c_transfer(client->adapter, &read_msg, 1);
> > + if (ret < 0)
> > + return ret;
> > + if (ret != 1)
> > + return -EIO;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct regmap_bus aaeon_mcu_regmap_bus = {
> > + .write = aaeon_mcu_regmap_write,
> > + .read = aaeon_mcu_regmap_read,
> > +};
> > +
> > +static const struct regmap_config aaeon_mcu_regmap_config = {
> > + .reg_bits = 16,
> > + .val_bits = 8,
> > + .reg_format_endian = REGMAP_ENDIAN_BIG,
> > + .cache_type = REGCACHE_NONE,
>
> Are you sure? Why none?
The GPIO and watchdog states are managed entirely by the MCU firmware,
which makes the design safer because every access goes directly to the
hardware. I will look into adding a cache; otherwise I will add a
comment in v5.
>
> > +};
> > +
> > +static int aaeon_mcu_probe(struct i2c_client *client)
> > +{
> > + struct regmap *regmap;
> > +
> > + regmap = devm_regmap_init(&client->dev,
> > &aaeon_mcu_regmap_bus,
> > + client,
> > &aaeon_mcu_regmap_config);
> > + if (IS_ERR(regmap))
> > + return PTR_ERR(regmap);
>
> dev_err_probe()
>
> > +
> > + return devm_mfd_add_devices(&client->dev,
> > PLATFORM_DEVID_NONE,
> > + aaeon_mcu_devs,
> > ARRAY_SIZE(aaeon_mcu_devs),
> > + NULL, 0, NULL);
>
> Why PLATFORM_DEVID_NONE over AUTO here?
No strong reason, it was an oversight. Since multiple instances of this
MCU could theoretically be present, AUTO is the safer choice and avoids
potential ID collisions.
Fixed in v5.
>
> > +}
> > +
> > +static const struct of_device_id aaeon_mcu_of_match[] = {
> > + { .compatible = "aaeon,srg-imx8p-mcu" },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, aaeon_mcu_of_match);
> > +
> > +static struct i2c_driver aaeon_mcu_driver = {
> > + .driver = {
> > + .name = "aaeon_mcu",
> > + .of_match_table = aaeon_mcu_of_match,
> > + },
> > + .probe = aaeon_mcu_probe,
> > +};
> > +module_i2c_driver(aaeon_mcu_driver);
> > +
> > +MODULE_DESCRIPTION("Aaeon MCU Driver");
> > +MODULE_AUTHOR("Jérémie Dautheribes
> > <jeremie.dautheribes@bootlin.com>");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/aaeon-mcu.h
> > b/include/linux/mfd/aaeon-mcu.h
> > new file mode 100644
> > index
> > 0000000000000000000000000000000000000000..861003f6dfd20424c3785008b
> > d2cf89aaa1715b9
> > --- /dev/null
> > +++ b/include/linux/mfd/aaeon-mcu.h
> > @@ -0,0 +1,20 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Aaeon MCU driver definitions
> > + *
> > + * Copyright (C) 2025 Bootlin
> > + * Author: Jérémie Dautheribes <jeremie.dautheribes@bootlin.com>
> > + * Author: Thomas Perrot <thomas.perrot@bootlin.com>
> > + */
>
> As above.
>
> > +
> > +#ifndef __LINUX_MFD_AAEON_MCU_H
> > +#define __LINUX_MFD_AAEON_MCU_H
> > +
> > +/*
> > + * MCU register address: the high byte is the command opcode, the
> > low
> > + * byte is the argument. This matches the 3-byte wire format
> > + * [opcode, arg, value] used by the MCU I2C protocol.
> > + */
> > +#define AAEON_MCU_REG(op, arg) (((op) << 8) | (arg))
>
> Where else is this used?
It is used by both child drivers:
- drivers/gpio/gpio-aaeon-mcu.c
- drivers/watchdog/aaeon_mcu_wdt.c
This macro encodes the regmap register address from the opcode and
argument that form the first two bytes of the MCU's 3-byte wire
command, so keeping it in the shared header avoids duplicating that
encoding in each child.
Kind regards,
Thomas Perrot
>
> > +#endif /* __LINUX_MFD_AAEON_MCU_H */
> >
> > --
> > 2.53.0
> >
--
Thomas Perrot, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply
* Re: [PATCH v7 05/15] arm64: dts: qcom: sdm845-lg-common: Enable qups and their dma controllers
From: Dmitry Baryshkov @ 2026-04-01 13:40 UTC (permalink / raw)
To: Paul Sajna
Cc: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, David Heidelberg, linux-arm-msm, devicetree,
linux-kernel, phone-devel, ~postmarketos/upstreaming, Amir Dahan,
Christopher Brown
In-Reply-To: <20260331-judyln-dts-v7-5-87217b15fefb@postmarketos.org>
On Tue, Mar 31, 2026 at 08:15:10PM -0700, Paul Sajna wrote:
> Qualcomm serial communicators required for i2c, serial, and spi
>
> Signed-off-by: Paul Sajna <sajattack@postmarketos.org>
> ---
> arch/arm64/boot/dts/qcom/sdm845-lg-common.dtsi | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v7 13/15] arm64: dts: qcom: sdm845-lg-common: Add camera flash
From: Dmitry Baryshkov @ 2026-04-01 13:38 UTC (permalink / raw)
To: Paul Sajna
Cc: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, David Heidelberg, linux-arm-msm, devicetree,
linux-kernel, phone-devel, ~postmarketos/upstreaming, Amir Dahan,
Christopher Brown, Konrad Dybcio
In-Reply-To: <20260331-judyln-dts-v7-13-fbbc4b7cc557@postmarketos.org>
On Tue, Mar 31, 2026 at 08:22:18PM -0700, Paul Sajna wrote:
> Camera doesn't work yet (imx351), but we can use the flash as a flashlight.
>
> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Paul Sajna <sajattack@postmarketos.org>
> ---
> arch/arm64/boot/dts/qcom/sdm845-lg-common.dtsi | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v7 14/15] arm64: dts: qcom: sdm845-lg-common: Change ipa gsi-loader to 'self', add memory-region
From: Dmitry Baryshkov @ 2026-04-01 13:38 UTC (permalink / raw)
To: Paul Sajna
Cc: Bjorn Andersson, Konrad Dybcio, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, David Heidelberg, linux-arm-msm, devicetree,
linux-kernel, phone-devel, ~postmarketos/upstreaming, Amir Dahan,
Christopher Brown, Konrad Dybcio
In-Reply-To: <20260331-judyln-dts-v7-14-fbbc4b7cc557@postmarketos.org>
On Tue, Mar 31, 2026 at 08:22:48PM -0700, Paul Sajna wrote:
> The modem firmware for this device doesn't preload the IPA firmware
> and requires the OS handles that instead. Set qcom,gsi-loader = "self"
> to reflect that.
>
> Ensure the ipa uses the correct memory.
>
> ipa 1e40000.ipa: channel 4 limited to 256 TREs
> ipa 1e40000.ipa: IPA driver initialized
> ipa 1e40000.ipa: received modem starting event
> ipa 1e40000.ipa: received modem running event
>
> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Paul Sajna <sajattack@postmarketos.org>
> ---
> arch/arm64/boot/dts/qcom/sdm845-lg-common.dtsi | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH 4/6] arm64: dts: qcom: kaanapali-mtp: Enable bluetooth and Wifi
From: Zijun Hu @ 2026-04-01 13:34 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Jingyi Wang, Bjorn Andersson, Konrad Dybcio, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, aiqun.yu, tingwei.zhang,
trilok.soni, yijie.yang, linux-arm-msm, devicetree, linux-kernel,
20260224-knp-dts-misc-v6-0-79d20dab8a60
In-Reply-To: <vxps2mbj572en5yjickrfdeebdjmk33olxdw6qd7vpfsye4x2d@xrgbjahhgdrz>
On 4/1/2026 7:08 PM, Dmitry Baryshkov wrote:
>>>>> - Is the pin wired in the hardware?
>>>> pin SW_CTRL is wired in hardware.
>>> Granted your three answers, it can and should be described in the DT.
>>>
>>>> i have below confusions about 'swctrl-gpios' of 'qcom,wcn7850-pmu'
>>>> which WCN7850 pin is 'swctrl-gpios' mean for ?
>>>> Why to introduce 'swctrl-gpios' ?
>>>> what problem does it solve ?
>>>> how to solve the problem ?
>>> Please descibe the hardware in the DT. Problem solving belongs to the
>>> driver.
>> sorry for not agreeing with your points here.
>>
>> it is better to correct or remove 'swctrl-gpios' within DT binding spec at least
>> for 'qcom,wcn7850-pmu' with below reasons:
>>
>> 1) provided that 'swctrl-gpios' is for pin SW_CTRL of datasheet, binding spec's
>> both description and its expected usage are wrong.
> Please correct it.
>
>> 2) its driver does not parse and use the property 'swctrl-gpios', moreover, the
>> property have no user within upstream DT tree.
> There is no "driver" in the "DT bindings"
>
'its driver' i mean here is the driver which drives the device which is generated
by this DT node 'qcom,wcn7850-pmu'.
source code of the driver is drivers/power/sequencing/pwrseq-qcom-wcn.c
>> 3) the property is not mandatory based on binding spec.
> Which is expected, because on some platforms it might be not wired up
> and on the other platforms the pin to which it is wired to might be
> unknown (think about all the phones for which the community doesn't have
> schematics).
>
got your points and will explain mine at below 2) together.
>> 4) upstream DT tree have had many such usages as mine which just set default pin
>> configuration and not specify 'swctrl-gpios' explicitly.
> I don't understand this part.
>
For DT node 'qcom,wcn7850-pmu' of products identified by the following dts file at least:
wcn7850-pmu {
compatible = "qcom,wcn7850-pmu";
pinctrl-names = "default"; // config SW_CTRL pin default settings, but
pinctrl-0 = ....; // this DT node does not specify property 'swctrl-gpios'.
....
}
grep -l -r "qcom,wcn7850-pmu" arch/arm64/boot/dts/qcom/ | xargs grep -l -r "sw[_-]ctrl"
arch/arm64/boot/dts/qcom/sm8550-hdk.dts
arch/arm64/boot/dts/qcom/sm8650-qrd.dts
arch/arm64/boot/dts/qcom/sm8750-mtp.dts
arch/arm64/boot/dts/qcom/sm8650-ayaneo-pocket-s2.dts
arch/arm64/boot/dts/qcom/sm8550-qrd.dts
arch/arm64/boot/dts/qcom/sm8650-hdk.dts
>> 5) kaanapali-mtp is originally preinstalled with android OS which supports some
>> qualcomm specific feature which have not been supported by up-stream kernel.
>> so kaanapali-mtp H/W has some wired pins which is not used by up-stream
>> kernel sometimes
> Again, what does that have to do with the hardware description?
kaanapali-mtp hardware supports the feature pin SW_CTRL involved, but we can decide
not to enable the feature based on requirements.
any advise about how to correct DTS to not enable the feature SW_CTRL involved ?
^ permalink raw reply
* Re: [PATCH v5 3/3] arm64: dts: rockchip: Add Orange Pi 5 Pro board support
From: Dennis Gilmore @ 2026-04-01 13:29 UTC (permalink / raw)
To: Andrew Lunn
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner,
FUKAUMI Naoki, Hsun Lai, Jonas Karlman, Chaoyi Chen, John Clark,
Michael Opdenacker, Quentin Schulz, Chukun Pan, Alexey Charkov,
Peter Robinson, Michael Riesch, Mykola Kvach, Jimmy Hon,
devicetree, linux-arm-kernel, linux-rockchip, linux-kernel
In-Reply-To: <7c4791e9-6621-4afb-a166-55211b18fb08@lunn.ch>
Hi Andrew,
On Wed, Apr 1, 2026 at 6:56 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Mar 31, 2026 at 08:07:07PM -0500, dennis@ausil.us wrote:
> > From: Dennis Gilmore <dennis@ausil.us>
> >
> > Add device tree for the Xunlong Orange Pi 5 Pro (RK3588S).
> >
> > - eMMC module, you can optionally solder a SPI NOR in place and turn
> > off the eMMC
> > - PCIe-attached NIC (pcie2x1l1)
> > - PCIe NVMe slot (pcie2x1l2)
> > - AP6256 WiFi (BCM43456) via SDIO with mmc-pwrseq
> > - BCM4345C5 Bluetooth
> > - es8388 audio
> > - USB 2.0 and USB 3.0
>
> You said in patch 0/X that the Ethernet driver had just been
> accepted. So i was expecting a node for it here?
>
> Andrew
On the Orange Pi 5 Pro, Ethernet is connected to PCIe; there is no
need to define anything
Dennis
^ permalink raw reply
* Re: [PATCH v5 2/3] arm64: dts: rockchip: refactor items from Orange Pi 5/b to prep for Pro
From: Dennis Gilmore @ 2026-04-01 13:27 UTC (permalink / raw)
To: Andrew Lunn
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner,
FUKAUMI Naoki, Hsun Lai, Jonas Karlman, Chaoyi Chen, John Clark,
Michael Opdenacker, Quentin Schulz, Chukun Pan, Alexey Charkov,
Peter Robinson, Michael Riesch, Mykola Kvach, Jimmy Hon,
devicetree, linux-arm-kernel, linux-rockchip, linux-kernel
In-Reply-To: <b733883d-e515-4946-a81f-d1a595985e01@lunn.ch>
Hi Andrew,
On Wed, Apr 1, 2026 at 6:52 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > +&gmac1 {
> > + clock_in_out = "output";
> > + phy-handle = <&rgmii_phy1>;
> > + phy-mode = "rgmii-rxid";
> > + pinctrl-0 = <&gmac1_miim
> > + &gmac1_tx_bus2
> > + &gmac1_rx_bus2
> > + &gmac1_rgmii_clk
> > + &gmac1_rgmii_bus>;
> > + pinctrl-names = "default";
> > + tx_delay = <0x42>;
>
> phy-mode = "rgmii-rxid" means the PCB provides the 2ns delay for
> TX. This is unlikely to be correct. Please try "rgmii-id" and delete
> the tx_delay.
As I mentioned to you in v2 I do not have the affected hardware to
test any changes. This patch is just moving the existing definition
from the common dtsi to the two devices that have gmac1 wired up. I
am not comfortable making changes here that I can not verify if they
work or not.
Dennis
> https://elixir.bootlin.com/linux/v6.15/source/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L287
>
> Andrew
>
> ---
> pw-bot: cr
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: pinctrl: describe Hawi TLMM
From: Krzysztof Kozlowski @ 2026-04-01 13:23 UTC (permalink / raw)
To: Mukesh Ojha, Bjorn Andersson, Linus Walleij, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-arm-msm, linux-gpio, devicetree, linux-kernel
In-Reply-To: <20260401-hawi-pinctrl-v1-1-4718da24e531@oss.qualcomm.com>
On 01/04/2026 13:52, Mukesh Ojha wrote:
> The Top Level Mode Multiplexer (TLMM) in the Hawi SoC provide GPIO and
> pinctrl functionality for UFS, SDC and 226 GPIO pins.
>
Please write subjects not only from your Qualcomm point of view. No one
outside of Qualcomm cares and knows what is Hawi.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype
From: Rodrigo Alencar @ 2026-04-01 13:16 UTC (permalink / raw)
To: Petr Mladek, Rodrigo Alencar
Cc: Andy Shevchenko, rodrigo.alencar, linux-kernel, linux-iio,
devicetree, linux-doc, Jonathan Cameron, David Lechner,
Andy Shevchenko, Lars-Peter Clausen, Michael Hennerich,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet,
Andrew Morton, Steven Rostedt, Rasmus Villemoes,
Sergey Senozhatsky, Shuah Khan
In-Reply-To: <ac0N89sNYcKAJkAP@pathway.suse.cz>
On 26/04/01 02:22PM, Petr Mladek wrote:
> On Mon 2026-03-30 13:49:48, Rodrigo Alencar wrote:
> > On 26/03/27 03:17PM, Rodrigo Alencar wrote:
> > > On 26/03/27 12:21PM, Andy Shevchenko wrote:
> > > > On Fri, Mar 27, 2026 at 10:11:56AM +0000, Rodrigo Alencar wrote:
> > > > > On 26/03/27 11:17AM, Andy Shevchenko wrote:
> > > > > > On Fri, Mar 27, 2026 at 09:45:17AM +0100, Petr Mladek wrote:
> > > > > > > On Fri 2026-03-20 16:27:27, Rodrigo Alencar via B4 Relay wrote:
> >
> > ...
> >
> > > > > > Maybe we want to have kstrtof32() and kstrtof64() for these two cases?
> > > > > >
> > > > > > With that we will always consider the fraction part as 32- or 64-bit,
> > > > > > imply floor() on the fraction for the sake of simplicity and require
> > > > > > it to be NUL-terminated with possible trailing '\n'.
> > > > >
> > > > > I think this is a good idea, but calling it float or fixed point itself
> > > > > is a bit confusing as float often refers to the IEEE 754 standard and
> > > > > fixed point types is often expressed in Q-format.
> > > >
> > > > Yeah... I am lack of better naming.
> > >
> > > decimals is the name, but they are often represented as:
> > >
> > > DECIMAL = INT * 10^X + FRAC
> > >
> > > in a single 64-bit number, which would be fine for my end use case.
> > > However IIO decimal fixed point parsing is out there for quite some time a
> > > lot of drivers use that. The interface often relies on breaking parsed values
> > > into an integer array (for standard attributes int val and int val2 are expected).
> >
> > Thinking about this again and in IIO drivers we end up doing something like:
> >
> > val64 = (u64)val * MICRO + val2;
> >
> > so that drivers often work with scaled versions of the decimal value.
> > then, would it make sense to have a function that already outputs such value?
> > That would allow to have more freedom over the 64-bit split between integer
> > and fractional parts.
> > As a draft:
>
> My understanding is that you want to allow parsing frequencies
> in the range from microHz to GHz.
Correct, the ABI requires the values in Hz, and I would like to support
micro Hz resolution, so that 10 GHz can be represensted as:
10000000000.000000
> So, you might want to support input in simple float numbers
> with some precision, for example, 1.2GHz, 0.345Hz, ...
>
> By simple, I mean that there is no x10^3 or so.
>
> > static int _kstrtodec64(const char *s, unsigned int scale, u64 *res)
>
> I would personally change this to something like:
>
> static int _unit_float_ktstrtodec64(const char *s, unsigned int precision, u64 *res, char **unit)
I don't really need "unit" for my specific use case, IIUC this pattern is not
something to be handled by kstrto*(), because those function should requires NUL
termination. I am not sure why is that, but I like the idea of returning a
const char* pointer to the end of the conversion (that was the whole point of
having something like kstrntoull())
> It would allow to read float number in the the format XXXX.YYYYunit,
> for example 1.2Ghz
>
> , where:
>
> + _unit_ means that it might set @unit pointer which point to the unit
> string right after the number part.
as mentioned, the units is defined in the ABI, so this part is not really needed.
> + _float_ means that it will be able to read float numbers
its a decimal fixed precision, decimal point should not float.
>
> + @precisions parameter defines the number of digits accepted
> after the radix point. It is also used as multiplier for scaling
> the output number.
precision != scale, for this case we have a fixed precision of 64-bits.
while scale is passed as parameter.
Reference:
https://www.ibm.com/docs/ro/informix-servers/12.10.0?topic=types-decimalps-data
>
> + @res is pointer to the read number multiplied by the given
> @precision
>
> + @unit will be set to string after the number
>
> For example:
>
> + s="1.2GHz", precision=3 will result in *res=1200, *unit="GHz"
> + s="0.0100004", precision=3 will result in *res=10, *unit=""
> + s=1.234567GHz, precision=3 will result in *res=1235, *unit="GHz"
>
> Note that the result is rounded in the last example.
>
> The function might be used like simple_strtoull() in memparse(),
> see lib/cmdline.c. Which is able to read the given size in B
> and handle various units like kB, GB, ...
As dicussed above, scaling the value based on the units is not my use case.
>
> > {
> > u64 _res = 0, _frac = 0;
> > unsigned int rv;
> >
> > if (*s != '.') {
> > rv = _parse_integer(s, 10, &_res);
> > if (rv & KSTRTOX_OVERFLOW)
> > return -ERANGE;
> > if (rv == 0)
> > return -EINVAL;
> > s += rv;
> > }
> >
> > if (*s == '.') {
> > s++;
> > rv = _parse_integer_limit(s, 10, &_frac, scale);
> > if (rv & KSTRTOX_OVERFLOW)
> > return -ERANGE;
> > if (rv == 0)
> > return -EINVAL;
> > s += rv;
> > if (rv < scale)
> > _frac *= int_pow(10, scale - rv);
> > while (isdigit(*s)) /* truncate */
> > s++;
>
> We might/should use the first digit to round the _frac.
flooring should not be a problem if it is documented like that.
I suppose we cannot afford to carry over all roundings from
subsequent digits. If so, we would be parsing it all and use
div64 which I would like avoid.
> > }
> >
> > if (*s == '\n')
> > s++;
> > if (*s)
> > return -EINVAL;
>
> I would omit this. Instead I would set @unit pointer so that the
> caller might handle units defined after the number.
I understand that this is the whole point of creating a kstrto*()
function.
> > if (check_mul_overflow(_res, int_pow(10, scale), &_res) ||
> > check_add_overflow(_res, _frac, &_res))
> > return -ERANGE;
> >
> > *res = _res;
> > return 0;
> > }
>
> Otherwise, this approach looks sensible to me. IMHO, some generic
> API for reading numbers with misc units should be usable in more
> situations. And it would make the kernel interface more user
> friendly.
>
> Of course, we must not over-engineer it. But the above does not
> look much more complex than we already have.
I really appreciate your time looking into this, thanks.
--
Kind regards,
Rodrigo Alencar
^ permalink raw reply
* [PATCH 2/2] arm64: dts: rockchip: Replace deprecated snps,* props for NanoPi R5S
From: Diederik de Haas @ 2026-04-01 13:11 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner
Cc: Diederik de Haas, Arnd Bergmann, devicetree, linux-arm-kernel,
linux-rockchip, linux-kernel
In-Reply-To: <20260401131551.734456-1-diederik@cknow-tech.com>
The various snps,reset-* properties are deprecated, so convert them into
their replacements.
Signed-off-by: Diederik de Haas <diederik@cknow-tech.com>
---
arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts b/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts
index 90ce6f0e1dcf..92d044ec696b 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts
@@ -85,10 +85,6 @@ &gmac0_tx_bus2
&gmac0_rx_bus2
&gmac0_rgmii_clk
&gmac0_rgmii_bus>;
- snps,reset-gpio = <&gpio0 RK_PC5 GPIO_ACTIVE_LOW>;
- snps,reset-active-low;
- /* Reset time is 15ms, 50ms for rtl8211f */
- snps,reset-delays-us = <0 15000 50000>;
tx_delay = <0x3c>;
rx_delay = <0x2f>;
status = "okay";
@@ -100,6 +96,9 @@ rgmii_phy0: ethernet-phy@1 {
reg = <1>;
pinctrl-0 = <&gmac0_rstn_gpio0_c5_pin>;
pinctrl-names = "default";
+ reset-assert-us = <15000>;
+ reset-deassert-us = <50000>;
+ reset-gpios = <&gpio0 RK_PC5 GPIO_ACTIVE_LOW>;
};
};
--
2.53.0
^ permalink raw reply related
* [PATCH 1/2] arm64: dts: rockchip: Fix gmac0 reset pin for NanoPi R5S
From: Diederik de Haas @ 2026-04-01 13:11 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner
Cc: Diederik de Haas, Arnd Bergmann, devicetree, linux-arm-kernel,
linux-rockchip, linux-kernel
In-Reply-To: <20260401131551.734456-1-diederik@cknow-tech.com>
According to the NanoPi R5S 2204 schematic on page 6, GPIO0_C4 is for
GMAC0_INT/PMEB_GPIO0_C4, while GPIO0_C5 is for GMAC0_RSTn_GPIO0_C5.
While the 'reset-gpios' property was set correctly, the corresponding
pinctrl didn't match that.
Next to fixing the pinctrl definition, also change the node name and
phandle to match what is used in the schematic.
Fixes: c6629b9a6738 ("arm64: dts: rockchip: Add FriendlyElec Nanopi R5S")
Signed-off-by: Diederik de Haas <diederik@cknow-tech.com>
---
arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts b/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts
index 718d1a2da8e5..90ce6f0e1dcf 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts
@@ -98,7 +98,7 @@ &mdio0 {
rgmii_phy0: ethernet-phy@1 {
compatible = "ethernet-phy-ieee802.3-c22";
reg = <1>;
- pinctrl-0 = <ð_phy0_reset_pin>;
+ pinctrl-0 = <&gmac0_rstn_gpio0_c5_pin>;
pinctrl-names = "default";
};
};
@@ -132,8 +132,8 @@ &pcie3x2 {
&pinctrl {
gmac0 {
- eth_phy0_reset_pin: eth-phy0-reset-pin {
- rockchip,pins = <0 RK_PC4 RK_FUNC_GPIO &pcfg_pull_up>;
+ gmac0_rstn_gpio0_c5_pin: gmac0-rstn-gpio0-c5-pin {
+ rockchip,pins = <0 RK_PC5 RK_FUNC_GPIO &pcfg_pull_up>;
};
};
--
2.53.0
^ permalink raw reply related
* [PATCH 0/2] Improve gmac0 DT config for NanoPi R5S
From: Diederik de Haas @ 2026-04-01 13:11 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner
Cc: Diederik de Haas, Arnd Bergmann, devicetree, linux-arm-kernel,
linux-rockchip, linux-kernel
These 2 patches contain a fix for an incorrect pinctlr definition and
replaces several deprecated snps,reset* properties with their
non-deprecated replacements.
Diederik de Haas (2):
arm64: dts: rockchip: Fix gmac0 reset pin for NanoPi R5S
arm64: dts: rockchip: Replace deprecated snps,* props for NanoPi R5S
arch/arm64/boot/dts/rockchip/rk3568-nanopi-r5s.dts | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
--
2.53.0
^ permalink raw reply
* Re: [PATCH] dt-bindings: mailbox: qcom: Add IPCC support for Hawi Platform
From: Manivannan Sadhasivam @ 2026-04-01 13:10 UTC (permalink / raw)
To: Mukesh Ojha
Cc: Jassi Brar, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-arm-msm, linux-kernel, devicetree, Konrad Dybcio
In-Reply-To: <20260401125126.593254-1-mukesh.ojha@oss.qualcomm.com>
On Wed, Apr 01, 2026 at 06:21:26PM +0530, Mukesh Ojha wrote:
> Document the Inter-Processor Communication Controller on the Qualcomm
> Hawi Platform, which will be used to route interrupts across various
> subsystems found on the SoC.
>
> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
- Mani
> ---
> Documentation/devicetree/bindings/mailbox/qcom-ipcc.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/mailbox/qcom-ipcc.yaml b/Documentation/devicetree/bindings/mailbox/qcom-ipcc.yaml
> index 7c4d6170491d..7dbc3ac6c5c9 100644
> --- a/Documentation/devicetree/bindings/mailbox/qcom-ipcc.yaml
> +++ b/Documentation/devicetree/bindings/mailbox/qcom-ipcc.yaml
> @@ -25,6 +25,7 @@ properties:
> items:
> - enum:
> - qcom,glymur-ipcc
> + - qcom,hawi-ipcc
> - qcom,kaanapali-ipcc
> - qcom,milos-ipcc
> - qcom,qcs8300-ipcc
> --
> 2.53.0
>
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply
* Re: [PATCH v3 7/7] dt-bindings: PCI: intel,lgm-pcie: Add atu resource
From: Rob Herring @ 2026-04-01 13:07 UTC (permalink / raw)
To: Florian Eckert
Cc: linux-pci, Eckert.Florian, Rahul Tanwar, linux-kernel,
Lorenzo Pieralisi, Krzysztof Kozlowski, Sajid Dalvi,
Bjorn Helgaas, Manivannan Sadhasivam, Ajay Agarwal, devicetree,
Johan Hovold, Conor Dooley, ms, Krzysztof Wilczyński
In-Reply-To: <177503985369.3634917.6461713197883659054.robh@kernel.org>
On Wed, Apr 1, 2026 at 5:37 AM Rob Herring (Arm) <robh@kernel.org> wrote:
>
>
> On Wed, 01 Apr 2026 11:31:43 +0200, Florian Eckert wrote:
> > The 'atu' information is already set in the dwc core, if it is specified
> > in the devicetree. The driver uses its own default, if not set in the
> > devicetree. This information is hardware specific and should therefore be
> > maintained in the devicetree rather than in the source.
> >
> > To be backward compatibile, this field is not mandatory. If 'atu'
> > resource is not specified in the devicetree, the driver’s default value
> > is used.
> >
> > Old DTS entry for PCIe:
> >
> > reg = <0xd1000000 0x1000>,
> > <0xd3000000 0x20000>,
> > <0xd0c41000.0x1000>;
> > reg-names = "dbi", "config", "app";
> >
> > New DTS entry for PCIe:
> >
> > reg = <0xd1000000 0x1000>,
> > <0xd10c0000 0x1000>,
> > <0xd3000000 0x20000>,
> > <0xd0c41000.0x1000>;
> > reg-names = "dbi", "atu", "config", "app";
This is also wrong. But the diff of the example shows the old vs. new,
so there's really no reason for any of this in the commit msg.
> >
> > Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> > ---
> > Documentation/devicetree/bindings/pci/intel-gw-pcie.yaml | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
>
> My bot found errors running 'make dt_binding_check' on your patch:
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/intel-gw-pcie.example.dtb: pcie@d0e00000 (intel,lgm-pcie): reg-names:1: 'config' was expected
> from schema $id: http://devicetree.org/schemas/pci/intel-gw-pcie.yaml
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/intel-gw-pcie.example.dtb: pcie@d0e00000 (intel,lgm-pcie): reg-names:2: 'app' was expected
> from schema $id: http://devicetree.org/schemas/pci/intel-gw-pcie.yaml
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/intel-gw-pcie.example.dtb: pcie@d0e00000 (intel,lgm-pcie): reg-names:3: 'atu' was expected
> from schema $id: http://devicetree.org/schemas/pci/intel-gw-pcie.yaml
>
> doc reference errors (make refcheckdocs):
>
> See https://patchwork.kernel.org/project/devicetree/patch/20260401-pcie-intel-gw-v3-7-63b008c5b7b2@dev.tdt.de
>
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
>
>
^ permalink raw reply
* Re: [PATCH v4 0/2] phy: spacemit: Add USB2 PHY support for K3 SoC
From: Yixun Lan @ 2026-04-01 13:07 UTC (permalink / raw)
To: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Ze Huang
Cc: Junzhong Pan, linux-phy, devicetree, linux-riscv, spacemit,
linux-kernel, Krzysztof Kozlowski, Yao Zi
In-Reply-To: <20260326001443-GKB777612@kernel.org>
Hi Vinod Koul,
On 08:14 Thu 26 Mar , Yixun Lan wrote:
> Hi Vinod Koul,
>
> On 01:00 Thu 05 Mar , Yixun Lan wrote:
> > The series trys to add USB2 PHY support for SpacemiT K3 SoC, while
> > patch [1/2] implement a disconnect function which is needed during
> > next connection.
> >
> > No DTS part has been inclueded in this series, instead I plan to
> > submit them later while adding USB host support.
> >
> > I've collected all patches and pushed a complete review branch here[1],
> > for people who interested to test easily, which include DTS and necessary
> > changes, other patches may still need to improve, but sufficient for
> > verifying the functionality.
> >
> > Link: https://github.com/spacemit-com/linux/tree/WIP/k3/usb2 [1]
> >
>
> Just want to ping this, Can you queue it for v7.1? Thanks
>
Ping again, I'd like to see USB2.0 support in v7.1 for SpacemiT K3
SoC.. and I think this phy patch is the only dependency that waiting
for, the USB DWC[1], and hub[2] patch already merged
Link: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=c05cf9d274daf72dc7e433480cf2e0e888f6bd89 [1]
Link: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=00b4fe5be06aecd6426930de86b7cffc2330f4b8 [2]
--
Yixun Lan (dlan)
^ permalink raw reply
* [PATCH] dt-bindings: sram: Document qcom,hawi-imem compatible
From: Mukesh Ojha @ 2026-04-01 12:55 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: devicetree, linux-kernel, Mukesh Ojha, Konrad Dybcio
On Qualcomm Hawi platform, IMEM is a block of SRAM shared across
multiple IP blocks which can fall back to "mmio-sram". Document
its compatible.
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
---
Documentation/devicetree/bindings/sram/sram.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
index c451140962c8..9bf042f392b5 100644
--- a/Documentation/devicetree/bindings/sram/sram.yaml
+++ b/Documentation/devicetree/bindings/sram/sram.yaml
@@ -34,6 +34,7 @@ properties:
- nvidia,tegra186-sysram
- nvidia,tegra194-sysram
- nvidia,tegra234-sysram
+ - qcom,hawi-imem
- qcom,kaanapali-imem
- qcom,rpm-msg-ram
- rockchip,rk3288-pmu-sram
--
2.53.0
^ permalink raw reply related
* Re: [PATCH net-next v2 5/9] net: dsa: lan9645x: add bridge support
From: Jens Emil Schulz Ostergaard @ 2026-04-01 12:52 UTC (permalink / raw)
To: Jakub Kicinski
Cc: UNGLinuxDriver, andrew, olteanv, davem, edumazet, pabeni, horms,
robh, krzk+dt, conor+dt, woojung.huh, linux, Steen.Hegelund,
daniel.machon, linux-kernel, netdev, devicetree
In-Reply-To: <20260329195631.2789158-1-kuba@kernel.org>
On Sun, 2026-03-29 at 12:56 -0700, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: dsa: lan9645x: add bridge support
>
> This commit adds support for hardware offloading of the bridge in the
> lan9645x DSA driver. It enables a single bridge device to be used with
> the switch.
>
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > index a0908cbf89c7..599e589c4ec3 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> [ ... ]
> > @@ -145,6 +155,8 @@ static int lan9645x_setup(struct dsa_switch *ds)
> > return err;
> > }
> >
> > + mutex_init(&lan9645x->fwd_domain_lock);
> > +
> > /* Link Aggregation Mode: NETDEV_LAG_HASH_L2 */
> > lan_wr(ANA_AGGR_CFG_AC_SMAC_ENA |
> > ANA_AGGR_CFG_AC_DMAC_ENA,
> [ ... ]
> > @@ -240,6 +252,11 @@ static int lan9645x_setup(struct dsa_switch *ds)
> >
> > lan9645x_port_set_tail_drop_wm(lan9645x);
> >
> > + lan9645x->owq = alloc_ordered_workqueue("%s-owq", 0,
> > + dev_name(lan9645x->dev));
> > + if (!lan9645x->owq)
> > + return -ENOMEM;
> > +
>
> Is it possible that the hardware remains initialized if the call to
> alloc_ordered_workqueue fails? Because the setup callback returns an error,
> the DSA core will skip the teardown callback. This would bypass
> lan9645x_npi_port_deinit, potentially leaving the conduit netdev in a
> corrupted state with hardware tagging enabled.
I do not think it is common practice to undo switch register writes in the
error path here. We reset the switch registers in .setup. If we exit
here there is nothing to teardown.
>
> Also in lan9645x_setup, the initialization loop for PGID_ENTRIES iterates
> through index 88:
>
> for (int i = 0; i < PGID_ENTRIES; ++i)
> lan_wr(ANA_PGID_CFG_OBEY_VLAN_SET(1),
> lan9645x, ANA_PGID_CFG(i));
>
> Could this leave the CPU port's source configuration in an undefined state?
> The CPU port is port 9, which has its source analysis register at PGID_SRC
> (80) + 9 = 89, but the loop terminates before reaching it.
>
No as I mentioned in another mail this OBEY_VLAN does not make sense for
pgid 89.
> > @@ -258,6 +275,271 @@ static void lan9645x_port_phylink_get_caps(struct dsa_switch *ds, int port,
> > lan9645x_phylink_get_caps(ds->priv, port, config);
> > }
> >
> > +static int lan9645x_set_ageing_time(struct dsa_switch *ds, unsigned int msecs)
> > +{
> > + u32 age_secs = max(1, msecs / MSEC_PER_SEC / 2);
> > + struct lan9645x *lan9645x = ds->priv;
> > +
> > + /* Entry is must suffer two aging scans before it is removed, so it is
> > + * aged after 2*AGE_PERIOD, and the unit is in seconds.
> > + * An age period of 0 disables automatic aging.
> > + */
> > + lan_rmw(ANA_AUTOAGE_AGE_PERIOD_SET(age_secs),
> > + ANA_AUTOAGE_AGE_PERIOD,
> > + lan9645x, ANA_AUTOAGE);
> > + return 0;
> > +}
>
> Does this function properly disable automatic aging when requested? When the
> Linux bridge passes msecs = 0 to disable aging, the max macro evaluates to 1.
> Since writing 1 configures an aggressive 2-second aging period, it appears
> this might not fulfill the user's intent.
>
Yes that is right. I will change it to:
lan_rmw(ANA_AUTOAGE_AGE_PERIOD_SET(msecs ? age_secs : 0),
ANA_AUTOAGE_AGE_PERIOD,
lan9645x, ANA_AUTOAGE);
> [ ... ]
> > +static void __lan9645x_port_set_host_flood(struct lan9645x *lan9645x, int port,
> > + bool uc, bool mc)
> > +{
> > + bool mc_ena, uc_ena;
> > +
> > + lockdep_assert_held(&lan9645x->fwd_domain_lock);
> > +
> > + /* We want promiscuous and all_multi to affect standalone ports, for
> > + * debug and test purposes.
> > + *
> > + * However, the linux bridge is incredibly eager to put bridged ports in
> > + * promiscuous mode.
> > +
> > + * This is unfortunate since lan9645x flood masks are global and not per
> > + * ingress port. When some port triggers unknown uc/mc to the CPU, the
> > + * traffic from any port is forwarded to the CPU.
> > + *
> > + * If the host CPU is weak, this can cause tremendous stress. Therefore,
> > + * we compromise by ignoring this host flood request for bridged ports.
> > + */
> > + if (lan9645x_port_is_bridged(lan9645x_to_port(lan9645x, port)))
> > + return;
>
> Will ignoring the host flood request for bridged ports break IPv6 Neighbor
> Discovery? IPv6 ND relies on multicast, and the PGID_MC and PGID_MCIPV6 masks
> do not include the CPU port by default. By ignoring the bridge's request to
> enable multicast flooding, the CPU port may fail to receive ICMPv6 ND packets.
>
No that is handled by other mechanisms. There are explicit handles to make sure
such traffic reaches the cpu.
> [ ... ]
> > +/* Called in atomic context */
> > +static void lan9645x_port_set_host_flood(struct dsa_switch *ds, int port,
> > + bool uc, bool mc)
> > +{
> > + struct lan9645x *lan9645x = ds->priv;
> > + struct lan9645x_host_flood_work *w;
> > +
> > + w = kzalloc_obj(*w, GFP_ATOMIC);
>
> Is kzalloc_obj an available kernel function? Using it might cause a build
> error, as the standard API would be kzalloc(sizeof(*w), GFP_ATOMIC).
>
> > + if (!w)
> > + return;
>
> Are there consequences to returning silently when the allocation fails?
> If this allocation fails under memory pressure, the network stack will still
> assume the configuration was successfully applied, leaving the hardware state
> out of sync.
>
But we can not return any error in the dsa op to the network stack, so I think
this is our only option.
> [ ... ]
> > +static int lan9645x_port_bridge_join(struct dsa_switch *ds, int port,
> > + struct dsa_bridge bridge,
> > + bool *tx_fwd_offload,
> > + struct netlink_ext_ack *extack)
> > +{
> [ ... ]
> > + /* The bridge puts ports in IFF_ALLMULTI before calling
> > + * port_bridge_join, so clean up before the port is marked as bridged.
> > + */
> > + __lan9645x_port_set_host_flood(lan9645x, port, false, false);
> > + lan9645x->bridge_mask |= BIT(p->chip_port);
> > +
> > + mutex_unlock(&lan9645x->fwd_domain_lock);
> > +
> > + /* Later: stp_state_set updates forwarding */
> > +
> > + return 0;
> > +}
>
> Could clearing the host flood masks here lead to lost hardware state when the
> port leaves the bridge? If a user previously enabled IFF_PROMISC or
> IFF_ALLMULTI on the standalone port, the software netdev flags remain set.
> When lan9645x_port_bridge_leave is called, it does not restore the hardware's
> promiscuous or multicast state to match the unchanged software flags.
>
Yes this is a bug. I will split flood requests and register writes, and make sure
the requested state is restored on bridge leave.
> > +static void lan9645x_port_bridge_stp_state_set(struct dsa_switch *ds, int port,
> > + u8 state)
> > +{
> > + struct lan9645x *lan9645x;
> > + struct lan9645x_port *p;
> > + bool learn_ena;
> > +
> > + lan9645x = ds->priv;
> > + p = lan9645x_to_port(lan9645x, port);
> > +
> > + mutex_lock(&lan9645x->fwd_domain_lock);
> > +
> > + p->stp_state = state;
> > +
> > + if (state == BR_STATE_FORWARDING)
> > + lan9645x->bridge_fwd_mask |= BIT(p->chip_port);
> > + else
> > + lan9645x->bridge_fwd_mask &= ~BIT(p->chip_port);
> > +
> > + learn_ena = (state == BR_STATE_LEARNING ||
> > + state == BR_STATE_FORWARDING) && p->learn_ena;
> > +
> > + lan_rmw(ANA_PORT_CFG_LEARN_ENA_SET(learn_ena),
> > + ANA_PORT_CFG_LEARN_ENA, lan9645x,
> > + ANA_PORT_CFG(p->chip_port));
> > +
> > + lan9645x_update_fwd_mask(lan9645x);
> > + mutex_unlock(&lan9645x->fwd_domain_lock);
> > +}
>
> Are ingress data frames correctly dropped for ports in non-forwarding STP
> states? When the port is removed from bridge_fwd_mask,
> lan9645x_update_fwd_mask calculates the source PGID mask as
> bridge_mask & bridge_fwd_mask & ~BIT(dp->index).
>
> This evaluates to the set of all other forwarding ports in the bridge. Since
> ingress forwarding is not explicitly disabled on the port (for example, by
> setting ANA_PORT_CFG_PORTID_VAL to NONE), data frames received on a blocking
> or learning port can enter the analyzer and be forwarded.
>
Yes this is a bug, I need to use a zero mask for the src pgid when a port is
in non-forwarding STP state.
> > +static void lan9645x_port_bridge_leave(struct dsa_switch *ds, int port,
> > + struct dsa_bridge bridge)
> > +{
> > + struct lan9645x *lan9645x = ds->priv;
> > + struct lan9645x_port *p;
> > +
> > + p = lan9645x_to_port(lan9645x, port);
> > +
> > + mutex_lock(&lan9645x->fwd_domain_lock);
> > +
> > + lan9645x->bridge_mask &= ~BIT(p->chip_port);
> > +
> > + /* Last port leaving clears bridge dev */
> > + if (!lan9645x->bridge_mask)
> > + lan9645x->bridge = NULL;
> > +
> > + lan9645x_update_fwd_mask(lan9645x);
> > +
> > + mutex_unlock(&lan9645x->fwd_domain_lock);
> > +}
>
> Should p->learn_ena be reset when a port leaves the bridge?
>
> During port_bridge_flags, p->learn_ena is set to true. When leaving the
> bridge, this flag remains true. The DSA core will transition the leaving
> port to BR_STATE_FORWARDING, which calls lan9645x_port_bridge_stp_state_set
> and leaves hardware learning enabled. This can pollute the shared FDB with
> MAC addresses from standalone ports, leading to silent packet drops if
> bridged ports attempt to forward traffic to them.
No I believe DSA core takes care of this with dsa_port_clear_brport_flags.
^ permalink raw reply
* [PATCH] dt-bindings: mailbox: qcom: Add IPCC support for Hawi Platform
From: Mukesh Ojha @ 2026-04-01 12:51 UTC (permalink / raw)
To: Manivannan Sadhasivam, Jassi Brar, Rob Herring,
Krzysztof Kozlowski, Conor Dooley
Cc: linux-arm-msm, linux-kernel, devicetree, Mukesh Ojha,
Konrad Dybcio
Document the Inter-Processor Communication Controller on the Qualcomm
Hawi Platform, which will be used to route interrupts across various
subsystems found on the SoC.
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
---
Documentation/devicetree/bindings/mailbox/qcom-ipcc.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/mailbox/qcom-ipcc.yaml b/Documentation/devicetree/bindings/mailbox/qcom-ipcc.yaml
index 7c4d6170491d..7dbc3ac6c5c9 100644
--- a/Documentation/devicetree/bindings/mailbox/qcom-ipcc.yaml
+++ b/Documentation/devicetree/bindings/mailbox/qcom-ipcc.yaml
@@ -25,6 +25,7 @@ properties:
items:
- enum:
- qcom,glymur-ipcc
+ - qcom,hawi-ipcc
- qcom,kaanapali-ipcc
- qcom,milos-ipcc
- qcom,qcs8300-ipcc
--
2.53.0
^ permalink raw reply related
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