Re: [Qemu-devel] [PATCH v10 00/16] SDHCI: clean v1/v2 Specs (part 2)

[Alistair Francis]

On Wed, Feb 7, 2018 at 10:19 AM, Philippe Mathieu-Daudé <address@hidden> wrote:
> Patches missing review: 2 (PCI qtests) and 11 (trivial)

I don't see patch 11 on the list.

I can see it in my Xilinx email as it was sent directly to that. I
can't cleanly reply to it from my Xilinx Outlook account though.

For patch 11 the commit message should be improved so that it can be
standalone from the title. Although you could just remove the message
as well as it doesn't add anything.

Once you have done that you can add my reviewed by to patch 11.

Alistair

>
> Since v9:
> - replace global by struct QSDHCI in qtests (suggested by Paolo)
>   (this generates changes in patches 3-6 but since they are only code adapted
>   and no logical changes I kept Stefan R-b)
> - added Alistair R-b
>
> Since v8:
> - we keep the 'capareg' property, this simplify a lot the series
> - the Zynq 7000 uses the datasheet CAPAREG
> - reset R-b/A-b
>
> Since v7:
> - use error_propagate()
>
> Since v6:
> - rebased on upstream to use DEFINE_SDHCI_COMMON_PROPERTIES
> - included qtests back
> - add PCI qtests
> - series was getting big, splitting again; next one is "implement Spec v3"
> - following Alistair advice, new properties default to current
>   SDHC_CAPAB_REG_DEFAULT (probably 'default' for historical reason, being
>   the first capabilities set). This leads to weird properties imho, i.e.
>   defaulting 'max-frequency' to 52 MHz, the Exynos SoC has to set this
>   property to 0.
> - addressed Alistair reviews
> - confirmed the Exynos4210 specs after chatting on IRC with Krzysztof
>   Kozlowski who kindly verified in the datasheet.
>
> Since v5:
> - addressed Alistair reviews
> - dropped "abstract generic-sdhci"
> - dropped Linux Device Tree names
> - split qtests in another series
> - change the bcm2835 minimum blocksize to 1KB (Andrew Baumann)
> - added Alistair R-b
> - based on Alistair work:
>   - add SD tunning sequence via Host Control 2 to use UHS-I cards
>   - add CMD/DAT[] fields in the Present State (used in next series
>     to switch card voltage)
>
> based on Alistair work:
> - add SD tunning sequence via Host Control 2 to use UHS-I cards
> - add CMD/DAT[] fields in the Present State (used in next series
>   to switch card voltage)
>
> Since v4 ("SDHCI: add qtests and fix few issues"):
> - spec_version default to v2 (current behaviour)
> - addressed Alistair review (no v1, tell user about valid version)
>
> Since v3:
> - no change, but split back in 2 series, 1st part is "SDHCI: housekeeping v5",
>
> Since v2:
> - more detailed 'capabilities', all boards converted to use these properties
> - since all qtests pass, removed the previous 'capareg' property
> - added Stefan/Alistair R-b
> - corrected 'access' LED behavior (Alistair's review)
> - more uses of the registerfields API
> - remove some dead code
> - cosmetix:
>   - added more comments
>   - renamed a pair of registers
>   - reordered few struct members
>
> Since v1:
> - addressed Alistair Francis review comments, added some R-b
> - only move register defines to "sd-internal.h"
> - fixed deposit64() arguments
> - dropped unuseful s->fifo_buffer = NULL
> - use a qemu_irq for the LED, restrict the logging to ON/OFF
> - fixed a trace format string error
> - included Andrey Smirnov ACMD12ERRSTS write patch
> - dropped few unuseful patches, and separate the Python polemical ones for 
> later
>
> From the "SDHCI housekeeping" series:
> - 1: we restrict part of "sd/sd.h" into local "sd-internal.h",
> - 2,3: we somehow beautiful the code, no logical changes,
> - 4-7: we refactor the common sysbus/pci qdev code,
> - 8-10: we add plenty of trace events which will result useful later,
> - 11: we finally expose a "dma-memory" property.
> From the "SDHCI: add a qtest and fix few issues" series:
> - 12,13: fix registers
> - 14,15: boards can specify which SDHCI Spec to use (v2 and v3 so far)
> - 15-20: HCI qtest
>
> Regards,
>
> Phil.
>
> $ git backport-diff with v9
> 001/16:[----] [--] 'sdhci: use error_propagate(local_err) in realize()'
> 002/16:[0065] [FC] 'sdhci: add qtest to check the SD capabilities register'
> 003/16:[0020] [FC] 'sdhci: add check_capab_readonly() qtest'
> 004/16:[0010] [FC] 'sdhci: add a check_capab_baseclock() qtest'
> 005/16:[0006] [FC] 'sdhci: add a check_capab_sdma() qtest'
> 006/16:[0022] [FC] 'sdhci: add qtest to check the SD Spec version'
> 007/16:[----] [--] 'sdhci: add a 'spec_version property' (default to v2)'
> 008/16:[----] [--] 'sdhci: use a numeric value for the default CAPAB register'
> 009/16:[----] [--] 'sdhci: simplify sdhci_get_fifolen()'
> 010/16:[----] [--] 'sdhci: check the Spec v1 capabilities correctness'
> 011/16:[----] [--] 'sdhci: add BLOCK_SIZE_MASK for DMA'
> 012/16:[----] [--] 'sdhci: Fix 64-bit ADMA2'
> 013/16:[----] [--] 'sdhci: check Spec v2 capabilities (DMA and 64-bit bus)'
> 014/16:[----] [--] 'hw/arm/exynos4210: access the 64-bit capareg with 
> qdev_prop_set_uint64()'
> 015/16:[----] [--] 'hw/arm/exynos4210: add a comment about a very similar 
> SDHCI (Spec. v2)'
> 016/16:[----] [-C] 'hw/arm/xilinx_zynq: fix the capabilities register to 
> match the datasheet'
>
> Philippe Mathieu-Daudé (15):
>   sdhci: use error_propagate(local_err) in realize()
>   sdhci: add qtest to check the SD capabilities register
>   sdhci: add check_capab_readonly() qtest
>   sdhci: add a check_capab_baseclock() qtest
>   sdhci: add a check_capab_sdma() qtest
>   sdhci: add qtest to check the SD Spec version
>   sdhci: add a 'spec_version property' (default to v2)
>   sdhci: use a numeric value for the default CAPAB register
>   sdhci: simplify sdhci_get_fifolen()
>   sdhci: check the Spec v1 capabilities correctness
>   sdhci: add BLOCK_SIZE_MASK for DMA
>   sdhci: check Spec v2 capabilities (DMA and 64-bit bus)
>   hw/arm/exynos4210: access the 64-bit capareg with qdev_prop_set_uint64()
>   hw/arm/exynos4210: add a comment about a very similar SDHCI (Spec. v2)
>   hw/arm/xilinx_zynq: fix the capabilities register to match the datasheet
>
> Sai Pavan Boddu (1):
>   sdhci: Fix 64-bit ADMA2
>
>  include/hw/sd/sdhci.h  |   2 +
>  hw/sd/sdhci-internal.h |  43 ++++++---
>  hw/arm/exynos4210.c    |  14 ++-
>  hw/arm/xilinx_zynq.c   |  53 +++++-----
>  hw/sd/sdhci.c          | 257 
> ++++++++++++++++++++++++++++++++-----------------
>  hw/sd/trace-events     |   1 +
>  tests/sdhci-test.c     | 217 +++++++++++++++++++++++++++++++++++++++++
>  tests/Makefile.include |   3 +
>  8 files changed, 465 insertions(+), 125 deletions(-)
>  create mode 100644 tests/sdhci-test.c
>
> --
> 2.16.1
>
>