| Message ID | 1502366898-23691-4-git-send-email-adrian.hunter@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 10, 2017 at 2:08 PM, Adrian Hunter <adrian.hunter@intel.com> wrote: > Add CQE host operations, capabilities, and host members. > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> This looks like a clean and nice host-API to me. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 August 2017 at 14:08, Adrian Hunter <adrian.hunter@intel.com> wrote: > Add CQE host operations, capabilities, and host members. > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> Thanks, applied for next! However with some minor comments, explained below. [...] > > + /* Command Queue Engine (CQE) support */ > + const struct mmc_cqe_ops *cqe_ops; > + void *cqe_private; > + /* > + * Notify uppers layers (e.g. mmc block driver) that CQE needs recovery > + * due to an error associated with the mmc_request. > + */ Thanks for adding the explanation. > + void (*cqe_recovery_notifier)(struct mmc_host *, > + struct mmc_request *); Normally we don't put callbacks in the struct mmc_host that someone else than the host driver should assign - so this feels a bit upside down. Is there any reason to why you didn't want to add a new API? Something like mmc_cqe_recover(), which the host driver could call. > + int cqe_qdepth; > + bool cqe_enabled; > + bool cqe_on; > + > unsigned long private[0] ____cacheline_aligned; > }; > > -- > 1.9.1 > Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/08/17 14:13, Ulf Hansson wrote: > On 10 August 2017 at 14:08, Adrian Hunter <adrian.hunter@intel.com> wrote: >> + void (*cqe_recovery_notifier)(struct mmc_host *, >> + struct mmc_request *); > > Normally we don't put callbacks in the struct mmc_host that someone > else than the host driver should assign - so this feels a bit upside > down. > > Is there any reason to why you didn't want to add a new API? Something > like mmc_cqe_recover(), which the host driver could call. That would make the host driver dependent on the block driver. There needs to be a function pointer, even if we wrap it in an API. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23 August 2017 at 08:54, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 22/08/17 14:13, Ulf Hansson wrote: >> On 10 August 2017 at 14:08, Adrian Hunter <adrian.hunter@intel.com> wrote: >>> + void (*cqe_recovery_notifier)(struct mmc_host *, >>> + struct mmc_request *); >> >> Normally we don't put callbacks in the struct mmc_host that someone >> else than the host driver should assign - so this feels a bit upside >> down. >> >> Is there any reason to why you didn't want to add a new API? Something >> like mmc_cqe_recover(), which the host driver could call. > > That would make the host driver dependent on the block driver. There needs > to be a function pointer, even if we wrap it in an API. Okay, I see. I guess we could put such pointer somewhere closer to the mmc request queue. Anyway, I didn't find out how this pointer was being protected from concurrent access or perhaps that is managed via mmc_claim|release_host()? Moreover, I could find it ever it being reset to NULL. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23/08/17 15:48, Ulf Hansson wrote: > On 23 August 2017 at 08:54, Adrian Hunter <adrian.hunter@intel.com> wrote: >> On 22/08/17 14:13, Ulf Hansson wrote: >>> On 10 August 2017 at 14:08, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>> + void (*cqe_recovery_notifier)(struct mmc_host *, >>>> + struct mmc_request *); >>> >>> Normally we don't put callbacks in the struct mmc_host that someone >>> else than the host driver should assign - so this feels a bit upside >>> down. >>> >>> Is there any reason to why you didn't want to add a new API? Something >>> like mmc_cqe_recover(), which the host driver could call. >> >> That would make the host driver dependent on the block driver. There needs >> to be a function pointer, even if we wrap it in an API. > > Okay, I see. I guess we could put such pointer somewhere closer to the > mmc request queue. Another possibility is to put in into struct mmc_request like the "done" callback. > > Anyway, I didn't find out how this pointer was being protected from > concurrent access or perhaps that is managed via > mmc_claim|release_host()? It is assumed CQE is only used by the card driver so the callback can be set / reset in the card driver's probe / remove. > Moreover, I could find it ever it being > reset to NULL. Yeah, doesn't look like it gets reset. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 24 August 2017 at 08:53, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 23/08/17 15:48, Ulf Hansson wrote: >> On 23 August 2017 at 08:54, Adrian Hunter <adrian.hunter@intel.com> wrote: >>> On 22/08/17 14:13, Ulf Hansson wrote: >>>> On 10 August 2017 at 14:08, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>> + void (*cqe_recovery_notifier)(struct mmc_host *, >>>>> + struct mmc_request *); >>>> >>>> Normally we don't put callbacks in the struct mmc_host that someone >>>> else than the host driver should assign - so this feels a bit upside >>>> down. >>>> >>>> Is there any reason to why you didn't want to add a new API? Something >>>> like mmc_cqe_recover(), which the host driver could call. >>> >>> That would make the host driver dependent on the block driver. There needs >>> to be a function pointer, even if we wrap it in an API. >> >> Okay, I see. I guess we could put such pointer somewhere closer to the >> mmc request queue. > > Another possibility is to put in into struct mmc_request like the "done" > callback. Yes, I think like that. That would also solve the problem of having to protect the pointer as the request would then have a corresponding life cycle to the block device driver. > >> >> Anyway, I didn't find out how this pointer was being protected from >> concurrent access or perhaps that is managed via >> mmc_claim|release_host()? > > It is assumed CQE is only used by the card driver so the callback can be set > / reset in the card driver's probe / remove. > >> Moreover, I could find it ever it being >> reset to NULL. > > Yeah, doesn't look like it gets reset. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index 4617c21f97f7..89e1d7e2953e 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -162,6 +162,50 @@ struct mmc_host_ops { unsigned int direction, int blk_size); }; +struct mmc_cqe_ops { + /* Allocate resources, and make the CQE operational */ + int (*cqe_enable)(struct mmc_host *host, struct mmc_card *card); + /* Free resources, and make the CQE non-operational */ + void (*cqe_disable)(struct mmc_host *host); + /* + * Issue a read, write or DCMD request to the CQE. Also deal with the + * effect of ->cqe_off(). + */ + int (*cqe_request)(struct mmc_host *host, struct mmc_request *mrq); + /* Free resources (e.g. DMA mapping) associated with the request */ + void (*cqe_post_req)(struct mmc_host *host, struct mmc_request *mrq); + /* + * Prepare the CQE and host controller to accept non-CQ commands. There + * is no corresponding ->cqe_on(), instead ->cqe_request() is required + * to deal with that. + */ + void (*cqe_off)(struct mmc_host *host); + /* + * Wait for all CQE tasks to complete. Return an error if recovery + * becomes necessary. + */ + int (*cqe_wait_for_idle)(struct mmc_host *host); + /* + * Notify CQE that a request has timed out. Return false if the request + * completed or true if a timeout happened in which case indicate if + * recovery is needed. + */ + bool (*cqe_timeout)(struct mmc_host *host, struct mmc_request *mrq, + bool *recovery_needed); + /* + * Stop all CQE activity and prepare the CQE and host controller to + * accept recovery commands. + */ + void (*cqe_recovery_start)(struct mmc_host *host); + /* + * Clear the queue and call mmc_cqe_request_done() on all requests. + * Requests that errored will have the error set on the mmc_request + * (data->error or cmd->error for DCMD). Requests that did not error + * will have zero data bytes transferred. + */ + void (*cqe_recovery_finish)(struct mmc_host *host); +}; + struct mmc_async_req { /* active mmc request */ struct mmc_request *mrq; @@ -307,6 +351,8 @@ struct mmc_host { #define MMC_CAP2_HS400_ES (1 << 20) /* Host supports enhanced strobe */ #define MMC_CAP2_NO_SD (1 << 21) /* Do not send SD commands during initialization */ #define MMC_CAP2_NO_MMC (1 << 22) /* Do not send (e)MMC commands during initialization */ +#define MMC_CAP2_CQE (1 << 23) /* Has eMMC command queue engine */ +#define MMC_CAP2_CQE_DCMD (1 << 24) /* CQE can issue a direct command */ mmc_pm_flag_t pm_caps; /* supported pm features */ @@ -390,6 +436,19 @@ struct mmc_host { int dsr_req; /* DSR value is valid */ u32 dsr; /* optional driver stage (DSR) value */ + /* Command Queue Engine (CQE) support */ + const struct mmc_cqe_ops *cqe_ops; + void *cqe_private; + /* + * Notify uppers layers (e.g. mmc block driver) that CQE needs recovery + * due to an error associated with the mmc_request. + */ + void (*cqe_recovery_notifier)(struct mmc_host *, + struct mmc_request *); + int cqe_qdepth; + bool cqe_enabled; + bool cqe_on; + unsigned long private[0] ____cacheline_aligned; };
Add CQE host operations, capabilities, and host members. Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- include/linux/mmc/host.h | 59 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+)