Re: [Minios-devel] [PATCH 32/40] arm: parse out the address/size for gicd/gicc

[Huang Shijie <shijie.huang@xxxxxxx>]

On Mon, Nov 06, 2017 at 05:48:35PM +0000, Julien Grall wrote:
> Hi Shijie,
> 
> On 03/11/17 03:12, Huang Shijie wrote:
> > This patch parses out the address/size for gicd/gicc.
> > 
> > Change-Id: I5631afe697ddf21bd92e09e546effd8cbf8a85fe
> > Jira: ENTOS-247
> > Signed-off-by: Huang Shijie <shijie.huang@xxxxxxx>
> > ---
> >   arch/arm/gic.c | 75 
> > ++++++++++++++++++++++++++++++++++++++++++++++------------
> >   1 file changed, 60 insertions(+), 15 deletions(-)
> > 
> > diff --git a/arch/arm/gic.c b/arch/arm/gic.c
> > index 1e37cdc..e72c723 100644
> > --- a/arch/arm/gic.c
> > +++ b/arch/arm/gic.c
> > @@ -177,6 +177,61 @@ static void gic_handler(void) {
> >       gic_eoir(&gic, irq);
> >   }
> > +static unsigned long parse_out_value(const uint32_t *reg, uint32_t size)
> 
> The parse out value should be uint64_t.
I think the unsigned long is better then uint64_t.
We do not return 64bit value for the arm32 platform.

> 
> Also s/size/cell/
> 
> > +{
> > +    if (size == 1)
> > +        return (unsigned long)fdt32_to_cpu(reg[0]);
> > +
> > +    if (size == 2) {
> > +   uint64_t *reg64 = (uint64_t *)reg;
> 
> I don't think you can assume that the value from the Device-Tree will ever
> be 64-bit aligned.
> 
> Furthermore, this is rather ugly to check the size. It would be better to
It is okay. The size only can be 1 or 2.

> have a function reading an arbitrary number of cells and return a 64-bit
> value. This function could be implemented with a loop for instance.
> 
> Lastly, I would really prefer if we have a file containing FDT helpers
> rather than open-coding them in gic.c

could you please give the functions? I searched the FDT, and did not
found the proper function for the parsing.
> 
> > +
> > +   return (unsigned long)fdt64_to_cpu(reg64[0]);
> > +    }
> > +    BUG();
> > +}
> > +
> > +/*
> > + * Parse out the address/size for gicd/gicc.
> > + *
> > + * Return 0 on success; return 1 on error.
> > + */
> > +static int gic_parse(int node, unsigned long *gicd_addr, unsigned long 
> > *gicd_size, > +           unsigned long *gicc_addr, unsigned long 
> > *gicc_size)
> 
> All the unsigned long should be paddr_t.
> 
> > +{
> > +    uint32_t addr_size = 2, cell_size = 1; /* The default, refer to Spec. 
> > */
> 
> The naming is quite confusing. You store the number of cells for the address
> and size. So I would name it addr_cells, size_cells.
> 
> > +    const uint32_t *reg32;
> > +    int pnode;
> > +
> > +    pnode = fdt_parent_offset(device_tree, node);
> > +    if (pnode < 0)
> > +         return 1;
> > +
> > +    reg32 = (uint32_t *)fdt_getprop(device_tree, pnode, "#address-cells", 
> > NULL);
> 
> The cast is not necessary.
okay.
> 
> > +    if (reg32)
> > +         addr_size = fdt32_to_cpu(reg32[0]);
> If you implement an helper to read an arbitrary number of cells as suggested
> above, then you can re-use it here and avoid to open-coding it.
Why not use fdt32_to_cpu() here?

Why I implement an helper for this, while there is already
fdt32_to_cpu()? :(

Do we want to implement all the DT function ourselves?

> 
> > +
> > +    reg32 = (uint32_t *)fdt_getprop(device_tree, pnode, "#size-cells", 
> > NULL);
> 
> Ditto for the cast.
> 
> > +    if (reg32)
> > +         cell_size = fdt32_to_cpu(reg32[0]);
> 
> Ditto for reading the number.
> 
> > +
> > +    if (addr_size > 2 || cell_size > 2) {
> 
> There does not seem to have any sort of coding style in that file... It is
> rather annoying to read it because of that. Wei, Samuel, do you have a
> pointer to Mini-OS coding style?
this is the kernel coding style....
I have used to it.

> 
> > +         printk("Unsupported #address-cells: %d, #size-cells: %d\n",
> > +                addr_size, cell_size);
> > +    return 1;
> > +    }
> > +
> > +    reg32 = fdt_getprop(device_tree, node, "reg", NULL);
> > +    if (reg32) {
> > +         *gicd_addr = parse_out_value(reg32, addr_size);
> > +         *gicd_size = parse_out_value(reg32 + addr_size, cell_size);
> > +         *gicc_addr = parse_out_value(reg32 + addr_size + cell_size, 
> > addr_size);
> > +         *gicc_size = parse_out_value(reg32 + addr_size + cell_size + 
> > addr_size,
> > +                                      cell_size);
> 
> Please make parse_out_value (or a new function) incrementing reg32.
okay.
> 
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >   void gic_init(void) {
> >       gic.gicd_base = NULL;
> >       int node = 0;
> > @@ -188,7 +243,7 @@ void gic_init(void) {
> >               break;
> >           if (fdt_getprop(device_tree, node, "interrupt-controller", NULL)) 
> > {
> > -            int len = 0;
> > +            unsigned long gicd_addr, gicd_size, gicc_addr, gicc_size;
> 
> paddr_t.
okay.

Thanks
Huang Shijie

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/cgi-bin/mailman/listinfo/minios-devel