On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@xxxxxx> wrote:
> From: Ludovic Barre <ludovic.barre@xxxxxx>
>
> This patch adds command variant properties to define
> cpsm enable bit and responses.
> Needed to support the STM32 variant (shift of cpsm bit,
> specific definition of commands response).
>
> Signed-off-by: Ludovic Barre <ludovic.barre@xxxxxx>
> ---
> drivers/mmc/host/mmci.c | 47 +++++++++++++++++++++++++++++++++++++++++++----
> drivers/mmc/host/mmci.h | 8 ++++++++
> 2 files changed, 51 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 801c86b..52562fc 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -51,6 +51,10 @@ static unsigned int fmax = 515633;
> static struct variant_data variant_arm = {
> .fifosize = 16 * 4,
> .fifohalfsize = 8 * 4,
> + .cmdreg_cpsm_enable = MCI_CPSM_ENABLE,
> + .cmdreg_lrsp_crc = MCI_CPSM_RESPONSE | MCI_CPSM_LONGRSP,
> + .cmdreg_srsp_crc = MCI_CPSM_RESPONSE,
> + .cmdreg_srsp = MCI_CPSM_RESPONSE,
Do these really needs to be a superset of each other?
Maybe it becomes easier for the STM32 variant later though...
[...]
> @@ -603,16 +639,19 @@ mmci_start_command(struct mmci_host *host, struct mmc_command *cmd, u32 c)
While at it, would you mind folding in a cleanup patch removing the
"u32 c" as in-parameter? It's currently always set to "0" by the
caller, so it's not needed.
> dev_dbg(mmc_dev(host->mmc), "op %02x arg %08x flags %08x\n",
> cmd->opcode, cmd->arg, cmd->flags);
>
> - if (readl(base + MMCICOMMAND) & MCI_CPSM_ENABLE) {
> + if (readl(base + MMCICOMMAND) & host->variant->cmdreg_cpsm_enable) {
> writel(0, base + MMCICOMMAND);
> mmci_reg_delay(host);
> }
>
> - c |= cmd->opcode | MCI_CPSM_ENABLE;
> + c |= cmd->opcode | host->variant->cmdreg_cpsm_enable;
> if (cmd->flags & MMC_RSP_PRESENT) {
> if (cmd->flags & MMC_RSP_136)
> - c |= MCI_CPSM_LONGRSP;
> - c |= MCI_CPSM_RESPONSE;
> + c |= host->variant->cmdreg_lrsp_crc;
This looks weird. Probably because of the naming of the variant data.
Perhaps rename to "cmdreg_lrsp", thus skipping the "_crc" suffix?
> + else if (cmd->flags & MMC_RSP_CRC)
> + c |= host->variant->cmdreg_srsp_crc;
Why is here an else? We can have both MMC_RSP_136 and MMC_RSP_CRC bits
set, right!?
> + else
> + c |= host->variant->cmdreg_srsp;
What do you think about using a switch-case, perhaps with fall-through
- and then adding those bits that are needed for each response
bit+variant instead? Could that make this more readable?
> }
> if (/*interrupt*/0)
> c |= MCI_CPSM_INTERRUPT;
> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
> index 7265ca6..e173305 100644
> --- a/drivers/mmc/host/mmci.h
> +++ b/drivers/mmc/host/mmci.h
> @@ -239,6 +239,10 @@ struct mmci_host;
> * @clkreg_enable: enable value for MMCICLOCK register
> * @clkreg_8bit_bus_enable: enable value for 8 bit bus
> * @clkreg_neg_edge_enable: enable value for inverted data/cmd output
> + * @cmdreg_cpsm_enable: enable value for CPSM
> + * @cmdreg_lrsp_crc: enable value for long response with crc
> + * @cmdreg_srsp_crc: enable value for short response with crc
> + * @cmdreg_srsp: enable value for short response without crc
> * @datalength_bits: number of bits in the MMCIDATALENGTH register
> * @fifosize: number of bytes that can be written when MMCI_TXFIFOEMPTY
> * is asserted (likewise for RX)
> @@ -282,6 +286,10 @@ struct variant_data {
> unsigned int clkreg_enable;
> unsigned int clkreg_8bit_bus_enable;
> unsigned int clkreg_neg_edge_enable;
> + unsigned int cmdreg_cpsm_enable;
> + unsigned int cmdreg_lrsp_crc;
> + unsigned int cmdreg_srsp_crc;
> + unsigned int cmdreg_srsp;
> unsigned int datalength_bits;
> unsigned int fifosize;
> unsigned int fifohalfsize;
> --
> 2.7.4
>
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
