Re: [PATCH v4 01/10] ARM: sun9i: smp: Add sun9i dt parsing function

[Mylène Josserand]

Hi,

On Fri, 23 Feb 2018 15:54:23 +0100
Maxime Ripard  wrote:

> Hi,
> 
> On Fri, Feb 23, 2018 at 02:37:33PM +0100, Mylène Josserand wrote:
> > To prepare the support for sun8i-a83t, create a new function
> > that handles all the resources needed on sun9i-a80 (because
> > it will be different from sun8i-a83t).
> > 
> > All the parsing of device tree is moved into this new function:
> > sun9i_dt_parsing. Create also a function to release the resources
> > retrieved during the dt parsing in case there is an error in init
> > function.
> > 
> > Signed-off-by: Mylène Josserand 
> > ---
> >  arch/arm/mach-sunxi/mc_smp.c | 99 
> > ++--
> >  1 file changed, 58 insertions(+), 41 deletions(-)
> > 
> > diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c
> > index 11e46c6efb90..650a2ad4398f 100644
> > --- a/arch/arm/mach-sunxi/mc_smp.c
> > +++ b/arch/arm/mach-sunxi/mc_smp.c
> > @@ -684,35 +684,22 @@ static int __init sunxi_mc_smp_lookback(void)
> > return ret;
> >  }
> >  
> > -static int __init sunxi_mc_smp_init(void)
> > +static int sun9i_dt_parsing(struct resource res)  
> 
> Like I told you in the previous version, this should be _parse_dt, and
> not _dt_parsing.

Sorry, I forgot this review.

> 
> Also, I'm not sure why you are passing by copy the resource structure?
> 
> >  {
> > -   struct device_node *cpucfg_node, *sram_node, *node;
> > -   struct resource res;
> > +   struct device_node *prcm_node, *cpucfg_node, *sram_node;
> > int ret;
> >  
> > -   if (!of_machine_is_compatible("allwinner,sun9i-a80"))
> > -   return -ENODEV;
> > -
> > -   if (!sunxi_mc_smp_cpu_table_init())
> > -   return -EINVAL;
> > -
> > -   if (!cci_probed()) {
> > -   pr_err("%s: CCI-400 not available\n", __func__);
> > -   return -ENODEV;
> > -   }
> > -
> > -   node = of_find_compatible_node(NULL, NULL, "allwinner,sun9i-a80-prcm");
> > -   if (!node) {
> > -   pr_err("%s: PRCM not available\n", __func__);
> > +   prcm_node = of_find_compatible_node(NULL, NULL,
> > +  "allwinner,sun9i-a80-prcm");
> > +   if (!prcm_node)
> > return -ENODEV;
> > -   }
> >  
> > /*
> >  * Unfortunately we can not request the I/O region for the PRCM.
> >  * It is shared with the PRCM clock.
> >  */
> > -   prcm_base = of_iomap(node, 0);
> > -   of_node_put(node);
> > +   prcm_base = of_iomap(prcm_node, 0);  
> 
> So it does more a bit more than just parsing the DT?
> 
> Can you maybe just fill the DT nodes and have the common part map the
> memory regions if the pointer is not NULL?

Yep, I will rework this function to only parse device tree nodes.

Thanks,

Mylène

-- 
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v4 01/10] ARM: sun9i: smp: Add sun9i dt parsing function

[Mylène Josserand]

Hi,

On Fri, 23 Feb 2018 15:54:23 +0100
Maxime Ripard  wrote:

> Hi,
> 
> On Fri, Feb 23, 2018 at 02:37:33PM +0100, Mylène Josserand wrote:
> > To prepare the support for sun8i-a83t, create a new function
> > that handles all the resources needed on sun9i-a80 (because
> > it will be different from sun8i-a83t).
> > 
> > All the parsing of device tree is moved into this new function:
> > sun9i_dt_parsing. Create also a function to release the resources
> > retrieved during the dt parsing in case there is an error in init
> > function.
> > 
> > Signed-off-by: Mylène Josserand 
> > ---
> >  arch/arm/mach-sunxi/mc_smp.c | 99 
> > ++--
> >  1 file changed, 58 insertions(+), 41 deletions(-)
> > 
> > diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c
> > index 11e46c6efb90..650a2ad4398f 100644
> > --- a/arch/arm/mach-sunxi/mc_smp.c
> > +++ b/arch/arm/mach-sunxi/mc_smp.c
> > @@ -684,35 +684,22 @@ static int __init sunxi_mc_smp_lookback(void)
> > return ret;
> >  }
> >  
> > -static int __init sunxi_mc_smp_init(void)
> > +static int sun9i_dt_parsing(struct resource res)  
> 
> Like I told you in the previous version, this should be _parse_dt, and
> not _dt_parsing.

Sorry, I forgot this review.

> 
> Also, I'm not sure why you are passing by copy the resource structure?
> 
> >  {
> > -   struct device_node *cpucfg_node, *sram_node, *node;
> > -   struct resource res;
> > +   struct device_node *prcm_node, *cpucfg_node, *sram_node;
> > int ret;
> >  
> > -   if (!of_machine_is_compatible("allwinner,sun9i-a80"))
> > -   return -ENODEV;
> > -
> > -   if (!sunxi_mc_smp_cpu_table_init())
> > -   return -EINVAL;
> > -
> > -   if (!cci_probed()) {
> > -   pr_err("%s: CCI-400 not available\n", __func__);
> > -   return -ENODEV;
> > -   }
> > -
> > -   node = of_find_compatible_node(NULL, NULL, "allwinner,sun9i-a80-prcm");
> > -   if (!node) {
> > -   pr_err("%s: PRCM not available\n", __func__);
> > +   prcm_node = of_find_compatible_node(NULL, NULL,
> > +  "allwinner,sun9i-a80-prcm");
> > +   if (!prcm_node)
> > return -ENODEV;
> > -   }
> >  
> > /*
> >  * Unfortunately we can not request the I/O region for the PRCM.
> >  * It is shared with the PRCM clock.
> >  */
> > -   prcm_base = of_iomap(node, 0);
> > -   of_node_put(node);
> > +   prcm_base = of_iomap(prcm_node, 0);  
> 
> So it does more a bit more than just parsing the DT?
> 
> Can you maybe just fill the DT nodes and have the common part map the
> memory regions if the pointer is not NULL?

Yep, I will rework this function to only parse device tree nodes.

Thanks,

Mylène

-- 
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v4 01/10] ARM: sun9i: smp: Add sun9i dt parsing function

[Maxime Ripard]

Hi,

On Fri, Feb 23, 2018 at 02:37:33PM +0100, Mylène Josserand wrote:
> To prepare the support for sun8i-a83t, create a new function
> that handles all the resources needed on sun9i-a80 (because
> it will be different from sun8i-a83t).
> 
> All the parsing of device tree is moved into this new function:
> sun9i_dt_parsing. Create also a function to release the resources
> retrieved during the dt parsing in case there is an error in init
> function.
> 
> Signed-off-by: Mylène Josserand 
> ---
>  arch/arm/mach-sunxi/mc_smp.c | 99 
> ++--
>  1 file changed, 58 insertions(+), 41 deletions(-)
> 
> diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c
> index 11e46c6efb90..650a2ad4398f 100644
> --- a/arch/arm/mach-sunxi/mc_smp.c
> +++ b/arch/arm/mach-sunxi/mc_smp.c
> @@ -684,35 +684,22 @@ static int __init sunxi_mc_smp_lookback(void)
>   return ret;
>  }
>  
> -static int __init sunxi_mc_smp_init(void)
> +static int sun9i_dt_parsing(struct resource res)

Like I told you in the previous version, this should be _parse_dt, and
not _dt_parsing.

Also, I'm not sure why you are passing by copy the resource structure?

>  {
> - struct device_node *cpucfg_node, *sram_node, *node;
> - struct resource res;
> + struct device_node *prcm_node, *cpucfg_node, *sram_node;
>   int ret;
>  
> - if (!of_machine_is_compatible("allwinner,sun9i-a80"))
> - return -ENODEV;
> -
> - if (!sunxi_mc_smp_cpu_table_init())
> - return -EINVAL;
> -
> - if (!cci_probed()) {
> - pr_err("%s: CCI-400 not available\n", __func__);
> - return -ENODEV;
> - }
> -
> - node = of_find_compatible_node(NULL, NULL, "allwinner,sun9i-a80-prcm");
> - if (!node) {
> - pr_err("%s: PRCM not available\n", __func__);
> + prcm_node = of_find_compatible_node(NULL, NULL,
> +"allwinner,sun9i-a80-prcm");
> + if (!prcm_node)
>   return -ENODEV;
> - }
>  
>   /*
>* Unfortunately we can not request the I/O region for the PRCM.
>* It is shared with the PRCM clock.
>*/
> - prcm_base = of_iomap(node, 0);
> - of_node_put(node);
> + prcm_base = of_iomap(prcm_node, 0);

So it does more a bit more than just parsing the DT?

Can you maybe just fill the DT nodes and have the common part map the
memory regions if the pointer is not NULL?

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH v4 01/10] ARM: sun9i: smp: Add sun9i dt parsing function

[Maxime Ripard]

Hi,

On Fri, Feb 23, 2018 at 02:37:33PM +0100, Mylène Josserand wrote:
> To prepare the support for sun8i-a83t, create a new function
> that handles all the resources needed on sun9i-a80 (because
> it will be different from sun8i-a83t).
> 
> All the parsing of device tree is moved into this new function:
> sun9i_dt_parsing. Create also a function to release the resources
> retrieved during the dt parsing in case there is an error in init
> function.
> 
> Signed-off-by: Mylène Josserand 
> ---
>  arch/arm/mach-sunxi/mc_smp.c | 99 
> ++--
>  1 file changed, 58 insertions(+), 41 deletions(-)
> 
> diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c
> index 11e46c6efb90..650a2ad4398f 100644
> --- a/arch/arm/mach-sunxi/mc_smp.c
> +++ b/arch/arm/mach-sunxi/mc_smp.c
> @@ -684,35 +684,22 @@ static int __init sunxi_mc_smp_lookback(void)
>   return ret;
>  }
>  
> -static int __init sunxi_mc_smp_init(void)
> +static int sun9i_dt_parsing(struct resource res)

Like I told you in the previous version, this should be _parse_dt, and
not _dt_parsing.

Also, I'm not sure why you are passing by copy the resource structure?

>  {
> - struct device_node *cpucfg_node, *sram_node, *node;
> - struct resource res;
> + struct device_node *prcm_node, *cpucfg_node, *sram_node;
>   int ret;
>  
> - if (!of_machine_is_compatible("allwinner,sun9i-a80"))
> - return -ENODEV;
> -
> - if (!sunxi_mc_smp_cpu_table_init())
> - return -EINVAL;
> -
> - if (!cci_probed()) {
> - pr_err("%s: CCI-400 not available\n", __func__);
> - return -ENODEV;
> - }
> -
> - node = of_find_compatible_node(NULL, NULL, "allwinner,sun9i-a80-prcm");
> - if (!node) {
> - pr_err("%s: PRCM not available\n", __func__);
> + prcm_node = of_find_compatible_node(NULL, NULL,
> +"allwinner,sun9i-a80-prcm");
> + if (!prcm_node)
>   return -ENODEV;
> - }
>  
>   /*
>* Unfortunately we can not request the I/O region for the PRCM.
>* It is shared with the PRCM clock.
>*/
> - prcm_base = of_iomap(node, 0);
> - of_node_put(node);
> + prcm_base = of_iomap(prcm_node, 0);

So it does more a bit more than just parsing the DT?

Can you maybe just fill the DT nodes and have the common part map the
memory regions if the pointer is not NULL?

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature