Re: [PATCH v5 3/3] mmc: Add driver for LiteX's LiteSDCard interface

[Ulf Hansson]

[...]

> > > +
> > > +static void litex_mmc_request(struct mmc_host *mmc, struct mmc_request *mrq)
> > > +{
> > > +       struct litex_mmc_host *host = mmc_priv(mmc);
> > > +       struct device *dev = mmc_dev(mmc);
> > > +       struct mmc_command *cmd = mrq->cmd;
> > > +       struct mmc_command *sbc = mrq->sbc;
> > > +       struct mmc_data *data = mrq->data;
> > > +       struct mmc_command *stop = mrq->stop;
> > > +       unsigned int retries = cmd->retries;
> > > +       unsigned int len = 0;
> > > +       bool direct = false;
> > > +       u32 response_len = litex_mmc_response_len(cmd);
> > > +       u8 transfer = SD_CTL_DATA_XFER_NONE;
> > > +
> > > +       /* First check that the card is still there */
> > > +       if (!litex_mmc_get_cd(mmc)) {
> > > +               cmd->error = -ENOMEDIUM;
> > > +               mmc_request_done(mmc, mrq);
> > > +               return;
> > > +       }
> > > +
> > > +       /* Send set-block-count command if needed */
> > > +       if (sbc) {
> > > +               sbc->error = litex_mmc_send_cmd(host, sbc->opcode, sbc->arg,
> > > +                                               litex_mmc_response_len(sbc),
> > > +                                               SD_CTL_DATA_XFER_NONE);
> > > +               if (sbc->error) {
> > > +                       host->is_bus_width_set = false;
> > > +                       mmc_request_done(mmc, mrq);
> > > +                       return;
> > > +               }
> > > +       }
> > > +
> > > +       if (data) {
> > > +               /* LiteSDCard only supports 4-bit bus width; therefore, we MUST
> > > +                * inject a SET_BUS_WIDTH (acmd6) before the very first data
> > > +                * transfer, earlier than when the mmc subsystem would normally
> > > +                * get around to it!
> >
> > This means that you may end up trying to switch bus-width, to a width
> > that isn't supported by the card, for example.
> >
> > As also stated above, I wonder how this conforms to the SD spec from
> > the initialization sequence point of view. Have you verified that this
> > isn't a problem?
>
> During litex_mmc_probe(), I have:
>
>         ...
>         ret = mmc_of_parse(mmc);
>         if (ret)
>                 goto err;
>
>         /* force 4-bit bus_width (only width supported by hardware) */
>         mmc->caps &= ~MMC_CAP_8_BIT_DATA;
>         mmc->caps |= MMC_CAP_4_BIT_DATA;
>         ...
>
> This ensures no bus-width switches to anything other than 4-bit data
> should ever occur. As far as I understand the SDcard spec, it's legal
> to both send multiple redundant bus-width-set commands, and to start
> doing so before the very first data transfer request is processed
> (regardless of the fact that linux typically does a few 1-bit-wide
> data transfers during card initialization before switching to a wider
> mode, if available).
>
> This driver simply ensures that any time we ever have a data transfer,
> the bus width is set to 4 *before* said transfer is acted upon.
>
> As I mentioned earlier, if we get a "weird" SDcard that can't support
> 4-bit data transfers, its initialization should fail shortly after
> detection, and that's all there is to it, as far as I can tell.

Alright, I get the point. I guess it should work. I will have another
closer look at the corresponding code from your last submitted
version.

>
> > > +                */
> > > +               cmd->error = litex_mmc_set_bus_width(host);
> > > +               if (cmd->error) {
> > > +                       dev_err(dev, "Can't set bus width!\n");
> > > +                       mmc_request_done(mmc, mrq);
> > > +                       return;
> > > +               }
> > > +
> > > +               litex_mmc_do_dma(host, data, &len, &direct, &transfer);
> > > +       }
> > > +
> > > +       do {
> > > +               cmd->error = litex_mmc_send_cmd(host, cmd->opcode, cmd->arg,
> > > +                                               response_len, transfer);
> > > +       } while (cmd->error && retries-- > 0);
> > > +
> > > +       if (cmd->error) {
> > > +               /* card may be gone; don't assume bus width is still set */
> > > +               host->is_bus_width_set = false;
> > > +       }
> > > +
> > > +       if (response_len == SD_CTL_RESP_SHORT) {
> > > +               /* pull short response fields from appropriate host registers */
> > > +               cmd->resp[0] = host->resp[3];
> > > +               cmd->resp[1] = host->resp[2] & 0xFF;
> > > +       } else if (response_len == SD_CTL_RESP_LONG) {
> > > +               cmd->resp[0] = host->resp[0];
> > > +               cmd->resp[1] = host->resp[1];
> > > +               cmd->resp[2] = host->resp[2];
> > > +               cmd->resp[3] = host->resp[3];
> > > +       }
> > > +
> > > +       /* Send stop-transmission command if required */
> > > +       if (stop && (cmd->error || !sbc)) {
> > > +               stop->error = litex_mmc_send_cmd(host, stop->opcode, stop->arg,
> > > +                                                litex_mmc_response_len(stop),
> > > +                                                SD_CTL_DATA_XFER_NONE);
> > > +               if (stop->error)
> > > +                       host->is_bus_width_set = false;
> > > +       }
> > > +
> > > +       if (data) {
> > > +               dma_unmap_sg(dev, data->sg, data->sg_len,
> > > +                            mmc_get_dma_dir(data));
> > > +       }
> > > +
> > > +       if (!cmd->error && transfer != SD_CTL_DATA_XFER_NONE) {
> > > +               data->bytes_xfered = min(len, mmc->max_req_size);
> > > +               if (transfer == SD_CTL_DATA_XFER_READ && !direct) {
> > > +                       sg_copy_from_buffer(data->sg, sg_nents(data->sg),
> > > +                                           host->buffer, data->bytes_xfered);
> > > +               }
> > > +       }
> > > +
> > > +       mmc_request_done(mmc, mrq);
> > > +}
> > > +
> >
> > [...]
> >
> > > +
> > > +       mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> >
> > I noticed that you use these hard coded values and don't really care
> > to manage voltage changes via ->set_ios().
> >
> > Rather than doing it like this, I would prefer if you can hook up a
> > fixed vmmc regulator in the DTS. Then call mmc_regulator_get_supply()
> > to fetch it from here, which will let the mmc core create the
> > mmc->ocr_avail mask, based upon the voltage level the regulator
> > supports.
> >
> > This becomes more generic and allows more flexibility for the platform
> > configuration.
>
> The LiteSDCard "hardware" (i.e., *gateware*) does not allow modification
> or selection of voltage from the software side. When a CMD8 is issued,
> the "voltage supplied" bit pattern is expected to be '0001b', which per
> the spec means "2.7-3.6V".

If you provide a range (2.7-3.6V), that means that your hardware
supports the entire range, not just one single part of it.

>
> I tried adding this to the overall DTS:
>
>         vreg_mmc: vreg_mmc_3v {
>                 compatible = "regulator-fixed";
>                 regulator-min-microvolt = <3300000>;
>                 regulator-max-microvolt = <3300000>;
>         };
>
> and then added a reference to it to the LiteSDCard "mmc0" node in DTS,
> like so:
>
>         mmc0: mmc@12005000 {
>                 compatible = "litex,mmc";
>                 reg = <0x12005000 0x100>,
>                         <0x12003800 0x100>,
>                         <0x12003000 0x100>,
>                         <0x12004800 0x100>,
>                         <0x12004000 0x100>;
>                 reg-names = "phy", "core", "reader", "writer", "irq";
>                 clocks = <&sys_clk>;
>                 vmmc-supply = <&vreg_mmc>; /* <-------- HERE !!! */
>                 interrupt-parent = <&L1>;
>                 interrupts = <4>;
>         };
>
> Finally, I replaced the hardcoded setting of `mmc->ocr_avail` with a
> call to `mmc_regulator_get_supply(mmc)`. Now, I get a bunch of timeouts
> during attempts to send e.g., CMD8 and CMD55.
> (going for 3200000 and 3400000 for min- and max-microvolt, respectively,
>  -- or anything else in the allowed 2.7-3.6 range -- doesn't help either).
>
> I might be doing something subtly wrong in the way I set things up
> above, but it feels a bit overengineered, and IMHO fragile.

At a quick glance, the above looks correct to me. Maybe there is
something wrong with the code in the driver instead?

>
> OTOH, going all out and setting:
>
>         /* allow for generic 2.7-3.6V range, no software tuning available */
>         mmc->ocr_avail = MMC_VDD_27_28 | MMC_VDD_28_29 | MMC_VDD_29_30 |
>                          MMC_VDD_30_31 | MMC_VDD_31_32 | MMC_VDD_32_33 |
>                          MMC_VDD_33_34 | MMC_VDD_34_35 | MMC_VDD_35_36;
>
> seems to work just fine... :) Please do let me know what you think!

No, this isn't the way we want it to work. That's because it means
that we would lie to the card about what voltage range the HW actually
supports.

It's better to let the DTS file give that information about the HW.

[...]

Kind regards
Uffe