Re: [PATCH] ASoC: fsl: Check before clk_put() not needed
On Wed, Jun 01, 2022 at 06:14:29AM -0700, Yihao Han wrote: > clk_put() already checks the clk ptr using !clk and IS_ERR() > so there is no need to check it again before calling it. > > Signed-off-by: Yihao Han > --- > sound/soc/fsl/imx-sgtl5000.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c > index 580a0d963f0e..16a281820186 100644 > --- a/sound/soc/fsl/imx-sgtl5000.c > +++ b/sound/soc/fsl/imx-sgtl5000.c > @@ -185,8 +185,7 @@ static int imx_sgtl5000_probe(struct platform_device > *pdev) > put_device: > put_device(_dev->dev); > fail: > - if (data && !IS_ERR(data->codec_clk)) > - clk_put(data->codec_clk); > + clk_put(data->codec_clk); The fail label is used before data is initialized, so the if (data) check is really needed. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] ASoC: fsl_sai: fix 1:1 bclk:mclk ratio support
On Tue, Apr 05, 2022 at 05:57:31PM +0200, Ahmad Fatoum wrote:
> Refactoring in commit a50b7926d015 ("ASoC: fsl_sai: implement 1:1
> bclk:mclk ratio support") led to the bypass never happening
> as (ratio = 1) was caught in the existing if (ratio & 1) continue;
> check. The correct check sequence instead is:
>
> - skip all ratios lower than one and higher than 512
> - skip all odd ratios except for 1:1
> - skip 1:1 ratio if and only if !support_1_1_ratio
>
> And for all others, calculate the appropriate divider. Adjust the
> code to facilitate this.
>
> Fixes: a50b7926d015 ("ASoC: fsl_sai: implement 1:1 bclk:mclk ratio support")
> Signed-off-by: Ahmad Fatoum
Reviewed-by: Sascha Hauer
Sascha
> ---
> sound/soc/fsl/fsl_sai.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> index a992d51568cc..50c377f16097 100644
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -372,7 +372,7 @@ static int fsl_sai_set_bclk(struct snd_soc_dai *dai, bool
> tx, u32 freq)
> continue;
> if (ratio == 1 && !support_1_1_ratio)
> continue;
> - else if (ratio & 1)
> + if ((ratio & 1) && ratio > 1)
> continue;
>
> diff = abs((long)clk_rate - ratio * freq);
> --
> 2.30.2
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 2/3] dts: mpc512x: adjust clock specs for FEC nodes
On Wed, Mar 05, 2014 at 11:52:09AM +0100, Gerhard Sittig wrote: On Wed, Mar 05, 2014 at 09:48 +0800, Shawn Guo wrote: On Mon, Mar 03, 2014 at 10:22:31AM +0100, Gerhard Sittig wrote: On Mon, Feb 24, 2014 at 11:25 +0100, Gerhard Sittig wrote: a recent FEC binding document update that was motivated by i.MX development revealed that ARM and PowerPC implementations in Linux did not agree on the clock names to use for the FEC nodes change clock names from per to ipg in the FEC nodes of the mpc5121.dtsi include file such that the .dts specs comply with the common FEC binding this incompatible change does not break operation, because - COMMON_CLK support for MPC5121/23/25 and adjusted .dts files were only introduced in Linux v3.14-rc1, no mainline release provided these specs before - if this change won't make it for v3.14, the MPC512x CCF support provides full backwards compability, and keeps operating with device trees which lack clock specs or don't match in the names Signed-off-by: Gerhard Sittig g...@denx.de ping Are there opinions about making PowerPC users of FEC use the same clock names as ARM users do, to re-use (actually: keep sharing) the FEC binding? The alternative would be to fragment the FEC binding into several bindings for ARM and PowerPC, which I feel would be undesirable, and is not necessary. As I already said, Documentation/devicetree/bindings/net/fsl-fec.txt was created specifically for i.MX FEC controller from day one. And even as of today, it doesn't serve PowerPC, because for example the property 'phy-mode' documented as required one is not required by PowerPC FEC. My opinion would be to patch fsl-fec.txt a little bit to make it clear that it's a binding doc for i.MX FEC, and create the other one for PowerPC FEC. This is the way less confusing to people and easier for binding maintenance. Should we still try to have a common behaviour where possible? Such that even if there are two bindings, they don't diverge in unnecessary ways? Maybe the long term goal should be to share the code. The MPC5200 FEC and the i.MX FEC are very similar. Only the DMA engine is quite different. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 1/4] dma: imx-sdma: Add sdma firmware version 2 support
On Thu, Oct 31, 2013 at 02:32:03PM +0800, Nicolin Chen wrote:
On i.MX5/6 series, SDMA is using new version firmware to support SSI
dual FIFO feature and HDMI Audio (i.MX6Q/DL only). Thus add it.
Signed-off-by: Nicolin Chen b42...@freescale.com
---
diff --git a/include/linux/platform_data/dma-imx-sdma.h
b/include/linux/platform_data/dma-imx-sdma.h
index 3a39428..14ca582 100644
--- a/include/linux/platform_data/dma-imx-sdma.h
+++ b/include/linux/platform_data/dma-imx-sdma.h
@@ -43,6 +43,9 @@ struct sdma_script_start_addrs {
s32 dptc_dvfs_addr;
s32 utra_addr;
s32 ram_code_start_addr;
You could add a comment here, like: /* End of v1 array */
Otherwise:
Acked-by: Sascha Hauer s.ha...@pengutronix.de
+ s32 mcu_2_ssish_addr;
+ s32 ssish_2_mcu_addr;
+ s32 hdmi_dma_addr;
};
/**
--
1.8.4
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/3] dma: imx-sdma: Add ssi dual fifo script support
On Tue, Oct 29, 2013 at 08:33:15PM +0800, Nicolin Chen wrote:
There's a script for SSI missing in current sdma script list. Thus add it.
This script would allow SSI use its dual fifo mode to transimit/receive
data without occasional hardware underrun/overrun.
This patch also fixed a counting error for total number of scripts.
Look at drivers/dma/imx-sdma.c:
/**
* struct sdma_firmware_header - Layout of the firmware image
*
* @magic SDMA
* @version_major increased whenever layout of struct
* sdma_script_start_addrs
*changes.
Can you image why this firmware has a version field? Right, it's because
it encodes the layout of struct sdma_script_start_addrs.
As the comment clearly states you have to *increase this field* when you
add scripts.
Obviously you missed that, as the firmware on lkml posted recently
shows:
: 414d4453 0001 0001 001c SDMA
Still '1'
0010: 0026 00b4 067a 0282 ...z...
0020:
0030:
0040: 1a6a j...
0050: 02eb 18bb 0408
0060: 03c0
0070: 02ab 037b {...
0080: 044c 046e L...n...
0090: 1800
00a0: 1800 1862 1a16 b...
^
new script addresses introduced
-#define SDMA_SCRIPT_ADDRS_ARRAY_SIZE_V1 34
+#define SDMA_SCRIPT_ADDRS_ARRAY_SIZE_V1 37
And no, this is not a bug. It's your firmware header that is buggy.
What you need is:
#define SDMA_SCRIPT_ADDRS_ARRAY_SIZE_V2 37
You (you as a company, not you as a person) knew that it was me who
created this firmware format. So it was absolutely unnecessary to create
an incompatible firmware instead of dropping me a short note.
Please add a version check to the driver as necessary and provide a proper
firmware.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Fri, Aug 23, 2013 at 12:49:19AM +0200, Tomasz Figa wrote: On Thursday 22 of August 2013 15:43:31 Mike Turquette wrote: Quoting Sascha Hauer (2013-08-22 14:00:35) On Thu, Aug 22, 2013 at 01:09:31PM +0100, Mark Rutland wrote: On Thu, Aug 22, 2013 at 08:19:10AM +0100, Mike Turquette wrote: Quoting Tomasz Figa (2013-08-21 14:34:55) On Wednesday 21 of August 2013 09:50:15 Mark Rutland wrote: On Tue, Aug 20, 2013 at 01:06:25AM +0100, Mike Turquette wrote: Quoting Mark Rutland (2013-08-19 02:35:43) On Sat, Aug 17, 2013 at 04:17:18PM +0100, Tomasz Figa wrote: On Saturday 17 of August 2013 16:53:16 Sascha Hauer wrote: On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa wrote: Also I would make this option required. Use a dummy clock for mux inputs that are grounded for a specific SoC. Some clocks are not from CCM and we haven't defined in imx6q-clk.txt, so in most cases we can't provide a phandle for them, eg: spdif_ext. I think it's a bit hard to force it to be 'required'. An 'optional' looks more flexible to me and a default one is ensured even if it's missing. clks 0 is the dummy clock. This can be used for all input clocks not defined by the SoC. Where does this assumption come from? Is it documented anywhere? This is how all i.MX clock bindings currently are. See Documentation/devicetree/bindings/clock/imx*-clock.txt OK, thanks. I guess we need some discussion on dummy clocks vs skipped clocks. I think we want some consistency on this, don't we? If we really need a dummy clock, then we might also want a generic way to specify it. What do we actually mean by a dummy clock? We already have bindings for fixed-clock and co friends describe relatively simple preconfigured clocks. Some platforms have a fake clock which defines noops callbacks and basically doesn't do anything. This is analogous to the dummy regulator implementation. A central one could be registered by the clock core, as is done by the regulator core. When you say some platforms, you presumably mean the platform code in Linux? A dummy clock sounds like a completely Linux-specific abstraction rather than a description of the hardware, and I don't see why we need that in the DT: * If a clock is wired up and running (as presumably the dummy clock is), then surely it's a fixed-clock (it's running, we and we have no control over it, but we presumably know its rate) and can be described as such? * If no clock is wired up, then we should be able to describe that. If a driver believes that a clock is required when it isn't (for some level of functionality), then that driver should be fixed up to support the clock as being optional. Am I missing something? I second that. Moreover, I don't think that device tree should deal with dummy anything. It should be able to describe hardware that is available on given system, not list what hardware is not available. I wasn't clear. The dummy clock IS a completely Linux-specific abstraction. I'm not advocating a dummy clock in DT. I am advocating consolidation of the implementation of a clock that does nothing into the clock core. This code could easily live in drivers/clk/clk.c instead of having everyone open-code it. As far as specifying a dummy clock in DT? I dunno. DT should describe real hardware so there isn't much use for a dummy clock. Sorry, I misunderstood. Good to hear we're on the same page :) I'm guessing one of the reasons for such a clock are drivers do not honor the clk.h api and they freak out when clk_get gives them a NULL pointer? I'm not sure. Sascha, could you shed some light on the matter? The original reason introducing the dummy clocks in the i.MX dtbs was to provide devices a clock which the driver requests but is not software controllable. We often have the case where the same devices are on several SoCs, but not on all of them all clocks have a bit to en/disable them. Anyway, to accomplish this we don't need
Re: [alsa-devel] [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Fri, Aug 23, 2013 at 01:58:15PM +0100, Mark Rutland wrote: On Fri, Aug 23, 2013 at 07:34:21AM +0100, Sascha Hauer wrote: On Fri, Aug 23, 2013 at 12:49:19AM +0200, Tomasz Figa wrote: On Thursday 22 of August 2013 15:43:31 Mike Turquette wrote: Quoting Sascha Hauer (2013-08-22 14:00:35) On Thu, Aug 22, 2013 at 01:09:31PM +0100, Mark Rutland wrote: On Thu, Aug 22, 2013 at 08:19:10AM +0100, Mike Turquette wrote: Quoting Tomasz Figa (2013-08-21 14:34:55) On Wednesday 21 of August 2013 09:50:15 Mark Rutland wrote: On Tue, Aug 20, 2013 at 01:06:25AM +0100, Mike Turquette wrote: Quoting Mark Rutland (2013-08-19 02:35:43) On Sat, Aug 17, 2013 at 04:17:18PM +0100, Tomasz Figa wrote: On Saturday 17 of August 2013 16:53:16 Sascha Hauer wrote: On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa wrote: Also I would make this option required. Use a dummy clock for mux inputs that are grounded for a specific SoC. Some clocks are not from CCM and we haven't defined in imx6q-clk.txt, so in most cases we can't provide a phandle for them, eg: spdif_ext. I think it's a bit hard to force it to be 'required'. An 'optional' looks more flexible to me and a default one is ensured even if it's missing. clks 0 is the dummy clock. This can be used for all input clocks not defined by the SoC. Where does this assumption come from? Is it documented anywhere? This is how all i.MX clock bindings currently are. See Documentation/devicetree/bindings/clock/imx*-clock.txt OK, thanks. I guess we need some discussion on dummy clocks vs skipped clocks. I think we want some consistency on this, don't we? If we really need a dummy clock, then we might also want a generic way to specify it. What do we actually mean by a dummy clock? We already have bindings for fixed-clock and co friends describe relatively simple preconfigured clocks. Some platforms have a fake clock which defines noops callbacks and basically doesn't do anything. This is analogous to the dummy regulator implementation. A central one could be registered by the clock core, as is done by the regulator core. When you say some platforms, you presumably mean the platform code in Linux? A dummy clock sounds like a completely Linux-specific abstraction rather than a description of the hardware, and I don't see why we need that in the DT: * If a clock is wired up and running (as presumably the dummy clock is), then surely it's a fixed-clock (it's running, we and we have no control over it, but we presumably know its rate) and can be described as such? * If no clock is wired up, then we should be able to describe that. If a driver believes that a clock is required when it isn't (for some level of functionality), then that driver should be fixed up to support the clock as being optional. Am I missing something? I second that. Moreover, I don't think that device tree should deal with dummy anything. It should be able to describe hardware that is available on given system, not list what hardware is not available. I wasn't clear. The dummy clock IS a completely Linux-specific abstraction. I'm not advocating a dummy clock in DT. I am advocating consolidation of the implementation of a clock that does nothing into the clock core. This code could easily live in drivers/clk/clk.c instead of having everyone open-code it. As far as specifying a dummy clock in DT? I dunno. DT should describe real hardware so there isn't much use for a dummy clock. Sorry, I misunderstood. Good to hear we're on the same page :) I'm guessing one of the reasons for such a clock are drivers do not honor the clk.h api and they freak out when clk_get gives them a NULL pointer? I'm not sure. Sascha, could you shed some
Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Thu, Aug 22, 2013 at 01:09:31PM +0100, Mark Rutland wrote: On Thu, Aug 22, 2013 at 08:19:10AM +0100, Mike Turquette wrote: Quoting Tomasz Figa (2013-08-21 14:34:55) On Wednesday 21 of August 2013 09:50:15 Mark Rutland wrote: On Tue, Aug 20, 2013 at 01:06:25AM +0100, Mike Turquette wrote: Quoting Mark Rutland (2013-08-19 02:35:43) On Sat, Aug 17, 2013 at 04:17:18PM +0100, Tomasz Figa wrote: On Saturday 17 of August 2013 16:53:16 Sascha Hauer wrote: On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa wrote: Also I would make this option required. Use a dummy clock for mux inputs that are grounded for a specific SoC. Some clocks are not from CCM and we haven't defined in imx6q-clk.txt, so in most cases we can't provide a phandle for them, eg: spdif_ext. I think it's a bit hard to force it to be 'required'. An 'optional' looks more flexible to me and a default one is ensured even if it's missing. clks 0 is the dummy clock. This can be used for all input clocks not defined by the SoC. Where does this assumption come from? Is it documented anywhere? This is how all i.MX clock bindings currently are. See Documentation/devicetree/bindings/clock/imx*-clock.txt OK, thanks. I guess we need some discussion on dummy clocks vs skipped clocks. I think we want some consistency on this, don't we? If we really need a dummy clock, then we might also want a generic way to specify it. What do we actually mean by a dummy clock? We already have bindings for fixed-clock and co friends describe relatively simple preconfigured clocks. Some platforms have a fake clock which defines noops callbacks and basically doesn't do anything. This is analogous to the dummy regulator implementation. A central one could be registered by the clock core, as is done by the regulator core. When you say some platforms, you presumably mean the platform code in Linux? A dummy clock sounds like a completely Linux-specific abstraction rather than a description of the hardware, and I don't see why we need that in the DT: * If a clock is wired up and running (as presumably the dummy clock is), then surely it's a fixed-clock (it's running, we and we have no control over it, but we presumably know its rate) and can be described as such? * If no clock is wired up, then we should be able to describe that. If a driver believes that a clock is required when it isn't (for some level of functionality), then that driver should be fixed up to support the clock as being optional. Am I missing something? I second that. Moreover, I don't think that device tree should deal with dummy anything. It should be able to describe hardware that is available on given system, not list what hardware is not available. I wasn't clear. The dummy clock IS a completely Linux-specific abstraction. I'm not advocating a dummy clock in DT. I am advocating consolidation of the implementation of a clock that does nothing into the clock core. This code could easily live in drivers/clk/clk.c instead of having everyone open-code it. As far as specifying a dummy clock in DT? I dunno. DT should describe real hardware so there isn't much use for a dummy clock. Sorry, I misunderstood. Good to hear we're on the same page :) I'm guessing one of the reasons for such a clock are drivers do not honor the clk.h api and they freak out when clk_get gives them a NULL pointer? I'm not sure. Sascha, could you shed some light on the matter? The original reason introducing the dummy clocks in the i.MX dtbs was to provide devices a clock which the driver requests but is not software controllable. We often have the case where the same devices are on several SoCs, but not on all of them all clocks have a bit to en/disable them. Anyway, to accomplish this we don't need dummy clocks. We can just describe the real clocks. BTW with the S/PDIF core on which not all mux inputs are connected to actual clocks we could also describe the unconnected inputs as ground clocks with rate 0. This way we describe something which is really there instead of dummy clocks ;) Background to why it might be a good idea to connect a ground clock to the unconnected input pins is that a driver has a chance to successfully grab all clocks. Otherwise how does the driver distinguish between an unconnected and an erroneous clock? Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http
Re: [PATCH v8 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
Hi Nicolas,
Some misc other comments inline.
On Mon, Aug 19, 2013 at 08:08:48PM +0800, Nicolin Chen wrote:
This patch implements a device-tree-only CPU DAI driver for Freescale
+
+struct fsl_spdif_priv {
+ struct spdif_mixer_control fsl_spdif_control;
+ struct snd_soc_dai_driver cpu_dai_drv;
+ struct platform_device *pdev;
+ struct regmap *regmap;
+ atomic_t dpll_locked;
You don't need an atomic_t to track a bool variable. Use a plain bool or
int instead.
+ u8 txclk_div[SPDIF_TXRATE_MAX];
+ u8 txclk_src[SPDIF_TXRATE_MAX];
+ u8 rxclk_src;
+ struct clk *txclk[SPDIF_TXRATE_MAX];
+ struct clk *rxclk;
+ struct snd_dmaengine_dai_dma_data dma_params_tx;
+ struct snd_dmaengine_dai_dma_data dma_params_rx;
+
+ /* The name space will be allocated dynamically */
+ char name[0];
+};
+
+
+#ifdef DEBUG
+static void dumpregs(struct fsl_spdif_priv *spdif_priv)
+{
+ struct regmap *regmap = spdif_priv-regmap;
+ struct platform_device *pdev = spdif_priv-pdev;
+ u32 val, i;
+ int ret;
+
+ /* Valid address set of SPDIF is {[0x0-0x38], 0x44, 0x50} */
+ for (i = 0 ; i = REG_SPDIF_STC; i += 4) {
+ ret = regmap_read(regmap, REG_SPDIF_SCR + i, val);
+ if (!ret)
+ dev_dbg(pdev-dev, REG 0x%02x = 0x%06x\n, i, val);
+ }
+}
+#else
+static void dumpregs(struct fsl_spdif_priv *spdif_priv) {}
+#endif
Is this needed? regmap provides a register dump in debugfs.
+
+static int spdif_clk_set_rate(struct clk *clk, unsigned long rate)
+{
+ unsigned long rate_actual;
+
+ rate_actual = clk_round_rate(clk, rate);
+ return clk_set_rate(clk, rate_actual);
+}
clk_round_rate returns the rate which clk_set_rate would set if called
with the same rate. The clk_round_rate() is unnecessary.
+
+ dev_dbg(pdev-dev, expected clock rate = %d\n,
+ (int)(64 * sample_rate * div));
+ dev_dbg(pdev-dev, acutal clock rate = %d\n,
+ (int)clk_get_rate(spdif_priv-txclk[rate]));
s/acutal/actual/
Also please use %ld instead of casting the unsigned long to int.
+
+ dev_dbg(pdev-dev, FreqMeas: %d\n, (int)freqmeas);
+ dev_dbg(pdev-dev, BusclkFreq: %d\n, (int)busclk_freq);
+ dev_dbg(pdev-dev, RxRate: %d\n, (int)tmpval64);
Get rid of the casts
+
+ spdif_priv = devm_kzalloc(pdev-dev,
+ sizeof(struct fsl_spdif_priv) + strlen(np-name) + 1,
GFP_KERNEL);
+ if (!spdif_priv) {
+ dev_err(pdev-dev, could not allocate DAI object\n);
Please drop this message. You'll never see it.
+
+ for (i = 0; i SPDIF_TXRATE_MAX; i++) {
+ ret = fsl_spdif_probe_txclk(spdif_priv, i);
+ if (ret)
+ return ret;
+ }
+
+ /* Prepare rx/tx clock */
+ clk_prepare(spdif_priv-rxclk);
+ for (i = 0; i SPDIF_TXRATE_MAX; i++)
+ clk_prepare(spdif_priv-txclk[i]);
Why do you prepare all clocks instead of the one you actually use? Also,
no need to do this here. You can use clk_prepare_enable instead where
you have clk_enable now.
+
+/* SPDIF rx clock source */
+enum spdif_rxclk_src {
+ SRPC_CLKSRC_0 = 0,
+ SRPC_CLKSRC_1,
+ SRPC_CLKSRC_2,
+ SRPC_CLKSRC_3,
+ SRPC_CLKSRC_4,
+ SRPC_CLKSRC_5,
+ SRPC_CLKSRC_6,
+ SRPC_CLKSRC_7,
+ SRPC_CLKSRC_8,
+ SRPC_CLKSRC_9,
+ SRPC_CLKSRC_10,
+ SRPC_CLKSRC_11,
+ SRPC_CLKSRC_12,
+ SRPC_CLKSRC_13,
+ SRPC_CLKSRC_14,
+ SRPC_CLKSRC_15,
+};
These are unused and look unnecessary.
+
+/* SPDIF tx clksrc */
+enum spdif_txclk_src {
+ STC_TXCLK_SRC_0 = 0,
+ STC_TXCLK_SRC_1,
+ STC_TXCLK_SRC_2,
+ STC_TXCLK_SRC_3,
+ STC_TXCLK_SRC_4,
+ STC_TXCLK_SRC_5,
+ STC_TXCLK_SRC_6,
+ STC_TXCLK_SRC_7,
+};
Also unused.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa wrote: Also I would make this option required. Use a dummy clock for mux inputs that are grounded for a specific SoC. Some clocks are not from CCM and we haven't defined in imx6q-clk.txt, so in most cases we can't provide a phandle for them, eg: spdif_ext. I think it's a bit hard to force it to be 'required'. An 'optional' looks more flexible to me and a default one is ensured even if it's missing. clks 0 is the dummy clock. This can be used for all input clocks not defined by the SoC. Where does this assumption come from? Is it documented anywhere? This is how all i.MX clock bindings currently are. See Documentation/devicetree/bindings/clock/imx*-clock.txt Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Sat, Aug 17, 2013 at 02:26:40PM +0200, Tomasz Figa wrote: You mean tx0-7. Also I would make this option required. Use a dummy clock for mux inputs that are grounded for a specific SoC. Why do you need a dummy clock? The driver can simply try to grab all the possible clocks and discard those that failed, so you can just keep those grounded clocks unspecified. We don't need dummy clocks. My motivation saying this that I was afraid people try to configure the driver by skipping the clocks they don't want from the devicetree. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Sat, Aug 17, 2013 at 02:56:11PM +0200, Tomasz Figa wrote: Hi Nicolin, First, you are right that all the properties you just commented are software configurations. And I got the point that device tree now can't allow any software configuration even if the actual hardware connection will depend on it. If so, I would like to remove those abused clocks and also drop the unused clocks in src0-7, then just remain those needed clocks src. I think that can be plausible because there'll be no more clock abuse and the driver will be able to get the source index from the name 'srcnum'. OK. And you are right about the 9 clock inputs, just there're not only 9 inputs but also an extra external clock from S/PDIF transmitter via coaxial cable or optical fiber -- RxCLK. Please check the following list: if (DPLL Locked) SPDIF_RxClk else extal 0001 if (DPLL Locked) SPDIF_RxClk else spdif_clk 0010 if (DPLL Locked) SPDIF_RxClk else asrc_clk 0011 if (DPLL Locked) SPDIF_RxClk else spdif_extclk 0100 if (DPLL Locked) SPDIF_Rxclk else esai_hckt 0101 extal_clk 0110 spdif_clk 0111 asrc_clk 1000 spdif_extclk 1001 esai_hckt 1010 if (DPLL Locked) SPDIF_RxClk else mlb_clk 1011 if (DPLL Locked) SPDIF_RxClk else mlb_phy_clk 1100 mkb_clk 1101 mlb_phy_clk Could you explain what the above values are? If they are values written to a 4-bit mux that selects RX clock source, then all the 16 clocks should be specified from device tree, even if they are duplicated. The S/PDIF core can recover the clock for the tx signal from the rx signal. So if you have an S/PDIF input signal, then the DPLL will be locked and the SPDIF_RxClk can be used for tx. So the above are really 8 clocks and one If DPLL locked, use it bit. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Sat, Aug 17, 2013 at 05:13:20PM +0200, Tomasz Figa wrote: On Saturday 17 of August 2013 17:00:02 Sascha Hauer wrote: On Sat, Aug 17, 2013 at 02:26:40PM +0200, Tomasz Figa wrote: You mean tx0-7. Also I would make this option required. Use a dummy clock for mux inputs that are grounded for a specific SoC. Why do you need a dummy clock? The driver can simply try to grab all the possible clocks and discard those that failed, so you can just keep those grounded clocks unspecified. We don't need dummy clocks. My motivation saying this that I was afraid people try to configure the driver by skipping the clocks they don't want from the devicetree. I'm not really sure if the same abuse couldn't be easily achieved by putting dummy clocks in place of those skipped clocks. That's right, yes. Adding a note in binding documentation that says that all clocks that are fed to the IP shall be specified should be fine IMHO. ok. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Fri, Aug 16, 2013 at 12:43:31PM +0800, Nicolin Chen wrote: Hi Tomasz, Thank you for the comments. I'll revise them in v6. And below is my reply for you comments. On Thu, Aug 15, 2013 at 02:18:22PM +0200, Tomasz Figa wrote: + - clock-names : Includes the following entries: + nametypecomments + core RequiredThe core clock of spdif controller + + rxOptionalRx clock source for spdif record. + If absent, will use core clock. + + txOptionalTx clock source for spdif playback. + If absent, will use core clock. + + tx-32000 OptionalTx clock source for 32000Hz sample rate + playback. If absent, will use tx clock. + + tx-44100 OptionalTx clock source for 44100Hz sample rate + playback. If absent, will use tx clock. + + tx-48000 OptionalTx clock source for 48000Hz sample rate + playback. If absent, will use tx clock. + + src0-7 OptionalClock source list for tx and rx clock + to look up their clock source indexes. + This clock list should be identical to + the list of TxClk_Source bit value of + register SPDIF_STC. If absent or failed + to look up, tx and rx clock would then + ignore the rx, tx tx-32000, + tx-44100, tx-48000 clock phandles + and select the core clock as default + tx and rx clock. I suspect a little abuse of clocks property here. From the description of core and src0-7 clocks I assume that the IP can have up to 9 clock inputs - core clock and up to 8 extra source clocks. Is it correct? If yes, this makes the tx, rx and tx-* clocks describe configuration, not hardware. IMHO it should be up to the driver which source clocks to use for tx and rx channels and for each sampling rate. First, you are right that all the properties you just commented are software configurations. And I got the point that device tree now can't allow any software configuration even if the actual hardware connection will depend on it. If so, I would like to remove those abused clocks and also drop the unused clocks in src0-7, then just remain those needed clocks src. I think that can be plausible because there'll be no more clock abuse and the driver will be able to get the source index from the name 'srcnum'. And you are right about the 9 clock inputs, just there're not only 9 inputs but also an extra external clock from S/PDIF transmitter via coaxial cable or optical fiber -- RxCLK. Please check the following list: if (DPLL Locked) SPDIF_RxClk else extal 0001 if (DPLL Locked) SPDIF_RxClk else spdif_clk 0010 if (DPLL Locked) SPDIF_RxClk else asrc_clk 0011 if (DPLL Locked) SPDIF_RxClk else spdif_extclk 0100 if (DPLL Locked) SPDIF_Rxclk else esai_hckt 0101 extal_clk 0110 spdif_clk 0111 asrc_clk 1000 spdif_extclk 1001 esai_hckt 1010 if (DPLL Locked) SPDIF_RxClk else mlb_clk 1011 if (DPLL Locked) SPDIF_RxClk else mlb_phy_clk 1100 mkb_clk 1101 mlb_phy_clk When (DPLL Locked) condition matches, the rx clock can ignore the 8 input clocks from clock mux then use the external one from a S/PDIF transmitter. So extal, spdif_clk, asrc_clk, spdif_extclk, esai_hckt, mlb_clk and mlb_phy_clk are clocks provided to the S/PDIF core and thus should be described in the devicetree. Which of them the driver should use is configuration and thus normally should *not* be described in the devicetree. However, there may be no good way for the driver to know which clock to use in which case. There may be additional board requirements which are unknown to the driver. So in this case it might be valid to put the information which clock to use into the devicetree. But be aware that from the moment you put this information into the devicetree the driver is no longer free to chose the best clock, even if in future we find a good way to automatically guess the best clock. Do you have some insights in which case I would use which input clock? Is this only about which clock has the best suitable input frequency or is this also about synchronization of the audio signal with some other unit? Likewise with the rx-clksrc-lock property. what are the reasons to enable/disable this property? Is this an option you want to change during runtime? Is it an option you always want to use when it's available? If you don't know the answer then it might be a good option to just let the driver pick a sane default. If someone feels the need
Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Fri, Aug 16, 2013 at 04:01:25PM +0800, Nicolin Chen wrote: Hi Sascha, Thank you for the detailed comments. On Fri, Aug 16, 2013 at 09:08:18AM +0200, Sascha Hauer wrote: Which of them the driver should use is configuration and thus normally should *not* be described in the devicetree. However, there may be no good way for the driver to know which clock to use in which case. There may be additional board requirements which are unknown to the driver. So in this case it might be valid to put the information which clock to use into the devicetree. But be aware that from the moment you put this information into the devicetree the driver is no longer free to chose the best clock, even if in future we find a good way to automatically guess the best clock. Do you have some insights in which case I would use which input clock? Is this only about which clock has the best suitable input frequency or is this also about synchronization of the audio signal with some other unit? I understand. What I'm thinking now is to let the driver find the best clock source for tx clock and a correspond divisor like this: tx0-8 OptionalTx clock source for spdif playback. If absent, will use core clock. The index from 0 to 8 is identical to the clock source list described in TxClk_Source bit of register STC. Multiple clock source are allowed for this tx clock source. The driver will select one source from them for each supported sample rate according to the clock rates of these provided clock sources. You mean tx0-7. Also I would make this option required. Use a dummy clock for mux inputs that are grounded for a specific SoC. Please review this idea. And likewise for rx: rx0-16OptionalRx clock source for spdif record. If absent, will use core clock. The index from 0 to 16 is identical to the clock source list described in ClkSrc_Sel bit of register SRPC. If the index provided contains an if (DPLL Locked) condition in its source, the correspond clock phandle should be the one in else path. Only one rx clock source should be defined here. Again, describe the input clocks *to* *the* *S/PDIF* *core* in the devicetree. Nothing more, nothing less. We've already been at the point where we realized that half of the above clocks only describe the 'PDLL locked' condition. Also the tx clocks are from what I see identical to the rx clocks. The following are the clocks: clock-names: core, rxtx0-7 Required. The S/PDIF core has a core clock and 8 clocks which are muxed internally to provide input/output sample clocks. This is all binding that is needed for now. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Fri, Aug 16, 2013 at 05:53:58PM +0800, Nicolin Chen wrote: On Fri, Aug 16, 2013 at 10:56:32AM +0200, Sascha Hauer wrote: tx0-8 OptionalTx clock source for spdif playback. If absent, will use core clock. The index from 0 to 8 is identical to the clock source list described in TxClk_Source bit of register STC. Multiple clock source are allowed for this tx clock source. The driver will select one source from them for each supported sample rate according to the clock rates of these provided clock sources. You mean tx0-7 Yes. Thank you. Also I would make this option required. Use a dummy clock for mux inputs that are grounded for a specific SoC. Some clocks are not from CCM and we haven't defined in imx6q-clk.txt, so in most cases we can't provide a phandle for them, eg: spdif_ext. I think it's a bit hard to force it to be 'required'. An 'optional' looks more flexible to me and a default one is ensured even if it's missing. clks 0 is the dummy clock. This can be used for all input clocks not defined by the SoC. spdif_ext would be a fixed clock on boards which provide it, but wiring this up would be the job of the board maintainer. Again, describe the input clocks *to* *the* *S/PDIF* *core* in the devicetree. Nothing more, nothing less. We've already been at the point where we realized that half of the above clocks only describe the 'PDLL locked' condition. Also the tx clocks are from what I see identical to the rx clocks. The following are the clocks: clock-names: core, rxtx0-7 Required. The S/PDIF core has a core clock and 8 clocks which are muxed internally to provide input/output sample clocks. I know the reason why you suggest to combine two into 'rxtx0-7' is because the clock mux is defined so. And the previous suggestion 'the option required' is also because of it. But actually the rxclk itself, can be not only routed from the clock mux but also derived from DPLL of SPDIF Rx BLOCK as well. So, IMHO, it's more likely to be a fact that rxclk actually has 9 clock source, 8 from mux and 1 from DPLL. But why we here have to exclude it? Because this is configuration, not hardware description. WELL anyway, I know my opinion might not be concerned so much. So I would like to follow the suggestion as an expediency because I still wish this patch could be finally applied and merged into mainline :( But I still don't get why we need to be so obsessed to make this impenetrable rule of devicetree that we here have to sacrifice something we could have reasonably done. Look, it's really simple. Define the binding in a way that describes the hardware. Then use some sensible default in the driver for which clock to use. This doesn't have to cover all possible usecases, it only has to work. This is all that is necessary to get this driver mainline and will make most users happy. Then, later, someone might come along who needs more fine grained control over the clocks, but this guy is then able to justify *why* more control is needed. This is not about getting a full featured driver into mainline. Get a basic driver into mainline and improve it later. You'll make it easier for us all. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 resent 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Mon, Aug 12, 2013 at 08:05:27PM +0800, Nicolin Chen wrote:
This patch add S/PDIF controller driver for Freescale SoC.
Signed-off-by: Nicolin Chen b42...@freescale.com
+
+Required properties:
+
+ - compatible : Compatible list, contains fsl,chip-spdif. Using general
+ fsl,fsl-spdif will get the default SoC type -- imx6q-spdif.
+
+ - reg : Offset and length of the register set for the device.
+
+ - interrupts : Contains spdif interrupt.
+
+ - dmas : Generic dma devicetree binding as described in
+ Documentation/devicetree/bindings/dma/dma.txt.
+
+ - dma-names : Two dmas have to be defined, tx and rx.
+
+ - clocks : Contains an entry for each entry in clock-names.
+
+ - clock-names : Includes the following entries:
+ nametypecomments
+ core RequiredThe core clock of spdif controller
+ rxOptionalRx clock source for spdif record.
+ If absent, will use core clock.
+ txOptionalTx clock source for spdif playback.
+ If absent, will use core clock.
+ tx-32000 OptionalTx clock source for 32000Hz sample rate
+ playback. If absent, will use tx clock.
+ tx-44100 OptionalTx clock source for 44100Hz sample rate
+ playback. If absent, will use tx clock.
+ tx-48000 OptionalTx clock source for 48000Hz sample rate
+ playback. If absent, will use tx clock.
+
+ - tx-clksrc-names : The names for all available clock sources for tx, which
+ is also being listed in SoC reference manual, ClkSrc_Sel bit of SPDIF_SRPC.
+ And the name list would be different between different SoC. Use 'null' for
+ those unlisted names, and the max number of tx-clksrc-names should be 8.
+
+ - rx-clksrc-names : The names for all available clock sources for rx, which
+ is also being listed in SoC reference manual, TxClk_Source bit of
SPDIF_STC.
+ And the name list would be different between different SoC. Use 'null' for
+ those unlisted names, and the max number of rx-clksrc-names should be 16.
+
+Optional properties:
+
+ - rx-clksrc-lock: This is a boolean property. If present, ClkSrc_Sel bit
+ of SPDIF_SRPC would be set a clock source that cares DPLL locked condition.
+
+Example1:
+
+spdif: spdif@02004000 {
+ compatible = fsl,imx6q-spdif;
+ reg = 0x02004000 0x4000;
+ interrupts = 0 52 0x04;
+ dmas = sdma 14 18 0,
+sdma 15 18 0;
+ dma-names = rx, tx;
+
+ clocks = clks 197;
+ clock-names = core;
+ rx-clksrc-lock;
+ rx-clksrc-names =
+ lock.ext, lock.spdif, lock.asrc,
+ lock.spdif_ext, lock.esai, ext,
+ spdif, asrc, spdif_ext, esai,
+ lock.mlb, lock.mlb_phy, mlb,
+ mlb_phy;
+ tx-clksrc-names =
+ xtal, spdif, asrc, spdif_ext,
+ esai, ipg, mlb, mlb_phy;
I had a hard time understanding what you are doing here.
With this the clk names in arch/arm/mach-imx/clk-imx6q.c become an API
between the Kernel and the devicetree. Don't do that.
There is a standardized devicetree binding for clocks. Use it.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH v4 resent 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Wed, Aug 14, 2013 at 04:48:02PM +0800, Nicolin Chen wrote:
Hi Sascha,
On Wed, Aug 14, 2013 at 09:50:17AM +0200, Sascha Hauer wrote:
+ - tx-clksrc-names : The names for all available clock sources for tx,
which
+ is also being listed in SoC reference manual, ClkSrc_Sel bit of
SPDIF_SRPC.
+ And the name list would be different between different SoC. Use 'null'
for
+ those unlisted names, and the max number of tx-clksrc-names should be
8.
+
+ - rx-clksrc-names : The names for all available clock sources for rx,
which
+ is also being listed in SoC reference manual, TxClk_Source bit of
SPDIF_STC.
+ And the name list would be different between different SoC. Use 'null'
for
+ those unlisted names, and the max number of rx-clksrc-names should be
16.
+
+Optional properties:
+
+ - rx-clksrc-lock: This is a boolean property. If present, ClkSrc_Sel
bit
+ of SPDIF_SRPC would be set a clock source that cares DPLL locked
condition.
+
+Example1:
+
+spdif: spdif@02004000 {
+ clocks = clks 197;
+ clock-names = core;
+ rx-clksrc-lock;
+ rx-clksrc-names =
+ lock.ext, lock.spdif, lock.asrc,
+ lock.spdif_ext, lock.esai, ext,
+ spdif, asrc, spdif_ext, esai,
+ lock.mlb, lock.mlb_phy, mlb,
+ mlb_phy;
+ tx-clksrc-names =
+ xtal, spdif, asrc, spdif_ext,
+ esai, ipg, mlb, mlb_phy;
I had a hard time understanding what you are doing here.
With this the clk names in arch/arm/mach-imx/clk-imx6q.c become an API
between the Kernel and the devicetree. Don't do that.
There is a standardized devicetree binding for clocks. Use it.
I think I should first explain to you what this part is doing:
The driver needs to set Clk_source bit for TX/RX to select the
clock from a clock mux. The names listed above are those of the
clocks connecting to the mux, while they are not only internal
clocks which're included in clk-imx6q.c but also external ones,
an on-board external osc for example.
The driver does get the clock by using the standard DT binding,
see the 'clocks = clks 197' above, and then compare this
obtained clock-name with the name list to decide which value
should be set to the Clk_source bit.
==
ClkSrc_Sel from i.MX6Q reference manual:
Clock source selection, all other settings not shown are reserved:
if (DPLL Locked) SPDIF_RxClk else extal
0001 if (DPLL Locked) SPDIF_RxClk else spdif_clk
0010 if (DPLL Locked) SPDIF_RxClk else asrc_clk
0011 if (DPLL Locked) SPDIF_RxClk else spdif_extclk
0100 if (DPLL Locked) SPDIF_Rxclk else esai_hckt
0101 extal_clk
0110 spdif_clk
0111 asrc_clk
1000 spdif_extclk
1001 esai_hckt
1010 if (DPLL Locked) SPDIF_RxClk else mlb_clk
==
So the name list here basically is not being used to obtain a
clock like what standardized DT binding does but to provide the
driver a full list to look up which value should be exactly used
according to the obtained clock.
I think I should revise the binding doc for these two lists. It
might be hard to explain within that kinda short paragraph.
Surely, if I misunderstand your point, please correct me. And
if you have any sage idea, please guide me.
Something like this:
clocks = clks 197, clks 3, clks 197, clks 107, clks SPDIF_EXT,
clks 118, clks 62, clks 139, clks MLB_PHY
clock-names = core, rxtx0, rxtx1, rxtx2, rxtx3, rxtx4, rxtx5,
rxtx6, rxtx7
This describes the different input clocks to the spdif core and also
gives a hint to the array index (rxtx_n_) to use.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH v4 resent 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Wed, Aug 14, 2013 at 06:23:46PM +0800, Nicolin Chen wrote: Hi, Surely, if I misunderstand your point, please correct me. And if you have any sage idea, please guide me. Something like this: clocks = clks 197, clks 3, clks 197, clks 107, clks SPDIF_EXT, clks 118, clks 62, clks 139, clks MLB_PHY clock-names = core, rxtx0, rxtx1, rxtx2, rxtx3, rxtx4, rxtx5, rxtx6, rxtx7 This describes the different input clocks to the spdif core and also gives a hint to the array index (rxtx_n_) to use. Thank you for the idea, and..hmm..I'm a bit confused.. Is this really a nicer way? Yes, since the clk names are not an API. Exposing them to the devicetree is not an option. The fact that the names are defined in arch/arm/mach-imx/clk-imx6q.c and are used in the spdif driver makes this really clear. Actually the rx clock list and tx clock list are totally different. So doing this I have to list, in the maximum case, 24 (8 + 16) clock phandles for these two lists. And plussing another 6 I've listed in this binding doc -- thus there are totally 30 clock phanldes. But the 24 of 30 are only used to get two indexes. The spdif core has 8 input clocks which have to be described in the devicetree. Nobody says the mapping which clock name corresponds to which bit combination has to be in the devicetree. Look at the possible clocks: if (DPLL Locked) SPDIF_RxClk else extal 0001 if (DPLL Locked) SPDIF_RxClk else spdif_clk 0010 if (DPLL Locked) SPDIF_RxClk else asrc_clk 0011 if (DPLL Locked) SPDIF_RxClk else spdif_extclk 0100 if (DPLL Locked) SPDIF_Rxclk else esai_hckt 0101 extal_clk 0110 spdif_clk 0111 asrc_clk 1000 spdif_extclk 1001 esai_hckt 1010 if (DPLL Locked) SPDIF_RxClk else mlb_clk 1011 if (DPLL Locked) SPDIF_RxClk else mlb_phy_clk 1100 mkb_clk 1101 mlb_phy_clk Only half of them actually are clocks. if (DPLL Locked) SPDIF_RxClk else ... is not a clock. Every sane hardware developer would have introduced a mux with 8 entries and an additional Use DPLL if possible bit. Now this is not the case here so we have to live with it and maintain the above table in the driver. And another one for the i.MX35 and still another one for i.MX53. I think I need a little help here to understand why this is better. As said, the clock names are not an API. Nobody changing clk-imx6q.c expects to break devicetree bindings. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v1 05/24] clk: wrap I/O access for improved portability
On Thu, Jul 18, 2013 at 09:04:02AM +0200, Gerhard Sittig wrote: On Mon, Jul 15, 2013 at 21:38 +0200, Sascha Hauer wrote: On Mon, Jul 15, 2013 at 08:47:34PM +0200, Gerhard Sittig wrote: diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c index 6d55eb2..2c07061 100644 --- a/drivers/clk/clk-divider.c +++ b/drivers/clk/clk-divider.c @@ -104,7 +104,7 @@ static unsigned long clk_divider_recalc_rate(struct clk_hw *hw, struct clk_divider *divider = to_clk_divider(hw); unsigned int div, val; - val = readl(divider-reg) divider-shift; + val = clk_readl(divider-reg) divider-shift; val = div_mask(divider); Would it be an option to use regmap for the generic dividers/muxes instead? This should be suitable for ppc and also for people who want to use the generic clocks on i2c devices. Some other thought crossed my mind regarding access to clock control registers that reside behind some communication channel like I2C: The common clock API assumes (it's part of the contract) that there are potentially expensive operations like get, put, prepare and unprepare, as well as swift and non-blocking operations like enable and disable. Would the regmap abstraction hide the potentially blocking nature of a register access (I understand that you can implement local as well as remote register sets by this mechanism)? Or could you still meet the assumptions or requirements of the common clock API? It might as well be the responsibility of the clock driver's implementor to arrange for the availability of non-blocking enable/disable operations, just as it is today. Such that expensive register access need not be forbidden in general. regmap for mmio uses a spinlock for read/modify/write operations, just like you have to use a spinlock in the common clk dividers/gates. This part wouldn't change with regmap. For i2c connected clocks where a spinlock doesn't work due to the nonatomic nature of i2c devices we would have to move the enable/disble stuff to prepare/unprepare in the common gate code. This can be left for someone who works on i2c clocks though. I think regmap has the potential to solve a number of issues like the hardcoded readl/writel in the common clock blocks, issues with i2c clocks and your endianess issue. The biggest question probably is how to get there without putting too much of a burden on you. It's probably not an option to convert all users to regmap, so it seems additional functions like clk_register_gate_regmap are better to handle. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v1 05/24] clk: wrap I/O access for improved portability
On Mon, Jul 15, 2013 at 08:47:34PM +0200, Gerhard Sittig wrote: diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c index 6d55eb2..2c07061 100644 --- a/drivers/clk/clk-divider.c +++ b/drivers/clk/clk-divider.c @@ -104,7 +104,7 @@ static unsigned long clk_divider_recalc_rate(struct clk_hw *hw, struct clk_divider *divider = to_clk_divider(hw); unsigned int div, val; - val = readl(divider-reg) divider-shift; + val = clk_readl(divider-reg) divider-shift; val = div_mask(divider); Would it be an option to use regmap for the generic dividers/muxes instead? This should be suitable for ppc and also for people who want to use the generic clocks on i2c devices. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v1 15/24] serial: mpc512x: OF clock lookup, use the 'mclk' name
On Mon, Jul 15, 2013 at 11:46:01PM +0200, Gerhard Sittig wrote:
with device tree based clock lookup, the MCLK name no longer
depends on the PSC index
Signed-off-by: Gerhard Sittig g...@denx.de
---
drivers/tty/serial/mpc52xx_uart.c |8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/mpc52xx_uart.c
b/drivers/tty/serial/mpc52xx_uart.c
index 53c1093..221fb89 100644
--- a/drivers/tty/serial/mpc52xx_uart.c
+++ b/drivers/tty/serial/mpc52xx_uart.c
@@ -619,21 +619,17 @@ static irqreturn_t mpc512x_psc_handle_irq(struct
uart_port *port)
static int mpc512x_psc_clock(struct uart_port *port, int enable)
{
struct clk *psc_clk;
- int psc_num;
- char clk_name[10];
if (uart_console(port))
return 0;
- psc_num = (port-mapbase 0xf00) 8;
- snprintf(clk_name, sizeof(clk_name), psc%d_mclk, psc_num);
- psc_clk = clk_get(port-dev, clk_name);
+ psc_clk = clk_get(port-dev, mclk);
Same comment applies here as Mark made to the spi driver.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 0/7] update USB gadget driver fsl-usb2-udc
Hi Christoph, What system are you working on? If it's i.MX you should use/work on the chipidea driver instead. Sascha On Fri, Oct 19, 2012 at 12:22:36PM +0200, Christoph Fritz wrote: This series updates USB gadget driver fsl-usb2-udc. Christoph Fritz (7): usb: gadget: fsl_udc: simplify driver init usb: gadget: fsl_udc: protect fsl_pullup() with spin_lock usb: gadget: fsl_udc: convert to new ulc style usb: gadget: fsl_udc: drop ARCH dependency usb: gadget: fsl_udc: postpone freeing current dTD usb: gadget: fsl_udc: purge global pointer usb_sys_regs usb: gadget: fsl_udc: purge global pointer dr_regs drivers/usb/gadget/fsl_udc_core.c | 755 - drivers/usb/gadget/fsl_usb2_udc.h |4 + 2 files changed, 322 insertions(+), 437 deletions(-) -- 1.7.2.5 -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 01/19] mxc_udc: add workaround for ENGcm09152 for i.MX25
Hi Eric, To make the handling of your patches easier, please - send a cover letter for multiple patches - separate them at least by fixes and features - if possible, do not send patches aimed at multiple maintainers in a single series. I picked every patch looking like a fix for now myself, but please remember next time. I'll have a look at the non-fix patches later. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [net-next-2.6 PATCH v2] can: SJA1000: generic OF platform bus driver
On Tue, May 26, 2009 at 10:42:05AM +0100, Arnd Bergmann wrote: On Tuesday 26 May 2009, David Miller wrote: It's such a baroque thing, there is no reason to set it at all if you ask me. It's only use is to allow ISA and similar primitive bus devices to have their I/O ports changed via ifconfig. My original comment was about the fact that sja1000 was doing dev-base_addr = (unsigned long)ioremap(phys_addr, size), I didn't even think about SIOCGIFMAP and command line overrides, but that surely makes it worse and the driver should be changed to store the virtual register address in its private data structure. drivers/net/fec.c seems to have the same problem, which manifests in a number of ugly casts and direct pointer dereferences in places where it should do writel() or out_be32(). Ack. I'll prepare a patch for fec.c. Internally the driver already uses a void __iomem * and writel/readl in -next. There is only one usage left. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: Comments on device tree for pcm030
Hi,
On Sun, Jun 08, 2008 at 03:08:33PM -0400, Jon Smirl wrote:
Why not a compatible field in the top of the tree? Then you wouldn't
need to list the boards in mpc5200_simple.c.
compatible = phytec,pcm030,simple-mpc5200;
Device tree has an entry for AC97 on PSC1. I don't think the Phytec
module or carrier board has AC97 hardware.
The RTC chip says pcf8563, phytec doc says it is a pcf8564.
There should be an i2c entry for the eeprom but I don't know the part
number for it.
What about the flash on the local bus? Could we use something like
this, or the same without the partition data?
[EMAIL PROTECTED] {
compatible = fsl,lpb;
ranges = 0 ff00 0100;
[EMAIL PROTECTED] {
compatible = cfi-flash;
reg = 0100;
bank-width = 2;
The board comes with different flash sizes with different bank widths.
That's why I decided to no put the flash entries into the tree.
We used to detect the flash size/bankwidth using the bootcs config
register (which was previously set by U-Boot). We could change U-Boot to
adjust the device tree accordingly, but we haven't done this so far,
sorry.
#size-cells = 1;
#address-cells = 1;
[EMAIL PROTECTED] {
label = ubootl;
reg = 0004;
};
[EMAIL PROTECTED] {
label = kernel;
reg = 0004 001c;
};
[EMAIL PROTECTED] {
label = jffs2;
reg = 0020 00D0;
};
[EMAIL PROTECTED] {
label = uboot;
reg = 00f0 0004;
};
[EMAIL PROTECTED] {
label = oftree;
reg = 00f4 0004;
};
[EMAIL PROTECTED] {
label = space;
reg = 00f8 0008;
};
I think partitions shouldn't go into the default device tree, as people
may have different partitioning.
Sascha
--
Pengutronix e.K. - Linux Solutions for Science and Industry
---
Kontakt-Informationen finden Sie im Header dieser Mail oder
auf der Webseite - http://www.pengutronix.de/impressum/ -
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: jffs2 and unaligned access
On Wed, May 07, 2008 at 11:53:49AM +0100, David Woodhouse wrote: On Wed, 2008-05-07 at 12:27 +0200, Sascha Hauer wrote: memcpy_from/to_io() use word aligned accesses on the io side of memory. The MPC5200 local plus bus where our flashes are connected does not allow unaligned accesses, so we have to use the io versions of memcpy. But this region of flash is marked as suitable for execute-in-place, otherwise the point() function wouldn't be working to give a direct pointer to it. It sounds like we shouldn't be allowing that. It actually is suitable for execute-in-place. It's the flash U-Boot starts from. The compiler will generate a proper alignment for you. Which in turn means that perhaps we should have a property in the corresponding node in the device-tree which indicates that it's not suitable for direct access? So far we did not work with the device-tree flash binding but with the physmap-flash driver, but ok, this is subject to change anyway. I gave it a quick try to disable direct accesses. It works, but has a 1:10 performance impact on mounting a jffs2. At least it's a clean solution. Sascha -- Pengutronix e.K. - Linux Solutions for Science and Industry --- Kontakt-Informationen finden Sie im Header dieser Mail oder auf der Webseite - http://www.pengutronix.de/impressum/ - ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] add Phytec pcm030 board support
On Mon, May 05, 2008 at 01:22:40PM -0400, Jon Smirl wrote:
On 5/5/08, Grant Likely [EMAIL PROTECTED] wrote:
On Mon, May 5, 2008 at 11:01 AM, Jon Smirl [EMAIL PROTECTED] wrote:
Did this get fixed somehow? I used to need this to boot a pcm030.
I'm sorry; I'm at a lost as to context. Are you asking for this patch
to be applied? Or are you asking if this has been addressed in
another way?
Sascha said the pcm030 was working with the simple dts. I always
needed that patch to get a pcm030 to boot. Sasha's company wrote the
patch. I'm just wondering how it got handled, do we still need the
patch or did he come up with some other solution.
Yes, it is working with the simple dts except for the flash support. For
this we need the Flash description in the oftree.
diff --git a/fs/jffs2/scan.c b/fs/jffs2/scan.c
index 272872d..c982adc 100644
--- a/fs/jffs2/scan.c
+++ b/fs/jffs2/scan.c
@@ -16,6 +16,7 @@
#include linux/pagemap.h
#include linux/crc32.h
#include linux/compiler.h
+#include asm/io.h
#include nodelist.h
#include summary.h
#include debug.h
@@ -505,7 +506,7 @@ static int jffs2_scan_eraseblock (struct
jffs2_sb_info *c, struct jffs2_eraseblo
sumptr = kmalloc(sumlen,
GFP_KERNEL);
if (!sumptr)
return -ENOMEM;
- memcpy(sumptr + sumlen -
buf_len, buf + buf_size - buf_len, buf_len);
+ memcpy_fromio(sumptr + sumlen
- buf_len, buf + buf_size - buf_len, buf_len);
}
if (buf_len sumlen) {
/* Need to read more so that
the entire summary node is present */
@@ -1035,7 +1036,7 @@ static int jffs2_scan_dirent_node(struct
jffs2_sb_info *c, struct jffs2_eraseblo
if (!fd) {
return -ENOMEM;
}
- memcpy(fd-name, rd-name, checkedlen);
+ memcpy_fromio(fd-name, rd-name, checkedlen);
This patch is needed because memcpy uses unaligned accesses whereas
memcpy_fromio only uses aligned accesses on the io side. See this
thread: http://ozlabs.org/pipermail/linuxppc-embedded/2006-April/022544.html
--
Pengutronix e.K. - Linux Solutions for Science and Industry
---
Kontakt-Informationen finden Sie im Header dieser Mail oder
auf der Webseite - http://www.pengutronix.de/impressum/ -
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] add gpiolib support for mpc5200
On Thu, Apr 24, 2008 at 12:45:49PM -0600, Grant Likely wrote:
On Thu, Apr 24, 2008 at 9:36 AM, Sascha Hauer [EMAIL PROTECTED] wrote:
Hi all,
Feel free to comment on this.
Sascha
This patch adds gpiolib support for mpc5200 SOCs. I'm not sure
whether it's a good idea to make this optional via kconfig.
The gpt devices only support a single gpio. In the current of_gpio
implementation each chip consumes 32 GPIOs which leads to huge
gaps.
Signed-off-by: Sascha Hauer [EMAIL PROTECTED]
Looks pretty good. You've saved me the need to go write a driver
myself. Comments below, but I'll pull it into my tree and give it a
spin.
I don't see any mechanism for setting the open drain state of the pin.
That will either need to be done by platform code or encoded into the
device tree. Does the OF gpio infrastructure provide any callback to
the driver when something requests the pin? That would seem to be the
ideal place to set the open drain state.
No, unfortunately not. The generic gpio stuff does not provide this
callback, so of gpio has no chance to do so.
This would also be a good place to catch the registration of reserved
pins, so maybe it's worth discussing this with the gpiolib maintainers.
You'll also need to document the format of the gpio pin specifier for
these devices (ie. first cell is GPIO number, second cell is ).
I've taken the two-cell approach from booting-without-of.txt. There the
second cell is for flags. We could encode the open drain state into
this.
As for the wide spans caused by gpt gpios, it is probably okay for
now, but we can rework it to do something clever (like have a single
registration for all gpt gpios) at a later date.
I would rather teach the of gpio infrastructure not to reserve 32 gpios
for each chip. This will bite us once we want to support gpio chips with
more than 32 gpios anyway.
+ *
+ */
+static int mpc52xx_wkup_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct mpc52xx_gpio_wkup __iomem *regs = mm_gc-regs;
+ unsigned int ret;
+
+ ret = (in_8(regs-wkup_ival) (7 - gpio)) 1;
+
+ pr_debug(%s: gpio: %d ret: %d\n, __func__, gpio, ret);
dev_dbg maybe?
We would have to carry the device from the probe function to this
function just for the debugging output. I'm not sure if it's worth it.
+
+ return ret;
+}
+
+static void mpc52xx_wkup_gpio_set(struct gpio_chip *gc, unsigned int
gpio, int val)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct mpc52xx_gpio_wkup __iomem *regs = mm_gc-regs;
+ unsigned int tmp;
+ unsigned long flags;
+
+ spin_lock_irqsave(gpio_lock, flags);
+
+ tmp = in_8(regs-wkup_dvo);
+ if (val)
+ tmp |= 1 (7 - gpio);
+ else
+ tmp = ~(1 (7 - gpio));
+ out_8(regs-wkup_dvo, tmp);
Rather than read/modify/write of the device register; the function
would probably be faster (one fewer barrier) if you used a shadow
register of the pin state and the critical region would be
shorter/simpler. Also, while this device doesn't have the side
effects associated with shared input/output register, it might still
be good form to use a shadow register just for the sake of clarity.
OK
+
+ spin_unlock_irqrestore(gpio_lock, flags);
+
+ pr_debug(%s: gpio: %d val: %d\n, __func__, gpio, val);
+}
+
+static int mpc52xx_wkup_gpio_dir_in(struct gpio_chip *gc, unsigned int
gpio)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct mpc52xx_gpio_wkup *regs = mm_gc-regs;
+ unsigned int tmp;
+ unsigned long flags;
+
+ spin_lock_irqsave(gpio_lock, flags);
+
+ tmp = in_8(regs-wkup_ddr);
+ tmp = ~(1 (7 - gpio));
+ out_8(regs-wkup_ddr, tmp);
+
+ spin_unlock_irqrestore(gpio_lock, flags);
+
+ return 0;
+}
+
+static int mpc52xx_wkup_gpio_dir_out(struct gpio_chip *gc, unsigned int
gpio, int val)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct mpc52xx_gpio_wkup *regs = mm_gc-regs;
+ unsigned int tmp;
+ unsigned long flags;
+
+ /* First set initial value */
+ mpc52xx_wkup_gpio_set(gc, gpio, val);
+
+ spin_lock_irqsave(gpio_lock, flags);
+
+ /* Then set direction */
+ tmp = in_8(regs-wkup_ddr);
+ tmp |= 1 (7 - gpio);
+ out_8(regs-wkup_ddr, tmp);
+
+ /* Finally enable the pin */
+ tmp = in_8(regs-wkup_gpioe);
+ tmp |= 1 (7 - gpio);
+ out_8(regs-wkup_gpioe, tmp);
Do you want/need the cost of enabling the pin every time dir_out is
called? Can it be done when the pin is requested instead
[PATCH] add gpiolib support for mpc5200
This patch adds gpiolib support for mpc5200 SOCs.
Changes since last submit:
- fixed checkpatch warnings
- use shadow variables for register accesses
- make match tables const
- Add documentation
Signed-off-by: Sascha Hauer [EMAIL PROTECTED]
---
Documentation/powerpc/mpc52xx-device-tree-bindings.txt | 12
arch/powerpc/platforms/52xx/Kconfig|6
arch/powerpc/platforms/52xx/Makefile |2
arch/powerpc/platforms/52xx/mpc52xx_gpio.c | 465 +
4 files changed, 485 insertions(+)
Index: linux-2.6-powerpc/arch/powerpc/platforms/52xx/mpc52xx_gpio.c
===
--- /dev/null
+++ linux-2.6-powerpc/arch/powerpc/platforms/52xx/mpc52xx_gpio.c
@@ -0,0 +1,465 @@
+/*
+ * MPC52xx gpio driver
+ *
+ * Copyright (c) 2008 Sascha Hauer [EMAIL PROTECTED], Pengutronix
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include linux/of.h
+#include linux/kernel.h
+#include linux/of_gpio.h
+#include linux/io.h
+#include linux/of_platform.h
+
+#include asm/gpio.h
+#include asm/mpc52xx.h
+#include sysdev/fsl_soc.h
+
+static DEFINE_SPINLOCK(gpio_lock);
+
+struct mpc52xx_gpiochip {
+ struct of_mm_gpio_chip mmchip;
+ unsigned int shadow_dvo;
+ unsigned int shadow_gpioe;
+ unsigned int shadow_ddr;
+};
+
+/*
+ * GPIO LIB API implementation for wakeup GPIOs.
+ *
+ * There's a maximum of 8 wakeup GPIOs. Which of these are available
+ * for use depends on your board setup.
+ *
+ * 0 - GPIO_WKUP_7
+ * 1 - GPIO_WKUP_6
+ * 2 - PSC6_1
+ * 3 - PSC6_0
+ * 4 - ETH_17
+ * 5 - PSC3_9
+ * 6 - PSC2_4
+ * 7 - PSC1_4
+ *
+ */
+static int mpc52xx_wkup_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct mpc52xx_gpio_wkup __iomem *regs = mm_gc-regs;
+ unsigned int ret;
+
+ ret = (in_8(regs-wkup_ival) (7 - gpio)) 1;
+
+ pr_debug(%s: gpio: %d ret: %d\n, __func__, gpio, ret);
+
+ return ret;
+}
+
+static inline void
+__mpc52xx_wkup_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct mpc52xx_gpiochip *chip = container_of(mm_gc,
+ struct mpc52xx_gpiochip, mmchip);
+ struct mpc52xx_gpio_wkup __iomem *regs = mm_gc-regs;
+
+ if (val)
+ chip-shadow_dvo |= 1 (7 - gpio);
+ else
+ chip-shadow_dvo = ~(1 (7 - gpio));
+
+ out_8(regs-wkup_dvo, chip-shadow_dvo);
+}
+
+static void
+mpc52xx_wkup_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(gpio_lock, flags);
+
+ __mpc52xx_wkup_gpio_set(gc, gpio, val);
+
+ spin_unlock_irqrestore(gpio_lock, flags);
+
+ pr_debug(%s: gpio: %d val: %d\n, __func__, gpio, val);
+}
+
+static int mpc52xx_wkup_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct mpc52xx_gpiochip *chip = container_of(mm_gc,
+ struct mpc52xx_gpiochip, mmchip);
+ struct mpc52xx_gpio_wkup *regs = mm_gc-regs;
+ unsigned long flags;
+
+ spin_lock_irqsave(gpio_lock, flags);
+
+ /* set the direction */
+ chip-shadow_ddr = ~(1 (7 - gpio));
+ out_8(regs-wkup_ddr, chip-shadow_ddr);
+
+ /* and enable the pin */
+ chip-shadow_gpioe |= 1 (7 - gpio);
+ out_8(regs-wkup_gpioe, chip-shadow_gpioe);
+
+ spin_unlock_irqrestore(gpio_lock, flags);
+
+ return 0;
+}
+
+static int
+mpc52xx_wkup_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct mpc52xx_gpio_wkup *regs = mm_gc-regs;
+ struct mpc52xx_gpiochip *chip = container_of(mm_gc,
+ struct mpc52xx_gpiochip, mmchip);
+ unsigned long flags;
+
+ spin_lock_irqsave(gpio_lock, flags);
+
+ __mpc52xx_wkup_gpio_set(gc, gpio, val);
+
+ /* Then set direction */
+ chip-shadow_ddr |= 1 (7 - gpio);
+ out_8(regs-wkup_ddr, chip-shadow_ddr);
+
+ /* Finally enable the pin */
+ chip-shadow_gpioe |= 1 (7 - gpio);
+ out_8(regs-wkup_gpioe, chip-shadow_gpioe);
+
+ spin_unlock_irqrestore
[PATCH] add Phytec pcm030 board support
Add board support for the Phytec pcm030 mpc5200b based board. It
does not need any platform specific fixups and as such is handled
as a mpc5200 simple platform.
Signed-off-by: Sascha Hauer [EMAIL PROTECTED]
---
arch/powerpc/boot/dts/pcm030.dts | 363
arch/powerpc/configs/52xx/pcm030_defconfig | 1109 +++
arch/powerpc/platforms/52xx/mpc5200_simple.c |1
3 files changed, 1473 insertions(+)
Index: linux-2.6-powerpc/arch/powerpc/boot/dts/pcm030.dts
===
--- /dev/null
+++ linux-2.6-powerpc/arch/powerpc/boot/dts/pcm030.dts
@@ -0,0 +1,363 @@
+/*
+ * phyCORE-MPC5200B-tiny (pcm030) board Device Tree Source
+ *
+ * Copyright 2006 Pengutronix
+ * Sascha Hauer [EMAIL PROTECTED]
+ * Copyright 2007 Pengutronix
+ * Juergen Beisert [EMAIL PROTECTED]
+ *
+ * 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.
+ */
+
+/dts-v1/;
+
+/ {
+ model = phytec,pcm030;
+ compatible = phytec,pcm030;
+ #address-cells = 1;
+ #size-cells = 1;
+
+ cpus {
+ #address-cells = 1;
+ #size-cells = 0;
+
+ PowerPC,[EMAIL PROTECTED] {
+ device_type = cpu;
+ reg = 0;
+ d-cache-line-size = 32;
+ i-cache-line-size = 32;
+ d-cache-size = 0x4000;/* L1, 16K */
+ i-cache-size = 0x4000;/* L1, 16K */
+ timebase-frequency = 0; /* From Bootloader */
+ bus-frequency = 0;/* From Bootloader */
+ clock-frequency = 0; /* From Bootloader */
+ };
+ };
+
+ memory {
+ device_type = memory;
+ reg = 0x 0x0400; /* 64MB */
+ };
+
+ [EMAIL PROTECTED] {
+ #address-cells = 1;
+ #size-cells = 1;
+ compatible = fsl,mpc5200b-immr;
+ ranges = 0x0 0xf000 0xc000;
+ bus-frequency = 0;/* From bootloader */
+ system-frequency = 0; /* From bootloader */
+
+ [EMAIL PROTECTED] {
+ compatible = fsl,mpc5200b-cdm,fsl,mpc5200-cdm;
+ reg = 0x200 0x38;
+ };
+
+ mpc5200_pic: [EMAIL PROTECTED] {
+ /* 5200 interrupts are encoded into two levels; */
+ interrupt-controller;
+ #interrupt-cells = 3;
+ device_type = interrupt-controller;
+ compatible = fsl,mpc5200b-pic,fsl,mpc5200-pic;
+ reg = 0x500 0x80;
+ };
+
+ [EMAIL PROTECTED] { /* General Purpose Timer */
+ compatible = fsl,mpc5200b-gpt,fsl,mpc5200-gpt;
+ cell-index = 0;
+ reg = 0x600 0x10;
+ interrupts = 0x1 0x9 0x0;
+ interrupt-parent = mpc5200_pic;
+ fsl,has-wdt;
+ };
+
+ [EMAIL PROTECTED] { /* General Purpose Timer */
+ compatible = fsl,mpc5200b-gpt,fsl,mpc5200-gpt;
+ cell-index = 1;
+ reg = 0x610 0x10;
+ interrupts = 0x1 0xa 0x0;
+ interrupt-parent = mpc5200_pic;
+ };
+
+ gpt2: [EMAIL PROTECTED] { /* General Purpose Timer in GPIO mode
*/
+ compatible =
fsl,mpc5200b-gpt-gpio,fsl,mpc5200-gpt-gpio;
+ cell-index = 2;
+ reg = 0x620 0x10;
+ interrupts = 0x1 0xb 0x0;
+ interrupt-parent = mpc5200_pic;
+ gpio-controller;
+ #gpio-cells = 2;
+ };
+
+ gpt3: [EMAIL PROTECTED] { /* General Purpose Timer in GPIO mode
*/
+ compatible =
fsl,mpc5200b-gpt-gpio,fsl,mpc5200-gpt-gpio;
+ cell-index = 3;
+ reg = 0x630 0x10;
+ interrupts = 0x1 0xc 0x0;
+ interrupt-parent = mpc5200_pic;
+ gpio-controller;
+ #gpio-cells = 2;
+ };
+
+ gpt4: [EMAIL PROTECTED] { /* General Purpose Timer in GPIO mode
*/
+ compatible =
fsl,mpc5200b-gpt-gpio,fsl,mpc5200-gpt-gpio;
+ cell-index = 4;
+ reg = 0x640 0x10;
+ interrupts = 0x1 0xd 0x0
Re: [PATCH] add gpiolib support for mpc5200
On Fri, Apr 25, 2008 at 04:30:55PM +0100, Matt Sealey wrote: Can I make a suggestion for this chip support? On certain 5200 boards these devices are not usable since they are not connected. My concern is the Efika where we only have 2 wakeup and 1 simple GPIO available on the board and maybe a few others with a bit of tweaking and messing around. This is probably true for most boards. I haven't tested if this is true for all gpios, but the peripheral usage settings seem to take precedence over the gpio settings. So at least you can play with already for peripheral claimed gpios without bad things happening. But ok, seems to be a bad idea to rely on this. Instead of having a block for GPIO which has 32 pins, and the ability to use all of them just by asking for it (get will return a pin regardless) I implemented a silly hackish gpio-mask property in the efika.forth device tree so the gpiolib allocate style functions can tell if a pin is actually usable or not. I thought about the same thing, but we have to improve gpiolib first. Currently there is no hook for gpio_request(). Can we put some small thought into defining a standard for this, and possibly moving the number of gpios in the chip into the device tree? How does the number of gpios help? the gpio-mask you mentioned above seems to do better. It doesn't seem particularly complete to just allow blanket use (which may be unsafe and may change per-board) of pins just because you can twiddle a bit.. (or use a pin when another driver may be using it). This will tie into Grant's comments about optimizing the enabling of the GPIO on every direction change etc. (it should be enabled on allocation and then you have to make sure you free up your use of the pin, resource tracking in the kernel is important.. flipping bits here and there without something in the way in the API is kind of clumsy) -- Matt Sealey [EMAIL PROTECTED] Genesi, Manager, Developer Relations Grant Likely wrote: On Thu, Apr 24, 2008 at 9:36 AM, Sascha Hauer [EMAIL PROTECTED] wrote: Hi all, Feel free to comment on this. Sascha This patch adds gpiolib support for mpc5200 SOCs. I'm not sure whether it's a good idea to make this optional via kconfig. The gpt devices only support a single gpio. In the current of_gpio implementation each chip consumes 32 GPIOs which leads to huge gaps. Signed-off-by: Sascha Hauer [EMAIL PROTECTED] Looks pretty good. You've saved me the need to go write a driver myself. Comments below, but I'll pull it into my tree and give it a spin. I don't see any mechanism for setting the open drain state of the pin. That will either need to be done by platform code or encoded into the device tree. Does the OF gpio infrastructure provide any callback to the driver when something requests the pin? That would seem to be the ideal place to set the open drain state. You'll also need to document the format of the gpio pin specifier for these devices (ie. first cell is GPIO number, second cell is ). As for the wide spans caused by gpt gpios, it is probably okay for now, but we can rework it to do something clever (like have a single registration for all gpt gpios) at a later date. Cheers, g. --- arch/powerpc/platforms/52xx/Kconfig|6 arch/powerpc/platforms/52xx/Makefile |2 arch/powerpc/platforms/52xx/mpc52xx_gpio.c | 408 + 3 files changed, 416 insertions(+) Index: arch/powerpc/platforms/52xx/mpc52xx_gpio.c === --- /dev/null +++ b/arch/powerpc/platforms/52xx/mpc52xx_gpio.c @@ -0,0 +1,408 @@ +/* + * MPC52xx gpio driver + * + * Copyright (c) 2008 Sascha Hauer [EMAIL PROTECTED], Pengutronix + * + * 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. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ + +#include linux/of.h +#include linux/kernel.h +#include linux/of_gpio.h +#include linux/io.h +#include asm/of_platform.h +#include asm/prom.h +#include asm/gpio.h +#include asm/mpc52xx.h +#include sysdev/fsl_soc.h + +static DEFINE_SPINLOCK(gpio_lock); + +/* + * GPIO LIB API implementation for wakeup GPIOs. + * + * There's a maximum of 8 wakeup GPIOs. Which of these are available + * for use depends on your board setup. + * + * 0
mpc5200b custom board upstreamable?
Hi all, I had the intention to push the code for a custom mpc5200b board (freely available, no internal project) upstream. After cleaning up the code I realized that actually no board specific code is left and our board is well handled by the mpc5200_simple_platform machine. The only issue is that the machine only matches things like schindler,cm5200, there's no generic entry. Would it be possible to add a generic-mpc52xx entry to this list? Another question: are defconfig and dts files for custom boards acceptable for upstream? Sascha -- Pengutronix e.K. - Linux Solutions for Science and Industry --- Kontakt-Informationen finden Sie im Header dieser Mail oder auf der Webseite - http://www.pengutronix.de/impressum/ - ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
[PATCH] add gpiolib support for mpc5200
Hi all,
Feel free to comment on this.
Sascha
This patch adds gpiolib support for mpc5200 SOCs. I'm not sure
whether it's a good idea to make this optional via kconfig.
The gpt devices only support a single gpio. In the current of_gpio
implementation each chip consumes 32 GPIOs which leads to huge
gaps.
Signed-off-by: Sascha Hauer [EMAIL PROTECTED]
---
arch/powerpc/platforms/52xx/Kconfig|6
arch/powerpc/platforms/52xx/Makefile |2
arch/powerpc/platforms/52xx/mpc52xx_gpio.c | 408 +
3 files changed, 416 insertions(+)
Index: arch/powerpc/platforms/52xx/mpc52xx_gpio.c
===
--- /dev/null
+++ b/arch/powerpc/platforms/52xx/mpc52xx_gpio.c
@@ -0,0 +1,408 @@
+/*
+ * MPC52xx gpio driver
+ *
+ * Copyright (c) 2008 Sascha Hauer [EMAIL PROTECTED], Pengutronix
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include linux/of.h
+#include linux/kernel.h
+#include linux/of_gpio.h
+#include linux/io.h
+#include asm/of_platform.h
+#include asm/prom.h
+#include asm/gpio.h
+#include asm/mpc52xx.h
+#include sysdev/fsl_soc.h
+
+static DEFINE_SPINLOCK(gpio_lock);
+
+/*
+ * GPIO LIB API implementation for wakeup GPIOs.
+ *
+ * There's a maximum of 8 wakeup GPIOs. Which of these are available
+ * for use depends on your board setup.
+ *
+ * 0 - GPIO_WKUP_7
+ * 1 - GPIO_WKUP_6
+ * 2 - PSC6_1
+ * 3 - PSC6_0
+ * 4 - ETH_17
+ * 5 - PSC3_9
+ * 6 - PSC2_4
+ * 7 - PSC1_4
+ *
+ */
+static int mpc52xx_wkup_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct mpc52xx_gpio_wkup __iomem *regs = mm_gc-regs;
+ unsigned int ret;
+
+ ret = (in_8(regs-wkup_ival) (7 - gpio)) 1;
+
+ pr_debug(%s: gpio: %d ret: %d\n, __func__, gpio, ret);
+
+ return ret;
+}
+
+static void mpc52xx_wkup_gpio_set(struct gpio_chip *gc, unsigned int gpio, int
val)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct mpc52xx_gpio_wkup __iomem *regs = mm_gc-regs;
+ unsigned int tmp;
+ unsigned long flags;
+
+ spin_lock_irqsave(gpio_lock, flags);
+
+ tmp = in_8(regs-wkup_dvo);
+ if (val)
+ tmp |= 1 (7 - gpio);
+ else
+ tmp = ~(1 (7 - gpio));
+ out_8(regs-wkup_dvo, tmp);
+
+ spin_unlock_irqrestore(gpio_lock, flags);
+
+ pr_debug(%s: gpio: %d val: %d\n, __func__, gpio, val);
+}
+
+static int mpc52xx_wkup_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct mpc52xx_gpio_wkup *regs = mm_gc-regs;
+ unsigned int tmp;
+ unsigned long flags;
+
+ spin_lock_irqsave(gpio_lock, flags);
+
+ tmp = in_8(regs-wkup_ddr);
+ tmp = ~(1 (7 - gpio));
+ out_8(regs-wkup_ddr, tmp);
+
+ spin_unlock_irqrestore(gpio_lock, flags);
+
+ return 0;
+}
+
+static int mpc52xx_wkup_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio,
int val)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct mpc52xx_gpio_wkup *regs = mm_gc-regs;
+ unsigned int tmp;
+ unsigned long flags;
+
+ /* First set initial value */
+ mpc52xx_wkup_gpio_set(gc, gpio, val);
+
+ spin_lock_irqsave(gpio_lock, flags);
+
+ /* Then set direction */
+ tmp = in_8(regs-wkup_ddr);
+ tmp |= 1 (7 - gpio);
+ out_8(regs-wkup_ddr, tmp);
+
+ /* Finally enable the pin */
+ tmp = in_8(regs-wkup_gpioe);
+ tmp |= 1 (7 - gpio);
+ out_8(regs-wkup_gpioe, tmp);
+
+ spin_unlock_irqrestore(gpio_lock, flags);
+
+ pr_debug(%s: gpio: %d val: %d\n, __func__, gpio, val);
+
+ return 0;
+}
+
+static int __devinit mpc52xx_wkup_gpiochip_probe(struct of_device *ofdev,
+const struct of_device_id *match)
+{
+ struct of_mm_gpio_chip *mmchip;
+ struct of_gpio_chip *chip;
+
+ mmchip = kzalloc(sizeof(*mmchip), GFP_KERNEL);
+ if (!mmchip)
+ return -ENOMEM;
+
+ chip = mmchip-of_gc;
+
+ chip-gpio_cells = 2;
+ chip-gc.ngpio= 8;
+ chip-gc.direction_input = mpc52xx_wkup_gpio_dir_in;
+ chip-gc.direction_output = mpc52xx_wkup_gpio_dir_out;
+ chip-gc.get
Re: mpc5200b custom board upstreamable?
On Thu, Apr 24, 2008 at 09:13:45AM -0600, Grant Likely wrote: On Thu, Apr 24, 2008 at 9:12 AM, Sascha Hauer [EMAIL PROTECTED] wrote: Hi all, I had the intention to push the code for a custom mpc5200b board (freely available, no internal project) upstream. After cleaning up the code I realized that actually no board specific code is left and our board is well handled by the mpc5200_simple_platform machine. The only issue is that the machine only matches things like schindler,cm5200, there's no generic entry. Would it be possible to add a generic-mpc52xx entry to this list? I'm being cautious about this for the time being. I'd like to have a generic match mechanism, but I don't want to do something that isn't easy to recover from if it turns out to be brain dead. For now, just add your board name to the explicit match list. The board is called generic. No, just kidding ;) Another question: are defconfig and dts files for custom boards acceptable for upstream? Yes, in the arch/powerpc/configs/52xx/ directory. ok Sascha -- Pengutronix e.K. - Linux Solutions for Science and Industry --- Kontakt-Informationen finden Sie im Header dieser Mail oder auf der Webseite - http://www.pengutronix.de/impressum/ - ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: mpc5200b custom board upstreamable?
On Thu, Apr 24, 2008 at 10:07:20AM -0600, Grant Likely wrote: On Thu, Apr 24, 2008 at 9:53 AM, Sascha Hauer [EMAIL PROTECTED] wrote: On Thu, Apr 24, 2008 at 09:13:45AM -0600, Grant Likely wrote: On Thu, Apr 24, 2008 at 9:12 AM, Sascha Hauer [EMAIL PROTECTED] wrote: Hi all, I had the intention to push the code for a custom mpc5200b board (freely available, no internal project) upstream. After cleaning up the code I realized that actually no board specific code is left and our board is well handled by the mpc5200_simple_platform machine. The only issue is that the machine only matches things like schindler,cm5200, there's no generic entry. Would it be possible to add a generic-mpc52xx entry to this list? I'm being cautious about this for the time being. I'd like to have a generic match mechanism, but I don't want to do something that isn't easy to recover from if it turns out to be brain dead. For now, just add your board name to the explicit match list. The board is called generic. No, just kidding ;) /me slaps Sascha Seriously though; I do intend to fix this, but I don't think adding a generic entry to the compatible list is the right way to do it. For example, what would mpc5200-generic really mean anyway? Convention for usage of 'compatible' would indicate that it means the *entire board* is compatible (obviously not true). The use-case you're talking about is simply the board uses a 5200 and firmware is sane. On the other hand, I may just be overthinking things and compatible is the most appropriate place to specify that the board is a mpc5200 based board. (please feel free to argue with my; my opinion can probably be swayed... attaching promises of beer to your argument is probably an effective strategy) At the moment my compatible entry looks like this: compatible = phytec,pcm030,generic-mpc52xx; What I think would be nice is that phytec,pcm030 support is used when available and generic-mpc52xx as a fallback. We do not have any platform specific hacks at the moment, but we may have later. Having phytec,pcm030 in the simple machine would prevent us from doing so. This is an issue that probably affects the other embedded platforms too, so it would be nice to agree on a common method of handling it. Regardless, whatever method is chosen, it is also important that it is always possible for board specific fixups to override the generic behavior. agreed Sascha -- Pengutronix e.K. - Linux Solutions for Science and Industry --- Kontakt-Informationen finden Sie im Header dieser Mail oder auf der Webseite - http://www.pengutronix.de/impressum/ - ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: mpc5200: add interrupt type function
On Wed, Apr 16, 2008 at 11:41:08PM -0700, Grant Likely wrote: On Wed, Apr 16, 2008 at 11:00 PM, Robert Schwebel [EMAIL PROTECTED] wrote: On Tue, Apr 15, 2008 at 05:29:54PM +0200, Robert Schwebel wrote: From: Sascha Hauer [EMAIL PROTECTED] Add a set_type function for external (GPIO) interrupts. Signed-off-by: Juergen Beisert [EMAIL PROTECTED] Signed-off-by: Sascha Hauer [EMAIL PROTECTED] As we didn't get negative feedback yet, could this go into the 2.6.26 merge window? I didn't see this the first time (didn't get cc'd). It looks mostly okay, but in doesn't have any mutual exclusion which it probably should have. What are the users of this? No, it is used in kernel/irq/chip.c and kernel/irq/manage.c. Both callers already hold the mpc52xx_extirq_irqchip.lock spinlock. Sascha -- Pengutronix e.K. - Linux Solutions for Science and Industry --- Kontakt-Informationen finden Sie im Header dieser Mail oder auf der Webseite - http://www.pengutronix.de/impressum/ - ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] FCC: fix confused base / offset
On Thu, Apr 10, 2008 at 11:38:00AM -0500, Scott Wood wrote: Hi Scott, Thank you for your help so far Sascha Hauer wrote: See bottom of this mail. The board really is a 8260 based board. Our bootloader does not fill in the clock values, so they are hardcoded. I'm not very sure about the muram entries because the dpram is organized slightly different on the 8260. It has some dedicated FCC space and I don't know how to properly encode this in the device tree. I think the FCC space should just be left out. The old binding used the dpram offset 0xb080 for fcc2 while the new binding uses cpm_dpalloc which returns 0x80. When I use the old binding and change the hardcoded value from 0xb080 to 0x80 the fcc stopped working. I then hardcoded the value for the new binding to the same value as the old binding used, 0xb080, but no success. Does the PHY negotiate OK? Well I put some printks into the phy_read/write functions so I can say that it at least properly talks to the phy. Do you get a console message indicating that the link came up? No, but I didn't get a message for the old binding, too. I changed the device tree as you suggested, but still no success. BTW there is one thing I forgot to mention which could throw some light into this. This commit broke the FCC driver for me although it does the right thing: commit c6565331b7162a8348c70c37b4c33bedb6d4f02d Author: Scott Wood [EMAIL PROTECTED] Date: Mon Oct 1 14:20:50 2007 -0500 fs_enet: mac-fcc: Eliminate __fcc-* macros. These macros accomplish nothing other than defeating type checking. This patch also fixes one instance of the wrong register size being used that was revealed by enabling type checking. Signed-off-by: Scott Wood [EMAIL PROTECTED] Signed-off-by: Jeff Garzik [EMAIL PROTECTED] It fixes an access to the ftodr register in tx_kickstart(). After ftodr access the next console message is truncated and my bdi2000 shows me that the processor doesn't get out of cpm_uart_console_write(). Something strange is going on here... Sascha -- Pengutronix e.K. - Linux Solutions for Science and Industry --- Kontakt-Informationen finden Sie im Header dieser Mail oder auf der Webseite - http://www.pengutronix.de/impressum/ - ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] FCC: fix confused base / offset
On Wed, Apr 09, 2008 at 12:39:48PM -0500, Scott Wood wrote:
Sascha Hauer wrote:
Right, so it's probably not worth the effort. I stumbled on this while
porting my board to the new binding code. I was quite confused when
seeing this so I made this fix to understand what's going on here.
BTW have you tested the FCC driver with the new binding code?
Yes.
I do not
manage to get it working. Everything seems to be fine but
fs_enet_start_xmit does not send a packet and no interrupts are
arriving.
What does your device tree look like?
See bottom of this mail. The board really is a 8260 based board. Our
bootloader does not fill in the clock values, so they are hardcoded. I'm
not very sure about the muram entries because the dpram is organized
slightly different on the 8260. It has some dedicated FCC space and I
don't know how to properly encode this in the device tree.
Are all the pins and clocks set up properly?
should be. I temporarily removed all gpio setup code from the board
file, so it should be same as the bootloader leaves it.
Does the PHY negotiate OK?
Well I put some printks into the phy_read/write functions so I can say
that it at least properly talks to the phy.
Is any error reported in the descriptor?
No
arch/ppc is going away entirely in June or so.
I'm looking forward to it ;)
Sascha
/*
* MPC8272 ADS Device Tree Source
*
* Copyright 2005 Freescale Semiconductor Inc.
*
* 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.
*/
/ {
model = rsdproto;
compatible = fsl,rsdproto;
#address-cells = 1;
#size-cells = 1;
cpus {
#address-cells = 1;
#size-cells = 0;
PowerPC,[EMAIL PROTECTED] {
device_type = cpu;
reg = 0;
d-cache-line-size = d#32;
i-cache-line-size = d#32;
d-cache-size = d#16384;
i-cache-size = d#16384;
timebase-frequency = d#12500;
bus-frequency = d#5000;
clock-frequency = d#15000;
};
};
memory {
device_type = memory;
reg = 0 800;
};
[EMAIL PROTECTED] {
#address-cells = 1;
#size-cells = 1;
device_type = soc;
compatible = fsl,mpc8272, fsl,pq2-soc;
ranges = f000 00053000;
// Temporary -- will go away once kernel uses ranges for
get_immrbase().
reg = f000 00053000;
[EMAIL PROTECTED] {
#address-cells = 1;
#size-cells = 1;
compatible = fsl,mpc8272-cpm, fsl,cpm2;
reg = 119c0 30;
ranges;
[EMAIL PROTECTED] {
#address-cells = 1;
#size-cells = 1;
ranges = 0 0 1;
[EMAIL PROTECTED] {
compatible = fsl,cpm-muram-data;
reg = 0 2000 8000 800;
};
};
[EMAIL PROTECTED] {
compatible = fsl,mpc8272-brg,
fsl,cpm2-brg,
fsl,cpm-brg;
reg = 119f0 10 115f0 10;
clock-frequency = d#5000;
};
[EMAIL PROTECTED] {
device_type = serial;
compatible = fsl,mpc8272-scc-uart,
fsl,cpm2-scc-uart;
reg = 11a00 20 8000 100;
interrupts = 28 8;
interrupt-parent = PIC;
fsl,cpm-brg = 1;
fsl,cpm-command = 0080;
};
[EMAIL PROTECTED] {
device_type = serial;
compatible = fsl,mpc8272-scc-uart,
fsl,cpm2-scc-uart;
reg = 11a60 20 8300 100;
interrupts = 2b 8;
interrupt-parent = PIC;
fsl,cpm-brg = 4;
fsl,cpm-command = 0ce0
[PATCH] FCC: fix confused base / offset
Hi All,
This patch fixes a mixup in struct fs_platform_info. The struct has a field
dpram_offset which really is no offset but an io pointer to the dpram
(casted to u32).
It also has a field mem_offset which is used as the offset from the dpram
to the fcc RAM but then in turn filled into fcc.mem (casted to void __iomem *)
This patch fixes this in the way that dpram_offset holds the offset to the
fcc dpram as the name suggests and the mem_offset field is renamed to mem of
type void __iomem *.
This way we get rid of some #ifdefs and type casts.
Signed-off-by: Sascha Hauer [EMAIL PROTECTED]
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 2c5388c..578ced2 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -837,13 +837,13 @@ static int __init fs_enet_of_init(void)
int fcc_index = *id - 1;
const unsigned char *mdio_bb_prop;
- fs_enet_data.dpram_offset = (u32)cpm_dpram_addr(0);
+ fs_enet_data.dpram_offset = FCC_MEM_OFFSET(fcc_index);
fs_enet_data.rx_ring = 32;
fs_enet_data.tx_ring = 32;
fs_enet_data.rx_copybreak = 240;
fs_enet_data.use_napi = 0;
fs_enet_data.napi_weight = 17;
- fs_enet_data.mem_offset = FCC_MEM_OFFSET(fcc_index);
+ fs_enet_data.mem = cpm_dpram_addr(0);
fs_enet_data.cp_page = CPM_CR_FCC_PAGE(fcc_index);
fs_enet_data.cp_block = CPM_CR_FCC_SBLOCK(fcc_index);
diff --git a/drivers/net/fs_enet/mac-fcc.c b/drivers/net/fs_enet/mac-fcc.c
index e363211..1659ac7 100644
--- a/drivers/net/fs_enet/mac-fcc.c
+++ b/drivers/net/fs_enet/mac-fcc.c
@@ -157,7 +157,7 @@ out:
if (fep-fcc.fcccp == NULL)
return -EINVAL;
- fep-fcc.mem = (void __iomem *)fep-fpi-mem_offset;
+ fep-fcc.mem = fep-fpi-mem;
if (fep-fcc.mem == NULL)
return -EINVAL;
@@ -304,9 +304,6 @@ static void restart(struct net_device *dev)
fcc_enet_t __iomem *ep = fep-fcc.ep;
dma_addr_t rx_bd_base_phys, tx_bd_base_phys;
u16 paddrh, paddrm, paddrl;
-#ifndef CONFIG_PPC_CPM_NEW_BINDING
- u16 mem_addr;
-#endif
const unsigned char *mac;
int i;
@@ -338,19 +335,10 @@ static void restart(struct net_device *dev)
* this area.
*/
-#ifdef CONFIG_PPC_CPM_NEW_BINDING
W16(ep, fen_genfcc.fcc_riptr, fpi-dpram_offset);
W16(ep, fen_genfcc.fcc_tiptr, fpi-dpram_offset + 32);
W16(ep, fen_padptr, fpi-dpram_offset + 64);
-#else
- mem_addr = (u32) fep-fcc.mem; /* de-fixup dpram offset */
-
- W16(ep, fen_genfcc.fcc_riptr, (mem_addr 0x));
- W16(ep, fen_genfcc.fcc_tiptr, ((mem_addr + 32) 0x));
-
- W16(ep, fen_padptr, mem_addr + 64);
-#endif
/* fill with special symbol... */
memset_io(fep-fcc.mem + fpi-dpram_offset + 64, 0x88, 32);
diff --git a/include/linux/fs_enet_pd.h b/include/linux/fs_enet_pd.h
index 9bc045b..70080a0 100644
--- a/include/linux/fs_enet_pd.h
+++ b/include/linux/fs_enet_pd.h
@@ -128,7 +128,7 @@ struct fs_platform_info {
u32 clk_route;
u32 clk_mask;
- u32 mem_offset;
+ void __iomem *mem;
u32 dpram_offset;
u32 fcc_regs_c;
--
Pengutronix e.K. - Linux Solutions for Science and Industry
---
Kontakt-Informationen finden Sie im Header dieser Mail oder
auf der Webseite - http://www.pengutronix.de/impressum/ -
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] FCC: fix confused base / offset
On Wed, Apr 09, 2008 at 11:11:10AM -0500, Scott Wood wrote: On Wed, Apr 09, 2008 at 03:06:10PM +0200, Sascha Hauer wrote: This patch fixes a mixup in struct fs_platform_info. The struct has a field dpram_offset which really is no offset but an io pointer to the dpram (casted to u32). Right, I didn't want to touch that as the old-binding code should be going away soon anyway. Right, so it's probably not worth the effort. I stumbled on this while porting my board to the new binding code. I was quite confused when seeing this so I made this fix to understand what's going on here. BTW have you tested the FCC driver with the new binding code? I do not manage to get it working. Everything seems to be fine but fs_enet_start_xmit does not send a packet and no interrupts are arriving. Does this break arch/ppc, BTW? Yes. arch=ppc is out of my scope, so I forget about it regularly, sorry. It breaks arch/ppc/platforms/mpc8272ads_setup.c which can be easily fixed though. But as the mpc8272ads is supported with arch=powerpc, shouldn't it go away anyway? Sascha -- Pengutronix e.K. - Linux Solutions for Science and Industry --- Kontakt-Informationen finden Sie im Header dieser Mail oder auf der Webseite - http://www.pengutronix.de/impressum/ - ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
