| Message ID | 20160801205035.18514-1-jack@codezen.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Quoting Jack Miller (2016-08-02 06:50:35) > Skiboot will place the flash device tree node at ibm,opal/flash/flash@0 > on P9 and later systems, so Linux needs to search for it there as well > as ibm,opal/flash@0 for backwards compatibility. > > Signed-off-by: Jack Miller <jack@codezen.org> > --- > arch/powerpc/platforms/powernv/opal.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c > index ae29eaf..2847cb0 100644 > --- a/arch/powerpc/platforms/powernv/opal.c > +++ b/arch/powerpc/platforms/powernv/opal.c > @@ -755,9 +755,14 @@ static int __init opal_init(void) > > /* Initialize platform devices: IPMI backend, PRD & flash interface */ > opal_pdev_init(opal_node, "ibm,opal-ipmi"); > - opal_pdev_init(opal_node, "ibm,opal-flash"); > + opal_pdev_init(opal_node, "ibm,opal-flash"); // old <= P8 flash location > opal_pdev_init(opal_node, "ibm,opal-prd"); > > + /* New >= P9 flash location */ > + np = of_get_child_by_name(opal_node, "flash"); > + if (np) > + opal_pdev_init(np, "ibm,opal-flash"); We could instead just search for all nodes that are compatible with "ibm,opal-flash". We do that for i2c, see opal_i2c_create_devs(). Is there a particular reason not to do that? cheers
On Wed, Aug 03, 2016 at 05:16:34PM +1000, Michael Ellerman wrote: > We could instead just search for all nodes that are compatible with > "ibm,opal-flash". We do that for i2c, see opal_i2c_create_devs(). > > Is there a particular reason not to do that? I'm actually surprised that this is preferred. Jeremy mentioned something similar, but I guess I just don't like the idea of finding devices in weird places in the tree. Then again, if we can't trust the DT we're in bigger trouble than erroneous flash nodes =). If we really just want to find compatible nodes anywhere, let's simplify i2c and pdev_init into one function and make that behavior consistent with this new patch. - Jack
Jack Miller <jack@codezen.org> writes: > On Wed, Aug 03, 2016 at 05:16:34PM +1000, Michael Ellerman wrote: >> We could instead just search for all nodes that are compatible with >> "ibm,opal-flash". We do that for i2c, see opal_i2c_create_devs(). >> >> Is there a particular reason not to do that? > > I'm actually surprised that this is preferred. Jeremy mentioned something > similar, but I guess I just don't like the idea of finding devices in weird > places in the tree. But where is "weird". Arguably "/opal/flash" is weird. What does it mean? There's a bus called "opal" and a device on it called "flash"? No. Point being the structure is fairly arbitrary, or at least debatable, so tying the code 100% to the structure is inflexible. As we have discovered. Our other option is to tell skiboot to get stuffed, and leave the flash node where it was on P8. > Then again, if we can't trust the DT we're in bigger > trouble than erroneous flash nodes =). Quite :) > If we really just want to find compatible nodes anywhere, let's simplify i2c > and pdev_init into one function and make that behavior consistent with this > new patch. That seems OK to me. We should get an ack from Stewart though for the other node types. cheers
Michael Ellerman <mpe@ellerman.id.au> writes: > Jack Miller <jack@codezen.org> writes: > >> On Wed, Aug 03, 2016 at 05:16:34PM +1000, Michael Ellerman wrote: >>> We could instead just search for all nodes that are compatible with >>> "ibm,opal-flash". We do that for i2c, see opal_i2c_create_devs(). >>> >>> Is there a particular reason not to do that? >> >> I'm actually surprised that this is preferred. Jeremy mentioned something >> similar, but I guess I just don't like the idea of finding devices in weird >> places in the tree. > > But where is "weird". Arguably "/opal/flash" is weird. What does it > mean? There's a bus called "opal" and a device on it called "flash"? No. > > Point being the structure is fairly arbitrary, or at least debatable, so > tying the code 100% to the structure is inflexible. As we have discovered. > > Our other option is to tell skiboot to get stuffed, and leave the flash > node where it was on P8. > >> Then again, if we can't trust the DT we're in bigger >> trouble than erroneous flash nodes =). > > Quite :) > >> If we really just want to find compatible nodes anywhere, let's simplify i2c >> and pdev_init into one function and make that behavior consistent with this >> new patch. > > That seems OK to me. > > We should get an ack from Stewart though for the other node types. For finding nodes based on compatible no matter where they are in the tree, Acked-by: Stewart Smith <stewart@linux.vnet.ibm.com> (and yes, includes other nodes too) The exact location then isn't too important, and having a /flash that's ibm,opal-flash and allows for some other driver to bind to it I think is also something we shouldn't rule out.
diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c index ae29eaf..2847cb0 100644 --- a/arch/powerpc/platforms/powernv/opal.c +++ b/arch/powerpc/platforms/powernv/opal.c @@ -755,9 +755,14 @@ static int __init opal_init(void) /* Initialize platform devices: IPMI backend, PRD & flash interface */ opal_pdev_init(opal_node, "ibm,opal-ipmi"); - opal_pdev_init(opal_node, "ibm,opal-flash"); + opal_pdev_init(opal_node, "ibm,opal-flash"); // old <= P8 flash location opal_pdev_init(opal_node, "ibm,opal-prd"); + /* New >= P9 flash location */ + np = of_get_child_by_name(opal_node, "flash"); + if (np) + opal_pdev_init(np, "ibm,opal-flash"); + /* Initialise OPAL kmsg dumper for flushing console on panic */ opal_kmsg_init();
Skiboot will place the flash device tree node at ibm,opal/flash/flash@0 on P9 and later systems, so Linux needs to search for it there as well as ibm,opal/flash@0 for backwards compatibility. Signed-off-by: Jack Miller <jack@codezen.org> --- arch/powerpc/platforms/powernv/opal.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)