From patchwork Sat Sep 24 08:00:37 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: zhichang X-Patchwork-Id: 674292 Return-Path: X-Original-To: incoming-imx@patchwork.ozlabs.org Delivered-To: patchwork-incoming-imx@bilbo.ozlabs.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3sh2l559Yqz9s9Y for ; Sat, 24 Sep 2016 18:02:23 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b=hmaS6uv+; dkim-atps=neutral Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.85_2 #1 (Red Hat Linux)) id 1bnhse-0008MM-8l; Sat, 24 Sep 2016 08:00:16 +0000 Received: from mail-pa0-x243.google.com ([2607:f8b0:400e:c03::243]) by bombadil.infradead.org with esmtps (Exim 4.85_2 #1 (Red Hat Linux)) id 1bnhsZ-0007KA-Ri for linux-arm-kernel@lists.infradead.org; Sat, 24 Sep 2016 08:00:13 +0000 Received: by mail-pa0-x243.google.com with SMTP id oz2so6044955pac.0 for ; Sat, 24 Sep 2016 00:59:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding; bh=7S+Lc6MWdChsrZzEmoAK63TUK36954fjZQIlCutNRT4=; b=hmaS6uv+b4CI6gDVka6bCR1WPPKK/ro0vFQA3R5w4imbrFa0GLorkwPtMJEhP+pin3 rPuMi/8pGiUKNtKSsFV7xntpTwZGy/sVIsViddEvBRv/1C9ev+kZo5rWyb7dJS4xzhi0 7x2kc5eDK9y3JUeDJfWg8Lnreol/zTsdnp6Cnz51yhWlJ+c9sq8qd24PRtFkZtlTAeYH pRyd0lqRDiGBIV4bXDjcXMDMuvCLovVTZEaq1FSIX2X6t/9IGUTu9ZkGWLjQz+mXALI9 3MOW6MyhRymgkIfLnmF4ATlIPe5QY1Le2W5F6tRMm4j8W6x9PhGSWLdhR27AndmFYOV+ ZvKA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=7S+Lc6MWdChsrZzEmoAK63TUK36954fjZQIlCutNRT4=; b=OEGJ39l7nCUzvoikRR3gJH4/qVqgLgu1xU3yKkhX9R65YhapcmqnIukEnxtbLtcqVv dDcNPP98v7lKWtjdIJEgO+J4G75dCGwwdVzn1713F58a0d1ABa+cwIDnUAJgcOuVu8qt yrbNcNPZ+qcjfDWFqTbh+7hdeGnvxuTf+C0YlitzD8XI6yrFQBZW0rxwXGXC8FgAKfFS BI76s7CwieYpCoLJJmMLQmJc6A8BgpYETIb87Uc0mvKZyKnB4k/WbC3c34F5ZRFX0D+z +eKzQbn/k3I8uktWr3I+QWuLwjb77cxMdgtq223G39nShBcSpR0erl+PDiUdojVnVHK/ h6sQ== X-Gm-Message-State: AE9vXwO2iExzrcdt9oHuB2grJaNNZcyIyUqeJJCflKa6CmSTSmay+jTyEKJacAE5L1CWtg== X-Received: by 10.66.249.200 with SMTP id yw8mr19814198pac.13.1474703990963; Sat, 24 Sep 2016 00:59:50 -0700 (PDT) Received: from [192.168.201.126] ([58.251.159.252]) by smtp.gmail.com with ESMTPSA id f7sm16313504pfk.79.2016.09.24.00.59.46 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 24 Sep 2016 00:59:50 -0700 (PDT) Subject: Re: [PATCH V3 2/4] ARM64 LPC: LPC driver implementation on Hip06 To: Arnd Bergmann , "will.deacon@arm.com" , "liviu.dudau@arm.com" , bhelgaas@google.com References: <1473855354-150093-1-git-send-email-yuanzhichang@hisilicon.com> <9178320.n4yHmfyPA3@wuerfel> <57E40665.8080005@gmail.com> <1760643.vMTR5o5E9g@wuerfel> From: zhichang X-Enigmail-Draft-Status: N1110 Message-ID: <378cda2d-e8fb-af10-01ff-c555def3d639@gmail.com> Date: Sat, 24 Sep 2016 16:00:37 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <1760643.vMTR5o5E9g@wuerfel> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160924_010011_982498_11E0B6FE X-CRM114-Status: GOOD ( 31.40 ) X-Spam-Score: -2.5 (--) X-Spam-Report: SpamAssassin version 3.4.0 on bombadil.infradead.org summary: Content analysis details: (-2.5 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [2607:f8b0:400e:c03:0:0:0:243 listed in] [list.dnswl.org] 0.2 FREEMAIL_ENVFROM_END_DIGIT Envelope-from freemail username ends in digit (zhichang.yuan02[at]gmail.com) -0.0 SPF_PASS SPF: sender matches SPF record 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider (zhichang.yuan02[at]gmail.com) -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "devicetree@vger.kernel.org" , "lorenzo.pieralisi@arm.com" , "benh@kernel.crashing.org" , Gabriele Paoloni , "minyard@acm.org" , "linux-pci@vger.kernel.org" , John Garry , "linux-kernel@vger.kernel.org" , Yuanzhichang , Linuxarm , "xuwei \(O\)" , "linux-serial@vger.kernel.org" , "gregkh@linuxfoundation.org" , "zourongrong@gmail.com" , "kantyzc@163.com" , "linux-arm-kernel@lists.infradead.org" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+incoming-imx=patchwork.ozlabs.org@lists.infradead.org List-Id: linux-imx-kernel.lists.patchwork.ozlabs.org Hi, Arnd, On 2016年09月23日 17:51, Arnd Bergmann wrote: > On Friday, September 23, 2016 12:27:17 AM CEST zhichang.yuan wrote: >> For this patch sketch, I have a question. >> Do we call pci_address_to_pio in arch_of_address_to_pio to get the >> corresponding logical IO port >> for LPC?? > > > No, of course not, that would be silly: > > The argument to pci_address_to_pio() is a phys_addr_t, and we we don't > have one because there is no address associated with your PIO, that > is the entire point of your driver! > ok. I think I know you points. The physical addresses of LPC are only the LPC domain addresses, not the really CPU physical addresses. That is just why you don't support the ranges property usage in patch V3. Consequently, It is not so reasonable to call pci_address_to_pio() with LPC address because that function is only suitable for cpu physical address. But just as you said in the next email reply to Gabriele, "Any IORESOURCE_IO address always refers to the logical I/O port range in Linux, not the physical address that is used on a bus.", Any devices which support IO accesses should have their own unique logical IO range to drive the corresponding hardware. It means that the drivers should know the mapping between physical port/memory address and logical IO depend on the device specific I/O mode. At this moment, only PCI host bridge setup a logical IO range allocation mechanism to manipulate this logical IO range, and this way applies cpu physical address(memory) as the input. Now, our LPC also need subrange from this common logical IO range, but with legacy I/O port rather than CPU memory address. Ok, it break the precondition of pci_register_io_range/pci_pio_to_address, we should not use them directly for LPC although the calling of pci_pio_to_address is simple and less change on the relevant code. We had done like that in V3... So, the key issue is how to get a logical IO subrange which is not conflicted with others, such as pci host bridges?? I list several ideas for discussion: 1. reserve a specific logical IO subrange for LPC I describe this in "Note 1" below. Please check it. This way seems simple without much changes, but it is not generic. 2. setup a separate logical IO subrange allocation mechanism specific for LPC/ISA Just as your suggestion before, add the arch_of_address_to_pio() for the devices which operate I/O with legacy I/O port address rather than memory address in MMIO mode. That arch_of_address_to_pio() will return non-conflict logical IO with PCI host bridge at last. But the logical IO range is global, those functions for LPC/ISA specific logical IO subrange allocation must be synchronized with pci_register_io_range/pci_pio_to_address to know what logical ranges had been populated. It is not good for the implement dispersion on same issue. 3. setup a new underlying method to control the logical IO range management Based on the existing resource management, add a simplified logical IO range management support which only request the logical IO ranges according the IO range size ( similar to IORESOURCE_SIZEALIGN mode ), no matter what type the physical address is. Then revise the current pci_register_io_range to adopt this new method. Of-course, LPC/ISA request the logical IO with this new method too. This is just a proposition. It is more workload compared with other solutions. What do you think about these? Any more ideas? > Also, we already know the mapping because this is what the inb/outb > workaround is looking at, so there is absolutely no reason to call it > either. > >> If we don't, it seems the LPC specific IO address will conflict with PCI >> host bridges' logical IO. >> >> Supposed our LPC populated the IO range from 0x100 to 0x3FF( this is >> normal for ISA similar >> devices), after arch_of_address_to_pio(), the r->start will be set as >> 0x100, r->end will be set as >> 0x3FF. And if there is one PCI host bridge who request a IO window size >> over 0x400 at the same >> time, the corresponding r->start and r->end will be set as 0x0, 0x3FF >> after of_address_to_resource >> for this host bridge. Then the IO conflict happens. > > You would still need to reserve some space in the io_range_list > to avoid possible conflicts, which is a bit ugly with the current > definition of pci_register_io_range, but I'm sure can be done. > Note 1) Do you remember patch V2? There, I modified the pci.c like that to reserve 0 - PCIBIOS_MIN_IO (it is 0x1000) : Based on this, a exclusive logical IO subrange is for LPC now. Then we certainly can add some special handling in __of_address_to_resource or __of_translate_address --> of_translate_one to return the untranslated LPC/ISA IO address. But to be honest, I think we don't need this special handling in address.c anymore. We had known the LPC/ISA IO is 1:1 to logical IO, just think any logical IO port among 0 - 0x1000 should call the LPC registered I/O hooks in the new in/out(). Furthermore, we can make the reservation is not fixed as PCIBIOS_MIN_IO. If the LPC/ISA probing is run before PCI host bridge probing, we can reserve a non-fixed logcial IO subrange what LPC/ISA ask for. This solution is based on an assumption that no any other devices have to request the specific logical IO subrange for LPC/ISA. Probably this assumption is ok on arm64, you known, there is no real IO space as X86. But anyway, this reservation is not so generic, depended on some special handling. Does this idea match your comments?? > One way I can think of would be to change pci_register_io_range() > to just return the logical port number directly (it already > knows it!), and pass an invalid physical address (e.g. > #define ISA_WORKAROUND_IO_PORT_WINDOW -0x10000) into it for > invalid translations. > I am not so clear know your idea here. Do you want to select an unpopulated CPU address as the parent address in range property?? or anything else??? > Another alternative that just occurred to me would be to move > the pci_address_to_pio() call from __of_address_to_resource() > into of_bus_pci_translate() and then do the special handling > for the ISA/LPC bus in of_bus_isa_translate(). As for this idea, do you mean that of_translate_address will directly return the final logical IO start address?? It seems to extend the definition of of_translate_address. Thanks, Zhichang > > Arnd > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index aab9d51..ac2e569 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -3221,7 +3221,7 @@ int __weak pci_register_io_range(phys_addr_t addr, resourc #ifdef PCI_IOBASE struct io_range *range; - resource_size_t allocated_size = 0; + resource_size_t allocated_size = PCIBIOS_MIN_IO; /* check if the range hasn't been previously recorded */ spin_lock(&io_range_lock); @@ -3270,7 +3270,7 @@ phys_addr_t pci_pio_to_address(unsigned long pio) #ifdef PCI_IOBASE struct io_range *range; - resource_size_t allocated_size = 0; + resource_size_t allocated_size = PCIBIOS_MIN_IO; if (pio > IO_SPACE_LIMIT) return address; @@ -3293,7 +3293,7 @@ unsigned long __weak pci_address_to_pio(phys_addr_t addres { #ifdef PCI_IOBASE struct io_range *res; - resource_size_t offset = 0; + resource_size_t offset = PCIBIOS_MIN_IO; unsigned long addr = -1; spin_lock(&io_range_lock);