<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
mso-fareast-language:EN-US;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:#0563C1;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:#954F72;
text-decoration:underline;}
p.MsoPlainText, li.MsoPlainText, div.MsoPlainText
{mso-style-priority:99;
mso-style-link:"Plain Text Char";
margin:0cm;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
mso-fareast-language:EN-US;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0cm;
mso-margin-bottom-alt:auto;
margin-left:0cm;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
span.PlainTextChar
{mso-style-name:"Plain Text Char";
mso-style-priority:99;
mso-style-link:"Plain Text";
font-family:"Calibri",sans-serif;}
span.EmailStyle20
{mso-style-type:personal-compose;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;
font-family:"Calibri",sans-serif;
mso-fareast-language:EN-US;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 72.0pt 72.0pt 72.0pt;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="RU" link="#0563C1" vlink="#954F72">
<div class="WordSection1">
<p class="MsoPlainText"><span lang="EN-US">Hi Vadim.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Please check my answer to Andy.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Hi Andy. <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Thanks for review.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Please read my answers inline.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> -----Original Message-----<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> From: Andy Shevchenko [<a href="mailto:andy.shevchenko@gmail.com">mailto:andy.shevchenko@gmail.com</a>]<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Sent: 16 мая 2018 г. 0:00<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> To: Oleksandr Shamray <<a href="mailto:oleksandrs@mellanox.com">oleksandrs@mellanox.com</a>><o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Cc: Greg Kroah-Hartman <<a href="mailto:gregkh@linuxfoundation.org">gregkh@linuxfoundation.org</a>>; Arnd Bergmann
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <<a href="mailto:arnd@arndb.de">arnd@arndb.de</a>>; Linux Kernel Mailing List
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <<a href="mailto:linux-kernel@vger.kernel.org">linux-kernel@vger.kernel.org</a>>; linux-arm Mailing List
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <<a href="mailto:linux-arm-kernel@lists.infradead.org">linux-arm-kernel@lists.infradead.org</a>>; devicetree
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <<a href="mailto:devicetree@vger.kernel.org">devicetree@vger.kernel.org</a>>;
<a href="mailto:openbmc@lists.ozlabs.org">openbmc@lists.ozlabs.org</a>; Joel Stanley
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <<a href="mailto:joel@jms.id.au">joel@jms.id.au</a>>; Jiří Pírko <<a href="mailto:jiri@resnulli.us">jiri@resnulli.us</a>>; Tobias Klauser
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <<a href="mailto:tklauser@distanz.ch">tklauser@distanz.ch</a>>; open list:SERIAL DRIVERS <linux-
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <a href="mailto:serial@vger.kernel.org">
serial@vger.kernel.org</a>>; Vadim Pasternak <<a href="mailto:vadimp@mellanox.com">vadimp@mellanox.com</a>>;
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> system-sw-low-level <<a href="mailto:system-sw-low-level@mellanox.com">system-sw-low-level@mellanox.com</a>>; Rob Herring
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <<a href="mailto:robh+dt@kernel.org">robh+dt@kernel.org</a>>;
<a href="mailto:openocd-devel-owner@lists.sourceforge.net">openocd-devel-owner@lists.sourceforge.net</a>;
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> linux- <a href="mailto:api@vger.kernel.org">
api@vger.kernel.org</a>; David S. Miller <<a href="mailto:davem@davemloft.net">davem@davemloft.net</a>>;
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Mauro Carvalho Chehab <<a href="mailto:mchehab@kernel.org">mchehab@kernel.org</a>>; Jiri Pirko
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <<a href="mailto:jiri@mellanox.com">jiri@mellanox.com</a>><o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Subject: Re: [patch v21 2/4] drivers: jtag: Add Aspeed SoC 24xx and
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> 25xx families JTAG master driver<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> On Tue, May 15, 2018 at 5:21 PM, Oleksandr Shamray
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <<a href="mailto:oleksandrs@mellanox.com">oleksandrs@mellanox.com</a>> wrote:<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > Driver adds support of Aspeed 2500/2400 series SOC JTAG master<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> controller.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> ><o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > Driver implements the following jtag ops:<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > - freq_get;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > - freq_set;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > - status_get;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > - idle;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > - xfer;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> ><o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > It has been tested on Mellanox system with BMC equipped with Aspeed<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > 2520 SoC for programming CPLD devices.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +#define ASPEED_JTAG_DATA 0x00<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +#define ASPEED_JTAG_INST 0x04<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +#define ASPEED_JTAG_CTRL 0x08<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +#define ASPEED_JTAG_ISR 0x0C<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +#define ASPEED_JTAG_SW 0x10<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +#define ASPEED_JTAG_TCK 0x14<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +#define ASPEED_JTAG_EC 0x18<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +#define ASPEED_JTAG_DATA_MSB 0x01<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +#define ASPEED_JTAG_DATA_CHUNK_SIZE 0x20<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +#define ASPEED_JTAG_IOUT_LEN(len) (ASPEED_JTAG_CTL_ENG_EN |\<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + ASPEED_JTAG_CTL_ENG_OUT_EN
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +|\<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +ASPEED_JTAG_CTL_INST_LEN(len))<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Better to read<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> #define MY_COOL_CONST_OR_MACRO(xxx) \<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> ...<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +#define ASPEED_JTAG_DOUT_LEN(len) (ASPEED_JTAG_CTL_ENG_EN<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> |\<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + ASPEED_JTAG_CTL_ENG_OUT_EN
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + |\<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +ASPEED_JTAG_CTL_DATA_LEN(len))<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Ditto.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Ok. Changed to:<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">#define ASPEED_JTAG_IOUT_LEN(len) \<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"> (ASPEED_JTAG_CTL_ENG_EN | \<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"> ASPEED_JTAG_CTL_ENG_OUT_EN | \<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"> ASPEED_JTAG_CTL_INST_LEN(len))<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">#define ASPEED_JTAG_DOUT_LEN(len) \<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"> (ASPEED_JTAG_CTL_ENG_EN | \<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"> ASPEED_JTAG_CTL_ENG_OUT_EN | \<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"> ASPEED_JTAG_CTL_DATA_LEN(len))<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +static char *end_status_str[] = {"idle", "ir pause", "drpause"};<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +static int aspeed_jtag_freq_set(struct jtag *jtag, u32 freq) {<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + struct aspeed_jtag *aspeed_jtag = jtag_priv(jtag);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + unsigned long apb_frq;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + u32 tck_val;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + u16 div;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + apb_frq = clk_get_rate(aspeed_jtag->pclk);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + div = (apb_frq % freq == 0) ? (apb_frq / freq) - 1 :
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + (apb_frq / freq);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Isn't it the same as<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> div = (apb_frq - 1) / freq;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> ?<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Seems it is same. Thanks.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + tck_val = aspeed_jtag_read(aspeed_jtag, ASPEED_JTAG_TCK);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + aspeed_jtag_write(aspeed_jtag,<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + (tck_val & ASPEED_JTAG_TCK_DIVISOR_MASK) | div,<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + ASPEED_JTAG_TCK);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + return 0;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +}<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +static void aspeed_jtag_sw_delay(struct aspeed_jtag *aspeed_jtag,
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +int<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +cnt) {<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + int i;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + for (i = 0; i < cnt; i++)<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + aspeed_jtag_read(aspeed_jtag, ASPEED_JTAG_SW);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Isn't it readsl() (or how it's called, I don't remember).<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">No, readsl reads data into buffer. But in this place read used for make software delay.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Aspeed jtag driver supports 2 modes:<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">1 - hw mode with the hardware controlled JTAG states<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">and pins<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">2 - with software controlled pins. <o:p>
</o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">This part of code used in sw-mode and generates delay for JTAG bit-bang .<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">I will change it to ndelay().<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +}<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +static void aspeed_jtag_wait_instruction_pause(struct aspeed_jtag<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +*aspeed_jtag) {<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + wait_event_interruptible(aspeed_jtag->jtag_wq, aspeed_jtag->flag &<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + ASPEED_JTAG_ISR_INST_PAUSE);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> In such cases I prefer to see a new line with a parameter in full.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Check all places.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Full parameter doesn't fit into max 80 symbols per line limitation.
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">But following to LINUX kernel style, it should be no longer than 80 symbols.
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">It will not pass by ./scripts/checkpatch.pl<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">What do you think about this?<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + aspeed_jtag->flag &= ~ASPEED_JTAG_ISR_INST_PAUSE; }<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +static void aspeed_jtag_sm_cycle(struct aspeed_jtag *aspeed_jtag,
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +const<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> u8 *tms,<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + int len) {<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + int i;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + for (i = 0; i < len; i++)<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + aspeed_jtag_tck_cycle(aspeed_jtag, tms[i], 0); }<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +static void aspeed_jtag_run_test_idle_sw(struct aspeed_jtag<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> *aspeed_jtag,<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + struct jtag_run_test_idle<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +*runtest) {<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + static const u8 sm_pause_irpause[] = {1, 1, 1, 1, 0, 1, 0};<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + static const u8 sm_pause_drpause[] = {1, 1, 1, 0, 1, 0};<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + static const u8 sm_idle_irpause[] = {1, 1, 0, 1, 0};<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + static const u8 sm_idle_drpause[] = {1, 0, 1, 0};<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + static const u8 sm_pause_idle[] = {1, 1, 0};<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + int i;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + /* SW mode from idle/pause-> to pause/idle */<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + if (runtest->reset) {<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + for (i = 0; i < ASPEED_JTAG_RESET_CNTR; i++)<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + aspeed_jtag_tck_cycle(aspeed_jtag, 1, 0);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + }<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> I would rather split this big switch to a few helper functions per
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> each status of surrounding switch.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Ok.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Will do it.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + /* Stay on IDLE for at least TCK cycle */<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + for (i = 0; i < runtest->tck; i++)<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + aspeed_jtag_tck_cycle(aspeed_jtag, 0, 0); }<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +/**<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + * aspeed_jtag_run_test_idle:<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + * JTAG reset: generates at least 9 TMS high and 1 TMS low to force<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + * devices into Run-Test/Idle State.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + */<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> It's rather broken kernel doc.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Deleted.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_CTL_ENG_EN |<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + ASPEED_JTAG_CTL_ENG_OUT_EN |<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + ASPEED_JTAG_CTL_FORCE_TMS,
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + ASPEED_JTAG_CTRL);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_EC_GO_IDLE,<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + ASPEED_JTAG_EC);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_SW_MODE_EN |<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + ASPEED_JTAG_SW_MODE_TDIO, ASPEED_JTAG_SW);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Here you have permutations of flag some of which are repeatetive in
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> the code. Perhaps make additional definitions instead.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Check other similar places.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Ok. Will add defined for repeated flags<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + char tdo;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Indentation.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Ok.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + if (xfer->direction == JTAG_READ_XFER)<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + tdi = UINT_MAX;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + else<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + tdi = data[index];<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + if (xfer->direction == JTAG_READ_XFER)<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + tdi = UINT_MAX;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + else<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + tdi = data[index];<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Take your time to think how the above duplication can be avoided.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">I can't avoid this but it can changed to define:<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">#define ASPEED_JTAG_GET_TDI(direction, data) \<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"> (direction == JTAG_READ_XFER) ? UNIT_MAX : data; And use as tdi = ASPEED_JTAG_GET_TDI(xfer->direction, data[index]);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + }<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + }<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + tdo = aspeed_jtag_tck_cycle(aspeed_jtag, 1, tdi &<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> ASPEED_JTAG_DATA_MSB);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + data[index] |= tdo << (shift_bits %
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +ASPEED_JTAG_DATA_CHUNK_SIZE); }<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + if (endstate != JTAG_STATE_IDLE) {<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Why not to use positive check?<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Will restructure to have positive check<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">if (endstate == JTAG_STATE_IDLE) {<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">...<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">} else {<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">...<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">} <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + int i;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + for (i = 0; i <= xfer->length / BITS_PER_BYTE; i++) {<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + pos += snprintf(&dbg_str[pos], sizeof(dbg_str) - pos,<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + "0x%02x ", xfer_data[i]);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + }<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Oh, NO! Consider reading printk-formats (for %*ph) and other
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> documentation about available APIs.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Ok.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + if (!(aspeed_jtag->mode & JTAG_XFER_HW_MODE)) {<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + /* SW mode */<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> This is rather too complex to be in one function.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Will split to separate functions.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + } else {<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + /* hw mode */<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + aspeed_jtag_write(aspeed_jtag, 0, ASPEED_JTAG_SW);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + aspeed_jtag_xfer_hw(aspeed_jtag, xfer, data);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> For symmetry it might be another function.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + }<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + dev_dbg(aspeed_jtag->dev, "status %x\n", status);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Perhaps someone should become familiar with tracepoints?<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + dev_err(aspeed_jtag->dev, "irq status:%x\n",<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + status);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Huh, really?! SPAM.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">I will review and delete redundant debug messages.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> (I would drop it completely, though you may use ratelimited variant)<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + ret = IRQ_NONE;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + }<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + return ret;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +}<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + clk_prepare_enable(aspeed_jtag->pclk);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> This might fail.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Will add error check<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + dev_dbg(&pdev->dev, "IRQ %d.\n", aspeed_jtag->irq);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Noise even for debug.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Agree.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + err = jtag_register(jtag);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Perhaps we might have devm_ variant of this. Check how SPI framework
<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> deal with a such.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Jtag driver uses miscdevice and related misc_register and misc_deregister calls for creation and destruction. There is no device object prior to call to misc_register, which could be used in devm_jtag_register.<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +static int aspeed_jtag_remove(struct platform_device *pdev) {<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + struct jtag *jtag;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + jtag = platform_get_drvdata(pdev);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Usually we put this on one line<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">+<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + aspeed_jtag_deinit(pdev, jtag_priv(jtag));<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + jtag_unregister(jtag);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + jtag_free(jtag);<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > + return 0;<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> > +}<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> <o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> --<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> With Best Regards,<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">> Andy Shevchenko<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Best Regards,<o:p></o:p></span></p>
<p class="MsoPlainText"><span lang="EN-US">Oleksandr Shamray<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
</div>
</body>
</html>