2016-04-06 08:39:32

by Sugar Zhang

[permalink] [raw]
Subject: [PATCH] ASoC: rockchip: i2s: configure the sdio pins' iomux mode

There are 3 i2s sdio pins, which iomux mode is as follows:

- sdi3_sdo1
- sdi2_sdo2
- sdi1_sdo3

we need to configure these pins' iomux mode via the GRF register
when use multi channel playback/capture.

Signed-off-by: Sugar Zhang <[email protected]>
---

.../devicetree/bindings/sound/rockchip-i2s.txt | 5 +++
sound/soc/rockchip/rockchip_i2s.c | 39 +++++++++++++++++++++-
sound/soc/rockchip/rockchip_i2s.h | 8 +++++
3 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/sound/rockchip-i2s.txt b/Documentation/devicetree/bindings/sound/rockchip-i2s.txt
index 6e86d8a..ad72a7d 100644
--- a/Documentation/devicetree/bindings/sound/rockchip-i2s.txt
+++ b/Documentation/devicetree/bindings/sound/rockchip-i2s.txt
@@ -23,6 +23,11 @@ Required properties:
- rockchip,playback-channels: max playback channels, if not set, 8 channels default.
- rockchip,capture-channels: max capture channels, if not set, 2 channels default.

+Required properties for controller which support multi channels playback/capture:
+
+- rockchip,grf: Should be phandle/offset pair. the phandle of the syscon node for GRF register,
+ and the offset of the GRF for control register.
+
Example for rk3288 I2S controller:

i2s@ff890000 {
diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
index 2f8e204..bc72780 100644
--- a/sound/soc/rockchip/rockchip_i2s.c
+++ b/sound/soc/rockchip/rockchip_i2s.c
@@ -11,6 +11,7 @@
*/

#include <linux/module.h>
+#include <linux/mfd/syscon.h>
#include <linux/delay.h>
#include <linux/of_gpio.h>
#include <linux/clk.h>
@@ -33,6 +34,8 @@ struct rk_i2s_dev {
struct snd_dmaengine_dai_dma_data playback_dma_data;

struct regmap *regmap;
+ struct regmap *grf;
+ u32 iocfg_reg;

bool is_master_mode;
};
@@ -277,6 +280,29 @@ static int rockchip_i2s_hw_params(struct snd_pcm_substream *substream,
I2S_TXCR_VDW_MASK | I2S_TXCR_CSR_MASK,
val);

+ if (!IS_ERR(i2s->grf)) {
+ regmap_read(i2s->regmap, I2S_TXCR, &val);
+ val &= I2S_TXCR_CSR_MASK;
+
+ switch (val) {
+ case I2S_CHN_4:
+ val = I2S_IO_4CH_OUT_6CH_IN;
+ break;
+ case I2S_CHN_6:
+ val = I2S_IO_6CH_OUT_4CH_IN;
+ break;
+ case I2S_CHN_8:
+ val = I2S_IO_8CH_OUT_2CH_IN;
+ break;
+ default:
+ val = I2S_IO_2CH_OUT_8CH_IN;
+ break;
+ }
+
+ regmap_write(i2s->grf, i2s->iocfg_reg,
+ I2S_IO_DIRECTION_MASK << 16 | val);
+ }
+
regmap_update_bits(i2s->regmap, I2S_DMACR, I2S_DMACR_TDL_MASK,
I2S_DMACR_TDL(16));
regmap_update_bits(i2s->regmap, I2S_DMACR, I2S_DMACR_RDL_MASK,
@@ -478,6 +504,18 @@ static int rockchip_i2s_probe(struct platform_device *pdev)
return -ENOMEM;
}

+ i2s->dev = &pdev->dev;
+
+ i2s->grf = syscon_regmap_lookup_by_phandle(node, "rockchip,grf");
+ if (!IS_ERR(i2s->grf)) {
+ ret = of_property_read_u32_index(node, "rockchip,grf",
+ 1, &i2s->iocfg_reg);
+ if (ret) {
+ dev_err(&pdev->dev, "Can't get iocfg_reg offset\n");
+ return ret;
+ }
+ }
+
/* try to prepare related clocks */
i2s->hclk = devm_clk_get(&pdev->dev, "i2s_hclk");
if (IS_ERR(i2s->hclk)) {
@@ -517,7 +555,6 @@ static int rockchip_i2s_probe(struct platform_device *pdev)
i2s->capture_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
i2s->capture_dma_data.maxburst = 4;

- i2s->dev = &pdev->dev;
dev_set_drvdata(&pdev->dev, i2s);

pm_runtime_enable(&pdev->dev);
diff --git a/sound/soc/rockchip/rockchip_i2s.h b/sound/soc/rockchip/rockchip_i2s.h
index dc6e2c7..9a6aabf 100644
--- a/sound/soc/rockchip/rockchip_i2s.h
+++ b/sound/soc/rockchip/rockchip_i2s.h
@@ -236,4 +236,12 @@ enum {
#define I2S_TXDR (0x0024)
#define I2S_RXDR (0x0028)

+/* io direction cfg register */
+#define I2S_IO_DIRECTION_SHIFT 11
+#define I2S_IO_DIRECTION_MASK (7 << I2S_IO_DIRECTION_SHIFT)
+#define I2S_IO_8CH_OUT_2CH_IN (0 << I2S_IO_DIRECTION_SHIFT)
+#define I2S_IO_6CH_OUT_4CH_IN (1 << I2S_IO_DIRECTION_SHIFT)
+#define I2S_IO_4CH_OUT_6CH_IN (3 << I2S_IO_DIRECTION_SHIFT)
+#define I2S_IO_2CH_OUT_8CH_IN (7 << I2S_IO_DIRECTION_SHIFT)
+
#endif /* _ROCKCHIP_IIS_H */
--
1.9.1



2016-04-07 17:58:22

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH] ASoC: rockchip: i2s: configure the sdio pins' iomux mode

On Wed, Apr 06, 2016 at 04:38:22PM +0800, Sugar Zhang wrote:
> There are 3 i2s sdio pins, which iomux mode is as follows:

s/i2s sdio/I2S\/SDIO muxed/

>
> - sdi3_sdo1
> - sdi2_sdo2
> - sdi1_sdo3
>
> we need to configure these pins' iomux mode via the GRF register
> when use multi channel playback/capture.

Why not a pinctrl binding here? If you want SDIO mode, then you need
similar code in the SDIO driver (or just rely on default mode).

>
> Signed-off-by: Sugar Zhang <[email protected]>
> ---
>
> .../devicetree/bindings/sound/rockchip-i2s.txt | 5 +++
> sound/soc/rockchip/rockchip_i2s.c | 39 +++++++++++++++++++++-
> sound/soc/rockchip/rockchip_i2s.h | 8 +++++
> 3 files changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/sound/rockchip-i2s.txt b/Documentation/devicetree/bindings/sound/rockchip-i2s.txt
> index 6e86d8a..ad72a7d 100644
> --- a/Documentation/devicetree/bindings/sound/rockchip-i2s.txt
> +++ b/Documentation/devicetree/bindings/sound/rockchip-i2s.txt
> @@ -23,6 +23,11 @@ Required properties:
> - rockchip,playback-channels: max playback channels, if not set, 8 channels default.
> - rockchip,capture-channels: max capture channels, if not set, 2 channels default.
>
> +Required properties for controller which support multi channels playback/capture:
> +
> +- rockchip,grf: Should be phandle/offset pair. the phandle of the syscon node for GRF register,

Wrap your lines at less than 80 chars.

> + and the offset of the GRF for control register.
> +
> Example for rk3288 I2S controller:
>
> i2s@ff890000 {

2016-04-08 09:27:24

by Sugar Zhang

[permalink] [raw]
Subject: Re: [PATCH] ASoC: rockchip: i2s: configure the sdio pins' iomux mode

Hi Rob,

On 4/8/2016 01:58, Rob Herring Wrote:
> On Wed, Apr 06, 2016 at 04:38:22PM +0800, Sugar Zhang wrote:
>> There are 3 i2s sdio pins, which iomux mode is as follows:
>
> s/i2s sdio/I2S\/SDIO muxed/
>
>>
>> - sdi3_sdo1
>> - sdi2_sdo2
>> - sdi1_sdo3
>>
>> we need to configure these pins' iomux mode via the GRF register
>> when use multi channel playback/capture.
>
> Why not a pinctrl binding here? If you want SDIO mode, then you need
> similar code in the SDIO driver (or just rely on default mode).
>
here, i2s_sdio means i2s_sdi/i2s_sdo, not means SDIO bus interface.
for example:

gpio3d[6] iomux select:
2'b 00: gpio
2'b 01: i2s0_sdi3sdo1
2'b 10: xxx
2'b 11: xxx

we configure the i2s_sdi3sdo1 mode via the pinctrl, and then need another
grf register to configure it is used for i2s0_sdi3 or i2s0_sdo1.

>>
>> Signed-off-by: Sugar Zhang <[email protected]>
>> ---
>>
>> .../devicetree/bindings/sound/rockchip-i2s.txt | 5 +++
>> sound/soc/rockchip/rockchip_i2s.c | 39 +++++++++++++++++++++-
>> sound/soc/rockchip/rockchip_i2s.h | 8 +++++
>> 3 files changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/sound/rockchip-i2s.txt b/Documentation/devicetree/bindings/sound/rockchip-i2s.txt
>> index 6e86d8a..ad72a7d 100644
>> --- a/Documentation/devicetree/bindings/sound/rockchip-i2s.txt
>> +++ b/Documentation/devicetree/bindings/sound/rockchip-i2s.txt
>> @@ -23,6 +23,11 @@ Required properties:
>> - rockchip,playback-channels: max playback channels, if not set, 8 channels default.
>> - rockchip,capture-channels: max capture channels, if not set, 2 channels default.
>>
>> +Required properties for controller which support multi channels playback/capture:
>> +
>> +- rockchip,grf: Should be phandle/offset pair. the phandle of the syscon node for GRF register,
>
> Wrap your lines at less than 80 chars.

Ok, will fix.
>
>> + and the offset of the GRF for control register.
>> +
>> Example for rk3288 I2S controller:
>>
>> i2s@ff890000 {
>
> _______________________________________________
> Linux-rockchip mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
>

2016-04-08 17:15:52

by Heiko Stübner

[permalink] [raw]
Subject: Re: [PATCH] ASoC: rockchip: i2s: configure the sdio pins' iomux mode

Hi,

Am Mittwoch, 6. April 2016, 16:38:22 schrieb Sugar Zhang:
> There are 3 i2s sdio pins, which iomux mode is as follows:
>
> - sdi3_sdo1
> - sdi2_sdo2
> - sdi1_sdo3
>
> we need to configure these pins' iomux mode via the GRF register
> when use multi channel playback/capture.
>
> Signed-off-by: Sugar Zhang <[email protected]>
> ---
>
> .../devicetree/bindings/sound/rockchip-i2s.txt | 5 +++
> sound/soc/rockchip/rockchip_i2s.c | 39
> +++++++++++++++++++++- sound/soc/rockchip/rockchip_i2s.h
> | 8 +++++
> 3 files changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/sound/rockchip-i2s.txt
> b/Documentation/devicetree/bindings/sound/rockchip-i2s.txt index
> 6e86d8a..ad72a7d 100644
> --- a/Documentation/devicetree/bindings/sound/rockchip-i2s.txt
> +++ b/Documentation/devicetree/bindings/sound/rockchip-i2s.txt
> @@ -23,6 +23,11 @@ Required properties:
> - rockchip,playback-channels: max playback channels, if not set, 8
> channels default. - rockchip,capture-channels: max capture channels, if
> not set, 2 channels default.
>
> +Required properties for controller which support multi channels
> playback/capture: +
> +- rockchip,grf: Should be phandle/offset pair. the phandle of the syscon
> node for GRF register, + and the offset of the GRF for control register.

I think I'd like it more to use the generic grf-binding we always use
everwhere else (just the phandle without any offset) and keep the actual
offset in the driver on a per-soc basis.

That way rockchip,grf stays consistent over all users.

We already have the per-soc compatible values, so it should a easy to add a
.data element with the necessary offset-information.

> +
> Example for rk3288 I2S controller:
>
> i2s@ff890000 {
> diff --git a/sound/soc/rockchip/rockchip_i2s.c
> b/sound/soc/rockchip/rockchip_i2s.c index 2f8e204..bc72780 100644
> --- a/sound/soc/rockchip/rockchip_i2s.c
> +++ b/sound/soc/rockchip/rockchip_i2s.c

[...]

> @@ -478,6 +504,18 @@ static int rockchip_i2s_probe(struct platform_device
> *pdev) return -ENOMEM;
> }
>
> + i2s->dev = &pdev->dev;
> +
> + i2s->grf = syscon_regmap_lookup_by_phandle(node, "rockchip,grf");
> + if (!IS_ERR(i2s->grf)) {
> + ret = of_property_read_u32_index(node, "rockchip,grf",
> + 1, &i2s->iocfg_reg);
> + if (ret) {
> + dev_err(&pdev->dev, "Can't get iocfg_reg offset\n");
> + return ret;
> + }
> + }
> +

as said in the binding part, please use the generic grf handling and get the
io-offset from per-soc devicetree data in the driver itself.


> /* try to prepare related clocks */
> i2s->hclk = devm_clk_get(&pdev->dev, "i2s_hclk");
> if (IS_ERR(i2s->hclk)) {
> @@ -517,7 +555,6 @@ static int rockchip_i2s_probe(struct platform_device
> *pdev) i2s->capture_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> i2s->capture_dma_data.maxburst = 4;
>
> - i2s->dev = &pdev->dev;
> dev_set_drvdata(&pdev->dev, i2s);
>
> pm_runtime_enable(&pdev->dev);
> diff --git a/sound/soc/rockchip/rockchip_i2s.h
> b/sound/soc/rockchip/rockchip_i2s.h index dc6e2c7..9a6aabf 100644
> --- a/sound/soc/rockchip/rockchip_i2s.h
> +++ b/sound/soc/rockchip/rockchip_i2s.h
> @@ -236,4 +236,12 @@ enum {
> #define I2S_TXDR (0x0024)
> #define I2S_RXDR (0x0028)
>
> +/* io direction cfg register */
> +#define I2S_IO_DIRECTION_SHIFT 11

this setting is sitting in GRF_SOC_CON8 on the rk3399. That is part of the
very volatile register set where settings move around a lot for each soc.

So if we're having the io-offset in per-soc data, we can easily put the
shift into it as well.

> +#define I2S_IO_DIRECTION_MASK (7 << I2S_IO_DIRECTION_SHIFT)
> +#define I2S_IO_8CH_OUT_2CH_IN (0 << I2S_IO_DIRECTION_SHIFT)
> +#define I2S_IO_6CH_OUT_4CH_IN (1 << I2S_IO_DIRECTION_SHIFT)
> +#define I2S_IO_4CH_OUT_6CH_IN (3 << I2S_IO_DIRECTION_SHIFT)
> +#define I2S_IO_2CH_OUT_8CH_IN (7 << I2S_IO_DIRECTION_SHIFT)

Keep the settings without the shift here and do the shift dynamically with
the per-soc setting when changing the setting.


Thanks
Heiko

2016-04-11 01:19:16

by Sugar Zhang

[permalink] [raw]
Subject: Re: [PATCH] ASoC: rockchip: i2s: configure the sdio pins' iomux mode

Hi Heiko,

Got it, thanks for your advice, these will be done in next version.

On 4/9/2016 01:14, Heiko Stuebner Wrote:
> Hi,
>
> Am Mittwoch, 6. April 2016, 16:38:22 schrieb Sugar Zhang:
>> There are 3 i2s sdio pins, which iomux mode is as follows:
>>
>> - sdi3_sdo1
>> - sdi2_sdo2
>> - sdi1_sdo3
>>
>> we need to configure these pins' iomux mode via the GRF register
>> when use multi channel playback/capture.
>>
>> Signed-off-by: Sugar Zhang <[email protected]>
>> ---
>>
>> .../devicetree/bindings/sound/rockchip-i2s.txt | 5 +++
>> sound/soc/rockchip/rockchip_i2s.c | 39
>> +++++++++++++++++++++- sound/soc/rockchip/rockchip_i2s.h
>> | 8 +++++
>> 3 files changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/sound/rockchip-i2s.txt
>> b/Documentation/devicetree/bindings/sound/rockchip-i2s.txt index
>> 6e86d8a..ad72a7d 100644
>> --- a/Documentation/devicetree/bindings/sound/rockchip-i2s.txt
>> +++ b/Documentation/devicetree/bindings/sound/rockchip-i2s.txt
>> @@ -23,6 +23,11 @@ Required properties:
>> - rockchip,playback-channels: max playback channels, if not set, 8
>> channels default. - rockchip,capture-channels: max capture channels, if
>> not set, 2 channels default.
>>
>> +Required properties for controller which support multi channels
>> playback/capture: +
>> +- rockchip,grf: Should be phandle/offset pair. the phandle of the syscon
>> node for GRF register, + and the offset of the GRF for control register.
>
> I think I'd like it more to use the generic grf-binding we always use
> everwhere else (just the phandle without any offset) and keep the actual
> offset in the driver on a per-soc basis.
>
> That way rockchip,grf stays consistent over all users.
>
> We already have the per-soc compatible values, so it should a easy to add a
> .data element with the necessary offset-information.
>
>> +
>> Example for rk3288 I2S controller:
>>
>> i2s@ff890000 {
>> diff --git a/sound/soc/rockchip/rockchip_i2s.c
>> b/sound/soc/rockchip/rockchip_i2s.c index 2f8e204..bc72780 100644
>> --- a/sound/soc/rockchip/rockchip_i2s.c
>> +++ b/sound/soc/rockchip/rockchip_i2s.c
>
> [...]
>
>> @@ -478,6 +504,18 @@ static int rockchip_i2s_probe(struct platform_device
>> *pdev) return -ENOMEM;
>> }
>>
>> + i2s->dev = &pdev->dev;
>> +
>> + i2s->grf = syscon_regmap_lookup_by_phandle(node, "rockchip,grf");
>> + if (!IS_ERR(i2s->grf)) {
>> + ret = of_property_read_u32_index(node, "rockchip,grf",
>> + 1, &i2s->iocfg_reg);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Can't get iocfg_reg offset\n");
>> + return ret;
>> + }
>> + }
>> +
>
> as said in the binding part, please use the generic grf handling and get the
> io-offset from per-soc devicetree data in the driver itself.
>
>
>> /* try to prepare related clocks */
>> i2s->hclk = devm_clk_get(&pdev->dev, "i2s_hclk");
>> if (IS_ERR(i2s->hclk)) {
>> @@ -517,7 +555,6 @@ static int rockchip_i2s_probe(struct platform_device
>> *pdev) i2s->capture_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
>> i2s->capture_dma_data.maxburst = 4;
>>
>> - i2s->dev = &pdev->dev;
>> dev_set_drvdata(&pdev->dev, i2s);
>>
>> pm_runtime_enable(&pdev->dev);
>> diff --git a/sound/soc/rockchip/rockchip_i2s.h
>> b/sound/soc/rockchip/rockchip_i2s.h index dc6e2c7..9a6aabf 100644
>> --- a/sound/soc/rockchip/rockchip_i2s.h
>> +++ b/sound/soc/rockchip/rockchip_i2s.h
>> @@ -236,4 +236,12 @@ enum {
>> #define I2S_TXDR (0x0024)
>> #define I2S_RXDR (0x0028)
>>
>> +/* io direction cfg register */
>> +#define I2S_IO_DIRECTION_SHIFT 11
>
> this setting is sitting in GRF_SOC_CON8 on the rk3399. That is part of the
> very volatile register set where settings move around a lot for each soc.
>
> So if we're having the io-offset in per-soc data, we can easily put the
> shift into it as well.
>
>> +#define I2S_IO_DIRECTION_MASK (7 << I2S_IO_DIRECTION_SHIFT)
>> +#define I2S_IO_8CH_OUT_2CH_IN (0 << I2S_IO_DIRECTION_SHIFT)
>> +#define I2S_IO_6CH_OUT_4CH_IN (1 << I2S_IO_DIRECTION_SHIFT)
>> +#define I2S_IO_4CH_OUT_6CH_IN (3 << I2S_IO_DIRECTION_SHIFT)
>> +#define I2S_IO_2CH_OUT_8CH_IN (7 << I2S_IO_DIRECTION_SHIFT)
>
> Keep the settings without the shift here and do the shift dynamically with
> the per-soc setting when changing the setting.
>
>
> Thanks
> Heiko
>
> _______________________________________________
> Linux-rockchip mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
>