| Message ID | 20210922103116.30652-5-chin-ting_kuo@aspeedtech.com |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | ASPEED SD/eMMC controller clock configuration | expand |
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> The implementation here can't be changed without a change to the binding documentation: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mmc/mmc-controller.yaml?h=v5.15-rc7#n345 > --- > drivers/mmc/core/host.c | 10 ++++++---- > include/linux/mmc/host.h | 2 ++ > 2 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c > index d4683b1d263f..c2de7cbc7838 100644 > --- a/drivers/mmc/core/host.c > +++ b/drivers/mmc/core/host.c > @@ -214,14 +214,16 @@ static void mmc_retune_timer(struct timer_list *t) > static void mmc_of_parse_timing_phase(struct device *dev, const char *prop, > struct mmc_clk_phase *phase) > { > - int degrees[2] = {0}; > + int degrees[4] = {0}; > int rc; > > - rc = device_property_read_u32_array(dev, prop, degrees, 2); > + rc = device_property_read_u32_array(dev, prop, degrees, 4); > phase->valid = !rc; > if (phase->valid) { > - phase->in_deg = degrees[0]; > - phase->out_deg = degrees[1]; > + phase->inv_in_deg = degrees[0] ? true : false; > + phase->in_deg = degrees[1]; > + phase->inv_out_deg = degrees[2] ? true : false; > + phase->out_deg = degrees[3]; 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
Hi Andrew, > -----Original Message----- > From: Andrew Jeffery <andrew@aj.id.au> > 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: > > 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. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docume > ntation/devicetree/bindings/mmc/mmc-controller.yaml?h=v5.15-rc7#n345 > > > --- > > drivers/mmc/core/host.c | 10 ++++++---- include/linux/mmc/host.h | > > 2 ++ > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c index > > d4683b1d263f..c2de7cbc7838 100644 > > --- a/drivers/mmc/core/host.c > > +++ b/drivers/mmc/core/host.c > > @@ -214,14 +214,16 @@ static void mmc_retune_timer(struct timer_list > > *t) static void mmc_of_parse_timing_phase(struct device *dev, const char > *prop, > > struct mmc_clk_phase *phase) { > > - int degrees[2] = {0}; > > + int degrees[4] = {0}; > > int rc; > > > > - rc = device_property_read_u32_array(dev, prop, degrees, 2); > > + rc = device_property_read_u32_array(dev, prop, degrees, 4); > > phase->valid = !rc; > > if (phase->valid) { > > - phase->in_deg = degrees[0]; > > - phase->out_deg = degrees[1]; > > + phase->inv_in_deg = degrees[0] ? true : false; > > + phase->in_deg = degrees[1]; > > + phase->inv_out_deg = degrees[2] ? true : false; > > + phase->out_deg = degrees[3]; > > 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 Chin-Ting
On Sat, 6 Nov 2021, at 20:32, Chin-Ting Kuo wrote: > Hi Andrew, >>> > - rc = device_property_read_u32_array(dev, prop, degrees, 2); >> > + rc = device_property_read_u32_array(dev, prop, degrees, 4); >> > phase->valid = !rc; >> > if (phase->valid) { >> > - phase->in_deg = degrees[0]; >> > - phase->out_deg = degrees[1]; >> > + phase->inv_in_deg = degrees[0] ? true : false; >> > + phase->in_deg = degrees[1]; >> > + phase->inv_out_deg = degrees[2] ? true : false; >> > + phase->out_deg = degrees[3]; >> >> 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
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c index d4683b1d263f..c2de7cbc7838 100644 --- a/drivers/mmc/core/host.c +++ b/drivers/mmc/core/host.c @@ -214,14 +214,16 @@ static void mmc_retune_timer(struct timer_list *t) static void mmc_of_parse_timing_phase(struct device *dev, const char *prop, struct mmc_clk_phase *phase) { - int degrees[2] = {0}; + int degrees[4] = {0}; int rc; - rc = device_property_read_u32_array(dev, prop, degrees, 2); + rc = device_property_read_u32_array(dev, prop, degrees, 4); phase->valid = !rc; if (phase->valid) { - phase->in_deg = degrees[0]; - phase->out_deg = degrees[1]; + phase->inv_in_deg = degrees[0] ? true : false; + phase->in_deg = degrees[1]; + phase->inv_out_deg = degrees[2] ? true : false; + phase->out_deg = degrees[3]; } } diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index 0c0c9a0fdf57..3c13010683e0 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -82,7 +82,9 @@ struct mmc_ios { struct mmc_clk_phase { bool valid; + bool inv_in_deg; u16 in_deg; + bool inv_out_deg; u16 out_deg; };
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(-)