| Message ID | 4A9B5ACF.3080900@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Sunday 30 August 2009 11:08:31 pm Kenji Kaneshige wrote: > 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. Yes, you're right, I should mention that change. Oh, actually, I *did* mention it in the changelog for patch 7/9. Do you think I should call that out more? > 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. I'll take a look at this. It's connected with a mistake I made in version 2 of this series: by moving pci_configure_slot() entirely into acpi_pcihp.c, I inadvertently removed the SHPC path that configured default PCI settings for non-ACPI systems. I'll have to work on that a bit more. Thanks a lot for testing and reviewing these! Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas wrote: > On Sunday 30 August 2009 11:08:31 pm Kenji Kaneshige wrote: >> 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. > > Yes, you're right, I should mention that change. Oh, actually, > I *did* mention it in the changelog for patch 7/9. Do you think > I should call that out more? Sorry, I overlooked it. I think it is enough. > >> 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. > > I'll take a look at this. It's connected with a mistake I made > in version 2 of this series: by moving pci_configure_slot() > entirely into acpi_pcihp.c, I inadvertently removed the SHPC path > that configured default PCI settings for non-ACPI systems. > > I'll have to work on that a bit more. Thanks a lot for testing > and reviewing these! > I was wondering if I could ask you a favor. The program_hpp_type0() programs latency timer and cache-line size even for PCI-E, but I don't think we need to program those values for PCI-E. It is original pciehp's behavior, but could you consider fixing it in you set of patches? Thanks, Kenji Kaneshige -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 31 August 2009 06:19:10 pm Kenji Kaneshige wrote: > I was wondering if I could ask you a favor. The program_hpp_type0() > programs latency timer and cache-line size even for PCI-E, but I > don't think we need to program those values for PCI-E. It is original > pciehp's behavior, but could you consider fixing it in you set of > patches? It's true that the latency timer and cache-line size are not relevant for PCIe devices (but it appears harmless to write them). However, my reading of sections 7.5.1.1 and 7.5.1.7 of the PCI Express Base Spec (rev 1.1) is that SERR and PERR are still relevant, though we may prefer the new PCI Express error reporting mechanism. So I agree with you that there's an opportunity to make this better, but I don't feel qualified to change that behavior because I don't understand the interaction with PCI Express error reporting. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas wrote: > On Monday 31 August 2009 06:19:10 pm Kenji Kaneshige wrote: >> I was wondering if I could ask you a favor. The program_hpp_type0() >> programs latency timer and cache-line size even for PCI-E, but I >> don't think we need to program those values for PCI-E. It is original >> pciehp's behavior, but could you consider fixing it in you set of >> patches? > > It's true that the latency timer and cache-line size are not > relevant for PCIe devices (but it appears harmless to write them). > However, my reading of sections 7.5.1.1 and 7.5.1.7 of the PCI > Express Base Spec (rev 1.1) is that SERR and PERR are still > relevant, though we may prefer the new PCI Express error > reporting mechanism. What I thought was we latency timer and cache-line size don't need to be programed but we still need SERR and PERR, as you explained. BTW, ACPI spec also mentions this topic in the definition of PCI Setting Record (Type 0). > > So I agree with you that there's an opportunity to make this > better, but I don't feel qualified to change that behavior > because I don't understand the interaction with PCI Express > error reporting. > Ok, I think it should be done in another patch. It's my homework after your work is done. Thank you for your detailed comment and sorry for troubling you. Thanks, Kenji Kaneshige -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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>