[Skiboot] [RFC PATCH 4/7] external/common: Add POWERPC code reenable building pflash for POWER
Alistair Popple
alistair at popple.id.au
Tue Aug 4 11:44:09 AEST 2015
On Mon, 3 Aug 2015 15:02:08 Cyril Bur wrote:
> On Mon, 20 Jul 2015 14:28:13 +0930
> Joel Stanley <joel at jms.id.au> wrote:
>
> > On Fri, 2015-07-17 at 17:01 +1000, Cyril Bur wrote:
> > > As per commit to create the external/common code, this commit introduces
> > > the POWER arch compatibility flash reading code.
> > >
> > > This commit actually should cause no functional change to pflash (it
won't
> > > be able to access the BMC flash if it ever could), however rather than
> > > accessing the flash via the debug xscom interface it will access it
though
> > > the MTD interface provided by linux and skiboot.
> >
>
> Thanks for the review! I'll go pflash some new firmware on a palmetto or two
> just to be sure there isn't a massive regression but I can't see myself
testing
> all the corner cases...
>
> > Did we ever use xscom? I think you mean LPC.
>
> Maybe....
The LPC bus is accessed via scom registers, so you're both right. I believe
pflash used to access the LPC bus via poking at scom registers using the xscom
debugfs interface. In reality the scom registers are actually used to access
the OPB bus which is what the LPC bus is connected to on the host.
>
> >
> > Although it happens to be ARM/Power, when writing your commit messages
> > it would make sense to talk about it in terms of the method you're using
> > to access it:
> >
> > AHB is the bus that attaches PNOR to the BMC, which happens to be ARM
> > (but could also be ColdFire - but lets not go there)
> >
> > LPC is how the Power8 is attached to the PNOR.
> >
> > When the PNOR is mapped on the LPC bus, Linux can talk to it using the:
> >
> > - mtd device, which uses opal_flash_{read,write,erase}
> > - debugfs lpc, which use opal_lpc_{read,write}
> >
>
> So this may be the best explaination I have of what used to be done... (i'm
> going to run with it!), I'll edit the commit message, probs get you to proof
> read it once more...
>
> > (cc+benh so he can correct me where I'm wrong)
> > > --- /dev/null
> > > +++ b/external/common/arch_flash_powerpc.c
> >
> > > +static int get_dev_attr(const char *dev, const char *attr_file,
uint32_t *attr)
> > > +{
> > > + char dev_path[PATH_MAX] = SYSFS_MTD_PATH;
> > > + /*
> > > + * Needs to be large enough to hold at most uint32_t represented as a
> > > + * string in hex with leading 0x
> > > + */
> > > + char attr_buf[10];
> > > + int fd, rc;
> > > +
> > > + /*
> > > + * sizeof(dev_path) - (strlen(dev_path) + 1) is the remaining space in
> > > + * dev_path, + 1 to account for the '\0'. As strncat could write n+1
bytes
> > > + * to dev_path the correct calulcation for n is:
> > > + * (sizeof(dev_path) - (strlen(dev_path) + 1) - 1)
> > > + */
> > > + strncat(dev_path, dev, (sizeof(dev_path) - (strlen(dev_path) + 1) -
1));
> > > + strncat(dev_path, "/", (sizeof(dev_path) - (strlen(dev_path) + 1) -
1));
> > > + strncat(dev_path, attr_file, (sizeof(dev_path) - (strlen(dev_path) +
1) - 1));
> >
> > I think this is more readable:
> >
> > char *dev_path;
> > rc = asprintf(&dev_path, "%s/%s/%s", FDT_FLASH_PATH, dev, attr_file);
> > if (rc < 0)
> > goto out;
>
> Very nice yes.
>
> >
> > don't forget to free dev_path after reading it.
> >
>
> I bet I did...
>
> > > + fd = open(dev_path, O_RDONLY);
> > > + if (fd == -1)
> > > + goto out;
> > > +
> > > + rc = read(fd, attr_buf, sizeof(attr_buf));
> > > + close(fd);
> > > + if (rc == -1)
> > > + goto out;
> > > +
> > > + if (attr)
> > > + *attr = strtol(attr_buf, NULL, 0);
> > > +
> > > + return 0;
> > > +
> > > +out:
> > > + fprintf(stderr, "Couldn't get MTD device attribute '%s' from '%s'\n",
dev, attr_file);
> > > + return -1;
> > > +}
> > > +
> > > +static int get_dev_mtd(const char *fdt_flash_path, char **r_path)
> >
> > r_path means something else to me. Perhaps a different name? fdt_path
> > and mtd_path?
> >
>
> Sure
>
> > This function is searching for /dev/mtdN devices, and then looking in
> > the dt for a matching entry?
> >
>
> Yeah, I wasn't thrilled with this method of finding out which /dev/mtdN is
the
> flash but its the best I could come up with...
>
> > > +{
> > > + struct dirent **namelist;
> > > + char fdt_node_path[PATH_MAX];
> > > + int count, i, rc, fd, done;
> > > +
> > > + if (!fdt_flash_path)
> > > + return -1;
> > > +
> > > + fd = open(fdt_flash_path, O_RDONLY);
> > > + if (fd == -1) {
> > > + fprintf(stderr, "Couldn't open '%s' FDT attribute to determine
which flash device to use\n",
> > > + fdt_flash_path);
> > > + return -1;
> > > + }
> > > +
> > > + rc = read(fd, fdt_node_path, sizeof(fdt_node_path));
> > > + close(fd);
> > > + if (rc == -1) {
> > > + fprintf(stderr, "Couldn't read flash FDT node from '%s'\n",
fdt_flash_path);
> >
> > We can give the user a hint that they might not have permissions
> > (root?), or that their kernel/skiboot may not be new enough to have the
> > flash node in the device tree.
> >
>
> Yeah, hints are always nice.
>
> > Is there a --force mode so the user can say "just use the flash device
> > I've pointed you to?"
>
> It's not really up to this layer to worry too much about that but it can be
> done.
>
> If you look at arch_flash_init(), what you would want is a tool which
> passes through something in with *file, the gard tool is written that way
> (although it doesn't use this infrastructure yet. pflash would need to be
> updated --force or --file or whatever is what it needs... this is something
I'm
> planning on doing, it's basically a must for running pflash on x86, which,
is
> going to be nice!
>
> >
> > > + return -1;
> > > + }
> > > +
> > > + count = scandir(SYSFS_MTD_PATH, &namelist, NULL, alphasort);
> > > + if (count == -1) {
> > > + fprintf(stderr, "Couldn't scan '%s' for MTD devices\n",
SYSFS_MTD_PATH);
> >
> > Again, give the user a hint to run as root?
> >
> > > + return -1;
> > > + }
> > > +
> > > + rc = 0;
> > > + done = 0;
> > > + for (i = 0; i < count; i++) {
> > > + struct dirent *dirent;
> > > + char dev_path[PATH_MAX] = SYSFS_MTD_PATH;
> > > + char fdt_node_path_tmp[PATH_MAX];
> > > +
> > > + dirent = namelist[i];
> > > + if (dirent->d_name[0] == '.' || rc || done) {
> > > + free(namelist[i]);
> > > + continue;
> > > + }
> > > +
> > > + strncat(dev_path, dirent->d_name, sizeof(dev_path) -
strlen(dev_path) - 2);
> > > + strncat(dev_path, "/device/of_node", sizeof(dev_path) -
strlen(dev_path) - 2);
> >
> > asprintf
> >
> > > +
> > > + rc = readlink(dev_path, fdt_node_path_tmp,
sizeof(fdt_node_path_tmp) - 1);
> > > + if (rc == -1) {
> > > + /*
> > > + * This might fail because it could not exist if the
system has flash
> > > + * devices that present as mtd but don't have
corresponding FDT
> > > + * nodes, just continue silently.
> > > + */
> > > + free(namelist[i]);
> > > + /* Should still try the next dir so reset rc */
> > > + rc = 0;
> > > + continue;
> > > + }
> > > + fdt_node_path_tmp[rc] = '\0';
> > > +
> > > + if (strstr(fdt_node_path_tmp, fdt_node_path)) {
> > > + uint32_t flags, size;
> > > +
> > > + /*
> > > + * size and flags could perhaps have be gotten another
way but this
> > > + * method is super unlikely to fail so it will do.
> > > + */
> > > +
> > > + /* Check to see if device is writeable */
> > > + rc = get_dev_attr(dirent->d_name, "flags", &flags);
> > > + if (rc) {
> > > + free(namelist[i]);
> > > + continue;
> > > + }
> > > +
> > > + /* Get the size of the mtd device while we're at it */
> > > + rc = get_dev_attr(dirent->d_name, "size", &size);
> > > + if (rc) {
> > > + free(namelist[i]);
> > > + continue;
> > > + }
> > > +
> > > + strcpy(dev_path, "/dev/");
> > > + strncat(dev_path, dirent->d_name, sizeof(dev_path) -
strlen(dev_path) - 2);
> > > + *r_path = strdup(dev_path);
> >
> > asprintf
> >
> > > + done = 1;
> >
> > true?
>
> true!
>
> >
> > > + }
> > > + free(namelist[i]);
> > > + }
> > > + free(namelist);
> > > +
> > > + if (!done)
> > > + fprintf(stderr, "Couldn't find '%s' corresponding MTD\n",
fdt_flash_path);
> > > +
> > > + /* explicit negative value so as to not return a libflash code */
> > > + return done ? rc : -1;
> > > +}
> >
> >
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
>
More information about the Skiboot
mailing list