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

[Huang Shijie <shijie.huang@xxxxxxx>]

On Wed, Nov 08, 2017 at 10:56:50AM +0000, Julien Grall wrote:
> 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...
okay...
> 
> > 
> > > 
> > > 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...
I check it again, I am wrong, it could be 3 sometime..

> 
> > 
> > > 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...
okay, in this way, we do _not_ need FDT module at all.
I will drop the FDT module in next version.

Thanks
> 
> > > 
> > > > +
> > > > +       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);
okay.
> 
> > 
> > > 
> > > > +
> > > > +    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...
Please see the code in arch/x86/mm.c, line 756...

Thanks
Huang Shijie

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