| 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
| ||
|
Message-ID: <CAK00qKB4-UHkP4D2ZU50L7aBzcYwRXPRd-HFnWzHEZvvLtm9bg@mail.gmail.com>
Date: Fri, 24 Feb 2023 18:23:15 +0800
From: Victor Shih <victorshihgli@...il.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: adrian.hunter@...el.com, linux-mmc@...r.kernel.org,
linux-kernel@...r.kernel.org, benchuanggli@...il.com,
HL.Liu@...esyslogic.com.tw, Greg.tu@...esyslogic.com.tw,
takahiro.akashi@...aro.org, dlunev@...omium.org,
Victor Shih <victor.shih@...esyslogic.com.tw>,
Jason Lai <jason.lai@...esyslogic.com.tw>
Subject: Re: [PATCH V6 05/24] mmc: core: Add definitions for SD UHS-II cards
Hi, Ulf
On Tue, Feb 7, 2023 at 12:13 AM Ulf Hansson <ulf.hansson@...aro.org> wrote:
>
> Hi Victor.
>
> On Tue, 13 Dec 2022 at 10:01, Victor Shih <victorshihgli@...il.com> wrote:
> >
> > Add UHS-II specific data structures for commands and defines for
> > registers, as described in Part 1 UHS-II Addendum Version 1.01.
> >
> > UHS-II related definitions are listed below:
> > 1. UHS-II card capability: sd_uhs2_caps{}
> > 2. UHS-II configuration: sd_uhs2_config{}
> > 3. UHS-II Command structure: uhs2_command{}
> > 4. UHS-II register I/O address and register field definitions: sd_uhs2.h
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@...aro.org>
> > Signed-off-by: Jason Lai <jason.lai@...esyslogic.com.tw>
> > Signed-off-by: Victor Shih <victor.shih@...esyslogic.com.tw>
>
> I have looked through the series in detail and it's certainly moving
> in the right direction, thanks for being persistent and working on
> this!
>
> > ---
> > include/linux/mmc/card.h | 42 +++++-
> > include/linux/mmc/core.h | 13 ++
> > include/linux/mmc/host.h | 63 ++++++++-
> > include/linux/mmc/sd_uhs2.h | 263 ++++++++++++++++++++++++++++++++++++
> > 4 files changed, 379 insertions(+), 2 deletions(-)
> > create mode 100644 include/linux/mmc/sd_uhs2.h
> >
> > diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> > index 4a42f31b7bb0..d638a2c689cf 100644
> > --- a/include/linux/mmc/card.h
> > +++ b/include/linux/mmc/card.h
> > @@ -190,6 +190,13 @@ struct sd_switch_caps {
> > #define SD_MAX_CURRENT_400 (1 << SD_SET_CURRENT_LIMIT_400)
> > #define SD_MAX_CURRENT_600 (1 << SD_SET_CURRENT_LIMIT_600)
> > #define SD_MAX_CURRENT_800 (1 << SD_SET_CURRENT_LIMIT_800)
> > + unsigned int sd4_curr_limit;
>
> This is unused, can we drop it?
>
I will drop it and update in V7 version.
> > +#define SD4_SET_POWER_LIMIT_0_72W 0
> > +#define SD4_SET_POWER_LIMIT_1_44W 1
> > +#define SD4_SET_POWER_LIMIT_2_16W 2
> > +#define SD4_SET_POWER_LIMIT_2_88W 3
> > +#define SD4_SET_POWER_LIMIT_1_80W 4
> > +#define SD4_SET_POWER_NO_CHANGE (-1)
>
> SD4_SET_POWER_NO_CHANGE seems to be redundant, please drop it.
>
I will drop it and update in V7 version.
> Note that, I realize that the series only supports
> SD4_SET_POWER_LIMIT_1_80W, but I think it's fine to keep the others
> definitions, as they are just mirroring what exists in the SD spec.
> Right?
>
> > };
> >
> > struct sd_ext_reg {
> > @@ -213,7 +220,35 @@ struct sd_ext_reg {
> >
> > struct sd_uhs2_config {
> > u32 node_id;
> > - /* TODO: Extend with more register configs. */
> > +
> > + u32 dap;
> > + u32 gap;
> > + u32 n_fcu;
> > + u32 maxblk_len;
> > + u8 n_lanes;
> > + u8 dadr_len;
> > + u8 app_type;
> > + u8 phy_minor_rev;
> > + u8 phy_major_rev;
> > + u8 can_hibernate;
> > + u8 n_lss_sync;
> > + u8 n_lss_dir;
> > + u8 link_minor_rev;
> > + u8 link_major_rev;
> > + u8 dev_type;
> > + u8 n_data_gap;
> > +
> > + u32 n_fcu_set;
> > + u32 maxblk_len_set;
> > + u8 n_lanes_set;
> > + u8 speed_range_set;
> > + u8 n_lss_sync_set;
> > + u8 n_lss_dir_set;
> > + u8 n_data_gap_set;
> > + u8 pwrctrl_mode_set;
> > + u8 max_retry_set;
> > +
> > + u8 cfg_complete;
>
> There seems to be a couple of variables in the above struct that are
> unused (like dap, gap, cfg_complete, etc). Can we drop them?
>
I will drop it and update in V7 version.
> > };
> >
> > struct sdio_cccr {
> > @@ -323,6 +358,9 @@ struct mmc_card {
> > struct sd_ext_reg ext_perf; /* SD extension reg for PERF */
> >
> > struct sd_uhs2_config uhs2_config; /* SD UHS-II config */
> > + u8 uhs2_state; /* SD UHS-II states */
> > +#define MMC_UHS2_INITIALIZED BIT(1)
>
> Rather than introducing a new variable in the struct mmc_card for
> this, I suggest we add a new bool in the struct mmc_host for this. If
> I understand correctly, this flag indicates that the host
> communication is prepared to support SD-tran commands? Perhaps also
> name the variable to "uhs2_sd_tran", to try to reflect its purpose?
>
> Moreover, it looks to me that sd_uhs2_power_off() would be a good
> place for where to reset the flag, rather than having that sprinkled
> across several error paths.
>
> > +#define MMC_UHS2_SPEED_B BIT(2)
>
> I was looking through the series to figure out how this bit is being
> used. So, it gets set in sd_uhs2_config_write(), based upon whether
> the host and the card, both supports UHS2 speed mode B. Then, in
> sd_uhs2_config_write() we calls ->uhs2_control(host, UHS2_SET_CONFIG).
>
> By looking at the changes for the sdhci driver, the MMC_UHS2_SPEED_B
> bit gets checked at "UHS2_SET_CONFIG" through the ->uhs2_control()
> callbacks, to determine whether the SDHCI_UHS2_PHY_SET_SPEED_B bit
> should be written to the sdhci_uhs2_phy_set_reg or not. This seems
> reasonable to me. However, I wonder if the call to
> ->uhs2_control(host, UHS2_SET_SPEED_B) in sd_uhs2_change_speed() is
> then really needed, as it looks like that would just be re-writing the
> same bit again in sdhci. Is this deliberate and needed? If not, should
> we drop the call to ->uhs2_control(host, UHS2_SET_SPEED_B)?
>
> When it comes to the actual MMC_UHS2_SPEED_B bit, I think it better
> belongs in the ios->timing variable. In this regard, I suggest we add
> the below bits, to indicate the current UHS2 speed mode.
>
> MMC_TIMING_UHS2_SPEED_A
> MMC_TIMING_UHS2_SPEED_A_HD (HD for half-duplex)
> MMC_TIMING_UHS2_SPEED_B
> MMC_TIMING_UHS2_SPEED_B_HD (HD for half-duplex)
>
> These should help us to support all the various modes, for now. Let's
> leave UHS2 speed range C and D for later.
>
> I would also suggest extending mmc_card_uhs2(), that is introduced in
> patch3, to return true, if any of the bits above are being set.
>
> Finally, in sd_uhs2_power_up() I think it would be better to set the
> ->ios.timing to MMC_TIMING_UHS2_SPEED_A, as the initial value, rather
> than MMC_TIMING_SD_UHS2 (as was introduced in patch2). In other words,
> let's drop MMC_TIMING_SD_UHS2 in favor of MMC_TIMING_UHS2_SPEED_A.
>
> >
> > unsigned int sdio_funcs; /* number of SDIO functions */
> > atomic_t sdio_funcs_probed; /* number of probed SDIO funcs */
> > @@ -364,4 +402,6 @@ bool mmc_card_is_blockaddr(struct mmc_card *card);
> > #define mmc_card_sdio(c) ((c)->type == MMC_TYPE_SDIO)
> > #define mmc_card_sd_combo(c) ((c)->type == MMC_TYPE_SD_COMBO)
> >
> > +#define mmc_card_can_poweroff_notify(c) ((c)->ext_power.feature_support & SD_EXT_POWER_OFF_NOTIFY)
> > +
>
> Please move this to drivers/mmc/core/sd_ops.h and implement this as a
> regular c-function. I also suggest renaming it to
> mmc_sd_can_poweroff_notify().
>
> [...]
>
> > --- a/include/linux/mmc/host.h
> > +++ b/include/linux/mmc/host.h
> > @@ -16,6 +16,7 @@
> > #include <linux/mmc/pm.h>
> > #include <linux/dma-direction.h>
> > #include <linux/blk-crypto-profile.h>
> > +#include <linux/mmc/sd_uhs2.h>
> >
> > struct mmc_ios {
> > unsigned int clock; /* clock rate */
> > @@ -96,7 +97,48 @@ struct mmc_clk_phase_map {
> > };
> >
> > struct sd_uhs2_caps {
> > - /* TODO: Add UHS-II capabilities for the host. */
> > + u32 dap;
> > + u32 gap;
> > + u32 group_desc;
> > + u32 maxblk_len;
> > + u32 n_fcu;
> > + u8 n_lanes;
> > + u8 addr64;
> > + u8 card_type;
> > + u8 phy_rev;
> > + u8 speed_range;
> > + u8 can_hibernate;
> > + u8 n_lss_sync;
> > + u8 n_lss_dir;
> > + u8 link_rev;
> > + u8 host_type;
> > + u8 n_data_gap;
> > +
> > + u32 maxblk_len_set;
> > + u32 n_fcu_set;
> > + u8 n_lanes_set;
> > + u8 n_lss_sync_set;
> > + u8 n_lss_dir_set;
> > + u8 n_data_gap_set;
> > + u8 max_retry_set;
>
> As I found a few unused variables in the earlier struct definitions,
> please walk through this too, to make sure we don't waste data.
>
I will drop it and update in V7 version.
> > +};
> > +
> > +struct sd_uhs2_ios {
> > + bool is_2L_HD_mode;
>
> I think we should use the ios->timing variable for this instead. If
> the two data-lines are supported to run in half-duplex mode, I think
> it's better to use one of the below bits, as I kind of also suggested
> earlier above.
>
> MMC_TIMING_UHS2_SPEED_A_HD
> MMC_TIMING_UHS2_SPEED_B_HD
>
> As "is_2L_HD_mode" seems to be checked at couple of different places
> in the core and in sdhci, perhaps we should add a new helper function,
> "mmc_card_uhs2_hd_mode()" that would return true if any of the above
> bits are set?
>
> > + bool is_APP_CMD;
>
> I think this better belongs in the struct mmc_host, please move it
> there and then also rename it into "uhs2_app_cmd". Note that, there is
> also one case in mmc_sd_num_wr_blocks() that sends an MMC_APP_CMD, but
> without calling mmc_app_cmd(). This needs to be fixed.
>
> > + unsigned int power_delay_ms; /* waiting for stable power */
>
> This seems to be unused, so let's drop it!
>
This power_delay_ms is used in the sd_uhs2_power_up function.
> > +};
> > +
> > +enum sd_uhs2_operation {
> > + UHS2_PHY_INIT = 0,
> > + UHS2_SET_CONFIG,
> > + UHS2_ENABLE_INT,
> > + UHS2_DISABLE_INT,
> > + UHS2_ENABLE_CLK,
> > + UHS2_DISABLE_CLK,
> > + UHS2_CHECK_DORMANT,
> > + UHS2_SET_SPEED_B,
> > + UHS2_POST_ATTACH_SD,
> > };
> >
> > struct mmc_host;
> > @@ -231,6 +273,14 @@ struct mmc_host_ops {
> >
> > /* Initialize an SD express card, mandatory for MMC_CAP2_SD_EXP. */
> > int (*init_sd_express)(struct mmc_host *host, struct mmc_ios *ios);
> > +
> > + /*
> > + * The uhs2_control callback is used to execute SD UHS-II specific
> > + * operations. It's mandatory to implement for hosts that supports the
> > + * SD UHS-II interface (MMC_CAP2_SD_UHS2). Expected return values are a
> > + * negative errno in case of a failure or zero for success.
> > + */
> > + int (*uhs2_control)(struct mmc_host *host, enum sd_uhs2_operation op);
> > };
> >
> > struct mmc_cqe_ops {
> > @@ -323,6 +373,7 @@ struct mmc_pwrseq;
> >
> > struct mmc_supply {
> > struct regulator *vmmc; /* Card power supply */
> > + struct regulator *vmmc2; /* UHS2 VDD2 power supply */
>
> In patch4 I added support for vqmmc2, which is targeted to be used for
> VDD2. Is there any reason that can't be used?
>
> > struct regulator *vqmmc; /* Optional Vccq supply */
> > struct regulator *vqmmc2; /* Optional supply for phy */
> > };
> > @@ -344,10 +395,12 @@ struct mmc_host {
> > u32 ocr_avail_sdio; /* SDIO-specific OCR */
> > u32 ocr_avail_sd; /* SD-specific OCR */
> > u32 ocr_avail_mmc; /* MMC-specific OCR */
> > + u32 ocr_avail_uhs2; /* UHS2-specific OCR */
>
> The way I implemented support for vqmmc2, we shouldn't need a specific
> ocr mask for UHS2. So, unless I am mistaken we should be able to use
> only 1.8V, for now.
>
> > struct wakeup_source *ws; /* Enable consume of uevents */
> > u32 max_current_330;
> > u32 max_current_300;
> > u32 max_current_180;
> > + u32 max_current_180_vdd2; /* UHS2 vdd2 max curt. */
>
> This looks sdhci specific. Should we move it to the sdhci struct instead?
>
> >
> > #define MMC_VDD_165_195 0x00000080 /* VDD voltage 1.65 - 1.95 */
> > #define MMC_VDD_20_21 0x00000100 /* VDD voltage 2.0 ~ 2.1 */
> > @@ -366,6 +419,7 @@ struct mmc_host {
> > #define MMC_VDD_33_34 0x00200000 /* VDD voltage 3.3 ~ 3.4 */
> > #define MMC_VDD_34_35 0x00400000 /* VDD voltage 3.4 ~ 3.5 */
> > #define MMC_VDD_35_36 0x00800000 /* VDD voltage 3.5 ~ 3.6 */
> > +#define MMC_VDD2_165_195 0x00000080 /* UHS2 VDD2 1.65 ~ 1.95 */
>
> Please drop this. We already have MMC_VDD_165_195, for these bits, see above.
>
I will drop it and update in V7 version.
> >
> > u32 caps; /* Host capabilities */
> >
> > @@ -443,7 +497,11 @@ struct mmc_host {
> > #endif
> > #define MMC_CAP2_ALT_GPT_TEGRA (1 << 28) /* Host with eMMC that has GPT entry at a non-standard location */
> >
> > + int flags;
> > +#define MMC_UHS2_SUPPORT (1 << 0)
>
> According to my comments above, I don't think this is needed at all.
> Please drop it.
>
> > +
> > struct sd_uhs2_caps uhs2_caps; /* Host UHS-II capabilities */
> > + struct sd_uhs2_ios uhs2_ios; /* Host UHS-II capabilities */
>
> As indicated above, the struct sd_uhs2_ios can be dropped.
>
> >
> > int fixed_drv_type; /* fixed driver type for non-removable media */
> >
> > @@ -695,4 +753,7 @@ int mmc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error);
> > int mmc_send_abort_tuning(struct mmc_host *host, u32 opcode);
> > int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
> >
> > +#define mmc_uhs2_2L_HD_mode(h) ((h)->uhs2_ios.is_2L_HD_mode)
> > +#define mmc_uhs2_APP_cmd(h) ((h)->uhs2_ios.is_APP_CMD)
> > +
>
> These are unused. Please drop or replace them with something that can be used.
>
I will drop it and update in V7 version.
> > #endif /* LINUX_MMC_HOST_H */
> > diff --git a/include/linux/mmc/sd_uhs2.h b/include/linux/mmc/sd_uhs2.h
> > new file mode 100644
> > index 000000000000..8fcf702cf4a5
> > --- /dev/null
> > +++ b/include/linux/mmc/sd_uhs2.h
> > @@ -0,0 +1,263 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Header file for UHS-II packets, Host Controller registers and I/O
> > + * accessors.
> > + *
> > + * Copyright (C) 2014 Intel Corp, All Rights Reserved.
> > + */
> > +#ifndef LINUX_MMC_UHS2_H
> > +#define LINUX_MMC_UHS2_H
> > +
> > +/* LINK Layer definition */
> > +/*
> > + * UHS2 Header:
> > + * Refer to UHS-II Addendum Version 1.02 Figure 5-2, the format of CCMD Header is described below:
> > + * bit [3:0] : DID(Destination ID = Node ID of UHS2 card)
> > + * bit [6:4] : TYP(Packet Type)
> > + * 000b: CCMD(Control command packet)
> > + * 001b: DCMD(Data command packet)
> > + * 010b: RES(Response packet)
> > + * 011b: DATA(Data payload packet)
> > + * 111b: MSG(Message packet)
> > + * Others: Reserved
> > + * bit [7] : NP(Native Packet)
> > + * bit [10:8] : TID(Transaction ID)
> > + * bit [11] : Reserved
> > + * bit [15:12]: SID(Source ID 0: Node ID of Host)
> > + *
> > + * Broadcast CCMD issued by Host is represented as DID=SID=0.
> > + */
> > +/*
> > + * UHS2 Argument:
> > + * Refer to UHS-II Addendum Version 1.02 Figure 6-5, the format of CCMD Argument is described below:
> > + * bit [3:0] : MSB of IOADR
> > + * bit [5:4] : PLEN(Payload Length)
> > + * 00b: 0 byte
> > + * 01b: 4 bytes
> > + * 10b: 8 bytes
> > + * 11b: 16 bytes
> > + * bit [6] : Reserved
> > + * bit [7] : R/W(Read/Write)
> > + * 0: Control read command
> > + * 1: Control write command
> > + * bit [15:8] : LSB of IOADR
> > + *
> > + * I/O Address specifies the address of register in UHS-II I/O space accessed by CCMD.
> > + * The unit of I/O Address is 4 Bytes. It is transmitted in MSB first, LSB last.
> > + */
> > +#define UHS2_NATIVE_PACKET_POS 7
> > +#define UHS2_NATIVE_PACKET (1 << UHS2_NATIVE_PACKET_POS)
> > +
> > +#define UHS2_PACKET_TYPE_POS 4
> > +#define UHS2_PACKET_TYPE_CCMD (0 << UHS2_PACKET_TYPE_POS)
> > +#define UHS2_PACKET_TYPE_DCMD (1 << UHS2_PACKET_TYPE_POS)
> > +#define UHS2_PACKET_TYPE_RES (2 << UHS2_PACKET_TYPE_POS)
> > +#define UHS2_PACKET_TYPE_DATA (3 << UHS2_PACKET_TYPE_POS)
> > +#define UHS2_PACKET_TYPE_MSG (7 << UHS2_PACKET_TYPE_POS)
> > +
> > +#define UHS2_DEST_ID_MASK 0x0F
> > +#define UHS2_DEST_ID 0x1
> > +
> > +#define UHS2_SRC_ID_POS 12
> > +#define UHS2_SRC_ID_MASK 0xF000
> > +
> > +#define UHS2_TRANS_ID_POS 8
> > +#define UHS2_TRANS_ID_MASK 0x0700
> > +
> > +/* UHS2 MSG */
> > +#define UHS2_MSG_CTG_POS 5
> > +#define UHS2_MSG_CTG_LMSG 0x00
> > +#define UHS2_MSG_CTG_INT 0x60
> > +#define UHS2_MSG_CTG_AMSG 0x80
> > +
> > +#define UHS2_MSG_CTG_FCREQ 0x00
> > +#define UHS2_MSG_CTG_FCRDY 0x01
> > +#define UHS2_MSG_CTG_STAT 0x02
> > +
> > +#define UHS2_MSG_CODE_POS 8
> > +#define UHS2_MSG_CODE_FC_UNRECOVER_ERR 0x8
> > +#define UHS2_MSG_CODE_STAT_UNRECOVER_ERR 0x8
> > +#define UHS2_MSG_CODE_STAT_RECOVER_ERR 0x1
> > +
> > +/* TRANS Layer definition */
> > +
> > +/* Native packets*/
> > +#define UHS2_NATIVE_CMD_RW_POS 7
> > +#define UHS2_NATIVE_CMD_WRITE (1 << UHS2_NATIVE_CMD_RW_POS)
> > +#define UHS2_NATIVE_CMD_READ (0 << UHS2_NATIVE_CMD_RW_POS)
> > +
> > +#define UHS2_NATIVE_CMD_PLEN_POS 4
> > +#define UHS2_NATIVE_CMD_PLEN_4B (1 << UHS2_NATIVE_CMD_PLEN_POS)
> > +#define UHS2_NATIVE_CMD_PLEN_8B (2 << UHS2_NATIVE_CMD_PLEN_POS)
> > +#define UHS2_NATIVE_CMD_PLEN_16B (3 << UHS2_NATIVE_CMD_PLEN_POS)
> > +
> > +#define UHS2_NATIVE_CCMD_GET_MIOADR_MASK 0xF00
> > +#define UHS2_NATIVE_CCMD_MIOADR_MASK 0x0F
> > +
> > +#define UHS2_NATIVE_CCMD_LIOADR_POS 8
> > +#define UHS2_NATIVE_CCMD_GET_LIOADR_MASK 0x0FF
> > +
> > +#define UHS2_CCMD_DEV_INIT_COMPLETE_FLAG BIT(11)
> > +#define UHS2_DEV_INIT_PAYLOAD_LEN 1
> > +#define UHS2_DEV_INIT_RESP_LEN 6
> > +#define UHS2_DEV_ENUM_PAYLOAD_LEN 1
> > +#define UHS2_DEV_ENUM_RESP_LEN 8
> > +#define UHS2_CFG_WRITE_PAYLOAD_LEN 2
> > +#define UHS2_CFG_WRITE_PHY_SET_RESP_LEN 4
> > +#define UHS2_CFG_WRITE_GENERIC_SET_RESP_LEN 5
> > +#define UHS2_GO_DORMANT_PAYLOAD_LEN 1
> > +
> > +/*
> > + * UHS2 Argument:
> > + * Refer to UHS-II Addendum Version 1.02 Figure 6-8, the format of DCMD Argument is described below:
> > + * bit [3:0] : Reserved
> > + * bit [6:3] : TMODE(Transfer Mode)
> > + * bit 3: DAM(Data Access Mode)
> > + * bit 4: TLUM(TLEN Unit Mode)
> > + * bit 5: LM(Length Mode)
> > + * bit 6: DM(Duplex Mode)
> > + * bit [7] : R/W(Read/Write)
> > + * 0: Control read command
> > + * 1: Control write command
> > + * bit [15:8] : Reserved
> > + *
> > + * I/O Address specifies the address of register in UHS-II I/O space accessed by CCMD.
> > + * The unit of I/O Address is 4 Bytes. It is transmitted in MSB first, LSB last.
> > + */
> > +#define UHS2_DCMD_DM_POS 6
> > +#define UHS2_DCMD_2L_HD_MODE (1 << UHS2_DCMD_DM_POS)
> > +#define UHS2_DCMD_LM_POS 5
> > +#define UHS2_DCMD_LM_TLEN_EXIST (1 << UHS2_DCMD_LM_POS)
> > +#define UHS2_DCMD_TLUM_POS 4
> > +#define UHS2_DCMD_TLUM_BYTE_MODE (1 << UHS2_DCMD_TLUM_POS)
> > +#define UHS2_NATIVE_DCMD_DAM_POS 3
> > +#define UHS2_NATIVE_DCMD_DAM_IO (1 << UHS2_NATIVE_DCMD_DAM_POS)
> > +
> > +/*
> > + * Per UHS2 spec, DCMD payload should be MSB first. There may be
> > + * two types of data be assembled to MSB:
> > + * 1. TLEN: Input block size for single read/write and number of blocks
> > + * for multiple read/write to calculate TLEN as MSB first per spec.
> > + * 2. SD command argument.
> > + */
> > +static inline __be32 uhs2_dcmd_convert_msb(u32 input)
> > +{
> > + u32 ret = 0;
> > +
> > + ret = ((input & 0xFF) << 24) |
> > + (((input >> 8) & 0xFF) << 16) |
> > + (((input >> 16) & 0xFF) << 8) |
> > + ((input >> 24) & 0xFF);
> > + return cpu_to_be32(ret);
> > +}
>
> This looks unused, can we drop it?
>
I will drop it and update in V7 version.
> > +
> > +#define UHS2_RES_NACK_POS 7
> > +#define UHS2_RES_NACK_MASK (0x1 << UHS2_RES_NACK_POS)
> > +
> > +#define UHS2_RES_ECODE_POS 4
> > +#define UHS2_RES_ECODE_MASK 0x7
> > +#define UHS2_RES_ECODE_COND 1
> > +#define UHS2_RES_ECODE_ARG 2
> > +#define UHS2_RES_ECODE_GEN 3
> > +
> > +/* IOADR of device registers */
> > +#define UHS2_IOADR_GENERIC_CAPS 0x00
> > +#define UHS2_IOADR_PHY_CAPS 0x02
> > +#define UHS2_IOADR_LINK_CAPS 0x04
> > +#define UHS2_IOADR_RSV_CAPS 0x06
> > +#define UHS2_IOADR_GENERIC_SETTINGS 0x08
> > +#define UHS2_IOADR_PHY_SETTINGS 0x0A
> > +#define UHS2_IOADR_LINK_SETTINGS 0x0C
> > +#define UHS2_IOADR_PRESET 0x40
> > +
> > +/* SD application packets */
> > +#define UHS2_SD_CMD_INDEX_POS 8
> > +
> > +#define UHS2_SD_CMD_APP_POS 14
> > +#define UHS2_SD_CMD_APP (1 << UHS2_SD_CMD_APP_POS)
> > +
> > +/* UHS-II Device Registers */
> > +#define UHS2_DEV_CONFIG_REG 0x000
> > +
> > +/* General Caps and Settings registers */
> > +#define UHS2_DEV_CONFIG_GEN_CAPS (UHS2_DEV_CONFIG_REG + 0x000)
> > +#define UHS2_DEV_CONFIG_N_LANES_POS 8
> > +#define UHS2_DEV_CONFIG_N_LANES_MASK 0x3F
> > +#define UHS2_DEV_CONFIG_2L_HD_FD 0x1
> > +#define UHS2_DEV_CONFIG_2D1U_FD 0x2
> > +#define UHS2_DEV_CONFIG_1D2U_FD 0x4
> > +#define UHS2_DEV_CONFIG_2D2U_FD 0x8
> > +#define UHS2_DEV_CONFIG_DADR_POS 14
> > +#define UHS2_DEV_CONFIG_DADR_MASK 0x1
> > +#define UHS2_DEV_CONFIG_APP_POS 16
> > +#define UHS2_DEV_CONFIG_APP_MASK 0xFF
> > +#define UHS2_DEV_CONFIG_APP_SD_MEM 0x1
> > +
> > +#define UHS2_DEV_CONFIG_GEN_SET (UHS2_DEV_CONFIG_REG + 0x008)
> > +#define UHS2_DEV_CONFIG_GEN_SET_N_LANES_POS 8
> > +#define UHS2_DEV_CONFIG_GEN_SET_2L_FD_HD 0x0
> > +#define UHS2_DEV_CONFIG_GEN_SET_2D1U_FD 0x2
> > +#define UHS2_DEV_CONFIG_GEN_SET_1D2U_FD 0x3
> > +#define UHS2_DEV_CONFIG_GEN_SET_2D2U_FD 0x4
> > +#define UHS2_DEV_CONFIG_GEN_SET_CFG_COMPLETE BIT(31)
> > +
> > +/* PHY Caps and Settings registers */
> > +#define UHS2_DEV_CONFIG_PHY_CAPS (UHS2_DEV_CONFIG_REG + 0x002)
> > +#define UHS2_DEV_CONFIG_PHY_MINOR_MASK 0xF
> > +#define UHS2_DEV_CONFIG_PHY_MAJOR_POS 4
> > +#define UHS2_DEV_CONFIG_PHY_MAJOR_MASK 0x3
> > +#define UHS2_DEV_CONFIG_CAN_HIBER_POS 15
> > +#define UHS2_DEV_CONFIG_CAN_HIBER_MASK 0x1
> > +#define UHS2_DEV_CONFIG_PHY_CAPS1 (UHS2_DEV_CONFIG_REG + 0x003)
> > +#define UHS2_DEV_CONFIG_N_LSS_SYN_MASK 0xF
> > +#define UHS2_DEV_CONFIG_N_LSS_DIR_POS 4
> > +#define UHS2_DEV_CONFIG_N_LSS_DIR_MASK 0xF
> > +
> > +#define UHS2_DEV_CONFIG_PHY_SET (UHS2_DEV_CONFIG_REG + 0x00A)
> > +#define UHS2_DEV_CONFIG_PHY_SET_SPEED_POS 6
> > +#define UHS2_DEV_CONFIG_PHY_SET_SPEED_A 0x0
> > +#define UHS2_DEV_CONFIG_PHY_SET_SPEED_B 0x1
> > +
> > +/* LINK-TRAN Caps and Settings registers */
> > +#define UHS2_DEV_CONFIG_LINK_TRAN_CAPS (UHS2_DEV_CONFIG_REG + 0x004)
> > +#define UHS2_DEV_CONFIG_LT_MINOR_MASK 0xF
> > +#define UHS2_DEV_CONFIG_LT_MAJOR_POS 4
> > +#define UHS2_DEV_CONFIG_LT_MAJOR_MASK 0x3
> > +#define UHS2_DEV_CONFIG_N_FCU_POS 8
> > +#define UHS2_DEV_CONFIG_N_FCU_MASK 0xFF
> > +#define UHS2_DEV_CONFIG_DEV_TYPE_POS 16
> > +#define UHS2_DEV_CONFIG_DEV_TYPE_MASK 0x7
> > +#define UHS2_DEV_CONFIG_MAX_BLK_LEN_POS 20
> > +#define UHS2_DEV_CONFIG_MAX_BLK_LEN_MASK 0xFFF
> > +#define UHS2_DEV_CONFIG_LINK_TRAN_CAPS1 (UHS2_DEV_CONFIG_REG + 0x005)
> > +#define UHS2_DEV_CONFIG_N_DATA_GAP_MASK 0xFF
> > +
> > +#define UHS2_DEV_CONFIG_LINK_TRAN_SET (UHS2_DEV_CONFIG_REG + 0x00C)
> > +#define UHS2_DEV_CONFIG_LT_SET_MAX_BLK_LEN 0x200
> > +#define UHS2_DEV_CONFIG_LT_SET_MAX_RETRY_POS 16
> > +
> > +/* Preset register */
> > +#define UHS2_DEV_CONFIG_PRESET (UHS2_DEV_CONFIG_REG + 0x040)
> > +
> > +#define UHS2_DEV_INT_REG 0x100
> > +
> > +#define UHS2_DEV_STATUS_REG 0x180
> > +
> > +#define UHS2_DEV_CMD_REG 0x200
> > +#define UHS2_DEV_CMD_FULL_RESET (UHS2_DEV_CMD_REG + 0x000)
> > +#define UHS2_DEV_CMD_GO_DORMANT_STATE (UHS2_DEV_CMD_REG + 0x001)
> > +#define UHS2_DEV_CMD_DORMANT_HIBER BIT(7)
> > +#define UHS2_DEV_CMD_DEVICE_INIT (UHS2_DEV_CMD_REG + 0x002)
> > +#define UHS2_DEV_INIT_COMPLETE_FLAG BIT(11)
> > +#define UHS2_DEV_CMD_ENUMERATE (UHS2_DEV_CMD_REG + 0x003)
> > +#define UHS2_DEV_CMD_TRANS_ABORT (UHS2_DEV_CMD_REG + 0x004)
> > +
> > +#define UHS2_RCLK_MAX 52000000
> > +#define UHS2_RCLK_MIN 26000000
> > +
> > +struct sd_uhs2_wait_active_state_data {
> > + struct mmc_host *host;
> > + struct mmc_command *cmd;
> > +};
>
> Please move this to drivers/mmc/core/sd_uhs2.c.
>
I will update it in V7 version.
> > +
> > +#endif /* LINUX_MMC_UHS2_H */
> > --
> > 2.25.1
> >
>
> I plan to have a closer look at the VDD2 things - but I think there
> should be plenty of stuff for us to discuss and hopefully for you to
> continue to work on, in the meantime.
>
> Kind regards
> Uffe
Thanks, Victor Shih
Powered by blists - more mailing lists