* [Potential Spoof] Re: [PATCH net-next v4] net/ncsi: Add NCSI Broadcom OEM command
From: Samuel Mendoza-Jonas @ 2018-10-16 5:46 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <E82A7C2E-6DBF-437D-B571-5376F0122176@fb.com>
On Mon, 2018-10-15 at 17:38 +0000, Vijay Khemka wrote:
>
> ?On 10/15/18, 10:27 AM, "Linux-aspeed on behalf of Vijay Khemka" <linux-aspeed-bounces+vijaykhemka=fb.com at lists.ozlabs.org on behalf of vijaykhemka@fb.com> wrote:
>
>
>
> On 10/14/18, 8:51 PM, "Samuel Mendoza-Jonas" <sam@mendozajonas.com> wrote:
>
> On Mon, 2018-10-15 at 13:08 +1100, Samuel Mendoza-Jonas wrote:
> > On Fri, 2018-10-12 at 11:20 -0700, Vijay Khemka wrote:
> > > This patch adds OEM Broadcom commands and response handling. It also
> > > defines OEM Get MAC Address handler to get and configure the device.
> > >
> > > ncsi_oem_gma_handler_bcm: This handler send NCSI broadcom command for
> > > getting mac address.
> > > ncsi_rsp_handler_oem_bcm: This handles response received for all
> > > broadcom OEM commands.
> > > ncsi_rsp_handler_oem_bcm_gma: This handles get mac address response and
> > > set it to device.
> > >
> > > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
> > > ---
> > > v4: updated as per comment from Sam, I was just wondering if I can remove
> > > NCSI_OEM_CMD_GET_MAC config option and let this code be valid always and
> > > it will configure mac address if there is get mac address handler for given
> > > manufacture id.
> >
> > Hi Vijay,
> >
> > We can look at handling this a different way, but I don't think we want
> > to unconditionally set the system's MAC address based on the OEM GMA
> > command. If the user wants to set a custom MAC address, or in the case of
> > OpenBMC for example who have their MAC address saved in flash, this will
> > override that value with whatever the Network Controller has saved. In
> > particular as it is set up it will override any MAC address every time a
> > channel is configured, such as during a failover event.
> >
> > We *could* always send the GMA command if it is available and move the
> > decision whether to use the resulting address or not into the response
> > handler. That would simplify the ncsi_configure_channel() logic a bit.
> > Another idea may be to have a Netlink command to tell NCSI to ignore the
> > GMA result; then we could drop the config option and the system can
> > safely change the address if desired.
> >
> > Any thoughts? I'll also ping some of the OpenBMC people and see what
> > their expectations are.
>
> After a bit of a think and an ask around, to quote a colleague:
> > I think we'd want it handled (overall) like any other net device; the MAC
> > address in the device's ROM provides a default, and is overridden by anything
> > specified by userspace
>
> Which describes what I was thinking pretty well.
> So if we can have it such that the NCSI driver only sets the MAC address
> _once_, and then after then does not update it again, we should be able to call
> the OEM GMA command without hiding it behind a config option. So the first time
> a channel was configured we store and set the MAC address given, but then on
> later configure events we don't continue to update it. What do you think?
>
> Cheers,
> Sam
>
> I agree with you setting it only once. I gave a thought about config option and realize that
> we should allow user to configure it. If user wants to set mac address through device tree
> and not through ROM then we must not override mac set by device tree. So my proposal is
> setting of mac address in response should be hidden under config option. Getting mac address
> can still go without config option. Your thought?
>
> or simply guard following block under config and no other function declaration guard required.
> And set static variable flag in function " ncsi_oem_handler" for calling this only once.
>
> #if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
> nca.type = NCSI_PKT_CMD_OEM;
> nca.package = np->id;
> nca.channel = nc->id;
> ndp->pending_req_num = 1;
> ret = ncsi_oem_handler(&nca, nc->version.mf_id);
> #endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
>
Hi Vijay,
Either way is likely fine; although you might need to take care you don't
get an unused function warning depending on how you guard
ncsi_oem_handler(). Also a flag in the ncsi_dev_priv struct could be
easier to keep track of than having a static variable in the handler
function.
Sam
^ permalink raw reply
* [PATCH net-next v5] net/ncsi: Add NCSI Broadcom OEM command
From: Vijay Khemka @ 2018-10-16 19:13 UTC (permalink / raw)
To: linux-aspeed
This patch adds OEM Broadcom commands and response handling. It also
defines OEM Get MAC Address handler to get and configure the device.
ncsi_oem_gma_handler_bcm: This handler send NCSI broadcom command for
getting mac address.
ncsi_rsp_handler_oem_bcm: This handles response received for all
broadcom OEM commands.
ncsi_rsp_handler_oem_bcm_gma: This handles get mac address response and
set it to device.
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
net/ncsi/Kconfig | 6 ++++
net/ncsi/internal.h | 9 +++++
net/ncsi/ncsi-manage.c | 82 ++++++++++++++++++++++++++++++++++++++++++
net/ncsi/ncsi-pkt.h | 8 +++++
net/ncsi/ncsi-rsp.c | 44 +++++++++++++++++++++--
5 files changed, 147 insertions(+), 2 deletions(-)
diff --git a/net/ncsi/Kconfig b/net/ncsi/Kconfig
index 08a8a6031fd7..7f2b46108a24 100644
--- a/net/ncsi/Kconfig
+++ b/net/ncsi/Kconfig
@@ -10,3 +10,9 @@ config NET_NCSI
support. Enable this only if your system connects to a network
device via NCSI and the ethernet driver you're using supports
the protocol explicitly.
+config NCSI_OEM_CMD_GET_MAC
+ bool "Get NCSI OEM MAC Address"
+ depends on NET_NCSI
+ ---help---
+ This allows to get MAC address from NCSI firmware and set them back to
+ controller.
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 13c9b5eeb3b7..1dae77c54009 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -71,6 +71,13 @@ enum {
/* OEM Vendor Manufacture ID */
#define NCSI_OEM_MFR_MLX_ID 0x8119
#define NCSI_OEM_MFR_BCM_ID 0x113d
+/* Broadcom specific OEM Command */
+#define NCSI_OEM_BCM_CMD_GMA 0x01 /* CMD ID for Get MAC */
+/* OEM Command payload lengths*/
+#define NCSI_OEM_BCM_CMD_GMA_LEN 12
+/* Mac address offset in OEM response */
+#define BCM_MAC_ADDR_OFFSET 28
+
struct ncsi_channel_version {
u32 version; /* Supported BCD encoded NCSI version */
@@ -246,6 +253,7 @@ enum {
ncsi_dev_state_probe_dp,
ncsi_dev_state_config_sp = 0x0301,
ncsi_dev_state_config_cis,
+ ncsi_dev_state_config_oem_gma,
ncsi_dev_state_config_clear_vids,
ncsi_dev_state_config_svf,
ncsi_dev_state_config_ev,
@@ -279,6 +287,7 @@ struct ncsi_dev_priv {
#define NCSI_DEV_PROBED 1 /* Finalized NCSI topology */
#define NCSI_DEV_HWA 2 /* Enabled HW arbitration */
#define NCSI_DEV_RESHUFFLE 4
+ unsigned int gma_flag; /* OEM GMA flag */
spinlock_t lock; /* Protect the NCSI device */
#if IS_ENABLED(CONFIG_IPV6)
unsigned int inet6_addr_num; /* Number of IPv6 addresses */
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 6aa0614d2d28..bfc43b28c7a6 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -651,6 +651,72 @@ static int set_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc,
return 0;
}
+#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
+
+/* NCSI OEM Command APIs */
+static int ncsi_oem_gma_handler_bcm(struct ncsi_cmd_arg *nca)
+{
+ unsigned char data[NCSI_OEM_BCM_CMD_GMA_LEN];
+ int ret = 0;
+
+ nca->payload = NCSI_OEM_BCM_CMD_GMA_LEN;
+
+ memset(data, 0, NCSI_OEM_BCM_CMD_GMA_LEN);
+ *(unsigned int *)data = ntohl(NCSI_OEM_MFR_BCM_ID);
+ data[5] = NCSI_OEM_BCM_CMD_GMA;
+
+ nca->data = data;
+
+ ret = ncsi_xmit_cmd(nca);
+ if (ret)
+ netdev_err(nca->ndp->ndev.dev,
+ "NCSI: Failed to transmit cmd 0x%x during configure\n",
+ nca->type);
+ return ret;
+}
+
+/* OEM Command handlers initialization */
+static struct ncsi_oem_gma_handler {
+ unsigned int mfr_id;
+ int (*handler)(struct ncsi_cmd_arg *nca);
+} ncsi_oem_gma_handlers[] = {
+ { NCSI_OEM_MFR_BCM_ID, ncsi_oem_gma_handler_bcm }
+};
+
+static int ncsi_gma_handler(struct ncsi_cmd_arg *nca, unsigned int mf_id)
+{
+ struct ncsi_oem_gma_handler *nch = NULL;
+ int i;
+
+ /* This function should only be called once, return if flag set */
+ if (nca->ndp->gma_flag == 1)
+ return -1;
+
+ /* Find gma handler for given manufacturer id */
+ for (i = 0; i < ARRAY_SIZE(ncsi_oem_gma_handlers); i++) {
+ if (ncsi_oem_gma_handlers[i].mfr_id == mf_id) {
+ if (ncsi_oem_gma_handlers[i].handler)
+ nch = &ncsi_oem_gma_handlers[i];
+ break;
+ }
+ }
+
+ if (!nch) {
+ netdev_err(nca->ndp->ndev.dev,
+ "NCSI: No GMA handler available for MFR-ID (0x%x)\n",
+ mf_id);
+ return -1;
+ }
+
+ /* Set the flag for GMA command which should only be called once */
+ nca->ndp->gma_flag = 1;
+
+ /* Get Mac address from NCSI device */
+ return nch->handler(nca);
+}
+
+#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
+
static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
{
struct ncsi_dev *nd = &ndp->ndev;
@@ -701,7 +767,23 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
goto error;
}
+ nd->state = ncsi_dev_state_config_oem_gma;
+ break;
+ case ncsi_dev_state_config_oem_gma:
nd->state = ncsi_dev_state_config_clear_vids;
+ ret = -1;
+
+#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
+ nca.type = NCSI_PKT_CMD_OEM;
+ nca.package = np->id;
+ nca.channel = nc->id;
+ ndp->pending_req_num = 1;
+ ret = ncsi_gma_handler(&nca, nc->version.mf_id);
+#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
+
+ if (ret < 0)
+ schedule_work(&ndp->work);
+
break;
case ncsi_dev_state_config_clear_vids:
case ncsi_dev_state_config_svf:
diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
index 0f2087c8d42a..4d3f06be38bd 100644
--- a/net/ncsi/ncsi-pkt.h
+++ b/net/ncsi/ncsi-pkt.h
@@ -165,6 +165,14 @@ struct ncsi_rsp_oem_pkt {
unsigned char data[]; /* Payload data */
};
+/* Broadcom Response Data */
+struct ncsi_rsp_oem_bcm_pkt {
+ unsigned char ver; /* Payload Version */
+ unsigned char type; /* OEM Command type */
+ __be16 len; /* Payload Length */
+ unsigned char data[]; /* Cmd specific Data */
+};
+
/* Get Link Status */
struct ncsi_rsp_gls_pkt {
struct ncsi_rsp_pkt_hdr rsp; /* Response header */
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 85fa59afae34..77e07ba3f493 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -611,19 +611,59 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
return 0;
}
+/* Response handler for Broadcom command Get Mac Address */
+static int ncsi_rsp_handler_oem_bcm_gma(struct ncsi_request *nr)
+{
+ struct ncsi_dev_priv *ndp = nr->ndp;
+ struct net_device *ndev = ndp->ndev.dev;
+ const struct net_device_ops *ops = ndev->netdev_ops;
+ struct ncsi_rsp_oem_pkt *rsp;
+ struct sockaddr saddr;
+ int ret = 0;
+
+ /* Get the response header */
+ rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
+
+ saddr.sa_family = ndev->type;
+ ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
+ memcpy(saddr.sa_data, &rsp->data[BCM_MAC_ADDR_OFFSET], ETH_ALEN);
+ /* Increase mac address by 1 for BMC's address */
+ saddr.sa_data[ETH_ALEN - 1]++;
+ ret = ops->ndo_set_mac_address(ndev, &saddr);
+ if (ret < 0)
+ netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");
+
+ return ret;
+}
+
+/* Response handler for Broadcom card */
+static int ncsi_rsp_handler_oem_bcm(struct ncsi_request *nr)
+{
+ struct ncsi_rsp_oem_bcm_pkt *bcm;
+ struct ncsi_rsp_oem_pkt *rsp;
+
+ /* Get the response header */
+ rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
+ bcm = (struct ncsi_rsp_oem_bcm_pkt *)(rsp->data);
+
+ if (bcm->type == NCSI_OEM_BCM_CMD_GMA)
+ return ncsi_rsp_handler_oem_bcm_gma(nr);
+ return 0;
+}
+
static struct ncsi_rsp_oem_handler {
unsigned int mfr_id;
int (*handler)(struct ncsi_request *nr);
} ncsi_rsp_oem_handlers[] = {
{ NCSI_OEM_MFR_MLX_ID, NULL },
- { NCSI_OEM_MFR_BCM_ID, NULL }
+ { NCSI_OEM_MFR_BCM_ID, ncsi_rsp_handler_oem_bcm }
};
/* Response handler for OEM command */
static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
{
- struct ncsi_rsp_oem_pkt *rsp;
struct ncsi_rsp_oem_handler *nrh = NULL;
+ struct ncsi_rsp_oem_pkt *rsp;
unsigned int mfr_id, i;
/* Get the response header */
--
2.17.1
^ permalink raw reply related
* [PATCH net-next v5] net/ncsi: Add NCSI Broadcom OEM command
From: Samuel Mendoza-Jonas @ 2018-10-17 2:50 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181016191319.1909502-1-vijaykhemka@fb.com>
On Tue, 2018-10-16 at 12:13 -0700, Vijay Khemka wrote:
> This patch adds OEM Broadcom commands and response handling. It also
> defines OEM Get MAC Address handler to get and configure the device.
>
> ncsi_oem_gma_handler_bcm: This handler send NCSI broadcom command for
> getting mac address.
> ncsi_rsp_handler_oem_bcm: This handles response received for all
> broadcom OEM commands.
> ncsi_rsp_handler_oem_bcm_gma: This handles get mac address response and
> set it to device.
>
> Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
Reviewed-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
^ permalink raw reply
* [[PATCH] 0/9] *** DMA support for UART in ASPEED's AST2500 ***
From: sudheer.v @ 2018-10-17 4:10 UTC (permalink / raw)
To: linux-aspeed
DMA controller driver and UART dma client driver for aspeed's AST2500
sudheer.v (9):
DT-changes-for-DMA-UART-of-AST2500
Defconfig-changes-for-DMA-UART-of-AST2500
configuration-for-DMA-of-AST2500
Documentation-DTbindings-DMA-controller-of-AST2500
DMA-driver-for-AST2500
configuration-for-DMA-UART-of-AST2500
Documentation-DTbindings-DMA-UART-of-AST2500
DMA-UART-Driver-for-AST2500
updating MAINTAINERS for DMA and DMA-UART drivers of AST2500
.../devicetree/bindings/dma/ast-uart-sdma.txt | 23 +
.../devicetree/bindings/serial/ast-sdma-uart.txt | 34 +
MAINTAINERS | 26 +
arch/arm/boot/dts/aspeed-ast2500-evb.dts | 20 +
arch/arm/boot/dts/aspeed-g5.dtsi | 85 ++
arch/arm/configs/aspeed_g5_defconfig | 6 +-
drivers/dma/Kconfig | 9 +
drivers/dma/Makefile | 1 +
drivers/dma/aspeed_uart_sdma.c | 810 ++++++++++
drivers/tty/serial/8250/8250_aspeed_uart_dma.c | 1594 ++++++++++++++++++++
drivers/tty/serial/8250/Kconfig | 34 +
drivers/tty/serial/8250/Makefile | 1 +
12 files changed, 2641 insertions(+), 2 deletions(-)
create mode 100644 Documentation/devicetree/bindings/dma/ast-uart-sdma.txt
create mode 100644 Documentation/devicetree/bindings/serial/ast-sdma-uart.txt
create mode 100644 drivers/dma/aspeed_uart_sdma.c
create mode 100644 drivers/tty/serial/8250/8250_aspeed_uart_dma.c
--
1.9.1
^ permalink raw reply
* [[PATCH] 1/9] DT-changes-for-DMA-UART-of-AST2500
From: sudheer.v @ 2018-10-17 4:10 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1539749466-3912-1-git-send-email-open.sudheer@gmail.com>
Signed-off-by: sudheer.v <open.sudheer@gmail.com>
---
arch/arm/boot/dts/aspeed-ast2500-evb.dts | 20 ++++++++
arch/arm/boot/dts/aspeed-g5.dtsi | 85 ++++++++++++++++++++++++++++++++
2 files changed, 105 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-ast2500-evb.dts b/arch/arm/boot/dts/aspeed-ast2500-evb.dts
index 5dbb33c..f98d55b 100644
--- a/arch/arm/boot/dts/aspeed-ast2500-evb.dts
+++ b/arch/arm/boot/dts/aspeed-ast2500-evb.dts
@@ -64,6 +64,26 @@
status = "okay";
};
+&ast_uart_sdma{
+ status = "okay";
+};
+
+&dma_uart0 {
+ status = "disabled";
+};
+
+&dma_uart1 {
+ status = "disabled";
+};
+
+&dma_uart2 {
+ status = "okay";
+};
+
+&dma_uart3 {
+ status = "okay";
+};
+
&mac0 {
status = "okay";
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index d92f047..ba8edd1 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -436,6 +436,91 @@
status = "disabled";
};
+ ast_uart_sdma: uart_sdma at 1e79e000 {
+ compatible = "aspeed,ast-uart-sdma";
+ reg = <0x1e79e000 0x400>;
+ interrupts = <50>;
+ #dma-cells = <1>;
+ dma-channels = <8>;
+ status = "disabled";
+ };
+
+ dma_uart0: dma_uart0 at 1e783000{
+ compatible = "aspeed,ast-sdma-uart";
+ reg = <0x1e783000 0x1000>;
+ interrupts = <9>;
+ clocks = <&syscon ASPEED_CLK_GATE_UART1CLK>;
+ reg-shift = <2>;
+ dma-channel = <0>;
+ no-loopback-test;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_txd1_default
+ &pinctrl_rxd1_default &pinctrl_ncts1_default
+ &pinctrl_ndcd1_default &pinctrl_ndsr1_default
+ &pinctrl_ndtr1_default &pinctrl_nri1_default
+ &pinctrl_nrts1_default>;
+ dma-names = "rx", "tx";
+ dmas = <&ast_uart_sdma 1>, <&ast_uart_sdma 0>;
+ status = "disabled";
+ };
+
+ dma_uart1: dma_uart1 at 1e78d000{
+ compatible = "aspeed,ast-sdma-uart";
+ reg = <0x1e78d000 0x1000>;
+ interrupts = <32>;
+ clocks = <&syscon ASPEED_CLK_GATE_UART2CLK>;
+ reg-shift = <2>;
+ dma-channel = <1>;
+ no-loopback-test;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_txd2_default
+ &pinctrl_rxd2_default &pinctrl_ncts2_default
+ &pinctrl_ndcd2_default &pinctrl_ndsr2_default
+ &pinctrl_ndtr2_default &pinctrl_nri2_default
+ &pinctrl_nrts2_default>;
+ dma-names = "rx", "tx";
+ dmas = <&ast_uart_sdma 3>, <&ast_uart_sdma 2>;
+ status = "disabled";
+};
+
+ dma_uart2: dma_uart2 at 1e78e000{
+ compatible = "aspeed,ast-sdma-uart";
+ reg = <0x1e78e000 0x1000>;
+ interrupts = <33>;
+ clocks = <&syscon ASPEED_CLK_GATE_UART3CLK>;
+ reg-shift = <2>;
+ dma-channel = <2>;
+ no-loopback-test;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_txd3_default
+ &pinctrl_rxd3_default &pinctrl_ncts3_default
+ &pinctrl_ndcd3_default &pinctrl_ndsr3_default
+ &pinctrl_ndtr3_default &pinctrl_nri3_default
+ &pinctrl_nrts3_default>;
+ dma-names = "rx", "tx";
+ dmas = <&ast_uart_sdma 5>, <&ast_uart_sdma 4>;
+ status = "disabled";
+};
+
+ dma_uart3: dma_uart3 at 1e78f000{
+ compatible = "aspeed,ast-sdma-uart";
+ reg = <0x1e78f000 0x1000>;
+ interrupts = <34>;
+ clocks = <&syscon ASPEED_CLK_GATE_UART4CLK>;
+ reg-shift = <2>;
+ dma-channel = <3>;
+ no-loopback-test;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_txd4_default
+ &pinctrl_rxd4_default &pinctrl_ncts4_default
+ &pinctrl_ndcd4_default &pinctrl_ndsr4_default
+ &pinctrl_ndtr4_default &pinctrl_nri4_default
+ &pinctrl_nrts4_default>;
+ dma-names = "rx", "tx";
+ dmas = <&ast_uart_sdma 7>, <&ast_uart_sdma 6>;
+ status = "disabled";
+};
+
i2c: i2c at 1e78a000 {
compatible = "simple-bus";
#address-cells = <1>;
--
1.9.1
^ permalink raw reply related
* [[PATCH] 2/9] Defconfig-changes-for-DMA-UART-of-AST2500
From: sudheer.v @ 2018-10-17 4:10 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1539749466-3912-1-git-send-email-open.sudheer@gmail.com>
Signed-off-by: sudheer.v <open.sudheer@gmail.com>
---
arch/arm/configs/aspeed_g5_defconfig | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/arm/configs/aspeed_g5_defconfig b/arch/arm/configs/aspeed_g5_defconfig
index b7f8fa1..25813b5 100644
--- a/arch/arm/configs/aspeed_g5_defconfig
+++ b/arch/arm/configs/aspeed_g5_defconfig
@@ -132,8 +132,8 @@ CONFIG_KEYBOARD_GPIO_POLLED=y
CONFIG_SERIAL_8250=y
# CONFIG_SERIAL_8250_DEPRECATED_OPTIONS is not set
CONFIG_SERIAL_8250_CONSOLE=y
-CONFIG_SERIAL_8250_NR_UARTS=6
-CONFIG_SERIAL_8250_RUNTIME_UARTS=6
+CONFIG_SERIAL_8250_NR_UARTS=1
+CONFIG_SERIAL_8250_RUNTIME_UARTS=1
CONFIG_SERIAL_8250_EXTENDED=y
CONFIG_SERIAL_8250_ASPEED_VUART=y
CONFIG_SERIAL_8250_SHARE_IRQ=y
@@ -211,6 +211,8 @@ CONFIG_RTC_CLASS=y
CONFIG_RTC_DRV_DS1307=y
CONFIG_RTC_DRV_PCF8523=y
CONFIG_RTC_DRV_RV8803=y
+CONFIG_DMADEVICES=y
+CONFIG_AST_UART_SDMA=y
# CONFIG_VIRTIO_MENU is not set
# CONFIG_IOMMU_SUPPORT is not set
CONFIG_IIO=y
--
1.9.1
^ permalink raw reply related
* [[PATCH] 4/9] Documentation-DTbindings-DMA-controller-of-AST2500
From: sudheer.v @ 2018-10-17 4:11 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1539749466-3912-1-git-send-email-open.sudheer@gmail.com>
Signed-off-by: sudheer.v <open.sudheer@gmail.com>
---
.../devicetree/bindings/dma/ast-uart-sdma.txt | 23 ++++++++++++++++++++++
1 file changed, 23 insertions(+)
create mode 100644 Documentation/devicetree/bindings/dma/ast-uart-sdma.txt
diff --git a/Documentation/devicetree/bindings/dma/ast-uart-sdma.txt b/Documentation/devicetree/bindings/dma/ast-uart-sdma.txt
new file mode 100644
index 0000000..e770df2
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/ast-uart-sdma.txt
@@ -0,0 +1,23 @@
+* Aspeed Direct Memory Access (DMA) Controller for AST25XX
+
+
+* DMA controller
+
+Required properties:
+- compatible : Should be "aspeed,ast-uart-sdma"
+- reg : Should contain SDMA registers location and length
+- interrupts : Should contain SDMA interrupt
+- dma-channels: number of DMA channels in DMA controller
+
+Optional properties:
+
+Example
+ ast_uart_sdma: uart_sdma at 1e79e000 {
+ compatible = "aspeed,ast-uart-sdma";
+ reg = <0x1e79e000 0x400>;
+ interrupts = <50>;
+ #dma-cells = <1>;
+ dma-channels = <8>;
+ status = "disabled";
+ };
+
--
1.9.1
^ permalink raw reply related
* [[PATCH] 8/9] DMA-UART-Driver-for-AST2500
From: sudheer.v @ 2018-10-17 4:11 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1539749466-3912-1-git-send-email-open.sudheer@gmail.com>
Signed-off-by: sudheer.v <open.sudheer@gmail.com>
---
drivers/tty/serial/8250/8250_aspeed_uart_dma.c | 1594 ++++++++++++++++++++++++
1 file changed, 1594 insertions(+)
create mode 100644 drivers/tty/serial/8250/8250_aspeed_uart_dma.c
diff --git a/drivers/tty/serial/8250/8250_aspeed_uart_dma.c b/drivers/tty/serial/8250/8250_aspeed_uart_dma.c
new file mode 100644
index 0000000..e1019a8
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_aspeed_uart_dma.c
@@ -0,0 +1,1594 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * drivers/tty/serial/8250/8250_aspeed_uart_dma.c
+ * 1. 2018/07/01 Shivah Shankar created
+ * 2. 2018/08/25 sudheer.veliseti<open.sudheer@gmail.com> modified
+ *
+ */
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/ioport.h>
+#include <linux/init.h>
+#include <linux/console.h>
+#include<linux/slab.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
+#include <linux/serial_reg.h>
+#include <linux/serial_core.h>
+#include <linux/serial.h>
+#include <linux/serial_8250.h>
+#include <linux/nmi.h>
+#include <linux/mutex.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <asm/irq.h>
+
+#include "8250.h"
+#include <linux/dma-mapping.h>
+#define SDMA_RX_BUFF_SIZE 0x10000 //65536
+#define DMA_BUFF_SIZE 0x1000 //4096
+
+
+
+
+#undef UART_XMIT_SIZE
+#define UART_XMIT_SIZE 0x1000
+#define UART_RX_SIZE 0x10000
+
+#ifdef UART_DMA_DEBUG
+ #define UART_DBG(fmt, args...) pr_debug("%s() " fmt, __func__, ## args)
+#else
+ #define UART_DBG(fmt, args...)
+#endif
+
+#ifdef CONFIG_UART_TX_DMA_DEBUG
+ #define UART_TX_DBG(fmt, args...) pr_debug("%s()"fmt, __func__, ## args)
+#else
+ #define UART_TX_DBG(fmt, args...)
+#endif
+
+/*
+ * Configuration:
+ * share_irqs - whether we pass IRQF_SHARED to request_irq(). This option
+ * is unsafe when used on edge-triggered interrupts.
+ */
+static unsigned int share_irqs = SERIAL8250_SHARE_IRQS;
+
+static unsigned int nr_uarts = CONFIG_AST_RUNTIME_DMA_UARTS;
+
+/*
+ * Debugging.
+ */
+#if 0
+#define DEBUG_AUTOCONF(fmt...) UART_DBG(fmt)
+#else
+#define DEBUG_AUTOCONF(fmt...) do { } while (0)
+#endif
+
+#if 0
+#define DEBUG_INTR(fmt...) UART_DBG(fmt)
+#else
+#define DEBUG_INTR(fmt...) do { } while (0)
+#endif
+
+#define PASS_LIMIT 256
+
+#include <asm/serial.h>
+
+
+#define UART_DMA_NR CONFIG_AST_NR_DMA_UARTS
+
+struct ast_uart_port {
+ struct uart_port port;
+ struct platform_device *pdev;
+ unsigned short capabilities; /* port capabilities */
+ unsigned short bugs; /* port bugs */
+ unsigned int tx_loadsz; /* transmit fifo load size */
+ unsigned char acr;
+ unsigned char ier;
+ unsigned char lcr;
+ unsigned char mcr;
+ unsigned char mcr_mask; /* mask of user bits */
+ unsigned char mcr_force; /* mask of forced bits */
+ unsigned int channel_no;
+ struct scatterlist rx_sgl;
+ struct dma_chan *rx_dma_chan;
+ struct circ_buf rx_dma_buf;
+ dma_addr_t dma_rx_addr;
+ u8 rx_in_progress;
+ struct dma_async_tx_descriptor *rx_dma_desc;
+ dma_cookie_t rx_cookie;
+ unsigned int rx_bytes_requested;
+ unsigned int rx_bytes_transferred;
+ struct tasklet_struct rx_tasklet;
+ struct scatterlist tx_sgl;
+ struct dma_chan *tx_dma_chan;
+ struct circ_buf tx_dma_buf;
+ dma_addr_t dma_tx_addr;
+ u8 tx_in_progress;
+ struct dma_async_tx_descriptor *tx_dma_desc;
+ dma_cookie_t tx_cookie;
+ unsigned int tx_bytes_requested;
+ unsigned int tx_bytes_transferred;
+ struct tasklet_struct tx_tasklet;
+ spinlock_t lock;
+ int tx_done;
+ int tx_count;
+ /*
+ * Some bits in registers are cleared on a read, so they must
+ * be saved whenever the register is read but the bits will not
+ * be immediately processed.
+ */
+#define LSR_SAVE_FLAGS UART_LSR_BRK_ERROR_BITS
+ unsigned char lsr_saved_flags;
+#define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
+ unsigned char msr_saved_flags;
+
+ /*
+ * We provide a per-port pm hook.
+ */
+ void (*pm)(struct uart_port *port,
+ unsigned int state, unsigned int old);
+};
+
+static struct ast_uart_port ast_uart_ports[UART_DMA_NR];
+
+static int ast_dma_channel_setup(struct ast_uart_port *up);
+static inline struct ast_uart_port *
+to_ast_dma_uart_port(struct uart_port *uart)
+{
+ return container_of(uart, struct ast_uart_port, port);
+}
+
+struct irq_info {
+ spinlock_t lock;
+ struct ast_uart_port *up;
+};
+
+static void ast_dma_channel_teardown(struct ast_uart_port *s);
+static struct irq_info ast_uart_irq[1];
+static DEFINE_MUTEX(ast_uart_mutex);
+
+/*
+ * Here we define the default xmit fifo size used for each type of UART.
+ */
+static const struct serial8250_config uart_config[] = {
+ [PORT_UNKNOWN] = {
+ .name = "unknown",
+ .fifo_size = 1,
+ .tx_loadsz = 1,
+ },
+ [PORT_8250] = {
+ .name = "8250",
+ .fifo_size = 1,
+ .tx_loadsz = 1,
+ },
+ [PORT_16450] = {
+ .name = "16450",
+ .fifo_size = 1,
+ .tx_loadsz = 1,
+ },
+ [PORT_16550] = {
+ .name = "16550",
+ .fifo_size = 1,
+ .tx_loadsz = 1,
+ },
+ [PORT_16550A] = {
+ .name = "16550A",
+ .fifo_size = 16,
+ .tx_loadsz = 16,
+ .fcr = UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10
+ | UART_FCR_DMA_SELECT,
+ .flags = UART_CAP_FIFO,
+ },
+};
+
+/* sane hardware needs no mapping */
+#define map_8250_in_reg(up, offset) (offset)
+#define map_8250_out_reg(up, offset) (offset)
+
+static void ast_uart_unregister_port(int line);
+static int ast_uart_register_port(struct uart_port *port,
+ unsigned int channel_no);
+
+static unsigned int ast_serial_in(struct ast_uart_port *up, int offset)
+{
+ offset = map_8250_in_reg(up, offset) << up->port.regshift;
+
+ return readb(up->port.membase + offset);
+}
+
+static void
+ast_serial_out(struct ast_uart_port *up, int offset, int value)
+{
+ /* Save the offset before it's remapped */
+ offset = map_8250_out_reg(up, offset) << up->port.regshift;
+
+ writeb(value, up->port.membase + offset);
+}
+
+
+/*
+ * We used to support using pause I/O for certain machines. We
+ * haven't supported this for a while, but just in case it's badly
+ * needed for certain old 386 machines, I've left these #define's
+ * in....
+ */
+#define serial_inp(up, offset) ast_serial_in(up, offset)
+#define serial_outp(up, offset, value) ast_serial_out(up, offset, value)
+
+/* Uart divisor latch read */
+static inline int _serial_dl_read(struct ast_uart_port *up)
+{
+ return serial_inp(up, UART_DLL) | serial_inp(up, UART_DLM) << 8;
+}
+
+/* Uart divisor latch write */
+static inline void _serial_dl_write(struct ast_uart_port *up, int value)
+{
+ serial_outp(up, UART_DLL, value & 0xff);
+ serial_outp(up, UART_DLM, value >> 8 & 0xff);
+}
+
+#define serial_dl_read(up) _serial_dl_read(up)
+#define serial_dl_write(up, value) _serial_dl_write(up, value)
+
+static void ast_uart_tx_dma_complete(void *args);
+
+static int ast_uart_start_tx_dma(struct ast_uart_port *up,
+ unsigned long count)
+{
+ struct circ_buf *xmit = &up->port.state->xmit;
+ dma_addr_t tx_phys_addr;
+
+ UART_DBG("Entered %s count is %d\n", __func__, count);
+ UART_DBG("up->tx_cookie = %d\n", up->tx_cookie);
+ if (up->tx_cookie == 0) {
+ dma_sync_single_for_device(up->port.dev, up->dma_tx_addr,
+ UART_XMIT_SIZE, DMA_TO_DEVICE);
+ tx_phys_addr = up->dma_tx_addr + xmit->tail;
+ UART_DBG("Transmit address is %x actual is %x\n", tx_phys_addr,
+ up->dma_tx_addr);
+ up->tx_dma_desc = dmaengine_prep_slave_single(up->tx_dma_chan,
+ tx_phys_addr, count, DMA_MEM_TO_DEV, DMA_PREP_INTERRUPT);
+ if (!up->tx_dma_desc) {
+ dev_err(up->port.dev, "Not able to get desc for Tx\n");
+ return -EIO;
+ }
+
+ up->tx_dma_desc->callback = ast_uart_tx_dma_complete;
+ up->tx_dma_desc->callback_param = up;
+ up->tx_in_progress = 1;
+ up->tx_bytes_requested = count;
+ up->tx_bytes_transferred = 0;
+ up->tx_cookie = dmaengine_submit(up->tx_dma_desc);
+ }
+ dma_async_issue_pending(up->tx_dma_chan);
+ return 0;
+}
+static void ast_uart_start_next_tx(struct ast_uart_port *up)
+{
+ unsigned long count;
+ struct circ_buf *xmit = &up->port.state->xmit;
+
+ count = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);
+ if (count)
+ ast_uart_start_tx_dma(up, count);
+}
+static void ast_uart_tx_sdma_tasklet_func(unsigned long data)
+{
+ struct ast_uart_port *up = ((struct ast_uart_port *)data);
+ struct circ_buf *xmit = &up->port.state->xmit;
+ unsigned long flags = 0;
+
+ UART_DBG("In %s bytes to send is %d\n", __func__, CIRC_CNT(xmit->head,
+ spin_lock_irqsave(&up->port.lock, flags);
+ xmit->tail, UART_XMIT_SIZE));
+ if (!uart_circ_empty(xmit) && !up->tx_in_progress) {
+ UART_DBG("Calling ast_uart_start_next_tx\n");
+ ast_uart_start_next_tx(up);
+ }
+ spin_unlock_irqrestore(&up->port.lock, flags);
+}
+
+static void ast_uart_tx_dma_complete(void *args)
+{
+ struct ast_uart_port *up = args;
+ struct circ_buf *xmit = &up->port.state->xmit;
+ struct dma_tx_state state;
+ unsigned long flags;
+ unsigned int count;
+ enum dma_status status;
+
+ status = dmaengine_tx_status(up->tx_dma_chan, up->tx_cookie, &state);
+ UART_DBG("%s:state.residue=%d\n", __func__, state.residue);
+ UART_DBG("up->tx_bytes_requested=%d up->tx_bytes_transferred=%d\n",
+ up->tx_bytes_requested, up->tx_bytes_transferred);
+ if (status == DMA_COMPLETE) {
+ up->tx_cookie = 0;
+ count = up->tx_bytes_requested - up->tx_bytes_transferred;
+ UART_DBG("DMA_COMPLETE:bytes till end of Buffer=%d\n", count);
+ } else{
+ count = up->tx_bytes_requested - state.residue;
+ up->tx_bytes_transferred += count;
+ UART_DBG("DMA_not_COMPLETE: count=%d\n", count);
+ }
+ UART_DBG("up->tx_bytes_requested=%d up->tx_bytes_transferred=%d\n",
+ up->tx_bytes_requested, up->tx_bytes_transferred);
+ UART_DBG("xmit->head=%d and xmit->tail=%d\n", xmit->head, xmit->tail);
+ async_tx_ack(up->tx_dma_desc);
+ spin_lock_irqsave(&up->port.lock, flags);
+ xmit->tail = (xmit->tail + count) & (UART_XMIT_SIZE - 1);
+ up->tx_in_progress = 0;
+ UART_DBG("updated xmit->head=%d and xmit->tail=%d\n",
+ xmit->head, xmit->tail);
+ if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+ uart_write_wakeup(&up->port);
+ ast_uart_start_next_tx(up);
+ spin_unlock_irqrestore(&up->port.lock, flags);
+}
+
+static void ast_uart_rx_sdma_tasklet_func(unsigned long data);
+static void ast_uart_rx_dma_complete(void *args)
+{
+ struct ast_uart_port *up = args;
+ struct dma_tx_state state;
+ struct circ_buf *rx_ring = &up->rx_dma_buf;
+ unsigned int count;
+ unsigned int temp = 0;
+ enum dma_status status;
+
+ UART_DBG("line [%d],head = %d, len : %d\n",
+ up->port.line, up->rx_dma_buf.head, count);
+ status = dmaengine_tx_status(up->rx_dma_chan, up->rx_cookie, &state);
+ UART_DBG("Freespace in buffer=%d\n", state.residue);
+ UART_DBG("up->rx_bytes_requested=%d up->rx_bytes_transferred=%d\n",
+ up->rx_bytes_requested, up->rx_bytes_transferred);
+ if (status == DMA_COMPLETE) {
+ up->rx_cookie = 0;
+ count = up->rx_bytes_requested - up->rx_bytes_transferred;
+ up->rx_in_progress = 0;
+ UART_DBG("DMA_COMPLETE:bytes till end of Buffer=%d\n", count);
+ } else{
+ temp = up->rx_bytes_requested - state.residue;
+ count = temp - up->rx_bytes_transferred;
+ up->rx_bytes_transferred = temp;
+ UART_DBG("DMA_not_COMPLETE:fill index =%d\n", temp, count);
+ UART_DBG("bytes to be rxed in current lap=%d\n", count);
+ UART_DBG("rx_bytes_transfred=%d\n", up->rx_bytes_transferred);
+ }
+
+ UART_DBG("rx_ring->head=%d rx_ring->tail=%d\n",
+ rx_ring->head, rx_ring->tail);
+ rx_ring->head = (rx_ring->head + count) & (UART_RX_SIZE - 1);
+ UART_DBG("updated rx_ring->head=%d rx_ring->tail=%d\n",
+ rx_ring->head, rx_ring->tail);
+ ast_uart_rx_sdma_tasklet_func((unsigned long)up);
+}
+
+static int ast_uart_start_rx_dma(struct ast_uart_port *up,
+ unsigned long count)
+{
+ struct circ_buf *rx_ring = &up->rx_dma_buf;
+ dma_addr_t rx_phys_addr;
+
+ UART_DBG("%s:up->rx_dma_chan= %d\n", __func__, up->rx_dma_chan);
+ UART_DBG("up->rx_cookie = %d\n", up->rx_cookie);
+ if (up->rx_cookie == 0) {
+ dma_sync_single_for_device(up->port.dev, up->dma_rx_addr,
+ UART_RX_SIZE, DMA_FROM_DEVICE);
+ rx_phys_addr = up->dma_rx_addr + rx_ring->tail;
+ up->rx_dma_desc = dmaengine_prep_slave_single(up->rx_dma_chan,
+ rx_phys_addr, UART_RX_SIZE, DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT);
+ if (!up->rx_dma_desc) {
+ dev_err(up->port.dev, "Not able to get desc for Rx\n");
+ return -EIO;
+ }
+ up->rx_dma_desc->callback = ast_uart_rx_dma_complete;
+ up->rx_dma_desc->callback_param = up;
+ up->rx_in_progress = 1;
+ up->rx_bytes_requested = UART_RX_SIZE;
+ up->rx_bytes_transferred = 0;
+ up->rx_cookie = dmaengine_submit(up->rx_dma_desc);
+ }
+ dma_async_issue_pending(up->rx_dma_chan);
+ return 0;
+}
+
+static void ast_uart_rx_sdma_tasklet_func(unsigned long data)
+{
+ struct ast_uart_port *up = ((struct ast_uart_port *)data);
+ struct tty_port *port = &up->port.state->port;
+ struct circ_buf *rx_ring = &up->rx_dma_buf;
+ unsigned long flags;
+ int count;
+ int copy = 0;
+
+ UART_DBG("line [%d], rx_ring->head = %d, rx_ring->tail = %d\n",
+ up->port.line, rx_ring->head, rx_ring->tail);
+ spin_lock_irqsave(&up->port.lock, flags);
+ if (rx_ring->head > rx_ring->tail) {
+ count = rx_ring->head - rx_ring->tail;
+ UART_DBG("^^^^ count=%d rx_ring->head=%d rx_ring->tail=%d\n",
+ count, rx_ring->head, rx_ring->tail);
+ copy = tty_insert_flip_string(port,
+ rx_ring->buf + rx_ring->tail, count);
+ } else if (rx_ring->head < rx_ring->tail) {
+ count = SDMA_RX_BUFF_SIZE - rx_ring->tail;
+ UART_DBG("rollovr:count=%d rx_ring->head=%d rx_ring->tail=%d\n",
+ count, rx_ring->head, rx_ring->tail);
+ copy = tty_insert_flip_string(port,
+ rx_ring->buf + rx_ring->tail, count);
+ } else {
+ count = 0;
+ }
+ if (copy != count)
+ UART_DBG("!!!!!!!! ERROR 111\n");
+ if (count) {
+ UART_DBG("count = %d\n", count);
+ rx_ring->tail += count;
+ rx_ring->tail &= (SDMA_RX_BUFF_SIZE - 1);
+ up->port.icount.rx += count;
+ tty_flip_buffer_push(port);
+ spin_unlock_irqrestore(&up->port.lock, flags);
+ ast_uart_start_rx_dma(up, count);
+ spin_lock_irqsave(&up->port.lock, flags);
+ }
+ spin_unlock_irqrestore(&up->port.lock, flags);
+}
+
+/*
+ * FIFO support.
+ */
+static inline void serial8250_clear_fifos(struct ast_uart_port *p)
+{
+ serial_outp(p, UART_FCR, UART_FCR_ENABLE_FIFO);
+ serial_outp(p, UART_FCR, UART_FCR_ENABLE_FIFO |
+ UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT);
+ serial_outp(p, UART_FCR, 0);
+}
+
+/*
+ * This routine is called by rs_init() to initialize a specific serial
+ * port.
+ */
+static void autoconfig(struct ast_uart_port *up)
+{
+ unsigned long flags;
+
+ UART_DBG("line [%d]\n", up->port.line);
+ if (!up->port.iobase && !up->port.mapbase && !up->port.membase)
+ return;
+
+ DEBUG_AUTOCONF("ttyDMA%d: autoconf (0x%04x, 0x%p): ",
+ up->port.line, up->port.iobase, up->port.membase);
+
+ spin_lock_irqsave(&up->port.lock, flags);
+
+ up->capabilities = 0;
+ up->bugs = 0;
+
+ up->port.type = PORT_16550A;
+ up->capabilities |= UART_CAP_FIFO;
+
+ up->port.fifosize = uart_config[up->port.type].fifo_size;
+ up->capabilities = uart_config[up->port.type].flags;
+ up->tx_loadsz = uart_config[up->port.type].tx_loadsz;
+
+ if (up->port.type == PORT_UNKNOWN)
+ goto out;
+
+ /*
+ * Reset the UART.
+ */
+ serial8250_clear_fifos(up);
+ ast_serial_in(up, UART_RX);
+ serial_outp(up, UART_IER, 0);
+
+ out:
+ spin_unlock_irqrestore(&up->port.lock, flags);
+ DEBUG_AUTOCONF("type=%s\n", uart_config[up->port.type].name);
+}
+
+
+static inline void __stop_tx(struct ast_uart_port *p)
+{
+ if (p->ier & UART_IER_THRI) {
+ p->ier &= ~UART_IER_THRI;
+ ast_serial_out(p, UART_IER, p->ier);
+ }
+}
+
+static void serial8250_stop_tx(struct uart_port *port)
+{
+ struct ast_uart_port *up = to_ast_dma_uart_port(port);
+ struct circ_buf *xmit = &up->port.state->xmit;
+ struct dma_tx_state state;
+ unsigned int count;
+
+ __stop_tx(up);
+ if (!up->tx_in_progress)
+ return;
+ dmaengine_terminate_all(up->tx_dma_chan);
+ dmaengine_tx_status(up->tx_dma_chan, up->tx_cookie, &state);
+ count = up->tx_bytes_requested - state.residue;
+ async_tx_ack(up->tx_dma_desc);
+ xmit->tail = (xmit->tail + count) & (UART_XMIT_SIZE - 1);
+ up->tx_in_progress = 0;
+}
+
+static void transmit_chars(struct ast_uart_port *up);
+
+static void serial8250_start_tx(struct uart_port *port)
+{
+ struct ast_uart_port *up = to_ast_dma_uart_port(port);
+ struct circ_buf *xmit = &up->port.state->xmit;
+
+ UART_DBG("\n%s:line %d", __func__, port->line);
+ UART_TX_DBG("line [%d]\n", port->line);
+ if (!uart_circ_empty(xmit) && !up->tx_in_progress) {
+ UART_DBG("Calling ast_uart_start_next_tx\n");
+ ast_uart_start_next_tx(up);
+ }
+}
+
+static void serial8250_stop_rx(struct uart_port *port)
+{
+ struct ast_uart_port *up = to_ast_dma_uart_port(port);
+ struct dma_tx_state state;
+
+ up->ier &= ~UART_IER_RLSI;
+ up->port.read_status_mask &= ~UART_LSR_DR;
+ ast_serial_out(up, UART_IER, up->ier);
+ if (!up->rx_in_progress)
+ return;
+ dmaengine_terminate_all(up->rx_dma_chan);
+ dmaengine_tx_status(up->rx_dma_chan, up->rx_cookie, &state);
+ up->rx_in_progress = 0;
+ up->rx_bytes_transferred = 0;
+}
+
+static void serial8250_enable_ms(struct uart_port *port)
+{
+ struct ast_uart_port *up = to_ast_dma_uart_port(port);
+
+ UART_DBG("line [%d]\n", port->line);
+ up->ier |= UART_IER_MSI;
+ ast_serial_out(up, UART_IER, up->ier);
+}
+
+static void transmit_chars(struct ast_uart_port *up)
+{
+ struct circ_buf *xmit = &up->port.state->xmit;
+ int count;
+
+ if (up->port.x_char) {
+ serial_outp(up, UART_TX, up->port.x_char);
+ up->port.icount.tx++;
+ up->port.x_char = 0;
+ return;
+ }
+ if (uart_tx_stopped(&up->port)) {
+ serial8250_stop_tx(&up->port);
+ return;
+ }
+ if (uart_circ_empty(xmit)) {
+ __stop_tx(up);
+ return;
+ }
+
+ count = up->tx_loadsz;
+ do {
+ ast_serial_out(up, UART_TX, xmit->buf[xmit->tail]);
+ xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
+ up->port.icount.tx++;
+ if (uart_circ_empty(xmit))
+ break;
+ } while (--count > 0);
+
+ if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+ uart_write_wakeup(&up->port);
+
+ if (uart_circ_empty(xmit))
+ __stop_tx(up);
+}
+
+static unsigned int check_modem_status(struct ast_uart_port *up)
+{
+ unsigned int status = ast_serial_in(up, UART_MSR);
+
+ UART_DBG("line [%d]\n", up->port.line);
+ status |= up->msr_saved_flags;
+ up->msr_saved_flags = 0;
+ if (status & UART_MSR_ANY_DELTA && up->ier & UART_IER_MSI &&
+ up->port.state != NULL) {
+ if (status & UART_MSR_TERI)
+ up->port.icount.rng++;
+ if (status & UART_MSR_DDSR)
+ up->port.icount.dsr++;
+ if (status & UART_MSR_DDCD)
+ uart_handle_dcd_change(&up->port,
+ status & UART_MSR_DCD);
+ if (status & UART_MSR_DCTS)
+ uart_handle_cts_change(&up->port,
+ status & UART_MSR_CTS);
+ wake_up_interruptible(&up->port.state->port.delta_msr_wait);
+ }
+ return status;
+}
+
+/*
+ * This handles the interrupt from one port.
+ */
+static inline void
+serial8250_handle_port(struct ast_uart_port *up)
+{
+ unsigned int status;
+ unsigned long flags;
+
+ spin_lock_irqsave(&up->port.lock, flags);
+
+ status = serial_inp(up, UART_LSR);
+
+ DEBUG_INTR("status = %x...", status);
+
+ check_modem_status(up);
+ if (status & UART_LSR_THRE)
+ transmit_chars(up);
+
+ spin_unlock_irqrestore(&up->port.lock, flags);
+}
+
+/*
+ * This is the serial driver's interrupt routine.
+ */
+static irqreturn_t ast_uart_interrupt(int irq, void *dev_id)
+{
+ struct irq_info *i = dev_id;
+ int pass_counter = 0, handled = 0, end = 0;
+
+ DEBUG_INTR("(%d) ", irq);
+ spin_lock(&i->lock);
+ do {
+ struct ast_uart_port *up;
+ unsigned int iir;
+
+ up = (struct ast_uart_port *)(i->up);
+ iir = ast_serial_in(up, UART_IIR);
+ if (!(iir & UART_IIR_NO_INT)) {
+ serial8250_handle_port(up);
+ handled = 1;
+ } else
+ end = 1;
+
+ if (pass_counter++ > PASS_LIMIT) {
+ /* If we hit this, we're dead. */
+ UART_DBG(KERN_ERR
+ "ast-uart-dma:too much work for irqi%d", irq);
+ break;
+ }
+ } while (end);
+
+ spin_unlock(&i->lock);
+
+ DEBUG_INTR("end.\n");
+
+ return IRQ_RETVAL(handled);
+}
+
+static unsigned int serial8250_tx_empty(struct uart_port *port)
+{
+ struct ast_uart_port *up = to_ast_dma_uart_port(port);
+ unsigned long flags;
+ unsigned int lsr;
+
+ UART_TX_DBG("line [%d]\n", up->port.line);
+
+ spin_lock_irqsave(&up->port.lock, flags);
+ lsr = ast_serial_in(up, UART_LSR);
+ up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
+ spin_unlock_irqrestore(&up->port.lock, flags);
+
+ return lsr & UART_LSR_TEMT ? TIOCSER_TEMT : 0;
+}
+
+static unsigned int serial8250_get_mctrl(struct uart_port *port)
+{
+ struct ast_uart_port *up = to_ast_dma_uart_port(port);
+ unsigned int status;
+ unsigned int ret;
+
+ status = check_modem_status(up);
+
+ ret = 0;
+ if (status & UART_MSR_DCD)
+ ret |= TIOCM_CAR;
+ if (status & UART_MSR_RI)
+ ret |= TIOCM_RNG;
+ if (status & UART_MSR_DSR)
+ ret |= TIOCM_DSR;
+ if (status & UART_MSR_CTS)
+ ret |= TIOCM_CTS;
+ return ret;
+}
+
+static void serial8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
+{
+ struct ast_uart_port *up = to_ast_dma_uart_port(port);
+ unsigned char mcr = 0;
+ //UART_DBG("serial8250_set_mctrl %x\n",mctrl);
+ //TODO .... Issue for fix ......
+ mctrl = 0;
+
+ if (mctrl & TIOCM_RTS)
+ mcr |= UART_MCR_RTS;
+ if (mctrl & TIOCM_DTR)
+ mcr |= UART_MCR_DTR;
+ if (mctrl & TIOCM_OUT1)
+ mcr |= UART_MCR_OUT1;
+ if (mctrl & TIOCM_OUT2)
+ mcr |= UART_MCR_OUT2;
+ if (mctrl & TIOCM_LOOP)
+ mcr |= UART_MCR_LOOP;
+
+ mcr = (mcr & up->mcr_mask) | up->mcr_force | up->mcr;
+
+ ast_serial_out(up, UART_MCR, mcr);
+}
+
+static void serial8250_break_ctl(struct uart_port *port, int break_state)
+{
+ struct ast_uart_port *up = to_ast_dma_uart_port(port);
+ unsigned long flags;
+
+ spin_lock_irqsave(&up->port.lock, flags);
+ if (break_state == -1)
+ up->lcr |= UART_LCR_SBC;
+ else
+ up->lcr &= ~UART_LCR_SBC;
+ ast_serial_out(up, UART_LCR, up->lcr);
+ spin_unlock_irqrestore(&up->port.lock, flags);
+}
+
+static int serial8250_startup(struct uart_port *port)
+{
+ struct ast_uart_port *up = to_ast_dma_uart_port(port);
+ //TX DMA
+ struct circ_buf *xmit = &up->port.state->xmit;
+ unsigned long flags;
+ unsigned char lsr, iir;
+ int retval;
+ struct dma_slave_config dma_sconfig;
+ int irq_flags = up->port.flags & UPF_SHARE_IRQ ? IRQF_SHARED : 0;
+
+ up->capabilities = uart_config[up->port.type].flags;
+ up->mcr = 0;
+ /*
+ * Clear the FIFO buffers and disable them.
+ * (they will be reenabled in set_termios())
+ */
+ serial8250_clear_fifos(up);
+ UART_DBG("1: line [%d]\n", port->line);
+
+ /*
+ * Clear the interrupt registers.
+ */
+ (void) serial_inp(up, UART_LSR);
+ (void) serial_inp(up, UART_RX);
+ (void) serial_inp(up, UART_IIR);
+ (void) serial_inp(up, UART_MSR);
+
+ ast_uart_irq[0].up = up;
+ retval = request_irq(up->port.irq, ast_uart_interrupt,
+ irq_flags, "ast-uart-dma", ast_uart_irq);
+ if (retval)
+ return retval;
+
+ /*
+ * Now, initialize the UART
+ */
+ serial_outp(up, UART_LCR, UART_LCR_WLEN8);
+
+ spin_lock_irqsave(&up->port.lock, flags);
+ up->port.mctrl |= TIOCM_OUT2;
+
+ serial8250_set_mctrl(&up->port, up->port.mctrl);
+
+ /*
+ * Do a quick test to see if we receive an
+ * interrupt when we enable the TX irq.
+ */
+ serial_outp(up, UART_IER, UART_IER_THRI);
+ lsr = ast_serial_in(up, UART_LSR);
+ iir = ast_serial_in(up, UART_IIR);
+ serial_outp(up, UART_IER, 0);
+
+ if (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT) {
+ if (!(up->bugs & UART_BUG_TXEN)) {
+ up->bugs |= UART_BUG_TXEN;
+ UART_DBG("ttyDMA%d - enabling bad tx status\n",
+ port->line);
+ }
+ } else {
+ up->bugs &= ~UART_BUG_TXEN;
+ }
+
+ spin_unlock_irqrestore(&up->port.lock, flags);
+
+ /*
+ * Clear the interrupt registers again for luck, and clear the
+ * saved flags to avoid getting false values from polling
+ * routines or the previous session.
+ */
+ serial_inp(up, UART_LSR);
+ serial_inp(up, UART_RX);
+ serial_inp(up, UART_IIR);
+ serial_inp(up, UART_MSR);
+ up->lsr_saved_flags = 0;
+ up->msr_saved_flags = 0;
+
+ //RX DMA
+ up->rx_dma_buf.head = 0;
+ up->rx_dma_buf.tail = 0;
+ up->port.icount.rx = 0;
+ ast_dma_channel_setup(up);
+ up->rx_dma_buf.buf = dma_alloc_coherent(port->dev, UART_RX_SIZE,
+ &up->dma_rx_addr, GFP_KERNEL);
+ if (!up->rx_dma_buf.buf)
+ ast_dma_channel_teardown(up);
+#if 1
+ memset(&dma_sconfig, 0, sizeof(struct dma_slave_config));
+ dma_sconfig.dst_addr = up->dma_rx_addr;
+ dma_sconfig.dst_port_window_size = UART_RX_SIZE;
+ dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconfig.dst_maxburst = 4;
+ dma_sconfig.slave_id = up->channel_no;
+
+ dmaengine_slave_config(up->rx_dma_chan, &dma_sconfig);
+
+ //ast_uart_start_rx_dma(up, UART_RX_SIZE);
+ dma_sync_single_for_device(up->port.dev, up->dma_rx_addr,
+ UART_RX_SIZE, DMA_FROM_DEVICE);
+
+ up->rx_dma_desc = dmaengine_prep_slave_single(up->rx_dma_chan,
+ up->dma_rx_addr, UART_RX_SIZE, DMA_DEV_TO_MEM,
+ DMA_PREP_INTERRUPT);
+ if (!up->rx_dma_desc) {
+ dev_err(up->port.dev, "Not able to get desc for Rx\n");
+ return -EIO;
+ }
+ up->rx_dma_desc->callback = ast_uart_rx_dma_complete;
+ up->rx_dma_desc->callback_param = up;
+ up->rx_in_progress = 1;
+ up->rx_bytes_requested = UART_RX_SIZE;
+ up->rx_cookie = dmaengine_submit(up->rx_dma_desc);
+#endif
+
+ memset(&dma_sconfig, 0, sizeof(struct dma_slave_config));
+
+ up->tx_done = 1;
+ up->tx_count = 0;
+ up->tx_dma_buf.head = 0;
+ up->tx_dma_buf.tail = 0;
+ up->tx_dma_buf.buf = xmit->buf;
+ UART_DBG("head:0x%x tail:0x%x\n", xmit->head, xmit->tail);
+ xmit->head = 0;
+ xmit->tail = 0;
+
+ up->dma_tx_addr = dma_map_single(port->dev,
+ up->tx_dma_buf.buf,
+ UART_XMIT_SIZE,
+ DMA_TO_DEVICE);
+#if 1
+ dma_sconfig.src_addr = up->dma_tx_addr;
+ dma_sconfig.src_port_window_size = UART_XMIT_SIZE;
+ dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconfig.src_maxburst = 4;
+ dma_sconfig.slave_id = up->channel_no;
+ dmaengine_slave_config(up->tx_dma_chan, &dma_sconfig);
+#endif
+ //STOP and TRIGGER is done in SDMA driver
+ return 0;
+}
+
+static void serial8250_shutdown(struct uart_port *port)
+{
+ struct ast_uart_port *up = to_ast_dma_uart_port(port);
+ unsigned long flags;
+
+ up->ier = 0;
+ serial_outp(up, UART_IER, 0);
+
+ spin_lock_irqsave(&up->port.lock, flags);
+ up->port.mctrl &= ~TIOCM_OUT2;
+
+ serial8250_set_mctrl(&up->port, up->port.mctrl);
+ spin_unlock_irqrestore(&up->port.lock, flags);
+
+ /*
+ * Disable break condition and FIFOs
+ */
+ ast_serial_out(up, UART_LCR, serial_inp(up, UART_LCR) & ~UART_LCR_SBC);
+ serial8250_clear_fifos(up);
+
+ (void) ast_serial_in(up, UART_RX);
+
+ up->rx_in_progress = 0;
+ up->tx_in_progress = 0;
+ dma_release_channel(up->rx_dma_chan);
+ dma_release_channel(up->tx_dma_chan);
+ dma_free_coherent(port->dev, UART_RX_SIZE,
+ up->rx_dma_buf.buf, up->dma_rx_addr);
+ dma_unmap_single(port->dev, up->dma_tx_addr,
+ UART_XMIT_SIZE, DMA_TO_DEVICE);
+ up->rx_dma_chan = NULL;
+ up->tx_dma_chan = NULL;
+ up->dma_rx_addr = 0;
+ up->dma_rx_addr = 0;
+ //Tx buffer will free by serial_core.c
+ free_irq(up->port.irq, ast_uart_irq);
+}
+
+static unsigned int serial8250_get_divisor(struct uart_port *port,
+ unsigned int baud)
+{
+ unsigned int quot;
+
+ quot = uart_get_divisor(port, baud);
+ return quot;
+}
+
+static void
+serial8250_set_termios(struct uart_port *port, struct ktermios *termios,
+ struct ktermios *old)
+{
+ struct ast_uart_port *up = to_ast_dma_uart_port(port);
+ unsigned char cval, fcr = 0;
+ unsigned long flags;
+ unsigned int baud, quot;
+
+ switch (termios->c_cflag & CSIZE) {
+ case CS5:
+ cval = UART_LCR_WLEN5;
+ break;
+ case CS6:
+ cval = UART_LCR_WLEN6;
+ break;
+ case CS7:
+ cval = UART_LCR_WLEN7;
+ break;
+ default:
+ case CS8:
+ cval = UART_LCR_WLEN8;
+ break;
+ }
+
+ if (termios->c_cflag & CSTOPB)
+ cval |= UART_LCR_STOP;
+ if (termios->c_cflag & PARENB)
+ cval |= UART_LCR_PARITY;
+ if (!(termios->c_cflag & PARODD))
+ cval |= UART_LCR_EPAR;
+#ifdef CMSPAR
+ if (termios->c_cflag & CMSPAR)
+ cval |= UART_LCR_SPAR;
+#endif
+
+ /*
+ * Ask the core to calculate the divisor for us.
+ */
+ baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk/16);
+ quot = serial8250_get_divisor(port, baud);
+
+ if (up->capabilities & UART_CAP_FIFO && up->port.fifosize > 1) {
+ if (baud < 2400)
+ fcr = UART_FCR_ENABLE_FIFO | UART_FCR_TRIGGER_1;
+ else
+ fcr = uart_config[up->port.type].fcr;
+ }
+
+ /*
+ * Ok, we're now changing the port state. Do it with
+ * interrupts disabled.
+ */
+ spin_lock_irqsave(&up->port.lock, flags);
+
+ /*
+ * Update the per-port timeout.
+ */
+ uart_update_timeout(port, termios->c_cflag, baud);
+
+ up->port.read_status_mask = UART_LSR_OE | UART_LSR_THRE | UART_LSR_DR;
+ if (termios->c_iflag & INPCK)
+ up->port.read_status_mask |= UART_LSR_FE | UART_LSR_PE;
+ if (termios->c_iflag & (BRKINT | PARMRK))
+ up->port.read_status_mask |= UART_LSR_BI;
+
+ /*
+ * Characteres to ignore
+ */
+ up->port.ignore_status_mask = 0;
+ if (termios->c_iflag & IGNPAR)
+ up->port.ignore_status_mask |= UART_LSR_PE | UART_LSR_FE;
+ if (termios->c_iflag & IGNBRK) {
+ up->port.ignore_status_mask |= UART_LSR_BI;
+ /*
+ * If we're ignoring parity and break indicators,
+ * ignore overruns too (for real raw support).
+ */
+ if (termios->c_iflag & IGNPAR)
+ up->port.ignore_status_mask |= UART_LSR_OE;
+ }
+
+ /*
+ * ignore all characters if CREAD is not set
+ */
+ if ((termios->c_cflag & CREAD) == 0)
+ up->port.ignore_status_mask |= UART_LSR_DR;
+
+ /*
+ * CTS flow control flag and modem status interrupts
+ */
+ up->ier &= ~UART_IER_MSI;
+ if (UART_ENABLE_MS(&up->port, termios->c_cflag))
+ up->ier |= UART_IER_MSI;
+
+ ast_serial_out(up, UART_IER, up->ier);
+
+
+ serial_outp(up, UART_LCR, cval | UART_LCR_DLAB);/* set DLAB */
+
+ serial_dl_write(up, quot);
+
+ /*
+ * LCR DLAB must be set to enable 64-byte FIFO mode. If the FCR
+ * is written without DLAB set, this mode will be disabled.
+ */
+
+ serial_outp(up, UART_LCR, cval); /* reset DLAB */
+ up->lcr = cval; /* Save LCR */
+ if (fcr & UART_FCR_ENABLE_FIFO) {
+ /* emulated UARTs (Lucent Venus 167x) need two steps */
+ serial_outp(up, UART_FCR, UART_FCR_ENABLE_FIFO);
+ }
+ serial_outp(up, UART_FCR, fcr); /* set fcr */
+ serial8250_set_mctrl(&up->port, up->port.mctrl);
+ spin_unlock_irqrestore(&up->port.lock, flags);
+ /* Don't rewrite B0 */
+ if (tty_termios_baud_rate(termios))
+ tty_termios_encode_baud_rate(termios, baud, baud);
+}
+
+static void
+serial8250_pm(struct uart_port *port, unsigned int state,
+ unsigned int oldstate)
+{
+ struct ast_uart_port *p = (struct ast_uart_port *)port;
+
+ if (p->pm)
+ p->pm(port, state, oldstate);
+}
+
+/*
+ * Resource handling.
+ */
+static int serial8250_request_std_resource(struct ast_uart_port *up)
+{
+ unsigned int size = 8 << up->port.regshift;
+ int ret = 0;
+
+ if (!up->port.mapbase)
+ return ret;
+
+ if (!request_mem_region(up->port.mapbase, size, "ast-uart-dma")) {
+ ret = -EBUSY;
+ return ret;
+ }
+
+ if (up->port.flags & UPF_IOREMAP) {
+ up->port.membase = ioremap_nocache(up->port.mapbase, size);
+ if (!up->port.membase) {
+ release_mem_region(up->port.mapbase, size);
+ ret = -ENOMEM;
+ return ret;
+ }
+ }
+ return ret;
+}
+
+static void serial8250_release_std_resource(struct ast_uart_port *up)
+{
+ unsigned int size = 8 << up->port.regshift;
+
+ if (!up->port.mapbase)
+ return;
+
+ if (up->port.flags & UPF_IOREMAP) {
+ iounmap(up->port.membase);
+ up->port.membase = NULL;
+ }
+
+ release_mem_region(up->port.mapbase, size);
+}
+
+
+static void serial8250_release_port(struct uart_port *port)
+{
+ struct ast_uart_port *up = (struct ast_uart_port *)port;
+
+ serial8250_release_std_resource(up);
+}
+
+static int serial8250_request_port(struct uart_port *port)
+{
+ struct ast_uart_port *up = (struct ast_uart_port *)port;
+ int ret = 0;
+
+ ret = serial8250_request_std_resource(up);
+ if (ret == 0)
+ serial8250_release_std_resource(up);
+
+ return ret;
+}
+
+static void serial8250_config_port(struct uart_port *port, int flags)
+{
+ struct ast_uart_port *up = (struct ast_uart_port *)port;
+ int ret;
+
+ /*
+ * Find the region that we can probe for. This in turn
+ * tells us whether we can probe for the type of port.
+ */
+ ret = serial8250_request_std_resource(up);
+ if (ret < 0)
+ return;
+
+ if (flags & UART_CONFIG_TYPE)
+ autoconfig(up);
+
+ if (up->port.type == PORT_UNKNOWN)
+ serial8250_release_std_resource(up);
+}
+
+static int
+serial8250_verify_port(struct uart_port *port, struct serial_struct *ser)
+{
+ return 0;
+}
+
+static const char *
+serial8250_type(struct uart_port *port)
+{
+ int type = port->type;
+
+ if (type >= ARRAY_SIZE(uart_config))
+ type = 0;
+ return uart_config[type].name;
+}
+
+static const struct uart_ops serial8250_pops = {
+ .tx_empty = serial8250_tx_empty,
+ .set_mctrl = serial8250_set_mctrl,
+ .get_mctrl = serial8250_get_mctrl,
+ .stop_tx = serial8250_stop_tx,
+ .start_tx = serial8250_start_tx,
+ .stop_rx = serial8250_stop_rx,
+ .enable_ms = serial8250_enable_ms,
+ .break_ctl = serial8250_break_ctl,
+ .startup = serial8250_startup,
+ .shutdown = serial8250_shutdown,
+ .set_termios = serial8250_set_termios,
+ .pm = serial8250_pm,
+ .type = serial8250_type,
+ .release_port = serial8250_release_port,
+ .request_port = serial8250_request_port,
+ .config_port = serial8250_config_port,
+ .verify_port = serial8250_verify_port,
+};
+
+static void __init serial8250_isa_init_ports(void)
+{
+ static int first = 1;
+ int i;
+
+ if (!first)
+ return;
+ first = 0;
+
+ for (i = 0; i < nr_uarts; i++) {
+ struct ast_uart_port *up = &ast_uart_ports[i];
+
+ up->port.line = i;
+ spin_lock_init(&up->port.lock);
+
+ /*
+ * ALPHA_KLUDGE_MCR needs to be killed.
+ */
+ up->mcr_mask = ~ALPHA_KLUDGE_MCR;
+ up->mcr_force = ALPHA_KLUDGE_MCR;
+
+ up->port.ops = &serial8250_pops;
+ }
+
+}
+
+static void __init
+serial8250_register_ports(struct uart_driver *drv, struct device *dev)
+{
+ int i;
+ struct ast_uart_port *up = NULL;
+
+ serial8250_isa_init_ports();
+ for (i = 0; i < nr_uarts; i++) {
+ up = &ast_uart_ports[i];
+ up->port.dev = dev;
+ uart_add_one_port(drv, &up->port);
+ }
+}
+
+#define SERIAL8250_CONSOLE NULL
+
+static struct uart_driver serial8250_reg = {
+ .owner = THIS_MODULE,
+ .driver_name = "ast-uart-dma",
+ .dev_name = "ttyDMA",
+#if 0
+ .major = TTY_MAJOR,
+ .minor = 64,
+#else
+ .major = 204, // like atmel_serial
+ .minor = 155,
+#endif
+ .nr = UART_DMA_NR,
+ .cons = SERIAL8250_CONSOLE,
+};
+
+static void ast_dma_channel_teardown(struct ast_uart_port *s)
+{
+ UART_DBG("Teardown called\n");
+ if (s->tx_dma_chan) {
+ dma_release_channel(s->tx_dma_chan);
+ s->tx_dma_chan = NULL;
+ }
+ if (s->rx_dma_chan) {
+ dma_release_channel(s->rx_dma_chan);
+ s->rx_dma_chan = NULL;
+ }
+
+}
+static int ast_dma_channel_setup(struct ast_uart_port *up)
+{
+ up->rx_dma_chan = dma_request_slave_channel(up->port.dev, "rx");
+ UART_DBG("Entered %s ptr is %p\n", __func__, up->rx_dma_chan);
+ if (!up->rx_dma_chan)
+ goto err_out;
+
+ up->tx_dma_chan = dma_request_slave_channel(up->port.dev, "tx");
+ UART_DBG("Entered %s ptr is %p\n", __func__, up->tx_dma_chan);
+ if (!up->tx_dma_chan)
+ goto err_out;
+
+ return 0;
+err_out:
+ ast_dma_channel_teardown(up);
+ return -EINVAL;
+}
+
+/*
+ * Register a set of serial devices attached to a platform device. The
+ * list is terminated with a zero flags entry, which means we expect
+ * all entries to have at least UPF_BOOT_AUTOCONF set.
+ */
+struct clk *clk;
+static int serial8250_probe(struct platform_device *dev)
+{
+ struct device_node *np = dev->dev.of_node;
+ struct uart_port port;
+ int ret;
+ u32 read = 0;
+ struct resource *res = 0;
+
+ if (UART_XMIT_SIZE > DMA_BUFF_SIZE)
+ UART_DBG("UART_XMIT_SIZE > DMA_BUFF_SIZE : Please Check\n");
+ memset(&port, 0, sizeof(struct uart_port));
+
+ port.irq = platform_get_irq(dev, 0);
+ if (port.irq < 0) {
+ dev_err(&dev->dev, "cannot get irq\n");
+ return port.irq;
+ }
+ res = platform_get_resource(dev, IORESOURCE_MEM, 0);
+ if (!res) {
+ dev_err(&dev->dev, "Register base not found");
+ return -ENODEV;
+ }
+ port.mapbase = res->start;
+ clk = devm_clk_get(&dev->dev, NULL);
+ if (IS_ERR(clk))
+ dev_err(&dev->dev, "missing controller clock");
+
+ ret = clk_prepare_enable(clk);
+ if (ret) {
+ dev_err(&dev->dev, "failed to enable DMA UART Clk\n");
+ return ret;
+ }
+ port.uartclk = clk_get_rate(clk);
+
+ if (of_property_read_u32(np, "reg-shift", &read) == 0)
+ port.regshift = read;
+ port.iotype = UPIO_MEM;
+ port.flags = UPF_IOREMAP | UPF_BOOT_AUTOCONF | UPF_SKIP_TEST;
+ port.dev = &dev->dev;
+ if (share_irqs)
+ port.flags |= UPF_SHARE_IRQ;
+ ret = ast_uart_register_port(&port, read);
+ if (ret < 0) {
+ dev_err(&dev->dev,
+ "Fail:register_port at index %d(IO%lx MEM%llx IRQ%d): %d\n",
+ read, port.iobase, (unsigned long long)port.mapbase,
+ port.irq, ret);
+ }
+ return ret;
+}
+
+/*
+ * Remove serial ports registered against a platform device.
+ */
+static int serial8250_remove(struct platform_device *dev)
+{
+ int i;
+
+ for (i = 0; i < nr_uarts; i++) {
+ struct ast_uart_port *up = &ast_uart_ports[i];
+
+ if (up->port.dev == &dev->dev)
+ ast_uart_unregister_port(i);
+ ast_dma_channel_teardown(up);
+ }
+ return 0;
+}
+
+static int serial8250_suspend(struct platform_device *dev, pm_message_t state)
+{
+ int i;
+
+ for (i = 0; i < UART_DMA_NR; i++) {
+ struct ast_uart_port *up = &ast_uart_ports[i];
+
+ if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev->dev)
+ uart_suspend_port(&serial8250_reg, &up->port);
+ }
+
+ return 0;
+}
+
+static int serial8250_resume(struct platform_device *dev)
+{
+ int i;
+
+ for (i = 0; i < UART_DMA_NR; i++) {
+ struct ast_uart_port *up = &ast_uart_ports[i];
+
+ if (up->port.type != PORT_UNKNOWN && up->port.dev == &dev->dev)
+ serial8250_resume_port(i);
+ }
+
+ return 0;
+}
+
+static const struct of_device_id ast_serial_dt_ids[] = {
+ { .compatible = "aspeed,ast-sdma-uart", },
+ { /* sentinel */ }
+};
+
+static struct platform_driver serial8250_ast_dma_driver = {
+ .probe = serial8250_probe,
+ .remove = serial8250_remove,
+ .suspend = serial8250_suspend,
+ .resume = serial8250_resume,
+ .driver = {
+ .name = "ast-uart-dma",
+ .of_match_table = of_match_ptr(ast_serial_dt_ids),
+ },
+};
+
+/*
+ * This "device" covers _all_ ISA 8250-compatible serial devices listed
+ * in the table in include/asm/serial.h
+ */
+static struct platform_device *serial8250_isa_devs;
+
+/*
+ * serial8250_register_port and serial8250_unregister_port allows for
+ * 16x50 serial ports to be configured at run-time, to support PCMCIA
+ * modems and PCI multiport cards.
+ */
+
+static struct ast_uart_port *
+ serial8250_find_match_or_unused(struct uart_port *port)
+{
+ int i;
+
+ /*
+ * First, find a port entry which matches.
+ */
+ for (i = 0; i < nr_uarts; i++) {
+ if (uart_match_port(&ast_uart_ports[i].port, port))
+ return &ast_uart_ports[i];
+ }
+ /*
+ * We didn't find a matching entry, so look for the first
+ * free entry. We look for one which hasn't been previously
+ * used (indicated by zero iobase).
+ */
+ for (i = 0; i < nr_uarts; i++)
+ if (ast_uart_ports[i].port.type == PORT_UNKNOWN &&
+ ast_uart_ports[i].port.iobase == 0)
+ return &ast_uart_ports[i];
+ /*
+ * That also failed. Last resort is to find any entry which
+ * doesn't have a real port associated with it.
+ */
+ for (i = 0; i < nr_uarts; i++)
+ if (ast_uart_ports[i].port.type == PORT_UNKNOWN)
+ return &ast_uart_ports[i];
+
+ return NULL;
+}
+
+/**
+ * serial8250_register_port - register a serial port
+ * @port: serial port template
+ *
+ * Configure the serial port specified by the request. If the
+ * port exists and is in use, it is hung up and unregistered
+ * first.
+ *
+ * The port is then probed and if necessary the IRQ is autodetected
+ * If this fails an error is returned.
+ *
+ * On success the port is ready to use and the line number is returned.
+ */
+static int ast_uart_register_port(struct uart_port *port,
+ unsigned int channel_no)
+{
+ struct ast_uart_port *uart;
+ int ret = -ENOSPC;
+
+ if (port->uartclk == 0)
+ return -EINVAL;
+
+ mutex_lock(&ast_uart_mutex);
+
+ uart = serial8250_find_match_or_unused(port);
+ if (uart) {
+ uart_remove_one_port(&serial8250_reg, &uart->port);
+ uart->port.iobase = port->iobase;
+ uart->port.membase = port->membase;
+ uart->port.irq = port->irq;
+ uart->port.uartclk = port->uartclk;
+ uart->port.fifosize = port->fifosize;
+ uart->port.regshift = port->regshift;
+ uart->port.iotype = port->iotype;
+ uart->port.flags = port->flags | UPF_BOOT_AUTOCONF;
+ uart->port.mapbase = port->mapbase;
+ uart->port.private_data = uart;
+ if (port->dev) {
+ UART_DBG("Writing dev\n");
+ uart->port.dev = port->dev;
+ }
+ ret = uart_add_one_port(&serial8250_reg, &uart->port);
+ if (ret != 0) {
+ UART_DBG("uart_add_one_port: Failed for port=%p",
+ &uart->port);
+ return ret;
+ }
+ uart->channel_no = channel_no;
+ spin_lock_init(&uart->lock);
+
+ tasklet_init(&uart->tx_tasklet, ast_uart_tx_sdma_tasklet_func,
+ (unsigned long)uart);
+ tasklet_init(&uart->rx_tasklet, ast_uart_rx_sdma_tasklet_func,
+ (unsigned long)uart);
+ }
+
+ mutex_unlock(&ast_uart_mutex);
+ return ret;
+}
+EXPORT_SYMBOL(ast_uart_register_port);
+
+/**
+ * serial8250_unregister_port - remove a 16x50 serial port at runtime
+ * @line: serial line number
+ *
+ * Remove one serial port. This may not be called from interrupt
+ * context. We hand the port back to the our control.
+ */
+static void ast_uart_unregister_port(int line)
+{
+ struct ast_uart_port *uart = &ast_uart_ports[line];
+
+ mutex_lock(&ast_uart_mutex);
+ uart_remove_one_port(&serial8250_reg, &uart->port);
+ if (serial8250_isa_devs) {
+ uart->port.flags &= ~UPF_BOOT_AUTOCONF;
+ uart->port.type = PORT_UNKNOWN;
+ uart->port.dev = &serial8250_isa_devs->dev;
+ uart_add_one_port(&serial8250_reg, &uart->port);
+ } else {
+ uart->port.dev = NULL;
+ }
+ mutex_unlock(&ast_uart_mutex);
+}
+EXPORT_SYMBOL(ast_uart_unregister_port);
+
+static int __init ast_uart_init(void)
+{
+ int ret;
+
+ if (nr_uarts > UART_DMA_NR)
+ nr_uarts = UART_DMA_NR;
+
+ UART_DBG(KERN_INFO
+ "ast-uart-dma: UART driver with DMA %d ports, IRQ sharing %sabled\n",
+ nr_uarts, share_irqs ? "en" : "dis");
+ spin_lock_init(&ast_uart_irq[0].lock);
+
+ ret = uart_register_driver(&serial8250_reg);
+ if (ret)
+ goto out;
+
+ serial8250_isa_devs = platform_device_alloc("ast-uart-dma",
+ PLAT8250_DEV_LEGACY);
+ if (!serial8250_isa_devs) {
+ ret = -ENOMEM;
+ goto unreg_uart_drv;
+ }
+
+ ret = platform_device_add(serial8250_isa_devs);
+ if (ret)
+ goto put_dev;
+
+ serial8250_register_ports(&serial8250_reg, &serial8250_isa_devs->dev);
+
+ ret = platform_driver_register(&serial8250_ast_dma_driver);
+ if (ret == 0)
+ goto out;
+
+ platform_device_del(serial8250_isa_devs);
+ put_dev:
+ platform_device_put(serial8250_isa_devs);
+ unreg_uart_drv:
+ uart_unregister_driver(&serial8250_reg);
+ out:
+ return ret;
+}
+
+static void __exit ast_uart_exit(void)
+{
+ struct platform_device *isa_dev = serial8250_isa_devs;
+
+ /*
+ * This tells serial8250_unregister_port() not to re-register
+ * the ports (thereby making serial8250_ast_dma_driver permanently
+ * in use.)
+ */
+ serial8250_isa_devs = NULL;
+
+ platform_driver_unregister(&serial8250_ast_dma_driver);
+ platform_device_unregister(isa_dev);
+
+ uart_unregister_driver(&serial8250_reg);
+}
+
+late_initcall(ast_uart_init);
+module_exit(ast_uart_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("AST DMA serial driver");
+MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR);
--
1.9.1
^ permalink raw reply related
* [[PATCH] 8/9] DMA-UART-Driver-for-AST2500
From: Vinod @ 2018-10-17 6:05 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1539749466-3912-9-git-send-email-open.sudheer@gmail.com>
On 17-10-18, 09:41, sudheer.v wrote:
Please add the change log describing the driver and its features
> Signed-off-by: sudheer.v <open.sudheer@gmail.com>
> ---
> drivers/tty/serial/8250/8250_aspeed_uart_dma.c | 1594 ++++++++++++++++++++++++
> 1 file changed, 1594 insertions(+)
> create mode 100644 drivers/tty/serial/8250/8250_aspeed_uart_dma.c
>
> diff --git a/drivers/tty/serial/8250/8250_aspeed_uart_dma.c b/drivers/tty/serial/8250/8250_aspeed_uart_dma.c
> new file mode 100644
> index 0000000..e1019a8
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_aspeed_uart_dma.c
why is this in serial. It is dmaengine driver so belongs to drivers/dma/
like other controllers. Please move it out and resubmit.
While doing resubmission please take some time to understand subsystem
tags to use. (hint git log <subsystem> will tell you)
Also series has [[PATCH] 8/9] whereas it should be [PATCH 8/9] please
let git generate that for you (hint git format-patch start..end does a
good job)
> @@ -0,0 +1,1594 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * drivers/tty/serial/8250/8250_aspeed_uart_dma.c
> + * 1. 2018/07/01 Shivah Shankar created
> + * 2. 2018/08/25 sudheer.veliseti<open.sudheer@gmail.com> modified
we dont use this log in kernel. I do not see s-o-b by Shivah, that
should be added. I think he should be author and you need to list
changes you did..
--
~Vinod
^ permalink raw reply
* [[PATCH] 8/9] DMA-UART-Driver-for-AST2500
From: Benjamin Herrenschmidt @ 2018-10-17 8:56 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181017060531.GU2400@vkoul-mobl>
On Wed, 2018-10-17 at 11:35 +0530, Vinod wrote:
> On 17-10-18, 09:41, sudheer.v wrote:
>
> Please add the change log describing the driver and its features
>
> > Signed-off-by: sudheer.v <open.sudheer@gmail.com>
>
>
> > ---
> > drivers/tty/serial/8250/8250_aspeed_uart_dma.c | 1594 ++++++++++++++++++++++++
> > 1 file changed, 1594 insertions(+)
> > create mode 100644 drivers/tty/serial/8250/8250_aspeed_uart_dma.c
> >
> > diff --git a/drivers/tty/serial/8250/8250_aspeed_uart_dma.c b/drivers/tty/serial/8250/8250_aspeed_uart_dma.c
> > new file mode 100644
> > index 0000000..e1019a8
> > --- /dev/null
> > +++ b/drivers/tty/serial/8250/8250_aspeed_uart_dma.c
>
> why is this in serial. It is dmaengine driver so belongs to drivers/dma/
> like other controllers. Please move it out and resubmit.
It's not a dmaengine driver. It's a serial UART driver that happens to
use a dedicated DMA engine.
It's unclear whether it should be split into two drivers, or just have
the serial driver directly use the dma engine since that engine is
dedicated in HW to only work on those UARTs and nothing else...
Cheers,
Ben.
> While doing resubmission please take some time to understand subsystem
> tags to use. (hint git log <subsystem> will tell you)
>
> Also series has [[PATCH] 8/9] whereas it should be [PATCH 8/9] please
> let git generate that for you (hint git format-patch start..end does a
> good job)
>
> > @@ -0,0 +1,1594 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * drivers/tty/serial/8250/8250_aspeed_uart_dma.c
> > + * 1. 2018/07/01 Shivah Shankar created
> > + * 2. 2018/08/25 sudheer.veliseti<open.sudheer@gmail.com> modified
>
> we dont use this log in kernel. I do not see s-o-b by Shivah, that
> should be added. I think he should be author and you need to list
> changes you did..
>
^ permalink raw reply
* [PATCH v4 2/2] media: platform: Add Aspeed Video Engine driver
From: Hans Verkuil @ 2018-10-17 10:41 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1538769466-14860-3-git-send-email-eajames@linux.ibm.com>
On 10/05/2018 09:57 PM, Eddie James wrote:
> The Video Engine (VE) embedded in the Aspeed AST2400 and AST2500 SOCs
> can capture and compress video data from digital or analog sources. With
> the Aspeed chip acting a service processor, the Video Engine can capture
> the host processor graphics output.
>
> Add a V4L2 driver to capture video data and compress it to JPEG images.
> Make the video frames available through the V4L2 streaming interface.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
> MAINTAINERS | 8 +
> drivers/media/platform/Kconfig | 8 +
> drivers/media/platform/Makefile | 1 +
> drivers/media/platform/aspeed-video.c | 1674 +++++++++++++++++++++++++++++++++
> 4 files changed, 1691 insertions(+)
> create mode 100644 drivers/media/platform/aspeed-video.c
>
<snip>
> +static void aspeed_video_check_polarity(struct aspeed_video *video)
> +{
> + int i;
> + int hsync_counter = 0;
> + int vsync_counter = 0;
> + u32 sts;
> +
> + for (i = 0; i < NUM_POLARITY_CHECKS; ++i) {
> + sts = aspeed_video_read(video, VE_MODE_DETECT_STATUS);
> + if (sts & VE_MODE_DETECT_STATUS_VSYNC)
> + vsync_counter--;
> + else
> + vsync_counter++;
> +
> + if (sts & VE_MODE_DETECT_STATUS_HSYNC)
> + hsync_counter--;
> + else
> + hsync_counter++;
> + }
> +
> + if (hsync_counter < 0 || vsync_counter < 0) {
> + u32 ctrl;
> +
> + if (hsync_counter < 0)
> + ctrl = VE_CTRL_HSYNC_POL;
> + else
> + video->timings.polarities |= V4L2_DV_HSYNC_POS_POL;
> +
> + if (vsync_counter < 0)
> + ctrl = VE_CTRL_VSYNC_POL;
> + else
> + video->timings.polarities |= V4L2_DV_VSYNC_POS_POL;
> +
> + aspeed_video_update(video, VE_CTRL, 0, ctrl);
> + }
> +}
> +
> +static bool aspeed_video_alloc_buf(struct aspeed_video *video,
> + struct aspeed_video_addr *addr,
> + unsigned int size)
> +{
> + addr->virt = dma_alloc_coherent(video->dev, size, &addr->dma,
> + GFP_KERNEL);
> + if (!addr->virt)
> + return false;
> +
> + addr->size = size;
> + return true;
> +}
> +
> +static void aspeed_video_free_buf(struct aspeed_video *video,
> + struct aspeed_video_addr *addr)
> +{
> + dma_free_coherent(video->dev, addr->size, addr->virt, addr->dma);
> + addr->size = 0;
> + addr->dma = 0ULL;
> + addr->virt = NULL;
> +}
> +
> +/*
> + * Get the minimum HW-supported compression buffer size for the frame size.
> + * Assume worst-case JPEG compression size is 1/8 raw size. This should be
> + * plenty even for maximum quality; any worse and the engine will simply return
> + * incomplete JPEGs.
> + */
> +static void aspeed_video_calc_compressed_size(struct aspeed_video *video)
> +{
> + int i, j;
> + u32 compression_buffer_size_reg = 0;
> + unsigned int size;
> + const unsigned int num_compression_packets = 4;
> + const unsigned int compression_packet_size = 1024;
> + const unsigned int max_compressed_size =
> + video->width * video->height / 2; /* 4 Bpp / 8 */
> +
> + video->max_compressed_size = UINT_MAX;
> +
> + for (i = 0; i < 6; ++i) {
> + for (j = 0; j < 8; ++j) {
> + size = (num_compression_packets << i) *
> + (compression_packet_size << j);
> + if (size < max_compressed_size)
> + continue;
> +
> + if (size < video->max_compressed_size) {
> + compression_buffer_size_reg = (i << 3) | j;
> + video->max_compressed_size = size;
> + }
> + }
> + }
> +
> + aspeed_video_write(video, VE_STREAM_BUF_SIZE,
> + compression_buffer_size_reg);
> +
> + dev_dbg(video->dev, "max compressed size: %x\n",
> + video->max_compressed_size);
> +}
> +
> +#define res_check(v) test_and_clear_bit(VIDEO_MODE_DETECT_DONE, &(v)->flags)
> +
> +static int aspeed_video_get_resolution(struct aspeed_video *video)
> +{
> + bool invalid_resolution = true;
> + int rc;
> + int tries = 0;
> + unsigned int bottom;
> + unsigned int left;
> + unsigned int right;
> + unsigned int size;
> + unsigned int top;
> + u32 mds;
> + u32 src_lr_edge;
> + u32 src_tb_edge;
> + u32 sync;
> + struct aspeed_video_addr src;
> +
> + if (video->srcs[1].size)
> + aspeed_video_free_buf(video, &video->srcs[1]);
> +
> + if (video->srcs[0].size >= VE_MAX_SRC_BUFFER_SIZE) {
> + src = video->srcs[0];
> + } else {
> + if (video->srcs[0].size)
> + aspeed_video_free_buf(video, &video->srcs[0]);
> +
> + if (!aspeed_video_alloc_buf(video, &src,
> + VE_MAX_SRC_BUFFER_SIZE))
> + goto err_mem;
> + }
> +
> + aspeed_video_write(video, VE_SRC0_ADDR, src.dma);
> +
> + video->width = 0;
> + video->height = 0;
> +
> + do {
> + if (tries) {
> + set_current_state(TASK_INTERRUPTIBLE);
> + if (schedule_timeout(INVALID_RESOLUTION_DELAY))
> + return -EINTR;
> + }
> +
> + aspeed_video_start_mode_detect(video);
> +
> + rc = wait_event_interruptible_timeout(video->wait,
> + res_check(video),
> + MODE_DETECT_TIMEOUT);
> + if (!rc) {
> + dev_err(video->dev, "timed out on 1st mode detect\n");
> + aspeed_video_disable_mode_detect(video);
> + return -ETIMEDOUT;
> + }
> +
> + /* Disable mode detect in order to re-trigger */
> + aspeed_video_update(video, VE_SEQ_CTRL,
> + VE_SEQ_CTRL_TRIG_MODE_DET, 0);
> +
> + aspeed_video_check_polarity(video);
> +
> + aspeed_video_start_mode_detect(video);
> +
> + rc = wait_event_interruptible_timeout(video->wait,
> + res_check(video),
> + MODE_DETECT_TIMEOUT);
> + if (!rc) {
> + dev_err(video->dev, "timed out on 2nd mode detect\n");
> + aspeed_video_disable_mode_detect(video);
> + return -ETIMEDOUT;
> + }
> +
> + src_lr_edge = aspeed_video_read(video, VE_SRC_LR_EDGE_DET);
> + src_tb_edge = aspeed_video_read(video, VE_SRC_TB_EDGE_DET);
> + mds = aspeed_video_read(video, VE_MODE_DETECT_STATUS);
> + sync = aspeed_video_read(video, VE_SYNC_STATUS);
> +
> + bottom = (src_tb_edge & VE_SRC_TB_EDGE_DET_BOT) >>
> + VE_SRC_TB_EDGE_DET_BOT_SHF;
> + top = src_tb_edge & VE_SRC_TB_EDGE_DET_TOP;
> + video->timings.vfrontporch = top;
> + video->timings.vbackporch = ((mds & VE_MODE_DETECT_V_LINES) >>
> + VE_MODE_DETECT_V_LINES_SHF) - bottom;
> + video->timings.vsync = (sync & VE_SYNC_STATUS_VSYNC) >>
> + VE_SYNC_STATUS_VSYNC_SHF;
> + if (top > bottom)
> + continue;
> +
> + right = (src_lr_edge & VE_SRC_LR_EDGE_DET_RT) >>
> + VE_SRC_LR_EDGE_DET_RT_SHF;
> + left = src_lr_edge & VE_SRC_LR_EDGE_DET_LEFT;
> + video->timings.hfrontporch = left;
> + video->timings.hbackporch = (mds & VE_MODE_DETECT_H_PIXELS) -
> + right;
> + video->timings.hsync = sync & VE_SYNC_STATUS_HSYNC;
It's a bit odd that timings.width/height isn't set here.
> + if (left > right)
> + continue;
> +
> + invalid_resolution = false;
> + } while (invalid_resolution && (tries++ < INVALID_RESOLUTION_RETRIES));
> +
> + if (invalid_resolution) {
> + dev_err(video->dev, "invalid resolution detected\n");
> + return -ERANGE;
> + }
So far, so good. You now have detected the new resolution, but...
> +
> + video->height = (bottom - top) + 1;
> + video->width = (right - left) + 1;
> + size = video->height * video->width;
here and below you appear to actually switch the capture timings to the new
resolution as well.
That should only happen when s_dv_timings is called.
It is always very dangerous to automatically switch resolution since a new
resolution might mean larger buffers, and userspace has to prepare for that.
The normal sequence is:
1) the driver detects a change in resolution (or the video signal disappears
completely). It will send V4L2_EVENT_SOURCE_CHANGE to let userspace know.
2) Userspace stops streaming, frees all buffers, calls QUERY_DV_TIMINGS and,
if valid, follows with S_DV_TIMINGS.
3) Userspace calls REQBUFS to allocate the new buffers sized for the new timings
and starts streaming again.
That way everything remains under control of the application.
> +
> + /* Don't use direct mode below 1024 x 768 (irqs don't fire) */
> + if (size < DIRECT_FETCH_THRESHOLD) {
> + aspeed_video_write(video, VE_TGS_0,
> + FIELD_PREP(VE_TGS_FIRST, left - 1) |
> + FIELD_PREP(VE_TGS_LAST, right));
> + aspeed_video_write(video, VE_TGS_1,
> + FIELD_PREP(VE_TGS_FIRST, top) |
> + FIELD_PREP(VE_TGS_LAST, bottom + 1));
> + aspeed_video_update(video, VE_CTRL, 0, VE_CTRL_INT_DE);
> + } else {
> + aspeed_video_update(video, VE_CTRL, 0, VE_CTRL_DIRECT_FETCH);
> + }
> +
> + aspeed_video_write(video, VE_CAP_WINDOW,
> + video->width << 16 | video->height);
> + aspeed_video_write(video, VE_COMP_WINDOW,
> + video->width << 16 | video->height);
> + aspeed_video_write(video, VE_SRC_SCANLINE_OFFSET, video->width * 4);
> +
> + aspeed_video_update(video, VE_INTERRUPT_CTRL, 0,
> + VE_INTERRUPT_MODE_DETECT_WD);
> + aspeed_video_update(video, VE_SEQ_CTRL, 0,
> + VE_SEQ_CTRL_AUTO_COMP | VE_SEQ_CTRL_EN_WATCHDOG);
> +
> + dev_dbg(video->dev, "got resolution[%dx%d]\n", video->width,
> + video->height);
> +
> + size *= 4;
> + if (size == src.size / 2) {
> + aspeed_video_write(video, VE_SRC1_ADDR, src.dma + size);
> + video->srcs[0] = src;
> + } else if (size == src.size) {
> + video->srcs[0] = src;
> +
> + if (!aspeed_video_alloc_buf(video, &video->srcs[1], size))
> + goto err_mem;
> +
> + aspeed_video_write(video, VE_SRC1_ADDR, video->srcs[1].dma);
> + } else {
> + aspeed_video_free_buf(video, &src);
> +
> + if (!aspeed_video_alloc_buf(video, &video->srcs[0], size))
> + goto err_mem;
> +
> + if (!aspeed_video_alloc_buf(video, &video->srcs[1], size))
> + goto err_mem;
> +
> + aspeed_video_write(video, VE_SRC0_ADDR, video->srcs[0].dma);
> + aspeed_video_write(video, VE_SRC1_ADDR, video->srcs[1].dma);
> + }
> +
> + aspeed_video_calc_compressed_size(video);
> +
> + return 0;
> +
> +err_mem:
> + dev_err(video->dev, "failed to allocate source buffers\n");
> +
> + if (video->srcs[0].size)
> + aspeed_video_free_buf(video, &video->srcs[0]);
> +
> + return -ENOMEM;
> +}
Regards,
Hans
^ permalink raw reply
* [PATCH net-next v5] net/ncsi: Add NCSI Broadcom OEM command
From: David Miller @ 2018-10-18 5:15 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181016191319.1909502-1-vijaykhemka@fb.com>
From: Vijay Khemka <vijaykhemka@fb.com>
Date: Tue, 16 Oct 2018 12:13:19 -0700
> This patch adds OEM Broadcom commands and response handling. It also
> defines OEM Get MAC Address handler to get and configure the device.
>
> ncsi_oem_gma_handler_bcm: This handler send NCSI broadcom command for
> getting mac address.
> ncsi_rsp_handler_oem_bcm: This handles response received for all
> broadcom OEM commands.
> ncsi_rsp_handler_oem_bcm_gma: This handles get mac address response and
> set it to device.
>
> Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
Applied.
^ permalink raw reply
* [[PATCH] 8/9] DMA-UART-Driver-for-AST2500
From: Vinod @ 2018-10-18 9:55 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <351eecd5b1b21893e94f76b34c058c6257b7f837.camel@kernel.crashing.org>
On 17-10-18, 19:56, Benjamin Herrenschmidt wrote:
> On Wed, 2018-10-17 at 11:35 +0530, Vinod wrote:
> > On 17-10-18, 09:41, sudheer.v wrote:
> >
> > Please add the change log describing the driver and its features
> >
> > > Signed-off-by: sudheer.v <open.sudheer@gmail.com>
> > > ---
> > > drivers/tty/serial/8250/8250_aspeed_uart_dma.c | 1594 ++++++++++++++++++++++++
> > > 1 file changed, 1594 insertions(+)
> > > create mode 100644 drivers/tty/serial/8250/8250_aspeed_uart_dma.c
> > >
> > > diff --git a/drivers/tty/serial/8250/8250_aspeed_uart_dma.c b/drivers/tty/serial/8250/8250_aspeed_uart_dma.c
> > > new file mode 100644
> > > index 0000000..e1019a8
> > > --- /dev/null
> > > +++ b/drivers/tty/serial/8250/8250_aspeed_uart_dma.c
> >
> > why is this in serial. It is dmaengine driver so belongs to drivers/dma/
> > like other controllers. Please move it out and resubmit.
>
> It's not a dmaengine driver. It's a serial UART driver that happens to
> use a dedicated DMA engine.
Then I see no reason for it to use dmaengine APIs. The framework allows
people to share a controller for many clients, but if you have dedicated
one then you may use it directly
> It's unclear whether it should be split into two drivers, or just have
> the serial driver directly use the dma engine since that engine is
> dedicated in HW to only work on those UARTs and nothing else...
>
> Cheers,
> Ben.
>
>
> > While doing resubmission please take some time to understand subsystem
> > tags to use. (hint git log <subsystem> will tell you)
> >
> > Also series has [[PATCH] 8/9] whereas it should be [PATCH 8/9] please
> > let git generate that for you (hint git format-patch start..end does a
> > good job)
> >
> > > @@ -0,0 +1,1594 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * drivers/tty/serial/8250/8250_aspeed_uart_dma.c
> > > + * 1. 2018/07/01 Shivah Shankar created
> > > + * 2. 2018/08/25 sudheer.veliseti<open.sudheer@gmail.com> modified
> >
> > we dont use this log in kernel. I do not see s-o-b by Shivah, that
> > should be added. I think he should be author and you need to list
> > changes you did..
> >
--
~Vinod
^ permalink raw reply
* [[PATCH] 8/9] DMA-UART-Driver-for-AST2500
From: Benjamin Herrenschmidt @ 2018-10-18 23:32 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181018095507.GC2400@vkoul-mobl>
On Thu, 2018-10-18 at 15:25 +0530, Vinod wrote:
>
> > It's not a dmaengine driver. It's a serial UART driver that happens to
> > use a dedicated DMA engine.
>
> Then I see no reason for it to use dmaengine APIs. The framework allows
> people to share a controller for many clients, but if you have dedicated
> one then you may use it directly
Well... the engine is shared by a few UARTs, they have dedicated rings
but there's a common set of regs for interrupt handling etc.
That said, I still think it could be contained within a UART driver,
there's little benefit in adding the framework overhead, esp since
these are really weak cores, any overhead will be felt.
Ben.
> > It's unclear whether it should be split into two drivers, or just have
> > the serial driver directly use the dma engine since that engine is
> > dedicated in HW to only work on those UARTs and nothing else...
> >
> > Cheers,
> > Ben.
> >
> >
> > > While doing resubmission please take some time to understand subsystem
> > > tags to use. (hint git log <subsystem> will tell you)
> > >
> > > Also series has [[PATCH] 8/9] whereas it should be [PATCH 8/9] please
> > > let git generate that for you (hint git format-patch start..end does a
> > > good job)
> > >
> > > > @@ -0,0 +1,1594 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * drivers/tty/serial/8250/8250_aspeed_uart_dma.c
> > > > + * 1. 2018/07/01 Shivah Shankar created
> > > > + * 2. 2018/08/25 sudheer.veliseti<open.sudheer@gmail.com> modified
> > >
> > > we dont use this log in kernel. I do not see s-o-b by Shivah, that
> > > should be added. I think he should be author and you need to list
> > > changes you did..
> > >
>
>
^ permalink raw reply
* [[PATCH] 8/9] DMA-UART-Driver-for-AST2500
From: sudheer.v @ 2018-10-19 7:11 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <a3fecf7913d6a85a6294afbc5a1a18b7714d6756.camel@kernel.crashing.org>
On Fri, Oct 19, 2018 at 10:32:24AM +1100, Benjamin Herrenschmidt wrote:
> On Thu, 2018-10-18 at 15:25 +0530, Vinod wrote:
> >
> > > It's not a dmaengine driver. It's a serial UART driver that happens to
> > > use a dedicated DMA engine.
> >
> > Then I see no reason for it to use dmaengine APIs. The framework allows
> > people to share a controller for many clients, but if you have dedicated
> > one then you may use it directly
>
> Well... the engine is shared by a few UARTs, they have dedicated rings
> but there's a common set of regs for interrupt handling etc.
>
> That said, I still think it could be contained within a UART driver,
> there's little benefit in adding the framework overhead, esp since
> these are really weak cores, any overhead will be felt.
>
> Ben.
>
> > > It's unclear whether it should be split into two drivers, or just have
> > > the serial driver directly use the dma engine since that engine is
> > > dedicated in HW to only work on those UARTs and nothing else...
> > >
> > > Cheers,
> > > Ben.
Initially we wanted to have a single driver,
however we had an informal discussion with one of the maintainer
and based on the feedback, followed the Linux DMA and UART architecture.
If this seperate DMA-engine driver adds more overhead than benifit,
we will merge them into a single UART driver and resubmitt the patches.
Vinod,
can this dma-controller driver sit under dma subsystem?.
or better to move it under UART framework.
Thank you.
-- Sudheer
^ permalink raw reply
* [PATCH v4 2/2] media: platform: Add Aspeed Video Engine driver
From: Eddie James @ 2018-10-19 16:31 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <4a02833e-67a6-ec4f-d2f6-e0368412eca8@xs4all.nl>
On 10/17/2018 05:41 AM, Hans Verkuil wrote:
> On 10/05/2018 09:57 PM, Eddie James wrote:
>> The Video Engine (VE) embedded in the Aspeed AST2400 and AST2500 SOCs
>> can capture and compress video data from digital or analog sources. With
>> the Aspeed chip acting a service processor, the Video Engine can capture
>> the host processor graphics output.
>>
>> Add a V4L2 driver to capture video data and compress it to JPEG images.
>> Make the video frames available through the V4L2 streaming interface.
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>> MAINTAINERS | 8 +
>> drivers/media/platform/Kconfig | 8 +
>> drivers/media/platform/Makefile | 1 +
>> drivers/media/platform/aspeed-video.c | 1674 +++++++++++++++++++++++++++++++++
>> 4 files changed, 1691 insertions(+)
>> create mode 100644 drivers/media/platform/aspeed-video.c
>>
> <snip>
>
>> +static void aspeed_video_check_polarity(struct aspeed_video *video)
>> +{
>> + int i;
>> + int hsync_counter = 0;
>> + int vsync_counter = 0;
>> + u32 sts;
>> +
>> + for (i = 0; i < NUM_POLARITY_CHECKS; ++i) {
>> + sts = aspeed_video_read(video, VE_MODE_DETECT_STATUS);
>> + if (sts & VE_MODE_DETECT_STATUS_VSYNC)
>> + vsync_counter--;
>> + else
>> + vsync_counter++;
>> +
>> + if (sts & VE_MODE_DETECT_STATUS_HSYNC)
>> + hsync_counter--;
>> + else
>> + hsync_counter++;
>> + }
>> +
>> + if (hsync_counter < 0 || vsync_counter < 0) {
>> + u32 ctrl;
>> +
>> + if (hsync_counter < 0)
>> + ctrl = VE_CTRL_HSYNC_POL;
>> + else
>> + video->timings.polarities |= V4L2_DV_HSYNC_POS_POL;
>> +
>> + if (vsync_counter < 0)
>> + ctrl = VE_CTRL_VSYNC_POL;
>> + else
>> + video->timings.polarities |= V4L2_DV_VSYNC_POS_POL;
>> +
>> + aspeed_video_update(video, VE_CTRL, 0, ctrl);
>> + }
>> +}
>> +
>> +static bool aspeed_video_alloc_buf(struct aspeed_video *video,
>> + struct aspeed_video_addr *addr,
>> + unsigned int size)
>> +{
>> + addr->virt = dma_alloc_coherent(video->dev, size, &addr->dma,
>> + GFP_KERNEL);
>> + if (!addr->virt)
>> + return false;
>> +
>> + addr->size = size;
>> + return true;
>> +}
>> +
>> +static void aspeed_video_free_buf(struct aspeed_video *video,
>> + struct aspeed_video_addr *addr)
>> +{
>> + dma_free_coherent(video->dev, addr->size, addr->virt, addr->dma);
>> + addr->size = 0;
>> + addr->dma = 0ULL;
>> + addr->virt = NULL;
>> +}
>> +
>> +/*
>> + * Get the minimum HW-supported compression buffer size for the frame size.
>> + * Assume worst-case JPEG compression size is 1/8 raw size. This should be
>> + * plenty even for maximum quality; any worse and the engine will simply return
>> + * incomplete JPEGs.
>> + */
>> +static void aspeed_video_calc_compressed_size(struct aspeed_video *video)
>> +{
>> + int i, j;
>> + u32 compression_buffer_size_reg = 0;
>> + unsigned int size;
>> + const unsigned int num_compression_packets = 4;
>> + const unsigned int compression_packet_size = 1024;
>> + const unsigned int max_compressed_size =
>> + video->width * video->height / 2; /* 4 Bpp / 8 */
>> +
>> + video->max_compressed_size = UINT_MAX;
>> +
>> + for (i = 0; i < 6; ++i) {
>> + for (j = 0; j < 8; ++j) {
>> + size = (num_compression_packets << i) *
>> + (compression_packet_size << j);
>> + if (size < max_compressed_size)
>> + continue;
>> +
>> + if (size < video->max_compressed_size) {
>> + compression_buffer_size_reg = (i << 3) | j;
>> + video->max_compressed_size = size;
>> + }
>> + }
>> + }
>> +
>> + aspeed_video_write(video, VE_STREAM_BUF_SIZE,
>> + compression_buffer_size_reg);
>> +
>> + dev_dbg(video->dev, "max compressed size: %x\n",
>> + video->max_compressed_size);
>> +}
>> +
>> +#define res_check(v) test_and_clear_bit(VIDEO_MODE_DETECT_DONE, &(v)->flags)
>> +
>> +static int aspeed_video_get_resolution(struct aspeed_video *video)
>> +{
>> + bool invalid_resolution = true;
>> + int rc;
>> + int tries = 0;
>> + unsigned int bottom;
>> + unsigned int left;
>> + unsigned int right;
>> + unsigned int size;
>> + unsigned int top;
>> + u32 mds;
>> + u32 src_lr_edge;
>> + u32 src_tb_edge;
>> + u32 sync;
>> + struct aspeed_video_addr src;
>> +
>> + if (video->srcs[1].size)
>> + aspeed_video_free_buf(video, &video->srcs[1]);
>> +
>> + if (video->srcs[0].size >= VE_MAX_SRC_BUFFER_SIZE) {
>> + src = video->srcs[0];
>> + } else {
>> + if (video->srcs[0].size)
>> + aspeed_video_free_buf(video, &video->srcs[0]);
>> +
>> + if (!aspeed_video_alloc_buf(video, &src,
>> + VE_MAX_SRC_BUFFER_SIZE))
>> + goto err_mem;
>> + }
>> +
>> + aspeed_video_write(video, VE_SRC0_ADDR, src.dma);
>> +
>> + video->width = 0;
>> + video->height = 0;
>> +
>> + do {
>> + if (tries) {
>> + set_current_state(TASK_INTERRUPTIBLE);
>> + if (schedule_timeout(INVALID_RESOLUTION_DELAY))
>> + return -EINTR;
>> + }
>> +
>> + aspeed_video_start_mode_detect(video);
>> +
>> + rc = wait_event_interruptible_timeout(video->wait,
>> + res_check(video),
>> + MODE_DETECT_TIMEOUT);
>> + if (!rc) {
>> + dev_err(video->dev, "timed out on 1st mode detect\n");
>> + aspeed_video_disable_mode_detect(video);
>> + return -ETIMEDOUT;
>> + }
>> +
>> + /* Disable mode detect in order to re-trigger */
>> + aspeed_video_update(video, VE_SEQ_CTRL,
>> + VE_SEQ_CTRL_TRIG_MODE_DET, 0);
>> +
>> + aspeed_video_check_polarity(video);
>> +
>> + aspeed_video_start_mode_detect(video);
>> +
>> + rc = wait_event_interruptible_timeout(video->wait,
>> + res_check(video),
>> + MODE_DETECT_TIMEOUT);
>> + if (!rc) {
>> + dev_err(video->dev, "timed out on 2nd mode detect\n");
>> + aspeed_video_disable_mode_detect(video);
>> + return -ETIMEDOUT;
>> + }
>> +
>> + src_lr_edge = aspeed_video_read(video, VE_SRC_LR_EDGE_DET);
>> + src_tb_edge = aspeed_video_read(video, VE_SRC_TB_EDGE_DET);
>> + mds = aspeed_video_read(video, VE_MODE_DETECT_STATUS);
>> + sync = aspeed_video_read(video, VE_SYNC_STATUS);
>> +
>> + bottom = (src_tb_edge & VE_SRC_TB_EDGE_DET_BOT) >>
>> + VE_SRC_TB_EDGE_DET_BOT_SHF;
>> + top = src_tb_edge & VE_SRC_TB_EDGE_DET_TOP;
>> + video->timings.vfrontporch = top;
>> + video->timings.vbackporch = ((mds & VE_MODE_DETECT_V_LINES) >>
>> + VE_MODE_DETECT_V_LINES_SHF) - bottom;
>> + video->timings.vsync = (sync & VE_SYNC_STATUS_VSYNC) >>
>> + VE_SYNC_STATUS_VSYNC_SHF;
>> + if (top > bottom)
>> + continue;
>> +
>> + right = (src_lr_edge & VE_SRC_LR_EDGE_DET_RT) >>
>> + VE_SRC_LR_EDGE_DET_RT_SHF;
>> + left = src_lr_edge & VE_SRC_LR_EDGE_DET_LEFT;
>> + video->timings.hfrontporch = left;
>> + video->timings.hbackporch = (mds & VE_MODE_DETECT_H_PIXELS) -
>> + right;
>> + video->timings.hsync = sync & VE_SYNC_STATUS_HSYNC;
> It's a bit odd that timings.width/height isn't set here.
>
>> + if (left > right)
>> + continue;
>> +
>> + invalid_resolution = false;
>> + } while (invalid_resolution && (tries++ < INVALID_RESOLUTION_RETRIES));
>> +
>> + if (invalid_resolution) {
>> + dev_err(video->dev, "invalid resolution detected\n");
>> + return -ERANGE;
>> + }
> So far, so good. You now have detected the new resolution, but...
>
>> +
>> + video->height = (bottom - top) + 1;
>> + video->width = (right - left) + 1;
>> + size = video->height * video->width;
> here and below you appear to actually switch the capture timings to the new
> resolution as well.
Hmm. I'm not sure I understand, the video->height and width represent
the actual detected resolution. When a resolution change occurs, the
entire engine is switched off and all buffers are set to error state
(see the irq handler). Then (after a delay to wait for VGA stability)
the engine is switched back on and the new resolution detected.
Regardless of how I keep track of the resolution, the video engine will
return frames of the new size. The driver cannot control the actual
resolution.
If userspace is determined to ignore the source change event, probably
there will be bad memory access regardless of how everything is set up I
would think.
>
> That should only happen when s_dv_timings is called.
>
> It is always very dangerous to automatically switch resolution since a new
> resolution might mean larger buffers, and userspace has to prepare for that.
>
> The normal sequence is:
>
> 1) the driver detects a change in resolution (or the video signal disappears
> completely). It will send V4L2_EVENT_SOURCE_CHANGE to let userspace know.
>
> 2) Userspace stops streaming, frees all buffers, calls QUERY_DV_TIMINGS and,
> if valid, follows with S_DV_TIMINGS.
>
> 3) Userspace calls REQBUFS to allocate the new buffers sized for the new timings
> and starts streaming again.
>
> That way everything remains under control of the application.
Yep, that's exactly how it works now, its just that the driver stops
streaming internally before detecting new resolution.
Thanks,
Eddie
>
>> +
>> + /* Don't use direct mode below 1024 x 768 (irqs don't fire) */
>> + if (size < DIRECT_FETCH_THRESHOLD) {
>> + aspeed_video_write(video, VE_TGS_0,
>> + FIELD_PREP(VE_TGS_FIRST, left - 1) |
>> + FIELD_PREP(VE_TGS_LAST, right));
>> + aspeed_video_write(video, VE_TGS_1,
>> + FIELD_PREP(VE_TGS_FIRST, top) |
>> + FIELD_PREP(VE_TGS_LAST, bottom + 1));
>> + aspeed_video_update(video, VE_CTRL, 0, VE_CTRL_INT_DE);
>> + } else {
>> + aspeed_video_update(video, VE_CTRL, 0, VE_CTRL_DIRECT_FETCH);
>> + }
>> +
>> + aspeed_video_write(video, VE_CAP_WINDOW,
>> + video->width << 16 | video->height);
>> + aspeed_video_write(video, VE_COMP_WINDOW,
>> + video->width << 16 | video->height);
>> + aspeed_video_write(video, VE_SRC_SCANLINE_OFFSET, video->width * 4);
>> +
>> + aspeed_video_update(video, VE_INTERRUPT_CTRL, 0,
>> + VE_INTERRUPT_MODE_DETECT_WD);
>> + aspeed_video_update(video, VE_SEQ_CTRL, 0,
>> + VE_SEQ_CTRL_AUTO_COMP | VE_SEQ_CTRL_EN_WATCHDOG);
>> +
>> + dev_dbg(video->dev, "got resolution[%dx%d]\n", video->width,
>> + video->height);
>> +
>> + size *= 4;
>> + if (size == src.size / 2) {
>> + aspeed_video_write(video, VE_SRC1_ADDR, src.dma + size);
>> + video->srcs[0] = src;
>> + } else if (size == src.size) {
>> + video->srcs[0] = src;
>> +
>> + if (!aspeed_video_alloc_buf(video, &video->srcs[1], size))
>> + goto err_mem;
>> +
>> + aspeed_video_write(video, VE_SRC1_ADDR, video->srcs[1].dma);
>> + } else {
>> + aspeed_video_free_buf(video, &src);
>> +
>> + if (!aspeed_video_alloc_buf(video, &video->srcs[0], size))
>> + goto err_mem;
>> +
>> + if (!aspeed_video_alloc_buf(video, &video->srcs[1], size))
>> + goto err_mem;
>> +
>> + aspeed_video_write(video, VE_SRC0_ADDR, video->srcs[0].dma);
>> + aspeed_video_write(video, VE_SRC1_ADDR, video->srcs[1].dma);
>> + }
>> +
>> + aspeed_video_calc_compressed_size(video);
>> +
>> + return 0;
>> +
>> +err_mem:
>> + dev_err(video->dev, "failed to allocate source buffers\n");
>> +
>> + if (video->srcs[0].size)
>> + aspeed_video_free_buf(video, &video->srcs[0]);
>> +
>> + return -ENOMEM;
>> +}
> Regards,
>
> Hans
>
^ permalink raw reply
* [PATCH v4 2/2] media: platform: Add Aspeed Video Engine driver
From: Eddie James @ 2018-10-19 16:39 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <bf07eb1f-bc17-ac59-d341-f19e2ab0c2e2@xs4all.nl>
On 10/12/2018 07:22 AM, Hans Verkuil wrote:
> On 10/05/2018 09:57 PM, Eddie James wrote:
>> The Video Engine (VE) embedded in the Aspeed AST2400 and AST2500 SOCs
>> can capture and compress video data from digital or analog sources. With
>> the Aspeed chip acting a service processor, the Video Engine can capture
>> the host processor graphics output.
>>
>> Add a V4L2 driver to capture video data and compress it to JPEG images.
>> Make the video frames available through the V4L2 streaming interface.
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>> MAINTAINERS | 8 +
>> drivers/media/platform/Kconfig | 8 +
>> drivers/media/platform/Makefile | 1 +
>> drivers/media/platform/aspeed-video.c | 1674 +++++++++++++++++++++++++++++++++
>> 4 files changed, 1691 insertions(+)
>> create mode 100644 drivers/media/platform/aspeed-video.c
>>
> <snip>
>
>> +static int aspeed_video_enum_input(struct file *file, void *fh,
>> + struct v4l2_input *inp)
>> +{
>> + if (inp->index)
>> + return -EINVAL;
>> +
>> + strscpy(inp->name, "Host VGA capture", sizeof(inp->name));
>> + inp->type = V4L2_INPUT_TYPE_CAMERA;
>> + inp->capabilities = V4L2_IN_CAP_DV_TIMINGS;
>> + inp->status = V4L2_IN_ST_NO_SIGNAL | V4L2_IN_ST_NO_SYNC;
> This can't be right. If there is a valid signal, then status should be 0.
> And ideally you can tell the difference between no signal and no sync
> as well.
Ah, yes I misunderstood the status, thanks.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_get_input(struct file *file, void *fh, unsigned int *i)
>> +{
>> + *i = 0;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_set_input(struct file *file, void *fh, unsigned int i)
>> +{
>> + if (i)
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_get_parm(struct file *file, void *fh,
>> + struct v4l2_streamparm *a)
>> +{
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + a->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
>> + a->parm.capture.readbuffers = 3;
>> + a->parm.capture.timeperframe.numerator = 1;
>> + if (!video->frame_rate)
>> + a->parm.capture.timeperframe.denominator = MAX_FRAME_RATE + 1;
>> + else
>> + a->parm.capture.timeperframe.denominator = video->frame_rate;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_set_parm(struct file *file, void *fh,
>> + struct v4l2_streamparm *a)
>> +{
>> + unsigned int frame_rate = 0;
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + a->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
>> + a->parm.capture.readbuffers = 3;
>> +
>> + if (a->parm.capture.timeperframe.numerator)
>> + frame_rate = a->parm.capture.timeperframe.denominator /
>> + a->parm.capture.timeperframe.numerator;
>> +
>> + if (!frame_rate || frame_rate > MAX_FRAME_RATE) {
>> + frame_rate = 0;
>> +
>> + /*
>> + * Set to max + 1 to differentiate between max and 0, which
>> + * means "don't care".
> But what does "don't care" mean in practice? It's still not clear to me how this
> is supposed to work.
>
>> + */
>> + a->parm.capture.timeperframe.denominator = MAX_FRAME_RATE + 1;
> And regardless of anything else this timeperframe is out of the range that
> aspeed_video_enum_frameintervals() returns.
>
>> + a->parm.capture.timeperframe.numerator = 1;
>> + }
>> +
>> + if (video->frame_rate != frame_rate) {
>> + video->frame_rate = frame_rate;
>> + aspeed_video_update(video, VE_CTRL, VE_CTRL_FRC,
>> + FIELD_PREP(VE_CTRL_FRC, frame_rate));
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_enum_framesizes(struct file *file, void *fh,
>> + struct v4l2_frmsizeenum *fsize)
>> +{
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + if (fsize->index)
>> + return -EINVAL;
>> +
>> + if (fsize->pixel_format != V4L2_PIX_FMT_JPEG)
>> + return -EINVAL;
>> +
>> + fsize->discrete.width = video->pix_fmt.width;
>> + fsize->discrete.height = video->pix_fmt.height;
>> + fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_enum_frameintervals(struct file *file, void *fh,
>> + struct v4l2_frmivalenum *fival)
>> +{
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + if (fival->index)
>> + return -EINVAL;
>> +
>> + if (fival->width != video->width || fival->height != video->height)
>> + return -EINVAL;
>> +
>> + if (fival->pixel_format != V4L2_PIX_FMT_JPEG)
>> + return -EINVAL;
>> +
>> + fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
>> +
>> + fival->stepwise.min.denominator = MAX_FRAME_RATE;
>> + fival->stepwise.min.numerator = 1;
>> + fival->stepwise.max.denominator = 1;
>> + fival->stepwise.max.numerator = 1;
>> + fival->stepwise.step = fival->stepwise.max;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_set_dv_timings(struct file *file, void *fh,
>> + struct v4l2_dv_timings *timings)
>> +{
>> + struct aspeed_video *video = video_drvdata(file);
>> +
> If vb2_is_busy() returns true, then return -EBUSY here. It is not allowed to
> set the timings while vb2 is busy.
>
>> + if (video->width != timings->bt.width ||
>> + video->height != timings->bt.height)
>> + return -EINVAL;
>> +
>> + video->pix_fmt.width = timings->bt.width;
>> + video->pix_fmt.height = timings->bt.height;
>> + video->pix_fmt.sizeimage = video->max_compressed_size;
>> + video->timings.width = timings->bt.width;
>> + video->timings.height = timings->bt.height;
>> +
>> + timings->type = V4L2_DV_BT_656_1120;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_get_dv_timings(struct file *file, void *fh,
>> + struct v4l2_dv_timings *timings)
>> +{
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + timings->type = V4L2_DV_BT_656_1120;
>> + timings->bt = video->timings;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_query_dv_timings(struct file *file, void *fh,
>> + struct v4l2_dv_timings *timings)
>> +{
>> + int rc;
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + if (file->f_flags & O_NONBLOCK) {
>> + if (test_bit(VIDEO_RES_CHANGE, &video->flags))
>> + return -EAGAIN;
>> + } else {
>> + rc = wait_event_interruptible(video->wait,
>> + !test_bit(VIDEO_RES_CHANGE,
>> + &video->flags));
>> + if (rc)
>> + return -EINTR;
>> + }
>> +
>> + timings->type = V4L2_DV_BT_656_1120;
>> + timings->bt = video->timings;
>> + timings->bt.width = video->width;
>> + timings->bt.height = video->height;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_enum_dv_timings(struct file *file, void *fh,
>> + struct v4l2_enum_dv_timings *timings)
>> +{
>> + if (timings->index)
>> + return -EINVAL;
>> +
>> + return aspeed_video_get_dv_timings(file, fh, &timings->timings);
>> +}
>> +
>> +static int aspeed_video_dv_timings_cap(struct file *file, void *fh,
>> + struct v4l2_dv_timings_cap *cap)
>> +{
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + cap->type = V4L2_DV_BT_656_1120;
>> + cap->bt.capabilities = V4L2_DV_BT_CAP_PROGRESSIVE;
>> + cap->bt.min_width = video->width;
>> + cap->bt.max_width = video->width;
>> + cap->bt.min_height = video->height;
>> + cap->bt.max_height = video->height;
> This should return the capabilities of the aspeed. In this case I'd
> guess that the max width/height is 1920x1080 (or perhaps 1200).
>
> The minimum is probably the VGA resolution.
OK that works, but keep in mind a call to s_dv_timings must return
EINVAL if the timings resolution doesn't match the detected
resolution... driver can't change the capture resolution.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_sub_event(struct v4l2_fh *fh,
>> + const struct v4l2_event_subscription *sub)
>> +{
>> + switch (sub->type) {
>> + case V4L2_EVENT_SOURCE_CHANGE:
>> + return v4l2_src_change_event_subscribe(fh, sub);
>> + }
>> +
>> + return v4l2_ctrl_subscribe_event(fh, sub);
>> +}
>> +
>> +static const struct v4l2_ioctl_ops aspeed_video_ioctl_ops = {
>> + .vidioc_querycap = aspeed_video_querycap,
>> +
>> + .vidioc_enum_fmt_vid_cap = aspeed_video_enum_format,
>> + .vidioc_g_fmt_vid_cap = aspeed_video_get_format,
>> + .vidioc_s_fmt_vid_cap = aspeed_video_get_format,
>> + .vidioc_try_fmt_vid_cap = aspeed_video_get_format,
>> +
>> + .vidioc_reqbufs = vb2_ioctl_reqbufs,
>> + .vidioc_querybuf = vb2_ioctl_querybuf,
>> + .vidioc_qbuf = vb2_ioctl_qbuf,
>> + .vidioc_dqbuf = vb2_ioctl_dqbuf,
>> + .vidioc_create_bufs = vb2_ioctl_create_bufs,
>> + .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
>> + .vidioc_streamon = vb2_ioctl_streamon,
>> + .vidioc_streamoff = vb2_ioctl_streamoff,
>> +
>> + .vidioc_enum_input = aspeed_video_enum_input,
>> + .vidioc_g_input = aspeed_video_get_input,
>> + .vidioc_s_input = aspeed_video_set_input,
>> +
>> + .vidioc_g_parm = aspeed_video_get_parm,
>> + .vidioc_s_parm = aspeed_video_set_parm,
>> + .vidioc_enum_framesizes = aspeed_video_enum_framesizes,
>> + .vidioc_enum_frameintervals = aspeed_video_enum_frameintervals,
>> +
>> + .vidioc_s_dv_timings = aspeed_video_set_dv_timings,
>> + .vidioc_g_dv_timings = aspeed_video_get_dv_timings,
>> + .vidioc_query_dv_timings = aspeed_video_query_dv_timings,
>> + .vidioc_enum_dv_timings = aspeed_video_enum_dv_timings,
>> + .vidioc_dv_timings_cap = aspeed_video_dv_timings_cap,
>> +
>> + .vidioc_subscribe_event = aspeed_video_sub_event,
>> + .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
>> +};
>> +
>> +static void aspeed_video_update_jpeg_quality(struct aspeed_video *video)
>> +{
>> + u32 comp_ctrl = FIELD_PREP(VE_COMP_CTRL_DCT_LUM, video->jpeg_quality) |
>> + FIELD_PREP(VE_COMP_CTRL_DCT_CHR, video->jpeg_quality | 0x10);
>> +
>> + aspeed_video_update(video, VE_COMP_CTRL,
>> + VE_COMP_CTRL_DCT_LUM | VE_COMP_CTRL_DCT_CHR,
>> + comp_ctrl);
>> +}
>> +
>> +static void aspeed_video_update_subsampling(struct aspeed_video *video)
>> +{
>> + if (video->jpeg.virt)
>> + aspeed_video_init_jpeg_table(video->jpeg.virt, video->yuv420);
>> +
>> + if (video->yuv420)
>> + aspeed_video_update(video, VE_SEQ_CTRL, 0, VE_SEQ_CTRL_YUV420);
>> + else
>> + aspeed_video_update(video, VE_SEQ_CTRL, VE_SEQ_CTRL_YUV420, 0);
>> +}
>> +
>> +static int aspeed_video_set_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> + struct aspeed_video *video = container_of(ctrl->handler,
>> + struct aspeed_video,
>> + ctrl_handler);
>> +
>> + switch (ctrl->id) {
>> + case V4L2_CID_JPEG_COMPRESSION_QUALITY:
>> + video->jpeg_quality = ctrl->val;
>> + aspeed_video_update_jpeg_quality(video);
>> + break;
>> + case V4L2_CID_JPEG_CHROMA_SUBSAMPLING:
>> + if (ctrl->val == V4L2_JPEG_CHROMA_SUBSAMPLING_420) {
>> + video->yuv420 = true;
>> + aspeed_video_update_subsampling(video);
>> + } else {
>> + video->yuv420 = false;
>> + aspeed_video_update_subsampling(video);
>> + }
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct v4l2_ctrl_ops aspeed_video_ctrl_ops = {
>> + .s_ctrl = aspeed_video_set_ctrl,
>> +};
>> +
>> +static void aspeed_video_resolution_work(struct work_struct *work)
>> +{
>> + int rc;
>> + struct delayed_work *dwork = to_delayed_work(work);
>> + struct aspeed_video *video = container_of(dwork, struct aspeed_video,
>> + res_work);
>> +
>> + /* No clients remaining after delay */
>> + if (atomic_read(&video->clients) == 0)
>> + goto done;
>> +
>> + aspeed_video_on(video);
>> +
>> + aspeed_video_init_regs(video);
>> +
>> + rc = aspeed_video_get_resolution(video);
>> + if (rc)
>> + dev_err(video->dev,
>> + "resolution changed; couldn't get new resolution\n");
>> + else if (test_bit(VIDEO_STREAMING, &video->flags))
>> + aspeed_video_start_frame(video);
>> +
>> + if (video->width != video->pix_fmt.width ||
>> + video->height != video->pix_fmt.height) {
>> + static const struct v4l2_event ev = {
>> + .type = V4L2_EVENT_SOURCE_CHANGE,
>> + .u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION,
>> + };
>> +
>> + v4l2_event_queue(&video->vdev, &ev);
>> + }
>> +
>> +done:
>> + clear_bit(VIDEO_RES_CHANGE, &video->flags);
>> + wake_up_interruptible_all(&video->wait);
>> +}
>> +
>> +static int aspeed_video_open(struct file *file)
>> +{
>> + int rc;
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + mutex_lock(&video->video_lock);
>> +
>> + if (atomic_inc_return(&video->clients) == 1) {
>> + rc = aspeed_video_start(video);
>> + if (rc) {
>> + dev_err(video->dev, "Failed to start video engine\n");
>> + atomic_dec(&video->clients);
>> + mutex_unlock(&video->video_lock);
>> + return rc;
>> + }
>> + }
>> +
>> + mutex_unlock(&video->video_lock);
>> +
>> + return v4l2_fh_open(file);
>> +}
>> +
>> +static int aspeed_video_release(struct file *file)
>> +{
>> + int rc;
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + rc = vb2_fop_release(file);
>> +
>> + mutex_lock(&video->video_lock);
>> +
>> + if (atomic_dec_return(&video->clients) == 0)
>> + aspeed_video_stop(video);
>> +
>> + mutex_unlock(&video->video_lock);
>> +
>> + return rc;
>> +}
>> +
>> +static const struct v4l2_file_operations aspeed_video_v4l2_fops = {
>> + .owner = THIS_MODULE,
>> + .read = vb2_fop_read,
>> + .poll = vb2_fop_poll,
>> + .unlocked_ioctl = video_ioctl2,
>> + .mmap = vb2_fop_mmap,
>> + .open = aspeed_video_open,
>> + .release = aspeed_video_release,
>> +};
>> +
>> +static int aspeed_video_queue_setup(struct vb2_queue *q,
>> + unsigned int *num_buffers,
>> + unsigned int *num_planes,
>> + unsigned int sizes[],
>> + struct device *alloc_devs[])
>> +{
>> + struct aspeed_video *video = vb2_get_drv_priv(q);
>> +
>> + if (*num_planes) {
>> + if (sizes[0] < video->max_compressed_size)
>> + return -EINVAL;
>> +
>> + return 0;
>> + }
>> +
>> + *num_planes = 1;
>> + sizes[0] = video->max_compressed_size;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_buf_prepare(struct vb2_buffer *vb)
>> +{
>> + struct aspeed_video *video = vb2_get_drv_priv(vb->vb2_queue);
>> +
>> + if (vb2_plane_size(vb, 0) < video->max_compressed_size)
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_start_streaming(struct vb2_queue *q,
>> + unsigned int count)
>> +{
>> + int rc;
>> + struct aspeed_video *video = vb2_get_drv_priv(q);
>> +
>> + rc = aspeed_video_start_frame(video);
>> + if (rc) {
>> + aspeed_video_bufs_done(video, VB2_BUF_STATE_QUEUED);
>> + return rc;
>> + }
>> +
>> + video->sequence = 0;
>> + set_bit(VIDEO_STREAMING, &video->flags);
>> + return 0;
>> +}
>> +
>> +static void aspeed_video_stop_streaming(struct vb2_queue *q)
>> +{
>> + int rc;
>> + struct aspeed_video *video = vb2_get_drv_priv(q);
>> +
>> + clear_bit(VIDEO_STREAMING, &video->flags);
>> +
>> + rc = wait_event_timeout(video->wait,
>> + !test_bit(VIDEO_FRAME_INPRG, &video->flags),
>> + STOP_TIMEOUT);
>> + if (!rc) {
>> + dev_err(video->dev, "Timed out when stopping streaming\n");
>> + aspeed_video_stop(video);
>> + }
>> +
>> + aspeed_video_bufs_done(video, VB2_BUF_STATE_ERROR);
>> +}
>> +
>> +static void aspeed_video_buf_queue(struct vb2_buffer *vb)
>> +{
>> + struct aspeed_video *video = vb2_get_drv_priv(vb->vb2_queue);
>> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> + struct aspeed_video_buffer *avb = to_aspeed_video_buffer(vbuf);
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&video->lock, flags);
>> + list_add_tail(&avb->link, &video->buffers);
>> + spin_unlock_irqrestore(&video->lock, flags);
>> +}
>> +
>> +static const struct vb2_ops aspeed_video_vb2_ops = {
>> + .queue_setup = aspeed_video_queue_setup,
>> + .wait_prepare = vb2_ops_wait_prepare,
>> + .wait_finish = vb2_ops_wait_finish,
>> + .buf_prepare = aspeed_video_buf_prepare,
>> + .start_streaming = aspeed_video_start_streaming,
>> + .stop_streaming = aspeed_video_stop_streaming,
>> + .buf_queue = aspeed_video_buf_queue,
>> +};
>> +
>> +static int aspeed_video_setup_video(struct aspeed_video *video)
>> +{
>> + const u64 mask = ~(BIT(V4L2_JPEG_CHROMA_SUBSAMPLING_444) |
>> + BIT(V4L2_JPEG_CHROMA_SUBSAMPLING_420));
>> + struct v4l2_device *v4l2_dev = &video->v4l2_dev;
>> + struct vb2_queue *vbq = &video->queue;
>> + struct video_device *vdev = &video->vdev;
>> + int rc;
>> +
>> + video->pix_fmt.pixelformat = V4L2_PIX_FMT_JPEG;
>> + video->pix_fmt.field = V4L2_FIELD_NONE;
>> + video->pix_fmt.colorspace = V4L2_COLORSPACE_SRGB;
>> + video->pix_fmt.quantization = V4L2_QUANTIZATION_FULL_RANGE;
>> +
>> + rc = v4l2_device_register(video->dev, v4l2_dev);
>> + if (rc) {
>> + dev_err(video->dev, "Failed to register v4l2 device\n");
>> + return rc;
>> + }
>> +
>> + v4l2_ctrl_handler_init(&video->ctrl_handler, 2);
>> + v4l2_ctrl_new_std(&video->ctrl_handler, &aspeed_video_ctrl_ops,
>> + V4L2_CID_JPEG_COMPRESSION_QUALITY, 0,
>> + ASPEED_VIDEO_JPEG_NUM_QUALITIES - 1, 1, 0);
>> + v4l2_ctrl_new_std_menu(&video->ctrl_handler, &aspeed_video_ctrl_ops,
>> + V4L2_CID_JPEG_CHROMA_SUBSAMPLING,
>> + V4L2_JPEG_CHROMA_SUBSAMPLING_420, mask,
>> + V4L2_JPEG_CHROMA_SUBSAMPLING_444);
>> +
>> + if (video->ctrl_handler.error) {
>> + v4l2_ctrl_handler_free(&video->ctrl_handler);
>> + v4l2_device_unregister(v4l2_dev);
>> +
>> + dev_err(video->dev, "Failed to init controls: %d\n",
>> + video->ctrl_handler.error);
>> + return rc;
>> + }
>> +
>> + v4l2_dev->ctrl_handler = &video->ctrl_handler;
>> +
>> + vbq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> + vbq->io_modes = VB2_MMAP | VB2_READ | VB2_DMABUF;
>> + vbq->dev = v4l2_dev->dev;
>> + vbq->lock = &video->video_lock;
>> + vbq->ops = &aspeed_video_vb2_ops;
>> + vbq->mem_ops = &vb2_dma_contig_memops;
>> + vbq->drv_priv = video;
>> + vbq->buf_struct_size = sizeof(struct aspeed_video_buffer);
>> + vbq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>> + vbq->min_buffers_needed = 3;
>> +
>> + rc = vb2_queue_init(vbq);
>> + if (rc) {
>> + v4l2_ctrl_handler_free(&video->ctrl_handler);
>> + v4l2_device_unregister(v4l2_dev);
>> +
>> + dev_err(video->dev, "Failed to init vb2 queue\n");
>> + return rc;
>> + }
>> +
>> + vdev->queue = vbq;
>> + vdev->fops = &aspeed_video_v4l2_fops;
>> + vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_READWRITE |
> READWRITE doesn't work for JPEG since there is no clear end of a frame. Just drop
> this and the read op in aspeed_video_v4l2_fops.
I don't follow. The buffer will have the payload set correctly, defining
the end of the frame, right? I haven't tested the read interface now but
looking at the vb2_fop_read I think it would work?
Thanks,
Eddie
>
>> + V4L2_CAP_STREAMING;
>> + vdev->v4l2_dev = v4l2_dev;
>> + strscpy(vdev->name, DEVICE_NAME, sizeof(vdev->name));
>> + vdev->vfl_type = VFL_TYPE_GRABBER;
>> + vdev->vfl_dir = VFL_DIR_RX;
>> + vdev->release = video_device_release_empty;
>> + vdev->ioctl_ops = &aspeed_video_ioctl_ops;
>> + vdev->lock = &video->video_lock;
>> +
>> + video_set_drvdata(vdev, video);
>> + rc = video_register_device(vdev, VFL_TYPE_GRABBER, 0);
>> + if (rc) {
>> + vb2_queue_release(vbq);
>> + v4l2_ctrl_handler_free(&video->ctrl_handler);
>> + v4l2_device_unregister(v4l2_dev);
>> +
>> + dev_err(video->dev, "Failed to register video device\n");
>> + return rc;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_init(struct aspeed_video *video)
>> +{
>> + int irq;
>> + int rc;
>> + struct device *dev = video->dev;
>> +
>> + irq = irq_of_parse_and_map(dev->of_node, 0);
>> + if (!irq) {
>> + dev_err(dev, "Unable to find IRQ\n");
>> + return -ENODEV;
>> + }
>> +
>> + rc = devm_request_irq(dev, irq, aspeed_video_irq, IRQF_SHARED,
>> + DEVICE_NAME, video);
>> + if (rc < 0) {
>> + dev_err(dev, "Unable to request IRQ %d\n", irq);
>> + return rc;
>> + }
>> +
>> + video->eclk = devm_clk_get(dev, "eclk");
>> + if (IS_ERR(video->eclk)) {
>> + dev_err(dev, "Unable to get ECLK\n");
>> + return PTR_ERR(video->eclk);
>> + }
>> +
>> + video->vclk = devm_clk_get(dev, "vclk");
>> + if (IS_ERR(video->vclk)) {
>> + dev_err(dev, "Unable to get VCLK\n");
>> + return PTR_ERR(video->vclk);
>> + }
>> +
>> + video->rst = devm_reset_control_get_exclusive(dev, NULL);
>> + if (IS_ERR(video->rst)) {
>> + dev_err(dev, "Unable to get VE reset\n");
>> + return PTR_ERR(video->rst);
>> + }
>> +
>> + rc = of_reserved_mem_device_init(dev);
>> + if (rc) {
>> + dev_err(dev, "Unable to reserve memory\n");
>> + return rc;
>> + }
>> +
>> + rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>> + if (rc) {
>> + dev_err(dev, "Failed to set DMA mask\n");
>> + of_reserved_mem_device_release(dev);
>> + return rc;
>> + }
>> +
>> + if (!aspeed_video_alloc_buf(video, &video->jpeg,
>> + VE_JPEG_HEADER_SIZE)) {
>> + dev_err(dev, "Failed to allocate DMA for JPEG header\n");
>> + of_reserved_mem_device_release(dev);
>> + return rc;
>> + }
>> +
>> + aspeed_video_init_jpeg_table(video->jpeg.virt, video->yuv420);
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_probe(struct platform_device *pdev)
>> +{
>> + int rc;
>> + struct resource *res;
>> + struct aspeed_video *video = kzalloc(sizeof(*video), GFP_KERNEL);
>> +
>> + if (!video)
>> + return -ENOMEM;
>> +
>> + video->frame_rate = 30;
>> + video->dev = &pdev->dev;
>> + mutex_init(&video->video_lock);
>> + init_waitqueue_head(&video->wait);
>> + INIT_DELAYED_WORK(&video->res_work, aspeed_video_resolution_work);
>> + INIT_LIST_HEAD(&video->buffers);
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> + video->base = devm_ioremap_resource(video->dev, res);
>> +
>> + if (IS_ERR(video->base))
>> + return PTR_ERR(video->base);
>> +
>> + rc = aspeed_video_init(video);
>> + if (rc)
>> + return rc;
>> +
>> + rc = aspeed_video_setup_video(video);
>> + if (rc)
>> + return rc;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_remove(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct v4l2_device *v4l2_dev = dev_get_drvdata(dev);
>> + struct aspeed_video *video = to_aspeed_video(v4l2_dev);
>> +
>> + video_unregister_device(&video->vdev);
>> +
>> + vb2_queue_release(&video->queue);
>> +
>> + v4l2_ctrl_handler_free(&video->ctrl_handler);
>> +
>> + v4l2_device_unregister(v4l2_dev);
>> +
>> + dma_free_coherent(video->dev, VE_JPEG_HEADER_SIZE, video->jpeg.virt,
>> + video->jpeg.dma);
>> +
>> + of_reserved_mem_device_release(dev);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id aspeed_video_of_match[] = {
>> + { .compatible = "aspeed,ast2400-video-engine" },
>> + { .compatible = "aspeed,ast2500-video-engine" },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, aspeed_video_of_match);
>> +
>> +static struct platform_driver aspeed_video_driver = {
>> + .driver = {
>> + .name = DEVICE_NAME,
>> + .of_match_table = aspeed_video_of_match,
>> + },
>> + .probe = aspeed_video_probe,
>> + .remove = aspeed_video_remove,
>> +};
>> +
>> +module_platform_driver(aspeed_video_driver);
>> +
>> +MODULE_DESCRIPTION("ASPEED Video Engine Driver");
>> +MODULE_AUTHOR("Eddie James");
>> +MODULE_LICENSE("GPL v2");
>>
> Regards,
>
> Hans
>
^ permalink raw reply
* [PATCH v4 2/2] media: platform: Add Aspeed Video Engine driver
From: Eddie James @ 2018-10-19 20:26 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <bf07eb1f-bc17-ac59-d341-f19e2ab0c2e2@xs4all.nl>
On 10/12/2018 07:22 AM, Hans Verkuil wrote:
> On 10/05/2018 09:57 PM, Eddie James wrote:
>> The Video Engine (VE) embedded in the Aspeed AST2400 and AST2500 SOCs
>> can capture and compress video data from digital or analog sources. With
>> the Aspeed chip acting a service processor, the Video Engine can capture
>> the host processor graphics output.
>>
>> Add a V4L2 driver to capture video data and compress it to JPEG images.
>> Make the video frames available through the V4L2 streaming interface.
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>> MAINTAINERS | 8 +
>> drivers/media/platform/Kconfig | 8 +
>> drivers/media/platform/Makefile | 1 +
>> drivers/media/platform/aspeed-video.c | 1674 +++++++++++++++++++++++++++++++++
>> 4 files changed, 1691 insertions(+)
>> create mode 100644 drivers/media/platform/aspeed-video.c
>>
> <snip>
>
>> +static int aspeed_video_enum_input(struct file *file, void *fh,
>> + struct v4l2_input *inp)
>> +{
>> + if (inp->index)
>> + return -EINVAL;
>> +
>> + strscpy(inp->name, "Host VGA capture", sizeof(inp->name));
>> + inp->type = V4L2_INPUT_TYPE_CAMERA;
>> + inp->capabilities = V4L2_IN_CAP_DV_TIMINGS;
>> + inp->status = V4L2_IN_ST_NO_SIGNAL | V4L2_IN_ST_NO_SYNC;
> This can't be right. If there is a valid signal, then status should be 0.
> And ideally you can tell the difference between no signal and no sync
> as well.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_get_input(struct file *file, void *fh, unsigned int *i)
>> +{
>> + *i = 0;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_set_input(struct file *file, void *fh, unsigned int i)
>> +{
>> + if (i)
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_get_parm(struct file *file, void *fh,
>> + struct v4l2_streamparm *a)
>> +{
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + a->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
>> + a->parm.capture.readbuffers = 3;
>> + a->parm.capture.timeperframe.numerator = 1;
>> + if (!video->frame_rate)
>> + a->parm.capture.timeperframe.denominator = MAX_FRAME_RATE + 1;
>> + else
>> + a->parm.capture.timeperframe.denominator = video->frame_rate;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_set_parm(struct file *file, void *fh,
>> + struct v4l2_streamparm *a)
>> +{
>> + unsigned int frame_rate = 0;
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + a->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
>> + a->parm.capture.readbuffers = 3;
>> +
>> + if (a->parm.capture.timeperframe.numerator)
>> + frame_rate = a->parm.capture.timeperframe.denominator /
>> + a->parm.capture.timeperframe.numerator;
>> +
>> + if (!frame_rate || frame_rate > MAX_FRAME_RATE) {
>> + frame_rate = 0;
>> +
>> + /*
>> + * Set to max + 1 to differentiate between max and 0, which
>> + * means "don't care".
> But what does "don't care" mean in practice? It's still not clear to me how this
> is supposed to work.
>
>> + */
>> + a->parm.capture.timeperframe.denominator = MAX_FRAME_RATE + 1;
> And regardless of anything else this timeperframe is out of the range that
> aspeed_video_enum_frameintervals() returns.
>
>> + a->parm.capture.timeperframe.numerator = 1;
>> + }
>> +
>> + if (video->frame_rate != frame_rate) {
>> + video->frame_rate = frame_rate;
>> + aspeed_video_update(video, VE_CTRL, VE_CTRL_FRC,
>> + FIELD_PREP(VE_CTRL_FRC, frame_rate));
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_enum_framesizes(struct file *file, void *fh,
>> + struct v4l2_frmsizeenum *fsize)
>> +{
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + if (fsize->index)
>> + return -EINVAL;
>> +
>> + if (fsize->pixel_format != V4L2_PIX_FMT_JPEG)
>> + return -EINVAL;
>> +
>> + fsize->discrete.width = video->pix_fmt.width;
>> + fsize->discrete.height = video->pix_fmt.height;
>> + fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_enum_frameintervals(struct file *file, void *fh,
>> + struct v4l2_frmivalenum *fival)
>> +{
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + if (fival->index)
>> + return -EINVAL;
>> +
>> + if (fival->width != video->width || fival->height != video->height)
>> + return -EINVAL;
>> +
>> + if (fival->pixel_format != V4L2_PIX_FMT_JPEG)
>> + return -EINVAL;
>> +
>> + fival->type = V4L2_FRMIVAL_TYPE_CONTINUOUS;
>> +
>> + fival->stepwise.min.denominator = MAX_FRAME_RATE;
>> + fival->stepwise.min.numerator = 1;
>> + fival->stepwise.max.denominator = 1;
>> + fival->stepwise.max.numerator = 1;
>> + fival->stepwise.step = fival->stepwise.max;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_set_dv_timings(struct file *file, void *fh,
>> + struct v4l2_dv_timings *timings)
>> +{
>> + struct aspeed_video *video = video_drvdata(file);
>> +
> If vb2_is_busy() returns true, then return -EBUSY here. It is not allowed to
> set the timings while vb2 is busy.
Buffer ioctls (Input 0):
??? ??? fail: v4l2-test-buffers.cpp(344): node->s_dv_timings(timings)
??? ??? fail: v4l2-test-buffers.cpp(452): testCanSetSameTimings(node)
??? test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL
Streaming ioctls:
??? test read/write: OK
??? test blocking wait: OK
??? ??? fail: v4l2-test-buffers.cpp(344): node->s_dv_timings(timings)
??? ??? fail: v4l2-test-buffers.cpp(637): testCanSetSameTimings(node)
??? ??? fail: v4l2-test-buffers.cpp(952): captureBufs(node, q, m2m_q,
frame_count, false)
??? test MMAP: FAIL
Built from v4l-utils c36dbbdfa8b30b2badd4f893b59d0bd4f0bd12aa
Adding this causes some of the v4l2-compliance streaming tests to fail,
and prevents my own application from being able to call S_DV_TIMINGS
after detecting a resolution change, despite calling streamoff and
unmapping all the buffers first.
Thanks,
Eddie
>
>> + if (video->width != timings->bt.width ||
>> + video->height != timings->bt.height)
>> + return -EINVAL;
>> +
>> + video->pix_fmt.width = timings->bt.width;
>> + video->pix_fmt.height = timings->bt.height;
>> + video->pix_fmt.sizeimage = video->max_compressed_size;
>> + video->timings.width = timings->bt.width;
>> + video->timings.height = timings->bt.height;
>> +
>> + timings->type = V4L2_DV_BT_656_1120;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_get_dv_timings(struct file *file, void *fh,
>> + struct v4l2_dv_timings *timings)
>> +{
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + timings->type = V4L2_DV_BT_656_1120;
>> + timings->bt = video->timings;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_query_dv_timings(struct file *file, void *fh,
>> + struct v4l2_dv_timings *timings)
>> +{
>> + int rc;
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + if (file->f_flags & O_NONBLOCK) {
>> + if (test_bit(VIDEO_RES_CHANGE, &video->flags))
>> + return -EAGAIN;
>> + } else {
>> + rc = wait_event_interruptible(video->wait,
>> + !test_bit(VIDEO_RES_CHANGE,
>> + &video->flags));
>> + if (rc)
>> + return -EINTR;
>> + }
>> +
>> + timings->type = V4L2_DV_BT_656_1120;
>> + timings->bt = video->timings;
>> + timings->bt.width = video->width;
>> + timings->bt.height = video->height;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_enum_dv_timings(struct file *file, void *fh,
>> + struct v4l2_enum_dv_timings *timings)
>> +{
>> + if (timings->index)
>> + return -EINVAL;
>> +
>> + return aspeed_video_get_dv_timings(file, fh, &timings->timings);
>> +}
>> +
>> +static int aspeed_video_dv_timings_cap(struct file *file, void *fh,
>> + struct v4l2_dv_timings_cap *cap)
>> +{
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + cap->type = V4L2_DV_BT_656_1120;
>> + cap->bt.capabilities = V4L2_DV_BT_CAP_PROGRESSIVE;
>> + cap->bt.min_width = video->width;
>> + cap->bt.max_width = video->width;
>> + cap->bt.min_height = video->height;
>> + cap->bt.max_height = video->height;
> This should return the capabilities of the aspeed. In this case I'd
> guess that the max width/height is 1920x1080 (or perhaps 1200).
>
> The minimum is probably the VGA resolution.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_sub_event(struct v4l2_fh *fh,
>> + const struct v4l2_event_subscription *sub)
>> +{
>> + switch (sub->type) {
>> + case V4L2_EVENT_SOURCE_CHANGE:
>> + return v4l2_src_change_event_subscribe(fh, sub);
>> + }
>> +
>> + return v4l2_ctrl_subscribe_event(fh, sub);
>> +}
>> +
>> +static const struct v4l2_ioctl_ops aspeed_video_ioctl_ops = {
>> + .vidioc_querycap = aspeed_video_querycap,
>> +
>> + .vidioc_enum_fmt_vid_cap = aspeed_video_enum_format,
>> + .vidioc_g_fmt_vid_cap = aspeed_video_get_format,
>> + .vidioc_s_fmt_vid_cap = aspeed_video_get_format,
>> + .vidioc_try_fmt_vid_cap = aspeed_video_get_format,
>> +
>> + .vidioc_reqbufs = vb2_ioctl_reqbufs,
>> + .vidioc_querybuf = vb2_ioctl_querybuf,
>> + .vidioc_qbuf = vb2_ioctl_qbuf,
>> + .vidioc_dqbuf = vb2_ioctl_dqbuf,
>> + .vidioc_create_bufs = vb2_ioctl_create_bufs,
>> + .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
>> + .vidioc_streamon = vb2_ioctl_streamon,
>> + .vidioc_streamoff = vb2_ioctl_streamoff,
>> +
>> + .vidioc_enum_input = aspeed_video_enum_input,
>> + .vidioc_g_input = aspeed_video_get_input,
>> + .vidioc_s_input = aspeed_video_set_input,
>> +
>> + .vidioc_g_parm = aspeed_video_get_parm,
>> + .vidioc_s_parm = aspeed_video_set_parm,
>> + .vidioc_enum_framesizes = aspeed_video_enum_framesizes,
>> + .vidioc_enum_frameintervals = aspeed_video_enum_frameintervals,
>> +
>> + .vidioc_s_dv_timings = aspeed_video_set_dv_timings,
>> + .vidioc_g_dv_timings = aspeed_video_get_dv_timings,
>> + .vidioc_query_dv_timings = aspeed_video_query_dv_timings,
>> + .vidioc_enum_dv_timings = aspeed_video_enum_dv_timings,
>> + .vidioc_dv_timings_cap = aspeed_video_dv_timings_cap,
>> +
>> + .vidioc_subscribe_event = aspeed_video_sub_event,
>> + .vidioc_unsubscribe_event = v4l2_event_unsubscribe,
>> +};
>> +
>> +static void aspeed_video_update_jpeg_quality(struct aspeed_video *video)
>> +{
>> + u32 comp_ctrl = FIELD_PREP(VE_COMP_CTRL_DCT_LUM, video->jpeg_quality) |
>> + FIELD_PREP(VE_COMP_CTRL_DCT_CHR, video->jpeg_quality | 0x10);
>> +
>> + aspeed_video_update(video, VE_COMP_CTRL,
>> + VE_COMP_CTRL_DCT_LUM | VE_COMP_CTRL_DCT_CHR,
>> + comp_ctrl);
>> +}
>> +
>> +static void aspeed_video_update_subsampling(struct aspeed_video *video)
>> +{
>> + if (video->jpeg.virt)
>> + aspeed_video_init_jpeg_table(video->jpeg.virt, video->yuv420);
>> +
>> + if (video->yuv420)
>> + aspeed_video_update(video, VE_SEQ_CTRL, 0, VE_SEQ_CTRL_YUV420);
>> + else
>> + aspeed_video_update(video, VE_SEQ_CTRL, VE_SEQ_CTRL_YUV420, 0);
>> +}
>> +
>> +static int aspeed_video_set_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> + struct aspeed_video *video = container_of(ctrl->handler,
>> + struct aspeed_video,
>> + ctrl_handler);
>> +
>> + switch (ctrl->id) {
>> + case V4L2_CID_JPEG_COMPRESSION_QUALITY:
>> + video->jpeg_quality = ctrl->val;
>> + aspeed_video_update_jpeg_quality(video);
>> + break;
>> + case V4L2_CID_JPEG_CHROMA_SUBSAMPLING:
>> + if (ctrl->val == V4L2_JPEG_CHROMA_SUBSAMPLING_420) {
>> + video->yuv420 = true;
>> + aspeed_video_update_subsampling(video);
>> + } else {
>> + video->yuv420 = false;
>> + aspeed_video_update_subsampling(video);
>> + }
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct v4l2_ctrl_ops aspeed_video_ctrl_ops = {
>> + .s_ctrl = aspeed_video_set_ctrl,
>> +};
>> +
>> +static void aspeed_video_resolution_work(struct work_struct *work)
>> +{
>> + int rc;
>> + struct delayed_work *dwork = to_delayed_work(work);
>> + struct aspeed_video *video = container_of(dwork, struct aspeed_video,
>> + res_work);
>> +
>> + /* No clients remaining after delay */
>> + if (atomic_read(&video->clients) == 0)
>> + goto done;
>> +
>> + aspeed_video_on(video);
>> +
>> + aspeed_video_init_regs(video);
>> +
>> + rc = aspeed_video_get_resolution(video);
>> + if (rc)
>> + dev_err(video->dev,
>> + "resolution changed; couldn't get new resolution\n");
>> + else if (test_bit(VIDEO_STREAMING, &video->flags))
>> + aspeed_video_start_frame(video);
>> +
>> + if (video->width != video->pix_fmt.width ||
>> + video->height != video->pix_fmt.height) {
>> + static const struct v4l2_event ev = {
>> + .type = V4L2_EVENT_SOURCE_CHANGE,
>> + .u.src_change.changes = V4L2_EVENT_SRC_CH_RESOLUTION,
>> + };
>> +
>> + v4l2_event_queue(&video->vdev, &ev);
>> + }
>> +
>> +done:
>> + clear_bit(VIDEO_RES_CHANGE, &video->flags);
>> + wake_up_interruptible_all(&video->wait);
>> +}
>> +
>> +static int aspeed_video_open(struct file *file)
>> +{
>> + int rc;
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + mutex_lock(&video->video_lock);
>> +
>> + if (atomic_inc_return(&video->clients) == 1) {
>> + rc = aspeed_video_start(video);
>> + if (rc) {
>> + dev_err(video->dev, "Failed to start video engine\n");
>> + atomic_dec(&video->clients);
>> + mutex_unlock(&video->video_lock);
>> + return rc;
>> + }
>> + }
>> +
>> + mutex_unlock(&video->video_lock);
>> +
>> + return v4l2_fh_open(file);
>> +}
>> +
>> +static int aspeed_video_release(struct file *file)
>> +{
>> + int rc;
>> + struct aspeed_video *video = video_drvdata(file);
>> +
>> + rc = vb2_fop_release(file);
>> +
>> + mutex_lock(&video->video_lock);
>> +
>> + if (atomic_dec_return(&video->clients) == 0)
>> + aspeed_video_stop(video);
>> +
>> + mutex_unlock(&video->video_lock);
>> +
>> + return rc;
>> +}
>> +
>> +static const struct v4l2_file_operations aspeed_video_v4l2_fops = {
>> + .owner = THIS_MODULE,
>> + .read = vb2_fop_read,
>> + .poll = vb2_fop_poll,
>> + .unlocked_ioctl = video_ioctl2,
>> + .mmap = vb2_fop_mmap,
>> + .open = aspeed_video_open,
>> + .release = aspeed_video_release,
>> +};
>> +
>> +static int aspeed_video_queue_setup(struct vb2_queue *q,
>> + unsigned int *num_buffers,
>> + unsigned int *num_planes,
>> + unsigned int sizes[],
>> + struct device *alloc_devs[])
>> +{
>> + struct aspeed_video *video = vb2_get_drv_priv(q);
>> +
>> + if (*num_planes) {
>> + if (sizes[0] < video->max_compressed_size)
>> + return -EINVAL;
>> +
>> + return 0;
>> + }
>> +
>> + *num_planes = 1;
>> + sizes[0] = video->max_compressed_size;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_buf_prepare(struct vb2_buffer *vb)
>> +{
>> + struct aspeed_video *video = vb2_get_drv_priv(vb->vb2_queue);
>> +
>> + if (vb2_plane_size(vb, 0) < video->max_compressed_size)
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_start_streaming(struct vb2_queue *q,
>> + unsigned int count)
>> +{
>> + int rc;
>> + struct aspeed_video *video = vb2_get_drv_priv(q);
>> +
>> + rc = aspeed_video_start_frame(video);
>> + if (rc) {
>> + aspeed_video_bufs_done(video, VB2_BUF_STATE_QUEUED);
>> + return rc;
>> + }
>> +
>> + video->sequence = 0;
>> + set_bit(VIDEO_STREAMING, &video->flags);
>> + return 0;
>> +}
>> +
>> +static void aspeed_video_stop_streaming(struct vb2_queue *q)
>> +{
>> + int rc;
>> + struct aspeed_video *video = vb2_get_drv_priv(q);
>> +
>> + clear_bit(VIDEO_STREAMING, &video->flags);
>> +
>> + rc = wait_event_timeout(video->wait,
>> + !test_bit(VIDEO_FRAME_INPRG, &video->flags),
>> + STOP_TIMEOUT);
>> + if (!rc) {
>> + dev_err(video->dev, "Timed out when stopping streaming\n");
>> + aspeed_video_stop(video);
>> + }
>> +
>> + aspeed_video_bufs_done(video, VB2_BUF_STATE_ERROR);
>> +}
>> +
>> +static void aspeed_video_buf_queue(struct vb2_buffer *vb)
>> +{
>> + struct aspeed_video *video = vb2_get_drv_priv(vb->vb2_queue);
>> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> + struct aspeed_video_buffer *avb = to_aspeed_video_buffer(vbuf);
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&video->lock, flags);
>> + list_add_tail(&avb->link, &video->buffers);
>> + spin_unlock_irqrestore(&video->lock, flags);
>> +}
>> +
>> +static const struct vb2_ops aspeed_video_vb2_ops = {
>> + .queue_setup = aspeed_video_queue_setup,
>> + .wait_prepare = vb2_ops_wait_prepare,
>> + .wait_finish = vb2_ops_wait_finish,
>> + .buf_prepare = aspeed_video_buf_prepare,
>> + .start_streaming = aspeed_video_start_streaming,
>> + .stop_streaming = aspeed_video_stop_streaming,
>> + .buf_queue = aspeed_video_buf_queue,
>> +};
>> +
>> +static int aspeed_video_setup_video(struct aspeed_video *video)
>> +{
>> + const u64 mask = ~(BIT(V4L2_JPEG_CHROMA_SUBSAMPLING_444) |
>> + BIT(V4L2_JPEG_CHROMA_SUBSAMPLING_420));
>> + struct v4l2_device *v4l2_dev = &video->v4l2_dev;
>> + struct vb2_queue *vbq = &video->queue;
>> + struct video_device *vdev = &video->vdev;
>> + int rc;
>> +
>> + video->pix_fmt.pixelformat = V4L2_PIX_FMT_JPEG;
>> + video->pix_fmt.field = V4L2_FIELD_NONE;
>> + video->pix_fmt.colorspace = V4L2_COLORSPACE_SRGB;
>> + video->pix_fmt.quantization = V4L2_QUANTIZATION_FULL_RANGE;
>> +
>> + rc = v4l2_device_register(video->dev, v4l2_dev);
>> + if (rc) {
>> + dev_err(video->dev, "Failed to register v4l2 device\n");
>> + return rc;
>> + }
>> +
>> + v4l2_ctrl_handler_init(&video->ctrl_handler, 2);
>> + v4l2_ctrl_new_std(&video->ctrl_handler, &aspeed_video_ctrl_ops,
>> + V4L2_CID_JPEG_COMPRESSION_QUALITY, 0,
>> + ASPEED_VIDEO_JPEG_NUM_QUALITIES - 1, 1, 0);
>> + v4l2_ctrl_new_std_menu(&video->ctrl_handler, &aspeed_video_ctrl_ops,
>> + V4L2_CID_JPEG_CHROMA_SUBSAMPLING,
>> + V4L2_JPEG_CHROMA_SUBSAMPLING_420, mask,
>> + V4L2_JPEG_CHROMA_SUBSAMPLING_444);
>> +
>> + if (video->ctrl_handler.error) {
>> + v4l2_ctrl_handler_free(&video->ctrl_handler);
>> + v4l2_device_unregister(v4l2_dev);
>> +
>> + dev_err(video->dev, "Failed to init controls: %d\n",
>> + video->ctrl_handler.error);
>> + return rc;
>> + }
>> +
>> + v4l2_dev->ctrl_handler = &video->ctrl_handler;
>> +
>> + vbq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> + vbq->io_modes = VB2_MMAP | VB2_READ | VB2_DMABUF;
>> + vbq->dev = v4l2_dev->dev;
>> + vbq->lock = &video->video_lock;
>> + vbq->ops = &aspeed_video_vb2_ops;
>> + vbq->mem_ops = &vb2_dma_contig_memops;
>> + vbq->drv_priv = video;
>> + vbq->buf_struct_size = sizeof(struct aspeed_video_buffer);
>> + vbq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>> + vbq->min_buffers_needed = 3;
>> +
>> + rc = vb2_queue_init(vbq);
>> + if (rc) {
>> + v4l2_ctrl_handler_free(&video->ctrl_handler);
>> + v4l2_device_unregister(v4l2_dev);
>> +
>> + dev_err(video->dev, "Failed to init vb2 queue\n");
>> + return rc;
>> + }
>> +
>> + vdev->queue = vbq;
>> + vdev->fops = &aspeed_video_v4l2_fops;
>> + vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_READWRITE |
> READWRITE doesn't work for JPEG since there is no clear end of a frame. Just drop
> this and the read op in aspeed_video_v4l2_fops.
>
>> + V4L2_CAP_STREAMING;
>> + vdev->v4l2_dev = v4l2_dev;
>> + strscpy(vdev->name, DEVICE_NAME, sizeof(vdev->name));
>> + vdev->vfl_type = VFL_TYPE_GRABBER;
>> + vdev->vfl_dir = VFL_DIR_RX;
>> + vdev->release = video_device_release_empty;
>> + vdev->ioctl_ops = &aspeed_video_ioctl_ops;
>> + vdev->lock = &video->video_lock;
>> +
>> + video_set_drvdata(vdev, video);
>> + rc = video_register_device(vdev, VFL_TYPE_GRABBER, 0);
>> + if (rc) {
>> + vb2_queue_release(vbq);
>> + v4l2_ctrl_handler_free(&video->ctrl_handler);
>> + v4l2_device_unregister(v4l2_dev);
>> +
>> + dev_err(video->dev, "Failed to register video device\n");
>> + return rc;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_init(struct aspeed_video *video)
>> +{
>> + int irq;
>> + int rc;
>> + struct device *dev = video->dev;
>> +
>> + irq = irq_of_parse_and_map(dev->of_node, 0);
>> + if (!irq) {
>> + dev_err(dev, "Unable to find IRQ\n");
>> + return -ENODEV;
>> + }
>> +
>> + rc = devm_request_irq(dev, irq, aspeed_video_irq, IRQF_SHARED,
>> + DEVICE_NAME, video);
>> + if (rc < 0) {
>> + dev_err(dev, "Unable to request IRQ %d\n", irq);
>> + return rc;
>> + }
>> +
>> + video->eclk = devm_clk_get(dev, "eclk");
>> + if (IS_ERR(video->eclk)) {
>> + dev_err(dev, "Unable to get ECLK\n");
>> + return PTR_ERR(video->eclk);
>> + }
>> +
>> + video->vclk = devm_clk_get(dev, "vclk");
>> + if (IS_ERR(video->vclk)) {
>> + dev_err(dev, "Unable to get VCLK\n");
>> + return PTR_ERR(video->vclk);
>> + }
>> +
>> + video->rst = devm_reset_control_get_exclusive(dev, NULL);
>> + if (IS_ERR(video->rst)) {
>> + dev_err(dev, "Unable to get VE reset\n");
>> + return PTR_ERR(video->rst);
>> + }
>> +
>> + rc = of_reserved_mem_device_init(dev);
>> + if (rc) {
>> + dev_err(dev, "Unable to reserve memory\n");
>> + return rc;
>> + }
>> +
>> + rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>> + if (rc) {
>> + dev_err(dev, "Failed to set DMA mask\n");
>> + of_reserved_mem_device_release(dev);
>> + return rc;
>> + }
>> +
>> + if (!aspeed_video_alloc_buf(video, &video->jpeg,
>> + VE_JPEG_HEADER_SIZE)) {
>> + dev_err(dev, "Failed to allocate DMA for JPEG header\n");
>> + of_reserved_mem_device_release(dev);
>> + return rc;
>> + }
>> +
>> + aspeed_video_init_jpeg_table(video->jpeg.virt, video->yuv420);
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_probe(struct platform_device *pdev)
>> +{
>> + int rc;
>> + struct resource *res;
>> + struct aspeed_video *video = kzalloc(sizeof(*video), GFP_KERNEL);
>> +
>> + if (!video)
>> + return -ENOMEM;
>> +
>> + video->frame_rate = 30;
>> + video->dev = &pdev->dev;
>> + mutex_init(&video->video_lock);
>> + init_waitqueue_head(&video->wait);
>> + INIT_DELAYED_WORK(&video->res_work, aspeed_video_resolution_work);
>> + INIT_LIST_HEAD(&video->buffers);
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> + video->base = devm_ioremap_resource(video->dev, res);
>> +
>> + if (IS_ERR(video->base))
>> + return PTR_ERR(video->base);
>> +
>> + rc = aspeed_video_init(video);
>> + if (rc)
>> + return rc;
>> +
>> + rc = aspeed_video_setup_video(video);
>> + if (rc)
>> + return rc;
>> +
>> + return 0;
>> +}
>> +
>> +static int aspeed_video_remove(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct v4l2_device *v4l2_dev = dev_get_drvdata(dev);
>> + struct aspeed_video *video = to_aspeed_video(v4l2_dev);
>> +
>> + video_unregister_device(&video->vdev);
>> +
>> + vb2_queue_release(&video->queue);
>> +
>> + v4l2_ctrl_handler_free(&video->ctrl_handler);
>> +
>> + v4l2_device_unregister(v4l2_dev);
>> +
>> + dma_free_coherent(video->dev, VE_JPEG_HEADER_SIZE, video->jpeg.virt,
>> + video->jpeg.dma);
>> +
>> + of_reserved_mem_device_release(dev);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id aspeed_video_of_match[] = {
>> + { .compatible = "aspeed,ast2400-video-engine" },
>> + { .compatible = "aspeed,ast2500-video-engine" },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, aspeed_video_of_match);
>> +
>> +static struct platform_driver aspeed_video_driver = {
>> + .driver = {
>> + .name = DEVICE_NAME,
>> + .of_match_table = aspeed_video_of_match,
>> + },
>> + .probe = aspeed_video_probe,
>> + .remove = aspeed_video_remove,
>> +};
>> +
>> +module_platform_driver(aspeed_video_driver);
>> +
>> +MODULE_DESCRIPTION("ASPEED Video Engine Driver");
>> +MODULE_AUTHOR("Eddie James");
>> +MODULE_LICENSE("GPL v2");
>>
> Regards,
>
> Hans
>
^ permalink raw reply
* [[PATCH] 8/9] DMA-UART-Driver-for-AST2500
From: Vinod @ 2018-10-20 16:26 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181019071154.GE13642@Pilot130>
On 19-10-18, 12:41, sudheer.v wrote:
> On Fri, Oct 19, 2018 at 10:32:24AM +1100, Benjamin Herrenschmidt wrote:
> > On Thu, 2018-10-18 at 15:25 +0530, Vinod wrote:
> > >
> > > > It's not a dmaengine driver. It's a serial UART driver that happens to
> > > > use a dedicated DMA engine.
> > >
> > > Then I see no reason for it to use dmaengine APIs. The framework allows
> > > people to share a controller for many clients, but if you have dedicated
> > > one then you may use it directly
> >
> > Well... the engine is shared by a few UARTs, they have dedicated rings
> > but there's a common set of regs for interrupt handling etc.
> >
> > That said, I still think it could be contained within a UART driver,
> > there's little benefit in adding the framework overhead, esp since
> > these are really weak cores, any overhead will be felt.
> >
> > Ben.
> >
> > > > It's unclear whether it should be split into two drivers, or just have
> > > > the serial driver directly use the dma engine since that engine is
> > > > dedicated in HW to only work on those UARTs and nothing else...
> > > >
> > > > Cheers,
> > > > Ben.
>
> Initially we wanted to have a single driver,
> however we had an informal discussion with one of the maintainer
> and based on the feedback, followed the Linux DMA and UART architecture.
>
> If this seperate DMA-engine driver adds more overhead than benifit,
> we will merge them into a single UART driver and resubmitt the patches.
> Vinod,
> can this dma-controller driver sit under dma subsystem?.
> or better to move it under UART framework.
My advise would be to see what you can do with the DMA IP block. If this
can/would be used in different places then it would make sense to do a
dmaengine driver and solve the problem for everyone.
If this is always going to be hidden behind serial then maybe it makes
sense to be inside serial driver and not use dmaengine APIs
If you decide to prefer the former case, please move it to dmaengine and
resubmit :)
HTH
--
~Vinod
^ permalink raw reply
* [PATCH v8 07/12] dt-bindings: mfd: Add a document for PECI client MFD
From: Lee Jones @ 2018-10-24 7:25 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180918215124.14003-8-jae.hyun.yoo@linux.intel.com>
On Tue, 18 Sep 2018, Jae Hyun Yoo wrote:
> This commit adds a dt-bindings document for PECI client MFD.
>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Andrew Jeffery <andrew@aj.id.au>
> Cc: James Feist <james.feist@linux.intel.com>
> Cc: Jason M Biils <jason.m.bills@linux.intel.com>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: Vernon Mauery <vernon.mauery@linux.intel.com>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
> .../bindings/mfd/intel-peci-client.txt | 34 +++++++++++++++++++
> 1 file changed, 34 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/intel-peci-client.txt
>
> diff --git a/Documentation/devicetree/bindings/mfd/intel-peci-client.txt b/Documentation/devicetree/bindings/mfd/intel-peci-client.txt
> new file mode 100644
> index 000000000000..cb341e363add
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/intel-peci-client.txt
> @@ -0,0 +1,34 @@
> +* Intel PECI client bindings
> +
> +PECI (Platform Environment Control Interface) is a one-wire bus interface that
> +provides a communication channel from PECI clients in Intel processors and
> +chipset components to external monitoring or control devices. PECI is designed
> +to support the following sideband functions:
> +
> +- Processor and DRAM thermal management
> +- Platform Manageability
> +- Processor Interface Tuning and Diagnostics
> +- Failure Analysis
> +
> +Required properties:
> +- compatible : Should be "intel,peci-client".
> +- reg : Should contain address of a client CPU. Address range of CPU
> + clients starts from 0x30 based on PECI specification.
Nit: "start"
Would be better worded:
"According to the PECI specification client addresses start from 0x30."
> +Example:
> + peci-bus at 0 {
> + compatible = "vendor,soc-peci";
> + reg = <0x0 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + peci-client at 30 {
> + compatible = "intel,peci-client";
> + reg = <0x30>;
> + };
> +
> + peci-client at 31 {
> + compatible = "intel,peci-client";
> + reg = <0x31>;
> + };
> + };
--
Lee Jones [???]
Linaro Services Technical Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH v8 08/12] mfd: intel-peci-client: Add PECI client MFD driver
From: Lee Jones @ 2018-10-24 10:59 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20180918215124.14003-9-jae.hyun.yoo@linux.intel.com>
On Tue, 18 Sep 2018, Jae Hyun Yoo wrote:
> This commit adds PECI client MFD driver.
>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Andrew Jeffery <andrew@aj.id.au>
> Cc: James Feist <james.feist@linux.intel.com>
> Cc: Jason M Biils <jason.m.bills@linux.intel.com>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: Vernon Mauery <vernon.mauery@linux.intel.com>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
> drivers/mfd/Kconfig | 14 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/intel-peci-client.c | 181 ++++++++++++++++++++++++++
> include/linux/mfd/intel-peci-client.h | 81 ++++++++++++
> 4 files changed, 277 insertions(+)
> create mode 100644 drivers/mfd/intel-peci-client.c
> create mode 100644 include/linux/mfd/intel-peci-client.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 11841f4b7b2b..e68ae5d820ba 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -595,6 +595,20 @@ config MFD_INTEL_MSIC
> Passage) chip. This chip embeds audio, battery, GPIO, etc.
> devices used in Intel Medfield platforms.
>
> +config MFD_INTEL_PECI_CLIENT
> + bool "Intel PECI client"
> + depends on (PECI || COMPILE_TEST)
> + select MFD_CORE
> + help
> + If you say yes to this option, support will be included for the
> + multi-functional Intel PECI (Platform Environment Control Interface)
Remove 'multi-functional' from this sentence.
> + client. PECI is a one-wire bus interface that provides a communication
> + channel from PECI clients in Intel processors and chipset components
> + to external monitoring or control devices.
> +
> + Additional drivers must be enabled in order to use the functionality
> + of the device.
> +
> config MFD_IPAQ_MICRO
> bool "Atmel Micro ASIC (iPAQ h3100/h3600/h3700) Support"
> depends on SA1100_H3100 || SA1100_H3600
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 5856a9489cbd..ed05583dc30e 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -203,6 +203,7 @@ obj-$(CONFIG_MFD_INTEL_LPSS) += intel-lpss.o
> obj-$(CONFIG_MFD_INTEL_LPSS_PCI) += intel-lpss-pci.o
> obj-$(CONFIG_MFD_INTEL_LPSS_ACPI) += intel-lpss-acpi.o
> obj-$(CONFIG_MFD_INTEL_MSIC) += intel_msic.o
> +obj-$(CONFIG_MFD_INTEL_PECI_CLIENT) += intel-peci-client.o
> obj-$(CONFIG_MFD_PALMAS) += palmas.o
> obj-$(CONFIG_MFD_VIPERBOARD) += viperboard.o
> obj-$(CONFIG_MFD_RC5T583) += rc5t583.o rc5t583-irq.o
> diff --git a/drivers/mfd/intel-peci-client.c b/drivers/mfd/intel-peci-client.c
> new file mode 100644
> index 000000000000..507e4ac66dfa
> --- /dev/null
> +++ b/drivers/mfd/intel-peci-client.c
> @@ -0,0 +1,181 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2018 Intel Corporation
> +
> +#include <linux/bitfield.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/intel-peci-client.h>
> +#include <linux/module.h>
> +#include <linux/peci.h>
> +#include <linux/of_device.h>
> +
> +enum cpu_gens {
> + CPU_GEN_HSX = 0, /* Haswell Xeon */
> + CPU_GEN_BRX, /* Broadwell Xeon */
> + CPU_GEN_SKX, /* Skylake Xeon */
> +};
> +
> +static struct mfd_cell peci_functions[] = {
> + {
> + .name = "peci-cputemp",
> + },
> + {
> + .name = "peci-dimmtemp",
> + },
{ .name = "peci-cputemp", },
{ .name = "peci-dimmtemp", },
Do these have 2 different drivers? Where are you putting them?
> + /* TODO: Add additional PECI sideband functions into here */
When will this be done?
> +};
> +
> +static const struct cpu_gen_info cpu_gen_info_table[] = {
> + [CPU_GEN_HSX] = {
> + .family = 6, /* Family code */
> + .model = INTEL_FAM6_HASWELL_X,
> + .core_max = CORE_MAX_ON_HSX,
> + .chan_rank_max = CHAN_RANK_MAX_ON_HSX,
> + .dimm_idx_max = DIMM_IDX_MAX_ON_HSX },
> + [CPU_GEN_BRX] = {
> + .family = 6, /* Family code */
> + .model = INTEL_FAM6_BROADWELL_X,
> + .core_max = CORE_MAX_ON_BDX,
> + .chan_rank_max = CHAN_RANK_MAX_ON_BDX,
> + .dimm_idx_max = DIMM_IDX_MAX_ON_BDX },
> + [CPU_GEN_SKX] = {
> + .family = 6, /* Family code */
> + .model = INTEL_FAM6_SKYLAKE_X,
> + .core_max = CORE_MAX_ON_SKX,
> + .chan_rank_max = CHAN_RANK_MAX_ON_SKX,
> + .dimm_idx_max = DIMM_IDX_MAX_ON_SKX },
The '},'s should go on the line below.
> +};
> +
> +static int peci_client_get_cpu_gen_info(struct peci_mfd *priv)
Remove all mention of 'mfd'. It's not a thing.
> +{
> + u32 cpu_id;
> + int i, rc;
> +
> + rc = peci_get_cpu_id(priv->adapter, priv->addr, &cpu_id);
> + if (rc)
> + return rc;
> +
> + for (i = 0; i < ARRAY_SIZE(cpu_gen_info_table); i++) {
> + if (FIELD_GET(CPU_ID_FAMILY_MASK, cpu_id) +
> + FIELD_GET(CPU_ID_EXT_FAMILY_MASK, cpu_id) ==
> + cpu_gen_info_table[i].family &&
> + FIELD_GET(CPU_ID_MODEL_MASK, cpu_id) ==
> + FIELD_GET(LOWER_NIBBLE_MASK,
> + cpu_gen_info_table[i].model) &&
> + FIELD_GET(CPU_ID_EXT_MODEL_MASK, cpu_id) ==
> + FIELD_GET(UPPER_NIBBLE_MASK,
> + cpu_gen_info_table[i].model)) {
> + break;
> + }
> + }
This is really read. Please reformat it, even if you have to use
local variables to make it more legible.
> + if (i >= ARRAY_SIZE(cpu_gen_info_table))
> + return -ENODEV;
Do you really want to fail silently?
> + priv->gen_info = &cpu_gen_info_table[i];
If you do this in the for(), you can then test priv->gen_info instead
of seeing if the iterator maxed out. Much nicer I think.
> + return 0;
> +}
> +
> +bool peci_temp_need_update(struct temp_data *temp)
> +{
> + if (temp->valid &&
> + time_before(jiffies, temp->last_updated + UPDATE_INTERVAL))
> + return false;
> +
> + return true;
> +}
> +EXPORT_SYMBOL_GPL(peci_temp_need_update);
> +
> +void peci_temp_mark_updated(struct temp_data *temp)
> +{
> + temp->valid = 1;
> + temp->last_updated = jiffies;
> +}
> +EXPORT_SYMBOL_GPL(peci_temp_mark_updated);
These are probably better suited as inline functions to be placed in
a header file. No need to export them, since they only use their own
data.
> +int peci_client_command(struct peci_mfd *priv, enum peci_cmd cmd, void *vmsg)
> +{
> + return peci_command(priv->adapter, cmd, vmsg);
> +}
> +EXPORT_SYMBOL_GPL(peci_client_command);
If you share the adaptor with the client, you can call peci_command()
directly. There should also be some locking in here somewhere too.
> +int peci_client_rd_pkg_cfg_cmd(struct peci_mfd *priv, u8 mbx_idx,
This is gobbledegook. What's rd? Read?
> + u16 param, u8 *data)
> +{
> + struct peci_rd_pkg_cfg_msg msg;
> + int rc;
> +
> + msg.addr = priv->addr;
> + msg.index = mbx_idx;
> + msg.param = param;
> + msg.rx_len = 4;
> +
> + rc = peci_command(priv->adapter, PECI_CMD_RD_PKG_CFG, &msg);
> + if (!rc)
> + memcpy(data, msg.pkg_config, 4);
> +
> + return rc;
> +}
> +EXPORT_SYMBOL_GPL(peci_client_rd_pkg_cfg_cmd);
This too should be set-up as an inline function, not an exported one.
> +static int peci_client_probe(struct peci_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct peci_mfd *priv;
> + int rc;
'ret' is more common.
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + dev_set_drvdata(dev, priv);
> + priv->client = client;
> + priv->dev = dev;
> + priv->adapter = client->adapter;
> + priv->addr = client->addr;
You're already saving client. No need to save its individual
attributes as well.
> + priv->cpu_no = client->addr - PECI_BASE_ADDR;
This seems clunky. Does the spec say that the addresses have to be in
numerical order with no gaps? What if someone chooses to implement 4
CPUs at 0x30, 0x35, 0x45 and 0x60?
What do you then do with cpu_no?
> + snprintf(priv->name, PECI_NAME_SIZE, "peci_client.cpu%d", priv->cpu_no);
> +
> + rc = peci_client_get_cpu_gen_info(priv);
> + if (rc)
> + return rc;
> +
> + rc = devm_mfd_add_devices(priv->dev, priv->cpu_no, peci_functions,
> + ARRAY_SIZE(peci_functions), NULL, 0, NULL);
> + if (rc < 0) {
> + dev_err(priv->dev, "devm_mfd_add_devices failed: %d\n", rc);
This to too specific.
"Failed to register child devices"
> + return rc;
> + }
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id peci_client_of_table[] = {
> + { .compatible = "intel,peci-client" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, peci_client_of_table);
> +#endif
> +
> +static const struct peci_device_id peci_client_ids[] = {
> + { .name = "peci-client", .driver_data = 0 },
Remove .driver_data if you're not going to use it.
> + { }
> +};
> +MODULE_DEVICE_TABLE(peci, peci_client_ids);
> +
> +static struct peci_driver peci_client_driver = {
> + .probe = peci_client_probe,
> + .id_table = peci_client_ids,
> + .driver = {
> + .name = "peci-client",
> + .of_match_table =
> + of_match_ptr(peci_client_of_table),
> + },
> +};
Odd tabbing.
> +module_peci_driver(peci_client_driver);
> +
> +MODULE_AUTHOR("Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>");
> +MODULE_DESCRIPTION("PECI client MFD driver");
Remove "MFD".
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/intel-peci-client.h b/include/linux/mfd/intel-peci-client.h
> new file mode 100644
> index 000000000000..7ec272cddceb
> --- /dev/null
> +++ b/include/linux/mfd/intel-peci-client.h
> @@ -0,0 +1,81 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (c) 2018 Intel Corporation */
> +
> +#ifndef __LINUX_MFD_PECI_CLIENT_H
> +#define __LINUX_MFD_PECI_CLIENT_H
> +
> +#include <linux/peci.h>
> +
> +#if IS_ENABLED(CONFIG_X86)
> +#include <asm/intel-family.h>
> +#else
> +/**
> + * Architectures other than x86 cannot include the header file so define these
> + * at here. These are needed for detecting type of client x86 CPUs behind a PECI
> + * connection.
> + */
> +#define INTEL_FAM6_HASWELL_X 0x3F
> +#define INTEL_FAM6_BROADWELL_X 0x4F
> +#define INTEL_FAM6_SKYLAKE_X 0x55
> +#endif
> +
> +#define LOWER_NIBBLE_MASK GENMASK(3, 0)
> +#define UPPER_NIBBLE_MASK GENMASK(7, 4)
> +
> +#define CPU_ID_MODEL_MASK GENMASK(7, 4)
> +#define CPU_ID_FAMILY_MASK GENMASK(11, 8)
> +#define CPU_ID_EXT_MODEL_MASK GENMASK(19, 16)
> +#define CPU_ID_EXT_FAMILY_MASK GENMASK(27, 20)
> +
> +#define CORE_MAX_ON_HSX 18 /* Max number of cores on Haswell */
> +#define CHAN_RANK_MAX_ON_HSX 8 /* Max number of channel ranks on Haswell */
> +#define DIMM_IDX_MAX_ON_HSX 3 /* Max DIMM index per channel on Haswell */
> +
> +#define CORE_MAX_ON_BDX 24 /* Max number of cores on Broadwell */
> +#define CHAN_RANK_MAX_ON_BDX 4 /* Max number of channel ranks on Broadwell */
> +#define DIMM_IDX_MAX_ON_BDX 3 /* Max DIMM index per channel on Broadwell */
> +
> +#define CORE_MAX_ON_SKX 28 /* Max number of cores on Skylake */
> +#define CHAN_RANK_MAX_ON_SKX 6 /* Max number of channel ranks on Skylake */
> +#define DIMM_IDX_MAX_ON_SKX 2 /* Max DIMM index per channel on Skylake */
> +
> +#define CORE_NUMS_MAX CORE_MAX_ON_SKX
> +#define CHAN_RANK_MAX CHAN_RANK_MAX_ON_HSX
> +#define DIMM_IDX_MAX DIMM_IDX_MAX_ON_HSX
> +#define DIMM_NUMS_MAX (CHAN_RANK_MAX * DIMM_IDX_MAX)
> +
> +#define TEMP_TYPE_PECI 6 /* Sensor type 6: Intel PECI */
> +
> +#define UPDATE_INTERVAL HZ
> +
> +struct temp_data {
> + uint valid;
> + s32 value;
> + ulong last_updated;
> +};
This should not live in here.
> +struct cpu_gen_info {
> + u16 family;
> + u8 model;
> + uint core_max;
> + uint chan_rank_max;
> + uint dimm_idx_max;
> +};
> +
> +struct peci_mfd {
Remove "_mfd".
> + struct peci_client *client;
> + struct device *dev;
> + struct peci_adapter *adapter;
> + char name[PECI_NAME_SIZE];
What is this used for?
> + u8 addr;
> + uint cpu_no;
> + const struct cpu_gen_info *gen_info;
Where is this used?
> +};
Both of these structs need kerneldoc headers.
> +bool peci_temp_need_update(struct temp_data *temp);
> +void peci_temp_mark_updated(struct temp_data *temp);
These should live in a temperature driver.
> +int peci_client_command(struct peci_mfd *mfd, enum peci_cmd cmd, void *vmsg);
> +int peci_client_rd_pkg_cfg_cmd(struct peci_mfd *mfd, u8 mbx_idx,
> + u16 param, u8 *data);
> +
> +#endif /* __LINUX_MFD_PECI_CLIENT_H */
--
Lee Jones [???]
Linaro Services Technical Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH v8 07/12] dt-bindings: mfd: Add a document for PECI client MFD
From: Jae Hyun Yoo @ 2018-10-24 16:39 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181024072540.GA4939@dell>
Hi Lee,
On 10/24/2018 12:25 AM, Lee Jones wrote:
> On Tue, 18 Sep 2018, Jae Hyun Yoo wrote:
>
>> +Required properties:
>> +- compatible : Should be "intel,peci-client".
>> +- reg : Should contain address of a client CPU. Address range of CPU
>> + clients starts from 0x30 based on PECI specification.
>
> Nit: "start"
>
> Would be better worded:
>
> "According to the PECI specification client addresses start from 0x30."
>
Yes, that would be better. Will fix it like you suggested.
Thanks for the review!
Jae
^ permalink raw reply
* [PATCH v8 08/12] mfd: intel-peci-client: Add PECI client MFD driver
From: Jae Hyun Yoo @ 2018-10-24 21:10 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20181024105903.GB4939@dell>
Hi Lee,
On 10/24/2018 3:59 AM, Lee Jones wrote:
> On Tue, 18 Sep 2018, Jae Hyun Yoo wrote:
>
>> This commit adds PECI client MFD driver.
>>
<snip>
>> +config MFD_INTEL_PECI_CLIENT
>> + bool "Intel PECI client"
>> + depends on (PECI || COMPILE_TEST)
>> + select MFD_CORE
>> + help
>> + If you say yes to this option, support will be included for the
>> + multi-functional Intel PECI (Platform Environment Control Interface)
>
> Remove 'multi-functional' from this sentence.
>
Will remove the word.
<snip>
>> +static struct mfd_cell peci_functions[] = {
>> + {
>> + .name = "peci-cputemp",
>> + },
>> + {
>> + .name = "peci-dimmtemp",
>> + },
>
> { .name = "peci-cputemp", },
> { .name = "peci-dimmtemp", },
>
Will change it like you suggested.
> Do these have 2 different drivers? Where are you putting them?
>
Yes, these have 2 different drivers as hwmon subsystem drivers.
I submitted them into this patch series.
Patch 10/12: https://lkml.org/lkml/2018/9/18/1524
Patch 11/12: https://lkml.org/lkml/2018/9/18/1523
>> + /* TODO: Add additional PECI sideband functions into here */
>
> When will this be done?
>
I'm hoping it will be done by the end of this year.
>> +};
>> +
>> +static const struct cpu_gen_info cpu_gen_info_table[] = {
>> + [CPU_GEN_HSX] = {
>> + .family = 6, /* Family code */
>> + .model = INTEL_FAM6_HASWELL_X,
>> + .core_max = CORE_MAX_ON_HSX,
>> + .chan_rank_max = CHAN_RANK_MAX_ON_HSX,
>> + .dimm_idx_max = DIMM_IDX_MAX_ON_HSX },
>> + [CPU_GEN_BRX] = {
>> + .family = 6, /* Family code */
>> + .model = INTEL_FAM6_BROADWELL_X,
>> + .core_max = CORE_MAX_ON_BDX,
>> + .chan_rank_max = CHAN_RANK_MAX_ON_BDX,
>> + .dimm_idx_max = DIMM_IDX_MAX_ON_BDX },
>> + [CPU_GEN_SKX] = {
>> + .family = 6, /* Family code */
>> + .model = INTEL_FAM6_SKYLAKE_X,
>> + .core_max = CORE_MAX_ON_SKX,
>> + .chan_rank_max = CHAN_RANK_MAX_ON_SKX,
>> + .dimm_idx_max = DIMM_IDX_MAX_ON_SKX },
>
> The '},'s should go on the line below.
>
Okay. Will fix it.
>> +};
>> +
>> +static int peci_client_get_cpu_gen_info(struct peci_mfd *priv)
>
> Remove all mention of 'mfd'. It's not a thing.
>
Will remove 'mfd'.
>> +{
>> + u32 cpu_id;
>> + int i, rc;
>> +
>> + rc = peci_get_cpu_id(priv->adapter, priv->addr, &cpu_id);
>> + if (rc)
>> + return rc;
>> +
>> + for (i = 0; i < ARRAY_SIZE(cpu_gen_info_table); i++) {
>> + if (FIELD_GET(CPU_ID_FAMILY_MASK, cpu_id) +
>> + FIELD_GET(CPU_ID_EXT_FAMILY_MASK, cpu_id) ==
>> + cpu_gen_info_table[i].family &&
>> + FIELD_GET(CPU_ID_MODEL_MASK, cpu_id) ==
>> + FIELD_GET(LOWER_NIBBLE_MASK,
>> + cpu_gen_info_table[i].model) &&
>> + FIELD_GET(CPU_ID_EXT_MODEL_MASK, cpu_id) ==
>> + FIELD_GET(UPPER_NIBBLE_MASK,
>> + cpu_gen_info_table[i].model)) {
>> + break;
>> + }
>> + }
>
> This is really read. Please reformat it, even if you have to use
> local variables to make it more legible.
>
Will reformat it using local variables for better readability as you
suggest.
>> + if (i >= ARRAY_SIZE(cpu_gen_info_table))
>> + return -ENODEV;
>
> Do you really want to fail silently?
>
No. I'll add a dev_err printing.
>> + priv->gen_info = &cpu_gen_info_table[i];
>
> If you do this in the for(), you can then test priv->gen_info instead
> of seeing if the iterator maxed out. Much nicer I think.
>
Yes, that would be much nicer. Will fix it.
>> + return 0;
>> +}
>> +
>> +bool peci_temp_need_update(struct temp_data *temp)
>> +{
>> + if (temp->valid &&
>> + time_before(jiffies, temp->last_updated + UPDATE_INTERVAL))
>> + return false;
>> +
>> + return true;
>> +}
>> +EXPORT_SYMBOL_GPL(peci_temp_need_update);
>> +
>> +void peci_temp_mark_updated(struct temp_data *temp)
>> +{
>> + temp->valid = 1;
>> + temp->last_updated = jiffies;
>> +}
>> +EXPORT_SYMBOL_GPL(peci_temp_mark_updated);
>
> These are probably better suited as inline functions to be placed in
> a header file. No need to export them, since they only use their own
> data.
>
Yes, that makes sense. I'll change these to inline functions.
>> +int peci_client_command(struct peci_mfd *priv, enum peci_cmd cmd, void *vmsg)
>> +{
>> + return peci_command(priv->adapter, cmd, vmsg);
>> +}
>> +EXPORT_SYMBOL_GPL(peci_client_command);
>
> If you share the adaptor with the client, you can call peci_command()
> directly. There should also be some locking in here somewhere too.
>
Yes, the client->adapter can be referenced from child drivers. Will make
them call peci_command() directly.
>> +int peci_client_rd_pkg_cfg_cmd(struct peci_mfd *priv, u8 mbx_idx,
>
> This is gobbledegook. What's rd? Read?
>
Yes, the 'rd' means 'read'. I intended to keep command names as listed
in the PECI specification such as RdPkgConfig, WrPkgConfig and so on.
Should I change it to 'peci_client_read_package_config_command' ?
>> + u16 param, u8 *data)
>> +{
>> + struct peci_rd_pkg_cfg_msg msg;
>> + int rc;
>> +
>> + msg.addr = priv->addr;
>> + msg.index = mbx_idx;
>> + msg.param = param;
>> + msg.rx_len = 4;
>> +
>> + rc = peci_command(priv->adapter, PECI_CMD_RD_PKG_CFG, &msg);
>> + if (!rc)
>> + memcpy(data, msg.pkg_config, 4);
>> +
>> + return rc;
>> +}
>> +EXPORT_SYMBOL_GPL(peci_client_rd_pkg_cfg_cmd);
>
> This too should be set-up as an inline function, not an exported one.
>
Will change it to inline function.
>> +static int peci_client_probe(struct peci_client *client)
>> +{
>> + struct device *dev = &client->dev;
>> + struct peci_mfd *priv;
>> + int rc;
>
> 'ret' is more common.
>
Will fix it.
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + dev_set_drvdata(dev, priv);
>> + priv->client = client;
>> + priv->dev = dev;
>
>> + priv->adapter = client->adapter;
>> + priv->addr = client->addr;
>
> You're already saving client. No need to save its individual
> attributes as well.
>
I intended to reduce pointer referencing depth because adapter and addr
are frequently used, but you are right. I'll remove adapter and addr
from the struct.
>> + priv->cpu_no = client->addr - PECI_BASE_ADDR;
>
> This seems clunky. Does the spec say that the addresses have to be in
> numerical order with no gaps? What if someone chooses to implement 4
> CPUs at 0x30, 0x35, 0x45 and 0x60?
>
The CPU address will be assigned by H/W strap settings in order. Also,
there would be a case of using some CPU slots left empty, so gaps could
be happen on the addresses but it's still acceptable.
> What do you then do with cpu_no?
>
It is being used for child device id parameter when this code calls
devm_mfd_add_devices() below. Also, it is referenced from cputemp and
dimmtemp hwmon drivers but it isn't needed because the hwmon drivers
already know client->addr. I'll change cpu_no as a local variable in
here for child device id and will change the hwmon drivers.
>> + snprintf(priv->name, PECI_NAME_SIZE, "peci_client.cpu%d", priv->cpu_no);
>> +
>> + rc = peci_client_get_cpu_gen_info(priv);
>> + if (rc)
>> + return rc;
>> +
>> + rc = devm_mfd_add_devices(priv->dev, priv->cpu_no, peci_functions,
>> + ARRAY_SIZE(peci_functions), NULL, 0, NULL);
>> + if (rc < 0) {
>> + dev_err(priv->dev, "devm_mfd_add_devices failed: %d\n", rc);
>
> This to too specific.
>
> "Failed to register child devices"
>
Will fix it like you suggested.
>> + return rc;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id peci_client_of_table[] = {
>> + { .compatible = "intel,peci-client" },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(of, peci_client_of_table);
>> +#endif
>> +
>> +static const struct peci_device_id peci_client_ids[] = {
>> + { .name = "peci-client", .driver_data = 0 },
>
> Remove .driver_data if you're not going to use it.
>
Okay. I'll remove .driver_data.
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(peci, peci_client_ids);
>> +
>> +static struct peci_driver peci_client_driver = {
>> + .probe = peci_client_probe,
>> + .id_table = peci_client_ids,
>> + .driver = {
>> + .name = "peci-client",
>> + .of_match_table =
>> + of_match_ptr(peci_client_of_table),
>> + },
>> +};
>
> Odd tabbing.
>
Will fix the indentation.
>> +module_peci_driver(peci_client_driver);
>> +
>> +MODULE_AUTHOR("Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>");
>> +MODULE_DESCRIPTION("PECI client MFD driver");
>
> Remove "MFD".
>
Will remove it.
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/mfd/intel-peci-client.h b/include/linux/mfd/intel-peci-client.h
>> new file mode 100644
>> index 000000000000..7ec272cddceb
>> --- /dev/null
>> +++ b/include/linux/mfd/intel-peci-client.h
>> @@ -0,0 +1,81 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (c) 2018 Intel Corporation */
>> +
>> +#ifndef __LINUX_MFD_PECI_CLIENT_H
>> +#define __LINUX_MFD_PECI_CLIENT_H
>> +
>> +#include <linux/peci.h>
>> +
>> +#if IS_ENABLED(CONFIG_X86)
>> +#include <asm/intel-family.h>
>> +#else
>> +/**
>> + * Architectures other than x86 cannot include the header file so define these
>> + * at here. These are needed for detecting type of client x86 CPUs behind a PECI
>> + * connection.
>> + */
>> +#define INTEL_FAM6_HASWELL_X 0x3F
>> +#define INTEL_FAM6_BROADWELL_X 0x4F
>> +#define INTEL_FAM6_SKYLAKE_X 0x55
>> +#endif
>> +
>> +#define LOWER_NIBBLE_MASK GENMASK(3, 0)
>> +#define UPPER_NIBBLE_MASK GENMASK(7, 4)
>> +
>> +#define CPU_ID_MODEL_MASK GENMASK(7, 4)
>> +#define CPU_ID_FAMILY_MASK GENMASK(11, 8)
>> +#define CPU_ID_EXT_MODEL_MASK GENMASK(19, 16)
>> +#define CPU_ID_EXT_FAMILY_MASK GENMASK(27, 20)
>> +
>> +#define CORE_MAX_ON_HSX 18 /* Max number of cores on Haswell */
>> +#define CHAN_RANK_MAX_ON_HSX 8 /* Max number of channel ranks on Haswell */
>> +#define DIMM_IDX_MAX_ON_HSX 3 /* Max DIMM index per channel on Haswell */
>> +
>> +#define CORE_MAX_ON_BDX 24 /* Max number of cores on Broadwell */
>> +#define CHAN_RANK_MAX_ON_BDX 4 /* Max number of channel ranks on Broadwell */
>> +#define DIMM_IDX_MAX_ON_BDX 3 /* Max DIMM index per channel on Broadwell */
>> +
>> +#define CORE_MAX_ON_SKX 28 /* Max number of cores on Skylake */
>> +#define CHAN_RANK_MAX_ON_SKX 6 /* Max number of channel ranks on Skylake */
>> +#define DIMM_IDX_MAX_ON_SKX 2 /* Max DIMM index per channel on Skylake */
>> +
>> +#define CORE_NUMS_MAX CORE_MAX_ON_SKX
>> +#define CHAN_RANK_MAX CHAN_RANK_MAX_ON_HSX
>> +#define DIMM_IDX_MAX DIMM_IDX_MAX_ON_HSX
>> +#define DIMM_NUMS_MAX (CHAN_RANK_MAX * DIMM_IDX_MAX)
>> +
>> +#define TEMP_TYPE_PECI 6 /* Sensor type 6: Intel PECI */
>> +
>> +#define UPDATE_INTERVAL HZ
>> +
>> +struct temp_data {
>> + uint valid;
>> + s32 value;
>> + ulong last_updated;
>> +};
>
> This should not live in here.
>
Will move it.
>> +struct cpu_gen_info {
>> + u16 family;
>> + u8 model;
>> + uint core_max;
>> + uint chan_rank_max;
>> + uint dimm_idx_max;
>> +};
>> +
>> +struct peci_mfd {
>
> Remove "_mfd".
>
Will remove 'mfd'.
>> + struct peci_client *client;
>> + struct device *dev;
>> + struct peci_adapter *adapter;
>> + char name[PECI_NAME_SIZE];
>
> What is this used for?
>
This isn't needed. Thanks for your pointing out. I'll remove it.
>> + u8 addr;
>> + uint cpu_no;
>> + const struct cpu_gen_info *gen_info;
>
> Where is this used?
>
It is referenced from cputemp and dimmtemp hwmon drivers to specify CPU
generation.
>> +};
>
> Both of these structs need kerneldoc headers.
>
Will add kerneldoc headers.
>> +bool peci_temp_need_update(struct temp_data *temp);
>> +void peci_temp_mark_updated(struct temp_data *temp);
>
> These should live in a temperature driver.
>
Agreed. I'll move it to temperature driver.
Thanks a lot for your careful review. I'll submit a new patch set soon.
Jae
>> +int peci_client_command(struct peci_mfd *mfd, enum peci_cmd cmd, void *vmsg);
>> +int peci_client_rd_pkg_cfg_cmd(struct peci_mfd *mfd, u8 mbx_idx,
>> + u16 param, u8 *data);
>> +
>> +#endif /* __LINUX_MFD_PECI_CLIENT_H */
>
^ permalink raw reply
* [PATCH v8 08/12] mfd: intel-peci-client: Add PECI client MFD driver
From: Lee Jones @ 2018-10-25 5:30 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <db7570cb-8f63-b551-d119-c54270ce6139@linux.intel.com>
On Wed, 24 Oct 2018, Jae Hyun Yoo wrote:
> On 10/24/2018 3:59 AM, Lee Jones wrote:
> > On Tue, 18 Sep 2018, Jae Hyun Yoo wrote:
> >
> > > This commit adds PECI client MFD driver.
> > >
>
> <snip>
[...]
> > > +bool peci_temp_need_update(struct temp_data *temp)
> > > +{
> > > + if (temp->valid &&
> > > + time_before(jiffies, temp->last_updated + UPDATE_INTERVAL))
> > > + return false;
> > > +
> > > + return true;
> > > +}
> > > +EXPORT_SYMBOL_GPL(peci_temp_need_update);
> > > +
> > > +void peci_temp_mark_updated(struct temp_data *temp)
> > > +{
> > > + temp->valid = 1;
> > > + temp->last_updated = jiffies;
> > > +}
> > > +EXPORT_SYMBOL_GPL(peci_temp_mark_updated);
> >
> > These are probably better suited as inline functions to be placed in
> > a header file. No need to export them, since they only use their own
> > data.
Also move them into the HWMON header file.
They have no business in MFD.
[...]
> > > +int peci_client_rd_pkg_cfg_cmd(struct peci_mfd *priv, u8 mbx_idx,
> >
> > This is gobbledegook. What's rd? Read?
> >
>
> Yes, the 'rd' means 'read'. I intended to keep command names as listed
> in the PECI specification such as RdPkgConfig, WrPkgConfig and so on.
> Should I change it to 'peci_client_read_package_config_command' ?
I looks/reads a lot nicer, don't you think?
[...]
--
Lee Jones [???]
Linaro Services Technical Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [[PATCH] 0/9] *** DMA support for UART in ASPEED's AST2500 ***
From: Rob Herring @ 2018-10-25 14:48 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1539749466-3912-1-git-send-email-open.sudheer@gmail.com>
On Wed, Oct 17, 2018 at 09:40:57AM +0530, sudheer.v wrote:
> DMA controller driver and UART dma client driver for aspeed's AST2500
>
> sudheer.v (9):
We need a full name for author and Signed-off-by.
> DT-changes-for-DMA-UART-of-AST2500
> Defconfig-changes-for-DMA-UART-of-AST2500
> configuration-for-DMA-of-AST2500
> Documentation-DTbindings-DMA-controller-of-AST2500
> DMA-driver-for-AST2500
> configuration-for-DMA-UART-of-AST2500
> Documentation-DTbindings-DMA-UART-of-AST2500
> DMA-UART-Driver-for-AST2500
> updating MAINTAINERS for DMA and DMA-UART drivers of AST2500
Your commit messages need some work. Use 'git log --oneline <dir>' on
each directory for inspiration as to what the subject should be.
DT bindings specifically are "dt-bindings: <binding dir>: ..."
Rob
^ permalink raw reply
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