| Message ID | 1d461044-07e9-48f2-a863-aa1920524b31@mary.at.omicron.at |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, May 05, 2014 at 08:14:27AM +0200, Christian Riesch wrote: > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > @@ -1157,11 +1157,42 @@ static int cfi_amdstd_read (struct mtd_info *mtd, loff_t from, size_t len, size_ > typedef int (*otp_op_t)(struct map_info *map, struct flchip *chip, > loff_t adr, size_t len, u_char *buf); > > +static inline void otp_enter(struct map_info *map, struct flchip *chip, Why is this function marked 'inline'? Is this to satisfy the comments regarding CONFIG_MTD_XIP and inlining functions? That all looks highly fragile ('inline' is not a guarantee, for instance). And does anyone use CONFIG_MTD_XIP=y these days? If there are no good reasons otherwise, I'd say this trips over Chapter 15 of Documentation/CodingStyle. But if this is the only issue, then I'm OK taking the series as-is -- it has been pending for a long time, as you note... > + loff_t adr, size_t len) > +{ > + struct cfi_private *cfi = map->fldrv_priv; > + > + cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, > + cfi->device_type, NULL); > + cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, > + cfi->device_type, NULL); > + cfi_send_gen_cmd(0x88, cfi->addr_unlock1, chip->start, map, cfi, > + cfi->device_type, NULL); > + > + INVALIDATE_CACHED_RANGE(map, chip->start + adr, len); > +} > + > +static inline void otp_exit(struct map_info *map, struct flchip *chip, Same here. > + loff_t adr, size_t len) > +{ > + struct cfi_private *cfi = map->fldrv_priv; > + > + cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, > + cfi->device_type, NULL); > + cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, > + cfi->device_type, NULL); > + cfi_send_gen_cmd(0x90, cfi->addr_unlock1, chip->start, map, cfi, > + cfi->device_type, NULL); > + cfi_send_gen_cmd(0x00, cfi->addr_unlock1, chip->start, map, cfi, > + cfi->device_type, NULL); > + > + INVALIDATE_CACHED_RANGE(map, chip->start + adr, len); > +} > + > static inline int do_read_secsi_onechip(struct map_info *map, struct flchip *chip, loff_t adr, size_t len, u_char *buf) > { > DECLARE_WAITQUEUE(wait, current); > unsigned long timeo = jiffies + HZ; > - struct cfi_private *cfi = map->fldrv_priv; > > retry: > mutex_lock(&chip->mutex); Brian
Hi Brian, Thank you for your comments! On Wed, May 28, 2014 at 8:46 AM, Brian Norris <computersforpeace@gmail.com> wrote: > > On Mon, May 05, 2014 at 08:14:27AM +0200, Christian Riesch wrote: > > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > > @@ -1157,11 +1157,42 @@ static int cfi_amdstd_read (struct mtd_info *mtd, loff_t from, size_t len, size_ > > typedef int (*otp_op_t)(struct map_info *map, struct flchip *chip, > > loff_t adr, size_t len, u_char *buf); > > > > +static inline void otp_enter(struct map_info *map, struct flchip *chip, > > Why is this function marked 'inline'? Is this to satisfy the comments > regarding CONFIG_MTD_XIP and inlining functions? That all looks highly > fragile ('inline' is not a guarantee, for instance). I really don't remember why I marked it inline, it was more than a year ago. Yes, probably due to the comments regarding CONFIG_MTD_XIP. Christian > And does anyone > use CONFIG_MTD_XIP=y these days? http://thread.gmane.org/gmane.linux.drivers.mtd/47127 It doesn't look like if there were lots of users. This is the only thread mentioning the use of CONFIG_MTD_XIP on the mailing list in the last years... > > If there are no good reasons otherwise, I'd say this trips over Chapter > 15 of Documentation/CodingStyle. > > But if this is the only issue, then I'm OK taking the series as-is -- it > has been pending for a long time, as you note... If you would like me to change it, I'll send an update. But up to now it seems to be the only issue. Christian > > > + loff_t adr, size_t len) > > +{ > > + struct cfi_private *cfi = map->fldrv_priv; > > + > > + cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, > > + cfi->device_type, NULL); > > + cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, > > + cfi->device_type, NULL); > > + cfi_send_gen_cmd(0x88, cfi->addr_unlock1, chip->start, map, cfi, > > + cfi->device_type, NULL); > > + > > + INVALIDATE_CACHED_RANGE(map, chip->start + adr, len); > > +} > > + > > +static inline void otp_exit(struct map_info *map, struct flchip *chip, > > Same here. > > > + loff_t adr, size_t len) > > +{ > > + struct cfi_private *cfi = map->fldrv_priv; > > + > > + cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, > > + cfi->device_type, NULL); > > + cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, > > + cfi->device_type, NULL); > > + cfi_send_gen_cmd(0x90, cfi->addr_unlock1, chip->start, map, cfi, > > + cfi->device_type, NULL); > > + cfi_send_gen_cmd(0x00, cfi->addr_unlock1, chip->start, map, cfi, > > + cfi->device_type, NULL); > > + > > + INVALIDATE_CACHED_RANGE(map, chip->start + adr, len); > > +} > > + > > static inline int do_read_secsi_onechip(struct map_info *map, struct flchip *chip, loff_t adr, size_t len, u_char *buf) > > { > > DECLARE_WAITQUEUE(wait, current); > > unsigned long timeo = jiffies + HZ; > > - struct cfi_private *cfi = map->fldrv_priv; > > > > retry: > > mutex_lock(&chip->mutex); > > Brian > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c index 6e2ce76..bf72128 100644 --- a/drivers/mtd/chips/cfi_cmdset_0002.c +++ b/drivers/mtd/chips/cfi_cmdset_0002.c @@ -1157,11 +1157,42 @@ static int cfi_amdstd_read (struct mtd_info *mtd, loff_t from, size_t len, size_ typedef int (*otp_op_t)(struct map_info *map, struct flchip *chip, loff_t adr, size_t len, u_char *buf); +static inline void otp_enter(struct map_info *map, struct flchip *chip, + loff_t adr, size_t len) +{ + struct cfi_private *cfi = map->fldrv_priv; + + cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, + cfi->device_type, NULL); + cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, + cfi->device_type, NULL); + cfi_send_gen_cmd(0x88, cfi->addr_unlock1, chip->start, map, cfi, + cfi->device_type, NULL); + + INVALIDATE_CACHED_RANGE(map, chip->start + adr, len); +} + +static inline void otp_exit(struct map_info *map, struct flchip *chip, + loff_t adr, size_t len) +{ + struct cfi_private *cfi = map->fldrv_priv; + + cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, + cfi->device_type, NULL); + cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, + cfi->device_type, NULL); + cfi_send_gen_cmd(0x90, cfi->addr_unlock1, chip->start, map, cfi, + cfi->device_type, NULL); + cfi_send_gen_cmd(0x00, cfi->addr_unlock1, chip->start, map, cfi, + cfi->device_type, NULL); + + INVALIDATE_CACHED_RANGE(map, chip->start + adr, len); +} + static inline int do_read_secsi_onechip(struct map_info *map, struct flchip *chip, loff_t adr, size_t len, u_char *buf) { DECLARE_WAITQUEUE(wait, current); unsigned long timeo = jiffies + HZ; - struct cfi_private *cfi = map->fldrv_priv; retry: mutex_lock(&chip->mutex); @@ -1183,16 +1214,9 @@ static inline int do_read_secsi_onechip(struct map_info *map, struct flchip *chi chip->state = FL_READY; - cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL); - cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL); - cfi_send_gen_cmd(0x88, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL); - + otp_enter(map, chip, adr, len); map_copy_from(map, buf, adr, len); - - cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL); - cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL); - cfi_send_gen_cmd(0x90, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL); - cfi_send_gen_cmd(0x00, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL); + otp_exit(map, chip, adr, len); wake_up(&chip->wq); mutex_unlock(&chip->mutex);
When the one time programmable (OTP) memory region is entered by issuing the 0xaa/0x55/0x88 command, the OTP memory occupies the addresses which are normally used by the first sector of the regular flash memory. This patch therefore invalidates cache for this addresses after entering/exiting OTP memory. This patch also moves the code into separate functions. Signed-off-by: Christian Riesch <christian.riesch@omicron.at> --- drivers/mtd/chips/cfi_cmdset_0002.c | 44 +++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 10 deletions(-)