From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by gabe.freedesktop.org (Postfix) with ESMTPS id 3876410E66F for ; Tue, 10 Jan 2023 21:14:51 +0000 (UTC) Date: Tue, 10 Jan 2023 13:14:50 -0800 Message-ID: <87h6wyay7p.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: "Gupta, Anshuman" In-Reply-To: References: <20230109120205.2259410-1-anshuman.gupta@intel.com> <20230109120205.2259410-3-anshuman.gupta@intel.com> <9e10dbb2-eaf3-6bbe-7461-b34fe818fbd9@intel.com> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Subject: Re: [igt-dev] [PATCH i-g-t 2/2] tests/device_reset: Add warm_reset test List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "igt-dev@lists.freedesktop.org" , "Nilawar, Badal" , "Vivi, Rodrigo" Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: On Tue, 10 Jan 2023 06:14:37 -0800, Gupta, Anshuman wrote: > > On 1/9/2023 5:32 PM, Anshuman Gupta wrote: > > > +bool igt_pci_bridge_warm_reset(struct pci_device *dev) { > > > + uint8_t header_type; > > > + uint16_t bridge_ctl; > > > + > > > + if (pci_device_cfg_read_u8(dev, &header_type, PCI_HEADER_TYPE)) > > > + return false; > > > + > > > + igt_assert_f((header_type & 0x7f) == 1, "PCI device is not a > > > +Bridge\n"); > > > + > > > + if (pci_device_cfg_read_u16(dev, &bridge_ctl, > > PCI_TYPE_1_HEADER_BRIDGE_CTL)) > > > + return false; > > > + > > > + bridge_ctl |= PCI_TYPE_1_SEC_BUS_RESET; > > > + > > > + if (pci_device_cfg_write_u16(dev, bridge_ctl, > > PCI_TYPE_1_HEADER_BRIDGE_CTL)) > > > + return false; > > > + > > > > > > The pci code adds a minimum reset duration (Trst) delay after setting the bit. > > Do we need to add the same here? > > https://elixir.bootlin.com/linux/latest/source/drivers/pci/pci.c#L5045 > Thanks for review. > Will add delay of 2ms to keep parity with the kernel. > > > > Is restoring sequence not necessary? > I think driver has the responsibility of restoring. > I am not sure about if IGT needs to restore it. > The test itself has sequence of unbind and bind, so driver should restore it. Hmm, not too sure about this stuff but looks like we should follow the entire sequence in the kernel function pci_reset_secondary_bus()? Thanks Riana for pointing out. > > > @@ -417,6 +421,7 @@ igt_main > > > set_device_filter(dev_path); > > > > > > igt_skip_on(!is_sysfs_reset_supported(dev.fds.dev)); > > > + dev.root = igt_device_get_pci_root_port(dev.fds.dev); Also is SBR done on the root port or the upstream bridge, in case there is a bridge in the middle (between the root port and the device/bus)? In which case this should be pci_device_get_parent_bridge()? Thanks. -- Ashutosh