Re: [RFC PATCH V3 00/21] Add support UHS-II for GL9755

[Ben Chuang]

On Fri, Jul 17, 2020 at 6:18 PM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>
> On Fri, 10 Jul 2020 at 13:07, Ben Chuang <benchuanggli@xxxxxxxxx> wrote:
> >
> > Summary
> > =======
> > These patches[1] support UHS-II and fix GL9755 UHS-II compatibility.
>
> First of all, thanks for posting this - and my apologies for not
> having the bandwidth to review the earlier versions.
>
> >
> > About UHS-II, roughly deal with the following three parts:
> > 1) A UHS-II detection and initialization:
> > - Host setup to support UHS-II (Section 3.13.1 Host Controller Setup Sequence
> >   [2]).
> > - Detect a UHS-II I/F (Section 3.13.2 Card Interface Detection Sequence[2]).
> > - In step(9) of Section 3.13.2 in [2], UHS-II initialization is include Section
> >   3.13.3 UHS-II Card Initialization and Section 3.13.4 UHS-II Setting Register
> >   Setup Sequence.
> >
> > 2) Send Legacy SD command through SD-TRAN
> > - Encapsulated SD packets are defined in SD-TRAN in order to ensure Legacy SD
> >   compatibility and preserve Legacy SD infrastructures (Section 7.1.1 Packet
> >   Types and Format Overview[3]).
> > - Host issue a UHS-II CCMD packet or a UHS-II DCMD (Section 3.13.5 UHS-II
> >   CCMD Packet issuing and Section 3.13.6 UHS-II DCMD Packet issuing[2]).
> >
> > 3) UHS-II Interrupt
> > - Except for UHS-II error interrupts, most interrupts share the original
> >   interrupt registers.
>
> The above points to some specifications, which is good, but what I
> really need to be able to do a proper review -  is an explanation of
> *what*, *why* and *how* the series implements the UHS-II support.

What:
 1. detect UHS-II interface of a card
 2. change card to UHS-II mode
 3. send SD command through SD-TRAN
 4. go back to legacy interface if not detect UHS-II interface

Why:
 1. UHS-II card support Legacy Interface and UHS-II interface

How:
  a) Using 3.13.2 Card Interface Detection Sequence
  1. power up, host provides RCLK and STB.L to D0.lane.
  2. host waits D1.lane to change EIDL to STB.L
  3. host starts UHS-II initialization if STB.L

>
> To be clear, I don't need in-depth details, as that should be
> described in each patch's commit message, but I would appreciate an
> overall description of the approach you have taken to implement this.
>
> The reason I need this is because UHS-II is a completely new
> interface/protocol. If it wasn't because that a UHS-II card is also
> required to be backwards compatible with the legacy SD interface, one
> could even consider introducing an entirely new subsystem. Not saying
> that we should, but just pointing out that the series is not trivial
> to review.
>
> That said, I am going to give it a real try to do the review. I will
> try to focus on the overall approach, rather than on the details, at
> least to start with.
>

Thank you, I will be patient.

> >
> > Patch structure
> > ===============
> > patch#1-#7: for core
> > patch#8-#17: for sdhci
> > patch#18-#21: for GL9755
> >
> > Tests
> > =====
> > Ran 'dd' command to evaluate the performance:
> > (SanDisk UHS-II card on GL9755 controller)
> >                              Read    Write
> > UHS-II disabled (UHS-I): 88.3MB/s 60.7MB/s
> > UHS-II enabled         :  206MB/s   80MB/s
>
> I like these comparisons, thanks for sharing!
>
> What UHS-II interface mode does you HW support? FD156, HD312, FD312 FD624?

GL9755 supports up to HD312.

>
> >
> > TODO
> > ====
> > - replace some define with BIT macro
> >
> > Reference
> > =========
> > [1] https://gitlab.com/ben.chuang/linux-uhs2-gl9755.git
> > [2] SD Host Controller Simplified Specification 4.20
> > [3] UHS-II Simplified Addendum 1.02
> >
> > Changes in v3 (Jul. 10, 2020)
> > * rebased to v5.8-rc4
> > * add copyright notice
> > * reorganize the patch set and split some commits into smaller ones
> > * separate uhs-2 headers from others
> > * correct wrong spellings
> > * fix most of checkpatch warnings/errors
> > * remove all k[cz]alloc() from the code
> > * guard sdhci-uhs2 specific code with
> >       'if (IS_ENABLED(CONFIG_MMC_SDHCI_UHS2))'
> > * make sdhci-uhs2.c as a module
> > * trivial changes, including
> >   - rename back sdhci-core.c to sdhci.c
> >   - allow vendor code to disable uhs2 if v4_mode == 0
> >       in __sdhci_add_host()
> >   - merge uhs2_power_up() into mmc_power_up()
> >   - remove flag_uhs2 from mmc_attach_sd()
> >   - add function descriptions to EXPORT'ed functions
> >   - other minor code optimization
> >
> > Changes in v2 (Jan. 9, 2020)
> > * rebased to v5.5-rc5
> >
> > AKASHI Takahiro (15):
> >   mmc: core: UHS-II support, modify power-up sequence
> >   mmc: core: UHS-II support, skip set_chip_select()
> >   mmc: core: UHS-II support, skip TMODE setup in some cases
> >   mmc: core: UHS-II support, generate UHS-II SD command packet
> >   mmc: core: UHS-II support, set APP_CMD bit if necessary
> >   mmc: sdhci: add a kernel configuration for enabling UHS-II support
> >   mmc: sdhci: add UHS-II related definitions in headers
> >   mmc: sdhci: UHS-II support, dump UHS-II registers
> >   mmc: sdhci: UHS-II support, export host operations to core
> >   mmc: sdhci: UHS-II support, skip signal_voltage_switch()
> >   mmc: sdhci: UHS-II support, handle vdd2 in case of power-off
> >   mmc: sdhci: UHS-II support, modify set_power() to handle vdd2
> >   mmc: sdhci: UHS-II support, export helper functions to a module
> >   mmc: sdhci: UHS-II support, implement operations as a module
> >   mmc: core: add post-mmc_attach_sd hook
> >
> > Ben Chuang (6):
> >   mmc: add UHS-II related definitions in public headers
> >   mmc: core: UHS-II support, try to select UHS-II interface
> >   mmc: sdhci: UHS-II support, add hooks for additional operations
> >   mmc: sdhci-uhs2: add pre-detect_init hook
> >   mmc: sdhci-uhs2: add post-mmc_attach_sd hook
> >   mmc: sdhci-pci-gli: enable UHS-II mode for GL9755
> >
> >  drivers/mmc/core/Makefile         |   2 +-
> >  drivers/mmc/core/block.c          |   7 +-
> >  drivers/mmc/core/bus.c            |   5 +-
> >  drivers/mmc/core/core.c           | 119 +++-
> >  drivers/mmc/core/regulator.c      |  14 +
> >  drivers/mmc/core/sd.c             |  32 ++
> >  drivers/mmc/core/sd_ops.c         |  12 +
> >  drivers/mmc/core/uhs2.c           | 874 ++++++++++++++++++++++++++++++
>
> Nitpick:
>
> I would prefer to prefix any new needed file with sd_*. In this case,
> sd_uhs2.c|h.

OK, change uhs2.c and uhs2.h to sd_uhs2.c and sd_uhs2.h later.

>
> >  drivers/mmc/core/uhs2.h           |  21 +
> >  drivers/mmc/host/Kconfig          |  10 +
> >  drivers/mmc/host/Makefile         |   1 +
> >  drivers/mmc/host/sdhci-omap.c     |   2 +-
> >  drivers/mmc/host/sdhci-pci-core.c |   4 +-
> >  drivers/mmc/host/sdhci-pci-gli.c  | 361 +++++++++++-
> >  drivers/mmc/host/sdhci-pxav3.c    |   4 +-
> >  drivers/mmc/host/sdhci-uhs2.c     | 797 +++++++++++++++++++++++++++
> >  drivers/mmc/host/sdhci-uhs2.h     | 215 ++++++++
> >  drivers/mmc/host/sdhci-xenon.c    |   4 +-
> >  drivers/mmc/host/sdhci.c          | 321 +++++++++--
> >  drivers/mmc/host/sdhci.h          | 113 +++-
> >  include/linux/mmc/card.h          |   1 +
> >  include/linux/mmc/core.h          |   6 +
> >  include/linux/mmc/host.h          |  31 ++
> >  include/linux/mmc/uhs2.h          | 268 +++++++++
> >  24 files changed, 3151 insertions(+), 73 deletions(-)
> >  create mode 100644 drivers/mmc/core/uhs2.c
> >  create mode 100644 drivers/mmc/core/uhs2.h
> >  create mode 100644 drivers/mmc/host/sdhci-uhs2.c
> >  create mode 100644 drivers/mmc/host/sdhci-uhs2.h
> >  create mode 100644 include/linux/mmc/uhs2.h
> >
> > --
> > 2.27.0
> >
>
> Kind regards
> Uffe