From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoph Hellwig Subject: Re: [RFC PATCH 01/11] pci: endpoint: add EP core layer to enable EP controller and EP functions Date: Wed, 12 Oct 2016 05:38:43 -0700 Message-ID: <20161012123843.GA942@infradead.org> References: <1473829927-20466-1-git-send-email-kishon@ti.com> <1473829927-20466-2-git-send-email-kishon@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1473829927-20466-2-git-send-email-kishon@ti.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Kishon Vijay Abraham I Cc: devicetree@vger.kernel.org, Joao.Pinto@synopsys.com, Arnd Bergmann , linux-doc@vger.kernel.org, Joao Pinto , Jingoo Han , Pratyush Anand , nsekhar@ti.com, linux-kernel@vger.kernel.org, Rob Herring , hch@infradead.org, m-karicheri2@ti.com, linux-arm-kernel@lists.infradead.org, linux-pci@vger.kernel.org, Bjorn Helgaas , linux-omap@vger.kernel.org, mingkai.hu@nxp.com List-Id: linux-omap@vger.kernel.org > +/** > + * pci_epc_stop() - stop the PCI link > + * @epc: the link of the EPC device that has to be stopped > + * > + * Invoke to stop the PCI link > + */ > +void pci_epc_stop(struct pci_epc *epc) > +{ > + if (IS_ERR(epc) || !epc->ops->stop) > + return; > + > + spin_lock_irq(&epc->irq_lock); > + epc->ops->stop(epc); > + spin_unlock_irq(&epc->irq_lock); > +} > +EXPORT_SYMBOL_GPL(pci_epc_stop); Can you elaborate on the synchronization strategy here? It seems like irq_lock is generally taken irq save and just around method calls. Wou;dn't it be better to leave locking to the methods themselves? > +/** > + * struct pci_epc - represents the PCI EPC device > + * @dev: PCI EPC device > + * @ops: function pointers for performing endpoint operations > + * @mutex: mutex to protect pci_epc ops > + */ > +struct pci_epc { > + struct device dev; > + /* support only single function PCI device for now */ > + struct pci_epf *epf; > + const struct pci_epc_ops *ops; > + spinlock_t irq_lock; > +}; And this still documentes a mutex instead of the irq save spinlock, while we're at it.. > +/** > + * struct pci_epf_bar - represents the BAR of EPF device > + * @phys_addr: physical address that should be mapped to the BAR > + * @size: the size of the address space present in BAR > + */ > +struct pci_epf_bar { > + dma_addr_t phys_addr; > + size_t size; > +}; Just curious: shouldn't this be a phys_addr_t instead of a dma_addr_t? Otherwise this looks like a nice little framework to get started! From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: Date: Wed, 12 Oct 2016 05:38:43 -0700 From: Christoph Hellwig To: Kishon Vijay Abraham I Subject: Re: [RFC PATCH 01/11] pci: endpoint: add EP core layer to enable EP controller and EP functions Message-ID: <20161012123843.GA942@infradead.org> References: <1473829927-20466-1-git-send-email-kishon@ti.com> <1473829927-20466-2-git-send-email-kishon@ti.com> MIME-Version: 1.0 In-Reply-To: <1473829927-20466-2-git-send-email-kishon@ti.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, Joao.Pinto@synopsys.com, Arnd Bergmann , linux-doc@vger.kernel.org, Joao Pinto , Jingoo Han , Pratyush Anand , nsekhar@ti.com, linux-kernel@vger.kernel.org, Rob Herring , hch@infradead.org, m-karicheri2@ti.com, linux-arm-kernel@lists.infradead.org, linux-pci@vger.kernel.org, Bjorn Helgaas , linux-omap@vger.kernel.org, mingkai.hu@nxp.com Content-Type: text/plain; charset="us-ascii" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+bjorn=helgaas.com@lists.infradead.org List-ID: > +/** > + * pci_epc_stop() - stop the PCI link > + * @epc: the link of the EPC device that has to be stopped > + * > + * Invoke to stop the PCI link > + */ > +void pci_epc_stop(struct pci_epc *epc) > +{ > + if (IS_ERR(epc) || !epc->ops->stop) > + return; > + > + spin_lock_irq(&epc->irq_lock); > + epc->ops->stop(epc); > + spin_unlock_irq(&epc->irq_lock); > +} > +EXPORT_SYMBOL_GPL(pci_epc_stop); Can you elaborate on the synchronization strategy here? It seems like irq_lock is generally taken irq save and just around method calls. Wou;dn't it be better to leave locking to the methods themselves? > +/** > + * struct pci_epc - represents the PCI EPC device > + * @dev: PCI EPC device > + * @ops: function pointers for performing endpoint operations > + * @mutex: mutex to protect pci_epc ops > + */ > +struct pci_epc { > + struct device dev; > + /* support only single function PCI device for now */ > + struct pci_epf *epf; > + const struct pci_epc_ops *ops; > + spinlock_t irq_lock; > +}; And this still documentes a mutex instead of the irq save spinlock, while we're at it.. > +/** > + * struct pci_epf_bar - represents the BAR of EPF device > + * @phys_addr: physical address that should be mapped to the BAR > + * @size: the size of the address space present in BAR > + */ > +struct pci_epf_bar { > + dma_addr_t phys_addr; > + size_t size; > +}; Just curious: shouldn't this be a phys_addr_t instead of a dma_addr_t? Otherwise this looks like a nice little framework to get started! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel From mboxrd@z Thu Jan 1 00:00:00 1970 From: hch@infradead.org (Christoph Hellwig) Date: Wed, 12 Oct 2016 05:38:43 -0700 Subject: [RFC PATCH 01/11] pci: endpoint: add EP core layer to enable EP controller and EP functions In-Reply-To: <1473829927-20466-2-git-send-email-kishon@ti.com> References: <1473829927-20466-1-git-send-email-kishon@ti.com> <1473829927-20466-2-git-send-email-kishon@ti.com> Message-ID: <20161012123843.GA942@infradead.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org > +/** > + * pci_epc_stop() - stop the PCI link > + * @epc: the link of the EPC device that has to be stopped > + * > + * Invoke to stop the PCI link > + */ > +void pci_epc_stop(struct pci_epc *epc) > +{ > + if (IS_ERR(epc) || !epc->ops->stop) > + return; > + > + spin_lock_irq(&epc->irq_lock); > + epc->ops->stop(epc); > + spin_unlock_irq(&epc->irq_lock); > +} > +EXPORT_SYMBOL_GPL(pci_epc_stop); Can you elaborate on the synchronization strategy here? It seems like irq_lock is generally taken irq save and just around method calls. Wou;dn't it be better to leave locking to the methods themselves? > +/** > + * struct pci_epc - represents the PCI EPC device > + * @dev: PCI EPC device > + * @ops: function pointers for performing endpoint operations > + * @mutex: mutex to protect pci_epc ops > + */ > +struct pci_epc { > + struct device dev; > + /* support only single function PCI device for now */ > + struct pci_epf *epf; > + const struct pci_epc_ops *ops; > + spinlock_t irq_lock; > +}; And this still documentes a mutex instead of the irq save spinlock, while we're at it.. > +/** > + * struct pci_epf_bar - represents the BAR of EPF device > + * @phys_addr: physical address that should be mapped to the BAR > + * @size: the size of the address space present in BAR > + */ > +struct pci_epf_bar { > + dma_addr_t phys_addr; > + size_t size; > +}; Just curious: shouldn't this be a phys_addr_t instead of a dma_addr_t? Otherwise this looks like a nice little framework to get started! From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933202AbcJLNL7 (ORCPT ); Wed, 12 Oct 2016 09:11:59 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:58841 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755336AbcJLNIF (ORCPT ); Wed, 12 Oct 2016 09:08:05 -0400 Date: Wed, 12 Oct 2016 05:38:43 -0700 From: Christoph Hellwig To: Kishon Vijay Abraham I Cc: Bjorn Helgaas , Arnd Bergmann , Jingoo Han , hch@infradead.org, Joao.Pinto@synopsys.com, mingkai.hu@nxp.com, m-karicheri2@ti.com, Pratyush Anand , linux-pci@vger.kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Joao Pinto , Rob Herring , nsekhar@ti.com Subject: Re: [RFC PATCH 01/11] pci: endpoint: add EP core layer to enable EP controller and EP functions Message-ID: <20161012123843.GA942@infradead.org> References: <1473829927-20466-1-git-send-email-kishon@ti.com> <1473829927-20466-2-git-send-email-kishon@ti.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1473829927-20466-2-git-send-email-kishon@ti.com> User-Agent: Mutt/1.6.1 (2016-04-27) X-SRS-Rewrite: SMTP reverse-path rewritten from by bombadil.infradead.org. See http://www.infradead.org/rpr.html Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > +/** > + * pci_epc_stop() - stop the PCI link > + * @epc: the link of the EPC device that has to be stopped > + * > + * Invoke to stop the PCI link > + */ > +void pci_epc_stop(struct pci_epc *epc) > +{ > + if (IS_ERR(epc) || !epc->ops->stop) > + return; > + > + spin_lock_irq(&epc->irq_lock); > + epc->ops->stop(epc); > + spin_unlock_irq(&epc->irq_lock); > +} > +EXPORT_SYMBOL_GPL(pci_epc_stop); Can you elaborate on the synchronization strategy here? It seems like irq_lock is generally taken irq save and just around method calls. Wou;dn't it be better to leave locking to the methods themselves? > +/** > + * struct pci_epc - represents the PCI EPC device > + * @dev: PCI EPC device > + * @ops: function pointers for performing endpoint operations > + * @mutex: mutex to protect pci_epc ops > + */ > +struct pci_epc { > + struct device dev; > + /* support only single function PCI device for now */ > + struct pci_epf *epf; > + const struct pci_epc_ops *ops; > + spinlock_t irq_lock; > +}; And this still documentes a mutex instead of the irq save spinlock, while we're at it.. > +/** > + * struct pci_epf_bar - represents the BAR of EPF device > + * @phys_addr: physical address that should be mapped to the BAR > + * @size: the size of the address space present in BAR > + */ > +struct pci_epf_bar { > + dma_addr_t phys_addr; > + size_t size; > +}; Just curious: shouldn't this be a phys_addr_t instead of a dma_addr_t? Otherwise this looks like a nice little framework to get started!