From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932367AbdCGLGe (ORCPT ); Tue, 7 Mar 2017 06:06:34 -0500 Received: from mail-bl2nam02on0084.outbound.protection.outlook.com ([104.47.38.84]:11808 "EHLO NAM02-BL2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1755461AbdCGLFu (ORCPT ); Tue, 7 Mar 2017 06:05:50 -0500 Authentication-Results: linaro.org; dkim=none (message not signed) header.d=none;linaro.org; dmarc=none action=none header.from=caviumnetworks.com; Date: Tue, 7 Mar 2017 11:49:22 +0100 From: Jan Glauber To: Ulf Hansson Cc: "linux-mmc@vger.kernel.org" , "linux-kernel@vger.kernel.org" , David Daney , "Steven J . Hill" , David Daney Subject: Re: [PATCH v11 2/9] mmc: cavium: Add core MMC driver for Cavium SOCs Message-ID: <20170307104922.GA19134@hardcore> References: <20170206133953.8390-1-jglauber@cavium.com> <20170206133953.8390-3-jglauber@cavium.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Originating-IP: [88.67.130.225] X-ClientProxiedBy: HE1PR0802CA0009.eurprd08.prod.outlook.com (10.172.123.147) To CO2PR07MB2582.namprd07.prod.outlook.com (10.166.201.21) X-MS-Office365-Filtering-Correlation-Id: 5a853082-6f4f-42bf-759d-08d46547a5cf X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001);SRVR:CO2PR07MB2582; X-Microsoft-Exchange-Diagnostics: 1;CO2PR07MB2582;3:wNpkFmAZsBsCqJs+4LmaoKIffV1kJgTe8wfGC5pp5MsbtQX7ZrvLn/GHkTVJJ4uvkos8SWywPavvPHGHCcgdnML4Nukoct5g46OBCrSykaz59g/UP00C3NiOIIiVn0IFTZKAf3KVDZgFDtWvaNsiS4TLU4xGDGJDj1tqefHgAUjsirf2xorHEASqnaRaeAo7yuGp7sW4TYsaqUBhW7BOymlGSRwaABkiQ0YDPgximppwGTnzW45tDWF8su10yw5g7skv260sQBd7yEfcgCqN9w==;25:tqMj5iUwx4thH2ZidC5298+kRXdOhoqI5SoB0ZoU+J/b7c+adlF9upfCejargm8+ZC9xcei4lsfB/wZ/BdSMAfaTCb9K0lEYwp9uTfZIOG0ho/YRVNpBlZ88XUZxuXOKaqUvAyghPNTw2jl/z+mfE4TRO+H98AsuGRsVrXMeAMNJjbpwqdNf3BHZ9b1SCy2y2R+u9pnEYQ918TENOORhWvbfI3WJ7oHlI4/ycHkJwqXoEv/rjkSEzkWUHB3hTETkwbfmgy+DVlHU82gqkNN87jzJTUPCc0N1u7CFQq9MDPKUmH3DzM1jpCusyb8ri9bUMsKz+VvW+edVMlCrTjnVtk4Vb7RPWCVZXsxWcBHI/IMK3xZBS9bMJT07Io1N/290GMpe/L613v6FRxsKhH0smqFTkknEOU5FGln4K8Wn3biREbavk9Jv5vj1VHrcAVRjlR5pGOqPc8DMhqHuACnVAw== X-Microsoft-Exchange-Diagnostics: 1;CO2PR07MB2582;31:GcDawnV0VY+FL8rjhFDoBKkk2nb5ZpITCKJHi+S3z/3NmXBraNIz6nJROduqzTpWkvtjgIBdWQdLx2vEFWqLsE0ayTWBkpGFwqom2I59QpB0kTQEByL+ScWEs2xTG36lSqbzJzrvg+UKBPXTI2jWqBPO0OLPYPss4qo+nx0yfIVtqkzOGjBWsa85o+k0tPNS4/HkO8o/yFCewLyYcv2FzbnXPcVyiXA0WDKTgHuIIwM=;20:JMa+4fRgWq5qV5WEeUi973SfbwUu67d8H0EGYaLskSVCL2vRlGRzuH7BrzFncLJ5PJwHeDTReZaU+xnQF/QvU3kHWGhI6W2U0sFVYUhWEUEYqkBMmy6hLJg1o8DPA3A4JTf48WxQbbZ4+b6QIMB4HuRcApcrkAmB5yJrl1W8vERjpfpHPygMNalrwsoqLxHNQqyyq7VMJerYzzkcovRg9vV3c+tr4uJVw/+Yt62vAK7H23GZOQYRzj6ixc9qBF1qW6hpVY+4C0Q4wORoJ/SNlTU/QEYQVnHJ+5AhbT3897miMGd+GWvJUub+SCOLDh48c7Js4nf2P6A49h6KUAJYJBHTkWUuuq8IAddlnW66DnZEAqep2DPCTrkNTOL546nu/4Q2JURdOnfu2JZAX5MBCGEo5TpTolHsQyYtPJAcoOsaO3MqNE9SFGOEAXGcSM23t1RzUtwEF+IdLO14hhUWxzLfI7bO4kVypDP4TcbY8z+2inFkRnfWj6VwkbF2x+/UmdKNw4uM1cdi7lmpsbOLtYmcMqpzvI7/2XZQHmpjyqyEnYbxHyimVW/krNukR3LtAKT40IK/3eijH+RS7MYNyEb0atLxp+G9Wy3wAzNSVA8= X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(6040375)(601004)(2401047)(8121501046)(5005006)(3002001)(10201501046)(6041248)(20161123562025)(20161123560025)(20161123555025)(20161123564025)(20161123558025)(6072148)(6042181);SRVR:CO2PR07MB2582;BCL:0;PCL:0;RULEID:;SRVR:CO2PR07MB2582; X-Microsoft-Exchange-Diagnostics: 1;CO2PR07MB2582;4:5Kb2RZ96gNS2/03N8UyfRCKj7w9vL/i/o65jueXWMp58J4pL+LQgAm4NtjtaKLekNYwqAcgk9Phs9ekvFeARe8Y1N5jr9VbZrYiJ+mzYG3qq+AverXAWMBRc6U5A5suXC9Rw3B9vrh/tCxyZvLGPrBnz4gEgtDJxf4/WSkiJwlpThFK7glSf/Tug+gZTLVAFEeT1HeSPulF6xF3MlwDvsK9opANJsw+EDD3PCSut3s8I8nfIq6Aps2wJN0EOfUeEhyr4JWESGIHMXivMyPlZL9lBvDbLoV0JAB++ZTlytLjs9RVFevcF8rrVgcrhJCUMssTqfKGfLGjIlna8mErOW+lPH03xaIxwA6SYK7OhQH8S1mKGP1yWEPQMT60murHfeYsbkyNQhpgnbsp1h7OOyraOmmS/Hk8Fbdox5sCCZlrekWPhjSnPuiCtCSbH+No1l+2aypH2/IhDzZyUYB0LxS579vyUeQp5IhmZy8prllyqE84lLzxc7k07LOCXVaeok6zWm6zO0lohf1YS1kETlssHZ+MJUmpzCvLeejDVx+uTrQN4cPZH9/JitYebBkUSBG0OaevGINX9+2PmvAOtEylIYWZ1ypabOPvPrIl8ljeprHgIvAtM7ZP3vrYAh/Sm X-Forefront-PRVS: 0239D46DB6 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(4630300001)(6009001)(7916002)(24454002)(35754003)(51914003)(53946003)(110136004)(7736002)(66066001)(107886003)(38730400002)(81166006)(4326008)(25786008)(54906002)(9686003)(5660300001)(8676002)(55016002)(2950100002)(4001350100001)(6666003)(42882006)(6916009)(83506001)(229853002)(50986999)(23726003)(53936002)(53546006)(189998001)(6496005)(6246003)(33656002)(92566002)(3846002)(50466002)(47776003)(33716001)(2906002)(42186005)(76176999)(1076002)(54356999)(6116002)(305945005)(18370500001);DIR:OUT;SFP:1101;SCL:1;SRVR:CO2PR07MB2582;H:hardcore;FPR:;SPF:None;MLV:sfv;LANG:en; X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;CO2PR07MB2582;23:UGbAX5iU/k2cPDB5lvafAESfaOKLuQt79/iu/c3iW?= =?us-ascii?Q?Dajw2aNvLDOvVHBgx+RZRGX4B9po+APra+SWAGfpci3Ky1Mqqi/dEDRQwHf8?= =?us-ascii?Q?50+FeMdolkGCrpHtMnXlGocFXNm5171I84iUOv1UZDFEoG5FL9rBiXVakPXg?= =?us-ascii?Q?MWpX5P/Id2z7ocI1FsL1rlh01Pj8CE0Gi990qJroMSbm1K6RZduCVbybAsSi?= =?us-ascii?Q?u3WG41aMlbXOdfV06Og20XGNFmzFdsdsQpmgEO/SIdww3unMGbioQkeMxE0K?= =?us-ascii?Q?I6ufgDKCbFrllyiPYziQ3d5nunq3CJ0F79LPW+sGSFVyYcoJ61YzNnqSu7K2?= =?us-ascii?Q?1u+meARSYiuFg0WDIHJR/1cdJB2kZHH5xnuzHQDu71Ou4U5GxY3N7kFRAESS?= =?us-ascii?Q?zJlg3s+rqS6Kzw5B7jbdOhzztqEfHwdP7NTdg48denioyyZfyDD/1M1w24zV?= =?us-ascii?Q?Bw1nuK5aQtZ4IdgjmVJ7IGN93qiTXC+WHS85L5IplAevn2b8mKcUqg9tejcL?= =?us-ascii?Q?cBE6dsif/+nzB1QwcC/P8wVfUVf/B2g9wz4YwsGEXL2uVeEHujSiR5jUe97d?= =?us-ascii?Q?qxgj97cti2o3QnGIPTO9ALiFFnWT4FjJNLRwmPeuy1Br4TQTl1QpUA6H0v57?= =?us-ascii?Q?b3qgFXm506i8RhbclJrNuuvvAVmSHpR4MXFtdetoUKiWGp3fRV2mq8jgInSF?= =?us-ascii?Q?Gy2Uo0gE9nFSxWtJdoobHbxp3Ag4xPcO85BtPcqhLDUUQyH0C3Q4/sXpYobF?= =?us-ascii?Q?rgYR5qUx/U+sSu2p/z4LOgDJY9X5XPy+l5L1ZsEmq/RxFxUQuQRXTPrb6xgn?= =?us-ascii?Q?kmFteuzn/9NHIHDHsIQXusb9d+mPk69nnk56IYGw6PdOvZz6YrInPvpUJzQt?= =?us-ascii?Q?apehc5ZqmgFCHrey4kRVCvA+OJXtMqgxxt4xzBU57LmzIKaapSXRtGhGW8cZ?= =?us-ascii?Q?MzNmQErq2/8igWy9W19o8+qTE00TpCJkfyV8PAfBl1lHHrUryaDwjRDEXYVs?= =?us-ascii?Q?NSpQJlukK6R7khb/EsO/38eb4ssFnOTuXMG+/dwdFs7aZHG6Rf7wJNCg48eS?= =?us-ascii?Q?0AcLsfZQkX+EagjWci9Or3kHrp/tjlZnEpVDpndYVCgfKDaCYfrM6CgAguXE?= =?us-ascii?Q?05dASeKzKdiELnZV2R5SHL2UJrlkbxZazkNBxTBR8hzmDR2ERK33Q=3D=3D?= X-Microsoft-Exchange-Diagnostics: 1;CO2PR07MB2582;6:5y8OhP0eaJz8jhmE4IK5IFwh0Ljn/zOYqGkY0yZA1WviiU3JMpdd5bsR5KLbs5JK4ArtCmrnoCoQEeWc+c7C+2rX0wL3fzLTOxzOlnn1lklGJy+HQ2gV2NimPPdVB+rkRa/Kzt9/qQu2gk/3ii0cqdZJWDy6/K86PH2sazNIJ3T20aYroZVAEs3zMAugEmPq3ZCBrwpXD79+RXyesv96cc3ay495WhtesIq4BlTG2Hsp/Rityox/tJI4DzYaku2XszKN5mY5ntoYAOXo5AVoP/z+dTdB7fmnioIszR+qJc9967m3X01q8/N5vAtBB1szr4fGY9Kzj0EviRz/JAoX5PQRXsOlFmZ26b1m/oIIdwj/MX23HfuBmQiqv07jLBaUgVjY3MErn8FmLTGAoqQ6tA==;5:fSCAIRSEJRZZ8yk1In7MxOioIy2vTXJUrBi0b7hbh0ZDo2dSWds1rR/aGoOaQJw1BPW6q5YbTlwxK2edKLc/b5yj0YZFk48c2P7AbPl8EsAMtxmivPJE1YMy049lbWC/g/epPJUfQZHniOk1dREHHVcx4X7ks8feTL70NP6GAV0=;24:jKlP+rMB9jKMzCLGJ7SSSmmKnriGVpXg/u22PphifGMlQgqeujLT+rx06GZjar6UIlqOeBvCRYCixGVBB6c69Z5mvo0GYJfKIC3f7VglENU= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1;CO2PR07MB2582;7:U0Y//AW9ekPHrlwm8ee8/37TCvAFsW6ZfhUry7bmzbRlfVj7l2LWyKAWLbFryDfv7CJYDtXB8L08DuywJtdIoYFTQbznhWb4kHreJTQzVSMFbK50Nc5+XO+l+D/jto6K5xhawH3KYDe+2HoI3b5mYuEVSn6emgNZTTHJFn23bKinKiEQwu06tBHzgOOuT+/bh9Uuok0CXnH8roZ+/oKTo2nrjU1cXluCjaOFqDnEYM6hBrPrJdMZ7fUbpDxUWNJcaivyLd+1UbdHgPj/h98XCQy53QOV+97RgNGxxKAieM8DihKow8HPJS8JRlzsEMFmCMCtchPOpHrd+nPE5kSbdw== X-OriginatorOrg: caviumnetworks.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Mar 2017 10:49:34.3474 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO2PR07MB2582 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Mar 03, 2017 at 12:47:14PM +0100, Ulf Hansson wrote: > On 6 February 2017 at 14:39, Jan Glauber wrote: > > This core driver will be used by a MIPS platform driver > > or by an ARM64 PCI driver. The core driver implements the > > mmc_host_ops and slot probe & remove functions. > > Callbacks are provided to allow platform specific interrupt > > enable and bus locking. > > > > The host controller supports: > > - up to 4 slots that can contain sd-cards or eMMC chips > > - 1, 4 and 8 bit bus width > > - SDR and DDR > > - transfers up to 52 Mhz (might be less when multiple slots are used) > > - DMA read/write > > - multi-block read/write (but not stream mode) > > > > Voltage is limited to 3.3v and shared for all slots. > > What voltage? The I/O voltage or the voltage for the card? > > VMMC or VMMCQ? >>From my understanding both, VMMC and VMMCQ are fixed at 3.3v. > > > > A global lock for all MMC devices is required because the host > > controller is shared. > > > > Signed-off-by: Jan Glauber > > Signed-off-by: David Daney > > Signed-off-by: Steven J. Hill > > --- > > drivers/mmc/host/cavium-mmc.c | 1029 +++++++++++++++++++++++++++++++++++++++++ > > drivers/mmc/host/cavium-mmc.h | 303 ++++++++++++ > > 2 files changed, 1332 insertions(+) > > create mode 100644 drivers/mmc/host/cavium-mmc.c > > create mode 100644 drivers/mmc/host/cavium-mmc.h > > > > diff --git a/drivers/mmc/host/cavium-mmc.c b/drivers/mmc/host/cavium-mmc.c > > new file mode 100644 > > index 0000000..40aee08 > > --- /dev/null > > +++ b/drivers/mmc/host/cavium-mmc.c > > [...] > > > + > > +static bool bad_status(union mio_emm_rsp_sts *rsp_sts) > > +{ > > + if (rsp_sts->s.rsp_bad_sts || rsp_sts->s.rsp_crc_err || > > + rsp_sts->s.rsp_timeout || rsp_sts->s.blk_crc_err || > > + rsp_sts->s.blk_timeout || rsp_sts->s.dbuf_err) > > + return true; > > + > > + return false; > > +} > > + > > +/* Try to clean up failed DMA. */ > > +static void cleanup_dma(struct cvm_mmc_host *host, > > + union mio_emm_rsp_sts *rsp_sts) > > +{ > > + union mio_emm_dma emm_dma; > > + > > + emm_dma.val = readq(host->base + MIO_EMM_DMA); > > + emm_dma.s.dma_val = 1; > > + emm_dma.s.dat_null = 1; > > + emm_dma.s.bus_id = rsp_sts->s.bus_id; > > + writeq(emm_dma.val, host->base + MIO_EMM_DMA); > > +} > > + > > +irqreturn_t cvm_mmc_interrupt(int irq, void *dev_id) > > +{ > > + struct cvm_mmc_host *host = dev_id; > > + union mio_emm_rsp_sts rsp_sts; > > + union mio_emm_int emm_int; > > + struct mmc_request *req; > > + bool host_done; > > + > > + /* Clear interrupt bits (write 1 clears ). */ > > + emm_int.val = readq(host->base + MIO_EMM_INT); > > + writeq(emm_int.val, host->base + MIO_EMM_INT); > > + > > + if (emm_int.s.switch_err) > > + check_switch_errors(host); > > + > > + req = host->current_req; > > + if (!req) > > + goto out; > > + > > + rsp_sts.val = readq(host->base + MIO_EMM_RSP_STS); > > + /* > > + * dma_val set means DMA is still in progress. Don't touch > > + * the request and wait for the interrupt indicating that > > + * the DMA is finished. > > + */ > > + if (rsp_sts.s.dma_val && host->dma_active) > > + goto out; > > + > > + if (!host->dma_active && emm_int.s.buf_done && req->data) { > > + unsigned int type = (rsp_sts.val >> 7) & 3; > > + > > + if (type == 1) > > + do_read(host, req, rsp_sts.s.dbuf); > > + else if (type == 2) > > + do_write(req); > > + } > > + > > + host_done = emm_int.s.cmd_done || emm_int.s.dma_done || > > + emm_int.s.cmd_err || emm_int.s.dma_err; > > + > > + if (!(host_done && req->done)) > > + goto no_req_done; > > + > > + if (bad_status(&rsp_sts)) > > + req->cmd->error = -EILSEQ; > > I don't think you should treat all errors as -EILSEQ. Please assign a > proper error code, depending on the error. Agreed, -ETIMEDOUT seems more appropriate for the timeouts. I'll go for -EIO for the dbuf_err (buffer space missing). What should I use for the CRC errors, -EILSEQ? > > + else > > + req->cmd->error = 0; > > + > > + if (host->dma_active && req->data) > > + if (!finish_dma(host, req->data)) > > + goto no_req_done; > > + > > + set_cmd_response(host, req, &rsp_sts); > > + if (emm_int.s.dma_err && rsp_sts.s.dma_pend) > > + cleanup_dma(host, &rsp_sts); > > + > > + host->current_req = NULL; > > + req->done(req); > > + > > +no_req_done: > > + if (host_done) > > + host->release_bus(host); > > +out: > > + return IRQ_RETVAL(emm_int.val != 0); > > +} > > + > > +/* > > + * Program DMA_CFG and if needed DMA_ADR. > > + * Returns 0 on error, DMA address otherwise. > > + */ > > +static u64 prepare_dma_single(struct cvm_mmc_host *host, struct mmc_data *data) > > +{ > > + union mio_emm_dma_cfg dma_cfg; > > + int count; > > + u64 addr; > > + > > + count = dma_map_sg(host->dev, data->sg, data->sg_len, > > + get_dma_dir(data)); > > + if (!count) > > + return 0; > > + > > + dma_cfg.val = 0; > > + dma_cfg.s.en = 1; > > + dma_cfg.s.rw = (data->flags & MMC_DATA_WRITE) ? 1 : 0; > > +#ifdef __LITTLE_ENDIAN > > + dma_cfg.s.endian = 1; > > +#endif > > + dma_cfg.s.size = (sg_dma_len(&data->sg[0]) / 8) - 1; > > + > > + addr = sg_dma_address(&data->sg[0]); > > + dma_cfg.s.adr = addr; > > + writeq(dma_cfg.val, host->dma_base + MIO_EMM_DMA_CFG); > > + > > + pr_debug("[%s] sg_dma_len: %u total sg_elem: %d\n", > > + (dma_cfg.s.rw) ? "W" : "R", sg_dma_len(&data->sg[0]), count); > > + return addr; > > +} > > + > > +static u64 prepare_dma(struct cvm_mmc_host *host, struct mmc_data *data) > > +{ > > + return prepare_dma_single(host, data); > > +} > > + > > +static void prepare_ext_dma(struct mmc_host *mmc, struct mmc_request *mrq, > > + union mio_emm_dma *emm_dma) > > +{ > > + struct cvm_mmc_slot *slot = mmc_priv(mmc); > > + > > + /* > > + * Our MMC host hardware does not issue single commands, > > + * because that would require the driver and the MMC core > > + * to do work to determine the proper sequence of commands. > > I don't get this. The allowed sequence of the commands is determined > by the SD/(e)MMC/SDIO spec and much of this knowledge is the > responsibility of the mmc core. > > > + * Instead, our hardware is superior to most other MMC bus > > No need to brag about your HW. Let's just describe how it works instead. I'll remove the comment. > > + * hosts. The sequence of MMC commands required to execute > > + * a transfer are issued automatically by the bus hardware. > > What does this really mean? Is this about HW support for better > dealing with data requests? Did David's reponse answer your questions? > > + * > > + * - David Daney > > + */ > > + emm_dma->val = 0; > > + emm_dma->s.bus_id = slot->bus_id; > > + emm_dma->s.dma_val = 1; > > + emm_dma->s.sector = (mrq->data->blksz == 512) ? 1 : 0; > > + emm_dma->s.rw = (mrq->data->flags & MMC_DATA_WRITE) ? 1 : 0; > > + emm_dma->s.block_cnt = mrq->data->blocks; > > + emm_dma->s.card_addr = mrq->cmd->arg; > > + if (mmc_card_mmc(mmc->card) || (mmc_card_sd(mmc->card) && > > + (mmc->card->scr.cmds & SD_SCR_CMD23_SUPPORT))) > > + emm_dma->s.multi = 1; > > + > > + pr_debug("[%s] blocks: %u multi: %d\n", (emm_dma->s.rw) ? "W" : "R", > > + mrq->data->blocks, emm_dma->s.multi); > > +} > > + > > [...] > > > + > > +static void cvm_mmc_set_ios(struct mmc_host *mmc, struct mmc_ios *ios) > > +{ > > + struct cvm_mmc_slot *slot = mmc_priv(mmc); > > + struct cvm_mmc_host *host = slot->host; > > + int clk_period, power_class = 10, bus_width = 0; > > + union mio_emm_switch emm_switch; > > + u64 clock; > > + > > + host->acquire_bus(host); > > + cvm_mmc_switch_to(slot); > > + > > + /* Set the power state */ > > + switch (ios->power_mode) { > > + case MMC_POWER_ON: > > + break; > > + > > + case MMC_POWER_OFF: > > + cvm_mmc_reset_bus(slot); > > + > > + if (host->global_pwr_gpiod) > > + gpiod_set_value_cansleep(host->global_pwr_gpiod, 0); > > + else > > + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, 0); > > + break; > > + > > + case MMC_POWER_UP: > > + if (host->global_pwr_gpiod) > > + gpiod_set_value_cansleep(host->global_pwr_gpiod, 1); > > + else > > + mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, ios->vdd); > > + break; > > + } > > + > > + /* Set bus width */ > > + switch (ios->bus_width) { > > + case MMC_BUS_WIDTH_8: > > + bus_width = 2; > > + break; > > + case MMC_BUS_WIDTH_4: > > + bus_width = 1; > > + break; > > + case MMC_BUS_WIDTH_1: > > + bus_width = 0; > > + break; > > + } > > + > > + slot->bus_width = bus_width; > > + > > + if (!ios->clock) > > There are cases when the core change the clock rate to 0, and then it > expects the mmc host to gate the clock. It probably a good idea for > you to do that as well. OK, seems to work. > > + goto out; > > + > > + /* Change the clock frequency. */ > > + clock = ios->clock; > > + if (clock > 52000000) > > + clock = 52000000; > > + slot->clock = clock; > > + clk_period = (host->sys_freq + clock - 1) / (2 * clock); > > + > > + emm_switch.val = 0; > > + emm_switch.s.hs_timing = (ios->timing == MMC_TIMING_MMC_HS); > > + emm_switch.s.bus_width = bus_width; > > + emm_switch.s.power_class = power_class; > > + emm_switch.s.clk_hi = clk_period; > > + emm_switch.s.clk_lo = clk_period; > > + emm_switch.s.bus_id = slot->bus_id; > > + > > + if (!switch_val_changed(slot, emm_switch.val)) > > + goto out; > > + > > + set_wdog(slot, 0); > > + do_switch(host, emm_switch.val); > > + slot->cached_switch = emm_switch.val; > > +out: > > + host->release_bus(host); > > +} > > [...] > > > + > > +static int set_bus_width(struct device *dev, struct cvm_mmc_slot *slot, u32 id) > > +{ > > + u32 bus_width; > > + int ret; > > + > > + /* > > + * The "cavium,bus-max-width" property is DEPRECATED and should > > + * not be used. We handle it here to support older firmware. > > + * Going forward, the standard "bus-width" property is used > > + * instead of the Cavium-specific property. > > + */ > > + if (!(slot->mmc->caps & (MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA))) { > > + /* Try legacy "cavium,bus-max-width" property. */ > > + ret = of_property_read_u32(dev->of_node, "cavium,bus-max-width", > > + &bus_width); > > + if (ret) { > > + /* No bus width specified, use default. */ > > + bus_width = 8; > > + dev_info(dev, "Default width 8 used for slot %u\n", id); > > + } > > + } else { > > + /* Hosts capable of 8-bit transfers can also do 4 bits */ > > + bus_width = (slot->mmc->caps & MMC_CAP_8_BIT_DATA) ? 8 : 4; > > + } > > This looks a bit unnessarry complex. > > I would instead suggest the following order of how to perform the OF > parsing. Bindings that get parsed later, overrides the earlier. > > 1. Parse deprecated bindings. > 2. Parse cavium specific bindings. > 3. Parse common mmc bindings. > 4. Check some caps, to make sure those have valid values as to cover > cases when the OF parsing didn't find values. > > The same comment applies for the other OF parsing functions below. OK. > > + > > + switch (bus_width) { > > + case 8: > > + slot->bus_width = (MMC_BUS_WIDTH_8 - 1); > > + slot->mmc->caps = MMC_CAP_8_BIT_DATA | MMC_CAP_4_BIT_DATA; > > + break; > > + case 4: > > + slot->bus_width = (MMC_BUS_WIDTH_4 - 1); > > + slot->mmc->caps = MMC_CAP_4_BIT_DATA; > > + break; > > + case 1: > > + slot->bus_width = MMC_BUS_WIDTH_1; > > + break; > > + default: > > + dev_err(dev, "Invalid bus width for slot %u\n", id); > > + return -EINVAL; > > + } > > + return 0; > > +} > > + > > +static void set_frequency(struct device *dev, struct mmc_host *mmc, u32 id) > > +{ > > + int ret; > > + > > + /* > > + * The "spi-max-frequency" property is DEPRECATED and should > > + * not be used. We handle it here to support older firmware. > > + * Going forward, the standard "max-frequency" property is > > + * used instead of the Cavium-specific property. > > + */ > > + if (mmc->f_max == 0) { > > + /* Try legacy "spi-max-frequency" property. */ > > + ret = of_property_read_u32(dev->of_node, "spi-max-frequency", > > + &mmc->f_max); > > + if (ret) { > > + /* No frequency properties found, use default. */ > > + mmc->f_max = 52000000; > > + dev_info(dev, "Default %u frequency used for slot %u\n", > > + mmc->f_max, id); > > + } > > + } else if (mmc->f_max > 52000000) > > + mmc->f_max = 52000000; > > + > > + /* Set minimum frequency */ > > + mmc->f_min = 400000; > > +} > > + > > +static int set_voltage(struct device *dev, struct mmc_host *mmc, > > + struct cvm_mmc_host *host) > > +{ > > + int ret; > > + > > + /* > > + * Legacy platform doesn't support regulator but enables power gpio > > + * directly during platform probe. > > + */ > > + if (host->global_pwr_gpiod) > > + /* Get a sane OCR mask for other parts of the MMC subsytem. */ > > + return mmc_of_parse_voltage(dev->of_node, &mmc->ocr_avail); > > Does really the legacy platforms use the mmc voltage range DT bindings!? The legacy DT's use (in the mmc slot nodes): voltage-ranges = <3300 3300>; > I would rather see that you assign a default value to mmc->ocr_avail, > than using this binding. The volatage seems to be identical for all legacy bindings I can find, so is it better to not parse it and use the 3.3 as default? > > + > > + mmc->supply.vmmc = devm_regulator_get(dev, "vmmc"); > > + if (IS_ERR(mmc->supply.vmmc)) { > > + ret = PTR_ERR(mmc->supply.vmmc); > > + } else { > > + ret = mmc_regulator_get_ocrmask(mmc->supply.vmmc); > > + if (ret > 0) { > > + mmc->ocr_avail = ret; > > + ret = 0; > > + } > > + } > > This if-else-if is a bit messy. > > Why not just return when you get an error instead. That should simply the code. OK, I'll simplify it. > Maybe you can have look and try to clean up this in the hole file > where you think it would make an improvment. > > > + return ret; > > +} > > + > > +int cvm_mmc_slot_probe(struct device *dev, struct cvm_mmc_host *host) > > To reflect that OF is needed, perhaps rename function to > cvm_mmc_of_slot_probe(). OK. > > +{ > > + struct device_node *node = dev->of_node; > > + u32 id, cmd_skew, dat_skew; > > + struct cvm_mmc_slot *slot; > > + struct mmc_host *mmc; > > + u64 clock_period; > > + int ret; > > + > > + ret = of_property_read_u32(node, "reg", &id); > > + if (ret) { > > + dev_err(dev, "Missing or invalid reg property on %s\n", > > + of_node_full_name(node)); > > + return ret; > > + } > > + > > + if (id >= CAVIUM_MAX_MMC || host->slot[id]) { > > + dev_err(dev, "Invalid reg property on %s\n", > > + of_node_full_name(node)); > > + return -EINVAL; > > + } > > + > > + mmc = mmc_alloc_host(sizeof(struct cvm_mmc_slot), dev); > > + if (!mmc) > > + return -ENOMEM; > > + > > + slot = mmc_priv(mmc); > > + slot->mmc = mmc; > > + slot->host = host; > > + > > + ret = mmc_of_parse(mmc); > > + if (ret) > > + goto error; > > + > > + ret = set_bus_width(dev, slot, id); > > + if (ret) > > + goto error; > > + > > + set_frequency(dev, mmc, id); > > + > > + /* Octeon-specific DT properties. */ > > + ret = of_property_read_u32(node, "cavium,cmd-clk-skew", &cmd_skew); > > + if (ret) > > + cmd_skew = 0; > > + ret = of_property_read_u32(node, "cavium,dat-clk-skew", &dat_skew); > > + if (ret) > > + dat_skew = 0; > > + > > + ret = set_voltage(dev, mmc, host); > > + if (ret < 0) > > + goto error; > > The functions set_bus_width(), set_freqeuncy(), set_voltage() all > performs OF parsing and there are some parsing also being done above. > > I would suggest you bundle all OF parsing into one function, perhaps > name it "cvm_mmc_of_parse()" or similar. That should make the code a > lot cleaner. OK. > > + > > + /* Set up host parameters */ > > + mmc->ops = &cvm_mmc_ops; > > + > > + mmc->caps |= MMC_CAP_MMC_HIGHSPEED | MMC_CAP_SD_HIGHSPEED | > > + MMC_CAP_ERASE | MMC_CAP_CMD23 | MMC_CAP_POWER_OFF_CARD; > > + > > + mmc->max_segs = 1; > > + > > + /* DMA size field can address up to 8 MB */ > > + mmc->max_seg_size = 8 * 1024 * 1024; > > + mmc->max_req_size = mmc->max_seg_size; > > + /* External DMA is in 512 byte blocks */ > > + mmc->max_blk_size = 512; > > + /* DMA block count field is 15 bits */ > > + mmc->max_blk_count = 32767; > > + > > + slot->clock = mmc->f_min; > > + slot->sclock = host->sys_freq; > > + > > + /* Period in picoseconds. */ > > + clock_period = 1000000000000ull / slot->sclock; > > + slot->cmd_cnt = (cmd_skew + clock_period / 2) / clock_period; > > + slot->dat_cnt = (dat_skew + clock_period / 2) / clock_period; > > + > > + slot->bus_id = id; > > + slot->cached_rca = 1; > > + > > + host->acquire_bus(host); > > + host->slot[id] = slot; > > + cvm_mmc_switch_to(slot); > > + cvm_mmc_init_lowlevel(slot); > > + host->release_bus(host); > > + > > + ret = mmc_add_host(mmc); > > + if (ret) { > > + dev_err(dev, "mmc_add_host() returned %d\n", ret); > > + goto error; > > + } > > + > > + return 0; > > + > > +error: > > + slot->host->slot[id] = NULL; > > + mmc_free_host(slot->mmc); > > + return ret; > > +} > > + > > +int cvm_mmc_slot_remove(struct cvm_mmc_slot *slot) > > +{ > > + mmc_remove_host(slot->mmc); > > + slot->host->slot[slot->bus_id] = NULL; > > + mmc_free_host(slot->mmc); > > + return 0; > > +} > > diff --git a/drivers/mmc/host/cavium-mmc.h b/drivers/mmc/host/cavium-mmc.h > > new file mode 100644 > > index 0000000..27fb02b > > --- /dev/null > > +++ b/drivers/mmc/host/cavium-mmc.h > > @@ -0,0 +1,303 @@ > > +/* > > + * Driver for MMC and SSD cards for Cavium OCTEON and ThunderX SOCs. > > + * > > + * This file is subject to the terms and conditions of the GNU General Public > > + * License. See the file "COPYING" in the main directory of this archive > > + * for more details. > > + * > > + * Copyright (C) 2012-2016 Cavium Inc. > > + */ > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#define CAVIUM_MAX_MMC 4 > > + > > +struct cvm_mmc_host { > > + struct device *dev; > > + void __iomem *base; > > + void __iomem *dma_base; > > + u64 emm_cfg; > > + int last_slot; > > + struct clk *clk; > > + int sys_freq; > > + > > + struct mmc_request *current_req; > > + struct sg_mapping_iter smi; > > + bool dma_active; > > + > > + struct gpio_desc *global_pwr_gpiod; > > + > > + struct cvm_mmc_slot *slot[CAVIUM_MAX_MMC]; > > + > > + void (*acquire_bus)(struct cvm_mmc_host *); > > + void (*release_bus)(struct cvm_mmc_host *); > > + void (*int_enable)(struct cvm_mmc_host *, u64); > > +}; > > + > > +struct cvm_mmc_slot { > > + struct mmc_host *mmc; /* slot-level mmc_core object */ > > + struct cvm_mmc_host *host; /* common hw for all slots */ > > + > > + u64 clock; > > + unsigned int sclock; > > + > > + u64 cached_switch; > > + u64 cached_rca; > > + > > + unsigned int cmd_cnt; /* sample delay */ > > + unsigned int dat_cnt; /* sample delay */ > > + > > + int bus_width; > > + int bus_id; > > +}; > > + > > +struct cvm_mmc_cr_type { > > + u8 ctype; > > + u8 rtype; > > +}; > > + > > +struct cvm_mmc_cr_mods { > > + u8 ctype_xor; > > + u8 rtype_xor; > > +}; > > + > > +/* Bitfield definitions */ > > + > > +union mio_emm_cmd { > > + u64 val; > > + struct mio_emm_cmd_s { > > +#ifdef __BIG_ENDIAN_BITFIELD > > Huh. Sorry, but this is a big nack from me. > > This isn't the common method for how we deal with endian issues in the > kernel. Please remove all use of the union types here and below. The > follow common patterns for how we deal with endian issues. May I ask why you dislike the bitfields? Or maybe it is easier when I explain why I decided to keep them: - One drawback of bitfields is poor performance on some architectures. That is not the case here, both MIPS64 and ARM64 have instructions capable of using bitfields without performance impact. - The used bitfield are all aligned to word size, usually the pattern in the driver is to readq / writeq the whole word (therefore the union val) and then set or read certain fields. That should avoid IMHO the unspecified behaviour the C standard mentions. - I prefer BIT_ULL and friends for single bits, but using macros for more then one bit is (again IMHO) much less readable then using bitfiels here. And all the endianess definitions are _only_ in the header file. Also, if I need to convert all of these I'll probably add some new bugs. What we have currently works fine on both MIPS and ARM64. > > + u64 :2; > > + u64 bus_id:2; > > + u64 cmd_val:1; > > + u64 :3; > > + u64 dbuf:1; > > + u64 offset:6; > > + u64 :6; > > + u64 ctype_xor:2; > > + u64 rtype_xor:3; > > + u64 cmd_idx:6; > > + u64 arg:32; > > +#else > > + u64 arg:32; > > + u64 cmd_idx:6; > > + u64 rtype_xor:3; > > + u64 ctype_xor:2; > > + u64 :6; > > + u64 offset:6; > > + u64 dbuf:1; > > + u64 :3; > > + u64 cmd_val:1; > > + u64 bus_id:2; > > + u64 :2; > > +#endif > > + } s; > > +}; > > + > > +union mio_emm_dma { > > + u64 val; > > + struct mio_emm_dma_s { > > +#ifdef __BIG_ENDIAN_BITFIELD > > + u64 :2; > > + u64 bus_id:2; > > + u64 dma_val:1; > > + u64 sector:1; > > + u64 dat_null:1; > > + u64 thres:6; > > + u64 rel_wr:1; > > + u64 rw:1; > > + u64 multi:1; > > + u64 block_cnt:16; > > + u64 card_addr:32; > > +#else > > + u64 card_addr:32; > > + u64 block_cnt:16; > > + u64 multi:1; > > + u64 rw:1; > > + u64 rel_wr:1; > > + u64 thres:6; > > + u64 dat_null:1; > > + u64 sector:1; > > + u64 dma_val:1; > > + u64 bus_id:2; > > + u64 :2; > > +#endif > > + } s; > > +}; > > + > > +union mio_emm_dma_cfg { > > + u64 val; > > + struct mio_emm_dma_cfg_s { > > +#ifdef __BIG_ENDIAN_BITFIELD > > + u64 en:1; > > + u64 rw:1; > > + u64 clr:1; > > + u64 :1; > > + u64 swap32:1; > > + u64 swap16:1; > > + u64 swap8:1; > > + u64 endian:1; > > + u64 size:20; > > + u64 adr:36; > > +#else > > + u64 adr:36; > > + u64 size:20; > > + u64 endian:1; > > + u64 swap8:1; > > + u64 swap16:1; > > + u64 swap32:1; > > + u64 :1; > > + u64 clr:1; > > + u64 rw:1; > > + u64 en:1; > > +#endif > > + } s; > > +}; > > + > > +union mio_emm_int { > > + u64 val; > > + struct mio_emm_int_s { > > +#ifdef __BIG_ENDIAN_BITFIELD > > + u64 :57; > > + u64 switch_err:1; > > + u64 switch_done:1; > > + u64 dma_err:1; > > + u64 cmd_err:1; > > + u64 dma_done:1; > > + u64 cmd_done:1; > > + u64 buf_done:1; > > +#else > > + u64 buf_done:1; > > + u64 cmd_done:1; > > + u64 dma_done:1; > > + u64 cmd_err:1; > > + u64 dma_err:1; > > + u64 switch_done:1; > > + u64 switch_err:1; > > + u64 :57; > > +#endif > > + } s; > > +}; > > + > > +union mio_emm_rsp_sts { > > + u64 val; > > + struct mio_emm_rsp_sts_s { > > +#ifdef __BIG_ENDIAN_BITFIELD > > + u64 :2; > > + u64 bus_id:2; > > + u64 cmd_val:1; > > + u64 switch_val:1; > > + u64 dma_val:1; > > + u64 dma_pend:1; > > + u64 :27; > > + u64 dbuf_err:1; > > + u64 :4; > > + u64 dbuf:1; > > + u64 blk_timeout:1; > > + u64 blk_crc_err:1; > > + u64 rsp_busybit:1; > > + u64 stp_timeout:1; > > + u64 stp_crc_err:1; > > + u64 stp_bad_sts:1; > > + u64 stp_val:1; > > + u64 rsp_timeout:1; > > + u64 rsp_crc_err:1; > > + u64 rsp_bad_sts:1; > > + u64 rsp_val:1; > > + u64 rsp_type:3; > > + u64 cmd_type:2; > > + u64 cmd_idx:6; > > + u64 cmd_done:1; > > +#else > > + u64 cmd_done:1; > > + u64 cmd_idx:6; > > + u64 cmd_type:2; > > + u64 rsp_type:3; > > + u64 rsp_val:1; > > + u64 rsp_bad_sts:1; > > + u64 rsp_crc_err:1; > > + u64 rsp_timeout:1; > > + u64 stp_val:1; > > + u64 stp_bad_sts:1; > > + u64 stp_crc_err:1; > > + u64 stp_timeout:1; > > + u64 rsp_busybit:1; > > + u64 blk_crc_err:1; > > + u64 blk_timeout:1; > > + u64 dbuf:1; > > + u64 :4; > > + u64 dbuf_err:1; > > + u64 :27; > > + u64 dma_pend:1; > > + u64 dma_val:1; > > + u64 switch_val:1; > > + u64 cmd_val:1; > > + u64 bus_id:2; > > + u64 :2; > > +#endif > > + } s; > > +}; > > + > > +union mio_emm_sample { > > + u64 val; > > + struct mio_emm_sample_s { > > +#ifdef __BIG_ENDIAN_BITFIELD > > + u64 :38; > > + u64 cmd_cnt:10; > > + u64 :6; > > + u64 dat_cnt:10; > > +#else > > + u64 dat_cnt:10; > > + u64 :6; > > + u64 cmd_cnt:10; > > + u64 :38; > > +#endif > > + } s; > > +}; > > + > > +union mio_emm_switch { > > + u64 val; > > + struct mio_emm_switch_s { > > +#ifdef __BIG_ENDIAN_BITFIELD > > + u64 :2; > > + u64 bus_id:2; > > + u64 switch_exe:1; > > + u64 switch_err0:1; > > + u64 switch_err1:1; > > + u64 switch_err2:1; > > + u64 :7; > > + u64 hs_timing:1; > > + u64 :5; > > + u64 bus_width:3; > > + u64 :4; > > + u64 power_class:4; > > + u64 clk_hi:16; > > + u64 clk_lo:16; > > +#else > > + u64 clk_lo:16; > > + u64 clk_hi:16; > > + u64 power_class:4; > > + u64 :4; > > + u64 bus_width:3; > > + u64 :5; > > + u64 hs_timing:1; > > + u64 :7; > > + u64 switch_err2:1; > > + u64 switch_err1:1; > > + u64 switch_err0:1; > > + u64 switch_exe:1; > > + u64 bus_id:2; > > + u64 :2; > > +#endif > > + } s; > > +}; > > + > > +/* Protoypes */ > > +irqreturn_t cvm_mmc_interrupt(int irq, void *dev_id); > > +int cvm_mmc_slot_probe(struct device *dev, struct cvm_mmc_host *host); > > +int cvm_mmc_slot_remove(struct cvm_mmc_slot *slot); > > Why do you need this here? Are those intended as library functions for > the different cavium variant drivers? Yes, this is the minimum I need to share the cavium-mmc-core. > > +extern const struct mmc_host_ops cvm_mmc_ops; > > Why do you need this? Left-over from development, can be removed now. > Kind regards > Uffe Thanks for the review! Jan