Re: [patch v21 2/4] drivers: jtag: Add Aspeed SoC 24xx and 25xx families JTAG master driver

[Andy Shevchenko <andy.shevchenko@gmail.com>]

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.

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

?

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

> +}

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

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

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


> +       char          tdo;

Indentation.

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

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

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

> +       if (!(aspeed_jtag->mode & JTAG_XFER_HW_MODE)) {
> +               /* SW mode */

This is rather too complex to be in one function.

> +       } 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 would drop it completely, though you may use ratelimited variant)

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

> +       clk_prepare_enable(aspeed_jtag->pclk);

This might fail.

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

Noise even for debug.

> +       err = jtag_register(jtag);

Perhaps we might have devm_ variant of this. Check how SPI framework
deal with a such.

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