lkml.org 
[lkml]   [2018]   [May]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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.

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

\
 
 \ /
  Last update: 2018-05-15 23:00    [W:0.119 / U:4.252 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site