Re: [PATCH 2 0/9] PCI hotplug: clean up slot configuration

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Bjorn Helgaas wrote:
> These patches clean up and unify the PCI slot configuration across
> the acpiphp, pciehp, and shpchp drivers.  When these drivers detect
> a newly-added device, they configure the bus.  On ACPI systems, this
> uses PCI bus settings from _HPP or _HPX methods.
> 
> Previously, each driver handled this separately with similar but not
> quite identical code.  This series adds a single pci_configure_slot()
> function and changes each driver to use it.
> 
> Changes from initial post to v2:
>     - use static inline empty function rather than #define for non-ACPI (Eike)
>     - export pci_configure_slot()
>     - add comment about why we don't program default PCI settings for PCIe
>     - tidy up PCIe setting check for PCIe device
>     - move pci_configure_slot() and related functions to acpi_pcihp.c (Kenji)
>     - make acpi_get_hp_params() static
> 
> ---
> 
> Bjorn Helgaas (9):
>       PCI hotplug: acpiphp: remove superfluous _HPP/_HPX evaluation
>       PCI hotplug: acpiphp: don't cache hotplug_params in acpiphp_bridge
>       PCI hotplug: rename acpi_get_hp_params_from_firmware() to acpi_get_hp_params()
>       PCI hotplug: add pci_configure_slot()
>       PCI hotplug: pciehp: use generic pci_configure_slot()
>       PCI hotplug: shpchp: use generic pci_configure_slot()
>       PCI hotplug: acpiphp: use generic pci_configure_slot()
>       PCI hotplug: clean up acpi_run_hpp()
>       PCI hotplug: make acpi_get_hp_params() static
> 
> 
>  drivers/pci/hotplug/acpi_pcihp.c   |  269 ++++++++++++++++++++++++++----------
>  drivers/pci/hotplug/acpiphp.h      |    3 
>  drivers/pci/hotplug/acpiphp_glue.c |   95 +------------
>  drivers/pci/hotplug/pciehp.h       |    9 -
>  drivers/pci/hotplug/pciehp_pci.c   |  132 ------------------
>  drivers/pci/hotplug/pcihp_slot.c   |    0 
>  drivers/pci/hotplug/shpchp.h       |    9 -
>  drivers/pci/hotplug/shpchp_pci.c   |   62 --------
>  include/linux/pci_hotplug.h        |    5 -
>  9 files changed, 205 insertions(+), 379 deletions(-)
>  create mode 100644 drivers/pci/hotplug/pcihp_slot.c
> 

Hi Bjorn-san,

I tried the set of patches on both SHPC and PCI-E hotplug slots
using shpchp driver and pciehp driver respectively and confirmed
it works well (I also confirmed there are no difference on
hot-plugging in PCI/PCIE configuration space between before and
after applying set of patches).

Unfortunately, I didn't test acpiphp due to the lack of testing
environment. So I can do only reviewing for acpiphp. In my
understanding, your patches are not only for cleanup but it also
enables acpiphp to program _HPX type2 parameters on PCI-E hotplug
slots. If I must say something about acpiphp, it would be good
if there is an description about it.

By the way, I think we can do further cleanup regarding hotplug
parameters. I think struct hpp_type? and struct hotplug_params
in include/linux/pci_hotplug.h can be moved to acpi_pcihp.c like
attached below.

Anyway, your patches looks very good to me.

Reviewed-by: Kenji Kaneshige <kaneshige.kenji@xxxxxxxxxxxxxx>
Tested-by: Kenji Kaneshige <kaneshige.kenji@xxxxxxxxxxxxxx>

Thanks,
Kenji Kaneshige


---
 drivers/pci/hotplug/acpi_pcihp.c |   47 +++++++++++++++++++++++++++++++++++++++
 include/linux/pci_hotplug.h      |   47 ---------------------------------------
 2 files changed, 47 insertions(+), 47 deletions(-)

Index: 20090827/drivers/pci/hotplug/acpi_pcihp.c
===================================================================
--- 20090827.orig/drivers/pci/hotplug/acpi_pcihp.c
+++ 20090827/drivers/pci/hotplug/acpi_pcihp.c
@@ -46,6 +46,53 @@
 #define	METHOD_NAME__SUN	"_SUN"
 #define	METHOD_NAME_OSHP	"OSHP"
 
+/* PCI Setting Record (Type 0) */
+struct hpp_type0 {
+	u32 revision;
+	u8  cache_line_size;
+	u8  latency_timer;
+	u8  enable_serr;
+	u8  enable_perr;
+};
+
+/* PCI-X Setting Record (Type 1) */
+struct hpp_type1 {
+	u32 revision;
+	u8  max_mem_read;
+	u8  avg_max_split;
+	u16 tot_max_split;
+};
+
+/* PCI Express Setting Record (Type 2) */
+struct hpp_type2 {
+	u32 revision;
+	u32 unc_err_mask_and;
+	u32 unc_err_mask_or;
+	u32 unc_err_sever_and;
+	u32 unc_err_sever_or;
+	u32 cor_err_mask_and;
+	u32 cor_err_mask_or;
+	u32 adv_err_cap_and;
+	u32 adv_err_cap_or;
+	u16 pci_exp_devctl_and;
+	u16 pci_exp_devctl_or;
+	u16 pci_exp_lnkctl_and;
+	u16 pci_exp_lnkctl_or;
+	u32 sec_unc_err_sever_and;
+	u32 sec_unc_err_sever_or;
+	u32 sec_unc_err_mask_and;
+	u32 sec_unc_err_mask_or;
+};
+
+struct hotplug_params {
+	struct hpp_type0 *t0;		/* Type0: NULL if not available */
+	struct hpp_type1 *t1;		/* Type1: NULL if not available */
+	struct hpp_type2 *t2;		/* Type2: NULL if not available */
+	struct hpp_type0 type0_data;
+	struct hpp_type1 type1_data;
+	struct hpp_type2 type2_data;
+};
+
 static int debug_acpi;
 
 static acpi_status
Index: 20090827/include/linux/pci_hotplug.h
===================================================================
--- 20090827.orig/include/linux/pci_hotplug.h
+++ 20090827/include/linux/pci_hotplug.h
@@ -177,53 +177,6 @@ static inline int pci_hp_register(struct
 				 THIS_MODULE, KBUILD_MODNAME);
 }
 
-/* PCI Setting Record (Type 0) */
-struct hpp_type0 {
-	u32 revision;
-	u8  cache_line_size;
-	u8  latency_timer;
-	u8  enable_serr;
-	u8  enable_perr;
-};
-
-/* PCI-X Setting Record (Type 1) */
-struct hpp_type1 {
-	u32 revision;
-	u8  max_mem_read;
-	u8  avg_max_split;
-	u16 tot_max_split;
-};
-
-/* PCI Express Setting Record (Type 2) */
-struct hpp_type2 {
-	u32 revision;
-	u32 unc_err_mask_and;
-	u32 unc_err_mask_or;
-	u32 unc_err_sever_and;
-	u32 unc_err_sever_or;
-	u32 cor_err_mask_and;
-	u32 cor_err_mask_or;
-	u32 adv_err_cap_and;
-	u32 adv_err_cap_or;
-	u16 pci_exp_devctl_and;
-	u16 pci_exp_devctl_or;
-	u16 pci_exp_lnkctl_and;
-	u16 pci_exp_lnkctl_or;
-	u32 sec_unc_err_sever_and;
-	u32 sec_unc_err_sever_or;
-	u32 sec_unc_err_mask_and;
-	u32 sec_unc_err_mask_or;
-};
-
-struct hotplug_params {
-	struct hpp_type0 *t0;		/* Type0: NULL if not available */
-	struct hpp_type1 *t1;		/* Type1: NULL if not available */
-	struct hpp_type2 *t2;		/* Type2: NULL if not available */
-	struct hpp_type0 type0_data;
-	struct hpp_type1 type1_data;
-	struct hpp_type2 type2_data;
-};
-
 #ifdef CONFIG_ACPI
 #include <acpi/acpi.h>
 #include <acpi/acpi_bus.h>

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux