* [PATCH v7 7/7] arm64: dts: qcom: sc7280: Add DT nodes for the TBUs
From: Georgi Djakov @ 2024-03-29 21:06 UTC (permalink / raw)
To: will, robin.murphy, joro, iommu
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, devicetree, andersson,
konrad.dybcio, robdclark, linux-arm-kernel, linux-kernel,
linux-arm-msm, quic_cgoldswo, quic_sukadev, quic_pdaly,
quic_sudaraja, djakov
In-Reply-To: <20240329210638.3647523-1-quic_c_gdjako@quicinc.com>
Add the device-tree nodes for the TBUs (translation buffer units) that
are present on the sc7280 platforms. The TBUs can be used debug the
kernel and provide additional information when a context faults occur.
Describe all the registers, clocks, interconnects and power-domain
resources that are needed for each of the TBUs.
Signed-off-by: Georgi Djakov <quic_c_gdjako@quicinc.com>
---
arch/arm64/boot/dts/qcom/sc7280.dtsi | 89 ++++++++++++++++++++++++++++
1 file changed, 89 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 7e7f0f0fb41b..5d8aa182e3a9 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -2970,6 +2970,18 @@ adreno_smmu: iommu@3da0000 {
dma-coherent;
};
+ gfx_0_tbu: tbu@3dd9000 {
+ compatible = "qcom,sc7280-tbu";
+ reg = <0x0 0x3dd9000 0x0 0x1000>;
+ qcom,stream-id-range = <&adreno_smmu 0x0 0x400>;
+ };
+
+ gfx_1_tbu: tbu@3ddd000 {
+ compatible = "qcom,sc7280-tbu";
+ reg = <0x0 0x3ddd000 0x0 0x1000>;
+ qcom,stream-id-range = <&adreno_smmu 0x400 0x400>;
+ };
+
remoteproc_mpss: remoteproc@4080000 {
compatible = "qcom,sc7280-mpss-pas";
reg = <0 0x04080000 0 0x10000>;
@@ -5778,6 +5790,83 @@ apps_smmu: iommu@15000000 {
<GIC_SPI 408 IRQ_TYPE_LEVEL_HIGH>;
};
+ anoc_1_tbu: tbu@151dd000 {
+ compatible = "qcom,sc7280-tbu";
+ reg = <0x0 0x151dd000 0x0 0x1000>;
+ interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+ &cnoc3 SLAVE_TCU QCOM_ICC_TAG_ACTIVE_ONLY>;
+ qcom,stream-id-range = <&apps_smmu 0x0 0x400>;
+ };
+
+ anoc_2_tbu: tbu@151e1000 {
+ compatible = "qcom,sc7280-tbu";
+ reg = <0x0 0x151e1000 0x0 0x1000>;
+ interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+ &cnoc3 SLAVE_TCU QCOM_ICC_TAG_ACTIVE_ONLY>;
+ qcom,stream-id-range = <&apps_smmu 0x400 0x400>;
+ };
+
+ mnoc_hf_0_tbu: tbu@151e5000 {
+ compatible = "qcom,sc7280-tbu";
+ reg = <0x0 0x151e5000 0x0 0x1000>;
+ interconnects = <&mmss_noc MASTER_MDP0 QCOM_ICC_TAG_ACTIVE_ONLY
+ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>;
+ power-domains = <&gcc HLOS1_VOTE_MMNOC_MMU_TBU_HF0_GDSC>;
+ qcom,stream-id-range = <&apps_smmu 0x800 0x400>;
+ };
+
+ mnoc_hf_1_tbu: tbu@151e9000 {
+ compatible = "qcom,sc7280-tbu";
+ reg = <0x0 0x151e9000 0x0 0x1000>;
+ interconnects = <&mmss_noc MASTER_MDP0 QCOM_ICC_TAG_ACTIVE_ONLY
+ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>;
+ power-domains = <&gcc HLOS1_VOTE_MMNOC_MMU_TBU_HF1_GDSC>;
+ qcom,stream-id-range = <&apps_smmu 0xc00 0x400>;
+ };
+
+ compute_dsp_0_tbu: tbu@151ed000 {
+ compatible = "qcom,sc7280-tbu";
+ reg = <0x0 0x151ed000 0x0 0x1000>;
+ interconnects = <&nsp_noc MASTER_CDSP_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>;
+ power-domains = <&gcc HLOS1_VOTE_TURING_MMU_TBU1_GDSC>;
+ qcom,stream-id-range = <&apps_smmu 0x1000 0x400>;
+ };
+
+ compute_dsp_1_tbu: tbu@151f1000 {
+ compatible = "qcom,sc7280-tbu";
+ reg = <0x0 0x151f1000 0x0 0x1000>;
+ interconnects = <&nsp_noc MASTER_CDSP_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>;
+ power-domains = <&gcc HLOS1_VOTE_TURING_MMU_TBU0_GDSC>;
+ qcom,stream-id-range = <&apps_smmu 0x1400 0x400>;
+ };
+
+ adsp_tbu: tbu@151f5000 {
+ compatible = "qcom,sc7280-tbu";
+ reg = <0x0 0x151f5000 0x0 0x1000>;
+ interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+ &lpass_ag_noc SLAVE_LPASS_CORE_CFG QCOM_ICC_TAG_ACTIVE_ONLY>;
+ qcom,stream-id-range = <&apps_smmu 0x1800 0x400>;
+ };
+
+ anoc_1_pcie_tbu: tbu@151f9000 {
+ compatible = "qcom,sc7280-tbu";
+ reg = <0x0 0x151f9000 0x0 0x1000>;
+ interconnects = <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+ &cnoc3 SLAVE_TCU QCOM_ICC_TAG_ACTIVE_ONLY>;
+ qcom,stream-id-range = <&apps_smmu 0x1c00 0x400>;
+ };
+
+ mnoc_sf_0_tbu: tbu@151fd000 {
+ compatible = "qcom,sc7280-tbu";
+ reg = <0x0 0x151fd000 0x0 0x1000>;
+ interconnects = <&mmss_noc MASTER_CAMNOC_SF QCOM_ICC_TAG_ACTIVE_ONLY
+ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ACTIVE_ONLY>;
+ power-domains = <&gcc HLOS1_VOTE_MMNOC_MMU_TBU_SF0_GDSC>;
+ qcom,stream-id-range = <&apps_smmu 0x2000 0x400>;
+ };
+
intc: interrupt-controller@17a00000 {
compatible = "arm,gic-v3";
reg = <0 0x17a00000 0 0x10000>, /* GICD */
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH v3 2/3] arch: Remove struct fb_info from video helpers
From: Thomas Zimmermann @ 2024-03-29 20:32 UTC (permalink / raw)
To: arnd, sam, javierm, deller, sui.jingfeng
Cc: linux-arch, dri-devel, linux-fbdev, sparclinux, linux-sh,
linuxppc-dev, linux-parisc, linux-mips, linux-m68k, loongarch,
linux-arm-kernel, linux-snps-arc, linux-kernel, Thomas Zimmermann,
James E.J. Bottomley, David S. Miller, Andreas Larsson,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin
In-Reply-To: <20240329203450.7824-1-tzimmermann@suse.de>
The per-architecture video helpers do not depend on struct fb_info
or anything else from fbdev. Remove it from the interface and replace
fb_is_primary_device() with video_is_primary_device(). The new helper
is similar in functionality, but can operate on non-fbdev devices.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: Helge Deller <deller@gmx.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Andreas Larsson <andreas@gaisler.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: x86@kernel.org
Cc: "H. Peter Anvin" <hpa@zytor.com>
---
arch/parisc/include/asm/fb.h | 8 +++++---
arch/parisc/video/fbdev.c | 9 +++++----
arch/sparc/include/asm/fb.h | 7 ++++---
arch/sparc/video/fbdev.c | 17 ++++++++---------
arch/x86/include/asm/fb.h | 8 +++++---
arch/x86/video/fbdev.c | 18 +++++++-----------
drivers/video/fbdev/core/fbcon.c | 2 +-
include/asm-generic/fb.h | 11 ++++++-----
8 files changed, 41 insertions(+), 39 deletions(-)
diff --git a/arch/parisc/include/asm/fb.h b/arch/parisc/include/asm/fb.h
index 658a8a7dc5312..ed2a195a3e762 100644
--- a/arch/parisc/include/asm/fb.h
+++ b/arch/parisc/include/asm/fb.h
@@ -2,11 +2,13 @@
#ifndef _ASM_FB_H_
#define _ASM_FB_H_
-struct fb_info;
+#include <linux/types.h>
+
+struct device;
#if defined(CONFIG_STI_CORE)
-int fb_is_primary_device(struct fb_info *info);
-#define fb_is_primary_device fb_is_primary_device
+bool video_is_primary_device(struct device *dev);
+#define video_is_primary_device video_is_primary_device
#endif
#include <asm-generic/fb.h>
diff --git a/arch/parisc/video/fbdev.c b/arch/parisc/video/fbdev.c
index e4f8ac99fc9e0..540fa0c919d59 100644
--- a/arch/parisc/video/fbdev.c
+++ b/arch/parisc/video/fbdev.c
@@ -5,12 +5,13 @@
* Copyright (C) 2001-2002 Thomas Bogendoerfer <tsbogend@alpha.franken.de>
*/
-#include <linux/fb.h>
#include <linux/module.h>
#include <video/sticore.h>
-int fb_is_primary_device(struct fb_info *info)
+#include <asm/fb.h>
+
+bool video_is_primary_device(struct device *dev)
{
struct sti_struct *sti;
@@ -21,6 +22,6 @@ int fb_is_primary_device(struct fb_info *info)
return true;
/* return true if it's the default built-in framebuffer driver */
- return (sti->dev == info->device);
+ return (sti->dev == dev);
}
-EXPORT_SYMBOL(fb_is_primary_device);
+EXPORT_SYMBOL(video_is_primary_device);
diff --git a/arch/sparc/include/asm/fb.h b/arch/sparc/include/asm/fb.h
index 24440c0fda490..07f0325d6921c 100644
--- a/arch/sparc/include/asm/fb.h
+++ b/arch/sparc/include/asm/fb.h
@@ -3,10 +3,11 @@
#define _SPARC_FB_H_
#include <linux/io.h>
+#include <linux/types.h>
#include <asm/page.h>
-struct fb_info;
+struct device;
#ifdef CONFIG_SPARC32
static inline pgprot_t pgprot_framebuffer(pgprot_t prot,
@@ -18,8 +19,8 @@ static inline pgprot_t pgprot_framebuffer(pgprot_t prot,
#define pgprot_framebuffer pgprot_framebuffer
#endif
-int fb_is_primary_device(struct fb_info *info);
-#define fb_is_primary_device fb_is_primary_device
+bool video_is_primary_device(struct device *dev);
+#define video_is_primary_device video_is_primary_device
static inline void fb_memcpy_fromio(void *to, const volatile void __iomem *from, size_t n)
{
diff --git a/arch/sparc/video/fbdev.c b/arch/sparc/video/fbdev.c
index bff66dd1909a4..e46f0499c2774 100644
--- a/arch/sparc/video/fbdev.c
+++ b/arch/sparc/video/fbdev.c
@@ -1,26 +1,25 @@
// SPDX-License-Identifier: GPL-2.0
#include <linux/console.h>
-#include <linux/fb.h>
+#include <linux/device.h>
#include <linux/module.h>
+#include <asm/fb.h>
#include <asm/prom.h>
-int fb_is_primary_device(struct fb_info *info)
+bool video_is_primary_device(struct device *dev)
{
- struct device *dev = info->device;
- struct device_node *node;
+ struct device_node *node = dev->of_node;
if (console_set_on_cmdline)
- return 0;
+ return false;
- node = dev->of_node;
if (node && node == of_console_device)
- return 1;
+ return true;
- return 0;
+ return false;
}
-EXPORT_SYMBOL(fb_is_primary_device);
+EXPORT_SYMBOL(video_is_primary_device);
MODULE_DESCRIPTION("Sparc fbdev helpers");
MODULE_LICENSE("GPL");
diff --git a/arch/x86/include/asm/fb.h b/arch/x86/include/asm/fb.h
index c3b9582de7efd..999db33792869 100644
--- a/arch/x86/include/asm/fb.h
+++ b/arch/x86/include/asm/fb.h
@@ -2,17 +2,19 @@
#ifndef _ASM_X86_FB_H
#define _ASM_X86_FB_H
+#include <linux/types.h>
+
#include <asm/page.h>
-struct fb_info;
+struct device;
pgprot_t pgprot_framebuffer(pgprot_t prot,
unsigned long vm_start, unsigned long vm_end,
unsigned long offset);
#define pgprot_framebuffer pgprot_framebuffer
-int fb_is_primary_device(struct fb_info *info);
-#define fb_is_primary_device fb_is_primary_device
+bool video_is_primary_device(struct device *dev);
+#define video_is_primary_device video_is_primary_device
#include <asm-generic/fb.h>
diff --git a/arch/x86/video/fbdev.c b/arch/x86/video/fbdev.c
index 1dd6528cc947c..4d87ce8e257fe 100644
--- a/arch/x86/video/fbdev.c
+++ b/arch/x86/video/fbdev.c
@@ -7,7 +7,6 @@
*
*/
-#include <linux/fb.h>
#include <linux/module.h>
#include <linux/pci.h>
#include <linux/vgaarb.h>
@@ -25,20 +24,17 @@ pgprot_t pgprot_framebuffer(pgprot_t prot,
}
EXPORT_SYMBOL(pgprot_framebuffer);
-int fb_is_primary_device(struct fb_info *info)
+bool video_is_primary_device(struct device *dev)
{
- struct device *device = info->device;
- struct pci_dev *pci_dev;
+ struct pci_dev *pdev;
- if (!device || !dev_is_pci(device))
- return 0;
+ if (!dev_is_pci(dev))
+ return false;
- pci_dev = to_pci_dev(device);
+ pdev = to_pci_dev(dev);
- if (pci_dev == vga_default_device())
- return 1;
- return 0;
+ return (pdev == vga_default_device());
}
-EXPORT_SYMBOL(fb_is_primary_device);
+EXPORT_SYMBOL(video_is_primary_device);
MODULE_LICENSE("GPL");
diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c
index fcabc668e9fbe..3f7333dca508c 100644
--- a/drivers/video/fbdev/core/fbcon.c
+++ b/drivers/video/fbdev/core/fbcon.c
@@ -2907,7 +2907,7 @@ void fbcon_remap_all(struct fb_info *info)
static void fbcon_select_primary(struct fb_info *info)
{
if (!map_override && primary_device == -1 &&
- fb_is_primary_device(info)) {
+ video_is_primary_device(info->device)) {
int i;
printk(KERN_INFO "fbcon: %s (fb%i) is primary device\n",
diff --git a/include/asm-generic/fb.h b/include/asm-generic/fb.h
index 6ccabb400aa66..4788c1e1c6bc0 100644
--- a/include/asm-generic/fb.h
+++ b/include/asm-generic/fb.h
@@ -10,8 +10,9 @@
#include <linux/io.h>
#include <linux/mm_types.h>
#include <linux/pgtable.h>
+#include <linux/types.h>
-struct fb_info;
+struct device;
#ifndef pgprot_framebuffer
#define pgprot_framebuffer pgprot_framebuffer
@@ -23,11 +24,11 @@ static inline pgprot_t pgprot_framebuffer(pgprot_t prot,
}
#endif
-#ifndef fb_is_primary_device
-#define fb_is_primary_device fb_is_primary_device
-static inline int fb_is_primary_device(struct fb_info *info)
+#ifndef video_is_primary_device
+#define video_is_primary_device video_is_primary_device
+static inline bool video_is_primary_device(struct device *dev)
{
- return 0;
+ return false;
}
#endif
--
2.44.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition
From: kernel test robot @ 2024-03-31 15:02 UTC (permalink / raw)
To: yu-chang.lee, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Ulf Hansson, Matthias Brugger, AngeloGioacchino Del Regno,
MandyJH Liu
Cc: oe-kbuild-all, devicetree, linux-kernel, linux-pm,
linux-arm-kernel, linux-mediatek,
Project_Global_Chrome_Upstream_Group, fan.chen, xiufeng.li,
yu-chang.lee
In-Reply-To: <20240327055732.28198-3-yu-chang.lee@mediatek.com>
Hi yu-chang.lee,
kernel test robot noticed the following build warnings:
[auto build test WARNING on robh/for-next]
[also build test WARNING on krzk-dt/for-next linus/master v6.9-rc1 next-20240328]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/yu-chang-lee/pmdomain-mediatek-add-smi_larb_reset-function-when-power-on/20240327-140007
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20240327055732.28198-3-yu-chang.lee%40mediatek.com
patch subject: [PATCH v2 2/3] dt-bindings: power: Add mediatek larb definition
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240331/202403312222.fjYPC06h-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403312222.fjYPC06h-lkp@intel.com/
dtcheck warnings: (new ones prefixed by >>)
>> Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:128:6: [error] syntax error: expected <block end>, but found '<block mapping start>' (syntax)
>> Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:129:9: [warning] wrong indentation: expected 7 but found 8 (indentation)
--
>> Documentation/devicetree/bindings/power/mediatek,power-controller.yaml:128:6: did not find expected key
>> Documentation/devicetree/bindings/mfd/mediatek,mt8195-scpsys.yaml:
while parsing a block mapping
in "<unicode string>", line 64, column 5
did not find expected key
in "<unicode string>", line 128, column 6
--
>> Documentation/devicetree/bindings/power/mediatek,power-controller.yaml: ignoring, error parsing file
vim +128 Documentation/devicetree/bindings/power/mediatek,power-controller.yaml
8
9 maintainers:
10 - MandyJH Liu <mandyjh.liu@mediatek.com>
11 - Matthias Brugger <mbrugger@suse.com>
12
13 description: |
14 Mediatek processors include support for multiple power domains which can be
15 powered up/down by software based on different application scenes to save power.
16
17 IP cores belonging to a power domain should contain a 'power-domains'
18 property that is a phandle for SCPSYS node representing the domain.
19
20 properties:
21 $nodename:
22 pattern: '^power-controller(@[0-9a-f]+)?$'
23
24 compatible:
25 enum:
26 - mediatek,mt6795-power-controller
27 - mediatek,mt8167-power-controller
28 - mediatek,mt8173-power-controller
29 - mediatek,mt8183-power-controller
30 - mediatek,mt8186-power-controller
31 - mediatek,mt8188-power-controller
32 - mediatek,mt8192-power-controller
33 - mediatek,mt8195-power-controller
34 - mediatek,mt8365-power-controller
35
36 '#power-domain-cells':
37 const: 1
38
39 '#address-cells':
40 const: 1
41
42 '#size-cells':
43 const: 0
44
45 patternProperties:
46 "^power-domain@[0-9a-f]+$":
47 $ref: "#/$defs/power-domain-node"
48 patternProperties:
49 "^power-domain@[0-9a-f]+$":
50 $ref: "#/$defs/power-domain-node"
51 patternProperties:
52 "^power-domain@[0-9a-f]+$":
53 $ref: "#/$defs/power-domain-node"
54 patternProperties:
55 "^power-domain@[0-9a-f]+$":
56 $ref: "#/$defs/power-domain-node"
57 unevaluatedProperties: false
58 unevaluatedProperties: false
59 unevaluatedProperties: false
60 unevaluatedProperties: false
61
62 $defs:
63 power-domain-node:
64 type: object
65 description: |
66 Represents the power domains within the power controller node as documented
67 in Documentation/devicetree/bindings/power/power-domain.yaml.
68
69 properties:
70
71 '#power-domain-cells':
72 description:
73 Must be 0 for nodes representing a single PM domain and 1 for nodes
74 providing multiple PM domains.
75
76 '#address-cells':
77 const: 1
78
79 '#size-cells':
80 const: 0
81
82 reg:
83 description: |
84 Power domain index. Valid values are defined in:
85 "include/dt-bindings/power/mt6795-power.h" - for MT8167 type power domain.
86 "include/dt-bindings/power/mt8167-power.h" - for MT8167 type power domain.
87 "include/dt-bindings/power/mt8173-power.h" - for MT8173 type power domain.
88 "include/dt-bindings/power/mt8183-power.h" - for MT8183 type power domain.
89 "include/dt-bindings/power/mediatek,mt8188-power.h" - for MT8188 type power domain.
90 "include/dt-bindings/power/mt8192-power.h" - for MT8192 type power domain.
91 "include/dt-bindings/power/mt8195-power.h" - for MT8195 type power domain.
92 "include/dt-bindings/power/mediatek,mt8365-power.h" - for MT8365 type power domain.
93 maxItems: 1
94
95 clocks:
96 description: |
97 A number of phandles to clocks that need to be enabled during domain
98 power-up sequencing.
99
100 clock-names:
101 description: |
102 List of names of clocks, in order to match the power-up sequencing
103 for each power domain we need to group the clocks by name. BASIC
104 clocks need to be enabled before enabling the corresponding power
105 domain, and should not have a '-' in their name (i.e mm, mfg, venc).
106 SUSBYS clocks need to be enabled before releasing the bus protection,
107 and should contain a '-' in their name (i.e mm-0, isp-0, cam-0).
108
109 In order to follow properly the power-up sequencing, the clocks must
110 be specified by order, adding first the BASIC clocks followed by the
111 SUSBSYS clocks.
112
113 domain-supply:
114 description: domain regulator supply.
115
116 mediatek,infracfg:
117 $ref: /schemas/types.yaml#/definitions/phandle
118 description: phandle to the device containing the INFRACFG register range.
119
120 mediatek,infracfg-nao:
121 $ref: /schemas/types.yaml#/definitions/phandle
122 description: phandle to the device containing the INFRACFG-NAO register range.
123
124 mediatek,smi:
125 $ref: /schemas/types.yaml#/definitions/phandle
126 description: phandle to the device containing the SMI register range.
127
> 128 mediatek,larb:
> 129 $ref: /schemas/types.yaml#/definitions/phandle
130 description: phandle to the device containing the LARB register range.
131
132 required:
133 - reg
134
135 required:
136 - compatible
137
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6 0/4] fpga: xilinx-selectmap: add new driver
From: Xu Yilun @ 2024-03-31 14:50 UTC (permalink / raw)
To: Charles Perry
Cc: mdf, avandiver, bcody, Wu Hao, Xu Yilun, Tom Rix, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Michal Simek, linux-fpga,
devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <20240321220447.3260065-1-charles.perry@savoirfairelinux.com>
On Thu, Mar 21, 2024 at 06:04:32PM -0400, Charles Perry wrote:
> Hello,
>
> This patchset adds a new driver for the 7 series FPGA's SelectMAP
> interface.
>
> The SelectMAP interface shares a common GPIO protocol with the SPI
> interface which is already in the kernel (drivers/fpga/xilinx-spi.c).
> The approach proposed in this patchset is to refactor xilinx-spi.c into
> xilinx-core.c which would handle the common GPIO protocol. This is then
> used to build two drivers, the already existing xilinx-spi.c driver and
> a newly added xilinx-selectmap.c driver.
>
> The SelectMAP driver proposed only supports 8 bit mode. This is because
> the 16 and 32 bits mode have limitations with regards to compressed
> bitstream support as well as introducing endianness considerations.
>
> I'm testing xilinx-selectmap.c on a custom i.MX6 board connected to an
> Artix 7 FPGA. Flashing a 913K bitstream takes 0.44 seconds.
>
> Changes since v5: (from Yilun review)
> * xilinx-core.h: remove private fields kernel-doc
> * xilinx-spi.c: rename conf into core in xilinx_spi_probe
> * xilinx-core.c: introduce the new gpio names in patch 4/4
> * xilinx-core.c: remove kernel-doc on xilinx_core_devm_gpiod_get()
> * xilinx-selectmap.c:
> * reorder includes in alphabetical order
> * xilinx_selectmap_probe(): remove unused resource *r variable
> * xilinx_selectmap_probe(): use a single gpio_desc* temporary
> * xilinx_selectmap_probe(): declare variables in reverse xmas tree
>
> Changes since v4: (from Yilun and Krzysztof review)
> * xilinx-core: use sizeof() instead of hardcoded immediate
> * xilinx-core: fix module compilation (EXPORT_SYMBOL_GPL, MODULE_LICENSE,
> MODULE_AUTHOR, MODULE_DESCRIPTION)
> * xilinx-core: add private/public qualifiers for struct xilinx_fpga_core
> * xilinx-spi: remove struct xilinx_spi_conf. This struct isn't needed as
> the struct spi_device* can be retrieved from the struct device*.
> * dt-bindings: remove usage of "_b" and "-b" for the new driver. We
> agreed that the spi and selectmap driver will use different bindings
> which will be handled by the driver core and that the legacy names will
> be used only for the spi compatible.
> * xilinx-core: select between prog/init and prog_b/init-b
>
> Changes since v3: (from Rob Herring review)
> * Fix an error in the DT binding example compatible.
> * Drop the renaming of "prog_b" to "prog" and "init-b" to "init".
> Patches 2 and 3 are removed.
>
> Changes since v2:
> * Inserted patch 2 and 3 which rename "prog_b" and "init-b" into "prog"
> and "init" for the SPI driver.
> * From Krzysztof Kozlowski review's:
> * Use more specific compatible names
> * Remove other missing occurences of the slave word missed in v2.
> * From Xu Yilun review's:
> * Fix vertical whitespace in get_done_gpio().
> * Combine write() and write_one_dummy_byte() together.
> * Eliminate most of the xilinx_core_probe() arguments, the driver
> needs to populate those directly into the xilinx_fpga_core struct.
> Added some documentation to struct xilinx_fpga_core to clarify
> this.
> * Removed typedefs from xilinx-core.h.
> * Moved null checks in xilinx_core_probe() to first patch.
> * Move csi_b and rdwr_b out of xilinx_selectmap_conf as they are not
> used out of the probe function.
>
> Changes since v1: (from Krzysztof Kozlowski review's)
> * Use more conventional names for gpio DT bindings
> * fix example in DT bindings
> * add mc-peripheral-props.yaml to DT bindings
> * fix various formatting mistakes
> * Remove all occurences of the "slave" word.
>
> Charles Perry (4):
> fpga: xilinx-spi: extract a common driver core
> dt-bindings: fpga: xlnx,fpga-selectmap: add DT schema
> fpga: xilinx-selectmap: add new driver
> xilinx-core: add new gpio names for prog and init
>
> .../bindings/fpga/xlnx,fpga-selectmap.yaml | 86 +++++++
> drivers/fpga/Kconfig | 12 +
> drivers/fpga/Makefile | 2 +
> drivers/fpga/xilinx-core.c | 229 ++++++++++++++++++
> drivers/fpga/xilinx-core.h | 27 +++
> drivers/fpga/xilinx-selectmap.c | 95 ++++++++
> drivers/fpga/xilinx-spi.c | 224 ++---------------
> 7 files changed, 466 insertions(+), 209 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,fpga-selectmap.yaml
> create mode 100644 drivers/fpga/xilinx-core.c
> create mode 100644 drivers/fpga/xilinx-core.h
> create mode 100644 drivers/fpga/xilinx-selectmap.c
Applied this series to for-next with a nit.
Thanks,
Yilun
>
> --
> 2.43.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6 3/4] fpga: xilinx-selectmap: add new driver
From: Xu Yilun @ 2024-03-31 14:34 UTC (permalink / raw)
To: Charles Perry
Cc: mdf, avandiver, bcody, Wu Hao, Xu Yilun, Tom Rix, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Michal Simek, linux-fpga,
devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <20240321220447.3260065-4-charles.perry@savoirfairelinux.com>
> +static int xilinx_selectmap_write(struct xilinx_fpga_core *core,
> + const char *buf, size_t count)
> +{
> + struct xilinx_selectmap_conf *conf = to_xilinx_selectmap_conf(core);
> + u32 i;
comparing u32 with size_t is problematic.
size_t i;
I can fix it in place.
Thanks,
Yilun
> +
> + for (i = 0; i < count; ++i)
> + writeb(buf[i], conf->base);
> +
> + return 0;
> +}
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
From: Peng Fan @ 2024-03-31 12:47 UTC (permalink / raw)
To: Dan Carpenter, Peng Fan (OSS)
Cc: Sudeep Holla, Cristian Marussi, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Linus Walleij, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, Oleksii Moisieiev
In-Reply-To: <4879ad5d-165c-4118-81f7-8f6348a5a5d4@moroto.mountain>
Hi Dan,
> Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
>
> Looks really nice. Just a few small comments below.
>
> On Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) wrote:
> > +
> > +struct scmi_msg_func_set {
> > + __le32 identifier;
> > + __le32 function_id;
> > + __le32 flags;
> > +};
>
> This scmi_msg_func_set struct is unused. Delete.
>
> > +static void
> > +iter_pinctrl_settings_get_prepare_message(void *message, u32 desc_index,
> > + const void *priv)
> > +{
> > + struct scmi_msg_settings_get *msg = message;
> > + const struct scmi_settings_get_ipriv *p = priv;
> > + u32 attributes;
> > +
> > + attributes = FIELD_PREP(CONFIG_FLAG_MASK, p->flag) |
> > + FIELD_PREP(SELECTOR_MASK, p->type);
> > +
> > + if (p->flag == 1)
> > + attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index);
> > + else if (!p->flag)
>
> This is a nit-pick but could you change these !p->flag conditions to
> p->flag == 0? It's a number zero, not a bool.
>
> > + attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p-
> >config_types[0]);
> > +
> > + msg->attributes = cpu_to_le32(attributes);
> > + msg->identifier = cpu_to_le32(p->selector); }
> > +
> > +static int
> > +iter_pinctrl_settings_get_update_state(struct scmi_iterator_state *st,
> > + const void *response, void *priv) {
> > + const struct scmi_resp_settings_get *r = response;
> > + struct scmi_settings_get_ipriv *p = priv;
> > +
> > + if (p->flag == 1) {
> > + st->num_returned = le32_get_bits(r->num_configs,
> GENMASK(7, 0));
> > + st->num_remaining = le32_get_bits(r->num_configs,
> > + GENMASK(31, 24));
> > + } else {
> > + st->num_returned = 1;
> > + st->num_remaining = 0;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +iter_pinctrl_settings_get_process_response(const struct
> scmi_protocol_handle *ph,
> > + const void *response,
> > + struct scmi_iterator_state *st,
> > + void *priv)
> > +{
> > + const struct scmi_resp_settings_get *r = response;
> > + struct scmi_settings_get_ipriv *p = priv;
> > +
> > + if (!p->flag) {
>
>
> if (p->flag == 0) {
>
> > + if (p->config_types[0] !=
> > + le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, 0)))
> > + return -EINVAL;
> > + } else if (p->flag == 1) {
> > + p->config_types[st->desc_index + st->loop_idx] =
> > + le32_get_bits(r->configs[st->loop_idx * 2],
> > + GENMASK(7, 0));
> > + } else if (p->flag == 2) {
> > + return 0;
> > + }
> > +
> > + p->config_values[st->desc_index + st->loop_idx] =
> > + le32_to_cpu(r->configs[st->loop_idx * 2 + 1]);
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +scmi_pinctrl_settings_get(const struct scmi_protocol_handle *ph, u32
> selector,
> > + enum scmi_pinctrl_selector_type type,
> > + enum scmi_pinctrl_conf_type config_type,
> > + u32 *config_value)
> > +{
> > + int ret;
> > + void *iter;
> > + struct scmi_iterator_ops ops = {
> > + .prepare_message =
> iter_pinctrl_settings_get_prepare_message,
> > + .update_state = iter_pinctrl_settings_get_update_state,
> > + .process_response =
> iter_pinctrl_settings_get_process_response,
> > + };
> > + struct scmi_settings_get_ipriv ipriv = {
> > + .selector = selector,
> > + .type = type,
> > + .flag = 0,
>
> ->flag should be 0-2.
>
> > + .config_types = &config_type,
> > + .config_values = config_value,
> > + };
> > +
> > + if (!config_value || type == FUNCTION_TYPE)
> ^^^^^^^^^^^^
> config_value should be optional for flag == 2.
As Cristian replied, I would keep it as is until we have a case in
linux that need flag == 2.
Thanks,
Peng
>
> regards,
> dan carpenter
>
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_validate_id(ph, selector, type);
> > + if (ret)
> > + return ret;
> > +
> > + iter = ph->hops->iter_response_init(ph, &ops, 1,
> PINCTRL_SETTINGS_GET,
> > + sizeof(struct
> scmi_msg_settings_get),
> > + &ipriv);
> > +
> > + if (IS_ERR(iter))
> > + return PTR_ERR(iter);
> > +
> > + return ph->hops->iter_response_run(iter);
> > +}
> > +
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
From: Peng Fan @ 2024-03-31 13:44 UTC (permalink / raw)
To: Andy Shevchenko, Peng Fan (OSS)
Cc: Sudeep Holla, Cristian Marussi, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Linus Walleij, Dan Carpenter,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, Oleksii Moisieiev
In-Reply-To: <ZgcP4IkTQGks9ehH@surfacebook.localdomain>
Hi Andy,
> Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
>
> Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) kirjoitti:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Add basic implementation of the SCMI v3.2 pincontrol protocol.
>
> ...
>
> > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> > scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o
> > system.o voltage.o powercap.o
>
> Actually you want to have := here.
>
> > +scmi-protocols-y += pinctrl.o
>
>
>
> > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y)
> > $(scmi-transport-y)
>
> Side note: The -objs has to be -y
>
> ...
>
> > +#include <linux/module.h>
> > +#include <linux/scmi_protocol.h>
> > +#include <linux/slab.h>
>
> This is semi-random list of headers. Please, follow IWYU principle (include
> what you use). There are a lot of inclusions I see missing (just in the context of
> this page I see bits.h, types.h, and asm/byteorder.h).
Is there any documentation about this requirement?
Some headers are already included by others.
>
> ...
>
> > +enum scmi_pinctrl_protocol_cmd {
> > + PINCTRL_ATTRIBUTES = 0x3,
> > + PINCTRL_LIST_ASSOCIATIONS = 0x4,
> > + PINCTRL_SETTINGS_GET = 0x5,
> > + PINCTRL_SETTINGS_CONFIGURE = 0x6,
> > + PINCTRL_REQUEST = 0x7,
> > + PINCTRL_RELEASE = 0x8,
> > + PINCTRL_NAME_GET = 0x9,
> > + PINCTRL_SET_PERMISSIONS = 0xa
>
> Leave trailing comma as it's not a termination.
>
> > +};
>
> ...
>
> > +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle
> *ph,
> > + struct scmi_pinctrl_info *pi) {
> > + int ret;
> > + struct scmi_xfer *t;
> > + struct scmi_msg_pinctrl_protocol_attributes *attr;
> > +
> > + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0,
> sizeof(*attr),
> > + &t);
>
> This looks much better on a single line.
Per Cristian, scmi drivers keep 80 max chars.
>
> > + if (ret)
> > + return ret;
> > +
> > + attr = t->rx.buf;
> > +
> > + ret = ph->xops->do_xfer(ph, t);
> > + if (!ret) {
> > + pi->nr_functions = GET_FUNCTIONS_NR(attr-
> >attributes_high);
> > + pi->nr_groups = GET_GROUPS_NR(attr->attributes_low);
> > + pi->nr_pins = GET_PINS_NR(attr->attributes_low);
> > + }
> > +
> > + ph->xops->xfer_put(ph, t);
> > + return ret;
> > +}
>
> ...
>
> > + ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx),
> > + sizeof(*rx), &t);
>
> Possible to have on a single line (if you use relaxed 100 limit).
> Or (re)split it more logically:
>
> ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES,
> sizeof(*tx), sizeof(*rx), &t);
>
> > + if (ret)
> > + return ret;
>
> ...
>
> > + /*
> > + * If supported overwrite short name with the extended one;
> > + * on error just carry on and use already provided short name.
> > + */
> > + if (!ret && ext_name_flag)
>
> Please, use standard pattern, i.e.
>
> if (ret)
> return ret;
>
> > + ph->hops->extended_name_get(ph, PINCTRL_NAME_GET,
> selector,
> > + (u32 *)&type, name,
>
> Why is an explicit casting needed?
The type is enum, not u32.
>
> > + SCMI_MAX_STR_SIZE);
> > + return ret;
>
> ...
>
> > + iter = ph->hops->iter_response_init(ph, &ops, size,
> > + PINCTRL_LIST_ASSOCIATIONS,
> > + sizeof(struct
> scmi_msg_pinctrl_list_assoc),
> > + &ipriv);
>
> > +
>
> Redundant blank line.
>
> > + if (IS_ERR(iter))
> > + return PTR_ERR(iter);
>
> ...
>
> > + if (p->flag == 1)
> > + attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index);
> > + else if (!p->flag)
>
> Be consistent, i.e. if (p->flag == 0)
>
> > + attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p-
> >config_types[0]);
>
> ...
>
> > + st->num_remaining = le32_get_bits(r->num_configs,
> > + GENMASK(31, 24));
>
> One line?
Scmi drivers use 80 max drivers.
>
> ...
>
> > + if (!p->flag) {
> > + if (p->config_types[0] !=
> > + le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, 0)))
> > + return -EINVAL;
> > + } else if (p->flag == 1) {
> > + p->config_types[st->desc_index + st->loop_idx] =
> > + le32_get_bits(r->configs[st->loop_idx * 2],
> > + GENMASK(7, 0));
>
> With a temporary variable for r->configs[st->loop_idx * 2] the above can be
> written in much better way.
ok. Fix in v7.
>
> > + } else if (p->flag == 2) {
> > + return 0;
> > + }
>
> > + p->config_values[st->desc_index + st->loop_idx] =
> > + le32_to_cpu(r->configs[st->loop_idx * 2 + 1]);
>
> For the sake of consistency with the above suggestion also temporary for next
> config value.
>
> ...
>
> > + iter = ph->hops->iter_response_init(ph, &ops, 1,
> PINCTRL_SETTINGS_GET,
> > + sizeof(struct
> scmi_msg_settings_get),
> > + &ipriv);
>
> > +
>
> Redundant blank line.
>
> > + if (IS_ERR(iter))
> > + return PTR_ERR(iter);
>
> ...
>
> > +static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle
> *ph,
> > + u32 selector,
> > + struct scmi_group_info *group) {
> > + int ret;
>
> > + if (!group)
> > + return -EINVAL;
>
> When the above is not a dead code?
It could be removed.
>
> > + ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector,
> > + group->name,
> > + &group->nr_pins);
> > + if (ret)
> > + return ret;
> > +
> > + if (!group->nr_pins) {
> > + dev_err(ph->dev, "Group %d has 0 elements", selector);
> > + return -ENODATA;
> > + }
> > +
> > + group->group_pins = kmalloc_array(group->nr_pins,
> > + sizeof(*group->group_pins),
> > + GFP_KERNEL);
> > + if (!group->group_pins)
> > + return -ENOMEM;
> > +
> > + ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
> > + group->nr_pins, group-
> >group_pins);
> > + if (ret) {
> > + kfree(group->group_pins);
> > + return ret;
> > + }
> > +
> > + group->present = true;
> > + return 0;
> > +}
>
> ...
>
> > + ret = scmi_pinctrl_get_group_info(ph, selector,
> > + &pi->groups[selector]);
>
> One line?
>
> > + if (ret)
> > + return ret;
>
> ...
>
> > + ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector,
> > + func->name,
> > + &func->nr_groups);
>
> At least last two lines can be joined.
>
> > + if (ret)
> > + return ret;
>
> ...
>
> > + ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector,
> > + pin->name, NULL);
>
> It's pleany of room on the previous line.
>
> > + if (ret)
> > + return ret;
>
> ...
>
> > + ret = scmi_pinctrl_get_pin_info(ph, selector,
> > + &pi->pins[selector]);
>
> Ditto.
>
> > + if (ret)
> > + return ret;
>
> ...
>
> > +static int scmi_pinctrl_protocol_init(const struct
> > +scmi_protocol_handle *ph) {
> > + int ret;
> > + u32 version;
> > + struct scmi_pinctrl_info *pinfo;
> > +
> > + ret = ph->xops->version_get(ph, &version);
> > + if (ret)
> > + return ret;
> > +
> > + dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
> > + PROTOCOL_REV_MAJOR(version),
> PROTOCOL_REV_MINOR(version));
> > +
> > + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
>
> Huh?!
>
> Please, get yourself familiar with the scope of devm APIs.
Please teach me if this is wrong.
>
> > + if (!pinfo)
> > + return -ENOMEM;
> > +
> > + ret = scmi_pinctrl_attributes_get(ph, pinfo);
> > + if (ret)
> > + return ret;
> > +
> > + pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins,
> > + sizeof(*pinfo->pins),
> > + GFP_KERNEL);
> > + if (!pinfo->pins)
> > + return -ENOMEM;
> > +
> > + pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups,
> > + sizeof(*pinfo->groups),
> > + GFP_KERNEL);
> > + if (!pinfo->groups)
> > + return -ENOMEM;
> > +
> > + pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions,
> > + sizeof(*pinfo->functions),
> > + GFP_KERNEL);
> > + if (!pinfo->functions)
> > + return -ENOMEM;
> > +
> > + pinfo->version = version;
> > +
> > + return ph->set_priv(ph, pinfo, version); }
> > +
> > +static int scmi_pinctrl_protocol_deinit(const struct
> > +scmi_protocol_handle *ph) {
> > + int i;
> > + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> > +
> > + for (i = 0; i < pi->nr_groups; i++) {
> > + if (pi->groups[i].present) {
> > + kfree(pi->groups[i].group_pins);
> > + pi->groups[i].present = false;
> > + }
> > + }
> > +
> > + for (i = 0; i < pi->nr_functions; i++) {
> > + if (pi->functions[i].present) {
> > + kfree(pi->functions[i].groups);
>
> This is wrong in conjunction with the above.
Yeah.
>
> > + pi->functions[i].present = false;
> > + }
> > + }
> > +
> > + return 0;
> > +}
>
> ...
>
> > +static const struct scmi_protocol scmi_pinctrl = {
> > + .id = SCMI_PROTOCOL_PINCTRL,
> > + .owner = THIS_MODULE,
> > + .instance_init = &scmi_pinctrl_protocol_init,
> > + .instance_deinit = &scmi_pinctrl_protocol_deinit,
> > + .ops = &pinctrl_proto_ops,
> > + .supported_version = SCMI_PROTOCOL_SUPPORTED_VERSION, };
>
> > +
>
> Redundant blank line.
Fix in v7
Thanks,
Peng.
>
> > +DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(pinctrl, scmi_pinctrl)
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support
From: Peng Fan @ 2024-03-31 13:28 UTC (permalink / raw)
To: Cristian Marussi, Peng Fan (OSS)
Cc: Sudeep Holla, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, Dan Carpenter,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-gpio@vger.kernel.org, Oleksii Moisieiev
In-Reply-To: <ZgWRA2V3PF_q9yRM@pluto>
Hi Cristian,
> Subject: Re: [PATCH v6 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
>
> On Sat, Mar 23, 2024 at 08:15:16PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Add basic implementation of the SCMI v3.2 pincontrol protocol.
> >
>
> Hi,
>
> a few more comments down below...
>
> > Co-developed-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@epam.com>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> > drivers/firmware/arm_scmi/Makefile | 1 +
> > drivers/firmware/arm_scmi/driver.c | 2 +
> > drivers/firmware/arm_scmi/pinctrl.c | 921
> ++++++++++++++++++++++++++++++++++
> > drivers/firmware/arm_scmi/protocols.h | 1 +
> > include/linux/scmi_protocol.h | 75 +++
> > 5 files changed, 1000 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_scmi/Makefile
> > b/drivers/firmware/arm_scmi/Makefile
> > index a7bc4796519c..8e3874ff1544 100644
> > --- a/drivers/firmware/arm_scmi/Makefile
> > +++ b/drivers/firmware/arm_scmi/Makefile
> > @@ -11,6 +11,7 @@ scmi-transport-$(CONFIG_ARM_SCMI_HAVE_MSG) +=
> msg.o
> > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_VIRTIO) += virtio.o
> > scmi-transport-$(CONFIG_ARM_SCMI_TRANSPORT_OPTEE) += optee.o
> > scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o
> > system.o voltage.o powercap.o
> > +scmi-protocols-y += pinctrl.o
> > scmi-module-objs := $(scmi-driver-y) $(scmi-protocols-y)
> > $(scmi-transport-y)
> >
> > obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-core.o diff --git
> > a/drivers/firmware/arm_scmi/driver.c
> > b/drivers/firmware/arm_scmi/driver.c
> > index 415e6f510057..ac2d4b19727c 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -3142,6 +3142,7 @@ static int __init scmi_driver_init(void)
> > scmi_voltage_register();
> > scmi_system_register();
> > scmi_powercap_register();
> > + scmi_pinctrl_register();
> >
> > return platform_driver_register(&scmi_driver);
> > }
> > @@ -3159,6 +3160,7 @@ static void __exit scmi_driver_exit(void)
> > scmi_voltage_unregister();
> > scmi_system_unregister();
> > scmi_powercap_unregister();
> > + scmi_pinctrl_unregister();
> >
> > scmi_transports_exit();
> >
> > diff --git a/drivers/firmware/arm_scmi/pinctrl.c
> > b/drivers/firmware/arm_scmi/pinctrl.c
> > new file mode 100644
> > index 000000000000..87d9b89cab13
> > --- /dev/null
> > +++ b/drivers/firmware/arm_scmi/pinctrl.c
> > @@ -0,0 +1,921 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * System Control and Management Interface (SCMI) Pinctrl Protocol
> > + *
> > + * Copyright (C) 2024 EPAM
> > + * Copyright 2024 NXP
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/scmi_protocol.h>
> > +#include <linux/slab.h>
> > +
> > +#include "common.h"
> > +#include "protocols.h"
> > +
> > +/* Updated only after ALL the mandatory features for that version are
> merged */
> > +#define SCMI_PROTOCOL_SUPPORTED_VERSION 0x0
> > +
>
> AFAICS, the only missing things are PINCTRL_SET_PERMISSIONS (optional
> command)
I not see users as of now, could we add it until we need it?
and the multiple-configs on SETTINGS_GET, but this latter is
> something really that we have to ask for in the request AND we did not as of
> now since we dont need it...so I would say to bump the version to 0x10000
ok.
> just to avoid needless warning as soon as a server supporting Pinctrl is met.
>
> > +#define GET_GROUPS_NR(x) le32_get_bits((x), GENMASK(31, 16))
> > +#define GET_PINS_NR(x) le32_get_bits((x), GENMASK(15, 0))
> > +#define GET_FUNCTIONS_NR(x) le32_get_bits((x), GENMASK(15, 0))
> > +
> > +#define EXT_NAME_FLAG(x) le32_get_bits((x), BIT(31))
> > +#define NUM_ELEMS(x) le32_get_bits((x), GENMASK(15, 0))
> > +
> > +#define REMAINING(x) le32_get_bits((x), GENMASK(31,
> 16))
> > +#define RETURNED(x) le32_get_bits((x), GENMASK(11, 0))
> > +
> > +#define CONFIG_FLAG_MASK GENMASK(19, 18)
> > +#define SELECTOR_MASK GENMASK(17, 16)
> > +#define SKIP_CONFIGS_MASK GENMASK(15, 8)
> > +#define CONFIG_TYPE_MASK GENMASK(7, 0)
> > +
> > +enum scmi_pinctrl_protocol_cmd {
> > + PINCTRL_ATTRIBUTES = 0x3,
> > + PINCTRL_LIST_ASSOCIATIONS = 0x4,
> > + PINCTRL_SETTINGS_GET = 0x5,
> > + PINCTRL_SETTINGS_CONFIGURE = 0x6,
> > + PINCTRL_REQUEST = 0x7,
> > + PINCTRL_RELEASE = 0x8,
> > + PINCTRL_NAME_GET = 0x9,
> > + PINCTRL_SET_PERMISSIONS = 0xa
> > +};
> > +
> > +struct scmi_msg_settings_conf {
> > + __le32 identifier;
> > + __le32 function_id;
> > + __le32 attributes;
> > + __le32 configs[];
> > +};
> > +
> > +struct scmi_msg_settings_get {
> > + __le32 identifier;
> > + __le32 attributes;
> > +};
> > +
> > +struct scmi_resp_settings_get {
> > + __le32 function_selected;
> > + __le32 num_configs;
> > + __le32 configs[];
> > +};
> > +
> > +struct scmi_msg_pinctrl_protocol_attributes {
> > + __le32 attributes_low;
> > + __le32 attributes_high;
> > +};
> > +
> > +struct scmi_msg_pinctrl_attributes {
> > + __le32 identifier;
> > + __le32 flags;
> > +};
> > +
> > +struct scmi_resp_pinctrl_attributes {
> > + __le32 attributes;
> > + u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> > +};
> > +
> > +struct scmi_msg_pinctrl_list_assoc {
> > + __le32 identifier;
> > + __le32 flags;
> > + __le32 index;
> > +};
> > +
> > +struct scmi_resp_pinctrl_list_assoc {
> > + __le32 flags;
> > + __le16 array[];
> > +};
> > +
> > +struct scmi_msg_func_set {
> > + __le32 identifier;
> > + __le32 function_id;
> > + __le32 flags;
> > +};
> > +
>
> As said by Dan...drop this.
>
> > +struct scmi_msg_request {
> > + __le32 identifier;
> > + __le32 flags;
> > +};
> > +
> > +struct scmi_group_info {
> > + char name[SCMI_MAX_STR_SIZE];
> > + bool present;
> > + u32 *group_pins;
> > + u32 nr_pins;
> > +};
> > +
> > +struct scmi_function_info {
> > + char name[SCMI_MAX_STR_SIZE];
> > + bool present;
> > + u32 *groups;
> > + u32 nr_groups;
> > +};
> > +
> > +struct scmi_pin_info {
> > + char name[SCMI_MAX_STR_SIZE];
> > + bool present;
> > +};
> > +
> > +struct scmi_pinctrl_info {
> > + u32 version;
> > + int nr_groups;
> > + int nr_functions;
> > + int nr_pins;
> > + struct scmi_group_info *groups;
> > + struct scmi_function_info *functions;
> > + struct scmi_pin_info *pins;
> > +};
> > +
> > +static int scmi_pinctrl_attributes_get(const struct scmi_protocol_handle
> *ph,
> > + struct scmi_pinctrl_info *pi) {
> > + int ret;
> > + struct scmi_xfer *t;
> > + struct scmi_msg_pinctrl_protocol_attributes *attr;
> > +
> > + ret = ph->xops->xfer_get_init(ph, PROTOCOL_ATTRIBUTES, 0,
> sizeof(*attr),
> > + &t);
> > + if (ret)
> > + return ret;
> > +
> > + attr = t->rx.buf;
> > +
> > + ret = ph->xops->do_xfer(ph, t);
> > + if (!ret) {
> > + pi->nr_functions = GET_FUNCTIONS_NR(attr-
> >attributes_high);
> > + pi->nr_groups = GET_GROUPS_NR(attr->attributes_low);
> > + pi->nr_pins = GET_PINS_NR(attr->attributes_low);
>
> I was thinking, does make sense to allow a nr_pins == 0 setup to probe
> successfully ? Becasuse is legit for the platform to return zero groups or zero
> functions BUT zero pins is just useless (spec does not say
> anything)
>
> Maybe you could just put a dev_warn() here on if (nr_pins == 0) and bail out
> with -EINVAL...
ok, fix in v7.
>
> On the other side looking at the zero groups/function case, that is plausible
> and handled properly by the driver since a 0-bytes devm_kcalloc will return
> ZERO_SIZE_PTR (not NULL) and all the remaining references to pinfo->groups
> and pinfo->functions are guarded by a check on selector >= nr_groups (or >=
> nr_functions), and by scmi_pinctrl_validate_id() so the zero grouyps/fuctions
> scenarios should be safely handled.
>
> > + }
> > +
> > + ph->xops->xfer_put(ph, t);
> > + return ret;
> > +}
> > +
> > +static int scmi_pinctrl_count_get(const struct scmi_protocol_handle *ph,
> > + enum scmi_pinctrl_selector_type type) {
> > + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> > +
> > + switch (type) {
> > + case PIN_TYPE:
> > + return pi->nr_pins;
> > + case GROUP_TYPE:
> > + return pi->nr_groups;
> > + case FUNCTION_TYPE:
> > + return pi->nr_functions;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int scmi_pinctrl_validate_id(const struct scmi_protocol_handle *ph,
> > + u32 identifier,
> > + enum scmi_pinctrl_selector_type type) {
> > + int value;
> > +
> > + value = scmi_pinctrl_count_get(ph, type);
> > + if (value < 0)
> > + return value;
> > +
> > + if (identifier >= value)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_attributes(const struct scmi_protocol_handle *ph,
> > + enum scmi_pinctrl_selector_type type,
> > + u32 selector, char *name,
> > + u32 *n_elems)
> > +{
> > + int ret;
> > + struct scmi_xfer *t;
> > + struct scmi_msg_pinctrl_attributes *tx;
> > + struct scmi_resp_pinctrl_attributes *rx;
> > + u32 ext_name_flag;
>
> what about a bool
>
> > +
> > + if (!name)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_validate_id(ph, selector, type);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ph->xops->xfer_get_init(ph, PINCTRL_ATTRIBUTES, sizeof(*tx),
> > + sizeof(*rx), &t);
> > + if (ret)
> > + return ret;
> > +
> > + tx = t->tx.buf;
> > + rx = t->rx.buf;
> > + tx->identifier = cpu_to_le32(selector);
> > + tx->flags = cpu_to_le32(type);
> > +
> > + ret = ph->xops->do_xfer(ph, t);
> > + if (!ret) {
> > + if (n_elems)
> > + *n_elems = NUM_ELEMS(rx->attributes);
> > +
> > + strscpy(name, rx->name, SCMI_SHORT_NAME_MAX_SIZE);
> > +
> > + ext_name_flag = EXT_NAME_FLAG(rx->attributes);
> > + } else
> > + ext_name_flag = 0;
>
> and you dont need this else branch to set ext_name_flag to false, since down
> below you will check ext_flag ONLY if !ret, so it will have surely been set if the
> do_xfer did not fail.
>
> > +
> > + ph->xops->xfer_put(ph, t);
> > +
> > + /*
> > + * If supported overwrite short name with the extended one;
> > + * on error just carry on and use already provided short name.
> > + */
> > + if (!ret && ext_name_flag)
> > + ph->hops->extended_name_get(ph, PINCTRL_NAME_GET,
> selector,
> > + (u32 *)&type, name,
> > + SCMI_MAX_STR_SIZE);
> > + return ret;
> > +}
> > +
> > +struct scmi_pinctrl_ipriv {
> > + u32 selector;
> > + enum scmi_pinctrl_selector_type type;
> > + u32 *array;
> > +};
> > +
> > +static void iter_pinctrl_assoc_prepare_message(void *message,
> > + u32 desc_index,
> > + const void *priv)
> > +{
> > + struct scmi_msg_pinctrl_list_assoc *msg = message;
> > + const struct scmi_pinctrl_ipriv *p = priv;
> > +
> > + msg->identifier = cpu_to_le32(p->selector);
> > + msg->flags = cpu_to_le32(p->type);
> > + /* Set the number of OPPs to be skipped/already read */
>
> OPP ? .. maybe drop this comment that was cut/pasted from somewhere
> else :D
>
> > + msg->index = cpu_to_le32(desc_index); }
> > +
> > +static int iter_pinctrl_assoc_update_state(struct scmi_iterator_state *st,
> > + const void *response, void *priv)
> {
> > + const struct scmi_resp_pinctrl_list_assoc *r = response;
> > +
> > + st->num_returned = RETURNED(r->flags);
> > + st->num_remaining = REMAINING(r->flags);
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +iter_pinctrl_assoc_process_response(const struct scmi_protocol_handle
> *ph,
> > + const void *response,
> > + struct scmi_iterator_state *st, void *priv)
> {
> > + const struct scmi_resp_pinctrl_list_assoc *r = response;
> > + struct scmi_pinctrl_ipriv *p = priv;
> > +
> > + p->array[st->desc_index + st->loop_idx] =
> > + le16_to_cpu(r->array[st->loop_idx]);
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_list_associations(const struct scmi_protocol_handle
> *ph,
> > + u32 selector,
> > + enum scmi_pinctrl_selector_type
> type,
> > + u16 size, u32 *array)
> > +{
> > + int ret;
> > + void *iter;
> > + struct scmi_iterator_ops ops = {
> > + .prepare_message = iter_pinctrl_assoc_prepare_message,
> > + .update_state = iter_pinctrl_assoc_update_state,
> > + .process_response = iter_pinctrl_assoc_process_response,
> > + };
> > + struct scmi_pinctrl_ipriv ipriv = {
> > + .selector = selector,
> > + .type = type,
> > + .array = array,
> > + };
> > +
> > + if (!array || !size || type == PIN_TYPE)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_validate_id(ph, selector, type);
> > + if (ret)
> > + return ret;
> > +
> > + iter = ph->hops->iter_response_init(ph, &ops, size,
> > + PINCTRL_LIST_ASSOCIATIONS,
> > + sizeof(struct
> scmi_msg_pinctrl_list_assoc),
> > + &ipriv);
> > +
> > + if (IS_ERR(iter))
> > + return PTR_ERR(iter);
> > +
> > + return ph->hops->iter_response_run(iter);
> > +}
> > +
> > +struct scmi_settings_get_ipriv {
> > + u32 selector;
> > + enum scmi_pinctrl_selector_type type;
> > + u32 flag;
> > + enum scmi_pinctrl_conf_type *config_types;
> > + u32 *config_values;
> > +};
> > +
> > +static void
> > +iter_pinctrl_settings_get_prepare_message(void *message, u32 desc_index,
> > + const void *priv)
> > +{
> > + struct scmi_msg_settings_get *msg = message;
> > + const struct scmi_settings_get_ipriv *p = priv;
> > + u32 attributes;
> > +
> > + attributes = FIELD_PREP(CONFIG_FLAG_MASK, p->flag) |
> > + FIELD_PREP(SELECTOR_MASK, p->type);
> > +
> > + if (p->flag == 1)
>
> A boolean like .get_all would be more clear..see down below why you dont
> need a flag 0|1|2
>
> > + attributes |= FIELD_PREP(SKIP_CONFIGS_MASK, desc_index);
> > + else if (!p->flag)
> > + attributes |= FIELD_PREP(CONFIG_TYPE_MASK, p-
> >config_types[0]);
> > +
> > + msg->attributes = cpu_to_le32(attributes);
> > + msg->identifier = cpu_to_le32(p->selector); }
> > +
> > +static int
> > +iter_pinctrl_settings_get_update_state(struct scmi_iterator_state *st,
> > + const void *response, void *priv) {
> > + const struct scmi_resp_settings_get *r = response;
> > + struct scmi_settings_get_ipriv *p = priv;
> > +
> > + if (p->flag == 1) {
>
> Ditto... see below the explanation
>
> > + st->num_returned = le32_get_bits(r->num_configs,
> GENMASK(7, 0));
> > + st->num_remaining = le32_get_bits(r->num_configs,
> > + GENMASK(31, 24));
> > + } else {
> > + st->num_returned = 1;
> > + st->num_remaining = 0;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +iter_pinctrl_settings_get_process_response(const struct
> scmi_protocol_handle *ph,
> > + const void *response,
> > + struct scmi_iterator_state *st,
> > + void *priv)
> > +{
> > + const struct scmi_resp_settings_get *r = response;
> > + struct scmi_settings_get_ipriv *p = priv;
> > +
> > + if (!p->flag) {
> > + if (p->config_types[0] !=
> > + le32_get_bits(r->configs[st->loop_idx * 2], GENMASK(7, 0)))
> > + return -EINVAL;
> > + } else if (p->flag == 1) {
> > + p->config_types[st->desc_index + st->loop_idx] =
> > + le32_get_bits(r->configs[st->loop_idx * 2],
> > + GENMASK(7, 0));
> > + } else if (p->flag == 2) {
> > + return 0;
> > + }
>
> Unneeded...see down below for explanation
>
> > +
> > + p->config_values[st->desc_index + st->loop_idx] =
> > + le32_to_cpu(r->configs[st->loop_idx * 2 + 1]);
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +scmi_pinctrl_settings_get(const struct scmi_protocol_handle *ph, u32
> selector,
> > + enum scmi_pinctrl_selector_type type,
> > + enum scmi_pinctrl_conf_type config_type,
> > + u32 *config_value)
> > +{
> > + int ret;
> > + void *iter;
> > + struct scmi_iterator_ops ops = {
> > + .prepare_message =
> iter_pinctrl_settings_get_prepare_message,
> > + .update_state = iter_pinctrl_settings_get_update_state,
> > + .process_response =
> iter_pinctrl_settings_get_process_response,
> > + };
> > + struct scmi_settings_get_ipriv ipriv = {
> > + .selector = selector,
> > + .type = type,
> > + .flag = 0,
> > + .config_types = &config_type,
> > + .config_values = config_value,
> > + };
> > +
>
> So this function is used to retrieve configs; as of now, just one, then it could
> be extended to fetch all the configs, and for this it uses the iterators helpers,
> BUT it is not and will not be used to just fetch the selected_function with
> flag_2 (even though is always provided), since in that case you wont get back
> a multi-part SCMI response and so there is no need to use iterators...
>
> IOW... no need here to handle flag_2 scenario and as a consequence I would
> change the ipriv flag to be be a boolean .get_all, like it was, since it is more
> readable (and so you wont need to add any comment..)
ok, so your suggestion is drop the iterators, and only support one config,
right?
Or keep iterators with get_all be passed as a function parameter?
>
> In the future could make sense to add here also a *selected_function output
> param to this function since you will always get it back for free when
> retrieving configs ... BUT for now is just not needed really...no users for this
> case till now...
>
> ...when the time will come that we will need a function_selected_get to be
> issued without retrieveing also the configs I would add a distinct routine that
> crafts properly a SETTINGS_GET with flag_2 without worrying about multi-
> part responses (and with no need for iterators support)
>
> Trying to handle all in here just complicates stuff...
>
> > + if (!config_value || type == FUNCTION_TYPE)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_validate_id(ph, selector, type);
> > + if (ret)
> > + return ret;
> > +
> > + iter = ph->hops->iter_response_init(ph, &ops, 1,
> PINCTRL_SETTINGS_GET,
> > + sizeof(struct
> scmi_msg_settings_get),
> > + &ipriv);
> > +
> > + if (IS_ERR(iter))
> > + return PTR_ERR(iter);
> > +
> > + return ph->hops->iter_response_run(iter);
> > +}
> > +
> > +static int
> > +scmi_pinctrl_settings_conf(const struct scmi_protocol_handle *ph,
> > + u32 selector,
> > + enum scmi_pinctrl_selector_type type,
> > + u32 nr_configs,
> > + enum scmi_pinctrl_conf_type *config_type,
> > + u32 *config_value)
> > +{
> > + struct scmi_xfer *t;
> > + struct scmi_msg_settings_conf *tx;
> > + u32 attributes;
> > + int ret, i;
> > + u32 configs_in_chunk, conf_num = 0;
> > + u32 chunk;
> > + int max_msg_size = ph->hops->get_max_msg_size(ph);
> > +
> > + if (!config_type || !config_value || type == FUNCTION_TYPE)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_validate_id(ph, selector, type);
> > + if (ret)
> > + return ret;
> > +
> > + configs_in_chunk = (max_msg_size - sizeof(*tx)) / (sizeof(__le32) * 2);
> > + while (conf_num < nr_configs) {
> > + chunk = (nr_configs - conf_num > configs_in_chunk) ?
> > + configs_in_chunk : nr_configs - conf_num;
> > +
> > + ret = ph->xops->xfer_get_init(ph,
> PINCTRL_SETTINGS_CONFIGURE,
> > + sizeof(*tx) +
> > + chunk * 2 * sizeof(__le32),
> > + 0, &t);
> > + if (ret)
> > + return ret;
> for consistency I would
> break;
>
> like below and you will exit always from the last return ret;
>
> > +
> > + tx = t->tx.buf;
> > + tx->identifier = cpu_to_le32(selector);
> > + attributes = FIELD_PREP(GENMASK(1, 0), type) |
> > + FIELD_PREP(GENMASK(9, 2), chunk);
> > + tx->attributes = cpu_to_le32(attributes);
> > +
> > + for (i = 0; i < chunk; i++) {
> > + tx->configs[i * 2] =
> > + cpu_to_le32(config_type[conf_num + i]);
> > + tx->configs[i * 2 + 1] =
> > + cpu_to_le32(config_value[conf_num + i]);
> > + }
> > +
> > + ret = ph->xops->do_xfer(ph, t);
> > +
> > + ph->xops->xfer_put(ph, t);
> > +
> > + if (ret)
> > + break;
> > +
> > + conf_num += chunk;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int scmi_pinctrl_function_select(const struct scmi_protocol_handle
> *ph,
> > + u32 group,
> > + enum scmi_pinctrl_selector_type
> type,
> > + u32 function_id)
> > +{
> > + int ret;
> > + struct scmi_xfer *t;
> > + struct scmi_msg_settings_conf *tx;
> > + u32 attributes;
> > +
> > + ret = scmi_pinctrl_validate_id(ph, group, type);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ph->xops->xfer_get_init(ph, PINCTRL_SETTINGS_CONFIGURE,
> > + sizeof(*tx), 0, &t);
> > + if (ret)
> > + return ret;
> > +
> > + tx = t->tx.buf;
> > + tx->identifier = cpu_to_le32(group);
> > + tx->function_id = cpu_to_le32(function_id);
> > + attributes = FIELD_PREP(GENMASK(1, 0), type) | BIT(10);
> > + tx->attributes = cpu_to_le32(attributes);
> > +
> > + ret = ph->xops->do_xfer(ph, t);
> > + ph->xops->xfer_put(ph, t);
> > +
> > + return ret;
> > +}
> > +
> > +static int scmi_pinctrl_request(const struct scmi_protocol_handle *ph,
> > + u32 identifier,
> > + enum scmi_pinctrl_selector_type type) {
> > + int ret;
> > + struct scmi_xfer *t;
> > + struct scmi_msg_request *tx;
> > +
> > + if (type == FUNCTION_TYPE)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_validate_id(ph, identifier, type);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ph->xops->xfer_get_init(ph, PINCTRL_REQUEST, sizeof(*tx), 0,
> &t);
> > + if (ret)
> > + return ret;
> > +
> > + tx = t->tx.buf;
> > + tx->identifier = cpu_to_le32(identifier);
> > + tx->flags = cpu_to_le32(type);
> > +
> > + ret = ph->xops->do_xfer(ph, t);
> > + ph->xops->xfer_put(ph, t);
> > +
> > + return ret;
> > +}
> > +
>
> ..this function ...
>
> > +static int scmi_pinctrl_pin_request(const struct scmi_protocol_handle *ph,
> > + u32 pin)
> > +{
> > + return scmi_pinctrl_request(ph, pin, PIN_TYPE); }
> > +
> > +static int scmi_pinctrl_free(const struct scmi_protocol_handle *ph,
> > + u32 identifier,
> > + enum scmi_pinctrl_selector_type type) {
> > + int ret;
> > + struct scmi_xfer *t;
> > + struct scmi_msg_request *tx;
> > +
> > + if (type == FUNCTION_TYPE)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_validate_id(ph, identifier, type);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ph->xops->xfer_get_init(ph, PINCTRL_RELEASE, sizeof(*tx), 0, &t);
> > + if (ret)
> > + return ret;
> > +
> > + tx = t->tx.buf;
> > + tx->identifier = cpu_to_le32(identifier);
> > + tx->flags = cpu_to_le32(type);
> > +
> > + ret = ph->xops->do_xfer(ph, t);
> > + ph->xops->xfer_put(ph, t);
> > +
> > + return ret;
> > +}
> > +
>
> ...and this are completely identical, beside the used command msg_id...please
> make it a common workhorse function by adding a param for the command...
>
> > +static int scmi_pinctrl_pin_free(const struct scmi_protocol_handle
> > +*ph, u32 pin) {
> > + return scmi_pinctrl_free(ph, pin, PIN_TYPE); }
> > +
>
> ...and convert these _request/_free functions into a pair odf simple wrapper
> invoking the common workhorse...
>
> > +static int scmi_pinctrl_get_group_info(const struct scmi_protocol_handle
> *ph,
> > + u32 selector,
> > + struct scmi_group_info *group) {
> > + int ret;
> > +
> > + if (!group)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_attributes(ph, GROUP_TYPE, selector,
> > + group->name,
> > + &group->nr_pins);
> > + if (ret)
> > + return ret;
> > +
> > + if (!group->nr_pins) {
> > + dev_err(ph->dev, "Group %d has 0 elements", selector);
> > + return -ENODATA;
> > + }
> > +
> > + group->group_pins = kmalloc_array(group->nr_pins,
> > + sizeof(*group->group_pins),
> > + GFP_KERNEL);
> > + if (!group->group_pins)
> > + return -ENOMEM;
> > +
> > + ret = scmi_pinctrl_list_associations(ph, selector, GROUP_TYPE,
> > + group->nr_pins, group-
> >group_pins);
> > + if (ret) {
> > + kfree(group->group_pins);
> > + return ret;
> > + }
> > +
> > + group->present = true;
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_get_group_name(const struct scmi_protocol_handle
> *ph,
> > + u32 selector, const char **name) {
> > + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> > +
> > + if (!name)
> > + return -EINVAL;
> > +
> > + if (selector >= pi->nr_groups)
> > + return -EINVAL;
> > +
> > + if (!pi->groups[selector].present) {
> > + int ret;
> > +
> > + ret = scmi_pinctrl_get_group_info(ph, selector,
> > + &pi->groups[selector]);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + *name = pi->groups[selector].name;
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_group_pins_get(const struct scmi_protocol_handle
> *ph,
> > + u32 selector, const u32 **pins,
> > + u32 *nr_pins)
> > +{
> > + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> > +
> > + if (!pins || !nr_pins)
> > + return -EINVAL;
> > +
> > + if (selector >= pi->nr_groups)
> > + return -EINVAL;
> > +
> > + if (!pi->groups[selector].present) {
> > + int ret;
> > +
> > + ret = scmi_pinctrl_get_group_info(ph, selector,
> > + &pi->groups[selector]);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + *pins = pi->groups[selector].group_pins;
> > + *nr_pins = pi->groups[selector].nr_pins;
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_get_function_info(const struct
> scmi_protocol_handle *ph,
> > + u32 selector,
> > + struct scmi_function_info *func) {
> > + int ret;
> > +
> > + if (!func)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_attributes(ph, FUNCTION_TYPE, selector,
> > + func->name,
> > + &func->nr_groups);
> > + if (ret)
> > + return ret;
> > +
> > + if (!func->nr_groups) {
> > + dev_err(ph->dev, "Function %d has 0 elements", selector);
> > + return -ENODATA;
> > + }
> > +
> > + func->groups = kmalloc_array(func->nr_groups, sizeof(*func->groups),
> > + GFP_KERNEL);
> > + if (!func->groups)
> > + return -ENOMEM;
> > +
> > + ret = scmi_pinctrl_list_associations(ph, selector, FUNCTION_TYPE,
> > + func->nr_groups, func->groups);
> > + if (ret) {
> > + kfree(func->groups);
> > + return ret;
> > + }
> > +
> > + func->present = true;
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_get_function_name(const struct
> scmi_protocol_handle *ph,
> > + u32 selector, const char **name) {
> > + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> > +
> > + if (!name)
> > + return -EINVAL;
> > +
> > + if (selector >= pi->nr_functions)
> > + return -EINVAL;
> > +
> > + if (!pi->functions[selector].present) {
> > + int ret;
> > +
> > + ret = scmi_pinctrl_get_function_info(ph, selector,
> > + &pi-
> >functions[selector]);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + *name = pi->functions[selector].name;
> > + return 0;
> > +}
> > +
> > +static int
> > +scmi_pinctrl_function_groups_get(const struct scmi_protocol_handle *ph,
> > + u32 selector, u32 *nr_groups,
> > + const u32 **groups)
> > +{
> > + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> > +
> > + if (!groups || !nr_groups)
> > + return -EINVAL;
> > +
> > + if (selector >= pi->nr_functions)
> > + return -EINVAL;
> > +
> > + if (!pi->functions[selector].present) {
> > + int ret;
> > +
> > + ret = scmi_pinctrl_get_function_info(ph, selector,
> > + &pi-
> >functions[selector]);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + *groups = pi->functions[selector].groups;
> > + *nr_groups = pi->functions[selector].nr_groups;
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_mux_set(const struct scmi_protocol_handle *ph,
> > + u32 selector, u32 group)
> > +{
> > + return scmi_pinctrl_function_select(ph, group, GROUP_TYPE,
> > +selector); }
> > +
> > +static int scmi_pinctrl_get_pin_info(const struct scmi_protocol_handle *ph,
> > + u32 selector, struct scmi_pin_info *pin) {
> > + int ret;
> > +
> > + if (!pin)
> > + return -EINVAL;
> > +
> > + ret = scmi_pinctrl_attributes(ph, PIN_TYPE, selector,
> > + pin->name, NULL);
> > + if (ret)
> > + return ret;
> > +
> > + pin->present = true;
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_get_pin_name(const struct scmi_protocol_handle
> *ph,
> > + u32 selector, const char **name) {
> > + struct scmi_pinctrl_info *pi = ph->get_priv(ph);
> > +
> > + if (!name)
> > + return -EINVAL;
> > +
> > + if (selector >= pi->nr_pins)
> > + return -EINVAL;
> > +
> > + if (!pi->pins[selector].present) {
> > + int ret;
> > +
> > + ret = scmi_pinctrl_get_pin_info(ph, selector,
> > + &pi->pins[selector]);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + *name = pi->pins[selector].name;
> > +
> > + return 0;
> > +}
> > +
> > +static int scmi_pinctrl_name_get(const struct scmi_protocol_handle *ph,
> > + u32 selector,
> > + enum scmi_pinctrl_selector_type type,
> > + const char **name)
> > +{
> > + switch (type) {
> > + case PIN_TYPE:
> > + return scmi_pinctrl_get_pin_name(ph, selector, name);
> > + case GROUP_TYPE:
> > + return scmi_pinctrl_get_group_name(ph, selector, name);
> > + case FUNCTION_TYPE:
> > + return scmi_pinctrl_get_function_name(ph, selector, name);
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static const struct scmi_pinctrl_proto_ops pinctrl_proto_ops = {
> > + .count_get = scmi_pinctrl_count_get,
> > + .name_get = scmi_pinctrl_name_get,
> > + .group_pins_get = scmi_pinctrl_group_pins_get,
> > + .function_groups_get = scmi_pinctrl_function_groups_get,
> > + .mux_set = scmi_pinctrl_mux_set,
> > + .settings_get = scmi_pinctrl_settings_get,
> > + .settings_conf = scmi_pinctrl_settings_conf,
> > + .pin_request = scmi_pinctrl_pin_request,
> > + .pin_free = scmi_pinctrl_pin_free,
> > +};
> > +
> > +static int scmi_pinctrl_protocol_init(const struct
> > +scmi_protocol_handle *ph) {
> > + int ret;
> > + u32 version;
> > + struct scmi_pinctrl_info *pinfo;
> > +
> > + ret = ph->xops->version_get(ph, &version);
> > + if (ret)
> > + return ret;
> > +
> > + dev_dbg(ph->dev, "Pinctrl Version %d.%d\n",
> > + PROTOCOL_REV_MAJOR(version),
> PROTOCOL_REV_MINOR(version));
> > +
> > + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
> > + if (!pinfo)
> > + return -ENOMEM;
> > +
> > + ret = scmi_pinctrl_attributes_get(ph, pinfo);
> > + if (ret)
> > + return ret;
>
> ..as a I was saying is nr_pins == 0 the scmi_pinctrl_attributes_get could return
> -EINVAL here and bail out....not sure that a running setup with zero pins has
> any values (even for testing...) BUT, as said above, I wuld certainly add a
> dev_warn in scmi_pinctrl_attributes_get() when nr_pins == 0
Fix it in v7.
Thanks,
Peng.
>
> Thanks,
> Cristian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH v5 4/4] clk: imx: add i.MX95 BLK CTL clk driver
From: Peng Fan @ 2024-03-31 12:01 UTC (permalink / raw)
To: Krzysztof Kozlowski, Peng Fan (OSS), Michael Turquette,
Stephen Boyd, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Abel Vesa
Cc: linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <d3770f5e-f3cc-40fd-a211-b229be46d974@linaro.org>
> Subject: Re: [PATCH v5 4/4] clk: imx: add i.MX95 BLK CTL clk driver
>
> On 24/03/2024 08:52, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
>
> ...
>
> > +
> > +static const struct of_device_id imx95_bc_of_match[] = {
> > + { .compatible = "nxp,imx95-camera-csr", .data = &camblk_dev_data },
> > + { .compatible = "nxp,imx95-display-master-csr", },
> > + { .compatible = "nxp,imx95-lvds-csr", .data = &lvds_csr_dev_data },
> > + { .compatible = "nxp,imx95-display-csr", .data =
> &dispmix_csr_dev_data },
> > + { .compatible = "nxp,imx95-vpu-csr", .data = &vpublk_dev_data },
> > + { /* Sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, imx95_bc_of_match);
> > +
> > +static struct platform_driver imx95_bc_driver = {
> > + .probe = imx95_bc_probe,
> > + .driver = {
> > + .name = "imx95-blk-ctl",
> > + .of_match_table = of_match_ptr(imx95_bc_of_match),
>
> Drop of_match_ptr(), causes warnings. From where did you copy such code?
> Which mainline driver has such pattern?
I recall that when COMPILE_TEST is selected, OF is not selected, kernel
robot reports warning. This may not be true now.
I could drop it in v6.
Thanks,
Peng.
>
> Best regards,
> Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH v5 0/4] Add support i.MX95 BLK CTL module clock features
From: Peng Fan @ 2024-03-31 12:00 UTC (permalink / raw)
To: Krzysztof Kozlowski, Peng Fan (OSS), Michael Turquette,
Stephen Boyd, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Abel Vesa
Cc: linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <ce1b814a-6b1b-4773-ad29-b572d00f56c9@linaro.org>
> Subject: Re: [PATCH v5 0/4] Add support i.MX95 BLK CTL module clock
> features
>
> On 24/03/2024 08:51, Peng Fan (OSS) wrote:
> > i.MX95's several MIXes has BLK CTL module which could be used for clk
> > settings, QoS settings, Misc settings for a MIX. This patchset is to
> > add the clk feature support, including dt-bindings
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> > Changes in v5:
> > - Merge bindings except the one has mux-controller
> > - Separate clock ID headers in a separate patch per Rob's comments
>
> Where did he suggest it?
See https://lore.kernel.org/all/20240315165422.GA1472059-robh@kernel.org/
Thanks,
Peng.
>
>
>
> Best regards,
> Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH v5 1/4] dt-bindings: clock: support i.MX95 BLK CTL module
From: Peng Fan @ 2024-03-31 11:58 UTC (permalink / raw)
To: Krzysztof Kozlowski, Peng Fan (OSS), Michael Turquette,
Stephen Boyd, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Abel Vesa
Cc: linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <1709e7df-268b-4da6-a75d-9d7ce80d9a41@linaro.org>
> Subject: Re: [PATCH v5 1/4] dt-bindings: clock: support i.MX95 BLK CTL
> module
>
> On 24/03/2024 08:52, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > i.MX95 includes BLK CTL module in several MIXes, such as VPU_CSR in
> > VPUMIX, CAMERA_CSR in CAMERAMIX and etc.
> >
> > The BLK CTL module is used for various settings of a specific MIX,
> > such as clock, QoS and etc.
> >
> > This patch is to add some BLK CTL modules that has clock features.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> > .../bindings/clock/nxp,imx95-blk-ctl.yaml | 56
> ++++++++++++++++++++++
> > 1 file changed, 56 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/clock/nxp,imx95-blk-ctl.yaml
> > b/Documentation/devicetree/bindings/clock/nxp,imx95-blk-ctl.yaml
> > new file mode 100644
> > index 000000000000..2dffc02dcd8b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/nxp,imx95-blk-ctl.yaml
> > @@ -0,0 +1,56 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > +cetree.org%2Fschemas%2Fclock%2Fnxp%2Cimx95-blk-
> ctl.yaml%23&data=05%7C
> >
> +02%7Cpeng.fan%40nxp.com%7Cd713a861f155495c922a08dc4d01346d%7
> C686ea1d3
> >
> +bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638469914764776121%7CUnk
> nown%7CTWF
> >
> +pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> VCI6
> >
> +Mn0%3D%7C0%7C%7C%7C&sdata=rW7%2BGedk3bloLsAqBIkMlXQNjDmRd
> Z0cHacQtKxjc
> > +mQ%3D&reserved=0
> > +$schema:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> > +cetree.org%2Fmeta-
> schemas%2Fcore.yaml%23&data=05%7C02%7Cpeng.fan%40nx
> >
> +p.com%7Cd713a861f155495c922a08dc4d01346d%7C686ea1d3bc2b4c6fa9
> 2cd99c5c
> >
> +301635%7C0%7C0%7C638469914764787067%7CUnknown%7CTWFpbGZs
> b3d8eyJWIjoiM
> >
> +C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7
> C%7C%7
> >
> +C&sdata=tFzM3%2BxuQVsit9lCEnNz8kYnZjT%2FXj%2Fdqzk9DB9oy1c%3D&r
> eserved
> > +=0
> > +
> > +title: NXP i.MX95 Block Control
> > +
> > +maintainers:
> > + - Peng Fan <peng.fan@nxp.com>
> > +
> > +properties:
> > + compatible:
> > + items:
> > + - enum:
> > + - nxp,imx95-lvds-csr
> > + - nxp,imx95-display-csr
> > + - nxp,imx95-camera-csr
> > + - nxp,imx95-vpu-csr
> > + - const: syscon
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + power-domains:
> > + maxItems: 1
> > +
> > + clocks:
> > + maxItems: 1
> > +
> > + '#clock-cells':
> > + const: 1
> > + description:
> > + The clock consumer should specify the desired clock by having the
> clock
> > + ID in its "clocks" phandle cell. See
> > + include/dt-bindings/clock/nxp,imx95-clock.h
>
> In such case, put header as your first patch in the patchset. I don't understand
> why it was split in the first place...
Rob gave a comment in v4, so I split the headers.
"
If this number can change, then it is not ABI and doesn't go in this
header. With that dropped,
"
https://lore.kernel.org/all/20240315165422.GA1472059-robh@kernel.org/
Thanks,
Peng.
>
>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> Best regards,
> Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 01/22] virtio: store owner from modules with register_virtio_driver()
From: Michael S. Tsirkin @ 2024-03-31 11:20 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: virtualization, linux-doc, linux-kernel, linux-um, linux-block,
linux-bluetooth, linux-crypto, linux-arm-kernel, linux-gpio,
dri-devel, iommu, netdev, v9fs, kvm, linux-wireless, nvdimm,
linux-remoteproc, linux-scsi, linux-fsdevel, alsa-devel,
linux-sound
In-Reply-To: <20240327-module-owner-virtio-v1-1-0feffab77d99@linaro.org>
On Wed, Mar 27, 2024 at 01:40:54PM +0100, Krzysztof Kozlowski wrote:
> Modules registering driver with register_virtio_driver() might forget to
> set .owner field. i2c-virtio.c for example has it missing. The field
> is used by some of other kernel parts for reference counting
> (try_module_get()), so it is expected that drivers will set it.
>
> Solve the problem by moving this task away from the drivers to the core
> amba bus code, just like we did for platform_driver in
> commit 9447057eaff8 ("platform_device: use a macro instead of
> platform_driver_register").
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
This makes sense. So this will be:
Fixes: 3cfc88380413 ("i2c: virtio: add a virtio i2c frontend driver")
Cc: "Jie Deng" <jie.deng@intel.com>
and I think I will pick this patch for this cycle to fix
the bug. The cleanups can go in the next cycle.
> ---
> Documentation/driver-api/virtio/writing_virtio_drivers.rst | 1 -
> drivers/virtio/virtio.c | 6 ++++--
> include/linux/virtio.h | 7 +++++--
> 3 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/driver-api/virtio/writing_virtio_drivers.rst b/Documentation/driver-api/virtio/writing_virtio_drivers.rst
> index e14c58796d25..e5de6f5d061a 100644
> --- a/Documentation/driver-api/virtio/writing_virtio_drivers.rst
> +++ b/Documentation/driver-api/virtio/writing_virtio_drivers.rst
> @@ -97,7 +97,6 @@ like this::
>
> static struct virtio_driver virtio_dummy_driver = {
> .driver.name = KBUILD_MODNAME,
> - .driver.owner = THIS_MODULE,
> .id_table = id_table,
> .probe = virtio_dummy_probe,
> .remove = virtio_dummy_remove,
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index f173587893cb..9510c551dce8 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -362,14 +362,16 @@ static const struct bus_type virtio_bus = {
> .remove = virtio_dev_remove,
> };
>
> -int register_virtio_driver(struct virtio_driver *driver)
> +int __register_virtio_driver(struct virtio_driver *driver, struct module *owner)
> {
> /* Catch this early. */
> BUG_ON(driver->feature_table_size && !driver->feature_table);
> driver->driver.bus = &virtio_bus;
> + driver->driver.owner = owner;
> +
> return driver_register(&driver->driver);
> }
> -EXPORT_SYMBOL_GPL(register_virtio_driver);
> +EXPORT_SYMBOL_GPL(__register_virtio_driver);
>
> void unregister_virtio_driver(struct virtio_driver *driver)
> {
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index b0201747a263..26c4325aa373 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -170,7 +170,7 @@ size_t virtio_max_dma_size(const struct virtio_device *vdev);
>
> /**
> * struct virtio_driver - operations for a virtio I/O driver
> - * @driver: underlying device driver (populate name and owner).
> + * @driver: underlying device driver (populate name).
> * @id_table: the ids serviced by this driver.
> * @feature_table: an array of feature numbers supported by this driver.
> * @feature_table_size: number of entries in the feature table array.
> @@ -208,7 +208,10 @@ static inline struct virtio_driver *drv_to_virtio(struct device_driver *drv)
> return container_of(drv, struct virtio_driver, driver);
> }
>
> -int register_virtio_driver(struct virtio_driver *drv);
> +/* use a macro to avoid include chaining to get THIS_MODULE */
> +#define register_virtio_driver(drv) \
> + __register_virtio_driver(drv, THIS_MODULE)
> +int __register_virtio_driver(struct virtio_driver *drv, struct module *owner);
> void unregister_virtio_driver(struct virtio_driver *drv);
>
> /* module_virtio_driver() - Helper macro for drivers that don't do
>
> --
> 2.34.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6 2/5] KVM: arm64: Add newly allocated ID registers to register descriptions
From: Marc Zyngier @ 2024-03-31 10:59 UTC (permalink / raw)
To: Mark Brown
Cc: Catalin Marinas, Will Deacon, Oliver Upton, James Morse,
Suzuki K Poulose, Jonathan Corbet, Shuah Khan, linux-arm-kernel,
linux-kernel, Dave Martin, kvmarm, linux-doc, linux-kselftest
In-Reply-To: <20240329-arm64-2023-dpisa-v6-2-ba42db6c27f3@kernel.org>
On Fri, 29 Mar 2024 00:13:43 +0000,
Mark Brown <broonie@kernel.org> wrote:
>
> The 2023 architecture extensions have allocated some new ID registers, add
> them to the KVM system register descriptions so that they are visible to
> guests.
>
> We make the newly introduced dpISA features writeable, as well as
> allowing writes to ID_AA64ISAR3_EL1.CPA for FEAT_CPA which only
> introduces straigforward new instructions with no additional
> architectural state or traps.
FPMR actively gets trapped by HCRX_EL2.
>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
> arch/arm64/kvm/sys_regs.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index c9f4f387155f..a3c20d1a36aa 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2293,12 +2293,15 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> ID_AA64PFR0_EL1_AdvSIMD |
> ID_AA64PFR0_EL1_FP), },
> ID_SANITISED(ID_AA64PFR1_EL1),
> - ID_UNALLOCATED(4,2),
> + ID_WRITABLE(ID_AA64PFR2_EL1, ~(ID_AA64PFR2_EL1_RES0 |
> + ID_AA64PFR2_EL1_MTEFAR |
> + ID_AA64PFR2_EL1_MTESTOREONLY |
> + ID_AA64PFR2_EL1_MTEPERM)),
> ID_UNALLOCATED(4,3),
> ID_WRITABLE(ID_AA64ZFR0_EL1, ~ID_AA64ZFR0_EL1_RES0),
> ID_HIDDEN(ID_AA64SMFR0_EL1),
> ID_UNALLOCATED(4,6),
> - ID_UNALLOCATED(4,7),
> + ID_WRITABLE(ID_AA64FPFR0_EL1, ~ID_AA64FPFR0_EL1_RES0),
>
> /* CRm=5 */
> { SYS_DESC(SYS_ID_AA64DFR0_EL1),
> @@ -2325,7 +2328,9 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> ID_WRITABLE(ID_AA64ISAR2_EL1, ~(ID_AA64ISAR2_EL1_RES0 |
> ID_AA64ISAR2_EL1_APA3 |
> ID_AA64ISAR2_EL1_GPA3)),
> - ID_UNALLOCATED(6,3),
> + ID_WRITABLE(ID_AA64ISAR3_EL1, ~(ID_AA64ISAR2_EL1_RES0 |
> + ID_AA64ISAR3_EL1_PACM |
> + ID_AA64ISAR3_EL1_TLBIW)),
> ID_UNALLOCATED(6,4),
> ID_UNALLOCATED(6,5),
> ID_UNALLOCATED(6,6),
>
Where is the code that enforces the lack of support for MTEFAR,
MTESTOREONLY, and MTEPERM for SCTLR_ELx, EnPACM and EnFPM in HCRX_EL2?
And I haven't checked whether TLBI VMALLWS2 can be trapped.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/2] arm64: dts: ti: k3-am62p5-sk: minor whitespace cleanup
From: Krzysztof Kozlowski @ 2024-03-31 10:26 UTC (permalink / raw)
To: Benoît Cousson, Tony Lindgren, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Nishanth Menon,
Vignesh Raghavendra, Tero Kristo, linux-omap, devicetree,
linux-kernel, linux-arm-kernel, Krzysztof Kozlowski
In-Reply-To: <20240208105146.128645-1-krzysztof.kozlowski@linaro.org>
On Thu, 08 Feb 2024 11:51:45 +0100, Krzysztof Kozlowski wrote:
> The DTS code coding style expects exactly one space before '{'
> character.
>
>
This is waiting on the lists for almost two months, so I just picked it up. Let
me know if anyone prefers to take it instead.
Applied, thanks!
[1/2] arm64: dts: ti: k3-am62p5-sk: minor whitespace cleanup
https://git.kernel.org/krzk/linux-dt/c/9d0ee097b3e5873e4e98770b94f11481f485e7c9
[2/2] ARM: dts: ti: omap: minor whitespace cleanup
https://git.kernel.org/krzk/linux-dt/c/021bc7094e8c8ac1380527d3f53561c9a234a190
Best regards,
--
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] clk: samsung: exynosautov9: fix wrong pll clock id value
From: Krzysztof Kozlowski @ 2024-03-31 10:19 UTC (permalink / raw)
To: Sylwester Nawrocki, Chanwoo Choi, Alim Akhtar, Michael Turquette,
Stephen Boyd, Jaewon Kim
Cc: linux-samsung-soc, linux-clk, linux-arm-kernel, linux-kernel
In-Reply-To: <20240328091000.17660-1-jaewon02.kim@samsung.com>
On Thu, 28 Mar 2024 18:10:00 +0900, Jaewon Kim wrote:
> All PLL id values of CMU_TOP were incorrectly set to FOUT_SHARED0_PLL.
> It modified to the correct PLL clock id value.
>
>
Applied, thanks!
[1/1] clk: samsung: exynosautov9: fix wrong pll clock id value
https://git.kernel.org/krzk/linux/c/04ee3a0b44e3d18cf6b0c712d14b98624877fd26
Best regards,
--
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6 1/5] KVM: arm64: Share all userspace hardened thread data with the hypervisor
From: Marc Zyngier @ 2024-03-31 10:00 UTC (permalink / raw)
To: Mark Brown
Cc: Catalin Marinas, Will Deacon, Oliver Upton, James Morse,
Suzuki K Poulose, Jonathan Corbet, Shuah Khan, linux-arm-kernel,
linux-kernel, Dave Martin, kvmarm, linux-doc, linux-kselftest
In-Reply-To: <20240329-arm64-2023-dpisa-v6-1-ba42db6c27f3@kernel.org>
On Fri, 29 Mar 2024 00:13:42 +0000,
Mark Brown <broonie@kernel.org> wrote:
>
> As part of the lazy FPSIMD state transitioning done by the hypervisor we
> currently share the userpsace FPSIMD state in thread->uw.fpsimd_state with
> the host. Since this struct is non-extensible userspace ABI we have to keep
Using the same representation is just pure convenience, and nothing
requires us to use the it in the kernel/hypervisor.
> the definition as is but the addition of FPMR in the 2023 dpISA means that
> we will want to share more storage with the host. To facilitate this
> refactor the current code to share the entire thread->uw rather than just
> the one field.
So this increase the required sharing with EL2 from 528 bytes to
560. Not a huge deal, but definitely moving in the wrong direction. Is
there any plans to add more stuff to this structure that wouldn't be
*directly* relevant to the hypervisor?
>
> The large number of references to fpsimd_state make it very inconvenient
> to add an additional wrapper struct.
>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
> arch/arm64/include/asm/kvm_host.h | 3 ++-
> arch/arm64/include/asm/processor.h | 2 +-
> arch/arm64/kvm/fpsimd.c | 13 ++++++-------
> arch/arm64/kvm/hyp/include/hyp/switch.h | 2 +-
> arch/arm64/kvm/hyp/nvhe/hyp-main.c | 4 ++--
> 5 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 9e8a496fb284..8a251f0da900 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -27,6 +27,7 @@
> #include <asm/fpsimd.h>
> #include <asm/kvm.h>
> #include <asm/kvm_asm.h>
> +#include <asm/processor.h>
> #include <asm/vncr_mapping.h>
>
> #define __KVM_HAVE_ARCH_INTC_INITIALIZED
> @@ -640,7 +641,7 @@ struct kvm_vcpu_arch {
> struct kvm_guest_debug_arch vcpu_debug_state;
> struct kvm_guest_debug_arch external_debug_state;
>
> - struct user_fpsimd_state *host_fpsimd_state; /* hyp VA */
> + struct thread_struct_uw *host_uw; /* hyp VA */
> struct task_struct *parent_task;
Well, this is going away, and you know it.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 0/2] Set PHY address of MT7531 switch to 0x1f on MediaTek arm64 boards
From: arinc.unal @ 2024-03-31 9:28 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno
Cc: mithat.guner, erkin.bozoglu, devicetree, linux-kernel,
linux-arm-kernel, linux-mediatek
In-Reply-To: <20240314-for-mediatek-mt7531-phy-address-v1-0-52f58db01acd@arinc9.com>
On 14.03.2024 15:20, Arınç ÜNAL via B4 Relay wrote:
> Hello.
>
> This is a small patch series setting the PHY address of MT7531 to 0x1f
> on
> all boards that have the switch.
>
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
> Arınç ÜNAL (2):
> arm64: dts: mediatek: mt7622: set PHY address of MT7531 switch
> to 0x1f
> arm64: dts: mediatek: mt7986: set PHY address of MT7531 switch
> to 0x1f
>
> arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts | 4 ++--
> arch/arm64/boot/dts/mediatek/mt7622-rfb1.dts | 4 ++--
> arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3.dts | 4 ++--
> arch/arm64/boot/dts/mediatek/mt7986a-rfb.dts | 4 ++--
> arch/arm64/boot/dts/mediatek/mt7986b-rfb.dts | 4 ++--
> 5 files changed, 10 insertions(+), 10 deletions(-)
> ---
> base-commit: ba90af39ba57b3fe3ecfdba0c87a80d20c7b788d
> change-id: 20240314-for-mediatek-mt7531-phy-address-9d0b4cfeca21
>
> Best regards,
Reminder that this patch series is waiting.
Arınç
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC PATCH] net: stmmac: Fix the problem about interrupt storm
From: Romain Gantois @ 2024-03-31 8:35 UTC (permalink / raw)
To: Cathy Cai
Cc: cathycai0714, davem, edumazet, kuba, pabeni, mcoquelin.stm32,
netdev, linux-kernel, xuewen.yan94, cixi.geng1, wade.shu,
zhiguo.niu, alexandre.torgue, joabreu, linux-stm32,
linux-arm-kernel
In-Reply-To: <20240327110142.159851-1-cathy.cai@unisoc.com>
Hello Cathy,
On Wed, 27 Mar 2024, Cathy Cai wrote:
> Tx queue time out then reset adapter. When reset the adapter, stmmac driver
> sets the state to STMMAC_DOWN and calls dev_close() function. If an interrupt
> is triggered at this instant after setting state to STMMAC_DOWN, before the
> dev_close() call.
>
...
> - set_bit(STMMAC_DOWN, &priv->state);
> dev_close(priv->dev);
> + set_bit(STMMAC_DOWN, &priv->state);
> dev_open(priv->dev, NULL);
> clear_bit(STMMAC_DOWN, &priv->state);
> clear_bit(STMMAC_RESETING, &priv->state);
If this IRQ issue can happen whenever STMMAC_DOWN is set while the net device is
open, then it could also happen between the dev_open() and
clear_bit(STMMAC_DOWN) calls right? So you'd have to clear STMMAC_DOWN before
calling dev_open() but then I don't see the usefulness of setting STMMAC_DOWN
and clearing it immediately. Maybe closing and opening the net device should be
enough?
Moreover, it seems strange to me that stmmac_interrupt() unconditionnally
ignores interrupts when the driver is in STMMAC_DOWN state. This seems like
dangerous behaviour, since it could cause IRQ storm issues whenever something
in the driver sets this state. I'm not too familiar with the interrupt handling
in this driver, but maybe stmmac_interrupt() could clear interrupts
unconditionnally in the STMMAC_DOWN state?
Best Regards,
--
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v1 2/2] dt-bindings: clock: rockchip: Add support for clk input / output switch
From: Krzysztof Kozlowski @ 2024-03-31 8:24 UTC (permalink / raw)
To: Sugar Zhang, heiko
Cc: linux-rockchip, Conor Dooley, Krzysztof Kozlowski,
Michael Turquette, Rob Herring, Stephen Boyd, devicetree,
linux-arm-kernel, linux-clk, linux-kernel
In-Reply-To: <1711340191-69588-2-git-send-email-sugar.zhang@rock-chips.com>
On 25/03/2024 05:16, Sugar Zhang wrote:
> This patch add support switch for clk-bidirection which located
Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> at GRF, such as SAIx_MCLK_{IN OUT} which share the same pin.
> and these config maybe located in many pieces of GRF,
> which hard to addressed in one single clk driver. so, we add
> this simple helper driver to address this situation.
>
> In order to simplify implement and usage, and also for safety
> clk usage (avoid high freq glitch), we set all clk out as disabled
> (which means Input default for clk-bidrection) in the pre-stage,
> such boot-loader or init by HW default. And then set a safety freq
> before enable clk-out, such as "assign-clock-rates" or clk_set_rate
> in drivers.
>
> e.g.
>
> 1. mclk{out,in}_sai0 define:
>
> mclkin_sai0: mclkin-sai0 {
> compatible = "fixed-clock";
> #clock-cells = <0>;
> clock-frequency = <12288000>;
> clock-output-names = "mclk_sai0_from_io";
> };
>
> mclkout_sai0: mclkout-sai0@ff040070 {
> compatible = "rockchip,clk-out";
> reg = <0 0xff040070 0 0x4>;
> clocks = <&cru MCLK_SAI0_OUT2IO>;
> #clock-cells = <0>;
> clock-output-names = "mclk_sai0_to_io";
> rockchip,bit-shift = <4>;
> //example with PD if reg access needed
> power-domains = <&power RK3562_PD_VO>;
> };
>
> Note:
>
> clock-output-names of mclkin_sai0 should equal to strings in drivers. such as:
>
> drivers/clk/rockchip/clk-rk3562.c:
> PNAME(clk_sai0_p) = { "clk_sai0_src", "clk_sai0_frac", "xin_osc0_half", "mclk_sai0_from_io" };
>
> 2. mclkout_sai0 usage:
>
> &ext_codec {
> clocks = <&mclkout_sai0>;
> clock-names = "mclk";
> assigned-clocks = <&mclkout_sai0>;
> assigned-clock-rates = <12288000>;
> pinctrl-names = "default";
> pinctrl-0 = <&i2s0m0_mclk>;
> };
>
> clk_summary on sai0 work:
>
> cat /sys/kernel/debug/clk/clk_summary | egrep "pll|sai0"
>
> clk_sai0_src 1 1 0 1188000000 0 0 50000
> clk_sai0_frac 1 1 0 12288000 0 0 50000
> clk_sai0 1 1 0 12288000 0 0 50000
> mclk_sai0 1 1 0 12288000 0 0 50000
> mclk_sai0_out2io 1 1 0 12288000 0 0 50000
> mclk_sai0_to_io 1 1 0 12288000 0 0 50000
>
> example with PD if reg access needed:
>
> * PD status when mclk_sai0_to_io on:
>
> cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
>
> domain status children
> /device runtime status
> ----------------------------------------------------------------------
> ...
>
> vo on
> /devices/platform/clocks/ff040070.mclkout-sai0 active
> ...
>
> * PD status when mclk_sai0_to_io off:
>
> cat /sys/kernel/debug/pm_genpd/pm_genpd_summary
>
> domain status children
> /device runtime status
> ----------------------------------------------------------------------
> ...
>
> vo off-0
> /devices/platform/clocks/ff040070.mclkout-sai0 suspended
> ...
>
> 3. mclkin_sai0 usage:
>
> please override freq of mclkin as the real external clkin, such as:
>
> &mclkin_sai0 {
> clock-frequency = <24576000>;
> }
>
> &ext_codec {
> clocks = <&mclkin_sai0>;
> clock-names = "mclk";
> assigned-clocks = <&cru CLK_SAI0>;
> assigned-clock-parents = <&mclkin_sai0>;
> pinctrl-names = "default";
> pinctrl-0 = <&i2s0m0_mclk>;
> };
>
> clk_summary on sai0 work:
>
> cat /sys/kernel/debug/clk/clk_summary | egrep "pll|sai0"
>
> mclk_sai0_from_io 1 1 0 12288000 0 0 50000
> clk_sai0 1 1 0 12288000 0 0 50000
> mclk_sai0 1 1 0 12288000 0 0 50000
> mclk_sai0_out2io 0 0 0 12288000 0 0 50000
> mclk_sai0_to_io 0 0 0 12288000 0 0 50000
None of this long commit msg is a description of hardware. Please remove
all unnecessary information and instead describe the problem you are
solving or the hardware.
>
> Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>
> ---
>
> .../bindings/clock/rockchip,clk-out.yaml | 107 +++++++++++++++++++++
> 1 file changed, 107 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/rockchip,clk-out.yaml
>
> diff --git a/Documentation/devicetree/bindings/clock/rockchip,clk-out.yaml b/Documentation/devicetree/bindings/clock/rockchip,clk-out.yaml
> new file mode 100644
> index 0000000..6582605
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/rockchip,clk-out.yaml
> @@ -0,0 +1,107 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/rockchip,clk-out.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rockchip Clock Out Control Module Binding
It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.
> +
> +maintainers:
> + - Sugar Zhang <sugar.zhang@rock-chips.com>
> +
> +description: |
> + This add support switch for clk-bidirection which located
No, "this does not add". This is description of hardware, thus describe
hardware.
> + at GRF, such as SAIx_MCLK_{IN OUT} which share the same pin.
> + and these config maybe located in many pieces of GRF,
> + which hard to addressed in one single clk driver. so, we add
> + this simple helper driver to address this situation.
Bindings are for hardware, not drivers. Rephrase EVERYTHING to focus on
hardware, not driver. Otherwise it looks like you wrote it for drivers,
which is a NAK.
> +
> + In order to simplify implement and usage, and also for safety
> + clk usage (avoid high freq glitch), we set all clk out as disabled
> + (which means Input default for clk-bidrection) in the pre-stage,
> + such boot-loader or init by HW default. And then set a safety freq
> + before enable clk-out, such as "assign-clock-rates" or clk_set_rate
> + in drivers.
> +
> +properties:
> + compatible:
> + enum:
> + - rockchip,clk-out
Missing SoC compatible. See writing bindings.
> +
> + reg:
> + maxItems: 1
> +
> + "#clock-cells":
> + const: 1
> +
> + clocks:
> + maxItems: 1
> + description: parent clocks.
Drop useless description.
> +
> + power-domains:
> + maxItems: 1
> +
> + clock-output-names:
> + maxItems: 1
> +
> + rockchip,bit-shift:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: Defines the bit shift of clk out enable.
No, this is deduced from compatible.
> +
> + rockchip,bit-set-to-disable:
> + type: boolean
> + description: |
> + By default this clock sets the bit at bit-shift to enable the clock.
> + Setting this property does the opposite: setting the bit disable
> + the clock and clearing it enables the clock.
No, this is deduced from compatible.
Binding looks really poor, like written for some debug driver.
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - "#clock-cells"
> + - clock-output-names
> + - rockchip,bit-shift
> +
> +additionalProperties: false
> +
> +examples:
> + # Clock Provider node:
> + - |
> + mclkin_sai0: mclkin-sai0 {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <12288000>;
> + clock-output-names = "mclk_sai0_from_io";
> + };
Drop, unrelated.
> +
> + mclkout_sai0: mclkout-sai0@ff040070 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + compatible = "rockchip,clk-out";
> + reg = <0 0xff040070 0 0x4>;
> + clocks = <&cru MCLK_SAI0_OUT2IO>;
> + #clock-cells = <0>;
> + clock-output-names = "mclk_sai0_to_io";
> + rockchip,bit-shift = <4>;
> + };
> +
> + # Clock mclkout Consumer node:
> + - |
> + ext_codec {
Drop, not related, incorrect name.
> + clocks = <&mclkout_sai0>;
> + clock-names = "mclk";
> + assigned-clocks = <&mclkout_sai0>;
> + assigned-clock-rates = <12288000>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&i2s0m0_mclk>;
> + };
> +
> + # Clock mclkin Consumer node:
> + - |
> + ext_codec {
Drop.
Don't upstream poor quality downstream code, but fix it first to match
upstream style. Read carefully writing bindings and DTS coding style.
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] pinctrl: sunxi: sun9i-a80-r: drop driver owner assignment
From: Chen-Yu Tsai @ 2024-03-31 4:05 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Linus Walleij, Jernej Skrabec, Samuel Holland, linux-gpio,
linux-arm-kernel, linux-sunxi, linux-kernel
In-Reply-To: <20240330210954.100842-1-krzysztof.kozlowski@linaro.org>
On Sun, Mar 31, 2024 at 5:10 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> Core in platform_driver_register() already sets the .owner, so driver
> does not need to.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Acked-by: Chen-Yu Tsai <wens@csie.org>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [External] Re: [PATCH bpf-next v2 1/9] bpf: tracing: add support to record and check the accessed args
From: Andrii Nakryiko @ 2024-03-31 2:34 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, 梦龙董, Alexei Starovoitov,
Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Eddy Z, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, David S. Miller,
David Ahern, Dave Hansen, X86 ML, Mathieu Desnoyers,
Quentin Monnet, bpf, linux-arm-kernel, LKML, linux-riscv,
linux-s390, Network Development, linux-trace-kernel,
open list:KERNEL SELFTEST FRAMEWORK, linux-stm32
In-Reply-To: <ZghRXtc8ZiTOKMR3@krava>
On Sat, Mar 30, 2024 at 10:52 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Sat, Mar 30, 2024 at 08:27:55AM -0400, Steven Rostedt wrote:
> > On Fri, 29 Mar 2024 16:28:33 -0700
> > Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > > I thought I'll just ask instead of digging through code, sorry for
> > > being lazy :) Is there any way to pass pt_regs/ftrace_regs captured
> > > before function execution to a return probe (fexit/kretprobe)? I.e.,
> > > how hard is it to pass input function arguments to a kretprobe? That's
> > > the biggest advantage of fexit over kretprobe, and if we can make
> > > these original pt_regs/ftrace_regs available to kretprobe, then
> > > multi-kretprobe will effectively be this multi-fexit.
> >
> > This should be possible with the updates that Masami is doing with the
> > fgraph code.
>
> yes, I have bpf kprobe-multi link support for that [0] (it's on top of
> Masami's fprobe-over-fgraph changes) we discussed that in [1]
Sorry, I forgot the regs/args part, mostly remembering we discussed
session cookie ideas. Thanks for reminder!
>
> jirka
>
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git/log/?h=bpf/session_data
> [1] https://lore.kernel.org/bpf/20240228090242.4040210-1-jolsa@kernel.org/
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 01/11] drm/mediatek: aal: drop driver owner initialization
From: Krzysztof Kozlowski @ 2024-03-30 20:43 UTC (permalink / raw)
To: Chun-Kuang Hu, Philipp Zabel, David Airlie, Daniel Vetter,
Matthias Brugger, AngeloGioacchino Del Regno
Cc: dri-devel, linux-mediatek, linux-kernel, linux-arm-kernel,
Krzysztof Kozlowski
In-Reply-To: <20240330-b4-module-owner-drm-mediatek-v1-0-fd5c4b8d633e@linaro.org>
Core in platform_driver_register() already sets the .owner, so driver
does not need to. Whatever is set here will be anyway overwritten by
main driver calling platform_driver_register().
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
drivers/gpu/drm/mediatek/mtk_disp_aal.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_aal.c b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
index 40fe403086c3..f6f2c24abc93 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
@@ -223,7 +223,6 @@ struct platform_driver mtk_disp_aal_driver = {
.remove_new = mtk_disp_aal_remove,
.driver = {
.name = "mediatek-disp-aal",
- .owner = THIS_MODULE,
.of_match_table = mtk_disp_aal_driver_dt_match,
},
};
--
2.34.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH RESEND] arm64: dts: marvell: add DTS for 7215-IXS-A1 board
From: Andrew Lunn @ 2024-03-30 21:36 UTC (permalink / raw)
To: Natarajan Subbiramani; +Cc: gregory.clement, linux-arm-kernel, nsubbara
In-Reply-To: <20240329223012.935-1-nataccet@gmail.com>
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2024 Nokia
> + * Copyright (C) 2020 Marvell International Ltd.
> + */
> +
> +#include "cn9130.dtsi" /* include SoC device tree */
> +
> +#include <dt-bindings/gpio/gpio.h>
> +
> +/ {
> + #address-cells = <0x02>;
> + #size-cells = <0x02>;
> + model = "7215 IXS-A1";
> + compatible = "marvell,cn9130\0marvell,armada-ap807-quad\0marvell,armada-ap807";
Could you please explain the \0 ?
Also, there is generally a compatible which has something to do with
the actual product, 7215 IXS-A1, not just the RDK it is based on.
> +&cp0_mdio {
> + status = "okay";
> + phy0: ethernet-phy@0 {
> + reg = <0>;
> + /* Management port LED blink activity*/
> + marvell,reg-init = <0x03 0x10 0x0 0x1140>;
Please configure this via /sys/class/leds. Take a look at armada 370
rd for an example for what you need in DT.
> diff --git a/arch/arm64/boot/dts/marvell/Makefile b/arch/arm64/boot/dts/marvell/Makefile
> index 99b8cb3c49e1..124a7d1fd5a8 100644
> --- a/arch/arm64/boot/dts/marvell/Makefile
> +++ b/arch/arm64/boot/dts/marvell/Makefile
> @@ -28,3 +28,4 @@ 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) += 7215-ixs-a1.dtb
This file is sorted alphabetically. It is not so important for this
file, but Makefiles which see a lot of changed get merge conflicts if
you always add at the end. By keeping it in alphabetic order it tends
to spread changes out so they are less likely to cause such conflicts.
Also, maybe this should be nokia-7215-ixs-a1.dtb?
Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] pmdomain: mediatek: scpsys: drop driver owner assignment
From: Krzysztof Kozlowski @ 2024-03-30 21:10 UTC (permalink / raw)
To: Ulf Hansson, Matthias Brugger, AngeloGioacchino Del Regno,
linux-pm, linux-kernel, linux-arm-kernel, linux-mediatek
Cc: Krzysztof Kozlowski
Core in platform_driver_register() already sets the .owner, so driver
does not need to.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
drivers/pmdomain/mediatek/mtk-scpsys.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/pmdomain/mediatek/mtk-scpsys.c b/drivers/pmdomain/mediatek/mtk-scpsys.c
index 59a7a8c261ed..1a80c1537a43 100644
--- a/drivers/pmdomain/mediatek/mtk-scpsys.c
+++ b/drivers/pmdomain/mediatek/mtk-scpsys.c
@@ -1138,7 +1138,6 @@ static struct platform_driver scpsys_drv = {
.driver = {
.name = "mtk-scpsys",
.suppress_bind_attrs = true,
- .owner = THIS_MODULE,
.of_match_table = of_scpsys_match_tbl,
},
};
--
2.34.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH] pinctrl: sunxi: sun9i-a80-r: drop driver owner assignment
From: Krzysztof Kozlowski @ 2024-03-30 21:09 UTC (permalink / raw)
To: Linus Walleij, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
linux-gpio, linux-arm-kernel, linux-sunxi, linux-kernel
Cc: Krzysztof Kozlowski
Core in platform_driver_register() already sets the .owner, so driver
does not need to.
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
drivers/pinctrl/sunxi/pinctrl-sun9i-a80-r.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/pinctrl/sunxi/pinctrl-sun9i-a80-r.c b/drivers/pinctrl/sunxi/pinctrl-sun9i-a80-r.c
index 919b6a20af83..5b4822f77d2a 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sun9i-a80-r.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sun9i-a80-r.c
@@ -169,7 +169,6 @@ static struct platform_driver sun9i_a80_r_pinctrl_driver = {
.probe = sun9i_a80_r_pinctrl_probe,
.driver = {
.name = "sun9i-a80-r-pinctrl",
- .owner = THIS_MODULE,
.of_match_table = sun9i_a80_r_pinctrl_match,
},
};
--
2.34.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox