Re: [PATCH] mmc: core: Initial support for SD express card/host

[Ulf Hansson]

On Thu, 16 Jul 2020 at 18:14, Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, Jul 16, 2020 at 04:15:34PM +0200, Ulf Hansson wrote:
> > +int mmc_send_if_cond_pcie(struct mmc_host *host, u32 ocr)
> > +{
> > +     u32 resp = 0;
> > +     u8 pcie_bits = 0;
> > +     int ret;
> > +
> > +     if (host->caps2 & MMC_CAP2_SD_EXP) {
> > +             /* Probe card for SD express support via PCIe. */
> > +             pcie_bits = 0x10;
> > +             if (host->caps2 & MMC_CAP2_SD_EXP_1_2V)
> > +                     /* Probe also for 1.2V support. */
> > +                     pcie_bits = 0x30;
> > +     }
> > +
> > +     ret = __mmc_send_if_cond(host, ocr, pcie_bits, &resp);
> > +     if (ret)
> > +             return 0;
> > +
> > +     /* Continue with the SD express init, if the card supports it. */
> > +     resp &= 0x3000;
> > +     if (pcie_bits && resp) {
> > +             if (resp == 0x3000)
>
> 0x3000 should be some defined value, right?  Otherwise it just looks
> like magic bits :)

Yeah, I was considering that, but there are already lots of magic bits
around here in this code. On top of that, the bits are shifted,
depending on how they are used.

We should probably look into doing a cleanup, so this gets clearer overall.

>
> > --- a/include/linux/mmc/host.h
> > +++ b/include/linux/mmc/host.h
> > @@ -60,6 +60,8 @@ struct mmc_ios {
> >  #define MMC_TIMING_MMC_DDR52 8
> >  #define MMC_TIMING_MMC_HS200 9
> >  #define MMC_TIMING_MMC_HS400 10
> > +#define MMC_TIMING_SD_EXP    11
> > +#define MMC_TIMING_SD_EXP_1_2V       12
> >
> >       unsigned char   signal_voltage;         /* signalling voltage (1.8V or 3.3V) */
> >
> > @@ -172,6 +174,9 @@ struct mmc_host_ops {
> >        */
> >       int     (*multi_io_quirk)(struct mmc_card *card,
> >                                 unsigned int direction, int blk_size);
> > +
> > +     /* Initialize an SD express card, mandatory for MMC_CAP2_SD_EXP. */
> > +     int     (*init_sd_express)(struct mmc_host *host, struct mmc_ios *ios);
> >  };
> >
> >  struct mmc_cqe_ops {
> > @@ -357,6 +362,8 @@ struct mmc_host {
> >  #define MMC_CAP2_HS200_1_2V_SDR      (1 << 6)        /* can support */
> >  #define MMC_CAP2_HS200               (MMC_CAP2_HS200_1_8V_SDR | \
> >                                MMC_CAP2_HS200_1_2V_SDR)
> > +#define MMC_CAP2_SD_EXP              (1 << 7)        /* SD express via PCIe */
>
> BIT(7)?
>
> > +#define MMC_CAP2_SD_EXP_1_2V (1 << 8)        /* SD express 1.2V */
>
> BIT(8)?

I can change to that, but it wouldn't be consistent with existing
code. Again, probably better targeted as a separate bigger cleanup on
top.

>
> thanks,
>
> greg k-h

Thanks for reviewing!

Kind regards
Uffe