* [PATCH v2 0/2] Fix the regulator-state-standby definition
From: Andrei Simion @ 2024-04-04 12:38 UTC (permalink / raw)
To: robh, krzysztof.kozlowski+dt, conor+dt, nicolas.ferre,
alexandre.belloni, claudiu.beznea, mihai.sain
Cc: linux-arm-kernel, devicetree, linux-kernel, Andrei Simion
make dtbs_check DT_SCHEMA_FILES=microchip,mcp16502.yaml
at91-sama7g5ek.dtb: mcp16502@5b: regulators:VDD_(CORE|OTHER)|LDO[1-2]:
regulator-state-standby 'regulator-suspend-voltage' does not match any of
the regexes 'pinctrl-[0-9]+' from schema
$id: http://devicetree.org/schemas/regulator/microchip,mcp16502.yaml#
at91-sama7g54_curiosity.dtb: pmic@5b: regulators:VDD_(CORE|OTHER)|LDO[1-2]:
regulator-state-standby 'regulator-suspend-voltage' does not match any of
the regexes 'pinctrl-[0-9]+' from schema
$id: http://devicetree.org/schemas/regulator/microchip,mcp16502.yaml#
This patch series proposes to correct the typo that was entered by mistake
into devicetree definition regulator-state-standby by replacing
regulator-suspend-voltage with regulator-suspend-microvolt.
--------------------
v1 -> v2:
- drop "boot" from title
- put in commit message a snippet with the warning to explain the chang
--------------------
Andrei Simion (2):
ARM: dts: microchip: at91-sama7g5ek: Replace regulator-suspend-voltage
with the valid property
ARM: dts: microchip: at91-sama7g54_curiosity: Replace
regulator-suspend-voltage with the valid property
arch/arm/boot/dts/microchip/at91-sama7g54_curiosity.dts | 8 ++++----
arch/arm/boot/dts/microchip/at91-sama7g5ek.dts | 8 ++++----
2 files changed, 8 insertions(+), 8 deletions(-)
--
2.34.1
^ permalink raw reply
* [PATCH v2 1/2] ARM: dts: microchip: at91-sama7g5ek: Replace regulator-suspend-voltage with the valid property
From: Andrei Simion @ 2024-04-04 12:38 UTC (permalink / raw)
To: robh, krzysztof.kozlowski+dt, conor+dt, nicolas.ferre,
alexandre.belloni, claudiu.beznea, mihai.sain
Cc: linux-arm-kernel, devicetree, linux-kernel, Andrei Simion
In-Reply-To: <20240404123824.19182-1-andrei.simion@microchip.com>
By checking the pmic node with microchip,mcp16502.yaml#
'regulator-suspend-voltage' does not match any of the
regexes 'pinctrl-[0-9]+' from schema microchip,mcp16502.yaml#
which inherits regulator.yaml# So replace regulator-suspend-voltage
with regulator-suspend-microvolt to avoid the inconsitency.
Fixes: 85b1304b9daa ("ARM: dts: at91: sama7g5ek: set regulator voltages for standby state")
Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
v1 -> v2:
- drop "boot" from title
- put in commit message a snippet with the warning to explain the change
---
arch/arm/boot/dts/microchip/at91-sama7g5ek.dts | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm/boot/dts/microchip/at91-sama7g5ek.dts b/arch/arm/boot/dts/microchip/at91-sama7g5ek.dts
index 217e9b96c61e..20b2497657ae 100644
--- a/arch/arm/boot/dts/microchip/at91-sama7g5ek.dts
+++ b/arch/arm/boot/dts/microchip/at91-sama7g5ek.dts
@@ -293,7 +293,7 @@ vddcore: VDD_CORE {
regulator-state-standby {
regulator-on-in-suspend;
- regulator-suspend-voltage = <1150000>;
+ regulator-suspend-microvolt = <1150000>;
regulator-mode = <4>;
};
@@ -314,7 +314,7 @@ vddcpu: VDD_OTHER {
regulator-state-standby {
regulator-on-in-suspend;
- regulator-suspend-voltage = <1050000>;
+ regulator-suspend-microvolt = <1050000>;
regulator-mode = <4>;
};
@@ -331,7 +331,7 @@ vldo1: LDO1 {
regulator-always-on;
regulator-state-standby {
- regulator-suspend-voltage = <1800000>;
+ regulator-suspend-microvolt = <1800000>;
regulator-on-in-suspend;
};
@@ -346,7 +346,7 @@ vldo2: LDO2 {
regulator-max-microvolt = <3700000>;
regulator-state-standby {
- regulator-suspend-voltage = <1800000>;
+ regulator-suspend-microvolt = <1800000>;
regulator-on-in-suspend;
};
--
2.34.1
^ permalink raw reply related
* [PATCH v2 2/2] ARM: dts: microchip: at91-sama7g54_curiosity: Replace regulator-suspend-voltage with the valid property
From: Andrei Simion @ 2024-04-04 12:38 UTC (permalink / raw)
To: robh, krzysztof.kozlowski+dt, conor+dt, nicolas.ferre,
alexandre.belloni, claudiu.beznea, mihai.sain
Cc: linux-arm-kernel, devicetree, linux-kernel, Andrei Simion
In-Reply-To: <20240404123824.19182-1-andrei.simion@microchip.com>
By checking the pmic node with microchip,mcp16502.yaml#
'regulator-suspend-voltage' does not match any of the
regexes 'pinctrl-[0-9]+' from schema microchip,mcp16502.yaml#
which inherits regulator.yaml#. So replace regulator-suspend-voltage
with regulator-suspend-microvolt to avoid the inconsitency.
Fixes: ebd6591f8ddb ("ARM: dts: microchip: sama7g54_curiosity: Add initial device tree of the board")
Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
v1 -> v2:
- drop "boot" from title
- put in commit message a snippet with the warning to explain the change
---
arch/arm/boot/dts/microchip/at91-sama7g54_curiosity.dts | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm/boot/dts/microchip/at91-sama7g54_curiosity.dts b/arch/arm/boot/dts/microchip/at91-sama7g54_curiosity.dts
index 4f609e9e510e..009d2c832421 100644
--- a/arch/arm/boot/dts/microchip/at91-sama7g54_curiosity.dts
+++ b/arch/arm/boot/dts/microchip/at91-sama7g54_curiosity.dts
@@ -242,7 +242,7 @@ vddcore: VDD_CORE {
regulator-state-standby {
regulator-on-in-suspend;
- regulator-suspend-voltage = <1150000>;
+ regulator-suspend-microvolt = <1150000>;
regulator-mode = <4>;
};
@@ -263,7 +263,7 @@ vddcpu: VDD_OTHER {
regulator-state-standby {
regulator-on-in-suspend;
- regulator-suspend-voltage = <1050000>;
+ regulator-suspend-microvolt = <1050000>;
regulator-mode = <4>;
};
@@ -280,7 +280,7 @@ vldo1: LDO1 {
regulator-always-on;
regulator-state-standby {
- regulator-suspend-voltage = <1800000>;
+ regulator-suspend-microvolt = <1800000>;
regulator-on-in-suspend;
};
@@ -296,7 +296,7 @@ vldo2: LDO2 {
regulator-always-on;
regulator-state-standby {
- regulator-suspend-voltage = <3300000>;
+ regulator-suspend-microvolt = <3300000>;
regulator-on-in-suspend;
};
--
2.34.1
^ permalink raw reply related
* [PATCH v3] arm64: dts: ti: k3-am62p: use eFuse MAC Address for CPSW3G Port 1
From: Siddharth Vadapalli @ 2024-04-04 12:46 UTC (permalink / raw)
To: nm, vigneshr, kristo, robh, krzk+dt, conor+dt
Cc: devicetree, linux-kernel, linux-arm-kernel, srk, s-vadapalli
Add the "ethernet-mac-syscon" node within "wkup_conf" node corresponding to
the CTRLMMR_MAC_IDx registers within the CTRL_MMR space. Assign the
compatible "ti,am62p-cpsw-mac-efuse" to enable "syscon_regmap" operations
on these registers. The MAC Address programmed in the eFuse is accessible
through the CTRLMMR_MAC_IDx registers. The "ti,syscon-efuse" device-tree
property points to the CTRLMMR_MAC_IDx registers, allowing the CPSW driver
to fetch the MAC Address and assign it to the network interface associated
with CPSW3G MAC Port 1.
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
This patch is based on linux-next tagged next-20240404.
Patch depends on:
https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240402105708.4114146-1-s-vadapalli@ti.com/
for the newly added "ti,am62p-cpsw-mac-efuse" compatible.
v2:
https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240404081845.622707-1-s-vadapalli@ti.com/
Changes since v2:
- Renamed "cpsw-mac-efuse" node as "ethernet-mac-syscon" based on
Krzysztof's suggestion.
- Renamed "cpsw_mac_efuse" label as "cpsw_mac_syscon" to match
node naming convention.
- Updated node-name in commit message to "ethernet-mac-syscon".
v1:
https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240402094200.4036076-1-s-vadapalli@ti.com/
Changes since v1:
- Since "wkup_conf" is modelled as a "simple-bus" rather than being
modelled as a System Controller node with the "syscon" compatible,
directly passing the reference to the "wkup_conf" node using the
"ti,syscon-efuse" device-tree property will not work.
Therefore, I posted the patch at:
https://patchwork.kernel.org/project/linux-arm-kernel/patch/20240402105708.4114146-1-s-vadapalli@ti.com/
in order to add a new compatible to be used for modelling the
CTRLMMR_MAC_IDx registers as System Controller nodes, thereby
allowing the existing "ti,syscon-efuse" property to be used.
Now, "ti,syscon-efuse" points to the "cpsw_mac_efuse" node within
"wkup_conf" node, with "cpsw_mac_efuse" being a "syscon" node.
Logs verifying that the CPSW driver assigns the MAC Address from the
eFuse based on the CTRLMMR_MAC_IDx registers at 0x43000200 and 0x43000204
to the interface eth0 corresponding to CPSW3G MAC Port 1:
https://gist.github.com/Siddharth-Vadapalli-at-TI/63473d68e7a34860566c1339ce3da9f0
arch/arm64/boot/dts/ti/k3-am62p-main.dtsi | 1 +
arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi | 5 +++++
2 files changed, 6 insertions(+)
diff --git a/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi
index 7337a9e13535..21020b7d3034 100644
--- a/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi
@@ -696,6 +696,7 @@ cpsw_port1: port@1 {
label = "port1";
phys = <&phy_gmii_sel 1>;
mac-address = [00 00 00 00 00 00];
+ ti,syscon-efuse = <&cpsw_mac_syscon 0x0>;
};
cpsw_port2: port@2 {
diff --git a/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi
index a84756c336d0..7469b3d3a8c9 100644
--- a/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi
@@ -18,6 +18,11 @@ chipid: chipid@14 {
reg = <0x14 0x4>;
bootph-all;
};
+
+ cpsw_mac_syscon: ethernet-mac-syscon@200 {
+ compatible = "ti,am62p-cpsw-mac-efuse", "syscon";
+ reg = <0x200 0x8>;
+ };
};
wkup_uart0: serial@2b300000 {
--
2.40.1
^ permalink raw reply related
* Re: [PATCH v19 2/9] usb: dwc3: core: Access XHCI address space temporarily to read port info
From: Greg Kroah-Hartman @ 2024-04-04 12:58 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Johan Hovold, Krishna Kurapati, Krzysztof Kozlowski, Rob Herring,
Bjorn Andersson, Wesley Cheng, Konrad Dybcio, Conor Dooley,
Thinh Nguyen, Felipe Balbi, devicetree, linux-arm-msm, linux-usb,
linux-kernel, quic_ppratap, quic_jackp, Johan Hovold
In-Reply-To: <f16e1280-8f7e-40a7-ab45-9acaeb3e90cb@linaro.org>
On Thu, Apr 04, 2024 at 10:07:27AM +0200, Krzysztof Kozlowski wrote:
> On 04/04/2024 09:21, Johan Hovold wrote:
> > On Thu, Apr 04, 2024 at 10:42:22AM +0530, Krishna Kurapati wrote:
> >
> >> +static int dwc3_get_num_ports(struct dwc3 *dwc)
> >> +{
> >> + void __iomem *base;
> >> + u8 major_revision;
> >> + u32 offset;
> >> + u32 val;
> >> +
> >> + /*
> >> + * Remap xHCI address space to access XHCI ext cap regs since it is
> >> + * needed to get information on number of ports present.
> >> + */
> >> + base = ioremap(dwc->xhci_resources[0].start,
> >> + resource_size(&dwc->xhci_resources[0]));
> >> + if (!base)
> >> + return PTR_ERR(base);
> >
> > This is obviously still broken. You need to update the return value as
> > well.
> >
> > Fix in v20.
>
> If one patchset reaches 20 versions, I think it is time to stop and
> really think from the beginning, why issues keep appearing and reviewers
> are still not happy.
>
> Maybe you did not perform extensive internal review, which you are
> encouraged to by your own internal policies, AFAIR. Before posting next
> version, please really get some internal review first.
Also get those internal reviewers to sign-off on the commits and have
that show up when you post them next. That way they are also
responsible for this patchset, it's not fair that they are making you do
all the work here :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v19 2/9] usb: dwc3: core: Access XHCI address space temporarily to read port info
From: Greg Kroah-Hartman @ 2024-04-04 12:58 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Johan Hovold, Krishna Kurapati, Krzysztof Kozlowski, Rob Herring,
Bjorn Andersson, Wesley Cheng, Konrad Dybcio, Conor Dooley,
Thinh Nguyen, Felipe Balbi, devicetree, linux-arm-msm, linux-usb,
linux-kernel, quic_ppratap, quic_jackp, Johan Hovold
In-Reply-To: <2024040455-sitting-dictator-170c@gregkh>
On Thu, Apr 04, 2024 at 02:58:29PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Apr 04, 2024 at 10:07:27AM +0200, Krzysztof Kozlowski wrote:
> > On 04/04/2024 09:21, Johan Hovold wrote:
> > > On Thu, Apr 04, 2024 at 10:42:22AM +0530, Krishna Kurapati wrote:
> > >
> > >> +static int dwc3_get_num_ports(struct dwc3 *dwc)
> > >> +{
> > >> + void __iomem *base;
> > >> + u8 major_revision;
> > >> + u32 offset;
> > >> + u32 val;
> > >> +
> > >> + /*
> > >> + * Remap xHCI address space to access XHCI ext cap regs since it is
> > >> + * needed to get information on number of ports present.
> > >> + */
> > >> + base = ioremap(dwc->xhci_resources[0].start,
> > >> + resource_size(&dwc->xhci_resources[0]));
> > >> + if (!base)
> > >> + return PTR_ERR(base);
> > >
> > > This is obviously still broken. You need to update the return value as
> > > well.
> > >
> > > Fix in v20.
> >
> > If one patchset reaches 20 versions, I think it is time to stop and
> > really think from the beginning, why issues keep appearing and reviewers
> > are still not happy.
> >
> > Maybe you did not perform extensive internal review, which you are
> > encouraged to by your own internal policies, AFAIR. Before posting next
> > version, please really get some internal review first.
>
> Also get those internal reviewers to sign-off on the commits and have
> that show up when you post them next. That way they are also
> responsible for this patchset, it's not fair that they are making you do
> all the work here :)
"you" meaning Krishna, sorry for any confusion here.
^ permalink raw reply
* Re: [PATCH 1/6] dt-bindings: adc: ad7173: add support for ad411x
From: Ceclan, Dumitru @ 2024-04-04 13:08 UTC (permalink / raw)
To: David Lechner
Cc: dumitru.ceclan, Lars-Peter Clausen, Michael Hennerich,
Jonathan Cameron, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-iio, devicetree, linux-kernel
In-Reply-To: <CAMknhBHhxi7mN88+peU7BGkzSP2vtipCuvM-XfQzgusqKvARsg@mail.gmail.com>
On 03/04/2024 18:22, David Lechner wrote:
> On Wed, Apr 3, 2024 at 2:50 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote:
>> On 02/04/2024 00:16, David Lechner wrote:
>>> On Mon, Apr 1, 2024 at 2:37 PM David Lechner <dlechner@baylibre.com> wrote:
>>>> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
>>>> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
>>>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>>>>>
>> ...
>>
>>>>> properties:
>>>>> reg:
>>>>> + description:
>>>>> + Reg values 16-19 are only permitted for ad4111/ad4112 current channels.
>>>>> minimum: 0
>>>>> - maximum: 15
>>>>> + maximum: 19
>>>> This looks wrong. Isn't reg describing the number of logical channels
>>>> (# of channel config registers)?
>>>>
>>>> After reviewing the driver, I see that > 16 is used as a way of
>>>> flagging current inputs, but still seems like the wrong way to do it.
>>>> See suggestion below.
>>>>
>>>>> diff-channels:
>>>>> + description:
>>>>> + For using current channels specify only the positive channel.
>>>>> + (IIN2+, IIN2−) -> diff-channels = <2 0>
>>>> I find this a bit confusing since 2 is already VIN2 and 0 is already
>>>> VIN0. I think it would make more sense to assign unique channel
>>>> numbers individually to the negative and positive current inputs.
>>>> Also, I think it makes sense to use the same numbers that the
>>>> registers in the datasheet use (8 - 11 for negative and 12 to 15 for
>>>> positive).
>>>>
>>>> So: (IIN2+, IIN2−) -> diff-channels = <13 10>
>>> Thinking about this a bit more...
>>>
>>> Since the current inputs have dedicated pins and aren't mix-and-match
>>> with multiple valid wiring configurations like the voltage inputs, do
>>> we even need to describe them in the devicetree?
>>>
>>> In the driver, the current channels would just be hard-coded like the
>>> temperature channel since there isn't any application-specific
>>> variation.
>> Sure, but we still need to offer the user a way to configure which
>> current inputs he wants and if they should use bipolar or unipolar coding.
> From the datasheet, it looks like only positive current input is
> allowed so I'm not sure bipolar applies here. But, yes, if there is
> some other variation in wiring or electrical signal that needs to be
> describe here, then it makes sense to allow a channel configuration
> node for it.
AD4111 datasheet pg.29:
When the ADC is configured for bipolar operation, the output
code is offset binary with a negative full-scale voltage resulting
in a code of 000 … 000, a zero differential input voltage resulting in
a code of 100 … 000, and a positive full-scale input voltage
resulting in a code of 111 … 111. The output code for any
analog input voltage can be represented as
Code = 2^(N – 1) × ((V_IN × 0.1/V REF) + 1)
The output code for any input current is represented as
Code = 2^(N − 1) × ((I_IN × 50 Ω/V REF) + 1)
I would say bipolar applies here, not a great idea because of the limitation on
the negative side (Input Current Range min:−0.5 max:+24 mA) so still, the option
is available.
^ permalink raw reply
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi
From: Dmitry Baryshkov @ 2024-04-04 13:14 UTC (permalink / raw)
To: Marc Gonzalez
Cc: Kalle Valo, Konrad Dybcio, Krzysztof Kozlowski, Jeff Johnson,
ath10k, wireless, DT, MSM, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Pierre-Hugues Husson, Arnaud Vrac, Bjorn Andersson,
Jami Kettunen, Jeffrey Hugo
In-Reply-To: <e804b257-4dc0-45f1-a5c5-66bda51cf296@freebox.fr>
On Thu, 4 Apr 2024 at 15:30, Marc Gonzalez <mgonzalez@freebox.fr> wrote:
>
> On 04/04/2024 13:57, Kalle Valo wrote:
>
> > Dmitry Baryshkov wrote:
> >
> >> I'd say, we should take a step back and actually verify how this was
> >> handled in the vendor kernel.
> >
> > One comment related to this: usually vendor driver and firmware branches
> > go "hand in hand", meaning that a version of driver supports only one
> > specific firmware branch. And there can be a lot of branches. So even if
> > one branch might have a check for something specific, there are no
> > guarantees what the other N+1 branches do :/
>
> The consequences and ramifications of the above comment are not clear to me.
>
> Does this mean:
> "It is pointless to analyze a given version (or even several versions)
> of the vendor driver downstream, because there are exist a large number
> of variations of the code." ?
>
> And thus, "it is nonsensical to try to "align" the mainline driver to
> "the" vendor driver, as there is no single "vendor driver"" ?
>
> Thus, the following patch (or one functionally-equivalent) is not acceptable?
For reference, I tested this patch on sdm845 (db845c), qcm2290 aka
qrb2210 (rb1), sm6115 aka qrb4210 (rb2) and sm8150 platforms.
I was not able to fully test it on sda660, modem crashes without this
patch (there is no MSA_READY indication) and with the patch applied
the device hangs, most likely because of the IOMMU or clocking issue.
>
> diff --git a/drivers/net/wireless/ath/ath10k/qmi.c b/drivers/net/wireless/ath/ath10k/qmi.c
> index 38e939f572a9e..fd9ac9717488a 100644
> --- a/drivers/net/wireless/ath/ath10k/qmi.c
> +++ b/drivers/net/wireless/ath/ath10k/qmi.c
> @@ -1040,6 +1040,8 @@ static void ath10k_qmi_driver_event_work(struct work_struct *work)
> switch (event->type) {
> case ATH10K_QMI_EVENT_SERVER_ARRIVE:
> ath10k_qmi_event_server_arrive(qmi);
> + printk(KERN_NOTICE "NOT WAITING FOR MSA_READY INDICATOR");
> + ath10k_qmi_event_msa_ready(qmi);
> break;
> case ATH10K_QMI_EVENT_SERVER_EXIT:
> ath10k_qmi_event_server_exit(qmi);
> @@ -1048,7 +1050,7 @@ static void ath10k_qmi_driver_event_work(struct work_struct *work)
> ath10k_qmi_event_fw_ready_ind(qmi);
> break;
> case ATH10K_QMI_EVENT_MSA_READY_IND:
> - ath10k_qmi_event_msa_ready(qmi);
> + printk(KERN_NOTICE "IGNORING ACTUAL MSA_READY INDICATOR");
> break;
> default:
> ath10k_warn(ar, "invalid event type: %d", event->type);
>
>
>
> Regards
>
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC
From: Matti Vaittinen @ 2024-04-04 13:15 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: <eb03ec33-0627-4986-be04-8e35da390d6b@sirena.org.uk>
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).
>
> So if we arrange to supply a name when we register multiple domains
> things should work fine?
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 :)
But yes, creating 2 regmap-IRQ controllers for one device seems to
generate naming conflict in the debugfs - so unless I'm mistaken, with
the current regmap-IRQ we can't have more than 1 regmap-IRQ entity for a
single device.
Just please give me some more time to see if I find the cause of the
corruption and I hope I can write more concrete description. For now it
was enough for me to hear having more than 1 IRQ domain / device is not
something on the "DON'T DO THIS" -list.
Yours,
-- Matti
--
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 08/17] clk: samsung: gs101: add support for cmu_hsi2
From: André Draszik @ 2024-04-04 13:24 UTC (permalink / raw)
To: Peter Griffin, mturquette, sboyd, robh, krzk+dt, conor+dt, vkoul,
kishon, alim.akhtar, avri.altman, bvanassche, s.nawrocki,
cw00.choi, jejb, martin.petersen, chanho61.park, ebiggers
Cc: linux-scsi, linux-phy, devicetree, linux-clk, linux-samsung-soc,
linux-kernel, linux-arm-kernel, tudor.ambarus, saravanak,
willmcvicker
In-Reply-To: <20240404122559.898930-9-peter.griffin@linaro.org>
Hi Pete,
Thanks for this!
I haven't reviewed this, but one immediate comment...
On Thu, 2024-04-04 at 13:25 +0100, Peter Griffin wrote:
> [...]
> diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c
> index d065e343a85d..b9f84c7d5c22 100644
> --- a/drivers/clk/samsung/clk-gs101.c
> +++ b/drivers/clk/samsung/clk-gs101.c
> @@ -22,6 +22,7 @@
> #define CLKS_NR_MISC (CLK_GOUT_MISC_XIU_D_MISC_ACLK + 1)
> #define CLKS_NR_PERIC0 (CLK_GOUT_PERIC0_SYSREG_PERIC0_PCLK + 1)
> #define CLKS_NR_PERIC1 (CLK_GOUT_PERIC1_SYSREG_PERIC1_PCLK + 1)
> +#define CLKS_NR_HSI2 (CLK_GOUT_HSI2_XIU_P_HSI2_ACLK + 1)
Can you please keep the #defines alphabetical (hsi before misc).
>
> /* ---- CMU_TOP ------------------------------------------------------------- */
>
> @@ -3409,6 +3410,560 @@ static const struct samsung_cmu_info peric1_cmu_info __initconst = {
> .clk_name = "bus",
> };
>
> +/* ---- CMU_HSI2 ---------------------------------------------------------- */
and this code block should be earlier in the file
> [..]
> static int __init gs101_cmu_probe(struct platform_device *pdev)
> @@ -3432,6 +3987,9 @@ static const struct of_device_id gs101_cmu_of_match[] = {
> }, {
> .compatible = "google,gs101-cmu-peric1",
> .data = &peric1_cmu_info,
> + }, {
> + .compatible = "google,gs101-cmu-hsi2",
> + .data = &hsi2_cmu_info,
> }, {
and this block should move up
> },
> };
> diff --git a/include/dt-bindings/clock/google,gs101.h b/include/dt-bindings/clock/google,gs101.h
> index 3dac3577788a..ac239ce6821b 100644
> --- a/include/dt-bindings/clock/google,gs101.h
> +++ b/include/dt-bindings/clock/google,gs101.h
> @@ -518,4 +518,67 @@
> #define CLK_GOUT_PERIC1_CLK_PERIC1_USI9_USI_CLK 45
> #define CLK_GOUT_PERIC1_SYSREG_PERIC1_PCLK 46
>
> +/* CMU_HSI2 */
and all these defines, too.
Cheers,
Andre'
^ permalink raw reply
* Re: [PATCH v2 2/5] irqchip/renesas-rzg2l: Add support for RZ/Five SoC
From: Lad, Prabhakar @ 2024-04-04 13:26 UTC (permalink / raw)
To: Biju Das
Cc: Geert Uytterhoeven, Thomas Gleixner, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Magnus Damm, Paul Walmsley,
Palmer Dabbelt, Albert Ou, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
linux-riscv@lists.infradead.org, Prabhakar Mahadev Lad
In-Reply-To: <OSAPR01MB1587ED05696A111612424CF6863C2@OSAPR01MB1587.jpnprd01.prod.outlook.com>
Hi Biju,
Thank you for the review.
On Thu, Apr 4, 2024 at 8:44 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Prabhakar,
>
> Thanks for the patch.
>
> > -----Original Message-----
> > From: Prabhakar <prabhakar.csengg@gmail.com>
> > Sent: Wednesday, April 3, 2024 9:35 PM
> > Subject: [PATCH v2 2/5] irqchip/renesas-rzg2l: Add support for RZ/Five SoC
> >
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > The IX45 block has additional mask registers (NMSK/IMSK/TMSK) as compared to the RZ/G2L (family)
> > SoC.
> >
> > Introduce masking/unmasking support for IRQ and TINT interrupts in IRQC controller driver. Two new
> > registers, IMSK and TMSK, are defined to handle masking on RZ/Five SoC. The implementation utilizes
> > a new data structure, `struct rzg2l_irqc_data`, to determine mask support for a specific controller
> > instance.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > v1->v2
> > - Added IRQCHIP_MATCH() for RZ/Five
> > - Retaining a copy of OF data in priv
> > - Rebased the changes
> > ---
> > drivers/irqchip/irq-renesas-rzg2l.c | 137 +++++++++++++++++++++++++++-
> > 1 file changed, 132 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
> > index f6484bf15e0b..6fa8d65605dc 100644
> > --- a/drivers/irqchip/irq-renesas-rzg2l.c
> > +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> > @@ -37,6 +37,8 @@
> > #define TSSEL_SHIFT(n) (8 * (n))
> > #define TSSEL_MASK GENMASK(7, 0)
> > #define IRQ_MASK 0x3
> > +#define IMSK 0x10010
> > +#define TMSK 0x10020
> >
> > #define TSSR_OFFSET(n) ((n) % 4)
> > #define TSSR_INDEX(n) ((n) / 4)
> > @@ -66,15 +68,25 @@ struct rzg2l_irqc_reg_cache {
> > u32 titsr[2];
> > };
> >
> > +/**
> > + * struct rzg2l_irqc_of_data - OF data structure
> > + * @mask_supported: Indicates if mask registers are available */
> > +struct rzg2l_irqc_of_data {
> > + bool mask_supported;
> > +};
> > +
> > /**
> > * struct rzg2l_irqc_priv - IRQ controller private data structure
> > * @base: Controller's base address
> > + * @data: OF data pointer
> > * @fwspec: IRQ firmware specific data
> > * @lock: Lock to serialize access to hardware registers
> > * @cache: Registers cache for suspend/resume
> > */
> > static struct rzg2l_irqc_priv {
> > void __iomem *base;
> > + const struct rzg2l_irqc_of_data *data;
> > struct irq_fwspec fwspec[IRQC_NUM_IRQ];
> > raw_spinlock_t lock;
> > struct rzg2l_irqc_reg_cache cache;
> > @@ -138,18 +150,102 @@ static void rzg2l_irqc_eoi(struct irq_data *d)
> > irq_chip_eoi_parent(d);
> > }
> >
> > +static void rzg2l_irqc_mask_irq_interrupt(struct rzg2l_irqc_priv *priv,
> > + unsigned int hwirq)
> > +{
> > + u32 imsk = readl_relaxed(priv->base + IMSK);
> > + u32 bit = BIT(hwirq - IRQC_IRQ_START);
> > +
> > + writel_relaxed(imsk | bit, priv->base + IMSK); }
> > +
> > +static void rzg2l_irqc_unmask_irq_interrupt(struct rzg2l_irqc_priv *priv,
> > + unsigned int hwirq)
> > +{
> > + u32 imsk = readl_relaxed(priv->base + IMSK);
> > + u32 bit = BIT(hwirq - IRQC_IRQ_START);
> > +
> > + writel_relaxed(imsk & ~bit, priv->base + IMSK); }
> > +
> > +static void rzg2l_irqc_mask_tint_interrupt(struct rzg2l_irqc_priv *priv,
> > + unsigned int hwirq)
> > +{
> > + u32 tmsk = readl_relaxed(priv->base + TMSK);
> > + u32 bit = BIT(hwirq - IRQC_TINT_START);
> > +
> > + writel_relaxed(tmsk | bit, priv->base + TMSK); }
> > +
> > +static void rzg2l_irqc_unmask_tint_interrupt(struct rzg2l_irqc_priv *priv,
> > + unsigned int hwirq)
> > +{
> > + u32 tmsk = readl_relaxed(priv->base + TMSK);
> > + u32 bit = BIT(hwirq - IRQC_TINT_START);
> > +
> > + writel_relaxed(tmsk & ~bit, priv->base + TMSK); }
> > +
> > +/* Must be called while priv->lock is held */ static void
> > +rzg2l_irqc_mask_once(struct rzg2l_irqc_priv *priv, unsigned int hwirq)
> > +{
> > + if (!priv->data->mask_supported)
> > + return;
> > +
> > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT)
> > + rzg2l_irqc_mask_irq_interrupt(priv, hwirq);
> > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ)
> > + rzg2l_irqc_mask_tint_interrupt(priv, hwirq); }
> > +
> > +static void rzg2l_irqc_mask(struct irq_data *d) {
> > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
> > +
> > + raw_spin_lock(&priv->lock);
> > + rzg2l_irqc_mask_once(priv, irqd_to_hwirq(d));
> > + raw_spin_unlock(&priv->lock);
> > + irq_chip_mask_parent(d);
> > +}
> > +
> > +/* Must be called while priv->lock is held */ static void
> > +rzg2l_irqc_unmask_once(struct rzg2l_irqc_priv *priv, unsigned int
> > +hwirq) {
> > + if (!priv->data->mask_supported)
> > + return;
> > +
> > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT)
> > + rzg2l_irqc_unmask_irq_interrupt(priv, hwirq);
> > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ)
> > + rzg2l_irqc_unmask_tint_interrupt(priv, hwirq); }
> > +
> > +static void rzg2l_irqc_unmask(struct irq_data *d) {
> > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
> > +
> > + raw_spin_lock(&priv->lock);
> > + rzg2l_irqc_unmask_once(priv, irqd_to_hwirq(d));
> > + raw_spin_unlock(&priv->lock);
> > + irq_chip_unmask_parent(d);
> > +}
> > +
> > static void rzg2l_tint_irq_endisable(struct irq_data *d, bool enable) {
> > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
> > unsigned int hw_irq = irqd_to_hwirq(d);
> >
> > if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) {
> > - struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
> > u32 offset = hw_irq - IRQC_TINT_START;
> > u32 tssr_offset = TSSR_OFFSET(offset);
> > u8 tssr_index = TSSR_INDEX(offset);
> > u32 reg;
> >
> > raw_spin_lock(&priv->lock);
> > + if (enable)
> > + rzg2l_irqc_unmask_once(priv, hw_irq);
> > + else
> > + rzg2l_irqc_mask_once(priv, hw_irq);
> > reg = readl_relaxed(priv->base + TSSR(tssr_index));
> > if (enable)
> > reg |= TIEN << TSSEL_SHIFT(tssr_offset); @@ -157,6 +253,13 @@ static void
> > rzg2l_tint_irq_endisable(struct irq_data *d, bool enable)
> > reg &= ~(TIEN << TSSEL_SHIFT(tssr_offset));
> > writel_relaxed(reg, priv->base + TSSR(tssr_index));
> > raw_spin_unlock(&priv->lock);
> > + } else {
> > + raw_spin_lock(&priv->lock);
> > + if (enable)
> > + rzg2l_irqc_unmask_once(priv, hw_irq);
> > + else
> > + rzg2l_irqc_mask_once(priv, hw_irq);
> > + raw_spin_unlock(&priv->lock);
> > }
> > }
> >
> > @@ -324,8 +427,8 @@ static struct syscore_ops rzg2l_irqc_syscore_ops = { static const struct
> > irq_chip irqc_chip = {
> > .name = "rzg2l-irqc",
> > .irq_eoi = rzg2l_irqc_eoi,
> > - .irq_mask = irq_chip_mask_parent,
> > - .irq_unmask = irq_chip_unmask_parent,
> > + .irq_mask = rzg2l_irqc_mask,
> > + .irq_unmask = rzg2l_irqc_unmask,
>
> I feel this will be clean, if we have
>
> static const struct irq_chip rzg2l_irqc_chip = {
> .name = "rzg2l-irqc",
> ...
> .irq_mask = irq_chip_mask_parent,
> .irq_unmask = irq_chip_unmask_parent,
> ....
> };
>
> static const struct irq_chip rzfive_irqc_chip = {
> .name = "rzfive-irqc",
> ...
> .irq_mask = rzfive_irqc_mask,
> .irq_unmask = rzfive_irqc_unmask,
> ....
> };
>
> And passing this in rzg2l_irqc_init() and rzfive_irqc_init(), see below
>
> return rzg2l_irqc_init_helper(node, parent, & rzg2l_irqc_chip);
> return rzg2l_irqc_init_helper(node, parent, & rzfive_irqc_chip);
>
If we do the above we are stuck with "struct irq_chip" as data, for
further upcoming SoCs (for example RZ/V2H) which have more features we
need to pass custom data to handle these features.
>
> > .irq_disable = rzg2l_irqc_irq_disable,
> > .irq_enable = rzg2l_irqc_irq_enable,
> > .irq_get_irqchip_state = irq_chip_get_parent_state,
> > @@ -401,7 +504,16 @@ static int rzg2l_irqc_parse_interrupts(struct rzg2l_irqc_priv *priv,
> > return 0;
> > }
> >
> > -static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
> > +static const struct rzg2l_irqc_of_data rzg2l_irqc_mask_supported_data = {
> > + .mask_supported = true,
> > +};
> > +
> > +static const struct rzg2l_irqc_of_data rzg2l_irqc_default_data = {
> > + .mask_supported = false,
> > +};
> > +
> > +static int rzg2l_irqc_init(struct device_node *node, struct device_node *parent,
> > + const struct rzg2l_irqc_of_data *of_data)
>
> Maybe rename this as rzg2l_irqc_init_helper()
OK.
> > {
> > struct irq_domain *irq_domain, *parent_domain;
> > struct platform_device *pdev;
> > @@ -422,6 +534,8 @@ static int rzg2l_irqc_init(struct device_node *node, struct device_node
> > *parent)
> > if (!rzg2l_irqc_data)
> > return -ENOMEM;
> >
> > + rzg2l_irqc_data->data = of_data;
> > +
> > rzg2l_irqc_data->base = devm_of_iomap(&pdev->dev, pdev->dev.of_node, 0, NULL);
> > if (IS_ERR(rzg2l_irqc_data->base))
> > return PTR_ERR(rzg2l_irqc_data->base); @@ -472,8 +586,21 @@ static int
> > rzg2l_irqc_init(struct device_node *node, struct device_node *parent)
> > return ret;
> > }
> >
> > +static int __init rzg2l_irqc_default_init(struct device_node *node,
> > + struct device_node *parent)
> > +{
> > + return rzg2l_irqc_init(node, parent, &rzg2l_irqc_default_data); }
> > +
> > +static int __init rzg2l_irqc_mask_supported_init(struct device_node *node,
> > + struct device_node *parent)
> > +{
> > + return rzg2l_irqc_init(node, parent, &rzg2l_irqc_mask_supported_data);
> > +}
> > +
> > IRQCHIP_PLATFORM_DRIVER_BEGIN(rzg2l_irqc)
> > -IRQCHIP_MATCH("renesas,rzg2l-irqc", rzg2l_irqc_init)
> Retain this name
>
OK.
> > +IRQCHIP_MATCH("renesas,rzg2l-irqc", rzg2l_irqc_default_init)
> > +IRQCHIP_MATCH("renesas,r9a07g043f-irqc",
> > +rzg2l_irqc_mask_supported_init)
> Maybe rename this as rzfive_irqc_init ??
>
OK.
Cheers,
Prabhakar
^ permalink raw reply
* RE: [PATCH v2 2/5] irqchip/renesas-rzg2l: Add support for RZ/Five SoC
From: Biju Das @ 2024-04-04 13:31 UTC (permalink / raw)
To: Lad, Prabhakar
Cc: Geert Uytterhoeven, Thomas Gleixner, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Magnus Damm, Paul Walmsley,
Palmer Dabbelt, Albert Ou, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
linux-riscv@lists.infradead.org, Prabhakar Mahadev Lad
In-Reply-To: <CA+V-a8s94e9PLuLipQo+rGZ8g7UHxZJJAZZgvL3PQ4b8PKR2Xw@mail.gmail.com>
Hi Lad, Prabhakar,
> -----Original Message-----
> From: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> Sent: Thursday, April 4, 2024 2:27 PM
> Subject: Re: [PATCH v2 2/5] irqchip/renesas-rzg2l: Add support for RZ/Five SoC
>
> Hi Biju,
>
> Thank you for the review.
>
> On Thu, Apr 4, 2024 at 8:44 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> >
> > Hi Prabhakar,
> >
> > Thanks for the patch.
> >
> > > -----Original Message-----
> > > From: Prabhakar <prabhakar.csengg@gmail.com>
> > > Sent: Wednesday, April 3, 2024 9:35 PM
> > > Subject: [PATCH v2 2/5] irqchip/renesas-rzg2l: Add support for
> > > RZ/Five SoC
> > >
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > The IX45 block has additional mask registers (NMSK/IMSK/TMSK) as
> > > compared to the RZ/G2L (family) SoC.
> > >
> > > Introduce masking/unmasking support for IRQ and TINT interrupts in
> > > IRQC controller driver. Two new registers, IMSK and TMSK, are
> > > defined to handle masking on RZ/Five SoC. The implementation
> > > utilizes a new data structure, `struct rzg2l_irqc_data`, to determine mask support for a
> specific controller instance.
> > >
> > > Signed-off-by: Lad Prabhakar
> > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > > v1->v2
> > > - Added IRQCHIP_MATCH() for RZ/Five
> > > - Retaining a copy of OF data in priv
> > > - Rebased the changes
> > > ---
> > > drivers/irqchip/irq-renesas-rzg2l.c | 137
> > > +++++++++++++++++++++++++++-
> > > 1 file changed, 132 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/irqchip/irq-renesas-rzg2l.c
> > > b/drivers/irqchip/irq-renesas-rzg2l.c
> > > index f6484bf15e0b..6fa8d65605dc 100644
> > > --- a/drivers/irqchip/irq-renesas-rzg2l.c
> > > +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> > > @@ -37,6 +37,8 @@
> > > #define TSSEL_SHIFT(n) (8 * (n))
> > > #define TSSEL_MASK GENMASK(7, 0)
> > > #define IRQ_MASK 0x3
> > > +#define IMSK 0x10010
> > > +#define TMSK 0x10020
> > >
> > > #define TSSR_OFFSET(n) ((n) % 4)
> > > #define TSSR_INDEX(n) ((n) / 4)
> > > @@ -66,15 +68,25 @@ struct rzg2l_irqc_reg_cache {
> > > u32 titsr[2];
> > > };
> > >
> > > +/**
> > > + * struct rzg2l_irqc_of_data - OF data structure
> > > + * @mask_supported: Indicates if mask registers are available */
> > > +struct rzg2l_irqc_of_data {
> > > + bool mask_supported;
> > > +};
> > > +
> > > /**
> > > * struct rzg2l_irqc_priv - IRQ controller private data structure
> > > * @base: Controller's base address
> > > + * @data: OF data pointer
> > > * @fwspec: IRQ firmware specific data
> > > * @lock: Lock to serialize access to hardware registers
> > > * @cache: Registers cache for suspend/resume
> > > */
> > > static struct rzg2l_irqc_priv {
> > > void __iomem *base;
> > > + const struct rzg2l_irqc_of_data *data;
> > > struct irq_fwspec fwspec[IRQC_NUM_IRQ];
> > > raw_spinlock_t lock;
> > > struct rzg2l_irqc_reg_cache cache;
> > > @@ -138,18 +150,102 @@ static void rzg2l_irqc_eoi(struct irq_data *d)
> > > irq_chip_eoi_parent(d);
> > > }
> > >
> > > +static void rzg2l_irqc_mask_irq_interrupt(struct rzg2l_irqc_priv *priv,
> > > + unsigned int hwirq) {
> > > + u32 imsk = readl_relaxed(priv->base + IMSK);
> > > + u32 bit = BIT(hwirq - IRQC_IRQ_START);
> > > +
> > > + writel_relaxed(imsk | bit, priv->base + IMSK); }
> > > +
> > > +static void rzg2l_irqc_unmask_irq_interrupt(struct rzg2l_irqc_priv *priv,
> > > + unsigned int hwirq) {
> > > + u32 imsk = readl_relaxed(priv->base + IMSK);
> > > + u32 bit = BIT(hwirq - IRQC_IRQ_START);
> > > +
> > > + writel_relaxed(imsk & ~bit, priv->base + IMSK); }
> > > +
> > > +static void rzg2l_irqc_mask_tint_interrupt(struct rzg2l_irqc_priv *priv,
> > > + unsigned int hwirq) {
> > > + u32 tmsk = readl_relaxed(priv->base + TMSK);
> > > + u32 bit = BIT(hwirq - IRQC_TINT_START);
> > > +
> > > + writel_relaxed(tmsk | bit, priv->base + TMSK); }
> > > +
> > > +static void rzg2l_irqc_unmask_tint_interrupt(struct rzg2l_irqc_priv *priv,
> > > + unsigned int hwirq) {
> > > + u32 tmsk = readl_relaxed(priv->base + TMSK);
> > > + u32 bit = BIT(hwirq - IRQC_TINT_START);
> > > +
> > > + writel_relaxed(tmsk & ~bit, priv->base + TMSK); }
> > > +
> > > +/* Must be called while priv->lock is held */ static void
> > > +rzg2l_irqc_mask_once(struct rzg2l_irqc_priv *priv, unsigned int
> > > +hwirq) {
> > > + if (!priv->data->mask_supported)
> > > + return;
> > > +
> > > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT)
> > > + rzg2l_irqc_mask_irq_interrupt(priv, hwirq);
> > > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ)
> > > + rzg2l_irqc_mask_tint_interrupt(priv, hwirq); }
> > > +
> > > +static void rzg2l_irqc_mask(struct irq_data *d) {
> > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
> > > +
> > > + raw_spin_lock(&priv->lock);
> > > + rzg2l_irqc_mask_once(priv, irqd_to_hwirq(d));
> > > + raw_spin_unlock(&priv->lock);
> > > + irq_chip_mask_parent(d);
> > > +}
> > > +
> > > +/* Must be called while priv->lock is held */ static void
> > > +rzg2l_irqc_unmask_once(struct rzg2l_irqc_priv *priv, unsigned int
> > > +hwirq) {
> > > + if (!priv->data->mask_supported)
> > > + return;
> > > +
> > > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT)
> > > + rzg2l_irqc_unmask_irq_interrupt(priv, hwirq);
> > > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ)
> > > + rzg2l_irqc_unmask_tint_interrupt(priv, hwirq); }
> > > +
> > > +static void rzg2l_irqc_unmask(struct irq_data *d) {
> > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
> > > +
> > > + raw_spin_lock(&priv->lock);
> > > + rzg2l_irqc_unmask_once(priv, irqd_to_hwirq(d));
> > > + raw_spin_unlock(&priv->lock);
> > > + irq_chip_unmask_parent(d);
> > > +}
> > > +
> > > static void rzg2l_tint_irq_endisable(struct irq_data *d, bool
> > > enable) {
> > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
> > > unsigned int hw_irq = irqd_to_hwirq(d);
> > >
> > > if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) {
> > > - struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
> > > u32 offset = hw_irq - IRQC_TINT_START;
> > > u32 tssr_offset = TSSR_OFFSET(offset);
> > > u8 tssr_index = TSSR_INDEX(offset);
> > > u32 reg;
> > >
> > > raw_spin_lock(&priv->lock);
> > > + if (enable)
> > > + rzg2l_irqc_unmask_once(priv, hw_irq);
> > > + else
> > > + rzg2l_irqc_mask_once(priv, hw_irq);
> > > reg = readl_relaxed(priv->base + TSSR(tssr_index));
> > > if (enable)
> > > reg |= TIEN << TSSEL_SHIFT(tssr_offset); @@
> > > -157,6 +253,13 @@ static void rzg2l_tint_irq_endisable(struct irq_data *d, bool enable)
> > > reg &= ~(TIEN << TSSEL_SHIFT(tssr_offset));
> > > writel_relaxed(reg, priv->base + TSSR(tssr_index));
> > > raw_spin_unlock(&priv->lock);
> > > + } else {
> > > + raw_spin_lock(&priv->lock);
> > > + if (enable)
> > > + rzg2l_irqc_unmask_once(priv, hw_irq);
> > > + else
> > > + rzg2l_irqc_mask_once(priv, hw_irq);
> > > + raw_spin_unlock(&priv->lock);
> > > }
> > > }
> > >
> > > @@ -324,8 +427,8 @@ static struct syscore_ops rzg2l_irqc_syscore_ops
> > > = { static const struct irq_chip irqc_chip = {
> > > .name = "rzg2l-irqc",
> > > .irq_eoi = rzg2l_irqc_eoi,
> > > - .irq_mask = irq_chip_mask_parent,
> > > - .irq_unmask = irq_chip_unmask_parent,
> > > + .irq_mask = rzg2l_irqc_mask,
> > > + .irq_unmask = rzg2l_irqc_unmask,
> >
> > I feel this will be clean, if we have
> >
> > static const struct irq_chip rzg2l_irqc_chip = {
> > .name = "rzg2l-irqc",
> > ...
> > .irq_mask = irq_chip_mask_parent,
> > .irq_unmask = irq_chip_unmask_parent,
> > ....
> > };
> >
> > static const struct irq_chip rzfive_irqc_chip = {
> > .name = "rzfive-irqc",
> > ...
> > .irq_mask = rzfive_irqc_mask,
> > .irq_unmask = rzfive_irqc_unmask,
> > ....
> > };
> >
> > And passing this in rzg2l_irqc_init() and rzfive_irqc_init(), see
> > below
> >
> > return rzg2l_irqc_init_helper(node, parent, & rzg2l_irqc_chip); return
> > rzg2l_irqc_init_helper(node, parent, & rzfive_irqc_chip);
> >
> If we do the above we are stuck with "struct irq_chip" as data, for further upcoming SoCs (for
> example RZ/V2H) which have more features we need to pass custom data to handle these features.
That time device data can be extended like below
struct rz_g2l_irq_chip {
struct irq_chip;
void *data; /* custom data */
}
Cheers,
Biju
^ permalink raw reply
* Re: [PATCH v2 2/5] irqchip/renesas-rzg2l: Add support for RZ/Five SoC
From: Lad, Prabhakar @ 2024-04-04 13:34 UTC (permalink / raw)
To: Biju Das
Cc: Geert Uytterhoeven, Thomas Gleixner, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Magnus Damm, Paul Walmsley,
Palmer Dabbelt, Albert Ou, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
linux-riscv@lists.infradead.org, Prabhakar Mahadev Lad
In-Reply-To: <OSAPR01MB15878C2C2EE7905D33182CFF863C2@OSAPR01MB1587.jpnprd01.prod.outlook.com>
On Thu, Apr 4, 2024 at 2:31 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Lad, Prabhakar,
>
> > -----Original Message-----
> > From: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> > Sent: Thursday, April 4, 2024 2:27 PM
> > Subject: Re: [PATCH v2 2/5] irqchip/renesas-rzg2l: Add support for RZ/Five SoC
> >
> > Hi Biju,
> >
> > Thank you for the review.
> >
> > On Thu, Apr 4, 2024 at 8:44 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > >
> > > Hi Prabhakar,
> > >
> > > Thanks for the patch.
> > >
> > > > -----Original Message-----
> > > > From: Prabhakar <prabhakar.csengg@gmail.com>
> > > > Sent: Wednesday, April 3, 2024 9:35 PM
> > > > Subject: [PATCH v2 2/5] irqchip/renesas-rzg2l: Add support for
> > > > RZ/Five SoC
> > > >
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > The IX45 block has additional mask registers (NMSK/IMSK/TMSK) as
> > > > compared to the RZ/G2L (family) SoC.
> > > >
> > > > Introduce masking/unmasking support for IRQ and TINT interrupts in
> > > > IRQC controller driver. Two new registers, IMSK and TMSK, are
> > > > defined to handle masking on RZ/Five SoC. The implementation
> > > > utilizes a new data structure, `struct rzg2l_irqc_data`, to determine mask support for a
> > specific controller instance.
> > > >
> > > > Signed-off-by: Lad Prabhakar
> > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > > v1->v2
> > > > - Added IRQCHIP_MATCH() for RZ/Five
> > > > - Retaining a copy of OF data in priv
> > > > - Rebased the changes
> > > > ---
> > > > drivers/irqchip/irq-renesas-rzg2l.c | 137
> > > > +++++++++++++++++++++++++++-
> > > > 1 file changed, 132 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/irqchip/irq-renesas-rzg2l.c
> > > > b/drivers/irqchip/irq-renesas-rzg2l.c
> > > > index f6484bf15e0b..6fa8d65605dc 100644
> > > > --- a/drivers/irqchip/irq-renesas-rzg2l.c
> > > > +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> > > > @@ -37,6 +37,8 @@
> > > > #define TSSEL_SHIFT(n) (8 * (n))
> > > > #define TSSEL_MASK GENMASK(7, 0)
> > > > #define IRQ_MASK 0x3
> > > > +#define IMSK 0x10010
> > > > +#define TMSK 0x10020
> > > >
> > > > #define TSSR_OFFSET(n) ((n) % 4)
> > > > #define TSSR_INDEX(n) ((n) / 4)
> > > > @@ -66,15 +68,25 @@ struct rzg2l_irqc_reg_cache {
> > > > u32 titsr[2];
> > > > };
> > > >
> > > > +/**
> > > > + * struct rzg2l_irqc_of_data - OF data structure
> > > > + * @mask_supported: Indicates if mask registers are available */
> > > > +struct rzg2l_irqc_of_data {
> > > > + bool mask_supported;
> > > > +};
> > > > +
> > > > /**
> > > > * struct rzg2l_irqc_priv - IRQ controller private data structure
> > > > * @base: Controller's base address
> > > > + * @data: OF data pointer
> > > > * @fwspec: IRQ firmware specific data
> > > > * @lock: Lock to serialize access to hardware registers
> > > > * @cache: Registers cache for suspend/resume
> > > > */
> > > > static struct rzg2l_irqc_priv {
> > > > void __iomem *base;
> > > > + const struct rzg2l_irqc_of_data *data;
> > > > struct irq_fwspec fwspec[IRQC_NUM_IRQ];
> > > > raw_spinlock_t lock;
> > > > struct rzg2l_irqc_reg_cache cache;
> > > > @@ -138,18 +150,102 @@ static void rzg2l_irqc_eoi(struct irq_data *d)
> > > > irq_chip_eoi_parent(d);
> > > > }
> > > >
> > > > +static void rzg2l_irqc_mask_irq_interrupt(struct rzg2l_irqc_priv *priv,
> > > > + unsigned int hwirq) {
> > > > + u32 imsk = readl_relaxed(priv->base + IMSK);
> > > > + u32 bit = BIT(hwirq - IRQC_IRQ_START);
> > > > +
> > > > + writel_relaxed(imsk | bit, priv->base + IMSK); }
> > > > +
> > > > +static void rzg2l_irqc_unmask_irq_interrupt(struct rzg2l_irqc_priv *priv,
> > > > + unsigned int hwirq) {
> > > > + u32 imsk = readl_relaxed(priv->base + IMSK);
> > > > + u32 bit = BIT(hwirq - IRQC_IRQ_START);
> > > > +
> > > > + writel_relaxed(imsk & ~bit, priv->base + IMSK); }
> > > > +
> > > > +static void rzg2l_irqc_mask_tint_interrupt(struct rzg2l_irqc_priv *priv,
> > > > + unsigned int hwirq) {
> > > > + u32 tmsk = readl_relaxed(priv->base + TMSK);
> > > > + u32 bit = BIT(hwirq - IRQC_TINT_START);
> > > > +
> > > > + writel_relaxed(tmsk | bit, priv->base + TMSK); }
> > > > +
> > > > +static void rzg2l_irqc_unmask_tint_interrupt(struct rzg2l_irqc_priv *priv,
> > > > + unsigned int hwirq) {
> > > > + u32 tmsk = readl_relaxed(priv->base + TMSK);
> > > > + u32 bit = BIT(hwirq - IRQC_TINT_START);
> > > > +
> > > > + writel_relaxed(tmsk & ~bit, priv->base + TMSK); }
> > > > +
> > > > +/* Must be called while priv->lock is held */ static void
> > > > +rzg2l_irqc_mask_once(struct rzg2l_irqc_priv *priv, unsigned int
> > > > +hwirq) {
> > > > + if (!priv->data->mask_supported)
> > > > + return;
> > > > +
> > > > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT)
> > > > + rzg2l_irqc_mask_irq_interrupt(priv, hwirq);
> > > > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ)
> > > > + rzg2l_irqc_mask_tint_interrupt(priv, hwirq); }
> > > > +
> > > > +static void rzg2l_irqc_mask(struct irq_data *d) {
> > > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
> > > > +
> > > > + raw_spin_lock(&priv->lock);
> > > > + rzg2l_irqc_mask_once(priv, irqd_to_hwirq(d));
> > > > + raw_spin_unlock(&priv->lock);
> > > > + irq_chip_mask_parent(d);
> > > > +}
> > > > +
> > > > +/* Must be called while priv->lock is held */ static void
> > > > +rzg2l_irqc_unmask_once(struct rzg2l_irqc_priv *priv, unsigned int
> > > > +hwirq) {
> > > > + if (!priv->data->mask_supported)
> > > > + return;
> > > > +
> > > > + if (hwirq >= IRQC_IRQ_START && hwirq <= IRQC_IRQ_COUNT)
> > > > + rzg2l_irqc_unmask_irq_interrupt(priv, hwirq);
> > > > + else if (hwirq >= IRQC_TINT_START && hwirq < IRQC_NUM_IRQ)
> > > > + rzg2l_irqc_unmask_tint_interrupt(priv, hwirq); }
> > > > +
> > > > +static void rzg2l_irqc_unmask(struct irq_data *d) {
> > > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
> > > > +
> > > > + raw_spin_lock(&priv->lock);
> > > > + rzg2l_irqc_unmask_once(priv, irqd_to_hwirq(d));
> > > > + raw_spin_unlock(&priv->lock);
> > > > + irq_chip_unmask_parent(d);
> > > > +}
> > > > +
> > > > static void rzg2l_tint_irq_endisable(struct irq_data *d, bool
> > > > enable) {
> > > > + struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
> > > > unsigned int hw_irq = irqd_to_hwirq(d);
> > > >
> > > > if (hw_irq >= IRQC_TINT_START && hw_irq < IRQC_NUM_IRQ) {
> > > > - struct rzg2l_irqc_priv *priv = irq_data_to_priv(d);
> > > > u32 offset = hw_irq - IRQC_TINT_START;
> > > > u32 tssr_offset = TSSR_OFFSET(offset);
> > > > u8 tssr_index = TSSR_INDEX(offset);
> > > > u32 reg;
> > > >
> > > > raw_spin_lock(&priv->lock);
> > > > + if (enable)
> > > > + rzg2l_irqc_unmask_once(priv, hw_irq);
> > > > + else
> > > > + rzg2l_irqc_mask_once(priv, hw_irq);
> > > > reg = readl_relaxed(priv->base + TSSR(tssr_index));
> > > > if (enable)
> > > > reg |= TIEN << TSSEL_SHIFT(tssr_offset); @@
> > > > -157,6 +253,13 @@ static void rzg2l_tint_irq_endisable(struct irq_data *d, bool enable)
> > > > reg &= ~(TIEN << TSSEL_SHIFT(tssr_offset));
> > > > writel_relaxed(reg, priv->base + TSSR(tssr_index));
> > > > raw_spin_unlock(&priv->lock);
> > > > + } else {
> > > > + raw_spin_lock(&priv->lock);
> > > > + if (enable)
> > > > + rzg2l_irqc_unmask_once(priv, hw_irq);
> > > > + else
> > > > + rzg2l_irqc_mask_once(priv, hw_irq);
> > > > + raw_spin_unlock(&priv->lock);
> > > > }
> > > > }
> > > >
> > > > @@ -324,8 +427,8 @@ static struct syscore_ops rzg2l_irqc_syscore_ops
> > > > = { static const struct irq_chip irqc_chip = {
> > > > .name = "rzg2l-irqc",
> > > > .irq_eoi = rzg2l_irqc_eoi,
> > > > - .irq_mask = irq_chip_mask_parent,
> > > > - .irq_unmask = irq_chip_unmask_parent,
> > > > + .irq_mask = rzg2l_irqc_mask,
> > > > + .irq_unmask = rzg2l_irqc_unmask,
> > >
> > > I feel this will be clean, if we have
> > >
> > > static const struct irq_chip rzg2l_irqc_chip = {
> > > .name = "rzg2l-irqc",
> > > ...
> > > .irq_mask = irq_chip_mask_parent,
> > > .irq_unmask = irq_chip_unmask_parent,
> > > ....
> > > };
> > >
> > > static const struct irq_chip rzfive_irqc_chip = {
> > > .name = "rzfive-irqc",
> > > ...
> > > .irq_mask = rzfive_irqc_mask,
> > > .irq_unmask = rzfive_irqc_unmask,
> > > ....
> > > };
> > >
> > > And passing this in rzg2l_irqc_init() and rzfive_irqc_init(), see
> > > below
> > >
> > > return rzg2l_irqc_init_helper(node, parent, & rzg2l_irqc_chip); return
> > > rzg2l_irqc_init_helper(node, parent, & rzfive_irqc_chip);
> > >
> > If we do the above we are stuck with "struct irq_chip" as data, for further upcoming SoCs (for
> > example RZ/V2H) which have more features we need to pass custom data to handle these features.
>
> That time device data can be extended like below
>
> struct rz_g2l_irq_chip {
> struct irq_chip;
> void *data; /* custom data */
> }
>
Ok, but i'll wait for Geert to come back on this as Geert suggested to
me to do it this way.
Cheers,
Prabhakar
^ permalink raw reply
* Re: [PATCH v9 04/10] media: bcm2835-unicam: Add support for CCP2/CSI2 camera interface
From: Hans Verkuil @ 2024-04-04 13:36 UTC (permalink / raw)
To: Laurent Pinchart, linux-media
Cc: Dave Stevenson, David Plowman, Jean-Michel Hautbois,
Naushir Patuck, Sakari Ailus, kernel-list, linux-rpi-kernel,
Florian Fainelli, Ray Jui, Scott Branden,
bcm-kernel-feedback-list, Conor Dooley, Krzysztof Kozlowski,
Rob Herring, devicetree
In-Reply-To: <20240402000424.4650-5-laurent.pinchart@ideasonboard.com>
Just two minor comments:
On 02/04/2024 02:04, Laurent Pinchart wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
>
> Add a driver for the Unicam camera receiver block on BCM283x processors.
> It is represented as two video device nodes: unicam-image and
> unicam-embedded which are connected to an internal subdev (named
> unicam-subdev) in order to manage streams routing.
>
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Co-developed-by: Naushir Patuck <naush@raspberrypi.com>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Co-developed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v8:
>
> - Use MIPI_CSI2_DT_* macros
> - Disable image stream on start error
> - Move hardware configuration to unicam_sd_enable_streams()
> - Get VC and DT from frame descriptor
> - Don't cache fmtinfo in unicam_node
> - Calculate line_int_freq based on subdev format
> - Fix try_fmt_meta regression from v5
>
> Changes since v7:
>
> - Indentation, line wrap and white space fixes
> - Add copyright notice for Ideas on Board
> - Use unsigned values for shifts in macro
> - Replace condition and assignment with max()
> - Don't set the serial to an empty string manually
> - Don't use loop for lane clocks setting
> - Store PUM value in struct unicam_format_info
>
> Changes since v6:
>
> - Fix typos in comments
> - Drop outdated comment
> - Indentation fixes
> - Turn UNICAM_SD_PAD_* into an enum
> - Drop unicam_format_info.metadata_fmt
> - Remove unneeded dev_dbg()
> - Report meta frame sizes as V4L2_FRMSIZE_TYPE_STEPWISE
> - Drop stray semicolons
> - Set V4L2_FMT_FLAG_META_LINE_BASED for metadata format
> - Rename error label
> - Use .get_mbus_config() to get number of data lanes
> - Drop minimum of 3 buffers in .queue_setup()
> - Merge locks for the two video nodes
> - Rework start/stop to avoid race conditions
>
> Changes since v5:
>
> - Move to drivers/media/platform/broadcom/
> - Port to the upstream V4L2 streams API
> - Rebase on latest metadata API proposal
> - Add missing error message
> - Drop unneeded documentation block for unicam_isr()
> - Drop unneeded dev_dbg() and dev_err() messages
> - Drop unneeded streams_mask and fmt checks
> - Drop unused unicam_sd_pad_is_sink()
> - Drop unneeded includes
> - Drop v4l2_ctrl_subscribe_event() call
> - Use pm_runtime_resume_and_get()
> - Indentation and line wrap fixes
> - Let the framework set bus_info
> - Use v4l2_fwnode_endpoint_parse()
> - Fix media device cleanup
> - Drop lane reordering checks
> - Fix subdev state locking
> - Drop extra debug messages
> - Move clock handling to runtime PM handlers
> - Reorder functions
> - Rename init functions for more clarity
> - Initialize runtime PM earlier
> - Clarify error messages
> - Simplify subdev init with local variable
> - Fix subdev cleanup
> - Fix typos and indentation
> - Don't initialize local variables needlessly
> - Simplify num lanes check
> - Fix metadata handling in subdev set_fmt
> - Drop manual fallback to .s_stream()
> - Pass v4l2_pix_format to unicam_calc_format_size_bpl()
> - Simplify unicam_set_default_format()
> - Fix default format settings
> - Add busy check in unicam_s_fmt_meta()
> - Add missing \n at end of format strings
> - Fix metadata handling in subdev set_fmt
> - Fix locking when starting streaming
> - Return buffers from start streaming fails
> - Fix format validation for metadata node
> - Use video_device_pipeline_{start,stop}() helpers
> - Simplify format enumeration
> - Drop unset variable
> - Update MAINTAINERS entry
> - Update to the upstream v4l2_async_nf API
> - Update to the latest subdev routing API
> - Update to the latest subdev state API
> - Move from subdev .init_cfg() to .init_state()
> - Update to the latest videobuf2 API
> - Fix v4l2_subdev_enable_streams() error check
> - Use correct pad for the connected subdev
> - Return buffers to vb2 when start streaming fails
> - Improve debugging in start streaming handler
> - Simplify DMA address management
> - Drop comment about bcm2835-camera driver
> - Clarify comments that explain min/max sizes
> - Pass v4l2_pix_format to unicam_try_fmt()
> - Drop unneeded local variables
> - Rename image-related constants and functions
> - Turn unicam_fmt.metadata_fmt into bool
> - Rename unicam_fmt to unicam_format_info
> - Rename unicam_format_info variables to fmtinfo
> - Rename unicam_node.v_fmt to fmt
> - Add metadata formats for RAW10, RAW12 and RAW14
> - Make metadata formats line-based
> - Validate format on metadata video device
> - Add Co-devlopped-by tags
>
> Changes since v3:
>
> - Add the vendor prefix for DT name
> - Use the reg-names in DT parsing
> - Remove MAINTAINERS entry
>
> Changes since v2:
>
> - Change code organization
> - Remove unused variables
> - Correct the fmt_meta functions
> - Rewrite the start/stop streaming
> - You can now start the image node alone, but not the metadata one
> - The buffers are allocated per-node
> - only the required stream is started, if the route exists and is
> enabled
> - Prefix the macros with UNICAM_ to not have too generic names
> - Drop colorspace support
>
> Changes since v1:
>
> - Replace the unicam_{info,debug,error} macros with dev_*()
> ---
> MAINTAINERS | 1 +
> drivers/media/platform/Kconfig | 1 +
> drivers/media/platform/Makefile | 1 +
> drivers/media/platform/broadcom/Kconfig | 23 +
> drivers/media/platform/broadcom/Makefile | 3 +
> .../platform/broadcom/bcm2835-unicam-regs.h | 246 ++
> .../media/platform/broadcom/bcm2835-unicam.c | 2745 +++++++++++++++++
> 7 files changed, 3020 insertions(+)
> create mode 100644 drivers/media/platform/broadcom/Kconfig
> create mode 100644 drivers/media/platform/broadcom/Makefile
> create mode 100644 drivers/media/platform/broadcom/bcm2835-unicam-regs.h
> create mode 100644 drivers/media/platform/broadcom/bcm2835-unicam.c
>
<snip>
> diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> new file mode 100644
> index 000000000000..1418f209d6ad
> --- /dev/null
> +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> @@ -0,0 +1,2745 @@
<snip>
> +static int unicam_start_streaming(struct vb2_queue *vq, unsigned int count)
> +{
> + struct unicam_node *node = vb2_get_drv_priv(vq);
> + struct unicam_device *unicam = node->dev;
> + struct unicam_buffer *buf;
> + struct media_pipeline_pad_iter iter;
> + struct media_pad *pad;
> + unsigned long flags;
> + int ret;
> +
> + dev_dbg(unicam->dev, "Starting stream on %s device\n",
> + is_metadata_node(node) ? "metadata" : "image");
> +
> + /*
> + * Start the pipeline. This validates all links, and populates the
> + * pipeline structure.
> + */
> + ret = video_device_pipeline_start(&node->video_dev, &unicam->pipe.pipe);
> + if (ret < 0) {
> + dev_dbg(unicam->dev, "Failed to start media pipeline: %d\n", ret);
> + goto err_buffers;
> + }
> +
> + /*
> + * Determine which video nodes are included in the pipeline, and get the
> + * number of data lanes.
> + */
> + if (unicam->pipe.pipe.start_count == 1) {
> + unicam->pipe.nodes = 0;
> +
> + media_pipeline_for_each_pad(&unicam->pipe.pipe, &iter, pad) {
> + if (pad->entity != &unicam->subdev.sd.entity)
> + continue;
> +
> + if (pad->index == UNICAM_SD_PAD_SOURCE_IMAGE)
> + unicam->pipe.nodes |= BIT(UNICAM_IMAGE_NODE);
> + else if (pad->index == UNICAM_SD_PAD_SOURCE_METADATA)
> + unicam->pipe.nodes |= BIT(UNICAM_METADATA_NODE);
> + }
> +
> + if (!(unicam->pipe.nodes & BIT(UNICAM_IMAGE_NODE))) {
> + dev_dbg(unicam->dev,
> + "Pipeline does not include image node\n");
> + ret = -EPIPE;
> + goto err_pipeline;
> + }
> +
> + ret = unicam_num_data_lanes(unicam);
> + if (ret < 0)
> + goto err_pipeline;
> +
> + unicam->pipe.num_data_lanes = ret;
> +
> + dev_dbg(unicam->dev, "Running with %u data lanes, nodes %u\n",
> + unicam->pipe.num_data_lanes, unicam->pipe.nodes);
> + }
> +
> + node->streaming = true;
Hmm, do you need to keep track of this here? Can't you use vb2_start_streaming_called()?
Generally I dislike keeping track of the same information in two places.
> +
> + /* Arm the node with the first buffer from the DMA queue. */
> + spin_lock_irqsave(&node->dma_queue_lock, flags);
> + buf = list_first_entry(&node->dma_queue, struct unicam_buffer, list);
> + node->cur_frm = buf;
> + node->next_frm = buf;
> + list_del(&buf->list);
> + spin_unlock_irqrestore(&node->dma_queue_lock, flags);
> +
> + /*
> + * Wait for all the video devices in the pipeline to have been started
> + * before starting the hardware. In the general case, this would
> + * prevent capturing multiple streams independently. However, the
> + * Unicam DMA engines are not generic, they have been designed to
> + * capture image data and embedded data from the same camera sensor.
> + * Not only does the main use case not benefit from independent
> + * capture, it requires proper synchronization of the streams at start
> + * time.
> + */
> + if (unicam->pipe.pipe.start_count < hweight32(unicam->pipe.nodes))
> + return 0;
> +
> + ret = pm_runtime_resume_and_get(unicam->dev);
> + if (ret < 0) {
> + dev_err(unicam->dev, "PM runtime resume failed: %d\n", ret);
> + goto err_pipeline;
> + }
> +
> + /* Enable the streams on the source. */
> + ret = v4l2_subdev_enable_streams(&unicam->subdev.sd,
> + UNICAM_SD_PAD_SOURCE_IMAGE,
> + BIT(0));
> + if (ret < 0) {
> + dev_err(unicam->dev, "stream on failed in subdev\n");
> + goto err_pm_put;
> + }
> +
> + if (unicam->pipe.nodes & BIT(UNICAM_METADATA_NODE)) {
> + ret = v4l2_subdev_enable_streams(&unicam->subdev.sd,
> + UNICAM_SD_PAD_SOURCE_METADATA,
> + BIT(0));
> + if (ret < 0) {
> + dev_err(unicam->dev, "stream on failed in subdev\n");
> + goto err_disable_streams;
> + }
> + }
> +
> + return 0;
> +
> +err_disable_streams:
> + v4l2_subdev_disable_streams(&unicam->subdev.sd,
> + UNICAM_SD_PAD_SOURCE_IMAGE, BIT(0));
> +err_pm_put:
> + pm_runtime_put_sync(unicam->dev);
> +err_pipeline:
> + video_device_pipeline_stop(&node->video_dev);
> +err_buffers:
> + unicam_return_buffers(node, VB2_BUF_STATE_QUEUED);
> + node->streaming = false;
> + return ret;
> +}
<snip>
> +static void unicam_unregister_nodes(struct unicam_device *unicam)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(unicam->node); i++) {
> + struct unicam_node *node = &unicam->node[i];
> +
> + if (node->dummy_buf_cpu_addr)
> + dma_free_coherent(unicam->dev, node->dummy_buf.size,
> + node->dummy_buf_cpu_addr,
> + node->dummy_buf.dma_addr);
> +
> + if (node->registered) {
> + video_unregister_device(&node->video_dev);
Call vb2_video_unregister_device instead of video_unregister_device.
That ensures that unregistering the device will also stop streaming.
See comments in include/media/videobuf2-v4l2.h.
> + node->registered = false;
> + }
> + }
> +}
<snip>
Regards,
Hans
^ permalink raw reply
* Re: [PATCH v7 00/37] Device Tree support for SH7751 based board
From: Bjorn Helgaas @ 2024-04-04 13:46 UTC (permalink / raw)
To: Yoshinori Sato
Cc: linux-sh, Damien Le Moal, Niklas Cassel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Michael Turquette, Stephen Boyd, David Airlie, Daniel Vetter,
Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
Thomas Gleixner, Bjorn Helgaas, Lorenzo Pieralisi,
Krzysztof Wilczyński, Greg Kroah-Hartman, Jiri Slaby,
Magnus Damm, Daniel Lezcano, Rich Felker,
John Paul Adrian Glaubitz, Lee Jones, Helge Deller,
Heiko Stuebner, Shawn Guo, Sebastian Reichel, Chris Morgan,
Linus Walleij, Arnd Bergmann, David Rientjes, Hyeonggon Yoo,
Vlastimil Babka, Baoquan He, Andrew Morton, Guenter Roeck,
Kefeng Wang, Stephen Rothwell, Javier Martinez Canillas, Guo Ren,
Azeem Shaikh, Max Filippov, Jonathan Corbet, Jacky Huang,
Herve Codina, Manikanta Guntupalli, Anup Patel, Biju Das,
Uwe Kleine-König, Sam Ravnborg, Sergey Shtylyov,
Laurent Pinchart, linux-ide, devicetree, linux-kernel,
linux-renesas-soc, linux-clk, dri-devel, linux-pci, linux-serial,
linux-fbdev
In-Reply-To: <cover.1712205900.git.ysato@users.sourceforge.jp>
On Thu, Apr 04, 2024 at 01:59:25PM +0900, Yoshinori Sato wrote:
> This is an updated version of something I wrote about 7 years ago.
> Minimum support for R2D-plus and LANDISK.
> I think R2D-1 will work if you add AX88796 to dts.
> And board-specific functions and SCI's SPI functions are not supported.
My comments/questions from
https://lore.kernel.org/r/20231120181600.GA205977@bhelgaas
https://lore.kernel.org/r/20231016172742.GA1215127@bhelgaas
still apply.
^ permalink raw reply
* Re: [PATCH v3] arm64: dts: ti: k3-am62p: use eFuse MAC Address for CPSW3G Port 1
From: Krzysztof Kozlowski @ 2024-04-04 13:50 UTC (permalink / raw)
To: Siddharth Vadapalli, nm, vigneshr, kristo, robh, krzk+dt,
conor+dt
Cc: devicetree, linux-kernel, linux-arm-kernel, srk
In-Reply-To: <20240404124614.891416-1-s-vadapalli@ti.com>
On 04/04/2024 14:46, Siddharth Vadapalli wrote:
> Add the "ethernet-mac-syscon" node within "wkup_conf" node corresponding to
> the CTRLMMR_MAC_IDx registers within the CTRL_MMR space. Assign the
> compatible "ti,am62p-cpsw-mac-efuse" to enable "syscon_regmap" operations
> on these registers. The MAC Address programmed in the eFuse is accessible
> through the CTRLMMR_MAC_IDx registers. The "ti,syscon-efuse" device-tree
> property points to the CTRLMMR_MAC_IDx registers, allowing the CPSW driver
> to fetch the MAC Address and assign it to the network interface associated
> with CPSW3G MAC Port 1.
>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [RESEND PATCH v9 2/4] dt-bindings: stm32: update DT bingding for stm32mp25
From: Rob Herring @ 2024-04-04 13:52 UTC (permalink / raw)
To: gabriel.fernandez
Cc: Michael Turquette, Stephen Boyd, Krzysztof Kozlowski,
Conor Dooley, Maxime Coquelin, Alexandre Torgue, Philipp Zabel,
linux-clk, devicetree, linux-stm32, linux-arm-kernel,
linux-kernel
In-Reply-To: <20240402125312.277052-3-gabriel.fernandez@foss.st.com>
On Tue, Apr 02, 2024 at 02:53:10PM +0200, gabriel.fernandez@foss.st.com wrote:
> From: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
>
> Now RCC driver use '.index' of clk_parent_data struct to define a parent.
> The majority of parents are SCMI clocks, then dt-bindings must be fixed.
This is an ABI change. Please make that clear and justify why that is
okay. Changing a driver is not a valid reason. What about other drivers
besides Linux?
>
> Fixes: b5be49db3d47 ("dt-bindings: stm32: add clocks and reset binding for stm32mp25 platform")
>
Should not have a blank line here.
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
> ---
Please put version history for a patch within the patch here.
> .../bindings/clock/st,stm32mp25-rcc.yaml | 171 ++++++++++++++++--
> 1 file changed, 155 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/clock/st,stm32mp25-rcc.yaml b/Documentation/devicetree/bindings/clock/st,stm32mp25-rcc.yaml
> index 7732e79a42b9..57bd4e7157bd 100644
> --- a/Documentation/devicetree/bindings/clock/st,stm32mp25-rcc.yaml
> +++ b/Documentation/devicetree/bindings/clock/st,stm32mp25-rcc.yaml
> @@ -38,22 +38,87 @@ properties:
> - description: CK_SCMI_MSI Low Power Internal oscillator (~ 4 MHz or ~ 16 MHz)
> - description: CK_SCMI_LSE Low Speed External oscillator (32 KHz)
> - description: CK_SCMI_LSI Low Speed Internal oscillator (~ 32 KHz)
> -
> - clock-names:
> - items:
> - - const: hse
> - - const: hsi
> - - const: msi
> - - const: lse
> - - const: lsi
> -
> + - description: CK_SCMI_HSE_DIV2 CK_SCMI_HSE divided by 2 (coud be gated)
> + - description: CK_SCMI_ICN_HS_MCU High Speed interconnect bus clock
> + - description: CK_SCMI_ICN_LS_MCU Low Speed interconnect bus clock
> + - description: CK_SCMI_ICN_SDMMC SDMMC interconnect bus clock
> + - description: CK_SCMI_ICN_DDR DDR interconnect bus clock
> + - description: CK_SCMI_ICN_DISPLAY Display interconnect bus clock
> + - description: CK_SCMI_ICN_HSL HSL interconnect bus clock
> + - description: CK_SCMI_ICN_NIC NIC interconnect bus clock
> + - description: CK_SCMI_ICN_VID Video interconnect bus clock
> + - description: CK_SCMI_FLEXGEN_07 flexgen clock 7
> + - description: CK_SCMI_FLEXGEN_08 flexgen clock 8
> + - description: CK_SCMI_FLEXGEN_09 flexgen clock 9
> + - description: CK_SCMI_FLEXGEN_10 flexgen clock 10
> + - description: CK_SCMI_FLEXGEN_11 flexgen clock 11
> + - description: CK_SCMI_FLEXGEN_12 flexgen clock 12
> + - description: CK_SCMI_FLEXGEN_13 flexgen clock 13
> + - description: CK_SCMI_FLEXGEN_14 flexgen clock 14
> + - description: CK_SCMI_FLEXGEN_15 flexgen clock 15
> + - description: CK_SCMI_FLEXGEN_16 flexgen clock 16
> + - description: CK_SCMI_FLEXGEN_17 flexgen clock 17
> + - description: CK_SCMI_FLEXGEN_18 flexgen clock 18
> + - description: CK_SCMI_FLEXGEN_19 flexgen clock 19
> + - description: CK_SCMI_FLEXGEN_20 flexgen clock 20
> + - description: CK_SCMI_FLEXGEN_21 flexgen clock 21
> + - description: CK_SCMI_FLEXGEN_22 flexgen clock 22
> + - description: CK_SCMI_FLEXGEN_23 flexgen clock 23
> + - description: CK_SCMI_FLEXGEN_24 flexgen clock 24
> + - description: CK_SCMI_FLEXGEN_25 flexgen clock 25
> + - description: CK_SCMI_FLEXGEN_26 flexgen clock 26
> + - description: CK_SCMI_FLEXGEN_27 flexgen clock 27
> + - description: CK_SCMI_FLEXGEN_28 flexgen clock 28
> + - description: CK_SCMI_FLEXGEN_29 flexgen clock 29
> + - description: CK_SCMI_FLEXGEN_30 flexgen clock 30
> + - description: CK_SCMI_FLEXGEN_31 flexgen clock 31
> + - description: CK_SCMI_FLEXGEN_32 flexgen clock 32
> + - description: CK_SCMI_FLEXGEN_33 flexgen clock 33
> + - description: CK_SCMI_FLEXGEN_34 flexgen clock 34
> + - description: CK_SCMI_FLEXGEN_35 flexgen clock 35
> + - description: CK_SCMI_FLEXGEN_36 flexgen clock 36
> + - description: CK_SCMI_FLEXGEN_37 flexgen clock 37
> + - description: CK_SCMI_FLEXGEN_38 flexgen clock 38
> + - description: CK_SCMI_FLEXGEN_39 flexgen clock 39
> + - description: CK_SCMI_FLEXGEN_40 flexgen clock 40
> + - description: CK_SCMI_FLEXGEN_41 flexgen clock 41
> + - description: CK_SCMI_FLEXGEN_42 flexgen clock 42
> + - description: CK_SCMI_FLEXGEN_43 flexgen clock 43
> + - description: CK_SCMI_FLEXGEN_44 flexgen clock 44
> + - description: CK_SCMI_FLEXGEN_45 flexgen clock 45
> + - description: CK_SCMI_FLEXGEN_46 flexgen clock 46
> + - description: CK_SCMI_FLEXGEN_47 flexgen clock 47
> + - description: CK_SCMI_FLEXGEN_48 flexgen clock 48
> + - description: CK_SCMI_FLEXGEN_49 flexgen clock 49
> + - description: CK_SCMI_FLEXGEN_50 flexgen clock 50
> + - description: CK_SCMI_FLEXGEN_51 flexgen clock 51
> + - description: CK_SCMI_FLEXGEN_52 flexgen clock 52
> + - description: CK_SCMI_FLEXGEN_53 flexgen clock 53
> + - description: CK_SCMI_FLEXGEN_54 flexgen clock 54
> + - description: CK_SCMI_FLEXGEN_55 flexgen clock 55
> + - description: CK_SCMI_FLEXGEN_56 flexgen clock 56
> + - description: CK_SCMI_FLEXGEN_57 flexgen clock 57
> + - description: CK_SCMI_FLEXGEN_58 flexgen clock 58
> + - description: CK_SCMI_FLEXGEN_59 flexgen clock 59
> + - description: CK_SCMI_FLEXGEN_60 flexgen clock 60
> + - description: CK_SCMI_FLEXGEN_61 flexgen clock 61
> + - description: CK_SCMI_FLEXGEN_62 flexgen clock 62
> + - description: CK_SCMI_FLEXGEN_63 flexgen clock 63
> + - description: CK_SCMI_ICN_APB1 Peripheral bridge 1
> + - description: CK_SCMI_ICN_APB2 Peripheral bridge 2
> + - description: CK_SCMI_ICN_APB3 Peripheral bridge 3
> + - description: CK_SCMI_ICN_APB4 Peripheral bridge 4
> + - description: CK_SCMI_ICN_APBDBG Peripheral bridge for degub
> + - description: CK_SCMI_TIMG1 Peripheral bridge for timer1
> + - description: CK_SCMI_TIMG2 Peripheral bridge for timer2
> + - description: CK_SCMI_PLL3 PLL3 clock
> + - description: clk_dsi_txbyte DSI byte clock
Need a blank line here.
> required:
> - compatible
> - reg
> - '#clock-cells'
> - '#reset-cells'
> - clocks
> - - clock-names
>
> additionalProperties: false
>
> @@ -66,11 +131,85 @@ examples:
> reg = <0x44200000 0x10000>;
> #clock-cells = <1>;
> #reset-cells = <1>;
> - clock-names = "hse", "hsi", "msi", "lse", "lsi";
> - clocks = <&scmi_clk CK_SCMI_HSE>,
> - <&scmi_clk CK_SCMI_HSI>,
> - <&scmi_clk CK_SCMI_MSI>,
> - <&scmi_clk CK_SCMI_LSE>,
> - <&scmi_clk CK_SCMI_LSI>;
> + clocks = <&scmi_clk CK_SCMI_HSE>,
> + <&scmi_clk CK_SCMI_HSI>,
> + <&scmi_clk CK_SCMI_MSI>,
> + <&scmi_clk CK_SCMI_LSE>,
> + <&scmi_clk CK_SCMI_LSI>,
> + <&scmi_clk CK_SCMI_HSE_DIV2>,
> + <&scmi_clk CK_SCMI_ICN_HS_MCU>,
> + <&scmi_clk CK_SCMI_ICN_LS_MCU>,
> + <&scmi_clk CK_SCMI_ICN_SDMMC>,
> + <&scmi_clk CK_SCMI_ICN_DDR>,
> + <&scmi_clk CK_SCMI_ICN_DISPLAY>,
> + <&scmi_clk CK_SCMI_ICN_HSL>,
> + <&scmi_clk CK_SCMI_ICN_NIC>,
> + <&scmi_clk CK_SCMI_ICN_VID>,
> + <&scmi_clk CK_SCMI_FLEXGEN_07>,
> + <&scmi_clk CK_SCMI_FLEXGEN_08>,
> + <&scmi_clk CK_SCMI_FLEXGEN_09>,
> + <&scmi_clk CK_SCMI_FLEXGEN_10>,
> + <&scmi_clk CK_SCMI_FLEXGEN_11>,
> + <&scmi_clk CK_SCMI_FLEXGEN_12>,
> + <&scmi_clk CK_SCMI_FLEXGEN_13>,
> + <&scmi_clk CK_SCMI_FLEXGEN_14>,
> + <&scmi_clk CK_SCMI_FLEXGEN_15>,
> + <&scmi_clk CK_SCMI_FLEXGEN_16>,
> + <&scmi_clk CK_SCMI_FLEXGEN_17>,
> + <&scmi_clk CK_SCMI_FLEXGEN_18>,
> + <&scmi_clk CK_SCMI_FLEXGEN_19>,
> + <&scmi_clk CK_SCMI_FLEXGEN_20>,
> + <&scmi_clk CK_SCMI_FLEXGEN_21>,
> + <&scmi_clk CK_SCMI_FLEXGEN_22>,
> + <&scmi_clk CK_SCMI_FLEXGEN_23>,
> + <&scmi_clk CK_SCMI_FLEXGEN_24>,
> + <&scmi_clk CK_SCMI_FLEXGEN_25>,
> + <&scmi_clk CK_SCMI_FLEXGEN_26>,
> + <&scmi_clk CK_SCMI_FLEXGEN_27>,
> + <&scmi_clk CK_SCMI_FLEXGEN_28>,
> + <&scmi_clk CK_SCMI_FLEXGEN_29>,
> + <&scmi_clk CK_SCMI_FLEXGEN_30>,
> + <&scmi_clk CK_SCMI_FLEXGEN_31>,
> + <&scmi_clk CK_SCMI_FLEXGEN_32>,
> + <&scmi_clk CK_SCMI_FLEXGEN_33>,
> + <&scmi_clk CK_SCMI_FLEXGEN_34>,
> + <&scmi_clk CK_SCMI_FLEXGEN_35>,
> + <&scmi_clk CK_SCMI_FLEXGEN_36>,
> + <&scmi_clk CK_SCMI_FLEXGEN_37>,
> + <&scmi_clk CK_SCMI_FLEXGEN_38>,
> + <&scmi_clk CK_SCMI_FLEXGEN_39>,
> + <&scmi_clk CK_SCMI_FLEXGEN_40>,
> + <&scmi_clk CK_SCMI_FLEXGEN_41>,
> + <&scmi_clk CK_SCMI_FLEXGEN_42>,
> + <&scmi_clk CK_SCMI_FLEXGEN_43>,
> + <&scmi_clk CK_SCMI_FLEXGEN_44>,
> + <&scmi_clk CK_SCMI_FLEXGEN_45>,
> + <&scmi_clk CK_SCMI_FLEXGEN_46>,
> + <&scmi_clk CK_SCMI_FLEXGEN_47>,
> + <&scmi_clk CK_SCMI_FLEXGEN_48>,
> + <&scmi_clk CK_SCMI_FLEXGEN_49>,
> + <&scmi_clk CK_SCMI_FLEXGEN_50>,
> + <&scmi_clk CK_SCMI_FLEXGEN_51>,
> + <&scmi_clk CK_SCMI_FLEXGEN_52>,
> + <&scmi_clk CK_SCMI_FLEXGEN_53>,
> + <&scmi_clk CK_SCMI_FLEXGEN_54>,
> + <&scmi_clk CK_SCMI_FLEXGEN_55>,
> + <&scmi_clk CK_SCMI_FLEXGEN_56>,
> + <&scmi_clk CK_SCMI_FLEXGEN_57>,
> + <&scmi_clk CK_SCMI_FLEXGEN_58>,
> + <&scmi_clk CK_SCMI_FLEXGEN_59>,
> + <&scmi_clk CK_SCMI_FLEXGEN_60>,
> + <&scmi_clk CK_SCMI_FLEXGEN_61>,
> + <&scmi_clk CK_SCMI_FLEXGEN_62>,
> + <&scmi_clk CK_SCMI_FLEXGEN_63>,
> + <&scmi_clk CK_SCMI_ICN_APB1>,
> + <&scmi_clk CK_SCMI_ICN_APB2>,
> + <&scmi_clk CK_SCMI_ICN_APB3>,
> + <&scmi_clk CK_SCMI_ICN_APB4>,
> + <&scmi_clk CK_SCMI_ICN_APBDBG>,
> + <&scmi_clk CK_SCMI_TIMG1>,
> + <&scmi_clk CK_SCMI_TIMG2>,
> + <&scmi_clk CK_SCMI_PLL3>,
> + <&clk_dsi_txbyte>;
> };
> ...
> --
> 2.25.1
>
^ permalink raw reply
* Re: [PATCH v9 04/10] media: bcm2835-unicam: Add support for CCP2/CSI2 camera interface
From: Laurent Pinchart @ 2024-04-04 13:53 UTC (permalink / raw)
To: Hans Verkuil
Cc: linux-media, Dave Stevenson, David Plowman, Jean-Michel Hautbois,
Naushir Patuck, Sakari Ailus, kernel-list, linux-rpi-kernel,
Florian Fainelli, Ray Jui, Scott Branden,
bcm-kernel-feedback-list, Conor Dooley, Krzysztof Kozlowski,
Rob Herring, devicetree
In-Reply-To: <11107704-46a7-4228-99da-55389e210553@xs4all.nl>
Hi Hans,
On Thu, Apr 04, 2024 at 03:36:12PM +0200, Hans Verkuil wrote:
> Just two minor comments:
For a new large driver, I'll take this as a sign we're very close to
completion :-)
> On 02/04/2024 02:04, Laurent Pinchart wrote:
> > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> >
> > Add a driver for the Unicam camera receiver block on BCM283x processors.
> > It is represented as two video device nodes: unicam-image and
> > unicam-embedded which are connected to an internal subdev (named
> > unicam-subdev) in order to manage streams routing.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Co-developed-by: Naushir Patuck <naush@raspberrypi.com>
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Co-developed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v8:
> >
> > - Use MIPI_CSI2_DT_* macros
> > - Disable image stream on start error
> > - Move hardware configuration to unicam_sd_enable_streams()
> > - Get VC and DT from frame descriptor
> > - Don't cache fmtinfo in unicam_node
> > - Calculate line_int_freq based on subdev format
> > - Fix try_fmt_meta regression from v5
> >
> > Changes since v7:
> >
> > - Indentation, line wrap and white space fixes
> > - Add copyright notice for Ideas on Board
> > - Use unsigned values for shifts in macro
> > - Replace condition and assignment with max()
> > - Don't set the serial to an empty string manually
> > - Don't use loop for lane clocks setting
> > - Store PUM value in struct unicam_format_info
> >
> > Changes since v6:
> >
> > - Fix typos in comments
> > - Drop outdated comment
> > - Indentation fixes
> > - Turn UNICAM_SD_PAD_* into an enum
> > - Drop unicam_format_info.metadata_fmt
> > - Remove unneeded dev_dbg()
> > - Report meta frame sizes as V4L2_FRMSIZE_TYPE_STEPWISE
> > - Drop stray semicolons
> > - Set V4L2_FMT_FLAG_META_LINE_BASED for metadata format
> > - Rename error label
> > - Use .get_mbus_config() to get number of data lanes
> > - Drop minimum of 3 buffers in .queue_setup()
> > - Merge locks for the two video nodes
> > - Rework start/stop to avoid race conditions
> >
> > Changes since v5:
> >
> > - Move to drivers/media/platform/broadcom/
> > - Port to the upstream V4L2 streams API
> > - Rebase on latest metadata API proposal
> > - Add missing error message
> > - Drop unneeded documentation block for unicam_isr()
> > - Drop unneeded dev_dbg() and dev_err() messages
> > - Drop unneeded streams_mask and fmt checks
> > - Drop unused unicam_sd_pad_is_sink()
> > - Drop unneeded includes
> > - Drop v4l2_ctrl_subscribe_event() call
> > - Use pm_runtime_resume_and_get()
> > - Indentation and line wrap fixes
> > - Let the framework set bus_info
> > - Use v4l2_fwnode_endpoint_parse()
> > - Fix media device cleanup
> > - Drop lane reordering checks
> > - Fix subdev state locking
> > - Drop extra debug messages
> > - Move clock handling to runtime PM handlers
> > - Reorder functions
> > - Rename init functions for more clarity
> > - Initialize runtime PM earlier
> > - Clarify error messages
> > - Simplify subdev init with local variable
> > - Fix subdev cleanup
> > - Fix typos and indentation
> > - Don't initialize local variables needlessly
> > - Simplify num lanes check
> > - Fix metadata handling in subdev set_fmt
> > - Drop manual fallback to .s_stream()
> > - Pass v4l2_pix_format to unicam_calc_format_size_bpl()
> > - Simplify unicam_set_default_format()
> > - Fix default format settings
> > - Add busy check in unicam_s_fmt_meta()
> > - Add missing \n at end of format strings
> > - Fix metadata handling in subdev set_fmt
> > - Fix locking when starting streaming
> > - Return buffers from start streaming fails
> > - Fix format validation for metadata node
> > - Use video_device_pipeline_{start,stop}() helpers
> > - Simplify format enumeration
> > - Drop unset variable
> > - Update MAINTAINERS entry
> > - Update to the upstream v4l2_async_nf API
> > - Update to the latest subdev routing API
> > - Update to the latest subdev state API
> > - Move from subdev .init_cfg() to .init_state()
> > - Update to the latest videobuf2 API
> > - Fix v4l2_subdev_enable_streams() error check
> > - Use correct pad for the connected subdev
> > - Return buffers to vb2 when start streaming fails
> > - Improve debugging in start streaming handler
> > - Simplify DMA address management
> > - Drop comment about bcm2835-camera driver
> > - Clarify comments that explain min/max sizes
> > - Pass v4l2_pix_format to unicam_try_fmt()
> > - Drop unneeded local variables
> > - Rename image-related constants and functions
> > - Turn unicam_fmt.metadata_fmt into bool
> > - Rename unicam_fmt to unicam_format_info
> > - Rename unicam_format_info variables to fmtinfo
> > - Rename unicam_node.v_fmt to fmt
> > - Add metadata formats for RAW10, RAW12 and RAW14
> > - Make metadata formats line-based
> > - Validate format on metadata video device
> > - Add Co-devlopped-by tags
> >
> > Changes since v3:
> >
> > - Add the vendor prefix for DT name
> > - Use the reg-names in DT parsing
> > - Remove MAINTAINERS entry
> >
> > Changes since v2:
> >
> > - Change code organization
> > - Remove unused variables
> > - Correct the fmt_meta functions
> > - Rewrite the start/stop streaming
> > - You can now start the image node alone, but not the metadata one
> > - The buffers are allocated per-node
> > - only the required stream is started, if the route exists and is
> > enabled
> > - Prefix the macros with UNICAM_ to not have too generic names
> > - Drop colorspace support
> >
> > Changes since v1:
> >
> > - Replace the unicam_{info,debug,error} macros with dev_*()
> > ---
> > MAINTAINERS | 1 +
> > drivers/media/platform/Kconfig | 1 +
> > drivers/media/platform/Makefile | 1 +
> > drivers/media/platform/broadcom/Kconfig | 23 +
> > drivers/media/platform/broadcom/Makefile | 3 +
> > .../platform/broadcom/bcm2835-unicam-regs.h | 246 ++
> > .../media/platform/broadcom/bcm2835-unicam.c | 2745 +++++++++++++++++
> > 7 files changed, 3020 insertions(+)
> > create mode 100644 drivers/media/platform/broadcom/Kconfig
> > create mode 100644 drivers/media/platform/broadcom/Makefile
> > create mode 100644 drivers/media/platform/broadcom/bcm2835-unicam-regs.h
> > create mode 100644 drivers/media/platform/broadcom/bcm2835-unicam.c
> >
>
> <snip>
>
> > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > new file mode 100644
> > index 000000000000..1418f209d6ad
> > --- /dev/null
> > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > @@ -0,0 +1,2745 @@
>
> <snip>
>
> > +static int unicam_start_streaming(struct vb2_queue *vq, unsigned int count)
> > +{
> > + struct unicam_node *node = vb2_get_drv_priv(vq);
> > + struct unicam_device *unicam = node->dev;
> > + struct unicam_buffer *buf;
> > + struct media_pipeline_pad_iter iter;
> > + struct media_pad *pad;
> > + unsigned long flags;
> > + int ret;
> > +
> > + dev_dbg(unicam->dev, "Starting stream on %s device\n",
> > + is_metadata_node(node) ? "metadata" : "image");
> > +
> > + /*
> > + * Start the pipeline. This validates all links, and populates the
> > + * pipeline structure.
> > + */
> > + ret = video_device_pipeline_start(&node->video_dev, &unicam->pipe.pipe);
> > + if (ret < 0) {
> > + dev_dbg(unicam->dev, "Failed to start media pipeline: %d\n", ret);
> > + goto err_buffers;
> > + }
> > +
> > + /*
> > + * Determine which video nodes are included in the pipeline, and get the
> > + * number of data lanes.
> > + */
> > + if (unicam->pipe.pipe.start_count == 1) {
> > + unicam->pipe.nodes = 0;
> > +
> > + media_pipeline_for_each_pad(&unicam->pipe.pipe, &iter, pad) {
> > + if (pad->entity != &unicam->subdev.sd.entity)
> > + continue;
> > +
> > + if (pad->index == UNICAM_SD_PAD_SOURCE_IMAGE)
> > + unicam->pipe.nodes |= BIT(UNICAM_IMAGE_NODE);
> > + else if (pad->index == UNICAM_SD_PAD_SOURCE_METADATA)
> > + unicam->pipe.nodes |= BIT(UNICAM_METADATA_NODE);
> > + }
> > +
> > + if (!(unicam->pipe.nodes & BIT(UNICAM_IMAGE_NODE))) {
> > + dev_dbg(unicam->dev,
> > + "Pipeline does not include image node\n");
> > + ret = -EPIPE;
> > + goto err_pipeline;
> > + }
> > +
> > + ret = unicam_num_data_lanes(unicam);
> > + if (ret < 0)
> > + goto err_pipeline;
> > +
> > + unicam->pipe.num_data_lanes = ret;
> > +
> > + dev_dbg(unicam->dev, "Running with %u data lanes, nodes %u\n",
> > + unicam->pipe.num_data_lanes, unicam->pipe.nodes);
> > + }
> > +
> > + node->streaming = true;
>
> Hmm, do you need to keep track of this here? Can't you use vb2_start_streaming_called()?
>
> Generally I dislike keeping track of the same information in two places.
It looks like I can, I'll give it a try for the next version.
> > +
> > + /* Arm the node with the first buffer from the DMA queue. */
> > + spin_lock_irqsave(&node->dma_queue_lock, flags);
> > + buf = list_first_entry(&node->dma_queue, struct unicam_buffer, list);
> > + node->cur_frm = buf;
> > + node->next_frm = buf;
> > + list_del(&buf->list);
> > + spin_unlock_irqrestore(&node->dma_queue_lock, flags);
> > +
> > + /*
> > + * Wait for all the video devices in the pipeline to have been started
> > + * before starting the hardware. In the general case, this would
> > + * prevent capturing multiple streams independently. However, the
> > + * Unicam DMA engines are not generic, they have been designed to
> > + * capture image data and embedded data from the same camera sensor.
> > + * Not only does the main use case not benefit from independent
> > + * capture, it requires proper synchronization of the streams at start
> > + * time.
> > + */
> > + if (unicam->pipe.pipe.start_count < hweight32(unicam->pipe.nodes))
> > + return 0;
> > +
> > + ret = pm_runtime_resume_and_get(unicam->dev);
> > + if (ret < 0) {
> > + dev_err(unicam->dev, "PM runtime resume failed: %d\n", ret);
> > + goto err_pipeline;
> > + }
> > +
> > + /* Enable the streams on the source. */
> > + ret = v4l2_subdev_enable_streams(&unicam->subdev.sd,
> > + UNICAM_SD_PAD_SOURCE_IMAGE,
> > + BIT(0));
> > + if (ret < 0) {
> > + dev_err(unicam->dev, "stream on failed in subdev\n");
> > + goto err_pm_put;
> > + }
> > +
> > + if (unicam->pipe.nodes & BIT(UNICAM_METADATA_NODE)) {
> > + ret = v4l2_subdev_enable_streams(&unicam->subdev.sd,
> > + UNICAM_SD_PAD_SOURCE_METADATA,
> > + BIT(0));
> > + if (ret < 0) {
> > + dev_err(unicam->dev, "stream on failed in subdev\n");
> > + goto err_disable_streams;
> > + }
> > + }
> > +
> > + return 0;
> > +
> > +err_disable_streams:
> > + v4l2_subdev_disable_streams(&unicam->subdev.sd,
> > + UNICAM_SD_PAD_SOURCE_IMAGE, BIT(0));
> > +err_pm_put:
> > + pm_runtime_put_sync(unicam->dev);
> > +err_pipeline:
> > + video_device_pipeline_stop(&node->video_dev);
> > +err_buffers:
> > + unicam_return_buffers(node, VB2_BUF_STATE_QUEUED);
> > + node->streaming = false;
> > + return ret;
> > +}
>
> <snip>
>
> > +static void unicam_unregister_nodes(struct unicam_device *unicam)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(unicam->node); i++) {
> > + struct unicam_node *node = &unicam->node[i];
> > +
> > + if (node->dummy_buf_cpu_addr)
> > + dma_free_coherent(unicam->dev, node->dummy_buf.size,
> > + node->dummy_buf_cpu_addr,
> > + node->dummy_buf.dma_addr);
> > +
> > + if (node->registered) {
> > + video_unregister_device(&node->video_dev);
>
> Call vb2_video_unregister_device instead of video_unregister_device.
> That ensures that unregistering the device will also stop streaming.
> See comments in include/media/videobuf2-v4l2.h.
I had forgotten about that function. I'll do so.
> > + node->registered = false;
> > + }
> > + }
> > +}
>
> <snip>
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 1/1] dt-bindings: media: imx-jpeg: add clocks,clock-names,slot to fix warning
From: Frank Li @ 2024-04-04 13:54 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Fabio Estevam, Mirela Rabulea, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team,
open list:NXP i.MX 8QXP/8QM JPEG V4L2 DRIVER,
open list:NXP i.MX 8QXP/8QM JPEG V4L2 DRIVER,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
open list
In-Reply-To: <e78c8c2e-1c83-4492-9db9-08f06856a414@linaro.org>
On Thu, Apr 04, 2024 at 01:59:57PM +0200, Krzysztof Kozlowski wrote:
> On 04/04/2024 13:03, Fabio Estevam wrote:
> > On Thu, Apr 4, 2024 at 3:54 AM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >
> >> And for the clocks, instead pick up this patch:
> >> https://lore.kernel.org/all/20230721111020.1234278-3-alexander.stein@ew.tq-group.com/
> >
> > Or maybe this one:
> > https://lore.kernel.org/linux-devicetree/DB9PR04MB923493D0DA82C9EC4386BC2A8FF1A@DB9PR04MB9234.eurprd04.prod.outlook.com/
>
>
> Three people were fixing same clocks issue... and three or more people
> were trying to fix the slot property.
>
> This is really bad binding maintenance and driver upstreaming, NXP.
Thanks everyone help make imx dts and binding better. I should google
before send.
Patchwork for imx already was created.
https://patchwork.kernel.org/project/imx/list/?series=&submitter=150701&state=&q=&archive=&delegate=
I hope to patchwork help reduce duplicate work.
Frank
>
> Best regards,
> Krzysztof
>
^ permalink raw reply
* Re: [PATCH v3 23/25] drivers: media: i2c: imx258: Add support for powerdown gpio
From: Dave Stevenson @ 2024-04-04 14:12 UTC (permalink / raw)
To: Luigi311
Cc: Ondřej Jirman, Sakari Ailus, linux-media, jacopo.mondi,
mchehab, robh, krzysztof.kozlowski+dt, conor+dt, shawnguo,
s.hauer, kernel, festevam, devicetree, imx, linux-arm-kernel,
linux-kernel, pavel, phone-devel
In-Reply-To: <dd0e64c8-5eef-421a-9d9f-8a5865743369@luigi311.com>
Hi Luigi
On Wed, 3 Apr 2024 at 20:34, Luigi311 <git@luigi311.com> wrote:
>
> On 4/3/24 10:57, Ondřej Jirman wrote:
> > Hi Sakari and Luis,
> >
> > On Wed, Apr 03, 2024 at 04:25:41PM GMT, Sakari Ailus wrote:
> >> Hi Luis, Ondrej,
> >>
> >> On Wed, Apr 03, 2024 at 09:03:52AM -0600, git@luigi311.com wrote:
> >>> From: Luis Garcia <git@luigi311.com>
> >>>
> >>> On some boards powerdown signal needs to be deasserted for this
> >>> sensor to be enabled.
> >>>
> >>> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> >>> Signed-off-by: Luis Garcia <git@luigi311.com>
> >>> ---
> >>> drivers/media/i2c/imx258.c | 13 +++++++++++++
> >>> 1 file changed, 13 insertions(+)
> >>>
> >>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> >>> index 30352c33f63c..163f04f6f954 100644
> >>> --- a/drivers/media/i2c/imx258.c
> >>> +++ b/drivers/media/i2c/imx258.c
> >>> @@ -679,6 +679,8 @@ struct imx258 {
> >>> unsigned int lane_mode_idx;
> >>> unsigned int csi2_flags;
> >>>
> >>> + struct gpio_desc *powerdown_gpio;
> >>> +
> >>> /*
> >>> * Mutex for serialized access:
> >>> * Protect sensor module set pad format and start/stop streaming safely.
> >>> @@ -1213,6 +1215,8 @@ static int imx258_power_on(struct device *dev)
> >>> struct imx258 *imx258 = to_imx258(sd);
> >>> int ret;
> >>>
> >>> + gpiod_set_value_cansleep(imx258->powerdown_gpio, 0);
> >>
> >> What does the spec say? Should this really happen before switching on the
> >> supplies below?
> >
> > There's no powerdown input in the IMX258 manual. The manual only mentions
> > that XCLR (reset) should be held low during power on.
> >
> > https://megous.com/dl/tmp/15b0992a720ab82d.png
> >
> > https://megous.com/dl/tmp/f2cc991046d97641.png
> >
> > This sensor doesn’t have a built-in “Power ON Reset” function. The XCLR pin
> > is set to “LOW” and the power supplies are brought up. Then the XCLR pin
> > should be set to “High” after INCK supplied.
> >
> > So this input is some feature on camera module itself outside of the
> > IMX258 chip, which I think is used to gate power to the module. Eg. on Pinephone
> > Pro, there are two modules with shared power rails, so enabling supply to
> > one module enables it to the other one, too. So this input becomes the only way
> > to really enable/disable power to the chip when both are used at once at some
> > point, because regulator_bulk_enable/disable becomes ineffective at that point.
> >
> > Luis, maybe you saw some other datasheet that mentions this input? IMO,
> > it just gates the power rails via some mosfets on the module itself, since
> > there's not power down input to the chip itself.
> >
> > kind regards,
> > o.
> >
>
> Ondrej, I did not see anything else in the datasheet since I'm pretty sure
> I'm looking at the same datasheet as it was supplied to me by Pine64. I'm
> not sure what datasheet Dave has access to since he got his for a
> completely different module than what we are testing with though.
I only have a leaked datasheet (isn't the internet wonderful!) [1]
XCLR is documented in that, as Ondrej has said.
If this powerdown GPIO is meant to be driving XCLR, then it is in the
wrong order against the supplies.
This does make me confused over the difference between this powerdown
GPIO and the reset GPIO that you implement in 24/25.
Following the PinePhone Pro DT [3] and schematics [4]
reset-gpios = <&gpio1 RK_PA0 GPIO_ACTIVE_LOW>;
powerdown-gpios = <&gpio2 RK_PD4 GPIO_ACTIVE_HIGH>;
Schematic page 11 upper right block
GPIO1_A0/ISP0_SHUTTER_EN/ISP1_SHUTTER_EN/TCPD_VBUS_SINK_EN_d becomes
Camera_RST_L. Page 18 feeds that through to the RESET on the camera
connector.
Page 11 left middle block GPIO2_D4/SDIO0_BKPWR_d becomes DVP_PDN1_H.
Page 18 feeds that through to the PWDN on the camera connector.
Seeing as we apparently have a lens driver kicking around as well,
potentially one is reset to the VCM, and one to the sensor? DW9714
does have an XSD shutdown pin.
Only the module integrator is going to really know the answer,
although potentially a little poking with gpioset and i2cdetect may
tell you more.
Dave
[1] https://web.archive.org/web/20201027131326/www.hi.app/IMX258-datasheet.pdf
[2] https://files.pine64.org/doc/PinePhonePro/PinephonePro-Schematic-V1.0-20211127.pdf
[3] https://xff.cz/git/linux/tree/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts?h=orange-pi-5.18#n868
[4] https://files.pine64.org/doc/PinePhonePro/PinephonePro-Schematic-V1.0-20211127.pdf
> >>> +
> >>> ret = regulator_bulk_enable(IMX258_NUM_SUPPLIES,
> >>> imx258->supplies);
> >>> if (ret) {
> >>> @@ -1224,6 +1228,7 @@ static int imx258_power_on(struct device *dev)
> >>> ret = clk_prepare_enable(imx258->clk);
> >>> if (ret) {
> >>> dev_err(dev, "failed to enable clock\n");
> >>> + gpiod_set_value_cansleep(imx258->powerdown_gpio, 1);
> >>> regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
> >>> }
> >>>
> >>> @@ -1238,6 +1243,8 @@ static int imx258_power_off(struct device *dev)
> >>> clk_disable_unprepare(imx258->clk);
> >>> regulator_bulk_disable(IMX258_NUM_SUPPLIES, imx258->supplies);
> >>>
> >>> + gpiod_set_value_cansleep(imx258->powerdown_gpio, 1);
> >>> +
> >>> return 0;
> >>> }
> >>>
> >>> @@ -1541,6 +1548,12 @@ static int imx258_probe(struct i2c_client *client)
> >>> if (!imx258->variant_cfg)
> >>> imx258->variant_cfg = &imx258_cfg;
> >>>
> >>> + /* request optional power down pin */
> >>> + imx258->powerdown_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
> >>> + GPIOD_OUT_HIGH);
> >>> + if (IS_ERR(imx258->powerdown_gpio))
> >>> + return PTR_ERR(imx258->powerdown_gpio);
> >>> +
> >>> /* Initialize subdev */
> >>> v4l2_i2c_subdev_init(&imx258->sd, client, &imx258_subdev_ops);
> >>>
> >>
> >> --
> >> Regards,
> >>
> >> Sakari Ailus
>
^ permalink raw reply
* [PATCH 0/3] of: Use __free() based cleanups
From: Rob Herring @ 2024-04-04 14:15 UTC (permalink / raw)
To: Saravana Kannan; +Cc: Jonathan Cameron, devicetree, linux-kernel
This small series converts the DT code to use __free() based cleanups
for kfree() and of_node_put(). Using __free() simplifies function exit
handling. Initial support for struct device_node was added in commit
9448e55d032d ("of: Add cleanup.h based auto release via
__free(device_node) markings").
Signed-off-by: Rob Herring <robh@kernel.org>
---
Rob Herring (3):
of: Add a helper to free property struct
of: Use scope based kfree() cleanups
of: Use scope based of_node_put() cleanups
drivers/of/address.c | 60 ++++++++++++++++---------------------------------
drivers/of/base.c | 34 +++++++---------------------
drivers/of/dynamic.c | 37 +++++++++++++-----------------
drivers/of/of_private.h | 1 +
drivers/of/overlay.c | 11 +++------
drivers/of/property.c | 22 ++++++------------
drivers/of/resolver.c | 35 +++++++++++------------------
drivers/of/unittest.c | 12 +++-------
8 files changed, 70 insertions(+), 142 deletions(-)
---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20240404-dt-cleanup-free-6d5334b4b368
Best regards,
--
Rob Herring <robh@kernel.org>
^ permalink raw reply
* [PATCH 1/3] of: Add a helper to free property struct
From: Rob Herring @ 2024-04-04 14:15 UTC (permalink / raw)
To: Saravana Kannan; +Cc: Jonathan Cameron, devicetree, linux-kernel
In-Reply-To: <20240404-dt-cleanup-free-v1-0-c60e6cba8da9@kernel.org>
Freeing a property struct is 3 kfree()'s which is duplicated in multiple
spots. Add a helper, __of_prop_free(), and replace all the open coded
cases in the DT code.
Signed-off-by: Rob Herring <robh@kernel.org>
---
drivers/of/dynamic.c | 26 ++++++++++++--------------
drivers/of/of_private.h | 1 +
drivers/of/overlay.c | 11 +++--------
drivers/of/unittest.c | 12 +++---------
4 files changed, 19 insertions(+), 31 deletions(-)
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 3bf27052832f..af7c57a7a25d 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -305,15 +305,20 @@ int of_detach_node(struct device_node *np)
}
EXPORT_SYMBOL_GPL(of_detach_node);
+void __of_prop_free(struct property *prop)
+{
+ kfree(prop->name);
+ kfree(prop->value);
+ kfree(prop);
+}
+
static void property_list_free(struct property *prop_list)
{
struct property *prop, *next;
for (prop = prop_list; prop != NULL; prop = next) {
next = prop->next;
- kfree(prop->name);
- kfree(prop->value);
- kfree(prop);
+ __of_prop_free(prop);
}
}
@@ -426,9 +431,7 @@ struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
return new;
err_free:
- kfree(new->name);
- kfree(new->value);
- kfree(new);
+ __of_prop_free(new);
return NULL;
}
@@ -470,9 +473,7 @@ struct device_node *__of_node_dup(const struct device_node *np,
if (!new_pp)
goto err_prop;
if (__of_add_property(node, new_pp)) {
- kfree(new_pp->name);
- kfree(new_pp->value);
- kfree(new_pp);
+ __of_prop_free(new_pp);
goto err_prop;
}
}
@@ -921,11 +922,8 @@ static int of_changeset_add_prop_helper(struct of_changeset *ocs,
return -ENOMEM;
ret = of_changeset_add_property(ocs, np, new_pp);
- if (ret) {
- kfree(new_pp->name);
- kfree(new_pp->value);
- kfree(new_pp);
- }
+ if (ret)
+ __of_prop_free(new_pp);
return ret;
}
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 485483524b7f..94fc0aa07af9 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -123,6 +123,7 @@ extern void *__unflatten_device_tree(const void *blob,
* own the devtree lock or work on detached trees only.
*/
struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags);
+void __of_prop_free(struct property *prop);
struct device_node *__of_node_dup(const struct device_node *np,
const char *full_name);
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 2ae7e9d24a64..4d861a75d694 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -262,9 +262,7 @@ static struct property *dup_and_fixup_symbol_prop(
return new_prop;
err_free_new_prop:
- kfree(new_prop->name);
- kfree(new_prop->value);
- kfree(new_prop);
+ __of_prop_free(new_prop);
err_free_target_path:
kfree(target_path);
@@ -361,11 +359,8 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
pr_err("WARNING: memory leak will occur if overlay removed, property: %pOF/%s\n",
target->np, new_prop->name);
- if (ret) {
- kfree(new_prop->name);
- kfree(new_prop->value);
- kfree(new_prop);
- }
+ if (ret)
+ __of_prop_free(new_prop);
return ret;
}
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 6b5c36b6a758..a8c01c953a29 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -795,15 +795,11 @@ static void __init of_unittest_property_copy(void)
new = __of_prop_dup(&p1, GFP_KERNEL);
unittest(new && propcmp(&p1, new), "empty property didn't copy correctly\n");
- kfree(new->value);
- kfree(new->name);
- kfree(new);
+ __of_prop_free(new);
new = __of_prop_dup(&p2, GFP_KERNEL);
unittest(new && propcmp(&p2, new), "non-empty property didn't copy correctly\n");
- kfree(new->value);
- kfree(new->name);
- kfree(new);
+ __of_prop_free(new);
#endif
}
@@ -3718,9 +3714,7 @@ static __init void of_unittest_overlay_high_level(void)
goto err_unlock;
}
if (__of_add_property(of_symbols, new_prop)) {
- kfree(new_prop->name);
- kfree(new_prop->value);
- kfree(new_prop);
+ __of_prop_free(new_prop);
/* "name" auto-generated by unflatten */
if (!strcmp(prop->name, "name"))
continue;
--
2.43.0
^ permalink raw reply related
* [PATCH 2/3] of: Use scope based kfree() cleanups
From: Rob Herring @ 2024-04-04 14:15 UTC (permalink / raw)
To: Saravana Kannan; +Cc: Jonathan Cameron, devicetree, linux-kernel
In-Reply-To: <20240404-dt-cleanup-free-v1-0-c60e6cba8da9@kernel.org>
Use the relatively new scope based kfree() cleanup to simplify error
handling. Doing so reduces the chances of memory leaks and simplifies
error paths by avoiding the need for goto statements.
Signed-off-by: Rob Herring <robh@kernel.org>
---
drivers/of/base.c | 34 ++++++++--------------------------
drivers/of/dynamic.c | 11 ++++-------
drivers/of/resolver.c | 35 +++++++++++++----------------------
3 files changed, 25 insertions(+), 55 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 8856c67c466a..20603d3c9931 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -16,6 +16,7 @@
#define pr_fmt(fmt) "OF: " fmt
+#include <linux/cleanup.h>
#include <linux/console.h>
#include <linux/ctype.h>
#include <linux/cpu.h>
@@ -1393,8 +1394,10 @@ int of_parse_phandle_with_args_map(const struct device_node *np,
const char *stem_name,
int index, struct of_phandle_args *out_args)
{
- char *cells_name, *map_name = NULL, *mask_name = NULL;
- char *pass_name = NULL;
+ char *cells_name __free(kfree) = kasprintf(GFP_KERNEL, "#%s-cells", stem_name);
+ char *map_name __free(kfree) = kasprintf(GFP_KERNEL, "%s-map", stem_name);
+ char *mask_name __free(kfree) = kasprintf(GFP_KERNEL, "%s-map-mask", stem_name);
+ char *pass_name __free(kfree) = kasprintf(GFP_KERNEL, "%s-map-pass-thru", stem_name);
struct device_node *cur, *new = NULL;
const __be32 *map, *mask, *pass;
static const __be32 dummy_mask[] = { [0 ... MAX_PHANDLE_ARGS] = cpu_to_be32(~0) };
@@ -1407,27 +1410,13 @@ int of_parse_phandle_with_args_map(const struct device_node *np,
if (index < 0)
return -EINVAL;
- cells_name = kasprintf(GFP_KERNEL, "#%s-cells", stem_name);
- if (!cells_name)
+ if (!cells_name || !map_name || !mask_name || !pass_name)
return -ENOMEM;
- ret = -ENOMEM;
- map_name = kasprintf(GFP_KERNEL, "%s-map", stem_name);
- if (!map_name)
- goto free;
-
- mask_name = kasprintf(GFP_KERNEL, "%s-map-mask", stem_name);
- if (!mask_name)
- goto free;
-
- pass_name = kasprintf(GFP_KERNEL, "%s-map-pass-thru", stem_name);
- if (!pass_name)
- goto free;
-
ret = __of_parse_phandle_with_args(np, list_name, cells_name, -1, index,
out_args);
if (ret)
- goto free;
+ return ret;
/* Get the #<list>-cells property */
cur = out_args->np;
@@ -1444,8 +1433,7 @@ int of_parse_phandle_with_args_map(const struct device_node *np,
/* Get the <list>-map property */
map = of_get_property(cur, map_name, &map_len);
if (!map) {
- ret = 0;
- goto free;
+ return 0;
}
map_len /= sizeof(u32);
@@ -1521,12 +1509,6 @@ int of_parse_phandle_with_args_map(const struct device_node *np,
put:
of_node_put(cur);
of_node_put(new);
-free:
- kfree(mask_name);
- kfree(map_name);
- kfree(cells_name);
- kfree(pass_name);
-
return ret;
}
EXPORT_SYMBOL(of_parse_phandle_with_args_map);
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index af7c57a7a25d..43f4e2c93bd2 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -9,6 +9,7 @@
#define pr_fmt(fmt) "OF: " fmt
+#include <linux/cleanup.h>
#include <linux/of.h>
#include <linux/spinlock.h>
#include <linux/slab.h>
@@ -1019,10 +1020,9 @@ int of_changeset_add_prop_u32_array(struct of_changeset *ocs,
const u32 *array, size_t sz)
{
struct property prop;
- __be32 *val;
- int i, ret;
+ __be32 *val __free(kfree) = kcalloc(sz, sizeof(__be32), GFP_KERNEL);
+ int i;
- val = kcalloc(sz, sizeof(__be32), GFP_KERNEL);
if (!val)
return -ENOMEM;
@@ -1032,9 +1032,6 @@ int of_changeset_add_prop_u32_array(struct of_changeset *ocs,
prop.length = sizeof(u32) * sz;
prop.value = (void *)val;
- ret = of_changeset_add_prop_helper(ocs, np, &prop);
- kfree(val);
-
- return ret;
+ return of_changeset_add_prop_helper(ocs, np, &prop);
}
EXPORT_SYMBOL_GPL(of_changeset_add_prop_u32_array);
diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index b278ab4338ce..2780928764a4 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -8,6 +8,7 @@
#define pr_fmt(fmt) "OF: resolver: " fmt
+#include <linux/cleanup.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/of.h>
@@ -74,11 +75,11 @@ static int update_usages_of_a_phandle_reference(struct device_node *overlay,
{
struct device_node *refnode;
struct property *prop;
- char *value, *cur, *end, *node_path, *prop_name, *s;
+ char *value __free(kfree) = kmemdup(prop_fixup->value, prop_fixup->length, GFP_KERNEL);
+ char *cur, *end, *node_path, *prop_name, *s;
int offset, len;
int err = 0;
- value = kmemdup(prop_fixup->value, prop_fixup->length, GFP_KERNEL);
if (!value)
return -ENOMEM;
@@ -89,23 +90,19 @@ static int update_usages_of_a_phandle_reference(struct device_node *overlay,
node_path = cur;
s = strchr(cur, ':');
- if (!s) {
- err = -EINVAL;
- goto err_fail;
- }
+ if (!s)
+ return -EINVAL;
*s++ = '\0';
prop_name = s;
s = strchr(s, ':');
- if (!s) {
- err = -EINVAL;
- goto err_fail;
- }
+ if (!s)
+ return -EINVAL;
*s++ = '\0';
err = kstrtoint(s, 10, &offset);
if (err)
- goto err_fail;
+ return err;
refnode = __of_find_node_by_full_path(of_node_get(overlay), node_path);
if (!refnode)
@@ -117,22 +114,16 @@ static int update_usages_of_a_phandle_reference(struct device_node *overlay,
}
of_node_put(refnode);
- if (!prop) {
- err = -ENOENT;
- goto err_fail;
- }
+ if (!prop)
+ return -ENOENT;
- if (offset < 0 || offset + sizeof(__be32) > prop->length) {
- err = -EINVAL;
- goto err_fail;
- }
+ if (offset < 0 || offset + sizeof(__be32) > prop->length)
+ return -EINVAL;
*(__be32 *)(prop->value + offset) = cpu_to_be32(phandle);
}
-err_fail:
- kfree(value);
- return err;
+ return 0;
}
/* compare nodes taking into account that 'name' strips out the @ part */
--
2.43.0
^ permalink raw reply related
* [PATCH 3/3] of: Use scope based of_node_put() cleanups
From: Rob Herring @ 2024-04-04 14:15 UTC (permalink / raw)
To: Saravana Kannan; +Cc: Jonathan Cameron, devicetree, linux-kernel
In-Reply-To: <20240404-dt-cleanup-free-v1-0-c60e6cba8da9@kernel.org>
Use the relatively new scope based of_node_put() cleanup to simplify
function exit handling. Doing so reduces the chances of forgetting an
of_node_put() and simplifies error paths by avoiding the need for goto
statements.
Signed-off-by: Rob Herring <robh@kernel.org>
---
drivers/of/address.c | 60 ++++++++++++++++-----------------------------------
drivers/of/property.c | 22 ++++++-------------
2 files changed, 26 insertions(+), 56 deletions(-)
diff --git a/drivers/of/address.c b/drivers/of/address.c
index ae46a3605904..f7b2d535a6d1 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -491,7 +491,6 @@ static u64 __of_translate_address(struct device_node *dev,
const __be32 *in_addr, const char *rprop,
struct device_node **host)
{
- struct device_node *parent = NULL;
struct of_bus *bus, *pbus;
__be32 addr[OF_MAX_ADDR_CELLS];
int na, ns, pna, pns;
@@ -504,7 +503,7 @@ static u64 __of_translate_address(struct device_node *dev,
*host = NULL;
/* Get parent & match bus type */
- parent = get_parent(dev);
+ struct device_node *parent __free(device_node) = get_parent(dev);
if (parent == NULL)
goto bail;
bus = of_match_bus(parent);
@@ -573,7 +572,6 @@ static u64 __of_translate_address(struct device_node *dev,
of_dump_addr("one level translation:", addr, na);
}
bail:
- of_node_put(parent);
of_node_put(dev);
return result;
@@ -654,19 +652,16 @@ EXPORT_SYMBOL(of_translate_dma_address);
const __be32 *of_translate_dma_region(struct device_node *dev, const __be32 *prop,
phys_addr_t *start, size_t *length)
{
- struct device_node *parent;
+ struct device_node *parent __free(device_node) = __of_get_dma_parent(dev);
u64 address, size;
int na, ns;
- parent = __of_get_dma_parent(dev);
if (!parent)
return NULL;
na = of_bus_n_addr_cells(parent);
ns = of_bus_n_size_cells(parent);
- of_node_put(parent);
-
address = of_translate_dma_address(dev, prop);
if (address == OF_BAD_ADDR)
return NULL;
@@ -688,21 +683,19 @@ const __be32 *__of_get_address(struct device_node *dev, int index, int bar_no,
{
const __be32 *prop;
unsigned int psize;
- struct device_node *parent;
+ struct device_node *parent __free(device_node) = of_get_parent(dev);
struct of_bus *bus;
int onesize, i, na, ns;
- /* Get parent & match bus type */
- parent = of_get_parent(dev);
if (parent == NULL)
return NULL;
+
+ /* match the parent's bus type */
bus = of_match_bus(parent);
- if (strcmp(bus->name, "pci") && (bar_no >= 0)) {
- of_node_put(parent);
+ if (strcmp(bus->name, "pci") && (bar_no >= 0))
return NULL;
- }
+
bus->count_cells(dev, &na, &ns);
- of_node_put(parent);
if (!OF_CHECK_ADDR_COUNT(na))
return NULL;
@@ -888,14 +881,13 @@ static u64 of_translate_ioport(struct device_node *dev, const __be32 *in_addr,
*/
int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map)
{
- struct device_node *node = of_node_get(np);
+ struct device_node *node __free(device_node) = of_node_get(np);
const __be32 *ranges = NULL;
bool found_dma_ranges = false;
struct of_range_parser parser;
struct of_range range;
struct bus_dma_region *r;
int len, num_ranges = 0;
- int ret = 0;
while (node) {
ranges = of_get_property(node, "dma-ranges", &len);
@@ -905,10 +897,9 @@ int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map)
break;
/* Once we find 'dma-ranges', then a missing one is an error */
- if (found_dma_ranges && !ranges) {
- ret = -ENODEV;
- goto out;
- }
+ if (found_dma_ranges && !ranges)
+ return -ENODEV;
+
found_dma_ranges = true;
node = of_get_next_dma_parent(node);
@@ -916,10 +907,8 @@ int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map)
if (!node || !ranges) {
pr_debug("no dma-ranges found for node(%pOF)\n", np);
- ret = -ENODEV;
- goto out;
+ return -ENODEV;
}
-
of_dma_range_parser_init(&parser, node);
for_each_of_range(&parser, &range) {
if (range.cpu_addr == OF_BAD_ADDR) {
@@ -930,16 +919,12 @@ int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map)
num_ranges++;
}
- if (!num_ranges) {
- ret = -EINVAL;
- goto out;
- }
+ if (!num_ranges)
+ return -EINVAL;
r = kcalloc(num_ranges + 1, sizeof(*r), GFP_KERNEL);
- if (!r) {
- ret = -ENOMEM;
- goto out;
- }
+ if (!r)
+ return -ENOMEM;
/*
* Record all info in the generic DMA ranges array for struct device,
@@ -957,9 +942,7 @@ int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map)
r->size = range.size;
r++;
}
-out:
- of_node_put(node);
- return ret;
+ return 0;
}
#endif /* CONFIG_HAS_DMA */
@@ -1016,11 +999,9 @@ phys_addr_t __init of_dma_get_max_cpu_address(struct device_node *np)
*/
bool of_dma_is_coherent(struct device_node *np)
{
- struct device_node *node;
+ struct device_node *node __free(device_node) = of_node_get(np);
bool is_coherent = dma_default_coherent;
- node = of_node_get(np);
-
while (node) {
if (of_property_read_bool(node, "dma-coherent")) {
is_coherent = true;
@@ -1032,7 +1013,6 @@ bool of_dma_is_coherent(struct device_node *np)
}
node = of_get_next_dma_parent(node);
}
- of_node_put(node);
return is_coherent;
}
EXPORT_SYMBOL_GPL(of_dma_is_coherent);
@@ -1049,19 +1029,17 @@ EXPORT_SYMBOL_GPL(of_dma_is_coherent);
*/
static bool of_mmio_is_nonposted(struct device_node *np)
{
- struct device_node *parent;
bool nonposted;
if (!IS_ENABLED(CONFIG_ARCH_APPLE))
return false;
- parent = of_get_parent(np);
+ struct device_node *parent __free(device_node) = of_get_parent(np);
if (!parent)
return false;
nonposted = of_property_read_bool(parent, "nonposted-mmio");
- of_node_put(parent);
return nonposted;
}
diff --git a/drivers/of/property.c b/drivers/of/property.c
index a6358ee99b74..b73daf81c99d 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -40,15 +40,12 @@
*/
bool of_graph_is_present(const struct device_node *node)
{
- struct device_node *ports, *port;
+ struct device_node *ports __free(device_node) = of_get_child_by_name(node, "ports");
- ports = of_get_child_by_name(node, "ports");
if (ports)
node = ports;
- port = of_get_child_by_name(node, "port");
- of_node_put(ports);
- of_node_put(port);
+ struct device_node *port __free(device_node) = of_get_child_by_name(node, "port");
return !!port;
}
@@ -610,9 +607,9 @@ EXPORT_SYMBOL(of_graph_parse_endpoint);
*/
struct device_node *of_graph_get_port_by_id(struct device_node *parent, u32 id)
{
- struct device_node *node, *port;
+ struct device_node *port;
+ struct device_node *node __free(device_node) = of_get_child_by_name(parent, "ports");
- node = of_get_child_by_name(parent, "ports");
if (node)
parent = node;
@@ -626,8 +623,6 @@ struct device_node *of_graph_get_port_by_id(struct device_node *parent, u32 id)
break;
}
- of_node_put(node);
-
return port;
}
EXPORT_SYMBOL(of_graph_get_port_by_id);
@@ -655,14 +650,13 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
* parent port node.
*/
if (!prev) {
- struct device_node *node;
+ struct device_node *node __free(device_node) =
+ of_get_child_by_name(parent, "ports");
- node = of_get_child_by_name(parent, "ports");
if (node)
parent = node;
port = of_get_child_by_name(parent, "port");
- of_node_put(node);
if (!port) {
pr_debug("graph: no port node found in %pOF\n", parent);
@@ -1052,15 +1046,13 @@ static int of_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
struct fwnode_endpoint *endpoint)
{
const struct device_node *node = to_of_node(fwnode);
- struct device_node *port_node = of_get_parent(node);
+ struct device_node *port_node __free(device_node) = of_get_parent(node);
endpoint->local_fwnode = fwnode;
of_property_read_u32(port_node, "reg", &endpoint->port);
of_property_read_u32(node, "reg", &endpoint->id);
- of_node_put(port_node);
-
return 0;
}
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v5 02/10] dt-bindings: mailbox: Add mboxes property for CMDQ secure driver
From: Conor Dooley @ 2024-04-04 14:52 UTC (permalink / raw)
To: Jason-JH Lin (林睿祥)
Cc: Shawn Sung (宋孝謙),
linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
Houlong Wei (魏厚龙),
devicetree@vger.kernel.org, CK Hu (胡俊光),
conor+dt@kernel.org, robh@kernel.org,
linux-arm-kernel@lists.infradead.org,
krzysztof.kozlowski+dt@linaro.org, matthias.bgg@gmail.com,
jassisinghbrar@gmail.com, angelogioacchino.delregno@collabora.com
In-Reply-To: <9b9707a4a0e285a12741fe4140680ad2578d8d2b.camel@mediatek.com>
[-- Attachment #1: Type: text/plain, Size: 2253 bytes --]
On Thu, Apr 04, 2024 at 04:31:06AM +0000, Jason-JH Lin (林睿祥) wrote:
> Hi Conor,
>
> Thanks for the reviews.
>
> On Wed, 2024-04-03 at 16:46 +0100, Conor Dooley wrote:
> > On Wed, Apr 03, 2024 at 06:25:54PM +0800, Shawn Sung wrote:
> > > From: "Jason-JH.Lin" <jason-jh.lin@mediatek.com>
> > >
> > > Add mboxes to define a GCE loopping thread as a secure irq handler.
> > > This property is only required if CMDQ secure driver is supported.
> > >
> > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com>
> > > ---
> > > .../bindings/mailbox/mediatek,gce-mailbox.yaml | 10
> > > ++++++++++
> > > 1 file changed, 10 insertions(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> > > mailbox.yaml
> > > b/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> > > mailbox.yaml
> > > index cef9d76013985..c0d80cc770899 100644
> > > --- a/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> > > mailbox.yaml
> > > +++ b/Documentation/devicetree/bindings/mailbox/mediatek,gce-
> > > mailbox.yaml
> > > @@ -49,6 +49,16 @@ properties:
> > > items:
> > > - const: gce
> > >
> > > + mediatek,gce-events:
> > > + description:
> > > + The event id which is mapping to the specific hardware event
> > > signal
> > > + to gce. The event id is defined in the gce header
> > > + include/dt-bindings/gce/<chip>-gce.h of each chips.
> >
> > Missing any info here about when this should be used, hint - you have
> > it
> > in the commit message.
> >
> > > + $ref: /schemas/types.yaml#/definitions/uint32-arrayi
> >
> > Why is the ID used by the CMDQ service not fixed for each SoC?
> >
> I forgot to sync with Shawn about this:
> https://lore.kernel.org/all/20240124011459.12204-1-jason-
> jh.lin@mediatek.com
>
> I'll fix it at the next version.
When I say "fixed" I don't mean "this is wrong, please fix it", I mean
"why is the value not static for a particular SoC". This needs to be
explained in the patch (and the description for the event here needs to
explain what the gce-mailbox is reserving an event for).
Thanks,
Conor.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ 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