* Re: [PATCH v2 3/7] soc: qcom: Add GENI based QUP Wrapper driver
From: Karthik Ramasubramanian @ 2018-01-31 19:02 UTC (permalink / raw)
To: Rob Herring, Bjorn Andersson
Cc: Rajendra Nayak, corbet, andy.gross, david.brown, mark.rutland,
wsa, gregkh, linux-doc, linux-arm-msm, devicetree, linux-i2c,
linux-serial, jslaby, Sagar Dharia, Girish Mahadevan
In-Reply-To: <20180119225708.4mo6fiy5zyd6kscj@rob-hp-laptop>
On 1/19/2018 3:57 PM, Rob Herring wrote:
> On Thu, Jan 18, 2018 at 08:57:45AM -0800, Bjorn Andersson wrote:
>> On Thu 18 Jan 01:13 PST 2018, Rajendra Nayak wrote:
>>
>>> []..
>>>
>>>>> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
>>>>> new file mode 100644
>>>>> index 0000000..3f43582
>>>>> --- /dev/null
>>>>> +++ b/drivers/soc/qcom/qcom-geni-se.c
>>>>> @@ -0,0 +1,1016 @@
>>>>> +/*
>>>>> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>> + * it under the terms of the GNU General Public License version 2 and
>>>>> + * only version 2 as published by the Free Software Foundation.
>>>>> + *
>>>>> + * This program is distributed in the hope that it will be useful,
>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>>>> + * GNU General Public License for more details.
>>>>> + *
>>>>> + */
>>>>
>>>> Please use SPDX style license header, i.e. this file should start with:
>>>>
>>>> // SPDX-License-Identifier: GPL-2.0
>>>> /*
>>>> * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>>>> */
>>>
>>> Looks like Mark Brown commented elsewhere [1] that we should use the C++
>>> commenting style even for the Linux Foundation copyright?
>>>
>>> [1] https://marc.info/?l=linux-clk&m=151497978626135&w=2
>>>
>>
>> While I can agree with Mark on the ugliness of the mixed commenting
>> style, this is the style that I found communicated and is what you find
>> in other files.
>
> Well, that's pretty new guidance. Moving target...
>
> Given that Linus said '//' comments are the only thing C++ got right, I
> expect to see more of them.
I believe that in the source file I have to use C++ style comments(as
per this discussion) and in the header file I have to use C-style
comments (as per https://lwn.net/Articles/739183/ for reasons related to
tooling). Please correct me otherwise.
>
> Rob
>
Regards,
Karthik.
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v2 0/7] Introduce GENI SE Controller Driver
From: Karthik Ramasubramanian @ 2018-01-31 18:59 UTC (permalink / raw)
To: Randy Dunlap, corbet, andy.gross, david.brown, robh+dt,
mark.rutland, wsa, gregkh
Cc: linux-doc, linux-arm-msm, devicetree, linux-i2c, linux-serial,
jslaby
In-Reply-To: <14fcdf68-e4ee-2182-2c3c-cc446c2c4d25@infradead.org>
On 1/19/2018 11:32 AM, Randy Dunlap wrote:
> On 01/12/2018 05:05 PM, Karthikeyan Ramasubramanian wrote:
>> Generic Interface (GENI) firmware based Qualcomm Universal Peripheral (QUP)
>> Wrapper is a next generation programmable module for supporting a wide
>> range of serial interfaces like UART, SPI, I2C, I3C, etc. A single QUP
>> module can provide upto 8 Serial Interfaces using its internal Serial
>> Engines (SE). The protocol supported by each interface is determined by
>> the firmware loaded to the Serial Engine.
>>
>> This patch series introduces GENI SE Driver to manage the GENI based QUP
>> Wrapper and the common aspects of all SEs inside the QUP Wrapper. This
>> patch series also introduces the UART and I2C Controller drivers to
>> drive the SEs that are programmed with the respective protocols.
>
> Hi,
>
> Will there be follow-up drivers for SPI, I3C, etc.?
Yes.
>
> Thanks.
>
>> [v2]
>> * Updated device tree bindings to describe the hardware
>> * Updated SE DT node as child node of QUP Wrapper DT node
>> * Moved common AHB clocks to QUP Wrapper DT node
>> * Use the standard "clock-frequency" I2C property
>> * Update compatible field in UART Controller to reflect hardware manual
>> * Addressed other device tree binding specific comments from Rob Herring
>>
>> Karthikeyan Ramasubramanian (7):
>> qcom-geni-se: Add QCOM GENI SE Driver summary
>> dt-bindings: soc: qcom: Add device tree binding for GENI SE
>> soc: qcom: Add GENI based QUP Wrapper driver
>> dt-bindings: i2c: Add device tree bindings for GENI I2C Controller
>> i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C
>> controller
>> dt-bindings: serial: Add bindings for GENI based UART Controller
>> tty: serial: msm_geni_serial: Add serial driver support for GENI based
>> QUP
>>
>> .../devicetree/bindings/i2c/i2c-qcom-geni.txt | 35 +
>> .../devicetree/bindings/serial/qcom,geni-uart.txt | 29 +
>> .../devicetree/bindings/soc/qcom/qcom,geni-se.txt | 66 +
>> Documentation/qcom-geni-se.txt | 56 +
>> drivers/i2c/busses/Kconfig | 10 +
>> drivers/i2c/busses/Makefile | 1 +
>> drivers/i2c/busses/i2c-qcom-geni.c | 656 +++++++++
>> drivers/soc/qcom/Kconfig | 8 +
>> drivers/soc/qcom/Makefile | 1 +
>> drivers/soc/qcom/qcom-geni-se.c | 1016 ++++++++++++++
>> drivers/tty/serial/Kconfig | 10 +
>> drivers/tty/serial/Makefile | 1 +
>> drivers/tty/serial/qcom_geni_serial.c | 1414 ++++++++++++++++++++
>> include/linux/qcom-geni-se.h | 807 +++++++++++
>> 14 files changed, 4110 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-qcom-geni.txt
>> create mode 100644 Documentation/devicetree/bindings/serial/qcom,geni-uart.txt
>> create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>> create mode 100644 Documentation/qcom-geni-se.txt
>> create mode 100644 drivers/i2c/busses/i2c-qcom-geni.c
>> create mode 100644 drivers/soc/qcom/qcom-geni-se.c
>> create mode 100644 drivers/tty/serial/qcom_geni_serial.c
>> create mode 100644 include/linux/qcom-geni-se.h
>>
>
>
Regards,
Karthik.
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v2 3/7] soc: qcom: Add GENI based QUP Wrapper driver
From: Karthik Ramasubramanian @ 2018-01-31 18:58 UTC (permalink / raw)
To: Bjorn Andersson
Cc: corbet, andy.gross, david.brown, robh+dt, mark.rutland, wsa,
gregkh, linux-doc, linux-arm-msm, devicetree, linux-i2c,
linux-serial, jslaby, Sagar Dharia, Girish Mahadevan
In-Reply-To: <20180117062002.GA6620@minitux>
On 1/16/2018 11:20 PM, Bjorn Andersson wrote:
> On Fri 12 Jan 17:05 PST 2018, Karthikeyan Ramasubramanian wrote:
>
>> This driver manages the Generic Interface (GENI) firmware based Qualcomm
>> Universal Peripheral (QUP) Wrapper. GENI based QUP is the next generation
>> programmable module composed of multiple Serial Engines (SE) and supports
>> a wide range of serial interfaces like UART, SPI, I2C, I3C, etc. This
>> driver also enables managing the serial interface independent aspects of
>> Serial Engines.
>>
>> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
>> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
>> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
>> ---
>> drivers/soc/qcom/Kconfig | 8 +
>> drivers/soc/qcom/Makefile | 1 +
>> drivers/soc/qcom/qcom-geni-se.c | 1016 +++++++++++++++++++++++++++++++++++++++
>
> I'm not aware of any non-serial-engine "geni" at Qualcomm, so can we
> drop the "se" throughout this driver?
Currently GENI is used to program just the serial engines. But it is not
restricted to that. It can be used to program other hardware cores.
Hence keeping "se" will help to clarify that this driver is meant for
GENI based serial engines only.
>
>> include/linux/qcom-geni-se.h | 807 +++++++++++++++++++++++++++++++
>> 4 files changed, 1832 insertions(+)
>> create mode 100644 drivers/soc/qcom/qcom-geni-se.c
>> create mode 100644 include/linux/qcom-geni-se.h
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index b81374b..b306d51 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -3,6 +3,14 @@
>> #
>> menu "Qualcomm SoC drivers"
>>
>> +config QCOM_GENI_SE
>> + tristate "QCOM GENI Serial Engine Driver"
>
> Options in drivers/soc/qcom/Kconfig should have "depends on ARCH_QCOM",
> as the makefile in this directory won't be processed when !ARCH_QCOM.
I will update the config to depend on ARCH_QCOM.
>
>> + help
>> + This module is used to manage Generic Interface (GENI) firmware based
>> + Qualcomm Technologies, Inc. Universal Peripheral (QUP) Wrapper. This
>> + module is also used to manage the common aspects of multiple Serial
>> + Engines present in the QUP.
>> +
>> config QCOM_GLINK_SSR
>> tristate "Qualcomm Glink SSR driver"
>> depends on RPMSG
>> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> index 40c56f6..74d5db8 100644
>> --- a/drivers/soc/qcom/Makefile
>> +++ b/drivers/soc/qcom/Makefile
>> @@ -1,4 +1,5 @@
>> # SPDX-License-Identifier: GPL-2.0
>> +obj-$(CONFIG_QCOM_GENI_SE) += qcom-geni-se.o
>> obj-$(CONFIG_QCOM_GLINK_SSR) += glink_ssr.o
>> obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o
>> obj-$(CONFIG_QCOM_MDT_LOADER) += mdt_loader.o
>> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
>> new file mode 100644
>> index 0000000..3f43582
>> --- /dev/null
>> +++ b/drivers/soc/qcom/qcom-geni-se.c
>> @@ -0,0 +1,1016 @@
>> +/*
>> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>
> Please use SPDX style license header, i.e. this file should start with:
>
> // SPDX-License-Identifier: GPL-2.0
> /*
> * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> */
>
>> +
>> +#include <linux/clk.h>
>> +#include <linux/slab.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/io.h>
>> +#include <linux/list.h>
>
> This isn't used.
>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/pm_runtime.h>
>
> Is this used?
>
>> +#include <linux/qcom-geni-se.h>
>> +#include <linux/spinlock.h>
>
> Is this used?
I will remove including unused header files.
>
>> +
>> +#define MAX_CLK_PERF_LEVEL 32
>> +
>> +/**
>> + * @struct geni_se_device - Data structure to represent the QUP Wrapper Core
>
> Make this name indicate that this is the wrapper; e.g. struct geni_wrapper
geni_wrapper makes sense.
>
>> + * @dev: Device pointer of the QUP wrapper core.
>> + * @base: Base address of this instance of QUP wrapper core.
>> + * @m_ahb_clk: Handle to the primary AHB clock.
>> + * @s_ahb_clk: Handle to the secondary AHB clock.
>> + * @geni_dev_lock: Lock to protect the device elements.
>> + * @num_clk_levels: Number of valid clock levels in clk_perf_tbl.
>> + * @clk_perf_tbl: Table of clock frequency input to Serial Engine clock.
>> + */
>> +struct geni_se_device {
>> + struct device *dev;
>> + void __iomem *base;
>> + struct clk *m_ahb_clk;
>> + struct clk *s_ahb_clk;
>
> As these two clocks always seems to be operated in tandem use
> clk_bulk_data to keep track of them and use the clk_bulk*() operations
> to simplify your prepare/unprepare sites.
I will use the clk_bulk* approach.
>
>> + struct mutex geni_dev_lock;
>
> There's only one lock and it should be obvious from context that it's
> the lock of the geni_se_device, so naming this "lock" should be
> sufficient.
I will rename it to "lock".
>
>> + unsigned int num_clk_levels;
>> + unsigned long *clk_perf_tbl;
>> +};
>> +
>> +/* Offset of QUP Hardware Version Register */
>> +#define QUP_HW_VER (0x4)
>
> Drop the parenthesis. It would be nice if this define indicated that
> this is a register and not a value. E.g. call it QUP_HW_VER_REG.
Ok.
>
>> +
>> +#define HW_VER_MAJOR_MASK GENMASK(31, 28)
>> +#define HW_VER_MAJOR_SHFT 28
>> +#define HW_VER_MINOR_MASK GENMASK(27, 16)
>> +#define HW_VER_MINOR_SHFT 16
>> +#define HW_VER_STEP_MASK GENMASK(15, 0)
>> +
>> +/**
>> + * geni_read_reg_nolog() - Helper function to read from a GENI register
>> + * @base: Base address of the serial engine's register block.
>> + * @offset: Offset within the serial engine's register block.
>> + *
>> + * Return: Return the contents of the register.
>> + */
>> +unsigned int geni_read_reg_nolog(void __iomem *base, int offset)
>
> Remove this function.
Ok.
>
>> +{
>> + return readl_relaxed(base + offset);
>> +}
>> +EXPORT_SYMBOL(geni_read_reg_nolog);
>> +
>> +/**
>> + * geni_write_reg_nolog() - Helper function to write into a GENI register
>> + * @value: Value to be written into the register.
>> + * @base: Base address of the serial engine's register block.
>> + * @offset: Offset within the serial engine's register block.
>> + */
>> +void geni_write_reg_nolog(unsigned int value, void __iomem *base, int offset)
>
> Ditto
Ok.
>
>> +{
>> + return writel_relaxed(value, (base + offset));
>> +}
>> +EXPORT_SYMBOL(geni_write_reg_nolog);
>> +
>> +/**
>> + * geni_read_reg() - Helper function to read from a GENI register
>> + * @base: Base address of the serial engine's register block.
>> + * @offset: Offset within the serial engine's register block.
>> + *
>> + * Return: Return the contents of the register.
>
> Drop the extra spaces after "Return:" in all your kerneldoc.
Ok.
>
>> + */
>> +unsigned int geni_read_reg(void __iomem *base, int offset)
>> +{
>> + return readl_relaxed(base + offset);
>> +}
>> +EXPORT_SYMBOL(geni_read_reg);
>> +
>> +/**
>> + * geni_write_reg() - Helper function to write into a GENI register
>> + * @value: Value to be written into the register.
>> + * @base: Base address of the serial engine's register block.
>> + * @offset: Offset within the serial engine's register block.
>> + */
>> +void geni_write_reg(unsigned int value, void __iomem *base, int offset)
>> +{
>> + return writel_relaxed(value, (base + offset));
>
> As written, this function adds no value and I find it quite confusing
> that this is used both for read/writes on the wrapper as well as the
> individual functions.Ok.
>
> Compare:
>
> geni_write_reg(common_geni_m_irq_en, base, SE_GENI_M_IRQ_EN);
>
> with just inlining:
>
> writel(common_geni_m_irq_en, base + SE_GENI_M_IRQ_EN);
>
>> +}
>> +EXPORT_SYMBOL(geni_write_reg);
>> +
>> +/**
>> + * geni_get_qup_hw_version() - Read the QUP wrapper Hardware version
>> + * @wrapper_dev: Pointer to the corresponding QUP wrapper core.
>> + * @major: Buffer for Major Version field.
>> + * @minor: Buffer for Minor Version field.
>> + * @step: Buffer for Step Version field.
>> + *
>> + * Return: 0 on success, standard Linux error codes on failure/error.
>> + */
>> +int geni_get_qup_hw_version(struct device *wrapper_dev, unsigned int *major,
>> + unsigned int *minor, unsigned int *step)
>> +{
>> + unsigned int version;
>> + struct geni_se_device *geni_se_dev;
>> +
>> + if (!wrapper_dev || !major || !minor || !step)
>> + return -EINVAL;
>
> This would only happen during development, as the engineer calls the
> function incorrectly. Consider it a contract that these has to be valid
> and skip the checks.
Ok.
>
>> +
>> + geni_se_dev = dev_get_drvdata(wrapper_dev);
>> + if (unlikely(!geni_se_dev))
>> + return -ENODEV;
>
> Make the children acquire the geni_se_dev handle (keep the type opaque)
> and pass that instead of wrapper_dev. Then you can just drop this chunk.
Ok.
>
>> +
>> + version = geni_read_reg(geni_se_dev->base, QUP_HW_VER);
>> + *major = (version & HW_VER_MAJOR_MASK) >> HW_VER_MAJOR_SHFT;
>> + *minor = (version & HW_VER_MINOR_MASK) >> HW_VER_MINOR_SHFT;
>> + *step = version & HW_VER_STEP_MASK;
>> + return 0;
>
> If you implement these two changes there's no way this function can
> fail, so you don't have to return a value here.
Ok.
>
>> +}
>> +EXPORT_SYMBOL(geni_get_qup_hw_version);
>> +
>> +/**
>> + * geni_se_get_proto() - Read the protocol configured for a serial engine
>> + * @base: Base address of the serial engine's register block.
>> + *
>> + * Return: Protocol value as configured in the serial engine.
>> + */
>> +int geni_se_get_proto(void __iomem *base)
>
> I keep reading this as "geni set get proto", you should be fine just
> dropping _se_ from these function names. And if you want to denote that
> it's the Qualcomm GENI then make it qcom_geni_*.
"_se" prefix is used to help differentiate operations on a serial engine
from those on the wrapper. I can rename it as geni_se_read_proto to
avoid the confusion.
>
> But more importantly, it is not obvious when reading this driver that
> the typeless "base" being passed is that of the individual functional
> block, and not the wrapper.
>
> If you want to do this in an "object oriented" fashion create a struct
> that's common for all geni instances, include it in the context of the
> individual function devices and pass it into these functions.
There is a structure named geni_se_rsc to group resources associated
with a serial engine. I can rename it to geni_serial_engine and include
the opaque "base" address in that structure. That structure can be
passed as an input parameter to all the serial engine operations.
>
>> +{
>> + int proto;
>> +
>> + proto = ((geni_read_reg(base, GENI_FW_REVISION_RO)
>> + & FW_REV_PROTOCOL_MSK) >> FW_REV_PROTOCOL_SHFT);
>
> Don't do everything in one go, as you can't fit it on one line anyways.
> Writing it like this instead makes it easier to read:
>
> u32 val;
>
> val = readl(base + GENI_FW_S_REVISION_RO);
>
> return (val & FW_REV_PROTOCOL_MSK) >> FW_REV_PROTOCOL_SHFT;
Ok.
>
>> + return proto;
>> +}
>> +EXPORT_SYMBOL(geni_se_get_proto);
>> +
>> +static int geni_se_irq_en(void __iomem *base)
>> +{
>> + unsigned int common_geni_m_irq_en;
>> + unsigned int common_geni_s_irq_en;
>
> The size of these values isn't unsigned int, it's u32. And if you give
> them a shorter name the function becomes easier to read.
>
> Further more, as you don't care about ordering you don't need two of
> them either. So you should be able to just do:
>
> u32 val;
>
> val = readl(base + SE_GENI_M_IRQ_EN);
> val |= M_COMMON_GENI_M_IRQ_EN;
> writel(val, base + SE_GENI_M_IRQ_EN);
>
> val = readl(base + SE_GENI_S_IRQ_EN);
> val |= S_COMMON_GENI_S_IRQ_EN;
> writel(val, base + SE_GENI_S_IRQ_EN);
Ok.
>
>> +
>> + common_geni_m_irq_en = geni_read_reg(base, SE_GENI_M_IRQ_EN);
>> + common_geni_s_irq_en = geni_read_reg(base, SE_GENI_S_IRQ_EN);
>> + /* Common to all modes */
>> + common_geni_m_irq_en |= M_COMMON_GENI_M_IRQ_EN;
>> + common_geni_s_irq_en |= S_COMMON_GENI_S_IRQ_EN;
>> +
>> + geni_write_reg(common_geni_m_irq_en, base, SE_GENI_M_IRQ_EN);
>> + geni_write_reg(common_geni_s_irq_en, base, SE_GENI_S_IRQ_EN);
>> + return 0;
>
> And as this can't fail there's no reason to have a return value.
Ok.
>
>> +}
>> +
>> +
>> +static void geni_se_set_rx_rfr_wm(void __iomem *base, unsigned int rx_wm,
>> + unsigned int rx_rfr)
>> +{
>> + geni_write_reg(rx_wm, base, SE_GENI_RX_WATERMARK_REG);
>> + geni_write_reg(rx_rfr, base, SE_GENI_RX_RFR_WATERMARK_REG);
>> +}
>> +
>> +static int geni_se_io_set_mode(void __iomem *base)
>> +{
>> + unsigned int io_mode;
>> + unsigned int geni_dma_mode;
>> +
>> + io_mode = geni_read_reg(base, SE_IRQ_EN);
>> + geni_dma_mode = geni_read_reg(base, SE_GENI_DMA_MODE_EN);
>> +
>> + io_mode |= (GENI_M_IRQ_EN | GENI_S_IRQ_EN);
>> + io_mode |= (DMA_TX_IRQ_EN | DMA_RX_IRQ_EN);
>> + geni_dma_mode &= ~GENI_DMA_MODE_EN;
>> +
>> + geni_write_reg(io_mode, base, SE_IRQ_EN);
>> + geni_write_reg(geni_dma_mode, base, SE_GENI_DMA_MODE_EN);
>> + geni_write_reg(0, base, SE_GSI_EVENT_EN);
>> + return 0;
>
> As this function can't fail, don't return a value.
Ok.
>
>> +}
>> +
>> +static void geni_se_io_init(void __iomem *base)
>> +{
>> + unsigned int io_op_ctrl;
>> + unsigned int geni_cgc_ctrl;
>> + unsigned int dma_general_cfg;
>
> These are all u32, be explicit.
Ok.
>
>> +
>> + geni_cgc_ctrl = geni_read_reg(base, GENI_CGC_CTRL);
>> + dma_general_cfg = geni_read_reg(base, SE_DMA_GENERAL_CFG);
>> + geni_cgc_ctrl |= DEFAULT_CGC_EN;
>> + dma_general_cfg |= (AHB_SEC_SLV_CLK_CGC_ON | DMA_AHB_SLV_CFG_ON |
>> + DMA_TX_CLK_CGC_ON | DMA_RX_CLK_CGC_ON);
>
> Drop the parenthesis and there's no harm in making multiple assignments
> in favour of splitting the line.
Ok.
>
>> + io_op_ctrl = DEFAULT_IO_OUTPUT_CTRL_MSK;
>> + geni_write_reg(geni_cgc_ctrl, base, GENI_CGC_CTRL);
>> + geni_write_reg(dma_general_cfg, base, SE_DMA_GENERAL_CFG);
>
> Is there a reason why this chunk of code is a mix of 3 independent
> register updates?
I am not sure I understand the context of your question. This is how the
hardware programming manual is describing to program the registers as
part of initializing a serial engine. Please let me know if this is not
the information you are looking for.
>
>> +
>> + geni_write_reg(io_op_ctrl, base, GENI_OUTPUT_CTRL);
>> + geni_write_reg(FORCE_DEFAULT, base, GENI_FORCE_DEFAULT_REG);
>> +}
>> +
>> +/**
>> + * geni_se_init() - Initialize the GENI Serial Engine
>> + * @base: Base address of the serial engine's register block.
>> + * @rx_wm: Receive watermark to be configured.
>> + * @rx_rfr_wm: Ready-for-receive watermark to be configured.
>
> What's the unit for these?
The unit is the number of hardware FIFO words.
>
>> + *
>> + * This function is used to initialize the GENI serial engine, configure
>> + * receive watermark and ready-for-receive watermarks.
>> + *
>> + * Return: 0 on success, standard Linux error codes on failure/error.
>
> As neither geni_se_io_set_mode() nor geni_se_irq_en() can fail there's
> no way geni_se_init() can fail and as such you don't need a return value
> of this function.
Ok.
>
>> + */
>> +int geni_se_init(void __iomem *base, unsigned int rx_wm, unsigned int rx_rfr)
>> +{
>> + int ret;
>> +
>> + geni_se_io_init(base);
>> + ret = geni_se_io_set_mode(base);
>> + if (ret)
>> + return ret;
>> +
>> + geni_se_set_rx_rfr_wm(base, rx_wm, rx_rfr);
>> + ret = geni_se_irq_en(base);
>
> Just inline these two functions here.
>
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(geni_se_init);
>
> This is an excellent candidate for initializing a common "geni
> function"-struct shared among the children, i.e. make this:
>
> void geni_init_port(struct geni_port *port, struct device *dev,
> size_t rx_wm, rfr_wm);
>
> And have this do the ioremap of the memory of dev and initialize some
> common "geni_port" struct, then you can pass this to some set of
> geni_port_*() helper functions.
Sure, this sounds like a good place to do ioremap of children and
initialize common serial engine resources.
>
>> +
>> +static int geni_se_select_fifo_mode(void __iomem *base)
>> +{
>> + int proto = geni_se_get_proto(base);
>> + unsigned int common_geni_m_irq_en;
>> + unsigned int common_geni_s_irq_en;
>> + unsigned int geni_dma_mode;
>
> These are of type u32.
>
> If you use more succinct names the function will be easier to read; what
> about master, slave and mode? (Or m_en, s_en and mode)
Ok.
>
>> +
>> + geni_write_reg(0, base, SE_GSI_EVENT_EN);
>> + geni_write_reg(0xFFFFFFFF, base, SE_GENI_M_IRQ_CLEAR);
>> + geni_write_reg(0xFFFFFFFF, base, SE_GENI_S_IRQ_CLEAR);
>> + geni_write_reg(0xFFFFFFFF, base, SE_DMA_TX_IRQ_CLR);
>> + geni_write_reg(0xFFFFFFFF, base, SE_DMA_RX_IRQ_CLR);
>> + geni_write_reg(0xFFFFFFFF, base, SE_IRQ_EN);
>
> Use lowercase for the hex constants.
Ok.
>
>> +
>> + common_geni_m_irq_en = geni_read_reg(base, SE_GENI_M_IRQ_EN);
>> + common_geni_s_irq_en = geni_read_reg(base, SE_GENI_S_IRQ_EN);
>> + geni_dma_mode = geni_read_reg(base, SE_GENI_DMA_MODE_EN);
>> + if (proto != UART) {
>> + common_geni_m_irq_en |=
>> + (M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN |
>> + M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
>
> Drop the parenthesis, split the assignment in multiple to make things
> not span 3 lines.
Ok.
>
>> + common_geni_s_irq_en |= S_CMD_DONE_EN;
>> + }
>> + geni_dma_mode &= ~GENI_DMA_MODE_EN;
>
> Please group things that are related.
>
> The compiler doesn't care if you stretch the life time of your local
> variables through your functions, but the brain does.
Ok.
>
>> +
>> + geni_write_reg(common_geni_m_irq_en, base, SE_GENI_M_IRQ_EN);
>> + geni_write_reg(common_geni_s_irq_en, base, SE_GENI_S_IRQ_EN);
>> + geni_write_reg(geni_dma_mode, base, SE_GENI_DMA_MODE_EN);
>> + return 0;
>
> This can't fail, and you ignore the returned value in
> geni_se_select_mode().
Ok.
>
>> +}
>> +
>> +static int geni_se_select_dma_mode(void __iomem *base)
>> +{
>> + unsigned int geni_dma_mode = 0;
>
> This is u32, can be shorten to "mode" and it's first use is an
> assignment - so you don't have to initialize it here.
Ok.
>
>> +
>> + geni_write_reg(0, base, SE_GSI_EVENT_EN);
>> + geni_write_reg(0xFFFFFFFF, base, SE_GENI_M_IRQ_CLEAR);
>> + geni_write_reg(0xFFFFFFFF, base, SE_GENI_S_IRQ_CLEAR);
>> + geni_write_reg(0xFFFFFFFF, base, SE_DMA_TX_IRQ_CLR);
>> + geni_write_reg(0xFFFFFFFF, base, SE_DMA_RX_IRQ_CLR);
>> + geni_write_reg(0xFFFFFFFF, base, SE_IRQ_EN);
>
> Please lower case your hex constants.
Ok.
>
>> +
>> + geni_dma_mode = geni_read_reg(base, SE_GENI_DMA_MODE_EN);
>> + geni_dma_mode |= GENI_DMA_MODE_EN;
>> + geni_write_reg(geni_dma_mode, base, SE_GENI_DMA_MODE_EN);
>> + return 0;
>
> Can't fail.
Ok.
>
>> +}
>> +
>> +/**
>> + * geni_se_select_mode() - Select the serial engine transfer mode
>> + * @base: Base address of the serial engine's register block.
>> + * @mode: Transfer mode to be selected.
>> + *
>> + * Return: 0 on success, standard Linux error codes on failure.
>> + */
>> +int geni_se_select_mode(void __iomem *base, int mode)
>> +{
>> + int ret = 0;
>
> Drop the return value and "ret", if you want to help the developer of
> child devices you can start off by a WARN_ON(mode != FIFO_MODE && mode
> != SE_DMA);
Ok.
>
>> +
>> + switch (mode) {
>> + case FIFO_MODE:
>> + geni_se_select_fifo_mode(base);
>> + break;
>> + case SE_DMA:
>> + geni_se_select_dma_mode(base);
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(geni_se_select_mode);
>> +
>> +/**
>> + * geni_se_setup_m_cmd() - Setup the primary sequencer
>> + * @base: Base address of the serial engine's register block.
>> + * @cmd: Command/Operation to setup in the primary sequencer.
>> + * @params: Parameter for the sequencer command.
>> + *
>> + * This function is used to configure the primary sequencer with the
>> + * command and its assoicated parameters.
>> + */
>> +void geni_se_setup_m_cmd(void __iomem *base, u32 cmd, u32 params)
>> +{
>> + u32 m_cmd = (cmd << M_OPCODE_SHFT);
>> +
>> + m_cmd |= (params & M_PARAMS_MSK);
>
> Rather than initializing the variable during declaration and then
> amending the value it's cleaner if you do:
>
> val = (cmd << M_OPCODE_SHFT) | (params & M_PARAMS_MSK);
Ok.
>
>> + geni_write_reg(m_cmd, base, SE_GENI_M_CMD0);
>> +}
>> +EXPORT_SYMBOL(geni_se_setup_m_cmd);
>> +
> [..]
>> +/**
>> + * geni_se_get_tx_fifo_depth() - Get the TX fifo depth of the serial engine
>> + * @base: Base address of the serial engine's register block.
>> + *
>> + * This function is used to get the depth i.e. number of elements in the
>> + * TX fifo of the serial engine.
>> + *
>> + * Return: TX fifo depth in units of FIFO words.
>> + */
>> +int geni_se_get_tx_fifo_depth(void __iomem *base)
>> +{
>> + int tx_fifo_depth;
>> +
>> + tx_fifo_depth = ((geni_read_reg(base, SE_HW_PARAM_0)
>> + & TX_FIFO_DEPTH_MSK) >> TX_FIFO_DEPTH_SHFT);
>
> It easier to read this way:
>
> u32 val;
>
> val = readl(base, SE_HW_PARAM_0);
>
> return (val & TX_FIFO_DEPTH_MSK) >> TX_FIFO_DEPTH_SHFT;
Ok.
>
>> + return tx_fifo_depth;
>> +}
>> +EXPORT_SYMBOL(geni_se_get_tx_fifo_depth);
>> +
>> +/**
>> + * geni_se_get_tx_fifo_width() - Get the TX fifo width of the serial engine
>> + * @base: Base address of the serial engine's register block.
>> + *
>> + * This function is used to get the width i.e. word size per element in the
>> + * TX fifo of the serial engine.
>> + *
>> + * Return: TX fifo width in bits
>> + */
>> +int geni_se_get_tx_fifo_width(void __iomem *base)
>> +{
>> + int tx_fifo_width;
>> +
>> + tx_fifo_width = ((geni_read_reg(base, SE_HW_PARAM_0)
>> + & TX_FIFO_WIDTH_MSK) >> TX_FIFO_WIDTH_SHFT);
>> + return tx_fifo_width;
>
> Ditto.
Ok.
>
>> +}
>> +EXPORT_SYMBOL(geni_se_get_tx_fifo_width);
>> +
>> +/**
>> + * geni_se_get_rx_fifo_depth() - Get the RX fifo depth of the serial engine
>> + * @base: Base address of the serial engine's register block.
>> + *
>> + * This function is used to get the depth i.e. number of elements in the
>> + * RX fifo of the serial engine.
>> + *
>> + * Return: RX fifo depth in units of FIFO words
>> + */
>> +int geni_se_get_rx_fifo_depth(void __iomem *base)
>> +{
>> + int rx_fifo_depth;
>> +
>> + rx_fifo_depth = ((geni_read_reg(base, SE_HW_PARAM_1)
>> + & RX_FIFO_DEPTH_MSK) >> RX_FIFO_DEPTH_SHFT);
>> + return rx_fifo_depth;
>
> Ditto.
Ok.
>
>> +}
>> +EXPORT_SYMBOL(geni_se_get_rx_fifo_depth);
>> +
>> +/**
>> + * geni_se_get_packing_config() - Get the packing configuration based on input
>> + * @bpw: Bits of data per transfer word.
>> + * @pack_words: Number of words per fifo element.
>> + * @msb_to_lsb: Transfer from MSB to LSB or vice-versa.
>> + * @cfg0: Output buffer to hold the first half of configuration.
>> + * @cfg1: Output buffer to hold the second half of configuration.
>> + *
>> + * This function is used to calculate the packing configuration based on
>> + * the input packing requirement and the configuration logic.
>> + */
>> +void geni_se_get_packing_config(int bpw, int pack_words, bool msb_to_lsb,
>> + unsigned long *cfg0, unsigned long *cfg1)
>
> This function is used only from geni_se_config_packing() which writes
> the new config to the associated registers and from the serial driver
> that does the same - but here pack_words differ for rx and tx.
>
> If you improve geni_se_config_packing() to take rx and tx fifo sizes
> independently you don't have to expose this function to the client
> drivers and you don't need to return cfg0 and cfg1 like you do here.
Ok.
>
>> +{
>> + u32 cfg[4] = {0};
>> + int len;
>> + int temp_bpw = bpw;
>> + int idx_start = (msb_to_lsb ? (bpw - 1) : 0);
>> + int idx = idx_start;
>> + int idx_delta = (msb_to_lsb ? -BITS_PER_BYTE : BITS_PER_BYTE);
>> + int ceil_bpw = ((bpw & (BITS_PER_BYTE - 1)) ?
>> + ((bpw & ~(BITS_PER_BYTE - 1)) + BITS_PER_BYTE) : bpw);
>> + int iter = (ceil_bpw * pack_words) >> 3;
>> + int i;
>> +
>> + if (unlikely(iter <= 0 || iter > 4)) {
>
> Don't use unlikely(), this function is not called one per port, there's
> no need to clutter the code to "optimize" it.
>
>> + *cfg0 = 0;
>> + *cfg1 = 0;
>> + return;
>> + }
>> +
>> + for (i = 0; i < iter; i++) {
>> + len = (temp_bpw < BITS_PER_BYTE) ?
>> + (temp_bpw - 1) : BITS_PER_BYTE - 1;
>> + cfg[i] = ((idx << 5) | (msb_to_lsb << 4) | (len << 1));
>> + idx = ((temp_bpw - BITS_PER_BYTE) <= 0) ?
>> + ((i + 1) * BITS_PER_BYTE) + idx_start :
>> + idx + idx_delta;
>> + temp_bpw = ((temp_bpw - BITS_PER_BYTE) <= 0) ?
>> + bpw : (temp_bpw - BITS_PER_BYTE);
>
> Each operation in this loop depend on temp_bpw being smaller or larger
> than BITS_PER_BYTE, please rewrite it with a single if/else based on
> this.
>
>> + }
>> + cfg[iter - 1] |= 1;
>
> The 1 you're writing here looks like an "EOF". It would be nice with a
> comment describing the format of these 4 registers and the meaning of
> BIT(0).
>
>> + *cfg0 = cfg[0] | (cfg[1] << 10);
>> + *cfg1 = cfg[2] | (cfg[3] << 10);
>> +}
>> +EXPORT_SYMBOL(geni_se_get_packing_config);
>> +
>> +/**
>> + * geni_se_config_packing() - Packing configuration of the serial engine
>> + * @base: Base address of the serial engine's register block.
>> + * @bpw: Bits of data per transfer word.
>> + * @pack_words: Number of words per fifo element.
>> + * @msb_to_lsb: Transfer from MSB to LSB or vice-versa.
>> + *
>> + * This function is used to configure the packing rules for the current
>> + * transfer.
>> + */
>> +void geni_se_config_packing(void __iomem *base, int bpw,
>> + int pack_words, bool msb_to_lsb)
>> +{
>> + unsigned long cfg0, cfg1;
>
> These are u32.
Ok.
>
>> +
>> + geni_se_get_packing_config(bpw, pack_words, msb_to_lsb, &cfg0, &cfg1);
>> + geni_write_reg(cfg0, base, SE_GENI_TX_PACKING_CFG0);
>> + geni_write_reg(cfg1, base, SE_GENI_TX_PACKING_CFG1);
>> + geni_write_reg(cfg0, base, SE_GENI_RX_PACKING_CFG0);
>> + geni_write_reg(cfg1, base, SE_GENI_RX_PACKING_CFG1);
>> + if (pack_words || bpw == 32)
>> + geni_write_reg((bpw >> 4), base, SE_GENI_BYTE_GRAN);
>> +}
>> +EXPORT_SYMBOL(geni_se_config_packing);
>> +
>> +static void geni_se_clks_off(struct geni_se_rsc *rsc)
>> +{
>> + struct geni_se_device *geni_se_dev;
>> +
>> + if (unlikely(!rsc || !rsc->wrapper_dev))
>
> Drop the unlikely(). Why would you be passed a rsc with wrapper_dev
> NULL?
Used it just to handle any development time issues. I will remove the
unlikely().
>
>> + return;
>> +
>> + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev);
>> + if (unlikely(!geni_se_dev))
>> + return;
>> +
>> + clk_disable_unprepare(rsc->se_clk);
>> + clk_disable_unprepare(geni_se_dev->s_ahb_clk);
>> + clk_disable_unprepare(geni_se_dev->m_ahb_clk);
>> +}
>> +
>> +/**
>> + * geni_se_resources_off() - Turn off resources associated with the serial
>> + * engine
>> + * @rsc: Handle to resources associated with the serial engine.
>> + *
>> + * Return: 0 on success, standard Linux error codes on failure/error.
>> + */
>> +int geni_se_resources_off(struct geni_se_rsc *rsc)
>> +{
>> + int ret = 0;
>
> No need to initialize ret.
Ok.
>
>> + struct geni_se_device *geni_se_dev;
>> +
>> + if (unlikely(!rsc || !rsc->wrapper_dev))
>> + return -EINVAL;
>> +
>> + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev);
>> + if (unlikely(!geni_se_dev))
>
>
> Only child devices would call this, so it's unlikely that this is a
> probe defer.
>
> Also the returned value is not used.
>
> And drop the unlikely()
Ok.
>
>> + return -ENODEV;
>> +
>> + ret = pinctrl_select_state(rsc->geni_pinctrl, rsc->geni_gpio_sleep);
>> + if (ret)
>> + return ret;
>> +
>> + geni_se_clks_off(rsc);
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(geni_se_resources_off);
>> +
>> +static int geni_se_clks_on(struct geni_se_rsc *rsc)
>> +{
>> + int ret;
>> + struct geni_se_device *geni_se_dev;
>
> If you name this "geni" instead, or "wrapper" the rest will be cleaner.
Ok.
>
>> +
>> + if (unlikely(!rsc || !rsc->wrapper_dev))
>> + return -EINVAL;
>> +
>> + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev);
>> + if (unlikely(!geni_se_dev))
>> + return -EPROBE_DEFER;
>> +
>> + ret = clk_prepare_enable(geni_se_dev->m_ahb_clk);
>> + if (ret)
>> + return ret;
>> +
>> + ret = clk_prepare_enable(geni_se_dev->s_ahb_clk);
>> + if (ret) {
>> + clk_disable_unprepare(geni_se_dev->m_ahb_clk);
>> + return ret;
>> + }
>
> These two could be dealt with in a single clk_bulk_prepare_enable().
Ok.
>
>> +
>> + ret = clk_prepare_enable(rsc->se_clk);
>> + if (ret) {
>> + clk_disable_unprepare(geni_se_dev->s_ahb_clk);
>> + clk_disable_unprepare(geni_se_dev->m_ahb_clk);
>> + }
>> + return ret;
>> +}
>> +
>> +/**
>> + * geni_se_resources_on() - Turn on resources associated with the serial
>> + * engine
>> + * @rsc: Handle to resources associated with the serial engine.
>> + *
>> + * Return: 0 on success, standard Linux error codes on failure/error.
>> + */
>> +int geni_se_resources_on(struct geni_se_rsc *rsc)
>> +{
>> + int ret = 0;
>> + struct geni_se_device *geni_se_dev;
>> +
>> + if (unlikely(!rsc || !rsc->wrapper_dev))
>> + return -EINVAL;
>> +
>> + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev);
>> + if (unlikely(!geni_se_dev))
>
> As with geni_se_resources_off()
Ok.
>
>> + return -EPROBE_DEFER;
>> +
>> + ret = geni_se_clks_on(rsc);
>> + if (ret)
>> + return ret;
>> +
>> + ret = pinctrl_select_state(rsc->geni_pinctrl, rsc->geni_gpio_active);
>
> pinctrl_pm_select_default_state(rsc->dev);
>
> So you need the dev associated with the rsc.
>
>> + if (ret)
>> + geni_se_clks_off(rsc);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(geni_se_resources_on);
>> +
>> +/**
>> + * geni_se_clk_tbl_get() - Get the clock table to program DFS
>> + * @rsc: Resource for which the clock table is requested.
>> + * @tbl: Table in which the output is returned.
>> + *
>> + * This function is called by the protocol drivers to determine the different
>> + * clock frequencies supported by Serail Engine Core Clock. The protocol
>
> s/Serail/Serial/
Ok.
>
>> + * drivers use the output to determine the clock frequency index to be
>> + * programmed into DFS.
>> + *
>> + * Return: number of valid performance levels in the table on success,
>> + * standard Linux error codes on failure.
>> + */
>> +int geni_se_clk_tbl_get(struct geni_se_rsc *rsc, unsigned long **tbl)
>> +{
>> + struct geni_se_device *geni_se_dev;
>> + int i;
>> + unsigned long prev_freq = 0;
>> + int ret = 0;
>> +
>> + if (unlikely(!rsc || !rsc->wrapper_dev || !rsc->se_clk || !tbl))
>
> Drop the "unlikely", you're only calling this once.
Ok.
>
>> + return -EINVAL;
>> +
>> + *tbl = NULL;
>
> Don't touch tbl on error.
Ok.
>
>> + geni_se_dev = dev_get_drvdata(rsc->wrapper_dev);
>> + if (unlikely(!geni_se_dev))
>> + return -EPROBE_DEFER;
>
> How would this even be possible? This function should only be called by
> child devices, which per definition means that this device been probed.
I agree. Prior to this patchset, the serial engine nodes were not
children nodes of wrapper devices. Hence the check existed. Now that the
device tree nodes are re-structured, this check can be removed.
>
>> +
>> + mutex_lock(&geni_se_dev->geni_dev_lock);
>> + if (geni_se_dev->clk_perf_tbl) {
>> + *tbl = geni_se_dev->clk_perf_tbl;
>> + ret = geni_se_dev->num_clk_levels;
>> + goto exit_se_clk_tbl_get;
>> + }
>
> You're never freeing clk_perf_tbl, and the only reason you have
> geni_dev_lock is to protect the "cached" array in geni_se_dev.
>
> Move the allocation and generation of clk_perf_tbl to geni_se_probe()
> and store the value, then make this function just return that array.
> That way you can drop the lock and the code becomes cleaner.
Currently this driver is initializing in arch_init. It can be moved to
module_init stage as long as the clk_round_rate() returns -EPROBE_DEFER
when it is not ready. I will look into it and do the appropriate change.
>
>> +
>> + geni_se_dev->clk_perf_tbl = kzalloc(sizeof(*geni_se_dev->clk_perf_tbl) *
>> + MAX_CLK_PERF_LEVEL, GFP_KERNEL);
>
> Use kcalloc()
Ok.
>
>> + if (!geni_se_dev->clk_perf_tbl) {
>> + ret = -ENOMEM;
>> + goto exit_se_clk_tbl_get;
>> + }
>> +
>> + for (i = 0; i < MAX_CLK_PERF_LEVEL; i++) {
>> + geni_se_dev->clk_perf_tbl[i] = clk_round_rate(rsc->se_clk,
>> + prev_freq + 1);
>> + if (geni_se_dev->clk_perf_tbl[i] == prev_freq) {
>> + geni_se_dev->clk_perf_tbl[i] = 0;
>
> Use a local variable to keep the return value of clk_round_rate(), that
> way you don't have to revert the value here (not that it matters...) and
> you don't need to line wrap the assignment above.
Ok.
>
>> + break;
>> + }
>> + prev_freq = geni_se_dev->clk_perf_tbl[i];
>
> ...and then you don't need a separate local variable to keep track of
> the last value...
Ok.
>
>> + }
>> + geni_se_dev->num_clk_levels = i;
>> + *tbl = geni_se_dev->clk_perf_tbl;
>> + ret = geni_se_dev->num_clk_levels;
>> +exit_se_clk_tbl_get:
>
> Name your labels based on what action they perform, e.g. "out_unlock"
> (not err_unlock here because it's the successful path as well)
Ok.
>
>> + mutex_unlock(&geni_se_dev->geni_dev_lock);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(geni_se_clk_tbl_get);
>> +
>> +/**
>> + * geni_se_clk_freq_match() - Get the matching or closest SE clock frequency
>> + * @rsc: Resource for which the clock frequency is requested.
>> + * @req_freq: Requested clock frequency.
>> + * @index: Index of the resultant frequency in the table.
>> + * @res_freq: Resultant frequency which matches or is closer to the
>> + * requested frequency.
>> + * @exact: Flag to indicate exact multiple requirement of the requested
>> + * frequency .
>> + *
>> + * This function is called by the protocol drivers to determine the matching
>> + * or closest frequency of the Serial Engine clock to be selected in order
>> + * to meet the performance requirements.
>
> What's the benefit compared to calling clk_round_rate()?
>
> Given that the function can return a rate that is a fraction of the
> requested frequency even though "exact" is set, this description isn't
> explaining the entire picture.
I agree, the description makes it look like clk_round_rate(). But it is
different from clk_round_rate() in the sense that it will try to return
the matching or exact multiple of the requested frequency. If there is
no matching or exact multiple value found, then it returns the closest
floor frequency available. I will update the description accordingly.
>
>> + *
>> + * Return: 0 on success, standard Linux error codes on failure.
>
> Returning the new clockrate would make more sense.
>
>> + */
>> +int geni_se_clk_freq_match(struct geni_se_rsc *rsc, unsigned long req_freq,
>> + unsigned int *index, unsigned long *res_freq,
>> + bool exact)
>> +{
>> + unsigned long *tbl;
>> + int num_clk_levels;
>> + int i;
>> +
>> + num_clk_levels = geni_se_clk_tbl_get(rsc, &tbl);
>> + if (num_clk_levels < 0)
>> + return num_clk_levels;
>> +
>> + if (num_clk_levels == 0)
>> + return -EFAULT;
>> +
>> + *res_freq = 0;
>> + for (i = 0; i < num_clk_levels; i++) {
>> + if (!(tbl[i] % req_freq)) {
>> + *index = i;
>> + *res_freq = tbl[i];
>
> So this will return a frequency that divides the requested frequency
> without a remainder, will index be used to calculate a multiplier from
> this?
The serial interface drivers need to program the hardware register with
the index of the clock rate from the clock controller source. Hence the
index is returned.
>
>> + return 0;
>> + }
>> +
>> + if (!(*res_freq) || ((tbl[i] > *res_freq) &&
>> + (tbl[i] < req_freq))) {
>> + *index = i;
>> + *res_freq = tbl[i];
>
> What if there's a previous value in tbl that given some multiplier has a
> smaller difference to the requested frequency?
At this point the requirement is to return matching or exact multiple of
the requested frequency. If those criteria are not met, return the
closest floor frequency.
>
>> + }
>> + }
>> +
>> + if (exact || !(*res_freq))
>
> *res_freq can't be 0, because in the case that num_clk_levels is 0 you
> already returned -EFAULT above, in all other cases you will assign it at
> least once.
Ok.
>
>> + return -ENOKEY;
>> +
>> + return 0;
>
> Why not use the return value for the calculated frequency?
With frequency being unsigned long, I dont see a clean way to return any
error.
>
>> +}
>> +EXPORT_SYMBOL(geni_se_clk_freq_match);
>> +
>> +/**
>> + * geni_se_tx_dma_prep() - Prepare the Serial Engine for TX DMA transfer
>> + * @wrapper_dev: QUP Wrapper Device to which the TX buffer is mapped.
>> + * @base: Base address of the SE register block.
>> + * @tx_buf: Pointer to the TX buffer.
>> + * @tx_len: Length of the TX buffer.
>> + * @tx_dma: Pointer to store the mapped DMA address.
>> + *
>> + * This function is used to prepare the buffers for DMA TX.
>> + *
>> + * Return: 0 on success, standard Linux error codes on error/failure.
>> + */
>> +int geni_se_tx_dma_prep(struct device *wrapper_dev, void __iomem *base,
>> + void *tx_buf, int tx_len, dma_addr_t *tx_dma)
>
> All child devices will have a "base", so if you move that into what you
> today call a geni_se_rsc and you pass that to all these functions
> operating on behalf of the child device you have the first two
> parameters passed in the same object.
>
> A "tx_len" is typically of type size_t.
>
> Also note that there are no other buf than tx_buf, no other len than
> tx_len and no other dma address than tx_dma in this function, so you can
> drop "tx_" without loosing any information - but improving code
> cleanliness.
Ok.
>
>> +{
>> + int ret;
>> +
>> + if (unlikely(!wrapper_dev || !base || !tx_buf || !tx_len || !tx_dma))
>> + return -EINVAL;
>
> Reduce the error checking here.
Ok.
>
>> +
>> + ret = geni_se_map_buf(wrapper_dev, tx_dma, tx_buf, tx_len,
>> + DMA_TO_DEVICE);
>
> I think you should pass the wrapper itself to geni_se_tx_dma_prep() and
> if you name this "wrapper" (instead of wrapper_dev) you're under 80
> chars and don't need the line break.
Ok.
>
>> + if (ret)
>> + return ret;
>> +
>> + geni_write_reg(7, base, SE_DMA_TX_IRQ_EN_SET);
>
> So that's DMA_RX_IRQ_EN | DMA_TX_IRQ_EN | GENI_M_IRQ_EN?
No, that is DMA_DONE_EN (BIT 0), EOT_EN (BIT 1) and AHB_ERR_EN (BIT 2).
>
>> + geni_write_reg((u32)(*tx_dma), base, SE_DMA_TX_PTR_L);
>> + geni_write_reg((u32)((*tx_dma) >> 32), base, SE_DMA_TX_PTR_H);
>
> If you return the dma_addr_t from this function you will have the happy
> side effect of being able to use tx_dma directly instead of *tx_dma and
> you can remove some craziness from these calls.
>
>
>
>> + geni_write_reg(1, base, SE_DMA_TX_ATTR);
>
> This "1" would be nice to have some clarification on.
This indicates to the hardware that the current buffer is of
End-of-Transfer Type. I will add a comment here or replace the magic 1
with an appropriate preprocessor macro.
>
>> + geni_write_reg(tx_len, base, SE_DMA_TX_LEN);
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(geni_se_tx_dma_prep);
>> +
>> +/**
>> + * geni_se_rx_dma_prep() - Prepare the Serial Engine for RX DMA transfer
>> + * @wrapper_dev: QUP Wrapper Device to which the RX buffer is mapped.
>> + * @base: Base address of the SE register block.
>> + * @rx_buf: Pointer to the RX buffer.
>> + * @rx_len: Length of the RX buffer.
>> + * @rx_dma: Pointer to store the mapped DMA address.
>> + *
>> + * This function is used to prepare the buffers for DMA RX.
>> + *
>> + * Return: 0 on success, standard Linux error codes on error/failure.
>
> The only real error that can come out of this is dma_mapping_error(), so
> just return the dma_addr_t.
Ok.
>
>> + */
>> +int geni_se_rx_dma_prep(struct device *wrapper_dev, void __iomem *base,
>> + void *rx_buf, int rx_len, dma_addr_t *rx_dma)
>
> As with tx, drop all the "rx_". And pass that geni_se_rsc object
> instead.
Ok.
>
>> +{
>> + int ret;
>> +
>> + if (unlikely(!wrapper_dev || !base || !rx_buf || !rx_len || !rx_dma))
>> + return -EINVAL;
>> +
>> + ret = geni_se_map_buf(wrapper_dev, rx_dma, rx_buf, rx_len,
>> + DMA_FROM_DEVICE);
>> + if (ret)
>> + return ret;
>> +
>> + geni_write_reg(7, base, SE_DMA_RX_IRQ_EN_SET);
>
> We enable same interrupts for rx as tx? Why enable TX interrupt here?
No, the interrupts enabled are DMA_DONE_EN, EOT_EN and AHB_ERR_EN.
>
>> + geni_write_reg((u32)(*rx_dma), base, SE_DMA_RX_PTR_L);
>> + geni_write_reg((u32)((*rx_dma) >> 32), base, SE_DMA_RX_PTR_H);
>> + /* RX does not have EOT bit */
>
> Okay? How does this relate to the 0 being written into RX_ATTR?
Instead of EOT bit, this register has RX DMA mode bits. Writing 0 means
the buffer is a single buffer. Writing 1 means the data is present as
part of the command itself and 2 means the buffer is scatter-gather buffer.
>
>> + geni_write_reg(0, base, SE_DMA_RX_ATTR);
>> + geni_write_reg(rx_len, base, SE_DMA_RX_LEN);
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(geni_se_rx_dma_prep);
>> +
>> +/**
>> + * geni_se_tx_dma_unprep() - Unprepare the Serial Engine after TX DMA transfer
>> + * @wrapper_dev: QUP Wrapper Device to which the RX buffer is mapped.
>> + * @tx_dma: DMA address of the TX buffer.
>> + * @tx_len: Length of the TX buffer.
>> + *
>> + * This function is used to unprepare the DMA buffers after DMA TX.
>> + */
>> +void geni_se_tx_dma_unprep(struct device *wrapper_dev,
>> + dma_addr_t tx_dma, int tx_len)
>> +{
>> + if (tx_dma)
>> + geni_se_unmap_buf(wrapper_dev, &tx_dma, tx_len,
>> + DMA_TO_DEVICE);
>> +}
>> +EXPORT_SYMBOL(geni_se_tx_dma_unprep);
>> +
>> +/**
>> + * geni_se_rx_dma_unprep() - Unprepare the Serial Engine after RX DMA transfer
>> + * @wrapper_dev: QUP Wrapper Device to which the RX buffer is mapped.
>> + * @rx_dma: DMA address of the RX buffer.
>> + * @rx_len: Length of the RX buffer.
>> + *
>> + * This function is used to unprepare the DMA buffers after DMA RX.
>> + */
>> +void geni_se_rx_dma_unprep(struct device *wrapper_dev,
>> + dma_addr_t rx_dma, int rx_len)
>> +{
>> + if (rx_dma)
>> + geni_se_unmap_buf(wrapper_dev, &rx_dma, rx_len,
>> + DMA_FROM_DEVICE);
>> +}
>> +EXPORT_SYMBOL(geni_se_rx_dma_unprep);
>> +
>> +/**
>> + * geni_se_map_buf() - Map a single buffer into QUP wrapper device
>
> I find the mixture of prepare and map in the API (where prepare also
> maps) confusing, but no-one calls genbi_se_map_buf() can it be made
> internal?
Sure it can be made internal and later exposed when the serial interface
drivers need it.
>
>> + * @wrapper_dev: Pointer to the corresponding QUP wrapper core.
>> + * @iova: Pointer in which the mapped virtual address is stored.
>> + * @buf: Address of the buffer that needs to be mapped.
>> + * @size: Size of the buffer.
>> + * @dir: Direction of the DMA transfer.
>> + *
>> + * This function is used to map an already allocated buffer into the
>> + * QUP device space.
>> + *
>> + * Return: 0 on success, standard Linux error codes on failure/error.
>> + */
>> +int geni_se_map_buf(struct device *wrapper_dev, dma_addr_t *iova,
>> + void *buf, size_t size, enum dma_data_direction dir)
>> +{
>> + struct device *dev_p;
>> + struct geni_se_device *geni_se_dev;
>> +
>> + if (!wrapper_dev || !iova || !buf || !size)
>> + return -EINVAL;
>
> No need to check that the programmer of the intermediate function
> checked that the programmer of the client filled all these out
> correctly.
Ok.
>
>> +
>> + geni_se_dev = dev_get_drvdata(wrapper_dev);
>> + if (!geni_se_dev || !geni_se_dev->dev)
>> + return -ENODEV;
>
> Pass the geni_se_device from the child driver, to remove the need for
> this.
Ok.
>
>> +
>> + dev_p = geni_se_dev->dev;
>
> Name your variables more succinct and you don't need this local alias.
Ok.
>
>> +
>> + *iova = dma_map_single(dev_p, buf, size, dir);
>
> Just inline dma_map_single() in the functions calling this.
Ok.
>
>> + if (dma_mapping_error(dev_p, *iova))
>> + return -EIO;
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(geni_se_map_buf);
>> +
>> +/**
>> + * geni_se_unmap_buf() - Unmap a single buffer from QUP wrapper device
>> + * @wrapper_dev: Pointer to the corresponding QUP wrapper core.
>> + * @iova: Pointer in which the mapped virtual address is stored.
>> + * @size: Size of the buffer.
>> + * @dir: Direction of the DMA transfer.
>> + *
>> + * This function is used to unmap an already mapped buffer from the
>> + * QUP device space.
>> + *
>> + * Return: 0 on success, standard Linux error codes on failure/error.
>> + */
>> +int geni_se_unmap_buf(struct device *wrapper_dev, dma_addr_t *iova,
>> + size_t size, enum dma_data_direction dir)
>
> There's no reason for iova to be a pointer. Just pass the dma_addr_t as
> is.
Ok.
>
> Should this function really be exposed to the clients?
It can be made internal based on the current use-case.
>
>> +{
>> + struct device *dev_p;
>> + struct geni_se_device *geni_se_dev;
>> +
>> + if (!wrapper_dev || !iova || !size)
>> + return -EINVAL;
>
> Reduce the error checking.
Ok.
>
>> +
>> + geni_se_dev = dev_get_drvdata(wrapper_dev);
>> + if (!geni_se_dev || !geni_se_dev->dev)
>> + return -ENODEV;
>
> And pass the geni_se_rsc to this function for symmetry purposes, which
> would give you the wrapper by just following the pointer and then the
> device from there.
Ok.
>
>> +
>> + dev_p = geni_se_dev->dev;
>> + dma_unmap_single(dev_p, *iova, size, dir);
>
> Just inline dma_unmap_single() in the calling functions.
Ok.
>
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(geni_se_unmap_buf);
>> +
>> +static const struct of_device_id geni_se_dt_match[] = {
>> + { .compatible = "qcom,geni-se-qup", },
>> + {}
>> +};
>
> Move this next to the geni_se_driver below and don't forget the
> MODULE_DEVICE_TABLE()
Ok.
>
>> +
>> +static int geni_se_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct resource *res;
>> + struct geni_se_device *geni_se_dev;
>
> Just name this "geni".
Ok.
>
>> + int ret = 0;
>
> No need to initialize ret, it's only ever used after assignment.
Ok.
>
>> +
>> + geni_se_dev = devm_kzalloc(dev, sizeof(*geni_se_dev), GFP_KERNEL);
>> + if (!geni_se_dev)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res) {
>> + dev_err(dev, "%s: Mandatory resource info not found\n",
>> + __func__);
>> + devm_kfree(dev, geni_se_dev);
>> + return -EINVAL;
>> + }
>
> It's idiomatic to not check for errors of platform_get_resource() as
> devm_ioremap_resource() will fail gracefully if this happens.
Ok, I will remove the check and use the error returned by
devm_ioremap_resource, if any.
>
>> +
>> + geni_se_dev->base = devm_ioremap_resource(dev, res);
>> + if (IS_ERR_OR_NULL(geni_se_dev->base)) {
>
> This should be IS_ERR() only.
Ok.
>
>> + dev_err(dev, "%s: Error mapping the resource\n", __func__);
>> + devm_kfree(dev, geni_se_dev);
>> + return -EFAULT;
>> + }
>> +
>> + geni_se_dev->m_ahb_clk = devm_clk_get(dev, "m-ahb");
>> + if (IS_ERR(geni_se_dev->m_ahb_clk)) {
>> + ret = PTR_ERR(geni_se_dev->m_ahb_clk);
>> + dev_err(dev, "Err getting M AHB clk %d\n", ret);
>> + devm_iounmap(dev, geni_se_dev->base);
>> + devm_kfree(dev, geni_se_dev);
>
> The reason we use the devm_ versions of these is so that we don't have
> to release our resources explicitly.
Ok, I will remove releasing the resources explicitly.
>
>> + return ret;
>> + }
>> +
>> + geni_se_dev->s_ahb_clk = devm_clk_get(dev, "s-ahb");
>> + if (IS_ERR(geni_se_dev->s_ahb_clk)) {
>> + ret = PTR_ERR(geni_se_dev->s_ahb_clk);
>> + dev_err(dev, "Err getting S AHB clk %d\n", ret);
>> + devm_clk_put(dev, geni_se_dev->m_ahb_clk);
>> + devm_iounmap(dev, geni_se_dev->base);
>> + devm_kfree(dev, geni_se_dev);
>> + return ret;
>> + }
>
> Use devm_clk_bulk_get().
Ok.
>
>> +
>> + geni_se_dev->dev = dev;
>> + mutex_init(&geni_se_dev->geni_dev_lock);
>> + dev_set_drvdata(dev, geni_se_dev);
>> + dev_dbg(dev, "GENI SE Driver probed\n");
>> + return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
>
> You're not depopulating these devices when the wrapper goes away. Use
> devm_of_platform_populate() here instead to make that happen.
Ok.
>
>> +}
>> +
>> +static int geni_se_remove(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct geni_se_device *geni_se_dev = dev_get_drvdata(dev);
>> +
>> + devm_clk_put(dev, geni_se_dev->s_ahb_clk);
>> + devm_clk_put(dev, geni_se_dev->m_ahb_clk);
>> + devm_iounmap(dev, geni_se_dev->base);
>> + devm_kfree(dev, geni_se_dev);
>
> Again, the reason to use devm_* is so that you don't have to free
> things...so if this is what you have here you don't even need a remove
> function.
Ok.
>
>> + return 0;
>> +}
>> +
>> +static struct platform_driver geni_se_driver = {
>> + .driver = {
>> + .name = "geni_se_qup",
>> + .of_match_table = geni_se_dt_match,
>> + },
>> + .probe = geni_se_probe,
>> + .remove = geni_se_remove,
>> +};
>> +
>> +static int __init geni_se_driver_init(void)
>> +{
>> + return platform_driver_register(&geni_se_driver);
>> +}
>> +arch_initcall(geni_se_driver_init);
>> +
>> +static void __exit geni_se_driver_exit(void)
>> +{
>> + platform_driver_unregister(&geni_se_driver);
>> +}
>> +module_exit(geni_se_driver_exit);
>
> Should be fine to just use module_platform_driver(), you need separate
> support for earlycon regardless.
Ok.
>
>> +
>> +MODULE_DESCRIPTION("GENI Serial Engine Driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
>> new file mode 100644
>> index 0000000..5695da9
>> --- /dev/null
>> +++ b/include/linux/qcom-geni-se.h
>> @@ -0,0 +1,807 @@
>> +/*
>> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> [..]
>> + */
>> +
>
> Use SPDX header as above.
Ok.
>
>> +#ifndef _LINUX_QCOM_GENI_SE
>> +#define _LINUX_QCOM_GENI_SE
>> +#include <linux/clk.h>
>> +#include <linux/dma-direction.h>
>> +#include <linux/io.h>
>> +#include <linux/list.h>
>
> I don't think there's a need for io.h or list.h here.
Ok.
>
>> +
>> +/* Transfer mode supported by GENI Serial Engines */
>> +enum geni_se_xfer_mode {
>> + INVALID,
>> + FIFO_MODE,
>> + SE_DMA,
>
> These are quite bad names for things in a header file.
I am not sure if you are suggesting me to use a prefix to make these
definitions look GENI specific. I would prefer some clarification in
this regard.
>
>> +};
>> +
>> +/* Protocols supported by GENI Serial Engines */
>> +enum geni_se_protocol_types {
>> + NONE,
>> + SPI,
>> + UART,
>> + I2C,
>> + I3C
>
> Ditto
>
>> +};
>> +
>> +/**
>> + * struct geni_se_rsc - GENI Serial Engine Resource
>> + * @wrapper_dev: Pointer to the parent QUP Wrapper core.
>> + * @se_clk: Handle to the core serial engine clock.
>> + * @geni_pinctrl: Handle to the pinctrl configuration.
>> + * @geni_gpio_active: Handle to the default/active pinctrl state.
>> + * @geni_gpi_sleep: Handle to the sleep pinctrl state.
>> + */
>> +struct geni_se_rsc {
>
> This looks like the common geni_port or geni_device I requested above.
Yes that is right. I will use it to include the serial engine base address.
>
>> + struct device *wrapper_dev;
>> + struct clk *se_clk;
>
> The one and only clock can be named "clk".
Ok.
>
>> + struct pinctrl *geni_pinctrl;
>> + struct pinctrl_state *geni_gpio_active;
>> + struct pinctrl_state *geni_gpio_sleep;
>
> The associated struct device already has init, default, idle and sleep
> pinctrl states associated through the device->pins struct. Typically
> this means that if you provide a "default" and "sleep" state, the
> default will be selected when the device probes.
>
> In order to select between the states call
> pinctrl_pm_select_{default,sleep,idle}_state(dev);
Ok.
>
>> +};
>> +
>> +#define PINCTRL_DEFAULT "default"
>> +#define PINCTRL_SLEEP "sleep"
>> +
>> +/* Common SE registers */
>
> The purpose of the helper functions in the wrapper driver is to hide
> the common details from the individual function drivers, so move these
> defines into the c-file as they shouldn't be needed in the individual
> drivers.
I will move the register definitions that are accessed through helper
functions inside the source file. I may have to keep those register
definitions that are common and are directly accessed by the serial
interface drivers here so that there are no duplicate definitions.
>
>> +#define GENI_INIT_CFG_REVISION (0x0)
>
> Drop the parenthesis.
Ok.
>
> [..]
>> +#ifdef CONFIG_QCOM_GENI_SE
>> +/**
>> + * geni_read_reg_nolog() - Helper function to read from a GENI register
>> + * @base: Base address of the serial engine's register block.
>> + * @offset: Offset within the serial engine's register block.
>> + *
>> + * Return: Return the contents of the register.
>> + */
>
> The kerneldoc goes in the implementation, not the header file. And you
> already have a copy there, so remove it from here.
Ok.
>
>> +unsigned int geni_read_reg_nolog(void __iomem *base, int offset);
> [..]
>> +#else
>
> I see no point in providing dummy functions for these, just make the
> individual drivers either depend or select the core helpers.
Ok.
>
>> +static inline unsigned int geni_read_reg_nolog(void __iomem *base, int offset)
>> +{
>> + return 0;
>> +}
>
> Regards,
> Bjorn
>
Regards,
Karthik.
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [v3,2/2] ACPI / scan: Fix enumeration for special UART devices
From: Graeme Gregory @ 2018-01-31 14:21 UTC (permalink / raw)
To: Frédéric Danis
Cc: robh-DgEjT+Ai2ygdnm+yROfE0A, marcel-kz+m5ild9QBg9hUCZPvPmw,
sre-DgEjT+Ai2ygdnm+yROfE0A, loic.poulain-Re5JQEeQqe8AvxtiuMwx3w,
johan-DgEjT+Ai2ygdnm+yROfE0A, lukas-JFq808J9C/izQB+pC5nmwQ,
hdegoede-H+wXaHxf7aLQT0dZR+AlfA, rafael-DgEjT+Ai2ygdnm+yROfE0A,
greg-U8xfFu+wG4EAvxtiuMwx3w,
linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1507710734-32520-3-git-send-email-frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Wed, Oct 11, 2017 at 10:32:14AM +0200, Frédéric Danis wrote:
> UART devices is expected to be enumerated by SerDev subsystem.
>
> During ACPI scan, serial devices behind SPI, I2C or UART buses are not
> enumerated, allowing them to be enumerated by their respective parents.
>
> Rename *spi_i2c_slave* to *serial_bus_slave* as this will be used for serial
> devices on serial buses (SPI, I2C or UART).
>
> On Macs an empty ResourceTemplate is returned for uart slaves.
> Instead the device properties "baud", "parity", "dataBits", "stopBits" are
> provided. Add a check for "baud" in acpi_is_serial_bus_slave().
>
This patch appears to break UART probing in ACPI on xgene based
plaforms.
The appropriate chunks of DSDT.
Device (_SB.AHBC)
{
OperationRegion (SRST, SystemMemory, 0x1F2AC000, 0x04)
OperationRegion (CLKE, SystemMemory, 0x1F2AC004, 0x04)
OperationRegion (SRRM, SystemMemory, 0x1F2AD070, 0x04)
OperationRegion (RD2F, SystemMemory, 0x1F2AE014, 0x04)
...
Device (UAR0)
{
Name (_HID, "APMC0D08") // _HID: Hardware ID
Name (_DDN, "UAR0") // _DDN: DOS Device Name
Name (_UID, "UAR0") // _UID: Unique ID
Name (_STR, Unicode ("APM88xxxx UART0 Controller")) // _STR: Description String
Name (_ADR, 0x1C021000) // _ADR: Address
Name (_CID, "NS16550A") // _CID: Compatible ID
...
Name (_CRS, ResourceTemplate () // _CRS: Current Resource Settings
{
Memory32Fixed (ReadWrite,
0x1C021000, // Address Base
0x00000100, // Address Length
)
UartSerialBusV2 (0x00002580, DataBitsEight, StopBitsOne,
0x00, LittleEndian, ParityTypeNone, FlowControlHardware,
0x0010, 0x0010, "UAR0",
0x00, ResourceConsumer, , Exclusive,
)
Interrupt (ResourceProducer, Level, ActiveHigh, Exclusive, ,, )
{
0x0000006D,
}
})
Thanks
Graeme
> Signed-off-by: Frédéric Danis <frederic.danis.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Sebastian Reichel <sebastian.reichel-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
> Tested-by: Ronald Tschalär <ronald-Cgq6lnktLNMTaf21n8AfGQ@public.gmane.org>
> Tested-by: Peter Y. Chuang <peteryuchuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> drivers/acpi/scan.c | 37 +++++++++++++++++--------------------
> include/acpi/acpi_bus.h | 2 +-
> 2 files changed, 18 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 602f8ff..860b698 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1505,41 +1505,38 @@ static void acpi_init_coherency(struct acpi_device *adev)
> adev->flags.coherent_dma = cca;
> }
>
> -static int acpi_check_spi_i2c_slave(struct acpi_resource *ares, void *data)
> +static int acpi_check_serial_bus_slave(struct acpi_resource *ares, void *data)
> {
> - bool *is_spi_i2c_slave_p = data;
> + bool *is_serial_bus_slave_p = data;
>
> if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
> return 1;
>
> - /*
> - * devices that are connected to UART still need to be enumerated to
> - * platform bus
> - */
> - if (ares->data.common_serial_bus.type != ACPI_RESOURCE_SERIAL_TYPE_UART)
> - *is_spi_i2c_slave_p = true;
> + *is_serial_bus_slave_p = true;
>
> /* no need to do more checking */
> return -1;
> }
>
> -static bool acpi_is_spi_i2c_slave(struct acpi_device *device)
> +static bool acpi_is_serial_bus_slave(struct acpi_device *device)
> {
> struct list_head resource_list;
> - bool is_spi_i2c_slave = false;
> + bool is_serial_bus_slave = false;
>
> /* Macs use device properties in lieu of _CRS resources */
> if (x86_apple_machine &&
> (fwnode_property_present(&device->fwnode, "spiSclkPeriod") ||
> - fwnode_property_present(&device->fwnode, "i2cAddress")))
> + fwnode_property_present(&device->fwnode, "i2cAddress") ||
> + fwnode_property_present(&device->fwnode, "baud")))
> return true;
>
> INIT_LIST_HEAD(&resource_list);
> - acpi_dev_get_resources(device, &resource_list, acpi_check_spi_i2c_slave,
> - &is_spi_i2c_slave);
> + acpi_dev_get_resources(device, &resource_list,
> + acpi_check_serial_bus_slave,
> + &is_serial_bus_slave);
> acpi_dev_free_resource_list(&resource_list);
>
> - return is_spi_i2c_slave;
> + return is_serial_bus_slave;
> }
>
> void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
> @@ -1557,7 +1554,7 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
> acpi_bus_get_flags(device);
> device->flags.match_driver = false;
> device->flags.initialized = true;
> - device->flags.spi_i2c_slave = acpi_is_spi_i2c_slave(device);
> + device->flags.serial_bus_slave = acpi_is_serial_bus_slave(device);
> acpi_device_clear_enumerated(device);
> device_initialize(&device->dev);
> dev_set_uevent_suppress(&device->dev, true);
> @@ -1841,10 +1838,10 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl_not_used,
> static void acpi_default_enumeration(struct acpi_device *device)
> {
> /*
> - * Do not enumerate SPI/I2C slaves as they will be enumerated by their
> - * respective parents.
> + * Do not enumerate SPI/I2C/UART slaves as they will be enumerated by
> + * their respective parents.
> */
> - if (!device->flags.spi_i2c_slave) {
> + if (!device->flags.serial_bus_slave) {
> acpi_create_platform_device(device, NULL);
> acpi_device_set_enumerated(device);
> } else {
> @@ -1941,7 +1938,7 @@ static void acpi_bus_attach(struct acpi_device *device)
> return;
>
> device->flags.match_driver = true;
> - if (ret > 0 && !device->flags.spi_i2c_slave) {
> + if (ret > 0 && !device->flags.serial_bus_slave) {
> acpi_device_set_enumerated(device);
> goto ok;
> }
> @@ -1950,7 +1947,7 @@ static void acpi_bus_attach(struct acpi_device *device)
> if (ret < 0)
> return;
>
> - if (!device->pnp.type.platform_id && !device->flags.spi_i2c_slave)
> + if (!device->pnp.type.platform_id && !device->flags.serial_bus_slave)
> acpi_device_set_enumerated(device);
> else
> acpi_default_enumeration(device);
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index fa15052..f849be2 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -211,7 +211,7 @@ struct acpi_device_flags {
> u32 of_compatible_ok:1;
> u32 coherent_dma:1;
> u32 cca_seen:1;
> - u32 spi_i2c_slave:1;
> + u32 serial_bus_slave:1;
> u32 reserved:19;
> };
>
^ permalink raw reply
* Re: Handling of chars received while a UART is not open
From: Dave Martin @ 2018-01-31 13:45 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Peter Maydell, Linus Walleij, linux-serial, linux-arm-kernel
In-Reply-To: <20180131121931.GH9418@n2100.armlinux.org.uk>
On Wed, Jan 31, 2018 at 12:19:31PM +0000, Russell King - ARM Linux wrote:
> On Wed, Jan 31, 2018 at 12:06:29PM +0000, Dave Martin wrote:
> > Hi all,
> >
> > Question:
> >
> > 1) Should a UART definitely discard the RX FIFO on ->startup()?
>
> Yes; POSIX makes no guarantees about the configuration of the port
> while it is closed, and when it is re-opened, it is permitted that
> the port is reset back to a set of default parameters rather than
> the previous parameter set that was configured.
>
> You also have the CREAD flag which indicates whether we want the
> receiver enabled or not, but it is not defined by POSIX whether the
> receiver remains enabled when the port is closed.
>
> Linux behaviour has always been to flush the port of received
> characters at close and open time (see 8250 driver), meaning that
> characters received while the TTY is closed are ignored - just like
> what happens on the physical console keyboard, which is also a TTY.
> Doing otherwise is likely to confuse applications and potentially
> introduce buggy behaviour that would only be detected on UARTs with
> differing behaviour to 8250.
>
> Linux follows POSIX General Terminal Interface for its TTY handling.
OK, that all sounds good.
Explicitly reading the RX FIFO until empty on startup seems compatible
with this, and harmless even on hardware that can't show the lockup.
I'll cook up another patch.
Cheers
---Dave
^ permalink raw reply
* Re: Handling of chars received while a UART is not open
From: Dave Martin @ 2018-01-31 13:32 UTC (permalink / raw)
To: Peter Maydell; +Cc: Linus Walleij, Russell King, arm-mail-list, linux-serial
In-Reply-To: <CAFEAcA8D2RLSbHO-OdULHre4oVr=vU+Yb+6sp55XiP6Mrtg1xA@mail.gmail.com>
On Wed, Jan 31, 2018 at 01:12:15PM +0000, Peter Maydell wrote:
> On 31 January 2018 at 12:06, Dave Martin <Dave.Martin@arm.com> wrote:
> > Now, Qemu has a facility for stuffing some input to an emulated UART
> > immediately on boot, and wouldn't work in case (2) above. OTOH, that
> > may be a mistaken approach: it's unlikely to work reliably on hardware
> > due to the problem of knowing when a physical UART is actually ready to
> > receive input. So maybe Qemu should be waiting for a prompt to be
> > transmitted before stuffing input. In case (1) or (3), qemu probably
> > needs to do something like that.
>
> FWIW, there isn't a QEMU "facility" for doing this particularly:
> we just don't take any trouble to stop the user from providing
> input whenever the user likes. (If the user is an automatic test
> script then it's that script's job to wait for suitable prompts
> from the guest before feeding input into the emulated serial port
> if it wants to be reliable, the same way you'd have to if you
> were feeding the data to a real serial line.)
Fair enough. I misunderstood slightly there.
> (I think) wouldn't affect the behaviour here, though, because the
> data will just get held in QEMU's input buffer until the guest
> driver sets UARTEN and then immediately fill the FIFO.
On the Linux side, I guess we nonetheless need to cope with input
stuffed early for the SBSA UART case, but if you're saying that
qemu (or software talking to qemu) shouldn't assume that input
stuffed at boot will actually be received (unless it qemu waits for
UARTEN) then we have some flexibility in the amba-pl011 driver here.
Based on Russell's comments, I guess the fix for the SBSA UART case
would be to read and discard the contents of the RX FIFO on UART open.
It seems harmless to do that for pl011 also, so I would suggest putting
it on the common code path. This likely minimises the chance of being
tripped up by vendor-specific quirks.
> What QEMU doesn't do and probably should fix is that we allow data
> into the FIFO even if UARTCR.UARTEN is 0 ("UART disabled"). This
That probably does makes sense, providing qemu models a pl011.
A real pl011 wouldn't receive anything while that bit is clear.
SBSA UART doesn't have this control (or most of the others): it's
just always on.
Cheers
---Dave
^ permalink raw reply
* Re: Handling of chars received while a UART is not open
From: Peter Maydell @ 2018-01-31 13:12 UTC (permalink / raw)
To: Dave Martin; +Cc: arm-mail-list, Linus Walleij, Russell King, linux-serial
In-Reply-To: <20180131120628.GL5862@e103592.cambridge.arm.com>
On 31 January 2018 at 12:06, Dave Martin <Dave.Martin@arm.com> wrote:
> Now, Qemu has a facility for stuffing some input to an emulated UART
> immediately on boot, and wouldn't work in case (2) above. OTOH, that
> may be a mistaken approach: it's unlikely to work reliably on hardware
> due to the problem of knowing when a physical UART is actually ready to
> receive input. So maybe Qemu should be waiting for a prompt to be
> transmitted before stuffing input. In case (1) or (3), qemu probably
> needs to do something like that.
FWIW, there isn't a QEMU "facility" for doing this particularly:
we just don't take any trouble to stop the user from providing
input whenever the user likes. (If the user is an automatic test
script then it's that script's job to wait for suitable prompts
from the guest before feeding input into the emulated serial port
if it wants to be reliable, the same way you'd have to if you
were feeding the data to a real serial line.)
What QEMU doesn't do and probably should fix is that we allow data
into the FIFO even if UARTCR.UARTEN is 0 ("UART disabled"). This
(I think) wouldn't affect the behaviour here, though, because the
data will just get held in QEMU's input buffer until the guest
driver sets UARTEN and then immediately fill the FIFO.
thanks
-- PMM
^ permalink raw reply
* Re: Handling of chars received while a UART is not open
From: Russell King - ARM Linux @ 2018-01-31 12:19 UTC (permalink / raw)
To: Dave Martin; +Cc: Peter Maydell, Linus Walleij, linux-arm-kernel, linux-serial
In-Reply-To: <20180131120628.GL5862@e103592.cambridge.arm.com>
On Wed, Jan 31, 2018 at 12:06:29PM +0000, Dave Martin wrote:
> Hi all,
>
> Question:
>
> 1) Should a UART definitely discard the RX FIFO on ->startup()?
Yes; POSIX makes no guarantees about the configuration of the port
while it is closed, and when it is re-opened, it is permitted that
the port is reset back to a set of default parameters rather than
the previous parameter set that was configured.
You also have the CREAD flag which indicates whether we want the
receiver enabled or not, but it is not defined by POSIX whether the
receiver remains enabled when the port is closed.
Linux behaviour has always been to flush the port of received
characters at close and open time (see 8250 driver), meaning that
characters received while the TTY is closed are ignored - just like
what happens on the physical console keyboard, which is also a TTY.
Doing otherwise is likely to confuse applications and potentially
introduce buggy behaviour that would only be detected on UARTs with
differing behaviour to 8250.
Linux follows POSIX General Terminal Interface for its TTY handling.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply
* Handling of chars received while a UART is not open
From: Dave Martin @ 2018-01-31 12:06 UTC (permalink / raw)
To: linux-serial; +Cc: Peter Maydell, Linus Walleij, Russell King, linux-arm-kernel
Hi all,
Question:
1) Should a UART definitely discard the RX FIFO on ->startup()?
2) Should a UART definitely *not* discard the RX FIFO on ->startup()?
(as in my fix).
3) Or is there no clear answer here?
Background:
Working on a fix for an RX lockup issue with amba-pl011 [1], I came up
with the idea that the driver's attempt to clear spurious interrupts on
startup is going wrong when the interrupt is not spurious (i.e., the RX
FIFO is initially full to the interrupt trigger threshold).
I have a fix [2] based on this. It removes the attempt to clear
spurious interrupts so that the RX FIFO will definitely get drained if
need be, in the pl011 interrupt handler.
This should mean that amba-pl011 implements (2) with my patch.
But this may have unwanted side-effects, depending on the answer to
the above question.
Now, Qemu has a facility for stuffing some input to an emulated UART
immediately on boot, and wouldn't work in case (2) above. OTOH, that
may be a mistaken approach: it's unlikely to work reliably on hardware
due to the problem of knowing when a physical UART is actually ready to
receive input. So maybe Qemu should be waiting for a prompt to be
transmitted before stuffing input. In case (1) or (3), qemu probably
needs to do something like that.
(1) is also odd though. Garbage on the wire while the UART is closed
will then be seen when a process opens the UART. If there is a
non-trivial amount of data, an overrun error will most likely be
reported too. User programs intended to be robust (getty, pppd etc.)
should perhaps be doing an explicit flush on startup, but this will
race against the UART driver unless the UART driver flushes the FIFO in
its ->startup(). Wire protocols are inherently realtime and need
another way to negotiate startup (DCD/DSR/DTR signalling for example)
or to recover from a bad state on one or both ends.
So I don't know what the right answer is.
Wei's proposed fix in qemu [1] attempts to detect readiness by waiting
for the emulated system to enable interrupts on the UART, but this
makes assumptions about the driver running inside the emultaion: for
a polled-mode driver, it would most likely not work.
The pl011 hardware provides no way to distinguish spurious from
non-spurious RX FIFO interrupts other than reading the FIFO to see if
there's anything in there. Moreover, the interrupt state machine in
the hardware can get stuck if the FIFO is not drained sufficiently.
[1] [Qemu-devel] [Qemu-arm] [PATCH] pl011: do not put into fifo before enabl
https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg06446.html
[2] [RFC PATCH v2] tty: pl011: Avoid spuriously stuck-off interrupts
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-January/556897.html
^ permalink raw reply
* Re: [RFC PATCH v2] tty: pl011: Avoid spuriously stuck-off interrupts
From: Wei Xu @ 2018-01-31 9:11 UTC (permalink / raw)
To: Dave Martin, linux-arm-kernel
Cc: Peter Maydell, Linus Walleij, Russell King, linux-serial
In-Reply-To: <1517334575-4698-1-git-send-email-Dave.Martin@arm.com>
Hi Dave,
On 2018/1/30 17:49, Dave Martin wrote:
> Commit 9b96fbacda34 ("serial: PL011: clear pending interrupts")
> clears the RX and receive timeout interrupts on pl011 startup, to
> avoid a screaming-interrupt scenario that can occur when the
> firmware or bootloader leaves these interrupts asserted.
>
> This has been noted as an issue when running Linux on qemu [1].
>
> Unfortunately, the above fix seems to lead to potential
> misbehaviour if the RX FIFO interrupt is asserted _non_ spuriously
> on driver startup, if the RX FIFO is also already full to the
> trigger level.
>
> Clearing the RX FIFO interrupt does not change the FIFO fill level.
> In this scenario, because the interrupt is now clear and because
> the FIFO is already full to the trigger level, no new assertion of
> the RX FIFO interrupt can occur unless the FIFO is drained back
> below the trigger level. This never occurs because the pl011
> driver is waiting for an RX FIFO interrupt to tell it that there is
> something to read, and does not read the FIFO at all until that
> interrupt occurs.
>
> Thus, simply clearing "spurious" interrupts on startup may be
> misguided, since there is no way to be sure that the interrupts are
> truly spurious, and things can go wrong if they are not.
>
> This patch attempts to handle (suspected) spurious interrupts more
> robustly, by allowing the interrupt(s) to fire but quenching the
> scream.
>
> pl011_int() runs and attempts to drain the FIFO anyway just as if
> the interrupts were real. If the FIFO is already empty, great. To
> avoid a screaming spurious interrupt, the RX FIFO and timeout
> interrupts are now explicitly cleared in between committing to
> drain the RX FIFO and actually draining it. We do not have to
> worry about lost interrupts here, because we are effectively in
> polled mode inside pl011_int() until the RX FIFO becomes empty:
>
> * A new char received before the RX FIFO is fully drained will be
> drained out synchronously by pl011_int() along with the other
> chars already pending. A new char received after the RX FIFO
> is drained will result in correct RX FIFO interrupt assertion,
> because emptying the RX FIFO guarantees that the RX FIFO /
> timeout interrupt state machines are back in a sane state.
>
> * A new RX timeout before the RX FIFO is fully drained is no
> problem, because pl011_int() has already committed to emptying
> the FIFO at this point, guaranteeing that no stray chars will
> be left behind. A new RX timeout after the RX FIFO is fully
> drained will result in correct interrupt assertion.
>
> This patch does not attempt to address the case where the RX FIFO
> fills faster than it can be drained: that is a pathological
> condition that is beyond the scope of the driver to work around.
> Users cannot expect this to work unless they enable hardware flow
> control.
>
> [1] [Qemu-devel] [Qemu-arm] [PATCH] pl011: do not put into fifo
> before enabled the interruption
> https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg06446.html
>
> Reported-by: Wei Xu <xuwei5@hisilicon.com>
> Cc: Wei Xu <xuwei5@hisilicon.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Fixes: 9b96fbacda34 ("serial: PL011: clear pending interrupts")
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
>
> ---
>
> Wei, are you happy for me to add your Tested-by?
Thanks!
Yes, Tested-by: Wei Xu <xuwei5@hisilicon.com>
Best Regards,
Wei
>
> Keeping this as RFC, since I'm still not sure about possible side-
> effects. I'll wait a bit to see if anyone else can test the patch
> or has comments.
>
> Changes since RFC v1:
>
> Requested by Wei Xu:
>
> * Also don't clear those interrupts in pl011_hwinit(), which can
> probably trigger the same issue.
> ---
> drivers/tty/serial/amba-pl011.c | 11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 04af8de..dd6c285 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -1492,9 +1492,7 @@ static irqreturn_t pl011_int(int irq, void *dev_id)
> do {
> check_apply_cts_event_workaround(uap);
>
> - pl011_write(status & ~(UART011_TXIS|UART011_RTIS|
> - UART011_RXIS),
> - uap, REG_ICR);
> + pl011_write(status & ~UART011_TXIS, uap, REG_ICR);
>
> if (status & (UART011_RTIS|UART011_RXIS)) {
> if (pl011_dma_rx_running(uap))
> @@ -1674,9 +1672,8 @@ static int pl011_hwinit(struct uart_port *port)
>
> uap->port.uartclk = clk_get_rate(uap->clk);
>
> - /* Clear pending error and receive interrupts */
> - pl011_write(UART011_OEIS | UART011_BEIS | UART011_PEIS |
> - UART011_FEIS | UART011_RTIS | UART011_RXIS,
> + /* Clear pending error interrupts */
> + pl011_write(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS,
> uap, REG_ICR);
>
> /*
> @@ -1733,8 +1730,6 @@ static void pl011_enable_interrupts(struct uart_amba_port *uap)
> {
> spin_lock_irq(&uap->port.lock);
>
> - /* Clear out any spuriously appearing RX interrupts */
> - pl011_write(UART011_RTIS | UART011_RXIS, uap, REG_ICR);
> uap->im = UART011_RTIM;
> if (!pl011_dma_rx_running(uap))
> uap->im |= UART011_RXIM;
>
^ permalink raw reply
* Re: [patch v18 0/4] JTAG driver introduction
From: Florian Fainelli @ 2018-01-31 3:03 UTC (permalink / raw)
To: Oleksandr Shamray, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r,
arnd-r2nGTMty4D4
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, openbmc-uLR06cmDAlY/bJ5BZ2RsiQ,
joel-U3u1mxZcP9KHXe+LvDLADg, jiri-rHqAuBHg3fBzbRFIqnYvSA,
tklauser-93Khv+1bN0NyDzI6CaY1VQ,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
vadimp-VPRAkNaXOzVWk0Htik3J/w,
system-sw-low-level-VPRAkNaXOzVWk0Htik3J/w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
openocd-devel-owner-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-api-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
mchehab-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <1517236305-4880-1-git-send-email-oleksandrs-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
On 01/29/2018 06:31 AM, Oleksandr Shamray wrote:
> When a need raise up to use JTAG interface for system's devices
> programming or CPU debugging, usually the user layer
> application implements jtag protocol by bit-bang or using a
> proprietary connection to vendor hardware.
> This method can be slow and not generic.
>
> We propose to implement general JTAG interface and infrastructure
> to communicate with user layer application. In such way, we can
> have the standard JTAG interface core part and separation from
> specific HW implementation.
> This allow new capability to debug the CPU or program system's
> device via BMC without additional devices nor cost.
Oleksandr, you have completed dodged my questions here:
https://lkml.org/lkml/2017/12/25/163
can you try to respond to some of these questions please?
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [RFC PATCH v2] tty: pl011: Avoid spuriously stuck-off interrupts
From: Dave Martin @ 2018-01-30 17:49 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Peter Maydell, Linus Walleij, Wei Xu, linux-serial, Russell King
Commit 9b96fbacda34 ("serial: PL011: clear pending interrupts")
clears the RX and receive timeout interrupts on pl011 startup, to
avoid a screaming-interrupt scenario that can occur when the
firmware or bootloader leaves these interrupts asserted.
This has been noted as an issue when running Linux on qemu [1].
Unfortunately, the above fix seems to lead to potential
misbehaviour if the RX FIFO interrupt is asserted _non_ spuriously
on driver startup, if the RX FIFO is also already full to the
trigger level.
Clearing the RX FIFO interrupt does not change the FIFO fill level.
In this scenario, because the interrupt is now clear and because
the FIFO is already full to the trigger level, no new assertion of
the RX FIFO interrupt can occur unless the FIFO is drained back
below the trigger level. This never occurs because the pl011
driver is waiting for an RX FIFO interrupt to tell it that there is
something to read, and does not read the FIFO at all until that
interrupt occurs.
Thus, simply clearing "spurious" interrupts on startup may be
misguided, since there is no way to be sure that the interrupts are
truly spurious, and things can go wrong if they are not.
This patch attempts to handle (suspected) spurious interrupts more
robustly, by allowing the interrupt(s) to fire but quenching the
scream.
pl011_int() runs and attempts to drain the FIFO anyway just as if
the interrupts were real. If the FIFO is already empty, great. To
avoid a screaming spurious interrupt, the RX FIFO and timeout
interrupts are now explicitly cleared in between committing to
drain the RX FIFO and actually draining it. We do not have to
worry about lost interrupts here, because we are effectively in
polled mode inside pl011_int() until the RX FIFO becomes empty:
* A new char received before the RX FIFO is fully drained will be
drained out synchronously by pl011_int() along with the other
chars already pending. A new char received after the RX FIFO
is drained will result in correct RX FIFO interrupt assertion,
because emptying the RX FIFO guarantees that the RX FIFO /
timeout interrupt state machines are back in a sane state.
* A new RX timeout before the RX FIFO is fully drained is no
problem, because pl011_int() has already committed to emptying
the FIFO at this point, guaranteeing that no stray chars will
be left behind. A new RX timeout after the RX FIFO is fully
drained will result in correct interrupt assertion.
This patch does not attempt to address the case where the RX FIFO
fills faster than it can be drained: that is a pathological
condition that is beyond the scope of the driver to work around.
Users cannot expect this to work unless they enable hardware flow
control.
[1] [Qemu-devel] [Qemu-arm] [PATCH] pl011: do not put into fifo
before enabled the interruption
https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg06446.html
Reported-by: Wei Xu <xuwei5@hisilicon.com>
Cc: Wei Xu <xuwei5@hisilicon.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
Fixes: 9b96fbacda34 ("serial: PL011: clear pending interrupts")
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
Wei, are you happy for me to add your Tested-by?
Keeping this as RFC, since I'm still not sure about possible side-
effects. I'll wait a bit to see if anyone else can test the patch
or has comments.
Changes since RFC v1:
Requested by Wei Xu:
* Also don't clear those interrupts in pl011_hwinit(), which can
probably trigger the same issue.
---
drivers/tty/serial/amba-pl011.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 04af8de..dd6c285 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1492,9 +1492,7 @@ static irqreturn_t pl011_int(int irq, void *dev_id)
do {
check_apply_cts_event_workaround(uap);
- pl011_write(status & ~(UART011_TXIS|UART011_RTIS|
- UART011_RXIS),
- uap, REG_ICR);
+ pl011_write(status & ~UART011_TXIS, uap, REG_ICR);
if (status & (UART011_RTIS|UART011_RXIS)) {
if (pl011_dma_rx_running(uap))
@@ -1674,9 +1672,8 @@ static int pl011_hwinit(struct uart_port *port)
uap->port.uartclk = clk_get_rate(uap->clk);
- /* Clear pending error and receive interrupts */
- pl011_write(UART011_OEIS | UART011_BEIS | UART011_PEIS |
- UART011_FEIS | UART011_RTIS | UART011_RXIS,
+ /* Clear pending error interrupts */
+ pl011_write(UART011_OEIS | UART011_BEIS | UART011_PEIS | UART011_FEIS,
uap, REG_ICR);
/*
@@ -1733,8 +1730,6 @@ static void pl011_enable_interrupts(struct uart_amba_port *uap)
{
spin_lock_irq(&uap->port.lock);
- /* Clear out any spuriously appearing RX interrupts */
- pl011_write(UART011_RTIS | UART011_RXIS, uap, REG_ICR);
uap->im = UART011_RTIM;
if (!pl011_dma_rx_running(uap))
uap->im |= UART011_RXIM;
--
2.1.4
^ permalink raw reply related
* Re: [PATCH v6 07/36] nds32: Exception handling
From: Arnd Bergmann @ 2018-01-30 15:27 UTC (permalink / raw)
To: Greentime Hu
Cc: Vincent Chen, Greentime, Linux Kernel Mailing List, linux-arch,
Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring,
Networking, DTML, Al Viro, David Howells, Will Deacon,
Daniel Lezcano, linux-serial, Geert Uytterhoeven, Linus Walleij,
Mark Rutland, Greg KH, Guo Ren
In-Reply-To: <CAEbi=3ekWX9aA5CXf1r4WsqfwvjxED-1XXTb2w0ydWmnA6jfSA@mail.gmail.com>
On Tue, Jan 30, 2018 at 3:49 PM, Greentime Hu <green.hu@gmail.com> wrote:
> Hi, Arnd:
>
> 2018-01-30 21:33 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>> On Tue, Jan 30, 2018 at 11:01 AM, Vincent Chen <deanbo422@gmail.com> wrote:
>>> 2018-01-24 19:10 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>>>> On Wed, Jan 24, 2018 at 12:09 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>>> On Wed, Jan 24, 2018 at 11:53 AM, Vincent Chen <deanbo422@gmail.com> wrote:
>>>>>> 2018-01-18 18:14 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>>>>
>>>>> Ok. I still wonder about the kernel part of this though: is it a good idea
>>>>> for user space to configure whether the kernel does unaligned
>>>>> accesses? I would think that the kernel should just be fixed in such
>>>>> a case.
>>>>
>>>> To clarify: I'm asking only about unaligned accesses from kernel code itself,
>>>> which is generally considered a bug when
>>>> CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is disabled.
>>>>
>>>> Arnd
>>>
>>> Thanks for your comments.
>>>
>>> For performance, we decide always disable
>>> CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS even if hardware supports
>>> unaligned accessing. Therefore, I will remove kernel unaligned accessing from
>>> nds32/mm/alignment.c. In other words, alignment.c only addresses unaligned
>>> accessing for user space.
>>
>> I'm not really following that logic, let's go through that again so I understand
>> the situation better.
>>
>> CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS should be set if and
>> only if you have a CPU that does not need to trap on unaligned accesses.
>>
>> What are the hardware capabilities on nds32? Do you have all three
>> categories:
>>
>> a) some CPUs that always trap on unaligned access
>> b) some CPUs that never trap on unaligned access
>> c) some CPUs that can be configured to either trap or not trap by
>> the kernel?
>>
> We have type a and c.
> We use CONFIG_ALIGNMENT_TRAP for a and
> CONFIG_HW_SUPPORT_UNALIGNMENT_ACCESS for c.
Ok, got it.
> Since unaligned access in kernel code itself should be considered as a
> bug, we will remove the emulation code to handle the kernel code
> unaligned accessed case.
> We think CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS and
> CONFIG_HW_SUPPORT_UNALIGNMENT_ACCESS have different purposes because
> it will still be more efficient to access by byte even if hardware
> support unaligned access.
> CONFIG_HW_SUPPORT_UNALIGNMENT_ACCESS is used to prevent generating
> unaligned access exception.
Hmm, this is a bit tricky. Most architectures actually assume that those
two are the same, and nothing else has a
HW_SUPPORT_UNALIGNMENT_ACCESS option.
We do actually have a related problem on 32-bit ARM, where the current
generation of processors (ARMv6 and higher) support unaligned
accesses for almost all instructions with the exception of those
instructions that operate on multiple memory locations (ldm/stm
and ldrd/strd). We can control the use of those instructions in inline
assembler, and gcc never uses them when it knows that a pointer
is unaligned, but when CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
is set, the kernel sometimes intentionally contains code sequences
that lead the compiler to believe that a variable is aligned when it
is not, so we end up needing a trap handler here.
We might at some point want to clean this up by going through
all uses of CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
and changing them in a way that leads to better results on both
arm32 and nds32.
Arnd
^ permalink raw reply
* Re: [PATCH v6 07/36] nds32: Exception handling
From: Greentime Hu @ 2018-01-30 14:49 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Vincent Chen, Greentime, Linux Kernel Mailing List, linux-arch,
Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring,
Networking, DTML, Al Viro, David Howells, Will Deacon,
Daniel Lezcano, linux-serial, Geert Uytterhoeven, Linus Walleij,
Mark Rutland, Greg KH, Guo Ren
In-Reply-To: <CAK8P3a1iv=10_pfvtfvDwD9YH1HRAXwDy_m8SC=tV59HEDuSsg@mail.gmail.com>
Hi, Arnd:
2018-01-30 21:33 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> On Tue, Jan 30, 2018 at 11:01 AM, Vincent Chen <deanbo422@gmail.com> wrote:
>> 2018-01-24 19:10 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>>> On Wed, Jan 24, 2018 at 12:09 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>>> On Wed, Jan 24, 2018 at 11:53 AM, Vincent Chen <deanbo422@gmail.com> wrote:
>>>>> 2018-01-18 18:14 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>>>
>>>> Ok. I still wonder about the kernel part of this though: is it a good idea
>>>> for user space to configure whether the kernel does unaligned
>>>> accesses? I would think that the kernel should just be fixed in such
>>>> a case.
>>>
>>> To clarify: I'm asking only about unaligned accesses from kernel code itself,
>>> which is generally considered a bug when
>>> CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is disabled.
>>>
>>> Arnd
>>
>> Thanks for your comments.
>>
>> For performance, we decide always disable
>> CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS even if hardware supports
>> unaligned accessing. Therefore, I will remove kernel unaligned accessing from
>> nds32/mm/alignment.c. In other words, alignment.c only addresses unaligned
>> accessing for user space.
>
> I'm not really following that logic, let's go through that again so I understand
> the situation better.
>
> CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS should be set if and
> only if you have a CPU that does not need to trap on unaligned accesses.
>
> What are the hardware capabilities on nds32? Do you have all three
> categories:
>
> a) some CPUs that always trap on unaligned access
> b) some CPUs that never trap on unaligned access
> c) some CPUs that can be configured to either trap or not trap by
> the kernel?
>
We have type a and c.
We use CONFIG_ALIGNMENT_TRAP for a and
CONFIG_HW_SUPPORT_UNALIGNMENT_ACCESS for c.
Since unaligned access in kernel code itself should be considered as a
bug, we will remove the emulation code to handle the kernel code
unaligned accessed case.
We think CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS and
CONFIG_HW_SUPPORT_UNALIGNMENT_ACCESS have different purposes because
it will still be more efficient to access by byte even if hardware
support unaligned access.
CONFIG_HW_SUPPORT_UNALIGNMENT_ACCESS is used to prevent generating
unaligned access exception.
Thus, we will
1. treat unaligned access in kernel code itself as a bug
2. don't select CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
3. disable CONFIG_HW_SUPPORT_UNALIGNMENT_ACCESS as default
^ permalink raw reply
* Re: [PATCH v6 07/36] nds32: Exception handling
From: Arnd Bergmann @ 2018-01-30 13:33 UTC (permalink / raw)
To: Vincent Chen
Cc: Greentime Hu, Greentime, Linux Kernel Mailing List, linux-arch,
Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring,
Networking, DTML, Al Viro, David Howells, Will Deacon,
Daniel Lezcano, linux-serial, Geert Uytterhoeven, Linus Walleij,
Mark Rutland, Greg KH, Guo Ren
In-Reply-To: <CAJsyPhx090fPsdhkydoKDHDw+LwByT66JHJNT4Tb5N0s_O7MPw@mail.gmail.com>
On Tue, Jan 30, 2018 at 11:01 AM, Vincent Chen <deanbo422@gmail.com> wrote:
> 2018-01-24 19:10 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>> On Wed, Jan 24, 2018 at 12:09 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Wed, Jan 24, 2018 at 11:53 AM, Vincent Chen <deanbo422@gmail.com> wrote:
>>>> 2018-01-18 18:14 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>>
>>> Ok. I still wonder about the kernel part of this though: is it a good idea
>>> for user space to configure whether the kernel does unaligned
>>> accesses? I would think that the kernel should just be fixed in such
>>> a case.
>>
>> To clarify: I'm asking only about unaligned accesses from kernel code itself,
>> which is generally considered a bug when
>> CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is disabled.
>>
>> Arnd
>
> Thanks for your comments.
>
> For performance, we decide always disable
> CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS even if hardware supports
> unaligned accessing. Therefore, I will remove kernel unaligned accessing from
> nds32/mm/alignment.c. In other words, alignment.c only addresses unaligned
> accessing for user space.
I'm not really following that logic, let's go through that again so I understand
the situation better.
CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS should be set if and
only if you have a CPU that does not need to trap on unaligned accesses.
What are the hardware capabilities on nds32? Do you have all three
categories:
a) some CPUs that always trap on unaligned access
b) some CPUs that never trap on unaligned access
c) some CPUs that can be configured to either trap or not trap by
the kernel?
Arnd
^ permalink raw reply
* Re: [PATCH v6 07/36] nds32: Exception handling
From: Vincent Chen @ 2018-01-30 10:01 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Greentime Hu, Greentime, Linux Kernel Mailing List, linux-arch,
Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring,
Networking, DTML, Al Viro, David Howells, Will Deacon,
Daniel Lezcano, linux-serial, Geert Uytterhoeven, Linus Walleij,
Mark Rutland, Greg KH, Guo Ren
In-Reply-To: <CAK8P3a3PxsmNXDqYbivfCTL8NmnR==M91yjv_VV0zXwjzLxHuA@mail.gmail.com>
2018-01-24 19:10 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> On Wed, Jan 24, 2018 at 12:09 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Wed, Jan 24, 2018 at 11:53 AM, Vincent Chen <deanbo422@gmail.com> wrote:
>>> 2018-01-18 18:14 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
>
>> Ok. I still wonder about the kernel part of this though: is it a good idea
>> for user space to configure whether the kernel does unaligned
>> accesses? I would think that the kernel should just be fixed in such
>> a case.
>
> To clarify: I'm asking only about unaligned accesses from kernel code itself,
> which is generally considered a bug when
> CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is disabled.
>
> Arnd
Thanks for your comments.
For performance, we decide always disable
CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS even if hardware supports
unaligned accessing. Therefore, I will remove kernel unaligned accessing from
nds32/mm/alignment.c. In other words, alignment.c only addresses unaligned
accessing for user space.
Vincent
^ permalink raw reply
* Re: [PATCH v2 1/7] qcom-geni-se: Add QCOM GENI SE Driver summary
From: Karthik Ramasubramanian @ 2018-01-29 21:52 UTC (permalink / raw)
To: Bjorn Andersson
Cc: corbet, andy.gross, david.brown, robh+dt, mark.rutland, wsa,
gregkh, linux-doc, linux-arm-msm, devicetree, linux-i2c,
linux-serial, jslaby
In-Reply-To: <20180116165515.GF478@tuxbook>
On 1/16/2018 9:55 AM, Bjorn Andersson wrote:
> On Fri 12 Jan 17:05 PST 2018, Karthikeyan Ramasubramanian wrote:
>
>> Generic Interface (GENI) firmware based Qualcomm Universal Peripheral (QUP)
>> Wrapper is a programmable module that is composed of multiple Serial
>> Engines (SE) and can support various Serial Interfaces like UART, SPI,
>> I2C, I3C, etc. This document provides a high level overview of the GENI
>> based QUP Wrapper.
>>
>> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
>
> Rather than adding a disconnected chunk of documentation move this into
> the driver(s), using the format described in
> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html?highlight=kernel%20doc#overview-documentation-comments
I will move the documentation into the driver source file.
>
> Regards,
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Regards,
Karthik.
--
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [patch v18 1/4] drivers: jtag: Add JTAG core driver
From: Julia Cartwright @ 2018-01-29 15:56 UTC (permalink / raw)
To: Oleksandr Shamray
Cc: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r, arnd-r2nGTMty4D4,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, openbmc-uLR06cmDAlY/bJ5BZ2RsiQ,
joel-U3u1mxZcP9KHXe+LvDLADg, jiri-rHqAuBHg3fBzbRFIqnYvSA,
tklauser-93Khv+1bN0NyDzI6CaY1VQ,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
vadimp-VPRAkNaXOzVWk0Htik3J/w,
system-sw-low-level-VPRAkNaXOzVWk0Htik3J/w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
openocd-devel-owner-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-api-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
mchehab-DgEjT+Ai2ygdnm+yROfE0A, Jiri Pirko
In-Reply-To: <1517236305-4880-2-git-send-email-oleksandrs-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
On Mon, Jan 29, 2018 at 04:31:42PM +0200, Oleksandr Shamray wrote:
> Initial patch for JTAG driver
> JTAG class driver provide infrastructure to support hardware/software
> JTAG platform drivers. It provide user layer API interface for flashing
> and debugging external devices which equipped with JTAG interface
> using standard transactions.
>
> Driver exposes set of IOCTL to user space for:
> - XFER:
> - SIR (Scan Instruction Register, IEEE 1149.1 Data Register scan);
> - SDR (Scan Data Register, IEEE 1149.1 Instruction Register scan);
> - RUNTEST (Forces the IEEE 1149.1 bus to a run state for a specified
> number of clocks).
> - SIOCFREQ/GIOCFREQ for setting and reading JTAG frequency.
>
> Driver core provides set of internal APIs for allocation and
> registration:
> - jtag_register;
> - jtag_unregister;
> - jtag_alloc;
> - jtag_free;
>
> Platform driver on registration with jtag-core creates the next
> entry in dev folder:
> /dev/jtagX
>
> Signed-off-by: Oleksandr Shamray <oleksandrs-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Jiri Pirko <jiri-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Acked-by: Philippe Ombredanne <pombredanne-od1rfyK75/E@public.gmane.org>
[..]
> diff --git a/drivers/jtag/jtag.c b/drivers/jtag/jtag.c
[..]
> +struct jtag *jtag_alloc(size_t priv_size, const struct jtag_ops *ops)
> +{
> + struct jtag *jtag;
> +
> + jtag = kzalloc(sizeof(*jtag) + priv_size, GFP_KERNEL);
> + if (!jtag)
> + return NULL;
> +
> + if (!ops)
> + return NULL;
> +
> + if (!ops->idle || !ops->mode_set || !ops->status_get || !ops->xfer)
> + return NULL;
Did you think through this?
You leak 'jtag' here and above.
Perform all the ops checks prior to the allocation.
Julia
^ permalink raw reply
* [patch v18 4/4] Documentation: jtag: Add ABI documentation
From: Oleksandr Shamray @ 2018-01-29 14:31 UTC (permalink / raw)
To: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r, arnd-r2nGTMty4D4
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, openbmc-uLR06cmDAlY/bJ5BZ2RsiQ,
joel-U3u1mxZcP9KHXe+LvDLADg, jiri-rHqAuBHg3fBzbRFIqnYvSA,
tklauser-93Khv+1bN0NyDzI6CaY1VQ,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
vadimp-VPRAkNaXOzVWk0Htik3J/w,
system-sw-low-level-VPRAkNaXOzVWk0Htik3J/w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
openocd-devel-owner-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-api-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
mchehab-DgEjT+Ai2ygdnm+yROfE0A, Oleksandr Shamray
In-Reply-To: <1517236305-4880-1-git-send-email-oleksandrs-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Added document that describe the ABI for JTAG class drivrer
Signed-off-by: Oleksandr Shamray <oleksandrs-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
---
v17->v18
v16->v17
v15->v16
v14->v15
v13->v14
v12->v13
v11->v12
Tobias Klauser <tklauser-93Khv+1bN0NyDzI6CaY1VQ@public.gmane.org>
- rename /Documentation/ABI/testing/jatg-dev -> jtag-dev
- Typo: s/interfase/interface
v10->v11
v9->v10
Fixes added by Oleksandr:
- change jtag-cdev to jtag-dev in documentation
- update Kernel Version and Date in jtag-dev documentation;
v8->v9
v7->v8
v6->v7
Comments pointed by Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
- Added jtag-cdev documentation to Documentation/ABI/testing folder
---
Documentation/ABI/testing/jtag-dev | 27 +++++++++++++++++++++++++++
1 files changed, 27 insertions(+), 0 deletions(-)
create mode 100644 Documentation/ABI/testing/jtag-dev
diff --git a/Documentation/ABI/testing/jtag-dev b/Documentation/ABI/testing/jtag-dev
new file mode 100644
index 0000000..9db83ea
--- /dev/null
+++ b/Documentation/ABI/testing/jtag-dev
@@ -0,0 +1,27 @@
+What: /dev/jtag[0-9]+
+Date: October 2017
+KernelVersion: 4.17
+Contact: oleksandrs-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org
+Description:
+ The misc device files /dev/jtag* are the interface
+ between JTAG master interface and userspace.
+
+ The ioctl(2)-based ABI is defined and documented in
+ [include/uapi]<linux/jtag.h>.
+
+ The following file operations are supported:
+
+ open(2)
+ The argument flag currently support only one access
+ mode O_RDWR.
+
+ ioctl(2)
+ Initiate various actions.
+ See the inline documentation in [include/uapi]<linux/jtag.h>
+ for descriptions of all ioctls.
+
+ close(2)
+ Stops and free up the I/O contexts that was associated
+ with the file descriptor.
+
+Users: TBD
--
1.7.1
^ permalink raw reply related
* [patch v18 3/4] Documentation: jtag: Add bindings for Aspeed SoC 24xx and 25xx families JTAG master driver
From: Oleksandr Shamray @ 2018-01-29 14:31 UTC (permalink / raw)
To: gregkh, arnd
Cc: system-sw-low-level, devicetree, jiri, vadimp, linux-api, openbmc,
linux-kernel, openocd-devel-owner, Jiri Pirko, robh+dt, joel,
linux-serial, Oleksandr Shamray, tklauser, mchehab, davem,
linux-arm-kernel
In-Reply-To: <1517236305-4880-1-git-send-email-oleksandrs@mellanox.com>
It has been tested on Mellanox system with BMC equipped with
Aspeed 2520 SoC for programming CPLD devices.
Signed-off-by: Oleksandr Shamray <oleksandrs@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Acked-by: Rob Herring <robh@kernel.org>
---
v17->v18
v16->v17
v15->v16
Comments pointed by Joel Stanley <joel.stan@gmail.com>
- change clocks = <&clk_apb> to proper clocks = <&syscon ASPEED_CLK_APB>
- add reset descriptions in bndings file
v14->v15
v13->v14
v12->v13
v11->v12
v10->v11
v9->v10
v8->v9
v7->v8
Comments pointed by pointed by Joel Stanley <joel.stan@gmail.com>
- Change compatible string to ast2400 and ast2000
V6->v7
Comments pointed by Tobias Klauser <tklauser@distanz.ch>
- Fix spell "Doccumentation" -> "Documentation"
v5->v6
Comments pointed by Tobias Klauser <tklauser@distanz.ch>
- Small nit: s/documentation/Documentation/
v4->v5
V3->v4
Comments pointed by Rob Herring <robh@kernel.org>
- delete unnecessary "status" and "reg-shift" descriptions in
bndings file
v2->v3
Comments pointed by Rob Herring <robh@kernel.org>
- split Aspeed jtag driver and binding to sepatrate patches
- delete unnecessary "status" and "reg-shift" descriptions in
bndings file
---
.../devicetree/bindings/jtag/aspeed-jtag.txt | 22 ++++++++++++++++++++
1 files changed, 22 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/jtag/aspeed-jtag.txt
diff --git a/Documentation/devicetree/bindings/jtag/aspeed-jtag.txt b/Documentation/devicetree/bindings/jtag/aspeed-jtag.txt
new file mode 100644
index 0000000..7c36eb6
--- /dev/null
+++ b/Documentation/devicetree/bindings/jtag/aspeed-jtag.txt
@@ -0,0 +1,22 @@
+Aspeed JTAG driver for ast2400 and ast2500 SoC
+
+Required properties:
+- compatible: Should be one of
+ - "aspeed,ast2400-jtag"
+ - "aspeed,ast2500-jtag"
+- reg contains the offset and length of the JTAG memory
+ region
+- clocks root clock of bus, should reference the APB
+ clock in the second cell
+- resets phandle to reset controller with the reset number in
+ the second cell
+- interrupts should contain JTAG controller interrupt
+
+Example:
+jtag: jtag@1e6e4000 {
+ compatible = "aspeed,ast2500-jtag";
+ reg = <0x1e6e4000 0x1c>;
+ clocks = <&syscon ASPEED_CLK_APB>;
+ resets = <&syscon ASPEED_RESET_JTAG_MASTER>;
+ interrupts = <43>;
+};
--
1.7.1
^ permalink raw reply related
* [patch v18 2/4] drivers: jtag: Add Aspeed SoC 24xx and 25xx families JTAG master driver
From: Oleksandr Shamray @ 2018-01-29 14:31 UTC (permalink / raw)
To: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r, arnd-r2nGTMty4D4
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, openbmc-uLR06cmDAlY/bJ5BZ2RsiQ,
joel-U3u1mxZcP9KHXe+LvDLADg, jiri-rHqAuBHg3fBzbRFIqnYvSA,
tklauser-93Khv+1bN0NyDzI6CaY1VQ,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
vadimp-VPRAkNaXOzVWk0Htik3J/w,
system-sw-low-level-VPRAkNaXOzVWk0Htik3J/w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
openocd-devel-owner-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-api-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
mchehab-DgEjT+Ai2ygdnm+yROfE0A, Oleksandr Shamray, Jiri Pirko
In-Reply-To: <1517236305-4880-1-git-send-email-oleksandrs-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
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.
Signed-off-by: Oleksandr Shamray <oleksandrs-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Jiri Pirko <jiri-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Acked-by: Philippe Ombredanne <pombredanne-od1rfyK75/E@public.gmane.org>
Acked-by: Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org>
---
v17->v18
v16->v17
v15->v16
Comments pointed by Joel Stanley <joel.stan-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
- Add reset_control on Jtag init/deinit
v14->v15
Comments pointed by Joel Stanley <joel.stan-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
- Add ARCH_ASPEED || COMPILE_TEST to Kconfig
- remove unused offset variable
- remove "aspeed_jtag" from dev_err and dev_dbg messages
- change clk_prepare_enable initialisation order
v13->v14
Comments pointed by Philippe Ombredanne <pombredanne-od1rfyK75/E@public.gmane.org>
- Change style of head block comment from /**/ to //
v12->v13
Comments pointed by Philippe Ombredanne <pombredanne-od1rfyK75/E@public.gmane.org>
- Change jtag-aspeed.c licence type to
SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note
and reorder line with license in description
Comments pointed by Kun Yi <kunyi-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
- Changed capability check for aspeed,ast2400-jtag/ast200-jtag
v11->v12
Comments pointed by Chip Bilbrey <chip-Gz1Ta9Qd61ZAfugRpC6u6w@public.gmane.org>
- Remove access mode from xfer and idle transactions
- Add new ioctl JTAG_SIOCMODE for set hw mode
v10->v11
v9->v10
V8->v9
Comments pointed by Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
- add *data parameter to xfer function prototype
v7->v8
Comments pointed by Joel Stanley <joel.stan-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
- aspeed_jtag_init replace goto to return;
- change input variables type from __u32 to u32
in functios freq_get, freq_set, status_get
- change sm_ variables type from char to u8
- in jatg_init add disable clocks on error case
- remove release_mem_region on error case
- remove devm_free_irq on jtag_deinit
- Fix misspelling Disabe/Disable
- Change compatible string to ast2400 and ast2000
v6->v7
Notifications from kbuild test robot <lkp-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
- Add include <linux/types.h> to jtag-asapeed.c
v5->v6
v4->v5
Comments pointed by Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
- Added HAS_IOMEM dependence in Kconfig to avoid
"undefined reference to `devm_ioremap_resource'" error,
because in some arch this not supported
v3->v4
Comments pointed by Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
- change transaction pointer tdio type to __u64
- change internal status type from enum to __u32
v2->v3
v1->v2
Comments pointed by Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
- change license type from GPLv2/BSD to GPLv2
Comments pointed by Neil Armstrong <narmstrong-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
- Add clk_prepare_enable/clk_disable_unprepare in clock init/deinit
- Change .compatible to soc-specific compatible names
aspeed,aspeed4000-jtag/aspeed5000-jtag
- Added dt-bindings
Comments pointed by Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
- Reorder functions and removed the forward declarations
- Add static const qualifier to state machine states transitions
- Change .compatible to soc-specific compatible names
aspeed,aspeed4000-jtag/aspeed5000-jtag
- Add dt-bindings
Comments pointed by Randy Dunlap <rdunlap-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
- Change module name jtag-aspeed in description in Kconfig
Comments pointed by kbuild test robot <lkp-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
- Remove invalid include <asm/mach-types.h>
- add resource_size instead of calculation
---
drivers/jtag/Kconfig | 14 +
drivers/jtag/Makefile | 1 +
drivers/jtag/jtag-aspeed.c | 786 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 801 insertions(+), 0 deletions(-)
create mode 100644 drivers/jtag/jtag-aspeed.c
diff --git a/drivers/jtag/Kconfig b/drivers/jtag/Kconfig
index 0fad1a3..63ddf1f 100644
--- a/drivers/jtag/Kconfig
+++ b/drivers/jtag/Kconfig
@@ -14,3 +14,17 @@ menuconfig JTAG
To compile this driver as a module, choose M here: the module will
be called jtag.
+
+menuconfig JTAG_ASPEED
+ tristate "Aspeed SoC JTAG controller support"
+ depends on JTAG && HAS_IOMEM
+ depends on ARCH_ASPEED || COMPILE_TEST
+ ---help---
+ This provides a support for Aspeed JTAG device, equipped on
+ Aspeed SoC 24xx and 25xx families. Drivers allows programming
+ of hardware devices, connected to SoC through the JTAG interface.
+
+ If you want this support, you should say Y here.
+
+ To compile this driver as a module, choose M here: the module will
+ be called jtag-aspeed.
diff --git a/drivers/jtag/Makefile b/drivers/jtag/Makefile
index af37493..04a855e 100644
--- a/drivers/jtag/Makefile
+++ b/drivers/jtag/Makefile
@@ -1 +1,2 @@
obj-$(CONFIG_JTAG) += jtag.o
+obj-$(CONFIG_JTAG_ASPEED) += jtag-aspeed.o
diff --git a/drivers/jtag/jtag-aspeed.c b/drivers/jtag/jtag-aspeed.c
new file mode 100644
index 0000000..f679041
--- /dev/null
+++ b/drivers/jtag/jtag-aspeed.c
@@ -0,0 +1,786 @@
+// SPDX-License-Identifier: GPL-2.0
+// drivers/jtag/aspeed-jtag.c
+//
+// Copyright (c) 2017 Mellanox Technologies. All rights reserved.
+// Copyright (c) 2017 Oleksandr Shamray <oleksandrs-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/jtag.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <uapi/linux/jtag.h>
+
+#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
+
+/* ASPEED_JTAG_CTRL: Engine Control */
+#define ASPEED_JTAG_CTL_ENG_EN BIT(31)
+#define ASPEED_JTAG_CTL_ENG_OUT_EN BIT(30)
+#define ASPEED_JTAG_CTL_FORCE_TMS BIT(29)
+#define ASPEED_JTAG_CTL_INST_LEN(x) ((x) << 20)
+#define ASPEED_JTAG_CTL_LASPEED_INST BIT(17)
+#define ASPEED_JTAG_CTL_INST_EN BIT(16)
+#define ASPEED_JTAG_CTL_DR_UPDATE BIT(10)
+#define ASPEED_JTAG_CTL_DATA_LEN(x) ((x) << 4)
+#define ASPEED_JTAG_CTL_LASPEED_DATA BIT(1)
+#define ASPEED_JTAG_CTL_DATA_EN BIT(0)
+
+/* ASPEED_JTAG_ISR : Interrupt status and enable */
+#define ASPEED_JTAG_ISR_INST_PAUSE BIT(19)
+#define ASPEED_JTAG_ISR_INST_COMPLETE BIT(18)
+#define ASPEED_JTAG_ISR_DATA_PAUSE BIT(17)
+#define ASPEED_JTAG_ISR_DATA_COMPLETE BIT(16)
+#define ASPEED_JTAG_ISR_INST_PAUSE_EN BIT(3)
+#define ASPEED_JTAG_ISR_INST_COMPLETE_EN BIT(2)
+#define ASPEED_JTAG_ISR_DATA_PAUSE_EN BIT(1)
+#define ASPEED_JTAG_ISR_DATA_COMPLETE_EN BIT(0)
+#define ASPEED_JTAG_ISR_INT_EN_MASK GENMASK(3, 0)
+#define ASPEED_JTAG_ISR_INT_MASK GENMASK(19, 16)
+
+/* ASPEED_JTAG_SW : Software Mode and Status */
+#define ASPEED_JTAG_SW_MODE_EN BIT(19)
+#define ASPEED_JTAG_SW_MODE_TCK BIT(18)
+#define ASPEED_JTAG_SW_MODE_TMS BIT(17)
+#define ASPEED_JTAG_SW_MODE_TDIO BIT(16)
+
+/* ASPEED_JTAG_TCK : TCK Control */
+#define ASPEED_JTAG_TCK_DIVISOR_MASK GENMASK(10, 0)
+#define ASPEED_JTAG_TCK_GET_DIV(x) ((x) & ASPEED_JTAG_TCK_DIVISOR_MASK)
+
+/* ASPEED_JTAG_EC : Controller set for go to IDLE */
+#define ASPEED_JTAG_EC_GO_IDLE BIT(0)
+
+#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))
+
+#define ASPEED_JTAG_TCK_WAIT 10
+#define ASPEED_JTAG_RESET_CNTR 10
+
+#define ASPEED_JTAG_NAME "jtag-aspeed"
+
+struct aspeed_jtag {
+ void __iomem *reg_base;
+ struct device *dev;
+ struct clk *pclk;
+ enum jtag_endstate status;
+ int irq;
+ struct reset_control *rst;
+ u32 flag;
+ wait_queue_head_t jtag_wq;
+ u32 mode;
+};
+
+static char *end_status_str[] = {"idle", "ir pause", "drpause"};
+
+static u32 aspeed_jtag_read(struct aspeed_jtag *aspeed_jtag, u32 reg)
+{
+ return readl(aspeed_jtag->reg_base + reg);
+}
+
+static void
+aspeed_jtag_write(struct aspeed_jtag *aspeed_jtag, u32 val, u32 reg)
+{
+ writel(val, aspeed_jtag->reg_base + reg);
+}
+
+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);
+ 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 int aspeed_jtag_freq_get(struct jtag *jtag, u32 *frq)
+{
+ struct aspeed_jtag *aspeed_jtag = jtag_priv(jtag);
+ u32 pclk;
+ u32 tck;
+
+ pclk = clk_get_rate(aspeed_jtag->pclk);
+ tck = aspeed_jtag_read(aspeed_jtag, ASPEED_JTAG_TCK);
+ *frq = pclk / (ASPEED_JTAG_TCK_GET_DIV(tck) + 1);
+
+ return 0;
+}
+
+static int aspeed_jtag_mode_set(struct jtag *jtag, u32 mode)
+{
+ struct aspeed_jtag *aspeed_jtag = jtag_priv(jtag);
+
+ aspeed_jtag->mode = mode;
+ 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);
+}
+
+static char aspeed_jtag_tck_cycle(struct aspeed_jtag *aspeed_jtag,
+ u8 tms, u8 tdi)
+{
+ char tdo = 0;
+
+ /* TCK = 0 */
+ aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_SW_MODE_EN |
+ (tms * ASPEED_JTAG_SW_MODE_TMS) |
+ (tdi * ASPEED_JTAG_SW_MODE_TDIO), ASPEED_JTAG_SW);
+
+ aspeed_jtag_sw_delay(aspeed_jtag, ASPEED_JTAG_TCK_WAIT);
+
+ /* TCK = 1 */
+ aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_SW_MODE_EN |
+ ASPEED_JTAG_SW_MODE_TCK |
+ (tms * ASPEED_JTAG_SW_MODE_TMS) |
+ (tdi * ASPEED_JTAG_SW_MODE_TDIO), ASPEED_JTAG_SW);
+
+ if (aspeed_jtag_read(aspeed_jtag, ASPEED_JTAG_SW) &
+ ASPEED_JTAG_SW_MODE_TDIO)
+ tdo = 1;
+
+ aspeed_jtag_sw_delay(aspeed_jtag, ASPEED_JTAG_TCK_WAIT);
+
+ /* TCK = 0 */
+ aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_SW_MODE_EN |
+ (tms * ASPEED_JTAG_SW_MODE_TMS) |
+ (tdi * ASPEED_JTAG_SW_MODE_TDIO), ASPEED_JTAG_SW);
+ return tdo;
+}
+
+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);
+ aspeed_jtag->flag &= ~ASPEED_JTAG_ISR_INST_PAUSE;
+}
+
+static void
+aspeed_jtag_wait_instruction_complete(struct aspeed_jtag *aspeed_jtag)
+{
+ wait_event_interruptible(aspeed_jtag->jtag_wq, aspeed_jtag->flag &
+ ASPEED_JTAG_ISR_INST_COMPLETE);
+ aspeed_jtag->flag &= ~ASPEED_JTAG_ISR_INST_COMPLETE;
+}
+
+static void
+aspeed_jtag_wait_data_pause_complete(struct aspeed_jtag *aspeed_jtag)
+{
+ wait_event_interruptible(aspeed_jtag->jtag_wq, aspeed_jtag->flag &
+ ASPEED_JTAG_ISR_DATA_PAUSE);
+ aspeed_jtag->flag &= ~ASPEED_JTAG_ISR_DATA_PAUSE;
+}
+
+static void aspeed_jtag_wait_data_complete(struct aspeed_jtag *aspeed_jtag)
+{
+ wait_event_interruptible(aspeed_jtag->jtag_wq, aspeed_jtag->flag &
+ ASPEED_JTAG_ISR_DATA_COMPLETE);
+ aspeed_jtag->flag &= ~ASPEED_JTAG_ISR_DATA_COMPLETE;
+}
+
+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);
+ }
+
+ switch (aspeed_jtag->status) {
+ case JTAG_STATE_IDLE:
+ switch (runtest->endstate) {
+ case JTAG_STATE_PAUSEIR:
+ /* ->DRSCan->IRSCan->IRCap->IRExit1->PauseIR */
+ aspeed_jtag_sm_cycle(aspeed_jtag, sm_idle_irpause,
+ sizeof(sm_idle_irpause));
+
+ aspeed_jtag->status = JTAG_STATE_PAUSEIR;
+ break;
+ case JTAG_STATE_PAUSEDR:
+ /* ->DRSCan->DRCap->DRExit1->PauseDR */
+ aspeed_jtag_sm_cycle(aspeed_jtag, sm_idle_drpause,
+ sizeof(sm_idle_drpause));
+
+ aspeed_jtag->status = JTAG_STATE_PAUSEDR;
+ break;
+ case JTAG_STATE_IDLE:
+ /* IDLE */
+ aspeed_jtag_tck_cycle(aspeed_jtag, 0, 0);
+ aspeed_jtag->status = JTAG_STATE_IDLE;
+ break;
+ default:
+ break;
+ }
+ break;
+
+ case JTAG_STATE_PAUSEIR:
+ /* Fall-through */
+ case JTAG_STATE_PAUSEDR:
+ /* From IR/DR Pause */
+ switch (runtest->endstate) {
+ case JTAG_STATE_PAUSEIR:
+ /*
+ * to Exit2 IR/DR->Updt IR/DR->DRSCan->IRSCan->IRCap->
+ * IRExit1->PauseIR
+ */
+ aspeed_jtag_sm_cycle(aspeed_jtag, sm_pause_irpause,
+ sizeof(sm_pause_irpause));
+
+ aspeed_jtag->status = JTAG_STATE_PAUSEIR;
+ break;
+ case JTAG_STATE_PAUSEDR:
+ /*
+ * to Exit2 IR/DR->Updt IR/DR->DRSCan->DRCap->
+ * DRExit1->PauseDR
+ */
+ aspeed_jtag_sm_cycle(aspeed_jtag, sm_pause_drpause,
+ sizeof(sm_pause_drpause));
+ aspeed_jtag->status = JTAG_STATE_PAUSEDR;
+ break;
+ case JTAG_STATE_IDLE:
+ /* to Exit2 IR/DR->Updt IR/DR->IDLE */
+ aspeed_jtag_sm_cycle(aspeed_jtag, sm_pause_idle,
+ sizeof(sm_pause_idle));
+ aspeed_jtag->status = JTAG_STATE_IDLE;
+ break;
+ default:
+ break;
+ }
+ break;
+
+ default:
+ dev_err(aspeed_jtag->dev, "aspeed_jtag_run_test_idle error\n");
+ break;
+ }
+
+ /* 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.
+ */
+static int aspeed_jtag_idle(struct jtag *jtag,
+ struct jtag_run_test_idle *runtest)
+{
+ struct aspeed_jtag *aspeed_jtag = jtag_priv(jtag);
+
+ dev_dbg(aspeed_jtag->dev, "runtest, status:%d, mode:%s, state:%s, reset:%d, tck:%d\n",
+ aspeed_jtag->status,
+ aspeed_jtag->mode & JTAG_XFER_HW_MODE ? "HW" : "SW",
+ end_status_str[runtest->endstate], runtest->reset,
+ runtest->tck);
+
+ if (!(aspeed_jtag->mode & JTAG_XFER_HW_MODE)) {
+ aspeed_jtag_run_test_idle_sw(aspeed_jtag, runtest);
+ return 0;
+ }
+
+ /* Disable sw mode */
+ aspeed_jtag_write(aspeed_jtag, 0, ASPEED_JTAG_SW);
+ /* x TMS high + 1 TMS low */
+ if (runtest->reset)
+ aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_CTL_ENG_EN |
+ ASPEED_JTAG_CTL_ENG_OUT_EN |
+ ASPEED_JTAG_CTL_FORCE_TMS, ASPEED_JTAG_CTRL);
+ else
+ 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);
+
+ aspeed_jtag->status = JTAG_STATE_IDLE;
+ return 0;
+}
+
+static void aspeed_jtag_xfer_sw(struct aspeed_jtag *aspeed_jtag,
+ struct jtag_xfer *xfer, unsigned long *data)
+{
+ unsigned long remain_xfer = xfer->length;
+ unsigned long shift_bits = 0;
+ unsigned long index = 0;
+ unsigned long tdi;
+ char tdo;
+
+ if (xfer->direction == JTAG_READ_XFER)
+ tdi = UINT_MAX;
+ else
+ tdi = data[index];
+
+ while (remain_xfer > 1) {
+ tdo = aspeed_jtag_tck_cycle(aspeed_jtag, 0,
+ tdi & ASPEED_JTAG_DATA_MSB);
+ data[index] |= tdo << (shift_bits %
+ ASPEED_JTAG_DATA_CHUNK_SIZE);
+
+ tdi >>= 1;
+ shift_bits++;
+ remain_xfer--;
+
+ if (shift_bits % ASPEED_JTAG_DATA_CHUNK_SIZE == 0) {
+ dev_dbg(aspeed_jtag->dev, "R/W data[%lu]:%lx\n",
+ index, data[index]);
+
+ tdo = 0;
+ index++;
+
+ if (xfer->direction == JTAG_READ_XFER)
+ tdi = UINT_MAX;
+ else
+ tdi = data[index];
+ }
+ }
+
+ tdo = aspeed_jtag_tck_cycle(aspeed_jtag, 1, tdi & ASPEED_JTAG_DATA_MSB);
+ data[index] |= tdo << (shift_bits % ASPEED_JTAG_DATA_CHUNK_SIZE);
+}
+
+static void aspeed_jtag_xfer_push_data(struct aspeed_jtag *aspeed_jtag,
+ enum jtag_xfer_type type, u32 bits_len)
+{
+ dev_dbg(aspeed_jtag->dev, "shift bits %d\n", bits_len);
+
+ if (type == JTAG_SIR_XFER) {
+ aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_IOUT_LEN(bits_len),
+ ASPEED_JTAG_CTRL);
+ aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_DOUT_LEN(bits_len) |
+ ASPEED_JTAG_CTL_INST_EN, ASPEED_JTAG_CTRL);
+ } else {
+ aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_DOUT_LEN(bits_len),
+ ASPEED_JTAG_CTRL);
+ aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_DOUT_LEN(bits_len) |
+ ASPEED_JTAG_CTL_DATA_EN, ASPEED_JTAG_CTRL);
+ }
+}
+
+static void aspeed_jtag_xfer_push_data_last(struct aspeed_jtag *aspeed_jtag,
+ enum jtag_xfer_type type,
+ u32 shift_bits,
+ enum jtag_endstate endstate)
+{
+ if (endstate != JTAG_STATE_IDLE) {
+ if (type == JTAG_SIR_XFER) {
+ dev_dbg(aspeed_jtag->dev, "IR Keep Pause\n");
+
+ aspeed_jtag_write(aspeed_jtag,
+ ASPEED_JTAG_IOUT_LEN(shift_bits),
+ ASPEED_JTAG_CTRL);
+ aspeed_jtag_write(aspeed_jtag,
+ ASPEED_JTAG_IOUT_LEN(shift_bits) |
+ ASPEED_JTAG_CTL_INST_EN,
+ ASPEED_JTAG_CTRL);
+ aspeed_jtag_wait_instruction_pause(aspeed_jtag);
+ } else {
+ dev_dbg(aspeed_jtag->dev, "DR Keep Pause\n");
+ aspeed_jtag_write(aspeed_jtag,
+ ASPEED_JTAG_DOUT_LEN(shift_bits) |
+ ASPEED_JTAG_CTL_DR_UPDATE,
+ ASPEED_JTAG_CTRL);
+ aspeed_jtag_write(aspeed_jtag,
+ ASPEED_JTAG_DOUT_LEN(shift_bits) |
+ ASPEED_JTAG_CTL_DR_UPDATE |
+ ASPEED_JTAG_CTL_DATA_EN,
+ ASPEED_JTAG_CTRL);
+ aspeed_jtag_wait_data_pause_complete(aspeed_jtag);
+ }
+ } else {
+ if (type == JTAG_SIR_XFER) {
+ dev_dbg(aspeed_jtag->dev, "IR go IDLE\n");
+
+ aspeed_jtag_write(aspeed_jtag,
+ ASPEED_JTAG_IOUT_LEN(shift_bits) |
+ ASPEED_JTAG_CTL_LASPEED_INST,
+ ASPEED_JTAG_CTRL);
+ aspeed_jtag_write(aspeed_jtag,
+ ASPEED_JTAG_IOUT_LEN(shift_bits) |
+ ASPEED_JTAG_CTL_LASPEED_INST |
+ ASPEED_JTAG_CTL_INST_EN,
+ ASPEED_JTAG_CTRL);
+ aspeed_jtag_wait_instruction_complete(aspeed_jtag);
+ } else {
+ dev_dbg(aspeed_jtag->dev, "DR go IDLE\n");
+
+ aspeed_jtag_write(aspeed_jtag,
+ ASPEED_JTAG_DOUT_LEN(shift_bits) |
+ ASPEED_JTAG_CTL_LASPEED_DATA,
+ ASPEED_JTAG_CTRL);
+ aspeed_jtag_write(aspeed_jtag,
+ ASPEED_JTAG_DOUT_LEN(shift_bits) |
+ ASPEED_JTAG_CTL_LASPEED_DATA |
+ ASPEED_JTAG_CTL_DATA_EN,
+ ASPEED_JTAG_CTRL);
+ aspeed_jtag_wait_data_complete(aspeed_jtag);
+ }
+ }
+}
+
+static void aspeed_jtag_xfer_hw(struct aspeed_jtag *aspeed_jtag,
+ struct jtag_xfer *xfer, unsigned long *data)
+{
+ unsigned long remain_xfer = xfer->length;
+ unsigned long index = 0;
+ char shift_bits;
+ u32 data_reg;
+
+ data_reg = xfer->type == JTAG_SIR_XFER ?
+ ASPEED_JTAG_INST : ASPEED_JTAG_DATA;
+ while (remain_xfer) {
+ if (xfer->direction == JTAG_WRITE_XFER) {
+ dev_dbg(aspeed_jtag->dev, "W dr->dr_data[%lu]:%lx\n",
+ index, data[index]);
+
+ aspeed_jtag_write(aspeed_jtag, data[index], data_reg);
+ } else {
+ aspeed_jtag_write(aspeed_jtag, 0, data_reg);
+ }
+
+ if (remain_xfer > ASPEED_JTAG_DATA_CHUNK_SIZE) {
+ shift_bits = ASPEED_JTAG_DATA_CHUNK_SIZE;
+
+ /*
+ * Read bytes were not equals to column length
+ * and go to Pause-DR
+ */
+ aspeed_jtag_xfer_push_data(aspeed_jtag, xfer->type,
+ shift_bits);
+ } else {
+ /*
+ * Read bytes equals to column length =>
+ * Update-DR
+ */
+ shift_bits = remain_xfer;
+ aspeed_jtag_xfer_push_data_last(aspeed_jtag, xfer->type,
+ shift_bits,
+ xfer->endstate);
+ }
+
+ if (xfer->direction == JTAG_READ_XFER) {
+ if (shift_bits < ASPEED_JTAG_DATA_CHUNK_SIZE) {
+ data[index] = aspeed_jtag_read(aspeed_jtag,
+ data_reg);
+
+ data[index] >>= ASPEED_JTAG_DATA_CHUNK_SIZE -
+ shift_bits;
+ } else {
+ data[index] = aspeed_jtag_read(aspeed_jtag,
+ data_reg);
+ }
+ dev_dbg(aspeed_jtag->dev, "R dr->dr_data[%lu]:%lx\n",
+ index, data[index]);
+ }
+
+ remain_xfer = remain_xfer - shift_bits;
+ index++;
+ dev_dbg(aspeed_jtag->dev, "remain_xfer %lu\n", remain_xfer);
+ }
+}
+
+static int aspeed_jtag_xfer(struct jtag *jtag, struct jtag_xfer *xfer,
+ u8 *xfer_data)
+{
+ static const u8 sm_update_shiftir[] = {1, 1, 0, 0};
+ static const u8 sm_update_shiftdr[] = {1, 0, 0};
+ static const u8 sm_pause_idle[] = {1, 1, 0};
+ static const u8 sm_pause_update[] = {1, 1};
+ struct aspeed_jtag *aspeed_jtag = jtag_priv(jtag);
+ unsigned long *data = (unsigned long *)xfer_data;
+ unsigned long remain_xfer = xfer->length;
+ char dbg_str[256];
+ int pos = 0;
+ 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]);
+ }
+
+ dev_dbg(aspeed_jtag->dev, " %s %s xfer, mode:%s, END:%d, len:%lu, TDI[%s]\n",
+ xfer->type == JTAG_SIR_XFER ? "SIR" : "SDR",
+ xfer->direction == JTAG_READ_XFER ? "READ" : "WRITE",
+ aspeed_jtag->mode & JTAG_XFER_HW_MODE ? "HW" : "SW",
+ xfer->endstate, remain_xfer, dbg_str);
+
+ if (!(aspeed_jtag->mode & JTAG_XFER_HW_MODE)) {
+ /* SW mode */
+ aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_SW_MODE_EN |
+ ASPEED_JTAG_SW_MODE_TDIO, ASPEED_JTAG_SW);
+
+ if (aspeed_jtag->status != JTAG_STATE_IDLE) {
+ /*IR/DR Pause->Exit2 IR / DR->Update IR /DR */
+ aspeed_jtag_sm_cycle(aspeed_jtag, sm_pause_update,
+ sizeof(sm_pause_update));
+ }
+
+ if (xfer->type == JTAG_SIR_XFER)
+ /* ->IRSCan->CapIR->ShiftIR */
+ aspeed_jtag_sm_cycle(aspeed_jtag, sm_update_shiftir,
+ sizeof(sm_update_shiftir));
+ else
+ /* ->DRScan->DRCap->DRShift */
+ aspeed_jtag_sm_cycle(aspeed_jtag, sm_update_shiftdr,
+ sizeof(sm_update_shiftdr));
+
+ aspeed_jtag_xfer_sw(aspeed_jtag, xfer, data);
+
+ /* DIPause/DRPause */
+ aspeed_jtag_tck_cycle(aspeed_jtag, 0, 0);
+
+ if (xfer->endstate == JTAG_STATE_IDLE) {
+ /* ->DRExit2->DRUpdate->IDLE */
+ aspeed_jtag_sm_cycle(aspeed_jtag, sm_pause_idle,
+ sizeof(sm_pause_idle));
+ }
+ } else {
+ /* hw mode */
+ aspeed_jtag_write(aspeed_jtag, 0, ASPEED_JTAG_SW);
+ aspeed_jtag_xfer_hw(aspeed_jtag, xfer, data);
+ }
+
+ aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_SW_MODE_EN |
+ ASPEED_JTAG_SW_MODE_TDIO, ASPEED_JTAG_SW);
+ aspeed_jtag->status = xfer->endstate;
+ return 0;
+}
+
+static int aspeed_jtag_status_get(struct jtag *jtag, u32 *status)
+{
+ struct aspeed_jtag *aspeed_jtag = jtag_priv(jtag);
+
+ *status = aspeed_jtag->status;
+ return 0;
+}
+
+static irqreturn_t aspeed_jtag_interrupt(s32 this_irq, void *dev_id)
+{
+ struct aspeed_jtag *aspeed_jtag = dev_id;
+ irqreturn_t ret;
+ u32 status;
+
+ status = aspeed_jtag_read(aspeed_jtag, ASPEED_JTAG_ISR);
+ dev_dbg(aspeed_jtag->dev, "status %x\n", status);
+
+ if (status & ASPEED_JTAG_ISR_INT_MASK) {
+ aspeed_jtag_write(aspeed_jtag,
+ (status & ASPEED_JTAG_ISR_INT_MASK)
+ | (status & ASPEED_JTAG_ISR_INT_EN_MASK),
+ ASPEED_JTAG_ISR);
+ aspeed_jtag->flag |= status & ASPEED_JTAG_ISR_INT_MASK;
+ }
+
+ if (aspeed_jtag->flag) {
+ wake_up_interruptible(&aspeed_jtag->jtag_wq);
+ ret = IRQ_HANDLED;
+ } else {
+ dev_err(aspeed_jtag->dev, "irq status:%x\n",
+ status);
+ ret = IRQ_NONE;
+ }
+ return ret;
+}
+
+int aspeed_jtag_init(struct platform_device *pdev,
+ struct aspeed_jtag *aspeed_jtag)
+{
+ struct resource *res;
+ int err;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ aspeed_jtag->reg_base = devm_ioremap_resource(aspeed_jtag->dev, res);
+ if (IS_ERR(aspeed_jtag->reg_base))
+ return -ENOMEM;
+
+ aspeed_jtag->pclk = devm_clk_get(aspeed_jtag->dev, NULL);
+ if (IS_ERR(aspeed_jtag->pclk)) {
+ dev_err(aspeed_jtag->dev, "devm_clk_get failed\n");
+ return PTR_ERR(aspeed_jtag->pclk);
+ }
+
+ aspeed_jtag->irq = platform_get_irq(pdev, 0);
+ if (aspeed_jtag->irq < 0) {
+ dev_err(aspeed_jtag->dev, "no irq specified\n");
+ return -ENOENT;
+ }
+
+ clk_prepare_enable(aspeed_jtag->pclk);
+
+ aspeed_jtag->rst = devm_reset_control_get_shared(aspeed_jtag->dev,
+ NULL);
+ if (IS_ERR(aspeed_jtag->rst)) {
+ dev_err(aspeed_jtag->dev,
+ "missing or invalid reset controller device tree entry");
+ return PTR_ERR(aspeed_jtag->rst);
+ }
+ reset_control_deassert(aspeed_jtag->rst);
+
+ /* Enable clock */
+ aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_CTL_ENG_EN |
+ ASPEED_JTAG_CTL_ENG_OUT_EN, ASPEED_JTAG_CTRL);
+ aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_SW_MODE_EN |
+ ASPEED_JTAG_SW_MODE_TDIO, ASPEED_JTAG_SW);
+
+ err = devm_request_irq(aspeed_jtag->dev, aspeed_jtag->irq,
+ aspeed_jtag_interrupt, 0,
+ "aspeed-jtag", aspeed_jtag);
+ if (err) {
+ dev_err(aspeed_jtag->dev, "unable to get IRQ");
+ goto clk_unprep;
+ }
+ dev_dbg(&pdev->dev, "IRQ %d.\n", aspeed_jtag->irq);
+
+ aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_ISR_INST_PAUSE |
+ ASPEED_JTAG_ISR_INST_COMPLETE |
+ ASPEED_JTAG_ISR_DATA_PAUSE |
+ ASPEED_JTAG_ISR_DATA_COMPLETE |
+ ASPEED_JTAG_ISR_INST_PAUSE_EN |
+ ASPEED_JTAG_ISR_INST_COMPLETE_EN |
+ ASPEED_JTAG_ISR_DATA_PAUSE_EN |
+ ASPEED_JTAG_ISR_DATA_COMPLETE_EN,
+ ASPEED_JTAG_ISR);
+
+ aspeed_jtag->flag = 0;
+ aspeed_jtag->mode = 0;
+ init_waitqueue_head(&aspeed_jtag->jtag_wq);
+ return 0;
+
+clk_unprep:
+ clk_disable_unprepare(aspeed_jtag->pclk);
+ return err;
+}
+
+int aspeed_jtag_deinit(struct platform_device *pdev,
+ struct aspeed_jtag *aspeed_jtag)
+{
+ aspeed_jtag_write(aspeed_jtag, 0, ASPEED_JTAG_ISR);
+ /* Disable clock */
+ aspeed_jtag_write(aspeed_jtag, 0, ASPEED_JTAG_CTRL);
+ reset_control_assert(aspeed_jtag->rst);
+ clk_disable_unprepare(aspeed_jtag->pclk);
+ return 0;
+}
+
+static const struct jtag_ops aspeed_jtag_ops = {
+ .freq_get = aspeed_jtag_freq_get,
+ .freq_set = aspeed_jtag_freq_set,
+ .status_get = aspeed_jtag_status_get,
+ .idle = aspeed_jtag_idle,
+ .xfer = aspeed_jtag_xfer,
+ .mode_set = aspeed_jtag_mode_set
+};
+
+static int aspeed_jtag_probe(struct platform_device *pdev)
+{
+ struct aspeed_jtag *aspeed_jtag;
+ struct device *dev;
+ struct jtag *jtag;
+ int err;
+
+ dev = &pdev->dev;
+ jtag = jtag_alloc(sizeof(*aspeed_jtag), &aspeed_jtag_ops);
+ if (!jtag)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, jtag);
+ aspeed_jtag = jtag_priv(jtag);
+ aspeed_jtag->dev = &pdev->dev;
+
+ /* Initialize device*/
+ err = aspeed_jtag_init(pdev, aspeed_jtag);
+ if (err)
+ goto err_jtag_init;
+
+ /* Initialize JTAG core structure*/
+ err = jtag_register(jtag);
+ if (err)
+ goto err_jtag_register;
+
+ return 0;
+
+err_jtag_register:
+ aspeed_jtag_deinit(pdev, aspeed_jtag);
+err_jtag_init:
+ jtag_free(jtag);
+ return err;
+}
+
+static int aspeed_jtag_remove(struct platform_device *pdev)
+{
+ struct jtag *jtag;
+
+ jtag = platform_get_drvdata(pdev);
+ aspeed_jtag_deinit(pdev, jtag_priv(jtag));
+ jtag_unregister(jtag);
+ jtag_free(jtag);
+ return 0;
+}
+
+static const struct of_device_id aspeed_jtag_of_match[] = {
+ { .compatible = "aspeed,ast2400-jtag", },
+ { .compatible = "aspeed,ast2500-jtag", },
+ {}
+};
+
+static struct platform_driver aspeed_jtag_driver = {
+ .probe = aspeed_jtag_probe,
+ .remove = aspeed_jtag_remove,
+ .driver = {
+ .name = ASPEED_JTAG_NAME,
+ .of_match_table = aspeed_jtag_of_match,
+ },
+};
+module_platform_driver(aspeed_jtag_driver);
+
+MODULE_AUTHOR("Oleksandr Shamray <oleksandrs-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>");
+MODULE_DESCRIPTION("ASPEED JTAG driver");
+MODULE_LICENSE("GPL v2");
--
1.7.1
^ permalink raw reply related
* [patch v18 1/4] drivers: jtag: Add JTAG core driver
From: Oleksandr Shamray @ 2018-01-29 14:31 UTC (permalink / raw)
To: gregkh, arnd
Cc: linux-kernel, linux-arm-kernel, devicetree, openbmc, joel, jiri,
tklauser, linux-serial, vadimp, system-sw-low-level, robh+dt,
openocd-devel-owner, linux-api, davem, mchehab, Oleksandr Shamray,
Jiri Pirko
In-Reply-To: <1517236305-4880-1-git-send-email-oleksandrs@mellanox.com>
Initial patch for JTAG driver
JTAG class driver provide infrastructure to support hardware/software
JTAG platform drivers. It provide user layer API interface for flashing
and debugging external devices which equipped with JTAG interface
using standard transactions.
Driver exposes set of IOCTL to user space for:
- XFER:
- SIR (Scan Instruction Register, IEEE 1149.1 Data Register scan);
- SDR (Scan Data Register, IEEE 1149.1 Instruction Register scan);
- RUNTEST (Forces the IEEE 1149.1 bus to a run state for a specified
number of clocks).
- SIOCFREQ/GIOCFREQ for setting and reading JTAG frequency.
Driver core provides set of internal APIs for allocation and
registration:
- jtag_register;
- jtag_unregister;
- jtag_alloc;
- jtag_free;
Platform driver on registration with jtag-core creates the next
entry in dev folder:
/dev/jtagX
Signed-off-by: Oleksandr Shamray <oleksandrs@mellanox.com>
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Acked-by: Philippe Ombredanne <pombredanne@nexb.com>
---
v17->v18
Comments pointed by Julia Cartwright <juliac@eso.teric.us>
- Change to return -EOPNOTSUPP in case of error in JTAG_GIOCFREQ
- Add ops callbacks check to jtag_alloc
- Add err check for copy_to_user
- Move the kfree() above the if (err) in JTAG_IOCXFER
- remove unnecessary check for error after put_user
- add padding to struct jtag_xfer
v16->v17
Comments pointed by Julia Cartwright <juliac@eso.teric.us>
- Fix memory allocation on jtag alloc
- Move out unnecessary form lock on jtag open
- Rework jtag register behavior
v15->v16
Commen ts pointed by Florian Fainelli <f.fainelli@gmail.com>
- move check jtag->ops->* in ioctl before get_user()
- change error type -EINVAL --> -EBUSY on open already opened jtag
- remove unnecessary ARCH_DMA_MINALIGN flag from kzalloc
- remove define ARCH_DMA_MINALIGN
v14->v15
v13->v14
Comments pointed by Philippe Ombredanne <pombredanne@nexb.com>
- Change style of head block comment from /**/ to //
v12->v13
Comments pointed by Philippe Ombredanne <pombredanne@nexb.com>
- Change jtag.c licence type to
SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note
and reorder line with license in description
v11->v12
Comments pointed by Greg KH <gregkh@linuxfoundation.org>
- Change jtag.h licence type to
SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note
and reorder line with license in description
Chip Bilbrey <chip@bilbrey.org>
- Remove Apeed reference from uapi jtag.h header
- Remove access mode from xfer and idle transactions
- Add new ioctl JTAG_SIOCMODE for set hw mode
- Add only one open per JTAG port blocking with mutex blocking
v10->v11
Notifications from kbuild test robot <lkp@intel.com>
- include types.h headeri to jtag.h
- fix incompatible type of xfer callback
- remove rdundant class defination
- Fix return order in case of xfer error
V9->v10
Comments pointed by Greg KH <gregkh@linuxfoundation.org>
- remove unnecessary alignment for pirv data
- move jtag_copy_to_user and jtag_copy_from_user code just to ioctl
- move int jtag_run_test_idle_op and jtag_xfer_op code
just to ioctl
- change return error codes to more applicable
- add missing error checks
- fix error check order in ioctl
- remove unnecessary blank lines
- add param validation to ioctl
- remove compat_ioctl
- remove only one open per JTAG port blocking.
User will care about this.
- Fix idr memory leak on jtag_exit
- change cdev device type to misc
V8->v9
Comments pointed by Arnd Bergmann <arnd@arndb.de>
- use get_user() instead of __get_user().
- change jtag->open type from int to atomic_t
- remove spinlock on jtg_open
- remove mutex on jtag_register
- add unregister_chrdev_region on jtag_init err
- add unregister_chrdev_region on jtag_exit
- remove unnecessary pointer casts
- add *data parameter to xfer function prototype
v7->v8
Comments pointed by Moritz Fischer <moritz.fischer@ettus.com>
- Fix misspelling s/friver/driver
v6->v7
Notifications from kbuild test robot <lkp@intel.com>
- Remove include asm/types.h from jtag.h
- Add include <linux/types.h> to jtag.c
v5->v6
v4->v5
v3->v4
Comments pointed by Arnd Bergmann <arnd@arndb.de>
- change transaction pointer tdio type to __u64
- change internal status type from enum to __u32
- reorder jtag_xfer members to avoid the implied padding
- add __packed attribute to jtag_xfer and jtag_run_test_idle
v2->v3
Notifications from kbuild test robot <lkp@intel.com>
- Change include path to <linux/types.h> in jtag.h
v1->v2
Comments pointed by Greg KH <gregkh@linuxfoundation.org>
- Change license type from GPLv2/BSD to GPLv2
- Change type of variables which crossed user/kernel to __type
- Remove "default n" from Kconfig
Comments pointed by Andrew Lunn <andrew@lunn.ch>
- Change list_add_tail in jtag_unregister to list_del
Comments pointed by Neil Armstrong <narmstrong@baylibre.com>
- Add SPDX-License-Identifier instead of license text
Comments pointed by Arnd Bergmann <arnd@arndb.de>
- Change __copy_to_user to memdup_user
- Change __put_user to put_user
- Change type of variables to __type for compatible 32 and 64-bit systems
- Add check for maximum xfer data size
- Change lookup data mechanism to get jtag data from inode
- Add .compat_ioctl to file ops
- Add mem alignment for jtag priv data
Comments pointed by Tobias Klauser <tklauser@distanz.ch>
- Change function names to avoid match with variable types
- Fix description for jtag_ru_test_idle in uapi jtag.h
- Fix misprints IDEL/IDLE, trough/through
---
Documentation/ioctl/ioctl-number.txt | 2 +
MAINTAINERS | 10 ++
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/jtag/Kconfig | 16 ++
drivers/jtag/Makefile | 1 +
drivers/jtag/jtag.c | 273 ++++++++++++++++++++++++++++++++++
include/linux/jtag.h | 41 +++++
include/uapi/linux/jtag.h | 105 +++++++++++++
9 files changed, 451 insertions(+), 0 deletions(-)
create mode 100644 drivers/jtag/Kconfig
create mode 100644 drivers/jtag/Makefile
create mode 100644 drivers/jtag/jtag.c
create mode 100644 include/linux/jtag.h
create mode 100644 include/uapi/linux/jtag.h
diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index 3e3fdae..1af2508 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -321,6 +321,8 @@ Code Seq#(hex) Include File Comments
0xB0 all RATIO devices in development:
<mailto:vgo@ratio.de>
0xB1 00-1F PPPoX <mailto:mostrows@styx.uwaterloo.ca>
+0xB2 00-0f linux/jtag.h JTAG driver
+ <mailto:oleksandrs@mellanox.com>
0xB3 00 linux/mmc/ioctl.h
0xB4 00-0F linux/gpio.h <mailto:linux-gpio@vger.kernel.org>
0xB5 00-0F uapi/linux/rpmsg.h <mailto:linux-remoteproc@vger.kernel.org>
diff --git a/MAINTAINERS b/MAINTAINERS
index b46c9ce..42aac3a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7513,6 +7513,16 @@ L: linux-serial@vger.kernel.org
S: Maintained
F: drivers/tty/serial/jsm/
+JTAG SUBSYSTEM
+M: Oleksandr Shamray <oleksandrs@mellanox.com>
+M: Vadim Pasternak <vadimp@mellanox.com>
+S: Maintained
+F: include/linux/jtag.h
+F: include/uapi/linux/jtag.h
+F: drivers/jtag/
+F: Documentation/devicetree/bindings/jtag/
+F: Documentation/ABI/testing/jtag-cdev
+
K10TEMP HARDWARE MONITORING DRIVER
M: Clemens Ladisch <clemens@ladisch.de>
L: linux-hwmon@vger.kernel.org
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 152744c..414a34b 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -211,4 +211,6 @@ source "drivers/mux/Kconfig"
source "drivers/opp/Kconfig"
+source "drivers/jtag/Kconfig"
+
endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index e06f7f6..6d50f74 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -184,3 +184,4 @@ obj-$(CONFIG_FPGA) += fpga/
obj-$(CONFIG_FSI) += fsi/
obj-$(CONFIG_TEE) += tee/
obj-$(CONFIG_MULTIPLEXER) += mux/
+obj-$(CONFIG_JTAG) += jtag/
diff --git a/drivers/jtag/Kconfig b/drivers/jtag/Kconfig
new file mode 100644
index 0000000..0fad1a3
--- /dev/null
+++ b/drivers/jtag/Kconfig
@@ -0,0 +1,16 @@
+menuconfig JTAG
+ tristate "JTAG support"
+ ---help---
+ This provides basic core functionality support for jtag class devices
+ Hardware equipped with JTAG microcontroller which can be built
+ on top of this drivers. Driver exposes the set of IOCTL to the
+ user space for:
+ SIR (Scan Instruction Register, IEEE 1149.1 Data Register scan);
+ SDR (Scan Data Register, IEEE 1149.1 Instruction Register scan);
+ RUNTEST (Forces IEEE 1149.1 bus to a run state for specified
+ number of clocks).
+
+ If you want this support, you should say Y here.
+
+ To compile this driver as a module, choose M here: the module will
+ be called jtag.
diff --git a/drivers/jtag/Makefile b/drivers/jtag/Makefile
new file mode 100644
index 0000000..af37493
--- /dev/null
+++ b/drivers/jtag/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_JTAG) += jtag.o
diff --git a/drivers/jtag/jtag.c b/drivers/jtag/jtag.c
new file mode 100644
index 0000000..69dc833
--- /dev/null
+++ b/drivers/jtag/jtag.c
@@ -0,0 +1,273 @@
+// SPDX-License-Identifier: GPL-2.0
+// drivers/jtag/jtag.c
+//
+// Copyright (c) 2017 Mellanox Technologies. All rights reserved.
+// Copyright (c) 2017 Oleksandr Shamray <oleksandrs@mellanox.com>
+
+#include <linux/cdev.h>
+#include <linux/device.h>
+#include <linux/jtag.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/rtnetlink.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+#include <uapi/linux/jtag.h>
+
+#define JTAG_NAME "jtag0"
+#define MAX_JTAG_NAME_LEN (sizeof("jtag") + 5)
+
+struct jtag {
+ struct miscdevice miscdev;
+ struct device *dev;
+ const struct jtag_ops *ops;
+ int id;
+ bool opened;
+ struct mutex open_lock;
+ unsigned long priv[0];
+};
+
+static DEFINE_IDA(jtag_ida);
+
+void *jtag_priv(struct jtag *jtag)
+{
+ return jtag->priv;
+}
+EXPORT_SYMBOL_GPL(jtag_priv);
+
+static long jtag_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ struct jtag *jtag = file->private_data;
+ struct jtag_run_test_idle idle;
+ struct jtag_xfer xfer;
+ u8 *xfer_data;
+ u32 data_size;
+ u32 value;
+ int err;
+
+ if (!arg)
+ return -EINVAL;
+
+ switch (cmd) {
+ case JTAG_GIOCFREQ:
+ if (!jtag->ops->freq_get)
+ return -EOPNOTSUPP;
+
+ err = jtag->ops->freq_get(jtag, &value);
+ if (err)
+ break;
+
+ if (put_user(value, (__u32 *)arg))
+ err = -EFAULT;
+ break;
+
+ case JTAG_SIOCFREQ:
+ if (!jtag->ops->freq_set)
+ return -EOPNOTSUPP;
+
+ if (get_user(value, (__u32 *)arg))
+ return -EFAULT;
+ if (value == 0)
+ return -EINVAL;
+
+ err = jtag->ops->freq_set(jtag, value);
+ break;
+
+ case JTAG_IOCRUNTEST:
+ if (copy_from_user(&idle, (void *)arg,
+ sizeof(struct jtag_run_test_idle)))
+ return -EFAULT;
+
+ if (idle.endstate > JTAG_STATE_PAUSEDR)
+ return -EINVAL;
+
+ err = jtag->ops->idle(jtag, &idle);
+ break;
+
+ case JTAG_IOCXFER:
+ if (copy_from_user(&xfer, (void *)arg,
+ sizeof(struct jtag_xfer)))
+ return -EFAULT;
+
+ if (xfer.length >= JTAG_MAX_XFER_DATA_LEN)
+ return -EINVAL;
+
+ if (xfer.type > JTAG_SDR_XFER)
+ return -EINVAL;
+
+ if (xfer.direction > JTAG_WRITE_XFER)
+ return -EINVAL;
+
+ if (xfer.endstate > JTAG_STATE_PAUSEDR)
+ return -EINVAL;
+
+ data_size = DIV_ROUND_UP(xfer.length, BITS_PER_BYTE);
+ xfer_data = memdup_user(u64_to_user_ptr(xfer.tdio), data_size);
+
+ if (IS_ERR(xfer_data))
+ return -EFAULT;
+
+ err = jtag->ops->xfer(jtag, &xfer, xfer_data);
+ if (err) {
+ kfree(xfer_data);
+ return -EFAULT;
+ }
+
+ err = copy_to_user(u64_to_user_ptr(xfer.tdio),
+ (void *)(xfer_data), data_size);
+ kfree(xfer_data);
+ if (err)
+ return -EFAULT;
+
+ if (copy_to_user((void *)arg, &xfer, sizeof(struct jtag_xfer)))
+ return -EFAULT;
+ break;
+
+ case JTAG_GIOCSTATUS:
+ err = jtag->ops->status_get(jtag, &value);
+ if (err)
+ break;
+
+ err = put_user(value, (__u32 *)arg);
+ break;
+ case JTAG_SIOCMODE:
+ if (get_user(value, (__u32 *)arg))
+ return -EFAULT;
+ if (value == 0)
+ return -EINVAL;
+
+ err = jtag->ops->mode_set(jtag, value);
+ break;
+
+ default:
+ return -EINVAL;
+ }
+ return err;
+}
+
+static int jtag_open(struct inode *inode, struct file *file)
+{
+ struct jtag *jtag = container_of(file->private_data, struct jtag,
+ miscdev);
+
+ if (mutex_lock_interruptible(&jtag->open_lock))
+ return -ERESTARTSYS;
+
+ if (jtag->opened) {
+ mutex_unlock(&jtag->open_lock);
+ return -EBUSY;
+ }
+ jtag->opened = true;
+ mutex_unlock(&jtag->open_lock);
+
+ nonseekable_open(inode, file);
+ file->private_data = jtag;
+ return 0;
+}
+
+static int jtag_release(struct inode *inode, struct file *file)
+{
+ struct jtag *jtag = file->private_data;
+
+ mutex_lock(&jtag->open_lock);
+ jtag->opened = false;
+ mutex_unlock(&jtag->open_lock);
+ return 0;
+}
+
+static const struct file_operations jtag_fops = {
+ .owner = THIS_MODULE,
+ .open = jtag_open,
+ .release = jtag_release,
+ .llseek = noop_llseek,
+ .unlocked_ioctl = jtag_ioctl,
+};
+
+struct jtag *jtag_alloc(size_t priv_size, const struct jtag_ops *ops)
+{
+ struct jtag *jtag;
+
+ jtag = kzalloc(sizeof(*jtag) + priv_size, GFP_KERNEL);
+ if (!jtag)
+ return NULL;
+
+ if (!ops)
+ return NULL;
+
+ if (!ops->idle || !ops->mode_set || !ops->status_get || !ops->xfer)
+ return NULL;
+
+ jtag->ops = ops;
+ return jtag;
+}
+EXPORT_SYMBOL_GPL(jtag_alloc);
+
+void jtag_free(struct jtag *jtag)
+{
+ kfree(jtag);
+}
+EXPORT_SYMBOL_GPL(jtag_free);
+
+int jtag_register(struct jtag *jtag)
+{
+ char *name;
+ int err;
+ int id;
+
+ id = ida_simple_get(&jtag_ida, 0, 0, GFP_KERNEL);
+ if (id < 0)
+ return id;
+
+ jtag->id = id;
+ jtag->opened = false;
+
+ name = kzalloc(MAX_JTAG_NAME_LEN, GFP_KERNEL);
+ if (!name) {
+ err = -ENOMEM;
+ goto err_jtag_alloc;
+ }
+
+ err = snprintf(name, MAX_JTAG_NAME_LEN, "jtag%d", id);
+ if (err < 0)
+ goto err_jtag_name;
+
+ mutex_init(&jtag->open_lock);
+ jtag->miscdev.fops = &jtag_fops;
+ jtag->miscdev.minor = MISC_DYNAMIC_MINOR;
+ jtag->miscdev.name = name;
+
+ err = misc_register(&jtag->miscdev);
+ if (err) {
+ dev_err(jtag->dev, "Unable to register device\n");
+ goto err_jtag_name;
+ }
+ return 0;
+
+err_jtag_name:
+ kfree(name);
+err_jtag_alloc:
+ ida_simple_remove(&jtag_ida, id);
+ return err;
+}
+EXPORT_SYMBOL_GPL(jtag_register);
+
+void jtag_unregister(struct jtag *jtag)
+{
+ misc_deregister(&jtag->miscdev);
+ kfree(jtag->miscdev.name);
+ ida_simple_remove(&jtag_ida, jtag->id);
+}
+EXPORT_SYMBOL_GPL(jtag_unregister);
+
+static void __exit jtag_exit(void)
+{
+ ida_destroy(&jtag_ida);
+}
+
+module_exit(jtag_exit);
+
+MODULE_AUTHOR("Oleksandr Shamray <oleksandrs@mellanox.com>");
+MODULE_DESCRIPTION("Generic jtag support");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/jtag.h b/include/linux/jtag.h
new file mode 100644
index 0000000..918cfe0
--- /dev/null
+++ b/include/linux/jtag.h
@@ -0,0 +1,41 @@
+// SPDX-License-Identifier: GPL-2.0
+// include/linux/jtag.h - JTAG class driver
+//
+// Copyright (c) 2017 Mellanox Technologies. All rights reserved.
+// Copyright (c) 2017 Oleksandr Shamray <oleksandrs@mellanox.com>
+
+#ifndef __JTAG_H
+#define __JTAG_H
+
+#include <uapi/linux/jtag.h>
+
+#define jtag_u64_to_ptr(arg) ((void *)(uintptr_t)arg)
+
+#define JTAG_MAX_XFER_DATA_LEN 65535
+
+struct jtag;
+/**
+ * struct jtag_ops - callbacks for jtag control functions:
+ *
+ * @freq_get: get frequency function. Filled by device driver
+ * @freq_set: set frequency function. Filled by device driver
+ * @status_get: set status function. Filled by device driver
+ * @idle: set JTAG to idle state function. Filled by device driver
+ * @xfer: send JTAG xfer function. Filled by device driver
+ */
+struct jtag_ops {
+ int (*freq_get)(struct jtag *jtag, u32 *freq);
+ int (*freq_set)(struct jtag *jtag, u32 freq);
+ int (*status_get)(struct jtag *jtag, u32 *state);
+ int (*idle)(struct jtag *jtag, struct jtag_run_test_idle *idle);
+ int (*xfer)(struct jtag *jtag, struct jtag_xfer *xfer, u8 *xfer_data);
+ int (*mode_set)(struct jtag *jtag, u32 mode_mask);
+};
+
+void *jtag_priv(struct jtag *jtag);
+int jtag_register(struct jtag *jtag);
+void jtag_unregister(struct jtag *jtag);
+struct jtag *jtag_alloc(size_t priv_size, const struct jtag_ops *ops);
+void jtag_free(struct jtag *jtag);
+
+#endif /* __JTAG_H */
diff --git a/include/uapi/linux/jtag.h b/include/uapi/linux/jtag.h
new file mode 100644
index 0000000..d688d24
--- /dev/null
+++ b/include/uapi/linux/jtag.h
@@ -0,0 +1,105 @@
+// SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note
+// include/uapi/linux/jtag.h - JTAG class driver uapi
+//
+// Copyright (c) 2017 Mellanox Technologies. All rights reserved.
+// Copyright (c) 2017 Oleksandr Shamray <oleksandrs@mellanox.com>
+
+#ifndef __UAPI_LINUX_JTAG_H
+#define __UAPI_LINUX_JTAG_H
+
+#include <linux/types.h>
+/*
+ * JTAG_XFER_HW_MODE: JTAG hardware mode. Used to set HW drived or bitbang
+ * mode. This is bitmask param of ioctl JTAG_SIOCMODE command
+ */
+#define JTAG_XFER_HW_MODE 1
+
+/**
+ * enum jtag_endstate:
+ *
+ * @JTAG_STATE_IDLE: JTAG state machine IDLE state
+ * @JTAG_STATE_PAUSEIR: JTAG state machine PAUSE_IR state
+ * @JTAG_STATE_PAUSEDR: JTAG state machine PAUSE_DR state
+ */
+enum jtag_endstate {
+ JTAG_STATE_IDLE,
+ JTAG_STATE_PAUSEIR,
+ JTAG_STATE_PAUSEDR,
+};
+
+/**
+ * enum jtag_xfer_type:
+ *
+ * @JTAG_SIR_XFER: SIR transfer
+ * @JTAG_SDR_XFER: SDR transfer
+ */
+enum jtag_xfer_type {
+ JTAG_SIR_XFER,
+ JTAG_SDR_XFER,
+};
+
+/**
+ * enum jtag_xfer_direction:
+ *
+ * @JTAG_READ_XFER: read transfer
+ * @JTAG_WRITE_XFER: write transfer
+ */
+enum jtag_xfer_direction {
+ JTAG_READ_XFER,
+ JTAG_WRITE_XFER,
+};
+
+/**
+ * struct jtag_run_test_idle - forces JTAG state machine to
+ * RUN_TEST/IDLE state
+ *
+ * @reset: 0 - run IDLE/PAUSE from current state
+ * 1 - go through TEST_LOGIC/RESET state before IDLE/PAUSE
+ * @end: completion flag
+ * @tck: clock counter
+ *
+ * Structure represents interface to JTAG device for jtag idle
+ * execution.
+ */
+struct jtag_run_test_idle {
+ __u8 reset;
+ __u8 endstate;
+ __u8 tck;
+};
+
+/**
+ * struct jtag_xfer - jtag xfer:
+ *
+ * @type: transfer type
+ * @direction: xfer direction
+ * @length: xfer bits len
+ * @tdio : xfer data array
+ * @endir: xfer end state
+ *
+ * Structure represents interface to JTAG device for jtag sdr xfer
+ * execution.
+ */
+struct jtag_xfer {
+ __u8 type;
+ __u8 direction;
+ __u8 endstate;
+ __u8 padding;
+ __u32 length;
+ __u64 tdio;
+};
+
+/* ioctl interface */
+#define __JTAG_IOCTL_MAGIC 0xb2
+
+#define JTAG_IOCRUNTEST _IOW(__JTAG_IOCTL_MAGIC, 0,\
+ struct jtag_run_test_idle)
+#define JTAG_SIOCFREQ _IOW(__JTAG_IOCTL_MAGIC, 1, unsigned int)
+#define JTAG_GIOCFREQ _IOR(__JTAG_IOCTL_MAGIC, 2, unsigned int)
+#define JTAG_IOCXFER _IOWR(__JTAG_IOCTL_MAGIC, 3, struct jtag_xfer)
+#define JTAG_GIOCSTATUS _IOWR(__JTAG_IOCTL_MAGIC, 4, enum jtag_endstate)
+#define JTAG_SIOCMODE _IOW(__JTAG_IOCTL_MAGIC, 5, unsigned int)
+
+#define JTAG_FIRST_MINOR 0
+#define JTAG_MAX_DEVICES 32
+
+#endif /* __UAPI_LINUX_JTAG_H */
--
1.7.1
^ permalink raw reply related
* [patch v18 0/4] JTAG driver introduction
From: Oleksandr Shamray @ 2018-01-29 14:31 UTC (permalink / raw)
To: gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r, arnd-r2nGTMty4D4
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, openbmc-uLR06cmDAlY/bJ5BZ2RsiQ,
joel-U3u1mxZcP9KHXe+LvDLADg, jiri-rHqAuBHg3fBzbRFIqnYvSA,
tklauser-93Khv+1bN0NyDzI6CaY1VQ,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
vadimp-VPRAkNaXOzVWk0Htik3J/w,
system-sw-low-level-VPRAkNaXOzVWk0Htik3J/w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
openocd-devel-owner-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
linux-api-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
mchehab-DgEjT+Ai2ygdnm+yROfE0A, Oleksandr Shamray
When a need raise up to use JTAG interface for system's devices
programming or CPU debugging, usually the user layer
application implements jtag protocol by bit-bang or using a
proprietary connection to vendor hardware.
This method can be slow and not generic.
We propose to implement general JTAG interface and infrastructure
to communicate with user layer application. In such way, we can
have the standard JTAG interface core part and separation from
specific HW implementation.
This allow new capability to debug the CPU or program system's
device via BMC without additional devices nor cost.
This patch purpose is to add JTAG master core infrastructure by
defining new JTAG class and provide generic JTAG interface
to allow hardware specific drivers to connect this interface.
This will enable all JTAG drivers to use the common interface
part and will have separate for hardware implementation.
The JTAG (Joint Test Action Group) core driver provides minimal generic
JTAG interface, which can be used by hardware specific JTAG master
controllers. By providing common interface for the JTAG controllers,
user space device programing is hardware independent.
Modern SoC which in use for embedded system' equipped with
internal JTAG master interface.
This interface is used for programming and debugging system's
hardware components, like CPLD, FPGA, CPU, voltage and
industrial controllers.
Firmware for such devices can be upgraded through JTAG interface during
Runtime. The JTAG standard support for multiple devices programming,
is in case their lines are daisy-chained together.
For example, systems which equipped with host CPU, BMC SoC or/and
number of programmable devices are capable to connect a pin and
select system components dynamically for programming and debugging,
This is using by the BMC which is equipped with internal SoC master
controller.
For example:
BMC JTAG master --> pin selected to CPLDs chain for programming (filed
upgrade, production)
BMC JTAG master --> pin selected to voltage monitors for programming
(field upgrade, production)
BMC JTAG master --> pin selected to host CPU (on-site debugging
and developers debugging)
For example, we can have application in user space which using calls
to JTAG driver executes CPLD programming directly from SVF file
The JTAG standard (IEEE 1149.1) defines the next connector pins:
- TDI (Test Data In);
- TDO (Test Data Out);
- TCK (Test Clock);
- TMS (Test Mode Select);
- TRST (Test Reset) (Optional);
The SoC equipped with JTAG master controller, performs
device programming on command or vector level. For example
a file in a standard SVF (Serial Vector Format) that contains
boundary scan vectors, can be used by sending each vector
to the JTAG interface and the JTAG controller will execute
the programming.
Initial version provides the system calls set for:
- SIR (Scan Instruction Register, IEEE 1149.1 Data Register scan);
- SDR (Scan Data Register, IEEE 1149.1 Instruction Register scan);
- RUNTEST (Forces the IEEE 1149.1 bus to a run state for a specified
number of clocks.
SoC which are not equipped with JTAG master interface, can be built
on top of JTAG core driver infrastructure, by applying bit-banging of
TDI, TDO, TCK and TMS pins within the hardware specific driver.
Oleksandr Shamray (4):
drivers: jtag: Add JTAG core driver
drivers: jtag: Add Aspeed SoC 24xx and 25xx families JTAG master
driver
Documentation: jtag: Add bindings for Aspeed SoC 24xx and 25xx
families JTAG master driver
Documentation: jtag: Add ABI documentation
Documentation/ABI/testing/jtag-dev | 27 +
.../devicetree/bindings/jtag/aspeed-jtag.txt | 22 +
Documentation/ioctl/ioctl-number.txt | 2 +
MAINTAINERS | 10 +
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/jtag/Kconfig | 30 +
drivers/jtag/Makefile | 2 +
drivers/jtag/jtag-aspeed.c | 786 ++++++++++++++++++++
drivers/jtag/jtag.c | 273 +++++++
include/linux/jtag.h | 41 +
include/uapi/linux/jtag.h | 105 +++
12 files changed, 1301 insertions(+), 0 deletions(-)
create mode 100644 Documentation/ABI/testing/jtag-dev
create mode 100644 Documentation/devicetree/bindings/jtag/aspeed-jtag.txt
create mode 100644 drivers/jtag/Kconfig
create mode 100644 drivers/jtag/Makefile
create mode 100644 drivers/jtag/jtag-aspeed.c
create mode 100644 drivers/jtag/jtag.c
create mode 100644 include/linux/jtag.h
create mode 100644 include/uapi/linux/jtag.h
^ permalink raw reply
* [PATCH 3.18 19/52] MIPS: AR7: ensure the port types FCR value is used
From: Greg Kroah-Hartman @ 2018-01-29 12:56 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kroah-Hartman, stable, Jonas Gorski, Yoshihiro YUNOMAE,
Florian Fainelli, Nicolas Schichan, linux-mips, linux-serial,
Ralf Baechle, James Hogan
In-Reply-To: <20180129123628.168904217@linuxfoundation.org>
3.18-stable review patch. If anyone has any objections, please let me know.
------------------
From: Jonas Gorski <jonas.gorski@gmail.com>
commit 0a5191efe06b5103909206e4fbcff81d30283f8e upstream.
Since commit aef9a7bd9b67 ("serial/uart/8250: Add tunable RX interrupt
trigger I/F of FIFO buffers"), the port's default FCR value isn't used
in serial8250_do_set_termios anymore, but copied over once in
serial8250_config_port and then modified as needed.
Unfortunately, serial8250_config_port will never be called if the port
is shared between kernel and userspace, and the port's flag doesn't have
UPF_BOOT_AUTOCONF, which would trigger a serial8250_config_port as well.
This causes garbled output from userspace:
[ 5.220000] random: procd urandom read with 49 bits of entropy available
ers
[kee
Fix this by forcing it to be configured on boot, resulting in the
expected output:
[ 5.250000] random: procd urandom read with 50 bits of entropy available
Press the [f] key and hit [enter] to enter failsafe mode
Press the [1], [2], [3] or [4] key and hit [enter] to select the debug level
Fixes: aef9a7bd9b67 ("serial/uart/8250: Add tunable RX interrupt trigger I/F of FIFO buffers")
Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Nicolas Schichan <nschichan@freebox.fr>
Cc: linux-mips@linux-mips.org
Cc: linux-serial@vger.kernel.org
Patchwork: https://patchwork.linux-mips.org/patch/17544/
Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
Cc: James Hogan <jhogan@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
arch/mips/ar7/platform.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/arch/mips/ar7/platform.c
+++ b/arch/mips/ar7/platform.c
@@ -581,7 +581,7 @@ static int __init ar7_register_uarts(voi
uart_port.type = PORT_AR7;
uart_port.uartclk = clk_get_rate(bus_clk) / 2;
uart_port.iotype = UPIO_MEM32;
- uart_port.flags = UPF_FIXED_TYPE;
+ uart_port.flags = UPF_FIXED_TYPE | UPF_BOOT_AUTOCONF;
uart_port.regshift = 2;
uart_port.line = 0;
^ permalink raw reply
* [PATCH] [PATCH] tty/serial: atmel: add new version check for usart
From: Jonas Danielsson @ 2018-01-29 11:39 UTC (permalink / raw)
To: linux-serial
Cc: Jonas Danielsson, Richard Genoud, Greg Kroah-Hartman,
Alexandre Belloni, Jiri Slaby, linux-arm-kernel
On our at91sam9260 based board the usart0 and usart1 ports report
their versions (ATMEL_US_VERSION) as 0x10302. This version is not
included in the current checks in the driver.
Signed-off-by: Jonas Danielsson <jonas@orbital-systems.com>
---
drivers/tty/serial/atmel_serial.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index efa25611ca0c..ae9f1dcbf3fc 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1734,6 +1734,7 @@ static void atmel_get_ip_name(struct uart_port *port)
switch (version) {
case 0x302:
case 0x10213:
+ case 0x10302:
dev_dbg(port->dev, "This version is usart\n");
atmel_port->has_frac_baudrate = true;
atmel_port->has_hw_timer = true;
--
2.14.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox