Re: [PATCH V2] drivers/mtd: add powernv flash MTD abstraction driver

[Cedric Le Goater]

Hello, 

On 05/28/2015 07:25 PM, Neelesh Gupta wrote:
 
 
 On 05/28/2015 06:36 PM, Cyril Bur wrote:
 Powerpc powernv platforms allow access to certain system flash devices
 through a firmwarwe interface. This change adds an mtd driver for these
 flash devices.

 Minor updates from Jeremy Kerr and Joel Stanley.

 Signed-off-by: Cyril Bur cyril...@gmail.com
 Signed-off-by: Joel Stanley j...@jms.id.au
 Signed-off-by: Jeremy Kerr j...@ozlabs.org
 ---
 V2: Address Brian Norris' review
 Fix typos
 Change from NAND flash type to NOR flash type
 Correctness tweaks
 ---
  drivers/mtd/devices/Kconfig |   8 +
  drivers/mtd/devices/Makefile|   1 +
  drivers/mtd/devices/powernv_flash.c | 286 
 
  3 files changed, 295 insertions(+)
  create mode 100644 drivers/mtd/devices/powernv_flash.c

 diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
 index c49d0b1..a8cc237 100644
 --- a/drivers/mtd/devices/Kconfig
 +++ b/drivers/mtd/devices/Kconfig
 @@ -195,6 +195,14 @@ config MTD_BLOCK2MTD
Testing MTD users (eg JFFS2) on large media and media that might
be removed during a write (using the floppy drive).

 +config MTD_POWERNV_FLASH
 +tristate powernv flash MTD driver
 +depends on PPC_POWERNV
 +help
 +  This provides an MTD device to access NVRAM on powernv OPAL
 
 NVRAM ? or to access FLASH device ?
 
 +  platforms from Linux. This device abstracts away the
 +  firmware interface for NVRAM access.
 +
  comment Disk-On-Chip Device Drivers

  config MTD_DOCG3
 diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
 index f0b0e61..7912d3a 100644
 --- a/drivers/mtd/devices/Makefile
 +++ b/drivers/mtd/devices/Makefile
 @@ -16,6 +16,7 @@ obj-$(CONFIG_MTD_SPEAR_SMI)+= spear_smi.o
  obj-$(CONFIG_MTD_SST25L)+= sst25l.o
  obj-$(CONFIG_MTD_BCM47XXSFLASH) += bcm47xxsflash.o
  obj-$(CONFIG_MTD_ST_SPI_FSM)+= st_spi_fsm.o
 +obj-$(CONFIG_MTD_POWERNV_FLASH) += powernv_flash.o


  CFLAGS_docg3.o  += -I$(src)
 diff --git a/drivers/mtd/devices/powernv_flash.c 
 b/drivers/mtd/devices/powernv_flash.c
 new file mode 100644
 index 000..f619e4a
 --- /dev/null
 +++ b/drivers/mtd/devices/powernv_flash.c
 @@ -0,0 +1,286 @@
 +/*
 + * OPAL PNOR flash MTD abstraction
 + *
 + * IBM 2015
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License as published by
 + * the Free Software Foundation; either version 2 of the License, or
 + * (at your option) any later version.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + */
 +
 +#include linux/kernel.h
 +#include linux/module.h
 +#include linux/errno.h
 +#include linux/of.h
 +#include linux/of_address.h
 +#include linux/platform_device.h
 +#include linux/string.h
 +#include linux/slab.h
 +#include linux/mtd/mtd.h
 +#include linux/mtd/partitions.h
 +
 +#include linux/debugfs.h
 +#include linux/seq_file.h
 +
 +#include asm/opal.h
 +
 +
 +/*
 + * This driver creates the a Linux MTD abstraction for platform PNOR flash
 + * backed by OPAL calls
 + */
 +
 +struct powernv_flash {
 +struct mtd_info mtd;
 +uint64_tid;
 
 'id' should be u32
 
 +};
 +
 +enum flash_op {
 +FLASH_OP_READ,
 +FLASH_OP_WRITE,
 +FLASH_OP_ERASE,
 +};
 +
 +static int powernv_flash_async_op(struct mtd_info *mtd, enum flash_op op,
 +loff_t offset, size_t len, size_t *retlen, u_char *buf)
 +{
 +struct powernv_flash *info = (struct powernv_flash *)mtd-priv;
 +struct device *dev = mtd-dev;
 +int token;
 +struct opal_msg msg;
 +int rc;
 +
 +dev_dbg(dev, %s(op=%d, offset=0x%llx, len=%zu)\n,
 +__func__, op, offset, len);
 +
 +token = opal_async_get_token_interruptible();
 +if (token  0) {
 +dev_err(dev, Failed to get an async token\n);
 +return -ENOMEM;
 
 ENOMEM is not correct code here.. 'token' itself has the right one in case of
 failure. To be more precise,
 if (token  0) {
 if (token != -ERESTARTSYS)
 dev_err(dev, Failed to get an async token\n);
 
 return token;
 }
 
 +}
 +
 +switch (op) {
 +case FLASH_OP_READ:
 +rc = opal_flash_read(info-id, offset, __pa(buf), len, token);
 +break;
 +case FLASH_OP_WRITE:
 +rc = opal_flash_write(info-id, offset, __pa(buf), len, token);
 +break;
 +case FLASH_OP_ERASE:
 +rc = opal_flash_erase(info-id, offset, len, token);
 +break;
 +default:
 +BUG_ON(1);
 +}
 +
 +if (rc != OPAL_ASYNC_COMPLETION) {
 +dev_err(dev, opal_flash_async_op(op=%d) failed (rc %d)\n,
 +

Re: [PATCH 4/4] hwmon: (ibmpowernv) pretty print labels

[Cedric Le Goater]

Hello Guenter,

On 04/07/2015 09:22 PM, Guenter Roeck wrote:
 Hi Cedric,
 
 On Tue, Apr 07, 2015 at 08:03:46PM +0200, Cedric Le Goater wrote:

 on a P7  :

  # ppc64_cpu --info
  Core   0:0*1*2*3* 
  Core   1:4*5*6*7* 
  Core   2:8*9*   10*   11* 
  Core   3:   12*   13*   14*   15* 
  Core   4:   16*   17*   18*   19* 
  Core   5:   20*   21*   22*   23* 

 How would the 'sensors' output look like on that system ?
 Wouldn't it be something like the following ?
 
   Core 0-7:+29.0°C  
   Core 4-11:   +29.0°C  

yep. 

 Also, how do you know that the range of CPU IDs is always 8 ?

 This is a shortcut. The code is for the ibmpowernv platform and assumes 
 that we are running on a P8 (8 hardware threads). It would be better to 
 use a maximum threads per core variable but I am not sure this is 
 available, as it is a tunable. I will look into it.

 Tunable how ? 

You can switch on and off threads.

 The core code must have some means to detect this number when it initialized 
 CPU entries, or am I missing something ?

threads_per_core is what the code needs. v3 is on its way.

Thanks !

C.




___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 2/4] hwmon: (ibmpowernv) add support for the new device tree

[Cedric Le Goater]

On 04/08/2015 05:20 PM, Guenter Roeck wrote:
 On Wed, Apr 01, 2015 at 12:15:04PM +0200, Cédric Le Goater wrote:
 The new OPAL device tree for sensors has a different layout and uses new
 property names, for the type and for the handler used to capture the
 sensor data.

 This patch modifies the ibmpowernv driver to support such a tree in a
 way preserving compatibility with older OPAL firmwares.

 This is achieved by changing the error path of the routine parsing
 an OPAL node name. The node is simply considered being from the new
 device tree layout and fallback values are used.

 Signed-off-by: Cédric Le Goater c...@fr.ibm.com
 
 Hi Cedric,
 
 I was about to apply the series, but then I found the following problem.
 
 ---
  drivers/hwmon/ibmpowernv.c |   47 
 +++-
  1 file changed, 38 insertions(+), 9 deletions(-)

 [ ... ]
  
 @@ -189,11 +204,16 @@ static u32 get_sensor_hwmon_index(struct sensor_data 
 *sdata,
  {
  int i;
  
 -for (i = 0; i  count; i++)
 -if (sdata_table[i].opal_index == sdata-opal_index 
 -sdata_table[i].type == sdata-type)
 -return sdata_table[i].hwmon_index;
 +/*
 + * We don't use the OPAL index on newer device trees
 + */
 +if (sdata-opal_index != -1) {
 
 opal_index is u32, so this won't work (or at least the result is
 unpredictable).
 
 Also, in patch 4/4 (v4), get_logical_cpu() takes unsigned int as parameter,
 but get_hard_smp_processor_id() returns an int, causing gcc to complain
 if the code is built with W=1.
 
 Please fix and resubmit the entire series.
 
 When you do that, please also ensure that continuation lines
 are aligned (in patch 3/4).

Sure. Working on it right now.

Thanks,

C. 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v5 0/4] hwmon: (ibmpowernv) add DTS support

[Cedric Le Goater]

On 04/08/2015 07:32 PM, Guenter Roeck wrote:
 On Wed, Apr 08, 2015 at 07:19:46PM +0200, Cédric Le Goater wrote:
 Hello !

 These patches extend the ibmpowernv driver to support the new OPAL firmware 
 which now exposes in its device tree the Digital Temperature Sensors of 
 a P8 system.

 They are based on Linux 4.0.0-rc7 + the initial cleanup serie :

  http://lists.lm-sensors.org/pipermail/lm-sensors/2015-March/043670.html

 and were tested on IBM Power and Open Power systems running Trusty.

 Series applied to -next.

Thanks for the review ! 

Cheers

C.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v3] hwmon: (ibmpowernv) pretty print labels

[Cedric Le Goater]

On 04/08/2015 03:26 PM, Guenter Roeck wrote:
 On 04/08/2015 12:00 AM, Cédric Le Goater wrote:
 The new OPAL device tree adds a few properties which can be used to add
 extra information on the sensor label.

 In the case of a cpu core sensor, the firmware exposes the physical
 identifier of the core in the ibm,pir property. The driver
 translates this identifier in a linux cpu number and prints out a
 range corresponding to the hardware threads of the core (as they
 share the same sensor).

 The numbering gives a hint on the localization of the core in the
 system (which socket, which chip).

 Signed-off-by: Cédric Le Goater c...@fr.ibm.com
 ---

   Changes since v2:

   - fix bogus logical cpu retrieval
   - use 'threads_per_core' to print out cpu range

   Changes since v1:

   - check cpu validity before printing out the attribute label.
 if invalid, use a phy prefix to distinguish a linux cpu
 number from a physical cpu number.

   drivers/hwmon/ibmpowernv.c |   43 
 +++
   1 file changed, 43 insertions(+)

 Index: linux.git/drivers/hwmon/ibmpowernv.c
 ===
 --- linux.git.orig/drivers/hwmon/ibmpowernv.c
 +++ linux.git/drivers/hwmon/ibmpowernv.c
 @@ -30,6 +30,7 @@
   #include linux/platform_device.h
   #include asm/opal.h
   #include linux/err.h
 +#include asm/cputhreads.h

   #define MAX_ATTR_LEN32
   #define MAX_LABEL_LEN64
 @@ -110,12 +111,54 @@ static ssize_t show_label(struct device
   return sprintf(buf, %s\n, sdata-label);
   }

 +static int __init get_logical_cpu(unsigned int hwcpu)
 +{
 +int cpu;
 +
 +for_each_possible_cpu(cpu)
 +if (get_hard_smp_processor_id(cpu) == hwcpu)
 +return cpu;
 +
 +pr_err(%s: could not find a cpu with physical id 0x%x\n,
 +   __func__, hwcpu);
 
 I like moving this into a function, but I really dislike this error
 message. If the devicetree data is wrong/bad, the log and the console
 will be clogged with that message. And the user will not be able
 to do anything about it.

yes. It is not super useful anyway and it is redundant with the phy 
prefix below. Do you want me to kill it in a v4 ? 

Thanks,

C. 

 
 Guenter
 
 +return -ENOENT;
 +}
 +
   static void __init make_sensor_label(struct device_node *np,
   struct sensor_data *sdata, const char *label)
   {
 +u32 id;
   size_t n;

   n = snprintf(sdata-label, sizeof(sdata-label), %s, label);
 +
 +/*
 + * Core temp pretty print
 + */
 +if (!of_property_read_u32(np, ibm,pir, id)) {
 +int cpuid = get_logical_cpu(id);
 +
 +if (cpuid = 0)
 +/*
 + * The digital thermal sensors are associated
 + * with a core. Let's print out the range of
 + * cpu ids corresponding to the hardware
 + * threads of the core.
 + */
 +n += snprintf(sdata-label + n,
 +  sizeof(sdata-label) - n,  %d-%d,
 +  cpuid, cpuid + threads_per_core - 1);
 +else
 +n += snprintf(sdata-label + n,
 +  sizeof(sdata-label) - n,  phy%d, id);
 +}
 +
 +/*
 + * Membuffer pretty print
 + */
 +if (!of_property_read_u32(np, ibm,chip-id, id))
 +n += snprintf(sdata-label + n, sizeof(sdata-label) - n,
 +   %d, id  0x);
   }

   static int get_sensor_index_attr(const char *name, u32 *index,


 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 4/4] hwmon: (ibmpowernv) pretty print labels

[Cedric Le Goater]

On 04/03/2015 05:49 PM, Guenter Roeck wrote:
 On 04/01/2015 03:15 AM, Cédric Le Goater wrote:
 The new OPAL device tree adds a few properties which can be used to add
 extra information on the sensor label.

 Signed-off-by: Cédric Le Goater c...@fr.ibm.com
 ---
   drivers/hwmon/ibmpowernv.c |   22 ++
   1 file changed, 22 insertions(+)

 diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
 index be6fe559b52a..3e753c215b40 100644
 --- a/drivers/hwmon/ibmpowernv.c
 +++ b/drivers/hwmon/ibmpowernv.c
 @@ -113,9 +113,31 @@ static ssize_t show_label(struct device *dev, struct 
 device_attribute *devattr,
   static void __init make_sensor_label(struct device_node *np,
   struct sensor_data *sdata, const char *label)
   {
 +u32 id;
   size_t n;

   n = snprintf(sdata-label, sizeof(sdata-label), %s, label);
 +
 +/*
 + * Core temp pretty print
 + */
 +if (!of_property_read_u32(np, ibm,pir, id)) {
 +int i;
 +
 +for_each_possible_cpu(i)
 +if (paca[i].hw_cpu_id == id)
 +break;
 +
 +n += snprintf(sdata-label + n, sizeof(sdata-label) - n,
 +   %d-%d, i, i+7);
 
 If ibm,pir points to a bad/invalid CPU id you just print an invalid value.
 Is that what you want ? 

Certainly not :) I am being over optimistic on the underlying layer. 

 Also, what relevance does 'i' have for the user ?
 It is the index into the paca array, sure, but what is its relevance
 outside this code, especially in the context of you printing both i and i+7 ?

It gives a hint on the localization of the core in the system, which
can be useful when developing custom heat sinks. The translation 
from physical to linux cpu id is here to be consistent with the user
layer. The physical id is rarely used at that level.  

I will send a v2 for this patch.

Thanks,

C.
 

 
 Guenter
 
 +}
 +
 +/*
 + * Membuffer pretty print
 + */
 +if (!of_property_read_u32(np, ibm,chip-id, id))
 +n += snprintf(sdata-label + n, sizeof(sdata-label) - n,
 +   %d, id  0x);
   }

   static int get_sensor_index_attr(const char *name, u32 *index,

 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 4/4] hwmon: (ibmpowernv) pretty print labels

[Cedric Le Goater]

Hello Guenter,

On 04/07/2015 06:44 PM, Guenter Roeck wrote:
 On Tue, Apr 07, 2015 at 04:45:56PM +0200, Cédric Le Goater wrote:
 The new OPAL device tree adds a few properties which can be used to add
 extra information on the sensor label.

 In the case of a cpu core sensor, the firmware exposes the physical 
 identifier of the core in the ibm,pir property. The driver 
 translates this identifier in a linux cpu number and prints out a 
 range corresponding to the hardware threads of the core (as they
 share the same sensor).

 The numbering gives a hint on the localization of the core in the 
 system (which socket, which chip). 

 Signed-off-by: Cédric Le Goater c...@fr.ibm.com
 
 Hi Cedric,
 
 Please do not send follow-up patches as reply, but as separate e-mail.

Ok. I will start a new thread when I resend this patch.

 Further comments below.
 
 ---

  Changes since v1:

  - check cpu validity before printing out the attribute label. 
if invalid, use a phy prefix to distinguish a linux cpu 
number from a physical cpu number. 

  drivers/hwmon/ibmpowernv.c |   34 ++
  1 file changed, 34 insertions(+)

 Index: linux.git/drivers/hwmon/ibmpowernv.c
 ===
 --- linux.git.orig/drivers/hwmon/ibmpowernv.c
 +++ linux.git/drivers/hwmon/ibmpowernv.c
 @@ -113,9 +113,43 @@ static ssize_t show_label(struct device
  static void __init make_sensor_label(struct device_node *np,
  struct sensor_data *sdata, const char *label)
  {
 +u32 id;
  size_t n;
  
  n = snprintf(sdata-label, sizeof(sdata-label), %s, label);
 +
 +/*
 + * Core temp pretty print
 + */
 +if (!of_property_read_u32(np, ibm,pir, id)) {
 +int i = -1;
 +
 +for_each_possible_cpu(i)
 +if (paca[i].hw_cpu_id == id)
 
 I think you should consider using get_hard_smp_processor_id() and avoid
 direct access to the paca array.

Yes. It will look better. Thanks.

 +break;
 +
 +if (i != -1)
 
 I don't think that works. i is initialized by for_each_possible_cpu(),
 either to -1 or 0 depending on the state of CONFIG_SMP. So pre-initializing
 it won't make a difference. Its last value is NR_CPUS (SMP) or 1 (non-SMP).
 
 You would need a second variable (which you could pre-initialize)
 to determine if the code found a matching ID or not.

gloups. I did that patch wy too quickly, it is completely bogus ... 
I will cook a new version. sorry for the noise. 

 +/*
 + * The digital thermal sensors are associated
 + * with a core. Let's print out the range of
 + * cpu ids corresponding to the hardware
 + * threads of the core.
 + */
 +n += snprintf(sdata-label + n,
 +  sizeof(sdata-label) - n,
 +   %d-%d, i, i+7);
 
 I would really like to see how this looks like in practice.
 
 The id in the devicetree entry is the physical CPU. The resulting index
 is the logical CPU number. So let's assume that the logical CPU number
 for the first physical CPU is 0. Output would be 0-7. If the second
 physical CPU matches logical CPU 1, output would be 1-8. 
 How does that make any sense ?

The logical CPU numbers on PowerPC are initialized looping on the cores
found in the device tree and then on the threads of the core. Something 
like :

logical_cpu = core_index * max_threads_per_core + thread_index 

which gives on a P8  :

# ppc64_cpu --info
Core   0:0*1*2*3*4*5*6*7* 
Core   1:8*9*   10*   11*   12*   13*   14*   15* 
Core   2:   16*   17*   18*   19*   20*   21*   22*   23* 
Core   3:   24*   25*   26*   27*   28*   29*   30*   31* 
Core   4:   32*   33*   34*   35*   36*   37*   38*   39* 
Core   5:   40*   41*   42*   43*   44*   45*   46*   47* 
Core   6:   48*   49*   50*   51*   52*   53*   54*   55* 
Core   7:   56*   57*   58*   59*   60*   61*   62*   63* 
Core   8:   64*   65*   66*   67*   68*   69*   70*   71* 
Core   9:   72*   73*   74*   75*   76*   77*   78*   79* 
Core  10:   80*   81*   82*   83*   84*   85*   86*   87* 
Core  11:   88*   89*   90*   91*   92*   93*   94*   95* 
Core  12:   96*   97*   98*   99*  100*  101*  102*  103* 
Core  13:  104*  105*  106*  107*  108*  109*  110*  111* 
Core  14:  112*  113*  114*  115*  116*  117*  118*  119* 
Core  15:  120*  121*  122*  123*  124*  125*  126*  127* 
Core  16:  128*  129*  130*  131*  132*  133*  134*  135* 
Core  17:  136*  137*  138*  139*  140*  141*  142*  143* 
Core  18:  144*  145*  146*  147*  148*  149*  150*  151* 
Core  19:  152*  153*  154*  155*  156* 

Re: [PATCH v3 1/3] powerpc/powernv: convert codes returned by OPAL calls

[Cedric Le Goater]

On 03/30/2015 04:05 AM, Michael Ellerman wrote:
 On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote:
 OPAL has its own list of return codes. The patch provides a translation
 of such codes in errnos for the opal_sensor_read call, and possibly 
 others if needed.

 Index: linux.git/arch/powerpc/platforms/powernv/opal.c
 ===
 --- linux.git.orig/arch/powerpc/platforms/powernv/opal.c
 +++ linux.git/arch/powerpc/platforms/powernv/opal.c
 @@ -894,6 +894,23 @@ void opal_free_sg_list(struct opal_sg_li
  }
  }
  
 +int opal_error_code(int rc)
 +{
 +switch (rc) {
 +case OPAL_SUCCESS:  return 0;
 
 Obviously correct.

He. Initially, I didn't put a case for SUCCESS, but we have code doing :

ret = be64_to_cpu(msg.params[1]);


 +case OPAL_PARAMETER:return -EINVAL;
 
 Yep.
 
 +case OPAL_UNSUPPORTED:  return -ENOSYS;
 
 You shouldn't use ENOSYS here, that should only ever mean no such syscall,
 otherwise you get very confusing results like read() returning ENOSYS.

Indeed. How about ENODEV then ? 

 +case OPAL_ASYNC_COMPLETION: return -EAGAIN;
 
 EAGAIN means try what you did again, I don't think that's what
 ASYNC_COMPLETION means, is it? It looks like it means, don't try again, but
 you need to wait for the result to be ready.
 
 I'm not sure it maps well to any of the Linux codes, maybe EINPROGRESS ?

Yes. This is better.

 +case OPAL_BUSY_EVENT:   return -EBUSY;
 
 Yep.
 
 +case OPAL_NO_MEM:   return -ENOMEM;
 
 Yep.
 
 +case OPAL_HARDWARE: return -ENOENT;
 
 This is another one which I think you shouldn't use as it can lead to 
 confusing
 results at user level. eg:
 
   $ cat /sysfs/some/file
   Error: No such file or directory
 
 Huh?
 
 Looking at the skiboot code this looks like EIO is a good match.

ok. 

 +case OPAL_INTERNAL_ERROR:   return -EIO;
 
 Yeah as good as anything I guess.
 
 +default:
 +pr_err(%s: unexpected OPAL error %d\n, __func__, rc);
 +return -ERANGE;
 
 I'm not sure about this one honestly, it means Math result not 
 representable.
 
 I suspect the reason RTAS chose it was just that it's not EINVAL.
 
 This should probably also just be EIO.

ok. I will change it. 

Thanks,

C.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v3 1/3] powerpc/powernv: convert codes returned by OPAL calls

[Cedric Le Goater]

On 03/30/2015 08:54 AM, Michael Ellerman wrote:
 On Mon, 2015-03-30 at 08:37 +0200, Cedric Le Goater wrote:
 On 03/30/2015 04:05 AM, Michael Ellerman wrote:
 On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote:
 OPAL has its own list of return codes. The patch provides a translation
 of such codes in errnos for the opal_sensor_read call, and possibly 
 others if needed.

 +  case OPAL_UNSUPPORTED:  return -ENOSYS;

 You shouldn't use ENOSYS here, that should only ever mean no such syscall,
 otherwise you get very confusing results like read() returning ENOSYS.

 Indeed. How about ENODEV then ? 
 
 That can also be confusing from userspace.
 
 I think it's probably best just to use EIO, as far as userspace is concerned 
 if
 the kernel lets it call an unsupported OPAL routine that is more or less a
 kernel bug.

OK. Will do.

Thanks,

C.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v3 3/3] powerpc/powernv: remove opal_sensor_mutex

[Cedric Le Goater]

On 03/30/2015 04:09 AM, Michael Ellerman wrote:
 On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote:
 The opal sensor mutex protects the opal_sensor_read call which
 can return a OPAL_BUSY code on IBM Power systems if a previous 
 request is in progress.

 This can be handled at user level with a retry.
 
 It can, but how does it actually look in practice?
 
 It looks like the only use of opal_get_sensor_data() is show_sensor() in
 drivers/hwmon/ibmpowernv.c.
 
 Because that's a sysfs attribute folks will be generally just dumping 
 that with cat, or reading it in a shell script, neither of which will 
 cope nicely with EBUSY I think?

It won't, I agree but it should only happen when running concurrent cat 
commands on the hwmon sysfs files. The event should be rare enough.

Anyhow, this is not a big issue. We can drop that patch. The real issue
is the time it takes to get some values back from the FSP. This is what
user space has been most surprised about.

Thanks,

C.


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v3 3/3] powerpc/powernv: remove opal_sensor_mutex

[Cedric Le Goater]

On 03/30/2015 08:59 AM, Michael Ellerman wrote:
 On Mon, 2015-03-30 at 08:51 +0200, Cedric Le Goater wrote:
 On 03/30/2015 04:09 AM, Michael Ellerman wrote:
 On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote:
 The opal sensor mutex protects the opal_sensor_read call which
 can return a OPAL_BUSY code on IBM Power systems if a previous 
 request is in progress.

 This can be handled at user level with a retry.

 It can, but how does it actually look in practice?

 It looks like the only use of opal_get_sensor_data() is show_sensor() in
 drivers/hwmon/ibmpowernv.c.

 Because that's a sysfs attribute folks will be generally just dumping 
 that with cat, or reading it in a shell script, neither of which will 
 cope nicely with EBUSY I think?

 It won't, I agree but it should only happen when running concurrent cat 
 commands on the hwmon sysfs files. The event should be rare enough.
 
 Rare enough maybe, but a real pain in the .. to cope with in a shell script if
 you're trying to automate something.
 
 Anyhow, this is not a big issue. We can drop that patch. The real issue
 is the time it takes to get some values back from the FSP. This is what
 user space has been most surprised about.
 
 OK. The other option would be to move the mutex into the sysfs show routine, 
 so
 only that is synchronous. That would give you nice behaviour from cat, ie. it
 would sleep on contention but still be killable with ctrl-c.

Let's keep it how it is and see if it is possible to the improve OPAL 
side first. 

I will send you an updated patchset shortly. 

Thanks for the review. 

C.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [Skiboot] [v2, 1/3] powerpc/powernv: convert codes returned by OPAL calls

[Cedric Le Goater]

On 03/27/2015 11:36 AM, Benjamin Herrenschmidt wrote:
 On Fri, 2015-03-27 at 20:59 +1100, Michael Ellerman wrote:

 Can you put it in opal.h and give it a better name, maybe
 opal_error_code() ?
 
 Do we want it to be inlined all the time ? Feels more like something we
 should have in opal.c
 
 Also we only want to call it when we forward the error code up the
 food chain, there are a number of cases where we look for specific OPAL
 error codes.

yes. the forward is not systematic. opal.c looks like a better place.

-ERANGE looks also better when the return code is unexpected. 

C.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [v2,1/3] powerpc/powernv: convert codes returned by OPAL calls

[Cedric Le Goater]

On 03/27/2015 10:59 AM, Michael Ellerman wrote:
 On Thu, 2015-26-03 at 16:04:45 UTC, =?utf-8?q?C=C3=A9dric_Le_Goater?= wrote:
 OPAL has its own list of return codes. The patch provides a translation
 of such codes in errnos for the opal_sensor_read call.

 Signed-off-by: Cédric Le Goater c...@fr.ibm.com
 ---
  arch/powerpc/platforms/powernv/opal-sensor.c |   37 
 ++-
  1 file changed, 36 insertions(+), 1 deletion(-)

 Index: linux.git/arch/powerpc/platforms/powernv/opal-sensor.c
 ===
 --- linux.git.orig/arch/powerpc/platforms/powernv/opal-sensor.c
 +++ linux.git/arch/powerpc/platforms/powernv/opal-sensor.c
 @@ -26,6 +26,38 @@
   
 +static int convert_opal_code(int ret)
 +{
 +switch (ret) {
 +case OPAL_SUCCESS:  return 0;
 +case OPAL_PARAMETER:return -EINVAL;
 +case OPAL_UNSUPPORTED:  return -ENOSYS;
 +case OPAL_ASYNC_COMPLETION: return -EAGAIN;
 +case OPAL_BUSY_EVENT:   return -EBUSY;
 +case OPAL_NO_MEM:   return -ENOMEM;
 +case OPAL_HARDWARE: return -ENOENT;
 +case OPAL_INTERNAL_ERROR:   return -EIO;
 +default:return -EIO;
 +}
 +}
 
 That looks a bit familiar :)

Ah ! I only looked in opal ...

 static int rtas_error_rc(int rtas_rc)
 {
   int rc;
 
   switch (rtas_rc) {
   case -1:/* Hardware Error */
   rc = -EIO;
   break;
   case -3:/* Bad indicator/domain/etc */
   rc = -EINVAL;
   break;
   case -9000: /* Isolation error */
   rc = -EFAULT;
   break;
   case -9001: /* Outstanding TCE/PTE */
   rc = -EEXIST;
   break;
   case -9002: /* No usable slot */
   rc = -ENODEV;
   break;
   default:
   printk(KERN_ERR %s: unexpected RTAS error %d\n,
   __func__, rtas_rc);
   rc = -ERANGE;

this a better code default value.


   break;
   }
   return rc;
 }
 
 
 But I guess we still should have it.
 
 Can you put it in opal.h and give it a better name, maybe opal_error_code() ?

Sure. I will change the name but opal.c looks better, knowing that opal.h is 
shared with skiboot.

 
  /*
   * This will return sensor information to driver based on the requested 
 sensor
   * handle. A handle is an opaque id for the powernv, read by the driver 
 from the
 @@ -46,8 +78,10 @@ int opal_get_sensor_data(u32 sensor_hndl
  
  mutex_lock(opal_sensor_mutex);
  ret = opal_sensor_read(sensor_hndl, token, data);
 -if (ret != OPAL_ASYNC_COMPLETION)
 +if (ret != OPAL_ASYNC_COMPLETION) {
 +ret = convert_opal_code(ret);
  goto out_token;
 +}
  
  ret = opal_async_wait_response(token, msg);
  if (ret) {
 @@ -58,6 +92,7 @@ int opal_get_sensor_data(u32 sensor_hndl
  
  *sensor_data = be32_to_cpu(data);
  ret = be64_to_cpu(msg.params[1]);
 +ret = convert_opal_code(ret);
 
 I'd do:
   ret = convert_opal_code(be64_to_cpu(msg.params[1]));

Yes. the double 'ret =' is ugly.

Thanks,

C. 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] powerpc/powernv: handle OPAL_SUCCESS return in opal_sensor_read

[Cedric Le Goater]

On 03/26/2015 12:07 AM, Stewart Smith wrote:
 Cédric Le Goater c...@fr.ibm.com writes:
 Currently, when a sensor value is read, the kernel calls OPAL, which in
 turn builds a message for the FSP, and waits for a message back. 

 The new device tree for OPAL sensors [1] adds new sensors that can be 
 read synchronously (core temperatures for instance) and that don't need 
 to wait for a response.

 This patch modifies the opal call to accept an OPAL_SUCCESS return value
 and cover the case above.

 [1] https://lists.ozlabs.org/pipermail/skiboot/2015-March/000639.html

 Signed-off-by: Cédric Le Goater c...@fr.ibm.com
 ---

  We still uselessly reserve a token (for the response) and take a
  lock, which might raise the need of a new 'opal_sensor_read_sync' 
  call.
 
 Actually why do we take a lock around the OPAL calls at all?

The sensor service in OPAL only handles one FSP request at a time and 
returns OPAL_BUSY if one is already in progress. The lock covers this case 
but we could also remove it return EBUSY to the driver or even retry the 
call. That might be dangerous though. 

Changing OPAL to handle simultaneously multiple requests does not seem really 
necessary, it won't speed up the communication with the FSP and that is the
main bottleneck.

C.


 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] powerpc/powernv: handle OPAL_SUCCESS return in opal_sensor_read

[Cedric Le Goater]

On 03/26/2015 10:44 AM, Cedric Le Goater wrote:
 On 03/26/2015 12:07 AM, Stewart Smith wrote:
 Cédric Le Goater c...@fr.ibm.com writes:
 Currently, when a sensor value is read, the kernel calls OPAL, which in
 turn builds a message for the FSP, and waits for a message back. 

 The new device tree for OPAL sensors [1] adds new sensors that can be 
 read synchronously (core temperatures for instance) and that don't need 
 to wait for a response.

 This patch modifies the opal call to accept an OPAL_SUCCESS return value
 and cover the case above.

 [1] https://lists.ozlabs.org/pipermail/skiboot/2015-March/000639.html

 Signed-off-by: Cédric Le Goater c...@fr.ibm.com
 ---

  We still uselessly reserve a token (for the response) and take a
  lock, which might raise the need of a new 'opal_sensor_read_sync' 
  call.

 Actually why do we take a lock around the OPAL calls at all?
 
 The sensor service in OPAL only handles one FSP request at a time and 
 returns OPAL_BUSY if one is already in progress. The lock covers this case 
 but we could also remove it return EBUSY to the driver or even retry the 
 call. That might be dangerous though. 
 
 Changing OPAL to handle simultaneously multiple requests does not seem really 
 necessary, it won't speed up the communication with the FSP and that is the
 main bottleneck.

opal_get_sensor_data() is mixing OPAL return codes and errnos. I will send
a v2 addressing this problem first.

C. 


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 4/5] hwmon: (ibmpowernv) change create_hwmon_attr_name() prototype

[Cedric Le Goater]

[ ... ] 


 @@ -265,10 +261,17 @@ static int create_device_attrs(struct pl
 
   sdata[count].id = sensor_id;
   sdata[count].type = type;
 - err = create_hwmon_attr_name(pdev-dev, type, np-name,
 -  sdata[count].name);
 - if (err)
 +
 + attr_name = parse_opal_node_name(np-name, type, opal_index);
 + if (IS_ERR(attr_name)) {
 + dev_err(pdev-dev, Sensor device node name '%s' is 
 invalid\n,
 + np-name);
 + err = IS_ERR(attr_name);
  ^^

Arg. Bad copy/paste. This should be PTR_ERR() ... 

Do you want a full patchset resend or just a resend of this patch ? or a fix 
maybe.

Sorry for the noise ...

C.  

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 0/5] hwmon: (ibmpowernv) remove dependency on OPAL index

[Cedric Le Goater]

On 03/20/2015 04:26 PM, Guenter Roeck wrote:
 On Thu, Mar 19, 2015 at 06:44:40PM +0100, Cédric Le Goater wrote:
 Hello !

 The current implementation of the driver uses an index for the hwmon 
 attribute which is extracted from the device node name. This index
 is calculated by the OPAL firmware and its usage creates a dependency 
 with the driver which makes changes a little more complex in OPAL.

 This patchset changes the ibmpowernv code to use its own index. It 
 starts with a few cleanups, mostly code shuffling around the creation 
 of the hwmon sysfs attributes and completes by removing the dependency.

 Series applied to hwmon-next (with patch #4 fixed up).

Thanks !

C.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [RFC PATCH 1/3] powerpc/powernv: Check OPAL sensor calls exist

[Cedric Le Goater]

On 02/24/2015 05:54 AM, Michael Ellerman wrote:
 On Fri, 2015-02-20 at 16:07 +0100, Cédric Le Goater wrote:
 Signed-off-by: Cédric Le Goater c...@fr.ibm.com
 ---
  arch/powerpc/platforms/powernv/opal-sensor.c |3 +++
  1 file changed, 3 insertions(+)

 diff --git a/arch/powerpc/platforms/powernv/opal-sensor.c 
 b/arch/powerpc/platforms/powernv/opal-sensor.c
 index 4ab67ef7abc9..544292f2020f 100644
 --- a/arch/powerpc/platforms/powernv/opal-sensor.c
 +++ b/arch/powerpc/platforms/powernv/opal-sensor.c
 @@ -72,6 +72,9 @@ static __init int opal_sensor_init(void)
  struct platform_device *pdev;
  struct device_node *sensor;
  
 +if (!opal_check_token(OPAL_SENSOR_READ))
 +return -ENODEV;
 +
  sensor = of_find_node_by_path(/ibm,opal/sensors);
  if (!sensor) {
  pr_err(Opal node 'sensors' not found\n);
 
 Are you actually seeing this in practice?

No. Not this one. I have seen others though. I will send you patches.

 It's a bit annoying that we have to check for the token, and then also check
 the device tree. It would be nice if one implied the presence of the other.

Should we expose the OPAL call token in the device tree ? 

Cheers,

C. 
 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] powerpc/powernv: check OPAL_REGISTER_DUMP_REGION calls exist

[Cedric Le Goater]

On 02/25/2015 10:16 PM, Stewart Smith wrote:
 Cédric Le Goater c...@fr.ibm.com writes:
 On Open Power systems, such call fails in OPAL : 

OPAL: Called with bad token 101 !

 The check on the OPAL_UNREGISTER_DUMP_REGION call is added for 
 symmetry. I did not see any errors for it.
 
 I've already put in a patch to squash this:
 Message-Id:
 1423718729-17992-2-git-send-email-stew...@linux.vnet.ibm.com
 
 Just waiting on mpe to merge :)

ok fine, I missed it :) 

I am also tempted to fix OPAL_READ_TPO but the proper way to do this 
would be to move the lowlevel OPAL code from drivers/rtc/rtc-opal.c to 
a new file under arch/powerpc/platforms/powernv/. How does that sound ? 

Cheers,

C.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [RFC PATCH 0/3] hwmon: (ibmpowernv) add DTS support

[Cedric Le Goater]

On 02/21/2015 12:03 PM, Guenter Roeck wrote:
 On 02/20/2015 11:14 PM, Cedric Le Goater wrote:
 On 02/21/2015 12:52 AM, Guenter Roeck wrote:
 On 02/20/2015 12:15 PM, Cedric Le Goater wrote:
 On 02/20/2015 05:52 PM, Guenter Roeck wrote:
 On Fri, Feb 20, 2015 at 04:07:34PM +0100, Cédric Le Goater wrote:
 Hello !

 These patches rework the ibmpowernv driver to support the new device
 tree as proposed by this patchset on the skiboot mailing list :

 https://lists.ozlabs.org/pipermail/skiboot/2015-February/000457.html

 They are based on Linux 3.19 and were tested on IBM Power and Open Power
 systems running trusty.

 The main issue is that the new device tree is incompatible with the
 previous ibmpowernv drivers. The consequence is no powernv sensors
 on systems with such a opal/linux configuration.

 I don't think that would be acceptable. There must be lots of such
 systems out there. Why does it have to be incompatible ?
 Can't it support both the old and new versions ?

 I should have provided more explanation in the Linux patchset. Sorry
 for that. Here is the rationale behind this brutal code change.

 The initial ibmpowernv driver was designed in the early days of the
 powernv platform and the device tree it is using to expose the sensors
 has some limitations that makes it difficult to add new ones. The current
 layout of the device tree is also tightly coupled to IBM Power systems
 and their service processor (FSP). Open Power systems are different and
 need a different solution.

 It is to get more sensors out the P8 (and there are quite a few) that
 the OPAL patchset [1] proposes a new device tree. On the Linux side, it
 feels simpler to make a jump forward and break the compatibility than
 to maintain multiple branches of code just to keep alive an early v1
 of the ibmpowernv driver.


 Would it possibly be appropriate to write a different driver for the new
 device tree ?

 Sure. That is an option.

 There are no conflicts between the trees so we can live with two drivers
 using the same sensors/ root node. With time we will deprecate the initial
 one.

 You lost me a bit. Are you saying you are going to replace the devicetree
 properties with something that is incompatible but retain the same
 compatible properties ? If so, how do you expect this to work ?
 How do you expect to be able to determine which version of devicetree
 is loaded, and thus which driver is needed ?

 I'll have to understand this way better. The link above doesn't explain
 what the differences are, nor how the driver is supposed to know what
 to do.
 
Sure. My bad. I did not provide enough information in this RFC. Thanks for
your patience ! 


The current hwmon driver ibmpowernv relies on a device tree provided by 
the OPAL firmware. Today, this tree has the following layout :

ibm,opal/sensors/
|-- amb-temp#1-data
|   |-- compatible
|   |-- linux,phandle
|   |-- name
|   |-- phandle
|   `-- sensor-id
|-- amb-temp#1-thrs
|   |-- compatible
|   |-- linux,phandle
|   |-- name
|   |-- phandle
|   `-- sensor-id
|-- cooling-fan#10-data
|   |-- compatible
|   |-- linux,phandle
|   |-- name
|   |-- phandle
|   `-- sensor-id
|-- cooling-fan#10-faulted
|   |-- compatible
|   |-- linux,phandle
|   |-- name
|   |-- phandle
|   `-- sensor-id
|-- cooling-fan#10-thrs
|   |-- compatible
|   |-- linux,phandle
|   |-- name
|   |-- phandle
|   `-- sensor-id
...

It has some limitations and we want to change it to something more flexible, 
giving us the ability to add new sensors. The new device tree layout is 
described here :

https://lists.ozlabs.org/pipermail/skiboot/2015-February/000457.html


Both layouts use the same root node ibm,opal/sensors but the underlying
nodes have nothing in common, which means that the current driver ibmpowernv
will not 'see' any sensors on a system with an OPAL firmware exposing the new 
device tree. One will need new code for it. 

We have a few options to add support for this new tree :

 1. modify the current driver to parse the two trees. It seems overly complex 
and the resulting code will be a pain to maintain. 

 2. add a new driver. As the two drivers can cohabitate, it is a viable 
solution.
We will let the old driver rot in its corner and deprecate it one day. Kind
of ugly to keep this code around.

 3. replace the current driver with a new one. The simplest and most brutal but 
it also means we loose sensor support for IBM P8 systems running the old 
OPAL 
firmware. I don't think it is a problem as these systems are bound to 
update 
their firmware due to the amount of development currently being done on the 
OPAL side.

Open Power systems are not impacted.


Is that clearer ? and I would go for option 3

Re: [PATCH 1/1]: thermal driver therm_adt746.c

[Cedric Le Goater]

On 02/23/2015 12:58 PM, Thomas Haschka wrote:
 Hi everyone,
 
 The current driver linux/drivers/macintosh/therm_adt746x.c does not take the 
 HDD BUTTOMSIDE sensor into account. It actually should as the 12 Powerbooks 
 and IBooks are build in a way that the airflow cools the harddrive and 
 components around it. Actually there are air intake openings just beneath the 
 Harddrive. If you experiance hot enviromental temperatures as I did in summer 
 on certain occations, you will find out that in MacOSX the fan spins up while 
 it doesn't in linux. As this probably causes harddrives, and maybe other 
 components to fail early I think this should be regarded as a fix to a severe 
 bug.
 
 Hence, I created this patch for single fan 12 Albooks, Ibooks etc.

I would happy to give it a try on the brand new iBook G4 I just 
got this week-end but I would need you to reformat your patch. 

Could you please send it again to the mailing using : 

git send-email --to linuxppc-dev@lists.ozlabs.org my.patch
 
You might want to run the script ./scripts/checkpatch.pl on the
patch file before sending.

Cheers,

C. 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [RFC PATCH 0/3] hwmon: (ibmpowernv) add DTS support

[Cedric Le Goater]

On 02/21/2015 12:52 AM, Guenter Roeck wrote:
 On 02/20/2015 12:15 PM, Cedric Le Goater wrote:
 On 02/20/2015 05:52 PM, Guenter Roeck wrote:
 On Fri, Feb 20, 2015 at 04:07:34PM +0100, Cédric Le Goater wrote:
 Hello !

 These patches rework the ibmpowernv driver to support the new device
 tree as proposed by this patchset on the skiboot mailing list :

https://lists.ozlabs.org/pipermail/skiboot/2015-February/000457.html

 They are based on Linux 3.19 and were tested on IBM Power and Open Power
 systems running trusty.

 The main issue is that the new device tree is incompatible with the
 previous ibmpowernv drivers. The consequence is no powernv sensors
 on systems with such a opal/linux configuration.

 I don't think that would be acceptable. There must be lots of such
 systems out there. Why does it have to be incompatible ?
 Can't it support both the old and new versions ?

 I should have provided more explanation in the Linux patchset. Sorry
 for that. Here is the rationale behind this brutal code change.

 The initial ibmpowernv driver was designed in the early days of the
 powernv platform and the device tree it is using to expose the sensors
 has some limitations that makes it difficult to add new ones. The current
 layout of the device tree is also tightly coupled to IBM Power systems
 and their service processor (FSP). Open Power systems are different and
 need a different solution.

 It is to get more sensors out the P8 (and there are quite a few) that
 the OPAL patchset [1] proposes a new device tree. On the Linux side, it
 feels simpler to make a jump forward and break the compatibility than
 to maintain multiple branches of code just to keep alive an early v1
 of the ibmpowernv driver.

 
 Would it possibly be appropriate to write a different driver for the new
 device tree ?

Sure. That is an option. 

There are no conflicts between the trees so we can live with two drivers 
using the same sensors/ root node. With time we will deprecate the initial
one.

Is that the preferred option ? How would we name the new driver ? 

1. powernv
2. powernv-hwmon
3. openpowernv
4. ibmpowernv2 

Thanks,

C.


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [RFC PATCH 1/3] powerpc/powernv: Check OPAL sensor calls exist

[Cedric Le Goater]

On 02/20/2015 05:53 PM, Guenter Roeck wrote:
 On Fri, Feb 20, 2015 at 04:07:35PM +0100, Cédric Le Goater wrote:
 
 You should explain here why this patch is needed.

Yes. What it does is to check that the firmware exposes the service
this driver is using (OPAL_SENSOR_READ). I will fix it.

Thanks,

C. 


 
 Signed-off-by: Cédric Le Goater c...@fr.ibm.com
 ---
  arch/powerpc/platforms/powernv/opal-sensor.c |3 +++
  1 file changed, 3 insertions(+)

 diff --git a/arch/powerpc/platforms/powernv/opal-sensor.c 
 b/arch/powerpc/platforms/powernv/opal-sensor.c
 index 4ab67ef7abc9..544292f2020f 100644
 --- a/arch/powerpc/platforms/powernv/opal-sensor.c
 +++ b/arch/powerpc/platforms/powernv/opal-sensor.c
 @@ -72,6 +72,9 @@ static __init int opal_sensor_init(void)
  struct platform_device *pdev;
  struct device_node *sensor;
  
 +if (!opal_check_token(OPAL_SENSOR_READ))
 +return -ENODEV;
 +
  sensor = of_find_node_by_path(/ibm,opal/sensors);
  if (!sensor) {
  pr_err(Opal node 'sensors' not found\n);
 -- 
 1.7.10.4

 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [RFC PATCH 0/3] hwmon: (ibmpowernv) add DTS support

[Cedric Le Goater]

On 02/20/2015 05:52 PM, Guenter Roeck wrote:
 On Fri, Feb 20, 2015 at 04:07:34PM +0100, Cédric Le Goater wrote:
 Hello !

 These patches rework the ibmpowernv driver to support the new device 
 tree as proposed by this patchset on the skiboot mailing list :

   https://lists.ozlabs.org/pipermail/skiboot/2015-February/000457.html

 They are based on Linux 3.19 and were tested on IBM Power and Open Power 
 systems running trusty.

 The main issue is that the new device tree is incompatible with the 
 previous ibmpowernv drivers. The consequence is no powernv sensors 
 on systems with such a opal/linux configuration.

 I don't think that would be acceptable. There must be lots of such
 systems out there. Why does it have to be incompatible ?
 Can't it support both the old and new versions ?

I should have provided more explanation in the Linux patchset. Sorry 
for that. Here is the rationale behind this brutal code change.

The initial ibmpowernv driver was designed in the early days of the
powernv platform and the device tree it is using to expose the sensors 
has some limitations that makes it difficult to add new ones. The current 
layout of the device tree is also tightly coupled to IBM Power systems 
and their service processor (FSP). Open Power systems are different and 
need a different solution.

It is to get more sensors out the P8 (and there are quite a few) that 
the OPAL patchset [1] proposes a new device tree. On the Linux side, it 
feels simpler to make a jump forward and break the compatibility than 
to maintain multiple branches of code just to keep alive an early v1 
of the ibmpowernv driver.

Open Power systems won't be impacted as ibmpowernv does not support them.


Cheers,

C.


[1] https://lists.ozlabs.org/pipermail/skiboot/2015-February/000457.html

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 2/5 v2] powerpc/boot/fdt: Add little-endian support to libfdt wrappers

[Cedric Le Goater]

On 02/11/2015 05:55 AM, Jeremy Kerr wrote:
 For epapr-style boot, we may be little-endian. This change implements
 the proper conversion for fdt*_to_cpu and cpu_to_fdt*. We also need the
 full cpu_to_* and *_to_cpu macros for this.
 
 Signed-off-by: Jeremy Kerr j...@ozlabs.org
 Acked-by: Benjamin Herrenschmidt b...@kernel.crashing.org


I did not work on the little endian epapr wrapper :/

To test the patchset, I used the 'le' branch of 
git://github.com/jk-ozlabs/linux 
and successfully booted a tuleta and an open-power system using the latest 
skiboot and a LE zImage.epapr. Looks good to me. 

You might want to add CONFIG_IPMI_WATCHDOG to openpower_defconfig 

Thanks.

C.


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: powerc: fix build failure when CONFIG_HUGETLB_PAGE is not set

[Cedric Le Goater]

Hello Michael,

On 10/28/2014 05:31 AM, Michael Ellerman wrote:
 On Mon, 2014-27-10 at 14:30:06 UTC, =?utf-8?q?C=C3=A9dric_Le_Goater?= wrote:
 CC  arch/powerpc/mm/slice.o
 In file included from ../arch/powerpc/mm/slice.c:33:0:
 ../include/linux/hugetlb.h:141:47: error: expected identifier or ‘(’ before 
 numeric constant
  #define is_hugepage_only_range(mm, addr, len) 0
^
 ../arch/powerpc/mm/slice.c:704:5: note: in expansion of macro 
 ‘is_hugepage_only_range’
  int is_hugepage_only_range(struct mm_struct *mm, unsigned long addr,
  ^
 
 Hi Cedric,
 
 I'm pretty sure this is fixed in my fixes branch:
 
   https://git.kernel.org/cgit/linux/kernel/git/mpe/linux.git/log/?h=fixes

Indeed. I just picked -rc2 without checking your branch. I will next time.

Thanks,

C.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: powerpc: crash with 3.16.0-rc2

[Cedric Le Goater]

Hi Suka,

On 06/26/2014 08:10 AM, Sukadev Bhattiprolu wrote:
 I got the following crash in Open Firmware, on two separate systems
 with recent mainline kernel (3.16.0-rc2). One was a P8 LPAR with no
 changes to kernel and another a Power7 LPAR with some kernel changes
 (24x7 perf counter patches). I will backout the patches and try but
 wanted to check if there is some config change I am missing.
 
 I am also attaching the config file (which is based on a 3.14
 kernel that boots ok).
 
 ---
 OF stdout device is: /vdevice/vty@3000
 Preparing to boot Linux version 3.16.0-rc2-unwind+ (root@saturn-lp2) (gcc 
 version 4.8.2 20131212 (Red Hat 4.8.2-7) (GCC) ) #5 SMP Thu Jun 26 00:01:47 
 CDT 2014
 Detected machine type: 0101
 Max number of cores passed to firmware: 256 (NR_CPUS = 1024)
 Calling ibm,client-architecture-support... done
 command line: BOOT_IMAGE=/vmlinux-3.16.0-rc2-unwind 
 root=UUID=017d60b7-9db9-4d91-9dc8-4e6f91b0ed40 ro biosdevname=0 
 vconsole.font=latarcyrheb-sun16
 memory layout at init:
   memory_limit :  (16 MB aligned)
   alloc_bottom : 0553
   alloc_top: 1000
   alloc_top_hi : 1000
   rmo_top  : 1000
   ram_top  : 1000
 instantiating rtas at 0x0ee9... done
 Querying for OPAL presence... not there.

A patch from Michael Ellerman was just merged : 

powerpc/powernv: Remove OPAL v1 takeover

I think it fixes the problem you are seeing.

Cheers,

C. 


 DEFAULT CATCH!, exception-handler=fff00700 
 at   %SRR0: 04133064   %SRR1: 80081002 
 Open Firmware exception handler entered from non-OF code
 
 Client's Fix Pt Regs:
  00 04133030 04133018 047b1b28 0002
  04 28005024 047b1b28 1002 04132f18
  08   04132f18 1002
  0c a001  01a3fd20 0401bb70
  10 0401c030 0401bd70 fffd 01a3fd20
  14 01b49080 0ee9 0117 0ee9
  18 0401c510 0369 0401bb28 04193da0
  1c 01a3fd60 794927e391410070 e87f408204cc 3cc2ff8b38c629d8
 Special Regs:
 %IV: 0700 %CR: 28005022%XER:   %DSISR: 4200 
   %SRR0: 04133064   %SRR1: 80081002 
 %LR: 04133030%CTR:  
%DAR: 01a3fcf00020b4ac 
 Virtual PID = 0 
  ok
 
 
 
 ___
 Linuxppc-dev mailing list
 Linuxppc-dev@lists.ozlabs.org
 https://lists.ozlabs.org/listinfo/linuxppc-dev
 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 18/18] powerpc/boot: add PPC64_BOOT_WRAPPER config option

[Cedric Le Goater]

Hi Benjamin,

 So I originally applied all 3 last patches of the series as one
 (collapsed them in git) in order to not break bisection.
 
 However, I had to take the series out in the end due to it
 causing this error on some of my test configs:
 
 powerpc64-linux-ld: cannot find arch/powerpc/boot/pseries-head.o: No such 
 file or directory

 I haven't had a chance to investigate yet, but sadly it looks like
 this series might have to wait for the next round.

OK. I will take a look. 

Thanks,

C.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 00/18] powerpc/boot: 64bit little endian wrapper

[Cedric Le Goater]

Hi Geoff,

 Do you have these in a git branch somewhere so I can merge and test
 them?

No, sorry, I don't have a public git repo. Let me see how I can fix 
that, it might be useful for future patchsets. 

Thanks,

C.  


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 00/18] powerpc/boot: 64bit little endian wrapper

[Cedric Le Goater]

Hi Geoff,
 
 Do you have these in a git branch somewhere so I can merge and test
 them?
 
 No, sorry, I don't have a public git repo. Let me see how I can fix 
 that, it might be useful for future patchsets. 

Please try that :

git pull git://github.com/legoater/linux.git zimage

Thanks,

C.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 00/18] powerpc/boot: 64bit little endian wrapper

[Cedric Le Goater]

On 03/21/2014 06:28 PM, Geoff Levand wrote:
 Hi Cédric,
 
 On Thu, 2014-03-20 at 16:09 +0100, Cédric Le Goater wrote:
 The following patchset adds support for 64bit little endian boot 
 wrapper. It is based on original code from Andrew Tauferner. 
 
 I tested this on PS3 (64 bit BE) and found no regression.
 
 Tested by: Geoff Levand ge...@infradead.org

Thanks !

C.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] offb: make the screen properties endian safe

[Cedric Le Goater]

On 11/23/2013 10:04 PM, Benjamin Herrenschmidt wrote:
 On Sat, 2013-11-23 at 18:38 +0100, Cedric Le Goater wrote:
 So, 32bpp works but 16 is broken ... I guess my palette fix is just a lucky
 hack and I need to dig deeper in fb code to have a better understanding of
 the color map. 

 I should have provided you two patches in the first place. Do you want the 
 device tree data fixes for the frame buffer screen properties ? It helps to 
 have a display for little endian guests even if the colors are wrong.
 
 Before you pull your hair out, check if it works in BE :-)

I spent some more time on this topic and I have a few more patches to 
send you.

The first patch is straight forward. It fixes host endian order issues 
in the offb code when accessing the OF device tree properties. 

The following patch is more of an attempt to fix the pseudo-palette 
entries on little endian. Hopefully this is the good direction. If not, 
I still have some hair to pull out :)

Cheers,

C.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] offb: make the screen properties endian safe

[Cedric Le Goater]

On 11/21/2013 08:57 PM, Benjamin Herrenschmidt wrote:
 On Thu, 2013-11-21 at 16:45 +0100, Cedric Le Goater wrote:
  - fbdev *generally* assume native endian framebuffer, but of course
 under qemu today, the adapter will use a big endian frame buffer
 aperture. You can compile in support for foreign endian but I don't know
 how that actually works.

 OK. I will see how I can extend the tests. But, are you suggesting I should
 be using the foreign endian framework for the frame buffer ? 
 
 Well, if it works ... did you try 16 and 32bpp ?

So, 32bpp works but 16 is broken ... I guess my palette fix is just a lucky
hack and I need to dig deeper in fb code to have a better understanding of
the color map. 

I should have provided you two patches in the first place. Do you want the 
device tree data fixes for the frame buffer screen properties ? It helps to 
have a display for little endian guests even if the colors are wrong.

Thanks,

C.



___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] offb: make the screen properties endian safe

[Cedric Le Goater]

Hi,

On 11/20/2013 11:50 PM, Benjamin Herrenschmidt wrote:
 On Thu, 2013-10-31 at 10:36 +0100, Cédric Le Goater wrote:
 The screen properties : depth, width, height, linebytes need
 to be converted to the host endian order when read from the device
 tree.

 Signed-off-by: Cédric Le Goater c...@fr.ibm.com
 ---
 
 Did you actually test that ? IE, using emulated VGA in qemu for
 example ?

Yes, that's how I did. I ran LE and BE guests with qemu -vga std and 
different depths.

 I'm asking because there are a few interesting nits here...
 
  - fbdev *generally* assume native endian framebuffer, but of course
 under qemu today, the adapter will use a big endian frame buffer
 aperture. You can compile in support for foreign endian but I don't know
 how that actually works.

OK. I will see how I can extend the tests. But, are you suggesting I should
be using the foreign endian framework for the frame buffer ? 
 
  - The setcolreg fix ... the value is used 2 lines above your endian
 swap, is that correct ?

The variables used to calculate value are in host endian order. It should
be fine.

Thanks,

C.

 
 Cheers
 Ben.
 
 
 Changes in v2:

  - replaced be32_to_cpu() by be32_to_cpup() 
  - fixed setcolreg ops 

  drivers/video/offb.c |   12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

 diff --git a/drivers/video/offb.c b/drivers/video/offb.c
 index 0c4f343..68e8415 100644
 --- a/drivers/video/offb.c
 +++ b/drivers/video/offb.c
 @@ -120,7 +120,7 @@ static int offb_setcolreg(u_int regno, u_int red, u_int 
 green, u_int blue,
  mask = info-var.transp.offset;
  value |= mask;
  }
 -pal[regno] = value;
 +pal[regno] = cpu_to_be32(value);
  return 0;
  }
  
 @@ -536,7 +536,7 @@ static void __init offb_init_nodriver(struct device_node 
 *dp, int no_real_node)
  unsigned int flags, rsize, addr_prop = 0;
  unsigned long max_size = 0;
  u64 rstart, address = OF_BAD_ADDR;
 -const u32 *pp, *addrp, *up;
 +const __be32 *pp, *addrp, *up;
  u64 asize;
  int foreign_endian = 0;
  
 @@ -552,25 +552,25 @@ static void __init offb_init_nodriver(struct 
 device_node *dp, int no_real_node)
  if (pp == NULL)
  pp = of_get_property(dp, depth, len);
  if (pp  len == sizeof(u32))
 -depth = *pp;
 +depth = be32_to_cpup(pp);
  
  pp = of_get_property(dp, linux,bootx-width, len);
  if (pp == NULL)
  pp = of_get_property(dp, width, len);
  if (pp  len == sizeof(u32))
 -width = *pp;
 +width = be32_to_cpup(pp);
  
  pp = of_get_property(dp, linux,bootx-height, len);
  if (pp == NULL)
  pp = of_get_property(dp, height, len);
  if (pp  len == sizeof(u32))
 -height = *pp;
 +height = be32_to_cpup(pp);
  
  pp = of_get_property(dp, linux,bootx-linebytes, len);
  if (pp == NULL)
  pp = of_get_property(dp, linebytes, len);
  if (pp  len == sizeof(u32)  (*pp != 0xu))
 -pitch = *pp;
 +pitch = be32_to_cpup(pp);
  else
  pitch = width * ((depth + 7) / 8);
  
 
 

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 16/51] DMA-API: ppc: vio.c: replace dma_set_mask()+dma_set_coherent_mask() with new helper

[Cedric Le Goater]

On 11/16/2013 04:32 PM, Russell King - ARM Linux wrote:
 On Fri, Nov 15, 2013 at 05:16:55PM +0100, Cedric Le Goater wrote:
 The new helper routine dma_set_mask_and_coherent() breaks the 
 initialization of the pseries vio devices which do not have an 
 initial dev-dma_mask. I think we need to use dma_coerce_mask_and_coherent()
 instead.
 
 Who wants to handle this patch?

 Also, is it possible to fix it so that dev-dma_mask is correctly setup
 by the code which creates the device, as it should be in the first place?

The vio_dev should probably be improved to setup devices as the other 
drivers do but we might be short on time to do that for 3.13 and pseries 
is really broken ... For now, I will just resend the patch with some 
context in the changelog. 

Thanks,

C.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 16/51] DMA-API: ppc: vio.c: replace dma_set_mask()+dma_set_coherent_mask() with new helper

[Cedric Le Goater]

Hi, 

On 09/19/2013 11:41 PM, Russell King wrote:
 Replace the following sequence:
 
   dma_set_mask(dev, mask);
   dma_set_coherent_mask(dev, mask);
 
 with a call to the new helper dma_set_mask_and_coherent().
 
 Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
 ---
  arch/powerpc/kernel/vio.c |3 +--
  1 files changed, 1 insertions(+), 2 deletions(-)
 
 diff --git a/arch/powerpc/kernel/vio.c b/arch/powerpc/kernel/vio.c
 index 78a3506..96b6c97 100644
 --- a/arch/powerpc/kernel/vio.c
 +++ b/arch/powerpc/kernel/vio.c
 @@ -1413,8 +1413,7 @@ struct vio_dev *vio_register_device_node(struct 
 device_node *of_node)
 
   /* needed to ensure proper operation of coherent allocations
* later, in case driver doesn't set it explicitly */
 - dma_set_mask(viodev-dev, DMA_BIT_MASK(64));
 - dma_set_coherent_mask(viodev-dev, DMA_BIT_MASK(64));
 + dma_set_mask_and_coherent(viodev-dev, DMA_BIT_MASK(64));
   }
 
   /* register with generic device framework */
 

The new helper routine dma_set_mask_and_coherent() breaks the 
initialization of the pseries vio devices which do not have an 
initial dev-dma_mask. I think we need to use dma_coerce_mask_and_coherent()
instead.

Signed-off-by: Cédric Le Goater c...@fr.ibm.com
---
 arch/powerpc/kernel/vio.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/vio.c b/arch/powerpc/kernel/vio.c
index e7d0c88f..76a6482 100644
--- a/arch/powerpc/kernel/vio.c
+++ b/arch/powerpc/kernel/vio.c
@@ -1419,7 +1419,7 @@ struct vio_dev *vio_register_device_node(struct 
device_node *of_node)
 
/* needed to ensure proper operation of coherent allocations
 * later, in case driver doesn't set it explicitly */
-   dma_set_mask_and_coherent(viodev-dev, DMA_BIT_MASK(64));
+   dma_coerce_mask_and_coherent(viodev-dev, DMA_BIT_MASK(64));
}
 
/* register with generic device framework */
-- 
1.7.10.4


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/3] powerpc: bare minimum checkpoint/restart implementation

[Cedric Le Goater]

 Again, how would 'cr' obtain exit status for these tasks, and how would
 it distinguish failure from normal operation?

Here's our solution to this issue.

mcr maintains in its kernel container object an exitcode attribute for 
the mcr-restart process. This process is detached from the fork tree of 
the restarted application.  

when the restart is finished, an mcr-wait command can be called to reap 
this exitcode. This make it possible to distinguish an exit of the 
application process from an exit of the mcr-restart process.

This is a must-have for batch managers in an HPC environment. 

Cheers,

C.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH 1/3] powerpc: bare minimum checkpoint/restart implementation

[Cedric Le Goater]

 More specifically, I envision restart to work like this:
 
 1) user invokes user-land utility (e.g. cr --restart ...
 2) 'cr' will create a new container
 3) 'cr' will start a child in that container

process 1 in its private namespaces.

 4) child will create rest of tree (in kernel or in user space - tbd)
 5) each task in that tree will restore itself
 6) 'cr' monitors this process
 7) if all goes well - 'cr' report ok.
 8) if something goes bad, 'cr' notices and notifies caller/user

that's MCR implementation of restart. 
 
 so tasks that are restarting may just as well die badly - we don't care.

just sigkill them, but at end, before releasing the container from 
a frozen state, we have to make sure the right number of tasks have 
restarted ... so you need to track them along the way.

C.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev