On Sun, May 8, 2022 at 9:13 AM Lukas Wunner <lukas@xxxxxxxxx> wrote: > On Thu, May 05, 2022 at 07:54:49PM +0200, Andy Shevchenko wrote: > > On Thu, May 5, 2022 at 4:55 PM Lukas Wunner <lukas@xxxxxxxxx> wrote: > > > On Mon, Jan 31, 2022 at 05:13:39PM +0200, Andy Shevchenko wrote: ... > > > > Background information > > > > ====================== > > > > > > The wealth of information in the commit message obscures what the > > > actual problem is, which is actually quite simple: SoC features > > > such as GPIO are accessed via a reserved MMIO area, we don't know > > > its address but can obtain it from the BAR of the P2SB device, > > > that device is normally hidden so we have to temporarily unhide it. > > > > Right, but this long commit message was a result of the previous > > discussions with Bjorn. If we're ever going to handle something like > > this in the PCI core, perhaps he won't be happy if I remove it. Maybe > > we can simply state what you wrote as a problem statement and move > > this chapter at the end? > > Yes, feel free to copy-paste the synopsis from my e-mail above > and rephrase as you see fit. Will do. ... > > > > On top of that in some cases P2SB is represented by function 0 on PCI > > > > slot (in terms of B:D.F) and according to the PCI specification any > > > > other function can't be seen until function 0 is present and visible. > > > > > > I find that paragraph confusing: Do multi-function P2SB devices exist? > > > What are the other functions? Are they visible but merely not enumerated > > > because function 0 is not visible? > > > > The case I see is when we want to read the BAR from another slot of a > > PCI device, 0 function of which is P2SB. Since P2SB is hidden, the > > other device is hidden as well. Any idea how to reformulate this? And > > yes, we have this in the existing SoCs. > > The spec you linked to in the commit message (for the 100 series chipset) > says that P2SB is located at Device 31 Function 1. > > In those chipsets where P2SB is function 0, what kind of devices are > at functions 1 and higher? In the Intel Broxton and Apollo Lake cases the P2SB is the function 0 and we want to have a BAR of SPI NOR, which is function 2. ... > > > Do you really need all the complicated logic in __pci_bus_read_base()? > > > For comparison, simatic_ipc_get_membase0() in simatic-ipc.c merely does: > > > > > > pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0); > > > > > > If that's sufficient for simatic-ipc.c, why is the more complicated code > > > necessary in p2sb.c? > > > > Since it's a PCI device I want to follow what PCI core does with it. > > As I explained somewhere that the current code (actually it's a > > simplified version of what is done in PCI core) follows what spec > > requires. I would like to be in alignment with the spec, while it > > still may work with less code. Besides that, it's theoretically > > possible that the base address may be 64-bit in new SoCs, I won't > > rewrite code again just because we abused the spec. > > So as an alternative to copy-pasting __pci_bus_read_base(), > you could just call pci_scan_single_device(). This will create > a proper pci_dev that you can work with. Note that no driver will > be bound to the device because of: > > pci_scan_single_device() > pci_device_add() > dev->match_driver = false > > After you've read the BAR, get rid of the pci_dev with pci_destroy_dev(). This is pretty nice, if it flies! I definitely try this ASAP (during working hours). Thanks for the hint. ... > > > > + /* > > > > + * I don't know how l can have all bits set. Copied from old code. > > > > + * Maybe it fixes a bug on some ancient platform. > > > > + */ > > > > + if (PCI_POSSIBLE_ERROR(l)) > > > > + l = 0; > > > > > > l can have all bits set if the device was hot-removed. That can't happen > > > with a built-in device such as P2SB. > > > > Can be dropped, indeed. But that chicken bit emulates that :-) Anyway, > > we unhide the device before looking into it, so we shouldn't have the > > surprise "removals". > > pci_lock_rescan_remove() prevents concurrent unhiding as well as > removal via sysfs. Yep, that's good. In any case this piece of code will be gone if your above suggestion works, have I got it right? -- With Best Regards, Andy Shevchenko
