physmap and pointless shutdown() function ?

[Mike Frysinger]

On Sat, Jun 6, 2009 at 08:27, David Woodhouse wrote:
> On Fri, 2009-06-05 at 19:42 -0400, Mike Frysinger wrote:
>> On Fri, Jun 5, 2009 at 13:51, David Woodhouse wrote:
>> > On Wed, 2009-05-06 at 22:41 -0400, Mike Frysinger wrote:
>> >> waaaay back when power management support was added to the physmap.c
>> >> driver, it added the standard suspend/resume functions.  no problem
>> >> there.  but it also added a shutdown function which causes the flash
>> >> to suspend/resume when rebooting:
>> >> static void physmap_flash_shutdown(struct platform_device *dev)
>> >> {
>> >>     struct physmap_flash_info *info = platform_get_drvdata(dev);
>> >>     int i;
>> >>     for (i = 0; i < MAX_RESOURCES && info->mtd[i]; i++)
>> >>         if (info->mtd[i]->suspend && info->mtd[i]->resume)
>> >>             if (info->mtd[i]->suspend(info->mtd[i]) == 0)
>> >>                 info->mtd[i]->resume(info->mtd[i]);
>> >> }
>> >>
>> >> i cant see any point in doing this.  it isnt like the flash has
>> >> buffers that need flushing, and if they did, using suspend/resume as a
>> >> hack for flushing sounds pretty broken to me.
>> >>
>> >> seems to me the function should just be dropped completely.
>> >
>> > Wasn't that just to put it into read mode in case the bootloader is
>> > running from the same chip?
>>
>> the flash isnt put into read mode though.  if i add printk's to the
>> shutdown function like so:
>> then during reboot i see:
>> physmap_flash_shutdown: 0: before: aded5006
>> physmap_flash_shutdown: 0: after: 800080
>> the first value is the correct value
>
> That's odd, and would seem to indicate that the attempt to ensure the
> flash is in read mode isn't _working_. But I still think that was the
> _intent_ of this code. It was just coincidence that it was in read mode
> before the call; we needed to make 100% sure.

looking through the nest of function pointers, this seems to be a bug
in the intel cfi command set then ?  that is what implements the
suspend/resume functions ...

i know this flash is using a command set version that was only added
somewhat recently (1.5 iirc) ...

>> > Reboot doesn't work very well if all you
>> > have is flash status bits at your reset vector,
>>
>> your assumption is faulty in that the reset vector is the base of the
>> flash.  i.e. the physmap base is 0x20000000 as is the reset vector.
>
> The reset vector may or may not be in the physmap flash. If it's not, of
> course, then the shutdown state doesn't matter. If it _is_, then the
> shutdown state _might_ matter (depending on the hardware reset
> arrangements).
>
> It doesn't matter whether it's at the base of the flash, at the end of
> the flash, or in the middle -- does it?

i didnt think it did, but your comment about flash status bits at
reset vector seemed to imply that it did

> The point remains -- I don't think we can remove the shutdown() function
> -- I think it needs to be fixed to leave the flash in read mode.

i'm fine with that intent if it worked ;)
-mike