Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

disk: sdhc: fix SPI clock setup #26319

Closed
wants to merge 3 commits into from
Closed

disk: sdhc: fix SPI clock setup #26319

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a463b8f
disk: sdhc: fix clock speed setup
vaussard Jun 19, 2020
37f6a27
disk: sdhc: boost the default speed
vaussard Jun 19, 2020
8f7db05
disk: sdhc: use the clock frequency from DTS
vaussard Jun 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 40 additions & 10 deletions subsys/disk/disk_access_spi_sdhc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,21 @@ LOG_MODULE_REGISTER(sdhc_spi, CONFIG_DISK_LOG_LEVEL);
#if !DT_NODE_HAS_STATUS(DT_INST(0, zephyr_mmc_spi_slot), okay)
#warning NO SDHC slot specified on board
#else
enum sdhc_spi_cfg {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we need this it would be less confusing and have less impact on potential namespace collisions if the enum name and the tag prefixes were consistent. Could you make this:

enum sdhc_spi_cfg {
	SDHC_SPI_CFG_INIT,
	SDHC_SPI_CFG_DEFAULT_SPEED,
	/* --- */
	SDHC_SPI_CFG_COUNT,
};

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any expectation that uses will require more configurations than the one used during device startup, and the one used during device operation? I'm going to assume "no", so subsequent comments will be from that perspective.

SDHC_SPI_INIT_CFG,
SDHC_SPI_DEFAULT_SPEED_CFG,
/* --- */
SDHC_CFG_COUNT,
};

struct sdhc_spi_data {
struct device *spi;
struct spi_config cfg;
struct spi_config spi_cfgs[SDHC_CFG_COUNT];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These structures are filled only with values that are known at the build time, so they could be extracted to the constant config_info structure for the device (which is currently not used at all; see the NULL in the DEVICE_AND_API_INIT call) and use static initialization. This would save some RAM and flash.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed; there should be a file-local type and object like:

struct spi_sdhc_config {
   struct spi_config init_cfg;
   struct spi_config oper_cfg;
} sdhc_spi_cfg_0;

with a static const instance that's passed via DEVICE_AND_API_INIT.

Then in this structure there should just be:

    const struct spi_config *spi_cfg;

that is assigned to either &config->init_cfg or &config->oper_cfg at the points where disk->status is changed to UNINIT or OK.

(just as @nzmichaelh recommended, now that I reach that comment.)

struct device *cs;
uint32_t pin;
gpio_dt_flags_t flags;

enum sdhc_spi_cfg current_spi_cfg;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional: change this to 'struct spi_config *spi_cfg'. This means you can drop the enum and the helper functions which makes the code easier to read.

bool high_capacity;
uint32_t sector_count;
uint8_t status;
Expand All @@ -40,6 +48,17 @@ struct sdhc_spi_data {

DEVICE_DECLARE(sdhc_spi_0);

static inline const struct spi_config *sdhc_get_spi_cfg(const struct sdhc_spi_data *data)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: each function in this file starts with 'sdhc_spi_'. Update the new functions to match like 'sdhc_spi_get_spi_cfg'

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need these if the pointer is maintained rather than the enum.

{
return &data->spi_cfgs[data->current_spi_cfg];
}

static inline void sdhc_select_spi_cfg(struct sdhc_spi_data *data,
enum sdhc_spi_cfg new_spi_cfg)
{
data->current_spi_cfg = new_spi_cfg;
}

/* Traces card traffic for LOG_LEVEL_DBG */
static int sdhc_spi_trace(struct sdhc_spi_data *data, int dir, int err,
const uint8_t *buf, int len)
Expand Down Expand Up @@ -103,7 +122,7 @@ static int sdhc_spi_rx_bytes(struct sdhc_spi_data *data, uint8_t *buf, int len)
};

return sdhc_spi_trace(data, -1,
spi_transceive(data->spi, &data->cfg, &tx, &rx),
spi_transceive(data->spi, sdhc_get_spi_cfg(data), &tx, &rx),
buf, len);
}

Expand Down Expand Up @@ -136,8 +155,8 @@ static int sdhc_spi_tx(struct sdhc_spi_data *data, const uint8_t *buf, int len)
};

return sdhc_spi_trace(data, 1,
spi_write(data->spi, &data->cfg, &tx), buf,
len);
spi_write(data->spi, sdhc_get_spi_cfg(data), &tx),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be come data->spi_cfg

buf, len);
}

/* Transmits the command and payload */
Expand Down Expand Up @@ -399,7 +418,7 @@ static int sdhc_spi_rx_block(struct sdhc_spi_data *data,
};

err = sdhc_spi_trace(data, -1,
spi_transceive(data->spi, &data->cfg,
spi_transceive(data->spi, sdhc_get_spi_cfg(data),
&tx, &rx),
&buf[i], remain);
if (err != 0) {
Expand Down Expand Up @@ -509,7 +528,7 @@ static int sdhc_spi_detect(struct sdhc_spi_data *data)
uint8_t buf[SDHC_CSD_SIZE];
bool is_v2;

data->cfg.frequency = SDHC_SPI_INITIAL_SPEED;
sdhc_select_spi_cfg(data, SDHC_SPI_INIT_CFG);
data->status = DISK_STATUS_UNINIT;

sdhc_retry_init(&retry, SDHC_INIT_TIMEOUT, SDHC_RETRY_DELAY);
Expand Down Expand Up @@ -647,7 +666,7 @@ static int sdhc_spi_detect(struct sdhc_spi_data *data)
buf[7], buf[8], sys_get_be32(&buf[9]));

/* Initilisation complete */
data->cfg.frequency = SDHC_SPI_SPEED;
sdhc_select_spi_cfg(data, SDHC_SPI_DEFAULT_SPEED_CFG);
data->status = DISK_STATUS_OK;

return 0;
Expand Down Expand Up @@ -768,9 +787,20 @@ static int sdhc_spi_init(struct device *dev)

data->spi = device_get_binding(DT_BUS_LABEL(DT_INST(0, zephyr_mmc_spi_slot)));

data->cfg.frequency = SDHC_SPI_INITIAL_SPEED;
data->cfg.operation = SPI_WORD_SET(8) | SPI_HOLD_ON_CS;
data->cfg.slave = DT_REG_ADDR(DT_INST(0, zephyr_mmc_spi_slot));
/* SPI config for initialization (default) */
sdhc_select_spi_cfg(data, SDHC_SPI_INIT_CFG);
data->spi_cfgs[SDHC_SPI_INIT_CFG] = (struct spi_config){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with this style, but I haven't seen it used much in the rest of Zephyr. People seem to prefer one line per member when this is inside a function.

No need to do anything unless someone else comments.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this style reads more naturally here, and is consistent with use like

var = (struct k_whatever)K_WHATEVER_INITIALIZER();

.frequency = SDHC_SPI_INITIAL_SPEED,
.operation = SPI_WORD_SET(8) | SPI_HOLD_ON_CS,
.slave = DT_REG_ADDR(DT_INST(0, zephyr_mmc_spi_slot)),
};
/* SPI config for default speed */
data->spi_cfgs[SDHC_SPI_DEFAULT_SPEED_CFG] = (struct spi_config){
.frequency = SDHC_SPI_SPEED,
.operation = SPI_WORD_SET(8) | SPI_HOLD_ON_CS,
.slave = DT_REG_ADDR(DT_INST(0, zephyr_mmc_spi_slot)),
};

data->cs = device_get_binding(
DT_SPI_DEV_CS_GPIOS_LABEL(DT_INST(0, zephyr_mmc_spi_slot)));
__ASSERT_NO_MSG(data->cs != NULL);
Expand Down