Differential D6437
Add an intrng GICv3 ITS driver Authored by andrew on May 18 2016, 4:24 PM. Tags None Referenced Files
Details
Add a new GICv3 ITS driver to handle intrng. As many of the interfaces have changed and to not break the existing driver the driver has been moved to a new file, however much of the code has been moved and been updated from the existing ITS driver. This driver is intended to reduce the interdependence between it and the GICv3. The ITS driver is able to know about the GICv3 driver, but the GICv3 driver needs to pass through intrng to interact with the ITS driver.
Diff Detail
Event TimelineComment Actions I will refrain from making further comments for separate lines or code blocks, I think what we all see here is clear.
Comment Actions I wanted to change many of the internal functions to handle the differences between the old interrupt handling code and intrng, e.g. the its_cmd_ now take a struct gicv3_its_irqsrc * as this fully describes the interrupt. I have also used this a chance to reduce the interdependence between the GICv3 and ITS drivers, e.g. we could build a kernel without ITS support if needed. We should also be able to change the GICv3 driver around if needed as the ITS driver should only know about a few public interfaces.
Comment Actions I get that we have new API, new callbacks to provide, new interrupt description, etc. but this doesn't explain all other modifications that do not change the functionality but rather naming, code location, etc. I can see the differences such as using desc.cmd_desc_movi.id = girq->gi_irq - girq->gi_its_dev->lpis.lpi_base; in movi command instead of i.e. irq = irq - its_dev->lpis.lpi_base; its_cmd_movi(sc, its_dev, col, irq); but that is not something that cannot be applied on-top of the current ITS implementation, don't you think?
Comment Actions I'm intending on pushing the struct gicv3_its_irqsrc * further into the code, but am also trying to touch the common headers as little as possible so some further cleanups will need to wait until we only have a single driver.
Comment Actions Again, I would prefer a patch against existing ITS code not instead of it. This would allow us to track the diff properly and here it's hardly possible. Most of my previous remarks still apply ;-).
Comment Actions Many of the remarks are how I've done things different than the existing driver. This is expected when one of the goals was to reduce the knowledge of the internals of the GICv3 driver. Comment Actions I would say few remarks were to the new code and they were explained. Most of the remarks however are for the existing code modifications that were not explained. - #define ITS_FLAGS_CMDQ_FLUSH (1UL << 0) + #define ITS_FLAGS_CMDQ_FLUSH 0x00000001 There is no sense to make such remarks line by line but I think it is pretty obvious what I'm asking you to explain, isn't it? For example when you get to the point when you would like to add superpages support to pmap I presume you will not create a new file, let's say "p-map.c" with the old code modified to support superpages and containing other modifications such as redefinition of bit masks, macros, variable names and code moved from one function to the upper level function, etc.? I suspect you would add the patch for the existing pmap.c file that will allow it to utilize superpages. That is why I'm curious why the approach here is different. Comment Actions Adding supperpages is different, there is no change in the interface to add support. If the pmap interface was to be significantly changed I would expect to create a new file to handle the change. This is not the case with superpages. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||