| Message ID | 20241120063701.8194-2-friday.yang@mediatek.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add SMI clamp in MediaTek SMI driver | expand |
On Wed, 20 Nov 2024 14:36:38 +0800, Friday Yang wrote: > On the MediaTek platform, some SMI LARBs are directly linked to SMI > Common. While some SMI LARBs are linked to SMI Sub Common, then SMI > Sub Common is linked to SMI Common. The hardware block diagram could > be described as below. > Add 'resets' and 'reset-names' for SMI LARBs to support SMI reset > and clamp operation. The SMI reset driver could get the reset signal > through the two properties. > > SMI-Common(Smart Multimedia Interface Common) > | > +----------------+------------------+ > | | | > | | | > | | | > | | | > | | | > larb0 SMI-Sub-Common0 SMI-Sub-Common1 > | | | | | > larb1 larb2 larb3 larb7 larb9 > > Signed-off-by: Friday Yang <friday.yang@mediatek.com> > --- > > Although this can pass the dtbs_check, maybe there is a better way > to describe the requirements for 'resets' and 'reset-names' in bindings. > But I don't find a better way to describe it that only SMI larbs located > in camera and image subsys requires the 'resets' and 'reset-names'. > I would appreciate it if you could give some suggestions. > > .../mediatek,smi-common.yaml | 2 + > .../memory-controllers/mediatek,smi-larb.yaml | 53 +++++++++++++++---- > 2 files changed, 44 insertions(+), 11 deletions(-) > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: ./Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml:143:13: [warning] wrong indentation: expected 10 but found 12 (indentation) dtschema/dtc warnings/errors: Error: Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.example.dts:29.43-44 syntax error FATAL ERROR: Unable to parse input tree make[2]: *** [scripts/Makefile.dtbs:129: Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.example.dtb] Error 1 make[2]: *** Waiting for unfinished jobs.... make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1442: dt_binding_check] Error 2 make: *** [Makefile:224: __sub-make] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241120063701.8194-2-friday.yang@mediatek.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. 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 after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Wed, Nov 20, 2024 at 02:36:38PM +0800, Friday Yang wrote: > diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml > index 2381660b324c..302c0f93b49d 100644 > --- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml > +++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml > @@ -69,6 +69,18 @@ properties: > description: the hardware id of this larb. It's only required when this > hardware id is not consecutive from its M4U point of view. > > + resets: > + maxItems: 1 > + description: This contains a phandle to the reset controller node and an index > + to a reset signal. SMI larbs need to get the reset controller by the node. First sentence is 100% redundant. Arguments depend on the reset-cells, not this binding. > + SMI could get the reset signal by the index number defined in the header > + include/dt-bindings/reset/mt8188-resets.h. What? How this can depend on consumer? Drop entire description, it is useless. > + > + reset-names: > + const: larb > + description: The name of reset controller. SMI driver need to obtain the > + reset controller based on this. Drop description, useless. > + > required: > - compatible > - reg > @@ -125,19 +137,38 @@ allOf: > required: > - mediatek,larb-id > > + - if: # only for camera and image subsys > + properties: > + mediatek,smi: > + contains: Never tested. > + enum: > + - smi_sub_common_img0_4x1 > + - smi_sub_common_img1_4x1 > + - smi_sub_common_cam_5x1 > + - smi_sub_common_cam_8x1 Does not work. Test your code before you send it. No clue what you want to achieve, so not sure how to help. > + > + then: > + required: > + - resets > + - reset-names > + > additionalProperties: false > > examples: > - |+ > - #include <dt-bindings/clock/mt8173-clk.h> > - #include <dt-bindings/power/mt8173-power.h> > - > - larb1: larb@16010000 { > - compatible = "mediatek,mt8173-smi-larb"; > - reg = <0x16010000 0x1000>; > - mediatek,smi = <&smi_common>; > - power-domains = <&scpsys MT8173_POWER_DOMAIN_VDEC>; > - clocks = <&vdecsys CLK_VDEC_CKEN>, > - <&vdecsys CLK_VDEC_LARB_CKEN>; > - clock-names = "apb", "smi"; > + #include <dt-bindings/clock/mediatek,mt8188-clk.h> > + #include <dt-bindings/power/mediatek,mt8188-power.h> > + #include <dt-bindings/reset/mt8188-resets.h> This is some total mess. Never tested, not correct. Sorry, run it internally through some sort of review or internal checklist which will ask you to test the code before sending. Best regards, Krzysztof
On Wed, 2024-11-20 at 01:43 -0600, Rob Herring (Arm) wrote: > External email : Please do not click links or open attachments until > you have verified the sender or the content. > > > On Wed, 20 Nov 2024 14:36:38 +0800, Friday Yang wrote: > > On the MediaTek platform, some SMI LARBs are directly linked to SMI > > Common. While some SMI LARBs are linked to SMI Sub Common, then SMI > > Sub Common is linked to SMI Common. The hardware block diagram > > could > > be described as below. > > Add 'resets' and 'reset-names' for SMI LARBs to support SMI reset > > and clamp operation. The SMI reset driver could get the reset > > signal > > through the two properties. > > > > SMI-Common(Smart Multimedia Interface Common) > > | > > +----------------+------------------+ > > | | | > > | | | > > | | | > > | | | > > | | | > > larb0 SMI-Sub-Common0 SMI-Sub-Common1 > > | | | | | > > larb1 larb2 larb3 larb7 larb9 > > > > Signed-off-by: Friday Yang <friday.yang@mediatek.com> > > --- > > > > Although this can pass the dtbs_check, maybe there is a better way > > to describe the requirements for 'resets' and 'reset-names' in > > bindings. > > But I don't find a better way to describe it that only SMI larbs > > located > > in camera and image subsys requires the 'resets' and 'reset-names'. > > I would appreciate it if you could give some suggestions. > > > > .../mediatek,smi-common.yaml | 2 + > > .../memory-controllers/mediatek,smi-larb.yaml | 53 > > +++++++++++++++---- > > 2 files changed, 44 insertions(+), 11 deletions(-) > > > > My bot found errors running 'make dt_binding_check' on your patch: > > yamllint warnings/errors: > ./Documentation/devicetree/bindings/memory-controllers/mediatek,smi- > larb.yaml:143:13: [warning] wrong indentation: expected 10 but found > 12 (indentation) > > dtschema/dtc warnings/errors: > Error: Documentation/devicetree/bindings/memory- > controllers/mediatek,smi-larb.example.dts:29.43-44 syntax error > FATAL ERROR: Unable to parse input tree > make[2]: *** [scripts/Makefile.dtbs:129: > Documentation/devicetree/bindings/memory-controllers/mediatek,smi- > larb.example.dtb] Error 1 > make[2]: *** Waiting for unfinished jobs.... > make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1442: > dt_binding_check] Error 2 > make: *** [Makefile:224: __sub-make] Error 2 > > doc reference errors (make refcheckdocs): > > See > https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241120063701.8194-2-friday.yang@mediatek.com > > The base for the series is generally the latest rc1. A different > dependency > should be noted in *this* patch. > > 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 after running the above command yourself. > Note > that DT_SCHEMA_FILES can be set to your schema file to speed up > checking > your schema. However, it must be unset to test all examples with your > schema. > Thanks for your comments, I used to use this cmd: make DT_CHECKER_FLAGS=-m dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/memory- controllers/mediatek,smi-larb.yaml And I will upgrade dtschema and change to use 'make dt_binding_check' again.
On Wed, 2024-11-20 at 08:45 +0100, Krzysztof Kozlowski wrote: > External email : Please do not click links or open attachments until > you have verified the sender or the content. > > > On Wed, Nov 20, 2024 at 02:36:38PM +0800, Friday Yang wrote: > > diff --git a/Documentation/devicetree/bindings/memory- > > controllers/mediatek,smi-larb.yaml > > b/Documentation/devicetree/bindings/memory- > > controllers/mediatek,smi-larb.yaml > > index 2381660b324c..302c0f93b49d 100644 > > --- a/Documentation/devicetree/bindings/memory- > > controllers/mediatek,smi-larb.yaml > > +++ b/Documentation/devicetree/bindings/memory- > > controllers/mediatek,smi-larb.yaml > > @@ -69,6 +69,18 @@ properties: > > description: the hardware id of this larb. It's only required > > when this > > hardware id is not consecutive from its M4U point of view. > > > > + resets: > > + maxItems: 1 > > + description: This contains a phandle to the reset controller > > node and an index > > + to a reset signal. SMI larbs need to get the reset > > controller by the node. > > First sentence is 100% redundant. Arguments depend on the reset- > cells, > not this binding. > > > + SMI could get the reset signal by the index number defined > > in the header > > + include/dt-bindings/reset/mt8188-resets.h. > > What? How this can depend on consumer? Drop entire description, it is > useless. > Thanks for your comments, I will remove the entire description. > > + > > + reset-names: > > + const: larb > > + description: The name of reset controller. SMI driver need to > > obtain the > > + reset controller based on this. > > Drop description, useless. OK, I will remove the entire description. > > > + > > required: > > - compatible > > - reg > > @@ -125,19 +137,38 @@ allOf: > > required: > > - mediatek,larb-id > > > > + - if: # only for camera and image subsys > > + properties: > > + mediatek,smi: > > + contains: > > Never tested. OK, I will fix the indentation. > > > + enum: > > + - smi_sub_common_img0_4x1 > > + - smi_sub_common_img1_4x1 > > + - smi_sub_common_cam_5x1 > > + - smi_sub_common_cam_8x1 > > Does not work. Test your code before you send it. No clue what you > want > to achieve, so not sure how to help. > As I mentioned in SMI driver, only SMI larbs located in camera and image subsys need to apply clamp and reset operation, others can be skipped. So I want to know if this description method meets your expectations here(smi_sub_common_img0_4x1, smi_sub_common_img1_4x1 ...). > > > + > > + then: > > + required: > > + - resets > > + - reset-names > > + > > additionalProperties: false > > > > examples: > > - |+ > > - #include <dt-bindings/clock/mt8173-clk.h> > > - #include <dt-bindings/power/mt8173-power.h> > > - > > - larb1: larb@16010000 { > > - compatible = "mediatek,mt8173-smi-larb"; > > - reg = <0x16010000 0x1000>; > > - mediatek,smi = <&smi_common>; > > - power-domains = <&scpsys MT8173_POWER_DOMAIN_VDEC>; > > - clocks = <&vdecsys CLK_VDEC_CKEN>, > > - <&vdecsys CLK_VDEC_LARB_CKEN>; > > - clock-names = "apb", "smi"; > > + #include <dt-bindings/clock/mediatek,mt8188-clk.h> > > + #include <dt-bindings/power/mediatek,mt8188-power.h> > > + #include <dt-bindings/reset/mt8188-resets.h> > > This is some total mess. Never tested, not correct. Sorry, run it > internally through some sort of review or internal checklist which > will > ask you to test the code before sending. > > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml index 2f36ac23604c..4392d349878c 100644 --- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml +++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml @@ -39,6 +39,7 @@ properties: - mediatek,mt8186-smi-common - mediatek,mt8188-smi-common-vdo - mediatek,mt8188-smi-common-vpp + - mediatek,mt8188-smi-sub-common - mediatek,mt8192-smi-common - mediatek,mt8195-smi-common-vdo - mediatek,mt8195-smi-common-vpp @@ -107,6 +108,7 @@ allOf: compatible: contains: enum: + - mediatek,mt8188-smi-sub-common - mediatek,mt8195-smi-sub-common then: required: diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml index 2381660b324c..302c0f93b49d 100644 --- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml +++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml @@ -69,6 +69,18 @@ properties: description: the hardware id of this larb. It's only required when this hardware id is not consecutive from its M4U point of view. + resets: + maxItems: 1 + description: This contains a phandle to the reset controller node and an index + to a reset signal. SMI larbs need to get the reset controller by the node. + SMI could get the reset signal by the index number defined in the header + include/dt-bindings/reset/mt8188-resets.h. + + reset-names: + const: larb + description: The name of reset controller. SMI driver need to obtain the + reset controller based on this. + required: - compatible - reg @@ -125,19 +137,38 @@ allOf: required: - mediatek,larb-id + - if: # only for camera and image subsys + properties: + mediatek,smi: + contains: + enum: + - smi_sub_common_img0_4x1 + - smi_sub_common_img1_4x1 + - smi_sub_common_cam_5x1 + - smi_sub_common_cam_8x1 + + then: + required: + - resets + - reset-names + additionalProperties: false examples: - |+ - #include <dt-bindings/clock/mt8173-clk.h> - #include <dt-bindings/power/mt8173-power.h> - - larb1: larb@16010000 { - compatible = "mediatek,mt8173-smi-larb"; - reg = <0x16010000 0x1000>; - mediatek,smi = <&smi_common>; - power-domains = <&scpsys MT8173_POWER_DOMAIN_VDEC>; - clocks = <&vdecsys CLK_VDEC_CKEN>, - <&vdecsys CLK_VDEC_LARB_CKEN>; - clock-names = "apb", "smi"; + #include <dt-bindings/clock/mediatek,mt8188-clk.h> + #include <dt-bindings/power/mediatek,mt8188-power.h> + #include <dt-bindings/reset/mt8188-resets.h> + + larb10: smi@15120000 { + compatible = "mediatek,mt8188-smi-larb"; + reg = <0 0x15120000 0 0x1000>; + clocks = <&imgsys CLK_IMGSYS_MAIN_DIP0>, + <&imgsys1_dip_top CLK_IMGSYS1_DIP_TOP_LARB10>; + clock-names = "apb", "smi"; + power-domains = <&spm MT8188_POWER_DOMAIN_DIP>; + resets = <&imgsys1_dip_nr_rst MT8188_SMI_RST_LARB10>; + reset-names = "larb"; + mediatek,larb-id = <10>; + mediatek,smi = <&smi_sub_common_img0_4x1>; };
On the MediaTek platform, some SMI LARBs are directly linked to SMI Common. While some SMI LARBs are linked to SMI Sub Common, then SMI Sub Common is linked to SMI Common. The hardware block diagram could be described as below. Add 'resets' and 'reset-names' for SMI LARBs to support SMI reset and clamp operation. The SMI reset driver could get the reset signal through the two properties. SMI-Common(Smart Multimedia Interface Common) | +----------------+------------------+ | | | | | | | | | | | | | | | larb0 SMI-Sub-Common0 SMI-Sub-Common1 | | | | | larb1 larb2 larb3 larb7 larb9 Signed-off-by: Friday Yang <friday.yang@mediatek.com> --- Although this can pass the dtbs_check, maybe there is a better way to describe the requirements for 'resets' and 'reset-names' in bindings. But I don't find a better way to describe it that only SMI larbs located in camera and image subsys requires the 'resets' and 'reset-names'. I would appreciate it if you could give some suggestions. .../mediatek,smi-common.yaml | 2 + .../memory-controllers/mediatek,smi-larb.yaml | 53 +++++++++++++++---- 2 files changed, 44 insertions(+), 11 deletions(-) -- 2.46.0