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@xxxxxxxxxx>
> >> ---
> >> 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:
signature.asc
Description: PGP signature
