From: Oleksandr Shamray <oleksandrs@mellanox.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Arnd Bergmann" <arnd@arndb.de>,
"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
"linux-arm Mailing List" <linux-arm-kernel@lists.infradead.org>,
devicetree <devicetree@vger.kernel.org>,
"openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>,
"Joel Stanley" <joel@jms.id.au>, "Jiří Pírko" <jiri@resnulli.us>,
"Tobias Klauser" <tklauser@distanz.ch>,
"open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
"Vadim Pasternak" <vadimp@mellanox.com>,
system-sw-low-level <system-sw-low-level@mellanox.com>,
"Rob Herring" <robh+dt@kernel.org>,
"openocd-devel-owner@lists.sourceforge.net"
<openocd-devel-owner@lists.sourceforge.net>,
"linux-api@vger.kernel.org" <linux-api@vger.kernel.org>,
"David S. Miller" <davem@davemloft.net>,
"Mauro Carvalho Chehab" <mchehab@kernel.org>,
"Jiri Pirko" <jiri@mellanox.com>
Subject: RE: [patch v21 2/4] drivers: jtag: Add Aspeed SoC 24xx and 25xx families JTAG master driver
Date: Tue, 22 May 2018 15:00:17 +0000 [thread overview]
Message-ID: <AM5PR0501MB244981D83DD33BD29FD64924B1940@AM5PR0501MB2449.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <CAHp75Ve7EYiWBiE73i0CmyJhPMOdSKsGzCr0SUvta3Uya=Ov1Q@mail.gmail.com>
Hi Andy.
Thanks for review.
Please read my answers inline.
> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: 16 мая 2018 г. 0:00
> To: Oleksandr Shamray <oleksandrs@mellanox.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Arnd Bergmann
> <arnd@arndb.de>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org>; linux-arm Mailing List
> <linux-arm-kernel@lists.infradead.org>; devicetree
> <devicetree@vger.kernel.org>; openbmc@lists.ozlabs.org; Joel Stanley
> <joel@jms.id.au>; Jiří Pírko <jiri@resnulli.us>; Tobias Klauser
> <tklauser@distanz.ch>; open list:SERIAL DRIVERS <linux-
> serial@vger.kernel.org>; Vadim Pasternak <vadimp@mellanox.com>;
> system-sw-low-level <system-sw-low-level@mellanox.com>; Rob Herring
> <robh+dt@kernel.org>; openocd-devel-owner@lists.sourceforge.net;
> linux- api@vger.kernel.org; David S. Miller <davem@davemloft.net>;
> Mauro Carvalho Chehab <mchehab@kernel.org>; Jiri Pirko
> <jiri@mellanox.com>
> Subject: Re: [patch v21 2/4] drivers: jtag: Add Aspeed SoC 24xx and
> 25xx families JTAG master driver
>
> On Tue, May 15, 2018 at 5:21 PM, Oleksandr Shamray
> <oleksandrs@mellanox.com> wrote:
> > Driver adds support of Aspeed 2500/2400 series SOC JTAG master
> controller.
> >
> > Driver implements the following jtag ops:
> > - freq_get;
> > - freq_set;
> > - status_get;
> > - idle;
> > - xfer;
> >
> > It has been tested on Mellanox system with BMC equipped with Aspeed
> > 2520 SoC for programming CPLD devices.
>
> > +#define ASPEED_JTAG_DATA 0x00
> > +#define ASPEED_JTAG_INST 0x04
> > +#define ASPEED_JTAG_CTRL 0x08
> > +#define ASPEED_JTAG_ISR 0x0C
> > +#define ASPEED_JTAG_SW 0x10
> > +#define ASPEED_JTAG_TCK 0x14
> > +#define ASPEED_JTAG_EC 0x18
> > +
> > +#define ASPEED_JTAG_DATA_MSB 0x01
> > +#define ASPEED_JTAG_DATA_CHUNK_SIZE 0x20
>
>
> > +#define ASPEED_JTAG_IOUT_LEN(len) (ASPEED_JTAG_CTL_ENG_EN |\
> > + ASPEED_JTAG_CTL_ENG_OUT_EN
> > +|\
> > +
> > +ASPEED_JTAG_CTL_INST_LEN(len))
>
> Better to read
>
> #define MY_COOL_CONST_OR_MACRO(xxx) \
> ...
>
> > +#define ASPEED_JTAG_DOUT_LEN(len) (ASPEED_JTAG_CTL_ENG_EN
> |\
> > + ASPEED_JTAG_CTL_ENG_OUT_EN
> > + |\
> > +
> > +ASPEED_JTAG_CTL_DATA_LEN(len))
>
> Ditto.
Ok. Changed to:
#define ASPEED_JTAG_IOUT_LEN(len) \
(ASPEED_JTAG_CTL_ENG_EN | \
ASPEED_JTAG_CTL_ENG_OUT_EN | \
ASPEED_JTAG_CTL_INST_LEN(len))
#define ASPEED_JTAG_DOUT_LEN(len) \
(ASPEED_JTAG_CTL_ENG_EN | \
ASPEED_JTAG_CTL_ENG_OUT_EN | \
ASPEED_JTAG_CTL_DATA_LEN(len))
>
> > +static char *end_status_str[] = {"idle", "ir pause", "drpause"};
>
> > +static int aspeed_jtag_freq_set(struct jtag *jtag, u32 freq) {
> > + struct aspeed_jtag *aspeed_jtag = jtag_priv(jtag);
> > + unsigned long apb_frq;
> > + u32 tck_val;
> > + u16 div;
> > +
> > + apb_frq = clk_get_rate(aspeed_jtag->pclk);
>
> > + div = (apb_frq % freq == 0) ? (apb_frq / freq) - 1 :
> > + (apb_frq / freq);
>
> Isn't it the same as
>
> div = (apb_frq - 1) / freq;
>
> ?
>
Seems it is same. Thanks.
> > + tck_val = aspeed_jtag_read(aspeed_jtag, ASPEED_JTAG_TCK);
> > + aspeed_jtag_write(aspeed_jtag,
> > + (tck_val & ASPEED_JTAG_TCK_DIVISOR_MASK) | div,
> > + ASPEED_JTAG_TCK);
> > + return 0;
> > +}
>
> > +static void aspeed_jtag_sw_delay(struct aspeed_jtag *aspeed_jtag,
> > +int
> > +cnt) {
> > + int i;
> > +
> > + for (i = 0; i < cnt; i++)
> > + aspeed_jtag_read(aspeed_jtag, ASPEED_JTAG_SW);
>
> Isn't it readsl() (or how it's called, I don't remember).
>
No, readsl reads data into buffer. But in this place read used for make
software delay.
Aspeed jtag driver supports 2 modes:
1 - hw mode with the hardware controlled JTAG states
and pins
2 - with software controlled pins.
This part of code used in sw-mode and generates delay for JTAG bit-bang .
I will change it to ndelay().
> > +}
>
> > +static void aspeed_jtag_wait_instruction_pause(struct aspeed_jtag
> > +*aspeed_jtag) {
> > + wait_event_interruptible(aspeed_jtag->jtag_wq, aspeed_jtag->flag &
> > + ASPEED_JTAG_ISR_INST_PAUSE);
>
> In such cases I prefer to see a new line with a parameter in full.
> Check all places.
>
> > + aspeed_jtag->flag &= ~ASPEED_JTAG_ISR_INST_PAUSE; }
>
> > +static void aspeed_jtag_sm_cycle(struct aspeed_jtag *aspeed_jtag,
> > +const
> u8 *tms,
> > + int len) {
> > + int i;
> > +
> > + for (i = 0; i < len; i++)
> > + aspeed_jtag_tck_cycle(aspeed_jtag, tms[i], 0); }
> > +
> > +static void aspeed_jtag_run_test_idle_sw(struct aspeed_jtag
> *aspeed_jtag,
> > + struct jtag_run_test_idle
> > +*runtest) {
> > + static const u8 sm_pause_irpause[] = {1, 1, 1, 1, 0, 1, 0};
> > + static const u8 sm_pause_drpause[] = {1, 1, 1, 0, 1, 0};
> > + static const u8 sm_idle_irpause[] = {1, 1, 0, 1, 0};
> > + static const u8 sm_idle_drpause[] = {1, 0, 1, 0};
> > + static const u8 sm_pause_idle[] = {1, 1, 0};
> > + int i;
> > +
> > + /* SW mode from idle/pause-> to pause/idle */
> > + if (runtest->reset) {
> > + for (i = 0; i < ASPEED_JTAG_RESET_CNTR; i++)
> > + aspeed_jtag_tck_cycle(aspeed_jtag, 1, 0);
> > + }
>
> I would rather split this big switch to a few helper functions per
> each status of surrounding switch.
>
Ok.
Will do it.
> > +
> > + /* Stay on IDLE for at least TCK cycle */
> > + for (i = 0; i < runtest->tck; i++)
> > + aspeed_jtag_tck_cycle(aspeed_jtag, 0, 0); }
>
>
> > +/**
> > + * aspeed_jtag_run_test_idle:
> > + * JTAG reset: generates at least 9 TMS high and 1 TMS low to force
> > + * devices into Run-Test/Idle State.
> > + */
>
> It's rather broken kernel doc.
Deleted.
>
> > + aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_CTL_ENG_EN |
> > + ASPEED_JTAG_CTL_ENG_OUT_EN |
> > + ASPEED_JTAG_CTL_FORCE_TMS,
> > + ASPEED_JTAG_CTRL);
>
> > + aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_EC_GO_IDLE,
> > + ASPEED_JTAG_EC);
>
> > + aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_SW_MODE_EN |
> > + ASPEED_JTAG_SW_MODE_TDIO, ASPEED_JTAG_SW);
>
> Here you have permutations of flag some of which are repeatetive in
> the code. Perhaps make additional definitions instead.
> Check other similar places.
>
Ok. Will add defined for repeated flags
>
> > + char tdo;
>
> Indentation.
Ok.
>
> > + if (xfer->direction == JTAG_READ_XFER)
> > + tdi = UINT_MAX;
> > + else
> > + tdi = data[index];
>
> > + if (xfer->direction == JTAG_READ_XFER)
> > + tdi = UINT_MAX;
> > + else
> > + tdi = data[index];
>
> Take your time to think how the above duplication can be avoided.
>
In both cases data[] is different, so I should check it twice, but I will
change it to, macro like:
#define ASPEED_JTAG_GET_TDI(direction, data) \
(direction == JTAG_READ_XFER) ? UNIT_MAX : data
> > + }
> > + }
> > +
> > + tdo = aspeed_jtag_tck_cycle(aspeed_jtag, 1, tdi &
> ASPEED_JTAG_DATA_MSB);
> > + data[index] |= tdo << (shift_bits %
> > +ASPEED_JTAG_DATA_CHUNK_SIZE); }
>
>
> > + if (endstate != JTAG_STATE_IDLE) {
>
> Why not to use positive check?
>
Will restructure to have positive check
if (endstate == JTAG_STATE_IDLE) {
...
} else {
...
}
> > + int i;
> > +
> > + for (i = 0; i <= xfer->length / BITS_PER_BYTE; i++) {
> > + pos += snprintf(&dbg_str[pos], sizeof(dbg_str) - pos,
> > + "0x%02x ", xfer_data[i]);
> > + }
>
> Oh, NO! Consider reading printk-formats (for %*ph) and other
> documentation about available APIs.
Ok.
>
> > + if (!(aspeed_jtag->mode & JTAG_XFER_HW_MODE)) {
> > + /* SW mode */
>
> This is rather too complex to be in one function.
>
Will split to separate functions.
> > + } else {
>
> > + /* hw mode */
> > + aspeed_jtag_write(aspeed_jtag, 0, ASPEED_JTAG_SW);
> > + aspeed_jtag_xfer_hw(aspeed_jtag, xfer, data);
>
> For symmetry it might be another function.
>
> > + }
>
> > + dev_dbg(aspeed_jtag->dev, "status %x\n", status);
>
> Perhaps someone should become familiar with tracepoints?
>
> > + dev_err(aspeed_jtag->dev, "irq status:%x\n",
> > + status);
>
>
> Huh, really?! SPAM.
I will review and delete redundant debug messages.
>
> (I would drop it completely, though you may use ratelimited variant)
>
> > + ret = IRQ_NONE;
> > + }
> > + return ret;
> > +}
>
> > + clk_prepare_enable(aspeed_jtag->pclk);
>
> This might fail.
Will add error check
>
> > + dev_dbg(&pdev->dev, "IRQ %d.\n", aspeed_jtag->irq);
>
> Noise even for debug.
Agree.
>
> > + err = jtag_register(jtag);
>
> Perhaps we might have devm_ variant of this. Check how SPI framework
> deal with a such.
>
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.
> > +static int aspeed_jtag_remove(struct platform_device *pdev) {
>
> > + struct jtag *jtag;
> > +
> > + jtag = platform_get_drvdata(pdev);
>
> Usually we put this on one line
+
>
> > + aspeed_jtag_deinit(pdev, jtag_priv(jtag));
> > + jtag_unregister(jtag);
> > + jtag_free(jtag);
> > + return 0;
> > +}
>
>
> --
> With Best Regards,
> Andy Shevchenko
Best Regards,
Oleksandr Shamray
next prev parent reply other threads:[~2018-05-22 15:00 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-15 14:21 [patch v21 0/4] JTAG driver introduction Oleksandr Shamray
2018-05-15 14:21 ` [patch v21 1/4] drivers: jtag: Add JTAG core driver Oleksandr Shamray
2018-05-15 21:21 ` Andy Shevchenko
2018-05-22 15:00 ` Oleksandr Shamray
2018-05-15 14:21 ` [patch v21 2/4] drivers: jtag: Add Aspeed SoC 24xx and 25xx families JTAG master driver Oleksandr Shamray
2018-05-15 21:00 ` Andy Shevchenko
2018-05-22 15:00 ` Oleksandr Shamray [this message]
2018-05-22 20:21 ` Andy Shevchenko
2018-05-15 14:21 ` [patch v21 3/4] Documentation: jtag: Add bindings for " Oleksandr Shamray
2018-05-15 14:21 ` [patch v21 4/4] Documentation: jtag: Add ABI documentation Oleksandr Shamray
2018-05-16 18:16 ` Randy Dunlap
2018-05-22 13:08 [patch v21 2/4] drivers: jtag: Add Aspeed SoC 24xx and 25xx families JTAG master driver Oleksandr Shamray
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AM5PR0501MB244981D83DD33BD29FD64924B1940@AM5PR0501MB2449.eurprd05.prod.outlook.com \
--to=oleksandrs@mellanox.com \
--cc=andy.shevchenko@gmail.com \
--cc=arnd@arndb.de \
--cc=davem@davemloft.net \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=jiri@mellanox.com \
--cc=jiri@resnulli.us \
--cc=joel@jms.id.au \
--cc=linux-api@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=openbmc@lists.ozlabs.org \
--cc=openocd-devel-owner@lists.sourceforge.net \
--cc=robh+dt@kernel.org \
--cc=system-sw-low-level@mellanox.com \
--cc=tklauser@distanz.ch \
--cc=vadimp@mellanox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).