PATCH V1 2022-04-08 20:38:00:
This series fixes many (not all) warnings from dt_binding_check/dtbs_check for the jz4780 based Imagination CI20 board.
H. Nikolaus Schaller (18):
MIPS: DTS: jz4780: remove cpu clock-names as reported by dtbscheck
MIPS: DTS: jz4780: fix cgu as reported by dtbscheck
MIPS: DTS: jz4780: fix tcu timer as reported by dtbscheck
MIPS: DTS: jz4780: fix ost timer as reported by dtbscheck
MIPS: DTS: jz4780: fix pinctrl as reported by dtbscheck
MIPS: DTS: jz4780: fix rtc node as reported by dtbscheck
MIPS: DTS: jz4780: fix otg node as reported by dtbscheck
MIPS: DTS: jz4780: fix lcd controllers as reported by dtbscheck
MIPS: DTS: jz4780: fix dma-controller as reported by dtbscheck
MIPS: DTS: jz4780: fix uart dmas as reported by dtbscheck
MIPS: DTS: jz4780: fix i2c dmas as reported by dtbscheck
MIPS: DTS: jz4780: fix nemc memory controller as reported by dtbscheck
dt-bindings: fix jz4780-nemc issue as reported by dtbscheck
MIPS: DTS: CI20: add missing model as reported by dtbscheck
MIPS: DTS: CI20: fix fixed regulators as reported by dtbscheck
MIPS: DTS: CI20: fix /memory as reported by dtbscheck
MIPS: DTS: CI20: fix wifi as reported by dtbscheck
MIPS: DTS: CI20: fix bluetooth as reported by dtbscheck
.../memory-controllers/ingenic,nemc.yaml | 2 +-
arch/mips/boot/dts/ingenic/ci20.dts | 14 ++--
arch/mips/boot/dts/ingenic/jz4780.dtsi | 71 ++++++++++++++-----
3 files changed, 64 insertions(+), 23 deletions(-)
--
2.33.0
/Volumes/CaseSensitive/master/arch/mips/boot/dts/ingenic/ci20.dtb: lcdc0@13050000: $nodename:0: 'lcdc0@13050000' does not match '^lcd-controller@[0-9a-f]+$'
From schema: /Volumes/CaseSensitive/master/Documentation/devicetree/bindings/display/ingenic,lcd.yaml
/Volumes/CaseSensitive/master/arch/mips/boot/dts/ingenic/ci20.dtb: lcdc0@13050000: clock-names:0: 'lcd_pclk' was expected
From schema: /Volumes/CaseSensitive/master/Documentation/devicetree/bindings/display/ingenic,lcd.yaml
/Volumes/CaseSensitive/master/arch/mips/boot/dts/ingenic/ci20.dtb: lcdc0@13050000: clock-names:1: 'lcd' was expected
From schema: /Volumes/CaseSensitive/master/Documentation/devicetree/bindings/display/ingenic,lcd.yaml
/Volumes/CaseSensitive/master/arch/mips/boot/dts/ingenic/ci20.dtb: lcdc1@130a0000: $nodename:0: 'lcdc1@130a0000' does not match '^lcd-controller@[0-9a-f]+$'
From schema: /Volumes/CaseSensitive/master/Documentation/devicetree/bindings/display/ingenic,lcd.yaml
/Volumes/CaseSensitive/master/arch/mips/boot/dts/ingenic/ci20.dtb: lcdc1@130a0000: clock-names:0: 'lcd_pclk' was expected
From schema: /Volumes/CaseSensitive/master/Documentation/devicetree/bindings/display/ingenic,lcd.yaml
/Volumes/CaseSensitive/master/arch/mips/boot/dts/ingenic/ci20.dtb: lcdc1@130a0000: clock-names:1: 'lcd' was expected
From schema: /Volumes/CaseSensitive/master/Documentation/devicetree/bindings/display/ingenic,lcd.yaml
Signed-off-by: H. Nikolaus Schaller <[email protected]>
---
arch/mips/boot/dts/ingenic/jz4780.dtsi | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index c5124459678b7..8b95486c8afa7 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -457,12 +457,12 @@ hdmi: hdmi@10180000 {
status = "disabled";
};
- lcdc0: lcdc0@13050000 {
+ lcdc0: lcd-controller@13050000 {
compatible = "ingenic,jz4780-lcd";
reg = <0x13050000 0x1800>;
- clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD0PIXCLK>;
- clock-names = "lcd", "lcd_pclk";
+ clocks = <&cgu JZ4780_CLK_LCD0PIXCLK>, <&cgu JZ4780_CLK_TVE>;
+ clock-names = "lcd_pclk", "lcd";
interrupt-parent = <&intc>;
interrupts = <31>;
@@ -470,12 +470,12 @@ lcdc0: lcdc0@13050000 {
status = "disabled";
};
- lcdc1: lcdc1@130a0000 {
+ lcdc1: lcd-controller@130a0000 {
compatible = "ingenic,jz4780-lcd";
reg = <0x130a0000 0x1800>;
- clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD1PIXCLK>;
- clock-names = "lcd", "lcd_pclk";
+ clocks = <&cgu JZ4780_CLK_LCD1PIXCLK>, <&cgu JZ4780_CLK_TVE>;
+ clock-names = "lcd_pclk", "lcd";
interrupt-parent = <&intc>;
interrupts = <23>;
--
2.33.0
jz4780-nemc needs to be compatible to simple-mfd as well or we get
arch/mips/boot/dts/ingenic/ci20.dtb: memory-controller@13410000: compatible: 'oneOf' conditional failed, one must be fixed:
['ingenic,jz4780-nemc', 'simple-mfd'] is too long
'ingenic,jz4725b-nemc' was expected
'ingenic,jz4740-nemc' was expected
From schema: Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
Signed-off-by: H. Nikolaus Schaller <[email protected]>
---
.../devicetree/bindings/memory-controllers/ingenic,nemc.yaml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml b/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
index 24f9e19820282..3b1116588de3d 100644
--- a/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
@@ -17,7 +17,7 @@ properties:
oneOf:
- enum:
- ingenic,jz4740-nemc
- - ingenic,jz4780-nemc
+ - [ ingenic,jz4780-nemc, simple-mfd ]
- items:
- const: ingenic,jz4725b-nemc
- const: ingenic,jz4740-nemc
--
2.33.0
arch/mips/boot/dts/ingenic/ci20.dtb: wifi@1: compatible: oneOf conditional failed, one must be fixed:
[brcm,bcm4330-fmac] is too short
brcm,bcm4329-fmac was expected
From schema: Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml
Signed-off-by: H. Nikolaus Schaller <[email protected]>
---
arch/mips/boot/dts/ingenic/ci20.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
index 98f0a5ccbb4da..dc587b4b36009 100644
--- a/arch/mips/boot/dts/ingenic/ci20.dts
+++ b/arch/mips/boot/dts/ingenic/ci20.dts
@@ -176,7 +176,7 @@ &mmc1 {
brcmf: wifi@1 {
/* reg = <4>;*/
- compatible = "brcm,bcm4330-fmac";
+ compatible = "brcm,bcm4330-fmac", "brcm,bcm4329-fmac";
vcc-supply = <&wlan0_power>;
device-wakeup-gpios = <&gpd 9 GPIO_ACTIVE_HIGH>;
shutdown-gpios = <&gpf 7 GPIO_ACTIVE_LOW>;
--
2.33.0
arch/mips/boot/dts/ingenic/ci20.dtb: timer@10002000: timer@e0:compatible: 'oneOf' conditional failed, one must be fixed:
['ingenic,jz4780-ost', 'ingenic,jz4770-ost'] is too long
'ingenic,jz4780-ost' is not one of ['ingenic,jz4725b-ost', 'ingenic,jz4760b-ost']
'ingenic,jz4760-ost' was expected
'ingenic,jz4725b-ost' was expected
'ingenic,jz4760b-ost' was expected
From schema: Documentation/devicetree/bindings/timer/ingenic,tcu.yaml
Signed-off-by: H. Nikolaus Schaller <[email protected]>
---
arch/mips/boot/dts/ingenic/jz4780.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 6027f14c393e3..5f44cf004d473 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -134,7 +134,7 @@ pwm: pwm@40 {
};
ost: timer@e0 {
- compatible = "ingenic,jz4780-ost", "ingenic,jz4770-ost";
+ compatible = "ingenic,jz4780-ost", "ingenic,jz4760b-ost";
reg = <0xe0 0x20>;
clocks = <&tcu TCU_CLK_OST>;
--
2.33.0
arch/mips/boot/dts/ingenic/ci20.dtb: pin-controller@10010000: $nodename:0: 'pin-controller@10010000' does not match '^(pinctrl|pinmux)(@[0-9a-f]+)?$'
From schema: Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.yaml
Signed-off-by: H. Nikolaus Schaller <[email protected]>
---
arch/mips/boot/dts/ingenic/jz4780.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 5f44cf004d473..b5299eaffb84a 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -155,7 +155,7 @@ rtc_dev: rtc@10003000 {
clock-names = "rtc";
};
- pinctrl: pin-controller@10010000 {
+ pinctrl: pinctrl@10010000 {
compatible = "ingenic,jz4780-pinctrl";
reg = <0x10010000 0x600>;
--
2.33.0
arch/mips/boot/dts/ingenic/ci20.dtb: usb@13500000: compatible: 'oneOf' conditional failed, one must be fixed:
['ingenic,jz4780-otg', 'snps,dwc2'] is too long
['ingenic,jz4780-otg', 'snps,dwc2'] is too short
'brcm,bcm2835-usb' was expected
'hisilicon,hi6220-usb' was expected
'rockchip,rk3066-usb' was expected
'ingenic,jz4780-otg' is not one of ['rockchip,px30-usb', 'rockchip,rk3036-usb', 'rockchip,rk3188-usb', 'rockchip,rk3228-usb', 'rockchip,rk3288-usb', 'rockchip,rk3308-usb', 'rockchip,rk3328-usb', 'rockchip,rk3368-usb', 'rockchip,rv1108-usb']
'lantiq,arx100-usb' was expected
'lantiq,xrx200-usb' was expected
'ingenic,jz4780-otg' is not one of ['amlogic,meson8-usb', 'amlogic,meson8b-usb', 'amlogic,meson-gxbb-usb', 'amlogic,meson-g12a-usb', 'intel,socfpga-agilex-hsotg']
'amcc,dwc-otg' was expected
'apm,apm82181-dwc-otg' was expected
'snps,dwc2' was expected
'st,stm32f4x9-fsotg' was expected
'st,stm32f4x9-hsotg' was expected
'st,stm32f7-hsotg' was expected
'st,stm32mp15-fsotg' was expected
'st,stm32mp15-hsotg' was expected
'samsung,s3c6400-hsotg' was expected
'intel,socfpga-agilex-hsotg' was expected
From schema: Documentation/devicetree/bindings/usb/dwc2.yaml
Signed-off-by: H. Nikolaus Schaller <[email protected]>
---
arch/mips/boot/dts/ingenic/jz4780.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 2021836983c96..c5124459678b7 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -576,7 +576,7 @@ bch: bch@134d0000 {
};
otg: usb@13500000 {
- compatible = "ingenic,jz4780-otg", "snps,dwc2";
+ compatible = "snps,dwc2";
reg = <0x13500000 0x40000>;
interrupt-parent = <&intc>;
--
2.33.0
> Am 09.04.2022 um 15:44 schrieb Krzysztof Kozlowski <[email protected]>:
>
> On 09/04/2022 15:32, H. Nikolaus Schaller wrote:
>>
>>
>>> Am 09.04.2022 um 15:15 schrieb Krzysztof Kozlowski <[email protected]>:
>>>
>>> On 09/04/2022 15:05, H. Nikolaus Schaller wrote:
>>>>>
>>>>> This looks wrong, the block usually should have a specific compatible.
>>>>> Please mention why it does not.
>>>>
>>>> Well, I did not even have that idea that it could need an explanation.
>>>>
>>>> There is no "ingenic,jz4780-otg" and none is needed here to make it work.
>>>
>>> Make it work in what terms? We talk about hardware description, right?
>>
>> Yes.
>>
>>>
>>>>
>>>> Therefore the generic "snps,dwc2" is sufficient.
>>>
>>> No, you are mixing now driver behavior (is sufficient) with hardware
>>> description.
>>
>> No. "snps,dwc2" is a hardware description for a licensed block.
>> Not a driver behavior.
>
> snps,dwc2 matches the original block, not necessarily this
> implementation. Unless you are sure?
I assume. Nobody has reported an issue without having any specific jz4780 driver in place.
Well, that is only evidence, not bullet proof.
>
>>
>>> Most of licensed blocks require the specific compatible to
>>> differentiate it.
>>
>> If there is a need to differentiate.
>
> No, regardless whether there is a need currently, most of them have
> specific compatibles, because there are some minor differences. Even if
> difference is not visible from programming model or wiring, it might
> justify it's own specific compatible. For example because maybe once
> that tiny difference will require some changes.
>
> Someone added the ingenic compatible, so why do you assume that one tool
> (bindings) is correct but other piece of code (using specific
> compatible) is not? You use the argument "bindings warning" which is not
> enough. Argument that blocks are 100% same, is good enough, if you are
> sure. Just use it in commit msg. But are you sure that these are the
> same? Same pins, same programming model (entire model, not used by Linux)?
The compatible ingenic,jz4780-otg was introduced in 158c774d3c64859e84dd20e04d5fb18c8d3d318e.
Hence I have added Yanjie for clarification why he added it in the .dts and not in the bindings.
BR and thanks,
Nikolaus
On 08/04/2022 20:37, H. Nikolaus Schaller wrote:
> arch/mips/boot/dts/ingenic/ci20.dtb: usb@13500000: compatible: 'oneOf' conditional failed, one must be fixed:
> ['ingenic,jz4780-otg', 'snps,dwc2'] is too long
> ['ingenic,jz4780-otg', 'snps,dwc2'] is too short
> 'brcm,bcm2835-usb' was expected
> 'hisilicon,hi6220-usb' was expected
> 'rockchip,rk3066-usb' was expected
> 'ingenic,jz4780-otg' is not one of ['rockchip,px30-usb', 'rockchip,rk3036-usb', 'rockchip,rk3188-usb', 'rockchip,rk3228-usb', 'rockchip,rk3288-usb', 'rockchip,rk3308-usb', 'rockchip,rk3328-usb', 'rockchip,rk3368-usb', 'rockchip,rv1108-usb']
> 'lantiq,arx100-usb' was expected
> 'lantiq,xrx200-usb' was expected
> 'ingenic,jz4780-otg' is not one of ['amlogic,meson8-usb', 'amlogic,meson8b-usb', 'amlogic,meson-gxbb-usb', 'amlogic,meson-g12a-usb', 'intel,socfpga-agilex-hsotg']
> 'amcc,dwc-otg' was expected
> 'apm,apm82181-dwc-otg' was expected
> 'snps,dwc2' was expected
> 'st,stm32f4x9-fsotg' was expected
> 'st,stm32f4x9-hsotg' was expected
> 'st,stm32f7-hsotg' was expected
> 'st,stm32mp15-fsotg' was expected
> 'st,stm32mp15-hsotg' was expected
> 'samsung,s3c6400-hsotg' was expected
> 'intel,socfpga-agilex-hsotg' was expected
You really don't need to paste entire warning.
> From schema: Documentation/devicetree/bindings/usb/dwc2.yaml
>
> Signed-off-by: H. Nikolaus Schaller <[email protected]>
> ---
> arch/mips/boot/dts/ingenic/jz4780.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> index 2021836983c96..c5124459678b7 100644
> --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> @@ -576,7 +576,7 @@ bch: bch@134d0000 {
> };
>
> otg: usb@13500000 {
> - compatible = "ingenic,jz4780-otg", "snps,dwc2";
> + compatible = "snps,dwc2";
This looks wrong, the block usually should have a specific compatible.
Please mention why it does not.
Best regards,
Best regards,
Krzysztof
> Am 09.04.2022 um 13:15 schrieb Krzysztof Kozlowski <[email protected]>:
>
> On 08/04/2022 20:37, H. Nikolaus Schaller wrote:
>> arch/mips/boot/dts/ingenic/ci20.dtb: usb@13500000: compatible: 'oneOf' conditional failed, one must be fixed:
>> ['ingenic,jz4780-otg', 'snps,dwc2'] is too long
>> ['ingenic,jz4780-otg', 'snps,dwc2'] is too short
>> 'brcm,bcm2835-usb' was expected
>> 'hisilicon,hi6220-usb' was expected
>> 'rockchip,rk3066-usb' was expected
>> 'ingenic,jz4780-otg' is not one of ['rockchip,px30-usb', 'rockchip,rk3036-usb', 'rockchip,rk3188-usb', 'rockchip,rk3228-usb', 'rockchip,rk3288-usb', 'rockchip,rk3308-usb', 'rockchip,rk3328-usb', 'rockchip,rk3368-usb', 'rockchip,rv1108-usb']
>> 'lantiq,arx100-usb' was expected
>> 'lantiq,xrx200-usb' was expected
>> 'ingenic,jz4780-otg' is not one of ['amlogic,meson8-usb', 'amlogic,meson8b-usb', 'amlogic,meson-gxbb-usb', 'amlogic,meson-g12a-usb', 'intel,socfpga-agilex-hsotg']
>> 'amcc,dwc-otg' was expected
>> 'apm,apm82181-dwc-otg' was expected
>> 'snps,dwc2' was expected
>> 'st,stm32f4x9-fsotg' was expected
>> 'st,stm32f4x9-hsotg' was expected
>> 'st,stm32f7-hsotg' was expected
>> 'st,stm32mp15-fsotg' was expected
>> 'st,stm32mp15-hsotg' was expected
>> 'samsung,s3c6400-hsotg' was expected
>> 'intel,socfpga-agilex-hsotg' was expected
>
> You really don't need to paste entire warning.
>
>
>> From schema: Documentation/devicetree/bindings/usb/dwc2.yaml
>>
>> Signed-off-by: H. Nikolaus Schaller <[email protected]>
>> ---
>> arch/mips/boot/dts/ingenic/jz4780.dtsi | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> index 2021836983c96..c5124459678b7 100644
>> --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> @@ -576,7 +576,7 @@ bch: bch@134d0000 {
>> };
>>
>> otg: usb@13500000 {
>> - compatible = "ingenic,jz4780-otg", "snps,dwc2";
>> + compatible = "snps,dwc2";
>
> This looks wrong, the block usually should have a specific compatible.
> Please mention why it does not.
Well, I did not even have that idea that it could need an explanation.
There is no "ingenic,jz4780-otg" and none is needed here to make it work.
Therefore the generic "snps,dwc2" is sufficient.
arch/mips/boot/dts/ingenic/ci20.dtb: cpu@0: clock-names does not match any of the regexes: pinctrl-[0-9]+
From schema: Documentation/devicetree/bindings/mips/ingenic/ingenic,cpu.yaml
arch/mips/boot/dts/ingenic/ci20.dtb: cpu@1: clock-names does not match any of the regexes: pinctrl-[0-9]+
From schema: Documentation/devicetree/bindings/mips/ingenic/ingenic,cpu.yaml
Signed-off-by: H. Nikolaus Schaller <[email protected]>
---
arch/mips/boot/dts/ingenic/jz4780.dtsi | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index b998301f179ce..710605df40ed3 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -18,7 +18,6 @@ cpu0: cpu@0 {
reg = <0>;
clocks = <&cgu JZ4780_CLK_CPU>;
- clock-names = "cpu";
};
cpu1: cpu@1 {
@@ -27,7 +26,6 @@ cpu1: cpu@1 {
reg = <1>;
clocks = <&cgu JZ4780_CLK_CORE1>;
- clock-names = "cpu";
};
};
--
2.33.0
> Am 09.04.2022 um 15:46 schrieb Krzysztof Kozlowski <[email protected]>:
>
> On 09/04/2022 15:41, H. Nikolaus Schaller wrote:
>>>
>>> No. I ask you to fix all pin-controller cases, for entire MIPS, not just
>>> one.
>>
>> Oops. Nope. I am a volunteer and neither your employee nor slave.
>
> No one thinks differently and I am sorry that you felt it. Please accept
> my apologies, if you get different impression. You understand though the
> meaning of word "ask for something" and "order something" (the latter
> which I did not use).
>
> I just asked.
Ok. Maybe english is not our mother language and we sometimes don't
get the nuances right. Sorry if I understood that wrongly.
At least I now understand what you did suggest.
Doing the same change for treewide MIPS is beyond my capabilities since
I can't easily test any compile setup. So far I only compile for CI20 and
as far as I know every machine still needs its own config for MIPS
(haven't checked recently). So I am not even sure if dtbscheck tells me
all locations.
BR and thanks,
Nikolaus
On 09/04/2022 15:41, H. Nikolaus Schaller wrote:
>>
>> No. I ask you to fix all pin-controller cases, for entire MIPS, not just
>> one.
>
> Oops. Nope. I am a volunteer and neither your employee nor slave.
No one thinks differently and I am sorry that you felt it. Please accept
my apologies, if you get different impression. You understand though the
meaning of word "ask for something" and "order something" (the latter
which I did not use).
I just asked.
Best regards,
Krzysztof
arch/mips/boot/dts/ingenic/ci20.dtb: serial@10030000: 'dmas' is a required property
From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml
arch/mips/boot/dts/ingenic/ci20.dtb: serial@10030000: 'dma-names' is a required property
From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml
arch/mips/boot/dts/ingenic/ci20.dtb: serial@10031000: 'dmas' is a required property
From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml
arch/mips/boot/dts/ingenic/ci20.dtb: serial@10031000: 'dma-names' is a required property
From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml
arch/mips/boot/dts/ingenic/ci20.dtb: serial@10032000: 'dmas' is a required property
From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml
arch/mips/boot/dts/ingenic/ci20.dtb: serial@10032000: 'dma-names' is a required property
From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml
arch/mips/boot/dts/ingenic/ci20.dtb: serial@10033000: 'dmas' is a required property
From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml
arch/mips/boot/dts/ingenic/ci20.dtb: serial@10033000: 'dma-names' is a required property
From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml
arch/mips/boot/dts/ingenic/ci20.dtb: serial@10034000: 'dmas' is a required property
From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml
arch/mips/boot/dts/ingenic/ci20.dtb: serial@10034000: 'dma-names' is a required property
From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml
arch/mips/boot/dts/ingenic/ci20.dtb: i2c@10050000: 'dmas' is a required property
Signed-off-by: H. Nikolaus Schaller <[email protected]>
---
arch/mips/boot/dts/ingenic/jz4780.dtsi | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index dc88f9e813453..73cd05cf26472 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -283,6 +283,10 @@ uart0: serial@10030000 {
clocks = <&ext>, <&cgu JZ4780_CLK_UART0>;
clock-names = "baud", "module";
+ dmas = <&dma JZ4780_DMA_UART0_RX 0xffffffff>,
+ <&dma JZ4780_DMA_UART0_TX 0xffffffff>;
+ dma-names = "rx", "tx";
+
status = "disabled";
};
@@ -296,6 +300,10 @@ uart1: serial@10031000 {
clocks = <&ext>, <&cgu JZ4780_CLK_UART1>;
clock-names = "baud", "module";
+ dmas = <&dma JZ4780_DMA_UART1_RX 0xffffffff>,
+ <&dma JZ4780_DMA_UART1_TX 0xffffffff>;
+ dma-names = "rx", "tx";
+
status = "disabled";
};
@@ -309,6 +317,10 @@ uart2: serial@10032000 {
clocks = <&ext>, <&cgu JZ4780_CLK_UART2>;
clock-names = "baud", "module";
+ dmas = <&dma JZ4780_DMA_UART2_RX 0xffffffff>,
+ <&dma JZ4780_DMA_UART2_TX 0xffffffff>;
+ dma-names = "rx", "tx";
+
status = "disabled";
};
@@ -322,6 +334,10 @@ uart3: serial@10033000 {
clocks = <&ext>, <&cgu JZ4780_CLK_UART3>;
clock-names = "baud", "module";
+ dmas = <&dma JZ4780_DMA_UART3_RX 0xffffffff>,
+ <&dma JZ4780_DMA_UART3_TX 0xffffffff>;
+ dma-names = "rx", "tx";
+
status = "disabled";
};
@@ -335,6 +351,10 @@ uart4: serial@10034000 {
clocks = <&ext>, <&cgu JZ4780_CLK_UART4>;
clock-names = "baud", "module";
+ dmas = <&dma JZ4780_DMA_UART4_RX 0xffffffff>,
+ <&dma JZ4780_DMA_UART4_TX 0xffffffff>;
+ dma-names = "rx", "tx";
+
status = "disabled";
};
--
2.33.0
On 08/04/2022 20:37, H. Nikolaus Schaller wrote:
> jz4780-nemc needs to be compatible to simple-mfd as well or we get
>
> arch/mips/boot/dts/ingenic/ci20.dtb: memory-controller@13410000: compatible: 'oneOf' conditional failed, one must be fixed:
> ['ingenic,jz4780-nemc', 'simple-mfd'] is too long
> 'ingenic,jz4725b-nemc' was expected
> 'ingenic,jz4740-nemc' was expected
> From schema: Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
>
> Signed-off-by: H. Nikolaus Schaller <[email protected]>
> ---
> .../devicetree/bindings/memory-controllers/ingenic,nemc.yaml | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml b/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
> index 24f9e19820282..3b1116588de3d 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
> @@ -17,7 +17,7 @@ properties:
> oneOf:
> - enum:
> - ingenic,jz4740-nemc
> - - ingenic,jz4780-nemc
> + - [ ingenic,jz4780-nemc, simple-mfd ]
This is not correct representation. If you really need simple-mfd, then
this should be a separate item below oneOf.
The true question is whether you need simple-mfd. Isn't the binding (and
the driver) expected to instantiate its children?
> - items:
> - const: ingenic,jz4725b-nemc
> - const: ingenic,jz4740-nemc
Best regards,
Krzysztof
/Volumes/CaseSensitive/master/arch/mips/boot/dts/ingenic/ci20.dtb: dma@13420000: $nodename:0: 'dma@13420000' does not match '^dma-controller(@.*)?$'
From schema: /Volumes/CaseSensitive/master/Documentation/devicetree/bindings/dma/ingenic,dma.yaml
Signed-off-by: H. Nikolaus Schaller <[email protected]>
---
arch/mips/boot/dts/ingenic/jz4780.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 8b95486c8afa7..dc88f9e813453 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -515,7 +515,7 @@ eth0_addr: eth-mac-addr@22 {
};
};
- dma: dma@13420000 {
+ dma: dma-controller@13420000 {
compatible = "ingenic,jz4780-dma";
reg = <0x13420000 0x400>, <0x13421000 0x40>;
#dma-cells = <2>;
--
2.33.0
On 09/04/2022 15:32, H. Nikolaus Schaller wrote:
>
>
>> Am 09.04.2022 um 15:15 schrieb Krzysztof Kozlowski <[email protected]>:
>>
>> On 09/04/2022 15:05, H. Nikolaus Schaller wrote:
>>>>
>>>> This looks wrong, the block usually should have a specific compatible.
>>>> Please mention why it does not.
>>>
>>> Well, I did not even have that idea that it could need an explanation.
>>>
>>> There is no "ingenic,jz4780-otg" and none is needed here to make it work.
>>
>> Make it work in what terms? We talk about hardware description, right?
>
> Yes.
>
>>
>>>
>>> Therefore the generic "snps,dwc2" is sufficient.
>>
>> No, you are mixing now driver behavior (is sufficient) with hardware
>> description.
>
> No. "snps,dwc2" is a hardware description for a licensed block.
> Not a driver behavior.
snps,dwc2 matches the original block, not necessarily this
implementation. Unless you are sure?
>
>> Most of licensed blocks require the specific compatible to
>> differentiate it.
>
> If there is a need to differentiate.
No, regardless whether there is a need currently, most of them have
specific compatibles, because there are some minor differences. Even if
difference is not visible from programming model or wiring, it might
justify it's own specific compatible. For example because maybe once
that tiny difference will require some changes.
Someone added the ingenic compatible, so why do you assume that one tool
(bindings) is correct but other piece of code (using specific
compatible) is not? You use the argument "bindings warning" which is not
enough. Argument that blocks are 100% same, is good enough, if you are
sure. Just use it in commit msg. But are you sure that these are the
same? Same pins, same programming model (entire model, not used by Linux)?
Best regards,
Krzysztof
arch/mips/boot/dts/ingenic/ci20.dtb: /: 'model' is a required property
From schema: dtschema/schemas/root-node.yaml
Signed-off-by: H. Nikolaus Schaller <[email protected]>
---
arch/mips/boot/dts/ingenic/ci20.dts | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
index ab6e3dc0bc1d0..cb5257d32b55e 100644
--- a/arch/mips/boot/dts/ingenic/ci20.dts
+++ b/arch/mips/boot/dts/ingenic/ci20.dts
@@ -11,6 +11,8 @@
/ {
compatible = "img,ci20", "ingenic,jz4780";
+ model = "Imagination creator CI20";
+
aliases {
serial0 = &uart0;
serial1 = &uart1;
--
2.33.0
Hi,
> Am 09.04.2022 um 14:27 schrieb Paul Cercueil <[email protected]>:
>
> Hi,
>
> Le sam., avril 9 2022 at 13:15:37 +0200, Krzysztof Kozlowski <[email protected]> a écrit :
>> On 08/04/2022 20:37, H. Nikolaus Schaller wrote:
>>> arch/mips/boot/dts/ingenic/ci20.dtb: usb@13500000: compatible: 'oneOf' conditional failed, one must be fixed:
>>> ['ingenic,jz4780-otg', 'snps,dwc2'] is too long
>>> ['ingenic,jz4780-otg', 'snps,dwc2'] is too short
>>> 'brcm,bcm2835-usb' was expected
>>> 'hisilicon,hi6220-usb' was expected
>>> 'rockchip,rk3066-usb' was expected
>>> 'ingenic,jz4780-otg' is not one of ['rockchip,px30-usb', 'rockchip,rk3036-usb', 'rockchip,rk3188-usb', 'rockchip,rk3228-usb', 'rockchip,rk3288-usb', 'rockchip,rk3308-usb', 'rockchip,rk3328-usb', 'rockchip,rk3368-usb', 'rockchip,rv1108-usb']
>>> 'lantiq,arx100-usb' was expected
>>> 'lantiq,xrx200-usb' was expected
>>> 'ingenic,jz4780-otg' is not one of ['amlogic,meson8-usb', 'amlogic,meson8b-usb', 'amlogic,meson-gxbb-usb', 'amlogic,meson-g12a-usb', 'intel,socfpga-agilex-hsotg']
>>> 'amcc,dwc-otg' was expected
>>> 'apm,apm82181-dwc-otg' was expected
>>> 'snps,dwc2' was expected
>>> 'st,stm32f4x9-fsotg' was expected
>>> 'st,stm32f4x9-hsotg' was expected
>>> 'st,stm32f7-hsotg' was expected
>>> 'st,stm32mp15-fsotg' was expected
>>> 'st,stm32mp15-hsotg' was expected
>>> 'samsung,s3c6400-hsotg' was expected
>>> 'intel,socfpga-agilex-hsotg' was expected
>> You really don't need to paste entire warning.
>>> From schema: Documentation/devicetree/bindings/usb/dwc2.yaml
>>> Signed-off-by: H. Nikolaus Schaller <[email protected]>
>>> ---
>>> arch/mips/boot/dts/ingenic/jz4780.dtsi | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>>> index 2021836983c96..c5124459678b7 100644
>>> --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
>>> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>>> @@ -576,7 +576,7 @@ bch: bch@134d0000 {
>>> };
>>> otg: usb@13500000 {
>>> - compatible = "ingenic,jz4780-otg", "snps,dwc2";
>>> + compatible = "snps,dwc2";
>> This looks wrong, the block usually should have a specific compatible.
>> Please mention why it does not.
>
> Agreed. The "snps,dwc2" should be a fallback string, otherwise there is no way to uniquely identify the JZ4780 implementation of the IP.
Well, there is no specifc implementation and driver for it. So no need to uniquely identify it.
BR and thanks,
Nikolaus
> Am 09.04.2022 um 15:13 schrieb Krzysztof Kozlowski <[email protected]>:
>
> On 09/04/2022 15:04, H. Nikolaus Schaller wrote:
>>
>>
>>> Am 09.04.2022 um 13:13 schrieb Krzysztof Kozlowski <[email protected]>:
>>>
>>> On 08/04/2022 20:37, H. Nikolaus Schaller wrote:
>>>> arch/mips/boot/dts/ingenic/ci20.dtb: pin-controller@10010000: $nodename:0: 'pin-controller@10010000' does not match '^(pinctrl|pinmux)(@[0-9a-f]+)?$'
>>>> From schema: Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.yaml
>>>>
>>>> Signed-off-by: H. Nikolaus Schaller <[email protected]>
>>>> ---
>>>> arch/mips/boot/dts/ingenic/jz4780.dtsi | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>>>> index 5f44cf004d473..b5299eaffb84a 100644
>>>> --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
>>>> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>>>> @@ -155,7 +155,7 @@ rtc_dev: rtc@10003000 {
>>>> clock-names = "rtc";
>>>> };
>>>>
>>>> - pinctrl: pin-controller@10010000 {
>>>> + pinctrl: pinctrl@10010000 {
>>>
>>> Do it once for all DTSes, not one file at a time. There are four more
>>> places with this.
>>
>> Well, automation has no notion of "similarity" in this case to
>> merge several patches.
>
> What does that mean? One cannot create multiple patches and apply them?
This patch set was created by some automatic scripts. And they produce one patch
per group of warnings.
But here you ask me to merge 4 unrelated topics into a single one.
Or do you mean something else?
>
>> And they are not related. Every one is based on a different .yaml
>> schema file.
>
> Which does not matter, because the name of the node does not matter. We
> enforce it in schema to makes things organized and easier in testing.
> This does not fix any real problem, just the problem we created by
> ourselves with schema.
>
>>
>> That in all cases the result looks similar comes from similar
>> requirements by the schemata and has no inherent connection.
>
> All schemas will require it, won't they? The same for arm...
We may be talking about different things here.
My understanding:
you ask me to merge 5/18, 8/18, 9/18, 12/18 because they contain "controller" in the node-name.
Right? If not then we must clarify that first.
For pinctrl it is not allowed to have a -controller suffix while for the other it is mandatory
by the schema pattern.
BR and thanks,
Nikolaus
> Am 09.04.2022 um 15:16 schrieb Krzysztof Kozlowski <[email protected]>:
>
> On 09/04/2022 15:07, H. Nikolaus Schaller wrote:
>>
>>
>>> Am 09.04.2022 um 13:18 schrieb Krzysztof Kozlowski <[email protected]>:
>>>
>>> On 08/04/2022 20:37, H. Nikolaus Schaller wrote:
>>>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10030000: 'dmas' is a required property
>>>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml
>>>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10030000: 'dma-names' is a required property
>>>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml
>>>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10031000: 'dmas' is a required property
>>>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml
>>>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10031000: 'dma-names' is a required property
>>>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml
>>>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10032000: 'dmas' is a required property
>>>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml
>>>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10032000: 'dma-names' is a required property
>>>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml
>>>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10033000: 'dmas' is a required property
>>>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml
>>>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10033000: 'dma-names' is a required property
>>>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml
>>>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10034000: 'dmas' is a required property
>>>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml
>>>> arch/mips/boot/dts/ingenic/ci20.dtb: serial@10034000: 'dma-names' is a required property
>>>> From schema: Documentation/devicetree/bindings/serial/ingenic,uart.yaml
>>>> arch/mips/boot/dts/ingenic/ci20.dtb: i2c@10050000: 'dmas' is a required property
>>>
>>> All these warnings are the same two warnings...
>>
>> See my earlier explanation that without them you can't verify by just reading commit message
>> and diff that all existing warnings have been addressed.
>
> Which does not make sense and there is no need... Automation does it
> (see Rob's tools). Don't make human life more difficult...
Ok, you are right. If you apply this patch and then run dtbscheck again, there would be
a warning left over.
But may I honestly ask why you review the commits and read the commit message at all?
You could simply ignore it... And it would be easier for both of us to leave it completely
to Rob's tools :)
BR and thanks,
Nikolaus
On 08/04/2022 20:37, H. Nikolaus Schaller wrote:
> /Volumes/CaseSensitive/master/arch/mips/boot/dts/ingenic/ci20.dtb: dma@13420000: $nodename:0: 'dma@13420000' does not match '^dma-controller(@.*)?$'
> From schema: /Volumes/CaseSensitive/master/Documentation/devicetree/bindings/dma/ingenic,dma.yaml
>
> Signed-off-by: H. Nikolaus Schaller <[email protected]>
> ---
> arch/mips/boot/dts/ingenic/jz4780.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> index 8b95486c8afa7..dc88f9e813453 100644
> --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> @@ -515,7 +515,7 @@ eth0_addr: eth-mac-addr@22 {
> };
> };
>
> - dma: dma@13420000 {
> + dma: dma-controller@13420000 {
Fix all the files.
Best regards,
Krzysztof
Le sam., avril 9 2022 at 14:47:23 +0200, Krzysztof Kozlowski
<[email protected]> a ?crit :
> On 09/04/2022 14:37, Paul Cercueil wrote:
>>> The true question is whether you need simple-mfd. Isn't the binding
>>> (and
>>> the driver) expected to instantiate its children?
>>
>> I can explain that one. There is the EFUSE controller located inside
>> the nemc's memory area, and the two are pretty much unrelated, hence
>> the "simple-mfd" compatible string.
>
> I saw the efuse children and that's why I asked who is expected to
> populate them. You said that simple-mfd is required for this, I say
> no.
> It should work without simple-mfd...
>
> I am kind of repeating myself but I really do not see the need of
> simple-mfd in the bindings.
Well, it is a "simple MFD", so I don't see why we can't use the
"simple-mfd" compatible. Why would we not want to use it?
Besides, if the nemc driver is responsible for populating the efuse
device, that means the nemc driver must be enabled for the efuse to
work, which is nonsense, the two IP blocks being unrelated.
-Paul
arch/mips/boot/dts/ingenic/ci20.dtb: rtc@10003000: compatible: 'oneOf' conditional failed, one must be fixed:
['ingenic,jz4780-rtc'] is too short
'ingenic,jz4780-rtc' is not one of ['ingenic,jz4740-rtc', 'ingenic,jz4760-rtc']
'ingenic,jz4725b-rtc' was expected
From schema: Documentation/devicetree/bindings/rtc/ingenic,rtc.yaml
Signed-off-by: H. Nikolaus Schaller <[email protected]>
---
arch/mips/boot/dts/ingenic/jz4780.dtsi | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index b5299eaffb84a..2021836983c96 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -145,7 +145,8 @@ ost: timer@e0 {
};
rtc_dev: rtc@10003000 {
- compatible = "ingenic,jz4780-rtc";
+ compatible = "ingenic,jz4780-rtc",
+ "ingenic,jz4760-rtc";
reg = <0x10003000 0x4c>;
interrupt-parent = <&intc>;
--
2.33.0
> Am 09.04.2022 um 16:00 schrieb Krzysztof Kozlowski <[email protected]>:
>
> On 09/04/2022 15:57, H. Nikolaus Schaller wrote:
>>
>>
>>> Am 09.04.2022 um 15:46 schrieb Krzysztof Kozlowski <[email protected]>:
>>>
>>> On 09/04/2022 15:41, H. Nikolaus Schaller wrote:
>>>>>
>>>>> No. I ask you to fix all pin-controller cases, for entire MIPS, not just
>>>>> one.
>>>>
>>>> Oops. Nope. I am a volunteer and neither your employee nor slave.
>>>
>>> No one thinks differently and I am sorry that you felt it. Please accept
>>> my apologies, if you get different impression. You understand though the
>>> meaning of word "ask for something" and "order something" (the latter
>>> which I did not use).
>>>
>>> I just asked.
>>
>> Ok. Maybe english is not our mother language and we sometimes don't
>> get the nuances right. Sorry if I understood that wrongly.
>>
>> At least I now understand what you did suggest.
>
> Yeah, probably I did not express my thoughts correctly.
>
> I would like to state that I appreciate your work and I think it is
> important, even if I do not express it correctly. Please accept my
> apologies if I am bit harsh or impolite. That's not my intention.
I also was a little harsh in my response. Sorry again.
>
>>
>> Doing the same change for treewide MIPS is beyond my capabilities since
>> I can't easily test any compile setup. So far I only compile for CI20 and
>> as far as I know every machine still needs its own config for MIPS
>> (haven't checked recently). So I am not even sure if dtbscheck tells me
>> all locations.
>
>
> OK, fair enough.
>
>
> Best regards,
> Krzysztof
Anyways thank you very much for your reviews and sharing comments.
As far as I see there are only the jz4780-nemc/simple-mfd and the snps,dwc2/jz4780-otg
issues really unsolved while the others are more cosmetics.
Let's see how these settle.
BR and thanks,
Nikolaus
On 09/04/2022 15:18, Krzysztof Kozlowski wrote:
> On 09/04/2022 15:09, H. Nikolaus Schaller wrote:
(...)
>>>> @@ -17,7 +17,7 @@ properties:
>>>> oneOf:
>>>> - enum:
>>>> -
>>>> - - ingenic,jz4780-nemc
>>>> + - [ , simple-mfd ]
>>>
>>> This is not correct representation. If you really need simple-mfd, then
>>> this should be a separate item below oneOf.
>>
>> Well, it is valid YAML syntax and seems to be accepted by dtbscheck.
Minor update:
Well, it is not a valid schema. Rob's checker now confirmed. If you run
dt_bindings_check by yourself you will see the error:
properties:compatible:oneOf:0:enum:1: ['ingenic', 'jz4780-nemc',
'simple-mfd'] is not of type 'string'
Probably because enum expects string, not another enum (so enum inside
enum is not correct).
If you do not see the error, you might be missing some packages
(mentioned in writing-schema + yamllint for a different issue) or your
dtschema is old.
>
> It's not how we code it. Please do not introduce inconsistent - even if
> valid - blocks.
Best regards,
Krzysztof
Hi folks,
On 2022/4/9 下午9:53, H. Nikolaus Schaller wrote:
>
>> Am 09.04.2022 um 15:44 schrieb Krzysztof Kozlowski <[email protected]>:
>>
>> On 09/04/2022 15:32, H. Nikolaus Schaller wrote:
>>>
>>>> Am 09.04.2022 um 15:15 schrieb Krzysztof Kozlowski <[email protected]>:
>>>>
>>>> On 09/04/2022 15:05, H. Nikolaus Schaller wrote:
>>>>>> This looks wrong, the block usually should have a specific compatible.
>>>>>> Please mention why it does not.
>>>>> Well, I did not even have that idea that it could need an explanation.
>>>>>
>>>>> There is no "ingenic,jz4780-otg" and none is needed here to make it work.
>>>> Make it work in what terms? We talk about hardware description, right?
>>> Yes.
>>>
>>>>> Therefore the generic "snps,dwc2" is sufficient.
>>>> No, you are mixing now driver behavior (is sufficient) with hardware
>>>> description.
>>> No. "snps,dwc2" is a hardware description for a licensed block.
>>> Not a driver behavior.
>> snps,dwc2 matches the original block, not necessarily this
>> implementation. Unless you are sure?
> I assume. Nobody has reported an issue without having any specific jz4780 driver in place.
> Well, that is only evidence, not bullet proof.
>
>>>> Most of licensed blocks require the specific compatible to
>>>> differentiate it.
>>> If there is a need to differentiate.
>> No, regardless whether there is a need currently, most of them have
>> specific compatibles, because there are some minor differences. Even if
>> difference is not visible from programming model or wiring, it might
>> justify it's own specific compatible. For example because maybe once
>> that tiny difference will require some changes.
>>
>> Someone added the ingenic compatible, so why do you assume that one tool
>> (bindings) is correct but other piece of code (using specific
>> compatible) is not? You use the argument "bindings warning" which is not
>> enough. Argument that blocks are 100% same, is good enough, if you are
>> sure. Just use it in commit msg. But are you sure that these are the
>> same? Same pins, same programming model (entire model, not used by Linux)?
> The compatible ingenic,jz4780-otg was introduced in 158c774d3c64859e84dd20e04d5fb18c8d3d318e.
> Hence I have added Yanjie for clarification why he added it in the .dts and not in the bindings.
It's my fault, last year I made an OTG driver for Ingenic SoCs and sent it
to the mailing list, and then I received some revision comments, but for
some personal reasons I didn't continue to improve it.
I'll finish these modifications as soon as possible and send them out.
Then after they merge into the mainline, this problem will be solved.
Thanks and best regards!
>
> BR and thanks,
> Nikolaus
>
On 08/04/2022 20:37, H. Nikolaus Schaller wrote:
> /Volumes/CaseSensitive/master/arch/mips/boot/dts/ingenic/ci20.dtb: lcdc0@13050000: $nodename:0: 'lcdc0@13050000' does not match '^lcd-controller@[0-9a-f]+$'
> From schema: /Volumes/CaseSensitive/master/Documentation/devicetree/bindings/display/ingenic,lcd.yaml
> /Volumes/CaseSensitive/master/arch/mips/boot/dts/ingenic/ci20.dtb: lcdc0@13050000: clock-names:0: 'lcd_pclk' was expected
> From schema: /Volumes/CaseSensitive/master/Documentation/devicetree/bindings/display/ingenic,lcd.yaml
> /Volumes/CaseSensitive/master/arch/mips/boot/dts/ingenic/ci20.dtb: lcdc0@13050000: clock-names:1: 'lcd' was expected
> From schema: /Volumes/CaseSensitive/master/Documentation/devicetree/bindings/display/ingenic,lcd.yaml
It's enough, no need to have two same warnings, so all duplicated
messages below can be removed.
With that change:
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
arch/mips/boot/dts/ingenic/ci20.dtb: jz4780-cgu@10000000: $nodename:0: 'jz4780-cgu@10000000' does not match '^clock-controller@[0-9a-f]+$'
From schema: Documentation/devicetree/bindings/clock/ingenic,cgu.yaml
Signed-off-by: H. Nikolaus Schaller <[email protected]>
---
arch/mips/boot/dts/ingenic/jz4780.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 710605df40ed3..6fb6229c3186b 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -58,7 +58,7 @@ rtc: rtc {
clock-frequency = <32768>;
};
- cgu: jz4780-cgu@10000000 {
+ cgu: clock-controller@10000000 {
compatible = "ingenic,jz4780-cgu", "simple-mfd";
reg = <0x10000000 0x100>;
#address-cells = <1>;
--
2.33.0
On 09/04/2022 14:37, Paul Cercueil wrote:
>> The true question is whether you need simple-mfd. Isn't the binding
>> (and
>> the driver) expected to instantiate its children?
>
> I can explain that one. There is the EFUSE controller located inside
> the nemc's memory area, and the two are pretty much unrelated, hence
> the "simple-mfd" compatible string.
I saw the efuse children and that's why I asked who is expected to
populate them. You said that simple-mfd is required for this, I say no.
It should work without simple-mfd...
I am kind of repeating myself but I really do not see the need of
simple-mfd in the bindings.
Best regards,
Krzysztof
Hi,
> Am 10.04.2022 um 18:32 schrieb Zhou Yanjie <[email protected]>:
>
> Hi folks,
>
> On 2022/4/9 下午9:53, H. Nikolaus Schaller wrote:
>>
>>> Am 09.04.2022 um 15:44 schrieb Krzysztof Kozlowski <[email protected]>:
>>>
>>> On 09/04/2022 15:32, H. Nikolaus Schaller wrote:
>>>>
>>>>> Am 09.04.2022 um 15:15 schrieb Krzysztof Kozlowski <[email protected]>:
>>>>>
>>>>> On 09/04/2022 15:05, H. Nikolaus Schaller wrote:
>>>>>>> This looks wrong, the block usually should have a specific compatible.
>>>>>>> Please mention why it does not.
>>>>>> Well, I did not even have that idea that it could need an explanation.
>>>>>>
>>>>>> There is no "ingenic,jz4780-otg" and none is needed here to make it work.
>>>>> Make it work in what terms? We talk about hardware description, right?
>>>> Yes.
>>>>
>>>>>> Therefore the generic "snps,dwc2" is sufficient.
>>>>> No, you are mixing now driver behavior (is sufficient) with hardware
>>>>> description.
>>>> No. "snps,dwc2" is a hardware description for a licensed block.
>>>> Not a driver behavior.
>>> snps,dwc2 matches the original block, not necessarily this
>>> implementation. Unless you are sure?
>> I assume. Nobody has reported an issue without having any specific jz4780 driver in place.
>> Well, that is only evidence, not bullet proof.
>>
>>>>> Most of licensed blocks require the specific compatible to
>>>>> differentiate it.
>>>> If there is a need to differentiate.
>>> No, regardless whether there is a need currently, most of them have
>>> specific compatibles, because there are some minor differences. Even if
>>> difference is not visible from programming model or wiring, it might
>>> justify it's own specific compatible. For example because maybe once
>>> that tiny difference will require some changes.
>>>
>>> Someone added the ingenic compatible, so why do you assume that one tool
>>> (bindings) is correct but other piece of code (using specific
>>> compatible) is not? You use the argument "bindings warning" which is not
>>> enough. Argument that blocks are 100% same, is good enough, if you are
>>> sure. Just use it in commit msg. But are you sure that these are the
>>> same? Same pins, same programming model (entire model, not used by Linux)?
>> The compatible ingenic,jz4780-otg was introduced in 158c774d3c64859e84dd20e04d5fb18c8d3d318e.
>> Hence I have added Yanjie for clarification why he added it in the .dts and not in the bindings.
>
>
> It's my fault, last year I made an OTG driver for Ingenic SoCs and sent it
> to the mailing list, and then I received some revision comments, but for
> some personal reasons I didn't continue to improve it.
>
> I'll finish these modifications as soon as possible and send them out.
> Then after they merge into the mainline, this problem will be solved.
No need to apologize.
If you agree I can add "ingenic,jz4780-otg" to the schema file and keep
the .dts in the v2 of my series.
And I'll add you to the list of reviewers, so you can please comment v2
if it is correct or if we are still missing something.
Best regards and thanks,
Nikolaus
On 09/04/2022 15:22, H. Nikolaus Schaller wrote:
>>
>> What does that mean? One cannot create multiple patches and apply them?
>
> This patch set was created by some automatic scripts. And they produce one patch
> per group of warnings.
>
> But here you ask me to merge 4 unrelated topics into a single one.
>
> Or do you mean something else?
You can edit a commit, right? git commit --amend? So where is the problem?
>
>>
>>> And they are not related. Every one is based on a different .yaml
>>> schema file.
>>
>> Which does not matter, because the name of the node does not matter. We
>> enforce it in schema to makes things organized and easier in testing.
>> This does not fix any real problem, just the problem we created by
>> ourselves with schema.
>>
>>>
>>> That in all cases the result looks similar comes from similar
>>> requirements by the schemata and has no inherent connection.
>>
>> All schemas will require it, won't they? The same for arm...
>
> We may be talking about different things here.
>
> My understanding:
> you ask me to merge 5/18, 8/18, 9/18, 12/18 because they contain "controller" in the node-name.
>
> Right? If not then we must clarify that first.
No. I ask you to fix all pin-controller cases, for entire MIPS, not just
one.
And in one month one more. And then again one more.
Best regards,
Krzysztof
On 08/04/2022 20:37, H. Nikolaus Schaller wrote:
> arch/mips/boot/dts/ingenic/ci20.dtb: timer@10002000: timer@e0:compatible: 'oneOf' conditional failed, one must be fixed:
> ['ingenic,jz4780-ost', 'ingenic,jz4770-ost'] is too long
> 'ingenic,jz4780-ost' is not one of ['ingenic,jz4725b-ost', 'ingenic,jz4760b-ost']
> 'ingenic,jz4760-ost' was expected
> 'ingenic,jz4725b-ost' was expected
> 'ingenic,jz4760b-ost' was expected
> From schema: Documentation/devicetree/bindings/timer/ingenic,tcu.yaml
>
> Signed-off-by: H. Nikolaus Schaller <[email protected]>
> ---
> arch/mips/boot/dts/ingenic/jz4780.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
The same as patch 3 - needs explanation.
Best regards,
Krzysztof
On 08/04/2022 20:37, H. Nikolaus Schaller wrote:
> arch/mips/boot/dts/ingenic/ci20.dtb: pin-controller@10010000: $nodename:0: 'pin-controller@10010000' does not match '^(pinctrl|pinmux)(@[0-9a-f]+)?$'
> From schema: Documentation/devicetree/bindings/pinctrl/ingenic,pinctrl.yaml
>
> Signed-off-by: H. Nikolaus Schaller <[email protected]>
> ---
> arch/mips/boot/dts/ingenic/jz4780.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> index 5f44cf004d473..b5299eaffb84a 100644
> --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> @@ -155,7 +155,7 @@ rtc_dev: rtc@10003000 {
> clock-names = "rtc";
> };
>
> - pinctrl: pin-controller@10010000 {
> + pinctrl: pinctrl@10010000 {
Do it once for all DTSes, not one file at a time. There are four more
places with this.
Best regards,
Krzysztof
> Am 09.04.2022 um 15:24 schrieb Krzysztof Kozlowski <[email protected]>:
>
> On 09/04/2022 15:22, H. Nikolaus Schaller wrote:
>>>
>>> What does that mean? One cannot create multiple patches and apply them?
>>
>> This patch set was created by some automatic scripts. And they produce one patch
>> per group of warnings.
>>
>> But here you ask me to merge 4 unrelated topics into a single one.
>>
>> Or do you mean something else?
>
> You can edit a commit, right? git commit --amend? So where is the problem?
It is not about capabilites...
It is about understanding why you want it and what you expect.
>
>>
>>>
>>>> And they are not related. Every one is based on a different .yaml
>>>> schema file.
>>>
>>> Which does not matter, because the name of the node does not matter. We
>>> enforce it in schema to makes things organized and easier in testing.
>>> This does not fix any real problem, just the problem we created by
>>> ourselves with schema.
>>>
>>>>
>>>> That in all cases the result looks similar comes from similar
>>>> requirements by the schemata and has no inherent connection.
>>>
>>> All schemas will require it, won't they? The same for arm...
>>
>> We may be talking about different things here.
>>
>> My understanding:
>> you ask me to merge 5/18, 8/18, 9/18, 12/18 because they contain "controller" in the node-name.
>>
>> Right? If not then we must clarify that first.
>
> No. I ask you to fix all pin-controller cases, for entire MIPS, not just
> one.
Oops. Nope. I am a volunteer and neither your employee nor slave.
> And in one month one more. And then again one more.
No. I work for the topics I choose to work on and share the results.
Open source lives from taking and giving...
If you want me to contribute, please be not demanding.
BR and thanks,
Nikolaus
On 09/04/2022 15:57, H. Nikolaus Schaller wrote:
>
>
>> Am 09.04.2022 um 15:46 schrieb Krzysztof Kozlowski <[email protected]>:
>>
>> On 09/04/2022 15:41, H. Nikolaus Schaller wrote:
>>>>
>>>> No. I ask you to fix all pin-controller cases, for entire MIPS, not just
>>>> one.
>>>
>>> Oops. Nope. I am a volunteer and neither your employee nor slave.
>>
>> No one thinks differently and I am sorry that you felt it. Please accept
>> my apologies, if you get different impression. You understand though the
>> meaning of word "ask for something" and "order something" (the latter
>> which I did not use).
>>
>> I just asked.
>
> Ok. Maybe english is not our mother language and we sometimes don't
> get the nuances right. Sorry if I understood that wrongly.
>
> At least I now understand what you did suggest.
Yeah, probably I did not express my thoughts correctly.
I would like to state that I appreciate your work and I think it is
important, even if I do not express it correctly. Please accept my
apologies if I am bit harsh or impolite. That's not my intention.
>
> Doing the same change for treewide MIPS is beyond my capabilities since
> I can't easily test any compile setup. So far I only compile for CI20 and
> as far as I know every machine still needs its own config for MIPS
> (haven't checked recently). So I am not even sure if dtbscheck tells me
> all locations.
OK, fair enough.
Best regards,
Krzysztof
arch/mips/boot/dts/ingenic/ci20.dtb: bluetooth: vcc-supply does not match any of the regexes: pinctrl-[0-9]+
From schema: Documentation/devicetree/bindings/net/broadcom-bluetooth.yaml
Signed-off-by: H. Nikolaus Schaller <[email protected]>
---
arch/mips/boot/dts/ingenic/ci20.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
index dc587b4b36009..8a120f9374331 100644
--- a/arch/mips/boot/dts/ingenic/ci20.dts
+++ b/arch/mips/boot/dts/ingenic/ci20.dts
@@ -207,7 +207,7 @@ &uart2 {
bluetooth {
compatible = "brcm,bcm4330-bt";
reset-gpios = <&gpf 8 GPIO_ACTIVE_HIGH>;
- vcc-supply = <&wlan0_power>;
+ vbat-supply = <&wlan0_power>;
device-wakeup-gpios = <&gpf 5 GPIO_ACTIVE_HIGH>;
host-wakeup-gpios = <&gpf 6 GPIO_ACTIVE_HIGH>;
shutdown-gpios = <&gpf 4 GPIO_ACTIVE_LOW>;
--
2.33.0
On Fri, 08 Apr 2022 20:37:56 +0200, H. Nikolaus Schaller wrote:
> jz4780-nemc needs to be compatible to simple-mfd as well or we get
>
> arch/mips/boot/dts/ingenic/ci20.dtb: memory-controller@13410000: compatible: 'oneOf' conditional failed, one must be fixed:
> ['ingenic,jz4780-nemc', 'simple-mfd'] is too long
> 'ingenic,jz4725b-nemc' was expected
> 'ingenic,jz4740-nemc' was expected
> From schema: Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
>
> Signed-off-by: H. Nikolaus Schaller <[email protected]>
> ---
> .../devicetree/bindings/memory-controllers/ingenic,nemc.yaml | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
./Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml:20:23: [warning] too few spaces after comma (commas)
dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml: properties:compatible:oneOf:0:enum:1: ['ingenic', 'jz4780-nemc', 'simple-mfd'] is not of type 'string'
from schema $id: http://devicetree.org/meta-schemas/string-array.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml: ignoring, error in schema: properties: compatible: oneOf: 0: enum: 1
Documentation/devicetree/bindings/mtd/ingenic,nand.example.dtb:0:0: /example-0/memory-controller@13410000: failed to match any schema with compatible: ['ingenic,jz4780-nemc']
Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.example.dtb:0:0: /example-0/memory-controller@13410000: failed to match any schema with compatible: ['ingenic,jz4780-nemc']
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/patch/
This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit.
> Am 09.04.2022 um 13:26 schrieb Krzysztof Kozlowski <[email protected]>:
>
> On 08/04/2022 20:37, H. Nikolaus Schaller wrote:
>> jz4780-nemc needs to be compatible to simple-mfd as well or we get
>>
>> arch/mips/boot/dts/ingenic/ci20.dtb: memory-controller@13410000: compatible: 'oneOf' conditional failed, one must be fixed:
>> ['ingenic,jz4780-nemc', 'simple-mfd'] is too long
>> 'ingenic,jz4725b-nemc' was expected
>> 'ingenic,jz4740-nemc' was expected
>> From schema: Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
>>
>> Signed-off-by: H. Nikolaus Schaller <[email protected]>
>> ---
>> .../devicetree/bindings/memory-controllers/ingenic,nemc.yaml | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml b/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
>> index 24f9e19820282..3b1116588de3d 100644
>> --- a/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
>> +++ b/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
>> @@ -17,7 +17,7 @@ properties:
>> oneOf:
>> - enum:
>> -
>> - - ingenic,jz4780-nemc
>> + - [ , simple-mfd ]
>
> This is not correct representation. If you really need simple-mfd, then
> this should be a separate item below oneOf.
Well, it is valid YAML syntax and seems to be accepted by dtbscheck.
> The true question is whether you need simple-mfd. Isn't the binding (and
> the driver) expected to instantiate its children?
I had expected that but current ingenic,jz4780-nemc code doesn't.
Everything is explained in 190607f2d59e174bcf8415efb1bb390737f8d428
But if someone writes a fix for the ingenic,jz4740-nemc driever
we can of course live without this patch. And partially revert
190607f2d59e174bcf8415efb1bb390737f8d428
arch/mips/boot/dts/ingenic/ci20.dtb: nemc@13410000: $nodename:0: 'nemc@13410000' does not match '^memory-controller@[0-9a-f]+$'
From schema: Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
Signed-off-by: H. Nikolaus Schaller <[email protected]>
---
arch/mips/boot/dts/ingenic/jz4780.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index dcb93d0158856..cf259dac519ad 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -523,7 +523,7 @@ lcdc1: lcd-controller@130a0000 {
status = "disabled";
};
- nemc: nemc@13410000 {
+ nemc: memory-controller@13410000 {
compatible = "ingenic,jz4780-nemc", "simple-mfd";
reg = <0x13410000 0x10000>;
#address-cells = <2>;
--
2.33.0
On 09/04/2022 14:55, Paul Cercueil wrote:
>>
>> I saw the efuse children and that's why I asked who is expected to
>> populate them. You said that simple-mfd is required for this, I say
>> no.
>> It should work without simple-mfd...
>>
>> I am kind of repeating myself but I really do not see the need of
>> simple-mfd in the bindings.
>
> Well, it is a "simple MFD",
It's not a simple MFD, it is a memory controller. MFD is a purely
Linux/software term, so there are no devices which are MFD. Everything
which we model as MFD is actually something else in real life (e.g.
PMIC, memory controller, system controller).
> so I don't see why we can't use the
> "simple-mfd" compatible. Why would we not want to use it?
No one said that you cannot. You just might not need...
>
> Besides, if the nemc driver is responsible for populating the efuse
> device, that means the nemc driver must be enabled for the efuse to
> work, which is nonsense, the two IP blocks being unrelated.
That's actually the explanation I was looking for. It would be nice to
use it in commit msg instead of the dtbs_check warning.
Best regards,
Krzysztof
> Am 09.04.2022 um 15:28 schrieb Krzysztof Kozlowski <[email protected]>:
>
> On 09/04/2022 15:26, H. Nikolaus Schaller wrote:
>>>
>>> Which does not make sense and there is no need... Automation does it
>>> (see Rob's tools). Don't make human life more difficult...
>>
>> Ok, you are right. If you apply this patch and then run dtbscheck again, there would be
>> a warning left over.
>>
>> But may I honestly ask why you review the commits and read the commit message at all?
>> You could simply ignore it... And it would be easier for both of us to leave it completely
>> to Rob's tools :)
>>
>
> I am not reading it. :) It takes more effort to scroll to the actual
> contents.
Ok, now I got it...
Maybe I have a larger screen so that it doesn't even need scrolling and therefore don't notice this difference.
So may I leave it as it is since you don't read it anyways :)
>
> Best regards,
> Krzysztof
BR and thanks,
Nikolaus
On 09/04/2022 15:26, H. Nikolaus Schaller wrote:
>>
>> Which does not make sense and there is no need... Automation does it
>> (see Rob's tools). Don't make human life more difficult...
>
> Ok, you are right. If you apply this patch and then run dtbscheck again, there would be
> a warning left over.
>
> But may I honestly ask why you review the commits and read the commit message at all?
> You could simply ignore it... And it would be easier for both of us to leave it completely
> to Rob's tools :)
>
I am not reading it. :) It takes more effort to scroll to the actual
contents.
Best regards,
Krzysztof
Hi,
Le sam., avril 9 2022 at 13:15:37 +0200, Krzysztof Kozlowski
<[email protected]> a ?crit :
> On 08/04/2022 20:37, H. Nikolaus Schaller wrote:
>> arch/mips/boot/dts/ingenic/ci20.dtb: usb@13500000: compatible:
>> 'oneOf' conditional failed, one must be fixed:
>> ['ingenic,jz4780-otg', 'snps,dwc2'] is too long
>> ['ingenic,jz4780-otg', 'snps,dwc2'] is too short
>> 'brcm,bcm2835-usb' was expected
>> 'hisilicon,hi6220-usb' was expected
>> 'rockchip,rk3066-usb' was expected
>> 'ingenic,jz4780-otg' is not one of ['rockchip,px30-usb',
>> 'rockchip,rk3036-usb', 'rockchip,rk3188-usb', 'rockchip,rk3228-usb',
>> 'rockchip,rk3288-usb', 'rockchip,rk3308-usb', 'rockchip,rk3328-usb',
>> 'rockchip,rk3368-usb', 'rockchip,rv1108-usb']
>> 'lantiq,arx100-usb' was expected
>> 'lantiq,xrx200-usb' was expected
>> 'ingenic,jz4780-otg' is not one of ['amlogic,meson8-usb',
>> 'amlogic,meson8b-usb', 'amlogic,meson-gxbb-usb',
>> 'amlogic,meson-g12a-usb', 'intel,socfpga-agilex-hsotg']
>> 'amcc,dwc-otg' was expected
>> 'apm,apm82181-dwc-otg' was expected
>> 'snps,dwc2' was expected
>> 'st,stm32f4x9-fsotg' was expected
>> 'st,stm32f4x9-hsotg' was expected
>> 'st,stm32f7-hsotg' was expected
>> 'st,stm32mp15-fsotg' was expected
>> 'st,stm32mp15-hsotg' was expected
>> 'samsung,s3c6400-hsotg' was expected
>> 'intel,socfpga-agilex-hsotg' was expected
>
> You really don't need to paste entire warning.
>
>
>> From schema: Documentation/devicetree/bindings/usb/dwc2.yaml
>>
>> Signed-off-by: H. Nikolaus Schaller <[email protected]>
>> ---
>> arch/mips/boot/dts/ingenic/jz4780.dtsi | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> index 2021836983c96..c5124459678b7 100644
>> --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> @@ -576,7 +576,7 @@ bch: bch@134d0000 {
>> };
>>
>> otg: usb@13500000 {
>> - compatible = "ingenic,jz4780-otg", "snps,dwc2";
>> + compatible = "snps,dwc2";
>
> This looks wrong, the block usually should have a specific compatible.
> Please mention why it does not.
Agreed. The "snps,dwc2" should be a fallback string, otherwise there is
no way to uniquely identify the JZ4780 implementation of the IP.
Cheers,
-Paul
On 08/04/2022 20:37, H. Nikolaus Schaller wrote:
> arch/mips/boot/dts/ingenic/ci20.dtb: nemc@13410000: $nodename:0: 'nemc@13410000' does not match '^memory-controller@[0-9a-f]+$'
> From schema: Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
>
> Signed-off-by: H. Nikolaus Schaller <[email protected]>
> ---
> arch/mips/boot/dts/ingenic/jz4780.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 08/04/2022 20:38, H. Nikolaus Schaller wrote:
> arch/mips/boot/dts/ingenic/ci20.dtb: wifi@1: compatible: oneOf conditional failed, one must be fixed:
> [brcm,bcm4330-fmac] is too short
> brcm,bcm4329-fmac was expected
> From schema: Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml
>
> Signed-off-by: H. Nikolaus Schaller <[email protected]>
> ---
> arch/mips/boot/dts/ingenic/ci20.dts | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
Hi Krzysztof,
Le sam., avril 9 2022 at 13:26:25 +0200, Krzysztof Kozlowski
<[email protected]> a ?crit :
> On 08/04/2022 20:37, H. Nikolaus Schaller wrote:
>> jz4780-nemc needs to be compatible to simple-mfd as well or we get
>>
>> arch/mips/boot/dts/ingenic/ci20.dtb: memory-controller@13410000:
>> compatible: 'oneOf' conditional failed, one must be fixed:
>> ['ingenic,jz4780-nemc', 'simple-mfd'] is too long
>> 'ingenic,jz4725b-nemc' was expected
>> 'ingenic,jz4740-nemc' was expected
>> From schema:
>> Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
>>
>> Signed-off-by: H. Nikolaus Schaller <[email protected]>
>> ---
>> .../devicetree/bindings/memory-controllers/ingenic,nemc.yaml |
>> 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
>> b/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
>> index 24f9e19820282..3b1116588de3d 100644
>> ---
>> a/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
>> +++
>> b/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
>> @@ -17,7 +17,7 @@ properties:
>> oneOf:
>> - enum:
>> - ingenic,jz4740-nemc
>> - - ingenic,jz4780-nemc
>> + - [ ingenic,jz4780-nemc, simple-mfd ]
>
> This is not correct representation. If you really need simple-mfd,
> then
> this should be a separate item below oneOf.
Correct.
> The true question is whether you need simple-mfd. Isn't the binding
> (and
> the driver) expected to instantiate its children?
I can explain that one. There is the EFUSE controller located inside
the nemc's memory area, and the two are pretty much unrelated, hence
the "simple-mfd" compatible string.
Cheers,
-Paul
arch/mips/boot/dts/ingenic/ci20.dtb: timer@10002000: compatible: 'oneOf' conditional failed, one must be fixed:
['ingenic,jz4780-tcu', 'ingenic,jz4770-tcu', 'simple-mfd'] is too long
'ingenic,jz4780-tcu' is not one of ['ingenic,jz4740-tcu', 'ingenic,jz4725b-tcu', 'ingenic,jz4760-tcu', 'ingenic,x1000-tcu']
'simple-mfd' was expected
'ingenic,jz4760-tcu' was expected
From schema: Documentation/devicetree/bindings/timer/ingenic,tcu.yaml
Signed-off-by: H. Nikolaus Schaller <[email protected]>
---
arch/mips/boot/dts/ingenic/jz4780.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 6fb6229c3186b..6027f14c393e3 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -91,7 +91,7 @@ rng: rng@d8 {
tcu: timer@10002000 {
compatible = "ingenic,jz4780-tcu",
- "ingenic,jz4770-tcu",
+ "ingenic,jz4760-tcu",
"simple-mfd";
reg = <0x10002000 0x1000>;
#address-cells = <1>;
--
2.33.0
On 08/04/2022 20:38, H. Nikolaus Schaller wrote:
> arch/mips/boot/dts/ingenic/ci20.dtb: bluetooth: vcc-supply does not match any of the regexes: pinctrl-[0-9]+
> From schema: Documentation/devicetree/bindings/net/broadcom-bluetooth.yaml
>
> Signed-off-by: H. Nikolaus Schaller <[email protected]>
> ---
> arch/mips/boot/dts/ingenic/ci20.dts | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
> index dc587b4b36009..8a120f9374331 100644
> --- a/arch/mips/boot/dts/ingenic/ci20.dts
> +++ b/arch/mips/boot/dts/ingenic/ci20.dts
> @@ -207,7 +207,7 @@ &uart2 {
> bluetooth {
> compatible = "brcm,bcm4330-bt";
> reset-gpios = <&gpf 8 GPIO_ACTIVE_HIGH>;
> - vcc-supply = <&wlan0_power>;
> + vbat-supply = <&wlan0_power>;
Could be also vddio...
Acked-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
Hi Yanjie,
> Am 12.04.2022 um 11:49 schrieb Zhou Yanjie <[email protected]>:
>
> Hi Nikolaus,
>
> On 2022/4/11 上午3:13, H. Nikolaus Schaller wrote:
>> Hi,
>>
>>
>> If you agree I can add "ingenic,jz4780-otg" to the schema file and keep
>> the .dts in the v2 of my series.
>
>
> Sure.
>
> Or you can wait a bit, I plan to send out new patches later today, it contains "ingenic,jz4780-otg".
Would be fine! I think you will be faster than me :)
So I'll check with your patch and drop mine from my v2 series.
BR and thanks,
Nikolaus
On 09/04/2022 15:09, H. Nikolaus Schaller wrote:
>
>
>> Am 09.04.2022 um 13:26 schrieb Krzysztof Kozlowski <[email protected]>:
>>
>> On 08/04/2022 20:37, H. Nikolaus Schaller wrote:
>>> jz4780-nemc needs to be compatible to simple-mfd as well or we get
>>>
>>> arch/mips/boot/dts/ingenic/ci20.dtb: memory-controller@13410000: compatible: 'oneOf' conditional failed, one must be fixed:
>>> ['ingenic,jz4780-nemc', 'simple-mfd'] is too long
>>> 'ingenic,jz4725b-nemc' was expected
>>> 'ingenic,jz4740-nemc' was expected
>>> From schema: Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
>>>
>>> Signed-off-by: H. Nikolaus Schaller <[email protected]>
>>> ---
>>> .../devicetree/bindings/memory-controllers/ingenic,nemc.yaml | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml b/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
>>> index 24f9e19820282..3b1116588de3d 100644
>>> --- a/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
>>> +++ b/Documentation/devicetree/bindings/memory-controllers/ingenic,nemc.yaml
>>> @@ -17,7 +17,7 @@ properties:
>>> oneOf:
>>> - enum:
>>> -
>>> - - ingenic,jz4780-nemc
>>> + - [ , simple-mfd ]
>>
>> This is not correct representation. If you really need simple-mfd, then
>> this should be a separate item below oneOf.
>
> Well, it is valid YAML syntax and seems to be accepted by dtbscheck.
It's not how we code it. Please do not introduce inconsistent - even if
valid - blocks.
>
>> The true question is whether you need simple-mfd. Isn't the binding (and
>> the driver) expected to instantiate its children?
>
> I had expected that but current ingenic,jz4780-nemc code doesn't.
Paul provided good reason for the simple-mfd. Use this one instead of dt
check warning. DT check warning means nothing, does not bring the actual
answer to "why", because it is artificial tool. The answer to "why" is
in what Paul wrote.
Best regards,
Krzysztof
Hi Nikolaus,
On 2022/4/11 上午3:13, H. Nikolaus Schaller wrote:
> Hi,
>
>> Am 10.04.2022 um 18:32 schrieb Zhou Yanjie <[email protected]>:
>>
>> Hi folks,
>>
>> On 2022/4/9 下午9:53, H. Nikolaus Schaller wrote:
>>>> Am 09.04.2022 um 15:44 schrieb Krzysztof Kozlowski <[email protected]>:
>>>>
>>>> On 09/04/2022 15:32, H. Nikolaus Schaller wrote:
>>>>>> Am 09.04.2022 um 15:15 schrieb Krzysztof Kozlowski <[email protected]>:
>>>>>>
>>>>>> On 09/04/2022 15:05, H. Nikolaus Schaller wrote:
>>>>>>>> This looks wrong, the block usually should have a specific compatible.
>>>>>>>> Please mention why it does not.
>>>>>>> Well, I did not even have that idea that it could need an explanation.
>>>>>>>
>>>>>>> There is no "ingenic,jz4780-otg" and none is needed here to make it work.
>>>>>> Make it work in what terms? We talk about hardware description, right?
>>>>> Yes.
>>>>>
>>>>>>> Therefore the generic "snps,dwc2" is sufficient.
>>>>>> No, you are mixing now driver behavior (is sufficient) with hardware
>>>>>> description.
>>>>> No. "snps,dwc2" is a hardware description for a licensed block.
>>>>> Not a driver behavior.
>>>> snps,dwc2 matches the original block, not necessarily this
>>>> implementation. Unless you are sure?
>>> I assume. Nobody has reported an issue without having any specific jz4780 driver in place.
>>> Well, that is only evidence, not bullet proof.
>>>
>>>>>> Most of licensed blocks require the specific compatible to
>>>>>> differentiate it.
>>>>> If there is a need to differentiate.
>>>> No, regardless whether there is a need currently, most of them have
>>>> specific compatibles, because there are some minor differences. Even if
>>>> difference is not visible from programming model or wiring, it might
>>>> justify it's own specific compatible. For example because maybe once
>>>> that tiny difference will require some changes.
>>>>
>>>> Someone added the ingenic compatible, so why do you assume that one tool
>>>> (bindings) is correct but other piece of code (using specific
>>>> compatible) is not? You use the argument "bindings warning" which is not
>>>> enough. Argument that blocks are 100% same, is good enough, if you are
>>>> sure. Just use it in commit msg. But are you sure that these are the
>>>> same? Same pins, same programming model (entire model, not used by Linux)?
>>> The compatible ingenic,jz4780-otg was introduced in 158c774d3c64859e84dd20e04d5fb18c8d3d318e.
>>> Hence I have added Yanjie for clarification why he added it in the .dts and not in the bindings.
>>
>> It's my fault, last year I made an OTG driver for Ingenic SoCs and sent it
>> to the mailing list, and then I received some revision comments, but for
>> some personal reasons I didn't continue to improve it.
>>
>> I'll finish these modifications as soon as possible and send them out.
>> Then after they merge into the mainline, this problem will be solved.
> No need to apologize.
>
> If you agree I can add "ingenic,jz4780-otg" to the schema file and keep
> the .dts in the v2 of my series.
Sure.
Or you can wait a bit, I plan to send out new patches later today, it
contains "ingenic,jz4780-otg".
> And I'll add you to the list of reviewers, so you can please comment v2
> if it is correct or if we are still missing something.
Okay, thanks!
>
> Best regards and thanks,
> Nikolaus
Hi,
> Am 10.04.2022 um 17:35 schrieb Krzysztof Kozlowski <[email protected]>:
>
> On 09/04/2022 15:18, Krzysztof Kozlowski wrote:
>> On 09/04/2022 15:09, H. Nikolaus Schaller wrote:
>
> (...)
>
>>>>> @@ -17,7 +17,7 @@ properties:
>>>>> oneOf:
>>>>> - enum:
>>>>> -
>>>>> - - ingenic,jz4780-nemc
>>>>> + - [ , simple-mfd ]
>>>>
>>>> This is not correct representation. If you really need simple-mfd, then
>>>> this should be a separate item below oneOf.
>>>
>>> Well, it is valid YAML syntax and seems to be accepted by dtbscheck.
>
> Minor update:
> Well, it is not a valid schema. Rob's checker now confirmed. If you run
> dt_bindings_check by yourself you will see the error:
I did run dt_bindings_check. Otherwise I wouldn't have seen that there is a problem
at all, before suggesting these changes...
>
> properties:compatible:oneOf:0:enum:1: ['ingenic', 'jz4780-nemc',
> 'simple-mfd'] is not of type 'string'
>
> Probably because enum expects string, not another enum (so enum inside
> enum is not correct).
>
> If you do not see the error, you might be missing some packages
> (mentioned in writing-schema + yamllint for a different issue) or your
> dtschema is old.
Neither.
yamllint etc. are all the most recent versions. Updated just two days ago.
But my filter looking for ci20.dtb did not catch it because the report
doesn't mention that file.
So Rob's robot is more mature than mine...
BR,
Nikolaus