[PATCH v2 6/7] mtd: sh_flctl: Add FLHOLDCR register

[Bastian Hecht]

Hello Laurent,

2012/2/18 Laurent Pinchart <laurent.pinchart at ideasonboard.com>:
> Hi Bastian,
>
> Thanks for the patch.
>
> On Saturday 11 February 2012 12:45:04 Bastian Hecht wrote:
>> Add a register used in new FLCTL hardware and a feature flag for it.
>>
>> Signed-off-by: Bastian Hecht <hechtb at gmail.com>
>> ---
>> changelog: the write to the register has been moved due to patch 5.
>>
>>  drivers/mtd/nand/sh_flctl.c  |    3 +++
>>  include/linux/mtd/sh_flctl.h |   12 ++++++++++++
>>  2 files changed, 15 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
>> index 1af41fd..40dda26 100644
>> --- a/drivers/mtd/nand/sh_flctl.c
>> +++ b/drivers/mtd/nand/sh_flctl.c
>> @@ -688,6 +688,8 @@ static void flctl_select_chip(struct mtd_info *mtd, int
>> chipnr) break;
>>       case 0:
>>               writel(flctl->flcmncr_val | CE0_ENABLE, FLCMNCR(flctl));
>> +             if (flctl->holden)
>> +                     writel(HOLDEN, FLHOLDCR(flctl));
>
> Can't this be done at probe time (maybe in flctl_chip_init_tail()) ? You could
> then get rid of the flctl->holden field and use platform data directly.

I need this information for runtime pm. I reworked this patch series
to be without rtpm, but from the next patch on (that's only waiting to
be posted), I'll want to have it like this.
In fact this is a design question:
I can either have a resume/runtime_resume callback whose only job is
to set FLHOLDCR. or I integrate this into select_chip. The first
variation saves 1 register write in case of no rtpm activated - the
second variation makes the code a bit slimmer.

What do you think?
1. Leave it like this and save the callbacks in the future
2. Rework it to be set at probe time() and postpone the design decision?
3. Rework it to be set at probe time() and implement callbacks in the
upcoming patches?

>>               break;
>>       default:
>>               BUG();
>> @@ -845,6 +847,7 @@ static int __devinit flctl_probe(struct platform_device
>> *pdev) flctl->pdev = pdev;
>>       flctl->flcmncr_val = pdata->flcmncr_val;
>>       flctl->hwecc = pdata->has_hwecc;
>> +     flctl->holden = pdata->use_holden;
>>
>>       nand->options = NAND_NO_AUTOINCR;
>>
>> diff --git a/include/linux/mtd/sh_flctl.h b/include/linux/mtd/sh_flctl.h
>> index 107fd8a..6046443 100644
>> --- a/include/linux/mtd/sh_flctl.h
>> +++ b/include/linux/mtd/sh_flctl.h
>> @@ -38,6 +38,7 @@
>>  #define FLDTFIFO(f)          (f->reg + 0x24)
>>  #define FLECFIFO(f)          (f->reg + 0x28)
>>  #define FLTRCR(f)            (f->reg + 0x2C)
>> +#define FLHOLDCR(f)          (f->reg + 0x38)
>>  #define      FL4ECCRESULT0(f)        (f->reg + 0x80)
>>  #define      FL4ECCRESULT1(f)        (f->reg + 0x84)
>>  #define      FL4ECCRESULT2(f)        (f->reg + 0x88)
>> @@ -109,6 +110,15 @@
>>  #define TRSTRT               (0x1 << 0)      /* translation start */
>>  #define TREND                (0x1 << 1)      /* translation end */
>>
>> +/*
>> + * FLHOLDCR control bits
>> + *
>> + * HOLDEN: Bus Occupancy Enable (inverted)
>> + * Enable this bit when the external bus might be used in between
>> transfers. + * If not set and the bus gets used by other modules, a
>> deadlock occurs. + */
>> +#define HOLDEN               (0x1 << 0)
>> +
>>  /* FL4ECCCR control bits */
>>  #define      _4ECCFA         (0x1 << 2)      /* 4 symbols correct fault */
>>  #define      _4ECCEND        (0x1 << 1)      /* 4 symbols end */
>> @@ -138,6 +148,7 @@ struct sh_flctl {
>>
>>       unsigned page_size:1;   /* NAND page size (0 = 512, 1 = 2048) */
>>       unsigned hwecc:1;       /* Hardware ECC (0 = disabled, 1 = enabled) */
>> +     unsigned holden:1;      /* Hardware has FLHOLDCR and HOLDEN is set */
>>  };
>>
>>  struct sh_flctl_platform_data {
>> @@ -146,6 +157,7 @@ struct sh_flctl_platform_data {
>>       unsigned long           flcmncr_val;
>>
>>       unsigned has_hwecc:1;
>> +     unsigned use_holden:1;
>>  };
>>
>>  static inline struct sh_flctl *mtd_to_flctl(struct mtd_info *mtdinfo)
>
> --
> Regards,
>
> Laurent Pinchart

thanks for all the reviews!

 Bastian