linux-kernel - Re: [PATCH V23 00/16] Add support UHS-II for GL9755 and GL9767

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <80c10cb0-20d8-4b36-93ec-1b2e4bd660ff@intel.com>
Date: Thu, 24 Oct 2024 11:51:53 +0300
From: Adrian Hunter <adrian.hunter@...el.com>
To: Victor Shih <victorshihgli@...il.com>, ulf.hansson@...aro.org
Cc: linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org,
 benchuanggli@...il.com, Lucas.Lai@...esyslogic.com.tw,
 HL.Liu@...esyslogic.com.tw, Greg.tu@...esyslogic.com.tw,
 dlunev@...omium.org, Victor Shih <victor.shih@...esyslogic.com.tw>
Subject: Re: [PATCH V23 00/16] Add support UHS-II for GL9755 and GL9767

On 18/10/24 13:53, Victor Shih wrote:
> From: Victor Shih <victor.shih@...esyslogic.com.tw>
> 
> Summary
> =======
> These patches[1] support UHS-II and fix GL9755 and GL9767
> UHS-II compatibility.
> 
> 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.
> 
> Patch structure
> ===============
> patch#1:     for core
> patch#2-#14: for sdhci
> patch#15:    for GL9755
> patch#16:    for GL9767
> 
> Tests
> =====
> Ran 'dd' command to evaluate the performance 3 times:
> (SanDisk UHS-II card on GL9755 controller)
>                              Read    Write
> UHS-II disabled (UHS-I): 81.9MB/s 51.4MB/s
> UHS-II enabled         :  206MB/s 80.5MB/s
>                              Read    Write
> UHS-II disabled (UHS-I): 82.3MB/s 49.7MB/s
> UHS-II enabled         :  208MB/s 80.8MB/s
>                              Read    Write
> UHS-II disabled (UHS-I): 82.9MB/s 50.8MB/s
> UHS-II enabled         :  205MB/s 90.0MB/s
> (SanDisk UHS-II card on GL9767 controller)
>                              Read    Write
> UHS-II disabled (UHS-I): 83.5MB/s 50.5MB/s
> UHS-II enabled         :  200MB/s 75.3MB/s
>                              Read    Write
> UHS-II disabled (UHS-I): 85.2MB/s 56.3MB/s
> UHS-II enabled         :  203MB/s 75.8MB/s
>                              Read    Write
> UHS-II disabled (UHS-I): 82.9MB/s 51.1MB/s
> UHS-II enabled         :  196MB/s 77.8MB/s
> 
> Test command
> =====
> Read: dd if=/dev/mmcxxx of=/dev/null bs=4096k count=2000 iflag=direct
> Write:dd if=/dev/zero of=/dev/mmcxxx bs=4096k count=2000 oflag=direct
> 
> Changes in v23 (October. 18, 2024)
> * Rebase on latest mmc/next.
> * Version 22 patch#1-patch#6 have already been applied to the mmc/next
>   branch, so the patch order for version 23 has been shifted forward.
> * Patch#1: Remove mmc_uhs2_card_prepare_cmd() function.
>            Remove mmc_sd_can_poweroff_notify() function.
>            Modify ios.timing setting in the sd_uhs2_power_off() function.
>            Restore the position of assign the host->card to original
>            position in the sd_uhs2_init_card() function.
>            Remove unnecessary error handle in the sd_uhs2_init_card()
>            function.
>            Add oldcard judgment to skip some programs in the
>            sd_uhs2_legacy_init() function.
>            Remove unnecessary error handle in the sd_uhs2_legacy_init()
>            function.
>            Remove mmc_card_set_present() function in the
>            sd_uhs2_reinit()function.
> 
> Reference
> =========
> [1] https://gitlab.com/VictorShih/linux-uhs2.git
> [2] SD Host Controller Simplified Specification 4.20
> [3] UHS-II Simplified Addendum 1.02
> [4] https://patchwork.kernel.org/project/linux-mmc/cover/20240913102836.6144-1-victorshihgli@gmail.com/

Not all SDHCI patches show my Acked-by although I did give it
in V22 for SDHCI patches.  So again for SDHCI:

Acked-by: Adrian Hunter <adrian.hunter@...el.com>

Ulf mentioned checkpatch warnings.  Here is a summary:

  ---------------------------------------------------------------------------------------
  heads/head-2024-10-24-01/0002-mmc-sdhci-add-UHS-II-related-definitions-in-headers.patch
  ---------------------------------------------------------------------------------------
  WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
  #20: 
  new file mode 100644

Nope - already covered by drivers/mmc/host/sdhci*

  WARNING: It's generally not useful to have the filename in the file
  #27: FILE: drivers/mmc/host/sdhci-uhs2.h:3:
  + *  linux/drivers/mmc/host/sdhci-uhs2.h - Secure Digital Host Controller Interface driver

Could drop the file name

  ----------------------------------------------------------------------------------------
  heads/head-2024-10-24-01/0003-mmc-sdhci-add-UHS-II-module-and-add-a-kernel-configu.patch
  ----------------------------------------------------------------------------------------
  WARNING: please write a help paragraph that fully describes the config symbol
  #33: FILE: drivers/mmc/host/Kconfig:101:
  +config MMC_SDHCI_UHS2
  +	tristate "UHS2 support on SDHCI controller"
  +	depends on MMC_SDHCI
  +	help
  +	  This option is selected by SDHCI controller drivers that want to
  +	  support UHS2-capable devices.
  +
  +	  If you have a controller with this feature, say Y or M here.
  +

A web-search for uhs2 says what it is, so I have no strong feelings
about adding more.

  WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
  #58: 
  new file mode 100644

Nope - already covered by drivers/mmc/host/sdhci*

  ----------------------------------------------------------------------------------------
  heads/head-2024-10-24-01/0010-mmc-sdhci-uhs2-add-related-functions-to-initialize-t.patch
  ----------------------------------------------------------------------------------------
  WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
  #11: 
  After detected the UHS-II interface, the host's UHS-II capabilities will be set up here and

Commit message could be wrapped as requested

There is also the ones below, but please _leave_ the code as it is
consistent with the current code in the struct.

------------------------------------------------------------------------------
heads/head-2024-10-24-01/0014-mmc-sdhci-pci-add-UHS-II-support-framework.patch
------------------------------------------------------------------------------
WARNING: Unnecessary space before function pointer arguments
#81: FILE: drivers/mmc/host/sdhci-pci.h:148:
+	void			(*remove_host) (struct sdhci_pci_slot *, int);
WARNING: function definition argument 'struct sdhci_pci_slot *' should also have an identifier name
#81: FILE: drivers/mmc/host/sdhci-pci.h:148:
+	void			(*remove_host) (struct sdhci_pci_slot *, int);
WARNING: function definition argument 'int' should also have an identifier name
#81: FILE: drivers/mmc/host/sdhci-pci.h:148:
+	void			(*remove_host) (struct sdhci_pci_slot *, int);
total: 0 errors, 3 warnings, 0 checks, 56 lines checked