On August 18, 2023 8:27:48 AM GMT+02:00, Sascha Hauer <sha@xxxxxxxxxxxxxx> wrote:
> Hi Jules,
>
> Some small things inline.
>
>
> On Wed, Aug 16, 2023 at 11:39:45AM +0200, Jules Maselbas wrote:
> > +#include "sdhci.h"
> > +
> > +#define CARD_STATUS_MASK (0x1e00)
> > +#define CARD_STATUS_TRAN (4 << 9)
> > +static int dwcmshc_mci_send_cmd(struct mci_host *mci, struct mci_cmd *cmd,
> > + struct mci_data *data);
> > +
> > +struct dwcmshc_host {
> > + struct mci_host mci;
> > + struct sdhci sdhci;
> > + unsigned int in_abort_sequence;
> > +};
> > +
> > +#define priv_from_mci_host(h) container_of(h, struct dwcmshc_host, mci)
>
> Writing this as static inline function immediately makes the type of 'h'
> clear.
ack
>
> > +
> > +static void mci_setup_cmd(struct mci_cmd *p, unsigned int cmd, unsigned int arg,
> > + unsigned int response)
> > +{
> > + p->cmdidx = cmd;
> > + p->cmdarg = arg;
> > + p->resp_type = response;
> > +}
> > +
> > +static int do_abort_sequence(struct mci_host *mci, struct mci_cmd *current_cmd)
> > +{
> > + int ret = 0;
> > + struct dwcmshc_host *host = priv_from_mci_host(mci);
> > + struct mci_cmd cmd;
> > + u64 start;
> > +
> > + host->in_abort_sequence = 1;
> > +
> > + mci_setup_cmd(&cmd, MMC_CMD_STOP_TRANSMISSION, 0, MMC_RSP_R1b);
> > + ret = dwcmshc_mci_send_cmd(mci, &cmd, NULL);
> > + if (ret) {
> > + dev_err(host->mci.hw_dev, "Abort failed at first cmd12!\n");
> > + goto out;
> > + }
> > + mci_setup_cmd(&cmd, MMC_CMD_SEND_STATUS, mci->mci->rca << 16,
> > + MMC_RSP_R1);
> > + ret = dwcmshc_mci_send_cmd(mci, &cmd, NULL);
> > + if (ret) {
> > + dev_err(host->mci.hw_dev, "Abort failed at first cmd13!\n");
> > + goto out;
> > + }
> > +
> > + if ((cmd.response[0] & CARD_STATUS_MASK) == CARD_STATUS_TRAN)
> > + goto out; /* All is OK! */
> > +
> > + mci_setup_cmd(&cmd, MMC_CMD_STOP_TRANSMISSION, 0, MMC_RSP_R1b);
> > + ret = dwcmshc_mci_send_cmd(mci, &cmd, NULL);
> > + if (ret) {
> > + dev_err(host->mci.hw_dev, "Abort failed at second cmd12!\n");
> > + goto out;
> > + }
> > +
> > + mci_setup_cmd(&cmd, MMC_CMD_SEND_STATUS, mci->mci->rca << 16,
> > + MMC_RSP_R1);
> > + ret = dwcmshc_mci_send_cmd(mci, &cmd, NULL);
> > + if (ret) {
> > + dev_err(host->mci.hw_dev, "Abort failed at second cmd13!\n");
> > + goto out;
> > + }
> > +
> > + if ((cmd.response[0] & CARD_STATUS_MASK) == CARD_STATUS_TRAN) {
> > + goto out; /* All is OK! */
> > + } else {
> > + dev_err(host->mci.hw_dev,
> > + "Abort sequence failed to put card in TRAN state!\n");
> > + ret = 1;
>
> This should be an error code.
ack, not sure which error code is the most appropriated here, I am going to use EPROTO or EIO
>
> > + goto out;
> > + }
> > +
> > +out:
> > + /* Perform SW reset if in abort sequence */
> > + sdhci_write8(&host->sdhci, SDHCI_SOFTWARE_RESET,
> > + SDHCI_RESET_DATA | SDHCI_RESET_CMD);
> > + start = get_time_ns();
> > + while (sdhci_read8(&host->sdhci, SDHCI_SOFTWARE_RESET) != 0) {
> > + if (is_timeout(start, 50 * MSECOND)) {
> > + dev_err(host->mci.hw_dev,
> > + "SDHCI data reset timeout\n");
> > + break;
> > + }
> > + }
> > + host->in_abort_sequence = 0;
> > + return ret;
> > +}
> > +
> > +static int dwcmshc_mci_send_cmd(struct mci_host *mci, struct mci_cmd *cmd,
> > + struct mci_data *data)
> > +{
> > + struct dwcmshc_host *host = priv_from_mci_host(mci);
> > + dma_addr_t dma = SDHCI_NO_DMA;
> > + u32 mask, command, xfer;
> > + int ret;
> > +
> > + if (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION
> > + && host->in_abort_sequence == 0)
> > + return do_abort_sequence(mci, cmd);
>
> You could factor the rest of this function out to some dwcmshc_mci_do_send_cmd()
> and call it from here and do_abort_sequence().
ack
>
> > +
> > + /* Do not wait for CMD_INHIBIT_DAT on stop commands */
> > + mask = SDHCI_CMD_INHIBIT_CMD;
> > + if (cmd->cmdidx != MMC_CMD_STOP_TRANSMISSION)
> > + mask |= SDHCI_CMD_INHIBIT_DATA;
> > +
> > + /* Wait for bus idle */
> > + ret = wait_on_timeout(50 * MSECOND,
> > + !(sdhci_read16(&host->sdhci, SDHCI_PRESENT_STATE) & mask));
> > + if (ret) {
> > + dev_err(host->mci.hw_dev, "SDHCI timeout while waiting for idle\n");
> > + return -ETIMEDOUT;
> > + }
> > +
> > + if (data)
> > + dev_dbg(host->mci.hw_dev, "cmd %d arg %d bcnt %d bsize %d\n",
> > + cmd->cmdidx, cmd->cmdarg, data->blocks, data->blocksize);
> > + else
> > + dev_dbg(host->mci.hw_dev, "cmd %d arg %d\n", cmd->cmdidx, cmd->cmdarg);
> > +
> > + sdhci_write32(&host->sdhci, SDHCI_INT_STATUS, ~0);
> > +
> > + sdhci_setup_data_dma(&host->sdhci, data, &dma);
> > +
> > + sdhci_write8(&host->sdhci, SDHCI_TIMEOUT_CONTROL, 0xe);
> > +
> > + sdhci_set_cmd_xfer_mode(&host->sdhci, cmd, data,
> > + dma == SDHCI_NO_DMA ? false : true,
> > + &command, &xfer);
> > +
> > + sdhci_write16(&host->sdhci, SDHCI_TRANSFER_MODE, xfer);
> > +
> > + sdhci_write32(&host->sdhci, SDHCI_ARGUMENT, cmd->cmdarg);
> > + sdhci_write16(&host->sdhci, SDHCI_COMMAND, command);
> > +
> > + ret = sdhci_wait_for_done(&host->sdhci, SDHCI_INT_CMD_COMPLETE);
> > + if (ret)
> > + goto error;
> > +
> > + sdhci_read_response(&host->sdhci, cmd);
> > +
> > + ret = sdhci_transfer_data(&host->sdhci, data, dma);
> > +error:
> > + if (ret) {
> > + sdhci_reset(&host->sdhci, SDHCI_RESET_CMD);
> > + sdhci_reset(&host->sdhci, SDHCI_RESET_DATA);
> > + }
> > +
> > + sdhci_write32(&host->sdhci, SDHCI_INT_STATUS, ~0);
> > + return ret;
> > +}
> > +
> > +static void dwcmshc_mci_set_ios(struct mci_host *mci, struct mci_ios *ios)
> > +{
> > + struct dwcmshc_host *host = priv_from_mci_host(mci);
> > + u16 val;
> > +
> > + debug("%s: clock = %u, bus-width = %d, timing = %02x\n", __func__,
> > + ios->clock, ios->bus_width, ios->timing);
>
> dev_dbg()
ack
>
> > +static int dwcmshc_mci_init(struct mci_host *mci, struct device *dev)
> > +{
> > + struct dwcmshc_host *host = priv_from_mci_host(mci);
> > + u16 ctrl2;
> > +
> > + /* reset mshci controller */
> > + sdhci_write8(&host->sdhci, SDHCI_SOFTWARE_RESET, SDHCI_RESET_ALL);
> > +
> > + /* wait for reset completion */
> > + if (wait_on_timeout(100 * MSECOND,
> > + (sdhci_read8(&host->sdhci, SDHCI_SOFTWARE_RESET)
> > + & SDHCI_RESET_ALL) == 0)) {
> > + dev_err(dev, "SDHCI reset timeout\n");
> > + return -ETIMEDOUT;
> > + }
> > +
> > + sdhci_write16(&host->sdhci, SDHCI_INT_ERROR_ENABLE, ~0);
> > + sdhci_write16(&host->sdhci, SDHCI_INT_ENABLE, ~0);
> > + sdhci_write32(&host->sdhci, SDHCI_INT_STATUS, ~0);
> > + sdhci_write32(&host->sdhci, SDHCI_SIGNAL_ENABLE, ~0);
> > +
> > + sdhci_enable_v4_mode(&host->sdhci);
> > +
> > + dev_dbg(host->mci.hw_dev, "host version4: %s\n",
> > + ctrl2 & SDHCI_CTRL_V4_MODE ? "enabled" : "disabled");
> > +
> > + return 0;
> > +}
> > +
>
> [...]
>
> > +static int dwcmshc_probe(struct device *dev)
> > +{
> > + struct dwcmshc_host *host;
> > + struct resource *iores;
> > + struct mci_host *mci;
> > + struct clk *clk;
> > + int ret;
> > +
> > + host = xzalloc(sizeof(*host));
> > + mci = &host->mci;
> > +
> > + iores = dev_request_mem_resource(dev, 0);
> > + if (IS_ERR(iores)) {
> > + ret = PTR_ERR(iores);
> > + goto err_mem_req;
> > + }
> > +
> > + clk = clk_get(dev, NULL);
> > + if (IS_ERR(clk)) {
> > + ret = PTR_ERR(clk);
> > + goto err_clk_get;
> > + }
> > + clk_enable(clk);
> > +
> > + host->sdhci.base = IOMEM(iores->start);
> > + host->sdhci.mci = mci;
> > + host->sdhci.max_clk = clk_get_rate(clk);
> > +
> > + mci->hw_dev = dev;
> > + mci->init = dwcmshc_mci_init;
> > + mci->set_ios = dwcmshc_mci_set_ios;
> > + mci->send_cmd = dwcmshc_mci_send_cmd;
> > + mci->card_present = dwcmshc_mci_card_present;
> > +
> > + mci_of_parse(&host->mci);
> > +
> > + /* Enable host_version4 */
> > + sdhci_enable_v4_mode(&host->sdhci);
>
> This is also called from dwcmshc_mci_init(). Do we need both?
IIRC sdhci_enable_v4_mode needs to be called before sdhci_read_caps which is called by sdhci_setup_host (called during probe),
and I think it also needs to be called after the controller is reset.
>
> Sascha
>
