Re: [PATCH] mmc: core: Initial support for SD express card/host - Greg Kroah-Hartman

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Ulf Hansson <ulf.hansson@linaro.org>
Cc: "linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
	Christoph Hellwig <hch@lst.de>, Arnd Bergmann <arnd@arndb.de>,
	Rui Feng <rui_feng@realsil.com.cn>,
	linux-nvme@lists.infradead.org,
	Linux PCI <linux-pci@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Adrian Hunter <adrian.hunter@intel.com>
Subject: Re: [PATCH] mmc: core: Initial support for SD express card/host
Date: Thu, 16 Jul 2020 19:21:41 +0200	[thread overview]
Message-ID: <20200716172141.GA3143216@kroah.com> (raw)
In-Reply-To: <CAPDyKFr6eSZr2V-=YvAEZH9SmsE2SZ9j5ZS5zZEFHBqwyWRfpw@mail.gmail.com>

On Thu, Jul 16, 2020 at 07:04:21PM +0200, Ulf Hansson wrote:
> On Thu, 16 Jul 2020 at 18:14, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> 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.

Ah, good point.

Ok, no objection from me!

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>