[RFD]: Solving qcom unique unit address warnings

[RFD]: Solving qcom unique unit address warnings

[Vinod Koul]

Hi Rob,

On all Qualcomm SoCs there exists QUP serial engine. These engines are
loaded with a firmware by bootloader and can support a specific protocol
like I2C/SPI/uart etc.

Since the serial engine can support different protocols we end up
describing all the supported ones for this, as illustrated below from
sdm845 dts upstream:

		qupv3_id_0: geniqup@8c0000 {
			compatible = "qcom,geni-se-qup";
			reg = <0 0x008c0000 0 0x6000>;
			clock-names = "m-ahb", "s-ahb";
			clocks = <&gcc GCC_QUPV3_WRAP_0_M_AHB_CLK>,
				 <&gcc GCC_QUPV3_WRAP_0_S_AHB_CLK>;
			iommus = <&apps_smmu 0x3 0x0>;
			#address-cells = <2>;
			#size-cells = <2>;
			ranges;
			interconnects = <&aggre1_noc MASTER_QUP_1 0 &config_noc SLAVE_BLSP_1 0>;
			interconnect-names = "qup-core";
			status = "disabled";

			i2c0: i2c@880000 {
				compatible = "qcom,geni-i2c";
				reg = <0 0x00880000 0 0x4000>;
				clock-names = "se";
				clocks = <&gcc GCC_QUPV3_WRAP0_S0_CLK>;
				pinctrl-names = "default";
				pinctrl-0 = <&qup_i2c0_default>;
				interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>;
				#address-cells = <1>;
				#size-cells = <0>;
				power-domains = <&rpmhpd SDM845_CX>;
				operating-points-v2 = <&qup_opp_table>;
				interconnects = <&aggre1_noc MASTER_QUP_1 0 &config_noc SLAVE_BLSP_1 0>,
						<&gladiator_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_BLSP_1 0>,
						<&aggre1_noc MASTER_QUP_1 0 &mem_noc SLAVE_EBI1 0>;
				interconnect-names = "qup-core", "qup-config", "qup-memory";
				status = "disabled";
			};

			spi0: spi@880000 {
				compatible = "qcom,geni-spi";
				reg = <0 0x00880000 0 0x4000>;
				clock-names = "se";
				clocks = <&gcc GCC_QUPV3_WRAP0_S0_CLK>;
				pinctrl-names = "default";
				pinctrl-0 = <&qup_spi0_default>;
				interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>;
				#address-cells = <1>;
				#size-cells = <0>;
				interconnects = <&aggre1_noc MASTER_QUP_1 0 &config_noc SLAVE_BLSP_1 0>,
						<&gladiator_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_BLSP_1 0>;
				interconnect-names = "qup-core", "qup-config";
				status = "disabled";
			};

			uart0: serial@880000 {
				compatible = "qcom,geni-uart";
				reg = <0 0x00880000 0 0x4000>;
				clock-names = "se";
				clocks = <&gcc GCC_QUPV3_WRAP0_S0_CLK>;
				pinctrl-names = "default";
				pinctrl-0 = <&qup_uart0_default>;
				interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>;
				power-domains = <&rpmhpd SDM845_CX>;
				operating-points-v2 = <&qup_opp_table>;
				interconnects = <&aggre1_noc MASTER_QUP_1 0 &config_noc SLAVE_BLSP_1 0>,
						<&gladiator_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_BLSP_1 0>;
				interconnect-names = "qup-core", "qup-config";
				status = "disabled";
			};


The problem comes with all these nodes having same unit addresses. This
is adding to ~2K warning for unique_unit_address upstream.

So to solve this we thought of creating a qup se node and then query the
protocol supported from the firmware on boot and create a child
auxillary_device. The problem with that approach is another warning
"node name for SPI buses should be 'spi'"! So that would not help

Now, I cant think of any better idea here, except maybe move these to
respective board dts and perhaps keep them commented here for
documentation.

Do we have any better idea to solve this problem?

Thanks
-- 
~Vinod

Re: [RFD]: Solving qcom unique unit address warnings

[Vinod Koul]

On 21-01-22, 22:38, Vinod Koul wrote:
> Hi Rob,

Any guidance on how to resolve this...?

> 
> On all Qualcomm SoCs there exists QUP serial engine. These engines are
> loaded with a firmware by bootloader and can support a specific protocol
> like I2C/SPI/uart etc.
> 
> Since the serial engine can support different protocols we end up
> describing all the supported ones for this, as illustrated below from
> sdm845 dts upstream:
> 
> 		qupv3_id_0: geniqup@8c0000 {
> 			compatible = "qcom,geni-se-qup";
> 			reg = <0 0x008c0000 0 0x6000>;
> 			clock-names = "m-ahb", "s-ahb";
> 			clocks = <&gcc GCC_QUPV3_WRAP_0_M_AHB_CLK>,
> 				 <&gcc GCC_QUPV3_WRAP_0_S_AHB_CLK>;
> 			iommus = <&apps_smmu 0x3 0x0>;
> 			#address-cells = <2>;
> 			#size-cells = <2>;
> 			ranges;
> 			interconnects = <&aggre1_noc MASTER_QUP_1 0 &config_noc SLAVE_BLSP_1 0>;
> 			interconnect-names = "qup-core";
> 			status = "disabled";
> 
> 			i2c0: i2c@880000 {
> 				compatible = "qcom,geni-i2c";
> 				reg = <0 0x00880000 0 0x4000>;
> 				clock-names = "se";
> 				clocks = <&gcc GCC_QUPV3_WRAP0_S0_CLK>;
> 				pinctrl-names = "default";
> 				pinctrl-0 = <&qup_i2c0_default>;
> 				interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>;
> 				#address-cells = <1>;
> 				#size-cells = <0>;
> 				power-domains = <&rpmhpd SDM845_CX>;
> 				operating-points-v2 = <&qup_opp_table>;
> 				interconnects = <&aggre1_noc MASTER_QUP_1 0 &config_noc SLAVE_BLSP_1 0>,
> 						<&gladiator_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_BLSP_1 0>,
> 						<&aggre1_noc MASTER_QUP_1 0 &mem_noc SLAVE_EBI1 0>;
> 				interconnect-names = "qup-core", "qup-config", "qup-memory";
> 				status = "disabled";
> 			};
> 
> 			spi0: spi@880000 {
> 				compatible = "qcom,geni-spi";
> 				reg = <0 0x00880000 0 0x4000>;
> 				clock-names = "se";
> 				clocks = <&gcc GCC_QUPV3_WRAP0_S0_CLK>;
> 				pinctrl-names = "default";
> 				pinctrl-0 = <&qup_spi0_default>;
> 				interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>;
> 				#address-cells = <1>;
> 				#size-cells = <0>;
> 				interconnects = <&aggre1_noc MASTER_QUP_1 0 &config_noc SLAVE_BLSP_1 0>,
> 						<&gladiator_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_BLSP_1 0>;
> 				interconnect-names = "qup-core", "qup-config";
> 				status = "disabled";
> 			};
> 
> 			uart0: serial@880000 {
> 				compatible = "qcom,geni-uart";
> 				reg = <0 0x00880000 0 0x4000>;
> 				clock-names = "se";
> 				clocks = <&gcc GCC_QUPV3_WRAP0_S0_CLK>;
> 				pinctrl-names = "default";
> 				pinctrl-0 = <&qup_uart0_default>;
> 				interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>;
> 				power-domains = <&rpmhpd SDM845_CX>;
> 				operating-points-v2 = <&qup_opp_table>;
> 				interconnects = <&aggre1_noc MASTER_QUP_1 0 &config_noc SLAVE_BLSP_1 0>,
> 						<&gladiator_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_BLSP_1 0>;
> 				interconnect-names = "qup-core", "qup-config";
> 				status = "disabled";
> 			};
> 
> 
> The problem comes with all these nodes having same unit addresses. This
> is adding to ~2K warning for unique_unit_address upstream.
> 
> So to solve this we thought of creating a qup se node and then query the
> protocol supported from the firmware on boot and create a child
> auxillary_device. The problem with that approach is another warning
> "node name for SPI buses should be 'spi'"! So that would not help
> 
> Now, I cant think of any better idea here, except maybe move these to
> respective board dts and perhaps keep them commented here for
> documentation.
> 
> Do we have any better idea to solve this problem?
> 
> Thanks
> -- 
> ~Vinod

-- 
~Vinod

Re: [RFD]: Solving qcom unique unit address warnings

[Rob Herring]

On Fri, Jan 21, 2022 at 10:38:37PM +0530, Vinod Koul wrote:
> Hi Rob,
> 
> On all Qualcomm SoCs there exists QUP serial engine. These engines are
> loaded with a firmware by bootloader and can support a specific protocol
> like I2C/SPI/uart etc.
> 
> Since the serial engine can support different protocols we end up
> describing all the supported ones for this, as illustrated below from
> sdm845 dts upstream:
> 
> 		qupv3_id_0: geniqup@8c0000 {
> 			compatible = "qcom,geni-se-qup";
> 			reg = <0 0x008c0000 0 0x6000>;
> 			clock-names = "m-ahb", "s-ahb";
> 			clocks = <&gcc GCC_QUPV3_WRAP_0_M_AHB_CLK>,
> 				 <&gcc GCC_QUPV3_WRAP_0_S_AHB_CLK>;
> 			iommus = <&apps_smmu 0x3 0x0>;
> 			#address-cells = <2>;
> 			#size-cells = <2>;
> 			ranges;
> 			interconnects = <&aggre1_noc MASTER_QUP_1 0 &config_noc SLAVE_BLSP_1 0>;
> 			interconnect-names = "qup-core";
> 			status = "disabled";
> 
> 			i2c0: i2c@880000 {
> 				compatible = "qcom,geni-i2c";
> 				reg = <0 0x00880000 0 0x4000>;
> 				clock-names = "se";
> 				clocks = <&gcc GCC_QUPV3_WRAP0_S0_CLK>;
> 				pinctrl-names = "default";
> 				pinctrl-0 = <&qup_i2c0_default>;
> 				interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>;
> 				#address-cells = <1>;
> 				#size-cells = <0>;
> 				power-domains = <&rpmhpd SDM845_CX>;
> 				operating-points-v2 = <&qup_opp_table>;
> 				interconnects = <&aggre1_noc MASTER_QUP_1 0 &config_noc SLAVE_BLSP_1 0>,
> 						<&gladiator_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_BLSP_1 0>,
> 						<&aggre1_noc MASTER_QUP_1 0 &mem_noc SLAVE_EBI1 0>;
> 				interconnect-names = "qup-core", "qup-config", "qup-memory";
> 				status = "disabled";
> 			};
> 
> 			spi0: spi@880000 {
> 				compatible = "qcom,geni-spi";
> 				reg = <0 0x00880000 0 0x4000>;
> 				clock-names = "se";
> 				clocks = <&gcc GCC_QUPV3_WRAP0_S0_CLK>;
> 				pinctrl-names = "default";
> 				pinctrl-0 = <&qup_spi0_default>;
> 				interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>;
> 				#address-cells = <1>;
> 				#size-cells = <0>;
> 				interconnects = <&aggre1_noc MASTER_QUP_1 0 &config_noc SLAVE_BLSP_1 0>,
> 						<&gladiator_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_BLSP_1 0>;
> 				interconnect-names = "qup-core", "qup-config";
> 				status = "disabled";
> 			};
> 
> 			uart0: serial@880000 {
> 				compatible = "qcom,geni-uart";
> 				reg = <0 0x00880000 0 0x4000>;
> 				clock-names = "se";
> 				clocks = <&gcc GCC_QUPV3_WRAP0_S0_CLK>;
> 				pinctrl-names = "default";
> 				pinctrl-0 = <&qup_uart0_default>;
> 				interrupts = <GIC_SPI 601 IRQ_TYPE_LEVEL_HIGH>;
> 				power-domains = <&rpmhpd SDM845_CX>;
> 				operating-points-v2 = <&qup_opp_table>;
> 				interconnects = <&aggre1_noc MASTER_QUP_1 0 &config_noc SLAVE_BLSP_1 0>,
> 						<&gladiator_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_BLSP_1 0>;
> 				interconnect-names = "qup-core", "qup-config";
> 				status = "disabled";
> 			};
> 
> 
> The problem comes with all these nodes having same unit addresses. This
> is adding to ~2K warning for unique_unit_address upstream.

This is with W=1, right? 
 
> So to solve this we thought of creating a qup se node and then query the
> protocol supported from the firmware on boot and create a child
> auxillary_device. The problem with that approach is another warning
> "node name for SPI buses should be 'spi'"! So that would not help
> 
> Now, I cant think of any better idea here, except maybe move these to
> respective board dts and perhaps keep them commented here for
> documentation.
> 
> Do we have any better idea to solve this problem?

There is another dtc warning option called 
unique_unit_address_if_enabled which we could enable under W=1 instead 
of unique_unit_address. Even that option has too many warnings to enable 
by default.

Rob

Re: [RFD]: Solving qcom unique unit address warnings

[Vinod Koul]

On 08-02-22, 14:05, Rob Herring wrote:
> On Fri, Jan 21, 2022 at 10:38:37PM +0530, Vinod Koul wrote:
> > 
> > 
> > The problem comes with all these nodes having same unit addresses. This
> > is adding to ~2K warning for unique_unit_address upstream.
> 
> This is with W=1, right? 

Yes

>  
> > So to solve this we thought of creating a qup se node and then query the
> > protocol supported from the firmware on boot and create a child
> > auxillary_device. The problem with that approach is another warning
> > "node name for SPI buses should be 'spi'"! So that would not help
> > 
> > Now, I cant think of any better idea here, except maybe move these to
> > respective board dts and perhaps keep them commented here for
> > documentation.
> > 
> > Do we have any better idea to solve this problem?
> 
> There is another dtc warning option called 
> unique_unit_address_if_enabled which we could enable under W=1 instead 
> of unique_unit_address. Even that option has too many warnings to enable 
> by default.

Bjorn pointed me to your proposal https://www.irccloud.com/pastebin/OnYqVn6p/

with this change I do get a better stats for warnings which we can focus
on reducing :)

Before:
   6483 unique_unit_address\n\
   1108 simple_bus_reg\n\
    764 avoid_unnecessary_addr_size\n\
    712 unit_address_vs_reg\n\
    120 graph_child_address\n\
     32 unique_unit_address_if_enabled

After:
    277 simple_bus_reg\n\
    191 avoid_unnecessary_addr_size\n\
    178 unit_address_vs_reg\n\
     32 unique_unit_address_if_enabled\n\
     30 graph_child_address

So, it would be helpful for now to merge this.

Thanks
-- 
~Vinod