* Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings
From: Sakari Ailus @ 2017-04-18 10:08 UTC (permalink / raw)
To: Philipp Zabel
Cc: Peter Rosin, Pavel Machek, Rob Herring, Mark Rutland,
Steve Longerbeam, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ
In-Reply-To: <1492503544.2432.32.camel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Hi Philipp,
On Tue, Apr 18, 2017 at 10:19:04AM +0200, Philipp Zabel wrote:
> On Thu, 2017-04-13 at 17:48 +0200, Philipp Zabel wrote:
> > This adds device tree binding documentation for mmio-based syscon
> > multiplexers controlled by a single bitfield in a syscon register
> > range.
> >
> > Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > ---
> > Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> > 1 file changed, 56 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> >
> > diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > new file mode 100644
> > index 0000000000000..11d96f5d98583
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > @@ -0,0 +1,56 @@
> > +MMIO bitfield-based multiplexer controller bindings
> > +
> > +Define a syscon bitfield to be used to control a multiplexer. The parent
> > +device tree node must be a syscon node to provide register access.
> > +
> > +Required properties:
> > +- compatible : "gpio-mux"
> > +- reg : register base of the register containing the control bitfield
> > +- bit-mask : bitmask of the control bitfield in the control register
> > +- bit-shift : bit offset of the control bitfield in the control register
> > +- #mux-control-cells : <0>
> > +* Standard mux-controller bindings as decribed in mux-controller.txt
> > +
> > +Optional properties:
> > +- idle-state : if present, the state the mux will have when idle. The
> > + special state MUX_IDLE_AS_IS is the default.
> > +
> > +The multiplexer state is defined as the value of the bitfield described
> > +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> > +syscon.
> > +
> > +Example:
> > +
> > + syscon {
> > + compatible = "syscon";
> > +
> > + mux: mux-controller@3 {
> > + compatible = "mmio-mux";
> > + reg = <0x3>;
> > + bit-mask = <0x1>;
> > + bit-shift = <5>;
> > + #mux-control-cells = <0>;
> > + };
> > + };
> > +
> > + video-mux {
> > + compatible = "video-mux";
> > + mux-controls = <&mux>;
> > +
> > + ports {
> > + /* input 0 */
> > + port@0 {
> > + reg = <0>;
> > + };
> > +
> > + /* input 1 */
> > + port@1 {
> > + reg = <1>;
> > + };
> > +
> > + /* output */
> > + port@2 {
> > + reg = <2>;
> > + };
> > + };
> > + };
>
> So Pavel (added to Cc:) suggested to merge these into one node for the
> video mux, as really we are describing a single hardware entity that
> happens to be multiplexing multiple video buses into one:
Two drivers will be needed in a way or another to disconnect the dependency
between the video switch driver and the MUX implementation. Are there ways
to do that cleanly other than having two devices?
And if there are two devices, shouldn't the video switch device be a child
of the MUX device? I think it'd be odd to have it hanging around in a
completely unrelated part of the device tree.
>
> syscon {
> compatible = "syscon";
>
> /* video multiplexer */
> mux: mux-controller@3 {
> compatible = "video-mmio-mux";
> reg = <0x3>;
> bit-mask = <0x1>;
> bit-shift = <5>;
> #mux-control-cells = <0>;
>
> mux-controls = <&mux>;
>
> ports {
> /* input 0 */
> port@0 {
> reg = <0>;
> };
>
> /* input 1 */
> port@1 {
> reg = <1>;
> };
>
> /* output */
> port@2 {
> reg = <2>;
> };
> };
> };
> };
>
> That would not touch on this "general purpose" mmio-mux binding itself,
> but would make it necessary to add a separate "video-mmio-mux" and a
> "video-gpio-mux" binding that mirror the "mmio-mux" and "gpio-mux"
> bindings but add the OF-graph connections.
>
> Also I think in this case the self-referencing mux-controls property
> would be superfluous, as the driver binding to this node is expected to
> control the mux according to activation of the links described by the
> OF-graph bindings.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [RESEND v2] mmc: mediatek: Support SDIO feature
From: Yong Mao @ 2017-04-18 10:13 UTC (permalink / raw)
To: Ulf Hansson, Rob Herring
Cc: Linus Walleij, Daniel Kurtz, Chaotian Jing, yong mao, Eddie Huang,
linux-mmc-u79uwXL29TY76Z2rM5mHXA,
srv_heupstream-NuS5LvNUpcJWk0Htik3J/w,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA
Documentation/devicetree/bindings/mmc/mtk-sd.txt | 2 +
arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 77 ++++++++++
drivers/mmc/host/mtk-sd.c | 182 ++++++++++++++++++-----
3 files changed, 222 insertions(+), 39 deletions(-)
--
1.8.1.1.dirty
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v2 1/3] mmc: dt-bindings: update Mediatek MMC bindings
From: Yong Mao @ 2017-04-18 10:13 UTC (permalink / raw)
To: Ulf Hansson, Rob Herring
Cc: Linus Walleij, Daniel Kurtz, Chaotian Jing, yong mao, Eddie Huang,
linux-mmc, srv_heupstream, linux-mediatek, linux-kernel,
linux-arm-kernel, devicetree
In-Reply-To: <1492510391-704-1-git-send-email-yong.mao@mediatek.com>
From: yong mao <yong.mao@mediatek.com>
Add description for mediatek,clk-pad-delay
Signed-off-by: Yong Mao <yong.mao@mediatek.com>
Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
---
Documentation/devicetree/bindings/mmc/mtk-sd.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.txt b/Documentation/devicetree/bindings/mmc/mtk-sd.txt
index 4182ea3..fbb3fd6 100644
--- a/Documentation/devicetree/bindings/mmc/mtk-sd.txt
+++ b/Documentation/devicetree/bindings/mmc/mtk-sd.txt
@@ -30,6 +30,7 @@ Optional properties:
- mediatek,hs400-cmd-resp-sel-rising: HS400 command response sample selection
If present,HS400 command responses are sampled on rising edges.
If not present,HS400 command responses are sampled on falling edges.
+- mediatek,clk-pad-delay: clock pad delay setting
Examples:
mmc0: mmc@11230000 {
@@ -50,4 +51,5 @@ mmc0: mmc@11230000 {
mediatek,hs200-cmd-int-delay = <26>;
mediatek,hs400-cmd-int-delay = <14>;
mediatek,hs400-cmd-resp-sel-rising;
+ mediatek,clk-pad-delay = <5>;
};
--
1.7.9.5
^ permalink raw reply related
* [PATCH v2 2/3] ARM64: dts: mediatek: Enable mmc3 for supporting sdio feature
From: Yong Mao @ 2017-04-18 10:13 UTC (permalink / raw)
To: Ulf Hansson, Rob Herring
Cc: linux-arm-kernel, devicetree, srv_heupstream, Linus Walleij,
linux-mmc, linux-kernel, yong mao, linux-mediatek, Daniel Kurtz,
Eddie Huang, Chaotian Jing
In-Reply-To: <1492510391-704-1-git-send-email-yong.mao@mediatek.com>
From: yong mao <yong.mao@mediatek.com>
Add description of mmc3 for supporting sdio feature
Signed-off-by: Yong Mao <yong.mao@mediatek.com>
Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
---
arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 77 +++++++++++++++++++++++++++
1 file changed, 77 insertions(+)
diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
index 1c3634f..fb8fa5c 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
+++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
@@ -68,6 +68,14 @@
gpio = <&pio 9 GPIO_ACTIVE_HIGH>;
enable-active-high;
};
+
+ sdio_fixed_3v3: regulator@2 {
+ compatible = "regulator-fixed";
+ regulator-name = "3V3";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ gpio = <&pio 85 GPIO_ACTIVE_HIGH>;
+ };
};
&cec {
@@ -156,6 +164,25 @@
vqmmc-supply = <&mt6397_vmc_reg>;
};
+&mmc3 {
+ status = "okay";
+ pinctrl-names = "default", "state_uhs";
+ pinctrl-0 = <&mmc3_pins_default>;
+ pinctrl-1 = <&mmc3_pins_uhs>;
+ bus-width = <4>;
+ max-frequency = <200000000>;
+ cap-sd-highspeed;
+ sd-uhs-sdr50;
+ sd-uhs-sdr104;
+ mediatek,clk-pad-delay = <5>;
+ keep-power-in-suspend;
+ enable-sdio-wakeup;
+ cap-sdio-irq;
+ vmmc-supply = <&sdio_fixed_3v3>;
+ vqmmc-supply = <&mt6397_vgp3_reg>;
+ non-removable;
+};
+
&pio {
disp_pwm0_pins: disp_pwm0_pins {
pins1 {
@@ -261,6 +288,56 @@
};
};
+ mmc3_pins_default: mmc3default {
+ pins_dat {
+ pinmux = <MT8173_PIN_22_MSDC3_DAT0__FUNC_MSDC3_DAT0>,
+ <MT8173_PIN_23_MSDC3_DAT1__FUNC_MSDC3_DAT1>,
+ <MT8173_PIN_24_MSDC3_DAT2__FUNC_MSDC3_DAT2>,
+ <MT8173_PIN_25_MSDC3_DAT3__FUNC_MSDC3_DAT3>;
+ input-enable;
+ drive-strength = <MTK_DRIVE_8mA>;
+ bias-pull-up = <MTK_PUPD_SET_R1R0_10>;
+ };
+
+ pins_cmd {
+ pinmux = <MT8173_PIN_27_MSDC3_CMD__FUNC_MSDC3_CMD>;
+ input-enable;
+ drive-strength = <MTK_DRIVE_8mA>;
+ bias-pull-up = <MTK_PUPD_SET_R1R0_10>;
+ };
+
+ pins_clk {
+ pinmux = <MT8173_PIN_26_MSDC3_CLK__FUNC_MSDC3_CLK>;
+ bias-pull-down;
+ drive-strength = <MTK_DRIVE_8mA>;
+ };
+ };
+
+ mmc3_pins_uhs: mmc3 {
+ pins_dat {
+ pinmux = <MT8173_PIN_22_MSDC3_DAT0__FUNC_MSDC3_DAT0>,
+ <MT8173_PIN_23_MSDC3_DAT1__FUNC_MSDC3_DAT1>,
+ <MT8173_PIN_24_MSDC3_DAT2__FUNC_MSDC3_DAT2>,
+ <MT8173_PIN_25_MSDC3_DAT3__FUNC_MSDC3_DAT3>;
+ input-enable;
+ drive-strength = <MTK_DRIVE_8mA>;
+ bias-pull-up = <MTK_PUPD_SET_R1R0_10>;
+ };
+
+ pins_cmd {
+ pinmux = <MT8173_PIN_27_MSDC3_CMD__FUNC_MSDC3_CMD>;
+ input-enable;
+ drive-strength = <MTK_DRIVE_8mA>;
+ bias-pull-up = <MTK_PUPD_SET_R1R0_10>;
+ };
+
+ pins_clk {
+ pinmux = <MT8173_PIN_26_MSDC3_CLK__FUNC_MSDC3_CLK>;
+ drive-strength = <MTK_DRIVE_8mA>;
+ bias-pull-down = <MTK_PUPD_SET_R1R0_10>;
+ };
+ };
+
usb_id_pins_float: usb_iddig_pull_up {
pins_iddig {
pinmux = <MT8173_PIN_16_IDDIG__FUNC_IDDIG>;
--
1.7.9.5
^ permalink raw reply related
* [PATCH v2 3/3] mmc: sdio: mediatek: Support SDIO feature
From: Yong Mao @ 2017-04-18 10:13 UTC (permalink / raw)
To: Ulf Hansson, Rob Herring
Cc: Linus Walleij, Daniel Kurtz, Chaotian Jing, yong mao, Eddie Huang,
linux-mmc, srv_heupstream, linux-mediatek, linux-kernel,
linux-arm-kernel, devicetree
In-Reply-To: <1492510391-704-1-git-send-email-yong.mao@mediatek.com>
From: yong mao <yong.mao@mediatek.com>
1. Add irqlock to protect accessing the shared register
2. Implement enable_sdio_irq interface
3. Add msdc_recheck_sdio_irq mechanism to make sure all interrupts
can be processed immediately
Signed-off-by: Yong Mao <yong.mao@mediatek.com>
Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
---
drivers/mmc/host/mtk-sd.c | 182 +++++++++++++++++++++++++++++++++++----------
1 file changed, 143 insertions(+), 39 deletions(-)
diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
index 07f3236..fdae197 100644
--- a/drivers/mmc/host/mtk-sd.c
+++ b/drivers/mmc/host/mtk-sd.c
@@ -118,6 +118,7 @@
#define MSDC_PS_CDSTS (0x1 << 1) /* R */
#define MSDC_PS_CDDEBOUNCE (0xf << 12) /* RW */
#define MSDC_PS_DAT (0xff << 16) /* R */
+#define MSDC_PS_DATA1 (0x1 << 17) /* R */
#define MSDC_PS_CMD (0x1 << 24) /* R */
#define MSDC_PS_WP (0x1 << 31) /* R */
@@ -312,6 +313,7 @@ struct msdc_host {
int cmd_rsp;
spinlock_t lock;
+ spinlock_t irqlock; /* irq lock */
struct mmc_request *mrq;
struct mmc_command *cmd;
struct mmc_data *data;
@@ -330,12 +332,14 @@ struct msdc_host {
struct pinctrl_state *pins_uhs;
struct delayed_work req_timeout;
int irq; /* host interrupt */
+ bool irq_thread_alive;
struct clk *src_clk; /* msdc source clock */
struct clk *h_clk; /* msdc h_clk */
u32 mclk; /* mmc subsystem clock frequency */
u32 src_clk_freq; /* source clock frequency */
u32 sclk; /* SD/MS bus clock frequency */
+ bool clock_on;
unsigned char timing;
bool vqmmc_enabled;
u32 hs400_ds_delay;
@@ -343,6 +347,7 @@ struct msdc_host {
u32 hs400_cmd_int_delay; /* cmd internal delay for HS400 */
bool hs400_cmd_resp_sel_rising;
/* cmd response sample selection for HS400 */
+ u32 clk_pad_delay;
bool hs400_mode; /* current eMMC will run at hs400 mode */
struct msdc_save_para save_para; /* used when gate HCLK */
struct msdc_tune_para def_tune_para; /* default tune setting */
@@ -399,6 +404,7 @@ static void msdc_reset_hw(struct msdc_host *host)
static void msdc_cmd_next(struct msdc_host *host,
struct mmc_request *mrq, struct mmc_command *cmd);
+static void msdc_recheck_sdio_irq(struct msdc_host *host);
static const u32 cmd_ints_mask = MSDC_INTEN_CMDRDY | MSDC_INTEN_RSPCRCERR |
MSDC_INTEN_CMDTMO | MSDC_INTEN_ACMDRDY |
@@ -525,6 +531,7 @@ static void msdc_gate_clock(struct msdc_host *host)
{
clk_disable_unprepare(host->src_clk);
clk_disable_unprepare(host->h_clk);
+ host->clock_on = false;
}
static void msdc_ungate_clock(struct msdc_host *host)
@@ -533,6 +540,7 @@ static void msdc_ungate_clock(struct msdc_host *host)
clk_prepare_enable(host->src_clk);
while (!(readl(host->base + MSDC_CFG) & MSDC_CFG_CKSTB))
cpu_relax();
+ host->clock_on = true;
}
static void msdc_set_mclk(struct msdc_host *host, unsigned char timing, u32 hz)
@@ -541,6 +549,7 @@ static void msdc_set_mclk(struct msdc_host *host, unsigned char timing, u32 hz)
u32 flags;
u32 div;
u32 sclk;
+ unsigned long irq_flags;
if (!hz) {
dev_dbg(host->dev, "set mclk to 0\n");
@@ -549,8 +558,11 @@ static void msdc_set_mclk(struct msdc_host *host, unsigned char timing, u32 hz)
return;
}
+ spin_lock_irqsave(&host->irqlock, irq_flags);
flags = readl(host->base + MSDC_INTEN);
sdr_clr_bits(host->base + MSDC_INTEN, flags);
+ spin_unlock_irqrestore(&host->irqlock, irq_flags);
+
sdr_clr_bits(host->base + MSDC_CFG, MSDC_CFG_HS400_CK_MODE);
if (timing == MMC_TIMING_UHS_DDR50 ||
timing == MMC_TIMING_MMC_DDR52 ||
@@ -600,7 +612,10 @@ static void msdc_set_mclk(struct msdc_host *host, unsigned char timing, u32 hz)
host->timing = timing;
/* need because clk changed. */
msdc_set_timeout(host, host->timeout_ns, host->timeout_clks);
+
+ spin_lock_irqsave(&host->irqlock, irq_flags);
sdr_set_bits(host->base + MSDC_INTEN, flags);
+ spin_unlock_irqrestore(&host->irqlock, irq_flags);
/*
* mmc_select_hs400() will drop to 50Mhz and High speed mode,
@@ -708,6 +723,7 @@ static inline u32 msdc_cmd_prepare_raw_cmd(struct msdc_host *host,
static void msdc_start_data(struct msdc_host *host, struct mmc_request *mrq,
struct mmc_command *cmd, struct mmc_data *data)
{
+ unsigned long flags;
bool read;
WARN_ON(host->data);
@@ -716,8 +732,12 @@ static void msdc_start_data(struct msdc_host *host, struct mmc_request *mrq,
mod_delayed_work(system_wq, &host->req_timeout, DAT_TIMEOUT);
msdc_dma_setup(host, &host->dma, data);
+
+ spin_lock_irqsave(&host->irqlock, flags);
sdr_set_bits(host->base + MSDC_INTEN, data_ints_mask);
sdr_set_field(host->base + MSDC_DMA_CTRL, MSDC_DMA_CTRL_START, 1);
+ spin_unlock_irqrestore(&host->irqlock, flags);
+
dev_dbg(host->dev, "DMA start\n");
dev_dbg(host->dev, "%s: cmd=%d DMA data: %d blocks; read=%d\n",
__func__, cmd->opcode, data->blocks, read);
@@ -774,6 +794,8 @@ static void msdc_request_done(struct msdc_host *host, struct mmc_request *mrq)
if (mrq->data)
msdc_unprepare_data(host, mrq);
mmc_request_done(host->mmc, mrq);
+
+ msdc_recheck_sdio_irq(host);
}
/* returns true if command is fully handled; returns false otherwise */
@@ -797,15 +819,17 @@ static bool msdc_cmd_done(struct msdc_host *host, int events,
| MSDC_INT_CMDTMO)))
return done;
- spin_lock_irqsave(&host->lock, flags);
done = !host->cmd;
+ spin_lock_irqsave(&host->lock, flags);
host->cmd = NULL;
spin_unlock_irqrestore(&host->lock, flags);
if (done)
return true;
+ spin_lock_irqsave(&host->irqlock, flags);
sdr_clr_bits(host->base + MSDC_INTEN, cmd_ints_mask);
+ spin_unlock_irqrestore(&host->irqlock, flags);
if (cmd->flags & MMC_RSP_PRESENT) {
if (cmd->flags & MMC_RSP_136) {
@@ -883,6 +907,7 @@ static inline bool msdc_cmd_is_ready(struct msdc_host *host,
static void msdc_start_command(struct msdc_host *host,
struct mmc_request *mrq, struct mmc_command *cmd)
{
+ unsigned long flags;
u32 rawcmd;
WARN_ON(host->cmd);
@@ -901,7 +926,10 @@ static void msdc_start_command(struct msdc_host *host,
rawcmd = msdc_cmd_prepare_raw_cmd(host, mrq, cmd);
mod_delayed_work(system_wq, &host->req_timeout, DAT_TIMEOUT);
+ spin_lock_irqsave(&host->irqlock, flags);
sdr_set_bits(host->base + MSDC_INTEN, cmd_ints_mask);
+ spin_unlock_irqrestore(&host->irqlock, flags);
+
writel(cmd->arg, host->base + SDC_ARG);
writel(rawcmd, host->base + SDC_CMD);
}
@@ -993,8 +1021,8 @@ static bool msdc_data_xfer_done(struct msdc_host *host, u32 events,
| MSDC_INT_DMA_BDCSERR | MSDC_INT_DMA_GPDCSERR
| MSDC_INT_DMA_PROTECT);
- spin_lock_irqsave(&host->lock, flags);
done = !host->data;
+ spin_lock_irqsave(&host->lock, flags);
if (check_data)
host->data = NULL;
spin_unlock_irqrestore(&host->lock, flags);
@@ -1009,7 +1037,11 @@ static bool msdc_data_xfer_done(struct msdc_host *host, u32 events,
1);
while (readl(host->base + MSDC_DMA_CFG) & MSDC_DMA_CFG_STS)
cpu_relax();
+
+ spin_lock_irqsave(&host->irqlock, flags);
sdr_clr_bits(host->base + MSDC_INTEN, data_ints_mask);
+ spin_unlock_irqrestore(&host->irqlock, flags);
+
dev_dbg(host->dev, "DMA stop\n");
if ((events & MSDC_INT_XFER_COMPL) && (!stop || !stop->error)) {
@@ -1123,44 +1155,47 @@ static void msdc_request_timeout(struct work_struct *work)
static irqreturn_t msdc_irq(int irq, void *dev_id)
{
+ unsigned long flags;
struct msdc_host *host = (struct msdc_host *) dev_id;
+ struct mmc_request *mrq;
+ struct mmc_command *cmd;
+ struct mmc_data *data;
+ u32 events, event_mask;
+
+ spin_lock_irqsave(&host->irqlock, flags);
+ events = readl(host->base + MSDC_INT);
+ event_mask = readl(host->base + MSDC_INTEN);
+ /* clear interrupts */
+ writel(events & event_mask, host->base + MSDC_INT);
+
+ mrq = host->mrq;
+ cmd = host->cmd;
+ data = host->data;
+ spin_unlock_irqrestore(&host->irqlock, flags);
+
+ if ((events & event_mask) & MSDC_INT_SDIOIRQ) {
+ mmc_signal_sdio_irq(host->mmc);
+ if (!mrq)
+ return IRQ_HANDLED;
+ }
- while (true) {
- unsigned long flags;
- struct mmc_request *mrq;
- struct mmc_command *cmd;
- struct mmc_data *data;
- u32 events, event_mask;
-
- spin_lock_irqsave(&host->lock, flags);
- events = readl(host->base + MSDC_INT);
- event_mask = readl(host->base + MSDC_INTEN);
- /* clear interrupts */
- writel(events & event_mask, host->base + MSDC_INT);
-
- mrq = host->mrq;
- cmd = host->cmd;
- data = host->data;
- spin_unlock_irqrestore(&host->lock, flags);
-
- if (!(events & event_mask))
- break;
+ if (!(events & (event_mask & ~MSDC_INT_SDIOIRQ)))
+ return IRQ_HANDLED;
- if (!mrq) {
- dev_err(host->dev,
- "%s: MRQ=NULL; events=%08X; event_mask=%08X\n",
- __func__, events, event_mask);
- WARN_ON(1);
- break;
- }
+ if (!mrq) {
+ dev_err(host->dev,
+ "%s: MRQ=NULL; events=%08X; event_mask=%08X\n",
+ __func__, events, event_mask);
+ WARN_ON(1);
+ return IRQ_HANDLED;
+ }
- dev_dbg(host->dev, "%s: events=%08X\n", __func__, events);
+ dev_dbg(host->dev, "%s: events=%08X\n", __func__, events);
- if (cmd)
- msdc_cmd_done(host, events, mrq, cmd);
- else if (data)
- msdc_data_xfer_done(host, events, mrq, data);
- }
+ if (cmd)
+ msdc_cmd_done(host, events, mrq, cmd);
+ else if (data)
+ msdc_data_xfer_done(host, events, mrq, data);
return IRQ_HANDLED;
}
@@ -1168,6 +1203,7 @@ static irqreturn_t msdc_irq(int irq, void *dev_id)
static void msdc_init_hw(struct msdc_host *host)
{
u32 val;
+ unsigned long flags;
/* Configure to MMC/SD mode, clock free running */
sdr_set_bits(host->base + MSDC_CFG, MSDC_CFG_MODE | MSDC_CFG_CKPDN);
@@ -1179,11 +1215,14 @@ static void msdc_init_hw(struct msdc_host *host)
sdr_clr_bits(host->base + MSDC_PS, MSDC_PS_CDEN);
/* Disable and clear all interrupts */
+ spin_lock_irqsave(&host->irqlock, flags);
writel(0, host->base + MSDC_INTEN);
val = readl(host->base + MSDC_INT);
writel(val, host->base + MSDC_INT);
+ spin_unlock_irqrestore(&host->irqlock, flags);
- writel(0, host->base + MSDC_PAD_TUNE);
+ sdr_set_field(host->base + MSDC_PAD_TUNE,
+ MSDC_PAD_TUNE_CLKTDLY, host->clk_pad_delay);
writel(0, host->base + MSDC_IOCON);
sdr_set_field(host->base + MSDC_IOCON, MSDC_IOCON_DDLSEL, 0);
writel(0x403c0046, host->base + MSDC_PATCH_BIT);
@@ -1196,9 +1235,11 @@ static void msdc_init_hw(struct msdc_host *host)
*/
sdr_set_bits(host->base + SDC_CFG, SDC_CFG_SDIO);
- /* disable detect SDIO device interrupt function */
- sdr_clr_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
-
+ if (host->mmc->caps & MMC_CAP_SDIO_IRQ)
+ sdr_set_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
+ else
+ /* disable detect SDIO device interrupt function */
+ sdr_clr_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
/* Configure to default data timeout */
sdr_set_field(host->base + SDC_CFG, SDC_CFG_DTOC, 3);
@@ -1210,11 +1251,15 @@ static void msdc_init_hw(struct msdc_host *host)
static void msdc_deinit_hw(struct msdc_host *host)
{
u32 val;
+ unsigned long flags;
+
/* Disable and clear all interrupts */
+ spin_lock_irqsave(&host->irqlock, flags);
writel(0, host->base + MSDC_INTEN);
val = readl(host->base + MSDC_INT);
writel(val, host->base + MSDC_INT);
+ spin_unlock_irqrestore(&host->irqlock, flags);
}
/* init gpd and bd list in msdc_drv_probe */
@@ -1582,6 +1627,48 @@ static void msdc_hw_reset(struct mmc_host *mmc)
sdr_clr_bits(host->base + EMMC_IOCON, 1);
}
+/**
+ * msdc_recheck_sdio_irq - recheck whether the SDIO IRQ is lost
+ * @host: The host to check.
+ *
+ * Host controller may lost interrupt in some special case.
+ * Add sdio IRQ recheck mechanism to make sure all interrupts
+ * can be processed immediately
+ */
+static void msdc_recheck_sdio_irq(struct msdc_host *host)
+{
+ u32 reg_int, reg_ps;
+
+ if (host->clock_on && (host->mmc->caps & MMC_CAP_SDIO_IRQ) &&
+ host->irq_thread_alive) {
+ reg_int = readl(host->base + MSDC_INT);
+ reg_ps = readl(host->base + MSDC_PS);
+ if (!((reg_int & MSDC_INT_SDIOIRQ) ||
+ (reg_ps & MSDC_PS_DATA1)))
+ mmc_signal_sdio_irq(host->mmc);
+ }
+}
+
+static void msdc_enable_sdio_irq(struct mmc_host *mmc, int enable)
+{
+ unsigned long flags;
+ struct msdc_host *host = mmc_priv(mmc);
+
+ host->irq_thread_alive = true;
+ if (enable) {
+ msdc_recheck_sdio_irq(host);
+
+ spin_lock_irqsave(&host->irqlock, flags);
+ sdr_set_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
+ sdr_set_bits(host->base + MSDC_INTEN, MSDC_INTEN_SDIOIRQ);
+ spin_unlock_irqrestore(&host->irqlock, flags);
+ } else {
+ spin_lock_irqsave(&host->irqlock, flags);
+ sdr_clr_bits(host->base + MSDC_INTEN, MSDC_INTEN_SDIOIRQ);
+ spin_unlock_irqrestore(&host->irqlock, flags);
+ }
+}
+
static struct mmc_host_ops mt_msdc_ops = {
.post_req = msdc_post_req,
.pre_req = msdc_pre_req,
@@ -1593,6 +1680,7 @@ static void msdc_hw_reset(struct mmc_host *mmc)
.execute_tuning = msdc_execute_tuning,
.prepare_hs400_tuning = msdc_prepare_hs400_tuning,
.hw_reset = msdc_hw_reset,
+ .enable_sdio_irq = msdc_enable_sdio_irq,
};
static void msdc_of_property_parse(struct platform_device *pdev,
@@ -1612,6 +1700,9 @@ static void msdc_of_property_parse(struct platform_device *pdev,
host->hs400_cmd_resp_sel_rising = true;
else
host->hs400_cmd_resp_sel_rising = false;
+
+ of_property_read_u32(pdev->dev.of_node, "mediatek,clk-pad-delay",
+ &host->clk_pad_delay);
}
static int msdc_drv_probe(struct platform_device *pdev)
@@ -1705,6 +1796,7 @@ static int msdc_drv_probe(struct platform_device *pdev)
mmc_dev(mmc)->dma_mask = &host->dma_mask;
host->timeout_clks = 3 * 1048576;
+ host->irq_thread_alive = false;
host->dma.gpd = dma_alloc_coherent(&pdev->dev,
2 * sizeof(struct mt_gpdma_desc),
&host->dma.gpd_addr, GFP_KERNEL);
@@ -1718,6 +1810,7 @@ static int msdc_drv_probe(struct platform_device *pdev)
msdc_init_gpd_bd(host, &host->dma);
INIT_DELAYED_WORK(&host->req_timeout, msdc_request_timeout);
spin_lock_init(&host->lock);
+ spin_lock_init(&host->irqlock);
platform_set_drvdata(pdev, mmc);
msdc_ungate_clock(host);
@@ -1732,6 +1825,10 @@ static int msdc_drv_probe(struct platform_device *pdev)
pm_runtime_set_autosuspend_delay(host->dev, MTK_MMC_AUTOSUSPEND_DELAY);
pm_runtime_use_autosuspend(host->dev);
pm_runtime_enable(host->dev);
+
+ /* In SDIO irq mode, DATA1 slways need to be detected */
+ if (host->mmc->caps & MMC_CAP_SDIO_IRQ)
+ pm_runtime_get_sync(host->dev);
ret = mmc_add_host(mmc);
if (ret)
@@ -1821,6 +1918,10 @@ static int msdc_runtime_suspend(struct device *dev)
msdc_save_reg(host);
msdc_gate_clock(host);
+ if (host->mmc->caps & MMC_CAP_SDIO_IRQ) {
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+ }
return 0;
}
@@ -1829,6 +1930,9 @@ static int msdc_runtime_resume(struct device *dev)
struct mmc_host *mmc = dev_get_drvdata(dev);
struct msdc_host *host = mmc_priv(mmc);
+ /* In SDIO irq mode, DATA1 slways need to be detected */
+ if (host->mmc->caps & MMC_CAP_SDIO_IRQ)
+ pm_runtime_get_sync(host->dev);
msdc_ungate_clock(host);
msdc_restore_reg(host);
return 0;
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH v2 1/3] ARM: dts: rockchip: Add support for phyCORE-RK3288 SoM
From: Jacob Chen @ 2017-04-18 10:15 UTC (permalink / raw)
To: Wadim Egorov
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
Heiko Stuebner, linux-I+IVW8TIWO2tmTQ+vhA3Yw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1491483866-18368-1-git-send-email-w.egorov-guT5V/WYfQezQB+pC5nmwQ@public.gmane.org>
Hi wadim,
2017-04-06 21:04 GMT+08:00 Wadim Egorov <w.egorov-guT5V/WYfQezQB+pC5nmwQ@public.gmane.org>:
> The phyCORE-RK3288 is a SoM (System on Module) containing a RK3288 SoC.
> The module can be connected to different carrier boards.
> It can be also equipped with different RAM, SPI flash and eMMC variants.
> The Rapid Development Kit option is using the following setup:
>
> - 1 GB DDR3 RAM (2 Banks)
> - 1x 4 KB EEPROM
> - DP83867 Gigabit Ethernet PHY
> - 16 MB SPI Flash
> - 4 GB eMMC Flash
>
> Signed-off-by: Wadim Egorov <w.egorov-guT5V/WYfQezQB+pC5nmwQ@public.gmane.org>
> ---
> Changes in v2:
> - Added a dual license which is used for all rk3288 based boards.
>
> Include minor changes from Heiko Stübner:
> - moved phy-handle property up a bit
> - switches compatible and #address+#size-cells in mdio0
> - dropped rockchip,grf from &io_domains (grf is a simple-mfd and can
> get the grf syscon on its own via its parent)
> - vdd_cpu: regulator@60 (from fan53555@60)
> - serial_flash: flash@0 (from m25p80@0)
> Nodes should be named after their "category" not the actual device
>
> ---
> arch/arm/boot/dts/rk3288-phycore-som.dtsi | 497 ++++++++++++++++++++++++++++++
> 1 file changed, 497 insertions(+)
> create mode 100644 arch/arm/boot/dts/rk3288-phycore-som.dtsi
>
> diff --git a/arch/arm/boot/dts/rk3288-phycore-som.dtsi b/arch/arm/boot/dts/rk3288-phycore-som.dtsi
> new file mode 100644
> index 0000000..26cd3ad
> --- /dev/null
> +++ b/arch/arm/boot/dts/rk3288-phycore-som.dtsi
> @@ -0,0 +1,497 @@
> +/*
> + * Device tree file for Phytec phyCORE-RK3288 SoM
> + * Copyright (C) 2017 PHYTEC Messtechnik GmbH
> + * Author: Wadim Egorov <w.egorov-guT5V/WYfQezQB+pC5nmwQ@public.gmane.org>
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + * a) This file is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + * This file is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * Or, alternatively,
> + *
> + * b) Permission is hereby granted, free of charge, to any person
> + * obtaining a copy of this software and associated documentation
> + * files (the "Software"), to deal in the Software without
> + * restriction, including without limitation the rights to use,
> + * copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following
> + * conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + * included in all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
> + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <dt-bindings/net/ti-dp83867.h>
> +#include "rk3288.dtsi"
> +
> +/ {
> + model = "Phytec RK3288 phyCORE";
> + compatible = "phytec,rk3288-phycore-som", "rockchip,rk3288";
> +
> + /*
> + * Set the minimum memory size here and
> + * let the bootloader set the real size.
> + */
> + memory {
> + device_type = "memory";
> + reg = <0 0x8000000>;
> + };
> +
> + aliases {
> + rtc0 = &i2c_rtc;
> + rtc1 = &rk818;
> + };
> +
> + ext_gmac: external-gmac-clock {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + clock-frequency = <125000000>;
> + clock-output-names = "ext_gmac";
> + };
> +
> + leds: user-leds {
> + compatible = "gpio-leds";
> + pinctrl-names = "default";
> + pinctrl-0 = <&user_led>;
> +
> + user {
> + label = "green_led";
> + gpios = <&gpio7 2 GPIO_ACTIVE_HIGH>;
> + linux,default-trigger = "heartbeat";
> + default-state = "keep";
> + };
> + };
> +
> + vdd_emmc_io: vdd-emmc-io {
> + compatible = "regulator-fixed";
> + regulator-name = "vdd_emmc_io";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + vin-supply = <&vdd_3v3_io>;
> + };
> +
> + vdd_in_otg_out: vdd-in-otg-out {
> + compatible = "regulator-fixed";
> + regulator-name = "vdd_in_otg_out";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + };
> +
> + vdd_misc_1v8: vdd-misc-1v8 {
> + compatible = "regulator-fixed";
> + regulator-name = "vdd_misc_1v8";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + };
> +};
> +
> +&cpu0 {
> + cpu0-supply = <&vdd_cpu>;
> + operating-points = <
> + /* KHz uV */
> + 1800000 1400000
> + 1608000 1350000
> + 1512000 1300000
> + 1416000 1200000
> + 1200000 1100000
> + 1008000 1050000
> + 816000 1000000
> + 696000 950000
> + 600000 900000
> + 408000 900000
> + 312000 900000
> + 216000 900000
> + 126000 900000
> + >;
> +};
> +
> +&emmc {
> + status = "okay";
> + bus-width = <8>;
> + cap-mmc-highspeed;
> + disable-wp;
> + non-removable;
> + num-slots = <1>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&emmc_clk &emmc_cmd &emmc_pwr &emmc_bus8>;
> + vmmc-supply = <&vdd_3v3_io>;
> + vqmmc-supply = <&vdd_emmc_io>;
> +};
> +
> +&gmac {
> + assigned-clocks = <&cru SCLK_MAC>;
> + assigned-clock-parents = <&ext_gmac>;
> + clock_in_out = "input";
> + pinctrl-names = "default";
> + pinctrl-0 = <&rgmii_pins &phy_rst &phy_int>;
> + phy-handle = <&phy0>;
> + phy-supply = <&vdd_eth_2v5>;
> + phy-mode = "rgmii-id";
> + snps,reset-active-low;
> + snps,reset-delays-us = <0 10000 1000000>;
> + snps,reset-gpio = <&gpio4 8 GPIO_ACTIVE_HIGH>;
> + tx_delay = <0x0>;
> + rx_delay = <0x0>;
> +
> + mdio0 {
> + compatible = "snps,dwmac-mdio";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + phy0: ethernet-phy@0 {
> + compatible = "ethernet-phy-ieee802.3-c22";
> + reg = <0>;
> + interrupt-parent = <&gpio4>;
> + interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
> + ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
> + ti,tx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
> + ti,fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
> + enet-phy-lane-no-swap;
> + };
> + };
> +};
> +
> +&hdmi {
> + ddc-i2c-bus = <&i2c5>;
> +};
> +
> +&io_domains {
> + status = "okay";
> + sdcard-supply = <&vdd_io_sd>;
> + flash0-supply = <&vdd_emmc_io>;
> + flash1-supply = <&vdd_misc_1v8>;
> + gpio1830-supply = <&vdd_3v3_io>;
> + gpio30-supply = <&vdd_3v3_io>;
> + bb-supply = <&vdd_3v3_io>;
> + dvp-supply = <&vdd_3v3_io>;
> + lcdc-supply = <&vdd_3v3_io>;
> + wifi-supply = <&vdd_3v3_io>;
> + audio-supply = <&vdd_3v3_io>;
> +};
> +
> +&i2c0 {
> + status = "okay";
> + clock-frequency = <400000>;
> +
> + rk818: pmic@1c {
> + compatible = "rockchip,rk818";
> + reg = <0x1c>;
> + interrupt-parent = <&gpio0>;
> + interrupts = <4 IRQ_TYPE_LEVEL_LOW>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pmic_int>;
> + rockchip,system-power-controller;
> + wakeup-source;
> + #clock-cells = <1>;
> +
I think you miss "clock-output-names = "xin32k" here.
> + vcc1-supply = <&vdd_sys>;
> + vcc2-supply = <&vdd_sys>;
> + vcc3-supply = <&vdd_sys>;
> + vcc4-supply = <&vdd_sys>;
> + boost-supply = <&vdd_in_otg_out>;
> + vcc6-supply = <&vdd_sys>;
> + vcc7-supply = <&vdd_misc_1v8>;
> + vcc8-supply = <&vdd_misc_1v8>;
> + vcc9-supply = <&vdd_3v3_io>;
> + vddio-supply = <&vdd_3v3_io>;
> +
> + regulators {
> + vdd_log: DCDC_REG1 {
> + regulator-name = "vdd_log";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <1100000>;
> + regulator-max-microvolt = <1100000>;
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + };
> + };
> +
> + vdd_gpu: DCDC_REG2 {
> + regulator-name = "vdd_gpu";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <800000>;
> + regulator-max-microvolt = <1250000>;
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <1000000>;
> + };
> + };
> +
> + vcc_ddr: DCDC_REG3 {
> + regulator-name = "vcc_ddr";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + };
> + };
> +
> + vdd_3v3_io: DCDC_REG4 {
> + regulator-name = "vdd_3v3_io";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <3300000>;
> + };
> + };
> +
> + vdd_sys: DCDC_BOOST {
> + regulator-name = "vdd_sys";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <5000000>;
> + };
> + };
> +
> + /* vcc9 */
> + vdd_sd: SWITCH_REG {
> + regulator-name = "vdd_sd";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + };
> + };
> +
> + /* vcc6 */
> + vdd_eth_2v5: LDO_REG2 {
> + regulator-name = "vdd_eth_2v5";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <2500000>;
> + regulator-max-microvolt = <2500000>;
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <2500000>;
> + };
> + };
> +
> + /* vcc7 */
> + vdd_1v0: LDO_REG3 {
> + regulator-name = "vdd_1v0";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <1000000>;
> + regulator-max-microvolt = <1000000>;
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <1000000>;
> + };
> + };
> +
> + /* vcc8 */
> + vdd_1v8_lcd_ldo: LDO_REG4 {
> + regulator-name = "vdd_1v8_lcd_ldo";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <1800000>;
> + };
> + };
> +
> + /* vcc8 */
> + vdd_1v0_lcd: LDO_REG6 {
> + regulator-name = "vdd_1v0_lcd";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <1000000>;
> + regulator-max-microvolt = <1000000>;
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <1000000>;
> + };
> + };
> +
> + /* vcc7 */
> + vdd_1v8_ldo: LDO_REG7 {
> + regulator-name = "vdd_1v8_ldo";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + regulator-suspend-microvolt = <1800000>;
> + };
> + };
> +
> + /* vcc9 */
> + vdd_io_sd: LDO_REG9 {
> + regulator-name = "vdd_io_sd";
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <3300000>;
> + };
> + };
> + };
> + };
> +
> + /* M24C32-D */
> + i2c_eeprom: eeprom@50 {
> + compatible = "atmel,24c32";
> + reg = <0x50>;
> + pagesize = <32>;
> + };
> +
> + vdd_cpu: regulator@60 {
> + compatible = "fcs,fan53555";
> + reg = <0x60>;
> + fcs,suspend-voltage-selector = <1>;
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-enable-ramp-delay = <300>;
> + regulator-name = "vdd_cpu";
> + regulator-min-microvolt = <800000>;
> + regulator-max-microvolt = <1430000>;
> + regulator-ramp-delay = <8000>;
> + vin-supply = <&vdd_sys>;
> + };
> +};
> +
> +&pinctrl {
> + pcfg_output_high: pcfg-output-high {
> + output-high;
> + };
> +
> + emmc {
> + /*
> + * We run eMMC at max speed; bump up drive strength.
> + * We also have external pulls, so disable the internal ones.
> + */
> + emmc_clk: emmc-clk {
> + rockchip,pins = <3 18 RK_FUNC_2 &pcfg_pull_none_12ma>;
> + };
> +
> + emmc_cmd: emmc-cmd {
> + rockchip,pins = <3 16 RK_FUNC_2 &pcfg_pull_none_12ma>;
> + };
> +
> + emmc_bus8: emmc-bus8 {
> + rockchip,pins = <3 0 RK_FUNC_2 &pcfg_pull_none_12ma>,
> + <3 1 RK_FUNC_2 &pcfg_pull_none_12ma>,
> + <3 2 RK_FUNC_2 &pcfg_pull_none_12ma>,
> + <3 3 RK_FUNC_2 &pcfg_pull_none_12ma>,
> + <3 4 RK_FUNC_2 &pcfg_pull_none_12ma>,
> + <3 5 RK_FUNC_2 &pcfg_pull_none_12ma>,
> + <3 6 RK_FUNC_2 &pcfg_pull_none_12ma>,
> + <3 7 RK_FUNC_2 &pcfg_pull_none_12ma>;
> + };
> + };
> +
> + gmac {
> + phy_int: phy-int {
> + rockchip,pins = <4 2 RK_FUNC_GPIO &pcfg_pull_up>;
> + };
> +
> + phy_rst: phy-rst {
> + rockchip,pins = <4 8 RK_FUNC_GPIO &pcfg_output_high>;
> + };
> + };
> +
> + leds {
> + user_led: user-led {
> + rockchip,pins = <7 2 RK_FUNC_GPIO &pcfg_output_high>;
> + };
> + };
> +
> + pmic {
> + pmic_int: pmic-int {
> + rockchip,pins = <RK_GPIO0 4 RK_FUNC_GPIO &pcfg_pull_up>;
> + };
> +
> + /* Pin for switching state between sleep and non-sleep state */
> + pmic_sleep: pmic-sleep {
> + rockchip,pins = <RK_GPIO0 0 RK_FUNC_GPIO &pcfg_pull_up>;
> + };
> + };
> +};
> +
> +&pwm1 {
> + status = "okay";
> +};
> +
> +&saradc {
> + status = "okay";
> + vref-supply = <&vdd_1v8_ldo>;
> +};
> +
> +&spi2 {
> + status = "okay";
> +
> + serial_flash: flash@0 {
> + compatible = "micron,n25q128a13", "jedec,spi-nor";
> + reg = <0x0>;
> + spi-max-frequency = <50000000>;
> + m25p,fast-read;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + status = "okay";
> + };
> +};
> +
> +&tsadc {
> + status = "okay";
> + rockchip,hw-tshut-mode = <0>;
> + rockchip,hw-tshut-polarity = <0>;
> +};
> +
> +&vopb {
> + status = "okay";
> +};
> +
> +&vopb_mmu {
> + status = "okay";
> +};
> +
> +&vopl {
> + status = "okay";
> +};
> +
> +&vopl_mmu {
> + status = "okay";
> +};
> +
> +&wdt {
> + status = "okay";
> +};
> --
> 1.9.1
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings
From: Pavel Machek @ 2017-04-18 10:34 UTC (permalink / raw)
To: Sakari Ailus
Cc: Philipp Zabel, Peter Rosin, Rob Herring, Mark Rutland,
Steve Longerbeam, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ
In-Reply-To: <20170418100840.GF7456-S+BSfZ9RZZmRSg0ZkenSGLdO1Tsj/99ntUK59QYPAWc@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 3369 bytes --]
On Tue 2017-04-18 13:08:41, Sakari Ailus wrote:
> Hi Philipp,
>
> On Tue, Apr 18, 2017 at 10:19:04AM +0200, Philipp Zabel wrote:
> > On Thu, 2017-04-13 at 17:48 +0200, Philipp Zabel wrote:
> > > This adds device tree binding documentation for mmio-based syscon
> > > multiplexers controlled by a single bitfield in a syscon register
> > > range.
> > >
> > > Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > ---
> > > Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> > > 1 file changed, 56 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > new file mode 100644
> > > index 0000000000000..11d96f5d98583
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > @@ -0,0 +1,56 @@
> > > +MMIO bitfield-based multiplexer controller bindings
> > > +
> > > +Define a syscon bitfield to be used to control a multiplexer. The parent
> > > +device tree node must be a syscon node to provide register access.
> > > +
> > > +Required properties:
> > > +- compatible : "gpio-mux"
> > > +- reg : register base of the register containing the control bitfield
> > > +- bit-mask : bitmask of the control bitfield in the control register
> > > +- bit-shift : bit offset of the control bitfield in the control register
> > > +- #mux-control-cells : <0>
> > > +* Standard mux-controller bindings as decribed in mux-controller.txt
> > > +
> > > +Optional properties:
> > > +- idle-state : if present, the state the mux will have when idle. The
> > > + special state MUX_IDLE_AS_IS is the default.
> > > +
> > > +The multiplexer state is defined as the value of the bitfield described
> > > +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> > > +syscon.
> > > +
> > > +Example:
> > > +
> > > + syscon {
> > > + compatible = "syscon";
> > > +
> > > + mux: mux-controller@3 {
> > > + compatible = "mmio-mux";
> > > + reg = <0x3>;
> > > + bit-mask = <0x1>;
> > > + bit-shift = <5>;
> > > + #mux-control-cells = <0>;
> > > + };
> > > + };
> > > +
> > > + video-mux {
> > > + compatible = "video-mux";
> > > + mux-controls = <&mux>;
> > > +
> > > + ports {
> > > + /* input 0 */
> > > + port@0 {
> > > + reg = <0>;
> > > + };
> > > +
> > > + /* input 1 */
> > > + port@1 {
> > > + reg = <1>;
> > > + };
> > > +
> > > + /* output */
> > > + port@2 {
> > > + reg = <2>;
> > > + };
> > > + };
> > > + };
> >
> > So Pavel (added to Cc:) suggested to merge these into one node for the
> > video mux, as really we are describing a single hardware entity that
> > happens to be multiplexing multiple video buses into one:
>
> Two drivers will be needed in a way or another to disconnect the dependency
> between the video switch driver and the MUX implementation. Are there ways
> to do that cleanly other than having two devices?
Yes.
Device tree describes hardware, not the driver structure.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 03/12] dt-bindings: make AXP20X compatible strings one per line
From: Chen-Yu Tsai @ 2017-04-18 10:35 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Lee Jones, Rob Herring, Chen-Yu Tsai, Maxime Ripard,
Liam Girdwood, Mark Brown, devicetree, linux-kernel,
linux-arm-kernel, linux-sunxi
In-Reply-To: <20170417115747.7300-4-icenowy-h8G6r0blFSE@public.gmane.org>
On Mon, Apr 17, 2017 at 7:57 PM, Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org> wrote:
> In the binding documentation of AXP20X mfd, the compatible strings used
> to be listed for three per line, which leads to some mess when trying to
> add AXP803 compatible string (as we have already AXP806 and AXP809
> compatibles, which is after AXP803 in ascending order).
>
> Make the compatible strings one per line, so that inserting a new
> compatible string will be directly a new line.
>
> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
^ permalink raw reply
* Re: [PATCH v3 07/12] dt-bindings: add AXP803's regulator info
From: Chen-Yu Tsai @ 2017-04-18 10:36 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Lee Jones, Rob Herring, Chen-Yu Tsai, Maxime Ripard,
Liam Girdwood, Mark Brown, devicetree, linux-kernel,
linux-arm-kernel, linux-sunxi
In-Reply-To: <20170417115747.7300-8-icenowy-h8G6r0blFSE@public.gmane.org>
On Mon, Apr 17, 2017 at 7:57 PM, Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org> wrote:
> AXP803 have the most regulators in currently supported AXP PMICs.
>
> Add info for the regulators in the dt-bindings document.
>
> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
^ permalink raw reply
* Re: [linux-sunxi] [PATCH v3 09/12] mfd: axp20x: add axp20x-regulator cell for AXP803
From: Chen-Yu Tsai @ 2017-04-18 10:38 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Lee Jones, Rob Herring, Chen-Yu Tsai, Maxime Ripard,
Liam Girdwood, Mark Brown, devicetree, linux-kernel,
linux-arm-kernel, linux-sunxi
In-Reply-To: <20170417115747.7300-10-icenowy-h8G6r0blFSE@public.gmane.org>
On Mon, Apr 17, 2017 at 7:57 PM, Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org> wrote:
> As axp20x-regulator now supports AXP803, add a cell for it.
>
> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> ---
> Changes in v3:
> - Make the new cell one-liner.
>
> drivers/mfd/axp20x.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 1dc6235778eb..431b7f118606 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -848,7 +848,8 @@ static struct mfd_cell axp803_cells[] = {
> .name = "axp20x-pek",
> .num_resources = ARRAY_SIZE(axp803_pek_resources),
> .resources = axp803_pek_resources,
> - }
> + },
> + { .name = "axp20x-regulator" }
It's best to have a trailing comma, so we don't have to change the line
again when we add more cells, like you just did with the previous line.
Otherwise,
Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 06/11] drm/sun4i: add support for Allwinner DE2 mixers
From: Icenowy Zheng @ 2017-04-18 10:47 UTC (permalink / raw)
To: Maxime Ripard
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, Jernej Skrabec, David Airlie,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Chen-Yu Tsai,
Rob Herring, linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20170418090047.7i2k6dtoqxfdqwwy@lukather>
于 2017年4月18日 GMT+08:00 下午5:00:47, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 写到:
>On Sun, Apr 16, 2017 at 08:08:44PM +0800, Icenowy Zheng wrote:
>> Allwinner have a new "Display Engine 2.0" in their new SoCs, which
>comes
>> with mixers to do graphic processing and feed data to TCON, like the
>old
>> backends and frontends.
>>
>> Add support for the mixer on Allwinner V3s SoC; it's the simplest
>one.
>>
>> Currently a lot of functions are still missing -- more investigations
>> are needed to gain enough information for them.
>>
>> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
>> ---
>> Changes in v4:
>> - Killed some dead code according to Jernej.
>>
>> drivers/gpu/drm/sun4i/Kconfig | 10 +
>> drivers/gpu/drm/sun4i/Makefile | 4 +
>> drivers/gpu/drm/sun4i/sun8i_layer.c | 142 ++++++++++++++
>> drivers/gpu/drm/sun4i/sun8i_layer.h | 36 ++++
>> drivers/gpu/drm/sun4i/sun8i_mixer.c | 381
>++++++++++++++++++++++++++++++++++++
>> drivers/gpu/drm/sun4i/sun8i_mixer.h | 131 +++++++++++++
>> 6 files changed, 704 insertions(+)
>> create mode 100644 drivers/gpu/drm/sun4i/sun8i_layer.c
>> create mode 100644 drivers/gpu/drm/sun4i/sun8i_layer.h
>> create mode 100644 drivers/gpu/drm/sun4i/sun8i_mixer.c
>> create mode 100644 drivers/gpu/drm/sun4i/sun8i_mixer.h
>>
>> diff --git a/drivers/gpu/drm/sun4i/Kconfig
>b/drivers/gpu/drm/sun4i/Kconfig
>> index 5a8227f37cc4..15557484520d 100644
>> --- a/drivers/gpu/drm/sun4i/Kconfig
>> +++ b/drivers/gpu/drm/sun4i/Kconfig
>> @@ -22,3 +22,13 @@ config DRM_SUN4I_BACKEND
>> original Allwinner Display Engine, which has a backend to
>> do some alpha blending and feed graphics to TCON. If M is
>> selected the module will be called sun4i-backend.
>> +
>> +config DRM_SUN4I_SUN8I_MIXER
>> + tristate "Support for Allwinner Display Engine 2.0 Mixer"
>> + depends on DRM_SUN4I
>> + default MACH_SUN8I
>> + help
>> + Choose this option if you have an Allwinner SoC with the
>> + Allwinner Display Engine 2.0, which has a mixer to do some
>> + graphics mixture and feed graphics to TCON, If M is
>> + selected the module will be called sun8i-mixer.
>> diff --git a/drivers/gpu/drm/sun4i/Makefile
>b/drivers/gpu/drm/sun4i/Makefile
>> index 1db1068b9be1..7625c2dad1bb 100644
>> --- a/drivers/gpu/drm/sun4i/Makefile
>> +++ b/drivers/gpu/drm/sun4i/Makefile
>> @@ -9,7 +9,11 @@ sun4i-tcon-y += sun4i_crtc.o
>> sun4i-backend-y += sun4i_layer.o
>> sun4i-backend-y += sun4i_backend.o
>>
>> +sun8i-mixer-y += sun8i_layer.o
>> +sun8i-mixer-y += sun8i_mixer.o
>> +
>> obj-$(CONFIG_DRM_SUN4I) += sun4i-drm.o sun4i-tcon.o
>> obj-$(CONFIG_DRM_SUN4I_BACKEND) += sun4i-backend.o
>> +obj-$(CONFIG_DRM_SUN4I_SUN8I_MIXER) += sun8i-mixer.o
>
>Please align the value using tabs.
Should I re-align existed items?
>
>> obj-$(CONFIG_DRM_SUN4I) += sun6i_drc.o
>> obj-$(CONFIG_DRM_SUN4I) += sun4i_tv.o
>> diff --git a/drivers/gpu/drm/sun4i/sun8i_layer.c
>b/drivers/gpu/drm/sun4i/sun8i_layer.c
>> new file mode 100644
>> index 000000000000..d70a90d963b0
>> --- /dev/null
>> +++ b/drivers/gpu/drm/sun4i/sun8i_layer.c
>> @@ -0,0 +1,142 @@
>> +/*
>> + * Copyright (C) Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
>> + *
>> + * Based on sun4i_layer.h, which is:
>> + * Copyright (C) 2015 Free Electrons
>> + * Copyright (C) 2015 NextThing Co
>> + *
>> + * Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + */
>> +
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_crtc.h>
>> +#include <drm/drm_plane_helper.h>
>> +#include <drm/drmP.h>
>> +
>> +#include "sun4i_crtc.h"
>> +#include "sun8i_layer.h"
>> +#include "sun8i_mixer.h"
>> +
>> +struct sun8i_plane_desc {
>> + enum drm_plane_type type;
>> + const uint32_t *formats;
>> + uint32_t nformats;
>> +};
>> +
>> +static int sun8i_mixer_layer_atomic_check(struct drm_plane *plane,
>> + struct drm_plane_state *state)
>> +{
>> + return 0;
>> +}
>> +
>> +static void sun8i_mixer_layer_atomic_disable(struct drm_plane
>*plane,
>> + struct drm_plane_state *old_state)
>> +{
>> + struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
>> + struct sun8i_mixer *mixer = layer->mixer;
>> +
>> + sun8i_mixer_layer_enable(mixer, layer->id, false);
>> +}
>> +
>> +static void sun8i_mixer_layer_atomic_update(struct drm_plane *plane,
>> + struct drm_plane_state *old_state)
>> +{
>> + struct sun8i_layer *layer = plane_to_sun8i_layer(plane);
>> + struct sun8i_mixer *mixer = layer->mixer;
>> +
>> + sun8i_mixer_update_layer_coord(mixer, layer->id, plane);
>> + sun8i_mixer_update_layer_formats(mixer, layer->id, plane);
>> + sun8i_mixer_update_layer_buffer(mixer, layer->id, plane);
>> + sun8i_mixer_layer_enable(mixer, layer->id, true);
>> +}
>> +
>> +static struct drm_plane_helper_funcs sun8i_mixer_layer_helper_funcs
>= {
>> + .atomic_check = sun8i_mixer_layer_atomic_check,
>> + .atomic_disable = sun8i_mixer_layer_atomic_disable,
>> + .atomic_update = sun8i_mixer_layer_atomic_update,
>> +};
>> +
>> +static const struct drm_plane_funcs sun8i_mixer_layer_funcs = {
>> + .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
>> + .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>> + .destroy = drm_plane_cleanup,
>> + .disable_plane = drm_atomic_helper_disable_plane,
>> + .reset = drm_atomic_helper_plane_reset,
>> + .update_plane = drm_atomic_helper_update_plane,
>> +};
>> +
>> +static const uint32_t sun8i_mixer_layer_formats[] = {
>> + DRM_FORMAT_RGB888,
>> + DRM_FORMAT_XRGB8888,
>> +};
>> +
>> +static const struct sun8i_plane_desc sun8i_mixer_planes[] = {
>> + {
>> + .type = DRM_PLANE_TYPE_PRIMARY,
>> + .formats = sun8i_mixer_layer_formats,
>> + .nformats = ARRAY_SIZE(sun8i_mixer_layer_formats),
>> + },
>> +};
>> +
>> +static struct sun8i_layer *sun8i_layer_init_one(struct drm_device
>*drm,
>> + struct sun8i_mixer *mixer,
>> + const struct sun8i_plane_desc *plane)
>> +{
>> + struct sun8i_layer *layer;
>> + int ret;
>> +
>> + layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL);
>> + if (!layer)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + /* possible crtcs are set later */
>> + ret = drm_universal_plane_init(drm, &layer->plane, 0,
>> + &sun8i_mixer_layer_funcs,
>> + plane->formats, plane->nformats,
>> + plane->type, NULL);
>> + if (ret) {
>> + dev_err(drm->dev, "Couldn't initialize layer\n");
>> + return ERR_PTR(ret);
>> + }
>> +
>> + drm_plane_helper_add(&layer->plane,
>> + &sun8i_mixer_layer_helper_funcs);
>> + layer->mixer = mixer;
>> +
>> + return layer;
>> +}
>> +
>> +struct drm_plane **sun8i_layers_init(struct drm_device *drm,
>> + struct sun4i_crtc *crtc)
>> +{
>> + struct drm_plane **planes;
>> + struct sun8i_mixer *mixer = crtc->engine;
>> + int i;
>> +
>> + planes = devm_kcalloc(drm->dev, ARRAY_SIZE(sun8i_mixer_planes) + 1,
>> + sizeof(*planes), GFP_KERNEL);
>> + if (!planes)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + for (i = 0; i < ARRAY_SIZE(sun8i_mixer_planes); i++) {
>> + const struct sun8i_plane_desc *plane = &sun8i_mixer_planes[i];
>> + struct sun8i_layer *layer;
>> +
>> + layer = sun8i_layer_init_one(drm, mixer, plane);
>> + if (IS_ERR(layer)) {
>> + dev_err(drm->dev, "Couldn't initialize %s plane\n",
>> + i ? "overlay" : "primary");
>> + return ERR_CAST(layer);
>> + };
>> +
>> + layer->id = i;
>> + planes[i] = &layer->plane;
>> + };
>> +
>> + return planes;
>> +}
>> diff --git a/drivers/gpu/drm/sun4i/sun8i_layer.h
>b/drivers/gpu/drm/sun4i/sun8i_layer.h
>> new file mode 100644
>> index 000000000000..fe7e8a069d71
>> --- /dev/null
>> +++ b/drivers/gpu/drm/sun4i/sun8i_layer.h
>> @@ -0,0 +1,36 @@
>> +/*
>> + * Copyright (C) Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
>> + *
>> + * Based on sun4i_layer.h, which is:
>> + * Copyright (C) 2015 Free Electrons
>> + * Copyright (C) 2015 NextThing Co
>> + *
>> + * Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + */
>> +
>> +#ifndef _SUN8I_LAYER_H_
>> +#define _SUN8I_LAYER_H_
>> +
>> +struct sun4i_crtc;
>> +
>> +struct sun8i_layer {
>> + struct drm_plane plane;
>> + struct sun4i_drv *drv;
>> + struct sun8i_mixer *mixer;
>> + int id;
>> +};
>> +
>> +static inline struct sun8i_layer *
>> +plane_to_sun8i_layer(struct drm_plane *plane)
>> +{
>> + return container_of(plane, struct sun8i_layer, plane);
>> +}
>> +
>> +struct drm_plane **sun8i_layers_init(struct drm_device *drm,
>> + struct sun4i_crtc *crtc);
>> +#endif /* _SUN8I_LAYER_H_ */
>> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c
>b/drivers/gpu/drm/sun4i/sun8i_mixer.c
>> new file mode 100644
>> index 000000000000..5cff3f3833a7
>> --- /dev/null
>> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
>> @@ -0,0 +1,381 @@
>> +/*
>> + * Copyright (C) 2017 Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
>> + *
>> + * Based on sun4i_backend.c, which is:
>> + * Copyright (C) 2015 Free Electrons
>> + * Copyright (C) 2015 NextThing Co
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + */
>> +
>> +#include <drm/drmP.h>
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_crtc.h>
>> +#include <drm/drm_crtc_helper.h>
>> +#include <drm/drm_fb_cma_helper.h>
>> +#include <drm/drm_gem_cma_helper.h>
>> +#include <drm/drm_plane_helper.h>
>> +
>> +#include <linux/component.h>
>> +#include <linux/reset.h>
>> +#include <linux/of_device.h>
>> +
>> +#include "sun4i_drv.h"
>> +#include "sun8i_mixer.h"
>> +#include "sun8i_layer.h"
>> +#include "sunxi_engine.h"
>> +
>> +void sun8i_mixer_commit(void *mixer)
>> +{
>> + struct sun8i_mixer *sun8i_mixer = mixer;
>> +
>> + DRM_DEBUG_DRIVER("Committing changes\n");
>> +
>> + regmap_write(sun8i_mixer->regs, SUN8I_MIXER_GLOBAL_DBUFF,
>> + SUN8I_MIXER_GLOBAL_DBUFF_ENABLE);
>> +}
>> +
>> +void sun8i_mixer_layer_enable(struct sun8i_mixer *mixer,
>> + int layer, bool enable)
>> +{
>> + u32 val;
>> + /* Currently the first UI channel is used */
>> + int chan = mixer->cfg->vi_num;
>> +
>> + DRM_DEBUG_DRIVER("Enabling layer %d in channel %d\n", layer, chan);
>> +
>> + if (enable)
>> + val = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN;
>> + else
>> + val = 0;
>> +
>> + regmap_update_bits(mixer->regs,
>> + SUN8I_MIXER_CHAN_UI_LAYER_ATTR(chan, layer),
>> + SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val);
>> +
>> + /* Set the alpha configuration */
>> + regmap_update_bits(mixer->regs,
>> + SUN8I_MIXER_CHAN_UI_LAYER_ATTR(chan, layer),
>> + SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_MASK,
>> + SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_DEF);
>> + regmap_update_bits(mixer->regs,
>> + SUN8I_MIXER_CHAN_UI_LAYER_ATTR(chan, layer),
>> + SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK,
>> + SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_DEF);
>> +}
>> +EXPORT_SYMBOL(sun8i_mixer_layer_enable);
>
>Why do you need to export it?
Oh... not needed. This export is dead code.
>
>> +
>> +static int sun8i_mixer_drm_format_to_layer(struct drm_plane *plane,
>> + u32 format, u32 *mode)
>> +{
>> + switch (format) {
>> + case DRM_FORMAT_XRGB8888:
>> + *mode = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_XRGB8888;
>> + break;
>> +
>> + case DRM_FORMAT_RGB888:
>> + *mode = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_RGB888;
>> + break;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int sun8i_mixer_update_layer_coord(struct sun8i_mixer *mixer,
>> + int layer, struct drm_plane *plane)
>> +{
>> + struct drm_plane_state *state = plane->state;
>> + struct drm_framebuffer *fb = state->fb;
>> + /* Currently the first UI channel is used */
>> + int chan = mixer->cfg->vi_num;
>> +
>> + DRM_DEBUG_DRIVER("Updating layer %d\n", layer);
>> +
>> + if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
>> + DRM_DEBUG_DRIVER("Primary layer, updating global size W: %u H:
>%u\n",
>> + state->crtc_w, state->crtc_h);
>> + regmap_write(mixer->regs, SUN8I_MIXER_GLOBAL_SIZE,
>> + SUN8I_MIXER_SIZE(state->crtc_w,
>> + state->crtc_h));
>> + DRM_DEBUG_DRIVER("Updating blender size\n");
>> + regmap_write(mixer->regs,
>> + SUN8I_MIXER_BLEND_ATTR_INSIZE(0),
>> + SUN8I_MIXER_SIZE(state->crtc_w,
>> + state->crtc_h));
>> + regmap_write(mixer->regs, SUN8I_MIXER_BLEND_OUTSIZE,
>> + SUN8I_MIXER_SIZE(state->crtc_w,
>> + state->crtc_h));
>> + DRM_DEBUG_DRIVER("Updating channel size\n");
>> + regmap_write(mixer->regs, SUN8I_MIXER_CHAN_UI_OVL_SIZE(chan),
>> + SUN8I_MIXER_SIZE(state->crtc_w,
>> + state->crtc_h));
>> + }
>> +
>> + /* Set the line width */
>> + DRM_DEBUG_DRIVER("Layer line width: %d bytes\n", fb->pitches[0]);
>> + regmap_write(mixer->regs, SUN8I_MIXER_CHAN_UI_LAYER_PITCH(chan,
>layer),
>> + fb->pitches[0]);
>> +
>> + /* Set height and width */
>> + DRM_DEBUG_DRIVER("Layer size W: %u H: %u\n",
>> + state->crtc_w, state->crtc_h);
>> + regmap_write(mixer->regs, SUN8I_MIXER_CHAN_UI_LAYER_SIZE(chan,
>layer),
>> + SUN8I_MIXER_SIZE(state->crtc_w, state->crtc_h));
>> +
>> + /* Set base coordinates */
>> + DRM_DEBUG_DRIVER("Layer coordinates X: %d Y: %d\n",
>> + state->crtc_x, state->crtc_y);
>> + regmap_write(mixer->regs, SUN8I_MIXER_CHAN_UI_LAYER_COORD(chan,
>layer),
>> + SUN8I_MIXER_COORD(state->crtc_x, state->crtc_y));
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(sun8i_mixer_update_layer_coord);
>> +
>> +int sun8i_mixer_update_layer_formats(struct sun8i_mixer *mixer,
>> + int layer, struct drm_plane *plane)
>> +{
>> + struct drm_plane_state *state = plane->state;
>> + struct drm_framebuffer *fb = state->fb;
>> + bool interlaced = false;
>> + u32 val;
>> + /* Currently the first UI channel is used */
>> + int chan = mixer->cfg->vi_num;
>> + int ret;
>> +
>> + if (plane->state->crtc)
>> + interlaced = plane->state->crtc->state->adjusted_mode.flags
>> + & DRM_MODE_FLAG_INTERLACE;
>> +
>> + regmap_update_bits(mixer->regs, SUN8I_MIXER_BLEND_OUTCTL,
>> + SUN8I_MIXER_BLEND_OUTCTL_INTERLACED,
>> + interlaced ?
>> + SUN8I_MIXER_BLEND_OUTCTL_INTERLACED : 0);
>> +
>> + DRM_DEBUG_DRIVER("Switching display mixer interlaced mode %s\n",
>> + interlaced ? "on" : "off");
>
>You're not using interlaced anywhere.
ok.
>
>> +
>> + ret = sun8i_mixer_drm_format_to_layer(plane, fb->format->format,
>> + &val);
>> + if (ret) {
>> + DRM_DEBUG_DRIVER("Invalid format\n");
>> + return ret;
>> + }
>> +
>> + regmap_update_bits(mixer->regs,
>> + SUN8I_MIXER_CHAN_UI_LAYER_ATTR(chan, layer),
>> + SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_MASK, val);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(sun8i_mixer_update_layer_formats);
>> +
>> +int sun8i_mixer_update_layer_buffer(struct sun8i_mixer *mixer,
>> + int layer, struct drm_plane *plane)
>> +{
>> + struct drm_plane_state *state = plane->state;
>> + struct drm_framebuffer *fb = state->fb;
>> + struct drm_gem_cma_object *gem;
>> + dma_addr_t paddr;
>> + uint32_t paddr_u32;
>> + /* Currently the first UI channel is used */
>> + int chan = mixer->cfg->vi_num;
>> + int bpp;
>> +
>> + /* Get the physical address of the buffer in memory */
>> + gem = drm_fb_cma_get_gem_obj(fb, 0);
>> +
>> + DRM_DEBUG_DRIVER("Using GEM @ %pad\n", &gem->paddr);
>> +
>> + /* Compute the start of the displayed memory */
>> + bpp = fb->format->cpp[0];
>> + paddr = gem->paddr + fb->offsets[0];
>> + paddr += (state->src_x >> 16) * bpp;
>> + paddr += (state->src_y >> 16) * fb->pitches[0];
>> +
>> + DRM_DEBUG_DRIVER("Setting buffer address to %pad\n", &paddr);
>> +
>> + paddr_u32 = (uint32_t) paddr;
>
>How does that work on 64-bits systems ?
The hardware is not designed to work on 64-bit systems.
Even 64-bit A64/H5 has also 3GiB memory limit.
The address cell in mixer hardware is also only 32-bit.
So we should just keep the force conversion here. If we then really met 4GiB-capable AW SoC without changing DE2, I think we should have other way to limit CMA pool inside 4GiB.
>
>> +
>> + regmap_write(mixer->regs,
>> + SUN8I_MIXER_CHAN_UI_LAYER_TOP_LADDR(chan, layer),
>> + paddr_u32);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(sun8i_mixer_update_layer_buffer);
>> +
>> +static const struct sunxi_engine_ops sun8i_engine_ops = {
>> + .commit = sun8i_mixer_commit,
>> + .layers_init = sun8i_layers_init,
>
>Align with tabs.
>
>> +};
>> +
>> +static struct regmap_config sun8i_mixer_regmap_config = {
>> + .reg_bits = 32,
>> + .val_bits = 32,
>> + .reg_stride = 4,
>> + .max_register = 0xbfffc, /* guessed */
>> +};
>> +
>> +static int sun8i_mixer_bind(struct device *dev, struct device
>*master,
>> + void *data)
>> +{
>> + struct platform_device *pdev = to_platform_device(dev);
>> + struct drm_device *drm = data;
>> + struct sun4i_drv *drv = drm->dev_private;
>> + struct sun8i_mixer *mixer;
>> + struct resource *res;
>> + void __iomem *regs;
>> + int i, ret;
>> +
>> + mixer = devm_kzalloc(dev, sizeof(*mixer), GFP_KERNEL);
>> + if (!mixer)
>> + return -ENOMEM;
>> + dev_set_drvdata(dev, mixer);
>> + drv->engine = mixer;
>> + drv->engine_ops = &sun8i_engine_ops;
>> +
>> + mixer->cfg = of_device_get_match_data(dev);
>> + if (!mixer->cfg)
>> + return -EINVAL;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + regs = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(regs))
>> + return PTR_ERR(regs);
>> +
>> + mixer->regs = devm_regmap_init_mmio(dev, regs,
>> + &sun8i_mixer_regmap_config);
>> + if (IS_ERR(mixer->regs)) {
>> + dev_err(dev, "Couldn't create the mixer regmap\n");
>> + return PTR_ERR(mixer->regs);
>> + }
>> +
>> + mixer->reset = devm_reset_control_get(dev, NULL);
>> + if (IS_ERR(mixer->reset)) {
>> + dev_err(dev, "Couldn't get our reset line\n");
>> + return PTR_ERR(mixer->reset);
>> + }
>> +
>> + ret = reset_control_deassert(mixer->reset);
>> + if (ret) {
>> + dev_err(dev, "Couldn't deassert our reset line\n");
>> + return ret;
>> + }
>> +
>> + mixer->bus_clk = devm_clk_get(dev, "bus");
>> + if (IS_ERR(mixer->bus_clk)) {
>> + dev_err(dev, "Couldn't get the mixer bus clock\n");
>> + ret = PTR_ERR(mixer->bus_clk);
>> + goto err_assert_reset;
>> + }
>> + clk_prepare_enable(mixer->bus_clk);
>> +
>> + mixer->mod_clk = devm_clk_get(dev, "mod");
>> + if (IS_ERR(mixer->mod_clk)) {
>> + dev_err(dev, "Couldn't get the mixer module clock\n");
>> + ret = PTR_ERR(mixer->mod_clk);
>> + goto err_disable_bus_clk;
>> + }
>> + clk_prepare_enable(mixer->mod_clk);
>> +
>> + /* Reset the registers */
>> + for (i = 0x0; i < 0x20000; i += 4)
>> + regmap_write(mixer->regs, i, 0);
>> +
>> + /* Enable the mixer */
>> + regmap_write(mixer->regs, SUN8I_MIXER_GLOBAL_CTL,
>> + SUN8I_MIXER_GLOBAL_CTL_RT_EN);
>> +
>> + /* Initialize blender */
>> + regmap_write(mixer->regs, SUN8I_MIXER_BLEND_FCOLOR_CTL,
>> + SUN8I_MIXER_BLEND_FCOLOR_CTL_DEF);
>> + regmap_write(mixer->regs, SUN8I_MIXER_BLEND_PREMULTIPLY,
>> + SUN8I_MIXER_BLEND_PREMULTIPLY_DEF);
>> + regmap_write(mixer->regs, SUN8I_MIXER_BLEND_BKCOLOR,
>> + SUN8I_MIXER_BLEND_BKCOLOR_DEF);
>> + regmap_write(mixer->regs, SUN8I_MIXER_BLEND_MODE(0),
>> + SUN8I_MIXER_BLEND_MODE_DEF);
>> + regmap_write(mixer->regs, SUN8I_MIXER_BLEND_CK_CTL,
>> + SUN8I_MIXER_BLEND_CK_CTL_DEF);
>> +
>> + regmap_write(mixer->regs,
>> + SUN8I_MIXER_BLEND_ATTR_FCOLOR(0),
>> + SUN8I_MIXER_BLEND_ATTR_FCOLOR_DEF);
>> +
>> + /* Select the first UI channel */
>> + DRM_DEBUG_DRIVER("Selecting channel %d (first UI channel)\n",
>> + mixer->cfg->vi_num);
>> + regmap_write(mixer->regs, SUN8I_MIXER_BLEND_ROUTE,
>> + mixer->cfg->vi_num);
>> +
>> + return 0;
>> +
>> + clk_disable_unprepare(mixer->mod_clk);
>> +err_disable_bus_clk:
>> + clk_disable_unprepare(mixer->bus_clk);
>> +err_assert_reset:
>> + reset_control_assert(mixer->reset);
>> + return ret;
>> +}
>> +
>> +static void sun8i_mixer_unbind(struct device *dev, struct device
>*master,
>> + void *data)
>> +{
>> + struct sun8i_mixer *mixer = dev_get_drvdata(dev);
>> +
>> + clk_disable_unprepare(mixer->mod_clk);
>> + clk_disable_unprepare(mixer->bus_clk);
>> + reset_control_assert(mixer->reset);
>> +}
>> +
>> +static const struct component_ops sun8i_mixer_ops = {
>> + .bind = sun8i_mixer_bind,
>> + .unbind = sun8i_mixer_unbind,
>> +};
>> +
>> +static int sun8i_mixer_probe(struct platform_device *pdev)
>> +{
>> + return component_add(&pdev->dev, &sun8i_mixer_ops);
>> +}
>> +
>> +static int sun8i_mixer_remove(struct platform_device *pdev)
>> +{
>> + component_del(&pdev->dev, &sun8i_mixer_ops);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct sun8i_mixer_cfg sun8i_v3s_mixer_cfg = {
>> + .vi_num = 2,
>> + .ui_num = 1,
>> +};
>> +
>> +static const struct of_device_id sun8i_mixer_of_table[] = {
>> + {
>> + .compatible = "allwinner,sun8i-v3s-de2-mixer",
>> + .data = &sun8i_v3s_mixer_cfg,
>> + },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(of, sun8i_mixer_of_table);
>> +
>> +static struct platform_driver sun8i_mixer_platform_driver = {
>> + .probe = sun8i_mixer_probe,
>> + .remove = sun8i_mixer_remove,
>> + .driver = {
>> + .name = "sun8i-mixer",
>> + .of_match_table = sun8i_mixer_of_table,
>> + },
>> +};
>> +module_platform_driver(sun8i_mixer_platform_driver);
>> +
>> +MODULE_AUTHOR("Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>");
>> +MODULE_DESCRIPTION("Allwinner DE2 Mixer driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
>b/drivers/gpu/drm/sun4i/sun8i_mixer.h
>> new file mode 100644
>> index 000000000000..a4a365ae44c9
>> --- /dev/null
>> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
>> @@ -0,0 +1,131 @@
>> +/*
>> + * Copyright (C) 2017 Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + */
>> +
>> +#ifndef _SUN8I_MIXER_H_
>> +#define _SUN8I_MIXER_H_
>> +
>> +#include <linux/clk.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +
>> +#include "sun4i_layer.h"
>> +
>> +#define SUN8I_MIXER_MAX_CHAN_COUNT 4
>> +
>> +#define SUN8I_MIXER_SIZE(w, h) (((h) - 1) << 16 | ((w) - 1))
>> +#define SUN8I_MIXER_COORD(x, y) ((y) << 16 | (x))
>> +
>> +#define SUN8I_MIXER_GLOBAL_CTL 0x0
>> +#define SUN8I_MIXER_GLOBAL_STATUS 0x4
>> +#define SUN8I_MIXER_GLOBAL_DBUFF 0x8
>> +#define SUN8I_MIXER_GLOBAL_SIZE 0xc
>> +
>> +#define SUN8I_MIXER_GLOBAL_CTL_RT_EN 0x1
>> +
>> +#define SUN8I_MIXER_GLOBAL_DBUFF_ENABLE 0x1
>> +
>> +#define SUN8I_MIXER_BLEND_FCOLOR_CTL 0x1000
>> +#define SUN8I_MIXER_BLEND_ATTR_FCOLOR(x) (0x1004 + 0x10 * (x) + 0x0)
>> +#define SUN8I_MIXER_BLEND_ATTR_INSIZE(x) (0x1004 + 0x10 * (x) + 0x4)
>> +#define SUN8I_MIXER_BLEND_ATTR_OFFSET(x) (0x1004 + 0x10 * (x) + 0x8)
>> +#define SUN8I_MIXER_BLEND_ROUTE 0x1080
>> +#define SUN8I_MIXER_BLEND_PREMULTIPLY 0x1084
>> +#define SUN8I_MIXER_BLEND_BKCOLOR 0x1088
>> +#define SUN8I_MIXER_BLEND_OUTSIZE 0x108c
>> +#define SUN8I_MIXER_BLEND_MODE(x) (0x1090 + 0x04 * (x))
>> +#define SUN8I_MIXER_BLEND_CK_CTL 0x10b0
>> +#define SUN8I_MIXER_BLEND_CK_CFG 0x10b4
>> +#define SUN8I_MIXER_BLEND_CK_MAX(x) (0x10c0 + 0x04 * (x))
>> +#define SUN8I_MIXER_BLEND_CK_MIN(x) (0x10e0 + 0x04 * (x))
>> +#define SUN8I_MIXER_BLEND_OUTCTL 0x10fc
>> +
>> +/* The following numbers are some still unknown magic numbers */
>> +#define SUN8I_MIXER_BLEND_ATTR_FCOLOR_DEF 0xff000000
>> +#define SUN8I_MIXER_BLEND_FCOLOR_CTL_DEF 0x00000101
>> +#define SUN8I_MIXER_BLEND_PREMULTIPLY_DEF 0x0
>> +#define SUN8I_MIXER_BLEND_BKCOLOR_DEF 0xff000000
>> +#define SUN8I_MIXER_BLEND_MODE_DEF 0x03010301
>> +#define SUN8I_MIXER_BLEND_CK_CTL_DEF 0x0
>> +
>> +#define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED BIT(1)
>> +
>> +/*
>> + * VI channels are not used now, but the support of them may be
>introduced in
>> + * the future.
>> + */
>> +
>> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch, layer) \
>> + (0x2000 + 0x1000 * (ch) + 0x20 * (layer) + 0x0)
>> +#define SUN8I_MIXER_CHAN_UI_LAYER_SIZE(ch, layer) \
>> + (0x2000 + 0x1000 * (ch) + 0x20 * (layer) + 0x4)
>> +#define SUN8I_MIXER_CHAN_UI_LAYER_COORD(ch, layer) \
>> + (0x2000 + 0x1000 * (ch) + 0x20 * (layer) + 0x8)
>> +#define SUN8I_MIXER_CHAN_UI_LAYER_PITCH(ch, layer) \
>> + (0x2000 + 0x1000 * (ch) + 0x20 * (layer) + 0xc)
>> +#define SUN8I_MIXER_CHAN_UI_LAYER_TOP_LADDR(ch, layer) \
>> + (0x2000 + 0x1000 * (ch) + 0x20 * (layer) + 0x10)
>> +#define SUN8I_MIXER_CHAN_UI_LAYER_BOT_LADDR(ch, layer) \
>> + (0x2000 + 0x1000 * (ch) + 0x20 * (layer) + 0x14)
>> +#define SUN8I_MIXER_CHAN_UI_LAYER_FCOLOR(ch, layer) \
>> + (0x2000 + 0x1000 * (ch) + 0x20 * (layer) + 0x18)
>> +#define SUN8I_MIXER_CHAN_UI_TOP_HADDR(ch) (0x2000 + 0x1000 * (ch) +
>0x80)
>> +#define SUN8I_MIXER_CHAN_UI_BOT_HADDR(ch) (0x2000 + 0x1000 * (ch) +
>0x84)
>> +#define SUN8I_MIXER_CHAN_UI_OVL_SIZE(ch) (0x2000 + 0x1000 * (ch) +
>0x88)
>> +
>> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN BIT(0)
>> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_MASK GENMASK(2, 1)
>> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_MASK GENMASK(11, 8)
>> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK GENMASK(31, 24)
>> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_DEF (1 << 1)
>> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_ARGB8888 (0 << 8)
>> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_XRGB8888 (4 << 8)
>> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_FBFMT_RGB888 (8 << 8)
>> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_DEF (0xff << 24)
>> +
>> +/*
>> + * These sub-engines are still unknown now, the EN registers are
>here only to
>> + * be used to disable these sub-engines.
>> + */
>> +#define SUN8I_MIXER_VSU_EN 0x20000
>> +#define SUN8I_MIXER_GSU1_EN 0x30000
>> +#define SUN8I_MIXER_GSU2_EN 0x40000
>> +#define SUN8I_MIXER_GSU3_EN 0x50000
>> +#define SUN8I_MIXER_FCE_EN 0xa0000
>> +#define SUN8I_MIXER_BWS_EN 0xa2000
>> +#define SUN8I_MIXER_LTI_EN 0xa4000
>> +#define SUN8I_MIXER_PEAK_EN 0xa6000
>> +#define SUN8I_MIXER_ASE_EN 0xa8000
>> +#define SUN8I_MIXER_FCC_EN 0xaa000
>> +#define SUN8I_MIXER_DCSC_EN 0xb0000
>> +
>> +struct sun8i_mixer_cfg {
>> + int vi_num;
>> + int ui_num;
>> +};
>> +
>> +struct sun8i_mixer {
>> + struct regmap *regs;
>> +
>> + const struct sun8i_mixer_cfg *cfg;
>> +
>> + struct reset_control *reset;
>> +
>> + struct clk *bus_clk;
>> + struct clk *mod_clk;
>> +};
>> +
>> +void sun8i_mixer_layer_enable(struct sun8i_mixer *mixer,
>> + int layer, bool enable);
>> +int sun8i_mixer_update_layer_coord(struct sun8i_mixer *mixer,
>> + int layer, struct drm_plane *plane);
>> +int sun8i_mixer_update_layer_formats(struct sun8i_mixer *mixer,
>> + int layer, struct drm_plane *plane);
>> +int sun8i_mixer_update_layer_buffer(struct sun8i_mixer *mixer,
>> + int layer, struct drm_plane *plane);
>> +#endif /* _SUN8I_MIXER_H_ */
>
>Thanks,
>Maxime
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings
From: Philipp Zabel @ 2017-04-18 10:51 UTC (permalink / raw)
To: Sakari Ailus
Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Pavel Machek,
Steve Longerbeam, Peter Rosin
In-Reply-To: <20170418100840.GF7456-S+BSfZ9RZZmRSg0ZkenSGLdO1Tsj/99ntUK59QYPAWc@public.gmane.org>
On Tue, 2017-04-18 at 13:08 +0300, Sakari Ailus wrote:
> Hi Philipp,
>
> On Tue, Apr 18, 2017 at 10:19:04AM +0200, Philipp Zabel wrote:
> > On Thu, 2017-04-13 at 17:48 +0200, Philipp Zabel wrote:
> > > This adds device tree binding documentation for mmio-based syscon
> > > multiplexers controlled by a single bitfield in a syscon register
> > > range.
> > >
> > > Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > ---
> > > Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> > > 1 file changed, 56 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > new file mode 100644
> > > index 0000000000000..11d96f5d98583
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > @@ -0,0 +1,56 @@
> > > +MMIO bitfield-based multiplexer controller bindings
> > > +
> > > +Define a syscon bitfield to be used to control a multiplexer. The parent
> > > +device tree node must be a syscon node to provide register access.
> > > +
> > > +Required properties:
> > > +- compatible : "gpio-mux"
> > > +- reg : register base of the register containing the control bitfield
> > > +- bit-mask : bitmask of the control bitfield in the control register
> > > +- bit-shift : bit offset of the control bitfield in the control register
> > > +- #mux-control-cells : <0>
> > > +* Standard mux-controller bindings as decribed in mux-controller.txt
> > > +
> > > +Optional properties:
> > > +- idle-state : if present, the state the mux will have when idle. The
> > > + special state MUX_IDLE_AS_IS is the default.
> > > +
> > > +The multiplexer state is defined as the value of the bitfield described
> > > +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> > > +syscon.
> > > +
> > > +Example:
> > > +
> > > + syscon {
> > > + compatible = "syscon";
> > > +
> > > + mux: mux-controller@3 {
> > > + compatible = "mmio-mux";
> > > + reg = <0x3>;
> > > + bit-mask = <0x1>;
> > > + bit-shift = <5>;
> > > + #mux-control-cells = <0>;
> > > + };
> > > + };
> > > +
> > > + video-mux {
> > > + compatible = "video-mux";
> > > + mux-controls = <&mux>;
> > > +
> > > + ports {
> > > + /* input 0 */
> > > + port@0 {
> > > + reg = <0>;
> > > + };
> > > +
> > > + /* input 1 */
> > > + port@1 {
> > > + reg = <1>;
> > > + };
> > > +
> > > + /* output */
> > > + port@2 {
> > > + reg = <2>;
> > > + };
> > > + };
> > > + };
> >
> > So Pavel (added to Cc:) suggested to merge these into one node for the
> > video mux, as really we are describing a single hardware entity that
> > happens to be multiplexing multiple video buses into one:
>
> Two drivers will be needed in a way or another to disconnect the dependency
> between the video switch driver and the MUX implementation. Are there ways
> to do that cleanly other than having two devices?
We are talking about the device tree bindings, drivers and devices
shouldn't factor into it yet. A video-mmio-mux driver could very well
create a mmio-mux platform device internally, if necessary. Or it could
just use the same library functions that the mmio-mux driver uses,
without creating a second device.
> And if there are two devices, shouldn't the video switch device be a child
> of the MUX device? I think it'd be odd to have it hanging around in a
> completely unrelated part of the device tree.
That boils down to whether you consider the connection between mux
controller and mux to be resource usage that should be described via a
phandle reference, like pwms, gpios, clocks, and so on, or whether you
consider it to be control flow in the device tree sense, which should be
described as a parent-child relationship of the nodes. The mux framework
is designed around the former.
We could embrace this and consider a syscon region that contains
multiple mux bitfields as one mux controller that controls multiple
muxes, with a binding similar to, for example, the
reset/ti-syscon-reset.txt bindings:
syscon {
compatible = "syscon";
mux: mux-controller@4 {
compatible = "mmio-mux";
/* register, bit shift, bit mask */
mux-bits = <0x4 19 0x1>, /* 0: CSI0 mux */
<0x4 20 0x1>; /* 1: CSI1 mux */
#mux-control-cells = <1>;
};
};
/* somewhere else */
csi0-mux {
compatible = "video-mux";
mux-controls = <&mux 0>;
ports {
/* ... */
};
};
csi1-mux {
compatible = "video-mux";
mux-controls = <&mux 1>;
ports {
/* ... */
};
};
regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings
From: Sakari Ailus @ 2017-04-18 10:55 UTC (permalink / raw)
To: Pavel Machek
Cc: Philipp Zabel, Peter Rosin, Rob Herring, Mark Rutland,
Steve Longerbeam, devicetree, linux-kernel, kernel
In-Reply-To: <20170418103453.GC14505@amd>
On Tue, Apr 18, 2017 at 12:34:53PM +0200, Pavel Machek wrote:
> On Tue 2017-04-18 13:08:41, Sakari Ailus wrote:
> > Hi Philipp,
> >
> > On Tue, Apr 18, 2017 at 10:19:04AM +0200, Philipp Zabel wrote:
> > > On Thu, 2017-04-13 at 17:48 +0200, Philipp Zabel wrote:
> > > > This adds device tree binding documentation for mmio-based syscon
> > > > multiplexers controlled by a single bitfield in a syscon register
> > > > range.
> > > >
> > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > ---
> > > > Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> > > > 1 file changed, 56 insertions(+)
> > > > create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > > new file mode 100644
> > > > index 0000000000000..11d96f5d98583
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > > @@ -0,0 +1,56 @@
> > > > +MMIO bitfield-based multiplexer controller bindings
> > > > +
> > > > +Define a syscon bitfield to be used to control a multiplexer. The parent
> > > > +device tree node must be a syscon node to provide register access.
> > > > +
> > > > +Required properties:
> > > > +- compatible : "gpio-mux"
> > > > +- reg : register base of the register containing the control bitfield
> > > > +- bit-mask : bitmask of the control bitfield in the control register
> > > > +- bit-shift : bit offset of the control bitfield in the control register
> > > > +- #mux-control-cells : <0>
> > > > +* Standard mux-controller bindings as decribed in mux-controller.txt
> > > > +
> > > > +Optional properties:
> > > > +- idle-state : if present, the state the mux will have when idle. The
> > > > + special state MUX_IDLE_AS_IS is the default.
> > > > +
> > > > +The multiplexer state is defined as the value of the bitfield described
> > > > +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> > > > +syscon.
> > > > +
> > > > +Example:
> > > > +
> > > > + syscon {
> > > > + compatible = "syscon";
> > > > +
> > > > + mux: mux-controller@3 {
> > > > + compatible = "mmio-mux";
> > > > + reg = <0x3>;
> > > > + bit-mask = <0x1>;
> > > > + bit-shift = <5>;
> > > > + #mux-control-cells = <0>;
> > > > + };
> > > > + };
> > > > +
> > > > + video-mux {
> > > > + compatible = "video-mux";
> > > > + mux-controls = <&mux>;
> > > > +
> > > > + ports {
> > > > + /* input 0 */
> > > > + port@0 {
> > > > + reg = <0>;
> > > > + };
> > > > +
> > > > + /* input 1 */
> > > > + port@1 {
> > > > + reg = <1>;
> > > > + };
> > > > +
> > > > + /* output */
> > > > + port@2 {
> > > > + reg = <2>;
> > > > + };
> > > > + };
> > > > + };
> > >
> > > So Pavel (added to Cc:) suggested to merge these into one node for the
> > > video mux, as really we are describing a single hardware entity that
> > > happens to be multiplexing multiple video buses into one:
> >
> > Two drivers will be needed in a way or another to disconnect the dependency
> > between the video switch driver and the MUX implementation. Are there ways
> > to do that cleanly other than having two devices?
>
> Yes.
>
> Device tree describes hardware, not the driver structure.
I think you you could view the MUX control as a device, too, and that's
separate from the actual video switch.
This isn't really related to the video switch as much as it's got to do with
the MUX framework, so having Peter's opinion here would be very helpful.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply
* Re: [PATCH v3 09/12] mfd: axp20x: add axp20x-regulator cell for AXP803
From: Icenowy Zheng @ 2017-04-18 10:55 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Lee Jones, Rob Herring, Maxime Ripard, Liam Girdwood, Mark Brown,
devicetree, linux-kernel, linux-arm-kernel, linux-sunxi
In-Reply-To: <CAGb2v648fhGSjWoVYRxU+_kF5fhbnhmY2mQG16ORtBHf7pGx5w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
于 2017年4月18日 GMT+08:00 下午6:38:09, Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> 写到:
>On Mon, Apr 17, 2017 at 7:57 PM, Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org> wrote:
>> As axp20x-regulator now supports AXP803, add a cell for it.
>>
>> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
>> ---
>> Changes in v3:
>> - Make the new cell one-liner.
>>
>> drivers/mfd/axp20x.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>> index 1dc6235778eb..431b7f118606 100644
>> --- a/drivers/mfd/axp20x.c
>> +++ b/drivers/mfd/axp20x.c
>> @@ -848,7 +848,8 @@ static struct mfd_cell axp803_cells[] = {
>> .name = "axp20x-pek",
>> .num_resources =
>ARRAY_SIZE(axp803_pek_resources),
>> .resources = axp803_pek_resources,
>> - }
>> + },
>> + { .name = "axp20x-regulator" }
>
>It's best to have a trailing comma, so we don't have to change the line
>again when we add more cells, like you just did with the previous line.
Should I also add it in previous mfd patch? (and remove the addition of the comma in this patch)
>
>Otherwise,
>
>Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: Re: [PATCH v3 02/12] arm64: allwinner: a64: add NMI controller on A64
From: Icenowy Zheng @ 2017-04-18 10:56 UTC (permalink / raw)
To: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
Cc: Lee Jones, Rob Herring, Chen-Yu Tsai, Liam Girdwood, Mark Brown,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20170418070016.qsng3qtk76bqxyc5@lukather>
于 2017年4月18日 GMT+08:00 下午3:00:16, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 写到:
>On Mon, Apr 17, 2017 at 07:57:37PM +0800, Icenowy Zheng wrote:
>> Allwinner A64 SoC features a NMI controller, which is usually
>connected
>> to the AXP PMIC.
>>
>> Add support for it.
>>
>> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
>> Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>> ---
>> Changes in v2:
>> - Added Chen-Yu's ACK.
>>
>> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> index 05ec9fc5e81f..53c18ca372ea 100644
>> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> @@ -403,6 +403,14 @@
>> <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
>> };
>>
>> + nmi_intc: interrupt-controller@01f00c0c {
>> + compatible = "allwinner,sun6i-a31-sc-nmi";
>> + interrupt-controller;
>> + #interrupt-cells = <2>;
>> + reg = <0x01f00c0c 0x38>;
>
>The base address is not correct, and there's uncertainty on whether
>this is this particular controller or not. Did you even test this?
Tested by axp20x-pek.
>
>Maxime
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
From: Peter Rosin @ 2017-04-18 10:59 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, Wolfram Sang, Rob Herring, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, linux-i2c, devicetree,
linux-iio, linux-doc, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel
In-Reply-To: <20170418085156.GA4773@kroah.com>
On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
> On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
>> +config MUX_GPIO
>> + tristate "GPIO-controlled Multiplexer"
>> + depends on OF && GPIOLIB
>
> Why have the gpio and mux core in the same patch?
One is not usable w/o the other. I can split them if you want to?
> And why does this depend on OF?
That's historical, I was originally using of_property_read_u32.
I'll remove the dep...
>> + help
>> + GPIO-controlled Multiplexer controller.
>> +
>> + The driver builds a single multiplexer controller using a number
>> + of gpio pins. For N pins, there will be 2^N possible multiplexer
>> + states. The GPIO pins can be connected (by the hardware) to several
>> + multiplexers, which in that case will be operated in parallel.
>> +
>> + To compile the driver as a module, choose M here: the module will
>> + be called mux-gpio.
>> +
>> +endif
>> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
>> new file mode 100644
>> index 000000000000..bb16953f6290
>> --- /dev/null
>> +++ b/drivers/mux/Makefile
>> @@ -0,0 +1,6 @@
>> +#
>> +# Makefile for multiplexer devices.
>> +#
>> +
>> +obj-$(CONFIG_MULTIPLEXER) += mux-core.o
>> +obj-$(CONFIG_MUX_GPIO) += mux-gpio.o
>> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
>> new file mode 100644
>> index 000000000000..66a8bccfc3d7
>> --- /dev/null
>> +++ b/drivers/mux/mux-core.c
>> @@ -0,0 +1,422 @@
>> +/*
>> + * Multiplexer subsystem
>> + *
>> + * Copyright (C) 2017 Axentia Technologies AB
>> + *
>> + * Author: Peter Rosin <peda@axentia.se>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#define pr_fmt(fmt) "mux-core: " fmt
>> +
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/export.h>
>> +#include <linux/idr.h>
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/mux.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/slab.h>
>> +
>> +/*
>> + * The idle-as-is "state" is not an actual state that may be selected, it
>> + * only implies that the state should not be changed. So, use that state
>> + * as indication that the cached state of the multiplexer is unknown.
>> + */
>> +#define MUX_CACHE_UNKNOWN MUX_IDLE_AS_IS
>> +
>> +static struct class mux_class = {
>> + .name = "mux",
>> + .owner = THIS_MODULE,
>> +};
>
> No Documentation/ABI/ update for your sysfs files? Please do so.
Ok I'll look into it. Wasn't even aware that I added any. But there's the
new class of course...
>> +
>> +static int __init mux_init(void)
>> +{
>> + return class_register(&mux_class);
>> +}
>> +
>> +static DEFINE_IDA(mux_ida);
>
> When your module is unloaded, you forgot to clean this structure up with
> what was done with it.
I was under the impression that not providing an exit function for modules
made the module infrastructure prevent unloading (by bumping some reference
counter). Maybe that is a misconception?
>> +
>> +static void mux_chip_release(struct device *dev)
>> +{
>> + struct mux_chip *mux_chip = to_mux_chip(dev);
>> +
>> + ida_simple_remove(&mux_ida, mux_chip->id);
>> + kfree(mux_chip);
>> +}
>> +
>> +static struct device_type mux_type = {
>> + .name = "mux-chip",
>> + .release = mux_chip_release,
>> +};
>> +
>> +struct mux_chip *mux_chip_alloc(struct device *dev,
>> + unsigned int controllers, size_t sizeof_priv)
>> +{
>> + struct mux_chip *mux_chip;
>> + int i;
>> +
>> + if (WARN_ON(!dev || !controllers))
>> + return NULL;
>> +
>> + mux_chip = kzalloc(sizeof(*mux_chip) +
>> + controllers * sizeof(*mux_chip->mux) +
>> + sizeof_priv, GFP_KERNEL);
>> + if (!mux_chip)
>> + return NULL;
>
> You don't return PTR_ERR(-ENOMEM)? Ok, why not? (I'm not arguing for
> it, just curious...)
There's no particular reason. Do you think I should change it?
>> +
>> + mux_chip->mux = (struct mux_control *)(mux_chip + 1);
>> + mux_chip->dev.class = &mux_class;
>> + mux_chip->dev.type = &mux_type;
>> + mux_chip->dev.parent = dev;
>> + mux_chip->dev.of_node = dev->of_node;
>> + dev_set_drvdata(&mux_chip->dev, mux_chip);
>> +
>> + mux_chip->id = ida_simple_get(&mux_ida, 0, 0, GFP_KERNEL);
>> + if (mux_chip->id < 0) {
>> + pr_err("muxchipX failed to get a device id\n");
>> + kfree(mux_chip);
>> + return NULL;
>> + }
>> + dev_set_name(&mux_chip->dev, "muxchip%d", mux_chip->id);
>> +
>> + mux_chip->controllers = controllers;
>> + for (i = 0; i < controllers; ++i) {
>> + struct mux_control *mux = &mux_chip->mux[i];
>> +
>> + mux->chip = mux_chip;
>> + init_rwsem(&mux->lock);
>> + mux->cached_state = MUX_CACHE_UNKNOWN;
>> + mux->idle_state = MUX_IDLE_AS_IS;
>> + }
>> +
>> + device_initialize(&mux_chip->dev);
>
> Why are you not registering the device here as well? Why have this be a
> two step process?
Because of idle state handling. The drivers are expected to fill in
the desired idle state(s) after allocating the mux controller(s).
Then, when registering, the desired idle state is activated (if the
idle state is not idle-as-is, of course) and as a last step the mux
is "advertised".
>> +
>> + return mux_chip;
>> +}
>> +EXPORT_SYMBOL_GPL(mux_chip_alloc);
>> +
>> +static int mux_control_set(struct mux_control *mux, int state)
>> +{
>> + int ret = mux->chip->ops->set(mux, state);
>> +
>> + mux->cached_state = ret < 0 ? MUX_CACHE_UNKNOWN : state;
>> +
>> + return ret;
>> +}
>> +
>> +int mux_chip_register(struct mux_chip *mux_chip)
>> +{
>> + int i;
>> + int ret;
>> +
>> + for (i = 0; i < mux_chip->controllers; ++i) {
>> + struct mux_control *mux = &mux_chip->mux[i];
>> +
>> + if (mux->idle_state == mux->cached_state)
>> + continue;
>> +
>> + ret = mux_control_set(mux, mux->idle_state);
>> + if (ret < 0) {
>> + dev_err(&mux_chip->dev, "unable to set idle state\n");
>> + return ret;
>> + }
>> + }
>> +
>> + ret = device_add(&mux_chip->dev);
>> + if (ret < 0)
>> + dev_err(&mux_chip->dev,
>> + "device_add failed in mux_chip_register: %d\n", ret);
>
> Did you run checkpatch.pl in strict mode on this new file? Please do so :)
I did, and did it again just to be sure, and I do not get any complaints.
So, what's wrong?
$ scripts/checkpatch.pl --strict mux-13/0003-mux-minimal-mux-subsystem-and-gpio-based-mux-control.patch
total: 0 errors, 0 warnings, 0 checks, 860 lines checked
mux-13/0003-mux-minimal-mux-subsystem-and-gpio-based-mux-control.patch has no obvious style problems and is ready for submission.
The chackpatch warnings I am aware of are three instances of (for the
'prop', 's' and 'i' arguments):
mux-13/0006-iio-multiplexer-new-iio-category-and-iio-mux-driver.patch
---------------------------------------------------------------------
CHECK: Macro argument reuse 'prop' - possible side-effects?
#433: FILE: drivers/iio/multiplexer/iio-mux.c:326:
+#define of_property_for_each_string_index(np, propname, prop, s, i) \
+ for (prop = of_find_property(np, propname, NULL), \
+ s = of_prop_next_string(prop, NULL), \
+ i = 0; \
+ s; \
+ s = of_prop_next_string(prop, s), \
+ i++)
But those kinds of warnings are also present in the code I plagiarized,
so I don't feel too bad...
And then there are a couple of false positives about files added w/o
adding an entry to MAINTAINERS (the files are covers by wildcards).
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(mux_chip_register);
>> +
>> +void mux_chip_unregister(struct mux_chip *mux_chip)
>> +{
>> + device_del(&mux_chip->dev);
>> +}
>> +EXPORT_SYMBOL_GPL(mux_chip_unregister);
>> +
>> +void mux_chip_free(struct mux_chip *mux_chip)
>> +{
>> + if (!mux_chip)
>> + return;
>> +
>> + put_device(&mux_chip->dev);
>> +}
>> +EXPORT_SYMBOL_GPL(mux_chip_free);
>> +
>> +static void devm_mux_chip_release(struct device *dev, void *res)
>> +{
>> + struct mux_chip *mux_chip = *(struct mux_chip **)res;
>> +
>> + mux_chip_free(mux_chip);
>> +}
>> +
>> +struct mux_chip *devm_mux_chip_alloc(struct device *dev,
>> + unsigned int controllers,
>> + size_t sizeof_priv)
>> +{
>> + struct mux_chip **ptr, *mux_chip;
>> +
>> + ptr = devres_alloc(devm_mux_chip_release, sizeof(*ptr), GFP_KERNEL);
>> + if (!ptr)
>> + return NULL;
>> +
>> + mux_chip = mux_chip_alloc(dev, controllers, sizeof_priv);
>> + if (!mux_chip) {
>> + devres_free(ptr);
>> + return NULL;
>> + }
>> +
>> + *ptr = mux_chip;
>> + devres_add(dev, ptr);
>> +
>> + return mux_chip;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_mux_chip_alloc);
>
>
> Having devm functions that create/destroy other struct devices worries
> me, do we have other examples of this happening today? Are you sure you
> got the reference counting all correct?
drivers/iio/industrialio_core.c:devm_iio_device_alloc
Or is the iio case different in some subtle way that I'm missing?
>> +
>> +static int devm_mux_chip_match(struct device *dev, void *res, void *data)
>> +{
>> + struct mux_chip **r = res;
>> +
>> + if (WARN_ON(!r || !*r))
>
> How can this happen?
It shouldn't. I copied the pattern from the iio subsystem.
>> + return 0;
>> +
>> + return *r == data;
>> +}
>> +
>> +void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip)
>> +{
>> + WARN_ON(devres_release(dev, devm_mux_chip_release,
>> + devm_mux_chip_match, mux_chip));
>
> What can someone do with these WARN_ON() splats in the kernel log?
Don't know. Again, I copied the pattern from the iio subsystem.
>
>> +}
>> +EXPORT_SYMBOL_GPL(devm_mux_chip_free);
>> +
>> +static void devm_mux_chip_reg_release(struct device *dev, void *res)
>> +{
>> + struct mux_chip *mux_chip = *(struct mux_chip **)res;
>> +
>> + mux_chip_unregister(mux_chip);
>> +}
>> +
>> +int devm_mux_chip_register(struct device *dev,
>> + struct mux_chip *mux_chip)
>> +{
>> + struct mux_chip **ptr;
>> + int res;
>> +
>> + ptr = devres_alloc(devm_mux_chip_reg_release, sizeof(*ptr), GFP_KERNEL);
>> + if (!ptr)
>> + return -ENOMEM;
>> +
>> + res = mux_chip_register(mux_chip);
>> + if (res) {
>> + devres_free(ptr);
>> + return res;
>> + }
>> +
>> + *ptr = mux_chip;
>> + devres_add(dev, ptr);
>> +
>> + return res;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_mux_chip_register);
>> +
>> +void devm_mux_chip_unregister(struct device *dev, struct mux_chip *mux_chip)
>> +{
>> + WARN_ON(devres_release(dev, devm_mux_chip_reg_release,
>> + devm_mux_chip_match, mux_chip));
>> +}
>> +EXPORT_SYMBOL_GPL(devm_mux_chip_unregister);
>> +
>> +int mux_control_select(struct mux_control *mux, int state)
>> +{
>> + int ret;
>> +
>> + if (down_read_trylock(&mux->lock)) {
>> + if (mux->cached_state == state)
>> + return 0;
>> +
>> + /* Sigh, the mux needs updating... */
>> + up_read(&mux->lock);
>> + }
>> +
>> + /* ...or it's just contended. */
>> + down_write(&mux->lock);
>
> Why use a read/write lock at all? Have you tested this to verify it
> really is faster and needed?
For one of the HW configuration that drove the development, the same mux
controller is used to mux both an I2C channel and a couple of ADC lines.
If there is no kind of reader/writer locking going on, there is no way to
do ADC readings concurrently with an I2C transfer even when the consumers
want the mux in the same position. With an ordinary mutex controlling the
mux position, the consumers will unconditionally get serialized, which
seems like a waste to me. Or maybe I'm missing something?
>> +
>> + if (mux->cached_state == state) {
>> + /*
>> + * Hmmm, someone else changed the mux to my liking.
>> + * That makes me wonder how long I waited for nothing?
>> + */
>> + downgrade_write(&mux->lock);
>
> Oh that always scares me... Are you _sure_ this is correct? And
> needed?
It might not be needed, and it would probably work ok to just fall
through and call mux_control_set unconditionally. What is it that
always scares you exactly? Relying on cached state to be correct?
Downgrading writer locks?
>> + return 0;
>> + }
>> +
>> + ret = mux_control_set(mux, state);
>> + if (ret < 0) {
>> + if (mux->idle_state != MUX_IDLE_AS_IS)
>> + mux_control_set(mux, mux->idle_state);
>> +
>> + up_write(&mux->lock);
>> + return ret;
>> + }
>> +
>> + downgrade_write(&mux->lock);
>> +
>> + return 1;
>> +}
>> +EXPORT_SYMBOL_GPL(mux_control_select);
>> +
>> +int mux_control_deselect(struct mux_control *mux)
>> +{
>> + int ret = 0;
>> +
>> + if (mux->idle_state != MUX_IDLE_AS_IS &&
>> + mux->idle_state != mux->cached_state)
>> + ret = mux_control_set(mux, mux->idle_state);
>> +
>> + up_read(&mux->lock);
>
> You require a lock to be held for a "global" function? Without
> documentation? Or even a sparse marking? That's asking for trouble...
Documentation I can handle, but where should I look to understand how I
should add sparse markings?
The mux needs to be locked somehow. But as I stated in the cover letter
the rwsem isn't a perfect fit.
I'm using an rwsem to lock a mux, but that isn't really a
perfect fit. Is there a better locking primitive that I don't
know about that fits better? I had a mutex at one point, but
that didn't allow any concurrent accesses at all. At least
the rwsem allows concurrent access as long as all users
agree on the mux state, but I suspect that the rwsem will
degrade to the mutex situation pretty quickly if there is
any contention.
Also, the lock doesn't add anything if there is only one consumer of
a mux controller. Maybe there should be some mechanism for shortcutting
the locking for the (more common?) single-consumer case?
But again, I need the locking for my multi-consumer use case.
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(mux_control_deselect);
>> +
>> +static int of_dev_node_match(struct device *dev, const void *data)
>> +{
>> + return dev->of_node == data;
>> +}
>> +
>> +static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
>> +{
>> + struct device *dev;
>> +
>> + dev = class_find_device(&mux_class, NULL, np, of_dev_node_match);
>> +
>> + return dev ? to_mux_chip(dev) : NULL;
>> +}
>> +
>> +struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>> +{
>> + struct device_node *np = dev->of_node;
>> + struct of_phandle_args args;
>> + struct mux_chip *mux_chip;
>> + unsigned int controller;
>> + int index = 0;
>> + int ret;
>> +
>> + if (mux_name) {
>> + index = of_property_match_string(np, "mux-control-names",
>> + mux_name);
>> + if (index < 0) {
>> + dev_err(dev, "mux controller '%s' not found\n",
>> + mux_name);
>> + return ERR_PTR(index);
>> + }
>> + }
>> +
>> + ret = of_parse_phandle_with_args(np,
>> + "mux-controls", "#mux-control-cells",
>> + index, &args);
>> + if (ret) {
>> + dev_err(dev, "%s: failed to get mux-control %s(%i)\n",
>> + np->full_name, mux_name ?: "", index);
>> + return ERR_PTR(ret);
>> + }
>> +
>> + mux_chip = of_find_mux_chip_by_node(args.np);
>> + of_node_put(args.np);
>> + if (!mux_chip)
>> + return ERR_PTR(-EPROBE_DEFER);
>> +
>> + if (args.args_count > 1 ||
>> + (!args.args_count && (mux_chip->controllers > 1))) {
>> + dev_err(dev, "%s: wrong #mux-control-cells for %s\n",
>> + np->full_name, args.np->full_name);
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + controller = 0;
>> + if (args.args_count)
>> + controller = args.args[0];
>> +
>> + if (controller >= mux_chip->controllers) {
>> + dev_err(dev, "%s: bad mux controller %u specified in %s\n",
>> + np->full_name, controller, args.np->full_name);
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + get_device(&mux_chip->dev);
>> + return &mux_chip->mux[controller];
>> +}
>> +EXPORT_SYMBOL_GPL(mux_control_get);
>> +
>> +void mux_control_put(struct mux_control *mux)
>> +{
>> + put_device(&mux->chip->dev);
>> +}
>> +EXPORT_SYMBOL_GPL(mux_control_put);
>> +
>> +static void devm_mux_control_release(struct device *dev, void *res)
>> +{
>> + struct mux_control *mux = *(struct mux_control **)res;
>> +
>> + mux_control_put(mux);
>> +}
>> +
>> +struct mux_control *devm_mux_control_get(struct device *dev,
>> + const char *mux_name)
>> +{
>> + struct mux_control **ptr, *mux;
>> +
>> + ptr = devres_alloc(devm_mux_control_release, sizeof(*ptr), GFP_KERNEL);
>> + if (!ptr)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + mux = mux_control_get(dev, mux_name);
>> + if (IS_ERR(mux)) {
>> + devres_free(ptr);
>> + return mux;
>> + }
>> +
>> + *ptr = mux;
>> + devres_add(dev, ptr);
>> +
>> + return mux;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_mux_control_get);
>> +
>> +static int devm_mux_control_match(struct device *dev, void *res, void *data)
>> +{
>> + struct mux_control **r = res;
>> +
>> + if (WARN_ON(!r || !*r))
>> + return 0;
>
> Same here, how can this happen?
Same response as above.
>> +
>> + return *r == data;
>> +}
>> +
>> +void devm_mux_control_put(struct device *dev, struct mux_control *mux)
>> +{
>> + WARN_ON(devres_release(dev, devm_mux_control_release,
>> + devm_mux_control_match, mux));
>> +}
>> +EXPORT_SYMBOL_GPL(devm_mux_control_put);
>> +
>> +/*
>> + * Using subsys_initcall instead of module_init here to ensure - for the
>> + * non-modular case - that the subsystem is initialized when mux consumers
>> + * and mux controllers start to use it /without/ relying on link order.
>> + * For the modular case, the ordering is ensured with module dependencies.
>> + */
>> +subsys_initcall(mux_init);
>
> Even with subsys_initcall you are relying on link order, you do realize
> that? What about other subsystems that rely on this? :)
Yes, that is true, but if others start relying on this, that's their problem,
right? :-)
>
>> +
>> +MODULE_DESCRIPTION("Multiplexer subsystem");
>> +MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/mux/mux-gpio.c b/drivers/mux/mux-gpio.c
>> new file mode 100644
>> index 000000000000..227d3572e6db
>> --- /dev/null
>> +++ b/drivers/mux/mux-gpio.c
>> @@ -0,0 +1,114 @@
>> +/*
>> + * GPIO-controlled multiplexer driver
>> + *
>> + * Copyright (C) 2017 Axentia Technologies AB
>> + *
>> + * Author: Peter Rosin <peda@axentia.se>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/module.h>
>> +#include <linux/mux.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/property.h>
>> +
>> +struct mux_gpio {
>> + struct gpio_descs *gpios;
>> + int *val;
>> +};
>> +
>> +static int mux_gpio_set(struct mux_control *mux, int state)
>> +{
>> + struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
>> + int i;
>> +
>> + for (i = 0; i < mux_gpio->gpios->ndescs; i++)
>> + mux_gpio->val[i] = (state >> i) & 1;
>> +
>> + gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
>> + mux_gpio->gpios->desc,
>> + mux_gpio->val);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct mux_control_ops mux_gpio_ops = {
>> + .set = mux_gpio_set,
>> +};
>> +
>> +static const struct of_device_id mux_gpio_dt_ids[] = {
>> + { .compatible = "gpio-mux", },
>> + { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, mux_gpio_dt_ids);
>> +
>> +static int mux_gpio_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct mux_chip *mux_chip;
>> + struct mux_gpio *mux_gpio;
>> + int pins;
>> + s32 idle_state;
>> + int ret;
>> +
>> + pins = gpiod_count(dev, "mux");
>> + if (pins < 0)
>> + return pins;
>> +
>> + mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_gpio) +
>> + pins * sizeof(*mux_gpio->val));
>> + if (!mux_chip)
>> + return -ENOMEM;
>> +
>> + mux_gpio = mux_chip_priv(mux_chip);
>> + mux_gpio->val = (int *)(mux_gpio + 1);
>> + mux_chip->ops = &mux_gpio_ops;
>> +
>> + mux_gpio->gpios = devm_gpiod_get_array(dev, "mux", GPIOD_OUT_LOW);
>> + if (IS_ERR(mux_gpio->gpios)) {
>> + ret = PTR_ERR(mux_gpio->gpios);
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(dev, "failed to get gpios\n");
>> + return ret;
>> + }
>> + WARN_ON(pins != mux_gpio->gpios->ndescs);
>> + mux_chip->mux->states = 1 << pins;
>> +
>> + ret = device_property_read_u32(dev, "idle-state", (u32 *)&idle_state);
>> + if (ret >= 0 && idle_state != MUX_IDLE_AS_IS) {
>> + if (idle_state < 0 || idle_state >= mux_chip->mux->states) {
>> + dev_err(dev, "invalid idle-state %u\n", idle_state);
>> + return -EINVAL;
>> + }
>> +
>> + mux_chip->mux->idle_state = idle_state;
>> + }
>> +
>> + ret = devm_mux_chip_register(dev, mux_chip);
>> + if (ret < 0)
>> + return ret;
>> +
>> + dev_info(dev, "%u-way mux-controller registered\n",
>> + mux_chip->mux->states);
>> +
>> + return 0;
>> +}
>> +
>> +static struct platform_driver mux_gpio_driver = {
>> + .driver = {
>> + .name = "gpio-mux",
>> + .of_match_table = of_match_ptr(mux_gpio_dt_ids),
>> + },
>> + .probe = mux_gpio_probe,
>> +};
>> +module_platform_driver(mux_gpio_driver);
>> +
>> +MODULE_DESCRIPTION("GPIO-controlled multiplexer driver");
>> +MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/mux.h b/include/linux/mux.h
>> new file mode 100644
>> index 000000000000..febdde4246df
>> --- /dev/null
>> +++ b/include/linux/mux.h
>> @@ -0,0 +1,252 @@
>> +/*
>> + * mux.h - definitions for the multiplexer interface
>> + *
>> + * Copyright (C) 2017 Axentia Technologies AB
>> + *
>> + * Author: Peter Rosin <peda@axentia.se>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef _LINUX_MUX_H
>> +#define _LINUX_MUX_H
>> +
>> +#include <linux/device.h>
>> +#include <linux/rwsem.h>
>> +
>> +struct mux_chip;
>> +struct mux_control;
>> +struct platform_device;
>> +
>> +/**
>> + * struct mux_control_ops - Mux controller operations for a mux chip.
>> + * @set: Set the state of the given mux controller.
>> + */
>> +struct mux_control_ops {
>> + int (*set)(struct mux_control *mux, int state);
>> +};
>> +
>> +/* These defines match the constants from the dt-bindings. On purpose. */
>
> Why on purpose?
I sure wasn't an accident? :-)
Want me to remove it?
>> +#define MUX_IDLE_AS_IS (-1)
>> +#define MUX_IDLE_DISCONNECT (-2)
>> +
>> +/**
>> + * struct mux_control - Represents a mux controller.
>> + * @lock: Protects the mux controller state.
>> + * @chip: The mux chip that is handling this mux controller.
>> + * @states: The number of mux controller states.
>> + * @cached_state: The current mux controller state, or -1 if none.
>> + * @idle_state: The mux controller state to use when inactive, or one
>> + * of MUX_IDLE_AS_IS and MUX_IDLE_DISCONNECT.
>> + */
>> +struct mux_control {
>> + struct rw_semaphore lock; /* protects the state of the mux */
>> +
>> + struct mux_chip *chip;
>> +
>> + unsigned int states;
>> + int cached_state;
>> + int idle_state;
>> +};
>> +
>> +/**
>> + * struct mux_chip - Represents a chip holding mux controllers.
>> + * @controllers: Number of mux controllers handled by the chip.
>> + * @mux: Array of mux controllers that are handled.
>> + * @dev: Device structure.
>> + * @id: Used to identify the device internally.
>> + * @ops: Mux controller operations.
>> + */
>> +struct mux_chip {
>> + unsigned int controllers;
>> + struct mux_control *mux;
>> + struct device dev;
>> + int id;
>> +
>> + const struct mux_control_ops *ops;
>> +};
>> +
>> +#define to_mux_chip(x) container_of((x), struct mux_chip, dev)
>> +
>> +/**
>> + * mux_chip_priv() - Get the extra memory reserved by mux_chip_alloc().
>> + * @mux_chip: The mux-chip to get the private memory from.
>> + *
>> + * Return: Pointer to the private memory reserved by the allocator.
>> + */
>> +static inline void *mux_chip_priv(struct mux_chip *mux_chip)
>> +{
>> + return &mux_chip->mux[mux_chip->controllers];
>> +}
>> +
>> +/**
>> + * mux_chip_alloc() - Allocate a mux-chip.
>> + * @dev: The parent device implementing the mux interface.
>> + * @controllers: The number of mux controllers to allocate for this chip.
>> + * @sizeof_priv: Size of extra memory area for private use by the caller.
>> + *
>> + * Return: A pointer to the new mux-chip, NULL on failure.
>> + */
>> +struct mux_chip *mux_chip_alloc(struct device *dev,
>> + unsigned int controllers, size_t sizeof_priv);
>> +
>
> Don't put kernel doc comments in a .h file, they normally go into the .c
> file, next to the code itself. That makes it easier to fix up and
> realise when they need to be changed when the code changes. The .h file
> rarely changes.
I'll move the lot over.
>> +/**
>> + * mux_chip_register() - Register a mux-chip, thus readying the controllers
>> + * for use.
>> + * @mux_chip: The mux-chip to register.
>> + *
>> + * Do not retry registration of the same mux-chip on failure. You should
>> + * instead put it away with mux_chip_free() and allocate a new one, if you
>> + * for some reason would like to retry registration.
>> + *
>> + * Return: Zero on success or a negative errno on error.
>> + */
>> +int mux_chip_register(struct mux_chip *mux_chip);
>> +
>> +/**
>> + * mux_chip_unregister() - Take the mux-chip off-line.
>> + * @mux_chip: The mux-chip to unregister.
>> + *
>> + * mux_chip_unregister() reverses the effects of mux_chip_register().
>> + * But not completely, you should not try to call mux_chip_register()
>> + * on a mux-chip that has been registered before.
>> + */
>> +void mux_chip_unregister(struct mux_chip *mux_chip);
>> +
>> +/**
>> + * mux_chip_free() - Free the mux-chip for good.
>> + * @mux_chip: The mux-chip to free.
>> + *
>> + * mux_chip_free() reverses the effects of mux_chip_alloc().
>> + */
>> +void mux_chip_free(struct mux_chip *mux_chip);
>> +
>> +/**
>> + * devm_mux_chip_alloc() - Resource-managed version of mux_chip_alloc().
>> + * @dev: The parent device implementing the mux interface.
>> + * @controllers: The number of mux controllers to allocate for this chip.
>> + * @sizeof_priv: Size of extra memory area for private use by the caller.
>> + *
>> + * See mux_chip_alloc() for more details.
>> + *
>> + * Return: A pointer to the new mux-chip, NULL on failure.
>> + */
>> +struct mux_chip *devm_mux_chip_alloc(struct device *dev,
>> + unsigned int controllers,
>> + size_t sizeof_priv);
>> +
>> +/**
>> + * devm_mux_chip_register() - Resource-managed version mux_chip_register().
>> + * @dev: The parent device implementing the mux interface.
>> + * @mux_chip: The mux-chip to register.
>> + *
>> + * See mux_chip_register() for more details.
>> + *
>> + * Return: Zero on success or a negative errno on error.
>> + */
>> +int devm_mux_chip_register(struct device *dev, struct mux_chip *mux_chip);
>> +
>> +/**
>> + * devm_mux_chip_unregister() - Resource-managed version mux_chip_unregister().
>> + * @dev: The device that originally registered the mux-chip.
>> + * @mux_chip: The mux-chip to unregister.
>> + *
>> + * See mux_chip_unregister() for more details.
>> + *
>> + * Note that you do not normally need to call this function.
>
> Odd, then why is it exported???
You normally don't call the devm_foo_{free,release,unregister,etc} functions.
The intention is of course that the resourse cleans up automatically. But there
are no cases where the manual clean up is not available, at least not that I can
find?
>> + */
>> +void devm_mux_chip_unregister(struct device *dev, struct mux_chip *mux_chip);
>> +
>> +/**
>> + * devm_mux_chip_free() - Resource-managed version mux_chip_free().
>> + * @dev: The device that originally got the mux-chip.
>> + * @mux_chip: The mux-chip to free.
>> + *
>> + * See mux_chip_free() for more details.
>> + *
>> + * Note that you do not normally need to call this function.
>> + */
>> +void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip);
>> +
>> +/**
>> + * mux_control_select() - Select the given multiplexer state.
>> + * @mux: The mux-control to request a change of state from.
>> + * @state: The new requested state.
>> + *
>> + * Make sure to call mux_control_deselect() when the operation is complete and
>> + * the mux-control is free for others to use, but do not call
>> + * mux_control_deselect() if mux_control_select() fails.
>> + *
>> + * Return: 0 if the requested state was already active, or 1 it the
>> + * mux-control state was changed to the requested state. Or a negative
>> + * errno on error.
>> + *
>> + * Note that the difference in return value of zero or one is of
>> + * questionable value; especially if the mux-control has several independent
>> + * consumers, which is something the consumers should perhaps not be making
>> + * assumptions about.
>
> I don't understand this note, what is a user of this api supposed to do
> differently between 1 and 0? Why make the difference at all?
If the consumer somehow *knows* that it is the only user of a mux controller,
it can use the return value to shortcut (perhaps costly) actions only needed
when the mux changes. The 1/0 difference was also a "free" extra given the
current implementation of mux_control_select. But it's cheep for the consumer
to keep track of this by itself if it needs it.
It's when there are several (independent) consumers of a mux controller that
the information in the return value is questionable and can't be used to
shortcut actions only needed on mux changes.
Now that you point the finger at it and I have been made to spell out the
problem, I think it's probably wise to remove the distiction so that users
do not start to use the return value when they shouldn't. It's generally
better to keep track of the expected mux state in the consumer and use that
local information to shortcut those (perhaps costly) actions instead.
> And I agree with the comment to split this up into 2 different .h files,
> if possible.
Will split.
> thanks,
>
> greg k-h
>
I'll wait for further feedback before posting a v14.
Cheers and thanks,
peda
^ permalink raw reply
* Re: [PATCH v4 05/11] drm/sun4i: abstract a engine type
From: Icenowy Zheng @ 2017-04-18 11:05 UTC (permalink / raw)
To: Maxime Ripard
Cc: Rob Herring, Chen-Yu Tsai, David Airlie, Jernej Skrabec,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20170418085548.4cisone2yfcuizcp@lukather>
于 2017年4月18日 GMT+08:00 下午4:55:48, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 写到:
>Hi,
>
>Thanks for that rework.
>
>On Sun, Apr 16, 2017 at 08:08:43PM +0800, Icenowy Zheng wrote:
>> As we are going to add support for the Allwinner DE2 engine in
>sun4i-drm
>> driver, we will finally have two types of display engines -- the DE1
>> backend and the DE2 mixer. They both do some display blending and
>feed
>> graphics data to TCON, so I choose to call them both "engine" here.
>>
>> Abstract the engine type to void * and a ops struct, which contains
>> functions that should be called outside the engine-specified code (in
>> TCON, CRTC or TV Encoder code).
>>
>> A dedicated Kconfig option is also added to control whether
>> sun4i-backend-specified code (sun4i_backend.c and sun4i_layer.c)
>should
>> be built. As we removed the codes in CRTC code that directly call the
>> layer code, we can now extract the layer part and combine it with the
>> backend part into a new module, sun4i-backend.ko.
>>
>> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
>> ---
>> Changes in v4:
>> - Comments to tag the color correction functions as optional.
>> - Check before calling the optional functions.
>> - Change layers_init to satisfy new PATCH v4 04/11.
>>
>> drivers/gpu/drm/sun4i/Kconfig | 10 ++++++++++
>> drivers/gpu/drm/sun4i/Makefile | 6 ++++--
>> drivers/gpu/drm/sun4i/sun4i_backend.c | 26
>+++++++++++++++++++-------
>> drivers/gpu/drm/sun4i/sun4i_backend.h | 5 -----
>> drivers/gpu/drm/sun4i/sun4i_crtc.c | 14 +++++++-------
>> drivers/gpu/drm/sun4i/sun4i_crtc.h | 7 ++++---
>> drivers/gpu/drm/sun4i/sun4i_drv.h | 3 ++-
>> drivers/gpu/drm/sun4i/sun4i_layer.c | 2 +-
>> drivers/gpu/drm/sun4i/sun4i_layer.h | 3 ++-
>> drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +-
>> drivers/gpu/drm/sun4i/sun4i_tv.c | 11 ++++++-----
>> drivers/gpu/drm/sun4i/sunxi_engine.h | 35
>+++++++++++++++++++++++++++++++++++
>> 12 files changed, 91 insertions(+), 33 deletions(-)
>> create mode 100644 drivers/gpu/drm/sun4i/sunxi_engine.h
>>
>> diff --git a/drivers/gpu/drm/sun4i/Kconfig
>b/drivers/gpu/drm/sun4i/Kconfig
>> index a4b357db8856..5a8227f37cc4 100644
>> --- a/drivers/gpu/drm/sun4i/Kconfig
>> +++ b/drivers/gpu/drm/sun4i/Kconfig
>> @@ -12,3 +12,13 @@ config DRM_SUN4I
>> Choose this option if you have an Allwinner SoC with a
>> Display Engine. If M is selected the module will be called
>> sun4i-drm.
>> +
>> +config DRM_SUN4I_BACKEND
>> + tristate "Support for Allwinner A10 Display Engine Backend"
>> + depends on DRM_SUN4I
>> + default DRM_SUN4I
>> + help
>> + Choose this option if you have an Allwinner SoC with the
>> + original Allwinner Display Engine, which has a backend to
>> + do some alpha blending and feed graphics to TCON. If M is
>> + selected the module will be called sun4i-backend.
>> diff --git a/drivers/gpu/drm/sun4i/Makefile
>b/drivers/gpu/drm/sun4i/Makefile
>> index 59b757350a1f..1db1068b9be1 100644
>> --- a/drivers/gpu/drm/sun4i/Makefile
>> +++ b/drivers/gpu/drm/sun4i/Makefile
>> @@ -5,9 +5,11 @@ sun4i-tcon-y += sun4i_tcon.o
>> sun4i-tcon-y += sun4i_rgb.o
>> sun4i-tcon-y += sun4i_dotclock.o
>> sun4i-tcon-y += sun4i_crtc.o
>> -sun4i-tcon-y += sun4i_layer.o
>> +
>> +sun4i-backend-y += sun4i_layer.o
>> +sun4i-backend-y += sun4i_backend.o
>>
>> obj-$(CONFIG_DRM_SUN4I) += sun4i-drm.o sun4i-tcon.o
>> -obj-$(CONFIG_DRM_SUN4I) += sun4i_backend.o
>> +obj-$(CONFIG_DRM_SUN4I_BACKEND) += sun4i-backend.o
>> obj-$(CONFIG_DRM_SUN4I) += sun6i_drc.o
>> obj-$(CONFIG_DRM_SUN4I) += sun4i_tv.o
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c
>b/drivers/gpu/drm/sun4i/sun4i_backend.c
>> index d660741ba475..a16c96a002a4 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
>> @@ -23,6 +23,8 @@
>>
>> #include "sun4i_backend.h"
>> #include "sun4i_drv.h"
>> +#include "sun4i_layer.h"
>> +#include "sunxi_engine.h"
>>
>> static const u32 sunxi_rgb2yuv_coef[12] = {
>> 0x00000107, 0x00000204, 0x00000064, 0x00000108,
>> @@ -30,9 +32,10 @@ static const u32 sunxi_rgb2yuv_coef[12] = {
>> 0x000001c1, 0x00003e88, 0x00003fb8, 0x00000808
>> };
>>
>> -void sun4i_backend_apply_color_correction(struct sun4i_backend
>*backend)
>> +static void sun4i_backend_apply_color_correction(void *engine)
>> {
>> int i;
>> + struct sun4i_backend *backend = engine;
>
>I'm not really fond of passing an opaque pointer here, and hope that
>things will work out.
>
>I think having a common structure, holding the common thingsand a more
>specific structure for that one would work better.
>
>Something like
>
>struct sunxi_engine {
> struct regmap *regs;
>};
>
>struct sun4i_backend {
> struct sunxi_engine engine;
>
> struct clk *sat_clk;
> struct reset_control *sat_reset;
>
>};
Sounds good ;-)
>
>static void sun4i_backend_apply_color_correction(struct sunxi_engine
>*engine)
>struct sun4i_backend *backend = container_of(engine, struct
>sun4i_backend, engine);
>
>...
>
>> +static const struct sunxi_engine_ops sun4i_backend_engine_ops = {
>> + .commit = sun4i_backend_commit,
>> + .layers_init = sun4i_layers_init,
>> + .apply_color_correction = sun4i_backend_apply_color_correction,
>> + .disable_color_correction = sun4i_backend_disable_color_correction,
>
>Please align the values with tabs, like done in the other structures
>created that way in this driver.
>
>> static void sun4i_crtc_atomic_begin(struct drm_crtc *crtc,
>> @@ -56,7 +55,7 @@ static void sun4i_crtc_atomic_flush(struct drm_crtc
>*crtc,
>>
>> DRM_DEBUG_DRIVER("Committing plane changes\n");
>>
>> - sun4i_backend_commit(scrtc->backend);
>> + scrtc->engine_ops->commit(scrtc->engine);
>
>You rely on the backend having setup things properly, which is pretty
>fragile. Ideally, you should have a function to check that engine_ops
>and commit is !NULL, and call it, and the consumers would use that
>function...
If it's really NULL how should the function return?
>
>> @@ -362,7 +361,9 @@ static void sun4i_tv_disable(struct drm_encoder
>*encoder)
>> regmap_update_bits(tv->regs, SUN4I_TVE_EN_REG,
>> SUN4I_TVE_EN_ENABLE,
>> 0);
>> - sun4i_backend_disable_color_correction(backend);
>> +
>> + if (crtc->engine_ops->disable_color_correction)
>> + crtc->engine_ops->disable_color_correction(crtc->engine);
>> }
>
>... Instead of having to do it in some cases, but not always ...
>
>> static void sun4i_tv_enable(struct drm_encoder *encoder)
>> @@ -370,11 +371,11 @@ static void sun4i_tv_enable(struct drm_encoder
>*encoder)
>> struct sun4i_tv *tv = drm_encoder_to_sun4i_tv(encoder);
>> struct sun4i_crtc *crtc = drm_crtc_to_sun4i_crtc(encoder->crtc);
>> struct sun4i_tcon *tcon = crtc->tcon;
>> - struct sun4i_backend *backend = crtc->backend;
>>
>> DRM_DEBUG_DRIVER("Enabling the TV Output\n");
>>
>> - sun4i_backend_apply_color_correction(backend);
>> + if (crtc->engine_ops->apply_color_correction)
>> + crtc->engine_ops->apply_color_correction(crtc->engine);
>>
>> regmap_update_bits(tv->regs, SUN4I_TVE_EN_REG,
>> SUN4I_TVE_EN_ENABLE,
>> diff --git a/drivers/gpu/drm/sun4i/sunxi_engine.h
>b/drivers/gpu/drm/sun4i/sunxi_engine.h
>> new file mode 100644
>> index 000000000000..a9128abda66f
>> --- /dev/null
>> +++ b/drivers/gpu/drm/sun4i/sunxi_engine.h
>> @@ -0,0 +1,35 @@
>> +/*
>> + * Copyright (C) 2017 Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + */
>> +
>> +#ifndef _SUNXI_ENGINE_H_
>> +#define _SUNXI_ENGINE_H_
>> +
>> +struct sun4i_crtc;
>> +
>> +struct sunxi_engine_ops {
>> + /* Commit the changes to the engine */
>> + void (*commit)(void *engine);
>> + /* Initialize layers (planes) for this engine */
>> + struct drm_plane **(*layers_init)(struct drm_device *drm,
>> + struct sun4i_crtc *crtc);
>> +
>> + /*
>> + * These are optional functions for the TV Encoder. Please check
>> + * their presence before calling them.
>> + *
>> + * The first function applies the color space correction needed
>> + * for outputing correct TV signal.
>> + *
>> + * The second function disabled the correction.
>> + */
>> + void (*apply_color_correction)(void *engine);
>> + void (*disable_color_correction)(void *engine);
>
>... And have to document it.
>
>Please also use kerneldoc for those comments.
>
>Thanks again!
>Maxime
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
From: Greg Kroah-Hartman @ 2017-04-18 11:44 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Wolfram Sang, Rob Herring,
Mark Rutland, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA, Andrew Morton, Colin Ian King,
Paul Gortmaker, Philipp Zabel, kernel-bIcnvbaLZ9MEGnE8C9+IrQ
In-Reply-To: <bdeecccf-02d6-226b-8516-1d41e3602a7a-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
On Tue, Apr 18, 2017 at 12:59:50PM +0200, Peter Rosin wrote:
> On 2017-04-18 10:51, Greg Kroah-Hartman wrote:
> > On Thu, Apr 13, 2017 at 06:43:07PM +0200, Peter Rosin wrote:
> >> +config MUX_GPIO
> >> + tristate "GPIO-controlled Multiplexer"
> >> + depends on OF && GPIOLIB
> >
> > Why have the gpio and mux core in the same patch?
>
> One is not usable w/o the other. I can split them if you want to?
Then why are they two different config options? Add the core code in
one patch, and then add the gpio controled mutiplexer in a separate
patch.
> >> +static struct class mux_class = {
> >> + .name = "mux",
> >> + .owner = THIS_MODULE,
> >> +};
> >
> > No Documentation/ABI/ update for your sysfs files? Please do so.
>
> Ok I'll look into it. Wasn't even aware that I added any. But there's the
> new class of course...
Hint, you have files, the devices that belong to the class :)
> >> +static int __init mux_init(void)
> >> +{
> >> + return class_register(&mux_class);
> >> +}
> >> +
> >> +static DEFINE_IDA(mux_ida);
> >
> > When your module is unloaded, you forgot to clean this structure up with
> > what was done with it.
>
> I was under the impression that not providing an exit function for modules
> made the module infrastructure prevent unloading (by bumping some reference
> counter). Maybe that is a misconception?
Ah, messy, don't do that. Make it so you can unload your module please,
why wouldn't you want that to happen?
> >> + mux_chip = kzalloc(sizeof(*mux_chip) +
> >> + controllers * sizeof(*mux_chip->mux) +
> >> + sizeof_priv, GFP_KERNEL);
> >> + if (!mux_chip)
> >> + return NULL;
> >
> > You don't return PTR_ERR(-ENOMEM)? Ok, why not? (I'm not arguing for
> > it, just curious...)
>
> There's no particular reason. Do you think I should change it?
What does the caller do with an error? Pass it up to where? Who gets
it? Don't you want the caller to know you are out of memory?
> >> +
> >> + device_initialize(&mux_chip->dev);
> >
> > Why are you not registering the device here as well? Why have this be a
> > two step process?
>
> Because of idle state handling. The drivers are expected to fill in
> the desired idle state(s) after allocating the mux controller(s).
> Then, when registering, the desired idle state is activated (if the
> idle state is not idle-as-is, of course) and as a last step the mux
> is "advertised".
Ok, is that documented in the functions somewhere?
> >> + ret = device_add(&mux_chip->dev);
> >> + if (ret < 0)
> >> + dev_err(&mux_chip->dev,
> >> + "device_add failed in mux_chip_register: %d\n", ret);
> >
> > Did you run checkpatch.pl in strict mode on this new file? Please do so :)
>
> I did, and did it again just to be sure, and I do not get any complaints.
> So, what's wrong?
You list the function name in the printk string, it should complain
that __func__ should be used. Oh well, it's just a perl script, it
doesn't always catch everything.
isn't always correct :)
> >> +EXPORT_SYMBOL_GPL(devm_mux_chip_alloc);
> >
> >
> > Having devm functions that create/destroy other struct devices worries
> > me, do we have other examples of this happening today? Are you sure you
> > got the reference counting all correct?
>
> drivers/iio/industrialio_core.c:devm_iio_device_alloc
>
> Or is the iio case different in some subtle way that I'm missing?
I don't know, hopefully you got it all correct, I haven't audited that
code path in a long time :)
> >> +
> >> +static int devm_mux_chip_match(struct device *dev, void *res, void *data)
> >> +{
> >> + struct mux_chip **r = res;
> >> +
> >> + if (WARN_ON(!r || !*r))
> >
> > How can this happen?
>
> It shouldn't. I copied the pattern from the iio subsystem.
Then it should be removed there too...
> >> +void devm_mux_chip_free(struct device *dev, struct mux_chip *mux_chip)
> >> +{
> >> + WARN_ON(devres_release(dev, devm_mux_chip_release,
> >> + devm_mux_chip_match, mux_chip));
> >
> > What can someone do with these WARN_ON() splats in the kernel log?
>
> Don't know. Again, I copied the pattern from the iio subsystem.
If you don't know what it should be used for, don't copy it!
Cargo-cult coding is horrible, please no.
> >> + /* ...or it's just contended. */
> >> + down_write(&mux->lock);
> >
> > Why use a read/write lock at all? Have you tested this to verify it
> > really is faster and needed?
>
> For one of the HW configuration that drove the development, the same mux
> controller is used to mux both an I2C channel and a couple of ADC lines.
>
> If there is no kind of reader/writer locking going on, there is no way to
> do ADC readings concurrently with an I2C transfer even when the consumers
> want the mux in the same position. With an ordinary mutex controlling the
> mux position, the consumers will unconditionally get serialized, which
> seems like a waste to me. Or maybe I'm missing something?
Why is serializing things a "waste"? Again, a rw semaphore is slower,
takes more logic to get correct, and is very complex at times. If you
are not SURE you need it, and that it matters, don't use it. And if you
do use it, document the heck out of it how you need it and why.
> >> +
> >> + if (mux->cached_state == state) {
> >> + /*
> >> + * Hmmm, someone else changed the mux to my liking.
> >> + * That makes me wonder how long I waited for nothing?
> >> + */
> >> + downgrade_write(&mux->lock);
> >
> > Oh that always scares me... Are you _sure_ this is correct? And
> > needed?
>
> It might not be needed, and it would probably work ok to just fall
> through and call mux_control_set unconditionally. What is it that
> always scares you exactly? Relying on cached state to be correct?
> Downgrading writer locks?
downgrading a writer lock scares me, especially for something as
"simple" as this type of interface. Again, don't use it unless you
_have_ to. Simple is good, start with that always.
> >> + if (mux->idle_state != MUX_IDLE_AS_IS &&
> >> + mux->idle_state != mux->cached_state)
> >> + ret = mux_control_set(mux, mux->idle_state);
> >> +
> >> + up_read(&mux->lock);
> >
> > You require a lock to be held for a "global" function? Without
> > documentation? Or even a sparse marking? That's asking for trouble...
>
> Documentation I can handle, but where should I look to understand how I
> should add sparse markings?
Run sparse on the code and see what it says :)
> The mux needs to be locked somehow. But as I stated in the cover letter
> the rwsem isn't a perfect fit.
>
> I'm using an rwsem to lock a mux, but that isn't really a
> perfect fit. Is there a better locking primitive that I don't
> know about that fits better? I had a mutex at one point, but
> that didn't allow any concurrent accesses at all. At least
> the rwsem allows concurrent access as long as all users
> agree on the mux state, but I suspect that the rwsem will
> degrade to the mutex situation pretty quickly if there is
> any contention.
>
> Also, the lock doesn't add anything if there is only one consumer of
> a mux controller. Maybe there should be some mechanism for shortcutting
> the locking for the (more common?) single-consumer case?
>
> But again, I need the locking for my multi-consumer use case.
Go back to a mutex, and having a function that requires it to be held
is, icky.
> >> +/*
> >> + * Using subsys_initcall instead of module_init here to ensure - for the
> >> + * non-modular case - that the subsystem is initialized when mux consumers
> >> + * and mux controllers start to use it /without/ relying on link order.
> >> + * For the modular case, the ordering is ensured with module dependencies.
> >> + */
> >> +subsys_initcall(mux_init);
> >
> > Even with subsys_initcall you are relying on link order, you do realize
> > that? What about other subsystems that rely on this? :)
>
> Yes, that is true, but if others start relying on this, that's their problem,
> right? :-)
Yes, but no need to document something that isn't true. You are relying
on link order here.
> >> +struct mux_control_ops {
> >> + int (*set)(struct mux_control *mux, int state);
> >> +};
> >> +
> >> +/* These defines match the constants from the dt-bindings. On purpose. */
> >
> > Why on purpose?
>
> I sure wasn't an accident? :-)
>
> Want me to remove it?
At least explain _why_ you are doing this, that would help, right?
thanks,
greg k-h
^ permalink raw reply
* Re: [RFC 1/2] dt-bindings: add mmio-based syscon mux controller DT bindings
From: Pavel Machek @ 2017-04-18 11:51 UTC (permalink / raw)
To: Sakari Ailus
Cc: Philipp Zabel, Peter Rosin, Rob Herring, Mark Rutland,
Steve Longerbeam, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ
In-Reply-To: <20170418105540.GH7456-S+BSfZ9RZZmRSg0ZkenSGLdO1Tsj/99ntUK59QYPAWc@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 4139 bytes --]
On Tue 2017-04-18 13:55:40, Sakari Ailus wrote:
> On Tue, Apr 18, 2017 at 12:34:53PM +0200, Pavel Machek wrote:
> > On Tue 2017-04-18 13:08:41, Sakari Ailus wrote:
> > > Hi Philipp,
> > >
> > > On Tue, Apr 18, 2017 at 10:19:04AM +0200, Philipp Zabel wrote:
> > > > On Thu, 2017-04-13 at 17:48 +0200, Philipp Zabel wrote:
> > > > > This adds device tree binding documentation for mmio-based syscon
> > > > > multiplexers controlled by a single bitfield in a syscon register
> > > > > range.
> > > > >
> > > > > Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > > > ---
> > > > > Documentation/devicetree/bindings/mux/mmio-mux.txt | 56 ++++++++++++++++++++++
> > > > > 1 file changed, 56 insertions(+)
> > > > > create mode 100644 Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/mux/mmio-mux.txt b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > > > new file mode 100644
> > > > > index 0000000000000..11d96f5d98583
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/mux/mmio-mux.txt
> > > > > @@ -0,0 +1,56 @@
> > > > > +MMIO bitfield-based multiplexer controller bindings
> > > > > +
> > > > > +Define a syscon bitfield to be used to control a multiplexer. The parent
> > > > > +device tree node must be a syscon node to provide register access.
> > > > > +
> > > > > +Required properties:
> > > > > +- compatible : "gpio-mux"
> > > > > +- reg : register base of the register containing the control bitfield
> > > > > +- bit-mask : bitmask of the control bitfield in the control register
> > > > > +- bit-shift : bit offset of the control bitfield in the control register
> > > > > +- #mux-control-cells : <0>
> > > > > +* Standard mux-controller bindings as decribed in mux-controller.txt
> > > > > +
> > > > > +Optional properties:
> > > > > +- idle-state : if present, the state the mux will have when idle. The
> > > > > + special state MUX_IDLE_AS_IS is the default.
> > > > > +
> > > > > +The multiplexer state is defined as the value of the bitfield described
> > > > > +by the reg, bit-mask, and bit-shift properties, accessed through the parent
> > > > > +syscon.
> > > > > +
> > > > > +Example:
> > > > > +
> > > > > + syscon {
> > > > > + compatible = "syscon";
> > > > > +
> > > > > + mux: mux-controller@3 {
> > > > > + compatible = "mmio-mux";
> > > > > + reg = <0x3>;
> > > > > + bit-mask = <0x1>;
> > > > > + bit-shift = <5>;
> > > > > + #mux-control-cells = <0>;
> > > > > + };
> > > > > + };
> > > > > +
> > > > > + video-mux {
> > > > > + compatible = "video-mux";
> > > > > + mux-controls = <&mux>;
> > > > > +
> > > > > + ports {
> > > > > + /* input 0 */
> > > > > + port@0 {
> > > > > + reg = <0>;
> > > > > + };
> > > > > +
> > > > > + /* input 1 */
> > > > > + port@1 {
> > > > > + reg = <1>;
> > > > > + };
> > > > > +
> > > > > + /* output */
> > > > > + port@2 {
> > > > > + reg = <2>;
> > > > > + };
> > > > > + };
> > > > > + };
> > > >
> > > > So Pavel (added to Cc:) suggested to merge these into one node for the
> > > > video mux, as really we are describing a single hardware entity that
> > > > happens to be multiplexing multiple video buses into one:
> > >
> > > Two drivers will be needed in a way or another to disconnect the dependency
> > > between the video switch driver and the MUX implementation. Are there ways
> > > to do that cleanly other than having two devices?
> >
> > Yes.
> >
> > Device tree describes hardware, not the driver structure.
>
> I think you you could view the MUX control as a device, too, and that's
> separate from the actual video switch.
Actually, I believe what matters here is hardware. There's some chip,
somewhere, that does the switching, and the device tree should should
basically describe that switch.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 09/12] mfd: axp20x: add axp20x-regulator cell for AXP803
From: Chen-Yu Tsai @ 2017-04-18 11:58 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Chen-Yu Tsai, Lee Jones, Rob Herring, Maxime Ripard,
Liam Girdwood, Mark Brown, devicetree, linux-kernel,
linux-arm-kernel, linux-sunxi
In-Reply-To: <6D57E4F4-DD87-400F-A21B-85778434AADC-h8G6r0blFSE@public.gmane.org>
On Tue, Apr 18, 2017 at 6:55 PM, Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org> wrote:
>
>
> 于 2017年4月18日 GMT+08:00 下午6:38:09, Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> 写到:
>>On Mon, Apr 17, 2017 at 7:57 PM, Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org> wrote:
>>> As axp20x-regulator now supports AXP803, add a cell for it.
>>>
>>> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
>>> ---
>>> Changes in v3:
>>> - Make the new cell one-liner.
>>>
>>> drivers/mfd/axp20x.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
>>> index 1dc6235778eb..431b7f118606 100644
>>> --- a/drivers/mfd/axp20x.c
>>> +++ b/drivers/mfd/axp20x.c
>>> @@ -848,7 +848,8 @@ static struct mfd_cell axp803_cells[] = {
>>> .name = "axp20x-pek",
>>> .num_resources =
>>ARRAY_SIZE(axp803_pek_resources),
>>> .resources = axp803_pek_resources,
>>> - }
>>> + },
>>> + { .name = "axp20x-regulator" }
>>
>>It's best to have a trailing comma, so we don't have to change the line
>>again when we add more cells, like you just did with the previous line.
>
> Should I also add it in previous mfd patch? (and remove the addition of the comma in this patch)
Since Lee already said he merged it, I suggest you keep it the way it is,
unless he says otherwise. Or your new patches might not apply correctly,
and you waste time doing one more round.
Maintainers don't always push the latest branches, for a number of reasons.
Sometimes it's because the merge window is closed, and you're not supposed
to put anything that's not for the next release into -next. Other times it
might because they are still working through their backlog. Or they may
have just forgotten.
ChenYu
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: [PATCH v3 08/12] regulator: axp20x-regulator: add support for AXP803
From: Chen-Yu Tsai @ 2017-04-18 12:07 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Lee Jones, Rob Herring, Chen-Yu Tsai, Maxime Ripard,
Liam Girdwood, Mark Brown, devicetree, linux-kernel,
linux-arm-kernel, linux-sunxi
In-Reply-To: <20170417115747.7300-9-icenowy-h8G6r0blFSE@public.gmane.org>
On Mon, Apr 17, 2017 at 7:57 PM, Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org> wrote:
> AXP803 PMIC also have a series of regulators (DCDCs and LDOs)
> controllable via I2C/RSB bus.
>
> Add support for them.
>
> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> ---
> Changes in v2:
> - Place AXP803 codes before AXP806/809 ones.
> - Fixed some errors in regulator description.
> - Reuse AXP803 DLDO2 range for AXP806 CLDO2 & AXP809 DLDO1.
>
> drivers/regulator/axp20x-regulator.c | 153 ++++++++++++++++++++++++++++++-----
> include/linux/mfd/axp20x.h | 37 +++++++++
> 2 files changed, 168 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
> index 0b9d4e3e52c7..2ed15e4a7a82 100644
> --- a/drivers/regulator/axp20x-regulator.c
> +++ b/drivers/regulator/axp20x-regulator.c
[...]
> @@ -492,20 +578,38 @@ static bool axp20x_is_polyphase_slave(struct axp20x_dev *axp20x, int id)
> {
> u32 reg = 0;
>
> - /* Only AXP806 has poly-phase outputs */
> - if (axp20x->variant != AXP806_ID)
> - return false;
> + /*
> + * Currently in our supported AXP variants, only AXP806 and AXP803
Nit: mention them in ascending order.
Otherwise,
Acked-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
^ permalink raw reply
* [PATCH] ARM: dts: rockchip: reuse firefly dtsi
From: Eddie Cai @ 2017-04-18 12:15 UTC (permalink / raw)
To: heiko-4mtYJXux2i+zQB+pC5nmwQ, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
mark.rutland-5wv7dgnIgG8, linux-I+IVW8TIWO2tmTQ+vhA3Yw
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Eddie Cai
firefly reload is very similar with firefly board, so reuse firefly dtsi
Signed-off-by: Eddie Cai <eddie.cai.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
arch/arm/boot/dts/rk3288-firefly-reload-core.dtsi | 310 ------------------
arch/arm/boot/dts/rk3288-firefly-reload.dts | 368 ++--------------------
2 files changed, 34 insertions(+), 644 deletions(-)
delete mode 100644 arch/arm/boot/dts/rk3288-firefly-reload-core.dtsi
diff --git a/arch/arm/boot/dts/rk3288-firefly-reload-core.dtsi b/arch/arm/boot/dts/rk3288-firefly-reload-core.dtsi
deleted file mode 100644
index 8134966..0000000
--- a/arch/arm/boot/dts/rk3288-firefly-reload-core.dtsi
+++ /dev/null
@@ -1,310 +0,0 @@
-/*
- * Device tree file for Firefly Rockchip RK3288 Core board
- * Copyright (c) 2016 Randy Li <ayaka-xPW3/0Ywev/iB9QmIjCX8w@public.gmane.org>
- *
- * This file is dual-licensed: you can use it either under the terms
- * of the GPL or the X11 license, at your option. Note that this dual
- * licensing only applies to this file, and not this project as a
- * whole.
- *
- * a) This file is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation; either version 2 of the
- * License, or (at your option) any later version.
- *
- * This file is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * Or, alternatively,
- *
- * b) Permission is hereby granted, free of charge, to any person
- * obtaining a copy of this software and associated documentation
- * files (the "Software"), to deal in the Software without
- * restriction, including without limitation the rights to use,
- * copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the
- * Software is furnished to do so, subject to the following
- * conditions:
- *
- * The above copyright notice and this permission notice shall be
- * included in all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
- * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
- * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
- * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
- * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
- * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
- * OTHER DEALINGS IN THE SOFTWARE.
- */
-
-#include <dt-bindings/input/input.h>
-#include "rk3288.dtsi"
-
-/ {
- memory@0 {
- device_type = "memory";
- reg = <0 0x80000000>;
- };
-
- ext_gmac: external-gmac-clock {
- compatible = "fixed-clock";
- #clock-cells = <0>;
- clock-frequency = <125000000>;
- clock-output-names = "ext_gmac";
- };
-
-
- vcc_flash: flash-regulator {
- compatible = "regulator-fixed";
- regulator-name = "vcc_flash";
- regulator-min-microvolt = <1800000>;
- regulator-max-microvolt = <1800000>;
- vin-supply = <&vcc_io>;
- };
-};
-
-&cpu0 {
- cpu0-supply = <&vdd_cpu>;
-};
-
-&emmc {
- bus-width = <8>;
- cap-mmc-highspeed;
- disable-wp;
- mmc-ddr-1_8v;
- mmc-hs200-1_8v;
- non-removable;
- num-slots = <1>;
- pinctrl-names = "default";
- pinctrl-0 = <&emmc_clk>, <&emmc_cmd>, <&emmc_pwr>, <&emmc_bus8>;
- vmmc-supply = <&vcc_io>;
- vqmmc-supply = <&vcc_flash>;
- status = "okay";
-};
-
-&gmac {
- assigned-clocks = <&cru SCLK_MAC>;
- assigned-clock-parents = <&ext_gmac>;
- clock_in_out = "input";
- pinctrl-names = "default";
- pinctrl-0 = <&rgmii_pins>, <&phy_rst>, <&phy_pmeb>, <&phy_int>;
- phy-supply = <&vcc_lan>;
- phy-mode = "rgmii";
- snps,reset-active-low;
- snps,reset-delays-us = <0 10000 1000000>;
- snps,reset-gpio = <&gpio4 RK_PB0 GPIO_ACTIVE_LOW>;
- tx_delay = <0x30>;
- rx_delay = <0x10>;
- status = "ok";
-};
-
-&i2c0 {
- clock-frequency = <400000>;
- status = "okay";
-
- vdd_cpu: syr827@40 {
- compatible = "silergy,syr827";
- fcs,suspend-voltage-selector = <1>;
- reg = <0x40>;
- regulator-name = "vdd_cpu";
- regulator-min-microvolt = <850000>;
- regulator-max-microvolt = <1350000>;
- regulator-always-on;
- regulator-boot-on;
- regulator-enable-ramp-delay = <300>;
- regulator-ramp-delay = <8000>;
- vin-supply = <&vcc_sys>;
- };
-
- vdd_gpu: syr828@41 {
- compatible = "silergy,syr828";
- fcs,suspend-voltage-selector = <1>;
- reg = <0x41>;
- regulator-name = "vdd_gpu";
- regulator-min-microvolt = <850000>;
- regulator-max-microvolt = <1350000>;
- regulator-always-on;
- vin-supply = <&vcc_sys>;
- };
-
- act8846: act8846@5a {
- compatible = "active-semi,act8846";
- reg = <0x5a>;
- pinctrl-names = "default";
- pinctrl-0 = <&pmic_vsel>, <&pwr_hold>;
- system-power-controller;
-
- vp1-supply = <&vcc_sys>;
- vp2-supply = <&vcc_sys>;
- vp3-supply = <&vcc_sys>;
- vp4-supply = <&vcc_sys>;
- inl1-supply = <&vcc_sys>;
- inl2-supply = <&vcc_sys>;
- inl3-supply = <&vcc_20>;
-
- regulators {
- vcc_ddr: REG1 {
- regulator-name = "vcc_ddr";
- regulator-min-microvolt = <1200000>;
- regulator-max-microvolt = <1200000>;
- regulator-always-on;
- };
-
- vcc_io: REG2 {
- regulator-name = "vcc_io";
- regulator-min-microvolt = <3300000>;
- regulator-max-microvolt = <3300000>;
- regulator-always-on;
- };
-
- vdd_log: REG3 {
- regulator-name = "vdd_log";
- regulator-min-microvolt = <1100000>;
- regulator-max-microvolt = <1100000>;
- regulator-always-on;
- };
-
- vcc_20: REG4 {
- regulator-name = "vcc_20";
- regulator-min-microvolt = <2000000>;
- regulator-max-microvolt = <2000000>;
- regulator-always-on;
- };
-
- vccio_sd: REG5 {
- regulator-name = "vccio_sd";
- regulator-min-microvolt = <3300000>;
- regulator-max-microvolt = <3300000>;
- };
-
- vdd10_lcd: REG6 {
- regulator-name = "vdd10_lcd";
- regulator-min-microvolt = <1000000>;
- regulator-max-microvolt = <1000000>;
- };
-
- vcca_18: REG7 {
- regulator-name = "vcca_18";
- regulator-min-microvolt = <1800000>;
- regulator-max-microvolt = <1800000>;
- regulator-always-on;
- };
-
- vcca_33: REG8 {
- regulator-name = "vcca_33";
- regulator-min-microvolt = <3300000>;
- regulator-max-microvolt = <3300000>;
- regulator-always-on;
- };
-
- vcc_lan: REG9 {
- regulator-name = "vcca_lan";
- regulator-min-microvolt = <3300000>;
- regulator-max-microvolt = <3300000>;
- };
-
- vdd_10: REG10 {
- regulator-name = "vdd_10";
- regulator-min-microvolt = <1000000>;
- regulator-max-microvolt = <1000000>;
- regulator-always-on;
- };
-
- vccio_wl: vcc_18: REG11 {
- regulator-name = "vcc_18";
- regulator-min-microvolt = <1800000>;
- regulator-max-microvolt = <1800000>;
- };
-
- vcc18_lcd: REG12 {
- regulator-name = "vcc18_lcd";
- regulator-min-microvolt = <1800000>;
- regulator-max-microvolt = <1800000>;
- };
- };
- };
-};
-
-&io_domains {
- status = "okay";
-
- audio-supply = <&vccio_wl>;
- bb-supply = <&vcc_io>;
- dvp-supply = <&dovdd_1v8>;
- flash0-supply = <&vcc_flash>;
- flash1-supply = <&vcc_lan>;
- gpio30-supply = <&vcc_io>;
- gpio1830-supply = <&vcc_io>;
- lcdc-supply = <&vcc_io>;
- sdcard-supply = <&vccio_sd>;
- wifi-supply = <&vccio_wl>;
-};
-
-&pinctrl {
- pcfg_output_high: pcfg-output-high {
- output-high;
- };
-
- pcfg_output_low: pcfg-output-low {
- output-low;
- };
-
- pcfg_pull_up_drv_12ma: pcfg-pull-up-drv-12ma {
- bias-pull-up;
- drive-strength = <12>;
- };
-
- act8846 {
- pwr_hold: pwr-hold {
- rockchip,pins = <0 1 RK_FUNC_GPIO &pcfg_output_high>;
- };
-
- pmic_vsel: pmic-vsel {
- rockchip,pins = <7 14 RK_FUNC_GPIO &pcfg_output_low>;
- };
- };
-
- gmac {
- phy_int: phy-int {
- rockchip,pins = <0 9 RK_FUNC_GPIO &pcfg_pull_up>;
- };
-
- phy_pmeb: phy-pmeb {
- rockchip,pins = <0 8 RK_FUNC_GPIO &pcfg_pull_up>;
- };
-
- phy_rst: phy-rst {
- rockchip,pins = <4 8 RK_FUNC_GPIO &pcfg_output_high>;
- };
- };
-};
-
-&tsadc {
- rockchip,hw-tshut-mode = <0>;
- rockchip,hw-tshut-polarity = <0>;
- status = "okay";
-};
-
-&vopb {
- status = "okay";
-};
-
-&vopb_mmu {
- status = "okay";
-};
-
-&vopl {
- status = "okay";
-};
-
-&vopl_mmu {
- status = "okay";
-};
-
-&wdt {
- status = "okay";
-};
diff --git a/arch/arm/boot/dts/rk3288-firefly-reload.dts b/arch/arm/boot/dts/rk3288-firefly-reload.dts
index d0b3204a..da8f219 100644
--- a/arch/arm/boot/dts/rk3288-firefly-reload.dts
+++ b/arch/arm/boot/dts/rk3288-firefly-reload.dts
@@ -1,6 +1,7 @@
/*
* Device tree file for Firefly Rockchip RK3288 Core board
* Copyright (c) 2016 Randy Li <ayaka-xPW3/0Ywev/iB9QmIjCX8w@public.gmane.org>
+ * Copyright (c) 2017 Eddie Cai <eddie.cai.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
*
* This file is dual-licensed: you can use it either under the terms
* of the GPL or the X11 license, at your option. Note that this dual
@@ -42,362 +43,61 @@
*/
/dts-v1/;
-#include "rk3288-firefly-reload-core.dtsi"
+#include "rk3288-firefly.dtsi"
/ {
model = "Firefly-RK3288-reload";
compatible = "firefly,firefly-rk3288-reload", "rockchip,rk3288";
+};
- gpio-keys {
- compatible = "gpio-keys";
-
- power {
- wakeup-source;
- gpios = <&gpio0 RK_PA5 GPIO_ACTIVE_LOW>;
- label = "GPIO Power";
- linux,code = <KEY_POWER>;
- pinctrl-names = "default";
- pinctrl-0 = <&pwr_key>;
- };
- };
-
- ir-receiver {
- compatible = "gpio-ir-receiver";
- gpios = <&gpio7 RK_PA0 GPIO_ACTIVE_LOW>;
- };
-
- leds {
- compatible = "gpio-leds";
-
- power {
- gpios = <&gpio8 RK_PA2 GPIO_ACTIVE_LOW>;
- label = "firefly:blue:power";
- pinctrl-names = "default";
- pinctrl-0 = <&power_led>;
- panic-indicator;
+&act8846 {
+ regulators {
+ /delete-node/ REG7;
+ /delete-node/ REG8;
+ /delete-node/ REG9;
+ vcca_33: REG7 {
+ regulator-name = "vcca_33";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
};
- work {
- gpios = <&gpio8 RK_PA1 GPIO_ACTIVE_LOW>;
- label = "firefly:blue:user";
- linux,default-trigger = "rc-feedback";
- pinctrl-names = "default";
- pinctrl-0 = <&work_led>;
+ vcc_lan: REG8 {
+ regulator-name = "vcc_lan";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
};
- };
-
- sdio_pwrseq: sdio-pwrseq {
- compatible = "mmc-pwrseq-simple";
- clocks = <&hym8563>;
- clock-names = "ext_clock";
- pinctrl-names = "default";
- pinctrl-0 = <&wifi_enable>;
- reset-gpios = <&gpio4 RK_PD4 GPIO_ACTIVE_LOW>;
- };
- sound {
- compatible = "simple-audio-card";
- simple-audio-card,name = "SPDIF";
- simple-audio-card,dai-link@1 { /* S/PDIF - S/PDIF */
- cpu { sound-dai = <&spdif>; };
- codec { sound-dai = <&spdif_out>; };
+ vccio_pmu: REG9 {
+ regulator-name = "vccio_pmu";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ regulator-always-on;
};
};
-
- spdif_out: spdif-out {
- compatible = "linux,spdif-dit";
- #sound-dai-cells = <0>;
- };
-
- vcc_host_5v: usb-host-regulator {
- compatible = "regulator-fixed";
- enable-active-high;
- gpio = <&gpio0 RK_PB6 GPIO_ACTIVE_HIGH>;
- pinctrl-names = "default";
- pinctrl-0 = <&host_vbus_drv>;
- regulator-name = "vcc_host_5v";
- regulator-min-microvolt = <5000000>;
- regulator-max-microvolt = <5000000>;
- regulator-always-on;
- vin-supply = <&vcc_5v>;
- };
-
- vcc_5v: vcc_sys: vsys-regulator {
- compatible = "regulator-fixed";
- regulator-name = "vcc_5v";
- regulator-min-microvolt = <5000000>;
- regulator-max-microvolt = <5000000>;
- regulator-always-on;
- regulator-boot-on;
- };
-
- vcc_sd: sdmmc-regulator {
- compatible = "regulator-fixed";
- gpio = <&gpio7 RK_PB3 GPIO_ACTIVE_LOW>;
- pinctrl-names = "default";
- pinctrl-0 = <&sdmmc_pwr>;
- regulator-name = "vcc_sd";
- regulator-min-microvolt = <3300000>;
- regulator-max-microvolt = <3300000>;
- startup-delay-us = <100000>;
- vin-supply = <&vcc_io>;
- };
-
- vcc_otg_5v: usb-otg-regulator {
- compatible = "regulator-fixed";
- enable-active-high;
- gpio = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>;
- pinctrl-names = "default";
- pinctrl-0 = <&otg_vbus_drv>;
- regulator-name = "vcc_otg_5v";
- regulator-min-microvolt = <5000000>;
- regulator-max-microvolt = <5000000>;
- regulator-always-on;
- vin-supply = <&vcc_5v>;
- };
-
- dovdd_1v8: dovdd-1v8-regulator {
- compatible = "regulator-fixed";
- enable-active-high;
- gpio = <&gpio0 RK_PB3 GPIO_ACTIVE_HIGH>;
- pinctrl-names = "default";
- pinctrl-0 = <&dvp_pwr>;
- regulator-name = "dovdd_1v8";
- regulator-min-microvolt = <1800000>;
- regulator-max-microvolt = <1800000>;
- vin-supply = <&vcc_io>;
- };
-
- vcc28_dvp: vcc28-dvp-regulator {
- compatible = "regulator-fixed";
- enable-active-high;
- gpio = <&gpio0 RK_PB3 GPIO_ACTIVE_HIGH>;
- pinctrl-names = "default";
- pinctrl-0 = <&dvp_pwr>;
- regulator-name = "vcc28_dvp";
- regulator-min-microvolt = <2800000>;
- regulator-max-microvolt = <2800000>;
- vin-supply = <&vcc_io>;
- };
-
- af_28: af_28-regulator {
- compatible = "regulator-fixed";
- enable-active-high;
- gpio = <&gpio0 RK_PB3 GPIO_ACTIVE_HIGH>;
- pinctrl-names = "default";
- pinctrl-0 = <&dvp_pwr>;
- regulator-name = "af_28";
- regulator-min-microvolt = <2800000>;
- regulator-max-microvolt = <2800000>;
- vin-supply = <&vcc_io>;
- };
-
- dvdd_1v2: af_28-regulator {
- compatible = "regulator-fixed";
- enable-active-high;
- gpio = <&gpio7 RK_PB4 GPIO_ACTIVE_HIGH>;
- pinctrl-names = "default";
- pinctrl-0 = <&cif_pwr>;
- regulator-name = "dvdd_1v2";
- regulator-min-microvolt = <1200000>;
- regulator-max-microvolt = <1200000>;
- vin-supply = <&vcc_io>;
- };
-
- vbat_wl: wifi-regulator {
- compatible = "regulator-fixed";
- regulator-name = "vbat_wl";
- regulator-min-microvolt = <3300000>;
- regulator-max-microvolt = <3300000>;
- vin-supply = <&vcc_io>;
- };
-};
-
-&i2c0 {
- hym8563: hym8563@51 {
- compatible = "haoyu,hym8563";
- reg = <0x51>;
- #clock-cells = <0>;
- clock-frequency = <32768>;
- clock-output-names = "xin32k";
- interrupt-parent = <&gpio7>;
- interrupts = <RK_PA4 IRQ_TYPE_EDGE_FALLING>;
- pinctrl-names = "default";
- pinctrl-0 = <&rtc_int>;
- };
-};
-
-&i2c2 {
- status = "okay";
-
- codec: es8328@10 {
- compatible = "everest,es8328";
- DVDD-supply = <&vcca_33>;
- AVDD-supply = <&vcca_33>;
- PVDD-supply = <&vcca_33>;
- HPVDD-supply = <&vcca_33>;
- clocks = <&cru HCLK_I2S0>, <&cru SCLK_I2S0>;
- clock-names = "i2s_hclk", "i2s_clk";
- reg = <0x10>;
- };
-};
-
-&i2s {
- status = "okay";
-};
-
-&sdmmc {
- bus-width = <4>;
- cap-mmc-highspeed;
- cap-sd-highspeed;
- card-detect-delay = <200>;
- disable-wp;
- num-slots = <1>;
- pinctrl-names = "default";
- pinctrl-0 = <&sdmmc_clk>, <&sdmmc_cmd>, <&sdmmc_cd>, <&sdmmc_bus4>;
- vmmc-supply = <&vcc_sd>;
- vqmmc-supply = <&vccio_sd>;
- status = "okay";
-};
-
-&sdio0 {
- bus-width = <4>;
- cap-sd-highspeed;
- cap-sdio-irq;
- disable-wp;
- mmc-pwrseq = <&sdio_pwrseq>;
- non-removable;
- num-slots = <1>;
- pinctrl-names = "default";
- pinctrl-0 = <&sdio0_bus4>, <&sdio0_cmd>, <&sdio0_clk>, <&sdio0_int>;
- sd-uhs-sdr12;
- sd-uhs-sdr25;
- sd-uhs-sdr50;
- sd-uhs-ddr50;
- vmmc-supply = <&vbat_wl>;
- vqmmc-supply = <&vccio_wl>;
- status = "okay";
-};
-
-&spdif {
- status = "okay";
-};
-
-&uart0 {
- pinctrl-names = "default";
- pinctrl-0 = <&uart0_xfer>, <&uart0_cts>, <&uart0_rts>;
- status = "okay";
};
-&uart1 {
- status = "okay";
-};
-
-&uart2 {
- status = "okay";
-};
-
-&uart3 {
- status = "okay";
+&emmc {
+ mmc-hs200-1_8v;
};
-&usbphy {
- status = "okay";
-};
-
-&usb_host1 {
- pinctrl-names = "default";
- pinctrl-0 = <&usbhub_rst>;
- status = "okay";
-};
-
-&usb_otg {
- status = "okay";
+&ir {
+ gpios = <&gpio7 RK_PA0 GPIO_ACTIVE_LOW>;
};
&pinctrl {
- ir {
- ir_int: ir-int {
- rockchip,pins = <7 0 RK_FUNC_GPIO &pcfg_pull_up>;
- };
- };
-
- dvp {
- dvp_pwr: dvp-pwr {
- rockchip,pins = <0 11 RK_FUNC_GPIO &pcfg_pull_none>;
- };
-
- cif_pwr: cif-pwr {
- rockchip,pins = <7 12 RK_FUNC_GPIO &pcfg_pull_none>;
- };
- };
-
- hym8563 {
- rtc_int: rtc-int {
- rockchip,pins = <7 4 RK_FUNC_GPIO &pcfg_pull_up>;
- };
- };
-
- keys {
- pwr_key: pwr-key {
- rockchip,pins = <0 5 RK_FUNC_GPIO &pcfg_pull_up>;
+ act8846 {
+ pmic_vsel: pmic-vsel {
+ rockchip,pins = <7 14 RK_FUNC_GPIO &pcfg_output_low>;
};
};
- leds {
- power_led: power-led {
- rockchip,pins = <8 2 RK_FUNC_GPIO &pcfg_pull_none>;
- };
-
- work_led: work-led {
- rockchip,pins = <8 1 RK_FUNC_GPIO &pcfg_pull_none>;
- };
- };
-
- sdmmc {
- /*
- * Default drive strength isn't enough to achieve even
- * high-speed mode on firefly board so bump up to 12ma.
- */
- sdmmc_bus4: sdmmc-bus4 {
- rockchip,pins = <6 16 RK_FUNC_1 &pcfg_pull_up_drv_12ma>,
- <6 17 RK_FUNC_1 &pcfg_pull_up_drv_12ma>,
- <6 18 RK_FUNC_1 &pcfg_pull_up_drv_12ma>,
- <6 19 RK_FUNC_1 &pcfg_pull_up_drv_12ma>;
- };
-
- sdmmc_clk: sdmmc-clk {
- rockchip,pins = <6 20 RK_FUNC_1 &pcfg_pull_none_12ma>;
- };
-
- sdmmc_cmd: sdmmc-cmd {
- rockchip,pins = <6 21 RK_FUNC_1 &pcfg_pull_up_drv_12ma>;
- };
-
- sdmmc_pwr: sdmmc-pwr {
- rockchip,pins = <7 11 RK_FUNC_GPIO &pcfg_pull_none>;
- };
- };
-
- sdio {
- wifi_enable: wifi-enable {
- rockchip,pins = <4 28 RK_FUNC_GPIO &pcfg_pull_none>;
- };
- };
-
- usb_host {
- host_vbus_drv: host-vbus-drv {
- rockchip,pins = <0 14 RK_FUNC_GPIO &pcfg_pull_none>;
- };
-
- usbhub_rst: usbhub-rst {
- rockchip,pins = <8 3 RK_FUNC_GPIO &pcfg_output_high>;
+ ir {
+ ir_int: ir-int {
+ rockchip,pins = <7 0 RK_FUNC_GPIO &pcfg_pull_up>;
};
};
+};
- usb_otg {
- otg_vbus_drv: otg-vbus-drv {
- rockchip,pins = <0 12 RK_FUNC_GPIO &pcfg_pull_none>;
- };
- };
+&pwm1 {
+ status = "okay";
};
--
2.10.2
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH] ARM: dts: omap4-droid4: Stop disabling SRAM and GPMC
From: Sebastian Reichel @ 2017-04-18 12:17 UTC (permalink / raw)
To: Tony Lindgren
Cc: linux-omap-u79uwXL29TY76Z2rM5mHXA, Benoît Cousson,
devicetree-u79uwXL29TY76Z2rM5mHXA, Marcel Partap, Michael Scott
In-Reply-To: <20170328004612.16350-1-tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1907 bytes --]
Hi,
On Mon, Mar 27, 2017 at 05:46:12PM -0700, Tony Lindgren wrote:
> I disabled SRAM and GPMC originally when seeing errors with
> omap_barriers_init(). But that is no longer happening probably
> because the memory range is now properly configured to 1021 MB
> instead of 1024 MB. So let's enable SRAM and GPMC so we get
> omap_barriers_init() working and can idle the GPMC.
>
> Cc: Marcel Partap <mpartap-hi6Y0CQ0nG0@public.gmane.org>
> Cc: Michael Scott <michael.scott-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
Tested-by: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
-- Sebastian
> arch/arm/boot/dts/omap4-droid4-xt894.dts | 13 +------------
> 1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/arch/arm/boot/dts/omap4-droid4-xt894.dts b/arch/arm/boot/dts/omap4-droid4-xt894.dts
> --- a/arch/arm/boot/dts/omap4-droid4-xt894.dts
> +++ b/arch/arm/boot/dts/omap4-droid4-xt894.dts
> @@ -24,8 +24,7 @@
>
> /*
> * We seem to have only 1021 MB accessible, 1021 - 1022 is locked,
> - * then 1023 - 1024 seems to contain mbm. For SRAM, see the notes
> - * below about SRAM and L3_ICLK2 being unused by default,
> + * then 1023 - 1024 seems to contain mbm.
> */
> memory {
> device_type = "memory";
> @@ -176,11 +175,6 @@
> };
> };
>
> -/* L3_2 interconnect is unused, SRAM, GPMC and L3_ICLK2 disabled */
> -&gpmc {
> - status = "disabled";
> -};
> -
> &hdmi {
> status = "okay";
> pinctrl-0 = <&dss_hdmi_pins>;
> @@ -356,11 +350,6 @@
> };
> };
>
> -/* L3_2 interconnect is unused, SRAM, GPMC and L3_ICLK2 disabled */
> -&ocmcram {
> - status = "disabled";
> -};
> -
> &omap4_pmx_core {
>
> /* hdmi_hpd.gpio_63 */
> --
> 2.12.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/2] dmaengine: Add DW AXI DMAC driver
From: Andy Shevchenko @ 2017-04-18 12:31 UTC (permalink / raw)
To: Eugeniy Paltsev, dmaengine-u79uwXL29TY76Z2rM5mHXA
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-snps-arc-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Dan Williams,
Vinod Koul, Rob Herring, Alexey Brodkin
In-Reply-To: <1491573855-1039-3-git-send-email-Eugeniy.Paltsev-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
On Fri, 2017-04-07 at 17:04 +0300, Eugeniy Paltsev wrote:
> This patch adds support for the DW AXI DMAC controller.
>
> DW AXI DMAC is a part of upcoming development board from Synopsys.
>
> In this driver implementation only DMA_MEMCPY and DMA_SG transfers
> are supported.
>
> +++ b/drivers/dma/axi_dma_platform.c
> @@ -0,0 +1,1044 @@
> +/*
> + * Synopsys DesignWare AXI DMA Controller driver.
> + *
> + * Copyright (C) 2017 Synopsys, Inc. (www.synopsys.com)
> + *
> + * This program is free software; you can redistribute it and/or
> modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dmapool.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
Are you sure you still need of.h along with depends OF ?
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/property.h>
> +#include <linux/types.h>
> +
> +#include "axi_dma_platform.h"
> +#include "axi_dma_platform_reg.h"
Can't you have this in one header?
> +#include "dmaengine.h"
> +#include "virt-dma.h"
> +#define AXI_DMA_BUSWIDTHS \
> + (DMA_SLAVE_BUSWIDTH_1_BYTE | \
> + DMA_SLAVE_BUSWIDTH_2_BYTES | \
> + DMA_SLAVE_BUSWIDTH_4_BYTES | \
> + DMA_SLAVE_BUSWIDTH_8_BYTES | \
> + DMA_SLAVE_BUSWIDTH_16_BYTES | \
> + DMA_SLAVE_BUSWIDTH_32_BYTES | \
> + DMA_SLAVE_BUSWIDTH_64_BYTES)
> +/* TODO: check: do we need to use BIT() macro here? */
Still TODO? I remember I answered to this on the first round.
> +
> +static inline void
> +axi_dma_iowrite32(struct axi_dma_chip *chip, u32 reg, u32 val)
> +{
> + iowrite32(val, chip->regs + reg);
Are you going to use IO ports for this IP? I don't think so.
Wouldn't be better to call readl()/writel() instead?
> +}
> +static inline void
> +axi_chan_iowrite64(struct axi_dma_chan *chan, u32 reg, u64 val)
> +{
> + iowrite32(val & 0xFFFFFFFF, chan->chan_regs + reg);
Useless conjunction.
> + iowrite32(val >> 32, chan->chan_regs + reg + 4);
> +}
Can your hardware get 8 bytes at once?
For such cases we have iowrite64() for 64-bit kernels
But with readq()/writeq() we have specific helpers to make this pretty,
i.e. lo_hi_readq() / lo_hi_writeq() (or hi_lo_*() variants).
> +static inline void axi_chan_irq_disable(struct axi_dma_chan *chan,
> u32 irq_mask)
> +{
> + u32 val;
> +
> + if (likely(irq_mask == DWAXIDMAC_IRQ_ALL)) {
> + axi_chan_iowrite32(chan, CH_INTSTATUS_ENA,
> DWAXIDMAC_IRQ_NONE);
> + } else {
I don't see the benefit. (Yes, I see one read-less path, I think it
makes perplexity for nothing here)
> + val = axi_chan_ioread32(chan, CH_INTSTATUS_ENA);
> + val &= ~irq_mask;
> + axi_chan_iowrite32(chan, CH_INTSTATUS_ENA, val);
> + }
> +}
> +static inline void axi_chan_disable(struct axi_dma_chan *chan)
> +{
> +}
> +
> +static inline void axi_chan_enable(struct axi_dma_chan *chan)
> +{
> +}
> +static u32 axi_chan_get_xfer_width(struct axi_dma_chan *chan,
> dma_addr_t src,
> + dma_addr_t dst, size_t len)
> +{
> + u32 max_width = chan->chip->dw->hdata->m_data_width;
> + size_t sdl = (src | dst | len);
Redundant parens, redundant temporary variable (you may do this in
place).
> +
> + return min_t(size_t, __ffs(sdl), max_width);
> +}
> +static void axi_desc_put(struct axi_dma_desc *desc)
> +{
> + struct axi_dma_chan *chan = desc->chan;
> + struct dw_axi_dma *dw = chan->chip->dw;
> + struct axi_dma_desc *child, *_next;
> + unsigned int descs_put = 0;
> + list_for_each_entry_safe(child, _next, &desc->xfer_list,
> xfer_list) {
xfer_list looks redundant.
Can you elaborate why virtual channel management is not working for you?
> + list_del(&child->xfer_list);
> + dma_pool_free(dw->desc_pool, child, child-
> >vd.tx.phys);
> + descs_put++;
> + }
> +}
> +/* Called in chan locked context */
> +static void axi_chan_block_xfer_start(struct axi_dma_chan *chan,
> + struct axi_dma_desc *first)
> +{
> + u32 reg, irq_mask;
> + u8 lms = 0;
Does it make any sense? It looks like lms is always 0.
Or I miss the source of its value?
> + u32 priority = chan->chip->dw->hdata->priority[chan->id];
Reversed xmas tree, please.
Btw, are you planning to use priority at all? For now on I didn't see a
single driver (from the set I have checked, like 4-5 of them) that uses
priority anyhow. It makes driver more complex for nothing.
> +
> + if (unlikely(axi_chan_is_hw_enable(chan))) {
> + dev_err(chan2dev(chan), "%s is non-idle!\n",
> + axi_chan_name(chan));
> +
> + return;
> + }
> +}
> +static void dma_chan_free_chan_resources(struct dma_chan *dchan)
> +{
> + struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);
> +
> + /* ASSERT: channel hw is idle */
> + if (axi_chan_is_hw_enable(chan))
> + dev_err(dchan2dev(dchan), "%s is non-idle!\n",
> + axi_chan_name(chan));
> +
> + axi_chan_disable(chan);
> + axi_chan_irq_disable(chan, DWAXIDMAC_IRQ_ALL);
> +
> + vchan_free_chan_resources(&chan->vc);
> +
> + dev_vdbg(dchan2dev(dchan), "%s: %s: descriptor still
> allocated: %u\n",
> + __func__, axi_chan_name(chan),
Redundant __func__ parameter for debug prints.
> + atomic_read(&chan->descs_allocated));
> +
> + pm_runtime_put(chan->chip->dev);
> +}
> +static struct dma_async_tx_descriptor *
> +dma_chan_prep_dma_sg(struct dma_chan *dchan,
> + struct scatterlist *dst_sg, unsigned int
> dst_nents,
> + struct scatterlist *src_sg, unsigned int
> src_nents,
> + unsigned long flags)
> +{
> + struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);
> + struct axi_dma_desc *first = NULL, *desc = NULL, *prev =
> NULL;
> + size_t dst_len = 0, src_len = 0, xfer_len = 0;
> + dma_addr_t dst_adr = 0, src_adr = 0;
> + u32 src_width, dst_width;
> + size_t block_ts, max_block_ts;
> + u32 reg;
> + u8 lms = 0;
Same about lms.
> +
> + dev_dbg(chan2dev(chan), "%s: %s: sn: %d dn: %d flags: 0x%lx",
> + __func__, axi_chan_name(chan), src_nents, dst_nents,
> flags);
Ditto for __func__.
> +
>
> + if (unlikely(dst_nents == 0 || src_nents == 0))
> + return NULL;
> +
> + if (unlikely(dst_sg == NULL || src_sg == NULL))
> + return NULL;
If we need those checks they should go to dmaengine.h/dmaengine.c.
> +static void axi_chan_list_dump_lli(struct axi_dma_chan *chan,
> + struct axi_dma_desc *desc_head)
> +{
> + struct axi_dma_desc *desc;
> +
> + axi_chan_dump_lli(chan, desc_head);
> + list_for_each_entry(desc, &desc_head->xfer_list, xfer_list)
> + axi_chan_dump_lli(chan, desc);
> +}
> +
> +
Redundant (one) empty line.
> +static void axi_chan_handle_err(struct axi_dma_chan *chan, u32
> status)
> +{
> + /* WARN about bad descriptor */
>
> + dev_err(chan2dev(chan),
> + "Bad descriptor submitted for %s, cookie: %d, irq:
> 0x%08x\n",
> + axi_chan_name(chan), vd->tx.cookie, status);
> + axi_chan_list_dump_lli(chan, vd_to_axi_desc(vd));
As I said earlier dw_dmac is *bad* example of the (virtual channel
based) DMA driver.
I guess you may just fail the descriptor and don't pretend it has been
processed successfully.
> +static int dma_chan_pause(struct dma_chan *dchan)
> +{
> + struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);
> + unsigned long flags;
> + unsigned int timeout = 20; /* timeout iterations */
> + int ret = -EAGAIN;
> + u32 val;
> +
> + spin_lock_irqsave(&chan->vc.lock, flags);
> +
> + val = axi_dma_ioread32(chan->chip, DMAC_CHEN);
> + val |= BIT(chan->id) << DMAC_CHAN_SUSP_SHIFT |
> + BIT(chan->id) << DMAC_CHAN_SUSP_WE_SHIFT;
> + axi_dma_iowrite32(chan->chip, DMAC_CHEN, val);
You have helpers which you don't use. Why?
> +
> + while (timeout--) {
In such cases I prefer do {} while (); to explicitly show that body goes
at least once.
> + if (axi_chan_irq_read(chan) &
> DWAXIDMAC_IRQ_SUSPENDED) {
> + ret = 0;
> + break;
> + }
> + udelay(2);
> + }
> +
> + axi_chan_irq_clear(chan, DWAXIDMAC_IRQ_SUSPENDED);
> +
> + chan->is_paused = true;
> +
> + spin_unlock_irqrestore(&chan->vc.lock, flags);
> +
> + return ret;
> +}
> +
> +/* Called in chan locked context */
> +static inline void axi_chan_resume(struct axi_dma_chan *chan)
> +{
> + u32 val;
> +
> + val = axi_dma_ioread32(chan->chip, DMAC_CHEN);
> + val &= ~(BIT(chan->id) << DMAC_CHAN_SUSP_SHIFT);
> + val |= (BIT(chan->id) << DMAC_CHAN_SUSP_WE_SHIFT);
> + axi_dma_iowrite32(chan->chip, DMAC_CHEN, val);
Use helper.
> +
> + chan->is_paused = false;
> +}
> +static int axi_dma_runtime_suspend(struct device *dev)
> +{
> + struct axi_dma_chip *chip = dev_get_drvdata(dev);
> +
> + dev_info(dev, "PAL: %s\n", __func__);
Noisy and useless.
We have functional tracer in kernel. Use it.
> +
> + axi_dma_irq_disable(chip);
> + axi_dma_disable(chip);
> +
> + clk_disable_unprepare(chip->clk);
> +
> + return 0;
> +}
> +
> +static int axi_dma_runtime_resume(struct device *dev)
> +{
> + struct axi_dma_chip *chip = dev_get_drvdata(dev);
> + int ret = 0;
> +
> + dev_info(dev, "PAL: %s\n", __func__);
Ditto.
> +
> + ret = clk_prepare_enable(chip->clk);
> + if (ret < 0)
> + return ret;
> +
> + axi_dma_enable(chip);
> + axi_dma_irq_enable(chip);
> +
> + return 0;
> +}
> +static int dw_probe(struct platform_device *pdev)
> +{
> + struct axi_dma_chip *chip;
> + struct resource *mem;
> + struct dw_axi_dma *dw;
> + struct dw_axi_dma_hcfg *hdata;
> + u32 i;
> + int ret;
> +
> + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> + if (!chip)
> + return -ENOMEM;
> +
> + dw = devm_kzalloc(&pdev->dev, sizeof(*dw), GFP_KERNEL);
> + if (!dw)
> + return -ENOMEM;
> +
> + hdata = devm_kzalloc(&pdev->dev, sizeof(*hdata), GFP_KERNEL);
> + if (!hdata)
> + return -ENOMEM;
> +
> + chip->dw = dw;
> + chip->dev = &pdev->dev;
> + chip->dw->hdata = hdata;
> +
> + chip->irq = platform_get_irq(pdev, 0);
> + if (chip->irq < 0)
> + return chip->irq;
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + chip->regs = devm_ioremap_resource(chip->dev, mem);
> + if (IS_ERR(chip->regs))
> + return PTR_ERR(chip->regs);
> +
> + chip->clk = devm_clk_get(chip->dev, NULL);
> + if (IS_ERR(chip->clk))
> + return PTR_ERR(chip->clk);
> +
> + ret = parse_device_properties(chip);
> + if (ret)
> + return ret;
> +
> + dw->chan = devm_kcalloc(chip->dev, hdata->nr_channels,
> + sizeof(*dw->chan), GFP_KERNEL);
> + if (!dw->chan)
> + return -ENOMEM;
> +
> + ret = devm_request_irq(chip->dev, chip->irq,
> dw_axi_dma_intretupt,
> + IRQF_SHARED, DRV_NAME, chip);
> + if (ret)
> + return ret;
> +
> + /* Lli address must be aligned to a 64-byte boundary */
> + dw->desc_pool = dmam_pool_create(DRV_NAME, chip->dev,
> + sizeof(struct axi_dma_desc),
> 64, 0);
> + if (!dw->desc_pool) {
> + dev_err(chip->dev, "No memory for descriptors dma
> pool\n");
> + return -ENOMEM;
> + }
> +
> + INIT_LIST_HEAD(&dw->dma.channels);
> + for (i = 0; i < hdata->nr_channels; i++) {
> + struct axi_dma_chan *chan = &dw->chan[i];
> +
> + chan->chip = chip;
> + chan->id = (u8)i;
> + chan->chan_regs = chip->regs + COMMON_REG_LEN + i *
> CHAN_REG_LEN;
> + atomic_set(&chan->descs_allocated, 0);
> +
> + chan->vc.desc_free = vchan_desc_put;
> + vchan_init(&chan->vc, &dw->dma);
> + }
> +
> + /* Set capabilities */
> + dma_cap_set(DMA_MEMCPY, dw->dma.cap_mask);
> + dma_cap_set(DMA_SG, dw->dma.cap_mask);
> +
> + /* DMA capabilities */
> + dw->dma.chancnt = hdata->nr_channels;
> + dw->dma.src_addr_widths = AXI_DMA_BUSWIDTHS;
> + dw->dma.dst_addr_widths = AXI_DMA_BUSWIDTHS;
> + dw->dma.directions = BIT(DMA_MEM_TO_MEM);
> + dw->dma.residue_granularity =
> DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
> +
> + dw->dma.dev = chip->dev;
> + dw->dma.device_tx_status = dma_chan_tx_status;
> + dw->dma.device_issue_pending = dma_chan_issue_pending;
> + dw->dma.device_terminate_all = dma_chan_terminate_all;
> + dw->dma.device_pause = dma_chan_pause;
> + dw->dma.device_resume = dma_chan_resume;
> +
> + dw->dma.device_alloc_chan_resources =
> dma_chan_alloc_chan_resources;
> + dw->dma.device_free_chan_resources =
> dma_chan_free_chan_resources;
> +
> + dw->dma.device_prep_dma_memcpy = dma_chan_prep_dma_memcpy;
> + dw->dma.device_prep_dma_sg = dma_chan_prep_dma_sg;
> +
> + platform_set_drvdata(pdev, chip);
> +
> + pm_runtime_enable(chip->dev);
> +
> + /*
> + * We can't just call pm_runtime_get here instead of
> + * pm_runtime_get_noresume + axi_dma_runtime_resume because
> we need
> + * driver to work also without Runtime PM.
> + */
> + pm_runtime_get_noresume(chip->dev);
> + ret = axi_dma_runtime_resume(chip->dev);
> + if (ret < 0)
> + goto err_pm_disable;
> +
> + axi_dma_hw_init(chip);
> +
> + pm_runtime_put(chip->dev);
> +
> + ret = dma_async_device_register(&dw->dma);
> + if (ret)
> + goto err_pm_disable;
> +
> + dev_info(chip->dev, "DesignWare AXI DMA Controller, %d
> channels\n",
> + dw->hdata->nr_channels);
> +
> + return 0;
> +
> +err_pm_disable:
> + pm_runtime_disable(chip->dev);
> +
> + return ret;
> +}
> +
> +static int dw_remove(struct platform_device *pdev)
> +{
> + struct axi_dma_chip *chip = platform_get_drvdata(pdev);
> + struct dw_axi_dma *dw = chip->dw;
> + struct axi_dma_chan *chan, *_chan;
> + u32 i;
> +
> + /* Enable clk before accessing to registers */
> + clk_prepare_enable(chip->clk);
> + axi_dma_irq_disable(chip);
> + for (i = 0; i < dw->hdata->nr_channels; i++) {
> + axi_chan_disable(&chip->dw->chan[i]);
> + axi_chan_irq_disable(&chip->dw->chan[i],
> DWAXIDMAC_IRQ_ALL);
> + }
> + axi_dma_disable(chip);
> +
> + pm_runtime_disable(chip->dev);
> + axi_dma_runtime_suspend(chip->dev);
> +
> + devm_free_irq(chip->dev, chip->irq, chip);
> +
> + list_for_each_entry_safe(chan, _chan, &dw->dma.channels,
> + vc.chan.device_node) {
> + list_del(&chan->vc.chan.device_node);
> + tasklet_kill(&chan->vc.task);
> + }
> +
> + dma_async_device_unregister(&dw->dma);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops dw_axi_dma_pm_ops = {
> + SET_RUNTIME_PM_OPS(axi_dma_runtime_suspend,
> axi_dma_runtime_resume, NULL)
> +};
Have you tried to build with CONFIG_PM disabled?
I'm pretty sure you need __maybe_unused applied to your PM ops.
> +struct axi_dma_chan {
> + struct axi_dma_chip *chip;
> + void __iomem *chan_regs;
> + u8 id;
> + atomic_t descs_allocated;
> +
> + struct virt_dma_chan vc;
> +
> + /* these other elements are all protected by vc.lock */
> + bool is_paused;
I still didn't get (already forgot) why you can't use dma_status instead
for the active descriptor?
> +};
> +/* LLI == Linked List Item */
> +struct __attribute__ ((__packed__)) axi_dma_lli {
...
> + __le64 sar;
> + __le64 dar;
> + __le32 block_ts_lo;
> + __le32 block_ts_hi;
> + __le64 llp;
> + __le32 ctl_lo;
> + __le32 ctl_hi;
> + __le32 sstat;
> + __le32 dstat;
> + __le32 status_lo;
> + __le32 ststus_hi;
> + __le32 reserved_lo;
> + __le32 reserved_hi;
> +};
Just __packed here.
> +
> +struct axi_dma_desc {
> + struct axi_dma_lli lli;
> +
> + struct virt_dma_desc vd;
> + struct axi_dma_chan *chan;
> + struct list_head xfer_list;
This looks redundant. Already asked above about it.
> +};
> +
> +/* Common registers offset */
> +#define DMAC_ID 0x000 /* R DMAC ID */
> +#define DMAC_COMPVER 0x008 /* R DMAC Component Version
> */
> +#define DMAC_CFG 0x010 /* R/W DMAC Configuration */
> +#define DMAC_CHEN 0x018 /* R/W DMAC Channel Enable */
> +#define DMAC_CHEN_L 0x018 /* R/W DMAC Channel Enable
> 00-31 */
> +#define DMAC_CHEN_H 0x01C /* R/W DMAC Channel Enable
> 32-63 */
> +#define DMAC_INTSTATUS 0x030 /* R DMAC Interrupt
> Status */
> +#define DMAC_COMMON_INTCLEAR 0x038 /* W DMAC Interrupt Clear
> */
> +#define DMAC_COMMON_INTSTATUS_ENA 0x040 /* R DMAC Interrupt Status
> Enable */
> +#define DMAC_COMMON_INTSIGNAL_ENA 0x048 /* R/W DMAC Interrupt Signal
> Enable */
> +#define DMAC_COMMON_INTSTATUS 0x050 /* R DMAC Interrupt Status
> */
> +#define DMAC_RESET 0x058 /* R DMAC Reset Register1 */
> +
> +/* DMA channel registers offset */
> +#define CH_SAR 0x000 /* R/W Chan Source
> Address */
> +#define CH_DAR 0x008 /* R/W Chan Destination
> Address */
> +#define CH_BLOCK_TS 0x010 /* R/W Chan Block Transfer
> Size */
> +#define CH_CTL 0x018 /* R/W Chan Control */
> +#define CH_CTL_L 0x018 /* R/W Chan Control 00-31 */
> +#define CH_CTL_H 0x01C /* R/W Chan Control 32-63 */
> +#define CH_CFG 0x020 /* R/W Chan Configuration
> */
> +#define CH_CFG_L 0x020 /* R/W Chan Configuration 00-31
> */
> +#define CH_CFG_H 0x024 /* R/W Chan Configuration 32-63
> */
> +#define CH_LLP 0x028 /* R/W Chan Linked List
> Pointer */
> +#define CH_STATUS 0x030 /* R Chan Status */
> +#define CH_SWHSSRC 0x038 /* R/W Chan SW Handshake
> Source */
> +#define CH_SWHSDST 0x040 /* R/W Chan SW Handshake
> Destination */
> +#define CH_BLK_TFR_RESUMEREQ 0x048 /* W Chan Block Transfer
> Resume Req */
> +#define CH_AXI_ID 0x050 /* R/W Chan AXI ID */
> +#define CH_AXI_QOS 0x058 /* R/W Chan AXI QOS */
> +#define CH_SSTAT 0x060 /* R Chan Source Status */
> +#define CH_DSTAT 0x068 /* R Chan Destination Status */
> +#define CH_SSTATAR 0x070 /* R/W Chan Source Status
> Fetch Addr */
> +#define CH_DSTATAR 0x078 /* R/W Chan Destination
> Status Fetch Addr */
> +#define CH_INTSTATUS_ENA 0x080 /* R/W Chan Interrupt Status
> Enable */
> +#define CH_INTSTATUS 0x088 /* R/W Chan Interrupt
> Status */
> +#define CH_INTSIGNAL_ENA 0x090 /* R/W Chan Interrupt Signal
> Enable */
> +#define CH_INTCLEAR 0x098 /* W Chan Interrupt Clear */
I'm wondering if you can use regmap API instead.
> +
> +
Redundant (one) empty line.
> +/* DMAC_CFG */
> +#define DMAC_EN_MASK 0x00000001U
GENMASK()
> +#define DMAC_EN_POS 0
Usually _SHIFT, but it's up to you.
> +
> +#define INT_EN_MASK 0x00000002U
GENMASK()
> +#define INT_EN_POS 1
> +
_SHIFT ?
> +#define CH_CTL_L_DST_MSIZE_POS 18
> +#define CH_CTL_L_SRC_MSIZE_POS 14
Ditto.
> +enum {
> + DWAXIDMAC_BURST_TRANS_LEN_1 = 0x0,
> + DWAXIDMAC_BURST_TRANS_LEN_4,
> + DWAXIDMAC_BURST_TRANS_LEN_8,
> + DWAXIDMAC_BURST_TRANS_LEN_16,
> + DWAXIDMAC_BURST_TRANS_LEN_32,
> + DWAXIDMAC_BURST_TRANS_LEN_64,
> + DWAXIDMAC_BURST_TRANS_LEN_128,
> + DWAXIDMAC_BURST_TRANS_LEN_256,
> + DWAXIDMAC_BURST_TRANS_LEN_512,
> + DWAXIDMAC_BURST_TRANS_LEN_1024
..._1024, ?
> +};
Hmm... do you need them in the header?
> +#define CH_CFG_H_TT_FC_POS 0
> +enum {
> + DWAXIDMAC_TT_FC_MEM_TO_MEM_DMAC = 0x0,
> + DWAXIDMAC_TT_FC_MEM_TO_PER_DMAC,
> + DWAXIDMAC_TT_FC_PER_TO_MEM_DMAC,
> + DWAXIDMAC_TT_FC_PER_TO_PER_DMAC,
> + DWAXIDMAC_TT_FC_PER_TO_MEM_SRC,
> + DWAXIDMAC_TT_FC_PER_TO_PER_SRC,
> + DWAXIDMAC_TT_FC_MEM_TO_PER_DST,
> + DWAXIDMAC_TT_FC_PER_TO_PER_DST
> +};
Some of definitions are the same as for dw_dmac, right? We might split
them to a common header, though I have no strong opinion about it.
Vinod?
> +/**
> + * Dw axi dma channel interrupts
Dw axi dma - > DW AXI DMA?
> + *
> + * @DWAXIDMAC_IRQ_NONE: Bitmask of no one interrupt
> + * @DWAXIDMAC_IRQ_BLOCK_TRF: Block transfer complete
> + * @DWAXIDMAC_IRQ_DMA_TRF: Dma transfer complete
> + * @DWAXIDMAC_IRQ_SRC_TRAN: Source transaction complete
> + * @DWAXIDMAC_IRQ_DST_TRAN: Destination transaction complete
> + * @DWAXIDMAC_IRQ_SRC_DEC_ERR: Source decode error
> + * @DWAXIDMAC_IRQ_DST_DEC_ERR: Destination decode error
> + * @DWAXIDMAC_IRQ_SRC_SLV_ERR: Source slave error
> + * @DWAXIDMAC_IRQ_DST_SLV_ERR: Destination slave error
> + * @DWAXIDMAC_IRQ_LLI_RD_DEC_ERR: LLI read decode error
> + * @DWAXIDMAC_IRQ_LLI_WR_DEC_ERR: LLI write decode error
> + * @DWAXIDMAC_IRQ_LLI_RD_SLV_ERR: LLI read slave error
> + * @DWAXIDMAC_IRQ_LLI_WR_SLV_ERR: LLI write slave error
> + * @DWAXIDMAC_IRQ_INVALID_ERR: LLI invalide error or Shadow register
> error
> + * @DWAXIDMAC_IRQ_MULTIBLKTYPE_ERR: Slave Interface Multiblock type
> error
> + * @DWAXIDMAC_IRQ_DEC_ERR: Slave Interface decode error
> + * @DWAXIDMAC_IRQ_WR2RO_ERR: Slave Interface write to read only error
> + * @DWAXIDMAC_IRQ_RD2RWO_ERR: Slave Interface read to write only
> error
> + * @DWAXIDMAC_IRQ_WRONCHEN_ERR: Slave Interface write to channel
> error
> + * @DWAXIDMAC_IRQ_SHADOWREG_ERR: Slave Interface shadow reg error
> + * @DWAXIDMAC_IRQ_WRONHOLD_ERR: Slave Interface hold error
> + * @DWAXIDMAC_IRQ_LOCK_CLEARED: Lock Cleared Status
> + * @DWAXIDMAC_IRQ_SRC_SUSPENDED: Source Suspended Status
> + * @DWAXIDMAC_IRQ_SUSPENDED: Channel Suspended Status
> + * @DWAXIDMAC_IRQ_DISABLED: Channel Disabled Status
> + * @DWAXIDMAC_IRQ_ABORTED: Channel Aborted Status
> + * @DWAXIDMAC_IRQ_ALL_ERR: Bitmask of all error interrupts
> + * @DWAXIDMAC_IRQ_ALL: Bitmask of all interrupts
> + */
> +enum {
> + DWAXIDMAC_IRQ_NONE = 0x0,
Just 0.
> + DWAXIDMAC_IRQ_BLOCK_TRF = BIT(0),
> + DWAXIDMAC_IRQ_DMA_TRF = BIT(1),
> + DWAXIDMAC_IRQ_SRC_TRAN = BIT(3),
> + DWAXIDMAC_IRQ_DST_TRAN = BIT(4),
> + DWAXIDMAC_IRQ_SRC_DEC_ERR = BIT(5),
> + DWAXIDMAC_IRQ_DST_DEC_ERR = BIT(6),
> + DWAXIDMAC_IRQ_SRC_SLV_ERR = BIT(7),
> + DWAXIDMAC_IRQ_DST_SLV_ERR = BIT(8),
> + DWAXIDMAC_IRQ_LLI_RD_DEC_ERR = BIT(9),
> + DWAXIDMAC_IRQ_LLI_WR_DEC_ERR = BIT(10),
> + DWAXIDMAC_IRQ_LLI_RD_SLV_ERR = BIT(11),
> + DWAXIDMAC_IRQ_LLI_WR_SLV_ERR = BIT(12),
> + DWAXIDMAC_IRQ_INVALID_ERR = BIT(13),
> + DWAXIDMAC_IRQ_MULTIBLKTYPE_ERR = BIT(14),
> + DWAXIDMAC_IRQ_DEC_ERR = BIT(16),
> + DWAXIDMAC_IRQ_WR2RO_ERR = BIT(17),
> + DWAXIDMAC_IRQ_RD2RWO_ERR = BIT(18),
> + DWAXIDMAC_IRQ_WRONCHEN_ERR = BIT(19),
> + DWAXIDMAC_IRQ_SHADOWREG_ERR = BIT(20),
> + DWAXIDMAC_IRQ_WRONHOLD_ERR = BIT(21),
> + DWAXIDMAC_IRQ_LOCK_CLEARED = BIT(27),
> + DWAXIDMAC_IRQ_SRC_SUSPENDED = BIT(28),
> + DWAXIDMAC_IRQ_SUSPENDED = BIT(29),
> + DWAXIDMAC_IRQ_DISABLED = BIT(30),
> + DWAXIDMAC_IRQ_ABORTED = BIT(31),
> + DWAXIDMAC_IRQ_ALL_ERR = 0x003F7FE0,
GENMASK() | GENMASK() ?
> + DWAXIDMAC_IRQ_ALL = 0xFFFFFFFF
GENMASK()
> +};
--
Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH net-next v3] bindings: net: stmmac: add missing note about LPI interrupt
From: Niklas Cassel @ 2017-04-18 12:39 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, David S. Miller, Joao Pinto,
Niklas Cassel, Alexandre TORGUE, Giuseppe CAVALLARO,
Thierry Reding, Eric Engestrom
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
From: Niklas Cassel <niklas.cassel-VrBV9hrLPhE@public.gmane.org>
The hardware has a LPI interrupt.
There is already code in the stmmac driver to parse and handle the
interrupt. However, this information was missing from the DT binding.
At the same time, improve the description of the existing interrupts.
Signed-off-by: Niklas Cassel <niklas.cassel-VrBV9hrLPhE@public.gmane.org>
---
Documentation/devicetree/bindings/net/stmmac.txt | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/stmmac.txt b/Documentation/devicetree/bindings/net/stmmac.txt
index f652b0c384ce..c3a7be6615c5 100644
--- a/Documentation/devicetree/bindings/net/stmmac.txt
+++ b/Documentation/devicetree/bindings/net/stmmac.txt
@@ -7,9 +7,12 @@ Required properties:
- interrupt-parent: Should be the phandle for the interrupt controller
that services interrupts for this device
- interrupts: Should contain the STMMAC interrupts
-- interrupt-names: Should contain the interrupt names "macirq"
- "eth_wake_irq" if this interrupt is supported in the "interrupts"
- property
+- interrupt-names: Should contain a list of interrupt names corresponding to
+ the interrupts in the interrupts property, if available.
+ Valid interrupt names are:
+ - "macirq" (combined signal for various interrupt events)
+ - "eth_wake_irq" (the interrupt to manage the remote wake-up packet detection)
+ - "eth_lpi" (the interrupt that occurs when Tx or Rx enters/exits LPI state)
- phy-mode: See ethernet.txt file in the same directory.
- snps,reset-gpio gpio number for phy reset.
- snps,reset-active-low boolean flag to indicate if phy reset is active low.
@@ -152,8 +155,8 @@ Examples:
compatible = "st,spear600-gmac";
reg = <0xe0800000 0x8000>;
interrupt-parent = <&vic1>;
- interrupts = <24 23>;
- interrupt-names = "macirq", "eth_wake_irq";
+ interrupts = <24 23 22>;
+ interrupt-names = "macirq", "eth_wake_irq", "eth_lpi";
mac-address = [000000000000]; /* Filled in by U-Boot */
max-frame-size = <3800>;
phy-mode = "gmii";
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox