| Message ID | 1437116484-6361-5-git-send-email-cyril.bur@au1.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
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. Did we ever use xscom? I think you mean LPC. 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} (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; don't forget to free dev_path after reading it. > + 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? This function is searching for /dev/mtdN devices, and then looking in the dt for a matching entry? > +{ > + 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. Is there a --force mode so the user can say "just use the flash device I've pointed you to?" > + 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? > + } > + 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; > +}
On Mon, 20 Jul 2015 14:28:13 +0930 Joel Stanley <joel@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.... > > 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; > > +} > >
On Mon, 3 Aug 2015 15:02:08 Cyril Bur wrote: > On Mon, 20 Jul 2015 14:28:13 +0930 > Joel Stanley <joel@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@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot >
On Tue, Aug 4, 2015 at 11:14 AM, Alistair Popple <alistair@popple.id.au> wrote: >> > 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. Yeah. pflash would use the lpc debugfs interface which the kernel turned into opal calls, and then skiboot would do the poking of the scom registers for us. Do you think we should add another layer of abstraction? Joel
On Tue, 4 Aug 2015 11:42:23 Joel Stanley wrote: > On Tue, Aug 4, 2015 at 11:14 AM, Alistair Popple <alistair@popple.id.au> wrote: > >> > 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. > > Yeah. pflash would use the lpc debugfs interface which the kernel > turned into opal calls, and then skiboot would do the poking of the > scom registers for us. > > Do you think we should add another layer of abstraction? Not at all! That was more just for information. I hadn't actually noticed the opal_lpc_read/write calls. - Alistair > Joel
diff --git a/external/common/arch_flash_powerpc.c b/external/common/arch_flash_powerpc.c new file mode 100644 index 0000000..921fba4 --- /dev/null +++ b/external/common/arch_flash_powerpc.c @@ -0,0 +1,200 @@ +/* Copyright 2013-2015 IBM Corp. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or + * implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include <stdlib.h> +#include <stdio.h> +#include <limits.h> +#include <unistd.h> +#include <dirent.h> +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <string.h> + +#include <libflash/file.h> + +#include "arch_flash.h" + +#define FDT_FLASH_PATH "/proc/device-tree/chosen/ibm,system-flash" +#define SYSFS_MTD_PATH "/sys/class/mtd/" + +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)); + 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) +{ + 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); + 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); + 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); + + 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); + done = 1; + } + 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; +} + +static struct blocklevel_device *arch_init_blocklevel(const char *file) +{ + int rc; + struct blocklevel_device *new_bl = NULL; + char *real_file; + + if (!file) { + rc = get_dev_mtd(FDT_FLASH_PATH, &real_file); + if (rc) + return NULL; + } + + file_init_path(file ? file : real_file, NULL, &new_bl); + return new_bl; +} + +int arch_flash_init(struct blocklevel_device **r_bl, const char *file) +{ + struct blocklevel_device *new_bl; + + new_bl = arch_init_blocklevel(file); + if (!new_bl) + return -1; + + *r_bl = new_bl; + return 0; +} + +void arch_flash_close(struct blocklevel_device *bl, const char *file) +{ + file_exit_close(bl); +} diff --git a/external/common/arch_flash_powerpc_io.h b/external/common/arch_flash_powerpc_io.h new file mode 100644 index 0000000..e69de29 diff --git a/external/pflash/rules.mk b/external/pflash/rules.mk index 4ad381c..7098131 100644 --- a/external/pflash/rules.mk +++ b/external/pflash/rules.mk @@ -4,8 +4,13 @@ ifeq ($(ARCH),ARCH_ARM) arch = arm ARCH_OBJS = common/arch_flash_common.o common/arch_flash_arm.o ast-sf-ctrl.o else +ifeq ($(ARCH),ARCH_POWERPC) +arch = powerpc +ARCH_OBJS = common/arch_flash_common.o common/arch_flash_powerpc.o +else $(error Unsupported architecture $(ARCH)) endif +endif .DEFAULT_GOAL := all
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. Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> --- external/common/arch_flash_powerpc.c | 200 ++++++++++++++++++++++++++++++++ external/common/arch_flash_powerpc_io.h | 0 external/pflash/rules.mk | 5 + 3 files changed, 205 insertions(+) create mode 100644 external/common/arch_flash_powerpc.c create mode 100644 external/common/arch_flash_powerpc_io.h