| Message ID | 20170804161004.27770-4-marcel@ziswiler.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Warren |
| Headers | show |
On 08/04/2017 10:10 AM, Marcel Ziswiler wrote: > From: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > Make tegra_pcie_port_reset() a weak function with an explicit index > parameter. This allows overriding the PCIe port reset functionality > from board specific code as e.g. required for Apalis TK1. I think this change should be implemented differently. Does the patch description mean that: a) This change allows the board code to know which port is being reset. If so, simply have the board-specific implementation access port->index since it's already being passed port, and all callers are passing port->index as the index parameter. If the type isn't available, then you can add a tegra_pcie_port_index_of_port() function to retrieve it through the opaque type. or: b) That by changing the function signature code is able to choose between calling the board-specific reset override function and the PCIe driver low-level reset function? If so, let's just have two different function names, have all callers call the board-specific function only, and have the board-specific function call the driver function. There can be a weak default board-specific implementation that simply calls the driver function.
Hi Stephen On Fri, 2017-08-04 at 10:33 -0600, Stephen Warren wrote: > On 08/04/2017 10:10 AM, Marcel Ziswiler wrote: > > From: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > > > Make tegra_pcie_port_reset() a weak function with an explicit index > > parameter. This allows overriding the PCIe port reset functionality > > from board specific code as e.g. required for Apalis TK1. > > I think this change should be implemented differently. > > Does the patch description mean that: > > a) This change allows the board code to know which port is being > reset. > > If so, simply have the board-specific implementation access port- > >index > since it's already being passed port, and all callers are passing > port->index as the index parameter. If the type isn't available, > then > you can add a tegra_pcie_port_index_of_port() function to retrieve > it > through the opaque type. I did of course know that the port struct already contained that information but did not think of any such elegant way to do it as you describe outside of adding an explicit index. So I agree and will just implement and make use of such a new tegra_pcie_port_index_of_port() function. > or: > > b) That by changing the function signature code is able to choose > between calling the board-specific reset override function and the > PCIe > driver low-level reset function? > > If so, let's just have two different function names, have all > callers > call the board-specific function only, and have the board-specific > function call the driver function. There can be a weak default > board-specific implementation that simply calls the driver function. Initially that was not really my intention but I believe you bring up a very valid point and it could be useful to use the default reset behaviour on certain ports while using a custom one on others. This would e.g. be the case on Apalis T30 where the on-module PCIe gigabit Ethernet chip is using the standard out-of-band signalling while the PCIe switch on the Apalis Evaluation board requires the same special reset work-around as optionally implemented for Apalis TK1. I will therefore implement it this way as well leaving it completely up to board code which way to go. Thanks again for your valuable input. Cheers Marcel
diff --git a/drivers/pci/pci_tegra.c b/drivers/pci/pci_tegra.c index cb5cf8b..d373a88 100644 --- a/drivers/pci/pci_tegra.c +++ b/drivers/pci/pci_tegra.c @@ -893,7 +893,8 @@ static unsigned long tegra_pcie_port_get_pex_ctrl(struct tegra_pcie_port *port) return ret; } -static void tegra_pcie_port_reset(struct tegra_pcie_port *port) +void __weak tegra_pcie_port_reset(struct tegra_pcie_port *port, + unsigned int index) { unsigned long ctrl = tegra_pcie_port_get_pex_ctrl(port); unsigned long value; @@ -928,7 +929,7 @@ static void tegra_pcie_port_enable(struct tegra_pcie_port *port) afi_writel(pcie, value, ctrl); - tegra_pcie_port_reset(port); + tegra_pcie_port_reset(port, port->index); if (soc->force_pca_enable) { value = rp_readl(port, RP_VEND_CTL2); @@ -979,7 +980,7 @@ static bool tegra_pcie_port_check_link(struct tegra_pcie_port *port) } while (--timeout); retry: - tegra_pcie_port_reset(port); + tegra_pcie_port_reset(port, port->index); } while (--retries); return false;