* [PATCH 1/3] arm64: dts: ti: k3-am62-main: Fix the reg-range for dma-controller
From: Jayesh Choudhary @ 2024-04-05 8:52 UTC (permalink / raw)
To: nm, vigneshr, bb, devicetree, j-choudhary
Cc: kristo, robh, krzk+dt, conor+dt, linux-kernel, linux-arm-kernel
In-Reply-To: <20240405085208.32227-1-j-choudhary@ti.com>
The TX Channel Realtime Registers region 'tchanrt' is 128KB and Ring
Realtime Registers region 'ringrt' is 2MB as shown in memory map in
the TRM[0]. So fix ranges for those reg-regions.
[0]: <https://www.ti.com/lit/pdf/spruiv7>
Fixes: c37c58fdeb8a ("arm64: dts: ti: k3-am62: Add more peripheral nodes")
Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
---
arch/arm64/boot/dts/ti/k3-am62-main.dtsi | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
index e9cffca073ef..e10cc9fc0b10 100644
--- a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
@@ -141,8 +141,8 @@ main_pktdma: dma-controller@485c0000 {
compatible = "ti,am64-dmss-pktdma";
reg = <0x00 0x485c0000 0x00 0x100>,
<0x00 0x4a800000 0x00 0x20000>,
- <0x00 0x4aa00000 0x00 0x40000>,
- <0x00 0x4b800000 0x00 0x400000>,
+ <0x00 0x4aa00000 0x00 0x20000>,
+ <0x00 0x4b800000 0x00 0x200000>,
<0x00 0x485e0000 0x00 0x10000>,
<0x00 0x484a0000 0x00 0x2000>,
<0x00 0x484c0000 0x00 0x2000>,
--
2.25.1
^ permalink raw reply related
* [PATCH 0/3] Fix reg ranges for dma-controller node
From: Jayesh Choudhary @ 2024-04-05 8:52 UTC (permalink / raw)
To: nm, vigneshr, bb, devicetree, j-choudhary
Cc: kristo, robh, krzk+dt, conor+dt, linux-kernel, linux-arm-kernel
The dma-controller node 'main_pktdma' has few memory regions with
wrong sizes.
DMASS0_PKTDMA_RINGRT is marked as 4MB region when it is actually a 2MB
region. Similarly, DMASS0_PKTDMA_TCHANRT is marked as 256KB region but
the actual size is 128KB as shown in TRM in the section for Main Memory
Map (Table 2-1)
Fix these region across AM62, AM62A and AM62P (which is also used in
J722S)
TRM:
AM625: <https://www.ti.com/lit/pdf/spruiv7>
AM62A7: <https://www.ti.com/lit/pdf/spruj16>
AM62P: <https://www.ti.com/lit/pdf/spruj83>
J722S: <https://www.ti.com/lit/zip/sprujb3>
Jayesh Choudhary (3):
arm64: dts: ti: k3-am62-main: Fix the reg-range for dma-controller
arm64: dts: ti: k3-am62a-main: Fix the reg-range for dma-controller
arm64: dts: ti: k3-am62p-main: Fix the reg-range for dma-controller
arch/arm64/boot/dts/ti/k3-am62-main.dtsi | 4 ++--
arch/arm64/boot/dts/ti/k3-am62a-main.dtsi | 4 ++--
arch/arm64/boot/dts/ti/k3-am62p-main.dtsi | 4 ++--
3 files changed, 6 insertions(+), 6 deletions(-)
--
2.25.1
^ permalink raw reply
* [PATCH 3/3] arm64: dts: ti: k3-am62p-main: Fix the reg-range for dma-controller
From: Jayesh Choudhary @ 2024-04-05 8:52 UTC (permalink / raw)
To: nm, vigneshr, bb, devicetree, j-choudhary
Cc: kristo, robh, krzk+dt, conor+dt, linux-kernel, linux-arm-kernel
In-Reply-To: <20240405085208.32227-1-j-choudhary@ti.com>
The TX Channel Realtime Registers region 'tchanrt' is 128KB and Ring
Realtime Registers region 'ringrt' is 2MB as shown in memory map in
the TRM[0]. So fix ranges for those reg-regions.
[0]: <https://www.ti.com/lit/pdf/spruj83>
Fixes: b5080c7c1f7e ("arm64: dts: ti: k3-am62p: Add nodes for more IPs")
Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
---
arch/arm64/boot/dts/ti/k3-am62p-main.dtsi | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi
index 7337a9e13535..514c201bd5c9 100644
--- a/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi
@@ -123,8 +123,8 @@ main_pktdma: dma-controller@485c0000 {
compatible = "ti,am64-dmss-pktdma";
reg = <0x00 0x485c0000 0x00 0x100>,
<0x00 0x4a800000 0x00 0x20000>,
- <0x00 0x4aa00000 0x00 0x40000>,
- <0x00 0x4b800000 0x00 0x400000>,
+ <0x00 0x4aa00000 0x00 0x20000>,
+ <0x00 0x4b800000 0x00 0x200000>,
<0x00 0x485e0000 0x00 0x10000>,
<0x00 0x484a0000 0x00 0x2000>,
<0x00 0x484c0000 0x00 0x2000>,
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v12 2/7] clk: meson: add vclk driver
From: Jerome Brunet @ 2024-04-05 7:00 UTC (permalink / raw)
To: neil.armstrong
Cc: Jerome Brunet, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Martin Blumenstingl, Kevin Hilman, Michael Turquette,
Stephen Boyd, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Daniel Vetter, Jagan Teki, Nicolas Belin,
devicetree, linux-kernel, linux-amlogic, linux-clk,
linux-arm-kernel, dri-devel
In-Reply-To: <2cf79f07-0ae1-4267-ac08-fe40366d87d4@linaro.org>
On Thu 04 Apr 2024 at 18:59, Neil Armstrong <neil.armstrong@linaro.org> wrote:
> On 04/04/2024 10:13, Jerome Brunet wrote:
>> On Wed 03 Apr 2024 at 09:46, Neil Armstrong <neil.armstrong@linaro.org>
>> wrote:
>>
>>> The VCLK and VCLK_DIV clocks have supplementary bits.
>>>
>>> The VCLK gate has a "SOFT RESET" bit to toggle after the whole
>>> VCLK sub-tree rate has been set, this is implemented in
>>> the gate enable callback.
>>>
>>> The VCLK_DIV clocks as enable and reset bits used to disable
>>> and reset the divider, associated with CLK_SET_RATE_GATE it ensures
>>> the rate is set while the divider is disabled and in reset mode.
>>>
>>> The VCLK_DIV enable bit isn't implemented as a gate since it's part
>>> of the divider logic and vendor does this exact sequence to ensure
>>> the divider is correctly set.
>> The checkpatch warning is still there. Is it a choice or a mistake ?
>> Documentation says "GPL v2" exists for historic reason which seems to
>> hint "GPL" is preferred, and I suppose this is why checkpatch warns for
>> it.
>
> Well I didn't see this warning, this is what I fixed:
>
> $ scripts/checkpatch.pl --strict drivers/clk/meson/vclk.c
> CHECK: Alignment should match open parenthesis
> #63: FILE: drivers/clk/meson/vclk.c:63:
> +static unsigned long meson_vclk_div_recalc_rate(struct clk_hw *hw,
> + unsigned long prate)
>
> CHECK: Alignment should match open parenthesis
> #73: FILE: drivers/clk/meson/vclk.c:73:
> +static int meson_vclk_div_determine_rate(struct clk_hw *hw,
> + struct clk_rate_request *req)
>
> CHECK: Alignment should match open parenthesis
> #83: FILE: drivers/clk/meson/vclk.c:83:
> +static int meson_vclk_div_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
>
I would not ask a respin solely for this. It's nice to fix it but I was
mostly after the warning TBH.
> <snip>
>
> It seems that checking a commit triggers an extra check....
>
> $ scripts/checkpatch.pl --strict -G 1bac9f6aa3c3
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #58:
> new file mode 100644
>
> <snip>
>
> WARNING: Prefer "GPL" over "GPL v2" - see commit bf7fbeeae6db ("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity")
> #203: FILE: drivers/clk/meson/vclk.c:141:
> +MODULE_LICENSE("GPL v2");
Hum, I'm running checkpatch against the mail itself, not the commit. I
still get the warning
>
> <snip>
>
> Neil
>
>>
>>>
>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>> ---
>>> drivers/clk/meson/Kconfig | 4 ++
>>> drivers/clk/meson/Makefile | 1 +
>>> drivers/clk/meson/vclk.c | 141 +++++++++++++++++++++++++++++++++++++++++++++
>>> drivers/clk/meson/vclk.h | 51 ++++++++++++++++
>>> 4 files changed, 197 insertions(+)
>>>
>>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>>> index 29ffd14d267b..8a9823789fa3 100644
>>> --- a/drivers/clk/meson/Kconfig
>>> +++ b/drivers/clk/meson/Kconfig
>>> @@ -30,6 +30,10 @@ config COMMON_CLK_MESON_VID_PLL_DIV
>>> tristate
>>> select COMMON_CLK_MESON_REGMAP
>>> +config COMMON_CLK_MESON_VCLK
>>> + tristate
>>> + select COMMON_CLK_MESON_REGMAP
>>> +
>>> config COMMON_CLK_MESON_CLKC_UTILS
>>> tristate
>>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>>> index 9ee4b954c896..9ba43fe7a07a 100644
>>> --- a/drivers/clk/meson/Makefile
>>> +++ b/drivers/clk/meson/Makefile
>>> @@ -12,6 +12,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_PLL) += clk-pll.o
>>> obj-$(CONFIG_COMMON_CLK_MESON_REGMAP) += clk-regmap.o
>>> obj-$(CONFIG_COMMON_CLK_MESON_SCLK_DIV) += sclk-div.o
>>> obj-$(CONFIG_COMMON_CLK_MESON_VID_PLL_DIV) += vid-pll-div.o
>>> +obj-$(CONFIG_COMMON_CLK_MESON_VCLK) += vclk.o
>>> # Amlogic Clock controllers
>>> diff --git a/drivers/clk/meson/vclk.c b/drivers/clk/meson/vclk.c
>>> new file mode 100644
>>> index 000000000000..45dc216941ea
>>> --- /dev/null
>>> +++ b/drivers/clk/meson/vclk.c
>>> @@ -0,0 +1,141 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (c) 2024 Neil Armstrong <neil.armstrong@linaro.org>
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include "vclk.h"
>>> +
>>> +/* The VCLK gate has a supplementary reset bit to pulse after ungating */
>>> +
>>> +static inline struct meson_vclk_gate_data *
>>> +clk_get_meson_vclk_gate_data(struct clk_regmap *clk)
>>> +{
>>> + return (struct meson_vclk_gate_data *)clk->data;
>>> +}
>>> +
>>> +static int meson_vclk_gate_enable(struct clk_hw *hw)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct meson_vclk_gate_data *vclk = clk_get_meson_vclk_gate_data(clk);
>>> +
>>> + meson_parm_write(clk->map, &vclk->enable, 1);
>>> +
>>> + /* Do a reset pulse */
>>> + meson_parm_write(clk->map, &vclk->reset, 1);
>>> + meson_parm_write(clk->map, &vclk->reset, 0);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void meson_vclk_gate_disable(struct clk_hw *hw)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct meson_vclk_gate_data *vclk = clk_get_meson_vclk_gate_data(clk);
>>> +
>>> + meson_parm_write(clk->map, &vclk->enable, 0);
>>> +}
>>> +
>>> +static int meson_vclk_gate_is_enabled(struct clk_hw *hw)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct meson_vclk_gate_data *vclk = clk_get_meson_vclk_gate_data(clk);
>>> +
>>> + return meson_parm_read(clk->map, &vclk->enable);
>>> +}
>>> +
>>> +const struct clk_ops meson_vclk_gate_ops = {
>>> + .enable = meson_vclk_gate_enable,
>>> + .disable = meson_vclk_gate_disable,
>>> + .is_enabled = meson_vclk_gate_is_enabled,
>>> +};
>>> +EXPORT_SYMBOL_GPL(meson_vclk_gate_ops);
>>> +
>>> +/* The VCLK Divider has supplementary reset & enable bits */
>>> +
>>> +static inline struct meson_vclk_div_data *
>>> +clk_get_meson_vclk_div_data(struct clk_regmap *clk)
>>> +{
>>> + return (struct meson_vclk_div_data *)clk->data;
>>> +}
>>> +
>>> +static unsigned long meson_vclk_div_recalc_rate(struct clk_hw *hw,
>>> + unsigned long prate)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
>>> +
>>> + return divider_recalc_rate(hw, prate, meson_parm_read(clk->map, &vclk->div),
>>> + vclk->table, vclk->flags, vclk->div.width);
>>> +}
>>> +
>>> +static int meson_vclk_div_determine_rate(struct clk_hw *hw,
>>> + struct clk_rate_request *req)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
>>> +
>>> + return divider_determine_rate(hw, req, vclk->table, vclk->div.width,
>>> + vclk->flags);
>>> +}
>>> +
>>> +static int meson_vclk_div_set_rate(struct clk_hw *hw, unsigned long rate,
>>> + unsigned long parent_rate)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
>>> + int ret;
>>> +
>>> + ret = divider_get_val(rate, parent_rate, vclk->table, vclk->div.width,
>>> + vclk->flags);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + meson_parm_write(clk->map, &vclk->div, ret);
>>> +
>>> + return 0;
>>> +};
>>> +
>>> +static int meson_vclk_div_enable(struct clk_hw *hw)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
>>> +
>>> + /* Unreset the divider when ungating */
>>> + meson_parm_write(clk->map, &vclk->reset, 0);
>>> + meson_parm_write(clk->map, &vclk->enable, 1);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void meson_vclk_div_disable(struct clk_hw *hw)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
>>> +
>>> + /* Reset the divider when gating */
>>> + meson_parm_write(clk->map, &vclk->enable, 0);
>>> + meson_parm_write(clk->map, &vclk->reset, 1);
>>> +}
>>> +
>>> +static int meson_vclk_div_is_enabled(struct clk_hw *hw)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
>>> +
>>> + return meson_parm_read(clk->map, &vclk->enable);
>>> +}
>>> +
>>> +const struct clk_ops meson_vclk_div_ops = {
>>> + .recalc_rate = meson_vclk_div_recalc_rate,
>>> + .determine_rate = meson_vclk_div_determine_rate,
>>> + .set_rate = meson_vclk_div_set_rate,
>>> + .enable = meson_vclk_div_enable,
>>> + .disable = meson_vclk_div_disable,
>>> + .is_enabled = meson_vclk_div_is_enabled,
>>> +};
>>> +EXPORT_SYMBOL_GPL(meson_vclk_div_ops);
>>> +
>>> +MODULE_DESCRIPTION("Amlogic vclk clock driver");
>>> +MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
>>> +MODULE_LICENSE("GPL v2");
>>> diff --git a/drivers/clk/meson/vclk.h b/drivers/clk/meson/vclk.h
>>> new file mode 100644
>>> index 000000000000..20b0b181db09
>>> --- /dev/null
>>> +++ b/drivers/clk/meson/vclk.h
>>> @@ -0,0 +1,51 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * Copyright (c) 2024 Neil Armstrong <neil.armstrong@linaro.org>
>>> + */
>>> +
>>> +#ifndef __VCLK_H
>>> +#define __VCLK_H
>>> +
>>> +#include "clk-regmap.h"
>>> +#include "parm.h"
>>> +
>>> +/**
>>> + * struct meson_vclk_gate_data - vclk_gate regmap backed specific data
>>> + *
>>> + * @enable: vclk enable field
>>> + * @reset: vclk reset field
>>> + * @flags: hardware-specific flags
>>> + *
>>> + * Flags:
>>> + * Same as clk_gate except CLK_GATE_HIWORD_MASK which is ignored
>>> + */
>>> +struct meson_vclk_gate_data {
>>> + struct parm enable;
>>> + struct parm reset;
>>> + u8 flags;
>>> +};
>>> +
>>> +extern const struct clk_ops meson_vclk_gate_ops;
>>> +
>>> +/**
>>> + * struct meson_vclk_div_data - vclk_div regmap back specific data
>>> + *
>>> + * @div: divider field
>>> + * @enable: vclk divider enable field
>>> + * @reset: vclk divider reset field
>>> + * @table: array of value/divider pairs, last entry should have div = 0
>>> + *
>>> + * Flags:
>>> + * Same as clk_divider except CLK_DIVIDER_HIWORD_MASK which is ignored
>>> + */
>>> +struct meson_vclk_div_data {
>>> + struct parm div;
>>> + struct parm enable;
>>> + struct parm reset;
>>> + const struct clk_div_table *table;
>>> + u8 flags;
>>> +};
>>> +
>>> +extern const struct clk_ops meson_vclk_div_ops;
>>> +
>>> +#endif /* __VCLK_H */
>>
--
Jerome
^ permalink raw reply
* [PATCH v1 0/2] Bluetooth: btnxpuart: Update firmware names
From: Neeraj Sanjay Kale @ 2024-04-05 9:01 UTC (permalink / raw)
To: marcel, luiz.dentz, davem, edumazet, kuba, pabeni, robh+dt,
krzysztof.kozlowski+dt, conor+dt
Cc: linux-bluetooth, netdev, devicetree, linux-kernel,
amitkumar.karwar, rohit.fule, neeraj.sanjaykale, sherry.sun,
ziniu.wang_1, haibo.chen, LnxRevLi
This patch series updates the BT firmware file names in BTNXPUART
driver, and adds a new optional firmware-name device tree property to
override the firmware file names hardcoded in the driver. This will
allow user to continue using the older firmware files.
This change is necessary as newer firmware releases will have
standardized naming convention aligned across all newer and legacy
chipsets.
Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
Neeraj Sanjay Kale (2):
dt-bindings: net: bluetooth: btnxpuart: Add firmware-name property
Bluetooth: btnxpuart: Update firmware names
.../bindings/net/bluetooth/nxp,88w8987-bt.yaml | 8 ++++++++
drivers/bluetooth/btnxpuart.c | 12 +++++++++---
2 files changed, 17 insertions(+), 3 deletions(-)
--
2.34.1
^ permalink raw reply
* [PATCH v1 1/2] dt-bindings: net: bluetooth: btnxpuart: Add firmware-name property
From: Neeraj Sanjay Kale @ 2024-04-05 9:01 UTC (permalink / raw)
To: marcel, luiz.dentz, davem, edumazet, kuba, pabeni, robh+dt,
krzysztof.kozlowski+dt, conor+dt
Cc: linux-bluetooth, netdev, devicetree, linux-kernel,
amitkumar.karwar, rohit.fule, neeraj.sanjaykale, sherry.sun,
ziniu.wang_1, haibo.chen, LnxRevLi
In-Reply-To: <20240405090118.582310-1-neeraj.sanjaykale@nxp.com>
This adds a new optional device tree property called firware-name.
If this property is specified, the driver overrides the firmware
name hardcoded in the driver source code.
Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
---
.../devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml b/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml
index f01a3988538c..25c258212bcd 100644
--- a/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml
+++ b/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml
@@ -31,6 +31,13 @@ properties:
This property depends on the module vendor's
configuration.
+ firmware-name:
+ $ref: /schemas/types.yaml#/definitions/string
+ description:
+ Specify firmware file name, prepended with nxp/.
+ This property overrides the firmware names hardcoded
+ in the driver source code.
+
required:
- compatible
@@ -42,5 +49,6 @@ examples:
bluetooth {
compatible = "nxp,88w8987-bt";
fw-init-baudrate = <3000000>;
+ firmware-name = "nxp/uartuart8987_bt_v0.bin";
};
};
--
2.34.1
^ permalink raw reply related
* [PATCH v1 2/2] Bluetooth: btnxpuart: Update firmware names
From: Neeraj Sanjay Kale @ 2024-04-05 9:01 UTC (permalink / raw)
To: marcel, luiz.dentz, davem, edumazet, kuba, pabeni, robh+dt,
krzysztof.kozlowski+dt, conor+dt
Cc: linux-bluetooth, netdev, devicetree, linux-kernel,
amitkumar.karwar, rohit.fule, neeraj.sanjaykale, sherry.sun,
ziniu.wang_1, haibo.chen, LnxRevLi
In-Reply-To: <20240405090118.582310-1-neeraj.sanjaykale@nxp.com>
This updates the firmware names of 3 chipsets: w8987, w8997, w9098.
These changes are been done to standardize chip specific firmware
file names.
To allow user to use older firmware file names, a new device tree
property has been introduced called firmware-name, which will override
the hardcoded firmware names in the driver.
Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
---
drivers/bluetooth/btnxpuart.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index 0b93c2ff29e4..45f2ae2b542b 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -33,9 +33,9 @@
/* NXP HW err codes */
#define BTNXPUART_IR_HW_ERR 0xb0
-#define FIRMWARE_W8987 "nxp/uartuart8987_bt.bin"
-#define FIRMWARE_W8997 "nxp/uartuart8997_bt_v4.bin"
-#define FIRMWARE_W9098 "nxp/uartuart9098_bt_v1.bin"
+#define FIRMWARE_W8987 "nxp/uart8987_bt_v0.bin"
+#define FIRMWARE_W8997 "nxp/uart8997_bt_v4.bin"
+#define FIRMWARE_W9098 "nxp/uart9098_bt_v1.bin"
#define FIRMWARE_IW416 "nxp/uartiw416_bt_v0.bin"
#define FIRMWARE_IW612 "nxp/uartspi_n61x_v1.bin.se"
#define FIRMWARE_IW624 "nxp/uartiw624_bt.bin"
@@ -685,12 +685,18 @@ static bool process_boot_signature(struct btnxpuart_dev *nxpdev)
static int nxp_request_firmware(struct hci_dev *hdev, const char *fw_name)
{
struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+ const char *fw_name_dt;
int err = 0;
if (!fw_name)
return -ENOENT;
if (!strlen(nxpdev->fw_name)) {
+ if (strcmp(fw_name, FIRMWARE_HELPER) &&
+ !device_property_read_string(&nxpdev->serdev->dev,
+ "firmware-name",
+ &fw_name_dt))
+ fw_name = fw_name_dt;
snprintf(nxpdev->fw_name, MAX_FW_FILE_NAME_LEN, "%s", fw_name);
bt_dev_dbg(hdev, "Request Firmware: %s", nxpdev->fw_name);
--
2.34.1
^ permalink raw reply related
* Re: [RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC
From: Matti Vaittinen @ 2024-04-05 9:19 UTC (permalink / raw)
To: Mark Brown
Cc: Matti Vaittinen, Lee Jones, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Liam Girdwood, Wim Van Sebroeck, Guenter Roeck,
devicetree, linux-kernel, linux-watchdog
In-Reply-To: <b6279be8-cf7d-4608-b556-3c01587f0d43@gmail.com>
On 4/4/24 16:15, Matti Vaittinen wrote:
> Hi Mark,
>
> On 4/4/24 15:09, Mark Brown wrote:
>> On Thu, Apr 04, 2024 at 10:26:34AM +0300, Matti Vaittinen wrote:
>>
>>> 1. Should we be able to have more than 1 IRQ domain / device?
>>> 2. Should regmap_irq support having more than 1 HWIRQ
>>
>> I would expect each parent interrupt to show up as a separate remap_irq.
>>
>>> then it seems that reading the IRQ information from the /proc/interrupts
>>> works as expected. Here I am making a wild guess that the name of the
>>> domain
>>> is used as a key for some data-lookups, and having two domains with a
>>> same
>>> name will either overwrite something or cause wrong domain data to be
>>> fetched. (This is just guessing for now).
This was wrong guessing.
>> So if we arrange to supply a name when we register multiple domains
>> things should work fine?
After my latest findings, yes, I think so. How to do this correctly is
beyond me though. The __irq_domain_create() seems to me that the name is
meant to be the dt-node name when the controller is backed by a real
dt-node. Naming of the irq_domain_alloc_named_fwnode() sounds to me like
it is only intended to be used when there is no real fwnode. All
suggestions appreciated. Using the:
irq_domain_update_bus_token(intb_domain, DOMAIN_BUS_WIRED);
feels like a dirty hack, and won't scale if there is more HWIRQs.
> Thanks for taking the time to look at my questions :)
> I have been debugging this thing whole day today, without getting too
> far :) It seems there is something beyond the name collision though.
>
> After I tried adding '-1' to the end of the other domain name to avoid
> the debugfs name collision I managed to do couple of successful runs -
> after which I reported here that problem seems to be just the naming.
> Soon after sending that mail I hit the oops again even though the naming
> was fixed.
>
> Further debugging shows that the desc->action->name for the last 28
> 'errb' IRQs get corrupted. This might point more to the IRQ requester
> side - so I need to further study the BD96801 driver side as well as the
> regulator_irq_helper. I'm having the creeping feeling that at the end of
> the day I need to find the guilty one from the mirror :)
I was not wrong on this one. The regulator_irq_helper() duplicates
memory for the data given in const struct regulator_irq_desc *d - but
it does not duplicate the irq name pointed from the given
regulator_irq_desc. Nor does the request_threaded_irq(). I passed some
of the IRQ names from the stack in the BD96801 driver ... a bug I
should've caught earlier.
Well, good thing is that now I can fix the regulator_irq_helper() to do:
--- a/drivers/regulator/irq_helpers.c
+++ b/drivers/regulator/irq_helpers.c
@@ -352,6 +352,9 @@ void *regulator_irq_helper(struct device *dev,
h->irq = irq;
h->desc = *d;
+ h->desc.name = devm_kstrdup(dev, d->name, GFP_KERNEL);
+ if (!h->desc.name)
+ return ERR_PTR(-ENOMEM);
ret = init_rdev_state(dev, h, rdev, common_errs, per_rdev_errs,
rdev_amount);
I'll send a patch if this sounds like a correct thing to do.
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply
* Re: [PATCH] dt-bindings: extcon: ptn5150: Document the 'port' node
From: Krzysztof Kozlowski @ 2024-04-05 9:24 UTC (permalink / raw)
To: Fabio Estevam, cw00.choi
Cc: myungjoo.ham, robh, conor+dt, devicetree, marex, Fabio Estevam
In-Reply-To: <20240404022943.528293-1-festevam@gmail.com>
On 04/04/2024 04:29, Fabio Estevam wrote:
> From: Fabio Estevam <festevam@denx.de>
>
> Doument the port node to link the PTN5150 to a TypeC controller.
>
> This fixes the following dt-schema warnings:
>
> imx8mp-dhcom-pdk3.dtb: typec@3d: 'port' does not match any of the regexes: 'pinctrl-[0-9]+'
> from schema $id: http://devicetree.org/schemas/extcon/extcon-ptn5150.yaml#
Your patch ends up in spam. Probably
>
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
> .../devicetree/bindings/extcon/extcon-ptn5150.yaml | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/extcon/extcon-ptn5150.yaml b/Documentation/devicetree/bindings/extcon/extcon-ptn5150.yaml
> index d5cfa32ea52d..3472c69056ac 100644
> --- a/Documentation/devicetree/bindings/extcon/extcon-ptn5150.yaml
> +++ b/Documentation/devicetree/bindings/extcon/extcon-ptn5150.yaml
> @@ -36,6 +36,11 @@ properties:
> description:
> GPIO pin (output) used to control VBUS. If skipped, no such control
> takes place.
Missing blank line.
> + port:
> + $ref: /schemas/graph.yaml#/$defs/port-base
Why not "$ref: /schemas/graph.yaml#/properties/port"?
But more important, what about USB C connector?
> + description:
> + A port node to link the PTN5150 to a TypeC controller.
> + unevaluatedProperties: false
>
> required:
> - compatible
> @@ -58,5 +63,11 @@ examples:
> interrupt-parent = <&msmgpio>;
> interrupts = <78 IRQ_TYPE_LEVEL_HIGH>;
> vbus-gpios = <&msmgpio 148 GPIO_ACTIVE_HIGH>;
> +
> + port {
> + ptn5150_out_ep: endpoint {
> + remote-endpoint = <&dwc3_0_ep>;
> + };
Messed indentation. Just compare the indentation with what is around. It
seems you repeat the same mistake in your recent bindings patches.
> + };
> };
> };
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH 1/2] arm64: dts: imx8mm-var-som-symphony: drop redundant status from typec
From: Krzysztof Kozlowski @ 2024-04-05 9:28 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, devicetree,
imx, linux-arm-kernel, linux-kernel
Cc: Krzysztof Kozlowski
"okay" is the default status, so drop redundant property from the typec
node.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
arch/arm64/boot/dts/freescale/imx8mm-var-som-symphony.dts | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/arm64/boot/dts/freescale/imx8mm-var-som-symphony.dts b/arch/arm64/boot/dts/freescale/imx8mm-var-som-symphony.dts
index d643381417f1..affbc67c2ef6 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-var-som-symphony.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mm-var-som-symphony.dts
@@ -117,7 +117,6 @@ extcon_usbotg1: typec@3d {
interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_ptn5150>;
- status = "okay";
};
};
--
2.34.1
^ permalink raw reply related
* [PATCH 2/2] arm64: dts: imx8mn-var-som-symphony: drop redundant status from typec
From: Krzysztof Kozlowski @ 2024-04-05 9:28 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, devicetree,
imx, linux-arm-kernel, linux-kernel
Cc: Krzysztof Kozlowski
In-Reply-To: <20240405092819.40994-1-krzysztof.kozlowski@linaro.org>
"okay" is the default status, so drop redundant property from the typec
node.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
arch/arm64/boot/dts/freescale/imx8mn-var-som-symphony.dts | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/arm64/boot/dts/freescale/imx8mn-var-som-symphony.dts b/arch/arm64/boot/dts/freescale/imx8mn-var-som-symphony.dts
index a6b94d1957c9..3434b189fa58 100644
--- a/arch/arm64/boot/dts/freescale/imx8mn-var-som-symphony.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mn-var-som-symphony.dts
@@ -126,7 +126,6 @@ extcon_usbotg1: typec@3d {
interrupts = <11 IRQ_TYPE_EDGE_FALLING>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_ptn5150>;
- status = "okay";
port {
typec1_dr_sw: endpoint {
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v6 05/16] dt-bindings: net: wireless: describe the ath12k PCI module
From: Kalle Valo @ 2024-04-05 9:33 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Marcel Holtmann, Luiz Augusto von Dentz, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Liam Girdwood, Mark Brown, Catalin Marinas, Will Deacon,
Bjorn Helgaas, Saravana Kannan, Geert Uytterhoeven, Arnd Bergmann,
Neil Armstrong, Marek Szyprowski, Alex Elder, Srini Kandagatla,
Greg Kroah-Hartman, Abel Vesa, Manivannan Sadhasivam,
Lukas Wunner, Dmitry Baryshkov, linux-bluetooth, netdev,
devicetree, linux-kernel, linux-wireless, linux-arm-msm,
linux-arm-kernel, linux-pci, linux-pm, Bartosz Golaszewski
In-Reply-To: <CAMRc=MeCjNn7QdDrcQMuj32JFYoemQ6A8WOYcwKJo1YhDTfY+Q@mail.gmail.com>
Bartosz Golaszewski <brgl@bgdev.pl> writes:
> On Mon, Mar 25, 2024 at 3:01 PM Kalle Valo <kvalo@kernel.org> wrote:
>>
>> Bartosz Golaszewski <brgl@bgdev.pl> writes:
>>
>> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>> >
>> > +
>> > +maintainers:
>> > + - Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>
>> IMHO it would be better to have just driver maintainers listed here.
>>
>
> Why? What's wrong with having the author of the bindings in the Cc list?
If you want follow the ath12k development and review patches then you
can join the ath12k list. I'm not fond of having too many maintainers,
it's not really helping anything and just extra work to periodically
cleanup the silent maintainers.
I would ask the opposite question: why add you as the maintainer?
There's not even a single ath12k patch from you, nor I haven't seen you
doing any patch review or otherwise helping others related to ath12k.
Don't get me wrong, I value the work you do with this important powerseq
feature and hopefully we get it into the tree soon. But I don't see
adding you as a maintainer at this point.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH v6 05/16] dt-bindings: net: wireless: describe the ath12k PCI module
From: Krzysztof Kozlowski @ 2024-04-05 9:37 UTC (permalink / raw)
To: Kalle Valo, Bartosz Golaszewski
Cc: Marcel Holtmann, Luiz Augusto von Dentz, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Liam Girdwood, Mark Brown, Catalin Marinas, Will Deacon,
Bjorn Helgaas, Saravana Kannan, Geert Uytterhoeven, Arnd Bergmann,
Neil Armstrong, Marek Szyprowski, Alex Elder, Srini Kandagatla,
Greg Kroah-Hartman, Abel Vesa, Manivannan Sadhasivam,
Lukas Wunner, Dmitry Baryshkov, linux-bluetooth, netdev,
devicetree, linux-kernel, linux-wireless, linux-arm-msm,
linux-arm-kernel, linux-pci, linux-pm, Bartosz Golaszewski
In-Reply-To: <87cyr440hr.fsf@kernel.org>
On 05/04/2024 11:33, Kalle Valo wrote:
> Bartosz Golaszewski <brgl@bgdev.pl> writes:
>
>> On Mon, Mar 25, 2024 at 3:01 PM Kalle Valo <kvalo@kernel.org> wrote:
>>>
>>> Bartosz Golaszewski <brgl@bgdev.pl> writes:
>>>
>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>
>>>> +
>>>> +maintainers:
>>>> + - Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>
>>> IMHO it would be better to have just driver maintainers listed here.
>>>
>>
>> Why? What's wrong with having the author of the bindings in the Cc list?
>
> If you want follow the ath12k development and review patches then you
> can join the ath12k list. I'm not fond of having too many maintainers,
> it's not really helping anything and just extra work to periodically
> cleanup the silent maintainers.
>
> I would ask the opposite question: why add you as the maintainer?
> There's not even a single ath12k patch from you, nor I haven't seen you
> doing any patch review or otherwise helping others related to ath12k.
> Don't get me wrong, I value the work you do with this important powerseq
> feature and hopefully we get it into the tree soon. But I don't see
> adding you as a maintainer at this point.
This is not a maintainer of driver. This is maintainer of bindings, so
someone who has hardware, datasheets, knowledge and/or interest in
keeping the bindings accurate.
All your arguments above suggest you talk about the driver. This is not
the point here.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v1 1/2] dt-bindings: net: bluetooth: btnxpuart: Add firmware-name property
From: Krzysztof Kozlowski @ 2024-04-05 9:39 UTC (permalink / raw)
To: Neeraj Sanjay Kale, marcel, luiz.dentz, davem, edumazet, kuba,
pabeni, robh+dt, krzysztof.kozlowski+dt, conor+dt
Cc: linux-bluetooth, netdev, devicetree, linux-kernel,
amitkumar.karwar, rohit.fule, sherry.sun, ziniu.wang_1,
haibo.chen, LnxRevLi
In-Reply-To: <20240405090118.582310-2-neeraj.sanjaykale@nxp.com>
On 05/04/2024 11:01, Neeraj Sanjay Kale wrote:
> This adds a new optional device tree property called firware-name.
> If this property is specified, the driver overrides the firmware
> name hardcoded in the driver source code.
>
> Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> ---
> .../devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml b/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml
> index f01a3988538c..25c258212bcd 100644
> --- a/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml
> +++ b/Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml
> @@ -31,6 +31,13 @@ properties:
> This property depends on the module vendor's
> configuration.
>
> + firmware-name:
> + $ref: /schemas/types.yaml#/definitions/string
drop, missing maxItems. Look at recent code.
> + description:
> + Specify firmware file name, prepended with nxp/.
Why with nxp? What if firmware is stored somewhere else?
> + This property overrides the firmware names hardcoded
> + in the driver source code.
Drop, you now describe current OS policy. What if U-boot treats it
differently?
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v6 05/16] dt-bindings: net: wireless: describe the ath12k PCI module
From: Krzysztof Kozlowski @ 2024-04-05 9:41 UTC (permalink / raw)
To: Kalle Valo, Bartosz Golaszewski
Cc: Marcel Holtmann, Luiz Augusto von Dentz, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Liam Girdwood, Mark Brown, Catalin Marinas, Will Deacon,
Bjorn Helgaas, Saravana Kannan, Geert Uytterhoeven, Arnd Bergmann,
Neil Armstrong, Marek Szyprowski, Alex Elder, Srini Kandagatla,
Greg Kroah-Hartman, Abel Vesa, Manivannan Sadhasivam,
Lukas Wunner, Dmitry Baryshkov, linux-bluetooth, netdev,
devicetree, linux-kernel, linux-wireless, linux-arm-msm,
linux-arm-kernel, linux-pci, linux-pm, Bartosz Golaszewski
In-Reply-To: <83dccd7e-e690-4803-adb9-aaedcee7dc94@linaro.org>
On 05/04/2024 11:37, Krzysztof Kozlowski wrote:
> On 05/04/2024 11:33, Kalle Valo wrote:
>> Bartosz Golaszewski <brgl@bgdev.pl> writes:
>>
>>> On Mon, Mar 25, 2024 at 3:01 PM Kalle Valo <kvalo@kernel.org> wrote:
>>>>
>>>> Bartosz Golaszewski <brgl@bgdev.pl> writes:
>>>>
>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>
>>>>> +
>>>>> +maintainers:
>>>>> + - Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>
>>>> IMHO it would be better to have just driver maintainers listed here.
>>>>
>>>
>>> Why? What's wrong with having the author of the bindings in the Cc list?
>>
>> If you want follow the ath12k development and review patches then you
>> can join the ath12k list. I'm not fond of having too many maintainers,
>> it's not really helping anything and just extra work to periodically
>> cleanup the silent maintainers.
>>
>> I would ask the opposite question: why add you as the maintainer?
>> There's not even a single ath12k patch from you, nor I haven't seen you
>> doing any patch review or otherwise helping others related to ath12k.
>> Don't get me wrong, I value the work you do with this important powerseq
>> feature and hopefully we get it into the tree soon. But I don't see
>> adding you as a maintainer at this point.
>
> This is not a maintainer of driver. This is maintainer of bindings, so
> someone who has hardware, datasheets, knowledge and/or interest in
> keeping the bindings accurate.
>
> All your arguments above suggest you talk about the driver. This is not
> the point here.
>
And to clarify: I do not have opinion whether Bartosz is a suitable
person here and whether driver maintainers should be instead or not. I
only want to clarify the purpose of the binding maintainer.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v6 05/16] dt-bindings: net: wireless: describe the ath12k PCI module
From: Bartosz Golaszewski @ 2024-04-05 9:52 UTC (permalink / raw)
To: Kalle Valo
Cc: Marcel Holtmann, Luiz Augusto von Dentz, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Liam Girdwood, Mark Brown, Catalin Marinas, Will Deacon,
Bjorn Helgaas, Saravana Kannan, Geert Uytterhoeven, Arnd Bergmann,
Neil Armstrong, Marek Szyprowski, Alex Elder, Srini Kandagatla,
Greg Kroah-Hartman, Abel Vesa, Manivannan Sadhasivam,
Lukas Wunner, Dmitry Baryshkov, linux-bluetooth, netdev,
devicetree, linux-kernel, linux-wireless, linux-arm-msm,
linux-arm-kernel, linux-pci, linux-pm, Bartosz Golaszewski
In-Reply-To: <87cyr440hr.fsf@kernel.org>
On Fri, Apr 5, 2024 at 11:34 AM Kalle Valo <kvalo@kernel.org> wrote:
>
> Bartosz Golaszewski <brgl@bgdev.pl> writes:
>
> > On Mon, Mar 25, 2024 at 3:01 PM Kalle Valo <kvalo@kernel.org> wrote:
> >>
> >> Bartosz Golaszewski <brgl@bgdev.pl> writes:
> >>
> >> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >> >
> >> > +
> >> > +maintainers:
> >> > + - Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>
> >> IMHO it would be better to have just driver maintainers listed here.
> >>
> >
> > Why? What's wrong with having the author of the bindings in the Cc list?
>
> If you want follow the ath12k development and review patches then you
> can join the ath12k list. I'm not fond of having too many maintainers,
> it's not really helping anything and just extra work to periodically
> cleanup the silent maintainers.
>
> I would ask the opposite question: why add you as the maintainer?
> There's not even a single ath12k patch from you, nor I haven't seen you
> doing any patch review or otherwise helping others related to ath12k.
> Don't get me wrong, I value the work you do with this important powerseq
> feature and hopefully we get it into the tree soon. But I don't see
> adding you as a maintainer at this point.
>
In addition to what Krzysztof already said about you seamingly
confusing the maintenance of the driver vs maintenance of the
device-tree bindings (IOW: structured hardware description) and in
response to your question: I don't see any functional change to any
dt-bindings neither from you nor from Jeff. Are you convinced you can
maintain and properly review any changes?
If so, I don't really care, I can drop myself and have less work.
Bartosz
> --
> https://patchwork.kernel.org/project/linux-wireless/list/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH v3 12/25] media: i2c: imx258: Allow configuration of clock lane behaviour
From: Pavel Machek @ 2024-04-05 9:54 UTC (permalink / raw)
To: Luigi311
Cc: linux-media, dave.stevenson, jacopo.mondi, mchehab, robh,
krzysztof.kozlowski+dt, conor+dt, shawnguo, s.hauer, kernel,
festevam, sakari.ailus, devicetree, imx, linux-arm-kernel,
linux-kernel, phone-devel
In-Reply-To: <6c15e492-411a-40aa-b02e-83b8a6d107da@luigi311.com>
[-- Attachment #1: Type: text/plain, Size: 968 bytes --]
On Thu 2024-04-04 16:29:11, Luigi311 wrote:
> On 4/3/24 12:48, Pavel Machek wrote:
> > Hi!
> >
> >> The sensor supports the clock lane either remaining in HS mode
> >> during frame blanking, or dropping to LP11.
> >>
> >> Add configuration of the mode via V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK.
> >
> >> + ret = imx258_write_reg(imx258, IMX258_CLK_BLANK_STOP,
> >> + IMX258_REG_VALUE_08BIT,
> >> + imx258->csi2_flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK ?
> >> + 1 : 0);
> >
> > !! can be used to turn value into 1/0. I find it easier to read than ?
> > 1 : 0 combination, but possibly that's fine, too.
> >
> > Best regards,
> > Pavel
> >
>
> I assume you mean by using
>
> !!(imx258->csi2_flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK)
>
> I can go ahead and use that instead
Yes, I'd do that.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] arm64: dts: rockchip: add Protonic MECSBC device-tree
From: Sascha Hauer @ 2024-04-05 10:09 UTC (permalink / raw)
To: Andrew Lunn
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner,
devicetree, linux-arm-kernel, linux-rockchip, linux-kernel,
David Jander
In-Reply-To: <9d325b4e-031c-4f6c-9788-fa5a68470efa@lunn.ch>
On Thu, Apr 04, 2024 at 05:10:41PM +0200, Andrew Lunn wrote:
> > +&gmac1 {
> > + assigned-clocks = <&cru SCLK_GMAC1_RX_TX>, <&cru SCLK_GMAC1>;
> > + assigned-clock-parents = <&cru SCLK_GMAC1_RGMII_SPEED>, <&cru CLK_MAC1_2TOP>;
> > + phy-handle = <&rgmii_phy1>;
> > + phy-mode = "rgmii";
> > + clock_in_out = "output";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&gmac1m1_miim
> > + &gmac1m1_tx_bus2
> > + &gmac1m1_rx_bus2
> > + &gmac1m1_rgmii_clk
> > + &gmac1m1_clkinout
> > + &gmac1m1_rgmii_bus>;
> > + status = "okay";
> > + tx_delay = <0x30>;
> > + rx_delay = <0x10>;
> > +};
>
> There was a discussion about phy-mode = "rgmii"; and these
> tx/rx_delays last month. Please could you go read that discussion and
> them make use of rgmii-id, and change the delays.
Ok, I'll switch to rgmii-id.
>
> Also, where did you copy this from? If possible, it would be good to
> fix the example everybody copies into new DT blobs.
These are the default values used in over a dozen boards and a also
given in the example in
Documentation/devicetree/bindings/net/rockchip-dwmac.yaml.
These are also the default values the driver uses when tx_delay and
rx_delay are not given in the device tree.
I can prepare a patch to fix the example.
Do you have a pointer why setting the delays in the phy is preferred
over setting them in the network driver? In the end this requires us
to have the correct phy driver whereas setting them in the network
driver would just work for any phy driver?
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* [PATCH v2 2/2] arm64: dts: rockchip: add Protonic MECSBC device-tree
From: Sascha Hauer @ 2024-04-05 10:14 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner
Cc: devicetree, linux-arm-kernel, linux-rockchip, linux-kernel,
David Jander, Sascha Hauer
In-Reply-To: <20240405-protonic-mecsbc-v2-0-0a6fedc78b9f@pengutronix.de>
From: David Jander <david@protonic.nl>
MECSBC is a single board computer for blood analysis machines from
RR-Mechatronics, designed and manufactured by Protonic Holland, based on
the Rockchip RK3568 SoC.
Signed-off-by: David Jander <david@protonic.nl>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
arch/arm64/boot/dts/rockchip/Makefile | 1 +
arch/arm64/boot/dts/rockchip/rk3568-mecsbc.dts | 404 +++++++++++++++++++++++++
2 files changed, 405 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
index f906a868b71ac..51c242c462b04 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -100,6 +100,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-evb1-v10.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-fastrhino-r66s.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-fastrhino-r68s.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-lubancat-2.dtb
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-mecsbc.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-nanopi-r5c.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-nanopi-r5s.dtb
dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-odroid-m1.dtb
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-mecsbc.dts b/arch/arm64/boot/dts/rockchip/rk3568-mecsbc.dts
new file mode 100644
index 0000000000000..edd54c5932337
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3568-mecsbc.dts
@@ -0,0 +1,404 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+
+/dts-v1/;
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/leds/common.h>
+#include <dt-bindings/pinctrl/rockchip.h>
+#include <dt-bindings/pwm/pwm.h>
+#include "rk3568.dtsi"
+
+/ {
+ model = "Protonic MECSBC";
+ compatible = "prt,mecsbc", "rockchip,rk3568";
+
+ aliases {
+ mmc0 = &sdhci;
+ mmc1 = &sdmmc0;
+ };
+
+ chosen: chosen {
+ stdout-path = "serial2:1500000n8";
+ };
+
+ tas2562-sound {
+ compatible = "simple-audio-card";
+ simple-audio-card,format = "i2s";
+ simple-audio-card,name = "Speaker";
+ simple-audio-card,mclk-fs = <256>;
+
+ simple-audio-card,cpu {
+ sound-dai = <&i2s1_8ch>;
+ };
+
+ simple-audio-card,codec {
+ sound-dai = <&tas2562>;
+ };
+ };
+
+ vdd_gpu: regulator-vdd-gpu {
+ compatible = "pwm-regulator";
+ pwms = <&pwm1 0 5000 PWM_POLARITY_INVERTED>;
+ regulator-name = "vdd_gpu";
+ regulator-min-microvolt = <915000>;
+ regulator-max-microvolt = <1000000>;
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-settling-time-up-us = <250>;
+ pwm-dutycycle-range = <0 100>; /* dutycycle inverted 0% => 0.915V */
+ };
+
+ vcc_sd: regulator-sd {
+ compatible = "regulator-gpio";
+ enable-gpios = <&gpio0 RK_PA5 GPIO_ACTIVE_HIGH>;
+ enable-active-high;
+ gpios = <&gpio0 RK_PA6 GPIO_ACTIVE_HIGH>;
+ regulator-name = "sdcard-gpio-supply";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <3300000>;
+ states = <1800000 0x1>, <3300000 0x0>;
+ };
+
+ vdd_npu: regulator-vdd-npu {
+ compatible = "pwm-regulator";
+ pwms = <&pwm2 0 5000 PWM_POLARITY_INVERTED>;
+ regulator-name = "vdd_npu";
+ regulator-min-microvolt = <915000>;
+ regulator-max-microvolt = <1000000>;
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-settling-time-up-us = <250>;
+ pwm-dutycycle-range = <0 100>; /* dutycycle inverted 0% => 0.915V */
+ };
+
+ p3v3: regulator-p3v3 {
+ compatible = "regulator-fixed";
+ regulator-name = "p3v3";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ };
+
+ p1v8: regulator-p1v8 {
+ compatible = "regulator-fixed";
+ regulator-name = "p1v8";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+};
+
+&combphy0 {
+ status = "okay";
+};
+
+&combphy1 {
+ status = "okay";
+};
+
+&combphy2 {
+ status = "okay";
+};
+
+&cpu0 {
+ cpu-supply = <&vdd_cpu>;
+};
+
+&cpu1 {
+ cpu-supply = <&vdd_cpu>;
+};
+
+&cpu2 {
+ cpu-supply = <&vdd_cpu>;
+};
+
+&cpu3 {
+ cpu-supply = <&vdd_cpu>;
+};
+
+&gmac1 {
+ assigned-clocks = <&cru SCLK_GMAC1_RX_TX>, <&cru SCLK_GMAC1>;
+ assigned-clock-parents = <&cru SCLK_GMAC1_RGMII_SPEED>, <&cru CLK_MAC1_2TOP>;
+ phy-handle = <&rgmii_phy1>;
+ phy-mode = "rgmii-id";
+ clock_in_out = "output";
+ pinctrl-names = "default";
+ pinctrl-0 = <&gmac1m1_miim
+ &gmac1m1_tx_bus2
+ &gmac1m1_rx_bus2
+ &gmac1m1_rgmii_clk
+ &gmac1m1_clkinout
+ &gmac1m1_rgmii_bus>;
+ status = "okay";
+};
+
+&gpu {
+ mali-supply = <&vdd_gpu>;
+ status = "okay";
+};
+
+&i2c0 {
+ status = "okay";
+
+ vdd_cpu: regulator@60 {
+ compatible = "fcs,fan53555";
+ reg = <0x60>;
+ fcs,suspend-voltage-selector = <1>;
+ regulator-name = "vdd_cpu";
+ regulator-always-on;
+ regulator-boot-on;
+ regulator-min-microvolt = <800000>;
+ regulator-max-microvolt = <1150000>;
+ regulator-ramp-delay = <2300>;
+
+ regulator-state-mem {
+ regulator-off-in-suspend;
+ };
+ };
+};
+
+&i2c2 {
+ status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <&i2c2m0_xfer>;
+};
+
+&i2c3 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&i2c3m0_xfer>;
+ status = "okay";
+
+ tas2562: amplifier@4c {
+ compatible = "ti,tas2562";
+ reg = <0x4c>;
+ #sound-dai-cells = <0>;
+ shutdown-gpios = <&gpio1 RK_PD4 GPIO_ACTIVE_HIGH>;
+ interrupt-parent = <&gpio1>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_tas2562>;
+ interrupts = <RK_PD1 IRQ_TYPE_LEVEL_LOW>;
+ ti,imon-slot-no = <0>;
+ };
+};
+
+&i2c5 {
+ status = "okay";
+
+ temperature-sensor@48 {
+ compatible = "ti,tmp1075";
+ reg = <0x48>;
+ };
+
+ rtc@51 {
+ compatible = "nxp,pcf85363";
+ reg = <0x51>;
+ #clock-cells = <0>;
+ clock-output-names = "rtcic_32kout";
+ };
+};
+
+&i2s1_8ch {
+ pinctrl-names = "default";
+ pinctrl-0 = <&i2s1m0_sclktx &i2s1m0_lrcktx &i2s1m0_sdi0 &i2s1m0_sdo0>;
+ rockchip,trcm-sync-tx-only;
+ status = "okay";
+};
+
+&mdio1 {
+ rgmii_phy1: ethernet-phy@2 {
+ compatible = "ethernet-phy-ieee802.3-c22";
+ reg = <0x2>;
+ pinctrl-names = "default";
+ pinctrl-0 = <ð_phy1_rst>;
+ reset-assert-us = <20000>;
+ reset-deassert-us = <100000>;
+ reset-gpios = <&gpio4 RK_PB3 GPIO_ACTIVE_LOW>;
+ };
+};
+
+&pcie2x1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pcie20m1_pins>;
+ reset-gpios = <&gpio3 RK_PC1 GPIO_ACTIVE_HIGH>;
+ status = "okay";
+};
+
+&pcie30phy {
+ status = "okay";
+};
+
+&pcie3x2 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pcie30x2m1_pins>;
+ reset-gpios = <&gpio2 RK_PD6 GPIO_ACTIVE_HIGH>;
+ vpcie3v3-supply = <&p3v3>;
+ status = "okay";
+};
+
+&pinctrl {
+ ethernet {
+ eth_phy1_rst: eth-phy1-rst {
+ rockchip,pins = <4 RK_PB3 RK_FUNC_GPIO &pcfg_pull_none>;
+ };
+ };
+
+ tas2562 {
+ pinctrl_tas2562: tas2562 {
+ rockchip,pins = <1 RK_PD4 RK_FUNC_GPIO &pcfg_pull_up>;
+ };
+ };
+};
+
+&pmu_io_domains {
+ pmuio1-supply = <&p3v3>;
+ pmuio2-supply = <&p3v3>;
+ vccio1-supply = <&p1v8>;
+ vccio2-supply = <&p1v8>;
+ vccio3-supply = <&vcc_sd>;
+ vccio4-supply = <&p1v8>;
+ vccio5-supply = <&p3v3>;
+ vccio6-supply = <&p1v8>;
+ vccio7-supply = <&p3v3>;
+ status = "okay";
+};
+
+&saradc {
+ vref-supply = <&p1v8>;
+ status = "okay";
+};
+
+&sdhci {
+ bus-width = <8>;
+ max-frequency = <200000000>;
+ non-removable;
+ pinctrl-names = "default";
+ pinctrl-0 = <&emmc_bus8 &emmc_clk &emmc_cmd &emmc_datastrobe>;
+ vmmc-supply = <&p3v3>;
+ vqmmc-supply = <&p1v8>;
+ mmc-hs200-1_8v;
+ non-removable;
+ no-sd;
+ no-sdio;
+ status = "okay";
+};
+
+&sdmmc0 {
+ bus-width = <4>;
+ cap-sd-highspeed;
+ cd-gpios = <&gpio0 RK_PA4 GPIO_ACTIVE_LOW>;
+ disable-wp;
+ pinctrl-names = "default";
+ pinctrl-0 = <&sdmmc0_bus4 &sdmmc0_clk &sdmmc0_cmd &sdmmc0_det>;
+ sd-uhs-sdr50;
+ sd-uhs-sdr104;
+ vmmc-supply = <&p3v3>;
+ vqmmc-supply = <&vcc_sd>;
+ status = "okay";
+};
+
+&tsadc {
+ rockchip,hw-tshut-mode = <1>;
+ rockchip,hw-tshut-polarity = <0>;
+ status = "okay";
+};
+
+&uart2 {
+ status = "okay";
+};
+
+&usb_host0_ehci {
+ status = "okay";
+};
+
+&usb_host0_ohci {
+ status = "okay";
+};
+
+&usb_host0_xhci {
+ extcon = <&usb2phy0>;
+ status = "okay";
+ dr_mode = "host";
+};
+
+&usb_host1_ehci {
+ status = "okay";
+};
+
+&usb_host1_ohci {
+ status = "okay";
+};
+
+&usb_host1_xhci {
+ status = "okay";
+};
+
+&usb2phy0 {
+ status = "okay";
+};
+
+&usb2phy0_host {
+ status = "okay";
+};
+
+&usb2phy0_otg {
+ status = "okay";
+};
+
+&usb2phy1 {
+ status = "okay";
+};
+
+&usb2phy1_host {
+ status = "okay";
+};
+
+&usb2phy1_otg {
+ status = "okay";
+};
+
+&pwm1 {
+ status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pwm1m0_pins>;
+};
+
+&pwm2 {
+ status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pwm2m0_pins>;
+};
+
+&gpu_opp_table {
+ compatible = "operating-points-v2";
+
+ opp-200000000 {
+ opp-hz = /bits/ 64 <200000000>;
+ opp-microvolt = <915000>;
+ };
+
+ opp-300000000 {
+ opp-hz = /bits/ 64 <300000000>;
+ opp-microvolt = <915000>;
+ };
+
+ opp-400000000 {
+ opp-hz = /bits/ 64 <400000000>;
+ opp-microvolt = <915000>;
+ };
+
+ opp-600000000 {
+ opp-hz = /bits/ 64 <600000000>;
+ opp-microvolt = <920000>;
+ };
+
+ opp-700000000 {
+ opp-hz = /bits/ 64 <700000000>;
+ opp-microvolt = <950000>;
+ };
+
+ opp-800000000 {
+ opp-hz = /bits/ 64 <800000000>;
+ opp-microvolt = <1000000>;
+ };
+};
--
2.39.2
^ permalink raw reply related
* [PATCH v2 1/2] dt-bindings: arm: rockchip: Add Protonic MECSBC board
From: Sascha Hauer @ 2024-04-05 10:14 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner
Cc: devicetree, linux-arm-kernel, linux-rockchip, linux-kernel,
David Jander, Sascha Hauer, Krzysztof Kozlowski
In-Reply-To: <20240405-protonic-mecsbc-v2-0-0a6fedc78b9f@pengutronix.de>
MECSBC is a single board computer for blood analysis machines from
RR-Mechatronics, designed and manufactured by Protonic Holland, based on
the Rockchip RK3568 SoC.
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
Documentation/devicetree/bindings/arm/rockchip.yaml | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/rockchip.yaml b/Documentation/devicetree/bindings/arm/rockchip.yaml
index fcf7316ecd74c..4847fe072d550 100644
--- a/Documentation/devicetree/bindings/arm/rockchip.yaml
+++ b/Documentation/devicetree/bindings/arm/rockchip.yaml
@@ -700,6 +700,11 @@ properties:
- powkiddy,x55
- const: rockchip,rk3566
+ - description: Protonic MECSBC board
+ items:
+ - const: prt,mecsbc
+ - const: rockchip,rk3568
+
- description: QNAP TS-433-4G 4-Bay NAS
items:
- const: qnap,ts433
--
2.39.2
^ permalink raw reply related
* [PATCH v2 0/2] Add Protonic MECSBC board support
From: Sascha Hauer @ 2024-04-05 10:14 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner
Cc: devicetree, linux-arm-kernel, linux-rockchip, linux-kernel,
David Jander, Sascha Hauer, Krzysztof Kozlowski
This series adds support for the Protonic MECSBC. MECSBC is a single
board computer for blood analysis machines from RR-Mechatronics,
designed and manufactured by Protonic Holland, based on the Rockchip
RK3568 SoC.
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
Changes in v2:
- use generic node names (Krzysztof)
- name regulators consistently (Krzysztof)
- do not use underscores in node names (Krzysztof)
- keep alphabetic order in arch/arm64/boot/dts/rockchip/Makefile (Heiko)
- use rgmii-id mode and drop [tr]x_delay (Andrew)
- add SD card regulator to support 1.8V modes
- Link to v1: https://lore.kernel.org/r/20240404-protonic-mecsbc-v1-0-ad5b42ade6c6@pengutronix.de
---
David Jander (1):
arm64: dts: rockchip: add Protonic MECSBC device-tree
Sascha Hauer (1):
dt-bindings: arm: rockchip: Add Protonic MECSBC board
.../devicetree/bindings/arm/rockchip.yaml | 5 +
arch/arm64/boot/dts/rockchip/Makefile | 1 +
arch/arm64/boot/dts/rockchip/rk3568-mecsbc.dts | 404 +++++++++++++++++++++
3 files changed, 410 insertions(+)
---
base-commit: 39cd87c4eb2b893354f3b850f916353f2658ae6f
change-id: 20240404-protonic-mecsbc-6cfa56a48fb7
Best regards,
--
Sascha Hauer <s.hauer@pengutronix.de>
^ permalink raw reply
* Re: [PATCH v8 3/7] PCI: qcom: Add ICC bandwidth vote for CPU to PCIe path
From: Krishna Chaitanya Chundru @ 2024-04-05 10:16 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: Bjorn Andersson, Konrad Dybcio, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Manivannan Sadhasivam,
Rob Herring, Johan Hovold, Brian Masney, Georgi Djakov,
linux-arm-msm, linux-pci, devicetree, linux-kernel, vireshk,
quic_vbadigan, quic_skananth, quic_nitegupt, quic_parass,
Bryan O'Donoghue
In-Reply-To: <20240405082944.GE2953@thinkpad>
On 4/5/2024 1:59 PM, Manivannan Sadhasivam wrote:
> On Tue, Mar 05, 2024 at 04:23:21PM +0530, Krishna Chaitanya Chundru wrote:
>>
>>
>> On 3/4/2024 11:11 PM, Manivannan Sadhasivam wrote:
>>> On Sat, Mar 02, 2024 at 09:29:57AM +0530, Krishna chaitanya chundru wrote:
>>>> To access PCIe registers, PCIe BAR space, config space the CPU-PCIe
>>>> ICC (interconnect consumers) path should be voted otherwise it may
>>>> lead to NoC (Network on chip) timeout. We are surviving because of
>>>> other driver vote for this path.
>>>>
>>>> As there is less access on this path compared to PCIe to mem path
>>>> add minimum vote i.e 1KBps bandwidth always.
>>>
>>> Please add the info that 1KBps is what shared by the HW team.
>>>
>> Ack to all the comments
>>>>
>>>> When suspending, disable this path after register space access
>>>> is done.
>>>>
>>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>>>> ---
>>>> drivers/pci/controller/dwc/pcie-qcom.c | 38 ++++++++++++++++++++++++++++++++--
>>>> 1 file changed, 36 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>>> index 10f2d0bb86be..a0266bfe71f1 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>
> [...]
>
>>>> + ret = icc_disable(pcie->icc_cpu);
>>>> + if (ret) {
>>>> + dev_err(dev, "failed to disable icc path of cpu-pcie: %d\n", ret);
>>>
>>> "CPU-PCIe"
>>>
>>>> + if (pcie->suspended) {
>>>> + qcom_pcie_host_init(&pcie->pci->pp);
>>>
>>> Interesting. So if icc_disable() fails, can the IP continue to function?
>>>
>> As the ICC already enable before icc_disable() fails, the IP should work.
>
> If icc_disable() fails, then most likely something is wrong with RPMh. How can
> the IP continue to work in that case?
>
Ok then I will log the error and return here
- Krishna Chaitanya.
> - Mani
>
^ permalink raw reply
* Re: [PATCH RFT 0/7] arm64: qcom: allow up to 4 lanes for the Type-C DisplayPort Altmode
From: Luca Weiss @ 2024-04-05 10:19 UTC (permalink / raw)
To: neil.armstrong, Konrad Dybcio, Bjorn Andersson
Cc: Vinod Koul, Kishon Vijay Abraham I, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Abhinav Kumar, linux-arm-msm,
linux-phy, devicetree, linux-kernel
In-Reply-To: <963b60e5-6ab7-4d9f-885a-ba744c2b7991@linaro.org>
On Fri Apr 5, 2024 at 10:08 AM CEST, Neil Armstrong wrote:
> Hi Luca,
>
> On 29/03/2024 10:02, Luca Weiss wrote:
> > On Tue Mar 26, 2024 at 10:02 PM CET, Konrad Dybcio wrote:
> >> On 16.03.2024 5:01 PM, Bjorn Andersson wrote:
> >>> On Fri, Mar 15, 2024 at 06:35:15PM +0100, Neil Armstrong wrote:
> >>>> On 15/03/2024 18:19, Luca Weiss wrote:
> >>>>> On Thu Feb 29, 2024 at 2:07 PM CET, Neil Armstrong wrote:
> >>>>>> Register a typec mux in order to change the PHY mode on the Type-C
> >>>>>> mux events depending on the mode and the svid when in Altmode setup.
> >>>>>>
> >>>>>> The DisplayPort phy should be left enabled if is still powered on
> >>>>>> by the DRM DisplayPort controller, so bail out until the DisplayPort
> >>>>>> PHY is not powered off.
> >>>>>>
> >>>>>> The Type-C Mode/SVID only changes on plug/unplug, and USB SAFE states
> >>>>>> will be set in between of USB-Only, Combo and DisplayPort Only so
> >>>>>> this will leave enough time to the DRM DisplayPort controller to
> >>>>>> turn of the DisplayPort PHY.
> >>>>>>
> >>>>>> The patchset also includes bindings changes and DT changes.
> >>>>>>
> >>>>>> This has been successfully tested on an SM8550 board, but the
> >>>>>> Thinkpad X13s deserved testing between non-PD USB, non-PD DisplayPort,
> >>>>>> PD USB Hubs and PD Altmode Dongles to make sure the switch works
> >>>>>> as expected.
> >>>>>>
> >>>>>> The DisplayPort 4 lanes setup can be check with:
> >>>>>> $ cat /sys/kernel/debug/dri/ae01000.display-controller/DP-1/dp_debug
> >>>>>> name = msm_dp
> >>>>>> drm_dp_link
> >>>>>> rate = 540000
> >>>>>> num_lanes = 4
> >>>>>
> >>>>> Hi Neil,
> >>>>>
> >>>>> I tried this on QCM6490/SC7280 which should also support 4-lane DP but I
> >>>>> haven't had any success so far.
> >>>>>
> >>> [..]
> >>>>> [ 1775.563969] [drm:dp_ctrl_link_train] *ERROR* max v_level reached
> >>>>> [ 1775.564031] [drm:dp_ctrl_link_train] *ERROR* link training #1 failed. ret=-11
> >>>>
> >>>> Interesting #1 means the 4 lanes are not physically connected to the other side,
> >>>> perhaps QCM6490/SC7280 requires a specific way to enable the 4 lanes in the PHY,
> >>>> or some fixups in the init tables.
> >>>>
> >>>
> >>> I tested the same on rb3gen2 (qcs6490) a couple of weeks ago, with the
> >>> same outcome. Looking at the AUX reads, after switching to 4-lane the
> >>> link training is failing on all 4 lanes, in contrast to succeeding only
> >>> on the first 2 if you e.g. forget to mux the other two.
> >>>
> >>> As such, my expectation is that there's something wrong in the QMP PHY
> >>> (or possibly redriver) for this platform.
> >>
> >> Do we have any downstream tag where 4lane dp works? I'm willing to believe
> >> the PHY story..
> >
> > Just tested on Fairphone 5 downstream and 4 lane appears to work there.
> > This is with an USB-C to HDMI adapter that only does HDMI.
> >
> > FP5:/ # cat /sys/kernel/debug/drm_dp/dp_debug
> > state=0x20a5
> > link_rate=270000
> > num_lanes=4
> > resolution=2560x1440@60Hz
> > pclock=241500KHz
> > bpp=24
> > test_req=DP_LINK_STATUS_UPDATED
> > lane_count=4
> > bw_code=10
> > v_level=0
> > p_level=0
> >
> > Sources are here:
> > https://gerrit-public.fairphone.software/plugins/gitiles/kernel/msm-5.4/+/refs/heads/odm/rc/target/13/fp5
> > And probably more importantly techpack/display:
> > https://gerrit-public.fairphone.software/plugins/gitiles/platform/vendor/opensource/display-drivers/+/refs/heads/odm/rc/target/13/fp5
> > Dts if useful:
> > https://gerrit-public.fairphone.software/plugins/gitiles/kernel/msm-extra/devicetree/+/refs/heads/kernel/13/fp5
>
> Could you retry with this applied ?
>
> https://lore.kernel.org/all/20240405000111.1450598-1-swboyd@chromium.org/
Unfortunately I do not see any change with this on QCM6490 Fairphone 5
and 4-lane DP.
Regards
Luca
>
> Thanks,
> Neil
>
> >
> > Regards
> > Luca
> >
> >>
> >> Konrad
> >
^ permalink raw reply
* Re: [PATCH v3 18/25] dt-bindings: media: imx258: Add alternate compatible strings
From: Dave Stevenson @ 2024-04-05 10:25 UTC (permalink / raw)
To: Conor Dooley
Cc: git, linux-media, jacopo.mondi, mchehab, robh,
krzysztof.kozlowski+dt, conor+dt, shawnguo, s.hauer, kernel,
festevam, sakari.ailus, devicetree, imx, linux-arm-kernel,
linux-kernel, pavel, phone-devel
In-Reply-To: <20240403-vista-defendant-ebadbaa52059@spud>
Hi Conor
On Wed, 3 Apr 2024 at 17:14, Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, Apr 03, 2024 at 09:03:47AM -0600, git@luigi311.com wrote:
> > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >
> > There are a number of variants of the imx258 modules that can not
> > be differentiated at runtime, so add compatible strings for the
> > PDAF variant.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Signed-off-by: Luis Garcia <git@luigi311.com>
> > ---
> > .../devicetree/bindings/media/i2c/sony,imx258.yaml | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
> > index bee61a443b23..c978abc0cdb3 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
> > @@ -13,11 +13,16 @@ description: |-
> > IMX258 is a diagonal 5.867mm (Type 1/3.06) 13 Mega-pixel CMOS active pixel
> > type stacked image sensor with a square pixel array of size 4208 x 3120. It
> > is programmable through I2C interface. Image data is sent through MIPI
> > - CSI-2.
> > + CSI-2. The sensor exists in two different models, a standard variant
> > + (IMX258) and a variant with phase detection autofocus (IMX258-PDAF).
> > + The camera module does not expose the model through registers, so the
> > + exact model needs to be specified.
> >
> > properties:
> > compatible:
> > - const: sony,imx258
> > + enum:
> > + - sony,imx258
> > + - sony,imx258-pdaf
>
> Does the pdaf variant support all of the features/is it register
> compatible with the regular variant? If it is, the regular variant
> should be a fallback compatible.
It has the same register set, but certain registers have to be
programmed differently so that the image is corrected for the
partially shielded pixels used for phase detect auto focus (PDAF).
Either compatible will "work" on either variant of the module, but
you'll get weird image artifacts when using the wrong one.
Dave
> Cheers,
> Conor.
^ permalink raw reply
* Re: [PATCH v3 2/2] mailbox: arm_mhuv3: Add driver
From: Jonathan Cameron @ 2024-04-05 10:32 UTC (permalink / raw)
To: Cristian Marussi
Cc: linux-kernel, linux-arm-kernel, devicetree, sudeep.holla,
jassisinghbrar, robh+dt, krzysztof.kozlowski+dt, conor+dt
In-Reply-To: <20240404062347.3219795-3-cristian.marussi@arm.com>
On Thu, 4 Apr 2024 07:23:47 +0100
Cristian Marussi <cristian.marussi@arm.com> wrote:
> Add support for ARM MHUv3 mailbox controller.
>
> Support is limited to the MHUv3 Doorbell extension using only the PBX/MBX
> combined interrupts.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
Drive by review (I was curious what this was :)
> diff --git a/drivers/mailbox/arm_mhuv3.c b/drivers/mailbox/arm_mhuv3.c
> new file mode 100644
> index 000000000000..e4125568bec0
> --- /dev/null
> +++ b/drivers/mailbox/arm_mhuv3.c
> @@ -0,0 +1,1063 @@
> +struct dummy_page {
> + u8 pad[0x1000];
> +} __packed;
> +
> +struct mhu3_pbx_frame_reg {
> + struct ctrl_page ctrl;
> + struct pdbcw_page dbcw[MHUV3_DBCW_MAX];
> + struct dummy_page ffcw;
> + struct dummy_page fcw;
> + u8 pad[0xF000 - 0x4000];
> + struct dummy_page impdef;
> +} __packed;
> +
> +struct mhu3_mbx_frame_reg {
> + struct ctrl_page ctrl;
> + struct mdbcw_page dbcw[MHUV3_DBCW_MAX];
> + struct dummy_page ffcw;
> + struct dummy_page fcw;
> + u8 pad[0xF000 - 0x4000];
Magic, numbers, Maybe give them a definition or base them on something
meaningful such as structure offsets?
> + struct dummy_page impdef;
> +} __packed;
> +
> +/* Macro for reading a bitfield within a physically mapped packed struct */
> +#define readl_relaxed_bitfield(_regptr, _field) \
> + ({ \
> + u32 _rval; \
> + typeof(_regptr) _rptr = _regptr; \
> + _rval = readl_relaxed(_rptr); \
> + ((typeof(*_rptr) __force *)(&_rval))->_field; \
> + })
> +
> +/* Macro for writing a bitfield within a physically mapped packed struct */
> +#define writel_relaxed_bitfield(_value, _regptr, _field) \
> + ({ \
> + u32 _rval; \
> + typeof(_regptr) _rptr = _regptr; \
> + _rval = readl_relaxed(_rptr); \
> + ((typeof(*_rptr) __force *)(&_rval))->_field = _value; \
> + writel_relaxed(_rval, _rptr); \
> + })
Similar, yet slightly different from ones in arm_mhuv2.c? Why the differences
and can these be shared code in a suitable header?
> +
> +/* ====== MHUv3 data structures ====== */
> +
> +enum mhuv3_frame {
> + PBX_FRAME,
> + MBX_FRAME
Trailing commas for last entries in enums unless they are in some sense terminators.
> +};
> +
> +static char *mhuv3_str[] = {
> + "PBX",
> + "MBX"
> +};
> +
> +enum mhuv3_extension_type {
> + FIRST_EXT = 0,
As mentioned inline, 0 is kind of default assumption for first so I wouldn't define it.
> + DBE_EXT = FIRST_EXT,
> + FCE_EXT,
> + FE_EXT,
> + MAX_EXT
That's one past normal meeting of MAX, maybe call it COUNT, or NUM?
> +};
> +static int mhuv3_doorbell_send_data(struct mhuv3 *mhu, struct mbox_chan *chan,
> + void *arg)
> +{
> + int ret = 0;
> + struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> + struct mhuv3_extension *e = mhu->ext[DBE_EXT];
> + unsigned long flags;
> +
> + spin_lock_irqsave(&e->pending_lock, flags);
guard() then you can do earlier returns and end up with cleaner code.
> + /* Only one in-flight Transfer is allowed per-doorbell */
> + if (!(e->pending_db[priv->ch_idx] & BIT(priv->doorbell))) {
> + e->pending_db[priv->ch_idx] |= BIT(priv->doorbell);
> + writel_relaxed(BIT(priv->doorbell),
> + &mhu->pbx->dbcw[priv->ch_idx].set);
> + } else {
> + ret = -EBUSY;
> + }
> + spin_unlock_irqrestore(&e->pending_lock, flags);
> +
> + return ret;
> +}
>
> +
> +static struct mbox_chan *mhuv3_dbe_chan_from_comb_irq_get(struct mhuv3 *mhu)
> +{
> + int i;
> + struct mhuv3_extension *e = mhu->ext[DBE_EXT];
> + struct device *dev = mhu->mbox.dev;
> +
> + for (i = 0; i < MHUV3_DBCH_CMB_INT_ST_REG_CNT; i++) {
> + unsigned int channel, db = MHUV3_INVALID_DOORBELL;
> + u32 cmb_st, st;
> +
> + cmb_st = readl_relaxed(&mhu->ctrl->dbch_int_st[i]);
> + if (!cmb_st)
> + continue;
> +
> + channel = i * MHUV3_STAT_BITS + __builtin_ctz(cmb_st);
> + if (channel >= e->max_chans) {
> + dev_err(dev, "Invalid %s channel:%d\n",
> + mhuv3_str[mhu->frame], channel);
return here rather than breaking out the loop. It is easier to follow
given nothing is done after the loop
> + break;
> + }
> +
> + if (mhu->frame == PBX_FRAME) {
> + unsigned long flags;
> + u32 active_dbs, fired_dbs;
> +
> + st = readl_relaxed_bitfield(&mhu->pbx->dbcw[channel].int_st,
> + tfr_ack);
> + if (!st) {
> + dev_warn(dev, "Spurios IRQ on %s channel:%d\n",
Spell check. Spurious.
> + mhuv3_str[mhu->frame], channel);
> + continue;
> + }
> +
> + active_dbs = readl_relaxed(&mhu->pbx->dbcw[channel].st);
> + spin_lock_irqsave(&e->pending_lock, flags);
> + fired_dbs = e->pending_db[channel] & ~active_dbs;
> + if (fired_dbs) {
> + db = __builtin_ctz(fired_dbs);
> + e->pending_db[channel] &= ~BIT(db);
> + fired_dbs &= ~BIT(db);
> + }
> + spin_unlock_irqrestore(&e->pending_lock, flags);
> +
> + /* Clear TFR Ack if no more doorbells pending */
> + if (!fired_dbs)
> + writel_relaxed_bitfield(0x1,
> + &mhu->pbx->dbcw[channel].int_clr,
> + tfr_ack);
> + } else {
> + st = readl_relaxed(&mhu->mbx->dbcw[channel].st_msk);
> + if (!st) {
> + dev_warn(dev, "Spurios IRQ on %s channel:%d\n",
> + mhuv3_str[mhu->frame], channel);
> + continue;
> + }
> + db = __builtin_ctz(st);
> + }
> +
> + if (db != MHUV3_INVALID_DOORBELL) {
> + dev_dbg(dev, "Found %s ch[%d]/db[%d]\n",
> + mhuv3_str[mhu->frame], channel, db);
> +
> + return &mhu->mbox.chans[channel * MHUV3_STAT_BITS + db];
> + }
> + }
> +
> + return ERR_PTR(-EIO);
> +}
> +
> +static int mhuv3_dbe_init(struct mhuv3 *mhu)
> +{
> + struct mhuv3_extension *e;
> + struct device *dev = mhu->mbox.dev;
> +
> + if (!readl_relaxed_bitfield(&mhu->ctrl->feat_spt0, dbe_spt))
> + return 0;
> +
> + dev_dbg(dev, "%s: Initializing DBE Extension.\n", mhuv3_str[mhu->frame]);
> +
> + e = devm_kzalloc(dev, sizeof(*e), GFP_KERNEL);
> + if (!e)
> + return -ENOMEM;
> +
> + e->type = DBE_EXT;
> + /* Note that, by the spec, the number of channels is (num_dbch + 1) */
> + e->max_chans =
> + readl_relaxed_bitfield(&mhu->ctrl->dbch_cfg0, num_dbch) + 1;
> + e->mbox_of_xlate = mhuv3_dbe_mbox_of_xlate;
> + e->combined_irq_setup = mhuv3_dbe_combined_irq_setup;
> + e->channels_init = mhuv3_dbe_channels_init;
> + e->chan_from_comb_irq_get = mhuv3_dbe_chan_from_comb_irq_get;
> +
> + mhu->tot_chans += e->max_chans * MHUV3_STAT_BITS;
> + mhu->ext[DBE_EXT] = e;
> +
> + dev_info(dev, "%s: found %d DBE channels.\n",
> + mhuv3_str[mhu->frame], e->max_chans);
dev_dbg() probably more appropriate.
> +
> + return 0;
> +}
> +
> +static int mhuv3_fce_init(struct mhuv3 *mhu)
> +{
> + struct device *dev = mhu->mbox.dev;
> +
> + if (!readl_relaxed_bitfield(&mhu->ctrl->feat_spt0, fce_spt))
> + return 0;
> +
> + dev_dbg(dev, "%s: FCE Extension not supported by driver.\n",
> + mhuv3_str[mhu->frame]);
> +
> + return 0;
> +}
> +
> +static int mhuv3_fe_init(struct mhuv3 *mhu)
> +{
> + struct device *dev = mhu->mbox.dev;
> +
> + if (!readl_relaxed_bitfield(&mhu->ctrl->feat_spt0, fe_spt))
> + return 0;
> +
> + dev_dbg(dev, "%s: FE Extension not supported by driver.\n",
> + mhuv3_str[mhu->frame]);
> +
> + return 0;
> +}
> +
> +static mhuv3_extension_initializer mhuv3_extension_init[MAX_EXT] = {
> + mhuv3_dbe_init,
> + mhuv3_fce_init,
> + mhuv3_fe_init,
> +};
> +
> +static int mhuv3_initialize_channels(struct device *dev, struct mhuv3 *mhu)
> +{
> + int i, ret = 0;
> + struct mbox_controller *mbox = &mhu->mbox;
> +
> + mbox->chans = devm_kcalloc(dev, mhu->tot_chans,
> + sizeof(*mbox->chans), GFP_KERNEL);
> + if (!mbox->chans)
> + return -ENOMEM;
> +
> + for (i = FIRST_EXT; i < MAX_EXT && !ret; i++)
Why this dance with FIRST_EXT if it is always 0? Cleaner to just use 0.
> + if (mhu->ext[i])
> + ret = mhu->ext[i]->channels_init(mhu);
> +
> + return ret;
> +}
> +
> +static struct mbox_chan *mhuv3_mbox_of_xlate(struct mbox_controller *mbox,
> + const struct of_phandle_args *pa)
> +{
> + unsigned int type, channel, param;
> + struct mhuv3 *mhu = mhu_from_mbox(mbox);
> +
> + if (pa->args_count != MHUV3_MBOX_CELLS)
> + return ERR_PTR(-EINVAL);
> +
> + type = pa->args[MHUV3_MBOX_CELL_TYPE];
> + if (type >= MAX_EXT)
> + return ERR_PTR(-EINVAL);
> +
> + channel = pa->args[MHUV3_MBOX_CELL_CHWN];
> + param = pa->args[MHUV3_MBOX_CELL_PARAM];
> +
> + return mhu->ext[type]->mbox_of_xlate(mhu, channel, param);
> +}
> +
> +static int mhuv3_frame_init(struct mhuv3 *mhu, void __iomem *regs)
> +{
> + int i, ret = 0;
> + struct device *dev = mhu->mbox.dev;
> +
> + mhu->ctrl = regs;
> + mhu->frame = readl_relaxed_bitfield(&mhu->ctrl->blk_id, blk_id);
> + if (mhu->frame > MBX_FRAME) {
> + dev_err(dev, "Invalid Frame type- %d\n", mhu->frame);
> + return -EINVAL;
dev_err_probe() etc (see later)
> + }
> +
> + mhu->major = readl_relaxed_bitfield(&mhu->ctrl->aidr, arch_major_rev);
> + mhu->minor = readl_relaxed_bitfield(&mhu->ctrl->aidr, arch_minor_rev);
> + if (mhu->major != MHUV3_MAJOR_VERSION) {
> + dev_warn(dev, "Unsupported MHU %s block - major:%d minor:%d\n",
> + mhuv3_str[mhu->frame], mhu->major, mhu->minor);
You are treating it as an error, so why only a warning?
> + return -EINVAL;
> + }
> + mhu->auto_op_full = !!readl_relaxed_bitfield(&mhu->ctrl->feat_spt1,
> + auto_op_spt);
> + /* Request the PBX/MBX to remain operational */
> + if (mhu->auto_op_full)
> + writel_relaxed_bitfield(0x1, &mhu->ctrl->ctrl, op_req);
> +
> + dev_dbg(dev, "Found MHU %s block - major:%d minor:%d\n",
> + mhuv3_str[mhu->frame], mhu->major, mhu->minor);
> +
> + if (mhu->frame == PBX_FRAME)
> + mhu->pbx = regs;
> + else
> + mhu->mbx = regs;
> +
> + for (i = FIRST_EXT; i < MAX_EXT && !ret; i++)
> + ret = mhuv3_extension_init[i](mhu);
Only dbe_init() returns any errors, so if I ready this correctly you always
eat that error.
> +
> + return ret;
> +}
> +
> +static irqreturn_t mhuv3_pbx_comb_interrupt(int irq, void *arg)
> +{
> + int ret = IRQ_NONE;
> + unsigned int i, found = 0;
> + struct mhuv3 *mhu = arg;
> + struct device *dev = mhu->mbox.dev;
> + struct mbox_chan *chan;
> +
> + for (i = FIRST_EXT; i < MAX_EXT; i++) {
> + /* FCE does not participate to the PBX combined */
> + if (i == FCE_EXT || !mhu->ext[i])
> + continue;
> +
> + chan = mhu->ext[i]->chan_from_comb_irq_get(mhu);
> + if (!IS_ERR(chan)) {
if (IS_ERR(chan))
continue;
will reduce indent and give more readable code.
> + struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> +
> + found++;
> + if (chan->cl) {
> + mbox_chan_txdone(chan, 0);
> + ret = IRQ_HANDLED;
> + } else {
> + dev_warn(dev,
> + "TX Ack on UNBOUND channel (%u)\n",
> + priv->ch_idx);
> + }
> + }
> + }
> +
> + if (!found)
> + dev_warn_once(dev, "Failed to find channel for the TX interrupt\n");
> +
> + return ret;
> +}
> +
> +static irqreturn_t mhuv3_mbx_comb_interrupt(int irq, void *arg)
> +{
> + int ret = IRQ_NONE;
> + unsigned int i, found = 0;
> + struct mhuv3 *mhu = arg;
> + struct device *dev = mhu->mbox.dev;
> + struct mbox_chan *chan;
> +
> + for (i = FIRST_EXT; i < MAX_EXT; i++) {
> + if (!mhu->ext[i])
> + continue;
> +
> + /* Process any extension which could be source of the IRQ */
> + chan = mhu->ext[i]->chan_from_comb_irq_get(mhu);
> + if (!IS_ERR(chan)) {
if (IS_ERR(chan))
continue;
is going to be easier to read.
> + void *data = NULL;
> + struct mhuv3_mbox_chan_priv *priv = chan->con_priv;
> +
> + found++;
> + /* Read and acknowledge optional in-band LE data first. */
> + if (priv->ops->read_data)
> + data = priv->ops->read_data(mhu, chan);
> +
> + if (chan->cl && !IS_ERR(data)) {
> + mbox_chan_received_data(chan, data);
> + ret = IRQ_HANDLED;
> + } else if (!chan->cl) {
> + dev_warn(dev,
> + "RX Data on UNBOUND channel (%u)\n",
> + priv->ch_idx);
> + } else {
> + dev_err(dev, "Failed to read data: %lu\n",
> + PTR_ERR(data));
> + }
I'd be tempted to factor out this code block into another function as I think
that will allow you to deal with the errors more directly.
> +
> + if (!IS_ERR(data))
> + kfree(data);
> +
> + /*
> + * Acknowledge transfer after any possible optional
> + * out-of-band data has also been retrieved via
> + * mbox_chan_received_data().
> + */
> + if (priv->ops->rx_complete)
> + priv->ops->rx_complete(mhu, chan);
> + }
> + }
> +
> + if (!found)
> + dev_warn_once(dev, "Failed to find channel for the RX interrupt\n");
> +
> + return ret;
> +}
> +
> +static int mhuv3_setup_pbx(struct mhuv3 *mhu)
> +{
> + struct device *dev = mhu->mbox.dev;
> +
> + mhu->mbox.ops = &mhuv3_sender_ops;
> +
> + if (mhu->cmb_irq > 0) {
> + int ret;
> +
> + ret = devm_request_threaded_irq(dev, mhu->cmb_irq, NULL,
> + mhuv3_pbx_comb_interrupt,
> + IRQF_ONESHOT, "mhuv3-pbx", mhu);
> + if (!ret) {
> + int i;
> +
> + mhu->mbox.txdone_irq = true;
> + mhu->mbox.txdone_poll = false;
> +
> + for (i = FIRST_EXT; i < MAX_EXT; i++)
> + if (mhu->ext[i])
> + mhu->ext[i]->combined_irq_setup(mhu);
> +
> + dev_dbg(dev, "MHUv3 PBX IRQs initialized.\n");
> +
> + return 0;
> + }
> +
> + dev_err(dev, "Failed to request PBX IRQ - ret:%d", ret);
If an irq was provided and it failed, I'd just return an error, not muddle on.
Broken system. If it's not an 'error' then don't use dev_err()
Papering over this leads to an odd code flow with if (!ret) so it would
be nice not to bother unless there is a strong reason to carry on.
> + }
> +
> + dev_info(dev, "Using PBX in Tx polling mode.\n");
That's noisy. dev_dbg() perhaps?
> + mhu->mbox.txdone_irq = false;
> + mhu->mbox.txdone_poll = true;
> + mhu->mbox.txpoll_period = 1;
> +
> + return 0;
> +}
> +
> +static int mhuv3_setup_mbx(struct mhuv3 *mhu)
> +{
> + int ret, i;
> + struct device *dev = mhu->mbox.dev;
> +
> + mhu->mbox.ops = &mhuv3_receiver_ops;
> +
> + if (mhu->cmb_irq <= 0) {
> + dev_err(dev, "Missing MBX combined IRQ !\n");
return dev_err_probe()
here as I think it's only called form init. Sure you might not
need the deferred handling it provides but it still leads to
cleaner code and no one has to think about whether deferal might
happen or not.
> + return -EINVAL;
> + }
> +
> + ret = devm_request_threaded_irq(dev, mhu->cmb_irq, NULL,
> + mhuv3_mbx_comb_interrupt, IRQF_ONESHOT,
> + "mhuv3-mbx", mhu);
> + if (ret) {
> + dev_err(dev, "Failed to request MBX IRQ - ret:%d\n", ret);
> + return ret;
return dev_err_probe()
> + }
> +
> + for (i = FIRST_EXT; i < MAX_EXT; i++)
> + if (mhu->ext[i])
> + mhu->ext[i]->combined_irq_setup(mhu);
> +
> + dev_dbg(dev, "MHUv3 MBX IRQs initialized.\n");
> +
> + return ret;
> +}
> +
> +static int mhuv3_irqs_init(struct mhuv3 *mhu, struct platform_device *pdev)
> +{
> + int ret;
> +
> + dev_dbg(mhu->mbox.dev, "Initializing %s block.\n", mhuv3_str[mhu->frame]);
> +
> + if (mhu->frame == PBX_FRAME) {
> + mhu->cmb_irq = platform_get_irq_byname_optional(pdev, "combined");
> + ret = mhuv3_setup_pbx(mhu);
return early is both shorter and easier to follow if people
are looking at particular paths through the function.
> + } else {
> + mhu->cmb_irq = platform_get_irq_byname(pdev, "combined");
> + ret = mhuv3_setup_mbx(mhu);
> + }
> +
> + return ret;
> +}
> +
> +static int mhuv3_probe(struct platform_device *pdev)
> +{
> + int ret;
> + struct mhuv3 *mhu;
> + void __iomem *regs;
> + struct device *dev = &pdev->dev;
> +
> + mhu = devm_kzalloc(dev, sizeof(*mhu), GFP_KERNEL);
> + if (!mhu)
> + return -ENOMEM;
> +
> + regs = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(regs))
> + return PTR_ERR(regs);
> +
> + mhu->mbox.dev = dev;
> + ret = mhuv3_frame_init(mhu, regs);
> + if (ret)
> + return ret;
> +
> + ret = mhuv3_irqs_init(mhu, pdev);
> + if (ret)
> + return ret;
> +
> + mhu->mbox.of_xlate = mhuv3_mbox_of_xlate;
> + ret = mhuv3_initialize_channels(dev, mhu);
> + if (ret)
> + return ret;
> +
> + ret = devm_mbox_controller_register(dev, &mhu->mbox);
> + if (ret)
> + dev_err(dev, "failed to register ARM MHUv3 driver %d\n", ret);
Use dev_err_probe() to get a few things for free in probe time error messages message.
return dev_err_probe(dev, reg, "failed to register ARM HMUv3 driver\n");
return 0;
> +
> + platform_set_drvdata(pdev, mhu);
With all devm as suggested below, can I think drop this.
> +
> + return ret;
> +}
> +
> +static int mhuv3_remove(struct platform_device *pdev)
> +{
> + struct mhuv3 *mhu = platform_get_drvdata(pdev);
> +
> + if (mhu->auto_op_full)
> + writel_relaxed_bitfield(0x0, &mhu->ctrl->ctrl, op_req);
> +
From a quick glance probably better to use a
devm_add_action_or_reset() so that this is turned off at
equivalent place in remove() path as where it is turned on in _init()
Only register the callback if auto_op_full()
Mixing and matching devm_ and calls in remove is a path to weird
races and corner cases so better to go all in on devm handling.
> + return 0;
> +}
> +
> +static const struct of_device_id mhuv3_of_match[] = {
> + { .compatible = "arm,mhuv3", .data = NULL },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, mhuv3_of_match);
> +
> +static struct platform_driver mhuv3_driver = {
> + .driver = {
> + .name = "arm-mhuv3-mailbox",
> + .of_match_table = mhuv3_of_match,
> + },
> + .probe = mhuv3_probe,
> + .remove = mhuv3_remove,
> +};
> +module_platform_driver(mhuv3_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("ARM MHUv3 Driver");
> +MODULE_AUTHOR("Cristian Marussi <cristian.marussi@arm.com>");
^ 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