[Skiboot] [PATCH 5/5] external/pflash: Make MTD accesses the default
Cyril Bur
cyril.bur at au1.ibm.com
Fri Oct 21 20:32:03 AEDT 2016
On Fri, 2016-10-21 at 19:59 +1030, Joel Stanley wrote:
> On Fri, Oct 21, 2016 at 7:22 PM, Cyril Bur <cyril.bur at au1.ibm.com>
> wrote:
> > Now that BMC and host kernel mtd drivers exist and have matured we
> > should use them by default.
> >
> > This is especially important since we seem to be telling everyone
> > to use
> > pflash.
> >
> > Signed-off-by: Cyril Bur <cyril.bur at au1.ibm.com>
> > ---
> > This change is going through testing now. There are still some
> > erasing
> > corner cases that will need to be resolved. The kernel seems happy
> > to
> > use 64K blocks but both the ffs layout and current pflash are very
> > 4K
> > centric, there may be teething issues.
> >
> >
> > external/pflash/pflash.c | 44 ++++++++++++++++++++--------------
> > ----------
> > 1 file changed, 20 insertions(+), 24 deletions(-)
> >
> > diff --git a/external/pflash/pflash.c b/external/pflash/pflash.c
> > index 608220f..2ae246a 100644
> > --- a/external/pflash/pflash.c
> > +++ b/external/pflash/pflash.c
> > @@ -459,10 +459,13 @@ static void print_help(const char *pname)
> > printf("\t\tDon't ask for confirmation before erasing or
> > flashing\n\n");
> > printf("\t-d, --dummy\n");
> > printf("\t\tDon't write to flash\n\n");
> > - printf("\t-m, --mtd\n");
> > - printf("\t\tAvoid accessing the flash directly if the BMC
> > supports it.\n");
> > - printf("\t\tThis will access the flash through the kernel
> > MTD layer and\n");
> > - printf("\t\tnot the flash directly\n");
>
> Do we want to keep the -m option for backwards compatibility?
>
>
> > + printf("\t--direct\n");
> > + printf("\t\tBypass all safety provided to you by the kernel
> > driver\n");
> > + printf("\t\tand use the flash driver built into
> > pflash.\n");
> > + printf("\t\tIf you are reading this sentence then this
> > flag\n");
> > + printf("\t\tis not what you want! This feature is not
> > supported,\n");
> > + printf("\t\tit can and likely will result in a bricked\n");
> > + printf("\t\tmachine\n\n");
>
> This isn't really true. Perhaps:
>
Yes but I don't want to be the guy unbricking :)
> "Direct MMIO access to the flash controller requiring /dev/mem. If
> you
> have mtd devices then you should be using those instead. If you have
> mtd devices and you use this command, the system may become
> unstable"
That last point is spot on, I'll include that.
>
> > printf("\t-b, --bmc\n");
> > printf("\t\tTarget BMC flash instead of host flash.\n");
> > printf("\t\tNote: This carries a high chance of bricking
> > your BMC if you\n");
> > @@ -533,7 +536,7 @@ int main(int argc, char *argv[])
> > bool show_help = false, show_version = false;
> > bool no_action = false, tune = false;
> > char *write_file = NULL, *read_file = NULL, *part_name =
> > NULL;
> > - bool ffs_toc_seen = false, mtd = false;
> > + bool ffs_toc_seen = false, direct = false;
> > int rc;
> > const char *flashfilename = NULL;
> >
> > @@ -543,7 +546,7 @@ int main(int argc, char *argv[])
> > {"size", required_argument, NUL
> > L, 's'},
> > {"partition", required_argument, NUL
> > L, 'P'},
> > {"bmc", no_argument, NUL
> > L, 'b'},
> > - {"mtd", no_argument, NUL
> > L, 'm'},
> > + {"direct", no_argument, NUL
> > L, 'D'},
> > {"enable-
> > 4B", no_argument, NULL, '4'},
> > {"disable-
> > 4B", no_argument, NULL, '3'},
> > {"read", required_argument, NUL
> > L, 'r'},
> > @@ -565,7 +568,7 @@ int main(int argc, char *argv[])
> > };
> > int c, oidx = 0;
> >
> > - c = getopt_long(argc, argv,
> > "+:a:s:P:r:43Eemp:fdihvbtgS:T:cF:",
> > + c = getopt_long(argc, argv,
> > "+:a:s:P:r:43Eep:fdihvbtgS:T:cF:",
> > long_opts, &oidx);
> > if (c == -1)
> > break;
> > @@ -595,8 +598,8 @@ int main(int argc, char *argv[])
> > case 'e':
> > erase = true;
> > break;
> > - case 'm':
> > - mtd = true;
> > + case 'D':
> > + direct = true;
> > break;
> > case 'p':
> > program = true;
> > @@ -748,8 +751,8 @@ int main(int argc, char *argv[])
> > exit(1);
> > }
> >
> > - if (flashfilename && mtd) {
> > - fprintf(stderr, "Filename or mtd access but not
> > both\n");
> > + if (flashfilename && direct) {
> > + fprintf(stderr, "Filename or direct access but not
> > both\n");
> > exit(1);
> > }
> >
> > @@ -765,22 +768,15 @@ int main(int argc, char *argv[])
> > write_size = stbuf.st_size;
> > }
> >
> > - if (bmc_flash) {
> > - /*
> > - * Try to see if we can access BMC flash on this
> > arch at all...
> > - * even if what we really want to do is use MTD.
> > - * This helps give a more meaningful error
> > messages.
> > - */
> > -
> > - if (arch_flash_access(NULL, BMC_DIRECT) ==
> > ACCESS_INVAL) {
> > - fprintf(stderr, "Can't access BMC flash on
> > this architecture\n");
> > + if (direct) {
> > + if (arch_flash_access(NULL, bmc_flash ? BMC_DIRECT
> > : PNOR_DIRECT) == ACCESS_INVAL) {
> > + fprintf(stderr, "Can't access %s flash
> > directly on this architecture\n",
> > + bmc_flash ? "BMC" : "PNOR");
> > exit(1);
> > }
> > - }
> > -
> > - if (mtd) {
> > + } else if (!flashfilename) {
> > if (arch_flash_access(NULL, bmc_flash ? BMC_MTD :
> > PNOR_MTD) == ACCESS_INVAL) {
> > - fprintf(stderr, "Can't access %s flash
> > through MTD on this architecture\n",
> > + fprintf(stderr, "Can't access %s flash
> > through MTD on this system\n",
> > bmc_flash ? "BMC" : "PNOR");
> > exit(1);
> > }
> > --
> > 2.10.0
> >
>
>
More information about the Skiboot
mailing list