Re: [Qemu-devel] SH: Improve the interrupt controller

[Vladimir Prus <vladimir@codesourcery.com>]

On Sunday 14 December 2008 19:46:39 takasi-y@ops.dti.ne.jp wrote:
> # a patch attached is not to be commit to repos, but for explanation.
> Hi,
> 
> > I have tested this only with SH4A and it's desirable to test
> > with 7751/R2D. However, I no longer sure I know which kernel
> > to use for that. Can anybody either provide me with instructions,
> > or test this patch with R2D for me?
> 
> It doesn't work at least here for me (for r2d).
> It stops with CPU load 100% (and I had to power my PC off...).

Bummer. Can you send me the kernel/rootfs combo you are using (either
link, or complete binary offlist) and I'll see what I've broken.

> BTW, I think your patch is not following to current sh_intc's design model.
> # please read to the end. this will be turned over later.
> 
> I think its design model is
>  - registering memory regions for each registers.
>  - read/write function doesn't check address
> But, you have add address check in read/write function.

It seems to me that read/write function does check address. Consider:

	static uint32_t sh_intc_read(void *opaque, target_phys_addr_t offset)
	{
	    ...
	    sh_intc_locate(desc, (unsigned long)offset, &valuep, 
			   &enum_ids, &first, &width, &mode);
            ...
	}

	static void sh_intc_locate(struct intc_desc *desc,
			   unsigned long address,
			   unsigned long **datap,
			   intc_enum **enums,
			   unsigned int *first,
			   unsigned int *width,
			   unsigned int *modep)
	{
	    ....
	    if (desc->mask_regs) {
                ...
	    	mode = sh_intc_mode(address, mr->set_reg, mr->clr_reg);
	        if (mode == INTC_MODE_NONE)
                    continue;
                ...
            }
            ...
        }
    }

So, read/write function do check addresses. On the other hand, each register
is registered separately. 

> 
> Mine(which I have forgotten to send..., only to move icr) is attached at the
>  end of this mail. This one is based on the recognition above.
> 
> It works, but I wonder if this model is valid or not on mmio system after #5849.
> Especially here is in question,
> +    cpu_register_physical_memory(P4ADDR(base), 2, io_memory);
> +    cpu_register_physical_memory(A7ADDR(base), 2, io_memory);
> 
> Paul(the author of #5849) said,
> > [1] It's actually the offset from the start of the first page of that region. 
> > In practice this difference doesn't matter, and makes the implementation a 
> > lot simpler.
> 
> I don't know why he thought it "doesn't matter", but from discussions on ML,
>  I guess the model he have is
>  - register one memory region for each modules.
>  - read/write function check address
>  - most of modules are aligned to page.

It is my understanding too. It seems that it's not hard to move sh_intc to this
model -- we just need to register entire region, as opposed to registering
memory for each register.

> In that case, your patch is nearer to the model.
> But, then, we should done with these,
>         cpu_register_physical_memory_offset(P4ADDR(address), 4,
>                                             desc->iomemtype, INTC_A7(address));
>         cpu_register_physical_memory_offset(A7ADDR(address), 4,
>                                             desc->iomemtype, INTC_A7(address));

Yes, both these, and sh_intc_register should not do piecewise registration if
we decide to change model.

- Volodya