All of lore.kernel.org
 help / color / mirror / Atom feed
From: Petr Machata <petrm@nvidia.com>
To: "Machnikowski, Maciej" <maciej.machnikowski@intel.com>
Cc: Petr Machata <petrm@nvidia.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"intel-wired-lan@lists.osuosl.org"
	<intel-wired-lan@lists.osuosl.org>,
	"richardcochran@gmail.com" <richardcochran@gmail.com>,
	"abyagowi@fb.com" <abyagowi@fb.com>,
	"Nguyen, Anthony L" <anthony.l.nguyen@intel.com>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"linux-kselftest@vger.kernel.org"
	<linux-kselftest@vger.kernel.org>,
	"idosch@idosch.org" <idosch@idosch.org>,
	"mkubecek@suse.cz" <mkubecek@suse.cz>,
	"saeed@kernel.org" <saeed@kernel.org>,
	"michael.chan@broadcom.com" <michael.chan@broadcom.com>
Subject: Re: [PATCH v3 net-next 6/6] docs: net: Add description of SyncE interfaces
Date: Tue, 16 Nov 2021 12:52:42 +0100	[thread overview]
Message-ID: <87fsrwtj05.fsf@nvidia.com> (raw)
In-Reply-To: <MW5PR11MB5812E733ED2BB621A3249CD5EA989@MW5PR11MB5812.namprd11.prod.outlook.com>


Machnikowski, Maciej <maciej.machnikowski@intel.com> writes:

>> - Reporting pins through the netdevices that use them allows for
>>   configurations that are likely invalid, like disjoint "frequency
>>   bridges".
>
> Not sure if I understand that comment. In target application a given
> netdev will receive an ESMC message containing the quality of the
> clock that it has on the receive side. The upper layer software will
> see QL_PRC on one port and QL_EEC on other and will need to enable
> clock output from the port that received QL_PRC, as it's the higher clock
> class. Once the EEC reports Locked state all other ports that are traceable
> to a given EEC (either using the DPLL subsystem, or the config file)
> will start reporting QL_PRC to downstream devices.

I think I had the reading of the UAPI wrong. So RTM_SETRCLKSTATE means,
take the clock recovered from ifindex, and send it to pins that I have
marked with the ENA flag.

But that still does not work well for multi-port devices. I can set it
up to forward frequency from swp1 to swp2 and swp3, from swp4 to swp5
and swp6, etc. But in reality I only have one underlying DPLL and can't
support this. So yeah, obviously, I bounce it in the driver. It also
means that when I want to switch tracking from swp1 to swp2, I first
need to unset all the swp1 pins (64 messages or whaveter) and then set
it up at swp2 (64 more messages). As a user I still don't know which of
my ports share DPLL. It's just not a great interface for multi-port
devices.

Having this stuff at a dedicated DPLL object would make the issue go
away completely. A driver then instantiates one DPLL, sets it up with
RCLK pins and TX pins. The DPLL can be configured with which pin to take
the frequency from, and which subset of pins to forward it to. There are
as many DPLL objects as there are DPLL circuits in the system.

This works for simple port devices as well as switches, as well as
non-networked devices.

The in-driver LOC overhead is a couple of _init / _fini calls and an ops
structure that the DPLL subsystem uses to talk to the driver. Everything
else remains the same.

>> - It's not clear what enabling several pins means, and it's not clear
>>   whether this genericity is not going to be an issue in the future when
>>   we know what enabling more pins means.
>
> It means that the recovered frequency will appear on 2 or more physical
> pins of the package.

Yes, agreed now.

WARNING: multiple messages have this Message-ID (diff)
From: Petr Machata <petrm@nvidia.com>
To: intel-wired-lan@osuosl.org
Subject: [Intel-wired-lan] [PATCH v3 net-next 6/6] docs: net: Add description of SyncE interfaces
Date: Tue, 16 Nov 2021 12:52:42 +0100	[thread overview]
Message-ID: <87fsrwtj05.fsf@nvidia.com> (raw)
In-Reply-To: <MW5PR11MB5812E733ED2BB621A3249CD5EA989@MW5PR11MB5812.namprd11.prod.outlook.com>


Machnikowski, Maciej <maciej.machnikowski@intel.com> writes:

>> - Reporting pins through the netdevices that use them allows for
>>   configurations that are likely invalid, like disjoint "frequency
>>   bridges".
>
> Not sure if I understand that comment. In target application a given
> netdev will receive an ESMC message containing the quality of the
> clock that it has on the receive side. The upper layer software will
> see QL_PRC on one port and QL_EEC on other and will need to enable
> clock output from the port that received QL_PRC, as it's the higher clock
> class. Once the EEC reports Locked state all other ports that are traceable
> to a given EEC (either using the DPLL subsystem, or the config file)
> will start reporting QL_PRC to downstream devices.

I think I had the reading of the UAPI wrong. So RTM_SETRCLKSTATE means,
take the clock recovered from ifindex, and send it to pins that I have
marked with the ENA flag.

But that still does not work well for multi-port devices. I can set it
up to forward frequency from swp1 to swp2 and swp3, from swp4 to swp5
and swp6, etc. But in reality I only have one underlying DPLL and can't
support this. So yeah, obviously, I bounce it in the driver. It also
means that when I want to switch tracking from swp1 to swp2, I first
need to unset all the swp1 pins (64 messages or whaveter) and then set
it up at swp2 (64 more messages). As a user I still don't know which of
my ports share DPLL. It's just not a great interface for multi-port
devices.

Having this stuff at a dedicated DPLL object would make the issue go
away completely. A driver then instantiates one DPLL, sets it up with
RCLK pins and TX pins. The DPLL can be configured with which pin to take
the frequency from, and which subset of pins to forward it to. There are
as many DPLL objects as there are DPLL circuits in the system.

This works for simple port devices as well as switches, as well as
non-networked devices.

The in-driver LOC overhead is a couple of _init / _fini calls and an ops
structure that the DPLL subsystem uses to talk to the driver. Everything
else remains the same.

>> - It's not clear what enabling several pins means, and it's not clear
>>   whether this genericity is not going to be an issue in the future when
>>   we know what enabling more pins means.
>
> It means that the recovered frequency will appear on 2 or more physical
> pins of the package.

Yes, agreed now.

  parent reply	other threads:[~2021-11-16 11:53 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-10 11:44 [PATCH v3 net-next 0/6] Add RTNL interface for SyncE Maciej Machnikowski
2021-11-10 11:44 ` [Intel-wired-lan] " Maciej Machnikowski
2021-11-10 11:44 ` [PATCH v3 net-next 1/6] ice: add support detecting features based on netlist Maciej Machnikowski
2021-11-10 11:44   ` [Intel-wired-lan] " Maciej Machnikowski
2021-11-10 11:44 ` [PATCH v3 net-next 2/6] rtnetlink: Add new RTM_GETEECSTATE message to get SyncE status Maciej Machnikowski
2021-11-10 11:44   ` [Intel-wired-lan] " Maciej Machnikowski
2021-11-11 16:01   ` Sabrina Dubroca
2021-11-11 16:01     ` [Intel-wired-lan] " Sabrina Dubroca
2021-11-11 16:22     ` Florian Westphal
2021-11-11 16:22       ` [Intel-wired-lan] " Florian Westphal
2021-11-16 14:40       ` Machnikowski, Maciej
2021-11-16 14:40         ` [Intel-wired-lan] " Machnikowski, Maciej
2021-11-16 15:41         ` Florian Westphal
2021-11-16 15:41           ` [Intel-wired-lan] " Florian Westphal
2021-11-16 19:30           ` Jakub Kicinski
2021-11-16 19:30             ` [Intel-wired-lan] " Jakub Kicinski
2021-11-16 14:37     ` Machnikowski, Maciej
2021-11-16 14:37       ` [Intel-wired-lan] " Machnikowski, Maciej
2021-11-10 11:44 ` [PATCH v3 net-next 3/6] ice: add support for reading SyncE DPLL state Maciej Machnikowski
2021-11-10 11:44   ` [Intel-wired-lan] " Maciej Machnikowski
2021-11-10 11:44 ` [PATCH v3 net-next 4/6] rtnetlink: Add support for SyncE recovered clock configuration Maciej Machnikowski
2021-11-10 11:44   ` [Intel-wired-lan] " Maciej Machnikowski
2021-11-10 11:44 ` [PATCH v3 net-next 5/6] ice: add support for SyncE recovered clocks Maciej Machnikowski
2021-11-10 11:44   ` [Intel-wired-lan] " Maciej Machnikowski
2021-11-10 11:44 ` [PATCH v3 net-next 6/6] docs: net: Add description of SyncE interfaces Maciej Machnikowski
2021-11-10 11:44   ` [Intel-wired-lan] " Maciej Machnikowski
2021-11-11 12:43   ` Petr Machata
2021-11-11 12:43     ` [Intel-wired-lan] " Petr Machata
2021-11-15 10:24     ` Machnikowski, Maciej
2021-11-15 10:24       ` [Intel-wired-lan] " Machnikowski, Maciej
2021-11-16 11:07       ` Petr Machata
2021-11-16 11:07         ` [Intel-wired-lan] " Petr Machata
2021-11-16 11:52       ` Petr Machata [this message]
2021-11-16 11:52         ` Petr Machata
2021-11-16 14:26         ` Machnikowski, Maciej
2021-11-16 14:26           ` [Intel-wired-lan] " Machnikowski, Maciej

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87fsrwtj05.fsf@nvidia.com \
    --to=petrm@nvidia.com \
    --cc=abyagowi@fb.com \
    --cc=anthony.l.nguyen@intel.com \
    --cc=davem@davemloft.net \
    --cc=idosch@idosch.org \
    --cc=intel-wired-lan@lists.osuosl.org \
    --cc=kuba@kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=maciej.machnikowski@intel.com \
    --cc=michael.chan@broadcom.com \
    --cc=mkubecek@suse.cz \
    --cc=netdev@vger.kernel.org \
    --cc=richardcochran@gmail.com \
    --cc=saeed@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.