Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1613012611-8489-1-git-send-email-min.li.xe@renesas.com>
 <CAK8P3a3YhAGEfrvmi4YhhnG_3uWZuQi0ChS=0Cu9c4XCf5oGdw@mail.gmail.com>
 <OSBPR01MB47732017A97D5C911C4528F0BA8B9@OSBPR01MB4773.jpnprd01.prod.outlook.com>
 <CAK8P3a2KDO4HutsXNJzjmRJTvW1QW4Kt8H7U53_QqpmgvZtd3A@mail.gmail.com>
 <OSBPR01MB4773B22EA094A362DD807F83BA8B9@OSBPR01MB4773.jpnprd01.prod.outlook.com>
 <CAK8P3a3k5dAF=X3_NrYAAp5gPJ_uvF3XfmC4rKz0oGTrGRriCw@mail.gmail.com>
 <OSBPR01MB47732AFC03DA8A0DDF626706BA879@OSBPR01MB4773.jpnprd01.prod.outlook.com>
 <CAK8P3a2TeeLfsTNkZPnC3YowdOS=bFM5yYj58crP6F5U9Y_r-Q@mail.gmail.com>
 <OSBPR01MB47739CBDE12E1F3A19649772BA879@OSBPR01MB4773.jpnprd01.prod.outlook.com>
 <CAK8P3a2fRgDJZv-vzy_X6Y5t3daaVdCiXtMwkmXUyG0EQZ0a6Q@mail.gmail.com> <OSBPR01MB477394546AE3BC1F186FC0E9BA869@OSBPR01MB4773.jpnprd01.prod.outlook.com>
In-Reply-To: <OSBPR01MB477394546AE3BC1F186FC0E9BA869@OSBPR01MB4773.jpnprd01.prod.outlook.com>
From:   Arnd Bergmann <arnd@kernel.org>
Date:   Wed, 17 Feb 2021 22:30:14 +0100
Message-ID: <CAK8P3a32jF+iCH5Sk82LaozyPJ0n=f92MRdseZwN9aOtf4DwKQ@mail.gmail.com>
Subject: Re: [PATCH net-next] misc: Add Renesas Synchronization Management
 Unit (SMU) support
To:     Min Li <min.li.xe@renesas.com>
Cc:     Derek Kiernan <derek.kiernan@xilinx.com>,
        Dragan Cvetic <dragan.cvetic@xilinx.com>,
        Arnd Bergmann <arnd@arndb.de>,
        gregkh <gregkh@linuxfoundation.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Networking <netdev@vger.kernel.org>,
        Richard Cochran <richardcochran@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Feb 17, 2021 at 9:20 PM Min Li <min.li.xe@renesas.com> wrote:
>
> I attached the G.8273.2 document, where chapter 6 is about supporting physical layer
> frequency. And combo mode is Renesas way to support this requirement. Other companies
> may come up with different ways to support it.
>
> When EEC quality is below certain level, we would wanna turn off combo mode.

Maybe this is something that could be handled inside of the device driver then?

If the driver can use the same algorithm that is in your user space software
today, that would seem to be a nicer way to handle it than requiring a separate
application.

> > > This function will read FCW first and convert it to FFO.
> >
> > Is this related to the information in the timex->freq field? It sounds like this
> > would already be accessible through the existing
> > clock_adjtime() interface.
> >
>
> They are related, but dealing with timex->freq has limitations
>
> 1) Renesas SMU has up to 8 DPLLs and only one of the them would be ptp
> clock and we want to be able to read any DPLL's FFO or state

Is this necessarily unique to Renesas SMU though? Not sure what
makes sense in terms of the phc/ptp interface. Could there just be
a separate instance for each DPLL in the phc subsystem even if it's
actually a ptp clock, or would that be an incorrect use?

> 2) timex->freq's unit is ppb and we want to read more precise ffo in smaller unit of ppqt

This also sounds like something that would not be vendor specific. If you
need a higher resolution, then at some point others would need it as well.
There is already precedence in 'struct timex' to redefine the resolution of
some fields based on a flag -- 'time.tv_usec' can either refer to microseconds
to nanoseconds.

If the range of the 'freq' field is sufficient to encode ppqt, you could add
another flag for that, otherwise another reserved field can be used.

> 3) there is no interface in the current ptp hardware clock infrastructure to read ffo back from hardware

Adding an internal interface is the easy part here, the hard part is defining
the user interface.

> > Wouldn't any PTP clock run in one of these modes? If this is just
> > informational, it might be appropriate to have another sysfs attribute for
> > each PTP clock that shows the state of the DPLL, and then have the PTP
> > driver either fill in the current value in 'struct ptp_clock', or provide a
> > callback to report the state when a user reads the sysfs attribute.
> >
>
> What you propose can work. But DPLL operating mode is not standardized
> so different vendor may have different explanation for various modes.

If it's a string, that could easily be extended to further modes, as long
as the kernel documents which names are allowed. If multiple vendors
refer to the same mode by different names, someone will have to decide
what to call it in the kernel, and everyone afterwards would use the same
name.

> Also, I thought sysfs is only for debug or informational purpose.
> Production software is not supposed to use it for critical tasks?

No, you are probably thinking of debugfs. sysfs is one of multiple
common ways to exchange this kind of data in a reliable way.

An ioctl would probably work just as well though, usually sysfs
is better when the information makes sense to human operators
or simple shell scripts, while an ioctl interface is better if performance
is important, or if the information is primarily used in C programs.

        Arnd