On Tue, May 22, 2018 at 6:00 PM, Oleksandr Shamray
[off-list ref] wrote:

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

What about

#define _JTAG_OUT_ENABLE \
( _ENG_EN | _ENG_OUT_EN)

#define _IOUT_LEN(len) \
 (_ENABLE | _INST_LEN(len))

#define _DOUT_LEN(len) \
...

?

quoted

quoted

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

Though be careful if apb_frq == 0.
In either case the hw will be screwed, but differently.

quoted

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

Perhaps choose better name for data, b/c in the above you are using data[index].

quoted

quoted

+               dev_err(aspeed_jtag->dev, "irq status:%x\n",
+                       status);

quoted

Huh, really?! SPAM.

I will review and delete redundant debug messages.

Just to be sure you got a point. This is interrupt context. Imagine
what might go wrong.

quoted

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.

Same question as per previous patch.

-- 
With Best Regards,
Andy Shevchenko