Hi Andy. 
Thanks for review.

Please read my answers inline.

-----Original Message-----
From: Andy Shevchenko [mailto:andy.shevchenko at gmail.com]
Sent: 16 ??? 2018 ?. 0:00
To: Oleksandr Shamray <redacted>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Arnd Bergmann 
[off-list ref]; Linux Kernel Mailing List 
[off-list ref]; linux-arm Mailing List 
[off-list ref]; devicetree 
[off-list ref]; openbmc at lists.ozlabs.org; Joel Stanley 
[off-list ref]; Ji?? P?rko [off-list ref]; Tobias Klauser 
[off-list ref]; open list:SERIAL DRIVERS <linux- 
serial at vger.kernel.org>; Vadim Pasternak [off-list ref]; 
system-sw-low-level [off-list ref]; Rob Herring 
[off-list ref]; openocd-devel-owner at lists.sourceforge.net; 
linux- api at vger.kernel.org; David S. Miller [off-list ref]; 
Mauro Carvalho Chehab [off-list ref]; Jiri Pirko 
[off-list ref]
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 
[off-list ref] wrote:

quoted

Driver adds support of Aspeed 2500/2400 series SOC JTAG master

controller.

quoted

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.

quoted

+#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

quoted

+#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) \
 ...

quoted

+#define ASPEED_JTAG_DOUT_LEN(len)      (ASPEED_JTAG_CTL_ENG_EN

|\

quoted

+                                        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))

quoted

+static char *end_status_str[] = {"idle", "ir pause", "drpause"};

quoted

+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);

quoted

+       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.

quoted

+       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;
+}

quoted

+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().

quoted

+}

quoted

+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.

quoted

+       aspeed_jtag->flag &= ~ASPEED_JTAG_ISR_INST_PAUSE; }

quoted

+static void aspeed_jtag_sm_cycle(struct aspeed_jtag *aspeed_jtag, 
+const

u8 *tms,

quoted

+                                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,

quoted

+                                        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.

quoted

+
+       /* Stay on IDLE for at least  TCK cycle */
+       for (i = 0; i < runtest->tck; i++)
+               aspeed_jtag_tck_cycle(aspeed_jtag, 0, 0); }

quoted

+/**
+ * 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.

quoted

+               aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_CTL_ENG_EN |
+                                 ASPEED_JTAG_CTL_ENG_OUT_EN |
+                                 ASPEED_JTAG_CTL_FORCE_TMS, 
+ ASPEED_JTAG_CTRL);

quoted

+               aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_EC_GO_IDLE,
+                                 ASPEED_JTAG_EC);

quoted

+       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

quoted

+       char          tdo;

Indentation.

Ok.

quoted

+       if (xfer->direction == JTAG_READ_XFER)
+               tdi = UINT_MAX;
+       else
+               tdi = data[index];

quoted

+                       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

quoted

+               }
+       }
+
+       tdo = aspeed_jtag_tck_cycle(aspeed_jtag, 1, tdi &

ASPEED_JTAG_DATA_MSB);

quoted

+       data[index] |= tdo << (shift_bits % 
+ASPEED_JTAG_DATA_CHUNK_SIZE); }

quoted

+       if (endstate != JTAG_STATE_IDLE) {

Why not to use positive check?

Will restructure to have positive check
if (endstate == JTAG_STATE_IDLE) {
...
} else {
...
} 

quoted

+       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.

quoted

+       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.

quoted

+       } else {

quoted

+               /* 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.

quoted

+       }

quoted

+       dev_dbg(aspeed_jtag->dev, "status %x\n", status);

Perhaps someone should become familiar with tracepoints?

quoted

+               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)

quoted

+               ret = IRQ_NONE;
+       }
+       return ret;
+}

quoted

+       clk_prepare_enable(aspeed_jtag->pclk);

This might fail.

Will add error check

quoted

+       dev_dbg(&pdev->dev, "IRQ %d.\n", aspeed_jtag->irq);

Noise even for debug.

Agree.

quoted

+       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.

quoted

+static int aspeed_jtag_remove(struct platform_device *pdev) {

quoted

+       struct jtag *jtag;
+
+       jtag = platform_get_drvdata(pdev);

Usually we put this on one line

+

quoted

+       aspeed_jtag_deinit(pdev, jtag_priv(jtag));
+       jtag_unregister(jtag);
+       jtag_free(jtag);
+       return 0;
+}

--
With Best Regards,
Andy Shevchenko

Best Regards,
Oleksandr Shamray