| Message ID | 1419967718-26909-4-git-send-email-robherring2@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tuesday 30 December 2014 13:28:32 Rob Herring wrote: > From: Rob Herring <robh@kernel.org> > > Disable the Versatile PCI DT node when no PCI backplane is detected. This > will prevent the Versatile PCI driver from probing when PCI is not > populated. Can you explain why the check is done in platform code instead of the PCI driver probe function itself? > + /* Check if PCI backplane is detected */ > + val = __raw_readl(base + VERSATILE_SYS_PCICTL_OFFSET); > + if (val & 1) > + goto err; Using __raw_readl prevents this from working with big-endian kernels, so just use readl here. Arnd
On Tue, Dec 30, 2014 at 3:37 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 30 December 2014 13:28:32 Rob Herring wrote: >> From: Rob Herring <robh@kernel.org> >> >> Disable the Versatile PCI DT node when no PCI backplane is detected. This >> will prevent the Versatile PCI driver from probing when PCI is not >> populated. > > Can you explain why the check is done in platform code instead of the PCI > driver probe function itself? Because the register to check is not part of the PCI controller, but part of the system registers. There was no infrastructure for the system registers when I originally wrote this, but there's some syscon support now for VExpress that may be able to be used. I think it is cleaner if we can avoid syscon dependencies in drivers in general, but it is pretty much zero chance the Versatile PCI driver will ever be used on another platform. >> + /* Check if PCI backplane is detected */ >> + val = __raw_readl(base + VERSATILE_SYS_PCICTL_OFFSET); >> + if (val & 1) >> + goto err; > > Using __raw_readl prevents this from working with big-endian kernels, so just > use readl here. So does the fact that this is an ARM926, but yes there's not really any reason not to use readl. Rob
On Tuesday 30 December 2014 17:05:16 Rob Herring wrote: > On Tue, Dec 30, 2014 at 3:37 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tuesday 30 December 2014 13:28:32 Rob Herring wrote: > >> From: Rob Herring <robh@kernel.org> > >> > >> Disable the Versatile PCI DT node when no PCI backplane is detected. This > >> will prevent the Versatile PCI driver from probing when PCI is not > >> populated. > > > > Can you explain why the check is done in platform code instead of the PCI > > driver probe function itself? > > Because the register to check is not part of the PCI controller, but > part of the system registers. There was no infrastructure for the > system registers when I originally wrote this, but there's some syscon > support now for VExpress that may be able to be used. I think it is > cleaner if we can avoid syscon dependencies in drivers in general, but > it is pretty much zero chance the Versatile PCI driver will ever be > used on another platform. Ok, I see. I guess ideally we would get a correct DTB file from the boot loader, but that is of course completely unrealistic on versatile, both on hardware and qemu. Maybe we can just take your current approach for now, and add a comment about the possibility of using syscon in the future to the function. I'm also not completely sure if your code gets it right: the existing driver writes '1' into this register in .preinit() and then reads the same register back in .setup(). Your driver just reads the value but never writes it. I suspect that either the existing behavior is completely bogus because the driver always reads what it write before, and then you can skip the logic completely, or your new driver is broken because reading the register without writing it first will result in undefined behavior on real hardware. > >> + /* Check if PCI backplane is detected */ > >> + val = __raw_readl(base + VERSATILE_SYS_PCICTL_OFFSET); > >> + if (val & 1) > >> + goto err; > > > > Using __raw_readl prevents this from working with big-endian kernels, so just > > use readl here. > > So does the fact that this is an ARM926, but yes there's not really > any reason not to use readl. Ok. I don't think there is anything preventing you from setting big-endian mode on arm926 though, it's just that nobody ever has the need in practice, and a lot of drivers are broken. Arnd
On 31 December 2014 at 15:23, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 30 December 2014 17:05:16 Rob Herring wrote: >> Because the register to check is not part of the PCI controller, but >> part of the system registers. There was no infrastructure for the >> system registers when I originally wrote this, but there's some syscon >> support now for VExpress that may be able to be used. I think it is >> cleaner if we can avoid syscon dependencies in drivers in general, but >> it is pretty much zero chance the Versatile PCI driver will ever be >> used on another platform. > > Ok, I see. I guess ideally we would get a correct DTB file from the boot > loader, but that is of course completely unrealistic on versatile, both > on hardware and qemu. Why would you want to get the bootloader to do this for you when the hardware provides a perfectly good mechanism for probing for the existence of the hardware? "Ask the hardware" is always going to be a more reliable and foolproof way to get the answer if you can do it -- device tree should just be for the cases where there is no way to probe. In any case the only way for the boot loader to determine the correct value to put into the device tree would be to read the register itself. > I suspect that either the existing behavior is completely bogus because > the driver always reads what it write before, and then you can skip > the logic completely, or your new driver is broken because reading > the register without writing it first will result in undefined behavior > on real hardware. The documentation describes this register as: # The SYS_PCICTL register at 0x10000044 enables the bridge to the PCI bus: # # * Setting bit 0 HIGH enables PCI bus accesses. # * Read returns a HIGH in bit 0 if a PCI board is present # in the expansion backplane. ie the read and write behaviour is independent. So I suspect that this new driver's failure to write 1 means that it will break PCI on real hardware. (QEMU doesn't attempt to model this behaviour: SYS_PCICTL will always read-as-one, writes-ignored.) thanks -- PMM
On Wed, Dec 31, 2014 at 10:13 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 31 December 2014 at 15:23, Arnd Bergmann <arnd@arndb.de> wrote: >> On Tuesday 30 December 2014 17:05:16 Rob Herring wrote: >>> Because the register to check is not part of the PCI controller, but >>> part of the system registers. There was no infrastructure for the >>> system registers when I originally wrote this, but there's some syscon >>> support now for VExpress that may be able to be used. I think it is >>> cleaner if we can avoid syscon dependencies in drivers in general, but >>> it is pretty much zero chance the Versatile PCI driver will ever be >>> used on another platform. >> >> Ok, I see. I guess ideally we would get a correct DTB file from the boot >> loader, but that is of course completely unrealistic on versatile, both >> on hardware and qemu. > > Why would you want to get the bootloader to do this for you when > the hardware provides a perfectly good mechanism for probing > for the existence of the hardware? "Ask the hardware" is always > going to be a more reliable and foolproof way to get the answer > if you can do it -- device tree should just be for the cases where > there is no way to probe. In any case the only way for the boot > loader to determine the correct value to put into the device tree > would be to read the register itself. Only that it is nice to hide all the ugly bits in the bootloader/firmware and DT already provides a standard way to enable or disable h/w. This h/w is not probeable in any sort of standard way. >> I suspect that either the existing behavior is completely bogus because >> the driver always reads what it write before, and then you can skip >> the logic completely, or your new driver is broken because reading >> the register without writing it first will result in undefined behavior >> on real hardware. > > The documentation describes this register as: > > # The SYS_PCICTL register at 0x10000044 enables the bridge to the PCI bus: > # > # * Setting bit 0 HIGH enables PCI bus accesses. > # * Read returns a HIGH in bit 0 if a PCI board is present > # in the expansion backplane. > > ie the read and write behaviour is independent. So I suspect > that this new driver's failure to write 1 means that it > will break PCI on real hardware. (QEMU doesn't attempt to > model this behaviour: SYS_PCICTL will always read-as-one, > writes-ignored.) From Table 4.4, I came to the conclusion that the write wasn't really necessary: SYS_PCICTL 0x10000044 Read only -Read returns a HIGH in bit [0] if a PCI board is present in the expansion backplane. I'll add it back. Rob
On 31 December 2014 at 19:22, Rob Herring <robherring2@gmail.com> wrote: > On Wed, Dec 31, 2014 at 10:13 AM, Peter Maydell > <peter.maydell@linaro.org> wrote: >> Why would you want to get the bootloader to do this for you when >> the hardware provides a perfectly good mechanism for probing >> for the existence of the hardware? "Ask the hardware" is always >> going to be a more reliable and foolproof way to get the answer >> if you can do it -- device tree should just be for the cases where >> there is no way to probe. In any case the only way for the boot >> loader to determine the correct value to put into the device tree >> would be to read the register itself. > > Only that it is nice to hide all the ugly bits in the > bootloader/firmware and DT already provides a standard way to enable > or disable h/w. This h/w is not probeable in any sort of standard way. This is an embedded system, you were expecting some kind of standard? :-) I believe that SYS_PCICTL tells you whether you're going to find the PCI controller or a decode error at the addresses in the memory map where the PCI controller usually lives, so robustness suggests it's worth retaining the check in the kernel. [I'd want to check this on hardware but my current belief is that if there's no backplane attached then the register will read zero and the PCI controller register space will abort with decode errors.] > From Table 4.4, I came to the conclusion that the write wasn't really necessary: > > SYS_PCICTL 0x10000044 Read only -Read returns a HIGH in bit [0] if > a PCI board is present in the expansion backplane. > > I'll add it back. The self-contradictory VersatilePB docs strike again, I see. Interestingly, in the PB1176 docs this register is documented as definitely read/only: # [1]Read only: Reserved. # # Note # # On the RealView Platform Baseboard for ARM926EJ-S, this bit # was PCICONTROLIN signal and indicated that 66MHz mode was # enabled. The PB1176JZF-S only supports 33MHz operation. # # [0]Read only: Status of P_DETECT pin. ...and in passing provides some info about the 926 board that isn't in the docs for the 926! A little digging suggests that it may in fact be the case that the 1176 docs are correct and the write to bit 0 does nothing. However the safest thing here is to keep doing what the old kernel did, I think. thanks -- PMM
On Wednesday 31 December 2014 21:07:25 Peter Maydell wrote: > On 31 December 2014 at 19:22, Rob Herring <robherring2@gmail.com> wrote: > > On Wed, Dec 31, 2014 at 10:13 AM, Peter Maydell > > <peter.maydell@linaro.org> wrote: > >> Why would you want to get the bootloader to do this for you when > >> the hardware provides a perfectly good mechanism for probing > >> for the existence of the hardware? "Ask the hardware" is always > >> going to be a more reliable and foolproof way to get the answer > >> if you can do it -- device tree should just be for the cases where > >> there is no way to probe. In any case the only way for the boot > >> loader to determine the correct value to put into the device tree > >> would be to read the register itself. > > > > Only that it is nice to hide all the ugly bits in the > > bootloader/firmware and DT already provides a standard way to enable > > or disable h/w. This h/w is not probeable in any sort of standard way. > > This is an embedded system, you were expecting some kind of > standard? But that is exactly the point: While we try to ask the hardware about anything that can be reasonably queried through an in-kernel interface that talks to the hardware, it works less well when that hardware interface is rather obscure and (like this one) only used on a few special machines. Similar examples are Marvell's SoC ID register, which you have to read by powering on the PCI host bridge and reading the Vendor/Device ID registers for the root bus, or the 'nvram' on Broadcom's wifi router chips that is in reality a partition on either NOR or NAND flash. Accessing those at early boot from a random device driver is a real pain, and it would be much nicer to have the boot loader read them and put the data into the DT for us if we had the option. Unfortunately we have to deal with legacy boot loaders that don't change the DTB. > > From Table 4.4, I came to the conclusion that the write wasn't really necessary: > > > > SYS_PCICTL 0x10000044 Read only -Read returns a HIGH in bit [0] if > > a PCI board is present in the expansion backplane. > > > > I'll add it back. > > The self-contradictory VersatilePB docs strike again, I see. > Interestingly, in the PB1176 docs this register is documented > as definitely read/only: > > # [1]Read only: Reserved. > # > # Note > # > # On the RealView Platform Baseboard for ARM926EJ-S, this bit > # was PCICONTROLIN signal and indicated that 66MHz mode was > # enabled. The PB1176JZF-S only supports 33MHz operation. > # > # [0]Read only: Status of P_DETECT pin. > > ...and in passing provides some info about the 926 board that > isn't in the docs for the 926! > > A little digging suggests that it may in fact be the case > that the 1176 docs are correct and the write to bit 0 does > nothing. However the safest thing here is to keep doing > what the old kernel did, I think. Agreed. This also means that we need a way to access that register anyway, and any discussion about finding a way to move the read access for probing the device elsewhere is moot. A syscon device node would probably be best, but the current approach also works. Arnd
On 1 January 2015 at 15:35, Arnd Bergmann <arnd@arndb.de> wrote: > But that is exactly the point: While we try to ask the hardware about > anything that can be reasonably queried through an in-kernel interface > that talks to the hardware, it works less well when that hardware interface > is rather obscure and (like this one) only used on a few special machines. "We put this random collection of status bits in to a register somewhere because it was convenient for the h/w engineers" is pretty common, I think. > Similar examples are Marvell's SoC ID register, which you have to read by > powering on the PCI host bridge and reading the Vendor/Device ID registers > for the root bus, or the 'nvram' on Broadcom's wifi router chips that is > in reality a partition on either NOR or NAND flash. Accessing those at > early boot from a random device driver is a real pain, and it would be > much nicer to have the boot loader read them and put the data into the > DT for us if we had the option. Unfortunately we have to deal with legacy > boot loaders that don't change the DTB. I don't think it's going to be any easier for the boot loader, which is quite possibly totally uninterested in PCI and would have no reason to need to prod this location on the versatilepb. I also trust the kernel much more to get bugfixes and be easily updatable. I appreciate that it's more work for you if you can't just say "punt this nastiness to the bootloader", but I think it's also the better (more robust and self-contained) design. -- PMM
On Tue, Dec 30, 2014 at 8:28 PM, Rob Herring <robherring2@gmail.com> wrote: > From: Rob Herring <robh@kernel.org> > > Disable the Versatile PCI DT node when no PCI backplane is detected. This > will prevent the Versatile PCI driver from probing when PCI is not > populated. > > Signed-off-by: Rob Herring <robh@kernel.org> > Cc: Russell King <linux@arm.linux.org.uk> > Cc: Linus Walleij <linus.walleij@linaro.org> (...) > + /* Check if PCI backplane is detected */ > + val = __raw_readl(base + VERSATILE_SYS_PCICTL_OFFSET); I think this kind of random syscon register access should be handled using the mfd/syscon.c hub and looked up in some way to be used by the driver. I'm thinking along the pattern of adding code in drivers/* by the pattern of e.g. drivers/video/fbdev/amba-clcd-versatile.c i.e. an add-on that gets compiled-in only for those platform but still married to the main driver. Yours, Linus Walleij
On Thu, Jan 8, 2015 at 1:37 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Dec 30, 2014 at 8:28 PM, Rob Herring <robherring2@gmail.com> wrote: > >> From: Rob Herring <robh@kernel.org> >> >> Disable the Versatile PCI DT node when no PCI backplane is detected. This >> will prevent the Versatile PCI driver from probing when PCI is not >> populated. >> >> Signed-off-by: Rob Herring <robh@kernel.org> >> Cc: Russell King <linux@arm.linux.org.uk> >> Cc: Linus Walleij <linus.walleij@linaro.org> > > (...) >> + /* Check if PCI backplane is detected */ >> + val = __raw_readl(base + VERSATILE_SYS_PCICTL_OFFSET); > > I think this kind of random syscon register access should be handled > using the mfd/syscon.c hub and looked up in some way to be used > by the driver. Perhaps, but I've not figured out how much of VExpress syscon can be used for Versatile. I looked at it briefly, but it appeared to be more work that I have time for ATM. We still have other accesses to system registers in Versatile as well. So if we want to merge all this anytime soon, the options are: - this patch - Create a versatilepb-pci.dts which includes versatilepb.dts and enables PCI - Default to PCI enabled (for QEMU) in the dts and users have to disable in their dts if they don't have a PCI backplane. > I'm thinking along the pattern of adding code in drivers/* > by the pattern of e.g. > drivers/video/fbdev/amba-clcd-versatile.c > i.e. an add-on that gets compiled-in only for those platform > but still married to the main driver. One difference here is the PCI driver is pretty much only ever going to be for a single platform. Rob
diff --git a/arch/arm/mach-versatile/versatile_dt.c b/arch/arm/mach-versatile/versatile_dt.c index 9f9bc61..5053810 100644 --- a/arch/arm/mach-versatile/versatile_dt.c +++ b/arch/arm/mach-versatile/versatile_dt.c @@ -22,15 +22,61 @@ */ #include <linux/init.h> +#include <linux/io.h> +#include <linux/of.h> +#include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/of_platform.h> +#include <linux/slab.h> #include <asm/mach-types.h> #include <asm/mach/arch.h> #include "core.h" +#define VERSATILE_SYS_PCICTL_OFFSET 0x44 + +static void __init versatile_dt_pci_init(void) +{ + u32 val; + void __iomem *base; + struct device_node *np; + struct property *newprop; + + np = of_find_compatible_node(NULL, NULL, "arm,core-module-versatile"); + if (!np) + return; + + base = of_iomap(np, 0); + if (!base) + return; + + /* Check if PCI backplane is detected */ + val = __raw_readl(base + VERSATILE_SYS_PCICTL_OFFSET); + if (val & 1) + goto err; + + np = of_find_compatible_node(NULL, NULL, "arm,versatile-pci"); + if (!np) + goto err; + + newprop = kzalloc(sizeof(*newprop), GFP_KERNEL); + if (!newprop) + goto err; + + newprop->name = kstrdup("status", GFP_KERNEL); + newprop->value = kstrdup("disabled", GFP_KERNEL); + newprop->length = sizeof("disabled"); + of_update_property(np, newprop); + + pr_info("Not plugged into PCI backplane!\n"); +err: + iounmap(base); +} + static void __init versatile_dt_init(void) { + versatile_dt_pci_init(); + of_platform_populate(NULL, of_default_bus_match_table, versatile_auxdata_lookup, NULL); }