diff mbox series

[V3,1/3] arm64: dts: sc7280: Add QSPI node

Message ID 20210604135439.19119-2-rojay@codeaurora.org (mailing list archive)
State Changes Requested
Headers show
Series Add QSPI and QUPv3 DT nodes for SC7280 SoC | expand

Commit Message

Roja Rani Yarubandi June 4, 2021, 1:54 p.m. UTC
Add QSPI DT node for SC7280 SoC.

Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
---
Changes in V3:
 - Broken the huge V2 patch into 3 smaller patches.
   1. QSPI DT nodes
   2. QUP wrapper_0 DT nodes
   3. QUP wrapper_1 DT nodes

Changes in V2:
 - As per Doug's comments removed pinmux/pinconf subnodes.
 - As per Doug's comments split of SPI, UART nodes has been done.
 - Moved QSPI node before aps_smmu as per the order.

 arch/arm64/boot/dts/qcom/sc7280-idp.dts | 29 ++++++++++++
 arch/arm64/boot/dts/qcom/sc7280.dtsi    | 61 +++++++++++++++++++++++++
 2 files changed, 90 insertions(+)

Comments

Stephen Boyd June 4, 2021, 9:45 p.m. UTC | #1
Quoting Roja Rani Yarubandi (2021-06-04 06:54:37)
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> index 3900cfc09562..d0edffc15736 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> @@ -268,6 +268,22 @@ pmr735b_die_temp {
>                 };
>  };
>  
> +&qspi {
> +       status = "okay";
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&qspi_clk>, <&qspi_cs0>, <&qspi_data01>;
> +
> +       flash@0 {
> +               compatible = "jedec,spi-nor";
> +               reg = <0>;
> +
> +               /* TODO: Increase frequency after testing */

Is this going to change? Please set it to 37.5MHz if that's the max
supported.

> +               spi-max-frequency = <25000000>;
> +               spi-tx-bus-width = <2>;
> +               spi-rx-bus-width = <2>;
> +       };
> +};
> +
>  &qupv3_id_0 {
>         status = "okay";
>  };
Bjorn Andersson June 6, 2021, 3:55 a.m. UTC | #2
On Fri 04 Jun 08:54 CDT 2021, Roja Rani Yarubandi wrote:

> Add QSPI DT node for SC7280 SoC.
> 
> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
> ---
> Changes in V3:
>  - Broken the huge V2 patch into 3 smaller patches.
>    1. QSPI DT nodes
>    2. QUP wrapper_0 DT nodes
>    3. QUP wrapper_1 DT nodes
> 
> Changes in V2:
>  - As per Doug's comments removed pinmux/pinconf subnodes.
>  - As per Doug's comments split of SPI, UART nodes has been done.
>  - Moved QSPI node before aps_smmu as per the order.
> 
>  arch/arm64/boot/dts/qcom/sc7280-idp.dts | 29 ++++++++++++
>  arch/arm64/boot/dts/qcom/sc7280.dtsi    | 61 +++++++++++++++++++++++++
>  2 files changed, 90 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> index 3900cfc09562..d0edffc15736 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> @@ -268,6 +268,22 @@ pmr735b_die_temp {
>  		};
>  };
>  
> +&qspi {
> +	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&qspi_clk>, <&qspi_cs0>, <&qspi_data01>;
> +
> +	flash@0 {
> +		compatible = "jedec,spi-nor";
> +		reg = <0>;
> +
> +		/* TODO: Increase frequency after testing */
> +		spi-max-frequency = <25000000>;
> +		spi-tx-bus-width = <2>;
> +		spi-rx-bus-width = <2>;
> +	};
> +};
> +
>  &qupv3_id_0 {
>  	status = "okay";
>  };
> @@ -278,6 +294,19 @@ &uart5 {
>  
>  /* PINCTRL - additions to nodes defined in sc7280.dtsi */
>  
> +&qspi_cs0 {
> +	bias-disable;
> +};
> +
> +&qspi_clk {
> +	bias-disable;
> +};
> +
> +&qspi_data01 {
> +	/* High-Z when no transfers; nice to park the lines */
> +	bias-pull-up;
> +};
> +
>  &qup_uart5_default {
>  	tx {
>  		pins = "gpio46";
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index 6c9d5eb93f93..3047ab802cd2 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -1061,6 +1061,42 @@ apss_merge_funnel_in: endpoint {
>  			};
>  		};
>  
> +		qspi_opp_table: qspi-opp-table {

This node doesn't represents anything on the mmio bus, so it shouldn't
live in in /soc. Can't you move it into &qspi?

Regards,
Bjorn

> +			compatible = "operating-points-v2";
> +
> +			opp-75000000 {
> +				opp-hz = /bits/ 64 <75000000>;
> +				required-opps = <&rpmhpd_opp_low_svs>;
> +			};
> +
> +			opp-150000000 {
> +				opp-hz = /bits/ 64 <150000000>;
> +				required-opps = <&rpmhpd_opp_svs>;
> +			};
> +
> +			opp-300000000 {
> +				opp-hz = /bits/ 64 <300000000>;
> +				required-opps = <&rpmhpd_opp_nom>;
> +			};
> +		};
> +
> +		qspi: spi@88dc000 {
> +			compatible = "qcom,qspi-v1";
> +			reg = <0 0x088dc000 0 0x1000>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&gcc GCC_QSPI_CNOC_PERIPH_AHB_CLK>,
> +				 <&gcc GCC_QSPI_CORE_CLK>;
> +			clock-names = "iface", "core";
> +			interconnects = <&gem_noc MASTER_APPSS_PROC 0
> +					&cnoc2 SLAVE_QSPI_0 0>;
> +			interconnect-names = "qspi-config";
> +			power-domains = <&rpmhpd SC7280_CX>;
> +			operating-points-v2 = <&qspi_opp_table>;
> +			status = "disabled";
> +		};
Roja Rani Yarubandi June 8, 2021, 8:05 a.m. UTC | #3
On 2021-06-05 03:15, Stephen Boyd wrote:
> Quoting Roja Rani Yarubandi (2021-06-04 06:54:37)
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts 
>> b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> index 3900cfc09562..d0edffc15736 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> @@ -268,6 +268,22 @@ pmr735b_die_temp {
>>                 };
>>  };
>> 
>> +&qspi {
>> +       status = "okay";
>> +       pinctrl-names = "default";
>> +       pinctrl-0 = <&qspi_clk>, <&qspi_cs0>, <&qspi_data01>;
>> +
>> +       flash@0 {
>> +               compatible = "jedec,spi-nor";
>> +               reg = <0>;
>> +
>> +               /* TODO: Increase frequency after testing */
> 
> Is this going to change? Please set it to 37.5MHz if that's the max
> supported.
> 

Okay, will set it to 37.5MHz.

Thanks,
Roja

>> +               spi-max-frequency = <25000000>;
>> +               spi-tx-bus-width = <2>;
>> +               spi-rx-bus-width = <2>;
>> +       };
>> +};
>> +
>>  &qupv3_id_0 {
>>         status = "okay";
>>  };
Roja Rani Yarubandi June 8, 2021, 8:07 a.m. UTC | #4
On 2021-06-06 09:25, Bjorn Andersson wrote:
> On Fri 04 Jun 08:54 CDT 2021, Roja Rani Yarubandi wrote:
> 
>> Add QSPI DT node for SC7280 SoC.
>> 
>> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
>> ---
>> Changes in V3:
>>  - Broken the huge V2 patch into 3 smaller patches.
>>    1. QSPI DT nodes
>>    2. QUP wrapper_0 DT nodes
>>    3. QUP wrapper_1 DT nodes
>> 
>> Changes in V2:
>>  - As per Doug's comments removed pinmux/pinconf subnodes.
>>  - As per Doug's comments split of SPI, UART nodes has been done.
>>  - Moved QSPI node before aps_smmu as per the order.
>> 
>>  arch/arm64/boot/dts/qcom/sc7280-idp.dts | 29 ++++++++++++
>>  arch/arm64/boot/dts/qcom/sc7280.dtsi    | 61 
>> +++++++++++++++++++++++++
>>  2 files changed, 90 insertions(+)
>> 
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts 
>> b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> index 3900cfc09562..d0edffc15736 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> @@ -268,6 +268,22 @@ pmr735b_die_temp {
>>  		};
>>  };
>> 
>> +&qspi {
>> +	status = "okay";
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&qspi_clk>, <&qspi_cs0>, <&qspi_data01>;
>> +
>> +	flash@0 {
>> +		compatible = "jedec,spi-nor";
>> +		reg = <0>;
>> +
>> +		/* TODO: Increase frequency after testing */
>> +		spi-max-frequency = <25000000>;
>> +		spi-tx-bus-width = <2>;
>> +		spi-rx-bus-width = <2>;
>> +	};
>> +};
>> +
>>  &qupv3_id_0 {
>>  	status = "okay";
>>  };
>> @@ -278,6 +294,19 @@ &uart5 {
>> 
>>  /* PINCTRL - additions to nodes defined in sc7280.dtsi */
>> 
>> +&qspi_cs0 {
>> +	bias-disable;
>> +};
>> +
>> +&qspi_clk {
>> +	bias-disable;
>> +};
>> +
>> +&qspi_data01 {
>> +	/* High-Z when no transfers; nice to park the lines */
>> +	bias-pull-up;
>> +};
>> +
>>  &qup_uart5_default {
>>  	tx {
>>  		pins = "gpio46";
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi 
>> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> index 6c9d5eb93f93..3047ab802cd2 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> @@ -1061,6 +1061,42 @@ apss_merge_funnel_in: endpoint {
>>  			};
>>  		};
>> 
>> +		qspi_opp_table: qspi-opp-table {
> 
> This node doesn't represents anything on the mmio bus, so it shouldn't
> live in in /soc. Can't you move it into &qspi?
> 
> Regards,
> Bjorn
> 

Sure, will move it into qspi node.

Thanks,
Roja

>> +			compatible = "operating-points-v2";
>> +
>> +			opp-75000000 {
>> +				opp-hz = /bits/ 64 <75000000>;
>> +				required-opps = <&rpmhpd_opp_low_svs>;
>> +			};
>> +
>> +			opp-150000000 {
>> +				opp-hz = /bits/ 64 <150000000>;
>> +				required-opps = <&rpmhpd_opp_svs>;
>> +			};
>> +
>> +			opp-300000000 {
>> +				opp-hz = /bits/ 64 <300000000>;
>> +				required-opps = <&rpmhpd_opp_nom>;
>> +			};
>> +		};
>> +
>> +		qspi: spi@88dc000 {
>> +			compatible = "qcom,qspi-v1";
>> +			reg = <0 0x088dc000 0 0x1000>;
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +			interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_HIGH>;
>> +			clocks = <&gcc GCC_QSPI_CNOC_PERIPH_AHB_CLK>,
>> +				 <&gcc GCC_QSPI_CORE_CLK>;
>> +			clock-names = "iface", "core";
>> +			interconnects = <&gem_noc MASTER_APPSS_PROC 0
>> +					&cnoc2 SLAVE_QSPI_0 0>;
>> +			interconnect-names = "qspi-config";
>> +			power-domains = <&rpmhpd SC7280_CX>;
>> +			operating-points-v2 = <&qspi_opp_table>;
>> +			status = "disabled";
>> +		};
Roja Rani Yarubandi July 6, 2021, 9:19 a.m. UTC | #5
On 2021-06-08 13:37, rojay@codeaurora.org wrote:
> On 2021-06-06 09:25, Bjorn Andersson wrote:
>> On Fri 04 Jun 08:54 CDT 2021, Roja Rani Yarubandi wrote:
>> 
>>> Add QSPI DT node for SC7280 SoC.
>>> 
>>> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
>>> ---
>>> Changes in V3:
>>>  - Broken the huge V2 patch into 3 smaller patches.
>>>    1. QSPI DT nodes
>>>    2. QUP wrapper_0 DT nodes
>>>    3. QUP wrapper_1 DT nodes
>>> 
>>> Changes in V2:
>>>  - As per Doug's comments removed pinmux/pinconf subnodes.
>>>  - As per Doug's comments split of SPI, UART nodes has been done.
>>>  - Moved QSPI node before aps_smmu as per the order.
>>> 
>>>  arch/arm64/boot/dts/qcom/sc7280-idp.dts | 29 ++++++++++++
>>>  arch/arm64/boot/dts/qcom/sc7280.dtsi    | 61 
>>> +++++++++++++++++++++++++
>>>  2 files changed, 90 insertions(+)
>>> 
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts 
>>> b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>>> index 3900cfc09562..d0edffc15736 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>>> @@ -268,6 +268,22 @@ pmr735b_die_temp {
>>>  		};
>>>  };
>>> 
>>> +&qspi {
>>> +	status = "okay";
>>> +	pinctrl-names = "default";
>>> +	pinctrl-0 = <&qspi_clk>, <&qspi_cs0>, <&qspi_data01>;
>>> +
>>> +	flash@0 {
>>> +		compatible = "jedec,spi-nor";
>>> +		reg = <0>;
>>> +
>>> +		/* TODO: Increase frequency after testing */
>>> +		spi-max-frequency = <25000000>;
>>> +		spi-tx-bus-width = <2>;
>>> +		spi-rx-bus-width = <2>;
>>> +	};
>>> +};
>>> +
>>>  &qupv3_id_0 {
>>>  	status = "okay";
>>>  };
>>> @@ -278,6 +294,19 @@ &uart5 {
>>> 
>>>  /* PINCTRL - additions to nodes defined in sc7280.dtsi */
>>> 
>>> +&qspi_cs0 {
>>> +	bias-disable;
>>> +};
>>> +
>>> +&qspi_clk {
>>> +	bias-disable;
>>> +};
>>> +
>>> +&qspi_data01 {
>>> +	/* High-Z when no transfers; nice to park the lines */
>>> +	bias-pull-up;
>>> +};
>>> +
>>>  &qup_uart5_default {
>>>  	tx {
>>>  		pins = "gpio46";
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi 
>>> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>> index 6c9d5eb93f93..3047ab802cd2 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>> @@ -1061,6 +1061,42 @@ apss_merge_funnel_in: endpoint {
>>>  			};
>>>  		};
>>> 
>>> +		qspi_opp_table: qspi-opp-table {
>> 
>> This node doesn't represents anything on the mmio bus, so it shouldn't
>> live in in /soc. Can't you move it into &qspi?
>> 
>> Regards,
>> Bjorn
>> 
> 
> Sure, will move it into qspi node.
> 
> Thanks,
> Roja
> 

Hi Bjorn,

Moving "qspi_opp_table" inside &qspi node causing this warning:
arch/arm64/boot/dts/qcom/sc7280.dtsi:1055.35-1072.6: Warning 
(spi_bus_reg): /soc@0/spi@88dc000/qspi-opp-table: missing or empty reg 
property

Shall I keep the qspi-opp-table out of &qspi node?

Thanks,
Roja

>>> +			compatible = "operating-points-v2";
>>> +
>>> +			opp-75000000 {
>>> +				opp-hz = /bits/ 64 <75000000>;
>>> +				required-opps = <&rpmhpd_opp_low_svs>;
>>> +			};
>>> +
>>> +			opp-150000000 {
>>> +				opp-hz = /bits/ 64 <150000000>;
>>> +				required-opps = <&rpmhpd_opp_svs>;
>>> +			};
>>> +
>>> +			opp-300000000 {
>>> +				opp-hz = /bits/ 64 <300000000>;
>>> +				required-opps = <&rpmhpd_opp_nom>;
>>> +			};
>>> +		};
>>> +
>>> +		qspi: spi@88dc000 {
>>> +			compatible = "qcom,qspi-v1";
>>> +			reg = <0 0x088dc000 0 0x1000>;
>>> +			#address-cells = <1>;
>>> +			#size-cells = <0>;
>>> +			interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_HIGH>;
>>> +			clocks = <&gcc GCC_QSPI_CNOC_PERIPH_AHB_CLK>,
>>> +				 <&gcc GCC_QSPI_CORE_CLK>;
>>> +			clock-names = "iface", "core";
>>> +			interconnects = <&gem_noc MASTER_APPSS_PROC 0
>>> +					&cnoc2 SLAVE_QSPI_0 0>;
>>> +			interconnect-names = "qspi-config";
>>> +			power-domains = <&rpmhpd SC7280_CX>;
>>> +			operating-points-v2 = <&qspi_opp_table>;
>>> +			status = "disabled";
>>> +		};
Stephen Boyd July 9, 2021, 12:56 a.m. UTC | #6
Quoting rojay@codeaurora.org (2021-07-06 02:19:27)
> On 2021-06-08 13:37, rojay@codeaurora.org wrote:
> > On 2021-06-06 09:25, Bjorn Andersson wrote:
> >> On Fri 04 Jun 08:54 CDT 2021, Roja Rani Yarubandi wrote:
> >>
> >>> Add QSPI DT node for SC7280 SoC.
> >>>
> >>> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
> >>> ---
> >>> Changes in V3:
> >>>  - Broken the huge V2 patch into 3 smaller patches.
> >>>    1. QSPI DT nodes
> >>>    2. QUP wrapper_0 DT nodes
> >>>    3. QUP wrapper_1 DT nodes
> >>>
> >>> Changes in V2:
> >>>  - As per Doug's comments removed pinmux/pinconf subnodes.
> >>>  - As per Doug's comments split of SPI, UART nodes has been done.
> >>>  - Moved QSPI node before aps_smmu as per the order.
> >>>
> >>>  arch/arm64/boot/dts/qcom/sc7280-idp.dts | 29 ++++++++++++
> >>>  arch/arm64/boot/dts/qcom/sc7280.dtsi    | 61
> >>> +++++++++++++++++++++++++
> >>>  2 files changed, 90 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> >>> b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> >>> index 3900cfc09562..d0edffc15736 100644
> >>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> >>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> >>> @@ -268,6 +268,22 @@ pmr735b_die_temp {
> >>>             };
> >>>  };
> >>>
> >>> +&qspi {
> >>> +   status = "okay";
> >>> +   pinctrl-names = "default";
> >>> +   pinctrl-0 = <&qspi_clk>, <&qspi_cs0>, <&qspi_data01>;
> >>> +
> >>> +   flash@0 {
> >>> +           compatible = "jedec,spi-nor";
> >>> +           reg = <0>;
> >>> +
> >>> +           /* TODO: Increase frequency after testing */
> >>> +           spi-max-frequency = <25000000>;
> >>> +           spi-tx-bus-width = <2>;
> >>> +           spi-rx-bus-width = <2>;
> >>> +   };
> >>> +};
> >>> +
> >>>  &qupv3_id_0 {
> >>>     status = "okay";
> >>>  };
> >>> @@ -278,6 +294,19 @@ &uart5 {
> >>>
> >>>  /* PINCTRL - additions to nodes defined in sc7280.dtsi */
> >>>
> >>> +&qspi_cs0 {
> >>> +   bias-disable;
> >>> +};
> >>> +
> >>> +&qspi_clk {
> >>> +   bias-disable;
> >>> +};
> >>> +
> >>> +&qspi_data01 {
> >>> +   /* High-Z when no transfers; nice to park the lines */
> >>> +   bias-pull-up;
> >>> +};
> >>> +
> >>>  &qup_uart5_default {
> >>>     tx {
> >>>             pins = "gpio46";
> >>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >>> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >>> index 6c9d5eb93f93..3047ab802cd2 100644
> >>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> >>> @@ -1061,6 +1061,42 @@ apss_merge_funnel_in: endpoint {
> >>>                     };
> >>>             };
> >>>
> >>> +           qspi_opp_table: qspi-opp-table {
> >>
> >> This node doesn't represents anything on the mmio bus, so it shouldn't
> >> live in in /soc. Can't you move it into &qspi?
> >>
> >> Regards,
> >> Bjorn
> >>
> >
> > Sure, will move it into qspi node.
> >
> > Thanks,
> > Roja
> >
>
> Hi Bjorn,
>
> Moving "qspi_opp_table" inside &qspi node causing this warning:
> arch/arm64/boot/dts/qcom/sc7280.dtsi:1055.35-1072.6: Warning
> (spi_bus_reg): /soc@0/spi@88dc000/qspi-opp-table: missing or empty reg
> property

If DT folks are OK I think we should hard-code 'opp-table' as not a
device for spi to populate on the spi bus and relax the warning in the
devicetree compiler (see [1] for more details). Technically, nodes that
are bus controllers assume all child nodes are devices on that bus.  In
this case, we want to stick the opp table as a child of the spi node so
that it can be called 'opp-table' and not be a node in the root of DT.

>
> Shall I keep the qspi-opp-table out of &qspi node?
>

If you do, please move it to / instead of putting it under /soc as it
doesn't have an address or a reg property.

[1] https://github.com/dgibson/dtc/blob/69595a167f06c4482ce784e30df1ac9b16ceb5b0/checks.c#L1844
Roja Rani Yarubandi July 14, 2021, 7:47 a.m. UTC | #7
On 2021-07-09 06:26, Stephen Boyd wrote:
> Quoting rojay@codeaurora.org (2021-07-06 02:19:27)
>> On 2021-06-08 13:37, rojay@codeaurora.org wrote:
>> > On 2021-06-06 09:25, Bjorn Andersson wrote:
>> >> On Fri 04 Jun 08:54 CDT 2021, Roja Rani Yarubandi wrote:
>> >>
>> >>> Add QSPI DT node for SC7280 SoC.
>> >>>
>> >>> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
>> >>> ---
>> >>> Changes in V3:
>> >>>  - Broken the huge V2 patch into 3 smaller patches.
>> >>>    1. QSPI DT nodes
>> >>>    2. QUP wrapper_0 DT nodes
>> >>>    3. QUP wrapper_1 DT nodes
>> >>>
>> >>> Changes in V2:
>> >>>  - As per Doug's comments removed pinmux/pinconf subnodes.
>> >>>  - As per Doug's comments split of SPI, UART nodes has been done.
>> >>>  - Moved QSPI node before aps_smmu as per the order.
>> >>>
>> >>>  arch/arm64/boot/dts/qcom/sc7280-idp.dts | 29 ++++++++++++
>> >>>  arch/arm64/boot/dts/qcom/sc7280.dtsi    | 61
>> >>> +++++++++++++++++++++++++
>> >>>  2 files changed, 90 insertions(+)
>> >>>
>> >>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> >>> b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> >>> index 3900cfc09562..d0edffc15736 100644
>> >>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> >>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
>> >>> @@ -268,6 +268,22 @@ pmr735b_die_temp {
>> >>>             };
>> >>>  };
>> >>>
>> >>> +&qspi {
>> >>> +   status = "okay";
>> >>> +   pinctrl-names = "default";
>> >>> +   pinctrl-0 = <&qspi_clk>, <&qspi_cs0>, <&qspi_data01>;
>> >>> +
>> >>> +   flash@0 {
>> >>> +           compatible = "jedec,spi-nor";
>> >>> +           reg = <0>;
>> >>> +
>> >>> +           /* TODO: Increase frequency after testing */
>> >>> +           spi-max-frequency = <25000000>;
>> >>> +           spi-tx-bus-width = <2>;
>> >>> +           spi-rx-bus-width = <2>;
>> >>> +   };
>> >>> +};
>> >>> +
>> >>>  &qupv3_id_0 {
>> >>>     status = "okay";
>> >>>  };
>> >>> @@ -278,6 +294,19 @@ &uart5 {
>> >>>
>> >>>  /* PINCTRL - additions to nodes defined in sc7280.dtsi */
>> >>>
>> >>> +&qspi_cs0 {
>> >>> +   bias-disable;
>> >>> +};
>> >>> +
>> >>> +&qspi_clk {
>> >>> +   bias-disable;
>> >>> +};
>> >>> +
>> >>> +&qspi_data01 {
>> >>> +   /* High-Z when no transfers; nice to park the lines */
>> >>> +   bias-pull-up;
>> >>> +};
>> >>> +
>> >>>  &qup_uart5_default {
>> >>>     tx {
>> >>>             pins = "gpio46";
>> >>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> >>> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> >>> index 6c9d5eb93f93..3047ab802cd2 100644
>> >>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> >>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>> >>> @@ -1061,6 +1061,42 @@ apss_merge_funnel_in: endpoint {
>> >>>                     };
>> >>>             };
>> >>>
>> >>> +           qspi_opp_table: qspi-opp-table {
>> >>
>> >> This node doesn't represents anything on the mmio bus, so it shouldn't
>> >> live in in /soc. Can't you move it into &qspi?
>> >>
>> >> Regards,
>> >> Bjorn
>> >>
>> >
>> > Sure, will move it into qspi node.
>> >
>> > Thanks,
>> > Roja
>> >
>> 
>> Hi Bjorn,
>> 
>> Moving "qspi_opp_table" inside &qspi node causing this warning:
>> arch/arm64/boot/dts/qcom/sc7280.dtsi:1055.35-1072.6: Warning
>> (spi_bus_reg): /soc@0/spi@88dc000/qspi-opp-table: missing or empty reg
>> property
> 
> If DT folks are OK I think we should hard-code 'opp-table' as not a
> device for spi to populate on the spi bus and relax the warning in the
> devicetree compiler (see [1] for more details). Technically, nodes that
> are bus controllers assume all child nodes are devices on that bus.  In
> this case, we want to stick the opp table as a child of the spi node so
> that it can be called 'opp-table' and not be a node in the root of DT.
> 
>> 
>> Shall I keep the qspi-opp-table out of &qspi node?
>> 
> 
> If you do, please move it to / instead of putting it under /soc as it
> doesn't have an address or a reg property.
> 

Hi Bjorn, Rob

Can we move this "qspi_opp_table" to / from /soc?

Thanks,
Roja

> [1]
> https://github.com/dgibson/dtc/blob/69595a167f06c4482ce784e30df1ac9b16ceb5b0/checks.c#L1844
Bjorn Andersson July 19, 2021, 8:08 p.m. UTC | #8
On Wed 14 Jul 02:47 CDT 2021, rojay@codeaurora.org wrote:

> On 2021-07-09 06:26, Stephen Boyd wrote:
> > Quoting rojay@codeaurora.org (2021-07-06 02:19:27)
> > > On 2021-06-08 13:37, rojay@codeaurora.org wrote:
> > > > On 2021-06-06 09:25, Bjorn Andersson wrote:
> > > >> On Fri 04 Jun 08:54 CDT 2021, Roja Rani Yarubandi wrote:
> > > >>
> > > >>> Add QSPI DT node for SC7280 SoC.
> > > >>>
> > > >>> Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
> > > >>> ---
> > > >>> Changes in V3:
> > > >>>  - Broken the huge V2 patch into 3 smaller patches.
> > > >>>    1. QSPI DT nodes
> > > >>>    2. QUP wrapper_0 DT nodes
> > > >>>    3. QUP wrapper_1 DT nodes
> > > >>>
> > > >>> Changes in V2:
> > > >>>  - As per Doug's comments removed pinmux/pinconf subnodes.
> > > >>>  - As per Doug's comments split of SPI, UART nodes has been done.
> > > >>>  - Moved QSPI node before aps_smmu as per the order.
> > > >>>
> > > >>>  arch/arm64/boot/dts/qcom/sc7280-idp.dts | 29 ++++++++++++
> > > >>>  arch/arm64/boot/dts/qcom/sc7280.dtsi    | 61
> > > >>> +++++++++++++++++++++++++
> > > >>>  2 files changed, 90 insertions(+)
> > > >>>
> > > >>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> > > >>> b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> > > >>> index 3900cfc09562..d0edffc15736 100644
> > > >>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> > > >>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> > > >>> @@ -268,6 +268,22 @@ pmr735b_die_temp {
> > > >>>             };
> > > >>>  };
> > > >>>
> > > >>> +&qspi {
> > > >>> +   status = "okay";
> > > >>> +   pinctrl-names = "default";
> > > >>> +   pinctrl-0 = <&qspi_clk>, <&qspi_cs0>, <&qspi_data01>;
> > > >>> +
> > > >>> +   flash@0 {
> > > >>> +           compatible = "jedec,spi-nor";
> > > >>> +           reg = <0>;
> > > >>> +
> > > >>> +           /* TODO: Increase frequency after testing */
> > > >>> +           spi-max-frequency = <25000000>;
> > > >>> +           spi-tx-bus-width = <2>;
> > > >>> +           spi-rx-bus-width = <2>;
> > > >>> +   };
> > > >>> +};
> > > >>> +
> > > >>>  &qupv3_id_0 {
> > > >>>     status = "okay";
> > > >>>  };
> > > >>> @@ -278,6 +294,19 @@ &uart5 {
> > > >>>
> > > >>>  /* PINCTRL - additions to nodes defined in sc7280.dtsi */
> > > >>>
> > > >>> +&qspi_cs0 {
> > > >>> +   bias-disable;
> > > >>> +};
> > > >>> +
> > > >>> +&qspi_clk {
> > > >>> +   bias-disable;
> > > >>> +};
> > > >>> +
> > > >>> +&qspi_data01 {
> > > >>> +   /* High-Z when no transfers; nice to park the lines */
> > > >>> +   bias-pull-up;
> > > >>> +};
> > > >>> +
> > > >>>  &qup_uart5_default {
> > > >>>     tx {
> > > >>>             pins = "gpio46";
> > > >>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > >>> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > >>> index 6c9d5eb93f93..3047ab802cd2 100644
> > > >>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > >>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > > >>> @@ -1061,6 +1061,42 @@ apss_merge_funnel_in: endpoint {
> > > >>>                     };
> > > >>>             };
> > > >>>
> > > >>> +           qspi_opp_table: qspi-opp-table {
> > > >>
> > > >> This node doesn't represents anything on the mmio bus, so it shouldn't
> > > >> live in in /soc. Can't you move it into &qspi?
> > > >>
> > > >> Regards,
> > > >> Bjorn
> > > >>
> > > >
> > > > Sure, will move it into qspi node.
> > > >
> > > > Thanks,
> > > > Roja
> > > >
> > > 
> > > Hi Bjorn,
> > > 
> > > Moving "qspi_opp_table" inside &qspi node causing this warning:
> > > arch/arm64/boot/dts/qcom/sc7280.dtsi:1055.35-1072.6: Warning
> > > (spi_bus_reg): /soc@0/spi@88dc000/qspi-opp-table: missing or empty reg
> > > property
> > 
> > If DT folks are OK I think we should hard-code 'opp-table' as not a
> > device for spi to populate on the spi bus and relax the warning in the
> > devicetree compiler (see [1] for more details). Technically, nodes that
> > are bus controllers assume all child nodes are devices on that bus.  In
> > this case, we want to stick the opp table as a child of the spi node so
> > that it can be called 'opp-table' and not be a node in the root of DT.
> > 
> > > 
> > > Shall I keep the qspi-opp-table out of &qspi node?
> > > 
> > 
> > If you do, please move it to / instead of putting it under /soc as it
> > doesn't have an address or a reg property.
> > 
> 
> Hi Bjorn, Rob
> 
> Can we move this "qspi_opp_table" to / from /soc?
> 

If you have made a proper attempt to convince Rob and Mark that
a child "opp-table" in a SPI master is not a SPI device - and the
conclusion is that this is not a good idea...then yes it should live
outside /soc.

Thanks,
Bjorn
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
index 3900cfc09562..d0edffc15736 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
@@ -268,6 +268,22 @@  pmr735b_die_temp {
 		};
 };
 
+&qspi {
+	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&qspi_clk>, <&qspi_cs0>, <&qspi_data01>;
+
+	flash@0 {
+		compatible = "jedec,spi-nor";
+		reg = <0>;
+
+		/* TODO: Increase frequency after testing */
+		spi-max-frequency = <25000000>;
+		spi-tx-bus-width = <2>;
+		spi-rx-bus-width = <2>;
+	};
+};
+
 &qupv3_id_0 {
 	status = "okay";
 };
@@ -278,6 +294,19 @@  &uart5 {
 
 /* PINCTRL - additions to nodes defined in sc7280.dtsi */
 
+&qspi_cs0 {
+	bias-disable;
+};
+
+&qspi_clk {
+	bias-disable;
+};
+
+&qspi_data01 {
+	/* High-Z when no transfers; nice to park the lines */
+	bias-pull-up;
+};
+
 &qup_uart5_default {
 	tx {
 		pins = "gpio46";
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 6c9d5eb93f93..3047ab802cd2 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -1061,6 +1061,42 @@  apss_merge_funnel_in: endpoint {
 			};
 		};
 
+		qspi_opp_table: qspi-opp-table {
+			compatible = "operating-points-v2";
+
+			opp-75000000 {
+				opp-hz = /bits/ 64 <75000000>;
+				required-opps = <&rpmhpd_opp_low_svs>;
+			};
+
+			opp-150000000 {
+				opp-hz = /bits/ 64 <150000000>;
+				required-opps = <&rpmhpd_opp_svs>;
+			};
+
+			opp-300000000 {
+				opp-hz = /bits/ 64 <300000000>;
+				required-opps = <&rpmhpd_opp_nom>;
+			};
+		};
+
+		qspi: spi@88dc000 {
+			compatible = "qcom,qspi-v1";
+			reg = <0 0x088dc000 0 0x1000>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			interrupts = <GIC_SPI 82 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&gcc GCC_QSPI_CNOC_PERIPH_AHB_CLK>,
+				 <&gcc GCC_QSPI_CORE_CLK>;
+			clock-names = "iface", "core";
+			interconnects = <&gem_noc MASTER_APPSS_PROC 0
+					&cnoc2 SLAVE_QSPI_0 0>;
+			interconnect-names = "qspi-config";
+			power-domains = <&rpmhpd SC7280_CX>;
+			operating-points-v2 = <&qspi_opp_table>;
+			status = "disabled";
+		};
+
 		system-cache-controller@9200000 {
 			compatible = "qcom,sc7280-llcc";
 			reg = <0 0x09200000 0 0xd0000>, <0 0x09600000 0 0x50000>;
@@ -1186,6 +1222,31 @@  tlmm: pinctrl@f100000 {
 			gpio-ranges = <&tlmm 0 0 175>;
 			wakeup-parent = <&pdc>;
 
+			qspi_clk: qspi-clk {
+				pins = "gpio14";
+				function = "qspi_clk";
+			};
+
+			qspi_cs0: qspi-cs0 {
+				pins = "gpio15";
+				function = "qspi_cs";
+			};
+
+			qspi_cs1: qspi-cs1 {
+				pins = "gpio19";
+				function = "qspi_cs";
+			};
+
+			qspi_data01: qspi-data01 {
+				pins = "gpio12", "gpio13";
+				function = "qspi_data";
+			};
+
+			qspi_data12: qspi-data12 {
+				pins = "gpio16", "gpio17";
+				function = "qspi_data";
+			};
+
 			qup_uart5_default: qup-uart5-default {
 				pins = "gpio46", "gpio47";
 				function = "qup13";