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

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi,

On 08/11/17 09:22, Huang Shijie wrote:
> 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.
It is valid to have 64bit value in Arm32 Device-Tree... Arm32 in fact 
supports more than 32-bits PA. It may not be used by Mini-OS but it 
doesn't mean you should not get the interface correct...
> > 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.
Please quote the spec and then we can discuss it...

> > 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.
I didn't say it was existing in the libfdt, but suggesting to create a 
new file in Mini-OS containing all the FDT helpers. You will need the 
same for GICv3 and other driver...
> > > +
> > > +       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?
You didn't get what I mean... If you implement an helper that read a 
number depending on the number of cells given in parameter, you don't 
have to worry how where to use fdt32_to_cpu() or fdt64_to_cpu().
That helper will do it for you. That helper could still use fdt32_to_cpu().

A prototype for the helper would be:

uint64_t fdt_get_number(fdt32_t *prop, unsigned int cells);

> > > +
> > > +    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....
Which kernel are you speaking about??? If I look at x86 code of Mini-OS, 
the { are on a separate line...
The Arm port seems to be a mess regarding the coding style.

> I have used to it.
Cheers,

--
Julien Grall

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