Re: [PATCH V2 1/3] scsi: ufs: set the device reference clock setting

[Rob Herring]

On Thu, Jun 21, 2018 at 2:52 AM sayali <sayalil@xxxxxxxxxxxxxx> wrote:
>
> Hi Rob,
>
> Please check my comment inline.

As mentioned in the back and forth comments previously in this thread,
please fix your email client (hint: you can't use Outlook) and
properly quote your replies (i.e. the leading ">")

> Thanks,
> Sayali
> -----Original Message-----
> From: Rob Herring [mailto:robh@xxxxxxxxxx]
> Sent: Thursday, June 14, 2018 7:59 PM
> To: sayali <sayalil@xxxxxxxxxxxxxx>
> Cc: Subhash Jadavani <subhashj@xxxxxxxxxxxxxx>; Can Guo <cang@xxxxxxxxxxxxxx>; Vivek Gautam <vivek.gautam@xxxxxxxxxxxxxx>; Rajendra Nayak <rnayak@xxxxxxxxxxxxxx>; Vinayak Holikatti <vinholikatti@xxxxxxxxx>; James E.J. Bottomley <jejb@xxxxxxxxxxxxxxxxxx>; Martin K. Petersen <martin.petersen@xxxxxxxxxx>; asutoshd@xxxxxxxxxxxxxx; Evan Green <evgreen@xxxxxxxxxxxx>; linux-scsi@xxxxxxxxxxxxxxx; Mark Rutland <mark.rutland@xxxxxxx>; Mathieu Malaterre <malat@xxxxxxxxxx>; open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@xxxxxxxxxxxxxxx>; open list <linux-kernel@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH V2 1/3] scsi: ufs: set the device reference clock setting
>
> On Thu, Jun 14, 2018 at 5:33 AM, sayali <sayalil@xxxxxxxxxxxxxx> wrote:
> > Comment inline.
> >
> > Thanks,
> > Sayali
> > -----Original Message-----
> > From: Rob Herring [mailto:robh@xxxxxxxxxx]
> > Sent: Wednesday, June 13, 2018 12:57 AM
> > To: Sayali Lokhande <sayalil@xxxxxxxxxxxxxx>
> > Cc: subhashj@xxxxxxxxxxxxxx; cang@xxxxxxxxxxxxxx;
> > vivek.gautam@xxxxxxxxxxxxxx; rnayak@xxxxxxxxxxxxxx;
> > vinholikatti@xxxxxxxxx; jejb@xxxxxxxxxxxxxxxxxx;
> > martin.petersen@xxxxxxxxxx; asutoshd@xxxxxxxxxxxxxx;
> > evgreen@xxxxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx; Mark Rutland
> > <mark.rutland@xxxxxxx>; Mathieu Malaterre <malat@xxxxxxxxxx>; open
> > list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
> > <devicetree@xxxxxxxxxxxxxxx>; open list <linux-kernel@xxxxxxxxxxxxxxx>
> > Subject: Re: [PATCH V2 1/3] scsi: ufs: set the device reference clock
> > setting
> >
> > On Fri, Jun 08, 2018 at 04:36:28PM +0530, Sayali Lokhande wrote:
> >> From: Subhash Jadavani <subhashj@xxxxxxxxxxxxxx>
> >>
> >> UFS host supplies the reference clock to UFS device and UFS device
> >> specification allows host to provide one of the 4 frequencies (19.2
> >> MHz,
> >> 26 MHz, 38.4 MHz, 52 MHz) for reference clock. Host should set the
> >> device reference clock frequency setting in the device based on what
> >> frequency it is supplying to UFS device.
> >>
> >> Signed-off-by: Subhash Jadavani <subhashj@xxxxxxxxxxxxxx>
> >> Signed-off-by: Can Guo <cang@xxxxxxxxxxxxxx>
> >> Signed-off-by: Sayali Lokhande <sayalil@xxxxxxxxxxxxxx>
> >> ---
> >>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt      |  7 +++
> >>  drivers/scsi/ufs/ufs.h                             |  9 ++++
> >>  drivers/scsi/ufs/ufshcd-pltfrm.c                   | 24 ++++++++++
> >>  drivers/scsi/ufs/ufshcd.c                          | 52
> > ++++++++++++++++++++++
> >>  drivers/scsi/ufs/ufshcd.h                          |  1 +
> >>  5 files changed, 93 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> >> b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> >> index c39dfef..4522434 100644
> >> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> >> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
> >> @@ -41,6 +41,12 @@ Optional properties:
> >>  -lanes-per-direction : number of lanes available per direction -
> >> either 1
> > or 2.
> >>                         Note that it is assume same number of lanes
> >> is
> > used both
> >>                         directions at once. If not specified, default
> >> is 2
> > lanes per direction.
> >> +- dev-ref-clk-freq   : Specify the device reference clock frequency, must
> > be one of the following:
> >> +                       0: 19.2 MHz
> >> +                       1: 26 MHz
> >> +                       2: 38.4 MHz
> >> +                       3: 52 MHz
> >> +                       Defaults to 26 MHz if not specified.
> >
> > I must have misunderstood your last response. I thought you could
> > handle things without DT. If not, my question remains.
> > [Sayali]: Ref clk frequency setting could vary from
> > platfrom-to-platform(vendor specific). Hence we need to pass it via DT.
> >         Currently in DT we do not set/mention any ref clk frequency
> > parameter. Hence I have added one new DT entry to configure
> >         required ref clk freq.
>
> The clocks property contains "ref_clk". Is that not the same clock?
> Why can't you read what that frequency is? Or you need to be able to change it to a specific frequency? These all look like typical oscillator frequencies, so I wouldn't expect you could change them (other than divide by 2 maybe).
> [Sayali] : It is the same "ref_clk", but we need to be able to change it to a specific frequency as per requirement. Thus, we need new DT entry to specify/override reference clock frequency as per need.

That is not what your patch does. It just tells the device what the
frequency is. If you need to get the rate, use "clk_get_rate" on
"ref_clk". If you need to actually set it to a specific frequency,
then we have properties for that already (assigned-clock-rates).

Seems to me that by the time you get to Linux, the bootloader would
have already set this. Otherwise, how do you boot? Seems like you
would want to read the attr and ensure "ref_clk" freq matches.

Rob