Re: [RFC][PATCH V4] axi: add AXI bus driver

[George Kashperko <george@znau.edu.ua>]

> 2011/4/13 George Kashperko <george@znau.edu.ua>:
> >
> > В Срд, 13/04/2011 в 21:39 +0200, Rafał Miłecki пишет:
> >> 2011/4/13 Greg KH <greg@kroah.com>:
> >> >> diff --git a/drivers/axi/axi_pci_bridge.c b/drivers/axi/axi_pci_bridge.c
> >> >> new file mode 100644
> >> >> index 0000000..17e882c
> >> >> --- /dev/null
> >> >> +++ b/drivers/axi/axi_pci_bridge.c
> >> >> @@ -0,0 +1,33 @@
> >> >> +/*
> >> >> + * AXI PCI bridge module
> >> >> + *
> >> >> + * Licensed under the GNU/GPL. See COPYING for details.
> >> >> + */
> >> >> +
> >> >> +#include "axi_private.h"
> >> >> +
> >> >> +#include <linux/axi/axi.h>
> >> >> +#include <linux/pci.h>
> >> >> +
> >> >> +static DEFINE_PCI_DEVICE_TABLE(axi_pci_bridge_tbl) = {
> >> >> +     { PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, 0x4331) },
> >> >> +     { PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, 0x4353) },
> >> >> +     { PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, 0x4727) },
> >> >> +     { 0, },
> >> >> +};
> >> >> +MODULE_DEVICE_TABLE(pci, axi_pci_bridge_tbl);
> >> >> +
> >> >> +static struct pci_driver axi_pci_bridge_driver = {
> >> >> +     .name = "axi-pci-bridge",
> >> >> +     .id_table = axi_pci_bridge_tbl,
> >> >> +};
> >> >> +
> >> >> +int __init axi_pci_bridge_init(void)
> >> >> +{
> >> >> +     return axi_host_pci_register(&axi_pci_bridge_driver);
> >> >> +}
> >> >> +
> >> >> +void __exit axi_pci_bridge_exit(void)
> >> >> +{
> >> >> +     axi_host_pci_unregister(&axi_pci_bridge_driver);
> >> >> +}
> >> >
> >> > You register a pci driver that does nothing?  That's not right, you need
> >> > to then base your axi bus off of that pci device, so it is hooked up
> >> > correctly in the /sys/devices/ tree.  Otherwise you are somewhere up in
> >> > the virtual location for your axi bus, right?
> >>
> >> Please take a look at:
> >> driver->probe = axi_host_pci_probe;
> >> driver->remove = axi_host_pci_remove;
> >> return pci_register_driver(driver);
> >>
> >>
> >> >> +bool axi_core_is_enabled(struct axi_device *core)
> >> >> +{
> >> >> +     if ((axi_aread32(core, AXI_IOCTL) & (AXI_IOCTL_CLK | AXI_IOCTL_FGC))
> >> >> +         != AXI_IOCTL_CLK)
> >> >> +             return false;
> >> >> +     if (axi_aread32(core, AXI_RESET_CTL) & AXI_RESET_CTL_RESET)
> >> >> +             return false;
> >> >> +     return true;
> >> >> +}
> >> >> +EXPORT_SYMBOL(axi_core_is_enabled);
> >> >
> >> > EXPORT_SYMBOL_GPL()?
> >> >
> >> > What module uses this?  And why would it care?
> >> >
> >> >> +EXPORT_SYMBOL(axi_core_enable);
> >> >
> >> > EXPORT_SYMBOL_GPL()?
> >> >
> >> > Same goes for your other exports, just want you to be sure here.
> >>
> >> Hm, I'm not sure. Using EXPORT_SYMBOL_GPL will forbid closed source
> >> drivers from using our bus driver, right? I'm don't have preferences
> >> on this, if you prefer us to force GPL, I can.
> >>
> >>
> >> >> +u32 xaxi_chipco_gpio_control(struct axi_drv_cc *cc, u32 mask, u32 value)
> >> >> +{
> >> >> +     return axi_cc_write32_masked(cc, AXI_CC_GPIOCTL, mask, value);
> >> >> +}
> >> >> +EXPORT_SYMBOL(xaxi_chipco_gpio_control);
> >> >
> >> > "xaxi"?  Shouldn't that be consistant with the other exports and start
> >> > with "axi"?
> >>
> >> Left from old tests/rewrites/splitting. Thanks.
> >>
> >>
> >> >> +static u8 axi_host_pci_read8(struct axi_device *core, u16 offset)
> >> >> +{
> >> >> +     if (unlikely(core->bus->mapped_core != core))
> >> >
> >> > Are you sure about the use of unlikely in this, and other functions?
> >> > The compiler almost always does a better job than we do for these types
> >> > of calls, just let it do it's job.
> >> >
> >> >> +             axi_host_pci_switch_core(core);
> >> >> +     return ioread8(core->bus->mmio + offset);
> >> >
> >> > I think because of that unlikely, you just slowed down all pci devices,
> >> > right?  That's not very nice :)
> >>
> >> Hm, my logic suggests it is alright, but please consider this once
> >> more with me ;)
> >>
> >> For the most of the time mapped_core (active core) do not change. We
> >> perform few hundreds of operations on one core in a row. This way
> >> mapped_core points to passed core for most of the time. Condition
> >> (mapped_core != core) is unlikely to happen.
> >>
> >> Is there anything wrong in my logic?
> >>
> > Yes, there is. You don't need that "if" at all.
> 
> Damn, WHY do you make me ask why, why, why, all the time?! Can't you
> just write word of explanation without being asked for?
> 
Errm... Sorry, but I've already explained PCIE host behaviour _several_
times several days ago. Personally I like to ask questions. Have
absolutely nothing agains anyone else asking good questions. Never try
to make people ask me questions I know they would ask anyway. Really you
might missed some my messages earlier or maybe my english is too awful ?

Yet again, for PCIE cores (not only for them, for some PCI cores as
well) buscommon, buscore and function core are all available
simultaneously. You dont need window sliding when access them.

Have nice day,
George