RE: [PATCH 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver
> -Original Message- > From: Chen, Alvin > Sent: Tuesday, November 4, 2014 8:46 AM > To: 'Bryan O'Donoghue'; Tan, Raymond; Lee Jones; Samuel Ortiz > Cc: linux-kernel@vger.kernel.org; Shevchenko, Andriy > Subject: RE: [PATCH 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 > I2C-GPIO MFD Driver > > > > > On 03/11/14 07:39, Raymond Tan wrote: > > > + pdata->properties->irq = pdev->irq; > > > + pdata->properties->irq_shared = true; > > > > OK I see it. > > > > Thanks. > > > > My question is. How extensively have edge triggered interrupts been > > tested on the GPIO block ? > > > > The BSP reference code is quite explicit about not missing edge interrupts. > > > > Have you tested GPIO input in edge mode ? > We indeed test edge mode. Now all are moved to gpio-dwapb module which > has been accepted by maintainer. > The BSP code doesn't meet the requirement of upstream, so we totally > re-implement this feature in gpio-dwapb. > This patch only passes the necessary parameters and registers the platform > devices. > > Just double check, the support of Quark designware GPIO has been in 3.18-rc2. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver
>
> On 03/11/14 07:39, Raymond Tan wrote:
> > + pdata->properties->irq = pdev->irq;
> > + pdata->properties->irq_shared = true;
>
> OK I see it.
>
> Thanks.
>
> My question is. How extensively have edge triggered interrupts been tested on
> the GPIO block ?
>
> The BSP reference code is quite explicit about not missing edge interrupts.
>
> Have you tested GPIO input in edge mode ?
We indeed test edge mode. Now all are moved to gpio-dwapb module which has been
accepted by maintainer.
The BSP code doesn't meet the requirement of upstream, so we totally
re-implement this feature in gpio-dwapb.
This patch only passes the necessary parameters and registers the platform
devices.
>
> +irqreturn_t intel_qrk_gpio_isr(int irq, void *dev_id) {
> + irqreturn_t ret = IRQ_NONE;
> + u32 pending = 0, gpio = 0;
> + void __iomem *reg_pending = reg_base + PORTA_INT_STATUS;
> + void __iomem *reg_eoi = reg_base + PORTA_INT_EOI;
> +
> + /* Which pin (if any) triggered the interrupt */
> + while ((pending = ioread32(reg_pending))) {
> + /*
> + * Acknowledge all the asserted GPIO interrupt lines before
> + * serving them, so that we don't lose an edge.
> + * This has only effect on edge-triggered interrupts.
> + */
> + iowrite32(pending, reg_eoi);
> +
> + /* Serve each asserted interrupt */
> + do {
> + gpio = __ffs(pending);
> + generic_handle_irq(
> + gpio_to_irq(INTEL_QRK_GIP_GPIO_BASE + gpio));
> + pending &= ~BIT(gpio);
> + ret = IRQ_HANDLED;
> + } while(pending);
> + }
> +
> + return ret;
> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver
On 03/11/14 07:39, Raymond Tan wrote:
+ pdata-properties-irq = pdev-irq;
+ pdata-properties-irq_shared = true;
OK I see it.
Thanks.
My question is. How extensively have edge triggered interrupts been tested on
the GPIO block ?
The BSP reference code is quite explicit about not missing edge interrupts.
Have you tested GPIO input in edge mode ?
We indeed test edge mode. Now all are moved to gpio-dwapb module which has been
accepted by maintainer.
The BSP code doesn't meet the requirement of upstream, so we totally
re-implement this feature in gpio-dwapb.
This patch only passes the necessary parameters and registers the platform
devices.
+irqreturn_t intel_qrk_gpio_isr(int irq, void *dev_id) {
+ irqreturn_t ret = IRQ_NONE;
+ u32 pending = 0, gpio = 0;
+ void __iomem *reg_pending = reg_base + PORTA_INT_STATUS;
+ void __iomem *reg_eoi = reg_base + PORTA_INT_EOI;
+
+ /* Which pin (if any) triggered the interrupt */
+ while ((pending = ioread32(reg_pending))) {
+ /*
+ * Acknowledge all the asserted GPIO interrupt lines before
+ * serving them, so that we don't lose an edge.
+ * This has only effect on edge-triggered interrupts.
+ */
+ iowrite32(pending, reg_eoi);
+
+ /* Serve each asserted interrupt */
+ do {
+ gpio = __ffs(pending);
+ generic_handle_irq(
+ gpio_to_irq(INTEL_QRK_GIP_GPIO_BASE + gpio));
+ pending = ~BIT(gpio);
+ ret = IRQ_HANDLED;
+ } while(pending);
+ }
+
+ return ret;
+}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver
-Original Message- From: Chen, Alvin Sent: Tuesday, November 4, 2014 8:46 AM To: 'Bryan O'Donoghue'; Tan, Raymond; Lee Jones; Samuel Ortiz Cc: linux-kernel@vger.kernel.org; Shevchenko, Andriy Subject: RE: [PATCH 1/1] mfd: intel_quark_i2c_gpio: Add Intel Quark X1000 I2C-GPIO MFD Driver On 03/11/14 07:39, Raymond Tan wrote: + pdata-properties-irq = pdev-irq; + pdata-properties-irq_shared = true; OK I see it. Thanks. My question is. How extensively have edge triggered interrupts been tested on the GPIO block ? The BSP reference code is quite explicit about not missing edge interrupts. Have you tested GPIO input in edge mode ? We indeed test edge mode. Now all are moved to gpio-dwapb module which has been accepted by maintainer. The BSP code doesn't meet the requirement of upstream, so we totally re-implement this feature in gpio-dwapb. This patch only passes the necessary parameters and registers the platform devices. Just double check, the support of Quark designware GPIO has been in 3.18-rc2. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/2 v3] SPI: spi-pxa2xx: Add helpers for regiseters' accessing
> > > > All of those patches are in v3.18-rc1, so you may rebase on top of > > 3.18-rcX safely I guess. > > > Andy, I remember you ask me to rebase on the slave-dma tree (git.infraded.org) > for-linus branch, and the slave-dma for-linus branch will be reapplied on top > of > v3.19-rc1. > Just to confirm, these patches can be applied well on top of 3.18-rc2. > So now I need rebase on top of 3.18-rcX, isn't it? Just to confirm.
RE: [PATCH 1/2 v3] SPI: spi-pxa2xx: Add helpers for regiseters' accessing
> > On Thu, 2014-10-30 at 01:02 +, Chen, Alvin wrote: > > > > > > > > > > Hi Alvin, Weike. > > > > > > I'm not clear if these patches apply to the current tip-of-tree. > > > > > > I thought the action required here, was for a resend of patches to > > > ensure they applied to tip-of-tree ? > > > > > As I said before, it is based on the slave-dma tree (git.infraded.org) > > for-linus > branch, which will be reapplied on top of v3.19-rc1. > > > > That is to say these patches will be applied to slave-dma tree for-linus > > branch, > and then the slave-dma tree for-linus branch will be applied to v3.19. > > Andy, am I right? > > All of those patches are in v3.18-rc1, so you may rebase on top of 3.18-rcX > safely I guess. > Andy, I remember you ask me to rebase on the slave-dma tree (git.infraded.org) for-linus branch, and the slave-dma for-linus branch will be reapplied on top of v3.19-rc1. So now I need rebase on top of 3.18-rcX, isn't it? Just to confirm.
RE: [PATCH 1/2 v3] SPI: spi-pxa2xx: Add helpers for regiseters' accessing
On Thu, 2014-10-30 at 01:02 +, Chen, Alvin wrote: Hi Alvin, Weike. I'm not clear if these patches apply to the current tip-of-tree. I thought the action required here, was for a resend of patches to ensure they applied to tip-of-tree ? As I said before, it is based on the slave-dma tree (git.infraded.org) for-linus branch, which will be reapplied on top of v3.19-rc1. That is to say these patches will be applied to slave-dma tree for-linus branch, and then the slave-dma tree for-linus branch will be applied to v3.19. Andy, am I right? All of those patches are in v3.18-rc1, so you may rebase on top of 3.18-rcX safely I guess. Andy, I remember you ask me to rebase on the slave-dma tree (git.infraded.org) for-linus branch, and the slave-dma for-linus branch will be reapplied on top of v3.19-rc1. So now I need rebase on top of 3.18-rcX, isn't it? Just to confirm.
RE: [PATCH 1/2 v3] SPI: spi-pxa2xx: Add helpers for regiseters' accessing
All of those patches are in v3.18-rc1, so you may rebase on top of 3.18-rcX safely I guess. Andy, I remember you ask me to rebase on the slave-dma tree (git.infraded.org) for-linus branch, and the slave-dma for-linus branch will be reapplied on top of v3.19-rc1. Just to confirm, these patches can be applied well on top of 3.18-rc2. So now I need rebase on top of 3.18-rcX, isn't it? Just to confirm.
RE: [PATCH 1/2 v3] SPI: spi-pxa2xx: Add helpers for regiseters' accessing
> > > > Hi Alvin, Weike. > > I'm not clear if these patches apply to the current tip-of-tree. > > I thought the action required here, was for a resend of patches to ensure they > applied to tip-of-tree ? > As I said before, it is based on the slave-dma tree (git.infraded.org) for-linus branch, which will be reapplied on top of v3.19-rc1. That is to say these patches will be applied to slave-dma tree for-linus branch, and then the slave-dma tree for-linus branch will be applied to v3.19. Andy, am I right?
RE: [PATCH 1/2 v3] SPI: spi-pxa2xx: Add helpers for regiseters' accessing
Hi Alvin, Weike. I'm not clear if these patches apply to the current tip-of-tree. I thought the action required here, was for a resend of patches to ensure they applied to tip-of-tree ? As I said before, it is based on the slave-dma tree (git.infraded.org) for-linus branch, which will be reapplied on top of v3.19-rc1. That is to say these patches will be applied to slave-dma tree for-linus branch, and then the slave-dma tree for-linus branch will be applied to v3.19. Andy, am I right?
RE: [PATCH 1/2 v3] SPI: spi-pxa2xx: Add helpers for regiseters' accessing
Any update for these patches? Just want to follow-up.
Best regards，Alvin Chen
ICFS Platform Engineering Solution
Flex Services (CMMI Level 3, IQA2005, IQA2008), Greater Asia Region Intel
Information Technology Tel. 010-82171960
inet.8-7581960
Email. alvin.c...@intel.com
> -Original Message-
> From: Chen, Alvin
> Sent: Wednesday, October 8, 2014 11:50 PM
> To: Eric Miao; Russell King; Haojian Zhuang; Mark Brown
> Cc: linux-arm-ker...@lists.infradead.org; linux-...@vger.kernel.org;
> linux-kernel@vger.kernel.org; Westerberg, Mika; Kweh, Hock Leong; Ong, Boon
> Leong; Tan, Raymond; Chen, Alvin; Andy Shevchenko
> Subject: [PATCH 1/2 v3] SPI: spi-pxa2xx: Add helpers for regiseters' accessing
>
> There are several registers for SPI, and the registers of 'SSCR0' and 'SSCR1'
> are accessed frequently. This path is to introduce helper functions to
> simplify
> the accessing of 'SSCR0' and 'SSCR1'.
>
> Reviewed-by: Andy Shevchenko
> Acked-by: Mika Westerberg
> Signed-off-by: Weike Chen
> ---
> drivers/spi/spi-pxa2xx.c | 107
> --
> 1 file changed, 84 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c index
> 256c0ab..33dba9b 100644
> --- a/drivers/spi/spi-pxa2xx.c
> +++ b/drivers/spi/spi-pxa2xx.c
> @@ -80,6 +80,73 @@ static bool is_lpss_ssp(const struct driver_data
> *drv_data)
> return drv_data->ssp_type == LPSS_SSP; }
>
> +static u32 pxa2xx_spi_get_ssrc1_change_mask(const struct driver_data
> +*drv_data) {
> + switch (drv_data->ssp_type) {
> + default:
> + return SSCR1_CHANGE_MASK;
> + }
> +}
> +
> +static u32
> +pxa2xx_spi_get_rx_default_thre(const struct driver_data *drv_data) {
> + switch (drv_data->ssp_type) {
> + default:
> + return RX_THRESH_DFLT;
> + }
> +}
> +
> +static bool pxa2xx_spi_txfifo_full(const struct driver_data *drv_data)
> +{
> + void __iomem *reg = drv_data->ioaddr;
> + u32 mask;
> +
> + switch (drv_data->ssp_type) {
> + default:
> + mask = SSSR_TFL_MASK;
> + break;
> + }
> +
> + return (read_SSSR(reg) & mask) == mask; }
> +
> +static void pxa2xx_spi_clear_rx_thre(const struct driver_data *drv_data,
> + u32 *sccr1_reg)
> +{
> + u32 mask;
> +
> + switch (drv_data->ssp_type) {
> + default:
> + mask = SSCR1_RFT;
> + break;
> + }
> + *sccr1_reg &= ~mask;
> +}
> +
> +static void pxa2xx_spi_set_rx_thre(const struct driver_data *drv_data,
> +u32 *sccr1_reg, u32 threshold)
> +{
> + switch (drv_data->ssp_type) {
> + default:
> + *sccr1_reg |= SSCR1_RxTresh(threshold);
> + break;
> + }
> +}
> +
> +static u32 pxa2xx_configure_sscr0(const struct driver_data *drv_data,
> + u32 clk_div, u8 bits)
> +{
> + switch (drv_data->ssp_type) {
> + default:
> + return clk_div
> + | SSCR0_Motorola
> + | SSCR0_DataSize(bits > 16 ? bits - 16 : bits)
> + | SSCR0_SSE
> + | (bits > 16 ? SSCR0_EDSS : 0);
> + }
> +}
> +
> /*
> * Read and write LPSS SSP private registers. Caller must first check that
> * is_lpss_ssp() returns true before these can be called.
> @@ -234,7 +301,7 @@ static int null_writer(struct driver_data *drv_data)
> void __iomem *reg = drv_data->ioaddr;
> u8 n_bytes = drv_data->n_bytes;
>
> - if (((read_SSSR(reg) & SSSR_TFL_MASK) == SSSR_TFL_MASK)
> + if (pxa2xx_spi_txfifo_full(drv_data)
> || (drv_data->tx == drv_data->tx_end))
> return 0;
>
> @@ -262,7 +329,7 @@ static int u8_writer(struct driver_data *drv_data) {
> void __iomem *reg = drv_data->ioaddr;
>
> - if (((read_SSSR(reg) & SSSR_TFL_MASK) == SSSR_TFL_MASK)
> + if (pxa2xx_spi_txfifo_full(drv_data)
> || (drv_data->tx == drv_data->tx_end))
> return 0;
>
> @@ -289,7 +356,7 @@ static int u16_writer(struct driver_data *drv_data) {
> void __iomem *reg = drv_data->ioaddr;
>
> - if (((read_SSSR(reg) & SSSR_TFL_MASK) == SSSR_TFL_MASK)
> + if (pxa2xx_spi_txfifo_full(drv_data)
> || (drv_data->tx == drv_data->tx_end))
> return 0;
>
> @@ -316,7 +383,7 @@ static int u32_writer(struct driver_data *drv_data) {
> void __iomem *reg = drv_data->ioaddr;
>
> - if (((re
RE: [PATCH 1/2 v3] SPI: spi-pxa2xx: Add helpers for regiseters' accessing
Any update for these patches? Just want to follow-up.
Best regards，Alvin Chen
ICFS Platform Engineering Solution
Flex Services (CMMI Level 3, IQA2005, IQA2008), Greater Asia Region Intel
Information Technology Tel. 010-82171960
inet.8-7581960
Email. alvin.c...@intel.com
-Original Message-
From: Chen, Alvin
Sent: Wednesday, October 8, 2014 11:50 PM
To: Eric Miao; Russell King; Haojian Zhuang; Mark Brown
Cc: linux-arm-ker...@lists.infradead.org; linux-...@vger.kernel.org;
linux-kernel@vger.kernel.org; Westerberg, Mika; Kweh, Hock Leong; Ong, Boon
Leong; Tan, Raymond; Chen, Alvin; Andy Shevchenko
Subject: [PATCH 1/2 v3] SPI: spi-pxa2xx: Add helpers for regiseters' accessing
There are several registers for SPI, and the registers of 'SSCR0' and 'SSCR1'
are accessed frequently. This path is to introduce helper functions to
simplify
the accessing of 'SSCR0' and 'SSCR1'.
Reviewed-by: Andy Shevchenko andriy.shevche...@linux.intel.com
Acked-by: Mika Westerberg mika.westerb...@linux.intel.com
Signed-off-by: Weike Chen alvin.c...@intel.com
---
drivers/spi/spi-pxa2xx.c | 107
--
1 file changed, 84 insertions(+), 23 deletions(-)
diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c index
256c0ab..33dba9b 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -80,6 +80,73 @@ static bool is_lpss_ssp(const struct driver_data
*drv_data)
return drv_data-ssp_type == LPSS_SSP; }
+static u32 pxa2xx_spi_get_ssrc1_change_mask(const struct driver_data
+*drv_data) {
+ switch (drv_data-ssp_type) {
+ default:
+ return SSCR1_CHANGE_MASK;
+ }
+}
+
+static u32
+pxa2xx_spi_get_rx_default_thre(const struct driver_data *drv_data) {
+ switch (drv_data-ssp_type) {
+ default:
+ return RX_THRESH_DFLT;
+ }
+}
+
+static bool pxa2xx_spi_txfifo_full(const struct driver_data *drv_data)
+{
+ void __iomem *reg = drv_data-ioaddr;
+ u32 mask;
+
+ switch (drv_data-ssp_type) {
+ default:
+ mask = SSSR_TFL_MASK;
+ break;
+ }
+
+ return (read_SSSR(reg) mask) == mask; }
+
+static void pxa2xx_spi_clear_rx_thre(const struct driver_data *drv_data,
+ u32 *sccr1_reg)
+{
+ u32 mask;
+
+ switch (drv_data-ssp_type) {
+ default:
+ mask = SSCR1_RFT;
+ break;
+ }
+ *sccr1_reg = ~mask;
+}
+
+static void pxa2xx_spi_set_rx_thre(const struct driver_data *drv_data,
+u32 *sccr1_reg, u32 threshold)
+{
+ switch (drv_data-ssp_type) {
+ default:
+ *sccr1_reg |= SSCR1_RxTresh(threshold);
+ break;
+ }
+}
+
+static u32 pxa2xx_configure_sscr0(const struct driver_data *drv_data,
+ u32 clk_div, u8 bits)
+{
+ switch (drv_data-ssp_type) {
+ default:
+ return clk_div
+ | SSCR0_Motorola
+ | SSCR0_DataSize(bits 16 ? bits - 16 : bits)
+ | SSCR0_SSE
+ | (bits 16 ? SSCR0_EDSS : 0);
+ }
+}
+
/*
* Read and write LPSS SSP private registers. Caller must first check that
* is_lpss_ssp() returns true before these can be called.
@@ -234,7 +301,7 @@ static int null_writer(struct driver_data *drv_data)
void __iomem *reg = drv_data-ioaddr;
u8 n_bytes = drv_data-n_bytes;
- if (((read_SSSR(reg) SSSR_TFL_MASK) == SSSR_TFL_MASK)
+ if (pxa2xx_spi_txfifo_full(drv_data)
|| (drv_data-tx == drv_data-tx_end))
return 0;
@@ -262,7 +329,7 @@ static int u8_writer(struct driver_data *drv_data) {
void __iomem *reg = drv_data-ioaddr;
- if (((read_SSSR(reg) SSSR_TFL_MASK) == SSSR_TFL_MASK)
+ if (pxa2xx_spi_txfifo_full(drv_data)
|| (drv_data-tx == drv_data-tx_end))
return 0;
@@ -289,7 +356,7 @@ static int u16_writer(struct driver_data *drv_data) {
void __iomem *reg = drv_data-ioaddr;
- if (((read_SSSR(reg) SSSR_TFL_MASK) == SSSR_TFL_MASK)
+ if (pxa2xx_spi_txfifo_full(drv_data)
|| (drv_data-tx == drv_data-tx_end))
return 0;
@@ -316,7 +383,7 @@ static int u32_writer(struct driver_data *drv_data) {
void __iomem *reg = drv_data-ioaddr;
- if (((read_SSSR(reg) SSSR_TFL_MASK) == SSSR_TFL_MASK)
+ if (pxa2xx_spi_txfifo_full(drv_data)
|| (drv_data-tx == drv_data-tx_end))
return 0;
@@ -508,8 +575,9 @@ static irqreturn_t interrupt_transfer(struct
driver_data *drv_data)
* remaining RX bytes.
*/
if (pxa25x_ssp_comp(drv_data)) {
+ u32 rx_thre;
- sccr1_reg = ~SSCR1_RFT;
+ pxa2xx_spi_clear_rx_thre(drv_data, sccr1_reg
RE: [PATCH 0/2 v3] SPI: spi-pxa2xx: Add support for Intel Quark X1000 SPI controller
Hi Mark, Any update for these patches? Just want to follow-up. Best regards，Alvin Chen ICFS Platform Engineering Solution Flex Services (CMMI Level 3, IQA2005, IQA2008), Greater Asia Region Intel Information Technology Tel. 010-82171960 inet.8-7581960 Email. alvin.c...@intel.com > -Original Message- > From: Chen, Alvin > Sent: Wednesday, October 8, 2014 11:50 PM > To: Eric Miao; Russell King; Haojian Zhuang; Mark Brown > Cc: linux-arm-ker...@lists.infradead.org; linux-...@vger.kernel.org; > linux-kernel@vger.kernel.org; Westerberg, Mika; Kweh, Hock Leong; Ong, Boon > Leong; Tan, Raymond; Chen, Alvin; Andy Shevchenko > Subject: [PATCH 0/2 v3] SPI: spi-pxa2xx: Add support for Intel Quark X1000 SPI > controller > > Hi, > Intel Quark X1000 consists of two SPI controllers which can be PCI enumerated. > SPI-PXA2XX PCI layer doesn't support it. Thus, we add support for Intel Quark > X1000 SPI as well. > > --- > v3: > [PATCH 1/2] > * Improve the commit message. > * A couple of minor fixes. > [PATCH 2/2] > * Set '.num_chipselect' to '1' instead of '4'. > * A couple of minor fixes. > > v2: > Split into two patches: one is for helper functions, > and another is for quark supporting. > > [PATCH 1/2] > * Add helper functions to access registers. > * Use 'switch' instead of 'if-else'. > [PATCH 2/2] > * Use 'switch' instead of 'if-else'. > * Add comments for the 'dds'&'clk_div' caculation. > * A couple of minor fixes. > > Weike Chen (2): > SPI: spi-pxa2xx: Add helpers for regiseters' accessing > SPI: spi-pxa2xx: SPI support for Intel Quark X1000 > > drivers/spi/spi-pxa2xx-pci.c |8 ++ > drivers/spi/spi-pxa2xx.c | 304 > -- > drivers/spi/spi-pxa2xx.h | 16 ++- > include/linux/pxa2xx_ssp.h | 21 +++ > 4 files changed, 304 insertions(+), 45 deletions(-) > > -- > 1.7.9.5
RE: [PATCH 0/2 v3] SPI: spi-pxa2xx: Add support for Intel Quark X1000 SPI controller
Hi Mark, Any update for these patches? Just want to follow-up. Best regards，Alvin Chen ICFS Platform Engineering Solution Flex Services (CMMI Level 3, IQA2005, IQA2008), Greater Asia Region Intel Information Technology Tel. 010-82171960 inet.8-7581960 Email. alvin.c...@intel.com -Original Message- From: Chen, Alvin Sent: Wednesday, October 8, 2014 11:50 PM To: Eric Miao; Russell King; Haojian Zhuang; Mark Brown Cc: linux-arm-ker...@lists.infradead.org; linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Westerberg, Mika; Kweh, Hock Leong; Ong, Boon Leong; Tan, Raymond; Chen, Alvin; Andy Shevchenko Subject: [PATCH 0/2 v3] SPI: spi-pxa2xx: Add support for Intel Quark X1000 SPI controller Hi, Intel Quark X1000 consists of two SPI controllers which can be PCI enumerated. SPI-PXA2XX PCI layer doesn't support it. Thus, we add support for Intel Quark X1000 SPI as well. --- v3: [PATCH 1/2] * Improve the commit message. * A couple of minor fixes. [PATCH 2/2] * Set '.num_chipselect' to '1' instead of '4'. * A couple of minor fixes. v2: Split into two patches: one is for helper functions, and another is for quark supporting. [PATCH 1/2] * Add helper functions to access registers. * Use 'switch' instead of 'if-else'. [PATCH 2/2] * Use 'switch' instead of 'if-else'. * Add comments for the 'dds''clk_div' caculation. * A couple of minor fixes. Weike Chen (2): SPI: spi-pxa2xx: Add helpers for regiseters' accessing SPI: spi-pxa2xx: SPI support for Intel Quark X1000 drivers/spi/spi-pxa2xx-pci.c |8 ++ drivers/spi/spi-pxa2xx.c | 304 -- drivers/spi/spi-pxa2xx.h | 16 ++- include/linux/pxa2xx_ssp.h | 21 +++ 4 files changed, 304 insertions(+), 45 deletions(-) -- 1.7.9.5
RE: [PATCH 2/2 v3] SPI: spi-pxa2xx: SPI support for Intel Quark X1000
> Hi Alvin, > > Doesn't apply. > > Applying: SPI: spi-pxa2xx: SPI support for Intel Quark X1000 > error: patch failed: drivers/spi/spi-pxa2xx-pci.c:19 > error: drivers/spi/spi-pxa2xx-pci.c: patch does not apply Patch failed at > 0002 SPI: > spi-pxa2xx: SPI support for Intel Quark X1000 > It is based on the slave-dma tree (git.infraded.org) for-linus branch, which will be reapplied on top of v3.19-rc1. More information can be got from Andy. > -- > Bryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2 v3] SPI: spi-pxa2xx: SPI support for Intel Quark X1000
Hi Alvin, Doesn't apply. Applying: SPI: spi-pxa2xx: SPI support for Intel Quark X1000 error: patch failed: drivers/spi/spi-pxa2xx-pci.c:19 error: drivers/spi/spi-pxa2xx-pci.c: patch does not apply Patch failed at 0002 SPI: spi-pxa2xx: SPI support for Intel Quark X1000 It is based on the slave-dma tree (git.infraded.org) for-linus branch, which will be reapplied on top of v3.19-rc1. More information can be got from Andy. -- Bryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2 v2] SPI: spi-pxa2xx: SPI support for Intel Quark X1000
> > On 29/09/14 15:22, Weike Chen wrote: > > + .num_chipselect = 4, > > How is this right ? > > There's only one physical chip-select line per SPI master... > > It's a 1:1 mapping. > Now, we have another board which can support 4 slave spi per master, but not only Galileo. Since that board is not public, after discussing with team, we decide to make the upstream code to support '1'. I will change it back to .num_chipselect = 1, > Best, > Bryan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2 v2] SPI: spi-pxa2xx: SPI support for Intel Quark X1000
On 29/09/14 15:22, Weike Chen wrote: + .num_chipselect = 4, How is this right ? There's only one physical chip-select line per SPI master... It's a 1:1 mapping. Now, we have another board which can support 4 slave spi per master, but not only Galileo. Since that board is not public, after discussing with team, we decide to make the upstream code to support '1'. I will change it back to .num_chipselect = 1, Best, Bryan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2 v2] SPI: spi-pxa2xx: SPI support for Intel Quark X1000
> > The SPI memory mapped I/O registers supported by Quark are different > > from the current implementation, and Quark only supports the registers > > of 'SSCR0', 'SSCR1', 'SSSR', 'SSDR', and 'DDS_RATE'. This patch is to > > enable the SPI for Intel Quark X1000. > > > > This piece of work is derived from Dan O'Donovan's initial work for > > Intel Quark > > X1000 SPI enabling. > > Minor comments are below. > > After addressing them > Reviewed-by: Andy Shevchenko > OK. > > + case QUARK_X1000_SSP: > > + return RX_THRESH_QUARK_X1000_DFLT; > > default: > > return RX_THRESH_DFLT; > > } > > + > > Redundant empty line. > OK. I will remove it. > > static void pxa2xx_spi_clear_rx_thre(const struct driver_data *drv_data, > > -u32 *sccr1_reg) > > + u32 *sccr1_reg) > > Unnecessary change. > Improve it in the first patch. > > static void pxa2xx_spi_set_rx_thre(const struct driver_data *drv_data, > > - u32 *sccr1_reg, u32 threshold) > > + u32 *sccr1_reg, u32 threshold) > > Ditto. > > Or you may do that in the first patch. > Improve it in the first patch. > > > > pxa2xx_spi_set_rx_thre(drv_data, _reg, > > - rx_thre); > > + rx_thre); > > Ditto. > Improve it in the first patch. > > + if (is_quark_x1000_ssp(drv_data) && > > + (read_DDS_RATE(reg) != chip->dds_rate)) > > Could it be one line? > No, it is beyond 80 characters if it is in one line.
RE: [PATCH 1/2 v2] SPI: spi-pxa2xx: Add helpers for regiseters' accessing
> I'm okay with the current version, though I have few minor comments below. > > > Introduce helper functions to access the 'SSCR0' and 'SSCR1'. > > > > Like you said in the summary there are many accessors to many registers, not > only cr1/cr0. Perhaps, you may extend your commit message. > OK. > In any case > Reviewed-by: Andy Shevchenko > OK. > > + > > /* > > * Read and write LPSS SSP private registers. Caller must first check that > > * is_lpss_ssp() returns true before these can be called. > > @@ -234,7 +301,7 @@ static int null_writer(struct driver_data *drv_data) > > void __iomem *reg = drv_data->ioaddr; > > u8 n_bytes = drv_data->n_bytes; > > > > - if (((read_SSSR(reg) & SSSR_TFL_MASK) == SSSR_TFL_MASK) > > + if (pxa2xx_spi_txfifo_full(drv_data) > > || (drv_data->tx == drv_data->tx_end)) > > Just wondering if those two could fit one line. > No, if make the two in one line, it is 84 characters.
RE: [PATCH 1/2 v2] SPI: spi-pxa2xx: Add helpers for regiseters' accessing
I'm okay with the current version, though I have few minor comments below. Introduce helper functions to access the 'SSCR0' and 'SSCR1'. Like you said in the summary there are many accessors to many registers, not only cr1/cr0. Perhaps, you may extend your commit message. OK. In any case Reviewed-by: Andy Shevchenko andriy.shevche...@linux.intel.com OK. + /* * Read and write LPSS SSP private registers. Caller must first check that * is_lpss_ssp() returns true before these can be called. @@ -234,7 +301,7 @@ static int null_writer(struct driver_data *drv_data) void __iomem *reg = drv_data-ioaddr; u8 n_bytes = drv_data-n_bytes; - if (((read_SSSR(reg) SSSR_TFL_MASK) == SSSR_TFL_MASK) + if (pxa2xx_spi_txfifo_full(drv_data) || (drv_data-tx == drv_data-tx_end)) Just wondering if those two could fit one line. No, if make the two in one line, it is 84 characters.
RE: [PATCH 2/2 v2] SPI: spi-pxa2xx: SPI support for Intel Quark X1000
The SPI memory mapped I/O registers supported by Quark are different from the current implementation, and Quark only supports the registers of 'SSCR0', 'SSCR1', 'SSSR', 'SSDR', and 'DDS_RATE'. This patch is to enable the SPI for Intel Quark X1000. This piece of work is derived from Dan O'Donovan's initial work for Intel Quark X1000 SPI enabling. Minor comments are below. After addressing them Reviewed-by: Andy Shevchenko andriy.shevche...@linux.intel.com OK. + case QUARK_X1000_SSP: + return RX_THRESH_QUARK_X1000_DFLT; default: return RX_THRESH_DFLT; } + Redundant empty line. OK. I will remove it. static void pxa2xx_spi_clear_rx_thre(const struct driver_data *drv_data, -u32 *sccr1_reg) + u32 *sccr1_reg) Unnecessary change. Improve it in the first patch. static void pxa2xx_spi_set_rx_thre(const struct driver_data *drv_data, - u32 *sccr1_reg, u32 threshold) + u32 *sccr1_reg, u32 threshold) Ditto. Or you may do that in the first patch. Improve it in the first patch. pxa2xx_spi_set_rx_thre(drv_data, sccr1_reg, - rx_thre); + rx_thre); Ditto. Improve it in the first patch. + if (is_quark_x1000_ssp(drv_data) + (read_DDS_RATE(reg) != chip-dds_rate)) Could it be one line? No, it is beyond 80 characters if it is in one line.
RE: [PATCH] SPI: spi-pxa2xx: SPI support for Intel Quark X1000
>
> >
> > > +/* see Quark SPI data sheet for implementation rationale */ static
> > > +u32 quark_x1000_set_clk_regvals(u32 rate, u32 *dds, u32 *clk_div) {
> >
> > Please document this in the driver - I don't know if this datasheet is
> > public but even if it is it may not stay that way.
>
> Datasheet is public.
> I'm just wondering if we can use just a formula instead of table.
>
As I said before, there are two formulas, but for the given rate, from the
formulas, we can get a lot of pairs,
and we also need to select them according to the guidelines which is not
formula. The table the datasheet given should be
the best one to meet the jitter and duty cycles as the datasheet documented.
RE: [PATCH] SPI: spi-pxa2xx: SPI support for Intel Quark X1000
>
> > +static u32 pxa2xx_spi_get_ssrc1_change_mask(const struct driver_data
> > +*drv_data) {
> > + if (!is_quark_x1000_ssp(drv_data))
> > + return SSCR1_CHANGE_MASK;
> > +
> > + return QUARK_X1000_SSCR1_CHANGE_MASK; }
>
> These functions would be much better written as switch statements - think
> how they're going to look when we've got another controller which needs
> custom values. It might also be helpful for review to have two patches, one
> splitting things out into the functions and another adding the Quark support.
>
OK. Let me use switch. BTW, for two patches, actually, using helpers is due to
we support quark.
Do you mean the first patch just introduce helpers, maybe it only support one
type, then another patch to add
quark supporting. Am I right?
> > +/* see Quark SPI data sheet for implementation rationale */ static
> > +u32 quark_x1000_set_clk_regvals(u32 rate, u32 *dds, u32 *clk_div) {
>
> Please document this in the driver - I don't know if this datasheet is public
> but
> even if it is it may not stay that way.
OK. I will document it.
>
> > @@ -613,6 +759,8 @@ static void pump_transfers(unsigned long data)
> > u32 cr1;
> > u32 dma_thresh = drv_data->cur_chip->dma_threshold;
> > u32 dma_burst = drv_data->cur_chip->dma_burst_size;
> > + u32 change_mask = pxa2xx_spi_get_ssrc1_change_mask(drv_data);
> > +
> >
>
> Extra blank line being added here.
>
I will remove it.
> > @@ -145,6 +147,9 @@ static inline int pxa25x_ssp_comp(struct driver_data
> *drv_data)
> > return 1;
> > if (drv_data->ssp_type == CE4100_SSP)
> > return 1;
> > + if (drv_data->ssp_type == QUARK_X1000_SSP)
> > + return 1;
> > +
> > return 0;
> > }
>
> Things like this should also be refactored into switch statements - in general
> anything that's deciding what to do based on ssp_type probably ought to be
> using switch statements.
OK. I will use switch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] SPI: spi-pxa2xx: SPI support for Intel Quark X1000
+static u32 pxa2xx_spi_get_ssrc1_change_mask(const struct driver_data
+*drv_data) {
+ if (!is_quark_x1000_ssp(drv_data))
+ return SSCR1_CHANGE_MASK;
+
+ return QUARK_X1000_SSCR1_CHANGE_MASK; }
These functions would be much better written as switch statements - think
how they're going to look when we've got another controller which needs
custom values. It might also be helpful for review to have two patches, one
splitting things out into the functions and another adding the Quark support.
OK. Let me use switch. BTW, for two patches, actually, using helpers is due to
we support quark.
Do you mean the first patch just introduce helpers, maybe it only support one
type, then another patch to add
quark supporting. Am I right?
+/* see Quark SPI data sheet for implementation rationale */ static
+u32 quark_x1000_set_clk_regvals(u32 rate, u32 *dds, u32 *clk_div) {
Please document this in the driver - I don't know if this datasheet is public
but
even if it is it may not stay that way.
OK. I will document it.
@@ -613,6 +759,8 @@ static void pump_transfers(unsigned long data)
u32 cr1;
u32 dma_thresh = drv_data-cur_chip-dma_threshold;
u32 dma_burst = drv_data-cur_chip-dma_burst_size;
+ u32 change_mask = pxa2xx_spi_get_ssrc1_change_mask(drv_data);
+
Extra blank line being added here.
I will remove it.
@@ -145,6 +147,9 @@ static inline int pxa25x_ssp_comp(struct driver_data
*drv_data)
return 1;
if (drv_data-ssp_type == CE4100_SSP)
return 1;
+ if (drv_data-ssp_type == QUARK_X1000_SSP)
+ return 1;
+
return 0;
}
Things like this should also be refactored into switch statements - in general
anything that's deciding what to do based on ssp_type probably ought to be
using switch statements.
OK. I will use switch.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] SPI: spi-pxa2xx: SPI support for Intel Quark X1000
+/* see Quark SPI data sheet for implementation rationale */ static
+u32 quark_x1000_set_clk_regvals(u32 rate, u32 *dds, u32 *clk_div) {
Please document this in the driver - I don't know if this datasheet is
public but even if it is it may not stay that way.
Datasheet is public.
I'm just wondering if we can use just a formula instead of table.
As I said before, there are two formulas, but for the given rate, from the
formulas, we can get a lot of dds, clk_div pairs,
and we also need to select them according to the guidelines which is not
formula. The table the datasheet given should be
the best one to meet the jitter and duty cycles as the datasheet documented.
RE: [PATCH 3/3 v2] GPIO: gpio-dwapb: Suspend & Resume PM enabling
>
> > +/* Store GPIO context across system-wide suspend/resume transitions
> > +*/ static struct dwapb_context {
> > + u32 data[DWAPB_MAX_PORTS];
> > + u32 dir[DWAPB_MAX_PORTS];
> > + u32 ext[DWAPB_MAX_PORTS];
> > + u32 int_en;
> > + u32 int_mask;
> > + u32 int_type;
> > + u32 int_pol;
> > + u32 int_deb;
> > +} dwapb_context;
>
> NAK, this should STILL be part of the device state container. Not a local
> variable.
>
> I've said this before. Please fix this, thanks.
>
Linus, please review Version 5 that I sent on Sep.17th. I have fixed it in the
v5.
RE: [PATCH 3/3 v2] GPIO: gpio-dwapb: Suspend Resume PM enabling
+/* Store GPIO context across system-wide suspend/resume transitions
+*/ static struct dwapb_context {
+ u32 data[DWAPB_MAX_PORTS];
+ u32 dir[DWAPB_MAX_PORTS];
+ u32 ext[DWAPB_MAX_PORTS];
+ u32 int_en;
+ u32 int_mask;
+ u32 int_type;
+ u32 int_pol;
+ u32 int_deb;
+} dwapb_context;
NAK, this should STILL be part of the device state container. Not a local
variable.
I've said this before. Please fix this, thanks.
Linus, please review Version 5 that I sent on Sep.17th. I have fixed it in the
v5.
RE: [PATCH 1/4 v4] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
> > + if (pp->idx == 0 &&
> > + of_property_read_bool(port_np, "interrupt-controller")) {
> > + pp->irq = irq_of_parse_and_map(port_np, 0);
> > + if (!pp->irq) {
> > + dev_warn(dev, "no irq for bank %s\n",
> > +port_np->full_name);
> > + }
> > + } else {
> > + pp->irq = 0;
> > + }
>
> The else clause is not needed since pp->irq == 0 already, right?
>
Yes, while call kcalloc, the memory is set to '0'. I will improve it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 3/4 v4] GPIO: gpio-dwapb: Support Debounce
> > + struct bgpio_chip *bgc = to_bgpio_chip(gc);
> > + struct dwapb_gpio_port *port =
> > + container_of(bgc, struct dwapb_gpio_port, bgc);
>
> Does it make sense to introduce an inline helper like
>
> static inline struct dwapb_gpio_port *to_dwapb_gpio_port(struct bgpio_chip
> *gc) {...}
>
> ?
OK.
> There is also another place where it could be used.
>
> > + struct dwapb_gpio *gpio = port->gpio;
> > + unsigned long flags, val_deb;
> > + unsigned long mask = bgc->pin2mask(bgc, offset);
> > +
> > + spin_lock_irqsave(>lock, flags);
> > +
> > + val_deb = dwapb_read(gpio, GPIO_PORTA_DEBOUNCE);
> > + if (debounce)
> > + dwapb_write(gpio, GPIO_PORTA_DEBOUNCE, mask | val_deb);
>
> May you put value on the first place? Like 'val_deb | mask'.
OK.
> > + else
> > + dwapb_write(gpio, GPIO_PORTA_DEBOUNCE, ~mask & val_deb);
>
> Ditto.
>
OK.
> > +
RE: [PATCH 4/4 v4] GPIO: gpio-dwapb: Suspend & Resume PM enabling
>
> > Reviewed-by: Hock Leong Kweh
>
> You still keep that guy as reviewer in a whole series, however I didn't see a
> word from him here. How is it possible?
In our internal review, he gave me a lot of suggestions.
> > + for (i = 0; i < gpio->nr_ports; i++) {
> > + unsigned int offset;
> > + unsigned int idx = gpio->ports[i].idx;
> > + struct dwapb_context *ctx = gpio->ports[i].ctx;
> > +
> > + if (!ctx) {
> > + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> > + gpio->ports[i].ctx = ctx;
> > + }
>
> I don't think it's a right place to allocate this resource, especially with
> devm_
> helper. Can you move this to probe() stage?
>
> Or you even can embed contex structure inside chip one with #ifdef
> CONFIG_PM_SLEEP.
>
OK, I will improve it.
> Maybe others could comment on this.
>
> > +
> > + offset = GPIO_SWPORTA_DDR + (idx * GPIO_SWPORT_DDR_SIZE);
>
> No need to have parentheses here. Check the code below as well.
OK. I will remove them.
RE: [PATCH 4/4 v4] GPIO: gpio-dwapb: Suspend Resume PM enabling
Reviewed-by: Hock Leong Kweh hock.leong.k...@intel.com
You still keep that guy as reviewer in a whole series, however I didn't see a
word from him here. How is it possible?
In our internal review, he gave me a lot of suggestions.
+ for (i = 0; i gpio-nr_ports; i++) {
+ unsigned int offset;
+ unsigned int idx = gpio-ports[i].idx;
+ struct dwapb_context *ctx = gpio-ports[i].ctx;
+
+ if (!ctx) {
+ ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+ gpio-ports[i].ctx = ctx;
+ }
I don't think it's a right place to allocate this resource, especially with
devm_
helper. Can you move this to probe() stage?
Or you even can embed contex structure inside chip one with #ifdef
CONFIG_PM_SLEEP.
OK, I will improve it.
Maybe others could comment on this.
+
+ offset = GPIO_SWPORTA_DDR + (idx * GPIO_SWPORT_DDR_SIZE);
No need to have parentheses here. Check the code below as well.
OK. I will remove them.
RE: [PATCH 3/4 v4] GPIO: gpio-dwapb: Support Debounce
+ struct bgpio_chip *bgc = to_bgpio_chip(gc);
+ struct dwapb_gpio_port *port =
+ container_of(bgc, struct dwapb_gpio_port, bgc);
Does it make sense to introduce an inline helper like
static inline struct dwapb_gpio_port *to_dwapb_gpio_port(struct bgpio_chip
*gc) {...}
?
OK.
There is also another place where it could be used.
+ struct dwapb_gpio *gpio = port-gpio;
+ unsigned long flags, val_deb;
+ unsigned long mask = bgc-pin2mask(bgc, offset);
+
+ spin_lock_irqsave(bgc-lock, flags);
+
+ val_deb = dwapb_read(gpio, GPIO_PORTA_DEBOUNCE);
+ if (debounce)
+ dwapb_write(gpio, GPIO_PORTA_DEBOUNCE, mask | val_deb);
May you put value on the first place? Like 'val_deb | mask'.
OK.
+ else
+ dwapb_write(gpio, GPIO_PORTA_DEBOUNCE, ~mask val_deb);
Ditto.
OK.
+
RE: [PATCH 1/4 v4] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
+ if (pp-idx == 0
+ of_property_read_bool(port_np, interrupt-controller)) {
+ pp-irq = irq_of_parse_and_map(port_np, 0);
+ if (!pp-irq) {
+ dev_warn(dev, no irq for bank %s\n,
+port_np-full_name);
+ }
+ } else {
+ pp-irq = 0;
+ }
The else clause is not needed since pp-irq == 0 already, right?
Yes, while call kcalloc, the memory is set to '0'. I will improve it.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
>
> > >
> > > > > static int dwapb_gpio_probe(struct platform_device *pdev) {
> > > > > + int i;
> > > > > struct resource *res;
> > > > > struct dwapb_gpio *gpio;
> > > > > - struct device_node *np;
> > > > > int err;
> > > > > - unsigned int offs = 0;
> > > > > + struct device *dev = >dev;
> > > > > + struct dwapb_platform_data *pdata = dev_get_platdata(dev);
> > > > > + bool is_pdata_alloc = !pdata;
> > > >
> > > > Please combine the int's in one line (int err, i;) and put them as
> > > > the last one on this list. It looks the same to the compiler of
> > > > course, but more uniform for human eyes :)
> > >
> > > Do you think it's a good idea? In this case I, for example, would
> > > like to see int err as a separate line at the end of definition
> > > block. It would be better to distinguish counters and return code storage.
> > > Moreover, often counters would be unsigned int.
> >
> > If they are both 'int' they should be combined. If 'i' is changed to
> > be an unsigned int they would be separate.
>
> Linus, do you have any idea about it? I think it is not a big issue.
> >
Since no further feedbacks, I decide to use 'unsigned int i' to align the two
feedbacks, since 'i' is just a counter.
And will send a new version with just this changes later.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
static int dwapb_gpio_probe(struct platform_device *pdev) {
+ int i;
struct resource *res;
struct dwapb_gpio *gpio;
- struct device_node *np;
int err;
- unsigned int offs = 0;
+ struct device *dev = pdev-dev;
+ struct dwapb_platform_data *pdata = dev_get_platdata(dev);
+ bool is_pdata_alloc = !pdata;
Please combine the int's in one line (int err, i;) and put them as
the last one on this list. It looks the same to the compiler of
course, but more uniform for human eyes :)
Do you think it's a good idea? In this case I, for example, would
like to see int err as a separate line at the end of definition
block. It would be better to distinguish counters and return code storage.
Moreover, often counters would be unsigned int.
If they are both 'int' they should be combined. If 'i' is changed to
be an unsigned int they would be separate.
Linus, do you have any idea about it? I think it is not a big issue.
Since no further feedbacks, I decide to use 'unsigned int i' to align the two
feedbacks, since 'i' is just a counter.
And will send a new version with just this changes later.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
> >
> > > > static int dwapb_gpio_probe(struct platform_device *pdev) {
> > > > + int i;
> > > > struct resource *res;
> > > > struct dwapb_gpio *gpio;
> > > > - struct device_node *np;
> > > > int err;
> > > > - unsigned int offs = 0;
> > > > + struct device *dev = >dev;
> > > > + struct dwapb_platform_data *pdata = dev_get_platdata(dev);
> > > > + bool is_pdata_alloc = !pdata;
> > >
> > > Please combine the int's in one line (int err, i;) and put them as
> > > the last one on this list. It looks the same to the compiler of
> > > course, but more uniform for human eyes :)
> >
> > Do you think it's a good idea? In this case I, for example, would like
> > to see int err as a separate line at the end of definition block. It
> > would be better to distinguish counters and return code storage.
> > Moreover, often counters would be unsigned int.
>
> If they are both 'int' they should be combined. If 'i' is changed to be an
> unsigned int they would be separate.
Linus, do you have any idea about it? I think it is not a big issue.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
static int dwapb_gpio_probe(struct platform_device *pdev) {
+ int i;
struct resource *res;
struct dwapb_gpio *gpio;
- struct device_node *np;
int err;
- unsigned int offs = 0;
+ struct device *dev = pdev-dev;
+ struct dwapb_platform_data *pdata = dev_get_platdata(dev);
+ bool is_pdata_alloc = !pdata;
Please combine the int's in one line (int err, i;) and put them as
the last one on this list. It looks the same to the compiler of
course, but more uniform for human eyes :)
Do you think it's a good idea? In this case I, for example, would like
to see int err as a separate line at the end of definition block. It
would be better to distinguish counters and return code storage.
Moreover, often counters would be unsigned int.
If they are both 'int' they should be combined. If 'i' is changed to be an
unsigned int they would be separate.
Linus, do you have any idea about it? I think it is not a big issue.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/4 v3] GPIO: gpio-dwapb: Suspend & Resume PM enabling
> On Tue, 9 Sep 2014, Weike Chen wrote:
>
> >
> > struct dwapb_gpio;
> > +struct dwapb_context;
> >
> > struct dwapb_gpio_port {
> > struct bgpio_chip bgc;
> > boolis_registered;
> > struct dwapb_gpio *gpio;
> > + struct dwapb_context*ctx;
>
> Alvin,
>
> Will this build if CONFIG_PM_SLEEP is not defined?
Actually, PM_SLEEP is always set as 'y' in 'kerne/power/Kconfig'. But I
manually change it to 'n', this module can be compiled correctly.
You may be concern with 'ctx', and you can see 'ctx' accessing is always in
CONFIG_PM_SLEEP.
> Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/4 v3] GPIO: gpio-dwapb: Suspend Resume PM enabling
On Tue, 9 Sep 2014, Weike Chen wrote:
struct dwapb_gpio;
+struct dwapb_context;
struct dwapb_gpio_port {
struct bgpio_chip bgc;
boolis_registered;
struct dwapb_gpio *gpio;
+ struct dwapb_context*ctx;
Alvin,
Will this build if CONFIG_PM_SLEEP is not defined?
Actually, PM_SLEEP is always set as 'y' in 'kerne/power/Kconfig'. But I
manually change it to 'n', this module can be compiled correctly.
You may be concern with 'ctx', and you can see 'ctx' accessing is always in
CONFIG_PM_SLEEP.
Alan
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
> > > > Hi Alvin, > > > > I did a quick test and this looks like it works for me (with device tree). > > I had a couple of small fixes below. > It is very appreciated to help testing. > > > > Alan > > > > > > > > - port->bgc.gc.ngpio = ngpio; > > > - port->bgc.gc.of_node = port_np; > > > +#ifdef CONFIG_OF_GPIO > > > + port->bgc.gc.of_node = pp->node; > > > +#endif > > > > Please use 'if (IS_ENABLED(CONFIG_OF_GPIO)) as a conditional as you do > > elsewhere. > OK. > Alan, I just do a quick test, here we can't use 'IS_ENABLED', it can't be compiled without OF_GPIO set. Because 'gc.of_node' is not defined without 'OF_GPIO'. You can refer the structure of 'gc'. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
>
> Hi Alvin,
>
> I did a quick test and this looks like it works for me (with device tree).
> I had a couple of small fixes below.
It is very appreciated to help testing.
> Alan
>
> >
> > - port->bgc.gc.ngpio = ngpio;
> > - port->bgc.gc.of_node = port_np;
> > +#ifdef CONFIG_OF_GPIO
> > + port->bgc.gc.of_node = pp->node;
> > +#endif
>
> Please use 'if (IS_ENABLED(CONFIG_OF_GPIO)) as a conditional as you do
> elsewhere.
OK.
>
> > static int dwapb_gpio_probe(struct platform_device *pdev) {
> > + int i;
> > struct resource *res;
> > struct dwapb_gpio *gpio;
> > - struct device_node *np;
> > int err;
> > - unsigned int offs = 0;
> > + struct device *dev = >dev;
> > + struct dwapb_platform_data *pdata = dev_get_platdata(dev);
> > + bool is_pdata_alloc = !pdata;
>
> Please combine the int's in one line (int err, i;) and put them as the last
> one on
> this list. It looks the same to the compiler of course, but more uniform for
> human eyes :)
OK.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
> > > You cover this specific dependencies with inline ifdefs, but you > > > lose the CONFIG_OF depends by dropping it, and there are no such > > > checks in the probe routine. Assumptions of OF are not limited to probe in > this driver. > > > > > > While I would like to see this assumption properly abstracted, the > > > most expedient/immediate fix is probably to add a depends on OF above. > > > > Andriy, how do you think? > > > > How about > > depends on OF_GPIO || MFD_GPIO_DWAPB, or depends on OF_GPIO || > > MFD_INTEL_QUARK_I2C_GPIO? > > > > Upon closer inspection, I think my concern is invalid. the OF header already > tests for CONFIG_OF and provides no-op / -ENOSYS (tsk tsk) alternatives. So > long as you can compile with "#CONFIG_OF is not set" as it is, then I withdraw > my comment. Yes, it can be compiled with "#CONFIG_OF is not set" and "#CONFIG_OF_GPIO is not set" > > Sorry for the noise. > > -- > Darren Hart > Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
You cover this specific dependencies with inline ifdefs, but you lose the CONFIG_OF depends by dropping it, and there are no such checks in the probe routine. Assumptions of OF are not limited to probe in this driver. While I would like to see this assumption properly abstracted, the most expedient/immediate fix is probably to add a depends on OF above. Andriy, how do you think? How about depends on OF_GPIO || MFD_GPIO_DWAPB, or depends on OF_GPIO || MFD_INTEL_QUARK_I2C_GPIO? Upon closer inspection, I think my concern is invalid. the OF header already tests for CONFIG_OF and provides no-op / -ENOSYS (tsk tsk) alternatives. So long as you can compile with #CONFIG_OF is not set as it is, then I withdraw my comment. Yes, it can be compiled with #CONFIG_OF is not set and #CONFIG_OF_GPIO is not set Sorry for the noise. -- Darren Hart Intel Open Source Technology Center -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
Hi Alvin,
I did a quick test and this looks like it works for me (with device tree).
I had a couple of small fixes below.
It is very appreciated to help testing.
Alan
- port-bgc.gc.ngpio = ngpio;
- port-bgc.gc.of_node = port_np;
+#ifdef CONFIG_OF_GPIO
+ port-bgc.gc.of_node = pp-node;
+#endif
Please use 'if (IS_ENABLED(CONFIG_OF_GPIO)) as a conditional as you do
elsewhere.
OK.
static int dwapb_gpio_probe(struct platform_device *pdev) {
+ int i;
struct resource *res;
struct dwapb_gpio *gpio;
- struct device_node *np;
int err;
- unsigned int offs = 0;
+ struct device *dev = pdev-dev;
+ struct dwapb_platform_data *pdata = dev_get_platdata(dev);
+ bool is_pdata_alloc = !pdata;
Please combine the int's in one line (int err, i;) and put them as the last
one on
this list. It looks the same to the compiler of course, but more uniform for
human eyes :)
OK.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
Hi Alvin, I did a quick test and this looks like it works for me (with device tree). I had a couple of small fixes below. It is very appreciated to help testing. Alan - port-bgc.gc.ngpio = ngpio; - port-bgc.gc.of_node = port_np; +#ifdef CONFIG_OF_GPIO + port-bgc.gc.of_node = pp-node; +#endif Please use 'if (IS_ENABLED(CONFIG_OF_GPIO)) as a conditional as you do elsewhere. OK. Alan, I just do a quick test, here we can't use 'IS_ENABLED', it can't be compiled without OF_GPIO set. Because 'gc.of_node' is not defined without 'OF_GPIO'. You can refer the structure of 'gc'. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
> >@@ -136,7 +136,6 @@ config GPIO_DWAPB > > tristate "Synopsys DesignWare APB GPIO driver" > > select GPIO_GENERIC > > select GENERIC_IRQ_CHIP > >-depends on OF_GPIO > > You cover this specific dependencies with inline ifdefs, but you lose the > CONFIG_OF depends by dropping it, and there are no such checks in the probe > routine. Assumptions of OF are not limited to probe in this driver. > > While I would like to see this assumption properly abstracted, the most > expedient/immediate fix is probably to add a depends on OF above. Andriy, how do you think? How about depends on OF_GPIO || MFD_GPIO_DWAPB, or depends on OF_GPIO || MFD_INTEL_QUARK_I2C_GPIO? > -- > Darren > Intel Open Source Technology Center > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/4 v3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
@@ -136,7 +136,6 @@ config GPIO_DWAPB tristate Synopsys DesignWare APB GPIO driver select GPIO_GENERIC select GENERIC_IRQ_CHIP -depends on OF_GPIO You cover this specific dependencies with inline ifdefs, but you lose the CONFIG_OF depends by dropping it, and there are no such checks in the probe routine. Assumptions of OF are not limited to probe in this driver. While I would like to see this assumption properly abstracted, the most expedient/immediate fix is probably to add a depends on OF above. Andriy, how do you think? How about depends on OF_GPIO || MFD_GPIO_DWAPB, or depends on OF_GPIO || MFD_INTEL_QUARK_I2C_GPIO? -- Darren Intel Open Source Technology Center -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/3 v2] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
> On Friday 05 September 2014 12:02:01 Shevchenko, Andriy wrote:
> > > irq = irq_of_parse_and_map(node, 0); If (!irq) {
> > > pp->irq = -1;
> > > return;
> > > } else {
> > > pp->irq = irq;
> > > }
> > > Then the code looks strange.
> > >
> > > How do you think?
> >
> > If I understood correctly you messed up with hwirq vs. virq.
> > Otherwise you have mention that you are using virq everywhere (I guess
> > you may rename the field in the structure), but in this case the field
> > in the platform_data looks a bit strange.
>
> The field in platform_data should be the mapped virtual irq number, it makes
> no
> sense to use the hwirq unless you also add a pointer to the domain in which
> that hwirq exists.
>
> Also the output of irq_of_parse_and_map() is a mapped irq, as the name
> suggests.
>
I agree with Arnd. Here, the 'irq' is 'virq'.
Andriy, you may be confused by the code like 'irq_create_mapping'. For Quark
case, it has 8 GPIO pins, and each pin can trigger
interrupt, but all these interrupts are triggered by PCI irq which is shared.
The 'irq' in pdata is PCI irq. As all GPIO interrupts connect to the PCI
irq, once the GPIO interrupt is triggered, and the PCI irq handler will be
called 'dwapb_irq_handler_mfd'. And in 'dwapb_do_irq', it will read the
interrupt register to see the interrupt is triggered by which GPIO pin, and
'irq_create_mapping' is for this case.
.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/3 v2] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
> >
> > +#ifdef CONFIG_OF_GPIO
> > +
> > +static struct dwapb_platform_data *
> > +dwapb_gpio_get_pdata_of(struct device *dev) {
> > + struct device_node *node, *port_np;
> > + struct dwapb_platform_data *pdata;
> > + struct dwapb_port_property *pp;
> > + int nports;
> > + int i;
> > +
> > + node = dev->of_node;
> > + if (!node)
> > + return ERR_PTR(-ENODEV);
>
> Please replace the #ifdef above with
>
> if (!IS_ENABLED(CONFIG_OF_GPIO) || !node)
>
> here so get you proper compile-time coverage of the DT code path.
OK, I will improve it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/3 v2] GPIO: gpio-dwapb: Support Debounce
> > Since the 'debounce' feature also needs read/write, if we splite this > > patch, then for 'debounce', One patch use readl/writel, and another > > patch change to dwapb_read/write. It seems duplicate since the previous > patch use readl/writel and the fllowing one change it immediately. > > Since this patch is small, could we just make them together? How do you > think? > > Make the dwapb_writel/readl patch goes first in the series. > OK.
RE: [PATCH 2/3 v2] GPIO: gpio-dwapb: Support Debounce
Since the 'debounce' feature also needs read/write, if we splite this patch, then for 'debounce', One patch use readl/writel, and another patch change to dwapb_read/write. It seems duplicate since the previous patch use readl/writel and the fllowing one change it immediately. Since this patch is small, could we just make them together? How do you think? Make the dwapb_writel/readl patch goes first in the series. OK.
RE: [PATCH 1/3 v2] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
+#ifdef CONFIG_OF_GPIO
+
+static struct dwapb_platform_data *
+dwapb_gpio_get_pdata_of(struct device *dev) {
+ struct device_node *node, *port_np;
+ struct dwapb_platform_data *pdata;
+ struct dwapb_port_property *pp;
+ int nports;
+ int i;
+
+ node = dev-of_node;
+ if (!node)
+ return ERR_PTR(-ENODEV);
Please replace the #ifdef above with
if (!IS_ENABLED(CONFIG_OF_GPIO) || !node)
here so get you proper compile-time coverage of the DT code path.
OK, I will improve it.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/3 v2] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
On Friday 05 September 2014 12:02:01 Shevchenko, Andriy wrote:
irq = irq_of_parse_and_map(node, 0); If (!irq) {
pp-irq = -1;
return;
} else {
pp-irq = irq;
}
Then the code looks strange.
How do you think?
If I understood correctly you messed up with hwirq vs. virq.
Otherwise you have mention that you are using virq everywhere (I guess
you may rename the field in the structure), but in this case the field
in the platform_data looks a bit strange.
The field in platform_data should be the mapped virtual irq number, it makes
no
sense to use the hwirq unless you also add a pointer to the domain in which
that hwirq exists.
Also the output of irq_of_parse_and_map() is a mapped irq, as the name
suggests.
I agree with Arnd. Here, the 'irq' is 'virq'.
Andriy, you may be confused by the code like 'irq_create_mapping'. For Quark
case, it has 8 GPIO pins, and each pin can trigger
interrupt, but all these interrupts are triggered by PCI irq which is shared.
The 'irq' in pdata is PCI irq. As all GPIO interrupts connect to the PCI
irq, once the GPIO interrupt is triggered, and the PCI irq handler will be
called 'dwapb_irq_handler_mfd'. And in 'dwapb_do_irq', it will read the
interrupt register to see the interrupt is triggered by which GPIO pin, and
'irq_create_mapping' is for this case.
.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/3 v2] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
> > >
> > > - irq_set_chained_handler(irq, dwapb_irq_handler);
> > > - irq_set_handler_data(irq, gpio);
> > > + if (!pp->irq_shared) {
> > > + irq_set_chained_handler(pp->irq, dwapb_irq_handler);
> > > + irq_set_handler_data(pp->irq, gpio);
> > > + } else {
> > > + /*
> > > + * Request a shared IRQ since where MFD would have devices
> > > + * using the same irq pin
> > > + */
> > > + err = devm_request_irq(gpio->dev, pp->irq,
> > > +dwapb_irq_handler_mfd,
> > > +IRQF_SHARED, "gpio-dwapb-mfd", gpio);
> > > + if (err) {
> > > + dev_err(gpio->dev, "error requesting IRQ\n");
> > > + irq_domain_remove(gpio->domain);
> > > + gpio->domain = NULL;
> > > + return;
> > > + }
> > > + }
> > >
> >
> > I think this need some better documentation. Why is it safe to use
> > devm_request_irq rather than irq_set_chained_handler here?
>
> Usually it is preferred to use irq_set_chained_handler() for the chained
> handler
> so the handler does not show up in /proc/interrupts.
> This requires an exclusive non-shared handler which is not the case on the
> intel
> platform. So they have to use devm_request_irq() instead.
>
Yes, for Intel Quark, it has a single PCI function exporting a GPIO and I2C
controller, and
the irq is shared by GPIO and I2C, so we need shared irq as the comments said.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/3 v2] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
- irq_set_chained_handler(irq, dwapb_irq_handler);
- irq_set_handler_data(irq, gpio);
+ if (!pp-irq_shared) {
+ irq_set_chained_handler(pp-irq, dwapb_irq_handler);
+ irq_set_handler_data(pp-irq, gpio);
+ } else {
+ /*
+ * Request a shared IRQ since where MFD would have devices
+ * using the same irq pin
+ */
+ err = devm_request_irq(gpio-dev, pp-irq,
+dwapb_irq_handler_mfd,
+IRQF_SHARED, gpio-dwapb-mfd, gpio);
+ if (err) {
+ dev_err(gpio-dev, error requesting IRQ\n);
+ irq_domain_remove(gpio-domain);
+ gpio-domain = NULL;
+ return;
+ }
+ }
I think this need some better documentation. Why is it safe to use
devm_request_irq rather than irq_set_chained_handler here?
Usually it is preferred to use irq_set_chained_handler() for the chained
handler
so the handler does not show up in /proc/interrupts.
This requires an exclusive non-shared handler which is not the case on the
intel
platform. So they have to use devm_request_irq() instead.
Yes, for Intel Quark, it has a single PCI function exporting a GPIO and I2C
controller, and
the irq is shared by GPIO and I2C, so we need shared irq as the comments said.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/3 v2] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
> > -static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
> > +static u32 _dwapb_irq_handler(struct dwapb_gpio *gpio)
>
> What about dwapb_do_irq() ?
OK, I will improve it.
> > +static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id) {
> > + u32 worked;
> > + struct dwapb_gpio *gpio = (struct dwapb_gpio *)dev_id;
>
> No need to cast explicitly from void *.
OK.
> > - port->bgc.gc.ngpio = ngpio;
> > - port->bgc.gc.of_node = port_np;
> > +#ifdef CONFIG_OF_GPIO
>
> Do we really need this #ifdef ?
> of_node will be NULL anyway, or I missed something?
Yes, otherwise, can't compile it. Please refer 'struct gpio_chip', 'gc.of_node'
is in OF_GPIO micro also.
> > + if (pp->irq)
>
> irq == 0 is a valid hwirq (hardware irq) number. Yes, there is unlikely we
> have it
> somewhere, but still it's possible. And yes, IRQ framework doesn't work with
> virq == 0 (*virtual* irq), but accepts hwirq == 0. I recommend to use int
> type for
> irq line number, and recognize negative value (usually -1) as no irq needed /
> found.
Understand. But if you refer the original code, you can see:
irq = irq_of_parse_and_map(node, 0);
If (!irq) {
..
return;
}
From above code, if irq=0, it indicates irq is not supported for OF devices. If
we use '-1' to indicate irq is not supported. To make OF work, then our code
should be:
irq = irq_of_parse_and_map(node, 0);
If (!irq) {
pp->irq = -1;
return;
} else {
pp->irq = irq;
}
Then the code looks strange.
How do you think?
> > + bool is_of;
>
> is_pdata_alloc (it might be not only OF case in future).
>
OK
> > + if (!pdata) {
>
> (*)
> > + pdata = dwapb_gpio_get_pdata_of(dev);
> > + if (IS_ERR(pdata))
> > + return PTR_ERR(pdata);
>
> > + is_of = true;
> > + } else {
> > + is_of = false;
>
> Instead of above three lines, how about
> bool is_pdata_alloc = !pdata;
>
> And (*) if (is_pdata_alloc) ...
>
OK. I will improve this part.
> > + if (is_of) {
> > + dwapb_free_pdata_of(dev, pdata);
> > + pdata = NULL;
>
> Besides that pdata assignment is probably redundant, you may use plain
> kmalloc/kfree and avoid unnecessary devm_* calls.
>
OK.
RE: [PATCH 2/3 v2] GPIO: gpio-dwapb: Support Debounce
> Subject: Re: [PATCH 2/3 v2] GPIO: gpio-dwapb: Support Debounce > > On Fri, 2014-09-05 at 07:53 -0700, Weike Chen wrote: > > This patch enables 'debounce' for the designware GPIO, and it is based > > on Josef Ahmad's previous work. > > Can we split dwapb_read/write introducing and changing from this patch to a > separate one? > Since the 'debounce' feature also needs read/write, if we splite this patch, then for 'debounce', One patch use readl/writel, and another patch change to dwapb_read/write. It seems duplicate since the previous patch use readl/writel and the fllowing one change it immediately. Since this patch is small, could we just make them together? How do you think?
RE: [PATCH 3/3 v2] GPIO: gpio-dwapb: Suspend & Resume PM enabling
> > On Fri, 2014-09-05 at 07:53 -0700, Weike Chen wrote: > > This patch enables suspend and resume mode for the power management, > > and it is based on Josef Ahmad's previous work. > > > > Reviewed-by: Hock Leong Kweh > > Reviewed-by: Shevchenko, Andriy > > I have to recall my reviwed-by tag since patch is quite changed and as I > understood Linus is continuing to be changed. OK.
RE: [PATCH 3/3] GPIO: gpio-dwapb: Suspend & Resume PM enabling
> >>
> >> Insert this into the dynamically allocated per-port or chip struct instead.
> >>
> > How about the following?
> >
> > static struct dwapb_context {
> > u32 data[DWAPB_MAX_PORTS];
> > u32 dir[DWAPB_MAX_PORTS];
> > u32 ext[DWAPB_MAX_PORTS];
> > u32 int_en;
> > u32 int_mask;
> > u32 int_type;
> > u32 int_pol;
> > u32 int_deb;
> > } dwapb_context;
>
> NO because this is still a singleton variable. Put it into the dynamically
> allocated
> structs.
>
> > Comparing to allocate for each port
> > dynamically, it is more directly and easy to understand.
>
> No, I disagree. The overall design pattern in the kernel is to allocate all
> state
> containers dynamically.
>
OK. Please also help review the v2 I just sent out, and after getting more
feedbacks, I will improve this part in the next version together.
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH 3/3] GPIO: gpio-dwapb: Suspend Resume PM enabling
Insert this into the dynamically allocated per-port or chip struct instead.
How about the following?
static struct dwapb_context {
u32 data[DWAPB_MAX_PORTS];
u32 dir[DWAPB_MAX_PORTS];
u32 ext[DWAPB_MAX_PORTS];
u32 int_en;
u32 int_mask;
u32 int_type;
u32 int_pol;
u32 int_deb;
} dwapb_context;
NO because this is still a singleton variable. Put it into the dynamically
allocated
structs.
Comparing to allocate for each port
dynamically, it is more directly and easy to understand.
No, I disagree. The overall design pattern in the kernel is to allocate all
state
containers dynamically.
OK. Please also help review the v2 I just sent out, and after getting more
feedbacks, I will improve this part in the next version together.
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH 3/3 v2] GPIO: gpio-dwapb: Suspend Resume PM enabling
On Fri, 2014-09-05 at 07:53 -0700, Weike Chen wrote: This patch enables suspend and resume mode for the power management, and it is based on Josef Ahmad's previous work. Reviewed-by: Hock Leong Kweh hock.leong.k...@intel.com Reviewed-by: Shevchenko, Andriy andriy.shevche...@intel.com I have to recall my reviwed-by tag since patch is quite changed and as I understood Linus is continuing to be changed. OK.
RE: [PATCH 2/3 v2] GPIO: gpio-dwapb: Support Debounce
Subject: Re: [PATCH 2/3 v2] GPIO: gpio-dwapb: Support Debounce On Fri, 2014-09-05 at 07:53 -0700, Weike Chen wrote: This patch enables 'debounce' for the designware GPIO, and it is based on Josef Ahmad's previous work. Can we split dwapb_read/write introducing and changing from this patch to a separate one? Since the 'debounce' feature also needs read/write, if we splite this patch, then for 'debounce', One patch use readl/writel, and another patch change to dwapb_read/write. It seems duplicate since the previous patch use readl/writel and the fllowing one change it immediately. Since this patch is small, could we just make them together? How do you think?
RE: [PATCH 1/3 v2] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
-static void dwapb_irq_handler(u32 irq, struct irq_desc *desc)
+static u32 _dwapb_irq_handler(struct dwapb_gpio *gpio)
What about dwapb_do_irq() ?
OK, I will improve it.
+static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id) {
+ u32 worked;
+ struct dwapb_gpio *gpio = (struct dwapb_gpio *)dev_id;
No need to cast explicitly from void *.
OK.
- port-bgc.gc.ngpio = ngpio;
- port-bgc.gc.of_node = port_np;
+#ifdef CONFIG_OF_GPIO
Do we really need this #ifdef ?
of_node will be NULL anyway, or I missed something?
Yes, otherwise, can't compile it. Please refer 'struct gpio_chip', 'gc.of_node'
is in OF_GPIO micro also.
+ if (pp-irq)
irq == 0 is a valid hwirq (hardware irq) number. Yes, there is unlikely we
have it
somewhere, but still it's possible. And yes, IRQ framework doesn't work with
virq == 0 (*virtual* irq), but accepts hwirq == 0. I recommend to use int
type for
irq line number, and recognize negative value (usually -1) as no irq needed /
found.
Understand. But if you refer the original code, you can see:
irq = irq_of_parse_and_map(node, 0);
If (!irq) {
..
return;
}
From above code, if irq=0, it indicates irq is not supported for OF devices. If
we use '-1' to indicate irq is not supported. To make OF work, then our code
should be:
irq = irq_of_parse_and_map(node, 0);
If (!irq) {
pp-irq = -1;
return;
} else {
pp-irq = irq;
}
Then the code looks strange.
How do you think?
+ bool is_of;
is_pdata_alloc (it might be not only OF case in future).
OK
+ if (!pdata) {
(*)
+ pdata = dwapb_gpio_get_pdata_of(dev);
+ if (IS_ERR(pdata))
+ return PTR_ERR(pdata);
+ is_of = true;
+ } else {
+ is_of = false;
Instead of above three lines, how about
bool is_pdata_alloc = !pdata;
And (*) if (is_pdata_alloc) ...
OK. I will improve this part.
+ if (is_of) {
+ dwapb_free_pdata_of(dev, pdata);
+ pdata = NULL;
Besides that pdata assignment is probably redundant, you may use plain
kmalloc/kfree and avoid unnecessary devm_* calls.
OK.
RE: [PATCH 3/3] GPIO: gpio-dwapb: Suspend & Resume PM enabling
>
> > +#if defined CONFIG_PM_SLEEP
>
> I wonder whether it's worth #ifdef:in out such things, it clutters the place.
OK. I will use '#ifdef'.
>
> > +/* Store GPIO context across system-wide suspend/resume transitions
> > +*/ static struct gpio_saved_regs {
>
> Call the struct:
>
> struct dwapb_context
>
> because that is easier to understand.
>
OK.
> > + unsigned long data;
> > + unsigned long dir;
> > + unsigned long int_en;
> > + unsigned long int_mask;
> > + unsigned long int_type;
> > + unsigned long int_pol;
> > + unsigned long int_deb;
> > +} saved_regs;
>
> Singleton huh?
>
> Insert this into the dynamically allocated per-port or chip struct instead.
>
How about the following?
static struct dwapb_context {
u32 data[DWAPB_MAX_PORTS];
u32 dir[DWAPB_MAX_PORTS];
u32 ext[DWAPB_MAX_PORTS];
u32 int_en;
u32 int_mask;
u32 int_type;
u32 int_pol;
u32 int_deb;
} dwapb_context;
Since only portA can support irq, and the irq related registers are only for
portA. Comparing to allocate for each port
dynamically, it is more directly and easy to understand.
> + dwapb_write(gpio, GPIO_SWPORTA_DR, saved_regs.data);
> + dwapb_write(gpio, GPIO_SWPORTA_DDR, saved_regs.dir);
>
> And port B, C, D?
>
> This looks like a crude hack.
I will add port B, C, D.
>
> Yours,
> Linus Walleij
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH 1/3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
> > Since we enable this module not only support OF devices, but also support
> MFD devices, so we remove OF_GPIO dependenc
> > For 'PCI', the original code is also not depended on PCI, and this patch
> > also
> not, do you think it is necessary?
>
> if not PCI then you should add something likwe
> "depends on OF_GPIO || MFD"
>
> looking further, you need also HAS_IOMEM for things like
> devm_ioremap_resource(). Linus, wouldn't it make sense to group this driver
> and make the block depend on it?
>
I can add "depends on OF_GPIO || INTEL_QUARK_I2C_GPIO_MFD", since now the MFD
path only support Intel Quark.
Andriy, how do you think?
> > > > struct dwapb_gpio {
> > > > struct device *dev;
> > > > void __iomem*regs;
> > > > struct dwapb_gpio_port *ports;
> > > > - unsigned intnr_ports;
> > > you could keep this the way it is
> > It has been moved to 'pdata'.
>
> I saw that. But there is no need keep a pointer to pdata across the whole
> driver
> since only need nr_ports in the driver removal part of the whole driver.
Got your idea. So can I just use a global static pdata pointer instead of
adding it as a member of ' struct dwapb_gpio'?
Since pdata is used in all 'probe' functions, and make pdata as global static
make programming more easy.
>
> > > > struct irq_domain *domain;
> > > > + const struct dwapb_gpio_platform_data *pdata;
> > >
> > > and not making this a member of this struct. I've been going up and
> > > down the source and I don't see the need to marry dwapb_gpio to
> > > dwapb_gpio_platform_data.
> > > That dwapb_gpio_port_property thing has a long name. Could you just
> > > set it up, pass it for registration and the free it? You can't free
> > > the pdata for the non-OF tree but for the OF case you keep that struct
> around for no reason.
> > > You could safe some memory and use pdata directly for setup.
> > Here, 'pdata' is used for both OF and MFD. For MFD, 'pdata' is passed from
> MFD.
> > For OF, 'pdata' is getting from device nodes properties. Why do we
> > have such design? Because it can make the handling more easy for
> > flowing routine. All necessary properties get from 'pdata', never care it is
> from OF or MFD. And someone are working on replacing OF interface with a
> firmware agnostic device_property* interface which will work with both OF and
> ACPI.
> > More information for this design, please contact Darren Hart
> . Darren Hart wrote to me:
> > "Generally speaking, rather than if/else blocks throughout the code
> > checking if it is enumerated via open firmware or as a platform device, a
> cleaner approach is to check if the pdata is null, and if so, populate the
> pdata
> from the open firmware description if present.
> > Then use the pdata throughout the driver.
>
> But isn't it enough to hold on to this pdata thing through the probe function
> only? After probe is done you could drop that memory in the OF-case, right?
OK. If it is OF-case, pdata can be freed in the end of probe.
> > > > + irq_set_handler_data(port->pp->irq, gpio);
> > >
> > > This does not look like it belongs here. It should only be used
> > > together with
> > > irq_set_chained_handler() or am I missing here something?
> > This patch reused ' dwapb_irq_handler', it needs the irq handler data. For '
> irq_set_handler_data', it just sets the irq data.
> > Why do you think it must be used together with ' irq_set_chained_handler'?
>
> because you do request_irq(…, driver_data). If you you look close to
> irq_set_chained_handler() it does not have such a member. Thus it uses
> irq_set_handler_data() for that same purpose.
>
OK. I can improve it.
RE: [PATCH 3/3] GPIO: gpio-dwapb: Suspend Resume PM enabling
+#if defined CONFIG_PM_SLEEP
I wonder whether it's worth #ifdef:in out such things, it clutters the place.
OK. I will use '#ifdef'.
+/* Store GPIO context across system-wide suspend/resume transitions
+*/ static struct gpio_saved_regs {
Call the struct:
struct dwapb_context
because that is easier to understand.
OK.
+ unsigned long data;
+ unsigned long dir;
+ unsigned long int_en;
+ unsigned long int_mask;
+ unsigned long int_type;
+ unsigned long int_pol;
+ unsigned long int_deb;
+} saved_regs;
Singleton huh?
Insert this into the dynamically allocated per-port or chip struct instead.
How about the following?
static struct dwapb_context {
u32 data[DWAPB_MAX_PORTS];
u32 dir[DWAPB_MAX_PORTS];
u32 ext[DWAPB_MAX_PORTS];
u32 int_en;
u32 int_mask;
u32 int_type;
u32 int_pol;
u32 int_deb;
} dwapb_context;
Since only portA can support irq, and the irq related registers are only for
portA. Comparing to allocate for each port
dynamically, it is more directly and easy to understand.
+ dwapb_write(gpio, GPIO_SWPORTA_DR, saved_regs.data);
+ dwapb_write(gpio, GPIO_SWPORTA_DDR, saved_regs.dir);
And port B, C, D?
This looks like a crude hack.
I will add port B, C, D.
Yours,
Linus Walleij
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH 1/3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
Since we enable this module not only support OF devices, but also support
MFD devices, so we remove OF_GPIO dependenc
For 'PCI', the original code is also not depended on PCI, and this patch
also
not, do you think it is necessary?
if not PCI then you should add something likwe
depends on OF_GPIO || MFD
looking further, you need also HAS_IOMEM for things like
devm_ioremap_resource(). Linus, wouldn't it make sense to group this driver
and make the block depend on it?
I can add depends on OF_GPIO || INTEL_QUARK_I2C_GPIO_MFD, since now the MFD
path only support Intel Quark.
Andriy, how do you think?
struct dwapb_gpio {
struct device *dev;
void __iomem*regs;
struct dwapb_gpio_port *ports;
- unsigned intnr_ports;
you could keep this the way it is
It has been moved to 'pdata'.
I saw that. But there is no need keep a pointer to pdata across the whole
driver
since only need nr_ports in the driver removal part of the whole driver.
Got your idea. So can I just use a global static pdata pointer instead of
adding it as a member of ' struct dwapb_gpio'?
Since pdata is used in all 'probe' functions, and make pdata as global static
make programming more easy.
struct irq_domain *domain;
+ const struct dwapb_gpio_platform_data *pdata;
and not making this a member of this struct. I've been going up and
down the source and I don't see the need to marry dwapb_gpio to
dwapb_gpio_platform_data.
That dwapb_gpio_port_property thing has a long name. Could you just
set it up, pass it for registration and the free it? You can't free
the pdata for the non-OF tree but for the OF case you keep that struct
around for no reason.
You could safe some memory and use pdata directly for setup.
Here, 'pdata' is used for both OF and MFD. For MFD, 'pdata' is passed from
MFD.
For OF, 'pdata' is getting from device nodes properties. Why do we
have such design? Because it can make the handling more easy for
flowing routine. All necessary properties get from 'pdata', never care it is
from OF or MFD. And someone are working on replacing OF interface with a
firmware agnostic device_property* interface which will work with both OF and
ACPI.
More information for this design, please contact Darren Hart
dvh...@linux.intel.com. Darren Hart wrote to me:
Generally speaking, rather than if/else blocks throughout the code
checking if it is enumerated via open firmware or as a platform device, a
cleaner approach is to check if the pdata is null, and if so, populate the
pdata
from the open firmware description if present.
Then use the pdata throughout the driver.
But isn't it enough to hold on to this pdata thing through the probe function
only? After probe is done you could drop that memory in the OF-case, right?
OK. If it is OF-case, pdata can be freed in the end of probe.
+ irq_set_handler_data(port-pp-irq, gpio);
This does not look like it belongs here. It should only be used
together with
irq_set_chained_handler() or am I missing here something?
This patch reused ' dwapb_irq_handler', it needs the irq handler data. For '
irq_set_handler_data', it just sets the irq data.
Why do you think it must be used together with ' irq_set_chained_handler'?
because you do request_irq(…, driver_data). If you you look close to
irq_set_chained_handler() it does not have such a member. Thus it uses
irq_set_handler_data() for that same purpose.
OK. I can improve it.
RE: [PATCH 1/3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
>
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -136,7 +136,6 @@ config GPIO_DWAPB
> > tristate "Synopsys DesignWare APB GPIO driver"
> > select GPIO_GENERIC
> > select GENERIC_IRQ_CHIP
> > - depends on OF_GPIO
> you need either OF_GPIO or PCI
Since we enable this module not only support OF devices, but also support MFD
devices, so we remove OF_GPIO dependency.
For 'PCI', the original code is also not depended on PCI, and this patch also
not, do you think it is necessary?
> >
> > struct dwapb_gpio {
> > struct device *dev;
> > void __iomem*regs;
> > struct dwapb_gpio_port *ports;
> > - unsigned intnr_ports;
> you could keep this the way it is
It has been moved to 'pdata'.
> > struct irq_domain *domain;
> > + const struct dwapb_gpio_platform_data *pdata;
>
> and not making this a member of this struct. I've been going up and down the
> source and I don't see the need to marry dwapb_gpio to
> dwapb_gpio_platform_data.
> That dwapb_gpio_port_property thing has a long name. Could you just set it up,
> pass it for registration and the free it? You can't free the pdata for the
> non-OF
> tree but for the OF case you keep that struct around for no reason.
> You could safe some memory and use pdata directly for setup.
Here, 'pdata' is used for both OF and MFD. For MFD, 'pdata' is passed from MFD.
For OF, 'pdata' is getting from device nodes properties. Why do we have such
design? Because it can make the handling
more easy for flowing routine. All necessary properties get from 'pdata', never
care it is from OF or MFD. And someone
are working on replacing OF interface with a firmware agnostic device_property*
interface which will work with both OF and ACPI.
More information for this design, please contact Darren Hart
. Darren Hart wrote to me:
"Generally speaking, rather than if/else blocks throughout the code checking if
it is enumerated via open firmware or as a platform device,
a cleaner approach is to check if the pdata is null, and if so, populate the
pdata from the open firmware description if present.
Then use the pdata throughout the driver.
Something to keep in mind is that we are working to replace the of_* interface
with a firmware agnostic device_property* interface
which will work with both OF and ACPI. Patches to hit LKML this week for the
acpi and driver core.
Organizing the code as described above will better encapsulate the
firmware-config logic and make it easier to update."
And you can also refer the code 'driver/input/keyboard/gpio_keys.c', and this
patch takes it as an example.
> > };
> >
> > static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
> > @@ -207,22 +209,24 @@ static int dwapb_irq_set_type(struct irq_data *d,
> u32 type)
> > return 0;
> > }
> >
> > +static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id) {
> > + struct irq_desc *desc = irq_to_desc(irq);
> > +
> > + dwapb_irq_handler(irq, desc);
> > +
> > + return IRQ_HANDLED;
>
> I suggest to teach dwapb_irq_handler() to return something that makes you
> decide whether or not IRQ_HANDLED is the thing to do here. If something goes
> crazy the core has no way of knowing and shutting you down.
> Also invoking ->irq_eoi() _before_ your shared was invoked might not smart.
> What you want is something like
>
> static u32 _dwapb_irq_handler(struct dwapb_gpio *gpio, struct irq_chip
> *chip) {
> u32 irq_status = readl_relaxed(gpio->regs + GPIO_INTSTATUS);
>u32 ret = irq_status;
>
> while (irq_status) {
> int hwirq = fls(irq_status) - 1;
> int gpio_irq = irq_find_mapping(gpio->domain, hwirq);
>
> generic_handle_irq(gpio_irq);
> irq_status &= ~BIT(hwirq);
>
> if ((irq_get_trigger_type(gpio_irq) &
> IRQ_TYPE_SENSE_MASK)
> == IRQ_TYPE_EDGE_BOTH)
> dwapb_toggle_trigger(gpio, hwirq);
> }
>return ret;
> }
>
> static void dwapb_irq_handler(u32 irq, struct irq_desc *desc) {
> struct dwapb_gpio *gpio = irq_get_handler_data(irq);
> struct irq_chip *chip = irq_desc_get_chip(desc);
>
> _dwapb_irq_handler(gpio, chip);
>
> if (chip->irq_eoi)
> chip->irq_eoi(irq_desc_get_irq_data(desc));
> }
>
> static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id) {
> struct irq_desc *desc = irq_to_desc(irq);
> struct irq_chip *chip = irq_desc_get_chip(desc);
> int worked;
>
> worked = dwapb_irq_handler(dev_id, chip);
> if (worked)
> return IRQ_HANDLED;
> else
> return IRQ_NONE;
> }
>
> How about it?
OK, I will improve it as your suggestion.
> > + irq_set_handler_data(port->pp->irq, gpio);
>
> This does not look like it belongs here. It should only be used together with
> irq_set_chained_handler()
RE: [PATCH 1/3] GPIO: gpio-dwapb: Enable platform driver binding to MFD driver
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -136,7 +136,6 @@ config GPIO_DWAPB
tristate Synopsys DesignWare APB GPIO driver
select GPIO_GENERIC
select GENERIC_IRQ_CHIP
- depends on OF_GPIO
you need either OF_GPIO or PCI
Since we enable this module not only support OF devices, but also support MFD
devices, so we remove OF_GPIO dependency.
For 'PCI', the original code is also not depended on PCI, and this patch also
not, do you think it is necessary?
struct dwapb_gpio {
struct device *dev;
void __iomem*regs;
struct dwapb_gpio_port *ports;
- unsigned intnr_ports;
you could keep this the way it is
It has been moved to 'pdata'.
struct irq_domain *domain;
+ const struct dwapb_gpio_platform_data *pdata;
and not making this a member of this struct. I've been going up and down the
source and I don't see the need to marry dwapb_gpio to
dwapb_gpio_platform_data.
That dwapb_gpio_port_property thing has a long name. Could you just set it up,
pass it for registration and the free it? You can't free the pdata for the
non-OF
tree but for the OF case you keep that struct around for no reason.
You could safe some memory and use pdata directly for setup.
Here, 'pdata' is used for both OF and MFD. For MFD, 'pdata' is passed from MFD.
For OF, 'pdata' is getting from device nodes properties. Why do we have such
design? Because it can make the handling
more easy for flowing routine. All necessary properties get from 'pdata', never
care it is from OF or MFD. And someone
are working on replacing OF interface with a firmware agnostic device_property*
interface which will work with both OF and ACPI.
More information for this design, please contact Darren Hart
dvh...@linux.intel.com. Darren Hart wrote to me:
Generally speaking, rather than if/else blocks throughout the code checking if
it is enumerated via open firmware or as a platform device,
a cleaner approach is to check if the pdata is null, and if so, populate the
pdata from the open firmware description if present.
Then use the pdata throughout the driver.
Something to keep in mind is that we are working to replace the of_* interface
with a firmware agnostic device_property* interface
which will work with both OF and ACPI. Patches to hit LKML this week for the
acpi and driver core.
Organizing the code as described above will better encapsulate the
firmware-config logic and make it easier to update.
And you can also refer the code 'driver/input/keyboard/gpio_keys.c', and this
patch takes it as an example.
};
static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
@@ -207,22 +209,24 @@ static int dwapb_irq_set_type(struct irq_data *d,
u32 type)
return 0;
}
+static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id) {
+ struct irq_desc *desc = irq_to_desc(irq);
+
+ dwapb_irq_handler(irq, desc);
+
+ return IRQ_HANDLED;
I suggest to teach dwapb_irq_handler() to return something that makes you
decide whether or not IRQ_HANDLED is the thing to do here. If something goes
crazy the core has no way of knowing and shutting you down.
Also invoking -irq_eoi() _before_ your shared was invoked might not smart.
What you want is something like
static u32 _dwapb_irq_handler(struct dwapb_gpio *gpio, struct irq_chip
*chip) {
u32 irq_status = readl_relaxed(gpio-regs + GPIO_INTSTATUS);
u32 ret = irq_status;
while (irq_status) {
int hwirq = fls(irq_status) - 1;
int gpio_irq = irq_find_mapping(gpio-domain, hwirq);
generic_handle_irq(gpio_irq);
irq_status = ~BIT(hwirq);
if ((irq_get_trigger_type(gpio_irq)
IRQ_TYPE_SENSE_MASK)
== IRQ_TYPE_EDGE_BOTH)
dwapb_toggle_trigger(gpio, hwirq);
}
return ret;
}
static void dwapb_irq_handler(u32 irq, struct irq_desc *desc) {
struct dwapb_gpio *gpio = irq_get_handler_data(irq);
struct irq_chip *chip = irq_desc_get_chip(desc);
_dwapb_irq_handler(gpio, chip);
if (chip-irq_eoi)
chip-irq_eoi(irq_desc_get_irq_data(desc));
}
static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id) {
struct irq_desc *desc = irq_to_desc(irq);
struct irq_chip *chip = irq_desc_get_chip(desc);
int worked;
worked = dwapb_irq_handler(dev_id, chip);
if (worked)
return IRQ_HANDLED;
else
return IRQ_NONE;
}
How about it?
OK, I will improve it as your suggestion.
+ irq_set_handler_data(port-pp-irq, gpio);
This does not look like it belongs here. It should only be used together with
irq_set_chained_handler() or am I missing here something?
This patch reused ' dwapb_irq_handler', it needs the irq handler data. For '
RE: [PATCH 3/3] GPIO: gpio-dwapb: Suspend & Resume PM enabling
> > +/* Store GPIO context across system-wide suspend/resume transitions
> > +*/ static struct gpio_saved_regs {
> > + unsigned long data;
> > + unsigned long dir;
> > + unsigned long int_en;
> > + unsigned long int_mask;
> > + unsigned long int_type;
> > + unsigned long int_pol;
> > + unsigned long int_deb;
> > +} saved_regs;
> > +
> > +static int dwapb_gpio_suspend(struct device *dev) {
> > + struct platform_device *pdev = to_platform_device(dev);
> > + struct dwapb_gpio *gpio = platform_get_drvdata(pdev);
> > + struct bgpio_chip *bgc = >ports[0].bgc;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(>lock, flags);
> > +
> > + saved_regs.int_mask = dwapb_read(gpio, GPIO_INTMASK);
> > + saved_regs.int_en = dwapb_read(gpio, GPIO_INTEN);
> > + saved_regs.int_deb = dwapb_read(gpio, GPIO_PORTA_DEBOUNCE);
> > + saved_regs.int_pol = dwapb_read(gpio, GPIO_INT_POLARITY);
> > + saved_regs.int_type = dwapb_read(gpio, GPIO_INTTYPE_LEVEL);
> > + saved_regs.dir = dwapb_read(gpio, GPIO_SWPORTA_DDR);
> > + saved_regs.data = dwapb_read(gpio, GPIO_SWPORTA_DR);
>
> Hello,
>
> The DesignWare GPIO IP can be configured to have ports a, b, c, and d. So you
> will need to save and restore any ports that are present. I think that *some*
> configurations of the IP include a register that can tell us how it was
> configured,
> but that register is also optional :) I don't have confidence that we can
> read/write the registers blindly whether they are known to be there or not.
> So you may be stuck with looking at the device tree or platform data to know
> whether banks b, c, or d exist.
>
>From the code, the device node has one property for port index. If the index
>is '0', then it is port A.
So I think we can store the status according to the port index.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/3] GPIO: gpio-dwapb: Support Debounce
> > > > I don't understand the reason for adding dwapb_read and dwapb_write here. > > The rest of the driver is using readl and writel. I'd rather not see > > two different methods being used in the same driver for register access. > > Maybe I'm missing something, but if we need to add dwapb_read/write, > > then it should be used for all register access. > > Alan, it was my proposal. I rather agree that is better to convert all > accesses to > that. OK, I will convert all readl to dwapb_read_write.
RE: [PATCH 2/3] GPIO: gpio-dwapb: Support Debounce
I don't understand the reason for adding dwapb_read and dwapb_write here. The rest of the driver is using readl and writel. I'd rather not see two different methods being used in the same driver for register access. Maybe I'm missing something, but if we need to add dwapb_read/write, then it should be used for all register access. Alan, it was my proposal. I rather agree that is better to convert all accesses to that. OK, I will convert all readlwritel to dwapb_readdwapb_write.
RE: [PATCH 3/3] GPIO: gpio-dwapb: Suspend Resume PM enabling
+/* Store GPIO context across system-wide suspend/resume transitions
+*/ static struct gpio_saved_regs {
+ unsigned long data;
+ unsigned long dir;
+ unsigned long int_en;
+ unsigned long int_mask;
+ unsigned long int_type;
+ unsigned long int_pol;
+ unsigned long int_deb;
+} saved_regs;
+
+static int dwapb_gpio_suspend(struct device *dev) {
+ struct platform_device *pdev = to_platform_device(dev);
+ struct dwapb_gpio *gpio = platform_get_drvdata(pdev);
+ struct bgpio_chip *bgc = gpio-ports[0].bgc;
+ unsigned long flags;
+
+ spin_lock_irqsave(bgc-lock, flags);
+
+ saved_regs.int_mask = dwapb_read(gpio, GPIO_INTMASK);
+ saved_regs.int_en = dwapb_read(gpio, GPIO_INTEN);
+ saved_regs.int_deb = dwapb_read(gpio, GPIO_PORTA_DEBOUNCE);
+ saved_regs.int_pol = dwapb_read(gpio, GPIO_INT_POLARITY);
+ saved_regs.int_type = dwapb_read(gpio, GPIO_INTTYPE_LEVEL);
+ saved_regs.dir = dwapb_read(gpio, GPIO_SWPORTA_DDR);
+ saved_regs.data = dwapb_read(gpio, GPIO_SWPORTA_DR);
Hello,
The DesignWare GPIO IP can be configured to have ports a, b, c, and d. So you
will need to save and restore any ports that are present. I think that *some*
configurations of the IP include a register that can tell us how it was
configured,
but that register is also optional :) I don't have confidence that we can
read/write the registers blindly whether they are known to be there or not.
So you may be stuck with looking at the device tree or platform data to know
whether banks b, c, or d exist.
From the code, the device node has one property for port index. If the index
is '0', then it is port A.
So I think we can store the status according to the port index.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 3/3] GPIO: gpio-dwapb: Suspend & Resume PM enabling
> > Hi Weike, > > I tried out these patches on the current master branch (v3.17-rc2-9-g68e3702) > with a socfpga cyclone5 board. > > If I apply all 3 patches, the kernel doesn't boot. > > If I rebuild with only the first patch, I get only one gpio block showing up > (should > have 3 for this board) and these messages: > > > How are you testing this? The patches try to not change the current OF flow, and from the log message, the other two GPIO registered failed due to duplicate name. Let me check the code to see what happen. > > Alan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 3/3] GPIO: gpio-dwapb: Suspend Resume PM enabling
Hi Weike, I tried out these patches on the current master branch (v3.17-rc2-9-g68e3702) with a socfpga cyclone5 board. If I apply all 3 patches, the kernel doesn't boot. If I rebuild with only the first patch, I get only one gpio block showing up (should have 3 for this board) and these messages: How are you testing this? The patches try to not change the current OF flow, and from the log message, the other two GPIO registered failed due to duplicate name. Let me check the code to see what happen. Alan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] USB: pch_udc: USB gadget device support for Intel Quark X1000
> Hi, > > On Mon, Aug 04, 2014 at 10:22:54AM -0700, Chen, Alvin wrote: > > From: Bryan O'Donoghue > > > > This patch is to enable the USB gadget device for Intel Quark X1000 > > > > Signed-off-by: Bryan O'Donoghue > > Signed-off-by: Bing Niu > > Signed-off-by: Alvin (Weike) Chen > > Can someone confirm to me this is not another incarnation of chipidea ? No, this is not another incarnation of chipidea. And its cover letter is as following: On Mon, Aug 04, 2014 at 11:00:07AM -0700, Chen, Alvin wrote: > From: "Alvin (Weike) Chen" > > Hi, > Intel Quark X1000 consists of one USB gadget device which can be PCI > enumerated. > pch_udc layer doesn't support it. Thus, we add support for Intel Quark > X1000 USB gadget device as well. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] USB: pch_udc: USB gadget device support for Intel Quark X1000
Hi, On Mon, Aug 04, 2014 at 10:22:54AM -0700, Chen, Alvin wrote: From: Bryan O'Donoghue bryan.odonog...@intel.com This patch is to enable the USB gadget device for Intel Quark X1000 Signed-off-by: Bryan O'Donoghue bryan.odonog...@intel.com Signed-off-by: Bing Niu bing@intel.com Signed-off-by: Alvin (Weike) Chen alvin.c...@intel.com Can someone confirm to me this is not another incarnation of chipidea ? No, this is not another incarnation of chipidea. And its cover letter is as following: On Mon, Aug 04, 2014 at 11:00:07AM -0700, Chen, Alvin wrote: From: Alvin (Weike) Chen alvin.c...@intel.com Hi, Intel Quark X1000 consists of one USB gadget device which can be PCI enumerated. pch_udc layer doesn't support it. Thus, we add support for Intel Quark X1000 USB gadget device as well. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] USB: pch_udc: USB gadget device support for Intel Quark X1000
Hi Felipe,
Any update for this patch? Just want to follow-up.
> -Original Message-
> From: Chen, Alvin
> Sent: Tuesday, August 05, 2014 1:23 AM
> To: Felipe Balbi
> Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Ong, Boon
> Leong
> Subject: [PATCH] USB: pch_udc: USB gadget device support for Intel
> Quark
> X1000
>
> From: Bryan O'Donoghue
>
> This patch is to enable the USB gadget device for Intel Quark X1000
>
> Signed-off-by: Bryan O'Donoghue
> Signed-off-by: Bing Niu
> Signed-off-by: Alvin (Weike) Chen
> ---
> drivers/usb/gadget/udc/Kconfig |3 ++-
> drivers/usb/gadget/udc/pch_udc.c | 22 +++---
> 2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/Kconfig
> b/drivers/usb/gadget/udc/Kconfig index 5151f94..34ebaa6 100644
> --- a/drivers/usb/gadget/udc/Kconfig
> +++ b/drivers/usb/gadget/udc/Kconfig
> @@ -332,7 +332,7 @@ config USB_GOKU
> gadget drivers to also be dynamically linked.
>
> config USB_EG20T
> - tristate "Intel EG20T PCH/LAPIS Semiconductor IOH(ML7213/ML7831)
> UDC"
> + tristate "Intel QUARK X1000/EG20T PCH/LAPIS Semiconductor
> IOH(ML7213/ML7831) UDC"
> depends on PCI
> help
> This is a USB device driver for EG20T PCH.
> @@ -353,6 +353,7 @@ config USB_EG20T
> ML7213/ML7831 is companion chip for Intel Atom E6xx series.
> ML7213/ML7831 is completely compatible for Intel EG20T PCH.
>
> + This driver can be used with Intel's Quark X1000 SOC platform
> #
> # LAST -- dummy/emulated controller
> #
> diff --git a/drivers/usb/gadget/udc/pch_udc.c
> b/drivers/usb/gadget/udc/pch_udc.c
> index eb8c3be..460d953 100644
> --- a/drivers/usb/gadget/udc/pch_udc.c
> +++ b/drivers/usb/gadget/udc/pch_udc.c
> @@ -343,6 +343,7 @@ struct pch_vbus_gpio_data {
> * @setup_data: Received setup data
> * @phys_addr: of device memory
> * @base_addr: for mapped device memory
> + * @bar: Indicates which PCI BAR for USB regs
> * @irq: IRQ line for the device
> * @cfg_data:current cfg, intf, and alt in use
> * @vbus_gpio: GPIO informaton for detecting VBUS
> @@ -370,14 +371,17 @@ struct pch_udc_dev {
> struct usb_ctrlrequest setup_data;
> unsigned long phys_addr;
> void __iomem*base_addr;
> + unsignedbar;
> unsignedirq;
> struct pch_udc_cfg_data cfg_data;
> struct pch_vbus_gpio_data vbus_gpio;
> };
> #define to_pch_udc(g)(container_of((g), struct pch_udc_dev,
> gadget))
>
> +#define PCH_UDC_PCI_BAR_QUARK_X1000 0
> #define PCH_UDC_PCI_BAR 1
> #define PCI_DEVICE_ID_INTEL_EG20T_UDC0x8808
> +#define PCI_DEVICE_ID_INTEL_QUARK_X1000_UDC 0x0939
> #define PCI_VENDOR_ID_ROHM 0x10DB
> #define PCI_DEVICE_ID_ML7213_IOH_UDC 0x801D
> #define PCI_DEVICE_ID_ML7831_IOH_UDC 0x8808
> @@ -3076,7 +3080,7 @@ static void pch_udc_remove(struct pci_dev *pdev)
> iounmap(dev->base_addr);
> if (dev->mem_region)
> release_mem_region(dev->phys_addr,
> -pci_resource_len(pdev,
> PCH_UDC_PCI_BAR));
> +pci_resource_len(pdev, dev->bar));
> if (dev->active)
> pci_disable_device(pdev);
> kfree(dev);
> @@ -3144,9 +3148,15 @@ static int pch_udc_probe(struct pci_dev *pdev,
> dev->active = 1;
> pci_set_drvdata(pdev, dev);
>
> + /* Determine BAR based on PCI ID */
> + if (id->device == PCI_DEVICE_ID_INTEL_QUARK_X1000_UDC)
> + dev->bar = PCH_UDC_PCI_BAR_QUARK_X1000;
> + else
> + dev->bar = PCH_UDC_PCI_BAR;
> +
> /* PCI resource allocation */
> - resource = pci_resource_start(pdev, 1);
> - len = pci_resource_len(pdev, 1);
> + resource = pci_resource_start(pdev, dev->bar);
> + len = pci_resource_len(pdev, dev->bar);
>
> if (!request_mem_region(resource, len, KBUILD_MODNAME)) {
> dev_err(>dev, "%s: pci device used already\n", __func__);
> @@
> -3212,6 +3222,12 @@ finished:
>
> static const struct pci_device_id pch_udc_pcidev_id[] = {
> {
> + PCI_DEVICE(PCI_VENDOR_ID_INTEL,
> +PCI_DEVICE_ID_INTEL_QUARK_X1000_UDC),
> + .class = (PCI_CLASS_SERIAL_USB << 8) | 0xfe,
> + .class_mask = 0x,
> + },
> +
RE: [PATCH v2] mmc: sdhci-pci: SDIO host controller support for Intel Quark X1000
Hi Ulf,
Any update for this patch? Just want to follow-up.
Best regards，Alvin Chen
ICFS Platform Engineering Solution
Flex Services (CMMI Level 3, IQA2005, IQA2008), Greater Asia Region Intel
Information Technology Tel. 010-82171960
inet.8-7581960
Email. alvin.c...@intel.com
> -Original Message-
> From: Ong, Boon Leong
> Sent: Wednesday, August 6, 2014 3:32 PM
> To: Ulf Hansson
> Cc: Chris Ball; linux-mmc; linux-kernel@vger.kernel.org; Chen, Alvin
> Subject: RE: [PATCH v2] mmc: sdhci-pci: SDIO host controller support for Intel
> Quark X1000
>
> > -Original Message-
> > From: Ulf Hansson [mailto:ulf.hans...@linaro.org]
> > Sent: Wednesday, July 02, 2014 5:01 PM
> > To: Chen, Alvin
> > Cc: Chris Ball; linux-mmc; linux-kernel@vger.kernel.org; Ong, Boon
> > Leong
> > Subject: Re: [PATCH v2] mmc: sdhci-pci: SDIO host controller support
> > for Intel Quark X1000
> >
> > On 24 June 2014 15:56, Chen, Alvin wrote:
> > > From: Derek Browne
> > >
> > > This patch is to enable SDIO host controller for Intel Quark X1000.
> > >
> > > Signed-off-by: Derek Browne
> > > Signed-off-by: Alvin (Weike) Chen
> >
> > Thanks! Applied for next.
> >
>
> Hi Ulf,
> For this patch, are you in progress of getting it into v3.17 merge
> window?
> Just want to follow-up.
>
> Many thanks
> Boon Leong
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v2] mmc: sdhci-pci: SDIO host controller support for Intel Quark X1000
Hi Ulf,
Any update for this patch? Just want to follow-up.
Best regards，Alvin Chen
ICFS Platform Engineering Solution
Flex Services (CMMI Level 3, IQA2005, IQA2008), Greater Asia Region Intel
Information Technology Tel. 010-82171960
inet.8-7581960
Email. alvin.c...@intel.com
-Original Message-
From: Ong, Boon Leong
Sent: Wednesday, August 6, 2014 3:32 PM
To: Ulf Hansson
Cc: Chris Ball; linux-mmc; linux-kernel@vger.kernel.org; Chen, Alvin
Subject: RE: [PATCH v2] mmc: sdhci-pci: SDIO host controller support for Intel
Quark X1000
-Original Message-
From: Ulf Hansson [mailto:ulf.hans...@linaro.org]
Sent: Wednesday, July 02, 2014 5:01 PM
To: Chen, Alvin
Cc: Chris Ball; linux-mmc; linux-kernel@vger.kernel.org; Ong, Boon
Leong
Subject: Re: [PATCH v2] mmc: sdhci-pci: SDIO host controller support
for Intel Quark X1000
On 24 June 2014 15:56, Chen, Alvin alvin.c...@intel.com wrote:
From: Derek Browne derek.bro...@intel.com
This patch is to enable SDIO host controller for Intel Quark X1000.
Signed-off-by: Derek Browne derek.bro...@intel.com
Signed-off-by: Alvin (Weike) Chen alvin.c...@intel.com
Thanks! Applied for next.
Hi Ulf,
For this patch, are you in progress of getting it into v3.17 merge
window?
Just want to follow-up.
Many thanks
Boon Leong
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
Re: [PATCH] USB: pch_udc: USB gadget device support for Intel Quark X1000
Hi Felipe,
Any update for this patch? Just want to follow-up.
-Original Message-
From: Chen, Alvin
Sent: Tuesday, August 05, 2014 1:23 AM
To: Felipe Balbi
Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Ong, Boon
Leong
Subject: [PATCH] USB: pch_udc: USB gadget device support for Intel
Quark
X1000
From: Bryan O'Donoghue bryan.odonog...@intel.com
This patch is to enable the USB gadget device for Intel Quark X1000
Signed-off-by: Bryan O'Donoghue bryan.odonog...@intel.com
Signed-off-by: Bing Niu bing@intel.com
Signed-off-by: Alvin (Weike) Chen alvin.c...@intel.com
---
drivers/usb/gadget/udc/Kconfig |3 ++-
drivers/usb/gadget/udc/pch_udc.c | 22 +++---
2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/udc/Kconfig
b/drivers/usb/gadget/udc/Kconfig index 5151f94..34ebaa6 100644
--- a/drivers/usb/gadget/udc/Kconfig
+++ b/drivers/usb/gadget/udc/Kconfig
@@ -332,7 +332,7 @@ config USB_GOKU
gadget drivers to also be dynamically linked.
config USB_EG20T
- tristate Intel EG20T PCH/LAPIS Semiconductor IOH(ML7213/ML7831)
UDC
+ tristate Intel QUARK X1000/EG20T PCH/LAPIS Semiconductor
IOH(ML7213/ML7831) UDC
depends on PCI
help
This is a USB device driver for EG20T PCH.
@@ -353,6 +353,7 @@ config USB_EG20T
ML7213/ML7831 is companion chip for Intel Atom E6xx series.
ML7213/ML7831 is completely compatible for Intel EG20T PCH.
+ This driver can be used with Intel's Quark X1000 SOC platform
#
# LAST -- dummy/emulated controller
#
diff --git a/drivers/usb/gadget/udc/pch_udc.c
b/drivers/usb/gadget/udc/pch_udc.c
index eb8c3be..460d953 100644
--- a/drivers/usb/gadget/udc/pch_udc.c
+++ b/drivers/usb/gadget/udc/pch_udc.c
@@ -343,6 +343,7 @@ struct pch_vbus_gpio_data {
* @setup_data: Received setup data
* @phys_addr: of device memory
* @base_addr: for mapped device memory
+ * @bar: Indicates which PCI BAR for USB regs
* @irq: IRQ line for the device
* @cfg_data:current cfg, intf, and alt in use
* @vbus_gpio: GPIO informaton for detecting VBUS
@@ -370,14 +371,17 @@ struct pch_udc_dev {
struct usb_ctrlrequest setup_data;
unsigned long phys_addr;
void __iomem*base_addr;
+ unsignedbar;
unsignedirq;
struct pch_udc_cfg_data cfg_data;
struct pch_vbus_gpio_data vbus_gpio;
};
#define to_pch_udc(g)(container_of((g), struct pch_udc_dev,
gadget))
+#define PCH_UDC_PCI_BAR_QUARK_X1000 0
#define PCH_UDC_PCI_BAR 1
#define PCI_DEVICE_ID_INTEL_EG20T_UDC0x8808
+#define PCI_DEVICE_ID_INTEL_QUARK_X1000_UDC 0x0939
#define PCI_VENDOR_ID_ROHM 0x10DB
#define PCI_DEVICE_ID_ML7213_IOH_UDC 0x801D
#define PCI_DEVICE_ID_ML7831_IOH_UDC 0x8808
@@ -3076,7 +3080,7 @@ static void pch_udc_remove(struct pci_dev *pdev)
iounmap(dev-base_addr);
if (dev-mem_region)
release_mem_region(dev-phys_addr,
-pci_resource_len(pdev,
PCH_UDC_PCI_BAR));
+pci_resource_len(pdev, dev-bar));
if (dev-active)
pci_disable_device(pdev);
kfree(dev);
@@ -3144,9 +3148,15 @@ static int pch_udc_probe(struct pci_dev *pdev,
dev-active = 1;
pci_set_drvdata(pdev, dev);
+ /* Determine BAR based on PCI ID */
+ if (id-device == PCI_DEVICE_ID_INTEL_QUARK_X1000_UDC)
+ dev-bar = PCH_UDC_PCI_BAR_QUARK_X1000;
+ else
+ dev-bar = PCH_UDC_PCI_BAR;
+
/* PCI resource allocation */
- resource = pci_resource_start(pdev, 1);
- len = pci_resource_len(pdev, 1);
+ resource = pci_resource_start(pdev, dev-bar);
+ len = pci_resource_len(pdev, dev-bar);
if (!request_mem_region(resource, len, KBUILD_MODNAME)) {
dev_err(pdev-dev, %s: pci device used already\n, __func__);
@@
-3212,6 +3222,12 @@ finished:
static const struct pci_device_id pch_udc_pcidev_id[] = {
{
+ PCI_DEVICE(PCI_VENDOR_ID_INTEL,
+PCI_DEVICE_ID_INTEL_QUARK_X1000_UDC),
+ .class = (PCI_CLASS_SERIAL_USB 8) | 0xfe,
+ .class_mask = 0x,
+ },
+ {
PCI_DEVICE(PCI_VENDOR_ID_INTEL,
PCI_DEVICE_ID_INTEL_EG20T_UDC),
.class = (PCI_CLASS_SERIAL_USB 8) | 0xfe,
.class_mask = 0x,
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] USB: pch_udc: Add support for Intel Quark X1000 USB gadget device
From: "Alvin (Weike) Chen" Hi, Intel Quark X1000 consists of one USB gadget device which can be PCI enumerated. pch_udc layer doesn't support it. Thus, we add support for Intel Quark X1000 USB gadget device as well. Bryan O'Donoghue (1): USB: pch_udc: USB gadget device support for Intel Quark X1000 drivers/usb/gadget/udc/Kconfig |3 ++- drivers/usb/gadget/udc/pch_udc.c | 22 +++--- 2 files changed, 21 insertions(+), 4 deletions(-) -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] USB: pch_udc: USB gadget device support for Intel Quark X1000
From: Bryan O'Donoghue
This patch is to enable the USB gadget device for Intel Quark X1000
Signed-off-by: Bryan O'Donoghue
Signed-off-by: Bing Niu
Signed-off-by: Alvin (Weike) Chen
---
drivers/usb/gadget/udc/Kconfig |3 ++-
drivers/usb/gadget/udc/pch_udc.c | 22 +++---
2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
index 5151f94..34ebaa6 100644
--- a/drivers/usb/gadget/udc/Kconfig
+++ b/drivers/usb/gadget/udc/Kconfig
@@ -332,7 +332,7 @@ config USB_GOKU
gadget drivers to also be dynamically linked.
config USB_EG20T
- tristate "Intel EG20T PCH/LAPIS Semiconductor IOH(ML7213/ML7831) UDC"
+ tristate "Intel QUARK X1000/EG20T PCH/LAPIS Semiconductor
IOH(ML7213/ML7831) UDC"
depends on PCI
help
This is a USB device driver for EG20T PCH.
@@ -353,6 +353,7 @@ config USB_EG20T
ML7213/ML7831 is companion chip for Intel Atom E6xx series.
ML7213/ML7831 is completely compatible for Intel EG20T PCH.
+ This driver can be used with Intel's Quark X1000 SOC platform
#
# LAST -- dummy/emulated controller
#
diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c
index eb8c3be..460d953 100644
--- a/drivers/usb/gadget/udc/pch_udc.c
+++ b/drivers/usb/gadget/udc/pch_udc.c
@@ -343,6 +343,7 @@ struct pch_vbus_gpio_data {
* @setup_data:Received setup data
* @phys_addr: of device memory
* @base_addr: for mapped device memory
+ * @bar: Indicates which PCI BAR for USB regs
* @irq: IRQ line for the device
* @cfg_data: current cfg, intf, and alt in use
* @vbus_gpio: GPIO informaton for detecting VBUS
@@ -370,14 +371,17 @@ struct pch_udc_dev {
struct usb_ctrlrequest setup_data;
unsigned long phys_addr;
void __iomem*base_addr;
+ unsignedbar;
unsignedirq;
struct pch_udc_cfg_data cfg_data;
struct pch_vbus_gpio_data vbus_gpio;
};
#define to_pch_udc(g) (container_of((g), struct pch_udc_dev, gadget))
+#define PCH_UDC_PCI_BAR_QUARK_X10000
#define PCH_UDC_PCI_BAR1
#define PCI_DEVICE_ID_INTEL_EG20T_UDC 0x8808
+#define PCI_DEVICE_ID_INTEL_QUARK_X1000_UDC0x0939
#define PCI_VENDOR_ID_ROHM 0x10DB
#define PCI_DEVICE_ID_ML7213_IOH_UDC 0x801D
#define PCI_DEVICE_ID_ML7831_IOH_UDC 0x8808
@@ -3076,7 +3080,7 @@ static void pch_udc_remove(struct pci_dev *pdev)
iounmap(dev->base_addr);
if (dev->mem_region)
release_mem_region(dev->phys_addr,
- pci_resource_len(pdev, PCH_UDC_PCI_BAR));
+ pci_resource_len(pdev, dev->bar));
if (dev->active)
pci_disable_device(pdev);
kfree(dev);
@@ -3144,9 +3148,15 @@ static int pch_udc_probe(struct pci_dev *pdev,
dev->active = 1;
pci_set_drvdata(pdev, dev);
+ /* Determine BAR based on PCI ID */
+ if (id->device == PCI_DEVICE_ID_INTEL_QUARK_X1000_UDC)
+ dev->bar = PCH_UDC_PCI_BAR_QUARK_X1000;
+ else
+ dev->bar = PCH_UDC_PCI_BAR;
+
/* PCI resource allocation */
- resource = pci_resource_start(pdev, 1);
- len = pci_resource_len(pdev, 1);
+ resource = pci_resource_start(pdev, dev->bar);
+ len = pci_resource_len(pdev, dev->bar);
if (!request_mem_region(resource, len, KBUILD_MODNAME)) {
dev_err(>dev, "%s: pci device used already\n", __func__);
@@ -3212,6 +3222,12 @@ finished:
static const struct pci_device_id pch_udc_pcidev_id[] = {
{
+ PCI_DEVICE(PCI_VENDOR_ID_INTEL,
+ PCI_DEVICE_ID_INTEL_QUARK_X1000_UDC),
+ .class = (PCI_CLASS_SERIAL_USB << 8) | 0xfe,
+ .class_mask = 0x,
+ },
+ {
PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_EG20T_UDC),
.class = (PCI_CLASS_SERIAL_USB << 8) | 0xfe,
.class_mask = 0x,
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] USB: pch_udc: USB gadget device support for Intel Quark X1000
From: Bryan O'Donoghue bryan.odonog...@intel.com
This patch is to enable the USB gadget device for Intel Quark X1000
Signed-off-by: Bryan O'Donoghue bryan.odonog...@intel.com
Signed-off-by: Bing Niu bing@intel.com
Signed-off-by: Alvin (Weike) Chen alvin.c...@intel.com
---
drivers/usb/gadget/udc/Kconfig |3 ++-
drivers/usb/gadget/udc/pch_udc.c | 22 +++---
2 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
index 5151f94..34ebaa6 100644
--- a/drivers/usb/gadget/udc/Kconfig
+++ b/drivers/usb/gadget/udc/Kconfig
@@ -332,7 +332,7 @@ config USB_GOKU
gadget drivers to also be dynamically linked.
config USB_EG20T
- tristate Intel EG20T PCH/LAPIS Semiconductor IOH(ML7213/ML7831) UDC
+ tristate Intel QUARK X1000/EG20T PCH/LAPIS Semiconductor
IOH(ML7213/ML7831) UDC
depends on PCI
help
This is a USB device driver for EG20T PCH.
@@ -353,6 +353,7 @@ config USB_EG20T
ML7213/ML7831 is companion chip for Intel Atom E6xx series.
ML7213/ML7831 is completely compatible for Intel EG20T PCH.
+ This driver can be used with Intel's Quark X1000 SOC platform
#
# LAST -- dummy/emulated controller
#
diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c
index eb8c3be..460d953 100644
--- a/drivers/usb/gadget/udc/pch_udc.c
+++ b/drivers/usb/gadget/udc/pch_udc.c
@@ -343,6 +343,7 @@ struct pch_vbus_gpio_data {
* @setup_data:Received setup data
* @phys_addr: of device memory
* @base_addr: for mapped device memory
+ * @bar: Indicates which PCI BAR for USB regs
* @irq: IRQ line for the device
* @cfg_data: current cfg, intf, and alt in use
* @vbus_gpio: GPIO informaton for detecting VBUS
@@ -370,14 +371,17 @@ struct pch_udc_dev {
struct usb_ctrlrequest setup_data;
unsigned long phys_addr;
void __iomem*base_addr;
+ unsignedbar;
unsignedirq;
struct pch_udc_cfg_data cfg_data;
struct pch_vbus_gpio_data vbus_gpio;
};
#define to_pch_udc(g) (container_of((g), struct pch_udc_dev, gadget))
+#define PCH_UDC_PCI_BAR_QUARK_X10000
#define PCH_UDC_PCI_BAR1
#define PCI_DEVICE_ID_INTEL_EG20T_UDC 0x8808
+#define PCI_DEVICE_ID_INTEL_QUARK_X1000_UDC0x0939
#define PCI_VENDOR_ID_ROHM 0x10DB
#define PCI_DEVICE_ID_ML7213_IOH_UDC 0x801D
#define PCI_DEVICE_ID_ML7831_IOH_UDC 0x8808
@@ -3076,7 +3080,7 @@ static void pch_udc_remove(struct pci_dev *pdev)
iounmap(dev-base_addr);
if (dev-mem_region)
release_mem_region(dev-phys_addr,
- pci_resource_len(pdev, PCH_UDC_PCI_BAR));
+ pci_resource_len(pdev, dev-bar));
if (dev-active)
pci_disable_device(pdev);
kfree(dev);
@@ -3144,9 +3148,15 @@ static int pch_udc_probe(struct pci_dev *pdev,
dev-active = 1;
pci_set_drvdata(pdev, dev);
+ /* Determine BAR based on PCI ID */
+ if (id-device == PCI_DEVICE_ID_INTEL_QUARK_X1000_UDC)
+ dev-bar = PCH_UDC_PCI_BAR_QUARK_X1000;
+ else
+ dev-bar = PCH_UDC_PCI_BAR;
+
/* PCI resource allocation */
- resource = pci_resource_start(pdev, 1);
- len = pci_resource_len(pdev, 1);
+ resource = pci_resource_start(pdev, dev-bar);
+ len = pci_resource_len(pdev, dev-bar);
if (!request_mem_region(resource, len, KBUILD_MODNAME)) {
dev_err(pdev-dev, %s: pci device used already\n, __func__);
@@ -3212,6 +3222,12 @@ finished:
static const struct pci_device_id pch_udc_pcidev_id[] = {
{
+ PCI_DEVICE(PCI_VENDOR_ID_INTEL,
+ PCI_DEVICE_ID_INTEL_QUARK_X1000_UDC),
+ .class = (PCI_CLASS_SERIAL_USB 8) | 0xfe,
+ .class_mask = 0x,
+ },
+ {
PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_EG20T_UDC),
.class = (PCI_CLASS_SERIAL_USB 8) | 0xfe,
.class_mask = 0x,
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] USB: pch_udc: Add support for Intel Quark X1000 USB gadget device
From: Alvin (Weike) Chen alvin.c...@intel.com Hi, Intel Quark X1000 consists of one USB gadget device which can be PCI enumerated. pch_udc layer doesn't support it. Thus, we add support for Intel Quark X1000 USB gadget device as well. Bryan O'Donoghue (1): USB: pch_udc: USB gadget device support for Intel Quark X1000 drivers/usb/gadget/udc/Kconfig |3 ++- drivers/usb/gadget/udc/pch_udc.c | 22 +++--- 2 files changed, 21 insertions(+), 4 deletions(-) -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5] USB: ehci-pci: USB host controller support for Intel Quark X1000
From: Bryan O'Donoghue
The EHCI packet buffer in/out threshold is programmable for Intel Quark X1000
USB host controller, and the default value is 0x20 dwords. The in/out threshold
can be programmed to 0x80 dwords (512 Bytes) to maximize the perfomrance,
but only when isochronous/interrupt transactions are not initiated by the USB
host controller. This patch is to reconfigure the packet buffer in/out
threshold as maximal as possible to maximize the performance, and 0x7F dwords
(508 Bytes) should be used because the USB host controller initiates
isochronous/interrupt transactions.
Signed-off-by: Bryan O'Donoghue
Signed-off-by: Alvin (Weike) Chen
Acked-by: Alan Stern
Reviewed-by: Jingoo Han
---
Changlog v5:
* Correct the wrong comment style.
drivers/usb/host/ehci-pci.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 3e86bf4..ca7b964 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -35,6 +35,21 @@ static const char hcd_name[] = "ehci-pci";
#define PCI_DEVICE_ID_INTEL_CE4100_USB 0x2e70
/*-*/
+#define PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC0x0939
+static inline bool is_intel_quark_x1000(struct pci_dev *pdev)
+{
+ return pdev->vendor == PCI_VENDOR_ID_INTEL &&
+ pdev->device == PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC;
+}
+
+/*
+ * 0x84 is the offset of in/out threshold register,
+ * and it is the same offset as the register of 'hostpc'.
+ */
+#defineintel_quark_x1000_insnreg01 hostpc
+
+/* Maximum usable threshold value is 0x7f dwords for both IN and OUT */
+#define INTEL_QUARK_X1000_EHCI_MAX_THRESHOLD 0x007f007f
/* called after powerup, by probe or system-pm "wakeup" */
static int ehci_pci_reinit(struct ehci_hcd *ehci, struct pci_dev *pdev)
@@ -50,6 +65,16 @@ static int ehci_pci_reinit(struct ehci_hcd *ehci, struct
pci_dev *pdev)
if (!retval)
ehci_dbg(ehci, "MWI active\n");
+ /* Reset the threshold limit */
+ if (is_intel_quark_x1000(pdev)) {
+ /*
+* For the Intel QUARK X1000, raise the I/O threshold to the
+* maximum usable value in order to improve performance.
+*/
+ ehci_writel(ehci, INTEL_QUARK_X1000_EHCI_MAX_THRESHOLD,
+ ehci->regs->intel_quark_x1000_insnreg01);
+ }
+
return 0;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4] USB: ehci-pci: USB host controller support for Intel Quark X1000
From: Bryan O'Donoghue
The EHCI packet buffer in/out threshold is programmable for Intel Quark X1000
USB host controller, and the default value is 0x20 dwords. The in/out threshold
can be programmed to 0x80 dwords (512 Bytes) to maximize the perfomrance,
but only when isochronous/interrupt transactions are not initiated by the USB
host controller. This patch is to reconfigure the packet buffer in/out
threshold as maximal as possible to maximize the performance, and 0x7F dwords
(508 Bytes) should be used because the USB host controller initiates
isochronous/interrupt transactions.
Signed-off-by: Bryan O'Donoghue
Signed-off-by: Alvin (Weike) Chen
---
changelog v4:
* Just define the maximum threshold value, and clarify it in the comments.
* Improve the comments.
drivers/usb/host/ehci-pci.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 3e86bf4..429434d 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -35,6 +35,21 @@ static const char hcd_name[] = "ehci-pci";
#define PCI_DEVICE_ID_INTEL_CE4100_USB 0x2e70
/*-*/
+#define PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC0x0939
+static inline bool is_intel_quark_x1000(struct pci_dev *pdev)
+{
+ return pdev->vendor == PCI_VENDOR_ID_INTEL &&
+ pdev->device == PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC;
+}
+
+/*
+ * 0x84 is the offset of in/out threshold register,
+ * and it is the same offset as the register of 'hostpc'.
+ */
+#defineintel_quark_x1000_insnreg01 hostpc
+
+/* Maximum usable threshold value is 0x7f dwords for both IN and OUT */
+#define INTEL_QUARK_X1000_EHCI_MAX_THRESHOLD 0x007f007f
/* called after powerup, by probe or system-pm "wakeup" */
static int ehci_pci_reinit(struct ehci_hcd *ehci, struct pci_dev *pdev)
@@ -50,6 +65,16 @@ static int ehci_pci_reinit(struct ehci_hcd *ehci, struct
pci_dev *pdev)
if (!retval)
ehci_dbg(ehci, "MWI active\n");
+ /* Reset the threshold limit */
+ if (is_intel_quark_x1000(pdev)) {
+ /*
+* For the Intel QUARK X1000, raise the I/O threshold to the
+* maximum usable value in order to improve performance.
+*/
+ ehci_writel(ehci, INTEL_QUARK_X1000_EHCI_MAX_THRESHOLD,
+ ehci->regs->intel_quark_x1000_insnreg01);
+ }
+
return 0;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4] USB: ehci-pci: Add support for Intel Quark X1000 USB
From: "Alvin (Weike) Chen" Hi, Intel Quark X1000 consists of one USB host controller which can be PCI enumerated. And the exsiting EHCI-PCI framework supports it with the default packet buffer in/out threshold. We reconfigure the in/out threshold as maximal as possible to maximize the performance. Bryan O'Donoghue (1): USB: ehci-pci: USB host controller support for Intel Quark X1000 drivers/usb/host/ehci-pci.c | 25 +++ 1 file changed, 25 insertions(+) -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4] USB: ehci-pci: USB host controller support for Intel Quark X1000
From: Bryan O'Donoghue bryan.odonog...@intel.com
The EHCI packet buffer in/out threshold is programmable for Intel Quark X1000
USB host controller, and the default value is 0x20 dwords. The in/out threshold
can be programmed to 0x80 dwords (512 Bytes) to maximize the perfomrance,
but only when isochronous/interrupt transactions are not initiated by the USB
host controller. This patch is to reconfigure the packet buffer in/out
threshold as maximal as possible to maximize the performance, and 0x7F dwords
(508 Bytes) should be used because the USB host controller initiates
isochronous/interrupt transactions.
Signed-off-by: Bryan O'Donoghue bryan.odonog...@intel.com
Signed-off-by: Alvin (Weike) Chen alvin.c...@intel.com
---
changelog v4:
* Just define the maximum threshold value, and clarify it in the comments.
* Improve the comments.
drivers/usb/host/ehci-pci.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 3e86bf4..429434d 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -35,6 +35,21 @@ static const char hcd_name[] = ehci-pci;
#define PCI_DEVICE_ID_INTEL_CE4100_USB 0x2e70
/*-*/
+#define PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC0x0939
+static inline bool is_intel_quark_x1000(struct pci_dev *pdev)
+{
+ return pdev-vendor == PCI_VENDOR_ID_INTEL
+ pdev-device == PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC;
+}
+
+/*
+ * 0x84 is the offset of in/out threshold register,
+ * and it is the same offset as the register of 'hostpc'.
+ */
+#defineintel_quark_x1000_insnreg01 hostpc
+
+/* Maximum usable threshold value is 0x7f dwords for both IN and OUT */
+#define INTEL_QUARK_X1000_EHCI_MAX_THRESHOLD 0x007f007f
/* called after powerup, by probe or system-pm wakeup */
static int ehci_pci_reinit(struct ehci_hcd *ehci, struct pci_dev *pdev)
@@ -50,6 +65,16 @@ static int ehci_pci_reinit(struct ehci_hcd *ehci, struct
pci_dev *pdev)
if (!retval)
ehci_dbg(ehci, MWI active\n);
+ /* Reset the threshold limit */
+ if (is_intel_quark_x1000(pdev)) {
+ /*
+* For the Intel QUARK X1000, raise the I/O threshold to the
+* maximum usable value in order to improve performance.
+*/
+ ehci_writel(ehci, INTEL_QUARK_X1000_EHCI_MAX_THRESHOLD,
+ ehci-regs-intel_quark_x1000_insnreg01);
+ }
+
return 0;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4] USB: ehci-pci: Add support for Intel Quark X1000 USB
From: Alvin (Weike) Chen alvin.c...@intel.com Hi, Intel Quark X1000 consists of one USB host controller which can be PCI enumerated. And the exsiting EHCI-PCI framework supports it with the default packet buffer in/out threshold. We reconfigure the in/out threshold as maximal as possible to maximize the performance. Bryan O'Donoghue (1): USB: ehci-pci: USB host controller support for Intel Quark X1000 drivers/usb/host/ehci-pci.c | 25 +++ 1 file changed, 25 insertions(+) -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5] USB: ehci-pci: USB host controller support for Intel Quark X1000
From: Bryan O'Donoghue bryan.odonog...@intel.com
The EHCI packet buffer in/out threshold is programmable for Intel Quark X1000
USB host controller, and the default value is 0x20 dwords. The in/out threshold
can be programmed to 0x80 dwords (512 Bytes) to maximize the perfomrance,
but only when isochronous/interrupt transactions are not initiated by the USB
host controller. This patch is to reconfigure the packet buffer in/out
threshold as maximal as possible to maximize the performance, and 0x7F dwords
(508 Bytes) should be used because the USB host controller initiates
isochronous/interrupt transactions.
Signed-off-by: Bryan O'Donoghue bryan.odonog...@intel.com
Signed-off-by: Alvin (Weike) Chen alvin.c...@intel.com
Acked-by: Alan Stern st...@rowland.harvard.edu
Reviewed-by: Jingoo Han jg1@samsung.com
---
Changlog v5:
* Correct the wrong comment style.
drivers/usb/host/ehci-pci.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 3e86bf4..ca7b964 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -35,6 +35,21 @@ static const char hcd_name[] = ehci-pci;
#define PCI_DEVICE_ID_INTEL_CE4100_USB 0x2e70
/*-*/
+#define PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC0x0939
+static inline bool is_intel_quark_x1000(struct pci_dev *pdev)
+{
+ return pdev-vendor == PCI_VENDOR_ID_INTEL
+ pdev-device == PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC;
+}
+
+/*
+ * 0x84 is the offset of in/out threshold register,
+ * and it is the same offset as the register of 'hostpc'.
+ */
+#defineintel_quark_x1000_insnreg01 hostpc
+
+/* Maximum usable threshold value is 0x7f dwords for both IN and OUT */
+#define INTEL_QUARK_X1000_EHCI_MAX_THRESHOLD 0x007f007f
/* called after powerup, by probe or system-pm wakeup */
static int ehci_pci_reinit(struct ehci_hcd *ehci, struct pci_dev *pdev)
@@ -50,6 +65,16 @@ static int ehci_pci_reinit(struct ehci_hcd *ehci, struct
pci_dev *pdev)
if (!retval)
ehci_dbg(ehci, MWI active\n);
+ /* Reset the threshold limit */
+ if (is_intel_quark_x1000(pdev)) {
+ /*
+* For the Intel QUARK X1000, raise the I/O threshold to the
+* maximum usable value in order to improve performance.
+*/
+ ehci_writel(ehci, INTEL_QUARK_X1000_EHCI_MAX_THRESHOLD,
+ ehci-regs-intel_quark_x1000_insnreg01);
+ }
+
return 0;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3] USB: ehci-pci: USB host controller support for Intel Quark X1000
> >
> > /*
> > -*/
> > +#define PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC0x0939
> > +static inline bool is_intel_quark_x1000(struct pci_dev *pdev) {
> > + return pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > + pdev->device == PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC;
> > +}
>
> Whether to put this test directly into ehci_pci_reset() or leave it as a
> separate
> subroutine is up to you. I don't care either way.
I will just keep it.
> > +
> > +/*
> > + * The offset of in/out threshold register is 0x84.
> > + * And it is the register of 'hostpc'
> > + * in memory-mapped EHCI controller.
> > +*/
>
> 0x84 is the same as offset of the hostpc register in the Intel Moorestown
> controller. hostpc is not present in general EHCI controllers.
>
OK, I will improve the comments.
> > +#defineintel_quark_x1000_insnreg01 hostpc
> > +
> > +/* The maximal ehci packet buffer size is 512 bytes */
> > +#define INTEL_QUARK_X1000_EHCI_MAX_PACKET_BUFFER_SIZE 512
> > +
> > +/* The threshold value set the register is in DWORD */
> > +#define INTEL_QUARK_X1000_EHCI_THRESHOLD(size) ((size)/4u)
> > +#define INTEL_QUARK_X1000_EHCI_THRESHOLD_OUT_SHIFT 16
> > +#define INTEL_QUARK_X1000_EHCI_THRESHOLD_IN_SHIFT 0
> > +
> > /* called after powerup, by probe or system-pm "wakeup" */ static
> > int ehci_pci_reinit(struct ehci_hcd *ehci, struct pci_dev *pdev) {
> > int retval;
> > + u32 val;
> > + u32 thr;
> >
> > /* we expect static quirk code to handle the "extended capabilities"
> > * (currently just BIOS handoff) allowed starting with EHCI 0.96 @@
> > -50,6 +74,22 @@ static int ehci_pci_reinit(struct ehci_hcd *ehci, struct
> pci_dev *pdev)
> > if (!retval)
> > ehci_dbg(ehci, "MWI active\n");
> >
> > + /* Reset the threshold limit */
> > + if (is_intel_quark_x1000(pdev)) {
> > + /*
> > + * In order to support the isochronous/interrupt
> > + * transactions, 508 bytes should be used as
> > + * max threshold values to maximize the
> > + * performance
> > + */
> > + thr = INTEL_QUARK_X1000_EHCI_THRESHOLD(
> > + INTEL_QUARK_X1000_EHCI_MAX_PACKET_BUFFER_SIZE - 4
> > + );
> > + val = thr< > + thr< > + ehci_writel(ehci, val, ehci->regs->intel_quark_x1000_insnreg01);
>
> I saw what other people told you about the original patch version, and I
> disagree with them. It is not necessary to include a detailed calculation
> like
> this, it only makes the code harder to read. It will be better to have a
> single
> #define with a comment explaining it, like
> this:
>
> /* Maximum usable threshold value is 0x7f dwords for both IN and OUT */
> #define INTEL_QUARK_X1000_EHCI_MAX_THRESHOLD 0x007f007f
>
> Then here, just use INTEL_QUARK_X1000_EHCI_MAX_THRESHOLD instead of
> val. The comment can simply say:
>
> /*
>* For the Intel QUARK X1000, raise the I/O threshold to the
>* maximum usable value in order to improve performance.
>*/
>
I think so also. It is not necessary to make so complicated. I will adopt your
suggestions, it is more simple and clearly.
> Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3] USB: ehci-pci: USB host controller support for Intel Quark X1000
> > /*
> > -*/
> > +#define PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC0x0939
> > +static inline bool is_intel_quark_x1000(struct pci_dev *pdev) {
> > + return pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > + pdev->device == PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC;
> > +}
>
> Why not just put this check inline into ehci_pci_reinit()?
Alan Stern said it is not a problem, I think so also since it just a inline
subroutine.
>
> > +
> > +/*
> > + * The offset of in/out threshold register is 0x84.
> > + * And it is the register of 'hostpc'
> > + * in memory-mapped EHCI controller.
> > +*/
>
> The preferred multi-line kernel style is this:
>
> /*
> * bla
> * bla
> */
I will improve it.
> > +#defineintel_quark_x1000_insnreg01 hostpc
> > +
> > +/* The maximal ehci packet buffer size is 512 bytes */
>
> s/ehci/EHCI/
>
> > +#define INTEL_QUARK_X1000_EHCI_MAX_PACKET_BUFFER_SIZE 512
> > +
> > +/* The threshold value set the register is in DWORD */
> > +#define INTEL_QUARK_X1000_EHCI_THRESHOLD(size) ((size)/4u)
> > +#define INTEL_QUARK_X1000_EHCI_THRESHOLD_OUT_SHIFT 16
> > +#define INTEL_QUARK_X1000_EHCI_THRESHOLD_IN_SHIFT 0
> > +
> >
>
> Too many empty lines...
>
> > /* called after powerup, by probe or system-pm "wakeup" */
> > static int ehci_pci_reinit(struct ehci_hcd *ehci, struct pci_dev *pdev)
> > {
> > int retval;
> > + u32 val;
> > + u32 thr;
>
> Why not declare these where they are used?
All will be removed as Alan Stern's suggestion.
> > + /* Reset the threshold limit */
> > + if (is_intel_quark_x1000(pdev)) {
> > + /*
> > + * In order to support the isochronous/interrupt
> > + * transactions, 508 bytes should be used as
> > + * max threshold values to maximize the
> > + * performance
> > + */
>
> Same comment about the comment style...
>
> > + thr = INTEL_QUARK_X1000_EHCI_THRESHOLD(
> > + INTEL_QUARK_X1000_EHCI_MAX_PACKET_BUFFER_SIZE - 4
> > + );
> > + val = thr< > + thr<
> Please surround << with spaces for consistency.
The above code will be removed as Alan Stern's suggestion.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3] USB: ehci-pci: Add support for Intel Quark X1000 USB
From: "Alvin (Weike) Chen" Hi, Intel Quark X1000 consists of one USB host controller which can be PCI enumerated. And the exsiting EHCI-PCI framework supports it with the default packet buffer in/out threshold. We reconfigure the in/out threshold as maximal as possible to maximize the performance. Bryan O'Donoghue (1): USB: ehci-pci: USB host controller support for Intel Quark X1000 drivers/usb/host/ehci-pci.c | 40 +++ 1 file changed, 40 insertions(+) -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3] USB: ehci-pci: USB host controller support for Intel Quark X1000
From: Bryan O'Donoghue
The EHCI packet buffer in/out threshold is programmable for Intel Quark X1000
USB host controller, and the default value is 0x20 dwords. The in/out threshold
can be programmed to 0x80 dwords (512 Bytes) to maximize the perfomrance,
but only when isochronous/interrupt transactions are not initiated by the USB
host controller. This patch is to reconfigure the packet buffer in/out
threshold as maximal as possible to maximize the performance, and 0x7F dwords
(508 Bytes) should be used because the USB host controller initiates
isochronous/interrupt transactions.
Signed-off-by: Bryan O'Donoghue
Signed-off-by: Alvin (Weike) Chen
---
changelog v3:
*Update the description to explain why set in/out threshold
as maximal as possible.
*Improve the threshold value micros more readable.
*Remove 'usb_set_qrk_bulk_thresh'.
*Set threshold value by 'ehci_writel' instead of 'usb_set_qrk_bulk_thresh'.
*Change the function name from 'usb_is_intel_quark_x1000'
to 'is_intel_quark_x1000'.
drivers/usb/host/ehci-pci.c | 40
1 file changed, 40 insertions(+)
diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 3e86bf4..78f1622 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -35,11 +35,35 @@ static const char hcd_name[] = "ehci-pci";
#define PCI_DEVICE_ID_INTEL_CE4100_USB 0x2e70
/*-*/
+#define PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC0x0939
+static inline bool is_intel_quark_x1000(struct pci_dev *pdev)
+{
+ return pdev->vendor == PCI_VENDOR_ID_INTEL &&
+ pdev->device == PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC;
+}
+
+/*
+ * The offset of in/out threshold register is 0x84.
+ * And it is the register of 'hostpc'
+ * in memory-mapped EHCI controller.
+*/
+#defineintel_quark_x1000_insnreg01 hostpc
+
+/* The maximal ehci packet buffer size is 512 bytes */
+#define INTEL_QUARK_X1000_EHCI_MAX_PACKET_BUFFER_SIZE 512
+
+/* The threshold value set the register is in DWORD */
+#define INTEL_QUARK_X1000_EHCI_THRESHOLD(size) ((size)/4u)
+#define INTEL_QUARK_X1000_EHCI_THRESHOLD_OUT_SHIFT 16
+#define INTEL_QUARK_X1000_EHCI_THRESHOLD_IN_SHIFT 0
+
/* called after powerup, by probe or system-pm "wakeup" */
static int ehci_pci_reinit(struct ehci_hcd *ehci, struct pci_dev *pdev)
{
int retval;
+ u32 val;
+ u32 thr;
/* we expect static quirk code to handle the "extended capabilities"
* (currently just BIOS handoff) allowed starting with EHCI 0.96
@@ -50,6 +74,22 @@ static int ehci_pci_reinit(struct ehci_hcd *ehci, struct
pci_dev *pdev)
if (!retval)
ehci_dbg(ehci, "MWI active\n");
+ /* Reset the threshold limit */
+ if (is_intel_quark_x1000(pdev)) {
+ /*
+ * In order to support the isochronous/interrupt
+ * transactions, 508 bytes should be used as
+ * max threshold values to maximize the
+ * performance
+ */
+ thr = INTEL_QUARK_X1000_EHCI_THRESHOLD(
+ INTEL_QUARK_X1000_EHCI_MAX_PACKET_BUFFER_SIZE - 4
+ );
+ val = thrintel_quark_x1000_insnreg01);
+ }
+
return 0;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2] USB: ehci-pci: USB host controller support for Intel Quark X1000
> > The EHCI packet buffer in/out threshold is programmable for Intel
> > Quark X1000 USB host controller, and the default value is 0x20 dwords.
> > The in/out threshold can be programmed to 0x80 dwords, but only when
> > isochronous/interrupt transactions are not initiated by the USB host
> > controller. This patch is to reconfigure the packet buffer in/out
> > threshold as maximal as possible, and it is 0x7F dwords since the USB host
> controller initiates isochronous/interrupt transactions.
>
> This is still incomplete. _Why_ do you want to increase the threshold?
> Does it fix a problem? Does it improve performance?
Try to set threshold as maximal as possible to maximize the performance. I will
update the descriptions.
> Also, the lines are too long. They should be wrapped before 80 columns.
Got you.
> > +#define PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC0x0939
> > +static inline bool usb_is_intel_quark_x1000(struct pci_dev *pdev) {
> > + return pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > + pdev->device == PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC;
> > +
> > +}
>
> The "usb_" prefix in the name isn't needed, because this is a static routine.
OK, I will remove the 'usb_' prefix.
> > +static void usb_set_qrk_bulk_thresh(struct pci_dev *pdev) {
> > + void __iomem *base, *op_reg_base;
> > + u8 cap_length;
> > + u32 val;
> > + u16 cmd;
> > +
> > + if (!pci_resource_start(pdev, 0))
> > + return;
> > +
> > + if (pci_read_config_word(pdev, PCI_COMMAND, )
> > + || !(cmd & PCI_COMMAND_MEMORY))
> > + return;
> > +
> > + base = pci_ioremap_bar(pdev, 0);
> > + if (base == NULL)
> > + return;
> > +
> > + cap_length = readb(base);
> > + op_reg_base = base + cap_length;
> > +
> > + val = EHCI_INSNREG01_THRESH;
> > + writel(val, op_reg_base + EHCI_INSNREG01);
> > +
> > + iounmap(base);
> > +}
>
> This is much more complicted that it needs to be. When this routine runs, the
> controller has already been memory-mapped. All you need to do is:
>
> ehci_writel(ehci, EHCI_INSNREG01_THRESH,
> ehci->regs->insnreg01_thresh);
>
> Since it's only one statement, you don't even need to put this in a separate
> function. It can go directly into ehci_pci_reinit().
OK, I will remove ' usb_set_qrk_bulk_thresh', and change the code as the
suggestions.
> Also, in include/linux/usb/ehci_defs.h, you'll have to add:
>
> #define insnreg01_thresh hostpc[0]
I will add it in ehci-pci.c instead of /linux/usb/ehci_defs.h, because it is
not a generic micro, but just used for Intel Quark X1000.
> with an explanatory comment, near the definition of the HOSTPC register.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2] USB: ehci-pci: USB host controller support for Intel Quark X1000
The EHCI packet buffer in/out threshold is programmable for Intel
Quark X1000 USB host controller, and the default value is 0x20 dwords.
The in/out threshold can be programmed to 0x80 dwords, but only when
isochronous/interrupt transactions are not initiated by the USB host
controller. This patch is to reconfigure the packet buffer in/out
threshold as maximal as possible, and it is 0x7F dwords since the USB host
controller initiates isochronous/interrupt transactions.
This is still incomplete. _Why_ do you want to increase the threshold?
Does it fix a problem? Does it improve performance?
Try to set threshold as maximal as possible to maximize the performance. I will
update the descriptions.
Also, the lines are too long. They should be wrapped before 80 columns.
Got you.
+#define PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC0x0939
+static inline bool usb_is_intel_quark_x1000(struct pci_dev *pdev) {
+ return pdev-vendor == PCI_VENDOR_ID_INTEL
+ pdev-device == PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC;
+
+}
The usb_ prefix in the name isn't needed, because this is a static routine.
OK, I will remove the 'usb_' prefix.
+static void usb_set_qrk_bulk_thresh(struct pci_dev *pdev) {
+ void __iomem *base, *op_reg_base;
+ u8 cap_length;
+ u32 val;
+ u16 cmd;
+
+ if (!pci_resource_start(pdev, 0))
+ return;
+
+ if (pci_read_config_word(pdev, PCI_COMMAND, cmd)
+ || !(cmd PCI_COMMAND_MEMORY))
+ return;
+
+ base = pci_ioremap_bar(pdev, 0);
+ if (base == NULL)
+ return;
+
+ cap_length = readb(base);
+ op_reg_base = base + cap_length;
+
+ val = EHCI_INSNREG01_THRESH;
+ writel(val, op_reg_base + EHCI_INSNREG01);
+
+ iounmap(base);
+}
This is much more complicted that it needs to be. When this routine runs, the
controller has already been memory-mapped. All you need to do is:
ehci_writel(ehci, EHCI_INSNREG01_THRESH,
ehci-regs-insnreg01_thresh);
Since it's only one statement, you don't even need to put this in a separate
function. It can go directly into ehci_pci_reinit().
OK, I will remove ' usb_set_qrk_bulk_thresh', and change the code as the
suggestions.
Also, in include/linux/usb/ehci_defs.h, you'll have to add:
#define insnreg01_thresh hostpc[0]
I will add it in ehci-pci.c instead of /linux/usb/ehci_defs.h, because it is
not a generic micro, but just used for Intel Quark X1000.
with an explanatory comment, near the definition of the HOSTPC register.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3] USB: ehci-pci: USB host controller support for Intel Quark X1000
From: Bryan O'Donoghue bryan.odonog...@intel.com
The EHCI packet buffer in/out threshold is programmable for Intel Quark X1000
USB host controller, and the default value is 0x20 dwords. The in/out threshold
can be programmed to 0x80 dwords (512 Bytes) to maximize the perfomrance,
but only when isochronous/interrupt transactions are not initiated by the USB
host controller. This patch is to reconfigure the packet buffer in/out
threshold as maximal as possible to maximize the performance, and 0x7F dwords
(508 Bytes) should be used because the USB host controller initiates
isochronous/interrupt transactions.
Signed-off-by: Bryan O'Donoghue bryan.odonog...@intel.com
Signed-off-by: Alvin (Weike) Chen alvin.c...@intel.com
---
changelog v3:
*Update the description to explain why set in/out threshold
as maximal as possible.
*Improve the threshold value micros more readable.
*Remove 'usb_set_qrk_bulk_thresh'.
*Set threshold value by 'ehci_writel' instead of 'usb_set_qrk_bulk_thresh'.
*Change the function name from 'usb_is_intel_quark_x1000'
to 'is_intel_quark_x1000'.
drivers/usb/host/ehci-pci.c | 40
1 file changed, 40 insertions(+)
diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 3e86bf4..78f1622 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -35,11 +35,35 @@ static const char hcd_name[] = ehci-pci;
#define PCI_DEVICE_ID_INTEL_CE4100_USB 0x2e70
/*-*/
+#define PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC0x0939
+static inline bool is_intel_quark_x1000(struct pci_dev *pdev)
+{
+ return pdev-vendor == PCI_VENDOR_ID_INTEL
+ pdev-device == PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC;
+}
+
+/*
+ * The offset of in/out threshold register is 0x84.
+ * And it is the register of 'hostpc'
+ * in memory-mapped EHCI controller.
+*/
+#defineintel_quark_x1000_insnreg01 hostpc
+
+/* The maximal ehci packet buffer size is 512 bytes */
+#define INTEL_QUARK_X1000_EHCI_MAX_PACKET_BUFFER_SIZE 512
+
+/* The threshold value set the register is in DWORD */
+#define INTEL_QUARK_X1000_EHCI_THRESHOLD(size) ((size)/4u)
+#define INTEL_QUARK_X1000_EHCI_THRESHOLD_OUT_SHIFT 16
+#define INTEL_QUARK_X1000_EHCI_THRESHOLD_IN_SHIFT 0
+
/* called after powerup, by probe or system-pm wakeup */
static int ehci_pci_reinit(struct ehci_hcd *ehci, struct pci_dev *pdev)
{
int retval;
+ u32 val;
+ u32 thr;
/* we expect static quirk code to handle the extended capabilities
* (currently just BIOS handoff) allowed starting with EHCI 0.96
@@ -50,6 +74,22 @@ static int ehci_pci_reinit(struct ehci_hcd *ehci, struct
pci_dev *pdev)
if (!retval)
ehci_dbg(ehci, MWI active\n);
+ /* Reset the threshold limit */
+ if (is_intel_quark_x1000(pdev)) {
+ /*
+ * In order to support the isochronous/interrupt
+ * transactions, 508 bytes should be used as
+ * max threshold values to maximize the
+ * performance
+ */
+ thr = INTEL_QUARK_X1000_EHCI_THRESHOLD(
+ INTEL_QUARK_X1000_EHCI_MAX_PACKET_BUFFER_SIZE - 4
+ );
+ val = thrINTEL_QUARK_X1000_EHCI_THRESHOLD_OUT_SHIFT |
+ thrINTEL_QUARK_X1000_EHCI_THRESHOLD_IN_SHIFT;
+ ehci_writel(ehci, val, ehci-regs-intel_quark_x1000_insnreg01);
+ }
+
return 0;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3] USB: ehci-pci: Add support for Intel Quark X1000 USB
From: Alvin (Weike) Chen alvin.c...@intel.com Hi, Intel Quark X1000 consists of one USB host controller which can be PCI enumerated. And the exsiting EHCI-PCI framework supports it with the default packet buffer in/out threshold. We reconfigure the in/out threshold as maximal as possible to maximize the performance. Bryan O'Donoghue (1): USB: ehci-pci: USB host controller support for Intel Quark X1000 drivers/usb/host/ehci-pci.c | 40 +++ 1 file changed, 40 insertions(+) -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
