archive mirror
 help / color / mirror / Atom feed
From: Serge Semin <>
To: Rob Herring <>
Cc: "Serge Semin" <>,
	"Jingoo Han" <>,
	"Gustavo Pimentel" <>,
	"Bjorn Helgaas" <>,
	"Lorenzo Pieralisi" <>,
	"Krzysztof Wilczyński" <>,
	"Manivannan Sadhasivam" <>,
	"Alexey Malahov" <>,
	"Pavel Parkhomenko" <>,
	"Frank Li" <>,,
Subject: Re: [PATCH v3 02/13] PCI: dwc: Don't use generic IO-ops for DBI-space access
Date: Fri, 27 May 2022 19:05:51 +0300	[thread overview]
Message-ID: <20220527160551.dh6fb5n6xbmtjpaa@mobilestation> (raw)
In-Reply-To: <>

On Thu, May 26, 2022 at 04:29:30PM -0500, Rob Herring wrote:
> On Tue, May 17, 2022 at 03:50:47PM +0300, Serge Semin wrote:
> > Commit c2b0c098fbd1 ("PCI: dwc: Use generic config accessors") replaced
> > the locally defined DW PCIe host controller config-space accessors with
> > the generic methods pci_generic_config_read() and
> > pci_generic_config_write(). It was intended that the corresponding
> > bus-mapping callback returned a correct virtual address of the passed PCI
> > config-space register. The problem of the proposed solution was that it
> > didn't take into account the way the host config-space is accessed on the
> > DW PCIe. Depending on the DW PCIe IP-core synthesize parameters different
> > interfaces can be used to access the host and peripheral config/memory
> > spaces. The former one can be accessed via the DBI interface, while the
> > later ones is reached via the AHB/AXI application bus. In case if the DW
> > PCIe controller is configured to have a dedicated DBI interface, the way
> > it is mapped into the IO-memory turns to be platform-specific. For such
> > setups the DWC PCIe driver provides a set of the callbacks
> > dw_pcie_ops.{read_dbi,write_dbi} so the platforms glue-drivers would be
> > able to take into account the DBI bus IO peculiarities. Since
> > commit c2b0c098fbd1 ("PCI: dwc: Use generic config accessors") these
> > methods haven't been utilized during the generic host initialization
> > performed by the PCIe subsystem code.
> > 
> > I don't really know how come there have been no problems spotted for the
> > Histb/Exynos/Kirin PCIe controllers so far, but in our case with dword

> Because they implement their own pci_ops for the root bus. You should 
> too.

Right. I should, but I would do that in a more generic way. Please see
the next comment.

> Who is 'our case'? 
> > aligned IO requirement the generic config-space accessors can't be
> > utilized for the host config-space. Thus in order to make sure the host
> > config-space is properly accessed via the DBI bus let's get back the
> > dw_pcie_rd_own_conf() and dw_pcie_wr_own_conf() methods. They are going to
> > be just wrappers around the already defined
> > dw_pcie_read_dbi()/dw_pcie_write_dbi() functions with proper arguments
> > conversion. These methods perform the platform-specific config-space IO if
> > the DBI accessors are specified, otherwise they call normal MMIO
> > operations.

> The idea was for DWC to not define its own way to have different 
> read/write for root bus vs. child bus as many PCI host bridges need the 
> same thing. So the host bridge struct now has 2 pci_ops pointers. And 
> the mess of function pointer indirection is gone.

Thanks for clarification. I should have investigated the problem more
thoroughly. Now I see what was the reason of that change.  It was
indeed wrong to blame the commit c2b0c098fbd1 ("PCI: dwc: Use generic
config accessors") that something was done incorrectly. After a more
thorough commit inspection I realized that you just replaced the
dw_pcie_rd_own_conf() and dw_pcie_wr_own_conf() with the generic
pci_generic_config_read and pci_generic_config_write() as they had
been equivalent anyway.  I thought they didn't have the same semantic
by confusing the dw_pcie_{read,write}() and dw_pcie_{read,write}_dbi()
methods usage (see the _dbi suffix) in the original own PCI
config-space accessors. So to speak I'll need to drop the Fixes tag
with your commit hash from the patch.

Getting back to the own-bus accessors. DW PCIe RP/EP own-config space
is accessed over the DBI-bus. If the particular platform is designed
in a way so the DBI MMIO space access has some non-specific
peculiarities then that platform implements its own read_dbi/write_dbi
accessors. In case if these callbacks are defined, the driver must
use them for all DBI MMIO accesses including for the ones performed
from the subsystem core in the framework of the host own config-space
setups. As I mentioned in the patch log currently the only platforms
with such requirement happen to be Histb, Exynos and Kirin DW PCIe. As
such we can freely get back the generic dw_pcie_rd_own_conf() and
dw_pcie_wr_own_conf() methods but use the dw_pcie_{read,write}_dbi()
methods in there in the same way as it is done in the Histb, Exynos
and Kirin DW PCIe drivers (see their own PCI config-space accessors
match). Due to that we can drop the pci_ops redefinition from these
platforms and just use the own-config space accessors for all such
platforms as it's suggested in this patch. So this modification can be
re-qualified to the cleanup one then:
1) Create the generic own config-space accessors (more portable as
the DBI-bus access specifics must be always taken into account) as it
is suggested in this patch already.
2) Drop the Kirin, Exynos, Histb own config-space re-definition.
3) Drop the dw_pcie_read_dbi() and dw_pcie_write_dbi() methods exporting.

What do you think?


> Rob

  reply	other threads:[~2022-05-27 16:06 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-17 12:50 [PATCH v3 00/13] PCI: dwc: Various fixes and cleanups Serge Semin
2022-05-17 12:50 ` [PATCH v3 01/13] PCI: dwc: Stop link in the host init error and de-initialization Serge Semin
2022-05-26 21:30   ` Rob Herring
2022-05-17 12:50 ` [PATCH v3 02/13] PCI: dwc: Don't use generic IO-ops for DBI-space access Serge Semin
2022-05-26 21:29   ` Rob Herring
2022-05-27 16:05     ` Serge Semin [this message]
2022-05-27 17:39       ` Serge Semin
2022-05-31 16:09         ` Rob Herring
2022-05-31 18:46           ` Serge Semin
2022-05-17 12:50 ` [PATCH v3 03/13] PCI: dwc: Add unroll iATU space support to the regions disable method Serge Semin
2022-05-17 12:50 ` [PATCH v3 04/13] PCI: dwc: Disable outbound windows for controllers with iATU Serge Semin
2022-05-17 12:50 ` [PATCH v3 05/13] PCI: dwc: Set INCREASE_REGION_SIZE flag based on limit address Serge Semin
2022-05-17 12:50 ` [PATCH v3 06/13] PCI: dwc: Add braces to the multi-line if-else statements Serge Semin
2022-05-17 12:50 ` [PATCH v3 07/13] PCI: dwc: Add trailing new-line literals to the log messages Serge Semin
2022-05-17 12:50 ` [PATCH v3 08/13] PCI: dwc: Discard IP-core version checking on unrolled iATU detection Serge Semin
2022-05-17 12:50 ` [PATCH v3 09/13] PCI: dwc: Convert Link-up status method to using dw_pcie_readl_dbi() Serge Semin
2022-05-17 12:50 ` [PATCH v3 10/13] PCI: dwc: Deallocate EPC memory on EP init error Serge Semin
2022-05-17 12:50 ` [PATCH v3 11/13] PCI: dwc-plat: Simplify the probe method return value handling Serge Semin
2022-05-17 12:50 ` [PATCH v3 12/13] PCI: dwc-plat: Discard unused regmap pointer Serge Semin
2022-05-17 12:50 ` [PATCH v3 13/13] PCI: dwc-plat: Drop dw_plat_pcie_of_match forward declaration Serge Semin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220527160551.dh6fb5n6xbmtjpaa@mobilestation \ \ \ \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).