Patchwork [PATCHv4,1/2] ppc64: perform proper max_bus_speed detection

login
register
mail settings
Submitter Lucas Kannebley Tavares
Date April 24, 2013, 10:54 p.m.
Message ID <1366844090-5492-2-git-send-email-lucaskt@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/239329/
State Accepted, archived
Headers show

Comments

Lucas Kannebley Tavares - April 24, 2013, 10:54 p.m.
From: Lucas Kannebley Tavares <lucaskt@linux.vnet.ibm.com>

On pseries machines the detection for max_bus_speed should be done
through an OpenFirmware property. This patch adds a function to perform
this detection and a hook to perform dynamic adding of the function only for
pseries. This is done by overwriting the weak
pcibios_root_bridge_prepare function which is called by pci_create_root_bus().

Signed-off-by: Lucas Kannebley Tavares <lucaskt@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/machdep.h       |  2 ++
 arch/powerpc/kernel/pci-common.c         |  8 +++++
 arch/powerpc/platforms/pseries/pci.c     | 51 ++++++++++++++++++++++++++++++++
 arch/powerpc/platforms/pseries/pseries.h |  4 +++
 arch/powerpc/platforms/pseries/setup.c   |  2 ++
 5 files changed, 67 insertions(+)
Tony Breeds - April 24, 2013, 11:48 p.m.
On Wed, Apr 24, 2013 at 07:54:49PM -0300, lucaskt@linux.vnet.ibm.com wrote:
> From: Lucas Kannebley Tavares <lucaskt@linux.vnet.ibm.com>
> 
> On pseries machines the detection for max_bus_speed should be done
> through an OpenFirmware property. This patch adds a function to perform
> this detection and a hook to perform dynamic adding of the function only for
> pseries. This is done by overwriting the weak
> pcibios_root_bridge_prepare function which is called by pci_create_root_bus().
> 
> Signed-off-by: Lucas Kannebley Tavares <lucaskt@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/asm/machdep.h       |  2 ++
>  arch/powerpc/kernel/pci-common.c         |  8 +++++
>  arch/powerpc/platforms/pseries/pci.c     | 51 ++++++++++++++++++++++++++++++++
>  arch/powerpc/platforms/pseries/pseries.h |  4 +++
>  arch/powerpc/platforms/pseries/setup.c   |  2 ++
>  5 files changed, 67 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
> index 3d6b410..8f558bf 100644
> --- a/arch/powerpc/include/asm/machdep.h
> +++ b/arch/powerpc/include/asm/machdep.h
> @@ -107,6 +107,8 @@ struct machdep_calls {
>  	void		(*pcibios_fixup)(void);
>  	int		(*pci_probe_mode)(struct pci_bus *);
>  	void		(*pci_irq_fixup)(struct pci_dev *dev);
> +	int		(*pcibios_root_bridge_prepare)(struct pci_host_bridge
> +				*bridge);
>  
>  	/* To setup PHBs when using automatic OF platform driver for PCI */
>  	int		(*pci_setup_phb)(struct pci_controller *host);
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index fa12ae4..80986cf 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -844,6 +844,14 @@ int pci_proc_domain(struct pci_bus *bus)
>  	return 1;
>  }
>  
> +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> +{
> +	if (ppc_md.pcibios_root_bridge_prepare)
> +		return ppc_md.pcibios_root_bridge_prepare(bridge);
> +
> +	return 0;
> +}
> +
>  /* This header fixup will do the resource fixup for all devices as they are
>   * probed, but not for bridge ranges
>   */
> diff --git a/arch/powerpc/platforms/pseries/pci.c b/arch/powerpc/platforms/pseries/pci.c
> index 0b580f4..7f9c956 100644
> --- a/arch/powerpc/platforms/pseries/pci.c
> +++ b/arch/powerpc/platforms/pseries/pci.c
> @@ -108,3 +108,54 @@ static void fixup_winbond_82c105(struct pci_dev* dev)
>  }
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_WINBOND, PCI_DEVICE_ID_WINBOND_82C105,
>  			 fixup_winbond_82c105);
> +
> +int pseries_root_bridge_prepare(struct pci_host_bridge *bridge)
> +{
> +	struct device_node *dn, *pdn;
> +	struct pci_bus *bus;
> +	const uint32_t *pcie_link_speed_stats;
> +
> +	bus = bridge->bus;
> +
> +	dn = pcibios_get_phb_of_node(bus);
> +	if (!dn)
> +		return 0;
> +
> +	for (pdn = dn; pdn != NULL; pdn = pdn->parent) {
> +		pcie_link_speed_stats = (const uint32_t *) of_get_property(dn,
> +			"ibm,pcie-link-speed-stats", NULL);
> +		if (pcie_link_speed_stats)
> +			break;
> +	}

Please use the helpers in include/linux/of.h rather than open coding
this.

Yours Tony
Lucas Kannebley Tavares - April 25, 2013, 5:34 p.m.
On 04/24/2013 08:48 PM, Tony Breeds wrote:
> On Wed, Apr 24, 2013 at 07:54:49PM -0300, lucaskt@linux.vnet.ibm.com wrote:
>> From: Lucas Kannebley Tavares<lucaskt@linux.vnet.ibm.com>
>>
>> On pseries machines the detection for max_bus_speed should be done
>> through an OpenFirmware property. This patch adds a function to perform
>> this detection and a hook to perform dynamic adding of the function only for
>> pseries. This is done by overwriting the weak
>> pcibios_root_bridge_prepare function which is called by pci_create_root_bus().
>>
>> Signed-off-by: Lucas Kannebley Tavares<lucaskt@linux.vnet.ibm.com>
>> ---
>>   arch/powerpc/include/asm/machdep.h       |  2 ++
>>   arch/powerpc/kernel/pci-common.c         |  8 +++++
>>   arch/powerpc/platforms/pseries/pci.c     | 51 ++++++++++++++++++++++++++++++++
>>   arch/powerpc/platforms/pseries/pseries.h |  4 +++
>>   arch/powerpc/platforms/pseries/setup.c   |  2 ++
>>   5 files changed, 67 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
>> index 3d6b410..8f558bf 100644
>> --- a/arch/powerpc/include/asm/machdep.h
>> +++ b/arch/powerpc/include/asm/machdep.h
>> @@ -107,6 +107,8 @@ struct machdep_calls {
>>   	void		(*pcibios_fixup)(void);
>>   	int		(*pci_probe_mode)(struct pci_bus *);
>>   	void		(*pci_irq_fixup)(struct pci_dev *dev);
>> +	int		(*pcibios_root_bridge_prepare)(struct pci_host_bridge
>> +				*bridge);
>>
>>   	/* To setup PHBs when using automatic OF platform driver for PCI */
>>   	int		(*pci_setup_phb)(struct pci_controller *host);
>> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
>> index fa12ae4..80986cf 100644
>> --- a/arch/powerpc/kernel/pci-common.c
>> +++ b/arch/powerpc/kernel/pci-common.c
>> @@ -844,6 +844,14 @@ int pci_proc_domain(struct pci_bus *bus)
>>   	return 1;
>>   }
>>
>> +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
>> +{
>> +	if (ppc_md.pcibios_root_bridge_prepare)
>> +		return ppc_md.pcibios_root_bridge_prepare(bridge);
>> +
>> +	return 0;
>> +}
>> +
>>   /* This header fixup will do the resource fixup for all devices as they are
>>    * probed, but not for bridge ranges
>>    */
>> diff --git a/arch/powerpc/platforms/pseries/pci.c b/arch/powerpc/platforms/pseries/pci.c
>> index 0b580f4..7f9c956 100644
>> --- a/arch/powerpc/platforms/pseries/pci.c
>> +++ b/arch/powerpc/platforms/pseries/pci.c
>> @@ -108,3 +108,54 @@ static void fixup_winbond_82c105(struct pci_dev* dev)
>>   }
>>   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_WINBOND, PCI_DEVICE_ID_WINBOND_82C105,
>>   			 fixup_winbond_82c105);
>> +
>> +int pseries_root_bridge_prepare(struct pci_host_bridge *bridge)
>> +{
>> +	struct device_node *dn, *pdn;
>> +	struct pci_bus *bus;
>> +	const uint32_t *pcie_link_speed_stats;
>> +
>> +	bus = bridge->bus;
>> +
>> +	dn = pcibios_get_phb_of_node(bus);
>> +	if (!dn)
>> +		return 0;
>> +
>> +	for (pdn = dn; pdn != NULL; pdn = pdn->parent) {
>> +		pcie_link_speed_stats = (const uint32_t *) of_get_property(dn,
>> +			"ibm,pcie-link-speed-stats", NULL);
>> +		if (pcie_link_speed_stats)
>> +			break;
>> +	}
>
> Please use the helpers in include/linux/of.h rather than open coding
> this.
>
> Yours Tony


Hi Tony,


This is what I can find as an equivalent code:

	for (pdn = dn; pdn != NULL; pdn = of_get_next_parent(pdn)) {
		pcie_link_speed_stats = (const uint32_t *)
			of_get_property(dn,
				"ibm,pcie-link-speed-stats", NULL);
		if (pcie_link_speed_stats)
			break;
	}

is this your suggestion, or was it another approach that will have the 
same result?

Thanks,
Kleber Sacilotto de Souza - May 2, 2013, 3:21 p.m.
On 04/25/2013 02:34 PM, Lucas Kannebley Tavares wrote:
> On 04/24/2013 08:48 PM, Tony Breeds wrote:
>>> diff --git a/arch/powerpc/platforms/pseries/pci.c
>>> b/arch/powerpc/platforms/pseries/pci.c
>>> index 0b580f4..7f9c956 100644
>>> --- a/arch/powerpc/platforms/pseries/pci.c
>>> +++ b/arch/powerpc/platforms/pseries/pci.c
>>> @@ -108,3 +108,54 @@ static void fixup_winbond_82c105(struct pci_dev*
>>> dev)
>>>   }
>>>   DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_WINBOND,
>>> PCI_DEVICE_ID_WINBOND_82C105,
>>>                fixup_winbond_82c105);
>>> +
>>> +int pseries_root_bridge_prepare(struct pci_host_bridge *bridge)
>>> +{
>>> +    struct device_node *dn, *pdn;
>>> +    struct pci_bus *bus;
>>> +    const uint32_t *pcie_link_speed_stats;
>>> +
>>> +    bus = bridge->bus;
>>> +
>>> +    dn = pcibios_get_phb_of_node(bus);
>>> +    if (!dn)
>>> +        return 0;
>>> +
>>> +    for (pdn = dn; pdn != NULL; pdn = pdn->parent) {
>>> +        pcie_link_speed_stats = (const uint32_t *) of_get_property(dn,
>>> +            "ibm,pcie-link-speed-stats", NULL);
>>> +        if (pcie_link_speed_stats)
>>> +            break;
>>> +    }
>>
>> Please use the helpers in include/linux/of.h rather than open coding
>> this.
>>
>> Yours Tony
> 
> 
> Hi Tony,
> 
> 
> This is what I can find as an equivalent code:
> 
>     for (pdn = dn; pdn != NULL; pdn = of_get_next_parent(pdn)) {
>         pcie_link_speed_stats = (const uint32_t *)
>             of_get_property(dn,
>                 "ibm,pcie-link-speed-stats", NULL);
>         if (pcie_link_speed_stats)
>             break;
>     }
> 
> is this your suggestion, or was it another approach that will have the
> same result?
> 
> Thanks,
> 

Hi Tony,

It seems Lucas' change is a bit incomplete and is not handling the reference counter to
the device_node correctly. Is the following change what you had in mind?


	dn = pcibios_get_phb_of_node(bus);
	if (!dn)
		return 0;

	for (pdn = dn; pdn != NULL; pdn = of_get_next_parent(pdn)) {
		pcie_link_speed_stats = (const uint32_t *) of_get_property(pdn,
			"ibm,pcie-link-speed-stats", NULL);
		if (pcie_link_speed_stats)
			break;
	}

	of_node_put(pdn);


Thanks,
Tony Breeds - May 3, 2013, 6:40 a.m.
On Thu, May 02, 2013 at 12:21:37PM -0300, Kleber Sacilotto de Souza wrote:

> Hi Tony,
> 
> It seems Lucas' change is a bit incomplete and is not handling the reference counter to
> the device_node correctly. Is the following change what you had in mind?

Ahh Sorry I expected there would be a for_each_parent_of_node macro.
I did a quick grep and it seems that's not very common, so open coding
it should be fine.
 
> 
> 	dn = pcibios_get_phb_of_node(bus);
> 	if (!dn)
> 		return 0;
> 
> 	for (pdn = dn; pdn != NULL; pdn = of_get_next_parent(pdn)) {
> 		pcie_link_speed_stats = (const uint32_t *) of_get_property(pdn,
> 			"ibm,pcie-link-speed-stats", NULL);
> 		if (pcie_link_speed_stats)
> 			break;
> 	}
> 
> 	of_node_put(pdn);

I think you need the of_node_put() in the body of the loop, otherwise
aren't you leaking refcounts?
 
Yours Tony
Kleber Sacilotto de Souza - May 3, 2013, 11:55 a.m.
On 05/03/2013 03:40 AM, Tony Breeds wrote:
> On Thu, May 02, 2013 at 12:21:37PM -0300, Kleber Sacilotto de Souza wrote:
> 
>> Hi Tony,
>>
>> It seems Lucas' change is a bit incomplete and is not handling the reference counter to
>> the device_node correctly. Is the following change what you had in mind?
> 
> Ahh Sorry I expected there would be a for_each_parent_of_node macro.
> I did a quick grep and it seems that's not very common, so open coding
> it should be fine.
>  
>>
>> 	dn = pcibios_get_phb_of_node(bus);
>> 	if (!dn)
>> 		return 0;
>>
>> 	for (pdn = dn; pdn != NULL; pdn = of_get_next_parent(pdn)) {
>> 		pcie_link_speed_stats = (const uint32_t *) of_get_property(pdn,
>> 			"ibm,pcie-link-speed-stats", NULL);
>> 		if (pcie_link_speed_stats)
>> 			break;
>> 	}
>>
>> 	of_node_put(pdn);
> 
> I think you need the of_node_put() in the body of the loop, otherwise
> aren't you leaking refcounts?

of_get_next_parent() takes care of that. It does of_node_put() on the
current node after doing of_node_get() on the parent.


Thanks,

Patch

diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
index 3d6b410..8f558bf 100644
--- a/arch/powerpc/include/asm/machdep.h
+++ b/arch/powerpc/include/asm/machdep.h
@@ -107,6 +107,8 @@  struct machdep_calls {
 	void		(*pcibios_fixup)(void);
 	int		(*pci_probe_mode)(struct pci_bus *);
 	void		(*pci_irq_fixup)(struct pci_dev *dev);
+	int		(*pcibios_root_bridge_prepare)(struct pci_host_bridge
+				*bridge);
 
 	/* To setup PHBs when using automatic OF platform driver for PCI */
 	int		(*pci_setup_phb)(struct pci_controller *host);
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index fa12ae4..80986cf 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -844,6 +844,14 @@  int pci_proc_domain(struct pci_bus *bus)
 	return 1;
 }
 
+int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
+{
+	if (ppc_md.pcibios_root_bridge_prepare)
+		return ppc_md.pcibios_root_bridge_prepare(bridge);
+
+	return 0;
+}
+
 /* This header fixup will do the resource fixup for all devices as they are
  * probed, but not for bridge ranges
  */
diff --git a/arch/powerpc/platforms/pseries/pci.c b/arch/powerpc/platforms/pseries/pci.c
index 0b580f4..7f9c956 100644
--- a/arch/powerpc/platforms/pseries/pci.c
+++ b/arch/powerpc/platforms/pseries/pci.c
@@ -108,3 +108,54 @@  static void fixup_winbond_82c105(struct pci_dev* dev)
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_WINBOND, PCI_DEVICE_ID_WINBOND_82C105,
 			 fixup_winbond_82c105);
+
+int pseries_root_bridge_prepare(struct pci_host_bridge *bridge)
+{
+	struct device_node *dn, *pdn;
+	struct pci_bus *bus;
+	const uint32_t *pcie_link_speed_stats;
+
+	bus = bridge->bus;
+
+	dn = pcibios_get_phb_of_node(bus);
+	if (!dn)
+		return 0;
+
+	for (pdn = dn; pdn != NULL; pdn = pdn->parent) {
+		pcie_link_speed_stats = (const uint32_t *) of_get_property(dn,
+			"ibm,pcie-link-speed-stats", NULL);
+		if (pcie_link_speed_stats)
+			break;
+	}
+
+	if (!pcie_link_speed_stats) {
+		pr_err("no ibm,pcie-link-speed-stats property\n");
+		return 0;
+	}
+
+	switch (pcie_link_speed_stats[0]) {
+	case 0x01:
+		bus->max_bus_speed = PCIE_SPEED_2_5GT;
+		break;
+	case 0x02:
+		bus->max_bus_speed = PCIE_SPEED_5_0GT;
+		break;
+	default:
+		bus->max_bus_speed = PCI_SPEED_UNKNOWN;
+		break;
+	}
+
+	switch (pcie_link_speed_stats[1]) {
+	case 0x01:
+		bus->cur_bus_speed = PCIE_SPEED_2_5GT;
+		break;
+	case 0x02:
+		bus->cur_bus_speed = PCIE_SPEED_5_0GT;
+		break;
+	default:
+		bus->cur_bus_speed = PCI_SPEED_UNKNOWN;
+		break;
+	}
+
+	return 0;
+}
diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h
index 9a3dda0..b79393d 100644
--- a/arch/powerpc/platforms/pseries/pseries.h
+++ b/arch/powerpc/platforms/pseries/pseries.h
@@ -60,4 +60,8 @@  extern int dlpar_detach_node(struct device_node *);
 /* Snooze Delay, pseries_idle */
 DECLARE_PER_CPU(long, smt_snooze_delay);
 
+/* PCI root bridge prepare function override for pseries */
+struct pci_host_bridge;
+int pseries_root_bridge_prepare(struct pci_host_bridge *bridge);
+
 #endif /* _PSERIES_PSERIES_H */
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index 8bcc9ca..bf34cc9 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -466,6 +466,8 @@  static void __init pSeries_setup_arch(void)
 	else
 		ppc_md.enable_pmcs = power4_enable_pmcs;
 
+	ppc_md.pcibios_root_bridge_prepare = pseries_root_bridge_prepare;
+
 	if (firmware_has_feature(FW_FEATURE_SET_MODE)) {
 		long rc;
 		if ((rc = pSeries_enable_reloc_on_exc()) != H_SUCCESS) {