<html><body><p>The OCC register definitions are set by the hardware and can be found in the P8 SCOM register documentation. They won't ever change for P8 and P8+.<br><br><br><tt><font size="4">> > +<br>> > +/* To generate attn to OCC */<br>> > +#define ATTN_DATA 0x0006B035<br>> > +/* For BMC to read/write SRAM */<br>> > +#define OCB_ADDRESS 0x0006B070<br>> > +#define OCB_DATA 0x0006B075<br>> > +#define OCB_STATUS_CONTROL_AND 0x0006B072<br>> > +#define OCB_STATUS_CONTROL_OR 0x0006B073<br>> > +#define OCC_COMMAND_ADDR 0xFFFF6000<br>> > +#define OCC_RESPONSE_ADDR 0xFFFF7000</font></tt><br><br>Regards,<br>Norman James<br>IBM - POWER Systems Architect<br>Phone: 1-512-286-6807 (T/L: 363-6807)<br>Internet: njames@us.ibm.com<br><br><br><img width="16" height="16" src="cid:1__=09BBF5A2DFD4C3078f9e8a93df938690918c09B@" border="0" alt="Inactive hide details for "Yi TZ Li" ---01/05/2016 02:30:26 AM---Hi Joel, Thanks many for your time to review the driver."><font color="#424282">"Yi TZ Li" ---01/05/2016 02:30:26 AM---Hi Joel, Thanks many for your time to review the driver.</font><br><br><font size="2" color="#5F5F5F">From: </font><font size="2">"Yi TZ Li" <shliyi@cn.ibm.com></font><br><font size="2" color="#5F5F5F">To: </font><font size="2">Joel Stanley <joel@jms.id.au></font><br><font size="2" color="#5F5F5F">Cc: </font><font size="2">OpenBMC Maillist <openbmc@lists.ozlabs.org></font><br><font size="2" color="#5F5F5F">Date: </font><font size="2">01/05/2016 02:30 AM</font><br><font size="2" color="#5F5F5F">Subject: </font><font size="2">Re: [PATCH occ hwmon v3] Add OCC hwmon driver to Kernel</font><br><font size="2" color="#5F5F5F">Sent by: </font><font size="2">"openbmc" <openbmc-bounces+njames=us.ibm.com@lists.ozlabs.org></font><br><hr width="100%" size="2" align="left" noshade style="color:#8091A5; "><br><br><br><tt><font size="4">Hi Joel,</font></tt><font size="4"><br></font><tt><font size="4"><br>Thanks many for your time to review the driver.</font></tt><font size="4"><br></font><tt><font size="4"><br>I re-submitted to patch, fixed most of the issues you pointed out, except a few showed bellow.<br>Please see my reply bellow for details:</font></tt><font size="4"><br></font><tt><font size="4"><br>joel.stan@gmail.com wrote on 12/10/2015 12:02:19 PM:<br><br>> From: Joel Stanley <joel@jms.id.au><br>> To: Yi Li <adamliyi@msn.com><br>> Cc: OpenBMC Maillist <openbmc@lists.ozlabs.org>, Jeremy Kerr <br>> <jk@ozlabs.org>, Yi TZ Li/China/IBM@IBMCN<br>> Date: 12/10/2015 12:03 PM<br>> Subject: Re: [PATCH occ hwmon v3] Add OCC hwmon driver to Kernel<br>> Sent by: joel.stan@gmail.com<br>> <br>> Hello Adam,<br>> <br>> On Tue, Dec 8, 2015 at 6:59 PM, Yi Li <adamliyi@msn.com> wrote:<br>> > The default patch triggered by git pull request is hard to read.<br>> > I resend a clean patch to the list. Please review.<br>> <br>> Thanks! This is the preferred method for reviewing code. Even better<br>> would be if you used git format-patch to create the email, that way we<br>> can review your commit message as well.<br>> <br>> Before you submit next, please run your patch through<br>> scripts/checkpatch.pl in the kernel tree and fix all of the errors and<br>> warnings. It will warn you about common style and syntax mistakes.<br>> <br>> ./scripts/checkpatch.pl -f drivers/hwmon/occ_i2c.c<br>> <br>> [...]<br>> <br>> total: 24 errors, 21 warnings, 1315 lines checked</font></tt><font size="4"><br></font><tt><font size="4"><br>I run the checkpatch.pl against the new patch, here are the result:</font></tt><font size="4"><br></font><tt><font size="4"><br>scripts/checkpatch.pl occ_v4.patch<br>WARNING: DT compatible string "ibm,occ-i2c" appears un-documented -- check ./Documentation/devicetree/bindings/<br>#11: FILE: arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts:30:<br>+ compatible = "ibm,occ-i2c";</font></tt><font size="4"><br></font><tt><font size="4"><br>WARNING: DT compatible string "ibm,occ-i2c" appears un-documented -- check ./Documentation/devicetree/bindings/<br>#15: FILE: arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts:34:<br>+ compatible = "ibm,occ-i2c";</font></tt><font size="4"><br></font><tt><font size="4"><br>WARNING: DT compatible string "ibm,occ-i2c" appears un-documented -- check ./Documentation/devicetree/bindings/<br>#33: FILE: arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts:35:<br>+ compatible = "ibm,occ-i2c";</font></tt><font size="4"><br></font><tt><font size="4"><br>WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?<br>#74:<br>new file mode 100644</font></tt><font size="4"><br></font><tt><font size="4"><br>WARNING: line over 80 characters<br>#127: FILE: drivers/hwmon/occ_i2c.c:49:<br>+ * </font></tt><a href="https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf"><tt><u><font size="4" color="#0000FF">https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf</font></u></tt></a><font size="4"><br></font><tt><font size="4"><br>WARNING: line over 80 characters<br>#420: FILE: drivers/hwmon/occ_i2c.c:342:<br>+ * </font></tt><a href="https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf"><tt><u><font size="4" color="#0000FF">https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf</font></u></tt></a><font size="4"><br></font><tt><font size="4"><br>WARNING: DT compatible string "ibm,occ-i2c" appears un-documented -- check ./Documentation/devicetree/bindings/<br>#1389: FILE: drivers/hwmon/occ_i2c.c:1311:<br>+ {.compatible = "ibm,occ-i2c"},</font></tt><font size="4"><br></font><tt><font size="4"><br>ERROR: Missing Signed-off-by: line(s)</font></tt><font size="4"><br></font><tt><font size="4"><br>total: 1 errors, 7 warnings, 1391 lines checked</font></tt><font size="4"><br></font><tt><font size="4"><br>occ_v4.patch has style problems, please review. </font></tt><font size="4"><br></font><tt><font size="4"><br>1) For "DT compatible" warnings, I am not sure whether it is necessary, since I run "scripts/checkpatch.pl -f arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts",<br>it will report many warnings. I can add a description if necessary.</font></tt><font size="4"><br></font><tt><font size="4"><br>2) for "line over 80" warnings, it would break the link if wrap at 80 characters.<br> <br>> > +<br>> > +/* To generate attn to OCC */<br>> > +#define ATTN_DATA 0x0006B035<br>> > +/* For BMC to read/write SRAM */<br>> > +#define OCB_ADDRESS 0x0006B070<br>> > +#define OCB_DATA 0x0006B075<br>> > +#define OCB_STATUS_CONTROL_AND 0x0006B072<br>> > +#define OCB_STATUS_CONTROL_OR 0x0006B073<br>> > +#define OCC_COMMAND_ADDR 0xFFFF6000<br>> > +#define OCC_RESPONSE_ADDR 0xFFFF7000<br>> <br>> Are these addresses going to change with different versions of the <br>> OCC firmware?<br>></font></tt><font size="4"><br></font><tt><font size="4"><br>Actually I did not find the document for most of the addresses. I copied them from Alvin's OCC code.<br>If anyone know the source (possibly from OCC firmware developers), please help.<br>The only OCC document I can find is:</font></tt><u><font size="4" color="#0000FF"><br></font></u><a href="https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf"><tt><u><font size="4" color="#0000FF">https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf</font></u></tt></a><tt><font size="4"><br>It defines OCC_COMMAND_ADDR and OCC_RESPONSE_ADDR only.</font></tt><font size="4"><br></font><tt><font size="4"><br>> <br>> > +<br>> > + if (!p)<br>> > + return;<br>> <br>> It's common in kernel code to omit checking for null arguments.<br>> <br>> If you think there is a likely hood of this happening, then perhaps<br>> WARN_ON(!b) would be better; this way we get a backtrace in the kernel<br>> log.<br>> </font></tt><font size="4"><br></font><tt><font size="4"><br>The deinit_occ_resp_buf() function is supposed to be called in multiple situation.<br>E.g, error handling code in a function. In those case, pointer might be NULL.<br>So WARN_ON() is not expected in that case.</font></tt><font size="4"><br><br></font><tt><font size="4"><br>> > +<br>> > + if (!p->blocks)<br>> > + return;<br>> > +<br>> > + for (b = 0; b < p->header.sensor_block_num; b++) {<br>> > + if (p->blocks[b].sensor)<br>> > + kfree(p->blocks[b].sensor);<br>> > + if (p->blocks[b].power)<br>> > + kfree(p->blocks[b].power);<br>> > + if (p->blocks[b].caps)<br>> > + kfree(p->blocks[b].caps);<br>> > + }<br>> <br>> I think Jeremy suggested in his review that you statically allocate<br>> these instead of kmalloc and kfreeing at runtime.<br>> </font></tt><font size="4"><br></font><tt><font size="4"><br>The OCC sensor type and sensor number will change depending on whether OCC is master or slave.<br>Also there is possible of bad cores and the number of sensors may change.<br>So using dynamically allocated sensor block is nature I think.<br>It is possible that the driver allocates a block of memory and manage itself.<br>But looks there is no great benefit.<br><br>> > +{<br>> > + int b;<br>> > + int s;<br>> > + int ret;<br>> > + int dnum = SENSOR_BLOCK_OFFSET;<br>> > + struct occ_sensor *f_sensor;<br>> > + struct occ_sensor *t_sensor;<br>> > + struct power_sensor *p_sensor;<br>> > + struct caps_sensor *c_sensor;<br>> > + uint8_t sensor_block_num;<br>> > + uint8_t sensor_type[4];<br>> > + uint8_t sensor_format;<br>> > + uint8_t sensor_length;<br>> > + uint8_t num_of_sensors;<br>> > +<br>> > + /* check if the data is valid */<br>> > + if (strncmp(&d[SENSOR_STR_OFFSET], "SENSOR", 6) != 0) {<br>> <br>> If you have a literal string you don't need to use strncmp.<br>> </font></tt><font size="4"><br></font><tt><font size="4"><br>"d[SENSOR_STR_OFFSET]" does not have '\0' to terminate a string. So have to use strncmp.</font></tt><font size="4"><br><br></font><tt><font size="4"><br>> <br>> > + p_sensor->accumulator =<br>> > + (d[dnum+6] << 24) | (d<br>> [dnum+7] << 16) |<br>> > + (d[dnum+8] << 8) | d[dnum+9];<br>> > + p_sensor->value =<br>> > + (d[dnum+10] << 8) | d[dnum+11];<br>> > +<br>> > + dev_dbg(&client->dev,<br>> > + "sensor[%d]-[%d]: id: %u, <br>> value: %u\n",<br>> > + b, s, p_sensor->sensor_id,<br>> > + p_sensor->value);<br>> > +<br>> > + dnum = dnum + sensor_length;<br>> > + }<br>> > + } else if (strncmp(sensor_type, "CAPS", 4) == 0) {<br>> <br>> If you have a literal string you don't need to use strncmp.</font></tt><font size="4"><br></font><tt><font size="4"><br>"sensor_type" has no '\0' to terminate it. strncmp() is necessary. <br><br><br>> > + val = -1;<br>> > + else<br>> > + /* in millidegree Celsius */<br>> > + val = sensor[n].value * 1000;<br>> > +<br>> > + return sprintf(buf, "%d\n", val);<br>> <br>> How do you know "%d\n" will fit into buf?</font></tt><font size="4"><br></font><tt><font size="4"><br>Do you mean using snprintf()? After reading the man page:<br>"Because sprintf() and vsprintf() assume an arbitrarily long string, callers must be careful not to overflow the actual space; this<br> is often impossible to assure. Note that the length of the strings produced is locale-dependent and difficult to predict. Use<br> snprintf() and vsnprintf() instead (or asprintf(3) and vasprintf(3))."</font></tt><font size="4"><br></font><tt><font size="4"><br>snprintf() seems more safe, but we can find a lot of "sprintf()" in sysfs show* functions in drivers/hwmon/. Looks to me not a big issue.<br>Anyway, I will change to snprintf() to be safer:<br>I use the same code as driver/hwmon/lm95241.c</font></tt><font size="4"><br></font><tt><font size="4"><br>" return snprintf(buf, PAGE_SIZE - 1,<br> data->config & to_sensor_dev_attr(attr)->index ?<br> "127000\n" : "255000\n");<br>"</font></tt><font size="4"><br></font><tt><font size="4"><br>Thanks,<br>-adam</font></tt><font size="4"><br></font><tt><font size="4"><br></font></tt><font size="4"><br></font><tt>_______________________________________________<br>openbmc mailing list<br>openbmc@lists.ozlabs.org<br></tt><tt><a href="https://lists.ozlabs.org/listinfo/openbmc">https://lists.ozlabs.org/listinfo/openbmc</a></tt><tt><br></tt><br><BR>
</body></html>