Re: [PATCH V4] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card

[sbhanu]

On 2021-06-23 01:47, Stephen Boyd wrote:
> Quoting Shaik Sajida Bhanu (2021-06-16 02:23:01)
> > diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts 
> > b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> > index 3900cfc..0f63cac 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> > +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> > @@ -11,6 +11,7 @@
> >  #include <dt-bindings/iio/qcom,spmi-adc7-pmr735b.h>
> >  #include <dt-bindings/iio/qcom,spmi-adc7-pm8350.h>
> >  #include <dt-bindings/iio/qcom,spmi-adc7-pmk8350.h>
> > +#include <dt-bindings/gpio/gpio.h>
>
> The letter g comes before i, please move this higher in the list.
Sure i will adrress this in my next patchset.
> >  #include "sc7280.dtsi"
> >  #include "pm7325.dtsi"
> >  #include "pmr735a.dtsi"
> > @@ -272,6 +273,34 @@
> >         status = "okay";
> >  };
> >
> > +&sdhc_1 {
> > +       status = "okay";
> > +
> > +       pinctrl-names = "default", "sleep";
> > +       pinctrl-0 = <&sdc1_on>;
> > +       pinctrl-1 = <&sdc1_off>;
> > +
> > +       non-removable;
> > +       no-sd;
> > +       no-sdio;
> > +
> > +       vmmc-supply = <&vreg_l7b_2p9>;
> > +       vqmmc-supply = <&vreg_l19b_1p8>;
> > +};
> > +
> > +&sdhc_2 {
> > +       status = "okay";
> > +
> > +       pinctrl-names = "default", "sleep";
> > +       pinctrl-0 = <&sdc2_on>;
> > +       pinctrl-1 = <&sdc2_off>;
> > +
> > +       vmmc-supply = <&vreg_l9c_2p9>;
> > +       vqmmc-supply = <&vreg_l6c_2p9>;
> > +
> > +       cd-gpios = <&tlmm 91 GPIO_ACTIVE_LOW>;
> > +};
> > +
> >  &uart5 {
> >         status = "okay";
> >  };
> > @@ -291,3 +320,55 @@
> >                 bias-pull-up;
> >         };
> >  };
> > +
> > +&tlmm {
> > +       sdc1_on: sdc1-on {
> > +               clk {
> > +                       pins = "sdc1_clk";
>
> Can the pins property at least be moved into sc7280.dtsi? Then this can
> add bias and drive strength overrides in the board file?
Sure i will adrress this in my next patchset.
> > +                       bias-disable;
> > +                       drive-strength = <16>;
> > +               };
> > +
> > +               cmd {
> > +                       pins = "sdc1_cmd";
> > +                       bias-pull-up;
> > +                       drive-strength = <10>;
> > +               };
> > +
> > +               data {
> > +                       pins = "sdc1_data";
> > +                       bias-pull-up;
> > +                       drive-strength = <10>;
> > +               };
> > +
> > +               rclk {
> > +                       pins = "sdc1_rclk";
> > +                       bias-pull-down;
> > +               };
> > +       };
> > +
> > +       sdc2_on: sdc2-on {
> > +               clk {
> > +                       pins = "sdc2_clk";
> > +                       bias-disable;
> > +                       drive-strength = <16>;
> > +               };
> > +
> > +               cmd {
> > +                       pins = "sdc2_cmd";
> > +                       bias-pull-up;
> > +                       drive-strength = <10>;
> > +               };
> > +
> > +               data {
> > +                       pins = "sdc2_data";
> > +                       bias-pull-up;
> > +                       drive-strength = <10>;
> > +               };
> > +
> > +               sd-cd {
> > +                       pins = "gpio91";
> > +                       bias-pull-up;
> > +               };
> > +       };
> > +};
> > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi 
> > b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > index d600bca..16d8e17 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > @@ -974,6 +1033,51 @@
> >                         };
> >                 };
> >
> > +               sdhc_2: sdhci@8804000 {
> > +                       compatible = "qcom,sc7280-sdhci", 
> > "qcom,sdhci-msm-v5";
> > +                       status = "disabled";
> > +
> > +                       reg = <0 0x08804000 0 0x1000>;
> > +
> > +                       iommus = <&apps_smmu 0x100 0x0>;
> > +                       interrupts = <GIC_SPI 207 
> > IRQ_TYPE_LEVEL_HIGH>,
> > +                                    <GIC_SPI 223 
> > IRQ_TYPE_LEVEL_HIGH>;
> > +                       interrupt-names = "hc_irq", "pwr_irq";
> > +
> > +                       clocks = <&gcc GCC_SDCC2_APPS_CLK>,
> > +                                <&gcc GCC_SDCC2_AHB_CLK>,
> > +                                <&rpmhcc RPMH_CXO_CLK>;
> > +                       clock-names = "core", "iface", "xo";
> > +                       interconnects = <&aggre1_noc MASTER_SDCC_2 0 
> > &mc_virt SLAVE_EBI1 0>,
> > +                                       <&gem_noc MASTER_APPSS_PROC 0 
> > &cnoc2 SLAVE_SDCC_2 0>;
> > +                       interconnect-names = "sdhc-ddr","cpu-sdhc";
> > +                       power-domains = <&rpmhpd SC7280_CX>;
> > +                       operating-points-v2 = <&sdhc2_opp_table>;
> > +
> > +                       bus-width = <4>;
> > +
> > +                       qcom,dll-config = <0x0007642c>;
> > +
> > +                       sdhc2_opp_table: sdhc2-opp-table {
>
> Any reason the node shouldn't be called opp-table?
Yes, it can be called but for consistency, we are using the same node 
names as that on sc7180.