Re: [PATCH V6 6/7] PCI: tegra: Broadcast PME_Turn_Off message before link goes to L2

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5244 bytes --]

On Mon, Jan 29, 2018 at 10:11:42AM +0530, Manikanta Maddireddy wrote:
> 
> 
> On 25-Jan-18 8:06 PM, Thierry Reding wrote:
> > On Thu, Jan 11, 2018 at 11:38:07AM +0530, Manikanta Maddireddy wrote:
> >> Per PCIe r3.0, sec 5.3.3.2.1, PCIe root port shoould broadcast PME_Turn_Off
> >> message before PCIe link goes to L2. PME_Turn_Off broadcast mechanism is
> >> implemented in AFI module. Each Tegra PCIe root port has its own
> >> PME_Turn_Off and PME_TO_Ack bitmap in AFI_PME register, program this
> >> register to broadcast PME_Turn_Off message.
> >>
> >> Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com>
> >> ---
> >> V2:
> >> * no change in this patch
> >> V3:
> >> * add PME bitmap in soc data instead of using compatible string
> >> * replace while loop with readl_poll_timeout() for polling
> >> * commit log correction
> >> V4:
> >> * no change in this patch
> >> V5:
> >> * Rebased on linux-next
> >> V6:
> >> * no change in this patch
> >>
> >>  drivers/pci/host/pci-tegra.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 45 insertions(+)
> >>
> >> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> >> index 981f126b14d6..cc33fc0fb300 100644
> >> --- a/drivers/pci/host/pci-tegra.c
> >> +++ b/drivers/pci/host/pci-tegra.c
> >> @@ -31,6 +31,7 @@
> >>  #include <linux/delay.h>
> >>  #include <linux/export.h>
> >>  #include <linux/interrupt.h>
> >> +#include <linux/iopoll.h>
> >>  #include <linux/irq.h>
> >>  #include <linux/irqdomain.h>
> >>  #include <linux/kernel.h>
> >> @@ -153,6 +154,8 @@
> >>  #define  AFI_INTR_EN_FPCI_TIMEOUT	(1 << 7)
> >>  #define  AFI_INTR_EN_PRSNT_SENSE	(1 << 8)
> >>  
> >> +#define AFI_PCIE_PME		0xf0
> >> +
> >>  #define AFI_PCIE_CONFIG					0x0f8
> >>  #define  AFI_PCIE_CONFIG_PCIE_DISABLE(x)		(1 << ((x) + 1))
> >>  #define  AFI_PCIE_CONFIG_PCIE_DISABLE_ALL		0xe
> >> @@ -233,6 +236,8 @@
> >>  #define PADS_REFCLK_CFG_PREDI_SHIFT		8  /* 11:8 */
> >>  #define PADS_REFCLK_CFG_DRVI_SHIFT		12 /* 15:12 */
> >>  
> >> +#define PME_ACK_TIMEOUT 10000
> >> +
> >>  struct tegra_msi {
> >>  	struct msi_controller chip;
> >>  	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
> >> @@ -251,6 +256,8 @@ struct tegra_pcie_soc {
> >>  	u32 tx_ref_sel;
> >>  	u32 pads_refclk_cfg0;
> >>  	u32 pads_refclk_cfg1;
> >> +	u8 pme_turnoff_bit[3];
> >> +	u8 pme_ack_bit[3];
> > 
> > This seems suboptimal to me. Perhaps a better way would be:
> > 
> > 	struct tegra_pcie_port_soc {
> > 		struct {
> > 			u8 turnoff_bit;
> > 			u8 ack_bit;
> > 		} pme;
> > 	};
> > 
> > And since we already have num_ports which defines exactly how many ports
> > we have:
> > 
> > 	struct tegra_pcie_soc {
> > 		unsigned int num_ports;
> > 		const struct tegra_pcie_port_soc *ports;
> > 		...
> > 	};
> > 
> > I suspect that as you're adding more features we may need more of this
> > data.
> > 
> > But I'm fine to keep it like this. We can always switch to something
> > different if the above becomes too much cluttered.
> > 
> I agree it is sub optimal. I moved pme bitmap values to soc data because
> tegra30 and tegra186 have different bitmap values. To differentiate this
> I allocated static memory in tegra_pcie_soc. I am a bit hesitant do dynamic
> allocation based on num_ports because I need to use compatible string
> to distinguish between tegra30 & tegra186 in runtime. I believe it is OK
> to have trade off 16 bits to avoid these compatible check?

Sorry if I was unclear. I didn't suggest that you'd dynamically allocate
memory, instead you'd hook it up like so:

	static const struct tegra_pcie_port_soc tegra186_pcie_ports[] = {
		{ .turnoff_bit =  0, .ack_bit =  5 },
		{ .turnoff_bit =  8, .ack_bit = 10 },
		{ .turnoff_bit = 12, .ack_bit = 14 },
	};

And then in the existing tegra186_pcie, you'd set this:

	static const struct tegra_pcie_soc tegra186_pcie = {
		.num_ports = 3,
		.ports = tegra186_pcie_ports,
		...
	};

And similar for the others. I realize that this is somewhat more verbose
than your original, but I think it's more readable.

It also allows you to reuse data, such as:

	static const struct tegra_pcie_port_soc tegra20_pcie_ports[] = {
		{ .turnoff_bit = 0, .ack_bit =  5 },
		{ .turnoff_bit = 8, .ack_bit = 10 },
	};

	...

	static const struct tegra_pcie_soc tegra20_pcie = {
		.num_ports = 2,
		.ports = tegra20_pcie_ports,
		...
	};

	...

	static const struct tegra_pcie_soc tegra124_pcie = {
		.num_ports = 2,
		.ports = tegra20_pcie_ports,
		...
	};

	static const struct tegra_pcie_soc tegra210_pcie = {
		.num_ports = 2,
		.ports = tegra20_pcie_ports,
		...
	};

Of course the sharing only works as long as the port definitions don't
contain data that's different between the chips.

You could even go and derive the .num_ports as ARRAY_SIZE() of the ports
definition.

So the above does not dynamically allocate memory, it is just a more
explicit way of specifying the data. It puts the data closer together,
and thereby makes it easier to read, in my opinion.

But as I said, feel free to leave this as-is. We can easily rework this
later on or if necessary.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]