* Re: [PATCH V2 RESEND 6/6] arm64: dts: qcom: sm8650: Add video and camera clock controllers
From: Dmitry Baryshkov @ 2024-04-04 16:05 UTC (permalink / raw)
To: Jagadeesh Kona
Cc: Bjorn Andersson, Konrad Dybcio, Michael Turquette, Stephen Boyd,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Vladimir Zapolskiy, linux-arm-msm, linux-clk, devicetree,
linux-kernel, Taniya Das, Satya Priya Kakitapalli, Ajit Pandey,
Imran Shaik
In-Reply-To: <8a5a3cf8-5b4f-487f-ad91-00499509f8ec@quicinc.com>
On Thu, 4 Apr 2024 at 13:06, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
>
>
>
> On 4/4/2024 11:00 AM, Dmitry Baryshkov wrote:
> > On Thu, 4 Apr 2024 at 08:13, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 4/3/2024 9:24 PM, Dmitry Baryshkov wrote:
> >>> On Wed, 3 Apr 2024 at 10:16, Jagadeesh Kona <quic_jkona@quicinc.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 3/25/2024 11:38 AM, Jagadeesh Kona wrote:
> >>>>>
> >>>>>
> >>>>> On 3/21/2024 6:43 PM, Dmitry Baryshkov wrote:
> >>>>>> On Thu, 21 Mar 2024 at 11:27, Jagadeesh Kona <quic_jkona@quicinc.com>
> >>>>>> wrote:
> >>>>>>>
> >>>>>>> Add device nodes for video and camera clock controllers on Qualcomm
> >>>>>>> SM8650 platform.
> >>>>>>>
> >>>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com>
> >>>>>>> ---
> >>>>>>> arch/arm64/boot/dts/qcom/sm8650.dtsi | 28 ++++++++++++++++++++++++++++
> >>>>>>> 1 file changed, 28 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi
> >>>>>>> b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> >>>>>>> index 32c0a7b9aded..d862aa6be824 100644
> >>>>>>> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
> >>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> >>>>>>> @@ -4,6 +4,8 @@
> >>>>>>> */
> >>>>>>>
> >>>>>>> #include <dt-bindings/clock/qcom,rpmh.h>
> >>>>>>> +#include <dt-bindings/clock/qcom,sm8450-videocc.h>
> >>>>>>> +#include <dt-bindings/clock/qcom,sm8650-camcc.h>
> >>>>>>> #include <dt-bindings/clock/qcom,sm8650-dispcc.h>
> >>>>>>> #include <dt-bindings/clock/qcom,sm8650-gcc.h>
> >>>>>>> #include <dt-bindings/clock/qcom,sm8650-gpucc.h>
> >>>>>>> @@ -3110,6 +3112,32 @@ opp-202000000 {
> >>>>>>> };
> >>>>>>> };
> >>>>>>>
> >>>>>>> + videocc: clock-controller@aaf0000 {
> >>>>>>> + compatible = "qcom,sm8650-videocc";
> >>>>>>> + reg = <0 0x0aaf0000 0 0x10000>;
> >>>>>>> + clocks = <&bi_tcxo_div2>,
> >>>>>>> + <&gcc GCC_VIDEO_AHB_CLK>;
> >>>>>>> + power-domains = <&rpmhpd RPMHPD_MMCX>;
> >>>>>>> + required-opps = <&rpmhpd_opp_low_svs>;
> >>>>>>
> >>>>>> The required-opps should no longer be necessary.
> >>>>>>
> >>>>>
> >>>>> Sure, will check and remove this if not required.
> >>>>
> >>>>
> >>>> I checked further on this and without required-opps, if there is no vote
> >>>> on the power-domain & its peer from any other consumers, when runtime
> >>>> get is called on device, it enables the power domain just at the minimum
> >>>> non-zero level. But in some cases, the minimum non-zero level of
> >>>> power-domain could be just retention and is not sufficient for clock
> >>>> controller to operate, hence required-opps property is needed to specify
> >>>> the minimum level required on power-domain for this clock controller.
> >>>
> >>> In which cases? If it ends up with the retention vote, it is a bug
> >>> which must be fixed.
> >>>
> >>
> >> The minimum non-zero level(configured from bootloaders) of MMCX is
> >> retention on few chipsets but it can vary across the chipsets. Hence to
> >> be on safer side from our end, it is good to have required-opps in DT to
> >> specify the minimum level required for this clock controller.
> >
> > We are discussing sm8650, not some abstract chipset. Does it list
> > retention or low_svs as a minimal level for MMCX?
> >
>
> Actually, the minimum level for MMCX is external to the clock
> controllers.
Yes, it comes from cmd-db
> But the clock controller requires MMCX to be atleast at
> lowsvs for it to be functional.
Correct
> Hence we need to keep required-opps to
> ensure the same without relying on the actual minimum level for MMCX.
And this is not correct. There is no need for the DT to be redundant.
I plan to send patches removing the existing required-opps when they
are not required.
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v5 4/5] PCI: Add PCIE_MSG_CODE_PME_TURN_OFF message macro
From: Manivannan Sadhasivam @ 2024-04-04 15:53 UTC (permalink / raw)
To: Frank Li
Cc: Bjorn Helgaas, Jingoo Han, Gustavo Pimentel, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, imx, linux-pci, linux-kernel, devicetree
In-Reply-To: <20240319-pme_msg-v5-4-af9ffe57f432@nxp.com>
On Tue, Mar 19, 2024 at 12:07:14PM -0400, Frank Li wrote:
> Add PCIE_MSG_CODE_PME_TURN_OFF macros to enable a PCIe host driver to send
> PME_Turn_Off messages.
>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> drivers/pci/pci.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index ffd066c15f3bb..989681a0d6057 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -40,6 +40,8 @@
> #define PCIE_MSG_CODE_DEASSERT_INTC 0x26
> #define PCIE_MSG_CODE_DEASSERT_INTD 0x27
>
> +#define PCIE_MSG_CODE_PME_TURN_OFF 0x19
> +
I think this could be moved before INTx to keep the codes sorted.
- Mani
> extern const unsigned char pcie_link_speed[];
> extern bool pci_early_dump;
>
>
> --
> 2.34.1
>
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply
* [PATCH v2 3/3] arm64: dts: add description for solidrun cn9130 som and clearfog boards
From: Josua Mayer @ 2024-04-04 15:35 UTC (permalink / raw)
To: Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Rob Herring
Cc: Yazan Shhady, linux-arm-kernel, devicetree, linux-kernel,
Josua Mayer
In-Reply-To: <20240404-cn9130-som-v2-0-3af2229c7d2d@solid-run.com>
Add description for the SolidRun CN9130 SoM, and Clearfog Base / Pro
reference boards.
The SoM has been designed as a pin-compatible replacement for the older
Armada 388 based SoM. Therefore it supports the same boards and a
similar feature set.
Most notable upgrades:
- 4x Cortex-A72
- 10Gbps SFP
- Both eMMC and SD supported at the same time
The developer first supporting this product at SolidRun decided to use
different filenames for the DTBs: Armada 388 uses the full
"clearfog" string while cn9130 uses the abbreviation "cf".
This name is already hard-coded in pre-installed vendor u-boot and can
not be changed easily.
NOTICE IN CASE ANYBODY WANTS TO SELF-UPGRADE:
CN9130 SoM has a different footprint from Armada 388 SoM.
Components on the carrier board below the SoM may collide causing
damage, such as on Clearfog Base.
Signed-off-by: Josua Mayer <josua@solid-run.com>
---
arch/arm64/boot/dts/marvell/Makefile | 2 +
arch/arm64/boot/dts/marvell/cn9130-cf-base.dts | 178 ++++++++++++
arch/arm64/boot/dts/marvell/cn9130-cf-pro.dts | 367 +++++++++++++++++++++++++
arch/arm64/boot/dts/marvell/cn9130-cf.dtsi | 193 +++++++++++++
arch/arm64/boot/dts/marvell/cn9130-sr-som.dtsi | 159 +++++++++++
5 files changed, 899 insertions(+)
diff --git a/arch/arm64/boot/dts/marvell/Makefile b/arch/arm64/boot/dts/marvell/Makefile
index 99b8cb3c49e1..019f2251d696 100644
--- a/arch/arm64/boot/dts/marvell/Makefile
+++ b/arch/arm64/boot/dts/marvell/Makefile
@@ -28,3 +28,5 @@ dtb-$(CONFIG_ARCH_MVEBU) += cn9130-crb-A.dtb
dtb-$(CONFIG_ARCH_MVEBU) += cn9130-crb-B.dtb
dtb-$(CONFIG_ARCH_MVEBU) += ac5x-rd-carrier-cn9131.dtb
dtb-$(CONFIG_ARCH_MVEBU) += ac5-98dx35xx-rd.dtb
+dtb-$(CONFIG_ARCH_MVEBU) += cn9130-cf-base.dtb
+dtb-$(CONFIG_ARCH_MVEBU) += cn9130-cf-pro.dtb
diff --git a/arch/arm64/boot/dts/marvell/cn9130-cf-base.dts b/arch/arm64/boot/dts/marvell/cn9130-cf-base.dts
new file mode 100644
index 000000000000..788a5c302b17
--- /dev/null
+++ b/arch/arm64/boot/dts/marvell/cn9130-cf-base.dts
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2024 Josua Mayer <josua@solid-run.com>
+ *
+ * DTS for SolidRun CN9130 Clearfog Base.
+ *
+ */
+
+/dts-v1/;
+
+#include <dt-bindings/input/input.h>
+#include <dt-bindings/leds/common.h>
+
+#include "cn9130.dtsi"
+#include "cn9130-sr-som.dtsi"
+#include "cn9130-cf.dtsi"
+
+/ {
+ model = "SolidRun CN9130 Clearfog Base";
+ compatible = "solidrun,cn9130-clearfog-base",
+ "solidrun,cn9130-sr-som", "marvell,cn9130";
+
+ gpio-keys {
+ compatible = "gpio-keys";
+ pinctrl-0 = <&rear_button_pins>;
+ pinctrl-names = "default";
+
+ button-0 {
+ /* The rear SW3 button */
+ label = "Rear Button";
+ gpios = <&cp0_gpio1 31 GPIO_ACTIVE_LOW>;
+ linux,can-disable;
+ linux,code = <BTN_0>;
+ };
+ };
+
+ rfkill-m2-gnss {
+ compatible = "rfkill-gpio";
+ label = "m.2 GNSS";
+ radio-type = "gps";
+ /* rfkill-gpio inverts internally */
+ shutdown-gpios = <&expander0 9 GPIO_ACTIVE_HIGH>;
+ };
+
+ /* M.2 is B-keyed, so w-disable is for WWAN */
+ rfkill-m2-wwan {
+ compatible = "rfkill-gpio";
+ label = "m.2 WWAN";
+ radio-type = "wwan";
+ /* rfkill-gpio inverts internally */
+ shutdown-gpios = <&expander0 8 GPIO_ACTIVE_HIGH>;
+ };
+};
+
+/* SRDS #3 - SGMII 1GE */
+&cp0_eth1 {
+ phy = <&phy1>;
+ phys = <&cp0_comphy3 1>;
+ phy-mode = "sgmii";
+ status = "okay";
+};
+
+&cp0_eth2_phy {
+ /*
+ * Configure LEDs default behaviour:
+ * - LED[0]: link/activity: On/blink (green)
+ * - LED[1]: link is 100/1000Mbps: On (yellow)
+ * - LED[2]: high impedance (floating)
+ */
+ marvell,reg-init = <3 16 0xf000 0x0a61>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_WAN;
+ default-state = "keep";
+ };
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_YELLOW>;
+ function = LED_FUNCTION_WAN;
+ default-state = "keep";
+ };
+ };
+};
+
+&cp0_gpio1 {
+ sim-select-hog {
+ gpio-hog;
+ gpios = <27 GPIO_ACTIVE_HIGH>;
+ output-high;
+ line-name = "sim-select";
+ };
+};
+
+&cp0_mdio {
+ phy1: ethernet-phy@1 {
+ reg = <1>;
+ /*
+ * Configure LEDs default behaviour:
+ * - LED[0]: link/activity: On/blink (green)
+ * - LED[1]: link is 100/1000Mbps: On (yellow)
+ * - LED[2]: high impedance (floating)
+ *
+ * Configure LEDs electrical polarity
+ * - on-state: low
+ * - off-state: high (not hi-z, to avoid residual glow)
+ */
+ marvell,reg-init = <3 16 0xf000 0x0a61>,
+ <3 17 0x003f 0x000a>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ default-state = "keep";
+ };
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_YELLOW>;
+ function = LED_FUNCTION_LAN;
+ default-state = "keep";
+ };
+ };
+ };
+};
+
+&cp0_pinctrl {
+ pinctrl-0 = <&sim_select_pins>;
+ pintrl-names = "default";
+
+ rear_button_pins: cp0-rear-button-pins {
+ marvell,pins = "mpp31";
+ marvell,function = "gpio";
+ };
+
+ sim_select_pins: cp0-sim-select-pins {
+ marvell,pins = "mpp27";
+ marvell,function = "gpio";
+ };
+};
+
+/*
+ * SRDS #4 - USB 3.0 host on M.2 connector
+ * USB-2.0 Host on Type-A connector
+ */
+&cp0_usb3_1 {
+ phys = <&cp0_comphy4 1>, <&cp0_utmi1>;
+ phy-names = "comphy", "utmi";
+ dr_mode = "host";
+ status = "okay";
+};
+
+&expander0 {
+ m2-full-card-power-off-hog {
+ gpio-hog;
+ gpios = <2 GPIO_ACTIVE_LOW>;
+ output-low;
+ line-name = "m2-full-card-power-off";
+ };
+
+ m2-reset-hog {
+ gpio-hog;
+ gpios = <10 GPIO_ACTIVE_LOW>;
+ output-low;
+ line-name = "m2-reset";
+ };
+};
diff --git a/arch/arm64/boot/dts/marvell/cn9130-cf-pro.dts b/arch/arm64/boot/dts/marvell/cn9130-cf-pro.dts
new file mode 100644
index 000000000000..0b95d5f7acfd
--- /dev/null
+++ b/arch/arm64/boot/dts/marvell/cn9130-cf-pro.dts
@@ -0,0 +1,367 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2024 Josua Mayer <josua@solid-run.com>
+ *
+ * DTS for SolidRun CN9130 Clearfog Pro.
+ *
+ */
+
+/dts-v1/;
+
+#include <dt-bindings/input/input.h>
+#include <dt-bindings/leds/common.h>
+
+#include "cn9130.dtsi"
+#include "cn9130-sr-som.dtsi"
+#include "cn9130-cf.dtsi"
+
+/ {
+ model = "SolidRun CN9130 Clearfog Pro";
+ compatible = "solidrun,cn9130-clearfog-pro",
+ "solidrun,cn9130-sr-som", "marvell,cn9130";
+
+ gpio-keys {
+ compatible = "gpio-keys";
+ pinctrl-0 = <&rear_button_pins>;
+ pinctrl-names = "default";
+
+ button-0 {
+ /* The rear SW3 button */
+ label = "Rear Button";
+ gpios = <&cp0_gpio2 0 GPIO_ACTIVE_LOW>;
+ linux,can-disable;
+ linux,code = <BTN_0>;
+ };
+ };
+};
+
+/* SRDS #3 - SGMII 1GE to L2 switch */
+&cp0_eth1 {
+ phys = <&cp0_comphy3 1>;
+ phy-mode = "sgmii";
+ status = "okay";
+
+ fixed-link {
+ speed = <1000>;
+ full-duplex;
+ };
+};
+
+&cp0_eth2_phy {
+ /*
+ * Configure LEDs default behaviour similar to switch ports:
+ * - LED[0]: link/activity: On/blink (green)
+ * - LED[1]: link is 100/1000Mbps: On (red)
+ * - LED[2]: high impedance (floating)
+ *
+ * Switch port defaults:
+ * - LED0: link/activity: On/blink (green)
+ * - LED1: link is 1000Mbps: On (red)
+ *
+ * Identical configuration is impossible with hardware offload.
+ */
+ marvell,reg-init = <3 16 0xf000 0x0a61>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_WAN;
+ label = "LED2";
+ default-state = "keep";
+ };
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_RED>;
+ function = LED_FUNCTION_WAN;
+ label = "LED1";
+ default-state = "keep";
+ };
+ };
+};
+
+&cp0_mdio {
+ ethernet-switch@4 {
+ compatible = "marvell,mv88e6085";
+ reg = <4>;
+ pinctrl-0 = <&dsa_clk_pins &dsa_pins>;
+ pinctrl-names = "default";
+ reset-gpios = <&cp0_gpio1 27 GPIO_ACTIVE_LOW>;
+ interrupt-parent = <&cp0_gpio1>;
+ interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
+
+ ethernet-ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ethernet-port@0 {
+ reg = <0>;
+ label = "lan5";
+ phy = <&switch0phy0>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ label = "LED12";
+ default-state = "keep";
+ };
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_RED>;
+ function = LED_FUNCTION_LAN;
+ label = "LED11";
+ default-state = "keep";
+ };
+ };
+ };
+
+ ethernet-port@1 {
+ reg = <1>;
+ label = "lan4";
+ phy = <&switch0phy1>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ label = "LED10";
+ default-state = "keep";
+ };
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_RED>;
+ function = LED_FUNCTION_LAN;
+ label = "LED9";
+ default-state = "keep";
+ };
+ };
+ };
+
+ ethernet-port@2 {
+ reg = <2>;
+ label = "lan3";
+ phy = <&switch0phy2>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ label = "LED8";
+ default-state = "keep";
+ };
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_RED>;
+ function = LED_FUNCTION_LAN;
+ label = "LED7";
+ default-state = "keep";
+ };
+ };
+ };
+
+ ethernet-port@3 {
+ reg = <3>;
+ label = "lan2";
+ phy = <&switch0phy3>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ label = "LED6";
+ default-state = "keep";
+ };
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_RED>;
+ function = LED_FUNCTION_LAN;
+ label = "LED5";
+ default-state = "keep";
+ };
+ };
+ };
+
+ ethernet-port@4 {
+ reg = <4>;
+ label = "lan1";
+ phy = <&switch0phy4>;
+
+ leds {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ color = <LED_COLOR_ID_GREEN>;
+ function = LED_FUNCTION_LAN;
+ label = "LED4";
+ default-state = "keep";
+ };
+
+ led@1 {
+ reg = <1>;
+ color = <LED_COLOR_ID_RED>;
+ function = LED_FUNCTION_LAN;
+ label = "LED3";
+ default-state = "keep";
+ };
+ };
+ };
+
+ ethernet-port@5 {
+ reg = <5>;
+ label = "cpu";
+ ethernet = <&cp0_eth1>;
+ phy-mode = "sgmii";
+
+ fixed-link {
+ speed = <1000>;
+ full-duplex;
+ };
+ };
+
+ ethernet-port@6 {
+ reg = <6>;
+ label = "lan6";
+ phy-mode = "rgmii";
+
+ /*
+ * Because of mdio address conflict the
+ * external phy is not readable.
+ * Force a fixed link instead.
+ */
+ fixed-link {
+ speed = <1000>;
+ full-duplex;
+ };
+ };
+ };
+
+ mdio {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ switch0phy0: ethernet-phy@0 {
+ reg = <0x0>;
+ };
+
+ switch0phy1: ethernet-phy@1 {
+ reg = <0x1>;
+ /*
+ * Indirectly configure default behaviour
+ * for port lan6 leds behind external phy.
+ * Internal PHYs are not using page 3,
+ * therefore writing to it is safe.
+ */
+ marvell,reg-init = <3 16 0xf000 0x0a61>;
+ };
+
+ switch0phy2: ethernet-phy@2 {
+ reg = <0x2>;
+ };
+
+ switch0phy3: ethernet-phy@3 {
+ reg = <0x3>;
+ };
+
+ switch0phy4: ethernet-phy@4 {
+ reg = <0x4>;
+ };
+ };
+
+ /*
+ * There is an external phy on the switch mdio bus.
+ * Because its mdio address collides with internal phys,
+ * it is not readable.
+ *
+ * mdio-external {
+ * compatible = "marvell,mv88e6xxx-mdio-external";
+ * #address-cells = <1>;
+ * #size-cells = <0>;
+ *
+ * ethernet-phy@1 {
+ * reg = <0x1>;
+ * };
+ * };
+ */
+ };
+};
+
+/* SRDS #4 - miniPCIe (CON2) */
+&cp0_pcie1 {
+ num-lanes = <1>;
+ phys = <&cp0_comphy4 1>;
+ /* dw-pcie inverts internally */
+ reset-gpios = <&expander0 2 GPIO_ACTIVE_HIGH>;
+ status = "okay";
+};
+
+&cp0_pinctrl {
+ dsa_clk_pins: cp0-dsa-clk-pins {
+ marvell,pins = "mpp40";
+ marvell,function = "synce1";
+ };
+
+ dsa_pins: cp0-dsa-pins {
+ marvell,pins = "mpp27", "mpp29";
+ marvell,function = "gpio";
+ };
+
+ rear_button_pins: cp0-rear-button-pins {
+ marvell,pins = "mpp32";
+ marvell,function = "gpio";
+ };
+};
+
+/*
+ * USB-2.0 Host on Type-A connector
+ */
+&cp0_usb3_1 {
+ phys = <&cp0_utmi1>;
+ phy-names = "utmi";
+ dr_mode = "host";
+ status = "okay";
+};
+
+&expander0 {
+ /* CON2 */
+ pcie1-0-clkreq-hog {
+ gpio-hog;
+ gpios = <4 GPIO_ACTIVE_LOW>;
+ input;
+ line-name = "pcie1.0-clkreq";
+ };
+
+ /* CON2 */
+ pcie1-0-w-disable-hog {
+ gpio-hog;
+ gpios = <7 GPIO_ACTIVE_LOW>;
+ output-low;
+ line-name = "pcie1.0-w-disable";
+ };
+};
diff --git a/arch/arm64/boot/dts/marvell/cn9130-cf.dtsi b/arch/arm64/boot/dts/marvell/cn9130-cf.dtsi
new file mode 100644
index 000000000000..53aedddf0e26
--- /dev/null
+++ b/arch/arm64/boot/dts/marvell/cn9130-cf.dtsi
@@ -0,0 +1,193 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2024 Josua Mayer <josua@solid-run.com>
+ *
+ * DTS for common base of SolidRun CN9130 Clearfog Base and Pro.
+ *
+ */
+
+/ {
+ aliases {
+ i2c1 = &cp0_i2c1;
+ mmc1 = &cp0_sdhci0;
+ };
+
+ reg_usb3_vbus0: regulator-usb3-vbus0 {
+ compatible = "regulator-fixed";
+ regulator-name = "vbus0";
+ regulator-min-microvolt = <5000000>;
+ regulator-max-microvolt = <5000000>;
+ gpio = <&expander0 6 GPIO_ACTIVE_LOW>;
+ };
+
+ sfp: sfp {
+ compatible = "sff,sfp";
+ i2c-bus = <&cp0_i2c1>;
+ los-gpios = <&expander0 12 GPIO_ACTIVE_HIGH>;
+ mod-def0-gpios = <&expander0 15 GPIO_ACTIVE_LOW>;
+ tx-disable-gpios = <&expander0 14 GPIO_ACTIVE_HIGH>;
+ tx-fault-gpios = <&expander0 13 GPIO_ACTIVE_HIGH>;
+ maximum-power-milliwatt = <2000>;
+ };
+};
+
+/* SRDS #2 - SFP+ 10GE */
+&cp0_eth0 {
+ managed = "in-band-status";
+ phys = <&cp0_comphy2 0>;
+ phy-mode = "10gbase-r";
+ sfp = <&sfp>;
+ status = "okay";
+};
+
+&cp0_i2c0 {
+ expander0: gpio-expander@20 {
+ compatible = "nxp,pca9555";
+ reg = <0x20>;
+ gpio-controller;
+ #gpio-cells = <2>;
+ pinctrl-0 = <&expander0_pins>;
+ pinctrl-names = "default";
+ interrupt-parent = <&cp0_gpio1>;
+ interrupts = <4 IRQ_TYPE_LEVEL_LOW>;
+
+ /* CON3 */
+ pcie2-0-clkreq-hog {
+ gpio-hog;
+ gpios = <0 GPIO_ACTIVE_LOW>;
+ input;
+ line-name = "pcie2.0-clkreq";
+ };
+
+ /* CON3 */
+ pcie2-0-w-disable-hog {
+ gpio-hog;
+ gpios = <3 GPIO_ACTIVE_LOW>;
+ output-low;
+ line-name = "pcie2.0-w-disable";
+ };
+
+ usb3-ilimit-hog {
+ gpio-hog;
+ gpios = <5 GPIO_ACTIVE_LOW>;
+ input;
+ line-name = "usb3-current-limit";
+ };
+
+ m2-devslp-hog {
+ gpio-hog;
+ gpios = <11 GPIO_ACTIVE_HIGH>;
+ output-low;
+ line-name = "m.2 devslp";
+ };
+ };
+
+ /* The MCP3021 supports standard and fast modes */
+ adc@4c {
+ compatible = "microchip,mcp3021";
+ reg = <0x4c>;
+ };
+
+ carrier_eeprom: eeprom@52 {
+ compatible = "atmel,24c02";
+ reg = <0x52>;
+ pagesize = <8>;
+ };
+};
+
+&cp0_i2c1 {
+ /*
+ * Routed to SFP, M.2, mikrobus, and miniPCIe
+ * SFP limits this to 100kHz, and requires an AT24C01A/02/04 with
+ * address pins tied low, which takes addresses 0x50 and 0x51.
+ * Mikrobus doesn't specify beyond an I2C bus being present.
+ * PCIe uses ARP to assign addresses, or 0x63-0x64.
+ */
+ clock-frequency = <100000>;
+ pinctrl-0 = <&cp0_i2c1_pins>;
+ pinctrl-names = "default";
+ status = "okay";
+};
+
+/* SRDS #5 - miniPCIe (CON3) */
+&cp0_pcie2 {
+ num-lanes = <1>;
+ phys = <&cp0_comphy5 2>;
+ /* dw-pcie inverts internally */
+ reset-gpios = <&expander0 1 GPIO_ACTIVE_HIGH>;
+ status = "okay";
+};
+
+&cp0_pinctrl {
+ cp0_i2c1_pins: cp0-i2c1-pins {
+ marvell,pins = "mpp35", "mpp36";
+ marvell,function = "i2c1";
+ };
+
+ cp0_mmc0_pins: cp0-mmc0-pins {
+ marvell,pins = "mpp43", "mpp56", "mpp57", "mpp58",
+ "mpp59", "mpp60", "mpp61";
+ marvell,function = "sdio";
+ };
+
+ mikro_spi_pins: cp0-spi1-cs1-pins {
+ marvell,pins = "mpp12";
+ marvell,function = "spi1";
+ };
+
+ mikro_uart_pins: cp0-uart-pins {
+ marvell,pins = "mpp2", "mpp3";
+ marvell,function = "uart1";
+ };
+
+ expander0_pins: cp0-expander0-pins {
+ marvell,pins = "mpp4";
+ marvell,function = "gpio";
+ };
+};
+
+/* SRDS #0 - SATA on M.2 connector */
+&cp0_sata0 {
+ phys = <&cp0_comphy0 1>;
+ status = "okay";
+
+ /* only port 1 is available */
+ /delete-node/ sata-port@0;
+};
+
+/* microSD */
+&cp0_sdhci0 {
+ pinctrl-0 = <&cp0_mmc0_pins>;
+ pinctrl-names = "default";
+ bus-width = <4>;
+ no-1-8-v;
+ status = "okay";
+};
+
+&cp0_spi1 {
+ /* CS1 for mikrobus */
+ pinctrl-0 = <&cp0_spi1_pins &mikro_spi_pins>;
+};
+
+/*
+ * SRDS #1 - 3.0 Host on Type-A connector
+ * USB-2.0 Host on mPCI-e connector (CON3)
+ */
+&cp0_usb3_0 {
+ phys = <&cp0_comphy1 0>, <&cp0_utmi0>;
+ phy-names = "comphy", "utmi";
+ vbus-supply = <®_usb3_vbus0>;
+ dr_mode = "host";
+ status = "okay";
+};
+
+&cp0_utmi {
+ status = "okay";
+};
+
+/* mikrobus uart */
+&cp0_uart0 {
+ pinctrl-0 = <&mikro_uart_pins>;
+ pinctrl-names = "default";
+ status = "okay";
+};
diff --git a/arch/arm64/boot/dts/marvell/cn9130-sr-som.dtsi b/arch/arm64/boot/dts/marvell/cn9130-sr-som.dtsi
new file mode 100644
index 000000000000..ec08066fb6e8
--- /dev/null
+++ b/arch/arm64/boot/dts/marvell/cn9130-sr-som.dtsi
@@ -0,0 +1,159 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (C) 2024 Josua Mayer <josua@solid-run.com>
+ *
+ */
+
+#include <dt-bindings/gpio/gpio.h>
+
+/ {
+ model = "SolidRun CN9130 SoM";
+ compatible = "solidrun,cn9130-sr-som", "marvell,cn9130";
+
+ aliases {
+ /* label nics like armada-388 som */
+ ethernet0 = &cp0_eth2;
+ ethernet1 = &cp0_eth1;
+ ethernet2 = &cp0_eth0;
+ i2c0 = &cp0_i2c0;
+ mmc0 = &ap_sdhci0;
+ rtc0 = &cp0_rtc;
+ };
+
+ chosen {
+ stdout-path = "serial0:115200n8";
+ };
+
+ v_1_8: regulator-1-8 {
+ compatible = "regulator-fixed";
+ regulator-name = "1v8";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+
+ /* requires assembly of R9307 */
+ vhv: regulator-vhv-1-8 {
+ compatible = "regulator-fixed";
+ regulator-name = "vhv-1v8";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ pinctrl-0 = <&cp0_reg_vhv_pins>;
+ pinctrl-names = "default";
+ gpio = <&cp0_gpio2 9 GPIO_ACTIVE_HIGH>;
+ enable-active-high;
+ };
+};
+
+&ap_pinctrl {
+ ap_mmc0_pins: ap-mmc0-pins {
+ marvell,pins = "mpp0", "mpp1", "mpp2", "mpp3", "mpp4", "mpp5",
+ "mpp6", "mpp7", "mpp8", "mpp9", "mpp10", "mpp12";
+ marvell,function = "sdio";
+ /*
+ * mpp12 is emmc reset, function should be sdio (hw_rst),
+ * but pinctrl-mvebu does not support this.
+ *
+ * From pinctrl-mvebu.h:
+ * "The name will be used to switch to this setting in DT description, e.g.
+ * marvell,function = "uart2". subname is only for debugging purposes."
+ */
+ };
+};
+
+&ap_sdhci0 {
+ bus-width = <8>;
+ pinctrl-0 = <&ap_mmc0_pins>;
+ pinctrl-names = "default";
+ vqmmc-supply = <&v_1_8>;
+ status = "okay";
+};
+
+&cp0_ethernet {
+ status = "okay";
+};
+
+/* for assembly with phy */
+&cp0_eth2 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&cp0_eth2_pins>;
+ phy-mode = "rgmii-id";
+ phy = <&cp0_eth2_phy>;
+ status = "okay";
+};
+
+&cp0_i2c0 {
+ status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <&cp0_i2c0_pins>;
+ clock-frequency = <100000>;
+ status = "okay";
+
+ som_eeprom: eeprom@53 {
+ compatible = "atmel,24c02";
+ reg = <0x53>;
+ pagesize = <8>;
+ };
+};
+
+&cp0_mdio {
+ status = "okay";
+ pinctrl-0 = <&cp0_mdio_pins>;
+
+ /* assembly option */
+ cp0_eth2_phy: ethernet-phy@0 {
+ reg = <0>;
+ };
+};
+
+&cp0_spi1 {
+ status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <&cp0_spi1_pins>;
+
+ flash@0 {
+ compatible = "jedec,spi-nor";
+ reg = <0>;
+ spi-max-frequency = <10000000>;
+ };
+};
+
+&cp0_syscon0 {
+ cp0_pinctrl: pinctrl {
+ compatible = "marvell,cp115-standalone-pinctrl";
+
+ cp0_eth2_pins: cp0-ge2-rgmii-pins {
+ marvell,pins = "mpp44", "mpp45", "mpp46", "mpp47",
+ "mpp48", "mpp49", "mpp50", "mpp51",
+ "mpp52", "mpp53", "mpp54", "mpp55";
+ /* docs call it "ge2", but cp110-pinctrl "ge1" */
+ marvell,function = "ge1";
+ };
+
+ cp0_i2c0_pins: cp0-i2c0-pins {
+ marvell,pins = "mpp37", "mpp38";
+ marvell,function = "i2c0";
+ };
+
+ cp0_mdio_pins: cp0-mdio-pins {
+ marvell,pins = "mpp40", "mpp41";
+ marvell,function = "ge";
+ };
+
+ cp0_spi1_pins: cp0-spi1-pins {
+ marvell,pins = "mpp13", "mpp14", "mpp15", "mpp16";
+ marvell,function = "spi1";
+ };
+
+ cp0_reg_vhv_pins: cp0-reg-vhv-pins {
+ marvell,pins = "mpp41";
+ marvell,function = "gpio";
+ };
+ };
+};
+
+/* AP default console */
+&uart0 {
+ pinctrl-0 = <&uart0_pins>;
+ pinctrl-names = "default";
+ status = "okay";
+};
--
2.35.3
^ permalink raw reply related
* [PATCH v2 2/3] dt-bindings: arm64: marvell: add solidrun cn9132 CEX-7 evaluation board
From: Josua Mayer @ 2024-04-04 15:35 UTC (permalink / raw)
To: Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Rob Herring
Cc: Yazan Shhady, linux-arm-kernel, devicetree, linux-kernel,
Josua Mayer
In-Reply-To: <20240404-cn9130-som-v2-0-3af2229c7d2d@solid-run.com>
Add bindings for the SolidRun CN9132 COM-Express Type 7 evaluation board.
The CEX is based on CN9130 SoC and includes two southbridges.
Because CN9132 and 9131 are just names for different designs around the
same SoC, there no soc compatibles beside marvell,cn9130 are needed.
Signed-off-by: Josua Mayer <josua@solid-run.com>
---
Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml b/Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml
index 74d935ea279c..538d91be8857 100644
--- a/Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml
+++ b/Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml
@@ -92,4 +92,12 @@ properties:
- const: solidrun,cn9130-sr-som
- const: marvell,cn9130
+ - description:
+ SolidRun CN9132 COM-Express Type 7 based single-board computers
+ items:
+ - enum:
+ - solidrun,cn9132-clearfog
+ - const: solidrun,cn9132-sr-cex7
+ - const: marvell,cn9130
+
additionalProperties: true
--
2.35.3
^ permalink raw reply related
* [PATCH v2 0/3] arm64: dts: add description for solidrun cn9130 som and clearfog boards
From: Josua Mayer @ 2024-04-04 15:35 UTC (permalink / raw)
To: Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Rob Herring
Cc: Yazan Shhady, linux-arm-kernel, devicetree, linux-kernel,
Josua Mayer
SolidRun CN9130 SoM is a mostly pin-comptible replacement for Armada 388
SoM used in Clearfog and Clearfog Pro boards.
1. Add new binding for compatible strings closely matching the original.
2. Add device-tree includes for SoM and carrier shared design.
3. Add device-tree for both Clearfog Base and Pro.
While dtbs_check is happy with LED descriptions behind dsa switch,
functionally they require supporting code by Andrew Lunn:
https://lore.kernel.org/r/20240401-v6-8-0-net-next-mv88e6xxx-leds-v4-v3-0-221b3fa55f78@lunn.ch
NOTICE IN CASE ANYBODY WANTS TO SELF-UPGRADE:
CN9130 SoM has a different footprint from Armada 388 SoM.
Components on the carrier board below the SoM may collide causing
damage, such as on Clearfog Base.
Signed-off-by: Josua Mayer <josua@solid-run.com>
---
Changes in v2:
- rewrote dt bindings dropping unnecessary compatibles
(Reported-By: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>)
- added bindings for two additional boards (cn9131/9132)
support planned for the coming weeks, mostly serves
illustrational purposes, to understand cn913x variants
- cf-pro: add description for LEDs behind DSA switch
- cf-base: add description for LEDs behind PHYs
(Reported-By: Andrew Lunn <andrew@lunn.ch>)
- Link to v1: https://lore.kernel.org/r/20240321-cn9130-som-v1-0-711127a409ae@solid-run.com
---
Josua Mayer (3):
dt-bindings: arm64: marvell: add solidrun cn9130 som based boards
dt-bindings: arm64: marvell: add solidrun cn9132 CEX-7 evaluation board
arm64: dts: add description for solidrun cn9130 som and clearfog boards
.../bindings/arm/marvell/armada-7k-8k.yaml | 18 +
arch/arm64/boot/dts/marvell/Makefile | 2 +
arch/arm64/boot/dts/marvell/cn9130-cf-base.dts | 178 ++++++++++
arch/arm64/boot/dts/marvell/cn9130-cf-pro.dts | 367 +++++++++++++++++++++
arch/arm64/boot/dts/marvell/cn9130-cf.dtsi | 193 +++++++++++
arch/arm64/boot/dts/marvell/cn9130-sr-som.dtsi | 159 +++++++++
6 files changed, 917 insertions(+)
---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20240318-cn9130-som-848e86acb0ac
Sincerely,
--
Josua Mayer <josua@solid-run.com>
^ permalink raw reply
* [PATCH v2 1/3] dt-bindings: arm64: marvell: add solidrun cn9130 som based boards
From: Josua Mayer @ 2024-04-04 15:35 UTC (permalink / raw)
To: Andrew Lunn, Gregory Clement, Sebastian Hesselbarth, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Rob Herring
Cc: Yazan Shhady, linux-arm-kernel, devicetree, linux-kernel,
Josua Mayer
In-Reply-To: <20240404-cn9130-som-v2-0-3af2229c7d2d@solid-run.com>
Add bindings for SolidRun boards based on CN9130 SoM.
Three boards are added in total:
- Clearfog Base
- Clearfog Pro
- SolidWAN
The Clearfog boards are identical to the older Armada 388 based boards,
upgraded with a new SoM and SoC.
However the feature set and performance characteristics are different,
therefore compatible strings from armada 388 versions are not included.
SolidWAN uses the same SoM adding a southbridge on the carrier.
Since 2019 there are bindings in-tree for two boards based on cn9130 and
9131. These are extremely verbose by listing cn9132, cn9131, cn9130,
ap807-quad, ap807 for the SoC alone.
CN9130 SoC combines an application processor (ap807) and a
communication processor (cp115) in a single package.
The communication processor (short CP) is also available separately as a
southbridge. It only functions in combination with the CN9130 SoC.
Complete systems adding one or two southbridges are by convention called
CN9131 and CN9132 respectively.
Despite different naming all systems are built around the same SoC.
Therefore marvell,cn9131 and marvell,cn9132 can be omitted. The number
of CPs is part of a board's BoM and can be reflected in the board
compatible string instead.
Existing bindings also describe cn9130 as a specialisation of
ap807-quad. Usually board-level compatibles stop at the SoC without
going into silicon versions or individual dies.
There is no programming model at this layer, and in particular not for
parts of an SoC. Therefore the ap compatibles can also be omitted.
Signed-off-by: Josua Mayer <josua@solid-run.com>
---
.../devicetree/bindings/arm/marvell/armada-7k-8k.yaml | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml b/Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml
index 16d2e132d3d1..74d935ea279c 100644
--- a/Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml
+++ b/Documentation/devicetree/bindings/arm/marvell/armada-7k-8k.yaml
@@ -82,4 +82,14 @@ properties:
- const: marvell,armada-ap807-quad
- const: marvell,armada-ap807
+ - description:
+ SolidRun CN9130 SoM based single-board computers
+ items:
+ - enum:
+ - solidrun,cn9130-clearfog-base
+ - solidrun,cn9130-clearfog-pro
+ - solidrun,cn9131-solidwan
+ - const: solidrun,cn9130-sr-som
+ - const: marvell,cn9130
+
additionalProperties: true
--
2.35.3
^ permalink raw reply related
* Re: [PATCH v2 3/3] arm64: dts: qcom: msm8998: set qcom,no-msa-ready-indicator for wifi
From: Kalle Valo @ 2024-04-04 15:28 UTC (permalink / raw)
To: Marc Gonzalez
Cc: Dmitry Baryshkov, 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>
Marc Gonzalez <mgonzalez@freebox.fr> writes:
> 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." ?
I was trying to say that because the design philosophy between vendor
drivers and upstream drivers is very different, we can't 100% trust
vendor drivers. It's a very good idea to check what vendor drivers do
but we just need to be careful before making any conclusions. Testing
real hardware (and corresponding firmware) is the most reliable way to
know how different products/firmware work, unfortunately.
> And thus, "it is nonsensical to try to "align" the mainline driver to
> "the" vendor driver, as there is no single "vendor driver"" ?
No no, I'm not saying that. I have suffered this "N+1 different firmware
branches behaving slighly differently" problem since ath6kl days so for
me this is business as usual, sadly. I'm sure we can find a solution for
ath10k.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH 2/2] arm64: dts: rockchip: add Protonic MECSBC device-tree
From: Diederik de Haas @ 2024-04-04 15:26 UTC (permalink / raw)
To: Sascha Hauer
Cc: linux-rockchip, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Heiko Stuebner, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel, David Jander, Andrew Lunn
In-Reply-To: <9d325b4e-031c-4f6c-9788-fa5a68470efa@lunn.ch>
[-- Attachment #1: Type: text/plain, Size: 970 bytes --]
On Thursday, 4 April 2024 17:10:41 CEST Andrew Lunn wrote:
> > +&gmac1 {
> > + assigned-clocks = <&cru SCLK_GMAC1_RX_TX>, <&cru SCLK_GMAC1>;
> > + assigned-clock-parents = <&cru SCLK_GMAC1_RGMII_SPEED>, <&cru
> > CLK_MAC1_2TOP>; + phy-handle = <&rgmii_phy1>;
> > + phy-mode = "rgmii";
> > + clock_in_out = "output";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&gmac1m1_miim
> > + &gmac1m1_tx_bus2
> > + &gmac1m1_rx_bus2
> > + &gmac1m1_rgmii_clk
> > + &gmac1m1_clkinout
> > + &gmac1m1_rgmii_bus>;
> > + status = "okay";
> > + tx_delay = <0x30>;
> > + rx_delay = <0x10>;
> > +};
>
> There was a discussion about phy-mode = "rgmii"; and these
> tx/rx_delays last month. Please could you go read that discussion and
> them make use of rgmii-id, and change the delays.
https://lore.kernel.org/linux-rockchip/20240304084612.711678-2-ukleinek@debian.org/
titled "[PATCH] arm64: dts: rockchip: qnap-ts433: Simplify network PHY connection"
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v2 15/15] arm64: dts: mediatek: mt8188: add default thermal zones
From: Daniel Lezcano @ 2024-04-04 15:16 UTC (permalink / raw)
To: Nicolas Pitre, linux-pm, linux-mediatek, devicetree
Cc: Nicolas Pitre, AngeloGioacchino Del Regno
In-Reply-To: <20240402032729.2736685-16-nico@fluxnic.net>
Hi Nico,
a few comments about this description.
On 02/04/2024 05:25, Nicolas Pitre wrote:
> From: Nicolas Pitre <npitre@baylibre.com>
>
> Inspired by the vendor kernel but adapted to the upstream thermal
> driver version.
[ ... ]
> + thermal_zones: thermal-zones {
> + cpu-little0-thermal {
> + polling-delay = <1000>;
Except if I'm wrong, the driver supports the interrupt mode, so it not
necessary to poll constantly when there is no mitigation. You can remove
the line and everywhere else.
> + polling-delay-passive = <250>;
As little CPU, 200ms or 150ms may be more adequate.
> + thermal-sensors = <&lvts_mcu MT8188_MCU_LITTLE_CPU0>;
> +
> + trips {
> + cpu_little0_alert: trip-alert {
> + temperature = <85000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };
You may want to add a 'hot' trip point in between, so the userspace can
be notified and take an action before reaching 'critical' (like
unplugging a CPU)
> + cpu_little0_crit: trip-crit {
> + temperature = <100000>;
> + hysteresis = <2000>;
critical is a point of no return. Hysteresis does not make sense.
These comments apply to all thermal zones.
[ .. ]
> + cpu_big0-thermal {
> + polling-delay = <1000>;
> + polling-delay-passive = <250>;
Same comments as the little but may be an even lower value. eg. 100ms.
> + thermal-sensors = <&lvts_mcu MT8188_MCU_BIG_CPU0>;
> +
> + trips {
> + cpu_big0_alert: trip-alert {
> + temperature = <85000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };
> +
> + cpu_big0_crit: trip-crit {
> + temperature = <100000>;
> + hysteresis = <2000>;
> + type = "critical";
> + };
> + };
> +
> + cooling-maps {
> + map0 {
> + trip = <&cpu_big0_alert>;
> + cooling-device = <&cpu6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> + <&cpu7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> + };
> + };
> + };
[ ... ]
> + gpu1-thermal {
> + polling-delay = <1000>;
> + polling-delay-passive = <250>;
> + thermal-sensors = <&lvts_ap MT8188_AP_GPU1>;
> +
> + trips {
> + gpu1_alert: trip-alert {
> + temperature = <85000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };
> +
> + gpu1_crit: trip-crit {
> + temperature = <100000>;
> + hysteresis = <2000>;
> + type = "critical";
> + };
> + };
> + };
> +
> + gpu2-thermal {
> + polling-delay = <1000>;
> + polling-delay-passive = <250>;
> + thermal-sensors = <&lvts_ap MT8188_AP_GPU2>;
> +
> + trips {
> + gpu2_alert: trip-alert {
> + temperature = <85000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };
> +
> + gpu2_crit: trip-crit {
> + temperature = <100000>;
> + hysteresis = <2000>;
> + type = "critical";
> + };
> + };
You can add a devfreq cooling device for the GPU here.
[ ... ]
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply
* Re: [PATCH 2/2] arm64: dts: rockchip: add Protonic MECSBC device-tree
From: Andrew Lunn @ 2024-04-04 15:10 UTC (permalink / raw)
To: Sascha Hauer
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner,
devicetree, linux-arm-kernel, linux-rockchip, linux-kernel,
David Jander
In-Reply-To: <20240404-protonic-mecsbc-v1-2-ad5b42ade6c6@pengutronix.de>
> +&gmac1 {
> + assigned-clocks = <&cru SCLK_GMAC1_RX_TX>, <&cru SCLK_GMAC1>;
> + assigned-clock-parents = <&cru SCLK_GMAC1_RGMII_SPEED>, <&cru CLK_MAC1_2TOP>;
> + phy-handle = <&rgmii_phy1>;
> + phy-mode = "rgmii";
> + clock_in_out = "output";
> + pinctrl-names = "default";
> + pinctrl-0 = <&gmac1m1_miim
> + &gmac1m1_tx_bus2
> + &gmac1m1_rx_bus2
> + &gmac1m1_rgmii_clk
> + &gmac1m1_clkinout
> + &gmac1m1_rgmii_bus>;
> + status = "okay";
> + tx_delay = <0x30>;
> + rx_delay = <0x10>;
> +};
There was a discussion about phy-mode = "rgmii"; and these
tx/rx_delays last month. Please could you go read that discussion and
them make use of rgmii-id, and change the delays.
Also, where did you copy this from? If possible, it would be good to
fix the example everybody copies into new DT blobs.
Andrew
^ permalink raw reply
* 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
* [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
* [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 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 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
* 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
* 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 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: [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 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: [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 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 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 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: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
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