On Thu, Jun 04, 2015 at 04:42:11PM +1000, Gavin Shan wrote:
The patch intends to add standalone driver to support PCI hotplug
for PowerPC PowerNV platform, which runs on top of skiboot firmware.
The firmware identified hotpluggable slots and marked their device
tree node with proper "ibm,slot-pluggable" and "ibm,reset-by-firmware".
The driver simply scans device-tree to create/register PCI hotplug slot
accordingly.
If the skiboot firmware doesn't support slot status retrieval, the PCI
slot device node shouldn't have property "ibm,reset-by-firmware". In
that case, none of valid PCI slots will be detected from device tree.
The skiboot firmware doesn't export the capability to access attention
LEDs yet and it's something for TBD.
Signed-off-by: Gavin Shan <redacted>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
But I do have a few comments (my ack is valid whether you do anything with
them or not):
+static void slot_power_off_handler(struct powernv_php_slot *slot)
+{
+ int ret;
+
+ /* Release the firmware data for the child device nodes */
+ remove_child_pdn(slot->dn);
+
+ /*
+ * Release the child device nodes. If the sub-tree was
+ * built with the help of overlay, we just need revert
+ * the changes introduced by the overlay
+ */
+ if (slot->overlay_id >= 0) {
+ ret = of_overlay_destroy(slot->overlay_id);
+ if (ret)
+ pr_warn("%s: Error %d destroying overlay %d\n",
+ __func__, ret, slot->overlay_id);
For this and similar messages: isn't there a device you can use with
dev_warn() here? I think a device name would be much better than a
function name.
+scan:
+ switch (presence) {
+ case POWERNV_PHP_SLOT_PRESENT:
+ if (rescan) {
+ pci_lock_rescan_remove();
+ pcibios_add_pci_devices(slot->bus);
You didn't add this, but "pcibios_add_pci_devices" doesn't seem like the
right name. "pcibios" generally refers to an arch-specific hook that's
called by the generic PCI core. In this case, pcibios_add_pci_devices()
contains powerpc-specific code, and it's only called from powerpc code, so
I think using "pcibios_" in the name is a bit misleading.
+ /* Remove all devices behind the slot */
+ pci_lock_rescan_remove();
+ pcibios_remove_pci_devices(slot->bus);
Same comment for pcibios_remove_pci_devices(). It would be better if the
name didn't suggest that this was part of the pcibios_ interface between
the PCI core and the arch code, because it's not.
+ /* Slot indentifier */
s/indentifier/identifier/
+ if (!php_slot_get_id(dn, &id))
+ return NULL;
+
+ /* PCI bus */
+ bus = pcibios_find_pci_bus(dn);
And pcibios_find_pci_bus() (it's also powerpc-specific).
Bjorn
On Fri, Jun 05, 2015 at 03:11:10PM -0500, Bjorn Helgaas wrote:
On Thu, Jun 04, 2015 at 04:42:11PM +1000, Gavin Shan wrote:
quoted
The patch intends to add standalone driver to support PCI hotplug
for PowerPC PowerNV platform, which runs on top of skiboot firmware.
The firmware identified hotpluggable slots and marked their device
tree node with proper "ibm,slot-pluggable" and "ibm,reset-by-firmware".
The driver simply scans device-tree to create/register PCI hotplug slot
accordingly.
If the skiboot firmware doesn't support slot status retrieval, the PCI
slot device node shouldn't have property "ibm,reset-by-firmware". In
that case, none of valid PCI slots will be detected from device tree.
The skiboot firmware doesn't export the capability to access attention
LEDs yet and it's something for TBD.
Signed-off-by: Gavin Shan <redacted>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
But I do have a few comments (my ack is valid whether you do anything with
them or not):
Thanks for your review, Bjorn.
quoted
+static void slot_power_off_handler(struct powernv_php_slot *slot)
+{
+ int ret;
+
+ /* Release the firmware data for the child device nodes */
+ remove_child_pdn(slot->dn);
+
+ /*
+ * Release the child device nodes. If the sub-tree was
+ * built with the help of overlay, we just need revert
+ * the changes introduced by the overlay
+ */
+ if (slot->overlay_id >= 0) {
+ ret = of_overlay_destroy(slot->overlay_id);
+ if (ret)
+ pr_warn("%s: Error %d destroying overlay %d\n",
+ __func__, ret, slot->overlay_id);
For this and similar messages: isn't there a device you can use with
dev_warn() here? I think a device name would be much better than a
function name.
There is PCI bus referred (struct powernv_php_slot::bus), but it's
not always valid. So I'll add one more field "struct pci_dev *pdev"
which is initialized to the parent PCI device of the slot, then print
those messages with dev_warn().
quoted
+scan:
+ switch (presence) {
+ case POWERNV_PHP_SLOT_PRESENT:
+ if (rescan) {
+ pci_lock_rescan_remove();
+ pcibios_add_pci_devices(slot->bus);
You didn't add this, but "pcibios_add_pci_devices" doesn't seem like the
right name. "pcibios" generally refers to an arch-specific hook that's
called by the generic PCI core. In this case, pcibios_add_pci_devices()
contains powerpc-specific code, and it's only called from powerpc code, so
I think using "pcibios_" in the name is a bit misleading.
Ben already suggested some better names in another reply. I'll pick
it if you agree: pci_add_pci_devices().
quoted
+ /* Remove all devices behind the slot */
+ pci_lock_rescan_remove();
+ pcibios_remove_pci_devices(slot->bus);
Same comment for pcibios_remove_pci_devices(). It would be better if the
name didn't suggest that this was part of the pcibios_ interface between
the PCI core and the arch code, because it's not.
According to Ben's suggestion in another reply, it would be pci_remove_pci_devices()
if you agree :-)
quoted
+ /* Slot indentifier */
s/indentifier/identifier/
Thanks for pointing it out. I'll fix it up in next revision.
quoted
+ if (!php_slot_get_id(dn, &id))
+ return NULL;
+
quoted
+ /* PCI bus */
+ bus = pcibios_find_pci_bus(dn);
And pcibios_find_pci_bus() (it's also powerpc-specific).
I'll pick Ben's suggested name if you agree: of_pci_node_to_bus().
Thanks,
Gavin
Bjorn