Re: [linux-sunxi] [PATCH 2/3] arm64: allwinner: h6: add device tree nodes for MMC controllers

From: Icenowy Zheng
Date: Mon Jun 25 2018 - 20:28:44 EST


å 2018-04-27äç 22:25 +0100ïAndrà Przywaraåéï
> On 27/04/18 10:23, Icenowy Zheng wrote:
> >
> >
> > ä 2018å4æ27æ GMT+08:00 äå5:18:23, Andre Przywara <andre.przywara@ar
> > m.com> åå:
> > > Hi,
> > >
> > > On 27/04/18 09:36, Icenowy Zheng wrote:
> > > >
> > > >
> > > > ä 2018å4æ27æ GMT+08:00 äå12:45:38, Andre Przywara
> > >
> > > <andre.przywara@xxxxxxx> åå:
> > > > > Hi,
> > > > >
> > > > > On 26/04/18 15:07, Icenowy Zheng wrote:
> > > > > > The Allwinner H6 SoC have 3 MMC controllers.
> > > > > >
> > > > > > Add device tree nodes for them.
> > > > > >
> > > > > > Signed-off-by: Icenowy Zheng <icenowy@xxxxxxx>
> > > > > > ---
> > > > > > arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 56
> > > > >
> > > > > ++++++++++++++++++++++++++++
> > > > > > 1 file changed, 56 insertions(+)
> > > > > >
> > > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > > >
> > > > > b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > > > > index 4debc3962830..3cbfc035c979 100644
> > > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
> > > > > > @@ -124,12 +124,68 @@
> > > > > > interrupt-controller;
> > > > > > #interrupt-cells = <3>;
> > > > > >
> > > > > > + mmc0_pins: mmc0-pins {
> > > > > > + pins = "PF0", "PF1",
> > > > > > "PF2", "PF3",
> > > > > > + "PF4", "PF5";
> > > > > > + function = "mmc0";
> > > > > > + drive-strength = <30>;
> > > > > > + bias-pull-up;
> > > > > > + };
> > > > > > +
> > > > > > + mmc2_pins: mmc2-pins {
> > > > > > + pins = "PC1", "PC4",
> > > > > > "PC5", "PC6",
> > > > > > + "PC7", "PC8",
> > > > > > "PC9", "PC10",
> > > > > > + "PC11", "PC12",
> > > > > > "PC13", "PC14";
> > > > > > + function = "mmc2";
> > > > > > + drive-strength = <30>;
> > > > > > + bias-pull-up;
> > > > > > + };
> > > > > > +
> > > > > > uart0_ph_pins: uart0-ph {
> > > > > > pins = "PH0", "PH1";
> > > > > > function = "uart0";
> > > > > > };
> > > > > > };
> > > > > >
> > > > > > + mmc0: mmc@4020000 {
> > > > > > + compatible = "allwinner,sun50i-h6-
> > > > > > mmc";
> > > > >
> > > > > This should be:
> > > > > compatible = "allwinner,sun50i-h6-mmc",
> > > > > "allwinner,sun50i-a64-
> > > > > mmc";
> > > >
> > > > I'm intended to not add A64 compatible, as
> > > > H6 is a quite new design
> > > > (new process) and there might be different behavior, even on
> > > > mmc0/1.
> > >
> > > But as your patch proves, it is fully backwards compatible: An
> > > A64
> > > driver works with this device.
> >
> > No, my patch only proves "the current A64 driver works
> > with this device", not "Any A64 driver works with device", as
> > the current driver doesn't fully use the capability provided
> > by A64 MMC cobtrollers.
>
> Good point, but I still believe every A64 driver would be capable of
> driving an H6 MMC controller, ....
>
> > > And this is what this compatible string list says: If your system
> > > does
> > > not have a specific H6 driver, you can use an A64 driver.
> > > You might not get all the (potentially) new features, but it
> > > covers
> > > everything the A64 has.
> > >
> > > And a new silicon process doesn't matter here, since the software
> > > interface is unchanged. *If* we find bugs, we can add quirks
> > > matching
> >
> > I think there's timing parameters for higher speed bins which
> > are different among chips. As we have currently no support
> > for speed bins higher than DDR50, they're not added yet.
>
> True, but what are those differences? I compared the A64 and H6
> manuals
> side by side, the differences I found are:
> SMHC_FIFOTH[+0x40]:
> BSIZE_OF_TRANS[30:28]:
> - H6 supports 16 transfers for SMHC0 also.
> other parameters:
> - H6 recommends better values for SMHC0 also
> SMHC_CSDC[+0x54]:
> - H6 doesn't mention restriction to SMHC2
> (though this might be a mistake)
> SMHC_NTSR_REG[+0x5C]:
> - H6 defines fields for bits[24:8]
> SMHC_EMCE[+0x64] and SMHC_EMCE_DBG[+0x68]:
> - H6 adds, for EMCE support
> EMMC_DDR_SBIT_DET_REG[0x10c]:
> - A64 doesn't mention restriction to SMHC2,
> but I believe this is a mistake
> SMHC_EMCE_BMn[0x150 + 0x4 * 0..31]
> - H6 adds, for EMCE support
>
> All those pieces are only *additions* to the H6 over the A64, so
> don't
> affect backwards compatibility.
>
> > > on
> > > the H6 compatible string - that's why we put it here already,
> > > despite
> > > having a matching string in the kernel at the moment.
> >
> > Device tree is not driver data but hardware description, so
> > it should follow "how the device is formed" rather than
> > "how the device works".
>
> True, but as shown above, the compatibility is really at the device
> level.
> Unless you have any other information ...

Rob, could you answer whether we should add the A64 compatible or not?

>
> Cheers,
> Andre.
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel