Hi Philippe,
>Hi Jeuk,
>
>[+Alistair]
>
>On 26/5/23 07:05, Jeuk Kim wrote:
>> Universal Flash Storage (UFS) is a high-performance mass storage device
>> with a serial interface. It is primarily used as a high-performance
>> data storage device for embedded applications.
>>
>> This commit contains code for UFS device to be recognized
>> as a UFS PCI device.
>> Patches to handle UFS logical unit and Transfer Request will follow.
>>
>> Signed-off-by: Jeuk Kim <jeuk20....@samsung.com>
>> ---
>> MAINTAINERS | 6 +
>> hw/Kconfig | 1 +
>> hw/meson.build | 1 +
>> hw/ufs/Kconfig | 4 +
>> hw/ufs/meson.build | 1 +
>> hw/ufs/trace-events | 33 +
>> hw/ufs/trace.h | 1 +
>> hw/ufs/ufs.c | 305 ++++++++++
>> hw/ufs/ufs.h | 42 ++
>> include/block/ufs.h | 1251 ++++++++++++++++++++++++++++++++++++++
>> include/hw/pci/pci.h | 1 +
>> include/hw/pci/pci_ids.h | 1 +
>> meson.build | 1 +
>> 13 files changed, 1648 insertions(+)
>> create mode 100644 hw/ufs/Kconfig
>> create mode 100644 hw/ufs/meson.build
>> create mode 100644 hw/ufs/trace-events
>> create mode 100644 hw/ufs/trace.h
>> create mode 100644 hw/ufs/ufs.c
>> create mode 100644 hw/ufs/ufs.h
>> create mode 100644 include/block/ufs.h
>
>
>> diff --git a/include/block/ufs.h b/include/block/ufs.h
>> new file mode 100644
>> index 0000000000..0ce3a19bc0
>> --- /dev/null
>> +++ b/include/block/ufs.h
>> @@ -0,0 +1,1251 @@
>> +#ifndef BLOCK_UFS_H
>> +#define BLOCK_UFS_H
>> +
>> +#include "hw/registerfields.h"
>
>Since you use the registerfields API, ...
>
>> +typedef struct QEMU_PACKED UfsReg {
>> + uint32_t cap;
>> + uint32_t rsvd0;
>> + uint32_t ver;
>> + uint32_t rsvd1;
>> + uint32_t hcpid;
>> + uint32_t hcmid;
>> + uint32_t ahit;
>> + uint32_t rsvd2;
>> + uint32_t is;
>> + uint32_t ie;
>> + uint32_t rsvd3[2];
>> + uint32_t hcs;
>> + uint32_t hce;
>> + uint32_t uecpa;
>> + uint32_t uecdl;
>> + uint32_t uecn;
>> + uint32_t uect;
>> + uint32_t uecdme;
>> + uint32_t utriacr;
>> + uint32_t utrlba;
>> + uint32_t utrlbau;
>> + uint32_t utrldbr;
>> + uint32_t utrlclr;
>> + uint32_t utrlrsr;
>> + uint32_t utrlcnr;
>> + uint32_t rsvd4[2];
>> + uint32_t utmrlba;
>> + uint32_t utmrlbau;
>> + uint32_t utmrldbr;
>> + uint32_t utmrlclr;
>> + uint32_t utmrlrsr;
>> + uint32_t rsvd5[3];
>> + uint32_t uiccmd;
>> + uint32_t ucmdarg1;
>> + uint32_t ucmdarg2;
>> + uint32_t ucmdarg3;
>> + uint32_t rsvd6[4];
>> + uint32_t rsvd7[4];
>> + uint32_t rsvd8[16];
>> + uint32_t ccap;
>> +} UfsReg;
>> +
>> +enum UfsRegOfs {
>> + UFS_REG_CAP = offsetof(UfsReg, cap),
>> + UFS_REG_VER = offsetof(UfsReg, ver),
>> + UFS_REG_HCPID = offsetof(UfsReg, hcpid),
>> + UFS_REG_HCMID = offsetof(UfsReg, hcmid),
>> + UFS_REG_AHIT = offsetof(UfsReg, ahit),
>> + UFS_REG_IS = offsetof(UfsReg, is),
>> + UFS_REG_IE = offsetof(UfsReg, ie),
>> + UFS_REG_HCS = offsetof(UfsReg, hcs),
>> + UFS_REG_HCE = offsetof(UfsReg, hce),
>> + UFS_REG_UECPA = offsetof(UfsReg, uecpa),
>> + UFS_REG_UECDL = offsetof(UfsReg, uecdl),
>> + UFS_REG_UECN = offsetof(UfsReg, uecn),
>> + UFS_REG_UECT = offsetof(UfsReg, uect),
>> + UFS_REG_UECDME = offsetof(UfsReg, uecdme),
>> + UFS_REG_UTRIACR = offsetof(UfsReg, utriacr),
>> + UFS_REG_UTRLBA = offsetof(UfsReg, utrlba),
>> + UFS_REG_UTRLBAU = offsetof(UfsReg, utrlbau),
>> + UFS_REG_UTRLDBR = offsetof(UfsReg, utrldbr),
>> + UFS_REG_UTRLCLR = offsetof(UfsReg, utrlclr),
>> + UFS_REG_UTRLRSR = offsetof(UfsReg, utrlrsr),
>> + UFS_REG_UTRLCNR = offsetof(UfsReg, utrlcnr),
>> + UFS_REG_UTMRLBA = offsetof(UfsReg, utmrlba),
>> + UFS_REG_UTMRLBAU = offsetof(UfsReg, utmrlbau),
>> + UFS_REG_UTMRLDBR = offsetof(UfsReg, utmrldbr),
>> + UFS_REG_UTMRLCLR = offsetof(UfsReg, utmrlclr),
>> + UFS_REG_UTMRLRSR = offsetof(UfsReg, utmrlrsr),
>> + UFS_REG_UICCMD = offsetof(UfsReg, uiccmd),
>> + UFS_REG_UCMDARG1 = offsetof(UfsReg, ucmdarg1),
>> + UFS_REG_UCMDARG2 = offsetof(UfsReg, ucmdarg2),
>> + UFS_REG_UCMDARG3 = offsetof(UfsReg, ucmdarg3),
>> + UFS_REG_CCAP = offsetof(UfsReg, ccap),
>> +};
>> +
>> +enum UfsCapShift {
>> + CAP_NUTRS_SHIFT = 0,
>> + CAP_RTT_SHIFT = 8,
>> + CAP_NUTMRS_SHIFT = 16,
>> + CAP_AUTOH8_SHIFT = 23,
>> + CAP_64AS_SHIFT = 24,
>> + CAP_OODDS_SHIFT = 25,
>> + CAP_UICDMETMS_SHIFT = 26,
>> + CAP_CS_SHIFT = 28,
>> +};
>> +
>> +enum UfsCapMask {
>> + CAP_NUTRS_MASK = 0x1f,
>> + CAP_RTT_MASK = 0xff,
>> + CAP_NUTMRS_MASK = 0x7,
>> + CAP_AUTOH8_MASK = 0x1,
>> + CAP_64AS_MASK = 0x1,
>> + CAP_OODDS_MASK = 0x1,
>> + CAP_UICDMETMS_MASK = 0x1,
>> + CAP_CS_MASK = 0x1,
>> +};
>> +
>> +#define UFS_CAP_NUTRS(cap) (((cap) >> CAP_NUTRS_SHIFT) & CAP_NUTRS_MASK)
>> +#define UFS_CAP_RTT(cap) (((cap) >> CAP_RTT_SHIFT) & CAP_RTT_MASK)
>> +#define UFS_CAP_NUTMRS(cap) (((cap) >> CAP_NUTMRS_SHIFT) & CAP_NUTMRS_MASK)
>> +#define UFS_CAP_AUTOH8(cap) (((cap) >> CAP_AUTOH8_SHIFT) & CAP_AUTOH8_MASK)
>> +#define UFS_CAP_64AS(cap) (((cap) >> CAP_64AS_SHIFT) & CAP_64AS_MASK)
>> +#define UFS_CAP_OODDS(cap) (((cap) >> CAP_OODDS_SHIFT) & CAP_OODDS_MASK)
>> +#define UFS_CAP_UICDMETMS(cap) \
>> + (((cap) >> CAP_UICDMETMS_SHIFT) & CAP_UICDMETMS_MASK)
>> +#define UFS_CAP_CS(cap) (((cap) >> CAP_CS_SHIFT) & CAP_CS_MASK)
>> +
>> +#define UFS_CAP_SET_NUTRS(cap, val) \
>> + (cap = (cap & ~(CAP_NUTRS_MASK << CAP_NUTRS_SHIFT)) | \
>> + ((uint32_t)val & CAP_NUTRS_MASK) << CAP_NUTRS_SHIFT)
>> +#define UFS_CAP_SET_RTT(cap, val) \
>> + (cap = (cap & ~(CAP_RTT_MASK << CAP_RTT_SHIFT)) | \
>> + ((uint32_t)val & CAP_RTT_MASK) << CAP_RTT_SHIFT)
>> +#define UFS_CAP_SET_NUTMRS(cap, val) \
>> + (cap = (cap & ~(CAP_NUTMRS_MASK << CAP_NUTMRS_SHIFT)) | \
>> + ((uint32_t)val & CAP_NUTMRS_MASK) << CAP_NUTMRS_SHIFT)
>> +#define UFS_CAP_SET_AUTOH8(cap, val) \
>> + (cap = (cap & ~(CAP_AUTOH8_MASK << CAP_AUTOH8_SHIFT)) | \
>> + ((uint32_t)val & CAP_AUTOH8_MASK) << CAP_AUTOH8_SHIFT)
>> +#define UFS_CAP_SET_64AS(cap, val) \
>> + (cap = (cap & ~(CAP_64AS_MASK << CAP_64AS_SHIFT)) | \
>> + ((uint32_t)val & CAP_64AS_MASK) << CAP_64AS_SHIFT)
>> +#define UFS_CAP_SET_OODDS(cap, val) \
>> + (cap = (cap & ~(CAP_OODDS_MASK << CAP_OODDS_SHIFT)) | \
>> + ((uint32_t)val & CAP_OODDS_MASK) << CAP_OODDS_SHIFT)
>> +#define UFS_CAP_SET_UICDMETMS(cap, val) \
>> + (cap = (cap & ~(CAP_UICDMETMS_MASK << CAP_UICDMETMS_SHIFT)) | \
>> + ((uint32_t)val & CAP_UICDMETMS_MASK) << CAP_UICDMETMS_SHIFT)
>> +#define UFS_CAP_SET_CS(cap, val) \
>> + (cap = (cap & ~(CAP_CS_MASK << CAP_CS_SHIFT)) | \
>> + ((uint32_t)val & CAP_CS_MASK) << CAP_CS_SHIFT)
>
>... could it be worth add the FIELD_SET32() macro once in that header?
I realized that hw/registerfields.h has FIELD_DP32(), which is the same
function as FIELD_SET32().
Instead of defining a new macro in my code, I will modify it to utilize the
macros in hw/registerfields.h.
Thanks!
