[Cbe-oss-dev] [patch 05/16] BEI specific part of the Axon driver
Arnd Bergmann
arnd at arndb.de
Thu May 31 23:52:28 EST 2007
On Thursday 24 May 2007, jdubois at mc.com wrote:
> +static dma_addr_t addr_xltr_bei_from_plb(addr_xltr_t * self,
> + plb_addr_t plb_bus_addr)
> +{
> + axon_priv_t *p_axon_priv = (axon_priv_t *) self->context;
> +
> +
> +
> + if ((plb_bus_addr & AXON_PLB_XDR_CHAN0_MASK) ==
> + AXON_PLB_XDR_CHAN0_OFFSET)
> + return (dma_addr_t) (plb_bus_addr & ~AXON_PLB_XDR_CHAN0_MASK);
> +
> + if (plb_bus_addr < 0xE000000000000000)
> + return (dma_addr_t) ((plb_bus_addr & AXON_PLB_MASK_FROM_BE)
> + | p_axon_priv->base_addr);
> +
> + return (dma_addr_t) 0xffffffffffffffff;
> +}
I can see how you need functions like this one the PCI interface.
But I think that on BEI, this should all be handled by
of_translate_address, and similar functions.
Am I missing something?
> +#define IIC_IRQ_NODE_SHIFT 8
> +#define IIC_IRQ_CLASS_SHIFT 4
and these should be handled by irq_of_parse_and_map()
> +static struct device *axon_get_device(axon_t * p_axon)
> +{
> + axon_priv_t *p_axon_priv = p_axon->context;
> +
> + return p_axon_priv->dev;
> +}
When you call a function *_get_*, it's normally implied to
do reference counting and have a matching put function,
see get_device()/put_device().
> +static irq_hw_number_t axon_irq_map[NR_IRQS];
> +
> +static irq_hw_number_t axon_virq_to_hw(int virq_nr)
> +{
> + return axon_irq_map[virq_nr];
> +}
> +
> +static irqreturn_t axon_interrupt_handler(int virq_nr, void *dev_id)
> +{
> + irqreturn_t ret = IRQ_NONE;
> +
> + irq_hw_number_t irq_nr = axon_virq_to_hw(virq_nr);
> +
> + axon_priv_t *p_axon_priv = dev_id;
> +
> + dbg_log("virq = %d, irq=%d\n", (int)virq_nr, (int)irq_nr);
> +
> + if (irq_nr == 0) {
> + dbg_inf("bad virt to hard interrupt mapping\n");
> + return ret;
> + }
> +
> + irq_nr -= p_axon_priv->node_id * 256;
> +
> + if (irq_nr <= 0) {
> + dbg_inf("bad node irq\n");
> + return ret;
> + }
> +
> + dbg_log("node irq = %d\n", (int)irq_nr);
> +
> + if ((irq_nr < AXON_BEI_INTR_COUNT)
> + && (p_axon_priv->irq_handlers[irq_nr].handler != NULL)) {
> +
> + p_axon_priv->irq_handlers[irq_nr].handler(p_axon_priv->
> + irq_handlers
> + [irq_nr].context,
> + irq_nr, dev_id);
> +
> + ret = IRQ_HANDLED;
> +
> + } else {
> + dbg_err
> + ("Unsupported interrupt has been generated by the Axon %d \n",
> + irq_nr);
> + }
> +
> + return ret;
> +}
Can you describe what you want to achieve here? It looks to me like this
is either an entirely pointless abstraction, or that you might want to
implement a nested interrupt controller instead.
> +static int axon_get_node_mbx_direct_interrupt(axon_t * p_axon)
> +{
> + axon_priv_t *p_axon_priv = p_axon->context;
> +
> + return 16 + (p_axon_priv->ioif ? 0xb : 0) +
> + (p_axon_priv->node_id * 256);
> +}
As mentioned in the review for the direct mailbox interrupt, this
should not require special treatment, if only the firmware does
the right thing.
> +static int axon_register_irq_handler(axon_t * p_axon, int irq_nr,
> + axon_irq_handler_t handler,
> + char *name, void *data)
> +{
> + int ret = 0;
> +
> + axon_priv_t *p_axon_priv = p_axon->context;
> +
> + irq_nr = axon_get_hard_interrupt_number(p_axon, irq_nr);
> +
> + if (irq_nr < AXON_BEI_INTR_COUNT) {
> + if (p_axon_priv->irq_handlers[irq_nr].handler == NULL) {
> +
> + int virq_nr;
> +
> + if (irq_nr == 136) {
> +
> + virq_nr =
> + irq_find_mapping(NULL,
> + axon_get_node_mbx_direct_interrupt
> + (p_axon));
> + } else {
> + virq_nr =
> + irq_find_mapping(p_axon_priv->irq_host,
> + irq_nr);
> + }
> +
> + axon_irq_map[virq_nr] =
> + axon_get_node_interrupt_number(p_axon, irq_nr);
> +
> + p_axon_priv->irq_handlers[irq_nr].handler = handler;
> + p_axon_priv->irq_handlers[irq_nr].virq_nr = virq_nr;
> + p_axon_priv->irq_handlers[irq_nr].irq_nr = irq_nr;
> + p_axon_priv->irq_handlers[irq_nr].context = data;
> +
> + ret =
> + request_irq(virq_nr, axon_interrupt_handler,
> + SA_INTERRUPT, name, p_axon_priv);
> + dbg_inf
> + ("request_irq for irq %d, ioif %d, node %d [=%d], virq=%d returned %d\n",
> + irq_nr, p_axon_priv->ioif,
> + p_axon_priv->node_id,
> + (int)axon_irq_map[virq_nr], virq_nr, ret);
> + } else {
> + dbg_err
> + ("Trying to register and irq handler for an already managed handler\n");
> + ret = -EINVAL;
> + }
> + } else {
> + dbg_err
> + ("Trying to register and irq handler for an invalid interrupt level\n");
> + ret = -EINVAL;
> + }
> +
> + return ret;
> +}
same as above, this seems pointless, but I may have missed something.
> +static int axon_map_regs(axon_priv_t * p_axon_priv, addr_xltr_t * p_addr_xltr)
> +{
> + int ret = 0;
> +
> + dma_addr_t regs_slave_addr;
> + dma_addr_t utl_regs_addr;
> + dma_addr_t cfg_regs_addr;
> + dma_addr_t dcr_regs_addr;
> +
> +
> + regs_slave_addr =
> + p_addr_xltr->from_plb(p_addr_xltr, AXON_PLB_REGS_SLV_OFFSET);
> + utl_regs_addr =
> + p_addr_xltr->from_plb(p_addr_xltr, AXON_PLB_UTL_REGS_OFFSET);
> + cfg_regs_addr =
> + p_addr_xltr->from_plb(p_addr_xltr, AXON_PLB_CFG_REGS_OFFSET);
> + dcr_regs_addr =
> + p_addr_xltr->from_plb(p_addr_xltr, AXON_PLB_DCR_REGS_OFFSET);
Those register areas really need to come from the device tree, don't
duplicate that knowledge in this driver!
> + p_axon_priv->p_cfg_base_addr = ioremap_nocache(regs_slave_addr,
> + AXON_PLB_REGS_SLV_SIZE);
Just use plain ioremap(), ioremap_nocache() is a deprecated alias for it,
as all ioremap is uncached by definition.
> + p_axon_priv->p_pcie_utl_regs = ioremap_nocache(utl_regs_addr,
> + AXON_PLB_UTL_REGS_SIZE);
> +
> + if (p_axon_priv->p_pcie_utl_regs == NULL) {
> + dbg_err("Unable to UTL Configuration register \n");
> + ret = -EINVAL;
> + goto err_map_pcie_utl;
> + }
> + dbg_log("UTL_REGS is mapped in 0x%p\n", p_axon_priv->p_pcie_utl_regs);
UTL should be a nested interrupt controller driver. It is a very broken
hardware design, but implementing the interrupt controller driver means
that you can encode the proper interrupt numbers in the device tree.
> + phys_addr = of_translate_address(of_dev->node, reg);
> +
> + if (phys_addr == OF_BAD_ADDR) {
> + dbg_err("Unable to find Axon reg property\n");
> + return -ENODEV;
> + }
Use of_iomap() here to directly translate and map the property.
> + node_id = (u32) ((*(u64 *) reg) >> 41) ;
What do you need the node_id for? it should be transparent to drivers.
> + p_axon_priv->irq_host =
> + irq_find_host(of_find_node_by_name
> + (of_dev->node, "interrupt-controller"));
should not be needed if you use the high-level irq_of_parse_and_map
> + p_axon_priv->axon.prepare_soft_reset = axon_prepare_soft_reset;
> + p_axon_priv->axon.finish_soft_reset = axon_finish_soft_reset;
> + p_axon_priv->axon.register_irq_handler = axon_register_irq_handler;
> + p_axon_priv->axon.unregister_irq_handler = axon_unregister_irq_handler;
> + p_axon_priv->axon.get_device = axon_get_device;
> + p_axon_priv->axon.sms_get = axon_sms_get;
> + p_axon_priv->axon.mbox_get = axon_mbox_get;
> + p_axon_priv->axon.dmax_get = axon_dmax_get;
> + p_axon_priv->axon.pio_get = axon_pio_get;
> + p_axon_priv->axon.addr_xltr_get = axon_addr_xltr_get;
> + p_axon_priv->axon.regs_mbox_get = axon_regs_mbox_get;
> + p_axon_priv->axon.regs_utl_get = axon_regs_utl_get;
> + p_axon_priv->axon.regs_dmax_get = axon_regs_dmax_get;
> + p_axon_priv->axon.regs_dma_aux_get = axon_regs_dma_aux_get;
> + p_axon_priv->axon.regs_c3po_get = axon_regs_c3po_get;
> + p_axon_priv->axon.regs_dcr_get = axon_regs_dcr_get;
> + p_axon_priv->axon.peer_mbox_get = _axon_peer_mbox_get;
> + p_axon_priv->axon.disable_irq = axon_disable_irq;
> + p_axon_priv->axon.enable_irq = axon_enable_irq;
I find it somewhat cleaner to assign the function pointers statically, like
static const struct axon_operations axon_bei_operations = {
.prepare_soft_reset = axon_prepare_soft_reset,
.finish_soft_reset = axon_finish_soft_reset,
...
};
> + dbg_inf("Creating BIE address translator \n");
> + ret = axon_addr_xltr_bei_create(p_axon_priv, &p_axon_priv->p_addr_xltr);
> + if (ret != 0) {
> + dbg_err("Unable to create address translator for BEI side\n");
> + goto err_create_xltr;
> + }
> +
> + dbg_inf("Mapping configuration registers\n");
> + ret = axon_map_regs(p_axon_priv, p_axon_priv->p_addr_xltr);
> + if (ret != 0) {
> + dbg_err
> + ("Unable to map configuration registers for "
> + PCI_DEVICE_BOARD_NAME "\n");
> + goto err_map_regs;
> + }
> +
> + ret = axon_pim_create(&p_axon_priv->axon, NULL, NULL, NULL);
> + if (ret != 0) {
> + dbg_err("Unable to create PIM object\n");
> + goto err_create_mbox_self;
> + }
> +
> + dbg_inf("Creating hardware mailbox\n");
> + ret = axon_mbox_hw_create(&p_axon_priv->axon,
> + p_axon_priv->p_addr_xltr,
> + 15,
> + p_axon_priv->p_addr_xltr->
> + from_plb(p_axon_priv->
> + p_addr_xltr,
> + AXON_PLB_MBOX_MESSAGE_ADDRESS),
> + &p_axon_priv->axon_mbox_self);
> + if (ret != 0) {
> + dbg_err("Unable to create Mbox object\n");
> + goto err_create_mbox_self;
> + }
Each of those create() calls should really boil down to a
call to the generic device_create(), so that the devices
are visible to the Linux driver subsystem, and you can autoload
the modules.
In general, I'd say you have two options how to do it:
1. bind the axon bei driver to each of the subdevices of the axon
node that you want to use, like you already do here, and then
create _another_ device with a different bus_type (e.g. using
sysdev_register) that you can bind to in the specific (mbx,
dma, ...) driver, independent of whether you are on PCI or
on BEI. This will be somewhat confusing in the device tree,
as you duplicate every device (you already do, you just hide the
ones you really use), but it means simpler specific drivers.
2. Make the axon BEI a really flat layer, to the point where
it basically goes away, and only provide the child devices
in case of PCI. Then in each specific driver file, register both
the of_platform_driver, and the system_driver, with separate
probe() functions, but everything else common. Of course,
the of_platform_driver can only be registered on powerpc,
so you need a little bit of #ifdef there.
I'd prefer the second option.
> + if (strcmp(match->name, "c3po") == 0) {
> + int i;
> +
> + struct device_node *pdn;
> +
> + unsigned long *reg =
> + (unsigned long *)get_property(of_dev->node, "reg",
> + NULL);
> +
> + unsigned long c3po_base =
> + (unsigned long)of_translate_address(of_dev->node,
> + (u32 *) reg);
This can be better expressed using of_iomap().
> + ptr = ioremap(c3po_base + 0x0828L, sizeof(u32) * 4);
then the 0x828 becomes an offset that you use when accessing the register
instead.
Arnd <><
More information about the cbe-oss-dev
mailing list