From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id A24C9C77B70 for ; Fri, 14 Apr 2023 05:57:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229990AbjDNF5d (ORCPT ); Fri, 14 Apr 2023 01:57:33 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39254 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229479AbjDNF5a (ORCPT ); Fri, 14 Apr 2023 01:57:30 -0400 Received: from bombadil.infradead.org (bombadil.infradead.org [IPv6:2607:7c80:54:3::133]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 67DBF4EEA; Thu, 13 Apr 2023 22:57:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20210309; h=In-Reply-To:Content-Transfer-Encoding :Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date: Sender:Reply-To:Content-ID:Content-Description; bh=vDEmKAwqsAzYhB9mA/dekTjtCBjY4b15AmaH++f9BvY=; b=Csiz/MdqOxesMZcuF5yeFmGPKm gVLN/72UmzB8H04ZwM8m4Hf5XOEYh+wmaVS10wkiDAgeWG8XDIbkkTrRTAWpVo6G9QevEDEmLEFOM K724Te9jy0zacsuaIOLmI5/D4Kxx4yqbFx9jG8+MOleS+9GMDdVoivmSvY6ZJpT/tu0o9URKHQBdT eYk4ZfvpQPo6kY2F9ks+EQ2ear3gBHMC7uu384IsedKe6a2nH7owoo7Air5lp8acC0gFtG7+CMySI VUeZMJcIqhWEc1HfAswU1Vi8oqFZWSVIXNezgjlibnf0o0i9XykYMCLKxXtoxbW1RnQ2IvPT6aBSa JTI/tgsw==; Received: from hch by bombadil.infradead.org with local (Exim 4.96 #2 (Red Hat Linux)) id 1pnCR7-008Pvy-0p; Fri, 14 Apr 2023 05:57:29 +0000 Date: Thu, 13 Apr 2023 22:57:29 -0700 From: Christoph Hellwig To: Kelvin.Cao@microchip.com Cc: hch@infradead.org, dmaengine@vger.kernel.org, vkoul@kernel.org, George.Ge@microchip.com, linux-kernel@vger.kernel.org, logang@deltatee.com Subject: Re: [PATCH v2 1/1] dmaengine: switchtec-dma: Introduce Switchtec DMA engine PCI driver Message-ID: References: <20230403180630.4186061-1-kelvin.cao@microchip.com> <20230403180630.4186061-2-kelvin.cao@microchip.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-SRS-Rewrite: SMTP reverse-path rewritten from by bombadil.infradead.org. See http://www.infradead.org/rpr.html Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Apr 13, 2023 at 11:22:14PM +0000, Kelvin.Cao@microchip.com wrote: > On Mon, 2023-04-10 at 18:42 +0200, Christoph Hellwig wrote: > > > > > +     writew((__force u16)cpu_to_le16(SWITCHTEC_DMA_SQ_SIZE), > > > +            &swdma_chan->mmio_chan_fw->sq_size); > > > +     writew((__force u16)cpu_to_le16(SWITCHTEC_DMA_CQ_SIZE), > > > +            &swdma_chan->mmio_chan_fw->cq_size); > > > > This looks broken to me, writew always expects cpu endian arguments > > and byte swaps on big endian systems. > > Do you mean writew assumes the peripherals be little-endian, and will > do the swap when host is bit-endian? Yes. All the standard mmio accessors ({read,write}{b,w,l,q}) do that. > > But I find the whole pcim_iomap_table concept very confusing to the > > reader of the driver, and given that it doesn't really use many > > devm or pcim routines I'd suggest removing them all and sticking to > > one well understood way of manging resource lifetimes. > > I didn't get it. Do you have specific suggestion? - instead of pcim_enable_device call pci_enable_device, and then just call pci_disable_device in ->remove and on error - instead of pcim_iomap_regions, call pci_request_mem_regions on probe, and pci_release_mem_regions on release / fail, and then do an ioremap(pci_resource_start(pdev, 1) for the actual bar (and iounmap on release/fail) instead of pcim_iomap_table. > > dma_set_mask_and_coherent for a smaller mask will never succeed when > > trying to set it to a larger one failed.  So you can remove the > > second > > call here. > > By default the kernel assumes the device can address 32-bit address > space, Yes. > I wonder why it wouldn't allow 32-bit mask when it failes 64- > bit? The kernel never fails setting a 64-bit DMA mask, it only fails setting too small masks.