From: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> Date: 2021-09-22 10:31:39
From the analog waveform analysis result, SD/SDIO controller
of AST2600 cannot always work well with 200MHz. The upper bound
stable frequency for SD/SDIO controller is 100MHz. Thus, SDR50
supported bit, instead of SDR104, in capability 2 register
should be set in advance.
Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
drivers/mmc/host/sdhci-of-aspeed.c | 8 ++++++++
1 file changed, 8 insertions(+)
From: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> Date: 2021-09-22 10:31:40
- There are two clock sources used to generate
SD/SDIO clock, APLL clock and HCLK (200MHz).
User can select which clock source should be used
by configuring SCU310[8].
- The SD/SDIO clock divider selection table SCU310[30:28]
is different between AST2600-A1 and AST2600-A2/A3.
For AST2600-A1, 200MHz SD/SDIO clock cannot be
gotten by the dividers in SCU310[30:28] if APLL
is not the multiple of 200MHz and HCLK is 200MHz.
For AST2600-A2/A3, a new divider, "1", is added and
200MHz SD/SDIO clock can be obtained by adopting HCLK
as clock source and setting SCU310[30:28] to 3b'111.
Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
drivers/clk/clk-ast2600.c | 69 ++++++++++++++++++++++++++++++++++-----
1 file changed, 61 insertions(+), 8 deletions(-)
From: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> Date: 2021-09-22 10:31:53
The maximum frequency for SD controller on AST2600 EVB is
100MHz. In order to achieve 100MHz, sd-uhs-sdr50 property
should be added and the driver will set the SDR50 supported
bit in capability 2 register during probing stage.
Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
arch/arm/boot/dts/aspeed-ast2600-evb.dts | 2 ++
1 file changed, 2 insertions(+)
From: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> Date: 2021-09-22 10:31:57
The clock phase degree may be between -360 to 360.
If the data signal is leading to the clock, the signedness
of clock phase is postive, otherwise, the signedness
is negative.
Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
drivers/mmc/core/host.c | 10 ++++++----
include/linux/mmc/host.h | 2 ++
2 files changed, 8 insertions(+), 4 deletions(-)
From: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> Date: 2021-09-22 10:32:03
- The maximum tap delay may be slightly different on
different platforms. It may also be different due to
different SoC processes or different manufacturers.
Thus, the maximum tap delay should be gotten from the
device tree through max-tap-delay property.
- The delay time for each tap is an absolute value which
is independent of clock frequency. But, in order to combine
this principle with "phase" concept, clock frequency is took
into consideration during calculating delay taps.
- The delay cell of eMMC device is non-uniform.
The time period of the first tap is two times of others.
- The clock phase degree range is from -360 to 360.
But, if the clock phase signedness is negative, clock signal
is output from the falling edge first by default and thus, clock
signal is leading to data signal by 90 degrees at least.
Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
drivers/mmc/host/sdhci-of-aspeed.c | 115 ++++++++++++++++++++++-------
1 file changed, 89 insertions(+), 26 deletions(-)
@@ -158,43 +160,60 @@ aspeed_sdc_set_phase_taps(struct aspeed_sdc *sdc,}#define PICOSECONDS_PER_SECOND 1000000000000ULL-#define ASPEED_SDHCI_NR_TAPS 15-/* Measured value with *handwave* environmentals and static loading */-#define ASPEED_SDHCI_MAX_TAP_DELAY_PS 1253+#define ASPEED_SDHCI_MAX_TAPS 15+staticintaspeed_sdhci_phase_to_tap(structdevice*dev,unsignedlongrate_hz,-intphase_deg)+boolinvert,intphase_deg,u32nr_taps){u64phase_period_ps;u64prop_delay_ps;u64clk_period_ps;-unsignedinttap;-u8inverted;+u32tap=0;+structaspeed_sdc*sdc=dev_get_drvdata(dev->parent);-phase_deg%=360;+if(sdc->max_tap_delay_ps==0)+return0;-if(phase_deg>=180){-inverted=ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;-phase_deg-=180;-dev_dbg(dev,-"Inverting clock to reduce phase correction from %d to %d degrees\n",-phase_deg+180,phase_deg);-}else{-inverted=0;+prop_delay_ps=sdc->max_tap_delay_ps/nr_taps;+clk_period_ps=div_u64(PICOSECONDS_PER_SECOND,(u64)rate_hz);++/*+*Forast2600,ifclockphasedegreeisnegative,clocksignalis+*outputfromfallingedgefirstbydefault.Namely,clocksignal+*isleadingtodatasignalby90degreesatleast.+*/+if(invert){+if(phase_deg>=90)+phase_deg-=90;+else+phase_deg=0;}-prop_delay_ps=ASPEED_SDHCI_MAX_TAP_DELAY_PS/ASPEED_SDHCI_NR_TAPS;-clk_period_ps=div_u64(PICOSECONDS_PER_SECOND,(u64)rate_hz);phase_period_ps=div_u64((u64)phase_deg*clk_period_ps,360ULL);-tap=div_u64(phase_period_ps,prop_delay_ps);-if(tap>ASPEED_SDHCI_NR_TAPS){+/*+*Thedelaycellisnon-uniformforeMMCcontroller.+*Thetimeperiodofthefirsttapistwotimesofothers.+*/+if(nr_taps==16&&phase_period_ps>prop_delay_ps*2){+phase_period_ps-=prop_delay_ps*2;+tap++;+}++tap+=div_u64(phase_period_ps,prop_delay_ps);+if(tap>ASPEED_SDHCI_MAX_TAPS){dev_dbg(dev,"Requested out of range phase tap %d for %d degrees of phase compensation at %luHz, clamping to tap %d\n",-tap,phase_deg,rate_hz,ASPEED_SDHCI_NR_TAPS);-tap=ASPEED_SDHCI_NR_TAPS;+tap,phase_deg,rate_hz,ASPEED_SDHCI_MAX_TAPS);+tap=ASPEED_SDHCI_MAX_TAPS;}-returninverted|tap;+if(invert){+dev_info(dev,"invert the clock\n");+tap|=ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;+}++returntap;}staticvoid
@@ -202,13 +221,19 @@ aspeed_sdhci_phases_to_taps(struct device *dev, unsigned long rate,conststructmmc_clk_phase*phases,structaspeed_sdhci_tap_param*taps){+structsdhci_host*host=dev->driver_data;+structaspeed_sdhci*sdhci;++sdhci=sdhci_pltfm_priv(sdhci_priv(host));taps->valid=phases->valid;if(!phases->valid)return;-taps->in=aspeed_sdhci_phase_to_tap(dev,rate,phases->in_deg);-taps->out=aspeed_sdhci_phase_to_tap(dev,rate,phases->out_deg);+taps->in=aspeed_sdhci_phase_to_tap(dev,rate,phases->inv_in_deg,+phases->in_deg,sdhci->phase_desc->nr_taps);+taps->out=aspeed_sdhci_phase_to_tap(dev,rate,phases->inv_out_deg,+phases->out_deg,sdhci->phase_desc->nr_taps);}staticvoid
@@ -230,8 +255,8 @@ aspeed_sdhci_configure_phase(struct sdhci_host *host, unsigned long rate)aspeed_sdc_set_phase_taps(sdhci->parent,sdhci->phase_desc,taps);dev_dbg(dev,"Using taps [%d, %d] for [%d, %d] degrees of phase correction at %luHz (%d)\n",-taps->in&ASPEED_SDHCI_NR_TAPS,-taps->out&ASPEED_SDHCI_NR_TAPS,+taps->in&ASPEED_SDHCI_MAX_TAPS,+taps->out&ASPEED_SDHCI_MAX_TAPS,params->in_deg,params->out_deg,rate,host->timing);}
From: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> Date: 2021-09-22 10:32:05
Since the eMMC device's delay parameters are different from
the SD's, a new compatible should be used to distinguish
between eMMC and SD device.
Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
arch/arm/boot/dts/aspeed-g6.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> Date: 2021-09-22 10:32:12
Add max-tap-delay proptery in order to record the maximum
tap delay on different platforms.
Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 3 +++
1 file changed, 3 insertions(+)
From: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> Date: 2021-09-22 10:32:16
Add "aspeed,ast2600-emmc" compatible string for the sake of
distinguishing between SD and eMMC device.
Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 1 +
1 file changed, 1 insertion(+)
From: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> Date: 2021-09-22 10:32:30
Change clock phase degree for AST2600 EVB.
These parameter has been verified with 100MHz
clock frequency for eMMC and SD controllers.
Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
arch/arm/boot/dts/aspeed-ast2600-evb-a1.dts | 8 ++++++++
arch/arm/boot/dts/aspeed-ast2600-evb.dts | 9 ++++++---
2 files changed, 14 insertions(+), 3 deletions(-)
@@ -143,13 +143,15 @@&emmc_controller{status="okay";+/* Measured value with *handwave* environmentals and static loading */+max-tap-delay=<736>;};&emmc{non-removable;bus-width=<4>;max-frequency=<100000000>;-clk-phase-mmc-hs200=<9>,<225>;+clk-phase-mmc-hs200=<027>,<195>;};&rtc{
From: Joel Stanley <joel@jms.id.au> Date: 2021-09-23 00:02:33
On Wed, 22 Sept 2021 at 10:31, Chin-Ting Kuo
[off-list ref] wrote:
quoted hunk
- There are two clock sources used to generate
SD/SDIO clock, APLL clock and HCLK (200MHz).
User can select which clock source should be used
by configuring SCU310[8].
- The SD/SDIO clock divider selection table SCU310[30:28]
is different between AST2600-A1 and AST2600-A2/A3.
For AST2600-A1, 200MHz SD/SDIO clock cannot be
gotten by the dividers in SCU310[30:28] if APLL
is not the multiple of 200MHz and HCLK is 200MHz.
For AST2600-A2/A3, a new divider, "1", is added and
200MHz SD/SDIO clock can be obtained by adopting HCLK
as clock source and setting SCU310[30:28] to 3b'111.
Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
drivers/clk/clk-ast2600.c | 69 ++++++++++++++++++++++++++++++++++-----
1 file changed, 61 insertions(+), 8 deletions(-)
I've got a little patch that I recommend you base your series on (feel
free to include it in your series when posting v2 to make it
self-contained):
https://lore.kernel.org/all/20210922235449.213631-1-joel@jms.id.au/
With this one you can do:
const struct clk_div_table* table;
if (soc_rev >= 2)
table = ast2600_sd_div_table;
else
table = ast2600_a1_sd_div_table;
Then you don't need to duplicate the registration for each case:
hw = clk_hw_register_divider_table(dev, "sd_extclk",
"sd_extclk_gate",
0, scu_g6_base +
ASPEED_G6_CLK_SELECTION4, 28, 3, 0,
table,
&aspeed_g6_clk_lock);
if (IS_ERR(hw))
return PTR_ERR(hw);
From: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> Date: 2021-09-23 05:31:15
Hi Joel,
Thanks for the review.
-----Original Message-----
From: Joel Stanley <joel@jms.id.au>
Sent: Thursday, September 23, 2021 8:02 AM
To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
Subject: Re: [PATCH 01/10] clk: aspeed: ast2600: Porting sdhci clock source
On Wed, 22 Sept 2021 at 10:31, Chin-Ting Kuo
[off-list ref] wrote:
quoted
- There are two clock sources used to generate
SD/SDIO clock, APLL clock and HCLK (200MHz).
User can select which clock source should be used
by configuring SCU310[8].
- The SD/SDIO clock divider selection table SCU310[30:28]
is different between AST2600-A1 and AST2600-A2/A3.
For AST2600-A1, 200MHz SD/SDIO clock cannot be
gotten by the dividers in SCU310[30:28] if APLL
is not the multiple of 200MHz and HCLK is 200MHz.
For AST2600-A2/A3, a new divider, "1", is added and
200MHz SD/SDIO clock can be obtained by adopting HCLK
as clock source and setting SCU310[30:28] to 3b'111.
Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
drivers/clk/clk-ast2600.c | 69
++++++++++++++++++++++++++++++++++-----
1 file changed, 61 insertions(+), 8 deletions(-)
+ 31, 0, &aspeed_g6_clk_lock); if (IS_ERR(hw)) return PTR_ERR(hw);++ regmap_read(map, 0x14, &val);+ /* AST2600-A2/A3 clock divisor is different from AST2600-A1 */+ if (((val & GENMASK(23, 16)) >> 16) >= 2) {
I've got a little patch that I recommend you base your series on (feel free to
include it in your series when posting v2 to make it
self-contained):
https://lore.kernel.org/all/20210922235449.213631-1-joel@jms.id.au/
With this one you can do:
const struct clk_div_table* table;
if (soc_rev >= 2)
table = ast2600_sd_div_table;
else
table = ast2600_a1_sd_div_table;
Then you don't need to duplicate the registration for each case:
hw = clk_hw_register_divider_table(dev, "sd_extclk",
"sd_extclk_gate",
0, scu_g6_base +
ASPEED_G6_CLK_SELECTION4, 28, 3, 0,
table,
&aspeed_g6_clk_lock);
if (IS_ERR(hw))
return PTR_ERR(hw);
Okay, I will include your patch into this patch series when posting v2.
From: Rob Herring <robh@kernel.org> Date: 2021-09-27 18:40:59
On Wed, Sep 22, 2021 at 06:31:15PM +0800, Chin-Ting Kuo wrote:
quoted hunk
Add max-tap-delay proptery in order to record the maximum
tap delay on different platforms.
Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 3 +++
1 file changed, 3 insertions(+)
From: Rob Herring <robh@kernel.org> Date: 2021-09-27 18:59:17
On Wed, Sep 22, 2021 at 06:31:16PM +0800, Chin-Ting Kuo wrote:
Add "aspeed,ast2600-emmc" compatible string for the sake of
distinguishing between SD and eMMC device.
Why?
Is the h/w block different? We already have properties to handle some of
the eMMC specifics. Also, you can have a child node for the eMMC device
if you need that.
From: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> Date: 2021-09-28 02:50:45
Hi Rob,
Thanks for your review.
-----Original Message-----
From: Rob Herring <robh@kernel.org>
Sent: Tuesday, September 28, 2021 2:41 AM
To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
Subject: Re: [PATCH 09/10] dt-bindings: mmc: aspeed: Add max-tap-delay
property
On Wed, Sep 22, 2021 at 06:31:15PM +0800, Chin-Ting Kuo wrote:
quoted
Add max-tap-delay proptery in order to record the maximum tap delay on
different platforms.
Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 3 +++
1 file changed, 3 insertions(+)
From: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> Date: 2021-09-28 02:51:09
Hi Rob,
-----Original Message-----
From: Rob Herring <robh@kernel.org>
Sent: Tuesday, September 28, 2021 2:59 AM
To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
Subject: Re: [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new compatible
string
On Wed, Sep 22, 2021 at 06:31:16PM +0800, Chin-Ting Kuo wrote:
quoted
Add "aspeed,ast2600-emmc" compatible string for the sake of
distinguishing between SD and eMMC device.
Why?
Is the h/w block different? We already have properties to handle some of the
eMMC specifics. Also, you can have a child node for the eMMC device if you
need that.
There are two SD/SDIO controllers in a AST2600 SoC.
One is for SD card and the other is for eMMC.
Although both of them are embedded in the same SoC, the design of delay cell and
the manufacture process are different. The delay phase is definitely different and, thus,
we need a flag, compatible, to distinguish the device, SD or eMMC.
Without "aspeed,ast2600-emmc" compatible, of course, eMMC device can work with original
sdhci driver and device tree setting. But, for ultra-speed or HS200 case, AST2600 SoC needs some
phase delay which (maximum) value is different between SD and eMMC device.
From: Rob Herring <robh@kernel.org> Date: 2021-09-28 22:28:42
On Mon, Sep 27, 2021 at 9:51 PM Chin-Ting Kuo
[off-list ref] wrote:
Hi Rob,
quoted
-----Original Message-----
From: Rob Herring <robh@kernel.org>
Sent: Tuesday, September 28, 2021 2:59 AM
To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
Subject: Re: [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new compatible
string
On Wed, Sep 22, 2021 at 06:31:16PM +0800, Chin-Ting Kuo wrote:
quoted
Add "aspeed,ast2600-emmc" compatible string for the sake of
distinguishing between SD and eMMC device.
Why?
Is the h/w block different? We already have properties to handle some of the
eMMC specifics. Also, you can have a child node for the eMMC device if you
need that.
There are two SD/SDIO controllers in a AST2600 SoC.
One is for SD card and the other is for eMMC.
Although both of them are embedded in the same SoC, the design of delay cell and
the manufacture process are different. The delay phase is definitely different and, thus,
we need a flag, compatible, to distinguish the device, SD or eMMC.
Without "aspeed,ast2600-emmc" compatible, of course, eMMC device can work with original
sdhci driver and device tree setting. But, for ultra-speed or HS200 case, AST2600 SoC needs some
phase delay which (maximum) value is different between SD and eMMC device.
This is quite common as tweaking the timing is also need per board.
Look at what other bindings have done. A property is more appropriate
here.
Rob
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> Date: 2021-09-29 03:03:59
Hi Rob
-----Original Message-----
From: Rob Herring <robh@kernel.org>
Sent: Wednesday, September 29, 2021 6:28 AM
To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
Subject: Re: [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new compatible
string
On Mon, Sep 27, 2021 at 9:51 PM Chin-Ting Kuo
[off-list ref] wrote:
quoted
Hi Rob,
quoted
-----Original Message-----
From: Rob Herring <robh@kernel.org>
Sent: Tuesday, September 28, 2021 2:59 AM
To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
Subject: Re: [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new
compatible string
On Wed, Sep 22, 2021 at 06:31:16PM +0800, Chin-Ting Kuo wrote:
quoted
Add "aspeed,ast2600-emmc" compatible string for the sake of
distinguishing between SD and eMMC device.
Why?
Is the h/w block different? We already have properties to handle
some of the eMMC specifics. Also, you can have a child node for the
eMMC device if you need that.
There are two SD/SDIO controllers in a AST2600 SoC.
One is for SD card and the other is for eMMC.
Although both of them are embedded in the same SoC, the design of
delay cell and the manufacture process are different. The delay phase
is definitely different and, thus, we need a flag, compatible, to distinguish the
device, SD or eMMC.
quoted
Without "aspeed,ast2600-emmc" compatible, of course, eMMC device can
work with original sdhci driver and device tree setting. But, for
ultra-speed or HS200 case, AST2600 SoC needs some phase delay which
(maximum) value is different between SD and eMMC device.
This is quite common as tweaking the timing is also need per board.
Look at what other bindings have done. A property is more appropriate here.
Okay, I will try to check whether there is an existing binding which can achieve this purpose.
Or, maybe, as you said, a property is better since this phase delay is a proprietary
HW design and is different between each chipset version.
From: Andrew Jeffery <hidden> Date: 2021-10-26 00:31:53
Hi Chin-Ting,
Sorry for the delay in looking at your series.
On Wed, 22 Sep 2021, at 20:01, Chin-Ting Kuo wrote:
quoted hunk
From the analog waveform analysis result, SD/SDIO controller
of AST2600 cannot always work well with 200MHz. The upper bound
stable frequency for SD/SDIO controller is 100MHz. Thus, SDR50
supported bit, instead of SDR104, in capability 2 register
should be set in advance.
Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
drivers/mmc/host/sdhci-of-aspeed.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/mmc/host/sdhci-of-aspeed.c
b/drivers/mmc/host/sdhci-of-aspeed.c
index 6e4e132903a6..c6eaeb02e3f9 100644
From: Andrew Jeffery <hidden> Date: 2021-10-26 00:42:58
On Wed, 22 Sep 2021, at 20:01, Chin-Ting Kuo wrote:
The maximum frequency for SD controller on AST2600 EVB is
100MHz. In order to achieve 100MHz, sd-uhs-sdr50 property
should be added and the driver will set the SDR50 supported
bit in capability 2 register during probing stage.
Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
As this is a limitation of the SoC it should be done in aspeed-g6.dtsi.
Unless I've misunderstood?
Andrew
From: Andrew Jeffery <hidden> Date: 2021-10-26 00:53:13
On Wed, 22 Sep 2021, at 20:01, Chin-Ting Kuo wrote:
The clock phase degree may be between -360 to 360.
If the data signal is leading to the clock, the signedness
of clock phase is postive, otherwise, the signedness
is negative.
Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
This fundamentally breaks any in-tree users. We can't do this.
In terms of the binding, if negative phase values are something we must
do, we can just extend the value range to include [-359, -1] right?
That avoids changing the type of the value positions in the manner this
patch does.
Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Andrew Jeffery <hidden> Date: 2021-10-26 03:10:30
Hi Chin-Ting,
I think we can split this up a bit:
On Wed, 22 Sep 2021, at 20:01, Chin-Ting Kuo wrote:
quoted hunk
- The maximum tap delay may be slightly different on different platforms. It may also be different due to different SoC processes or different manufacturers. Thus, the maximum tap delay should be gotten from the device tree through max-tap-delay property.
I think this could be a patch on its own
quoted hunk
- The delay time for each tap is an absolute value which is independent of clock frequency. But, in order to combine this principle with "phase" concept, clock frequency is took into consideration during calculating delay taps.- The delay cell of eMMC device is non-uniform. The time period of the first tap is two times of others.
Again, this could be a patch of its own
quoted hunk
- The clock phase degree range is from -360 to 360. But, if the clock phase signedness is negative, clock signal is output from the falling edge first by default and thus, clock signal is leading to data signal by 90 degrees at least.
This line of development is impacted by my comment on an earlier patch
in the series, so should be its own patch.
@@ -158,43 +160,60 @@ aspeed_sdc_set_phase_taps(struct aspeed_sdc *sdc,}#define PICOSECONDS_PER_SECOND 1000000000000ULL-#define ASPEED_SDHCI_NR_TAPS 15-/* Measured value with *handwave* environmentals and static loading */-#define ASPEED_SDHCI_MAX_TAP_DELAY_PS 1253+#define ASPEED_SDHCI_MAX_TAPS 15
Why are we renaming this? It looks to cause a bit of noise in the diff.
quoted hunk
+ static int aspeed_sdhci_phase_to_tap(struct device *dev, unsigned long rate_hz,- int phase_deg)+ bool invert, int phase_deg, u32 nr_taps)
Hmm.
quoted hunk
{ u64 phase_period_ps; u64 prop_delay_ps; u64 clk_period_ps;- unsigned int tap;- u8 inverted;+ u32 tap = 0;+ struct aspeed_sdc *sdc = dev_get_drvdata(dev->parent);- phase_deg %= 360;+ if (sdc->max_tap_delay_ps == 0)+ return 0;
I don't think just silently returning 0 here is the right thing to do.
What about -EINVAL, or printing a warning and using the old hard-coded
value?
quoted hunk
- if (phase_deg >= 180) {- inverted = ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;- phase_deg -= 180;- dev_dbg(dev,- "Inverting clock to reduce phase correction from %d to %d degrees\n",- phase_deg + 180, phase_deg);- } else {- inverted = 0;+ prop_delay_ps = sdc->max_tap_delay_ps / nr_taps;+ clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz);++ /*+ * For ast2600, if clock phase degree is negative, clock signal is+ * output from falling edge first by default. Namely, clock signal+ * is leading to data signal by 90 degrees at least.+ */
Have I missed something about a asymmetric clock timings? Otherwise the
falling edge is 180 degrees away from the rising edge? I'm still not
clear on why 90 degrees is used here.
quoted hunk
+ if (invert) {+ if (phase_deg >= 90)+ phase_deg -= 90;+ else+ phase_deg = 0;
Why are we throwing away information?
}
- prop_delay_ps = ASPEED_SDHCI_MAX_TAP_DELAY_PS / ASPEED_SDHCI_NR_TAPS;
- clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz);
phase_period_ps = div_u64((u64)phase_deg * clk_period_ps, 360ULL);
- tap = div_u64(phase_period_ps, prop_delay_ps);
- if (tap > ASPEED_SDHCI_NR_TAPS) {
+ /*
+ * The delay cell is non-uniform for eMMC controller.
+ * The time period of the first tap is two times of others.
+ */
+ if (nr_taps == 16 && phase_period_ps > prop_delay_ps * 2) {
+ phase_period_ps -= prop_delay_ps * 2;
+ tap++;
+ }
+
+ tap += div_u64(phase_period_ps, prop_delay_ps);
+ if (tap > ASPEED_SDHCI_MAX_TAPS) {
dev_dbg(dev,
"Requested out of range phase tap %d for %d degrees of phase
compensation at %luHz, clamping to tap %d\n",
- tap, phase_deg, rate_hz, ASPEED_SDHCI_NR_TAPS);
- tap = ASPEED_SDHCI_NR_TAPS;
+ tap, phase_deg, rate_hz, ASPEED_SDHCI_MAX_TAPS);
+ tap = ASPEED_SDHCI_MAX_TAPS;
}
- return inverted | tap;
+ if (invert) {
+ dev_info(dev, "invert the clock\n");
I prefer we drop this message
quoted hunk
+ tap |= ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;+ }++ return tap; } static void
ast2600_sdhci_phase[] = {
.enable_mask = ASPEED_SDC_S1_PHASE_OUT_EN,
.enable_value = 3,
},
+ .nr_taps = 15,
+ },
+};
+
+static const struct aspeed_sdhci_phase_desc ast2600_emmc_phase[] = {
+ /* eMMC slot 0 */
+ [0] = {
+ .in = {
+ .tap_mask = ASPEED_SDC_S0_PHASE_IN,
+ .enable_mask = ASPEED_SDC_S0_PHASE_IN_EN,
+ .enable_value = 1,
+ },
+ .out = {
+ .tap_mask = ASPEED_SDC_S0_PHASE_OUT,
+ .enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
+ .enable_value = 3,
+ },
+
+ /*
+ * There are 15 taps recorded in AST2600 datasheet.
+ * But, actually, the time period of the first tap
+ * is two times of others. Thus, 16 tap is used to
+ * emulate this situation.
+ */
+ .nr_taps = 16,
I think this is a very indirect way to communicate the problem. The
only time we look at nr_taps is in a test that explicitly compensates
for the non-uniform delay. I think we should just have a boolean struct
member called 'non_uniform_delay' rather than 'nr_taps', as the number
of taps isn't what's changing. But also see the discussion below about
a potential aspeed,tap-delays property.
This needs to be documented (and binding documentation patches need to
be the first patches in the series). Something further to consider is
whether we separate the compatibles or add e.g. an aspeed,tap-delays
property that specifies the specific delay of each logic element. This
might take the place of e.g. the max-tap-delay property?
Andrew
quoted hunk
{ }
};
@@ -562,6 +620,11 @@ static int aspeed_sdc_probe(struct platform_device *pdev) goto err_clk; }+ ret = of_property_read_u32(pdev->dev.of_node, "max-tap-delay",+ &sdc->max_tap_delay_ps);+ if (ret)+ sdc->max_tap_delay_ps = 0;+ dev_set_drvdata(&pdev->dev, sdc); parent = pdev->dev.of_node;
From: Paul Menzel <hidden> Date: 2021-10-26 06:10:24
Dear Chin-Ting,
Thank you for your patch. Some small things.
Please use imperative mood in the commit messages summary [1]:
clk: aspeed: ast2600: Port SDHCI clock source
On 22.09.21 12:31, Chin-Ting Kuo wrote:
quoted hunk
- There are two clock sources used to generate SD/SDIO clock, APLL clock and HCLK (200MHz). User can select which clock source should be used by configuring SCU310[8].- The SD/SDIO clock divider selection table SCU310[30:28] is different between AST2600-A1 and AST2600-A2/A3. For AST2600-A1, 200MHz SD/SDIO clock cannot be gotten by the dividers in SCU310[30:28] if APLL is not the multiple of 200MHz and HCLK is 200MHz. For AST2600-A2/A3, a new divider, "1", is added and 200MHz SD/SDIO clock can be obtained by adopting HCLK as clock source and setting SCU310[30:28] to 3b'111.
Please reference the datasheet name and version, and please reflow the commit message for 75 characters per line.
@@ -523,18 +552,42 @@ static int aspeed_g6_clk_probe(struct platform_device *pdev)returnPTR_ERR(hw);aspeed_g6_clk_data->hws[ASPEED_CLK_EMMC]=hw;-/* SD/SDIO clock divider and gate */-hw=clk_hw_register_gate(dev,"sd_extclk_gate","hpll",0,-scu_g6_base+ASPEED_G6_CLK_SELECTION4,31,0,-&aspeed_g6_clk_lock);+clk_hw_register_fixed_rate(NULL,"hclk",NULL,0,200000000);++regmap_read(map,0x310,&val);+hw=clk_hw_register_mux(dev,"sd_extclk_mux",+sd_extclk_parent_names,+ARRAY_SIZE(sd_extclk_parent_names),0,+scu_g6_base+ASPEED_G6_CLK_SELECTION4,8,1,+0,&aspeed_g6_clk_lock);if(IS_ERR(hw))returnPTR_ERR(hw);-hw=clk_hw_register_divider_table(dev,"sd_extclk","sd_extclk_gate",-0,scu_g6_base+ASPEED_G6_CLK_SELECTION4,28,3,0,-ast2600_div_table,-&aspeed_g6_clk_lock);++hw=clk_hw_register_gate(dev,"sd_extclk_gate","sd_extclk_mux",+0,scu_g6_base+ASPEED_G6_CLK_SELECTION4,+31,0,&aspeed_g6_clk_lock);if(IS_ERR(hw))returnPTR_ERR(hw);++regmap_read(map,0x14,&val);+/* AST2600-A2/A3 clock divisor is different from AST2600-A1 */+if(((val&GENMASK(23,16))>>16)>=2){+/* AST2600-A2/A3 */+hw=clk_hw_register_divider_table(dev,"sd_extclk","sd_extclk_gate",+0,scu_g6_base+ASPEED_G6_CLK_SELECTION4,28,3,0,+ast2600_sd_div_a2_table,+&aspeed_g6_clk_lock);+if(IS_ERR(hw))+returnPTR_ERR(hw);+}else{+/* AST2600-A1 */+hw=clk_hw_register_divider_table(dev,"sd_extclk","sd_extclk_gate",+0,scu_g6_base+ASPEED_G6_CLK_SELECTION4,28,3,0,+ast2600_sd_div_a1_table,+&aspeed_g6_clk_lock);+if(IS_ERR(hw))+returnPTR_ERR(hw);+}aspeed_g6_clk_data->hws[ASPEED_CLK_SDIO]=hw;/* MAC1/2 RMII 50MHz RCLK */
Does Linux already log, if A1 or A2/A3 is detected?
Should a debug message be added, what clock divisor is used?
Kind regards,
Paul
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> Date: 2021-11-06 10:01:31
Hi Andrew,
Thanks for the review.
-----Original Message-----
From: Andrew Jeffery <redacted>
Sent: Tuesday, October 26, 2021 8:31 AM
Subject: Re: [PATCH 02/10] sdhci: aspeed: Add SDR50 support
Hi Chin-Ting,
Sorry for the delay in looking at your series.
On Wed, 22 Sep 2021, at 20:01, Chin-Ting Kuo wrote:
quoted
From the analog waveform analysis result, SD/SDIO controller of
AST2600 cannot always work well with 200MHz. The upper bound stable
frequency for SD/SDIO controller is 100MHz. Thus, SDR50 supported bit,
instead of SDR104, in capability 2 register should be set in advance.
Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
drivers/mmc/host/sdhci-of-aspeed.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/mmc/host/sdhci-of-aspeed.c
b/drivers/mmc/host/sdhci-of-aspeed.c
index 6e4e132903a6..c6eaeb02e3f9 100644
From: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> Date: 2021-11-06 10:01:41
-----Original Message-----
From: Andrew Jeffery <redacted>
Sent: Tuesday, October 26, 2021 8:43 AM
Subject: Re: [PATCH 03/10] dts: aspeed: ast2600: Support SDR50 for SD device
On Wed, 22 Sep 2021, at 20:01, Chin-Ting Kuo wrote:
quoted
The maximum frequency for SD controller on AST2600 EVB is 100MHz. In
order to achieve 100MHz, sd-uhs-sdr50 property should be added and the
driver will set the SDR50 supported bit in capability 2 register
during probing stage.
Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
As this is a limitation of the SoC it should be done in aspeed-g6.dtsi.
Unless I've misunderstood?
Okay, it will be updated in the next patch version.
From: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> Date: 2021-11-06 10:03:02
Hi Andrew,
-----Original Message-----
From: Andrew Jeffery <redacted>
Sent: Tuesday, October 26, 2021 8:53 AM
Subject: Re: [PATCH 04/10] mmc: Add invert flag for clock phase signedness
On Wed, 22 Sep 2021, at 20:01, Chin-Ting Kuo wrote:
quoted
The clock phase degree may be between -360 to 360.
If the data signal is leading to the clock, the signedness of clock
phase is postive, otherwise, the signedness is negative.
Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
The implementation here can't be changed without a change to the binding
documentation:
Okay, the binding document will be changed in the next patch version.
This fundamentally breaks any in-tree users. We can't do this.
In terms of the binding, if negative phase values are something we must do,
we can just extend the value range to include [-359, -1] right?
Yes, agree it and I tried it before. But, it seems that the device tree doesn't support
negative value with "-" prefixed and there is no device tree related API used to get
the negative value from .dts. Thus, I tried to add an additional flag to present
negative value.
That avoids changing the type of the value positions in the manner this patch
does.
Andrew
From: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> Date: 2021-11-06 10:05:14
Hi Andrew,
-----Original Message-----
From: Andrew Jeffery <redacted>
Sent: Tuesday, October 26, 2021 11:10 AM
Subject: Re: [PATCH 05/10] mmc: aspeed: Adjust delay taps calculation method
Hi Chin-Ting,
I think we can split this up a bit:
On Wed, 22 Sep 2021, at 20:01, Chin-Ting Kuo wrote:
quoted
- The maximum tap delay may be slightly different on different platforms. It may also be different due to different SoC processes or different manufacturers. Thus, the maximum tap delay should be gotten from the device tree through max-tap-delay property.
I think this could be a patch on its own
Okay.
quoted
- The delay time for each tap is an absolute value which is independent of clock frequency. But, in order to combine this principle with "phase" concept, clock frequency is took into consideration during calculating delay taps.- The delay cell of eMMC device is non-uniform. The time period of the first tap is two times of others.
Again, this could be a patch of its own
Okay.
quoted
- The clock phase degree range is from -360 to 360. But, if the clock phase signedness is negative, clock signal is output from the falling edge first by default and thus, clock signal is leading to data signal by 90 degrees at least.
This line of development is impacted by my comment on an earlier patch in
the series, so should be its own patch.
*sdc, }
#define PICOSECONDS_PER_SECOND 1000000000000ULL
-#define ASPEED_SDHCI_NR_TAPS 15
-/* Measured value with *handwave* environmentals and static loading */
-#define ASPEED_SDHCI_MAX_TAP_DELAY_PS 1253
+#define ASPEED_SDHCI_MAX_TAPS 15
Why are we renaming this? It looks to cause a bit of noise in the diff.
Okay, it can be changed back to the original one in the next patch version.
quoted
+
static int aspeed_sdhci_phase_to_tap(struct device *dev, unsigned long
rate_hz,
quoted
- int phase_deg)
+ bool invert, int phase_deg, u32 nr_taps)
Hmm.
It will also be modified.
quoted
{ u64 phase_period_ps; u64 prop_delay_ps; u64 clk_period_ps;- unsigned int tap;- u8 inverted;+ u32 tap = 0;+ struct aspeed_sdc *sdc = dev_get_drvdata(dev->parent);- phase_deg %= 360;+ if (sdc->max_tap_delay_ps == 0)+ return 0;
I don't think just silently returning 0 here is the right thing to do.
What about -EINVAL, or printing a warning and using the old hard-coded
value?
Agree, both -EINVAL and printing a warning are better.
quoted
- if (phase_deg >= 180) {
- inverted = ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;
- phase_deg -= 180;
- dev_dbg(dev,
- "Inverting clock to reduce phase correction from %d to %d
degrees\n",
quoted
- phase_deg + 180, phase_deg);- } else {- inverted = 0;+ prop_delay_ps = sdc->max_tap_delay_ps / nr_taps;+ clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz);++ /*+ * For ast2600, if clock phase degree is negative, clock signal is+ * output from falling edge first by default. Namely, clock signal+ * is leading to data signal by 90 degrees at least.+ */
Have I missed something about a asymmetric clock timings? Otherwise the
falling edge is 180 degrees away from the rising edge? I'm still not clear on
why 90 degrees is used here.
Oh, you are right. It should be 180 degrees.
quoted
+ if (invert) {+ if (phase_deg >= 90)+ phase_deg -= 90;+ else+ phase_deg = 0;
Why are we throwing away information?
With the above correction, it should be modified as below.
If the "invert" is needed, we expect that its value should be greater than 180
degrees. We clear "phase_deg" if its value is unexpected. Maybe, a warning
should be shown and -EINVAL can be returned.
if (invert) {
if (phase_deg >= 180)
phase_deg -= 180;
else
phase_deg = 0;
}
- clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz);
phase_period_ps = div_u64((u64)phase_deg * clk_period_ps, 360ULL);
- tap = div_u64(phase_period_ps, prop_delay_ps);
- if (tap > ASPEED_SDHCI_NR_TAPS) {
+ /*
+ * The delay cell is non-uniform for eMMC controller.
+ * The time period of the first tap is two times of others.
+ */
+ if (nr_taps == 16 && phase_period_ps > prop_delay_ps * 2) {
+ phase_period_ps -= prop_delay_ps * 2;
+ tap++;
+ }
+
+ tap += div_u64(phase_period_ps, prop_delay_ps);
+ if (tap > ASPEED_SDHCI_MAX_TAPS) {
dev_dbg(dev,
"Requested out of range phase tap %d for %d degrees of phase
compensation at %luHz, clamping to tap %d\n",
- tap, phase_deg, rate_hz, ASPEED_SDHCI_NR_TAPS);
- tap = ASPEED_SDHCI_NR_TAPS;
+ tap, phase_deg, rate_hz, ASPEED_SDHCI_MAX_TAPS);
+ tap = ASPEED_SDHCI_MAX_TAPS;
}
- return inverted | tap;
+ if (invert) {
+ dev_info(dev, "invert the clock\n");
I prefer we drop this message
Okay.
quoted
+ tap |= ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;+ }++ return tap; } static void
ast2600_sdhci_phase[] = {
.enable_mask = ASPEED_SDC_S1_PHASE_OUT_EN,
.enable_value = 3,
},
+ .nr_taps = 15,
+ },
+};
+
+static const struct aspeed_sdhci_phase_desc ast2600_emmc_phase[] = {
+ /* eMMC slot 0 */
+ [0] = {
+ .in = {
+ .tap_mask = ASPEED_SDC_S0_PHASE_IN,
+ .enable_mask = ASPEED_SDC_S0_PHASE_IN_EN,
+ .enable_value = 1,
+ },
+ .out = {
+ .tap_mask = ASPEED_SDC_S0_PHASE_OUT,
+ .enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
+ .enable_value = 3,
+ },
+
+ /*
+ * There are 15 taps recorded in AST2600 datasheet.
+ * But, actually, the time period of the first tap
+ * is two times of others. Thus, 16 tap is used to
+ * emulate this situation.
+ */
+ .nr_taps = 16,
I think this is a very indirect way to communicate the problem. The only time
we look at nr_taps is in a test that explicitly compensates for the non-uniform
delay. I think we should just have a boolean struct member called
'non_uniform_delay' rather than 'nr_taps', as the number of taps isn't what's
changing. But also see the discussion below about a potential
aspeed,tap-delays property.
This needs to be documented (and binding documentation patches need to be
the first patches in the series).
Okay.
Something further to consider is whether we
separate the compatibles or add e.g. an aspeed,tap-delays property that
specifies the specific delay of each logic element. This might take the place of
e.g. the max-tap-delay property?
Yes, an additional property may be better.
Andrew
quoted
{ }
};
@@ -562,6 +620,11 @@ static int aspeed_sdc_probe(struct platform_device
*sdc, }
#define PICOSECONDS_PER_SECOND 1000000000000ULL
-#define ASPEED_SDHCI_NR_TAPS 15
-/* Measured value with *handwave* environmentals and static loading */
-#define ASPEED_SDHCI_MAX_TAP_DELAY_PS 1253
+#define ASPEED_SDHCI_MAX_TAPS 15
Why are we renaming this? It looks to cause a bit of noise in the diff.
Okay, it can be changed back to the original one in the next patch version.
Well, maybe it makes sense, but I think we have to take into account
how we describe the taps in the discussion below.
quoted
quoted
- if (phase_deg >= 180) {
- inverted = ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;
- phase_deg -= 180;
- dev_dbg(dev,
- "Inverting clock to reduce phase correction from %d to %d
degrees\n",
quoted
- phase_deg + 180, phase_deg);- } else {- inverted = 0;+ prop_delay_ps = sdc->max_tap_delay_ps / nr_taps;+ clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz);++ /*+ * For ast2600, if clock phase degree is negative, clock signal is+ * output from falling edge first by default. Namely, clock signal+ * is leading to data signal by 90 degrees at least.+ */
Have I missed something about a asymmetric clock timings? Otherwise the
falling edge is 180 degrees away from the rising edge? I'm still not clear on
why 90 degrees is used here.
Oh, you are right. It should be 180 degrees.
quoted
quoted
+ if (invert) {+ if (phase_deg >= 90)+ phase_deg -= 90;+ else+ phase_deg = 0;
Why are we throwing away information?
With the above correction, it should be modified as below.
If the "invert" is needed, we expect that its value should be greater than 180
degrees. We clear "phase_deg" if its value is unexpected. Maybe, a warning
should be shown and -EINVAL can be returned.
if (invert) {
if (phase_deg >= 180)
phase_deg -= 180;
else
phase_deg = 0;
Though we want this to return the EINVAL right?
\>> > + /*
quoted
quoted
+ * There are 15 taps recorded in AST2600 datasheet.+ * But, actually, the time period of the first tap+ * is two times of others. Thus, 16 tap is used to+ * emulate this situation.+ */+ .nr_taps = 16,
I think this is a very indirect way to communicate the problem. The only time
we look at nr_taps is in a test that explicitly compensates for the non-uniform
delay. I think we should just have a boolean struct member called
'non_uniform_delay' rather than 'nr_taps', as the number of taps isn't what's
changing. But also see the discussion below about a potential
aspeed,tap-delays property.
A new property may be the better choice.
I think I'm changing my mind on this sorry.
I'm think that aiming for fewer custom devicetree properties is better.
Using SoC-specific and device-specific compatibles (i.e. to
differentiate between the eMMC and SD controllers in the 2600) we can
just encode this data straight in the driver using the OF match data.
Rob and/or Joel: Thoughts?
quoted
Something further to consider is whether we
separate the compatibles or add e.g. an aspeed,tap-delays property that
specifies the specific delay of each logic element. This might take the place of
e.g. the max-tap-delay property?
Yes, an additional property may be better.
Again, flip-flopping on this a bit, the aspeed,ast2600-emmc compatible
is probably fine, and we add the tap delays in the OF match data for
the compatible. This means we get rid of any aspeed-specific devictree
properties with respect to the delays.
Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
This fundamentally breaks any in-tree users. We can't do this.
In terms of the binding, if negative phase values are something we must do,
we can just extend the value range to include [-359, -1] right?
Yes, agree it and I tried it before. But, it seems that the device tree
doesn't support
negative value with "-" prefixed and there is no device tree related
API used to get
the negative value from .dts. Thus, I tried to add an additional flag
to present
negative value.
Hmm. Still, I don't think we can break the binding this way.
Rob, Ulf, Adrian: What are your thoughts on handling phase offsets in
[-360, 360] in the binding? Do we append the flag field? Add a separate
property? I don't think interleaving the flags is desirable, though
interested in your thoughts.
Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com> Date: 2021-11-26 02:29:36
Hi Paul,
-----Original Message-----
From: Paul Menzel <redacted>
Sent: Tuesday, October 26, 2021 2:10 PM
To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
Cc: robh+dt@kernel.org; joel@jms.id.au; mturquette@baylibre.com;
sboyd@kernel.org; adrian.hunter@intel.com; linux-aspeed@lists.ozlabs.org;
openbmc@lists.ozlabs.org; linux-mmc@vger.kernel.org;
devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
linux-kernel@vger.kernel.org; linux-clk@vger.kernel.org; andrew@aj.id.au;
BMC-SW [off-list ref]; Steven Lee
[off-list ref]
Subject: Re: [PATCH 01/10] clk: aspeed: ast2600: Porting sdhci clock source
Dear Chin-Ting,
Thank you for your patch. Some small things.
Please use imperative mood in the commit messages summary [1]:
clk: aspeed: ast2600: Port SDHCI clock source
Okay.
On 22.09.21 12:31, Chin-Ting Kuo wrote:
quoted
- There are two clock sources used to generate SD/SDIO clock, APLL clock and HCLK (200MHz). User can select which clock source should be used by configuring SCU310[8].- The SD/SDIO clock divider selection table SCU310[30:28] is different between AST2600-A1 and AST2600-A2/A3. For AST2600-A1, 200MHz SD/SDIO clock cannot be gotten by the dividers in SCU310[30:28] if APLL is not the multiple of 200MHz and HCLK is 200MHz. For AST2600-A2/A3, a new divider, "1", is added and 200MHz SD/SDIO clock can be obtained by adopting HCLK as clock source and setting SCU310[30:28] to 3b'111.
Please reference the datasheet name and version, and please reflow the
commit message for 75 characters per line.
Okay, it will be updated in the next patch version.
quoted
Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
drivers/clk/clk-ast2600.c | 69