[03/11] gpio: davinci: Modify to platform driver

Commit Message

avinash philip June 13, 2013, 7:32 a.m. UTC

On Thu, Jun 13, 2013 at 11:47:52, Nori, Sekhar wrote:
> On 6/12/2013 5:40 PM, Philip, Avinash wrote:
> > On Wed, Jun 12, 2013 at 13:13:59, Nori, Sekhar wrote:
> >> On 6/11/2013 6:25 PM, Philip, Avinash wrote:
> >>
> >>> On Tue, Jun 11, 2013 at 17:26:06, Nori, Sekhar wrote:
> >>
> >>>> On 5/22/2013 12:40 PM, Philip Avinash wrote:
> >>
> >>>>> @@ -179,13 +204,10 @@ static int __init davinci_gpio_setup(void)
> >>>>>  		gpiochip_add(&ctlrs[i].chip);
> >>>>>  	}
> >>>>>  
> >>>>> -	soc_info->gpio_ctlrs = ctlrs;
> >>>>
> >>>>> -	soc_info->gpio_ctlrs_num = DIV_ROUND_UP(ngpio, 32);
> >>>>
> >>>> You drop setting gpio_ctlrs_num here and don't introduce it anywhere
> >>>> else in the patchset so in effect you render the inline gpio get/set API
> >>>> useless. Looks like this initialization should be moved to platform code?
> >>>
> >>> With [PATCH 08/11] ARM: davinci: start using gpiolib support gpio get/set API
> >>> Has no more dependency on soc_info->gpio_ctlrs_num.
> >>
> >> With this series, you have removed support for inline gpio get/set API.
> >> I see that the inline functions are still available for use on
> >> tnetv107x. I wonder why it is important to keep these for tnetv107x when
> >> not necessary for other DaVinci devices?
> > 
> > To support DT boot in da850, gpio davinci has to be converted to a driver and
> > remove references to davinci_soc_info from driver. But tnetv107x has 
> > separate GPIO driver and reference to davinci_soc_info can also be removed.
> > But I didn't found defconfig support for tnetv107x platforms and left untouched.
> > As I will not be able to build and test on tnetv107x, I prefer to not touch it.
> 
> You can always build it by enabling it in menuconfig. Its an ARMv6
> platform so you will have to disable other ARMv5 based platforms from
> while enabling it. ARMv5 and ARMv6 cannot co-exist in the same image.

I will try and update.

> 
> > 
> >>
> >> When you are removing this feature, please note it prominently in your
> >> cover letter and patch description.
> > 
> > Ok
> > 
> >> Also, please provide some data on 
> >> how the latency now compares to that of inline access earlier. This is
> >> important especially for the read.
> > 
> > I am not sure whether I understood correctly or not? Meanwhile I had done
> > an experiment by reading printk timing before and after gpio_get_value from
> > a test module. I think this will help in software latency for inline access over
> > gpiolib specific access.
> > 
> > gpio_get_value latency testing code
> > 
> > +
> > +       local_irq_disable();
> > +       pr_emerg("%d %x\n", __LINE__, jiffies);
> > +       gpio_get_value(gpio_num);
> > +       pr_emerg("%d %x\n", __LINE__, jiffies);
> > +       local_irq_enable();
> > 
> > inline gpio functions with interrupt disabled
> > [   29.734337] 81 ffff966c
> > [   29.736847] 83 ffff966c
> > 
> > Time diff = 	0.00251
> > 
> > gpio library with interrupt disabled
> > 
> > [  272.876763] 81 fffff567
> > [  272.879291] 83 fffff567
> > 
> > Time diff =	0.002528
> > 
> > Inline function takes less time as expected.
> 
> Okay, please note these experiments in your cover letter. Its an 18usec
> difference. I have no reference to say if that will affect any
> application, but it will at least serve as information for someone who
> may get affected by it.

Ok I will give above details in commit message.

> 
> > 
> >> For the writes, gpio clock will
> >> mostly determine how soon the value changes on the pin.
> >>
> >> I am okay with removing the inline access feature. It helps simplify
> >> code and most arm machines don't use them. I would just like to see some
> >> data for justification as this can be seen as feature regression. Also,
> >> if we are removing it, its better to also remove it completely and get
> >> the LOC savings instead of just stopping its usage.
> > 
> > I see build failure with below patch for tnetv107x
> > [v6,6/6] Davinci: tnetv107x default configuration 
> 
> Where is this patch?

This patch is not in mainline and got it from patchwork
https://patchwork.kernel.org/patch/97853/

> What is the commit-id if it is in mainline? Where
> is the failure log?

With tnetv107x_defconfig build is failing

arch/arm/mach-davinci/board-tnetv107x-evm.c:282:15: error:
 'davinci_timer_init' undeclared here (not in a function)
arch/arm/mach-davinci/board-tnetv107x-evm.c:284:15: error:
 'davinci_init_late' undeclared here (not in a function)
make[1]: *** [arch/arm/mach-davinci/board-tnetv107x-evm.o] Error 1

Following patch fixes the build above breakage




> 
> > 
> > So I prefer to leave tnetv107x platform for now.
> 
> I don't think that's acceptable. At least by me.

I think 2 options are available
1. Convert gpio-tnetv107x.c to platform driver. This will help in
	removing gpio references in davinci_soc_info structure.
2. Remove inline gpio api support and start use gpiolib support.

I prefer first option. It will help in removing
<arch/arm/mach-davinci/include/mach/gpio-davinci.h>.

Thanks
Avinash

> 
> Thanks,
> Sekhar
>

Comments

Sekhar Nori June 13, 2013, 8:29 a.m. UTC | #1

On 6/13/2013 1:02 PM, Philip, Avinash wrote:

> With tnetv107x_defconfig build is failing
> 
> arch/arm/mach-davinci/board-tnetv107x-evm.c:282:15: error:
>  'davinci_timer_init' undeclared here (not in a function)
> arch/arm/mach-davinci/board-tnetv107x-evm.c:284:15: error:
>  'davinci_init_late' undeclared here (not in a function)
> make[1]: *** [arch/arm/mach-davinci/board-tnetv107x-evm.o] Error 1
> 
> Following patch fixes the build above breakage

The error you are seeing and the patch you provided below have no
correlation.

> 
> diff --git a/arch/arm/mach-davinci/board-tnetv107x-evm.c b/arch/arm/mach-davinci/board-tnetv107x-evm.c
> index ba79837..4a9c320 100644
> --- a/arch/arm/mach-davinci/board-tnetv107x-evm.c
> +++ b/arch/arm/mach-davinci/board-tnetv107x-evm.c
> @@ -30,6 +30,7 @@
>  #include <asm/mach/arch.h>
>  #include <asm/mach-types.h>
> 
> +#include <mach/common.h>
>  #include <mach/irqs.h>
>  #include <mach/edma.h>
>  #include <mach/mux.h>
> @@ -147,7 +148,7 @@ static struct davinci_nand_pdata nand_config = {
>         .ecc_bits       = 1,
>  };
> 
> -static struct davinci_uart_config serial_config __initconst = {
> +static struct davinci_uart_config serial_config = {
>         .enabled_uarts  = BIT(1),
>  };

You can make this __initdata instead - assuming its okay to have this
memory discarded at init.

> 
> @@ -245,7 +246,7 @@ static struct ti_ssp_data ssp_config = {
>         },
>  };
> 
> -static struct tnetv107x_device_info evm_device_info __initconst = {
> +static struct tnetv107x_device_info evm_device_info = {

Same here. You can make this __initdata.

Please send a formal patch for the errors you have seen.

>         .serial_config          = &serial_config,
>         .mmc_config[1]          = &mmc_config,  /* controller 1 */
>         .nand_config[0]         = &nand_config, /* chip select 0 */
> 
> 
> 
>>
>>>
>>> So I prefer to leave tnetv107x platform for now.
>>
>> I don't think that's acceptable. At least by me.
> 
> I think 2 options are available
> 1. Convert gpio-tnetv107x.c to platform driver. This will help in
> 	removing gpio references in davinci_soc_info structure.
> 2. Remove inline gpio api support and start use gpiolib support.
> 
> I prefer first option. It will help in removing
> <arch/arm/mach-davinci/include/mach/gpio-davinci.h>.

Okay. Can you take this up in this series? I understand you may not have
an tnetv107x board, but at least you can get to a series that builds.

Even if you choose to do just option #2, I am OK. What I really want to
see is inline API gone completely (not just remain largely unused). This
will also help you avoid exposing internal data structures like
davinci_gpio_controller exposed to the whole kernel. The worse part
right now is you have two copies of the same structure exposed globally
from two different include folders.

Thanks,
Sekhar

avinash philip June 13, 2013, 9:18 a.m. UTC | #2

On Thu, Jun 13, 2013 at 13:59:53, Nori, Sekhar wrote:
> On 6/13/2013 1:02 PM, Philip, Avinash wrote:
> 
> > With tnetv107x_defconfig build is failing
> > 
> > arch/arm/mach-davinci/board-tnetv107x-evm.c:282:15: error:
> >  'davinci_timer_init' undeclared here (not in a function)
> > arch/arm/mach-davinci/board-tnetv107x-evm.c:284:15: error:
> >  'davinci_init_late' undeclared here (not in a function)
> > make[1]: *** [arch/arm/mach-davinci/board-tnetv107x-evm.o] Error 1
> > 
> > Following patch fixes the build above breakage
> 
> The error you are seeing and the patch you provided below have no
> correlation.

No. Above build error fixed by
 
+#include <mach/common.h>

Other changes are not related to above error.

> 
> > 
> > diff --git a/arch/arm/mach-davinci/board-tnetv107x-evm.c b/arch/arm/mach-davinci/board-tnetv107x-evm.c
> > index ba79837..4a9c320 100644
> > --- a/arch/arm/mach-davinci/board-tnetv107x-evm.c
> > +++ b/arch/arm/mach-davinci/board-tnetv107x-evm.c
> > @@ -30,6 +30,7 @@
> >  #include <asm/mach/arch.h>
> >  #include <asm/mach-types.h>
> > 
> > +#include <mach/common.h>
> >  #include <mach/irqs.h>
> >  #include <mach/edma.h>
> >  #include <mach/mux.h>
> > @@ -147,7 +148,7 @@ static struct davinci_nand_pdata nand_config = {
> >         .ecc_bits       = 1,
> >  };
> > 
> > -static struct davinci_uart_config serial_config __initconst = {
> > +static struct davinci_uart_config serial_config = {
> >         .enabled_uarts  = BIT(1),
> >  };
> 
> You can make this __initdata instead - assuming its okay to have this
> memory discarded at init.

I will check.

> 
> > 
> > @@ -245,7 +246,7 @@ static struct ti_ssp_data ssp_config = {
> >         },
> >  };
> > 
> > -static struct tnetv107x_device_info evm_device_info __initconst = {
> > +static struct tnetv107x_device_info evm_device_info = {
> 
> Same here. You can make this __initdata.
> 
> Please send a formal patch for the errors you have seen.

Ok

> 
> >         .serial_config          = &serial_config,
> >         .mmc_config[1]          = &mmc_config,  /* controller 1 */
> >         .nand_config[0]         = &nand_config, /* chip select 0 */
> > 
> > 
> > 
> >>
> >>>
> >>> So I prefer to leave tnetv107x platform for now.
> >>
> >> I don't think that's acceptable. At least by me.
> > 
> > I think 2 options are available
> > 1. Convert gpio-tnetv107x.c to platform driver. This will help in
> > 	removing gpio references in davinci_soc_info structure.
> > 2. Remove inline gpio api support and start use gpiolib support.
> > 
> > I prefer first option. It will help in removing
> > <arch/arm/mach-davinci/include/mach/gpio-davinci.h>.
> 
> Okay. Can you take this up in this series? I understand you may not have
> an tnetv107x board, but at least you can get to a series that builds.
> 
> Even if you choose to do just option #2, I am OK. What I really want to
> see is inline API gone completely (not just remain largely unused). This
> will also help you avoid exposing internal data structures like
> davinci_gpio_controller exposed to the whole kernel. The worse part
> right now is you have two copies of the same structure exposed globally
> from two different include folders.

I understood. I will take option 2.

Thanks
Avinash

> 
> Thanks,
> Sekhar
>

Patch

diff --git a/arch/arm/mach-davinci/board-tnetv107x-evm.c b/arch/arm/mach-davinci/board-tnetv107x-evm.c
index ba79837..4a9c320 100644
--- a/arch/arm/mach-davinci/board-tnetv107x-evm.c
+++ b/arch/arm/mach-davinci/board-tnetv107x-evm.c
@@ -30,6 +30,7 @@ 
 #include <asm/mach/arch.h>
 #include <asm/mach-types.h>

+#include <mach/common.h>
 #include <mach/irqs.h>
 #include <mach/edma.h>
 #include <mach/mux.h>
@@ -147,7 +148,7 @@  static struct davinci_nand_pdata nand_config = {
        .ecc_bits       = 1,
 };

-static struct davinci_uart_config serial_config __initconst = {
+static struct davinci_uart_config serial_config = {
        .enabled_uarts  = BIT(1),
 };

@@ -245,7 +246,7 @@  static struct ti_ssp_data ssp_config = {
        },
 };

-static struct tnetv107x_device_info evm_device_info __initconst = {
+static struct tnetv107x_device_info evm_device_info = {
        .serial_config          = &serial_config,
        .mmc_config[1]          = &mmc_config,  /* controller 1 */
        .nand_config[0]         = &nand_config, /* chip select 0 */