[v1,0/3] arm64: dts: qcom: sdm845: Add SD nodes

This series adds the nodes for the SD host controller on SDM845, and wires it
up on the MTP.

Though I tested similar changes on another board, I was unable to actually run
this on an MTP. If someone felt like trying this out on an MTP I would be
grateful.

The original downstream nodes had sleep pinctrl states. It's not obvious to
me that all this messing with drive-strength saves non-negligible amounts of
power, so I didn't add them and figured we could add them later if needed.


Evan Green (3):
  dt-bindings: mmc: sdhci-msm: Clarify register requirements
  arm64: dts: qcom: sdm845: Add SD nodes
  arm64: dts: qcom: sdm845: Add SD nodes for sdm845-mtp

 .../devicetree/bindings/mmc/sdhci-msm.txt     |  2 +-
 arch/arm64/boot/dts/qcom/sdm845-mtp.dts       | 53 +++++++++++++++++++
 arch/arm64/boot/dts/qcom/sdm845.dtsi          | 15 ++++++
 3 files changed, 69 insertions(+), 1 deletion(-)

Hi,

On Wed, Nov 28, 2018 at 2:34 PM Evan Green <evgreen@chromium.org> wrote:
>
> Add the SD controller to SDM845.
>
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
>
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)

Looks right to me.  Possibly you might want to make it known in the
subject and description that this is only sdhc_2 since there are two
SD controllers in SDM845 (oddly numbered sdc2 and sdc4 in the
documentation).  sdc2 (the one you're supporting here) is the
dual-voltage one (supports higher speeds) and also the only one hooked
up on MTP I think.  ...so maybe nobody will ever use sdhc_4, but it
might be worth mentioning anyway.  ;-)

Other than that suggestion this looks good to me.

[note to Andy: please double-check that this ends up in the right sort
order when applying.  If you happen to apply the Quad SPI patch first
then "git am" might end up putting the SDHC_2 node _after_ the Quad
SPI but it should sort numerically before it]

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Hi,

On Wed, Nov 28, 2018 at 2:34 PM Evan Green <evgreen@chromium.org> wrote:
> +&sdhc_2 {
> +       status = "okay";
> +
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&sdc2_clk &sdc2_cmd &sdc2_data &sd_cd_odl>;
> +
> +       vmmc-supply = <&vreg_l21a_2p95>;
> +       vqmmc-supply = <&vddpx_2>;
> +
> +       cd-gpios = <&tlmm 126 GPIO_ACTIVE_LOW>;
> +};
> +
>  &qupv3_id_1 {

Please sort alphabetically.  "s" comes after "q".


> +       /* It seems that mmc_test reports errors if drive strength is not 16. */

Possibly put this comment right next to one of the the "drive-strength" lines?


> +       sd_cd_odl: sd-cd-odl {

"sd_cd_odl" is the name on a different board.  On sdm845 this is
'sd_card_det_n", so it'd be better to use that name.


> +               pinmux {
> +                       pins = "gpio126";
> +                       function = "gpio";
> +               };
> +
> +               pinconf {
> +                       pins = "gpio126";
> +                       bias-pull-up;
> +                       drive-strength = <2>;

Maybe leave this off?  According to Bjorn's research "drive-strength"
has no affect on inputs and this is most certainly an input.

-Doug

On Wed, Dec 5, 2018 at 1:19 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Nov 28, 2018 at 2:34 PM Evan Green <evgreen@chromium.org> wrote:
> > +&sdhc_2 {
> > +       status = "okay";
> > +
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&sdc2_clk &sdc2_cmd &sdc2_data &sd_cd_odl>;
> > +
> > +       vmmc-supply = <&vreg_l21a_2p95>;
> > +       vqmmc-supply = <&vddpx_2>;
> > +
> > +       cd-gpios = <&tlmm 126 GPIO_ACTIVE_LOW>;
> > +};
> > +
> >  &qupv3_id_1 {
>
> Please sort alphabetically.  "s" comes after "q".

I've got A through F down cold, but those later letters are spotty :) Will fix.

>
>
> > +       /* It seems that mmc_test reports errors if drive strength is not 16. */
>
> Possibly put this comment right next to one of the the "drive-strength" lines?

Ok.

>
>
> > +       sd_cd_odl: sd-cd-odl {
>
> "sd_cd_odl" is the name on a different board.  On sdm845 this is
> 'sd_card_det_n", so it'd be better to use that name.
>
>
> > +               pinmux {
> > +                       pins = "gpio126";
> > +                       function = "gpio";
> > +               };
> > +
> > +               pinconf {
> > +                       pins = "gpio126";
> > +                       bias-pull-up;
> > +                       drive-strength = <2>;
>
> Maybe leave this off?  According to Bjorn's research "drive-strength"
> has no affect on inputs and this is most certainly an input.

Sure. Spin coming up.

>
> -Doug