Re: [PATCH v2 2/3] spi: meson-axg: enhance output enable feature
Hi Jerome, On 2018/12/13 17:04, Jerome Brunet wrote: On Thu, 2018-12-13 at 16:39 +0800, Sunny Luo wrote: The SPICC controller in Meson-AXG is capable of driving the CLK/MOSI/SS signal lines through the idle state (between two transmission operation), which avoid the signals floating in unexpected state. @@ -453,6 +474,8 @@ static int meson_spicc_prepare_message(struct spi_master *master, writel_bits_relaxed(BIT(24), BIT(24), spicc->base + SPICC_TESTREG); + meson_spicc_oen_enable(spicc); + Any specific reason for doing this in prepare_message() ? It looks like something that could/should be done during the probe Yes, as replied in Mark's mail, i will move it into probe next patch.
Re: [PATCH v2 2/3] spi: meson-axg: enhance output enable feature
Hi Neil,
On 2018/12/13 16:53, Neil Armstrong wrote:
Hi Sunny,
On 13/12/2018 09:39, Sunny Luo wrote:
The SPICC controller in Meson-AXG is capable of driving the CLK/MOSI/SS
signal lines through the idle state (between two transmission operation),
which avoid the signals floating in unexpected state.
This is welcome, because it's really missing on GX...
I tried implementing it with pinctrl at [1], but it's complex.
Can you provide more info on how we should implement in on GX to be on par ?
[1]
https://github.com/superna/linux/commit/9c3a95659dd532d186556c1570c54d79ea5a4d45
GX is incapable of OEN. To be on par with it ,we have to pullup/down clk
pin as
you did at[1].
static const struct meson_spicc_data meson_spicc_gx_data = {
- .max_speed_hz = 3000,
+ .max_speed_hz = 3000,
Nitpick, but I would have kept the indentation here ...
};
static const struct meson_spicc_data meson_spicc_axg_data = {
- .max_speed_hz = 8000,
+ .max_speed_hz = 8000,
+ .has_oen= true,
same here
Anywy it's nitpick,
ok, i will revert it next patch.
Re: [PATCH v2 2/3] spi: meson-axg: enhance output enable feature
Hi Mark, On 2018/12/13 19:53, Mark Brown wrote: On Thu, Dec 13, 2018 at 10:04:56AM +0100, Jerome Brunet wrote: On Thu, 2018-12-13 at 16:39 +0800, Sunny Luo wrote: writel_bits_relaxed(BIT(24), BIT(24), spicc->base + SPICC_TESTREG); + meson_spicc_oen_enable(spicc); + Any specific reason for doing this in prepare_message() ? It looks like something that could/should be done during the probe ? If it's for power management then there should be a matching disable in unprepare_message() (or this should just be in the runtime PM code, though it's possible there's stuff that's only needed while actually doing transfers in which case this could make sense). OEN is only used to avoid the signals floating in unexpected state, i will move it into probe next patch. Please delete unneeded context from mails when replying. Doing this makes it much easier to find your reply in the message, helping ensure it won't be missed by people scrolling through the irrelevant quoted material. OK.
Re: [PATCH v2 2/3] spi: meson-axg: enhance output enable feature
On Thu, Dec 13, 2018 at 10:04:56AM +0100, Jerome Brunet wrote: > On Thu, 2018-12-13 at 16:39 +0800, Sunny Luo wrote: > > > > writel_bits_relaxed(BIT(24), BIT(24), spicc->base + SPICC_TESTREG); > > > > + meson_spicc_oen_enable(spicc); > > + > Any specific reason for doing this in prepare_message() ? It looks like > something that could/should be done during the probe ? If it's for power management then there should be a matching disable in unprepare_message() (or this should just be in the runtime PM code, though it's possible there's stuff that's only needed while actually doing transfers in which case this could make sense). Please delete unneeded context from mails when replying. Doing this makes it much easier to find your reply in the message, helping ensure it won't be missed by people scrolling through the irrelevant quoted material. signature.asc Description: PGP signature
Re: [PATCH v2 2/3] spi: meson-axg: enhance output enable feature
On Thu, 2018-12-13 at 16:39 +0800, Sunny Luo wrote:
> The SPICC controller in Meson-AXG is capable of driving the CLK/MOSI/SS
> signal lines through the idle state (between two transmission operation),
> which avoid the signals floating in unexpected state.
>
> Signed-off-by: Sunny Luo
> Signed-off-by: Yixun Lan
> ---
> drivers/spi/spi-meson-spicc.c | 28 ++--
> 1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
> index b56249d..0384c28 100644
> --- a/drivers/spi/spi-meson-spicc.c
> +++ b/drivers/spi/spi-meson-spicc.c
> @@ -115,6 +115,13 @@
>
> #define SPICC_DWADDR 0x24/* Write Address of DMA */
>
> +#define SPICC_ENH_CTL0 0x38/* Enhanced Feature */
> +#define SPICC_ENH_MOSI_OEN BIT(25)
> +#define SPICC_ENH_CLK_OENBIT(26)
> +#define SPICC_ENH_CS_OEN BIT(27)
> +#define SPICC_ENH_CLK_CS_DELAY_ENBIT(28)
> +#define SPICC_ENH_MAIN_CLK_AOBIT(29)
> +
> #define writel_bits_relaxed(mask, val, addr) \
> writel_relaxed((readl_relaxed(addr) & ~(mask)) | (val), addr)
>
> @@ -123,6 +130,7 @@
>
> struct meson_spicc_data {
> unsigned intmax_speed_hz;
> + boolhas_oen;
> };
>
> struct meson_spicc_device {
> @@ -145,6 +153,19 @@ struct meson_spicc_device {
> boolis_last_burst;
> };
>
> +static void meson_spicc_oen_enable(struct meson_spicc_device *spicc)
> +{
> + u32 conf;
> +
> + if (!spicc->data->has_oen)
> + return;
> +
> + conf = readl_relaxed(spicc->base + SPICC_ENH_CTL0) |
> + SPICC_ENH_MOSI_OEN | SPICC_ENH_CLK_OEN | SPICC_ENH_CS_OEN;
> +
> + writel_relaxed(conf, spicc->base + SPICC_ENH_CTL0);
> +}
> +
> static inline bool meson_spicc_txfull(struct meson_spicc_device *spicc)
> {
> return !!FIELD_GET(SPICC_TF,
> @@ -453,6 +474,8 @@ static int meson_spicc_prepare_message(struct spi_master
> *master,
>
> writel_bits_relaxed(BIT(24), BIT(24), spicc->base + SPICC_TESTREG);
>
> + meson_spicc_oen_enable(spicc);
> +
Any specific reason for doing this in prepare_message() ? It looks like
something that could/should be done during the probe ?
> return 0;
> }
>
> @@ -610,11 +633,12 @@ static int meson_spicc_remove(struct platform_device
> *pdev)
> }
>
> static const struct meson_spicc_data meson_spicc_gx_data = {
> - .max_speed_hz = 3000,
> + .max_speed_hz = 3000,
> };
>
> static const struct meson_spicc_data meson_spicc_axg_data = {
> - .max_speed_hz = 8000,
> + .max_speed_hz = 8000,
> + .has_oen= true,
> };
>
> static const struct of_device_id meson_spicc_of_match[] = {
Re: [PATCH v2 2/3] spi: meson-axg: enhance output enable feature
Hi Sunny,
On 13/12/2018 09:39, Sunny Luo wrote:
> The SPICC controller in Meson-AXG is capable of driving the CLK/MOSI/SS
> signal lines through the idle state (between two transmission operation),
> which avoid the signals floating in unexpected state.
This is welcome, because it's really missing on GX...
I tried implementing it with pinctrl at [1], but it's complex.
Can you provide more info on how we should implement in on GX to be on par ?
[1]
https://github.com/superna/linux/commit/9c3a95659dd532d186556c1570c54d79ea5a4d45
>
> Signed-off-by: Sunny Luo
> Signed-off-by: Yixun Lan
> ---
> drivers/spi/spi-meson-spicc.c | 28 ++--
> 1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
> index b56249d..0384c28 100644
> --- a/drivers/spi/spi-meson-spicc.c
> +++ b/drivers/spi/spi-meson-spicc.c
> @@ -115,6 +115,13 @@
>
> #define SPICC_DWADDR 0x24/* Write Address of DMA */
>
> +#define SPICC_ENH_CTL0 0x38/* Enhanced Feature */
> +#define SPICC_ENH_MOSI_OEN BIT(25)
> +#define SPICC_ENH_CLK_OENBIT(26)
> +#define SPICC_ENH_CS_OEN BIT(27)
> +#define SPICC_ENH_CLK_CS_DELAY_ENBIT(28)
> +#define SPICC_ENH_MAIN_CLK_AOBIT(29)
> +
> #define writel_bits_relaxed(mask, val, addr) \
> writel_relaxed((readl_relaxed(addr) & ~(mask)) | (val), addr)
>
> @@ -123,6 +130,7 @@
>
> struct meson_spicc_data {
> unsigned intmax_speed_hz;
> + boolhas_oen;
> };
>
> struct meson_spicc_device {
> @@ -145,6 +153,19 @@ struct meson_spicc_device {
> boolis_last_burst;
> };
>
> +static void meson_spicc_oen_enable(struct meson_spicc_device *spicc)
> +{
> + u32 conf;
> +
> + if (!spicc->data->has_oen)
> + return;
> +
> + conf = readl_relaxed(spicc->base + SPICC_ENH_CTL0) |
> + SPICC_ENH_MOSI_OEN | SPICC_ENH_CLK_OEN | SPICC_ENH_CS_OEN;
> +
> + writel_relaxed(conf, spicc->base + SPICC_ENH_CTL0);
> +}
> +
> static inline bool meson_spicc_txfull(struct meson_spicc_device *spicc)
> {
> return !!FIELD_GET(SPICC_TF,
> @@ -453,6 +474,8 @@ static int meson_spicc_prepare_message(struct spi_master
> *master,
>
> writel_bits_relaxed(BIT(24), BIT(24), spicc->base + SPICC_TESTREG);
>
> + meson_spicc_oen_enable(spicc);
> +
> return 0;
> }
>
> @@ -610,11 +633,12 @@ static int meson_spicc_remove(struct platform_device
> *pdev)
> }
>
> static const struct meson_spicc_data meson_spicc_gx_data = {
> - .max_speed_hz = 3000,
> + .max_speed_hz = 3000,
Nitpick, but I would have kept the indentation here ...
> };
>
> static const struct meson_spicc_data meson_spicc_axg_data = {
> - .max_speed_hz = 8000,
> + .max_speed_hz = 8000,
> + .has_oen= true,
same here
> };
>
> static const struct of_device_id meson_spicc_of_match[] = {
>
Anywy it's nitpick,
Reviewed-by: Neil Armstrong
[PATCH v2 2/3] spi: meson-axg: enhance output enable feature
The SPICC controller in Meson-AXG is capable of driving the CLK/MOSI/SS
signal lines through the idle state (between two transmission operation),
which avoid the signals floating in unexpected state.
Signed-off-by: Sunny Luo
Signed-off-by: Yixun Lan
---
drivers/spi/spi-meson-spicc.c | 28 ++--
1 file changed, 26 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi-meson-spicc.c b/drivers/spi/spi-meson-spicc.c
index b56249d..0384c28 100644
--- a/drivers/spi/spi-meson-spicc.c
+++ b/drivers/spi/spi-meson-spicc.c
@@ -115,6 +115,13 @@
#define SPICC_DWADDR 0x24/* Write Address of DMA */
+#define SPICC_ENH_CTL0 0x38/* Enhanced Feature */
+#define SPICC_ENH_MOSI_OEN BIT(25)
+#define SPICC_ENH_CLK_OEN BIT(26)
+#define SPICC_ENH_CS_OEN BIT(27)
+#define SPICC_ENH_CLK_CS_DELAY_EN BIT(28)
+#define SPICC_ENH_MAIN_CLK_AO BIT(29)
+
#define writel_bits_relaxed(mask, val, addr) \
writel_relaxed((readl_relaxed(addr) & ~(mask)) | (val), addr)
@@ -123,6 +130,7 @@
struct meson_spicc_data {
unsigned intmax_speed_hz;
+ boolhas_oen;
};
struct meson_spicc_device {
@@ -145,6 +153,19 @@ struct meson_spicc_device {
boolis_last_burst;
};
+static void meson_spicc_oen_enable(struct meson_spicc_device *spicc)
+{
+ u32 conf;
+
+ if (!spicc->data->has_oen)
+ return;
+
+ conf = readl_relaxed(spicc->base + SPICC_ENH_CTL0) |
+ SPICC_ENH_MOSI_OEN | SPICC_ENH_CLK_OEN | SPICC_ENH_CS_OEN;
+
+ writel_relaxed(conf, spicc->base + SPICC_ENH_CTL0);
+}
+
static inline bool meson_spicc_txfull(struct meson_spicc_device *spicc)
{
return !!FIELD_GET(SPICC_TF,
@@ -453,6 +474,8 @@ static int meson_spicc_prepare_message(struct spi_master
*master,
writel_bits_relaxed(BIT(24), BIT(24), spicc->base + SPICC_TESTREG);
+ meson_spicc_oen_enable(spicc);
+
return 0;
}
@@ -610,11 +633,12 @@ static int meson_spicc_remove(struct platform_device
*pdev)
}
static const struct meson_spicc_data meson_spicc_gx_data = {
- .max_speed_hz = 3000,
+ .max_speed_hz = 3000,
};
static const struct meson_spicc_data meson_spicc_axg_data = {
- .max_speed_hz = 8000,
+ .max_speed_hz = 8000,
+ .has_oen= true,
};
static const struct of_device_id meson_spicc_of_match[] = {
--
2.7.4
