| Message ID | 20221129213724.10119-1-vfedorenko@novek.ru (mailing list archive) |
|---|---|
| Headers | show |
| Series | Create common DPLL/clock configuration API | expand |
Tue, Nov 29, 2022 at 10:37:20PM CET, vfedorenko@novek.ru wrote: >Implement common API for clock/DPLL configuration and status reporting. >The API utilises netlink interface as transport for commands and event >notifications. This API aim to extend current pin configuration and >make it flexible and easy to cover special configurations. Overall, I see a lot of issues on multiple levels. I will go over them in follow-up emails. So far, after couple of hours looking trought this, I have following general feelings/notes: 1) Netlink interface looks much saner than in previous versions. I will send couple of notes, mainly around events and object mixtures and couple of bugs/redundancies. But overall, looks fineish. 2) I don't like that concept of a shared pin, at all. It makes things unnecessary complicated. Just have a pin created for dpll instance and that's it. If another instance has the same pin, it should create it as well. Keeps things separate and easy to model. Let the hw/fw/driver figure out the implementation oddities. Why exactly you keep pushing the shared pin idea? Perhaps I'm missing something crucial. 3) I don't like the concept of muxed pins and hierarchies of pins. Why does user care? If pin is muxed, the rest of the pins related to this one should be in state disabled/disconnected. The user only cares about to see which pins are related to each other. It can be easily exposed by "muxid" like this: pin 1 pin 2 pin 3 muxid 100 pin 4 muxid 100 pin 5 muxid 101 pin 6 muxid 101 In this example pins 3,4 and 5,6 are muxed, therefore the user knows if he connects one, the other one gets disconnected (or will have to disconnect the first one explicitly first). 4) I don't like the "attr" indirection. It makes things very tangled. It comes from the concepts of classes and objects and takes it to extreme. Not really something we are commonly used to in kernel. Also, it brings no value from what I can see, only makes things very hard to read and follow. Please keep things direct and simple: * If some option could be changed for a pin or dpll, just have an op that is directly called from netlink handler to change it. There should be clear set of ops for configuration of pin and dpll object. This "attr" indirection make this totally invisible. * If some attribute is const during dpll or pin lifetime, have it passed during dpll or pin creation. > >v3 -> v4: > * redesign framework to make pins dynamically allocated (Arkadiusz) > * implement shared pins (Arkadiusz) >v2 -> v3: > * implement source select mode (Arkadiusz) > * add documentation > * implementation improvements (Jakub) >v1 -> v2: > * implement returning supported input/output types > * ptp_ocp: follow suggestions from Jonathan > * add linux-clk mailing list >v0 -> v1: > * fix code style and errors > * add linux-arm mailing list > > >Arkadiusz Kubalewski (1): > dpll: add dpll_attr/dpll_pin_attr helper classes > >Vadim Fedorenko (3): > dpll: Add DPLL framework base functions > dpll: documentation on DPLL subsystem interface > ptp_ocp: implement DPLL ops > > Documentation/networking/dpll.rst | 271 ++++++++ > Documentation/networking/index.rst | 1 + > MAINTAINERS | 8 + > drivers/Kconfig | 2 + > drivers/Makefile | 1 + > drivers/dpll/Kconfig | 7 + > drivers/dpll/Makefile | 11 + > drivers/dpll/dpll_attr.c | 278 +++++++++ > drivers/dpll/dpll_core.c | 760 +++++++++++++++++++++++ > drivers/dpll/dpll_core.h | 176 ++++++ > drivers/dpll/dpll_netlink.c | 963 +++++++++++++++++++++++++++++ > drivers/dpll/dpll_netlink.h | 24 + > drivers/dpll/dpll_pin_attr.c | 456 ++++++++++++++ > drivers/ptp/Kconfig | 1 + > drivers/ptp/ptp_ocp.c | 123 ++-- > include/linux/dpll.h | 261 ++++++++ > include/linux/dpll_attr.h | 433 +++++++++++++ > include/uapi/linux/dpll.h | 263 ++++++++ > 18 files changed, 4002 insertions(+), 37 deletions(-) > create mode 100644 Documentation/networking/dpll.rst > create mode 100644 drivers/dpll/Kconfig > create mode 100644 drivers/dpll/Makefile > create mode 100644 drivers/dpll/dpll_attr.c > create mode 100644 drivers/dpll/dpll_core.c > create mode 100644 drivers/dpll/dpll_core.h > create mode 100644 drivers/dpll/dpll_netlink.c > create mode 100644 drivers/dpll/dpll_netlink.h > create mode 100644 drivers/dpll/dpll_pin_attr.c > create mode 100644 include/linux/dpll.h > create mode 100644 include/linux/dpll_attr.h > create mode 100644 include/uapi/linux/dpll.h > >-- >2.27.0 >
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Wednesday, November 30, 2022 1:32 PM > >Tue, Nov 29, 2022 at 10:37:20PM CET, vfedorenko@novek.ru wrote: >>Implement common API for clock/DPLL configuration and status reporting. >>The API utilises netlink interface as transport for commands and event >>notifications. This API aim to extend current pin configuration and >>make it flexible and easy to cover special configurations. > >Overall, I see a lot of issues on multiple levels. I will go over them in >follow-up emails. So far, after couple of hours looking trought this, I >have following general feelings/notes: Hi Jiri, As we have been participating in last version, feel obligated to answer to the concerns. > >1) Netlink interface looks much saner than in previous versions. I will > send couple of notes, mainly around events and object mixtures and > couple of bugs/redundancies. But overall, looks fineish. > >2) I don't like that concept of a shared pin, at all. It makes things > unnecessary complicated. Just have a pin created for dpll instance > and that's it. If another instance has the same pin, it should create > it as well. Keeps things separate and easy to model. Let the > hw/fw/driver figure out the implementation oddities. > Why exactly you keep pushing the shared pin idea? Perhaps I'm missing > something crucial. If the user request change on pin#0 of dpll#0, the dpll#0 knows about the change, it reacts accordingly, and notifies the user the something has changed. Which is rather simple. Now, if the dpll#1 is using the same pin (pin#0 of dpll#0), the complicated part starts. First we have to assume: - it was initialized with the same description (as it should, to prevent confusing the user) - it was initialized with the same order (this is at least nice to have from user POV, as pin indices are auto generated), and also in case of multiple pins being shared it would be best for the user to have exactly the same number of pins initialized, so they have same indices and initialization order doesn't introduce additional confusion. Thus, one reason of shared pins was to prevent having this assumptions ever. If the pin is shared, all dplls sharing a pin would have the same description and pin index for a shared pin out of the box. Pin attribute changes The change on dpll#0 pin#0 impacts also dpll#1 pin#0. Notification about the change shall be also requested from the driver that handles dpll#1. In such case the driver has to have some dpll monitoring/notifying mechanics, which at first doesn't look very hard to do, but most likely only if both dplls are initialized and managed by a single instance of a driver/firmware. If board has 2 dplls but each one is managed by its own firmware/driver instance. User changes frequency of pin#0 signal, the driver of dpll#0 must also notify driver of dpll#1 that pin#0 frequency has changed, dpll#1 reacts on the change, notifies the user. But this is only doable with assumption, that the board is internally capable of such internal board level communication, which in case of separated firmwares handling multiple dplls might not be the case, or it would require to have some other sw component feel that gap. For complex boards with multiple dplls/sync channels, multiple ports, multiple firmware instances, it seems to be complicated to share a pin if each driver would have own copy and should notify all the other about changes. To summarize, that is certainly true, shared pins idea complicates stuff inside of dpll subsystem. But at the same time it removes complexity from all the drivers which would use it and is easier for the userspace due to common identification of pins. This solution scales up without any additional complexity in the driver, and without any need for internal per-board communication channels. Not sure if this is good or bad, but with current version, both approaches are possible, so it pretty much depending on the driver to initialize dplls with separated pin objects as you have suggested (and take its complexity into driver) or just share them. > >3) I don't like the concept of muxed pins and hierarchies of pins. Why > does user care? If pin is muxed, the rest of the pins related to this > one should be in state disabled/disconnected. The user only cares > about to see which pins are related to each other. It can be easily > exposed by "muxid" like this: > pin 1 > pin 2 > pin 3 muxid 100 > pin 4 muxid 100 > pin 5 muxid 101 > pin 6 muxid 101 > In this example pins 3,4 and 5,6 are muxed, therefore the user knows > if he connects one, the other one gets disconnected (or will have to > disconnect the first one explicitly first). > Currently DPLLA_PIN_PARENT_IDX is doing the same thing as you described, it groups MUXed pins, the parent pin index here was most straightforward to me, as in case of DPLL_MODE_AUTOMATIC, where dpll auto-selects highest priority available signal. The priority can be only assigned to the pins directly connected to the dpll. The rest of pins (which would have present attribute DPLLA_PIN_PARENT_IDX) are the ones that require manual selection even if DPLL_MODE_AUTOMATIC is enabled. Enabling a particular pin and sub-pin in DPLL_MODE_AUTOMATIC requires from user to select proper priority on on a dpll-level MUX-pin and manually select one of the sub-pins. On the other hand for DPLL_MODE_FORCED, this might be also beneficial, as the user could select a directly connected pin and muxed pin with two separated commands, which could be handled in separated driver instances (if HW design requires such approach) or either it can be handled just by one select call for the pin connected directly and handled entirely in the one driver instance. >4) I don't like the "attr" indirection. It makes things very tangled. It > comes from the concepts of classes and objects and takes it to > extreme. Not really something we are commonly used to in kernel. > Also, it brings no value from what I can see, only makes things very > hard to read and follow. > Yet again, true, I haven't find anything similar in the kernel, it was more like a try to find out a way to have a single structure with all the stuff that is passed between netlink/core/driver parts. Came up with this, and to be honest it suits pretty well, those are well defined containers. They store attributes that either user or driver have set, with ability to obtain a valid value only if it was set. Thus whoever reads a struct, knows which of those attributes were actually set. As you said, seems a bit revolutionary, but IMHO it simplifies stuff, and basically it is value and validity bit, which I believe is rather common in the kernel, this differs only with the fact it is encapsulated. No direct access to the fields of structure is available for the users. Most probably there are some things that could be improved with it, but in general it is very easy to use and understand how it works. What could be improved: - naming scheme as function names are a bit long right now, although mostly still fits the line-char limits, thus not that problematic - bit mask values are capable of storing 32 bits and bit(0) is always used as unspec, which ends up with 31 values available for the enums so if by any chance one of the attribute enums would go over 32 it could be an issue. It is especially useful for multiple values passed with the same netlink attribute id. I.e. please take a look at dpll_msg_add_pin_types_supported(..) function. > Please keep things direct and simple: > * If some option could be changed for a pin or dpll, just have an > op that is directly called from netlink handler to change it. > There should be clear set of ops for configuration of pin and > dpll object. This "attr" indirection make this totally invisible. In last review you have asked to have rather only set and get ops defined with a single attribute struct. This is exactly that, altough encapsulated. > * If some attribute is const during dpll or pin lifetime, have it > passed during dpll or pin creation. > > Only driver knows which attributes are const and which are not, this shall be also part of driver implementation. As I understand all the fields present in (dpll/dpll_pin)_attr, used in get/set ops, could be altered in run-time depending on HW design. Thanks, Arkadiusz > >> >>v3 -> v4: >> * redesign framework to make pins dynamically allocated (Arkadiusz) >> * implement shared pins (Arkadiusz) >>v2 -> v3: >> * implement source select mode (Arkadiusz) >> * add documentation >> * implementation improvements (Jakub) >>v1 -> v2: >> * implement returning supported input/output types >> * ptp_ocp: follow suggestions from Jonathan >> * add linux-clk mailing list >>v0 -> v1: >> * fix code style and errors >> * add linux-arm mailing list >> >> >>Arkadiusz Kubalewski (1): >> dpll: add dpll_attr/dpll_pin_attr helper classes >> >>Vadim Fedorenko (3): >> dpll: Add DPLL framework base functions >> dpll: documentation on DPLL subsystem interface >> ptp_ocp: implement DPLL ops >> >> Documentation/networking/dpll.rst | 271 ++++++++ >> Documentation/networking/index.rst | 1 + >> MAINTAINERS | 8 + >> drivers/Kconfig | 2 + >> drivers/Makefile | 1 + >> drivers/dpll/Kconfig | 7 + >> drivers/dpll/Makefile | 11 + >> drivers/dpll/dpll_attr.c | 278 +++++++++ >> drivers/dpll/dpll_core.c | 760 +++++++++++++++++++++++ >> drivers/dpll/dpll_core.h | 176 ++++++ >> drivers/dpll/dpll_netlink.c | 963 +++++++++++++++++++++++++++++ >> drivers/dpll/dpll_netlink.h | 24 + >> drivers/dpll/dpll_pin_attr.c | 456 ++++++++++++++ >> drivers/ptp/Kconfig | 1 + >> drivers/ptp/ptp_ocp.c | 123 ++-- >> include/linux/dpll.h | 261 ++++++++ >> include/linux/dpll_attr.h | 433 +++++++++++++ >> include/uapi/linux/dpll.h | 263 ++++++++ >> 18 files changed, 4002 insertions(+), 37 deletions(-) create mode >> 100644 Documentation/networking/dpll.rst create mode 100644 >> drivers/dpll/Kconfig create mode 100644 drivers/dpll/Makefile create >> mode 100644 drivers/dpll/dpll_attr.c create mode 100644 >> drivers/dpll/dpll_core.c create mode 100644 drivers/dpll/dpll_core.h >> create mode 100644 drivers/dpll/dpll_netlink.c create mode 100644 >> drivers/dpll/dpll_netlink.h create mode 100644 >> drivers/dpll/dpll_pin_attr.c create mode 100644 include/linux/dpll.h >> create mode 100644 include/linux/dpll_attr.h create mode 100644 >> include/uapi/linux/dpll.h >> >>-- >>2.27.0 >>
Fri, Dec 02, 2022 at 12:27:24PM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Wednesday, November 30, 2022 1:32 PM >> >>Tue, Nov 29, 2022 at 10:37:20PM CET, vfedorenko@novek.ru wrote: >>>Implement common API for clock/DPLL configuration and status reporting. >>>The API utilises netlink interface as transport for commands and event >>>notifications. This API aim to extend current pin configuration and >>>make it flexible and easy to cover special configurations. >> >>Overall, I see a lot of issues on multiple levels. I will go over them in >>follow-up emails. So far, after couple of hours looking trought this, I >>have following general feelings/notes: > >Hi Jiri, > >As we have been participating in last version, feel obligated to answer to >the concerns. Cool. > >> >>1) Netlink interface looks much saner than in previous versions. I will >> send couple of notes, mainly around events and object mixtures and >> couple of bugs/redundancies. But overall, looks fineish. >> >>2) I don't like that concept of a shared pin, at all. It makes things >> unnecessary complicated. Just have a pin created for dpll instance >> and that's it. If another instance has the same pin, it should create >> it as well. Keeps things separate and easy to model. Let the >> hw/fw/driver figure out the implementation oddities. >> Why exactly you keep pushing the shared pin idea? Perhaps I'm missing >> something crucial. > > >If the user request change on pin#0 of dpll#0, the dpll#0 knows about the >change, it reacts accordingly, and notifies the user the something has changed. >Which is rather simple. > >Now, if the dpll#1 is using the same pin (pin#0 of dpll#0), the complicated >part starts. First we have to assume: >- it was initialized with the same description (as it should, to prevent >confusing the user) >- it was initialized with the same order (this is at least nice to have from >user POV, as pin indices are auto generated), and also in case of multiple pins >being shared it would be best for the user to have exactly the same number of >pins initialized, so they have same indices and initialization order doesn't >introduce additional confusion. > >Thus, one reason of shared pins was to prevent having this assumptions ever. >If the pin is shared, all dplls sharing a pin would have the same description >and pin index for a shared pin out of the box. > >Pin attribute changes >The change on dpll#0 pin#0 impacts also dpll#1 pin#0. Notification about the >change shall be also requested from the driver that handles dpll#1. In such >case the driver has to have some dpll monitoring/notifying mechanics, which at >first doesn't look very hard to do, but most likely only if both dplls are >initialized and managed by a single instance of a driver/firmware. > >If board has 2 dplls but each one is managed by its own firmware/driver >instance. User changes frequency of pin#0 signal, the driver of dpll#0 must >also notify driver of dpll#1 that pin#0 frequency has changed, dpll#1 reacts on >the change, notifies the user. Right. >But this is only doable with assumption, that the board is internally capable >of such internal board level communication, which in case of separated >firmwares handling multiple dplls might not be the case, or it would require >to have some other sw component feel that gap. Yep, you have the knowledge of sharing inside the driver, so you should do it there. For multiple instances, use in-driver notifier for example. > >For complex boards with multiple dplls/sync channels, multiple ports, >multiple firmware instances, it seems to be complicated to share a pin if >each driver would have own copy and should notify all the other about changes. > >To summarize, that is certainly true, shared pins idea complicates stuff >inside of dpll subsystem. >But at the same time it removes complexity from all the drivers which would use There are currently 3 drivers for dpll I know of. This in ptp_ocp and mlx5 there is no concept of sharing pins. You you are talking about a single driver. What I'm trying to say is, looking at the code, the pin sharing, references and locking makes things uncomfortably complex. You are so far the only driver to need this, do it internally. If in the future other driver appears, this code would be eventually pushed into dpll core. No impact on UAPI from what I see. Please keep things as simple as possible. >it and is easier for the userspace due to common identification of pins. By identification, you mean "description" right? I see no problem of 2 instances have the same pin "description"/label. >This solution scales up without any additional complexity in the driver, >and without any need for internal per-board communication channels. > >Not sure if this is good or bad, but with current version, both approaches are >possible, so it pretty much depending on the driver to initialize dplls with >separated pin objects as you have suggested (and take its complexity into >driver) or just share them. > >> >>3) I don't like the concept of muxed pins and hierarchies of pins. Why >> does user care? If pin is muxed, the rest of the pins related to this >> one should be in state disabled/disconnected. The user only cares >> about to see which pins are related to each other. It can be easily >> exposed by "muxid" like this: >> pin 1 >> pin 2 >> pin 3 muxid 100 >> pin 4 muxid 100 >> pin 5 muxid 101 >> pin 6 muxid 101 >> In this example pins 3,4 and 5,6 are muxed, therefore the user knows >> if he connects one, the other one gets disconnected (or will have to >> disconnect the first one explicitly first). >> > >Currently DPLLA_PIN_PARENT_IDX is doing the same thing as you described, it >groups MUXed pins, the parent pin index here was most straightforward to me, There is a big difference if we model flat list of pins with a set of attributes for each, comparing to a tree of pins, some acting as leaf, node and root. Do we really need such complexicity? What value does it bring to the user to expose this? >as in case of DPLL_MODE_AUTOMATIC, where dpll auto-selects highest priority >available signal. The priority can be only assigned to the pins directly >connected to the dpll. The rest of pins (which would have present >attribute DPLLA_PIN_PARENT_IDX) are the ones that require manual selection >even if DPLL_MODE_AUTOMATIC is enabled. > >Enabling a particular pin and sub-pin in DPLL_MODE_AUTOMATIC requires from user >to select proper priority on on a dpll-level MUX-pin and manually select one of >the sub-pins. >On the other hand for DPLL_MODE_FORCED, this might be also beneficial, as the >user could select a directly connected pin and muxed pin with two separated >commands, which could be handled in separated driver instances (if HW design >requires such approach) or either it can be handled just by one select call >for the pin connected directly and handled entirely in the one driver instance. Talk netlink please. What are the actual commands with cmds used to select the source and select a mux pin? You are talking about 2 types of selections: 1) Source select - this is as you described either auto/forces (manual) mode, according to some prio, dpll select the best source 2) Pin select in a mux - here the pin could be source or output But again, from the user perspective, why does he have to distinguish these? Extending my example: pin 1 source pin 2 output pin 3 muxid 100 source pin 4 muxid 100 source pin 5 muxid 101 source pin 6 muxid 101 source pin 7 output User now can set individial prios for sources: dpll x pin 1 set prio 10 etc The result would be: pin 1 source prio 10 pin 2 output pin 3 muxid 100 source prio 8 pin 4 muxid 100 source prio 20 pin 5 muxid 101 source prio 50 pin 6 muxid 101 source prio 60 pin 7 output Now when auto is enabled, the pin 3 is selected. Why would user need to manually select between 3 and 4? This is should be abstracted out by the driver. Only issues I see when pins are output. User would have to somehow select one of the pins in the mux (perhaps another dpll cmd). But the mux pin instance does not help with anything there... > >>4) I don't like the "attr" indirection. It makes things very tangled. It >> comes from the concepts of classes and objects and takes it to >> extreme. Not really something we are commonly used to in kernel. >> Also, it brings no value from what I can see, only makes things very >> hard to read and follow. >> > >Yet again, true, I haven't find anything similar in the kernel, it was more >like a try to find out a way to have a single structure with all the stuff that >is passed between netlink/core/driver parts. Came up with this, and to be >honest it suits pretty well, those are well defined containers. They store >attributes that either user or driver have set, with ability to obtain a valid >value only if it was set. Thus whoever reads a struct, knows which of those >attributes were actually set. Sorry for being blunt here, but when I saw the code I remembered my days as a student where they forced us to do similar things Java :) There you tend to make things contained, everything is a class, getters, setters and whatnot. In kernel, this is overkill. I'm not saying it's functionally wrong. What I say is that it is completely unnecessary. I see no advantage, by having this indirection. I see only disadvantages. It makes code harder to read and follow. What I suggest, again, is to make things nice and simple. Set of ops that the driver implements for dpll commands or parts of commands, as we are used to in the rest of the kernel. >As you said, seems a bit revolutionary, but IMHO it simplifies stuff, and >basically it is value and validity bit, which I believe is rather common in the >kernel, this differs only with the fact it is encapsulated. No direct access to >the fields of structure is available for the users. I don't see any reason for any validity bits whan you just do it using driver-implemented ops. >Most probably there are some things that could be improved with it, but in >general it is very easy to use and understand how it works. >What could be improved: >- naming scheme as function names are a bit long right now, although mostly >still fits the line-char limits, thus not that problematic >- bit mask values are capable of storing 32 bits and bit(0) is always used as >unspec, which ends up with 31 values available for the enums so if by any >chance one of the attribute enums would go over 32 it could be an issue. > >It is especially useful for multiple values passed with the same netlink >attribute id. I.e. please take a look at dpll_msg_add_pin_types_supported(..) >function. > >> Please keep things direct and simple: >> * If some option could be changed for a pin or dpll, just have an >> op that is directly called from netlink handler to change it. >> There should be clear set of ops for configuration of pin and >> dpll object. This "attr" indirection make this totally invisible. > >In last review you have asked to have rather only set and get ops defined >with a single attribute struct. This is exactly that, altough encapsulated. For objects, yes. Pass a struct you directly read/write if the amount of function args would be bigger then say 4. The whole encapsulation is not good for anything. > >> * If some attribute is const during dpll or pin lifetime, have it >> passed during dpll or pin creation. >> >> > >Only driver knows which attributes are const and which are not, this shall Nonono. This is semantics defined by the subsystem. The pin label/description for example. That is const, cannot be set by the user. The type of the pin (synce/gnss/ext) is const, cannot be set by the user. This you have to clearly specify when you define driver API. This const attrs should be passed during pin creation/registration. Talking about dpll instance itself, the clock_id, clock_quality, these should be also const attrs. >be also part of driver implementation. >As I understand all the fields present in (dpll/dpll_pin)_attr, used in get/set >ops, could be altered in run-time depending on HW design. > >Thanks, >Arkadiusz > >> >>> >>>v3 -> v4: >>> * redesign framework to make pins dynamically allocated (Arkadiusz) >>> * implement shared pins (Arkadiusz) >>>v2 -> v3: >>> * implement source select mode (Arkadiusz) >>> * add documentation >>> * implementation improvements (Jakub) >>>v1 -> v2: >>> * implement returning supported input/output types >>> * ptp_ocp: follow suggestions from Jonathan >>> * add linux-clk mailing list >>>v0 -> v1: >>> * fix code style and errors >>> * add linux-arm mailing list >>> >>> >>>Arkadiusz Kubalewski (1): >>> dpll: add dpll_attr/dpll_pin_attr helper classes >>> >>>Vadim Fedorenko (3): >>> dpll: Add DPLL framework base functions >>> dpll: documentation on DPLL subsystem interface >>> ptp_ocp: implement DPLL ops >>> >>> Documentation/networking/dpll.rst | 271 ++++++++ >>> Documentation/networking/index.rst | 1 + >>> MAINTAINERS | 8 + >>> drivers/Kconfig | 2 + >>> drivers/Makefile | 1 + >>> drivers/dpll/Kconfig | 7 + >>> drivers/dpll/Makefile | 11 + >>> drivers/dpll/dpll_attr.c | 278 +++++++++ >>> drivers/dpll/dpll_core.c | 760 +++++++++++++++++++++++ >>> drivers/dpll/dpll_core.h | 176 ++++++ >>> drivers/dpll/dpll_netlink.c | 963 +++++++++++++++++++++++++++++ >>> drivers/dpll/dpll_netlink.h | 24 + >>> drivers/dpll/dpll_pin_attr.c | 456 ++++++++++++++ >>> drivers/ptp/Kconfig | 1 + >>> drivers/ptp/ptp_ocp.c | 123 ++-- >>> include/linux/dpll.h | 261 ++++++++ >>> include/linux/dpll_attr.h | 433 +++++++++++++ >>> include/uapi/linux/dpll.h | 263 ++++++++ >>> 18 files changed, 4002 insertions(+), 37 deletions(-) create mode >>> 100644 Documentation/networking/dpll.rst create mode 100644 >>> drivers/dpll/Kconfig create mode 100644 drivers/dpll/Makefile create >>> mode 100644 drivers/dpll/dpll_attr.c create mode 100644 >>> drivers/dpll/dpll_core.c create mode 100644 drivers/dpll/dpll_core.h >>> create mode 100644 drivers/dpll/dpll_netlink.c create mode 100644 >>> drivers/dpll/dpll_netlink.h create mode 100644 >>> drivers/dpll/dpll_pin_attr.c create mode 100644 include/linux/dpll.h >>> create mode 100644 include/linux/dpll_attr.h create mode 100644 >>> include/uapi/linux/dpll.h >>> >>>-- >>>2.27.0 >>>
On Fri, 2 Dec 2022 17:12:06 +0100 Jiri Pirko wrote: > >But this is only doable with assumption, that the board is internally capable > >of such internal board level communication, which in case of separated > >firmwares handling multiple dplls might not be the case, or it would require > >to have some other sw component feel that gap. > > Yep, you have the knowledge of sharing inside the driver, so you should > do it there. For multiple instances, use in-driver notifier for example. No, complexity in the drivers is not a good idea. The core should cover the complexity and let the drivers be simple. > >For complex boards with multiple dplls/sync channels, multiple ports, > >multiple firmware instances, it seems to be complicated to share a pin if > >each driver would have own copy and should notify all the other about changes. > > > >To summarize, that is certainly true, shared pins idea complicates stuff > >inside of dpll subsystem. > >But at the same time it removes complexity from all the drivers which would use > > There are currently 3 drivers for dpll I know of. This in ptp_ocp and > mlx5 there is no concept of sharing pins. You you are talking about a > single driver. > > What I'm trying to say is, looking at the code, the pin sharing, > references and locking makes things uncomfortably complex. You are so > far the only driver to need this, do it internally. If in the future > other driver appears, this code would be eventually pushed into dpll > core. No impact on UAPI from what I see. Please keep things as simple as > possible. But the pin is shared for one driver. Who cares if it's not shared in another. The user space must be able to reason about the constraints. You are suggesting drivers to magically flip state in core objects because of some hidden dependencies?! > >it and is easier for the userspace due to common identification of pins. > > By identification, you mean "description" right? I see no problem of 2 > instances have the same pin "description"/label. > > >This solution scales up without any additional complexity in the driver, > >and without any need for internal per-board communication channels. > > > >Not sure if this is good or bad, but with current version, both approaches are > >possible, so it pretty much depending on the driver to initialize dplls with > >separated pin objects as you have suggested (and take its complexity into > >driver) or just share them. > > > >> > >>3) I don't like the concept of muxed pins and hierarchies of pins. Why > >> does user care? If pin is muxed, the rest of the pins related to this > >> one should be in state disabled/disconnected. The user only cares > >> about to see which pins are related to each other. It can be easily > >> exposed by "muxid" like this: > >> pin 1 > >> pin 2 > >> pin 3 muxid 100 > >> pin 4 muxid 100 > >> pin 5 muxid 101 > >> pin 6 muxid 101 > >> In this example pins 3,4 and 5,6 are muxed, therefore the user knows > >> if he connects one, the other one gets disconnected (or will have to > >> disconnect the first one explicitly first). > > > >Currently DPLLA_PIN_PARENT_IDX is doing the same thing as you described, it > >groups MUXed pins, the parent pin index here was most straightforward to me, > > There is a big difference if we model flat list of pins with a set of > attributes for each, comparing to a tree of pins, some acting as leaf, > node and root. Do we really need such complexicity? What value does it > bring to the user to expose this? The fact that you can't auto select from devices behind muxes. The HW topology is of material importance to user space. How many times does Arkadiusz have to explain this :|
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Wednesday, December 7, 2022 3:48 AM > Subject: Re: [RFC PATCH v4 0/4] Create common DPLL/clock configuration API > > On Fri, 2 Dec 2022 17:12:06 +0100 Jiri Pirko wrote: > > >But this is only doable with assumption, that the board is internally capable > > >of such internal board level communication, which in case of separated > > >firmwares handling multiple dplls might not be the case, or it would require > > >to have some other sw component feel that gap. > > > > Yep, you have the knowledge of sharing inside the driver, so you should > > do it there. For multiple instances, use in-driver notifier for example. > > No, complexity in the drivers is not a good idea. The core should cover > the complexity and let the drivers be simple. But how does Driver A know where to connect its pin to? It makes sense to share pins between the DPLLs exposed by a single driver, but not really outside of it. And that can be done simply by putting the pin ptr from the DPLLA into the pin list of DPLLB. If we want the kitchen-and-sink solution, we need to think about corner cases. Which pin should the API give to the userspace app - original, or muxed/parent? How would a teardown look like - if Driver A registered DPLLA with Pin1 and Driver B added the muxed pin then how should Driver A properly release its pins? Should it just send a message to driver B and trust that it will receive it in time before we tear everything apart? There are many problems with that approach, and the submitted patch is not explaining any of them. E.g. it contains the dpll_muxed_pin_register but no free counterpart + no flows. If we want to get shared pins, we need a good example of how this mechanism can be used. > > > >For complex boards with multiple dplls/sync channels, multiple ports, > > >multiple firmware instances, it seems to be complicated to share a pin if > > >each driver would have own copy and should notify all the other about > changes. > > > > > >To summarize, that is certainly true, shared pins idea complicates stuff > > >inside of dpll subsystem. > > >But at the same time it removes complexity from all the drivers which would > use > > > > There are currently 3 drivers for dpll I know of. This in ptp_ocp and > > mlx5 there is no concept of sharing pins. You you are talking about a > > single driver. > > > > What I'm trying to say is, looking at the code, the pin sharing, > > references and locking makes things uncomfortably complex. You are so > > far the only driver to need this, do it internally. If in the future > > other driver appears, this code would be eventually pushed into dpll > > core. No impact on UAPI from what I see. Please keep things as simple as > > possible. > > But the pin is shared for one driver. Who cares if it's not shared in > another. The user space must be able to reason about the constraints. > > You are suggesting drivers to magically flip state in core objects > because of some hidden dependencies?! > If we want to go outside the device, we'd need some universal language to describe external connections - such as the devicetree. I don't see how we can reliably implement inter-driver dependency otherwise. I think this would be better served in the userspace with a board-specific config file. Especially since the pins can be externally connected anyway.
Wed, Dec 07, 2022 at 03:47:40AM CET, kuba@kernel.org wrote: >On Fri, 2 Dec 2022 17:12:06 +0100 Jiri Pirko wrote: >> >But this is only doable with assumption, that the board is internally capable >> >of such internal board level communication, which in case of separated >> >firmwares handling multiple dplls might not be the case, or it would require >> >to have some other sw component feel that gap. >> >> Yep, you have the knowledge of sharing inside the driver, so you should >> do it there. For multiple instances, use in-driver notifier for example. > >No, complexity in the drivers is not a good idea. The core should cover >the complexity and let the drivers be simple. Really, even in case only one driver actually consumes the complexicity? I understand having a "libs" to do common functionality for drivers, even in case there is one. But this case, I don't see any benefit. > >> >For complex boards with multiple dplls/sync channels, multiple ports, >> >multiple firmware instances, it seems to be complicated to share a pin if >> >each driver would have own copy and should notify all the other about changes. >> > >> >To summarize, that is certainly true, shared pins idea complicates stuff >> >inside of dpll subsystem. >> >But at the same time it removes complexity from all the drivers which would use >> >> There are currently 3 drivers for dpll I know of. This in ptp_ocp and >> mlx5 there is no concept of sharing pins. You you are talking about a >> single driver. >> >> What I'm trying to say is, looking at the code, the pin sharing, >> references and locking makes things uncomfortably complex. You are so >> far the only driver to need this, do it internally. If in the future >> other driver appears, this code would be eventually pushed into dpll >> core. No impact on UAPI from what I see. Please keep things as simple as >> possible. > >But the pin is shared for one driver. Who cares if it's not shared in >another. The user space must be able to reason about the constraints. Sorry, I don't follow :/ Could you please explain what do you mean by this? > >You are suggesting drivers to magically flip state in core objects >because of some hidden dependencies?! It's not a state flip. It's more like a well propagated event of a state change. The async changes may happen anyway, so the userspace needs to handle them. Why is this different? > >> >it and is easier for the userspace due to common identification of pins. >> >> By identification, you mean "description" right? I see no problem of 2 >> instances have the same pin "description"/label. >> >> >This solution scales up without any additional complexity in the driver, >> >and without any need for internal per-board communication channels. >> > >> >Not sure if this is good or bad, but with current version, both approaches are >> >possible, so it pretty much depending on the driver to initialize dplls with >> >separated pin objects as you have suggested (and take its complexity into >> >driver) or just share them. >> > >> >> >> >>3) I don't like the concept of muxed pins and hierarchies of pins. Why >> >> does user care? If pin is muxed, the rest of the pins related to this >> >> one should be in state disabled/disconnected. The user only cares >> >> about to see which pins are related to each other. It can be easily >> >> exposed by "muxid" like this: >> >> pin 1 >> >> pin 2 >> >> pin 3 muxid 100 >> >> pin 4 muxid 100 >> >> pin 5 muxid 101 >> >> pin 6 muxid 101 >> >> In this example pins 3,4 and 5,6 are muxed, therefore the user knows >> >> if he connects one, the other one gets disconnected (or will have to >> >> disconnect the first one explicitly first). >> > >> >Currently DPLLA_PIN_PARENT_IDX is doing the same thing as you described, it >> >groups MUXed pins, the parent pin index here was most straightforward to me, >> >> There is a big difference if we model flat list of pins with a set of >> attributes for each, comparing to a tree of pins, some acting as leaf, >> node and root. Do we really need such complexicity? What value does it >> bring to the user to expose this? > >The fact that you can't auto select from devices behind muxes. Why? What's wrong with the mechanism I described in another part of this thread? Extending my example from above pin 1 source pin 2 output pin 3 muxid 100 source pin 4 muxid 100 source pin 5 muxid 101 source pin 6 muxid 101 source pin 7 output User now can set individial prios for sources: dpll x pin 1 set prio 10 etc The result would be: pin 1 source prio 10 pin 2 output pin 3 muxid 100 source prio 8 pin 4 muxid 100 source prio 20 pin 5 muxid 101 source prio 50 pin 6 muxid 101 source prio 60 pin 7 output Now when auto is enabled, the pin 3 is selected. Why would user need to manually select between 3 and 4? This is should be abstracted out by the driver. Actually, this is neat as you have one cmd to do selection in manual mode and you have uniform way of configuring/monitoring selection in autosel. Would the muxed pin make this better? For muxed pin being output, only one pin from mux would be set: pin 1 source pin 2 output pin 3 muxid 100 disconnected pin 4 muxid 100 disconnected pin 5 muxid 101 output pin 6 muxid 101 disconnected pin 7 output >The HW topology is of material importance to user space. Interesting. When I was working on linecards, you said that the user does not care about the inner HW topology. And it makes sense. When things could be abstracted out to make iface clean, I think they should. >How many times does Arkadiusz have to explain this :| Pardon my ignorance, I don't see why exactly we need mux hierarchy (trees) exposed to user.
On Wed, 7 Dec 2022 15:09:03 +0100 netdev.dump@gmail.com wrote: > > -----Original Message----- > > From: Jakub Kicinski <kuba@kernel.org> > > Sent: Wednesday, December 7, 2022 3:48 AM > > Subject: Re: [RFC PATCH v4 0/4] Create common DPLL/clock configuration API > > > > On Fri, 2 Dec 2022 17:12:06 +0100 Jiri Pirko wrote: > [...] > capable > [...] > require > [...] Please fix line wrapping in your email client. And add a name to your account configuration :/ > > > Yep, you have the knowledge of sharing inside the driver, so you should > > > do it there. For multiple instances, use in-driver notifier for example. > > > > No, complexity in the drivers is not a good idea. The core should cover > > the complexity and let the drivers be simple. > > But how does Driver A know where to connect its pin to? It makes sense to > share I think we discussed using serial numbers. > pins between the DPLLs exposed by a single driver, but not really outside of > it. > And that can be done simply by putting the pin ptr from the DPLLA into the > pin > list of DPLLB. Are you saying within the driver it's somehow easier? The driver state is mostly per bus device, so I don't see how. > If we want the kitchen-and-sink solution, we need to think about corner > cases. > Which pin should the API give to the userspace app - original, or > muxed/parent? IDK if I parse but I think both. If selected pin is not directly attached the core should configure muxes. > How would a teardown look like - if Driver A registered DPLLA with Pin1 and > Driver B added the muxed pin then how should Driver A properly > release its pins? Should it just send a message to driver B and trust that > it > will receive it in time before we tear everything apart? Trivial. > There are many problems with that approach, and the submitted patch is not > explaining any of them. E.g. it contains the dpll_muxed_pin_register but no > free > counterpart + no flows. SMOC. > If we want to get shared pins, we need a good example of how this mechanism > can be used. Agreed. > > > There are currently 3 drivers for dpll I know of. This in ptp_ocp and > > > mlx5 there is no concept of sharing pins. You you are talking about a > > > single driver. > > > > > > What I'm trying to say is, looking at the code, the pin sharing, > > > references and locking makes things uncomfortably complex. You are so > > > far the only driver to need this, do it internally. If in the future > > > other driver appears, this code would be eventually pushed into dpll > > > core. No impact on UAPI from what I see. Please keep things as simple as > > > possible. > > > > But the pin is shared for one driver. Who cares if it's not shared in > > another. The user space must be able to reason about the constraints. > > > > You are suggesting drivers to magically flip state in core objects > > because of some hidden dependencies?! > > If we want to go outside the device, we'd need some universal language > to describe external connections - such as the devicetree. I don't see how > we can reliably implement inter-driver dependency otherwise. There's plenty examples in the tree. If we can't use serial number directly we can compare the driver pointer + whatever you'd compare in the driver internal solution. > I think this would be better served in the userspace with a board-specific > config file. Especially since the pins can be externally connected anyway. Opinions vary, progress is not being made.
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Friday, December 2, 2022 5:12 PM > >Fri, Dec 02, 2022 at 12:27:24PM CET, arkadiusz.kubalewski@intel.com wrote: >>>From: Jiri Pirko <jiri@resnulli.us> >>>Sent: Wednesday, November 30, 2022 1:32 PM >>> >>>Tue, Nov 29, 2022 at 10:37:20PM CET, vfedorenko@novek.ru wrote: >>>>Implement common API for clock/DPLL configuration and status reporting. >>>>The API utilises netlink interface as transport for commands and event >>>>notifications. This API aim to extend current pin configuration and >>>>make it flexible and easy to cover special configurations. >>> >>>Overall, I see a lot of issues on multiple levels. I will go over them in >>>follow-up emails. So far, after couple of hours looking trought this, I >>>have following general feelings/notes: >> >>Hi Jiri, >> >>As we have been participating in last version, feel obligated to answer to >>the concerns. > >Cool. > > >> >>> >>>1) Netlink interface looks much saner than in previous versions. I will >>> send couple of notes, mainly around events and object mixtures and >>> couple of bugs/redundancies. But overall, looks fineish. >>> >>>2) I don't like that concept of a shared pin, at all. It makes things >>> unnecessary complicated. Just have a pin created for dpll instance >>> and that's it. If another instance has the same pin, it should create >>> it as well. Keeps things separate and easy to model. Let the >>> hw/fw/driver figure out the implementation oddities. >>> Why exactly you keep pushing the shared pin idea? Perhaps I'm missing >>> something crucial. >> >> >>If the user request change on pin#0 of dpll#0, the dpll#0 knows about the >>change, it reacts accordingly, and notifies the user the something has >changed. >>Which is rather simple. >> >>Now, if the dpll#1 is using the same pin (pin#0 of dpll#0), the >complicated >>part starts. First we have to assume: >>- it was initialized with the same description (as it should, to prevent >>confusing the user) >>- it was initialized with the same order (this is at least nice to have >from >>user POV, as pin indices are auto generated), and also in case of multiple >pins >>being shared it would be best for the user to have exactly the same number >of >>pins initialized, so they have same indices and initialization order >doesn't >>introduce additional confusion. >> >>Thus, one reason of shared pins was to prevent having this assumptions >ever. >>If the pin is shared, all dplls sharing a pin would have the same >description >>and pin index for a shared pin out of the box. >> >>Pin attribute changes >>The change on dpll#0 pin#0 impacts also dpll#1 pin#0. Notification about >the >>change shall be also requested from the driver that handles dpll#1. In >such >>case the driver has to have some dpll monitoring/notifying mechanics, >which at >>first doesn't look very hard to do, but most likely only if both dplls are >>initialized and managed by a single instance of a driver/firmware. >> >>If board has 2 dplls but each one is managed by its own firmware/driver >>instance. User changes frequency of pin#0 signal, the driver of dpll#0 >must >>also notify driver of dpll#1 that pin#0 frequency has changed, dpll#1 >reacts on >>the change, notifies the user. > >Right. > > >>But this is only doable with assumption, that the board is internally >capable >>of such internal board level communication, which in case of separated >>firmwares handling multiple dplls might not be the case, or it would >require >>to have some other sw component feel that gap. > >Yep, you have the knowledge of sharing inside the driver, so you should >do it there. For multiple instances, use in-driver notifier for example. > This can be done right now, if driver doesn't need built-in pin sharing or doesn't want it, it doesn't have to use it. It was designed this way for a reason, reason of reduced complexity of drivers, why should we want to increase complexity of multiple objects instead of having it in one place? > >> >>For complex boards with multiple dplls/sync channels, multiple ports, >>multiple firmware instances, it seems to be complicated to share a pin if >>each driver would have own copy and should notify all the other about >changes. >> >>To summarize, that is certainly true, shared pins idea complicates stuff >>inside of dpll subsystem. >>But at the same time it removes complexity from all the drivers which >would use > >There are currently 3 drivers for dpll I know of. This in ptp_ocp and >mlx5 there is no concept of sharing pins. You you are talking about a >single driver. > >What I'm trying to say is, looking at the code, the pin sharing, >references and locking makes things uncomfortably complex. You are so >far the only driver to need this, do it internally. If in the future >other driver appears, this code would be eventually pushed into dpll >core. No impact on UAPI from what I see. Please keep things as simple as >possible. > The part of interface for drivers is right now 17 functions in total, thus I wouldn't call it complicated. References between dpll instances and pins, are other part of "increased" complexity, but I wouldn't either call it over-complicated, besides it is part of dpll-core. Any interface shouldn't make the user to look into the code. For users only documentation shall be important. If kernel module developer is able to read header and understand what he needs to do for his hardware, whether he want to use shared pins or not, and what impact would it have. Thus real question is, if it is easy enough to use both parts. I would say the kernel module part is easy to understand even by looking at the function names and their arguments. Proper documentation would clear anything that is still not clear. Netlink part is also moving in a right direction. > >>it and is easier for the userspace due to common identification of pins. > >By identification, you mean "description" right? I see no problem of 2 >instances have the same pin "description"/label. > "description"/label and index as they are the same pin. Index is auto-generated and depends on initialization order, this makes unnecessary dependency. > >>This solution scales up without any additional complexity in the driver, >>and without any need for internal per-board communication channels. >> >>Not sure if this is good or bad, but with current version, both approaches >are >>possible, so it pretty much depending on the driver to initialize dplls >with >>separated pin objects as you have suggested (and take its complexity into >>driver) or just share them. >> >>> >>>3) I don't like the concept of muxed pins and hierarchies of pins. Why >>> does user care? If pin is muxed, the rest of the pins related to this >>> one should be in state disabled/disconnected. The user only cares >>> about to see which pins are related to each other. It can be easily >>> exposed by "muxid" like this: >>> pin 1 >>> pin 2 >>> pin 3 muxid 100 >>> pin 4 muxid 100 >>> pin 5 muxid 101 >>> pin 6 muxid 101 >>> In this example pins 3,4 and 5,6 are muxed, therefore the user knows >>> if he connects one, the other one gets disconnected (or will have to >>> disconnect the first one explicitly first). >>> >> >>Currently DPLLA_PIN_PARENT_IDX is doing the same thing as you described, >it >>groups MUXed pins, the parent pin index here was most straightforward to >me, > >There is a big difference if we model flat list of pins with a set of >attributes for each, comparing to a tree of pins, some acting as leaf, >node and root. Do we really need such complexicity? What value does it >bring to the user to expose this? > If the one doesn't need to use muxed pins, everything is simple, as you have described. In fact in the example you have given, only benefit I can see from muxid is that user knows which pins would get disconnected when one of the mux group pins is selected. But do user really needs that knowledge? If one pin was selected why should he look on the other pins? Anyway I see your point, but this is not only about identifying a muxed pins, it is more about identifying connections between them because of hardware capabilities they can bring, and utilization of such capabilities. Please take a look on a comment below. > >>as in case of DPLL_MODE_AUTOMATIC, where dpll auto-selects highest >priority >>available signal. The priority can be only assigned to the pins directly >>connected to the dpll. The rest of pins (which would have present >>attribute DPLLA_PIN_PARENT_IDX) are the ones that require manual selection >>even if DPLL_MODE_AUTOMATIC is enabled. >> >>Enabling a particular pin and sub-pin in DPLL_MODE_AUTOMATIC requires from >user >>to select proper priority on on a dpll-level MUX-pin and manually select >one of >>the sub-pins. >>On the other hand for DPLL_MODE_FORCED, this might be also beneficial, as >the >>user could select a directly connected pin and muxed pin with two >separated >>commands, which could be handled in separated driver instances (if HW >design >>requires such approach) or either it can be handled just by one select >call >>for the pin connected directly and handled entirely in the one driver >instance. > >Talk netlink please. What are the actual commands with cmds used to >select the source and select a mux pin? You are talking about 2 types of >selections: >1) Source select > - this is as you described either auto/forces (manual) mode, > according to some prio, dpll select the best source >2) Pin select in a mux > - here the pin could be source or output > >But again, from the user perspective, why does he have to distinguish >these? Extending my example: > > pin 1 source > pin 2 output > pin 3 muxid 100 source > pin 4 muxid 100 source > pin 5 muxid 101 source > pin 6 muxid 101 source > pin 7 output > >User now can set individial prios for sources: > >dpll x pin 1 set prio 10 >etc >The result would be: > > pin 1 source prio 10 > pin 2 output > pin 3 muxid 100 source prio 8 > pin 4 muxid 100 source prio 20 > pin 5 muxid 101 source prio 50 > pin 6 muxid 101 source prio 60 > pin 7 output > >Now when auto is enabled, the pin 3 is selected. Why would user need to >manually select between 3 and 4? This is should be abstracted out by the >driver. Yes, this could be abstracted in the driver, but we are talking here about different things. We need automatic hardware-supported selection of a pin, not automatic software-supported selection, where driver (part of software stack) is doing autoselection. If dpll is capable of hw-autoselection, the priorities are configured in the hardware, not in the driver. This interface is a proxy between user and dpll. The one could use it as you have described by implementing auto-selection in SW, but those are two different modes, actually we could introduce modes like: DPLL_MODE_HW_AUTOMATIC and DPLL_MODE_SW_AUTOMATIC, to clear things up. Although, IMHO, DPLL_MODE_SW_AUTOMATIC wouldn't really differ from current behavior of DPLL_MODE_FORCED, where the userspace is explicitly selecting a source, and the only difference would be that selection comes from driver instead of userspace tool. Thus in the end it would just increase complexity of a driver (instead of bringing any benefits). > >Only issues I see when pins are output. User would have to somehow >select one of the pins in the mux (perhaps another dpll cmd). But the >mux pin instance does not help with anything there... > > > >> >>>4) I don't like the "attr" indirection. It makes things very tangled. It >>> comes from the concepts of classes and objects and takes it to >>> extreme. Not really something we are commonly used to in kernel. >>> Also, it brings no value from what I can see, only makes things very >>> hard to read and follow. >>> >> >>Yet again, true, I haven't find anything similar in the kernel, it was >more >>like a try to find out a way to have a single structure with all the stuff >that >>is passed between netlink/core/driver parts. Came up with this, and to be >>honest it suits pretty well, those are well defined containers. They store >>attributes that either user or driver have set, with ability to obtain a >valid >>value only if it was set. Thus whoever reads a struct, knows which of >those >>attributes were actually set. > >Sorry for being blunt here, but when I saw the code I remembered my days >as a student where they forced us to do similar things Java :) >There you tend to make things contained, everything is a class, getters, >setters and whatnot. In kernel, this is overkill. > >I'm not saying it's functionally wrong. What I say is that it is >completely unnecessary. I see no advantage, by having this indirection. >I see only disadvantages. It makes code harder to read and follow. >What I suggest, again, is to make things nice and simple. Set of ops >that the driver implements for dpll commands or parts of commands, >as we are used to in the rest of the kernel. > No problem, I think we will get rid of it, see comment below. > >>As you said, seems a bit revolutionary, but IMHO it simplifies stuff, and >>basically it is value and validity bit, which I believe is rather common >in the >>kernel, this differs only with the fact it is encapsulated. No direct >access to >>the fields of structure is available for the users. > >I don't see any reason for any validity bits whan you just do it using >driver-implemented ops. > > >>Most probably there are some things that could be improved with it, but in >>general it is very easy to use and understand how it works. >>What could be improved: >>- naming scheme as function names are a bit long right now, although >mostly >>still fits the line-char limits, thus not that problematic >>- bit mask values are capable of storing 32 bits and bit(0) is always used >as >>unspec, which ends up with 31 values available for the enums so if by any >>chance one of the attribute enums would go over 32 it could be an issue. >> >>It is especially useful for multiple values passed with the same netlink >>attribute id. I.e. please take a look at >dpll_msg_add_pin_types_supported(..) >>function. >> >>> Please keep things direct and simple: >>> * If some option could be changed for a pin or dpll, just have an >>> op that is directly called from netlink handler to change it. >>> There should be clear set of ops for configuration of pin and >>> dpll object. This "attr" indirection make this totally invisible. >> >>In last review you have asked to have rather only set and get ops defined >>with a single attribute struct. This is exactly that, altough >encapsulated. > >For objects, yes. Pass a struct you directly read/write if the amount of >function args would be bigger then say 4. The whole encapsulation is not >good for anything. If there is one set/get for whole object, then a writer of a struct (netlink or driver) has to have possibility to indicate which parts of the struct were actually set, so a reader can do things that were requested. But I agree this is probably not any better to the "ops per attribute" approach we have had before, thus I think we shall get back to this approach and remove dpll_attr/dpll_pin_attr entirely. > > >> >>> * If some attribute is const during dpll or pin lifetime, have it >>> passed during dpll or pin creation. >>> >>> >> >>Only driver knows which attributes are const and which are not, this shall > >Nonono. This is semantics defined by the subsystem. The pin >label/description for example. That is const, cannot be set by the user. True, it is const and it is passed on pin init. >The type of the pin (synce/gnss/ext) is const, cannot be set by the user. It can, as input/source can change, thus the same way, type of pin could be managed by some hardware fuses/switches/etc. >This you have to clearly specify when you define driver API. >This const attrs should be passed during pin creation/registration. > >Talking about dpll instance itself, the clock_id, clock_quality, these >should be also const attrs. > Actually, clock_quality can also vary on runtime (i.e. ext/synce). We cannot determine what Quality Level signal user has connected to the SMA or was received from the network. Only gnss/oscilattor could have const depending on used HW. But generally it shall not be const. Thanks, Arkadiusz > > >>be also part of driver implementation. >>As I understand all the fields present in (dpll/dpll_pin)_attr, used in >get/set >>ops, could be altered in run-time depending on HW design. >> >>Thanks, >>Arkadiusz >> >>> >>>> >>>>v3 -> v4: >>>> * redesign framework to make pins dynamically allocated (Arkadiusz) >>>> * implement shared pins (Arkadiusz) >>>>v2 -> v3: >>>> * implement source select mode (Arkadiusz) >>>> * add documentation >>>> * implementation improvements (Jakub) >>>>v1 -> v2: >>>> * implement returning supported input/output types >>>> * ptp_ocp: follow suggestions from Jonathan >>>> * add linux-clk mailing list >>>>v0 -> v1: >>>> * fix code style and errors >>>> * add linux-arm mailing list >>>> >>>> >>>>Arkadiusz Kubalewski (1): >>>> dpll: add dpll_attr/dpll_pin_attr helper classes >>>> >>>>Vadim Fedorenko (3): >>>> dpll: Add DPLL framework base functions >>>> dpll: documentation on DPLL subsystem interface >>>> ptp_ocp: implement DPLL ops >>>> >>>> Documentation/networking/dpll.rst | 271 ++++++++ >>>> Documentation/networking/index.rst | 1 + >>>> MAINTAINERS | 8 + >>>> drivers/Kconfig | 2 + >>>> drivers/Makefile | 1 + >>>> drivers/dpll/Kconfig | 7 + >>>> drivers/dpll/Makefile | 11 + >>>> drivers/dpll/dpll_attr.c | 278 +++++++++ >>>> drivers/dpll/dpll_core.c | 760 +++++++++++++++++++++++ >>>> drivers/dpll/dpll_core.h | 176 ++++++ >>>> drivers/dpll/dpll_netlink.c | 963 +++++++++++++++++++++++++++++ >>>> drivers/dpll/dpll_netlink.h | 24 + >>>> drivers/dpll/dpll_pin_attr.c | 456 ++++++++++++++ >>>> drivers/ptp/Kconfig | 1 + >>>> drivers/ptp/ptp_ocp.c | 123 ++-- >>>> include/linux/dpll.h | 261 ++++++++ >>>> include/linux/dpll_attr.h | 433 +++++++++++++ >>>> include/uapi/linux/dpll.h | 263 ++++++++ >>>> 18 files changed, 4002 insertions(+), 37 deletions(-) create mode >>>> 100644 Documentation/networking/dpll.rst create mode 100644 >>>> drivers/dpll/Kconfig create mode 100644 drivers/dpll/Makefile create >>>> mode 100644 drivers/dpll/dpll_attr.c create mode 100644 >>>> drivers/dpll/dpll_core.c create mode 100644 drivers/dpll/dpll_core.h >>>> create mode 100644 drivers/dpll/dpll_netlink.c create mode 100644 >>>> drivers/dpll/dpll_netlink.h create mode 100644 >>>> drivers/dpll/dpll_pin_attr.c create mode 100644 include/linux/dpll.h >>>> create mode 100644 include/linux/dpll_attr.h create mode 100644 >>>> include/uapi/linux/dpll.h >>>> >>>>-- >>>>2.27.0 >>>>
Thu, Dec 08, 2022 at 12:21:57AM CET, kuba@kernel.org wrote: >On Wed, 7 Dec 2022 15:09:03 +0100 netdev.dump@gmail.com wrote: >> > -----Original Message----- >> > From: Jakub Kicinski <kuba@kernel.org> >> > Sent: Wednesday, December 7, 2022 3:48 AM >> > Subject: Re: [RFC PATCH v4 0/4] Create common DPLL/clock configuration API >> > >> > On Fri, 2 Dec 2022 17:12:06 +0100 Jiri Pirko wrote: >> [...] >> capable >> [...] >> require >> [...] > >Please fix line wrapping in your email client. >And add a name to your account configuration :/ > >> > > Yep, you have the knowledge of sharing inside the driver, so you should >> > > do it there. For multiple instances, use in-driver notifier for example. >> > >> > No, complexity in the drivers is not a good idea. The core should cover >> > the complexity and let the drivers be simple. >> >> But how does Driver A know where to connect its pin to? It makes sense to >> share > >I think we discussed using serial numbers. Can you remind it? Do you mean serial number of pin? > >> pins between the DPLLs exposed by a single driver, but not really outside of >> it. >> And that can be done simply by putting the pin ptr from the DPLLA into the >> pin >> list of DPLLB. > >Are you saying within the driver it's somehow easier? The driver state >is mostly per bus device, so I don't see how. You can have some shared data for multiple instances in the driver code, why not? > >> If we want the kitchen-and-sink solution, we need to think about corner >> cases. >> Which pin should the API give to the userspace app - original, or >> muxed/parent? > >IDK if I parse but I think both. If selected pin is not directly >attached the core should configure muxes. > >> How would a teardown look like - if Driver A registered DPLLA with Pin1 and >> Driver B added the muxed pin then how should Driver A properly >> release its pins? Should it just send a message to driver B and trust that >> it >> will receive it in time before we tear everything apart? > >Trivial. > >> There are many problems with that approach, and the submitted patch is not >> explaining any of them. E.g. it contains the dpll_muxed_pin_register but no >> free >> counterpart + no flows. > >SMOC. Care to spell this out. I guess you didn't mean "South Middlesex Opportunity Council" :D > >> If we want to get shared pins, we need a good example of how this mechanism >> can be used. > >Agreed. > >> > > There are currently 3 drivers for dpll I know of. This in ptp_ocp and >> > > mlx5 there is no concept of sharing pins. You you are talking about a >> > > single driver. >> > > >> > > What I'm trying to say is, looking at the code, the pin sharing, >> > > references and locking makes things uncomfortably complex. You are so >> > > far the only driver to need this, do it internally. If in the future >> > > other driver appears, this code would be eventually pushed into dpll >> > > core. No impact on UAPI from what I see. Please keep things as simple as >> > > possible. >> > >> > But the pin is shared for one driver. Who cares if it's not shared in >> > another. The user space must be able to reason about the constraints. >> > >> > You are suggesting drivers to magically flip state in core objects >> > because of some hidden dependencies?! >> >> If we want to go outside the device, we'd need some universal language >> to describe external connections - such as the devicetree. I don't see how >> we can reliably implement inter-driver dependency otherwise. > >There's plenty examples in the tree. If we can't use serial number >directly we can compare the driver pointer + whatever you'd compare >in the driver internal solution. > >> I think this would be better served in the userspace with a board-specific >> config file. Especially since the pins can be externally connected anyway. > >Opinions vary, progress is not being made.
Thu, Dec 08, 2022 at 01:27:42AM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Friday, December 2, 2022 5:12 PM >> >>Fri, Dec 02, 2022 at 12:27:24PM CET, arkadiusz.kubalewski@intel.com wrote: >>>>From: Jiri Pirko <jiri@resnulli.us> >>>>Sent: Wednesday, November 30, 2022 1:32 PM >>>> >>>>Tue, Nov 29, 2022 at 10:37:20PM CET, vfedorenko@novek.ru wrote: >>>>>Implement common API for clock/DPLL configuration and status reporting. >>>>>The API utilises netlink interface as transport for commands and event >>>>>notifications. This API aim to extend current pin configuration and >>>>>make it flexible and easy to cover special configurations. >>>> >>>>Overall, I see a lot of issues on multiple levels. I will go over them in >>>>follow-up emails. So far, after couple of hours looking trought this, I >>>>have following general feelings/notes: >>> >>>Hi Jiri, >>> >>>As we have been participating in last version, feel obligated to answer to >>>the concerns. >> >>Cool. >> >> >>> >>>> >>>>1) Netlink interface looks much saner than in previous versions. I will >>>> send couple of notes, mainly around events and object mixtures and >>>> couple of bugs/redundancies. But overall, looks fineish. >>>> >>>>2) I don't like that concept of a shared pin, at all. It makes things >>>> unnecessary complicated. Just have a pin created for dpll instance >>>> and that's it. If another instance has the same pin, it should create >>>> it as well. Keeps things separate and easy to model. Let the >>>> hw/fw/driver figure out the implementation oddities. >>>> Why exactly you keep pushing the shared pin idea? Perhaps I'm missing >>>> something crucial. >>> >>> >>>If the user request change on pin#0 of dpll#0, the dpll#0 knows about the >>>change, it reacts accordingly, and notifies the user the something has >>changed. >>>Which is rather simple. >>> >>>Now, if the dpll#1 is using the same pin (pin#0 of dpll#0), the >>complicated >>>part starts. First we have to assume: >>>- it was initialized with the same description (as it should, to prevent >>>confusing the user) >>>- it was initialized with the same order (this is at least nice to have >>from >>>user POV, as pin indices are auto generated), and also in case of multiple >>pins >>>being shared it would be best for the user to have exactly the same number >>of >>>pins initialized, so they have same indices and initialization order >>doesn't >>>introduce additional confusion. >>> >>>Thus, one reason of shared pins was to prevent having this assumptions >>ever. >>>If the pin is shared, all dplls sharing a pin would have the same >>description >>>and pin index for a shared pin out of the box. >>> >>>Pin attribute changes >>>The change on dpll#0 pin#0 impacts also dpll#1 pin#0. Notification about >>the >>>change shall be also requested from the driver that handles dpll#1. In >>such >>>case the driver has to have some dpll monitoring/notifying mechanics, >>which at >>>first doesn't look very hard to do, but most likely only if both dplls are >>>initialized and managed by a single instance of a driver/firmware. >>> >>>If board has 2 dplls but each one is managed by its own firmware/driver >>>instance. User changes frequency of pin#0 signal, the driver of dpll#0 >>must >>>also notify driver of dpll#1 that pin#0 frequency has changed, dpll#1 >>reacts on >>>the change, notifies the user. >> >>Right. >> >> >>>But this is only doable with assumption, that the board is internally >>capable >>>of such internal board level communication, which in case of separated >>>firmwares handling multiple dplls might not be the case, or it would >>require >>>to have some other sw component feel that gap. >> >>Yep, you have the knowledge of sharing inside the driver, so you should >>do it there. For multiple instances, use in-driver notifier for example. >> > >This can be done right now, if driver doesn't need built-in pin sharing or >doesn't want it, it doesn't have to use it. >It was designed this way for a reason, reason of reduced complexity of drivers, >why should we want to increase complexity of multiple objects instead of having >it in one place? Nevermind, don't want to repeat myself. Looks like everyone wants this pin sharing infra in the core, let's now focus on designing it properly. > >> >>> >>>For complex boards with multiple dplls/sync channels, multiple ports, >>>multiple firmware instances, it seems to be complicated to share a pin if >>>each driver would have own copy and should notify all the other about >>changes. >>> >>>To summarize, that is certainly true, shared pins idea complicates stuff >>>inside of dpll subsystem. >>>But at the same time it removes complexity from all the drivers which >>would use >> >>There are currently 3 drivers for dpll I know of. This in ptp_ocp and >>mlx5 there is no concept of sharing pins. You you are talking about a >>single driver. >> >>What I'm trying to say is, looking at the code, the pin sharing, >>references and locking makes things uncomfortably complex. You are so >>far the only driver to need this, do it internally. If in the future >>other driver appears, this code would be eventually pushed into dpll >>core. No impact on UAPI from what I see. Please keep things as simple as >>possible. >> > >The part of interface for drivers is right now 17 functions in total, thus I >wouldn't call it complicated. References between dpll instances and pins, are >other part of "increased" complexity, but I wouldn't either call it >over-complicated, besides it is part of dpll-core. Any interface shouldn't make >the user to look into the code. For users only documentation shall be important. >If kernel module developer is able to read header and understand what he needs >to do for his hardware, whether he want to use shared pins or not, and what >impact would it have. > >Thus real question is, if it is easy enough to use both parts. >I would say the kernel module part is easy to understand even by looking at >the function names and their arguments. Proper documentation would clear >anything that is still not clear. > >Netlink part is also moving in a right direction. > >> >>>it and is easier for the userspace due to common identification of pins. >> >>By identification, you mean "description" right? I see no problem of 2 >>instances have the same pin "description"/label. >> > >"description"/label and index as they are the same pin. Index is auto-generated >and depends on initialization order, this makes unnecessary dependency. > >> >>>This solution scales up without any additional complexity in the driver, >>>and without any need for internal per-board communication channels. >>> >>>Not sure if this is good or bad, but with current version, both approaches >>are >>>possible, so it pretty much depending on the driver to initialize dplls >>with >>>separated pin objects as you have suggested (and take its complexity into >>>driver) or just share them. >>> >>>> >>>>3) I don't like the concept of muxed pins and hierarchies of pins. Why >>>> does user care? If pin is muxed, the rest of the pins related to this >>>> one should be in state disabled/disconnected. The user only cares >>>> about to see which pins are related to each other. It can be easily >>>> exposed by "muxid" like this: >>>> pin 1 >>>> pin 2 >>>> pin 3 muxid 100 >>>> pin 4 muxid 100 >>>> pin 5 muxid 101 >>>> pin 6 muxid 101 >>>> In this example pins 3,4 and 5,6 are muxed, therefore the user knows >>>> if he connects one, the other one gets disconnected (or will have to >>>> disconnect the first one explicitly first). >>>> >>> >>>Currently DPLLA_PIN_PARENT_IDX is doing the same thing as you described, >>it >>>groups MUXed pins, the parent pin index here was most straightforward to >>me, >> >>There is a big difference if we model flat list of pins with a set of >>attributes for each, comparing to a tree of pins, some acting as leaf, >>node and root. Do we really need such complexicity? What value does it >>bring to the user to expose this? >> > >If the one doesn't need to use muxed pins, everything is simple, as you have >described. In fact in the example you have given, only benefit I can see from >muxid is that user knows which pins would get disconnected when one of the mux >group pins is selected. But do user really needs that knowledge? If one pin >was selected why should he look on the other pins? Right. That would be probably valuable only for output pins. > >Anyway I see your point, but this is not only about identifying a muxed pins, >it is more about identifying connections between them because of hardware >capabilities they can bring, and utilization of such capabilities. >Please take a look on a comment below. > >> >>>as in case of DPLL_MODE_AUTOMATIC, where dpll auto-selects highest >>priority >>>available signal. The priority can be only assigned to the pins directly >>>connected to the dpll. The rest of pins (which would have present >>>attribute DPLLA_PIN_PARENT_IDX) are the ones that require manual selection >>>even if DPLL_MODE_AUTOMATIC is enabled. >>> >>>Enabling a particular pin and sub-pin in DPLL_MODE_AUTOMATIC requires from >>user >>>to select proper priority on on a dpll-level MUX-pin and manually select >>one of >>>the sub-pins. >>>On the other hand for DPLL_MODE_FORCED, this might be also beneficial, as >>the >>>user could select a directly connected pin and muxed pin with two >>separated >>>commands, which could be handled in separated driver instances (if HW >>design >>>requires such approach) or either it can be handled just by one select >>call >>>for the pin connected directly and handled entirely in the one driver >>instance. >> >>Talk netlink please. What are the actual commands with cmds used to >>select the source and select a mux pin? You are talking about 2 types of >>selections: >>1) Source select >> - this is as you described either auto/forces (manual) mode, >> according to some prio, dpll select the best source >>2) Pin select in a mux >> - here the pin could be source or output >> >>But again, from the user perspective, why does he have to distinguish >>these? Extending my example: >> >> pin 1 source >> pin 2 output >> pin 3 muxid 100 source >> pin 4 muxid 100 source >> pin 5 muxid 101 source >> pin 6 muxid 101 source >> pin 7 output >> >>User now can set individial prios for sources: >> >>dpll x pin 1 set prio 10 >>etc >>The result would be: >> >> pin 1 source prio 10 >> pin 2 output >> pin 3 muxid 100 source prio 8 >> pin 4 muxid 100 source prio 20 >> pin 5 muxid 101 source prio 50 >> pin 6 muxid 101 source prio 60 >> pin 7 output >> >>Now when auto is enabled, the pin 3 is selected. Why would user need to >>manually select between 3 and 4? This is should be abstracted out by the >>driver. > >Yes, this could be abstracted in the driver, but we are talking here about >different things. We need automatic hardware-supported selection of a pin, >not automatic software-supported selection, where driver (part of software >stack) is doing autoselection. > >If dpll is capable of hw-autoselection, the priorities are configured in the >hardware, not in the driver. This interface is a proxy between user and dpll. Okay, this is very nice example, where the confusion appeared. Looking into your code, function dpll_pin_attr_from_nlattr(), it clearly allows userspace to set the prio over DPLLA_PIN_PRIO. Yet you claim the prio is configured in the hardware. >The one could use it as you have described by implementing auto-selection >in SW, but those are two different modes, actually we could introduce modes >like: DPLL_MODE_HW_AUTOMATIC and DPLL_MODE_SW_AUTOMATIC, to clear things up. >Although, IMHO, DPLL_MODE_SW_AUTOMATIC wouldn't really differ from current >behavior of DPLL_MODE_FORCED, where the userspace is explicitly selecting a >source, and the only difference would be that selection comes from driver >instead of userspace tool. Thus in the end it would just increase complexity >of a driver (instead of bringing any benefits). Yeah, does not make sense to have DPLL_MODE_SW_AUTOMATIC as you describe it. I agree. > >> >>Only issues I see when pins are output. User would have to somehow >>select one of the pins in the mux (perhaps another dpll cmd). But the >>mux pin instance does not help with anything there... >> >> >> >>> >>>>4) I don't like the "attr" indirection. It makes things very tangled. It >>>> comes from the concepts of classes and objects and takes it to >>>> extreme. Not really something we are commonly used to in kernel. >>>> Also, it brings no value from what I can see, only makes things very >>>> hard to read and follow. >>>> >>> >>>Yet again, true, I haven't find anything similar in the kernel, it was >>more >>>like a try to find out a way to have a single structure with all the stuff >>that >>>is passed between netlink/core/driver parts. Came up with this, and to be >>>honest it suits pretty well, those are well defined containers. They store >>>attributes that either user or driver have set, with ability to obtain a >>valid >>>value only if it was set. Thus whoever reads a struct, knows which of >>those >>>attributes were actually set. >> >>Sorry for being blunt here, but when I saw the code I remembered my days >>as a student where they forced us to do similar things Java :) >>There you tend to make things contained, everything is a class, getters, >>setters and whatnot. In kernel, this is overkill. >> >>I'm not saying it's functionally wrong. What I say is that it is >>completely unnecessary. I see no advantage, by having this indirection. >>I see only disadvantages. It makes code harder to read and follow. >>What I suggest, again, is to make things nice and simple. Set of ops >>that the driver implements for dpll commands or parts of commands, >>as we are used to in the rest of the kernel. >> > >No problem, I think we will get rid of it, see comment below. > >> >>>As you said, seems a bit revolutionary, but IMHO it simplifies stuff, and >>>basically it is value and validity bit, which I believe is rather common >>in the >>>kernel, this differs only with the fact it is encapsulated. No direct >>access to >>>the fields of structure is available for the users. >> >>I don't see any reason for any validity bits whan you just do it using >>driver-implemented ops. >> >> >>>Most probably there are some things that could be improved with it, but in >>>general it is very easy to use and understand how it works. >>>What could be improved: >>>- naming scheme as function names are a bit long right now, although >>mostly >>>still fits the line-char limits, thus not that problematic >>>- bit mask values are capable of storing 32 bits and bit(0) is always used >>as >>>unspec, which ends up with 31 values available for the enums so if by any >>>chance one of the attribute enums would go over 32 it could be an issue. >>> >>>It is especially useful for multiple values passed with the same netlink >>>attribute id. I.e. please take a look at >>dpll_msg_add_pin_types_supported(..) >>>function. >>> >>>> Please keep things direct and simple: >>>> * If some option could be changed for a pin or dpll, just have an >>>> op that is directly called from netlink handler to change it. >>>> There should be clear set of ops for configuration of pin and >>>> dpll object. This "attr" indirection make this totally invisible. >>> >>>In last review you have asked to have rather only set and get ops defined >>>with a single attribute struct. This is exactly that, altough >>encapsulated. >> >>For objects, yes. Pass a struct you directly read/write if the amount of >>function args would be bigger then say 4. The whole encapsulation is not >>good for anything. > >If there is one set/get for whole object, then a writer of a struct (netlink >or driver) has to have possibility to indicate which parts of the struct were >actually set, so a reader can do things that were requested. > >But I agree this is probably not any better to the "ops per attribute" approach >we have had before, thus I think we shall get back to this approach and remove >dpll_attr/dpll_pin_attr entirely. > >> >> >>> >>>> * If some attribute is const during dpll or pin lifetime, have it >>>> passed during dpll or pin creation. >>>> >>>> >>> >>>Only driver knows which attributes are const and which are not, this shall >> >>Nonono. This is semantics defined by the subsystem. The pin >>label/description for example. That is const, cannot be set by the user. > >True, it is const and it is passed on pin init. > >>The type of the pin (synce/gnss/ext) is const, cannot be set by the user. > >It can, as input/source can change, thus the same way, type of pin could be >managed by some hardware fuses/switches/etc. Could you please describe this a bit more? For example how the user can change synce pin to ext pin? I still fail to understand how such thing is possible. How the HW could be rewired on user request? Perhaps we understand differently the "pin" object. I understand is as something out-facing. Not an actual pin of a chip. > >>This you have to clearly specify when you define driver API. >>This const attrs should be passed during pin creation/registration. >> >>Talking about dpll instance itself, the clock_id, clock_quality, these >>should be also const attrs. >> > >Actually, clock_quality can also vary on runtime (i.e. ext/synce). We cannot >determine what Quality Level signal user has connected to the SMA or was >received from the network. Only gnss/oscilattor could have const depending >on used HW. But generally it shall not be const. Sec. I'm talkign about the actual dpll quality, means the internal clock. How it can vary? > >Thanks, >Arkadiusz > >> >> >>>be also part of driver implementation. >>>As I understand all the fields present in (dpll/dpll_pin)_attr, used in >>get/set >>>ops, could be altered in run-time depending on HW design. >>> >>>Thanks, >>>Arkadiusz >>> >>>> >>>>> >>>>>v3 -> v4: >>>>> * redesign framework to make pins dynamically allocated (Arkadiusz) >>>>> * implement shared pins (Arkadiusz) >>>>>v2 -> v3: >>>>> * implement source select mode (Arkadiusz) >>>>> * add documentation >>>>> * implementation improvements (Jakub) >>>>>v1 -> v2: >>>>> * implement returning supported input/output types >>>>> * ptp_ocp: follow suggestions from Jonathan >>>>> * add linux-clk mailing list >>>>>v0 -> v1: >>>>> * fix code style and errors >>>>> * add linux-arm mailing list >>>>> >>>>> >>>>>Arkadiusz Kubalewski (1): >>>>> dpll: add dpll_attr/dpll_pin_attr helper classes >>>>> >>>>>Vadim Fedorenko (3): >>>>> dpll: Add DPLL framework base functions >>>>> dpll: documentation on DPLL subsystem interface >>>>> ptp_ocp: implement DPLL ops >>>>> >>>>> Documentation/networking/dpll.rst | 271 ++++++++ >>>>> Documentation/networking/index.rst | 1 + >>>>> MAINTAINERS | 8 + >>>>> drivers/Kconfig | 2 + >>>>> drivers/Makefile | 1 + >>>>> drivers/dpll/Kconfig | 7 + >>>>> drivers/dpll/Makefile | 11 + >>>>> drivers/dpll/dpll_attr.c | 278 +++++++++ >>>>> drivers/dpll/dpll_core.c | 760 +++++++++++++++++++++++ >>>>> drivers/dpll/dpll_core.h | 176 ++++++ >>>>> drivers/dpll/dpll_netlink.c | 963 +++++++++++++++++++++++++++++ >>>>> drivers/dpll/dpll_netlink.h | 24 + >>>>> drivers/dpll/dpll_pin_attr.c | 456 ++++++++++++++ >>>>> drivers/ptp/Kconfig | 1 + >>>>> drivers/ptp/ptp_ocp.c | 123 ++-- >>>>> include/linux/dpll.h | 261 ++++++++ >>>>> include/linux/dpll_attr.h | 433 +++++++++++++ >>>>> include/uapi/linux/dpll.h | 263 ++++++++ >>>>> 18 files changed, 4002 insertions(+), 37 deletions(-) create mode >>>>> 100644 Documentation/networking/dpll.rst create mode 100644 >>>>> drivers/dpll/Kconfig create mode 100644 drivers/dpll/Makefile create >>>>> mode 100644 drivers/dpll/dpll_attr.c create mode 100644 >>>>> drivers/dpll/dpll_core.c create mode 100644 drivers/dpll/dpll_core.h >>>>> create mode 100644 drivers/dpll/dpll_netlink.c create mode 100644 >>>>> drivers/dpll/dpll_netlink.h create mode 100644 >>>>> drivers/dpll/dpll_pin_attr.c create mode 100644 include/linux/dpll.h >>>>> create mode 100644 include/linux/dpll_attr.h create mode 100644 >>>>> include/uapi/linux/dpll.h >>>>> >>>>>-- >>>>>2.27.0 >>>>>
Wed, Dec 07, 2022 at 06:19:46PM CET, kuba@kernel.org wrote: >On Wed, 7 Dec 2022 15:51:42 +0100 Jiri Pirko wrote: >> Wed, Dec 07, 2022 at 03:47:40AM CET, kuba@kernel.org wrote: >> >On Fri, 2 Dec 2022 17:12:06 +0100 Jiri Pirko wrote: >> >> Yep, you have the knowledge of sharing inside the driver, so you should >> >> do it there. For multiple instances, use in-driver notifier for example. >> > >> >No, complexity in the drivers is not a good idea. The core should cover >> >the complexity and let the drivers be simple. >> >> Really, even in case only one driver actually consumes the complexicity? >> I understand having a "libs" to do common functionality for drivers, >> even in case there is one. But this case, I don't see any benefit. > >In the same email thread you admit that mlx5 has multiple devlink >instances for the same ASIC and refuse to try to prevent similar >situation happening in the new subsystem. I don't understand your point. In CX there is 1 clock for 2 pci PFs. I plan to have 1 dpll instance for the clock shared. But how is what you write relevant to the discussion? We are talking about: a) 1 pin in 2 dpll instances what I undestand you say here is to prevent: b) 2 dpll instances for 1 clock apples and oranges. Am I missing something? I'm totally against b) but that is not what we discuss here, correct? > >> >> There are currently 3 drivers for dpll I know of. This in ptp_ocp and >> >> mlx5 there is no concept of sharing pins. You you are talking about a >> >> single driver. >> >> >> >> What I'm trying to say is, looking at the code, the pin sharing, >> >> references and locking makes things uncomfortably complex. You are so >> >> far the only driver to need this, do it internally. If in the future >> >> other driver appears, this code would be eventually pushed into dpll >> >> core. No impact on UAPI from what I see. Please keep things as simple as >> >> possible. >> > >> >But the pin is shared for one driver. Who cares if it's not shared in >> >another. The user space must be able to reason about the constraints. >> >> Sorry, I don't follow :/ Could you please explain what do you mean by >> this? > >We don't wait with adding APIs until there is more than one driver that >needs them. Agreed. I was under impression that this is only kernel internals and won't affect the UAPI. Perhaps I'm wrong. > >> >You are suggesting drivers to magically flip state in core objects >> >because of some hidden dependencies?! >> >> It's not a state flip. It's more like a well propagated event of a state >> change. The async changes may happen anyway, so the userspace needs >> to handle them. Why is this different? > >What if the user space wants conflicting configurations for the muxes >behind a shared pin? > >The fact that there is a notification does not solve the problem of >user space not knowing what's going on. Why would the user space play >guessing games if the driver _knows_ the topology and can easily tell >it. Okay. I get your point. This visibility is probably something nice to have. If it weights over the added complexicity, I'm not sure. But it looks like you are, and I don't care that much. So let's focus on defining the shared pin model properly. >> >> There is a big difference if we model flat list of pins with a set of >> >> attributes for each, comparing to a tree of pins, some acting as leaf, >> >> node and root. Do we really need such complexicity? What value does it >> >> bring to the user to expose this? >> > >> >The fact that you can't auto select from devices behind muxes. >> >> Why? What's wrong with the mechanism I described in another part of this >> thread? >> >> Extending my example from above >> >> pin 1 source >> pin 2 output >> pin 3 muxid 100 source >> pin 4 muxid 100 source >> pin 5 muxid 101 source >> pin 6 muxid 101 source >> pin 7 output >> >> User now can set individial prios for sources: >> >> dpll x pin 1 set prio 10 >> etc >> The result would be: >> >> pin 1 source prio 10 >> pin 2 output >> pin 3 muxid 100 source prio 8 >> pin 4 muxid 100 source prio 20 >> pin 5 muxid 101 source prio 50 >> pin 6 muxid 101 source prio 60 >> pin 7 output >> >> Now when auto is enabled, the pin 3 is selected. Why would user need to >> manually select between 3 and 4? This is should be abstracted out by the >> driver. >> >> Actually, this is neat as you have one cmd to do selection in manual >> mode and you have uniform way of configuring/monitoring selection in >> autosel. Would the muxed pin make this better? >> >> For muxed pin being output, only one pin from mux would be set: >> >> pin 1 source >> pin 2 output >> pin 3 muxid 100 disconnected >> pin 4 muxid 100 disconnected >> pin 5 muxid 101 output >> pin 6 muxid 101 disconnected >> pin 7 output > >Sorry, can't parse, could you draw the diagram? There is no diagram. It's a plain list of pins with attributes, one pin with attributes per line. > >To answer the most basic question - my understanding is that for >prio-based selection there needs to be silicon that can tell if >there is a valid clock on the line. While mux is just a fancy switch, >it has no complex logic, just connects wires. > >Arkadiusz, is my understanding incorrect? I may have "intuited" this. > >IDK if there are any bidirectional pins after a mux, but that'd be >another problem. Muxes are only simple for inputs. > >> >The HW topology is of material importance to user space. >> >> Interesting. When I was working on linecards, you said that the user >> does not care about the inner HW topology. And it makes sense. When >> things could be abstracted out to make iface clean, I think they should. > >I recall only the FW related conversations, but what I think is key >is whether the information can be acted upon. What I was refering to was the device/gearbox exposure per-linecard. > >> >How many times does Arkadiusz have to explain this :| >> >> Pardon my ignorance, I don't see why exactly we need mux hierarchy >> (trees) exposed to user.
On 12/8/2022 12:21 AM, Jakub Kicinski wrote: > On Wed, 7 Dec 2022 15:09:03 +0100 netdev.dump@gmail.com wrote: >>> -----Original Message----- >>> From: Jakub Kicinski <kuba@kernel.org> >> pins between the DPLLs exposed by a single driver, but not really outside of >> it. >> And that can be done simply by putting the pin ptr from the DPLLA into the >> pin >> list of DPLLB. > > Are you saying within the driver it's somehow easier? The driver state > is mostly per bus device, so I don't see how. > >> If we want the kitchen-and-sink solution, we need to think about corner >> cases. >> Which pin should the API give to the userspace app - original, or >> muxed/parent? > > IDK if I parse but I think both. If selected pin is not directly > attached the core should configure muxes. > >> How would a teardown look like - if Driver A registered DPLLA with Pin1 and >> Driver B added the muxed pin then how should Driver A properly >> release its pins? Should it just send a message to driver B and trust that >> it >> will receive it in time before we tear everything apart? > > Trivial. > >> There are many problems with that approach, and the submitted patch is not >> explaining any of them. E.g. it contains the dpll_muxed_pin_register but no >> free >> counterpart + no flows. > > SMOC. > >> If we want to get shared pins, we need a good example of how this mechanism >> can be used. > > Agreed. My main complaint about the current pins implementation is that they put everything in a single bag. In a netdev world - it would be like we put TX queues and RX queues together, named them "Queues", expose a list to the userspace and let the user figure out which ones which by reading a "TX" flag. All DPLLs I know have a Sources block, DPLLs and Output blocks. See: https://www.renesas.com/us/en/products/clocks-timing/jitter-attenuators-frequency-translation/8a34044-multichannel-dpll-dco-four-eight-channels#overview https://ww1.microchip.com/downloads/aemDocuments/documents/TIM/ProductDocuments/ProductBrief/ZL3063x-System-Synchronizers-with-up-to-5-Channels-10-Inputs-20-Outputs-Product-Brief-DS20006634.pdf https://www.sitime.com/support/resource-library/product-briefs/cascade-sit9514x-clock-system-chip-family https://www.ti.com/lit/ds/symlink/lmk5b33414.pdf?ts=1670516132647&ref_url=https%253A%252F%252Fwww.ti.com%252Fclocks-timing%252Fjitter-cleaners-synchronizers%252Fproducts.html If we model everything as "pins" we won't be able to correctly extend the API to add new features. Sources can configure the expected frequency, input signal monitoring (on multiple layers), expected signal levels, input termination and so on. Outputs will need the enable flag, signal format, frequency, phase offset etc. Multiple DPLLs can reuse a single source inside the same package simultaneously. A source should be able to link to a pin or directly to the netdev for some embedded solutions. We don't need to go through the pin abstraction at all. An optional pin entity should only represent a physical connection with a name and maybe a 3-state selection of In/Out/HiZ and then link to sources or output of the DPLL(s). Finally, the DPLL object should keep track of the source priority list, have a proper status (locked/unlocked/holdover/freerunning), implement the NCO mode, lock thresholds, bandwidths, auto-switch mode and so on. Current implementation creates a lot of ambiguity, mixes input pins with output pins and assigns priories to pins. Every SW entity will receive a big list of pins and will need to parse it. I prefer the approach that the ptp subsystem set - with its abstraction of input/output channels and pins that can be assigned to them. While not perfect - it represents reality much closer. Thanks Maciek
>From: Jakub Kicinski <kuba@kernel.org> >Sent: Wednesday, December 7, 2022 6:20 PM >On Wed, 7 Dec 2022 15:51:42 +0100 Jiri Pirko wrote: >> Wed, Dec 07, 2022 at 03:47:40AM CET, kuba@kernel.org wrote: >> >On Fri, 2 Dec 2022 17:12:06 +0100 Jiri Pirko wrote: >> >> Yep, you have the knowledge of sharing inside the driver, so you >should >> >> do it there. For multiple instances, use in-driver notifier for >example. >> > >> >No, complexity in the drivers is not a good idea. The core should cover >> >the complexity and let the drivers be simple. >> >> Really, even in case only one driver actually consumes the complexicity? >> I understand having a "libs" to do common functionality for drivers, >> even in case there is one. But this case, I don't see any benefit. > >In the same email thread you admit that mlx5 has multiple devlink >instances for the same ASIC and refuse to try to prevent similar >situation happening in the new subsystem. > >> >> There are currently 3 drivers for dpll I know of. This in ptp_ocp and >> >> mlx5 there is no concept of sharing pins. You you are talking about a >> >> single driver. >> >> >> >> What I'm trying to say is, looking at the code, the pin sharing, >> >> references and locking makes things uncomfortably complex. You are so >> >> far the only driver to need this, do it internally. If in the future >> >> other driver appears, this code would be eventually pushed into dpll >> >> core. No impact on UAPI from what I see. Please keep things as simple >as >> >> possible. >> > >> >But the pin is shared for one driver. Who cares if it's not shared in >> >another. The user space must be able to reason about the constraints. >> >> Sorry, I don't follow :/ Could you please explain what do you mean by >> this? > >We don't wait with adding APIs until there is more than one driver that >needs them. > >> >You are suggesting drivers to magically flip state in core objects >> >because of some hidden dependencies?! >> >> It's not a state flip. It's more like a well propagated event of a state >> change. The async changes may happen anyway, so the userspace needs >> to handle them. Why is this different? > >What if the user space wants conflicting configurations for the muxes >behind a shared pin? > >The fact that there is a notification does not solve the problem of >user space not knowing what's going on. Why would the user space play >guessing games if the driver _knows_ the topology and can easily tell >it. >> >> There is a big difference if we model flat list of pins with a set of >> >> attributes for each, comparing to a tree of pins, some acting as leaf, >> >> node and root. Do we really need such complexicity? What value does it >> >> bring to the user to expose this? >> > >> >The fact that you can't auto select from devices behind muxes. >> >> Why? What's wrong with the mechanism I described in another part of this >> thread? >> >> Extending my example from above >> >> pin 1 source >> pin 2 output >> pin 3 muxid 100 source >> pin 4 muxid 100 source >> pin 5 muxid 101 source >> pin 6 muxid 101 source >> pin 7 output >> >> User now can set individial prios for sources: >> >> dpll x pin 1 set prio 10 >> etc >> The result would be: >> >> pin 1 source prio 10 >> pin 2 output >> pin 3 muxid 100 source prio 8 >> pin 4 muxid 100 source prio 20 >> pin 5 muxid 101 source prio 50 >> pin 6 muxid 101 source prio 60 >> pin 7 output >> >> Now when auto is enabled, the pin 3 is selected. Why would user need to >> manually select between 3 and 4? This is should be abstracted out by the >> driver. >> >> Actually, this is neat as you have one cmd to do selection in manual >> mode and you have uniform way of configuring/monitoring selection in >> autosel. Would the muxed pin make this better? >> >> For muxed pin being output, only one pin from mux would be set: >> >> pin 1 source >> pin 2 output >> pin 3 muxid 100 disconnected >> pin 4 muxid 100 disconnected >> pin 5 muxid 101 output >> pin 6 muxid 101 disconnected >> pin 7 output > >Sorry, can't parse, could you draw the diagram? > >To answer the most basic question - my understanding is that for >prio-based selection there needs to be silicon that can tell if >there is a valid clock on the line. While mux is just a fancy switch, >it has no complex logic, just connects wires. > >Arkadiusz, is my understanding incorrect? I may have "intuited" this. > Yes, exactly. +--+ +-----------+ p8--- | p0--- | | | | -----p5 p9--- ----p1--- | | | | -----p6 p10-- | p2--- | | | | | +--+ p3--- -----p7 | | p4--- | +-----------+ Silicon is configured with priorities for each of the directly connected source pins (p0-p4, assume p5-p7 are outputs). Thus it can select highest priority and valid signal of those. Silicon is responsible to determine if the signal is present and valid for clock recovery. If so, it can lock to it. If signal is not valid, then silicon would try to lock to the next highest priority, and so on. MUX-type pin is here aggregator for additional sources, They cannot be autoselected by silicon, as they are external for silicon. If the user want to have dpll "running" on p10, it requires to select p10 and configure p1 as the highest priority pin. >IDK if there are any bidirectional pins after a mux, but that'd be >another problem. Muxes are only simple for inputs. Same here, haven't heard about such design yet. IMHO mux-pin is either source that has multiple sources connected or an output with multiple outputs. i.e. extending above with: +----+ | ----p11 | | ---p5---| ----p12 | | | ----p13 +----+ Where p11-p13 are muxed outputs. The user is able to change i.e. frequency of p11/p12/p13 for some needs, or connect/disconnect only one of it. Of course all depends on HW. Thanks, Arkadiusz > >> >The HW topology is of material importance to user space. >> >> Interesting. When I was working on linecards, you said that the user >> does not care about the inner HW topology. And it makes sense. When >> things could be abstracted out to make iface clean, I think they should. > >I recall only the FW related conversations, but what I think is key >is whether the information can be acted upon. > >> >How many times does Arkadiusz have to explain this :| >> >> Pardon my ignorance, I don't see why exactly we need mux hierarchy >> (trees) exposed to user.
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Thursday, December 8, 2022 12:59 PM > >>>From: Jiri Pirko <jiri@resnulli.us> >>>Sent: Friday, December 2, 2022 5:12 PM >>> >>>Fri, Dec 02, 2022 at 12:27:24PM CET, arkadiusz.kubalewski@intel.com >wrote: >>>>>From: Jiri Pirko <jiri@resnulli.us> >>>>>Sent: Wednesday, November 30, 2022 1:32 PM >>>>> >>>>>Tue, Nov 29, 2022 at 10:37:20PM CET, vfedorenko@novek.ru wrote: >>>>>>Implement common API for clock/DPLL configuration and status >reporting. >>>>>>The API utilises netlink interface as transport for commands and event >>>>>>notifications. This API aim to extend current pin configuration and >>>>>>make it flexible and easy to cover special configurations. >>>>> >>>>>Overall, I see a lot of issues on multiple levels. I will go over them >in >>>>>follow-up emails. So far, after couple of hours looking trought this, I >>>>>have following general feelings/notes: >>>> >>>>Hi Jiri, >>>> >>>>As we have been participating in last version, feel obligated to answer >to >>>>the concerns. >>> >>>Cool. >>> >>> >>>> >>>>> >>>>>1) Netlink interface looks much saner than in previous versions. I will >>>>> send couple of notes, mainly around events and object mixtures and >>>>> couple of bugs/redundancies. But overall, looks fineish. >>>>> >>>>>2) I don't like that concept of a shared pin, at all. It makes things >>>>> unnecessary complicated. Just have a pin created for dpll instance >>>>> and that's it. If another instance has the same pin, it should >create >>>>> it as well. Keeps things separate and easy to model. Let the >>>>> hw/fw/driver figure out the implementation oddities. >>>>> Why exactly you keep pushing the shared pin idea? Perhaps I'm >missing >>>>> something crucial. >>>> >>>> >>>>If the user request change on pin#0 of dpll#0, the dpll#0 knows about >the >>>>change, it reacts accordingly, and notifies the user the something has >>>changed. >>>>Which is rather simple. >>>> >>>>Now, if the dpll#1 is using the same pin (pin#0 of dpll#0), the >>>complicated >>>>part starts. First we have to assume: >>>>- it was initialized with the same description (as it should, to prevent >>>>confusing the user) >>>>- it was initialized with the same order (this is at least nice to have >>>from >>>>user POV, as pin indices are auto generated), and also in case of >multiple >>>pins >>>>being shared it would be best for the user to have exactly the same >number >>>of >>>>pins initialized, so they have same indices and initialization order >>>doesn't >>>>introduce additional confusion. >>>> >>>>Thus, one reason of shared pins was to prevent having this assumptions >>>ever. >>>>If the pin is shared, all dplls sharing a pin would have the same >>>description >>>>and pin index for a shared pin out of the box. >>>> >>>>Pin attribute changes >>>>The change on dpll#0 pin#0 impacts also dpll#1 pin#0. Notification about >>>the >>>>change shall be also requested from the driver that handles dpll#1. In >>>such >>>>case the driver has to have some dpll monitoring/notifying mechanics, >>>which at >>>>first doesn't look very hard to do, but most likely only if both dplls >are >>>>initialized and managed by a single instance of a driver/firmware. >>>> >>>>If board has 2 dplls but each one is managed by its own firmware/driver >>>>instance. User changes frequency of pin#0 signal, the driver of dpll#0 >>>must >>>>also notify driver of dpll#1 that pin#0 frequency has changed, dpll#1 >>>reacts on >>>>the change, notifies the user. >>> >>>Right. >>> >>> >>>>But this is only doable with assumption, that the board is internally >>>capable >>>>of such internal board level communication, which in case of separated >>>>firmwares handling multiple dplls might not be the case, or it would >>>require >>>>to have some other sw component feel that gap. >>> >>>Yep, you have the knowledge of sharing inside the driver, so you should >>>do it there. For multiple instances, use in-driver notifier for example. >>> >> >>This can be done right now, if driver doesn't need built-in pin sharing or >>doesn't want it, it doesn't have to use it. >>It was designed this way for a reason, reason of reduced complexity of >drivers, >>why should we want to increase complexity of multiple objects instead of >having >>it in one place? > >Nevermind, don't want to repeat myself. Looks like everyone wants this >pin sharing infra in the core, let's now focus on designing it properly. > > >> >>> >>>> >>>>For complex boards with multiple dplls/sync channels, multiple ports, >>>>multiple firmware instances, it seems to be complicated to share a pin >if >>>>each driver would have own copy and should notify all the other about >>>changes. >>>> >>>>To summarize, that is certainly true, shared pins idea complicates stuff >>>>inside of dpll subsystem. >>>>But at the same time it removes complexity from all the drivers which >>>would use >>> >>>There are currently 3 drivers for dpll I know of. This in ptp_ocp and >>>mlx5 there is no concept of sharing pins. You you are talking about a >>>single driver. >>> >>>What I'm trying to say is, looking at the code, the pin sharing, >>>references and locking makes things uncomfortably complex. You are so >>>far the only driver to need this, do it internally. If in the future >>>other driver appears, this code would be eventually pushed into dpll >>>core. No impact on UAPI from what I see. Please keep things as simple as >>>possible. >>> >> >>The part of interface for drivers is right now 17 functions in total, thus >I >>wouldn't call it complicated. References between dpll instances and pins, >are >>other part of "increased" complexity, but I wouldn't either call it >>over-complicated, besides it is part of dpll-core. Any interface shouldn't >make >>the user to look into the code. For users only documentation shall be >important. >>If kernel module developer is able to read header and understand what he >needs >>to do for his hardware, whether he want to use shared pins or not, and >what >>impact would it have. >> >>Thus real question is, if it is easy enough to use both parts. >>I would say the kernel module part is easy to understand even by looking >at >>the function names and their arguments. Proper documentation would clear >>anything that is still not clear. >> >>Netlink part is also moving in a right direction. >> >>> >>>>it and is easier for the userspace due to common identification of pins. >>> >>>By identification, you mean "description" right? I see no problem of 2 >>>instances have the same pin "description"/label. >>> >> >>"description"/label and index as they are the same pin. Index is auto- >generated >>and depends on initialization order, this makes unnecessary dependency. >> >>> >>>>This solution scales up without any additional complexity in the driver, >>>>and without any need for internal per-board communication channels. >>>> >>>>Not sure if this is good or bad, but with current version, both >approaches >>>are >>>>possible, so it pretty much depending on the driver to initialize dplls >>>with >>>>separated pin objects as you have suggested (and take its complexity >into >>>>driver) or just share them. >>>> >>>>> >>>>>3) I don't like the concept of muxed pins and hierarchies of pins. Why >>>>> does user care? If pin is muxed, the rest of the pins related to >this >>>>> one should be in state disabled/disconnected. The user only cares >>>>> about to see which pins are related to each other. It can be easily >>>>> exposed by "muxid" like this: >>>>> pin 1 >>>>> pin 2 >>>>> pin 3 muxid 100 >>>>> pin 4 muxid 100 >>>>> pin 5 muxid 101 >>>>> pin 6 muxid 101 >>>>> In this example pins 3,4 and 5,6 are muxed, therefore the user knows >>>>> if he connects one, the other one gets disconnected (or will have to >>>>> disconnect the first one explicitly first). >>>>> >>>> >>>>Currently DPLLA_PIN_PARENT_IDX is doing the same thing as you described, >>>it >>>>groups MUXed pins, the parent pin index here was most straightforward to >>>me, >>> >>>There is a big difference if we model flat list of pins with a set of >>>attributes for each, comparing to a tree of pins, some acting as leaf, >>>node and root. Do we really need such complexicity? What value does it >>>bring to the user to expose this? >>> >> >>If the one doesn't need to use muxed pins, everything is simple, as you >have >>described. In fact in the example you have given, only benefit I can see >from >>muxid is that user knows which pins would get disconnected when one of the >mux >>group pins is selected. But do user really needs that knowledge? If one >pin >>was selected why should he look on the other pins? > >Right. That would be probably valuable only for output pins. > > >> >>Anyway I see your point, but this is not only about identifying a muxed >pins, >>it is more about identifying connections between them because of hardware >>capabilities they can bring, and utilization of such capabilities. >>Please take a look on a comment below. >> >>> >>>>as in case of DPLL_MODE_AUTOMATIC, where dpll auto-selects highest >>>priority >>>>available signal. The priority can be only assigned to the pins directly >>>>connected to the dpll. The rest of pins (which would have present >>>>attribute DPLLA_PIN_PARENT_IDX) are the ones that require manual >selection >>>>even if DPLL_MODE_AUTOMATIC is enabled. >>>> >>>>Enabling a particular pin and sub-pin in DPLL_MODE_AUTOMATIC requires >from >>>user >>>>to select proper priority on on a dpll-level MUX-pin and manually select >>>one of >>>>the sub-pins. >>>>On the other hand for DPLL_MODE_FORCED, this might be also beneficial, >as >>>the >>>>user could select a directly connected pin and muxed pin with two >>>separated >>>>commands, which could be handled in separated driver instances (if HW >>>design >>>>requires such approach) or either it can be handled just by one select >>>call >>>>for the pin connected directly and handled entirely in the one driver >>>instance. >>> >>>Talk netlink please. What are the actual commands with cmds used to >>>select the source and select a mux pin? You are talking about 2 types of >>>selections: >>>1) Source select >>> - this is as you described either auto/forces (manual) mode, >>> according to some prio, dpll select the best source >>>2) Pin select in a mux >>> - here the pin could be source or output >>> >>>But again, from the user perspective, why does he have to distinguish >>>these? Extending my example: >>> >>> pin 1 source >>> pin 2 output >>> pin 3 muxid 100 source >>> pin 4 muxid 100 source >>> pin 5 muxid 101 source >>> pin 6 muxid 101 source >>> pin 7 output >>> >>>User now can set individial prios for sources: >>> >>>dpll x pin 1 set prio 10 >>>etc >>>The result would be: >>> >>> pin 1 source prio 10 >>> pin 2 output >>> pin 3 muxid 100 source prio 8 >>> pin 4 muxid 100 source prio 20 >>> pin 5 muxid 101 source prio 50 >>> pin 6 muxid 101 source prio 60 >>> pin 7 output >>> >>>Now when auto is enabled, the pin 3 is selected. Why would user need to >>>manually select between 3 and 4? This is should be abstracted out by the >>>driver. >> >>Yes, this could be abstracted in the driver, but we are talking here about >>different things. We need automatic hardware-supported selection of a pin, >>not automatic software-supported selection, where driver (part of software >>stack) is doing autoselection. >> >>If dpll is capable of hw-autoselection, the priorities are configured in >the >>hardware, not in the driver. This interface is a proxy between user and >dpll. > >Okay, this is very nice example, where the confusion appeared. Looking >into your code, function dpll_pin_attr_from_nlattr(), it clearly allows >userspace to set the prio over DPLLA_PIN_PRIO. Yet you claim the prio is >configured in the hardware. > Yes, userspace is requesting this configuration in hardware. It doesn't change anything, the hw-supported auto-selection works only on the source pins connected directly. Mux-type pin can have priority assigned by the user and still need to select one of multiple pins associated with it. > >>The one could use it as you have described by implementing auto-selection >>in SW, but those are two different modes, actually we could introduce >modes >>like: DPLL_MODE_HW_AUTOMATIC and DPLL_MODE_SW_AUTOMATIC, to clear things >up. >>Although, IMHO, DPLL_MODE_SW_AUTOMATIC wouldn't really differ from current >>behavior of DPLL_MODE_FORCED, where the userspace is explicitly selecting >a >>source, and the only difference would be that selection comes from driver >>instead of userspace tool. Thus in the end it would just increase >complexity >>of a driver (instead of bringing any benefits). > >Yeah, does not make sense to have DPLL_MODE_SW_AUTOMATIC as you describe >it. I agree. > >> >>> >>>Only issues I see when pins are output. User would have to somehow >>>select one of the pins in the mux (perhaps another dpll cmd). But the >>>mux pin instance does not help with anything there... >>> >>> >>> >>>> >>>>>4) I don't like the "attr" indirection. It makes things very tangled. >It >>>>> comes from the concepts of classes and objects and takes it to >>>>> extreme. Not really something we are commonly used to in kernel. >>>>> Also, it brings no value from what I can see, only makes things very >>>>> hard to read and follow. >>>>> >>>> >>>>Yet again, true, I haven't find anything similar in the kernel, it was >>>more >>>>like a try to find out a way to have a single structure with all the >stuff >>>that >>>>is passed between netlink/core/driver parts. Came up with this, and to >be >>>>honest it suits pretty well, those are well defined containers. They >store >>>>attributes that either user or driver have set, with ability to obtain a >>>valid >>>>value only if it was set. Thus whoever reads a struct, knows which of >>>those >>>>attributes were actually set. >>> >>>Sorry for being blunt here, but when I saw the code I remembered my days >>>as a student where they forced us to do similar things Java :) >>>There you tend to make things contained, everything is a class, getters, >>>setters and whatnot. In kernel, this is overkill. >>> >>>I'm not saying it's functionally wrong. What I say is that it is >>>completely unnecessary. I see no advantage, by having this indirection. >>>I see only disadvantages. It makes code harder to read and follow. >>>What I suggest, again, is to make things nice and simple. Set of ops >>>that the driver implements for dpll commands or parts of commands, >>>as we are used to in the rest of the kernel. >>> >> >>No problem, I think we will get rid of it, see comment below. >> >>> >>>>As you said, seems a bit revolutionary, but IMHO it simplifies stuff, >and >>>>basically it is value and validity bit, which I believe is rather common >>>in the >>>>kernel, this differs only with the fact it is encapsulated. No direct >>>access to >>>>the fields of structure is available for the users. >>> >>>I don't see any reason for any validity bits whan you just do it using >>>driver-implemented ops. >>> >>> >>>>Most probably there are some things that could be improved with it, but >in >>>>general it is very easy to use and understand how it works. >>>>What could be improved: >>>>- naming scheme as function names are a bit long right now, although >>>mostly >>>>still fits the line-char limits, thus not that problematic >>>>- bit mask values are capable of storing 32 bits and bit(0) is always >used >>>as >>>>unspec, which ends up with 31 values available for the enums so if by >any >>>>chance one of the attribute enums would go over 32 it could be an issue. >>>> >>>>It is especially useful for multiple values passed with the same netlink >>>>attribute id. I.e. please take a look at >>>dpll_msg_add_pin_types_supported(..) >>>>function. >>>> >>>>> Please keep things direct and simple: >>>>> * If some option could be changed for a pin or dpll, just have an >>>>> op that is directly called from netlink handler to change it. >>>>> There should be clear set of ops for configuration of pin and >>>>> dpll object. This "attr" indirection make this totally invisible. >>>> >>>>In last review you have asked to have rather only set and get ops >defined >>>>with a single attribute struct. This is exactly that, altough >>>encapsulated. >>> >>>For objects, yes. Pass a struct you directly read/write if the amount of >>>function args would be bigger then say 4. The whole encapsulation is not >>>good for anything. >> >>If there is one set/get for whole object, then a writer of a struct >(netlink >>or driver) has to have possibility to indicate which parts of the struct >were >>actually set, so a reader can do things that were requested. >> >>But I agree this is probably not any better to the "ops per attribute" >approach >>we have had before, thus I think we shall get back to this approach and >remove >>dpll_attr/dpll_pin_attr entirely. >> >>> >>> >>>> >>>>> * If some attribute is const during dpll or pin lifetime, have it >>>>> passed during dpll or pin creation. >>>>> >>>>> >>>> >>>>Only driver knows which attributes are const and which are not, this >shall >>> >>>Nonono. This is semantics defined by the subsystem. The pin >>>label/description for example. That is const, cannot be set by the user. >> >>True, it is const and it is passed on pin init. >> >>>The type of the pin (synce/gnss/ext) is const, cannot be set by the user. >> >>It can, as input/source can change, thus the same way, type of pin could >be >>managed by some hardware fuses/switches/etc. > >Could you please describe this a bit more? For example how the user can >change synce pin to ext pin? I still fail to understand how such thing >is possible. How the HW could be rewired on user request? >Perhaps we understand differently the "pin" object. I understand is as >something out-facing. Not an actual pin of a chip. > Sorry, but I don't have any real-life example of it. I don't think someone would design this as a choice between ext and synce, but hardware design may allow user such thing, more likely it would be a pin where the user selects if it is ext or gnss. I agree, you are right that this can be modeled with the mux-type pin instead of changing the pin-type. Although there is small benefit of reduced complexity of a driver, by allowing change of pin-type instead of having mux type pin and additional pins associated with it, I won't insist on any of those solutions. If there is no other objections we can change dpll_pin_type attribute to become a const pin init parameter. > >> >>>This you have to clearly specify when you define driver API. >>>This const attrs should be passed during pin creation/registration. >>> >>>Talking about dpll instance itself, the clock_id, clock_quality, these >>>should be also const attrs. >>> >> >>Actually, clock_quality can also vary on runtime (i.e. ext/synce). We >cannot >>determine what Quality Level signal user has connected to the SMA or was >>received from the network. Only gnss/oscilattor could have const depending >>on used HW. But generally it shall not be const. > >Sec. I'm talkign about the actual dpll quality, means the internal >clock. How it can vary? Yes, the DPLL has some holdover capacity, thus can translate this into QL and it shall not ever change. Sure, we could add this. I was thinking about a source Quality Level. If that would be available here, the ptp-profiles implementation would be simpler, as ptp daemon could read it and embed that information in its frames. Although, this would have to be configurable from user space, at least for EXT and SYNCE pin types. Thanks, Arkadiusz > > >> >>Thanks, >>Arkadiusz >> >>> >>> >>>>be also part of driver implementation. >>>>As I understand all the fields present in (dpll/dpll_pin)_attr, used in >>>get/set >>>>ops, could be altered in run-time depending on HW design. >>>> >>>>Thanks, >>>>Arkadiusz >>>> >>>>> >>>>>> >>>>>>v3 -> v4: >>>>>> * redesign framework to make pins dynamically allocated (Arkadiusz) >>>>>> * implement shared pins (Arkadiusz) >>>>>>v2 -> v3: >>>>>> * implement source select mode (Arkadiusz) >>>>>> * add documentation >>>>>> * implementation improvements (Jakub) >>>>>>v1 -> v2: >>>>>> * implement returning supported input/output types >>>>>> * ptp_ocp: follow suggestions from Jonathan >>>>>> * add linux-clk mailing list >>>>>>v0 -> v1: >>>>>> * fix code style and errors >>>>>> * add linux-arm mailing list >>>>>> >>>>>> >>>>>>Arkadiusz Kubalewski (1): >>>>>> dpll: add dpll_attr/dpll_pin_attr helper classes >>>>>> >>>>>>Vadim Fedorenko (3): >>>>>> dpll: Add DPLL framework base functions >>>>>> dpll: documentation on DPLL subsystem interface >>>>>> ptp_ocp: implement DPLL ops >>>>>> >>>>>> Documentation/networking/dpll.rst | 271 ++++++++ >>>>>> Documentation/networking/index.rst | 1 + >>>>>> MAINTAINERS | 8 + >>>>>> drivers/Kconfig | 2 + >>>>>> drivers/Makefile | 1 + >>>>>> drivers/dpll/Kconfig | 7 + >>>>>> drivers/dpll/Makefile | 11 + >>>>>> drivers/dpll/dpll_attr.c | 278 +++++++++ >>>>>> drivers/dpll/dpll_core.c | 760 +++++++++++++++++++++++ >>>>>> drivers/dpll/dpll_core.h | 176 ++++++ >>>>>> drivers/dpll/dpll_netlink.c | 963 >+++++++++++++++++++++++++++++ >>>>>> drivers/dpll/dpll_netlink.h | 24 + >>>>>> drivers/dpll/dpll_pin_attr.c | 456 ++++++++++++++ >>>>>> drivers/ptp/Kconfig | 1 + >>>>>> drivers/ptp/ptp_ocp.c | 123 ++-- >>>>>> include/linux/dpll.h | 261 ++++++++ >>>>>> include/linux/dpll_attr.h | 433 +++++++++++++ >>>>>> include/uapi/linux/dpll.h | 263 ++++++++ >>>>>> 18 files changed, 4002 insertions(+), 37 deletions(-) create mode >>>>>> 100644 Documentation/networking/dpll.rst create mode 100644 >>>>>> drivers/dpll/Kconfig create mode 100644 drivers/dpll/Makefile create >>>>>> mode 100644 drivers/dpll/dpll_attr.c create mode 100644 >>>>>> drivers/dpll/dpll_core.c create mode 100644 drivers/dpll/dpll_core.h >>>>>> create mode 100644 drivers/dpll/dpll_netlink.c create mode 100644 >>>>>> drivers/dpll/dpll_netlink.h create mode 100644 >>>>>> drivers/dpll/dpll_pin_attr.c create mode 100644 include/linux/dpll.h >>>>>> create mode 100644 include/linux/dpll_attr.h create mode 100644 >>>>>> include/uapi/linux/dpll.h >>>>>> >>>>>>-- >>>>>>2.27.0 >>>>>>
On Thu, 8 Dec 2022 12:28:51 +0100 Jiri Pirko wrote: > >I think we discussed using serial numbers. > > Can you remind it? Do you mean serial number of pin? Serial number of the ASIC, board or device. Something will have a serno, append to that your pin id of choice - et voila! > >Are you saying within the driver it's somehow easier? The driver state > >is mostly per bus device, so I don't see how. > > You can have some shared data for multiple instances in the driver code, > why not? The question is whether it's easier. Easier to ensure quality of n implementations in random drivers. Or one implementation in the core, with a lot of clever people paying attention and reviewing the code. > >> There are many problems with that approach, and the submitted patch is not > >> explaining any of them. E.g. it contains the dpll_muxed_pin_register but no > >> free > >> counterpart + no flows. > > > >SMOC. > > Care to spell this out. I guess you didn't mean "South Middlesex > Opportunity Council" :D Simple matter of coding.
>From: Jakub Kicinski <kuba@kernel.org> >Sent: Thursday, December 8, 2022 12:22 AM > >On Wed, 7 Dec 2022 15:09:03 +0100 netdev.dump@gmail.com wrote: >> > -----Original Message----- >> > From: Jakub Kicinski <kuba@kernel.org> >> > Sent: Wednesday, December 7, 2022 3:48 AM >> > Subject: Re: [RFC PATCH v4 0/4] Create common DPLL/clock configuration >API >> > >> > On Fri, 2 Dec 2022 17:12:06 +0100 Jiri Pirko wrote: >> [...] >> capable >> [...] >> require >> [...] > >Please fix line wrapping in your email client. >And add a name to your account configuration :/ > >> > > Yep, you have the knowledge of sharing inside the driver, so you >should >> > > do it there. For multiple instances, use in-driver notifier for >example. >> > >> > No, complexity in the drivers is not a good idea. The core should cover >> > the complexity and let the drivers be simple. >> >> But how does Driver A know where to connect its pin to? It makes sense to >> share > >I think we discussed using serial numbers. Right now, driver can find dpll with: struct dpll_device *dpll_device_get_by_cookie(u8 cookie[DPLL_COOKIE_LEN], enum dpll_type type, u8 idx); Where arguments would be the same as given when first instance have allocated dpll with: struct dpll_device *dpll_device_alloc(struct dpll_device_ops *ops, enum dpll_type type, const u8 cookie[DPLL_COOKIE_LEN], u8 dev_driver_idx, void *priv, struct device *parent); Which means all driver instances must know those values if they need to share dpll or pins. > >> pins between the DPLLs exposed by a single driver, but not really outside >of >> it. >> And that can be done simply by putting the pin ptr from the DPLLA into >the >> pin >> list of DPLLB. > >Are you saying within the driver it's somehow easier? The driver state >is mostly per bus device, so I don't see how. > >> If we want the kitchen-and-sink solution, we need to think about corner >> cases. >> Which pin should the API give to the userspace app - original, or >> muxed/parent? > >IDK if I parse but I think both. If selected pin is not directly >attached the core should configure muxes. If there is real need for muxed pin (hardware with support for priority based Auto-selection or some other hardware constraints), then both. As priority is set on mux-type/parent pin, but selection of muxed pin would be done manually with DPLL_CMD_DEVICE_SET and given DPLLA_SOURCE_PIN_IDX. If the hardware doesn't support priority based auto-selection, then it might be better to just add new pin into existing dpll without any mux-type pins, this way your driver would be simpler. But also possible to follow the same approach, add mux-type parent on one instance and register new pins from different instances with that parent, both are propagated to userspace app. > >> How would a teardown look like - if Driver A registered DPLLA with Pin1 >and >> Driver B added the muxed pin then how should Driver A properly >> release its pins? Should it just send a message to driver B and trust >that >> it >> will receive it in time before we tear everything apart? > >Trivial. With current version... Driver A creates dpll (as it was initialized first). Driver B: - allocates new pin - finds existing parent pin (find dpll (dpll_device_get_by_cookie), find pin (dpll_pin_get_by_description). - registers new pin with parent pin found. For dealloc Driver B shall deregister and free it's pin: dpll_pin_deregister(struct dpll_device *dpll, struct dpll_pin *pin); dpll_pin_free(struct dpll_pin *pin); This shall be done before Driver A deregisters dpll. As long as there is a reference to the Driver B registered pin in dpll created by Driver A, the dpll won't be deregistered. > >> There are many problems with that approach, and the submitted patch is >not >> explaining any of them. E.g. it contains the dpll_muxed_pin_register but >no >> free >> counterpart + no flows. > >SMOC. > _register counterpart is _deregister _alloc counterpart is _free Whichever pin-register function the one would use, it shall use dpll_pin_deregister(..) when releasing resource. >> If we want to get shared pins, we need a good example of how this >mechanism >> can be used. > >Agreed. Shall be provided in next version of patch series. > >> > > There are currently 3 drivers for dpll I know of. This in ptp_ocp and >> > > mlx5 there is no concept of sharing pins. You you are talking about a >> > > single driver. >> > > >> > > What I'm trying to say is, looking at the code, the pin sharing, >> > > references and locking makes things uncomfortably complex. You are so >> > > far the only driver to need this, do it internally. If in the future >> > > other driver appears, this code would be eventually pushed into dpll >> > > core. No impact on UAPI from what I see. Please keep things as simple >as >> > > possible. >> > >> > But the pin is shared for one driver. Who cares if it's not shared in >> > another. The user space must be able to reason about the constraints. >> > >> > You are suggesting drivers to magically flip state in core objects >> > because of some hidden dependencies?! >> >> If we want to go outside the device, we'd need some universal language >> to describe external connections - such as the devicetree. I don't see >how >> we can reliably implement inter-driver dependency otherwise. > >There's plenty examples in the tree. If we can't use serial number >directly we can compare the driver pointer + whatever you'd compare >in the driver internal solution. > >> I think this would be better served in the userspace with a board- >specific >> config file. Especially since the pins can be externally connected >anyway. > >Opinions vary, progress is not being made.
On Thu, 8 Dec 2022 13:02:09 +0100 Jiri Pirko wrote: > Wed, Dec 07, 2022 at 06:19:46PM CET, kuba@kernel.org wrote: > >On Wed, 7 Dec 2022 15:51:42 +0100 Jiri Pirko wrote: > >> Really, even in case only one driver actually consumes the complexicity? > >> I understand having a "libs" to do common functionality for drivers, > >> even in case there is one. But this case, I don't see any benefit. > > > >In the same email thread you admit that mlx5 has multiple devlink > >instances for the same ASIC and refuse to try to prevent similar > >situation happening in the new subsystem. > > I don't understand your point. In CX there is 1 clock for 2 pci PFs. I > plan to have 1 dpll instance for the clock shared. > > But how is what you write relevant to the discussion? We are talking > about: > a) 1 pin in 2 dpll instances > what I undestand you say here is to prevent: > b) 2 dpll instances for 1 clock > apples and oranges. Am I missing something? > > I'm totally against b) but that is not what we discuss here, correct? Correct, neither (a), nor (b), and as much code for "find other PFs referring to this device on the board" moved into the core as possible. AFAIU (and if I'm reading the docs Maciek linked) in case of Intel there are actually multiple DPLL instances in a single package / Si die. I presume we want those to be a separate DPLL instance each? But being part of a single die they share pins..
>From: Jakub Kicinski <kuba@kernel.org> >Sent: Friday, December 9, 2022 1:40 AM >On Thu, 8 Dec 2022 12:28:51 +0100 Jiri Pirko wrote: >> >I think we discussed using serial numbers. >> >> Can you remind it? Do you mean serial number of pin? > >Serial number of the ASIC, board or device. >Something will have a serno, append to that your pin id of choice - et >voila! Right now, driver can find dpll with: struct dpll_device *dpll_device_get_by_cookie(u8 cookie[DPLL_COOKIE_LEN], enum dpll_type type, u8 idx); Where arguments would be the same as given when first instance have allocated dpll with: struct dpll_device *dpll_device_alloc(struct dpll_device_ops *ops, enum dpll_type type, const u8 cookie[DPLL_COOKIE_LEN], u8 dev_driver_idx, void *priv, struct device *parent); Which means all driver instances must know those values if they need to share dpll or pins. Thanks, Arkadiusz > >> >Are you saying within the driver it's somehow easier? The driver >> >state is mostly per bus device, so I don't see how. >> >> You can have some shared data for multiple instances in the driver >> code, why not? > >The question is whether it's easier. >Easier to ensure quality of n implementations in random drivers. >Or one implementation in the core, with a lot of clever people paying >attention and reviewing the code. > >> >> There are many problems with that approach, and the submitted patch >> >> is not explaining any of them. E.g. it contains the >> >> dpll_muxed_pin_register but no free counterpart + no flows. >> > >> >SMOC. >> >> Care to spell this out. I guess you didn't mean "South Middlesex >> Opportunity Council" :D > >Simple matter of coding.
Fri, Dec 09, 2022 at 12:05:43AM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Thursday, December 8, 2022 12:59 PM >> >>>>From: Jiri Pirko <jiri@resnulli.us> >>>>Sent: Friday, December 2, 2022 5:12 PM >>>> >>>>Fri, Dec 02, 2022 at 12:27:24PM CET, arkadiusz.kubalewski@intel.com >>wrote: >>>>>>From: Jiri Pirko <jiri@resnulli.us> >>>>>>Sent: Wednesday, November 30, 2022 1:32 PM >>>>>> >>>>>>Tue, Nov 29, 2022 at 10:37:20PM CET, vfedorenko@novek.ru wrote: [...] >>>>This you have to clearly specify when you define driver API. >>>>This const attrs should be passed during pin creation/registration. >>>> >>>>Talking about dpll instance itself, the clock_id, clock_quality, these >>>>should be also const attrs. >>>> >>> >>>Actually, clock_quality can also vary on runtime (i.e. ext/synce). We >>cannot >>>determine what Quality Level signal user has connected to the SMA or was >>>received from the network. Only gnss/oscilattor could have const depending >>>on used HW. But generally it shall not be const. >> >>Sec. I'm talkign about the actual dpll quality, means the internal >>clock. How it can vary? > >Yes, the DPLL has some holdover capacity, thus can translate this into QL and >it shall not ever change. Sure, we could add this. > >I was thinking about a source Quality Level. If that would be available here, >the ptp-profiles implementation would be simpler, as ptp daemon could read it >and embed that information in its frames. >Although, this would have to be configurable from user space, at least for EXT >and SYNCE pin types. The kernel would serve as a holder or info shared from one daemon to another one. That does not sound correct. PTP should ask SyncE deamon directly, I believe.
Thu, Dec 08, 2022 at 07:08:04PM CET, maciek@machnikowski.net wrote: >On 12/8/2022 12:21 AM, Jakub Kicinski wrote: >> On Wed, 7 Dec 2022 15:09:03 +0100 netdev.dump@gmail.com wrote: >>>> -----Original Message----- >>>> From: Jakub Kicinski <kuba@kernel.org> >>> pins between the DPLLs exposed by a single driver, but not really outside of >>> it. >>> And that can be done simply by putting the pin ptr from the DPLLA into the >>> pin >>> list of DPLLB. >> >> Are you saying within the driver it's somehow easier? The driver state >> is mostly per bus device, so I don't see how. >> >>> If we want the kitchen-and-sink solution, we need to think about corner >>> cases. >>> Which pin should the API give to the userspace app - original, or >>> muxed/parent? >> >> IDK if I parse but I think both. If selected pin is not directly >> attached the core should configure muxes. >> >>> How would a teardown look like - if Driver A registered DPLLA with Pin1 and >>> Driver B added the muxed pin then how should Driver A properly >>> release its pins? Should it just send a message to driver B and trust that >>> it >>> will receive it in time before we tear everything apart? >> >> Trivial. >> >>> There are many problems with that approach, and the submitted patch is not >>> explaining any of them. E.g. it contains the dpll_muxed_pin_register but no >>> free >>> counterpart + no flows. >> >> SMOC. >> >>> If we want to get shared pins, we need a good example of how this mechanism >>> can be used. >> >> Agreed. > >My main complaint about the current pins implementation is that they put >everything in a single bag. In a netdev world - it would be like we put >TX queues and RX queues together, named them "Queues", expose a list to >the userspace and let the user figure out which ones which by reading a >"TX" flag. > >All DPLLs I know have a Sources block, DPLLs and Output blocks. See: > >https://www.renesas.com/us/en/products/clocks-timing/jitter-attenuators-frequency-translation/8a34044-multichannel-dpll-dco-four-eight-channels#overview > >https://ww1.microchip.com/downloads/aemDocuments/documents/TIM/ProductDocuments/ProductBrief/ZL3063x-System-Synchronizers-with-up-to-5-Channels-10-Inputs-20-Outputs-Product-Brief-DS20006634.pdf > >https://www.sitime.com/support/resource-library/product-briefs/cascade-sit9514x-clock-system-chip-family > >https://www.ti.com/lit/ds/symlink/lmk5b33414.pdf?ts=1670516132647&ref_url=https%253A%252F%252Fwww.ti.com%252Fclocks-timing%252Fjitter-cleaners-synchronizers%252Fproducts.html > >If we model everything as "pins" we won't be able to correctly extend >the API to add new features. > >Sources can configure the expected frequency, input signal monitoring >(on multiple layers), expected signal levels, input termination and so >on. Outputs will need the enable flag, signal format, frequency, phase >offset etc. Multiple DPLLs can reuse a single source inside the same >package simultaneously. Looking at the documentation of the chips, they all have mupltiple DPLLs on a die. Arkadiusz, in your proposed implementation, do you model each DPLL separatelly? If yes, then I understand the urgency of need of a shared pin. So all DPLLs sharing the pin are part of the same chip? Question: can we have an entity, that would be 1:1 mapped to the actual device/chip here? Let's call is "a synchronizer". It would contain multiple DPLLs, user-facing-sources(input_connector), user-facing-outputs(output_connector), i/o pins. An example: SYNCHRONIZER ┌───────────────────────────────────────┐ │ │ │ │ SyncE in connector │ ┌─────────┐ │ SyncE out connector ┌───┐ │in pin 1 │DPLL_1 │ out pin 1│ ┌───┐ │ ├─────────┼──────────────┤ ├──────────────┼────┤ │ │ │ │ │ │ │ │ │ └───┘ │ │ │ │ └───┘ │ │ │ │ │ ┌──┤ │ │ GNSS in connector │ │ └─────────┘ │ ┌───┐ │in pin 2 │ out pin 2│ EXT SMA connector │ ├─────────┼───────────┘ │ ┌───┐ │ │ │ ┌───────────┼────┤ │ └───┘ │ │ │ │ │ │ │ │ └───┘ │ │ │ EXT SMA connector │ │ │ ┌───┐ mux │in pin 3 ┌─────────┐ │ │ │ ├────┬────┼───────────┐ │ │ │ │ │ │ │ │ │ │DPLL_2 │ │ │ └───┘ │ │ │ │ │ │ │ │ │ └──┤ ├──┘ │ │ │ │ │ │ EXT SMA connector │ │ │ │ │ ┌───┐ │ │ │ │ │ │ ├────┘ │ └─────────┘ │ │ │ │ │ └───┘ └───────────────────────────────────────┘ Do I get that remotelly correct? synch synchronizer_register(synch) dpll_1 synchronizer_dpll_register(synch, dpll_1) dpll_2 synchronizer_dpll_register(synch, dpll_2) source_pin_1 synchronizer_pin_register(synch, source_pin_1) output_pin_1 synchronizer_pin_register(synch, output_pin_1) output_pin_2 synchronizer_pin_register(synch, output_pin_2) synch_board synchronizer_board_register(synch_board) synch synchronizer_board_sync_register(synch_board, synch) source_connector_1 synchronizer_board_connector_register(synch_board, source_connector_1, source_pin_1) output_connector_1 synchronizer_board_connector_register(synch_board, output_connector_1, output_pin_1) output_connector_2 synchronizer_board_connector_register(synch_board, output_connector_2, output_pin_2) Thinking about it a bit more, this should be probably good to describe by device tree. The synchronizer itself dplls and pins it contains have constanc geometry, according to the synchronizer device type. The Connector-pin linkages may vary according to the board. So to divide it, there should be one synchronizer driver. Then probably some other one to connect/select/mux the connectors to the synchronizer. Makes sense?
On 12/9/2022 12:07 PM, Jiri Pirko wrote: > Thu, Dec 08, 2022 at 07:08:04PM CET, maciek@machnikowski.net wrote: >> On 12/8/2022 12:21 AM, Jakub Kicinski wrote: >> My main complaint about the current pins implementation is that they put >> everything in a single bag. In a netdev world - it would be like we put >> TX queues and RX queues together, named them "Queues", expose a list to >> the userspace and let the user figure out which ones which by reading a >> "TX" flag. >> >> All DPLLs I know have a Sources block, DPLLs and Output blocks. See: >> >> https://www.renesas.com/us/en/products/clocks-timing/jitter-attenuators-frequency-translation/8a34044-multichannel-dpll-dco-four-eight-channels#overview >> >> https://ww1.microchip.com/downloads/aemDocuments/documents/TIM/ProductDocuments/ProductBrief/ZL3063x-System-Synchronizers-with-up-to-5-Channels-10-Inputs-20-Outputs-Product-Brief-DS20006634.pdf >> >> https://www.sitime.com/support/resource-library/product-briefs/cascade-sit9514x-clock-system-chip-family >> >> https://www.ti.com/lit/ds/symlink/lmk5b33414.pdf?ts=1670516132647&ref_url=https%253A%252F%252Fwww.ti.com%252Fclocks-timing%252Fjitter-cleaners-synchronizers%252Fproducts.html >> >> If we model everything as "pins" we won't be able to correctly extend >> the API to add new features. >> >> Sources can configure the expected frequency, input signal monitoring >> (on multiple layers), expected signal levels, input termination and so >> on. Outputs will need the enable flag, signal format, frequency, phase >> offset etc. Multiple DPLLs can reuse a single source inside the same >> package simultaneously. > > > Looking at the documentation of the chips, they all have mupltiple DPLLs > on a die. Arkadiusz, in your proposed implementation, do you model each > DPLL separatelly? If yes, then I understand the urgency of need of a > shared pin. So all DPLLs sharing the pin are part of the same chip? > > Question: can we have an entity, that would be 1:1 mapped to the actual > device/chip here? Let's call is "a synchronizer". It would contain > multiple DPLLs, user-facing-sources(input_connector), > user-facing-outputs(output_connector), i/o pins. > > An example: > SYNCHRONIZER > > ┌───────────────────────────────────────┐ > │ │ > │ │ > SyncE in connector │ ┌─────────┐ │ SyncE out connector > ┌───┐ │in pin 1 │DPLL_1 │ out pin 1│ ┌───┐ > │ ├─────────┼──────────────┤ ├──────────────┼────┤ │ > │ │ │ │ │ │ │ │ > └───┘ │ │ │ │ └───┘ > │ │ │ │ > │ ┌──┤ │ │ > GNSS in connector │ │ └─────────┘ │ > ┌───┐ │in pin 2 │ out pin 2│ EXT SMA connector > │ ├─────────┼───────────┘ │ ┌───┐ > │ │ │ ┌───────────┼────┤ │ > └───┘ │ │ │ │ │ > │ │ │ └───┘ > │ │ │ > EXT SMA connector │ │ │ > ┌───┐ mux │in pin 3 ┌─────────┐ │ │ > │ ├────┬────┼───────────┐ │ │ │ │ > │ │ │ │ │ │DPLL_2 │ │ │ > └───┘ │ │ │ │ │ │ │ > │ │ └──┤ ├──┘ │ > │ │ │ │ │ > EXT SMA connector │ │ │ │ │ > ┌───┐ │ │ │ │ │ > │ ├────┘ │ └─────────┘ │ > │ │ │ │ > └───┘ └───────────────────────────────────────┘ > > Do I get that remotelly correct? It looks goot, hence two corrections are needed: - all inputs can go to all DPLLs, and a single source can drive more than one DPLL - The external mux for SMA connector should not be a part of the Synchronizer subsystem - I believe there's already a separate MUX subsystem in the kernel and all external connections should be handled by a devtree or a similar concept. The only "muxing" thing that could potentially be modeled is a synchronizer output to synchronizer input relation. Some synchronizers does that internally and can use the output of one DPLL as a source for another. Also, in theory, the DPLL->output relation may change, however I assume we can skip support for that at the beginning. So something like this would be roughly correct: ┌───────────────────────────┐ │ │ ┌──┐ │ src0 ┌─────────┐ out0 │ ┌──┐ │ ├───┼────────┤ DPLL1 ├────────┼────┤ │ └──┘ │ │ │ │ └──┘ │ │ │ │ │ │ │ out1 │ ┌──┐ ┌──┐ │ src1 │ ├───┬────┼────┤ │ │ ├───┼──┬─────┤ │ │ │ └──┘ └──┘ │ │ └─────────┘ │ │ │ │ ┌───────────────┘ │ │ │ │ src_dpll1 │ │ │ │ ┌─────────┐ out2 │ ┌──┐ │ │ └─┤ DPLL2 ├────────┼────┤ │ │ │ │ │ │ └──┘ │ └─────┤ │ │ ┌──┐ │ src2 │ │ │ │ ├───┼────────┤ │ │ └──┘ │ │ │ │ │ └─────────┘ │ │ │ │ │ │ │ └───────────────────────────┘ > synch > synchronizer_register(synch) > dpll_1 > synchronizer_dpll_register(synch, dpll_1) > dpll_2 > synchronizer_dpll_register(synch, dpll_2) > source_pin_1 > synchronizer_pin_register(synch, source_pin_1) > output_pin_1 > synchronizer_pin_register(synch, output_pin_1) > output_pin_2 > synchronizer_pin_register(synch, output_pin_2) > > synch_board > synchronizer_board_register(synch_board) > synch > synchronizer_board_sync_register(synch_board, synch) > source_connector_1 > synchronizer_board_connector_register(synch_board, source_connector_1, source_pin_1) > output_connector_1 > synchronizer_board_connector_register(synch_board, output_connector_1, output_pin_1) > output_connector_2 > synchronizer_board_connector_register(synch_board, output_connector_2, output_pin_2) I'd rather not use pins at all - just stick to sources and outputs. Both can use some labels to be identifiable. > Thinking about it a bit more, this should be probably good to describe > by device tree. The synchronizer itself dplls and pins it contains > have constanc geometry, according to the synchronizer device type. > > The Connector-pin linkages may vary according to the board. > > So to divide it, there should be one synchronizer driver. Then probably > some other one to connect/select/mux the connectors to the synchronizer. Agreed - we should not model external board connections inside the synchronizer driver subsystem. -Maciek
On Fri, 9 Dec 2022 15:09:08 +0100 Maciek Machnikowski wrote: > On 12/9/2022 12:07 PM, Jiri Pirko wrote: > > Looking at the documentation of the chips, they all have mupltiple DPLLs > > on a die. Arkadiusz, in your proposed implementation, do you model each > > DPLL separatelly? If yes, then I understand the urgency of need of a > > shared pin. So all DPLLs sharing the pin are part of the same chip? > > > > Question: can we have an entity, that would be 1:1 mapped to the actual > > device/chip here? Let's call is "a synchronizer". It would contain > > multiple DPLLs, user-facing-sources(input_connector), > > user-facing-outputs(output_connector), i/o pins. > > > > An example: > > SYNCHRONIZER > > > > ┌───────────────────────────────────────┐ > > │ │ > > │ │ > > SyncE in connector │ ┌─────────┐ │ SyncE out connector > > ┌───┐ │in pin 1 │DPLL_1 │ out pin 1│ ┌───┐ > > │ ├─────────┼──────────────┤ ├──────────────┼────┤ │ > > │ │ │ │ │ │ │ │ > > └───┘ │ │ │ │ └───┘ > > │ │ │ │ > > │ ┌──┤ │ │ > > GNSS in connector │ │ └─────────┘ │ > > ┌───┐ │in pin 2 │ out pin 2│ EXT SMA connector > > │ ├─────────┼───────────┘ │ ┌───┐ > > │ │ │ ┌───────────┼────┤ │ > > └───┘ │ │ │ │ │ > > │ │ │ └───┘ > > │ │ │ > > EXT SMA connector │ │ │ > > ┌───┐ mux │in pin 3 ┌─────────┐ │ │ > > │ ├────┬────┼───────────┐ │ │ │ │ > > │ │ │ │ │ │DPLL_2 │ │ │ > > └───┘ │ │ │ │ │ │ │ > > │ │ └──┤ ├──┘ │ > > │ │ │ │ │ > > EXT SMA connector │ │ │ │ │ > > ┌───┐ │ │ │ │ │ > > │ ├────┘ │ └─────────┘ │ > > │ │ │ │ > > └───┘ └───────────────────────────────────────┘ > > > > Do I get that remotelly correct? > > It looks goot, hence two corrections are needed: > - all inputs can go to all DPLLs, and a single source can drive more > than one DPLL > - The external mux for SMA connector should not be a part of the > Synchronizer subsystem - I believe there's already a separate MUX > subsystem in the kernel and all external connections should be handled > by a devtree or a similar concept. > > The only "muxing" thing that could potentially be modeled is a > synchronizer output to synchronizer input relation. Some synchronizers > does that internally and can use the output of one DPLL as a source for > another. My experience with DT and muxes is rapidly aging, have you worked with those recently? From what I remember the muxes were really.. "embedded" and static compared to what we want here. Using DT may work nicely for defining the topology, but for config we still need a different mechanism. > > synch > > synchronizer_register(synch) > > dpll_1 > > synchronizer_dpll_register(synch, dpll_1) > > dpll_2 > > synchronizer_dpll_register(synch, dpll_2) > > source_pin_1 > > synchronizer_pin_register(synch, source_pin_1) > > output_pin_1 > > synchronizer_pin_register(synch, output_pin_1) > > output_pin_2 > > synchronizer_pin_register(synch, output_pin_2) > > > > synch_board > > synchronizer_board_register(synch_board) > > synch > > synchronizer_board_sync_register(synch_board, synch) > > source_connector_1 > > synchronizer_board_connector_register(synch_board, source_connector_1, source_pin_1) > > output_connector_1 > > synchronizer_board_connector_register(synch_board, output_connector_1, output_pin_1) > > output_connector_2 > > synchronizer_board_connector_register(synch_board, output_connector_2, output_pin_2) > > I'd rather not use pins at all - just stick to sources and outputs. Both > can use some labels to be identifiable. TBH I can't comprehend your suggestion. IIUC you want an object for a source, but my brain can't handle modeling an external object. For instance the source could be GNSS, but this is not the GNSS subsystem. We have a pin connected to GNSS, not the GNSS itself. Maybe a diagram would help?
On 12/9/2022 5:31 PM, Jakub Kicinski wrote: > On Fri, 9 Dec 2022 15:09:08 +0100 Maciek Machnikowski wrote: >> On 12/9/2022 12:07 PM, Jiri Pirko wrote: >>> Looking at the documentation of the chips, they all have mupltiple DPLLs >>> on a die. Arkadiusz, in your proposed implementation, do you model each >>> DPLL separatelly? If yes, then I understand the urgency of need of a >>> shared pin. So all DPLLs sharing the pin are part of the same chip? >>> >>> Question: can we have an entity, that would be 1:1 mapped to the actual >>> device/chip here? Let's call is "a synchronizer". It would contain >>> multiple DPLLs, user-facing-sources(input_connector), >>> user-facing-outputs(output_connector), i/o pins. >>> >>> An example: >>> SYNCHRONIZER >>> >>> ┌───────────────────────────────────────┐ >>> │ │ >>> │ │ >>> SyncE in connector │ ┌─────────┐ │ SyncE out connector >>> ┌───┐ │in pin 1 │DPLL_1 │ out pin 1│ ┌───┐ >>> │ ├─────────┼──────────────┤ ├──────────────┼────┤ │ >>> │ │ │ │ │ │ │ │ >>> └───┘ │ │ │ │ └───┘ >>> │ │ │ │ >>> │ ┌──┤ │ │ >>> GNSS in connector │ │ └─────────┘ │ >>> ┌───┐ │in pin 2 │ out pin 2│ EXT SMA connector >>> │ ├─────────┼───────────┘ │ ┌───┐ >>> │ │ │ ┌───────────┼────┤ │ >>> └───┘ │ │ │ │ │ >>> │ │ │ └───┘ >>> │ │ │ >>> EXT SMA connector │ │ │ >>> ┌───┐ mux │in pin 3 ┌─────────┐ │ │ >>> │ ├────┬────┼───────────┐ │ │ │ │ >>> │ │ │ │ │ │DPLL_2 │ │ │ >>> └───┘ │ │ │ │ │ │ │ >>> │ │ └──┤ ├──┘ │ >>> │ │ │ │ │ >>> EXT SMA connector │ │ │ │ │ >>> ┌───┐ │ │ │ │ │ >>> │ ├────┘ │ └─────────┘ │ >>> │ │ │ │ >>> └───┘ └───────────────────────────────────────┘ >>> >>> Do I get that remotelly correct? >> >> It looks goot, hence two corrections are needed: >> - all inputs can go to all DPLLs, and a single source can drive more >> than one DPLL >> - The external mux for SMA connector should not be a part of the >> Synchronizer subsystem - I believe there's already a separate MUX >> subsystem in the kernel and all external connections should be handled >> by a devtree or a similar concept. >> >> The only "muxing" thing that could potentially be modeled is a >> synchronizer output to synchronizer input relation. Some synchronizers >> does that internally and can use the output of one DPLL as a source for >> another. > > My experience with DT and muxes is rapidly aging, have you worked with > those recently? From what I remember the muxes were really.. "embedded" > and static compared to what we want here. > > Using DT may work nicely for defining the topology, but for config we > still need a different mechanism. > >>> synch >>> synchronizer_register(synch) >>> dpll_1 >>> synchronizer_dpll_register(synch, dpll_1) >>> dpll_2 >>> synchronizer_dpll_register(synch, dpll_2) >>> source_pin_1 >>> synchronizer_pin_register(synch, source_pin_1) >>> output_pin_1 >>> synchronizer_pin_register(synch, output_pin_1) >>> output_pin_2 >>> synchronizer_pin_register(synch, output_pin_2) >>> >>> synch_board >>> synchronizer_board_register(synch_board) >>> synch >>> synchronizer_board_sync_register(synch_board, synch) >>> source_connector_1 >>> synchronizer_board_connector_register(synch_board, source_connector_1, source_pin_1) >>> output_connector_1 >>> synchronizer_board_connector_register(synch_board, output_connector_1, output_pin_1) >>> output_connector_2 >>> synchronizer_board_connector_register(synch_board, output_connector_2, output_pin_2) >> >> I'd rather not use pins at all - just stick to sources and outputs. Both >> can use some labels to be identifiable. > > TBH I can't comprehend your suggestion. > IIUC you want an object for a source, but my brain can't handle > modeling an external object. For instance the source could be GNSS, > but this is not the GNSS subsystem. We have a pin connected to GNSS, > not the GNSS itself. > Maybe a diagram would help? A source is just a more generic term for a frequency signal that can be used by a DPLL. For some solutions it can represent a pin, for others (integrated) it can represent an internal connection to a different DPLL/PHY/MAC/embedded oscillator or anything else that can produce periodic signal. This object will have a subset of properties listed in a previous mail: >>>> Sources can configure the expected frequency, input signal >>>> monitoring (on multiple layers), expected signal levels, input >>>> termination and so on. Outputs will need the enable flag, signal >>>> format, frequency, phase offset etc. Multiple DPLLs can reuse a >>>> single source inside the same package simultaneously. I'm absolutely not willing to connect the GNSS subsystem there :) A "pin" is too ambiguous - especially for differential inputs.
Fri, Dec 09, 2022 at 05:31:04PM CET, kuba@kernel.org wrote: >On Fri, 9 Dec 2022 15:09:08 +0100 Maciek Machnikowski wrote: >> On 12/9/2022 12:07 PM, Jiri Pirko wrote: >> > Looking at the documentation of the chips, they all have mupltiple DPLLs >> > on a die. Arkadiusz, in your proposed implementation, do you model each >> > DPLL separatelly? If yes, then I understand the urgency of need of a >> > shared pin. So all DPLLs sharing the pin are part of the same chip? >> > >> > Question: can we have an entity, that would be 1:1 mapped to the actual >> > device/chip here? Let's call is "a synchronizer". It would contain >> > multiple DPLLs, user-facing-sources(input_connector), >> > user-facing-outputs(output_connector), i/o pins. >> > >> > An example: >> > SYNCHRONIZER >> > >> > ┌───────────────────────────────────────┐ >> > │ │ >> > │ │ >> > SyncE in connector │ ┌─────────┐ │ SyncE out connector >> > ┌───┐ │in pin 1 │DPLL_1 │ out pin 1│ ┌───┐ >> > │ ├─────────┼──────────────┤ ├──────────────┼────┤ │ >> > │ │ │ │ │ │ │ │ >> > └───┘ │ │ │ │ └───┘ >> > │ │ │ │ >> > │ ┌──┤ │ │ >> > GNSS in connector │ │ └─────────┘ │ >> > ┌───┐ │in pin 2 │ out pin 2│ EXT SMA connector >> > │ ├─────────┼───────────┘ │ ┌───┐ >> > │ │ │ ┌───────────┼────┤ │ >> > └───┘ │ │ │ │ │ >> > │ │ │ └───┘ >> > │ │ │ >> > EXT SMA connector │ │ │ >> > ┌───┐ mux │in pin 3 ┌─────────┐ │ │ >> > │ ├────┬────┼───────────┐ │ │ │ │ >> > │ │ │ │ │ │DPLL_2 │ │ │ >> > └───┘ │ │ │ │ │ │ │ >> > │ │ └──┤ ├──┘ │ >> > │ │ │ │ │ >> > EXT SMA connector │ │ │ │ │ >> > ┌───┐ │ │ │ │ │ >> > │ ├────┘ │ └─────────┘ │ >> > │ │ │ │ >> > └───┘ └───────────────────────────────────────┘ >> > >> > Do I get that remotelly correct? >> >> It looks goot, hence two corrections are needed: >> - all inputs can go to all DPLLs, and a single source can drive more >> than one DPLL >> - The external mux for SMA connector should not be a part of the >> Synchronizer subsystem - I believe there's already a separate MUX >> subsystem in the kernel and all external connections should be handled >> by a devtree or a similar concept. >> >> The only "muxing" thing that could potentially be modeled is a >> synchronizer output to synchronizer input relation. Some synchronizers >> does that internally and can use the output of one DPLL as a source for >> another. > >My experience with DT and muxes is rapidly aging, have you worked with >those recently? From what I remember the muxes were really.. "embedded" >and static compared to what we want here. Why do you think we need something "non-static"? The mux is part of the board, isn't it? That sounds quite static to me. > >Using DT may work nicely for defining the topology, but for config we >still need a different mechanism. "config" of what? Each item in topology would be configure according to the item type, won't it? [...]
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Monday, December 12, 2022 2:59 PM > >Fri, Dec 09, 2022 at 05:31:04PM CET, kuba@kernel.org wrote: >>On Fri, 9 Dec 2022 15:09:08 +0100 Maciek Machnikowski wrote: >>> On 12/9/2022 12:07 PM, Jiri Pirko wrote: >>> > Looking at the documentation of the chips, they all have mupltiple >DPLLs >>> > on a die. Arkadiusz, in your proposed implementation, do you model >each >>> > DPLL separatelly? If yes, then I understand the urgency of need of a >>> > shared pin. So all DPLLs sharing the pin are part of the same chip? >>> > >>> > Question: can we have an entity, that would be 1:1 mapped to the >actual >>> > device/chip here? Let's call is "a synchronizer". It would contain >>> > multiple DPLLs, user-facing-sources(input_connector), >>> > user-facing-outputs(output_connector), i/o pins. >>> > >>> > An example: >>> > SYNCHRONIZER >>> > >>> > >┌───────────────────────────────────────┐ >>> > │ >│ >>> > │ >│ >>> > SyncE in connector │ ┌─────────┐ >│ SyncE out connector >>> > ┌───┐ │in pin 1 │DPLL_1 │ out pin >1│ ┌───┐ >>> > │ ├─────────┼──────────────┤ >├──────────────┼────┤ │ >>> > │ │ │ │ │ >│ │ │ >>> > └───┘ │ │ │ >│ └───┘ >>> > │ │ │ >│ >>> > │ ┌──┤ │ >│ >>> > GNSS in connector │ │ └─────────┘ >│ >>> > ┌───┐ │in pin 2 │ out pin >2│ EXT SMA connector >>> > │ ├─────────┼───────────┘ >│ ┌───┐ >>> > │ │ │ >┌───────────┼────┤ │ >>> > └───┘ │ │ >│ │ │ >>> > │ │ >│ └───┘ >>> > │ │ >│ >>> > EXT SMA connector │ │ >│ >>> > ┌───┐ mux │in pin 3 ┌─────────┐ │ >│ >>> > │ ├────┬────┼───────────┐ │ │ │ >│ >>> > │ │ │ │ │ │DPLL_2 │ │ >│ >>> > └───┘ │ │ │ │ │ │ >│ >>> > │ │ └──┤ ├──┘ >│ >>> > │ │ │ │ >│ >>> > EXT SMA connector │ │ │ │ >│ >>> > ┌───┐ │ │ │ │ >│ >>> > │ ├────┘ │ └─────────┘ >│ >>> > │ │ │ >│ >>> > └───┘ >└───────────────────────────────────────┘ >>> > >>> > Do I get that remotelly correct? >>> >>> It looks goot, hence two corrections are needed: >>> - all inputs can go to all DPLLs, and a single source can drive more >>> than one DPLL >>> - The external mux for SMA connector should not be a part of the >>> Synchronizer subsystem - I believe there's already a separate MUX >>> subsystem in the kernel and all external connections should be handled >>> by a devtree or a similar concept. >>> >>> The only "muxing" thing that could potentially be modeled is a >>> synchronizer output to synchronizer input relation. Some synchronizers >>> does that internally and can use the output of one DPLL as a source for >>> another. >> >>My experience with DT and muxes is rapidly aging, have you worked with >>those recently? From what I remember the muxes were really.. "embedded" >>and static compared to what we want here. > >Why do you think we need something "non-static"? The mux is part of the >board, isn't it? That sounds quite static to me. > > >> >>Using DT may work nicely for defining the topology, but for config we >>still need a different mechanism. > >"config" of what? Each item in topology would be configure according to >the item type, won't it? > >[...] Hi guys, We have been trying to figure out feasibility of new approach proposed on our latest meeting - to have a single object which encapsulates multiple DPLLs. Please consider following example: Shared common inputs: i0 - GPS / external i1 - SMA1 / external i2 - SMA2 / external i3 - MUX0 / clk recovered from PHY0.X driven by MAC0 i4 - MUX1 / clk recovered from PHY1.X driven by MAC1 +---------------------------------------------------------+ | Channel A / FW0 +---+ | | i0--| | | | +---+ | | | | PHY0.0--| | i1--| D | | | | | | P | | | PHY0.1--| M | i2--| L | +---+ +--------+ | | | U | | L |---| |---| PHY0.0 |--| | PHY0.2--| X |-+---------i3--| 0 | | | +--------+ | | | 0 | |+------+ | |---| M |---| PHY0.1 |--| | ... --| | || MUX1 |-i4--| | | A | +--------+ | | | | |+------+ +---+ | C |---| PHY0.2 |--| | PHY0.7--| | | i0--| | | 0 | +--------+ | | +---+ | | |---| |---| ... |--| | | i1--| D | | | +--------+ | | | | P |---| |---| PHY0.7 |--| | | i2--| L | +---+ +--------+ | | | | L | | | \---------i3--| 1 | | | +------+ | | | | | MUX1 |-i4--| | | | +------+ +---+ | +---------------------------------------------------------+ | Channel B / FW1 +---+ | | i0--| | | | | | | | i1--| D | | | +---+ | P | | | PHY1.0--| | i2--| L | +---+ +--------+ | | | | +------+ | L |---| |---| PHY1.0 |--| | PHY1.1--| M | | MUX0 |-i3--| 0 | | | +--------+ | | | U | +------+ | |---| M |---| PHY1.1 |--| | PHY1.2--| X |-+---------i4--| | | A | +--------+ | | | 1 | | +---+ | C |---| PHY1.2 |--| | ... --| | | i0--| | | 1 | +--------+ | | | | | | |---| |---| ... |--| | PHY1.7--| | | i1--| D | | | +--------+ | | +---+ | | P |---| |---| PHY1.7 |--| | | i2--| L | +---+ +--------+ | | |+------+ | L | | | || MUX0 |-i3--| 1 | | | |+------+ | | | | \---------i4--| | | | +---+ | +---------------------------------------------------------+ This is a simplified network switch board example. It has 2 synchronization channels, where each channel: - provides clk to 8 PHYs driven by separated MAC chips, - controls 2 DPLLs. Basically only given FW has control over its PHYs, so also a control over it's MUX inputs. All external sources are shared between the channels. This is why we believe it is not best idea to enclose multiple DPLLs with one object: - sources are shared even if DPLLs are not a single synchronizer chip, - control over specific MUX type input shall be controllable from different driver/firmware instances. As we know the proposal of having multiple DPLLs in one object was a try to simplify currently implemented shared pins. We fully support idea of having interfaces as simple as possible, but at the same time they shall be flexible enough to serve many use cases. Right now the use case of single "synchronizer chip" is possible (2 DPLLs with shared inputs), as well as multiple synchronizer chips with shared inputs. If we would entirely get rid of sharing pins idea and instead allowed only to have multiple DPLLs in one object, we would fall back to the problem where change on one input is braking another "synchronizer chip" input. I.e. considering above scheme, user configured both channels to use SMA1 1MHz. If SMA1 input is changed to 10MHz, all DPLLs are affected, thus all using that input shall be notified, as long as that input is shared. For the drivers that have single point of control over dpll, they might just skip those requests. But if there are multiple firmware instances controlling multiple DPLLs, they would process it independently. Current implementation is the most flexible and least complex for the level of flexibility it provides. BR, Happy new year! Arkadiusz
Mon, Jan 09, 2023 at 03:43:01PM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Monday, December 12, 2022 2:59 PM >> >>Fri, Dec 09, 2022 at 05:31:04PM CET, kuba@kernel.org wrote: >>>On Fri, 9 Dec 2022 15:09:08 +0100 Maciek Machnikowski wrote: >>>> On 12/9/2022 12:07 PM, Jiri Pirko wrote: >>>> > Looking at the documentation of the chips, they all have mupltiple >>DPLLs >>>> > on a die. Arkadiusz, in your proposed implementation, do you model >>each >>>> > DPLL separatelly? If yes, then I understand the urgency of need of a >>>> > shared pin. So all DPLLs sharing the pin are part of the same chip? >>>> > >>>> > Question: can we have an entity, that would be 1:1 mapped to the >>actual >>>> > device/chip here? Let's call is "a synchronizer". It would contain >>>> > multiple DPLLs, user-facing-sources(input_connector), >>>> > user-facing-outputs(output_connector), i/o pins. >>>> > >>>> > An example: >>>> > SYNCHRONIZER >>>> > >>>> > >>┌───────────────────────────────────────┐ >>>> > │ >>│ >>>> > │ >>│ >>>> > SyncE in connector │ ┌─────────┐ >>│ SyncE out connector >>>> > ┌───┐ │in pin 1 │DPLL_1 │ out pin >>1│ ┌───┐ >>>> > │ ├─────────┼──────────────┤ >>├──────────────┼────┤ │ >>>> > │ │ │ │ │ >>│ │ │ >>>> > └───┘ │ │ │ >>│ └───┘ >>>> > │ │ │ >>│ >>>> > │ ┌──┤ │ >>│ >>>> > GNSS in connector │ │ └─────────┘ >>│ >>>> > ┌───┐ │in pin 2 │ out pin >>2│ EXT SMA connector >>>> > │ ├─────────┼───────────┘ >>│ ┌───┐ >>>> > │ │ │ >>┌───────────┼────┤ │ >>>> > └───┘ │ │ >>│ │ │ >>>> > │ │ >>│ └───┘ >>>> > │ │ >>│ >>>> > EXT SMA connector │ │ >>│ >>>> > ┌───┐ mux │in pin 3 ┌─────────┐ │ >>│ >>>> > │ ├────┬────┼───────────┐ │ │ │ >>│ >>>> > │ │ │ │ │ │DPLL_2 │ │ >>│ >>>> > └───┘ │ │ │ │ │ │ >>│ >>>> > │ │ └──┤ ├──┘ >>│ >>>> > │ │ │ │ >>│ >>>> > EXT SMA connector │ │ │ │ >>│ >>>> > ┌───┐ │ │ │ │ >>│ >>>> > │ ├────┘ │ └─────────┘ >>│ >>>> > │ │ │ >>│ >>>> > └───┘ >>└───────────────────────────────────────┘ >>>> > >>>> > Do I get that remotelly correct? >>>> >>>> It looks goot, hence two corrections are needed: >>>> - all inputs can go to all DPLLs, and a single source can drive more >>>> than one DPLL >>>> - The external mux for SMA connector should not be a part of the >>>> Synchronizer subsystem - I believe there's already a separate MUX >>>> subsystem in the kernel and all external connections should be handled >>>> by a devtree or a similar concept. >>>> >>>> The only "muxing" thing that could potentially be modeled is a >>>> synchronizer output to synchronizer input relation. Some synchronizers >>>> does that internally and can use the output of one DPLL as a source for >>>> another. >>> >>>My experience with DT and muxes is rapidly aging, have you worked with >>>those recently? From what I remember the muxes were really.. "embedded" >>>and static compared to what we want here. >> >>Why do you think we need something "non-static"? The mux is part of the >>board, isn't it? That sounds quite static to me. >> >> >>> >>>Using DT may work nicely for defining the topology, but for config we >>>still need a different mechanism. >> >>"config" of what? Each item in topology would be configure according to >>the item type, won't it? >> >>[...] > > >Hi guys, > >We have been trying to figure out feasibility of new approach proposed on our >latest meeting - to have a single object which encapsulates multiple DPLLs. > >Please consider following example: > >Shared common inputs: >i0 - GPS / external >i1 - SMA1 / external >i2 - SMA2 / external >i3 - MUX0 / clk recovered from PHY0.X driven by MAC0 >i4 - MUX1 / clk recovered from PHY1.X driven by MAC1 > >+---------------------------------------------------------+ >| Channel A / FW0 +---+ | >| i0--| | | >| +---+ | | | >| PHY0.0--| | i1--| D | | >| | | | P | | >| PHY0.1--| M | i2--| L | +---+ +--------+ | >| | U | | L |---| |---| PHY0.0 |--| >| PHY0.2--| X |-+---------i3--| 0 | | | +--------+ | >| | 0 | |+------+ | |---| M |---| PHY0.1 |--| >| ... --| | || MUX1 |-i4--| | | A | +--------+ | >| | | |+------+ +---+ | C |---| PHY0.2 |--| >| PHY0.7--| | | i0--| | | 0 | +--------+ | >| +---+ | | |---| |---| ... |--| >| | i1--| D | | | +--------+ | >| | | P |---| |---| PHY0.7 |--| >| | i2--| L | +---+ +--------+ | >| | | L | | >| \---------i3--| 1 | | >| +------+ | | | >| | MUX1 |-i4--| | | >| +------+ +---+ | >+---------------------------------------------------------+ >| Channel B / FW1 +---+ | >| i0--| | | >| | | | >| i1--| D | | >| +---+ | P | | >| PHY1.0--| | i2--| L | +---+ +--------+ | >| | | +------+ | L |---| |---| PHY1.0 |--| >| PHY1.1--| M | | MUX0 |-i3--| 0 | | | +--------+ | >| | U | +------+ | |---| M |---| PHY1.1 |--| >| PHY1.2--| X |-+---------i4--| | | A | +--------+ | >| | 1 | | +---+ | C |---| PHY1.2 |--| >| ... --| | | i0--| | | 1 | +--------+ | >| | | | | |---| |---| ... |--| >| PHY1.7--| | | i1--| D | | | +--------+ | >| +---+ | | P |---| |---| PHY1.7 |--| >| | i2--| L | +---+ +--------+ | >| |+------+ | L | | >| || MUX0 |-i3--| 1 | | >| |+------+ | | | >| \---------i4--| | | >| +---+ | >+---------------------------------------------------------+ What is "a channel" here? Are these 2 channels part of the same physival chip? Could you add the synchronizer chip/device entities to your drawing? > >This is a simplified network switch board example. >It has 2 synchronization channels, where each channel: >- provides clk to 8 PHYs driven by separated MAC chips, >- controls 2 DPLLs. > >Basically only given FW has control over its PHYs, so also a control over it's >MUX inputs. >All external sources are shared between the channels. > >This is why we believe it is not best idea to enclose multiple DPLLs with one >object: >- sources are shared even if DPLLs are not a single synchronizer chip, >- control over specific MUX type input shall be controllable from different >driver/firmware instances. > >As we know the proposal of having multiple DPLLs in one object was a try to >simplify currently implemented shared pins. We fully support idea of having >interfaces as simple as possible, but at the same time they shall be flexible >enough to serve many use cases. > >Right now the use case of single "synchronizer chip" is possible (2 DPLLs with >shared inputs), as well as multiple synchronizer chips with shared inputs. > >If we would entirely get rid of sharing pins idea and instead allowed only to >have multiple DPLLs in one object, we would fall back to the problem where >change on one input is braking another "synchronizer chip" input. >I.e. considering above scheme, user configured both channels to use SMA1 1MHz. >If SMA1 input is changed to 10MHz, all DPLLs are affected, thus all using that You say "SMA1 input *is changed*". Could you add to your drawing: 1) Who is the one triggering the change. 2) Entity that manages the SMA input and applies the configuration. >input shall be notified, as long as that input is shared. >For the drivers that have single point of control over dpll, they might just >skip those requests. But if there are multiple firmware instances controlling >multiple DPLLs, they would process it independently. > >Current implementation is the most flexible and least complex for the level of >flexibility it provides. > >BR, Happy new year! >Arkadiusz
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Monday, January 9, 2023 5:30 PM > >Mon, Jan 09, 2023 at 03:43:01PM CET, arkadiusz.kubalewski@intel.com wrote: >>>From: Jiri Pirko <jiri@resnulli.us> >>>Sent: Monday, December 12, 2022 2:59 PM >>> >>>Fri, Dec 09, 2022 at 05:31:04PM CET, kuba@kernel.org wrote: >>>>On Fri, 9 Dec 2022 15:09:08 +0100 Maciek Machnikowski wrote: >>>>> On 12/9/2022 12:07 PM, Jiri Pirko wrote: >>>>> > Looking at the documentation of the chips, they all have mupltiple >>>DPLLs >>>>> > on a die. Arkadiusz, in your proposed implementation, do you model >>>each >>>>> > DPLL separatelly? If yes, then I understand the urgency of need of a >>>>> > shared pin. So all DPLLs sharing the pin are part of the same chip? >>>>> > >>>>> > Question: can we have an entity, that would be 1:1 mapped to the >>>actual >>>>> > device/chip here? Let's call is "a synchronizer". It would contain >>>>> > multiple DPLLs, user-facing-sources(input_connector), >>>>> > user-facing-outputs(output_connector), i/o pins. >>>>> > >>>>> > An example: >>>>> > SYNCHRONIZER >>>>> > >>>>> > >>>┌───────────────────────────────────────┐ >>>>> > │ >>>│ >>>>> > │ >>>│ >>>>> > SyncE in connector │ ┌─────────┐ >>>│ SyncE out connector >>>>> > ┌───┐ │in pin 1 │DPLL_1 │ out pin >>>1│ ┌───┐ >>>>> > │ ├─────────┼──────────────┤ >>>├──────────────┼────┤ │ >>>>> > │ │ │ │ │ >>>│ │ │ >>>>> > └───┘ │ │ │ >>>│ └───┘ >>>>> > │ │ │ >>>│ >>>>> > │ ┌──┤ │ >>>│ >>>>> > GNSS in connector │ │ └─────────┘ >>>│ >>>>> > ┌───┐ │in pin 2 │ out pin >>>2│ EXT SMA connector >>>>> > │ ├─────────┼───────────┘ >>>│ ┌───┐ >>>>> > │ │ │ >>>┌───────────┼────┤ │ >>>>> > └───┘ │ │ >>>│ │ │ >>>>> > │ │ >>>│ └───┘ >>>>> > │ │ >>>│ >>>>> > EXT SMA connector │ │ >>>│ >>>>> > ┌───┐ mux │in pin 3 ┌─────────┐ │ >>>│ >>>>> > │ ├────┬────┼───────────┐ │ │ │ >>>│ >>>>> > │ │ │ │ │ │DPLL_2 │ │ >>>│ >>>>> > └───┘ │ │ │ │ │ │ >>>│ >>>>> > │ │ └──┤ ├──┘ >>>│ >>>>> > │ │ │ │ >>>│ >>>>> > EXT SMA connector │ │ │ │ >>>│ >>>>> > ┌───┐ │ │ │ │ >>>│ >>>>> > │ ├────┘ │ └─────────┘ >>>│ >>>>> > │ │ │ >>>│ >>>>> > └───┘ >>>└───────────────────────────────────────┘ >>>>> > >>>>> > Do I get that remotelly correct? >>>>> >>>>> It looks goot, hence two corrections are needed: >>>>> - all inputs can go to all DPLLs, and a single source can drive more >>>>> than one DPLL >>>>> - The external mux for SMA connector should not be a part of the >>>>> Synchronizer subsystem - I believe there's already a separate MUX >>>>> subsystem in the kernel and all external connections should be >handled >>>>> by a devtree or a similar concept. >>>>> >>>>> The only "muxing" thing that could potentially be modeled is a >>>>> synchronizer output to synchronizer input relation. Some synchronizers >>>>> does that internally and can use the output of one DPLL as a source >for >>>>> another. >>>> >>>>My experience with DT and muxes is rapidly aging, have you worked with >>>>those recently? From what I remember the muxes were really.. "embedded" >>>>and static compared to what we want here. >>> >>>Why do you think we need something "non-static"? The mux is part of the >>>board, isn't it? That sounds quite static to me. >>> >>> >>>> >>>>Using DT may work nicely for defining the topology, but for config we >>>>still need a different mechanism. >>> >>>"config" of what? Each item in topology would be configure according to >>>the item type, won't it? >>> >>>[...] >> >> >>Hi guys, >> >>We have been trying to figure out feasibility of new approach proposed on >our >>latest meeting - to have a single object which encapsulates multiple >DPLLs. >> >>Please consider following example: >> >>Shared common inputs: >>i0 - GPS / external >>i1 - SMA1 / external >>i2 - SMA2 / external >>i3 - MUX0 / clk recovered from PHY0.X driven by MAC0 >>i4 - MUX1 / clk recovered from PHY1.X driven by MAC1 >> >>+---------------------------------------------------------+ >>| Channel A / FW0 +---+ | >>| i0--| | | >>| +---+ | | | >>| PHY0.0--| | i1--| D | | >>| | | | P | | >>| PHY0.1--| M | i2--| L | +---+ +--------+ | >>| | U | | L |---| |---| PHY0.0 |--| >>| PHY0.2--| X |-+---------i3--| 0 | | | +--------+ | >>| | 0 | |+------+ | |---| M |---| PHY0.1 |--| >>| ... --| | || MUX1 |-i4--| | | A | +--------+ | >>| | | |+------+ +---+ | C |---| PHY0.2 |--| >>| PHY0.7--| | | i0--| | | 0 | +--------+ | >>| +---+ | | |---| |---| ... |--| >>| | i1--| D | | | +--------+ | >>| | | P |---| |---| PHY0.7 |--| >>| | i2--| L | +---+ +--------+ | >>| | | L | | >>| \---------i3--| 1 | | >>| +------+ | | | >>| | MUX1 |-i4--| | | >>| +------+ +---+ | >>+---------------------------------------------------------+ >>| Channel B / FW1 +---+ | >>| i0--| | | >>| | | | >>| i1--| D | | >>| +---+ | P | | >>| PHY1.0--| | i2--| L | +---+ +--------+ | >>| | | +------+ | L |---| |---| PHY1.0 |--| >>| PHY1.1--| M | | MUX0 |-i3--| 0 | | | +--------+ | >>| | U | +------+ | |---| M |---| PHY1.1 |--| >>| PHY1.2--| X |-+---------i4--| | | A | +--------+ | >>| | 1 | | +---+ | C |---| PHY1.2 |--| >>| ... --| | | i0--| | | 1 | +--------+ | >>| | | | | |---| |---| ... |--| >>| PHY1.7--| | | i1--| D | | | +--------+ | >>| +---+ | | P |---| |---| PHY1.7 |--| >>| | i2--| L | +---+ +--------+ | >>| |+------+ | L | | >>| || MUX0 |-i3--| 1 | | >>| |+------+ | | | >>| \---------i4--| | | >>| +---+ | >>+---------------------------------------------------------+ > >What is "a channel" here? Are these 2 channels part of the same physival >chip? Could you add the synchronizer chip/device entities to your drawing? > No. A "Synchronization Channel" on a switch would allow to separate groups of physical ports. Each channel/group has own "Synchronizer Chip", which is used to drive PHY clocks of that group. "Synchronizer chip" would be the 2 DPLLs on old draw, something like this: +--------------------------------------------------------------+ | Channel A / FW0 +-------------+ +---+ +--------+ | | i0--|Synchronizer0|---| |---| PHY0.0 |--| | +---+ | | | | +--------+ | | PHY0.0--| | i1--| |---| M |---| PHY0.1 |--| | | | | +-----+ | | A | +--------+ | | PHY0.1--| M | i2--| |DPLL0| | | C |---| PHY0.2 |--| | | U | | +-----+ | | 0 | +--------+ | | PHY0.2--| X |--+---i3--| +-----+ |---| |---| ... |--| | | 0 | | | |DPLL1| | | | +--------+ | | ... --| | | /-i4--| +-----+ |---| |---| PHY0.7 |--| | | | | | +-------------+ +---+ +--------+ | | PHY0.7--| | | | | | +---+ | | | +----------------|-|-------------------------------------------+ | Channel B / FW1| | +-------------+ +---+ +--------+ | | | | i0--|Synchronizer1|---| |---| PHY1.0 |--| | +---+ | | | | | | +--------+ | | PHY1.0--| | | | i1--| |---| M |---| PHY1.1 |--| | | | | | | +-----+ | | A | +--------+ | | PHY1.1--| M | | | i2--| |DPLL0| | | C |---| PHY1.2 |--| | | U | | | | +-----+ | | 1 | +--------+ | | PHY1.2--| X | \-|-i3--| +-----+ |---| |---| ... |--| | | 1 | | | |DPLL1| | | | +--------+ | | ... --| |----+-i4--| +-----+ |---| |---| PHY1.7 |--| | | | +-------------+ +---+ +--------+ | | PHY1.7--| | | | +---+ | +--------------------------------------------------------------+ Also, please keep in mind that is an example, there could be easily 4 (or more) channels wired similarly. > >> >>This is a simplified network switch board example. >>It has 2 synchronization channels, where each channel: >>- provides clk to 8 PHYs driven by separated MAC chips, >>- controls 2 DPLLs. >> >>Basically only given FW has control over its PHYs, so also a control over >it's >>MUX inputs. >>All external sources are shared between the channels. >> >>This is why we believe it is not best idea to enclose multiple DPLLs with >one >>object: >>- sources are shared even if DPLLs are not a single synchronizer chip, >>- control over specific MUX type input shall be controllable from >different >>driver/firmware instances. >> >>As we know the proposal of having multiple DPLLs in one object was a try >to >>simplify currently implemented shared pins. We fully support idea of >having >>interfaces as simple as possible, but at the same time they shall be >flexible >>enough to serve many use cases. >> >>Right now the use case of single "synchronizer chip" is possible (2 DPLLs >with >>shared inputs), as well as multiple synchronizer chips with shared inputs. >> >>If we would entirely get rid of sharing pins idea and instead allowed only >to >>have multiple DPLLs in one object, we would fall back to the problem where >>change on one input is braking another "synchronizer chip" input. >>I.e. considering above scheme, user configured both channels to use SMA1 >1MHz. >>If SMA1 input is changed to 10MHz, all DPLLs are affected, thus all using >that > >You say "SMA1 input *is changed*". Could you add to your drawing: >1) Who is the one triggering the change. >2) Entity that manages the SMA input and applies the configuration. > A user or some tool, this change requires to switch a frequency on a signal generator connected to that SMA1. Whatever would make the change is an external entity here. The draw show connections on board, don't see a point on having a external signal generator or user connected to the board :) If something is not clear, we could prepare some different draw, please just let me know what exactly we want to see. It sound like a sequence diagram? Thanks! Arkadiusz > >>input shall be notified, as long as that input is shared. >>For the drivers that have single point of control over dpll, they might >just >>skip those requests. But if there are multiple firmware instances >controlling >>multiple DPLLs, they would process it independently. >> >>Current implementation is the most flexible and least complex for the >level of >>flexibility it provides. >> >>BR, Happy new year! >>Arkadiusz
Tue, Jan 10, 2023 at 11:54:20AM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Monday, January 9, 2023 5:30 PM >> >>Mon, Jan 09, 2023 at 03:43:01PM CET, arkadiusz.kubalewski@intel.com wrote: >>>>From: Jiri Pirko <jiri@resnulli.us> >>>>Sent: Monday, December 12, 2022 2:59 PM >>>> >>>>Fri, Dec 09, 2022 at 05:31:04PM CET, kuba@kernel.org wrote: >>>>>On Fri, 9 Dec 2022 15:09:08 +0100 Maciek Machnikowski wrote: >>>>>> On 12/9/2022 12:07 PM, Jiri Pirko wrote: >>>>>> > Looking at the documentation of the chips, they all have mupltiple >>>>DPLLs >>>>>> > on a die. Arkadiusz, in your proposed implementation, do you model >>>>each >>>>>> > DPLL separatelly? If yes, then I understand the urgency of need of a >>>>>> > shared pin. So all DPLLs sharing the pin are part of the same chip? >>>>>> > >>>>>> > Question: can we have an entity, that would be 1:1 mapped to the >>>>actual >>>>>> > device/chip here? Let's call is "a synchronizer". It would contain >>>>>> > multiple DPLLs, user-facing-sources(input_connector), >>>>>> > user-facing-outputs(output_connector), i/o pins. >>>>>> > >>>>>> > An example: >>>>>> > SYNCHRONIZER >>>>>> > >>>>>> > >>>>┌───────────────────────────────────────┐ >>>>>> > │ >>>>│ >>>>>> > │ >>>>│ >>>>>> > SyncE in connector │ ┌─────────┐ >>>>│ SyncE out connector >>>>>> > ┌───┐ │in pin 1 │DPLL_1 │ out pin >>>>1│ ┌───┐ >>>>>> > │ ├─────────┼──────────────┤ >>>>├──────────────┼────┤ │ >>>>>> > │ │ │ │ │ >>>>│ │ │ >>>>>> > └───┘ │ │ │ >>>>│ └───┘ >>>>>> > │ │ │ >>>>│ >>>>>> > │ ┌──┤ │ >>>>│ >>>>>> > GNSS in connector │ │ └─────────┘ >>>>│ >>>>>> > ┌───┐ │in pin 2 │ out pin >>>>2│ EXT SMA connector >>>>>> > │ ├─────────┼───────────┘ >>>>│ ┌───┐ >>>>>> > │ │ │ >>>>┌───────────┼────┤ │ >>>>>> > └───┘ │ │ >>>>│ │ │ >>>>>> > │ │ >>>>│ └───┘ >>>>>> > │ │ >>>>│ >>>>>> > EXT SMA connector │ │ >>>>│ >>>>>> > ┌───┐ mux │in pin 3 ┌─────────┐ │ >>>>│ >>>>>> > │ ├────┬────┼───────────┐ │ │ │ >>>>│ >>>>>> > │ │ │ │ │ │DPLL_2 │ │ >>>>│ >>>>>> > └───┘ │ │ │ │ │ │ >>>>│ >>>>>> > │ │ └──┤ ├──┘ >>>>│ >>>>>> > │ │ │ │ >>>>│ >>>>>> > EXT SMA connector │ │ │ │ >>>>│ >>>>>> > ┌───┐ │ │ │ │ >>>>│ >>>>>> > │ ├────┘ │ └─────────┘ >>>>│ >>>>>> > │ │ │ >>>>│ >>>>>> > └───┘ >>>>└───────────────────────────────────────┘ >>>>>> > >>>>>> > Do I get that remotelly correct? >>>>>> >>>>>> It looks goot, hence two corrections are needed: >>>>>> - all inputs can go to all DPLLs, and a single source can drive more >>>>>> than one DPLL >>>>>> - The external mux for SMA connector should not be a part of the >>>>>> Synchronizer subsystem - I believe there's already a separate MUX >>>>>> subsystem in the kernel and all external connections should be >>handled >>>>>> by a devtree or a similar concept. >>>>>> >>>>>> The only "muxing" thing that could potentially be modeled is a >>>>>> synchronizer output to synchronizer input relation. Some synchronizers >>>>>> does that internally and can use the output of one DPLL as a source >>for >>>>>> another. >>>>> >>>>>My experience with DT and muxes is rapidly aging, have you worked with >>>>>those recently? From what I remember the muxes were really.. "embedded" >>>>>and static compared to what we want here. >>>> >>>>Why do you think we need something "non-static"? The mux is part of the >>>>board, isn't it? That sounds quite static to me. >>>> >>>> >>>>> >>>>>Using DT may work nicely for defining the topology, but for config we >>>>>still need a different mechanism. >>>> >>>>"config" of what? Each item in topology would be configure according to >>>>the item type, won't it? >>>> >>>>[...] >>> >>> >>>Hi guys, >>> >>>We have been trying to figure out feasibility of new approach proposed on >>our >>>latest meeting - to have a single object which encapsulates multiple >>DPLLs. >>> >>>Please consider following example: >>> >>>Shared common inputs: >>>i0 - GPS / external >>>i1 - SMA1 / external >>>i2 - SMA2 / external >>>i3 - MUX0 / clk recovered from PHY0.X driven by MAC0 >>>i4 - MUX1 / clk recovered from PHY1.X driven by MAC1 >>> >>>+---------------------------------------------------------+ >>>| Channel A / FW0 +---+ | >>>| i0--| | | >>>| +---+ | | | >>>| PHY0.0--| | i1--| D | | >>>| | | | P | | >>>| PHY0.1--| M | i2--| L | +---+ +--------+ | >>>| | U | | L |---| |---| PHY0.0 |--| >>>| PHY0.2--| X |-+---------i3--| 0 | | | +--------+ | >>>| | 0 | |+------+ | |---| M |---| PHY0.1 |--| >>>| ... --| | || MUX1 |-i4--| | | A | +--------+ | >>>| | | |+------+ +---+ | C |---| PHY0.2 |--| >>>| PHY0.7--| | | i0--| | | 0 | +--------+ | >>>| +---+ | | |---| |---| ... |--| >>>| | i1--| D | | | +--------+ | >>>| | | P |---| |---| PHY0.7 |--| >>>| | i2--| L | +---+ +--------+ | >>>| | | L | | >>>| \---------i3--| 1 | | >>>| +------+ | | | >>>| | MUX1 |-i4--| | | >>>| +------+ +---+ | >>>+---------------------------------------------------------+ >>>| Channel B / FW1 +---+ | >>>| i0--| | | >>>| | | | >>>| i1--| D | | >>>| +---+ | P | | >>>| PHY1.0--| | i2--| L | +---+ +--------+ | >>>| | | +------+ | L |---| |---| PHY1.0 |--| >>>| PHY1.1--| M | | MUX0 |-i3--| 0 | | | +--------+ | >>>| | U | +------+ | |---| M |---| PHY1.1 |--| >>>| PHY1.2--| X |-+---------i4--| | | A | +--------+ | >>>| | 1 | | +---+ | C |---| PHY1.2 |--| >>>| ... --| | | i0--| | | 1 | +--------+ | >>>| | | | | |---| |---| ... |--| >>>| PHY1.7--| | | i1--| D | | | +--------+ | >>>| +---+ | | P |---| |---| PHY1.7 |--| >>>| | i2--| L | +---+ +--------+ | >>>| |+------+ | L | | >>>| || MUX0 |-i3--| 1 | | >>>| |+------+ | | | >>>| \---------i4--| | | >>>| +---+ | >>>+---------------------------------------------------------+ >> >>What is "a channel" here? Are these 2 channels part of the same physival >>chip? Could you add the synchronizer chip/device entities to your drawing? >> > >No. >A "Synchronization Channel" on a switch would allow to separate groups >of physical ports. Each channel/group has own "Synchronizer Chip", which is >used to drive PHY clocks of that group. > >"Synchronizer chip" would be the 2 DPLLs on old draw, something like this: >+--------------------------------------------------------------+ >| Channel A / FW0 +-------------+ +---+ +--------+ | >| i0--|Synchronizer0|---| |---| PHY0.0 |--| >| +---+ | | | | +--------+ | >| PHY0.0--| | i1--| |---| M |---| PHY0.1 |--| >| | | | +-----+ | | A | +--------+ | >| PHY0.1--| M | i2--| |DPLL0| | | C |---| PHY0.2 |--| >| | U | | +-----+ | | 0 | +--------+ | >| PHY0.2--| X |--+---i3--| +-----+ |---| |---| ... |--| >| | 0 | | | |DPLL1| | | | +--------+ | >| ... --| | | /-i4--| +-----+ |---| |---| PHY0.7 |--| >| | | | | +-------------+ +---+ +--------+ | >| PHY0.7--| | | | | >| +---+ | | | >+----------------|-|-------------------------------------------+ >| Channel B / FW1| | +-------------+ +---+ +--------+ | >| | | i0--|Synchronizer1|---| |---| PHY1.0 |--| >| +---+ | | | | | | +--------+ | >| PHY1.0--| | | | i1--| |---| M |---| PHY1.1 |--| >| | | | | | +-----+ | | A | +--------+ | >| PHY1.1--| M | | | i2--| |DPLL0| | | C |---| PHY1.2 |--| >| | U | | | | +-----+ | | 1 | +--------+ | >| PHY1.2--| X | \-|-i3--| +-----+ |---| |---| ... |--| >| | 1 | | | |DPLL1| | | | +--------+ | >| ... --| |----+-i4--| +-----+ |---| |---| PHY1.7 |--| >| | | +-------------+ +---+ +--------+ | >| PHY1.7--| | | >| +---+ | >+--------------------------------------------------------------+ >Also, please keep in mind that is an example, there could be easily 4 >(or more) channels wired similarly. Good. So there are 2 synchronizers out there, each has a list of pins and contain multiple dplls. 1 synchronizer is 1 device. No pin sharing between devices, just between dplls inside one device. That eliminates this odd concept of flying pin. Good. > >> >>> >>>This is a simplified network switch board example. >>>It has 2 synchronization channels, where each channel: >>>- provides clk to 8 PHYs driven by separated MAC chips, >>>- controls 2 DPLLs. >>> >>>Basically only given FW has control over its PHYs, so also a control over >>it's >>>MUX inputs. >>>All external sources are shared between the channels. >>> >>>This is why we believe it is not best idea to enclose multiple DPLLs with >>one >>>object: >>>- sources are shared even if DPLLs are not a single synchronizer chip, >>>- control over specific MUX type input shall be controllable from >>different >>>driver/firmware instances. >>> >>>As we know the proposal of having multiple DPLLs in one object was a try >>to >>>simplify currently implemented shared pins. We fully support idea of >>having >>>interfaces as simple as possible, but at the same time they shall be >>flexible >>>enough to serve many use cases. >>> >>>Right now the use case of single "synchronizer chip" is possible (2 DPLLs Btw, fix your email client not to mangle the text you reply to with line breaks like this one. >>with >>>shared inputs), as well as multiple synchronizer chips with shared inputs. >>> >>>If we would entirely get rid of sharing pins idea and instead allowed only >>to >>>have multiple DPLLs in one object, we would fall back to the problem where >>>change on one input is braking another "synchronizer chip" input. >>>I.e. considering above scheme, user configured both channels to use SMA1 >>1MHz. >>>If SMA1 input is changed to 10MHz, all DPLLs are affected, thus all using >>that >> >>You say "SMA1 input *is changed*". Could you add to your drawing: >>1) Who is the one triggering the change. >>2) Entity that manages the SMA input and applies the configuration. >> > >A user or some tool, this change requires to switch a frequency on a signal >generator connected to that SMA1. Whatever would make the change is an external >entity here. The draw show connections on board, don't see a point on having a >external signal generator or user connected to the board :) > >If something is not clear, we could prepare some different draw, please just >let me know what exactly we want to see. It sound like a sequence diagram? I think I got it. The pins are shared between DPLLS within single synchronizer entity. That clears up my modeling concerns. Also it makes your code much simplier, you don't need special shared pin beast with reference counting etc. You just have a pin with synchronizer entity as owner and expose the linkage inside this synchronizer entity between individual DPLLs and pins. > >Thanks! >Arkadiusz > >> >>>input shall be notified, as long as that input is shared. >>>For the drivers that have single point of control over dpll, they might >>just >>>skip those requests. But if there are multiple firmware instances >>controlling >>>multiple DPLLs, they would process it independently. >>> >>>Current implementation is the most flexible and least complex for the >>level of >>>flexibility it provides. >>> >>>BR, Happy new year! >>>Arkadiusz
On Mon, 9 Jan 2023 14:43:01 +0000 Kubalewski, Arkadiusz wrote: > This is a simplified network switch board example. > It has 2 synchronization channels, where each channel: > - provides clk to 8 PHYs driven by separated MAC chips, > - controls 2 DPLLs. > > Basically only given FW has control over its PHYs, so also a control over it's > MUX inputs. > All external sources are shared between the channels. > > This is why we believe it is not best idea to enclose multiple DPLLs with one > object: > - sources are shared even if DPLLs are not a single synchronizer chip, > - control over specific MUX type input shall be controllable from different > driver/firmware instances. > > As we know the proposal of having multiple DPLLs in one object was a try to > simplify currently implemented shared pins. We fully support idea of having > interfaces as simple as possible, but at the same time they shall be flexible > enough to serve many use cases. I must be missing context from other discussions but what is this proposal trying to solve? Well implemented shared pins is all we need.
Tue, Jan 10, 2023 at 09:05:49PM CET, kuba@kernel.org wrote: >On Mon, 9 Jan 2023 14:43:01 +0000 Kubalewski, Arkadiusz wrote: >> This is a simplified network switch board example. >> It has 2 synchronization channels, where each channel: >> - provides clk to 8 PHYs driven by separated MAC chips, >> - controls 2 DPLLs. >> >> Basically only given FW has control over its PHYs, so also a control over it's >> MUX inputs. >> All external sources are shared between the channels. >> >> This is why we believe it is not best idea to enclose multiple DPLLs with one >> object: >> - sources are shared even if DPLLs are not a single synchronizer chip, >> - control over specific MUX type input shall be controllable from different >> driver/firmware instances. >> >> As we know the proposal of having multiple DPLLs in one object was a try to >> simplify currently implemented shared pins. We fully support idea of having >> interfaces as simple as possible, but at the same time they shall be flexible >> enough to serve many use cases. > >I must be missing context from other discussions but what is this >proposal trying to solve? Well implemented shared pins is all we need. There is an entity containing the pins. The synchronizer chip. One synchronizer chip contains 1-n DPLLs. The source pins are connected to each DPLL (usually). What we missed in the original model was the synchronizer entity. If we have it, we don't need any notion of somehow floating pins as independent entities being attached to one or many DPLL refcounted, etc. The synchronizer device holds them in straightforward way. Example of a synchronizer chip: https://www.renesas.com/us/en/products/clocks-timing/jitter-attenuators-frequency-translation/8a34044-multichannel-dpll-dco-four-eight-channels#overview
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Wednesday, January 11, 2023 9:20 AM > >Tue, Jan 10, 2023 at 09:05:49PM CET, kuba@kernel.org wrote: >>On Mon, 9 Jan 2023 14:43:01 +0000 Kubalewski, Arkadiusz wrote: >>> This is a simplified network switch board example. >>> It has 2 synchronization channels, where each channel: >>> - provides clk to 8 PHYs driven by separated MAC chips, >>> - controls 2 DPLLs. >>> >>> Basically only given FW has control over its PHYs, so also a control >over it's >>> MUX inputs. >>> All external sources are shared between the channels. >>> >>> This is why we believe it is not best idea to enclose multiple DPLLs >with one >>> object: >>> - sources are shared even if DPLLs are not a single synchronizer chip, >>> - control over specific MUX type input shall be controllable from >different >>> driver/firmware instances. >>> >>> As we know the proposal of having multiple DPLLs in one object was a try >to >>> simplify currently implemented shared pins. We fully support idea of >having >>> interfaces as simple as possible, but at the same time they shall be >flexible >>> enough to serve many use cases. >> >>I must be missing context from other discussions but what is this >>proposal trying to solve? Well implemented shared pins is all we need. > >There is an entity containing the pins. The synchronizer chip. One >synchronizer chip contains 1-n DPLLs. The source pins are connected >to each DPLL (usually). What we missed in the original model was the >synchronizer entity. If we have it, we don't need any notion of somehow >floating pins as independent entities being attached to one or many >DPLL refcounted, etc. The synchronizer device holds them in >straightforward way. > >Example of a synchronizer chip: >https://www.renesas.com/us/en/products/clocks-timing/jitter-attenuators- >frequency-translation/8a34044-multichannel-dpll-dco-four-eight- >channels#overview Not really, as explained above, multiple separated synchronizer chips can be connected to the same external sources. This is why I wrote this email, to better explain need for references between DPLLs and shared pins. Synchronizer chip object with multiple DPLLs would have sense if the pins would only belong to that single chip, but this is not true. As the pins are shared between multiple DPLLs (both inside 1 integrated circuit and between multiple integrated circuits), all of them shall have current state of the source or output. Pins still need to be shared same as they would be inside of one synchronizer chip. BR, Arkadiusz
>From: Maciek Machnikowski <maciek@machnikowski.net> >Sent: Tuesday, January 10, 2023 3:59 PM >To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Jiri Pirko ><jiri@resnulli.us> > >On 1/10/2023 11:54 AM, Kubalewski, Arkadiusz wrote: >>> From: Jiri Pirko <jiri@resnulli.us> >>> Sent: Monday, January 9, 2023 5:30 PM >>>> >>>> Hi guys, >>>> >>>> We have been trying to figure out feasibility of new approach proposed >on >>> our >>>> latest meeting - to have a single object which encapsulates multiple >>> DPLLs. >>>> >>>> Please consider following example: >>>> >>>> Shared common inputs: >>>> i0 - GPS / external >>>> i1 - SMA1 / external >>>> i2 - SMA2 / external >>>> i3 - MUX0 / clk recovered from PHY0.X driven by MAC0 >>>> i4 - MUX1 / clk recovered from PHY1.X driven by MAC1 >>>> >>>> +---------------------------------------------------------+ >>>> | Channel A / FW0 +---+ | >>>> | i0--| | | >>>> | +---+ | | | >>>> | PHY0.0--| | i1--| D | | >>>> | | | | P | | >>>> | PHY0.1--| M | i2--| L | +---+ +--------+ | >>>> | | U | | L |---| |---| PHY0.0 |--| >>>> | PHY0.2--| X |-+---------i3--| 0 | | | +--------+ | >>>> | | 0 | |+------+ | |---| M |---| PHY0.1 |--| >>>> | ... --| | || MUX1 |-i4--| | | A | +--------+ | >>>> | | | |+------+ +---+ | C |---| PHY0.2 |--| >>>> | PHY0.7--| | | i0--| | | 0 | +--------+ | >>>> | +---+ | | |---| |---| ... |--| >>>> | | i1--| D | | | +--------+ | >>>> | | | P |---| |---| PHY0.7 |--| >>>> | | i2--| L | +---+ +--------+ | >>>> | | | L | | >>>> | \---------i3--| 1 | | >>>> | +------+ | | | >>>> | | MUX1 |-i4--| | | >>>> | +------+ +---+ | >>>> +---------------------------------------------------------+ >>>> | Channel B / FW1 +---+ | >>>> | i0--| | | >>>> | | | | >>>> | i1--| D | | >>>> | +---+ | P | | >>>> | PHY1.0--| | i2--| L | +---+ +--------+ | >>>> | | | +------+ | L |---| |---| PHY1.0 |--| >>>> | PHY1.1--| M | | MUX0 |-i3--| 0 | | | +--------+ | >>>> | | U | +------+ | |---| M |---| PHY1.1 |--| >>>> | PHY1.2--| X |-+---------i4--| | | A | +--------+ | >>>> | | 1 | | +---+ | C |---| PHY1.2 |--| >>>> | ... --| | | i0--| | | 1 | +--------+ | >>>> | | | | | |---| |---| ... |--| >>>> | PHY1.7--| | | i1--| D | | | +--------+ | >>>> | +---+ | | P |---| |---| PHY1.7 |--| >>>> | | i2--| L | +---+ +--------+ | >>>> | |+------+ | L | | >>>> | || MUX0 |-i3--| 1 | | >>>> | |+------+ | | | >>>> | \---------i4--| | | >>>> | +---+ | >>>> +---------------------------------------------------------+ >>> >>> What is "a channel" here? Are these 2 channels part of the same physival >>> chip? Could you add the synchronizer chip/device entities to your >drawing? >>> >> >> No. >> A "Synchronization Channel" on a switch would allow to separate groups >> of physical ports. Each channel/group has own "Synchronizer Chip", which >is >> used to drive PHY clocks of that group. >> >> "Synchronizer chip" would be the 2 DPLLs on old draw, something like >this: >> +--------------------------------------------------------------+ >> | Channel A / FW0 +-------------+ +---+ +--------+ | >> | i0--|Synchronizer0|---| |---| PHY0.0 |--| >> | +---+ | | | | +--------+ | >> | PHY0.0--| | i1--| |---| M |---| PHY0.1 |--| >> | | | | +-----+ | | A | +--------+ | >> | PHY0.1--| M | i2--| |DPLL0| | | C |---| PHY0.2 |--| >> | | U | | +-----+ | | 0 | +--------+ | >> | PHY0.2--| X |--+---i3--| +-----+ |---| |---| ... |--| >> | | 0 | | | |DPLL1| | | | +--------+ | >> | ... --| | | /-i4--| +-----+ |---| |---| PHY0.7 |--| >> | | | | | +-------------+ +---+ +--------+ | >> | PHY0.7--| | | | | >> | +---+ | | | >> +----------------|-|-------------------------------------------+ >> | Channel B / FW1| | +-------------+ +---+ +--------+ | >> | | | i0--|Synchronizer1|---| |---| PHY1.0 |--| >> | +---+ | | | | | | +--------+ | >> | PHY1.0--| | | | i1--| |---| M |---| PHY1.1 |--| >> | | | | | | +-----+ | | A | +--------+ | >> | PHY1.1--| M | | | i2--| |DPLL0| | | C |---| PHY1.2 |--| >> | | U | | | | +-----+ | | 1 | +--------+ | >> | PHY1.2--| X | \-|-i3--| +-----+ |---| |---| ... |--| >> | | 1 | | | |DPLL1| | | | +--------+ | >> | ... --| |----+-i4--| +-----+ |---| |---| PHY1.7 |--| >> | | | +-------------+ +---+ +--------+ | >> | PHY1.7--| | | >> | +---+ | >> +--------------------------------------------------------------+ >> Also, please keep in mind that is an example, there could be easily 4 >> (or more) channels wired similarly. >> > > >Hi, > >This model tries to put too much into the synchronizer subsystem. The >synchronizer device should only model inputs, DPLLs and outputs. > >The PHY lane to Synchronizer input muxing should be done in the >PHY/netdev subsystem. That's why I wanted to start with the full model >to specifically address this topic. > >The netdev should have an assigned list of Synchronizer inputs that it >can recover its SyncE clocks into. It can be done by having a connection >between the synchronizer input object(s) and the netdev, just like the >netdev is connected to PHC clocks in the PHC subsystem. This is the >model I initially presented about a year ago for solving this specific >issue. > >Analogically, the netdev will be connected to a given output, however >changing anything in the physical clock configuration sounds dangerous. > >Does that sound reasonable? > >Regards >Maciek It sounds reasonable to some point. You have mentioned list of Synchronizer inputs. If there is a list of inputs it means it was created somewhere. I assume dpll subsystem? If so you would like to export that list out of dpll subsystem, thus other entities would need to find such list, then find particular source and somehow register with it. All of this was proposed as part of netdev, I don't see any benefit in having this parts separated from dpll, as only dpll would use it, right? The same behavior is now provided by the MUX type pin, enclosed within dpll subsystem. BR, Arkadiusz
On 1/11/2023 3:17 PM, Kubalewski, Arkadiusz wrote: >> From: Maciek Machnikowski <maciek@machnikowski.net> >> Sent: Tuesday, January 10, 2023 3:59 PM >> To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Jiri Pirko >> <jiri@resnulli.us> >> >> On 1/10/2023 11:54 AM, Kubalewski, Arkadiusz wrote: >>>> From: Jiri Pirko <jiri@resnulli.us> >>>> Sent: Monday, January 9, 2023 5:30 PM >>>>> >>>>> Hi guys, >>>>> >>>>> We have been trying to figure out feasibility of new approach proposed >> on >>>> our >>>>> latest meeting - to have a single object which encapsulates multiple >>>> DPLLs. >>>>> >>>>> Please consider following example: >>>>> >>>>> Shared common inputs: >>>>> i0 - GPS / external >>>>> i1 - SMA1 / external >>>>> i2 - SMA2 / external >>>>> i3 - MUX0 / clk recovered from PHY0.X driven by MAC0 >>>>> i4 - MUX1 / clk recovered from PHY1.X driven by MAC1 >>>>> >>>>> +---------------------------------------------------------+ >>>>> | Channel A / FW0 +---+ | >>>>> | i0--| | | >>>>> | +---+ | | | >>>>> | PHY0.0--| | i1--| D | | >>>>> | | | | P | | >>>>> | PHY0.1--| M | i2--| L | +---+ +--------+ | >>>>> | | U | | L |---| |---| PHY0.0 |--| >>>>> | PHY0.2--| X |-+---------i3--| 0 | | | +--------+ | >>>>> | | 0 | |+------+ | |---| M |---| PHY0.1 |--| >>>>> | ... --| | || MUX1 |-i4--| | | A | +--------+ | >>>>> | | | |+------+ +---+ | C |---| PHY0.2 |--| >>>>> | PHY0.7--| | | i0--| | | 0 | +--------+ | >>>>> | +---+ | | |---| |---| ... |--| >>>>> | | i1--| D | | | +--------+ | >>>>> | | | P |---| |---| PHY0.7 |--| >>>>> | | i2--| L | +---+ +--------+ | >>>>> | | | L | | >>>>> | \---------i3--| 1 | | >>>>> | +------+ | | | >>>>> | | MUX1 |-i4--| | | >>>>> | +------+ +---+ | >>>>> +---------------------------------------------------------+ >>>>> | Channel B / FW1 +---+ | >>>>> | i0--| | | >>>>> | | | | >>>>> | i1--| D | | >>>>> | +---+ | P | | >>>>> | PHY1.0--| | i2--| L | +---+ +--------+ | >>>>> | | | +------+ | L |---| |---| PHY1.0 |--| >>>>> | PHY1.1--| M | | MUX0 |-i3--| 0 | | | +--------+ | >>>>> | | U | +------+ | |---| M |---| PHY1.1 |--| >>>>> | PHY1.2--| X |-+---------i4--| | | A | +--------+ | >>>>> | | 1 | | +---+ | C |---| PHY1.2 |--| >>>>> | ... --| | | i0--| | | 1 | +--------+ | >>>>> | | | | | |---| |---| ... |--| >>>>> | PHY1.7--| | | i1--| D | | | +--------+ | >>>>> | +---+ | | P |---| |---| PHY1.7 |--| >>>>> | | i2--| L | +---+ +--------+ | >>>>> | |+------+ | L | | >>>>> | || MUX0 |-i3--| 1 | | >>>>> | |+------+ | | | >>>>> | \---------i4--| | | >>>>> | +---+ | >>>>> +---------------------------------------------------------+ >>>> >>>> What is "a channel" here? Are these 2 channels part of the same physival >>>> chip? Could you add the synchronizer chip/device entities to your >> drawing? >>>> >>> >>> No. >>> A "Synchronization Channel" on a switch would allow to separate groups >>> of physical ports. Each channel/group has own "Synchronizer Chip", which >> is >>> used to drive PHY clocks of that group. >>> >>> "Synchronizer chip" would be the 2 DPLLs on old draw, something like >> this: >>> +--------------------------------------------------------------+ >>> | Channel A / FW0 +-------------+ +---+ +--------+ | >>> | i0--|Synchronizer0|---| |---| PHY0.0 |--| >>> | +---+ | | | | +--------+ | >>> | PHY0.0--| | i1--| |---| M |---| PHY0.1 |--| >>> | | | | +-----+ | | A | +--------+ | >>> | PHY0.1--| M | i2--| |DPLL0| | | C |---| PHY0.2 |--| >>> | | U | | +-----+ | | 0 | +--------+ | >>> | PHY0.2--| X |--+---i3--| +-----+ |---| |---| ... |--| >>> | | 0 | | | |DPLL1| | | | +--------+ | >>> | ... --| | | /-i4--| +-----+ |---| |---| PHY0.7 |--| >>> | | | | | +-------------+ +---+ +--------+ | >>> | PHY0.7--| | | | | >>> | +---+ | | | >>> +----------------|-|-------------------------------------------+ >>> | Channel B / FW1| | +-------------+ +---+ +--------+ | >>> | | | i0--|Synchronizer1|---| |---| PHY1.0 |--| >>> | +---+ | | | | | | +--------+ | >>> | PHY1.0--| | | | i1--| |---| M |---| PHY1.1 |--| >>> | | | | | | +-----+ | | A | +--------+ | >>> | PHY1.1--| M | | | i2--| |DPLL0| | | C |---| PHY1.2 |--| >>> | | U | | | | +-----+ | | 1 | +--------+ | >>> | PHY1.2--| X | \-|-i3--| +-----+ |---| |---| ... |--| >>> | | 1 | | | |DPLL1| | | | +--------+ | >>> | ... --| |----+-i4--| +-----+ |---| |---| PHY1.7 |--| >>> | | | +-------------+ +---+ +--------+ | >>> | PHY1.7--| | | >>> | +---+ | >>> +--------------------------------------------------------------+ >>> Also, please keep in mind that is an example, there could be easily 4 >>> (or more) channels wired similarly. >>> >> >> >> Hi, >> >> This model tries to put too much into the synchronizer subsystem. The >> synchronizer device should only model inputs, DPLLs and outputs. >> >> The PHY lane to Synchronizer input muxing should be done in the >> PHY/netdev subsystem. That's why I wanted to start with the full model >> to specifically address this topic. >> >> The netdev should have an assigned list of Synchronizer inputs that it >> can recover its SyncE clocks into. It can be done by having a connection >> between the synchronizer input object(s) and the netdev, just like the >> netdev is connected to PHC clocks in the PHC subsystem. This is the >> model I initially presented about a year ago for solving this specific >> issue. >> >> Analogically, the netdev will be connected to a given output, however >> changing anything in the physical clock configuration sounds dangerous. >> >> Does that sound reasonable? >> >> Regards >> Maciek > > It sounds reasonable to some point. > You have mentioned list of Synchronizer inputs. If there is a list of inputs > it means it was created somewhere. I assume dpll subsystem? If so you would > like to export that list out of dpll subsystem, thus other entities would need > to find such list, then find particular source and somehow register with it. > All of this was proposed as part of netdev, I don't see any benefit in having > this parts separated from dpll, as only dpll would use it, right? > The same behavior is now provided by the MUX type pin, enclosed within dpll > subsystem. > > BR, > Arkadiusz The synchronizer object should expose the list of inputs that represent possible sources of a given chip. The list will be the same for all DPLLs used by the same device, so it can be a single set of sources linked to multiple DPLLs inside the package. A netdev can then point to a given input of a synchronizer that it's connected to. The phy lane->recovered clock (or directly a synchronizer input) muxing should stay in the netdev subsystem, or in the PHY driver. The reason, and benefit, of such split is when you create a board with a netdev X and a synchronizer Y that is not instantiated by the same driver. In this scenario you'd get the ice driver to instantiate connections and the DPLL vendor's driver for the synchronizer. In such case the netdev driver will simply send a netlink message to the input/source with a requested configuration, such as expected frequency, and everything from this point can be handled by a completely different driver creating clean and logical split. If we mix the phy lanes into the DPLL subsystem it'll get very challenging to add PHY lanes to the existing synchronizer exposed by a different driver. Exporting and link between the synchronizer and the netdev is still a must no matter which way we go. And IMO it's best to link netdev to synchronizer sources, as that's the most natural way. Thanks, Maciek
Wed, Jan 11, 2023 at 03:16:59PM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Wednesday, January 11, 2023 9:20 AM >> >>Tue, Jan 10, 2023 at 09:05:49PM CET, kuba@kernel.org wrote: >>>On Mon, 9 Jan 2023 14:43:01 +0000 Kubalewski, Arkadiusz wrote: >>>> This is a simplified network switch board example. >>>> It has 2 synchronization channels, where each channel: >>>> - provides clk to 8 PHYs driven by separated MAC chips, >>>> - controls 2 DPLLs. >>>> >>>> Basically only given FW has control over its PHYs, so also a control >>over it's >>>> MUX inputs. >>>> All external sources are shared between the channels. >>>> >>>> This is why we believe it is not best idea to enclose multiple DPLLs >>with one >>>> object: >>>> - sources are shared even if DPLLs are not a single synchronizer chip, >>>> - control over specific MUX type input shall be controllable from >>different >>>> driver/firmware instances. >>>> >>>> As we know the proposal of having multiple DPLLs in one object was a try >>to >>>> simplify currently implemented shared pins. We fully support idea of >>having >>>> interfaces as simple as possible, but at the same time they shall be >>flexible >>>> enough to serve many use cases. >>> >>>I must be missing context from other discussions but what is this >>>proposal trying to solve? Well implemented shared pins is all we need. >> >>There is an entity containing the pins. The synchronizer chip. One >>synchronizer chip contains 1-n DPLLs. The source pins are connected >>to each DPLL (usually). What we missed in the original model was the >>synchronizer entity. If we have it, we don't need any notion of somehow >>floating pins as independent entities being attached to one or many >>DPLL refcounted, etc. The synchronizer device holds them in >>straightforward way. >> >>Example of a synchronizer chip: >>https://www.renesas.com/us/en/products/clocks-timing/jitter-attenuators- >>frequency-translation/8a34044-multichannel-dpll-dco-four-eight- >>channels#overview > >Not really, as explained above, multiple separated synchronizer chips can be >connected to the same external sources. >This is why I wrote this email, to better explain need for references between >DPLLs and shared pins. >Synchronizer chip object with multiple DPLLs would have sense if the pins would >only belong to that single chip, but this is not true. I don't understand how it is physically possible that 2 pins belong to 2 chips. Could you draw this to me? >As the pins are shared between multiple DPLLs (both inside 1 integrated circuit >and between multiple integrated circuits), all of them shall have current state >of the source or output. >Pins still need to be shared same as they would be inside of one synchronizer >chip. Do I understand correctly that you connect one synchronizer output to the input of the second synchronizer chip? > >BR, >Arkadiusz
>From: Maciek Machnikowski <maciek@machnikowski.net> >Sent: Wednesday, January 11, 2023 3:40 PM >To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Jiri Pirko ><jiri@resnulli.us> > > >On 1/11/2023 3:17 PM, Kubalewski, Arkadiusz wrote: >>> From: Maciek Machnikowski <maciek@machnikowski.net> >>> Sent: Tuesday, January 10, 2023 3:59 PM >>> To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Jiri Pirko >>> <jiri@resnulli.us> >>> >>> On 1/10/2023 11:54 AM, Kubalewski, Arkadiusz wrote: >>>>> From: Jiri Pirko <jiri@resnulli.us> >>>>> Sent: Monday, January 9, 2023 5:30 PM >>>>>> >>>>>> Hi guys, >>>>>> >>>>>> We have been trying to figure out feasibility of new approach >proposed >>> on >>>>> our >>>>>> latest meeting - to have a single object which encapsulates multiple >>>>> DPLLs. >>>>>> >>>>>> Please consider following example: >>>>>> >>>>>> Shared common inputs: >>>>>> i0 - GPS / external >>>>>> i1 - SMA1 / external >>>>>> i2 - SMA2 / external >>>>>> i3 - MUX0 / clk recovered from PHY0.X driven by MAC0 >>>>>> i4 - MUX1 / clk recovered from PHY1.X driven by MAC1 >>>>>> >>>>>> +---------------------------------------------------------+ >>>>>> | Channel A / FW0 +---+ | >>>>>> | i0--| | | >>>>>> | +---+ | | | >>>>>> | PHY0.0--| | i1--| D | | >>>>>> | | | | P | | >>>>>> | PHY0.1--| M | i2--| L | +---+ +--------+ | >>>>>> | | U | | L |---| |---| PHY0.0 |--| >>>>>> | PHY0.2--| X |-+---------i3--| 0 | | | +--------+ | >>>>>> | | 0 | |+------+ | |---| M |---| PHY0.1 |--| >>>>>> | ... --| | || MUX1 |-i4--| | | A | +--------+ | >>>>>> | | | |+------+ +---+ | C |---| PHY0.2 |--| >>>>>> | PHY0.7--| | | i0--| | | 0 | +--------+ | >>>>>> | +---+ | | |---| |---| ... |--| >>>>>> | | i1--| D | | | +--------+ | >>>>>> | | | P |---| |---| PHY0.7 |--| >>>>>> | | i2--| L | +---+ +--------+ | >>>>>> | | | L | | >>>>>> | \---------i3--| 1 | | >>>>>> | +------+ | | | >>>>>> | | MUX1 |-i4--| | | >>>>>> | +------+ +---+ | >>>>>> +---------------------------------------------------------+ >>>>>> | Channel B / FW1 +---+ | >>>>>> | i0--| | | >>>>>> | | | | >>>>>> | i1--| D | | >>>>>> | +---+ | P | | >>>>>> | PHY1.0--| | i2--| L | +---+ +--------+ | >>>>>> | | | +------+ | L |---| |---| PHY1.0 |--| >>>>>> | PHY1.1--| M | | MUX0 |-i3--| 0 | | | +--------+ | >>>>>> | | U | +------+ | |---| M |---| PHY1.1 |--| >>>>>> | PHY1.2--| X |-+---------i4--| | | A | +--------+ | >>>>>> | | 1 | | +---+ | C |---| PHY1.2 |--| >>>>>> | ... --| | | i0--| | | 1 | +--------+ | >>>>>> | | | | | |---| |---| ... |--| >>>>>> | PHY1.7--| | | i1--| D | | | +--------+ | >>>>>> | +---+ | | P |---| |---| PHY1.7 |--| >>>>>> | | i2--| L | +---+ +--------+ | >>>>>> | |+------+ | L | | >>>>>> | || MUX0 |-i3--| 1 | | >>>>>> | |+------+ | | | >>>>>> | \---------i4--| | | >>>>>> | +---+ | >>>>>> +---------------------------------------------------------+ >>>>> >>>>> What is "a channel" here? Are these 2 channels part of the same >physival >>>>> chip? Could you add the synchronizer chip/device entities to your >>> drawing? >>>>> >>>> >>>> No. >>>> A "Synchronization Channel" on a switch would allow to separate groups >>>> of physical ports. Each channel/group has own "Synchronizer Chip", >which >>> is >>>> used to drive PHY clocks of that group. >>>> >>>> "Synchronizer chip" would be the 2 DPLLs on old draw, something like >>> this: >>>> +--------------------------------------------------------------+ >>>> | Channel A / FW0 +-------------+ +---+ +--------+ | >>>> | i0--|Synchronizer0|---| |---| PHY0.0 |--| >>>> | +---+ | | | | +--------+ | >>>> | PHY0.0--| | i1--| |---| M |---| PHY0.1 |--| >>>> | | | | +-----+ | | A | +--------+ | >>>> | PHY0.1--| M | i2--| |DPLL0| | | C |---| PHY0.2 |--| >>>> | | U | | +-----+ | | 0 | +--------+ | >>>> | PHY0.2--| X |--+---i3--| +-----+ |---| |---| ... |--| >>>> | | 0 | | | |DPLL1| | | | +--------+ | >>>> | ... --| | | /-i4--| +-----+ |---| |---| PHY0.7 |--| >>>> | | | | | +-------------+ +---+ +--------+ | >>>> | PHY0.7--| | | | | >>>> | +---+ | | | >>>> +----------------|-|-------------------------------------------+ >>>> | Channel B / FW1| | +-------------+ +---+ +--------+ | >>>> | | | i0--|Synchronizer1|---| |---| PHY1.0 |--| >>>> | +---+ | | | | | | +--------+ | >>>> | PHY1.0--| | | | i1--| |---| M |---| PHY1.1 |--| >>>> | | | | | | +-----+ | | A | +--------+ | >>>> | PHY1.1--| M | | | i2--| |DPLL0| | | C |---| PHY1.2 |--| >>>> | | U | | | | +-----+ | | 1 | +--------+ | >>>> | PHY1.2--| X | \-|-i3--| +-----+ |---| |---| ... |--| >>>> | | 1 | | | |DPLL1| | | | +--------+ | >>>> | ... --| |----+-i4--| +-----+ |---| |---| PHY1.7 |--| >>>> | | | +-------------+ +---+ +--------+ | >>>> | PHY1.7--| | | >>>> | +---+ | >>>> +--------------------------------------------------------------+ >>>> Also, please keep in mind that is an example, there could be easily 4 >>>> (or more) channels wired similarly. >>>> >>> >>> >>> Hi, >>> >>> This model tries to put too much into the synchronizer subsystem. The >>> synchronizer device should only model inputs, DPLLs and outputs. >>> >>> The PHY lane to Synchronizer input muxing should be done in the >>> PHY/netdev subsystem. That's why I wanted to start with the full model >>> to specifically address this topic. >>> >>> The netdev should have an assigned list of Synchronizer inputs that it >>> can recover its SyncE clocks into. It can be done by having a connection >>> between the synchronizer input object(s) and the netdev, just like the >>> netdev is connected to PHC clocks in the PHC subsystem. This is the >>> model I initially presented about a year ago for solving this specific >>> issue. >>> >>> Analogically, the netdev will be connected to a given output, however >>> changing anything in the physical clock configuration sounds dangerous. >>> >>> Does that sound reasonable? >>> >>> Regards >>> Maciek >> >> It sounds reasonable to some point. >> You have mentioned list of Synchronizer inputs. If there is a list of >inputs >> it means it was created somewhere. I assume dpll subsystem? If so you >would >> like to export that list out of dpll subsystem, thus other entities would >need >> to find such list, then find particular source and somehow register with >it. >> All of this was proposed as part of netdev, I don't see any benefit in >having >> this parts separated from dpll, as only dpll would use it, right? >> The same behavior is now provided by the MUX type pin, enclosed within >dpll >> subsystem. >> >> BR, >> Arkadiusz > >The synchronizer object should expose the list of inputs that represent >possible sources of a given chip. The list will be the same for all >DPLLs used by the same device, so it can be a single set of sources >linked to multiple DPLLs inside the package. A netdev can then point to >a given input of a synchronizer that it's connected to. >The phy lane->recovered clock (or directly a synchronizer input) muxing >should stay in the netdev subsystem, or in the PHY driver. > >The reason, and benefit, of such split is when you create a board with a >netdev X and a synchronizer Y that is not instantiated by the same >driver. In this scenario you'd get the ice driver to instantiate >connections and the DPLL vendor's driver for the synchronizer. In such >case the netdev driver will simply send a netlink message to the >input/source with a requested configuration, such as expected frequency, >and everything from this point can be handled by a completely different >driver creating clean and logical split. > >If we mix the phy lanes into the DPLL subsystem it'll get very >challenging to add PHY lanes to the existing synchronizer exposed by a >different driver. This is possible right now: 1. obtain a dpll object: struct dpll_device *dpll_device_get_by_clock_id(u64 clock_id, enum dpll_type type, u8 idx); 2. register new pin with muxed type pin: int dpll_muxed_pin_register(struct dpll_device *dpll, const char *parent_pin_description, struct dpll_pin *pin, struct dpll_pin_ops *ops, void *priv); To find dpll driver must know clock_id, type of dpll and its index given when dpll was registered. To register a pin, parent_pin_description of MUX type pin given on registering it with dpll device. > >Exporting and link between the synchronizer and the netdev is still a >must no matter which way we go. And IMO it's best to link netdev to >synchronizer sources, as that's the most natural way. > The link is now just information for userspace Linux network interface index in DPLLA_PIN_NETIFINDEX attribute. BR, Arkadiusz >Thanks, >Maciek
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Wednesday, January 11, 2023 4:05 PM >To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com> > >Wed, Jan 11, 2023 at 03:16:59PM CET, arkadiusz.kubalewski@intel.com wrote: >>>From: Jiri Pirko <jiri@resnulli.us> >>>Sent: Wednesday, January 11, 2023 9:20 AM >>> >>>Tue, Jan 10, 2023 at 09:05:49PM CET, kuba@kernel.org wrote: >>>>On Mon, 9 Jan 2023 14:43:01 +0000 Kubalewski, Arkadiusz wrote: >>>>> This is a simplified network switch board example. >>>>> It has 2 synchronization channels, where each channel: >>>>> - provides clk to 8 PHYs driven by separated MAC chips, >>>>> - controls 2 DPLLs. >>>>> >>>>> Basically only given FW has control over its PHYs, so also a control >>>over it's >>>>> MUX inputs. >>>>> All external sources are shared between the channels. >>>>> >>>>> This is why we believe it is not best idea to enclose multiple DPLLs >>>with one >>>>> object: >>>>> - sources are shared even if DPLLs are not a single synchronizer chip, >>>>> - control over specific MUX type input shall be controllable from >>>different >>>>> driver/firmware instances. >>>>> >>>>> As we know the proposal of having multiple DPLLs in one object was a >try >>>to >>>>> simplify currently implemented shared pins. We fully support idea of >>>having >>>>> interfaces as simple as possible, but at the same time they shall be >>>flexible >>>>> enough to serve many use cases. >>>> >>>>I must be missing context from other discussions but what is this >>>>proposal trying to solve? Well implemented shared pins is all we need. >>> >>>There is an entity containing the pins. The synchronizer chip. One >>>synchronizer chip contains 1-n DPLLs. The source pins are connected >>>to each DPLL (usually). What we missed in the original model was the >>>synchronizer entity. If we have it, we don't need any notion of somehow >>>floating pins as independent entities being attached to one or many >>>DPLL refcounted, etc. The synchronizer device holds them in >>>straightforward way. >>> >>>Example of a synchronizer chip: >>>https://www.renesas.com/us/en/products/clocks-timing/jitter-attenuators- >>>frequency-translation/8a34044-multichannel-dpll-dco-four-eight- >>>channels#overview >> >>Not really, as explained above, multiple separated synchronizer chips can >be >>connected to the same external sources. >>This is why I wrote this email, to better explain need for references >between >>DPLLs and shared pins. >>Synchronizer chip object with multiple DPLLs would have sense if the pins >would >>only belong to that single chip, but this is not true. > >I don't understand how it is physically possible that 2 pins belong to 2 >chips. Could you draw this to me? > Well, sure, I was hoping this is clear, without extra connections on the draw: +----------+ |i0 - GPS |--------------\ +----------+ | +----------+ | |i1 - SMA1 |------------\ | +----------+ | | +----------+ | | |i2 - SMA2 |----------\ | | +----------+ | | | | | | +---------------------|-|-|-------------------------------------------+ | Channel A / FW0 | | | +-------------+ +---+ +--------+ | | | | |-i0--|Synchronizer0|---| |---| PHY0.0 |--| | +---+ | | | | | | | +--------+ | | PHY0.0--| | | |---i1--| |---| M |---| PHY0.1 |--| | | | | | | | +-----+ | | A | +--------+ | | PHY0.1--| M | |-----i2--| |DPLL0| | | C |---| PHY0.2 |--| | | U | | | | | +-----+ | | 0 | +--------+ | | PHY0.2--| X |--+----------i3--| +-----+ |---| |---| ... |--| | | 0 | | | | | | |DPLL1| | | | +--------+ | | ... --| | | /--------i4--| +-----+ |---| |---| PHY0.7 |--| | | | | | | | | +-------------+ +---+ +--------+ | | PHY0.7--| | | | | | | | | +---+ | | | | | | +----------------|-|--|-|-|-------------------------------------------+ | Channel B / FW1| | | | | +-------------+ +---+ +--------+ | | | | | | \-i0--|Synchronizer1|---| |---| PHY1.0 |--| | +---+ | | | | | | | | +--------+ | | PHY1.0--| | | | | \---i1--| |---| M |---| PHY1.1 |--| | | | | | | | +-----+ | | A | +--------+ | | PHY1.1--| M | | | \-----i2--| |DPLL0| | | C |---| PHY1.2 |--| | | U | | | | +-----+ | | 1 | +--------+ | | PHY1.2--| X | \-|--------i3--| +-----+ |---| |---| ... |--| | | 1 | | | |DPLL1| | | | +--------+ | | ... --| |----+--------i4--| +-----+ |---| |---| PHY1.7 |--| | | | +-------------+ +---+ +--------+ | | PHY1.7--| | | | +---+ | +---------------------------------------------------------------------+ > >>As the pins are shared between multiple DPLLs (both inside 1 integrated >circuit >>and between multiple integrated circuits), all of them shall have current >state >>of the source or output. >>Pins still need to be shared same as they would be inside of one >synchronizer >>chip. > >Do I understand correctly that you connect one synchronizer output to >the input of the second synchronizer chip? No, I don't recall such use case. At least nothing that needs to exposed in the DPLL subsystem itself. BR, Arkadiusz > >> >>BR, >>Arkadiusz
On 1/11/2023 4:30 PM, Kubalewski, Arkadiusz wrote: >> From: Maciek Machnikowski <maciek@machnikowski.net> >> Sent: Wednesday, January 11, 2023 3:40 PM >> To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Jiri Pirko >> <jiri@resnulli.us> >> >> >> On 1/11/2023 3:17 PM, Kubalewski, Arkadiusz wrote: >>>> From: Maciek Machnikowski <maciek@machnikowski.net> >>>> Sent: Tuesday, January 10, 2023 3:59 PM >>>> To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Jiri Pirko >>>> <jiri@resnulli.us> >>>> >>>> On 1/10/2023 11:54 AM, Kubalewski, Arkadiusz wrote: >>>>>> From: Jiri Pirko <jiri@resnulli.us> >>>>>> Sent: Monday, January 9, 2023 5:30 PM >>>>>>> >>>>>>> Hi guys, >>>>>>> >>>>>>> We have been trying to figure out feasibility of new approach >> proposed >>>> on >>>>>> our >>>>>>> latest meeting - to have a single object which encapsulates multiple >>>>>> DPLLs. >>>>>>> >>>>>>> Please consider following example: >>>>>>> >>>>>>> Shared common inputs: >>>>>>> i0 - GPS / external >>>>>>> i1 - SMA1 / external >>>>>>> i2 - SMA2 / external >>>>>>> i3 - MUX0 / clk recovered from PHY0.X driven by MAC0 >>>>>>> i4 - MUX1 / clk recovered from PHY1.X driven by MAC1 >>>>>>> >>>>>>> +---------------------------------------------------------+ >>>>>>> | Channel A / FW0 +---+ | >>>>>>> | i0--| | | >>>>>>> | +---+ | | | >>>>>>> | PHY0.0--| | i1--| D | | >>>>>>> | | | | P | | >>>>>>> | PHY0.1--| M | i2--| L | +---+ +--------+ | >>>>>>> | | U | | L |---| |---| PHY0.0 |--| >>>>>>> | PHY0.2--| X |-+---------i3--| 0 | | | +--------+ | >>>>>>> | | 0 | |+------+ | |---| M |---| PHY0.1 |--| >>>>>>> | ... --| | || MUX1 |-i4--| | | A | +--------+ | >>>>>>> | | | |+------+ +---+ | C |---| PHY0.2 |--| >>>>>>> | PHY0.7--| | | i0--| | | 0 | +--------+ | >>>>>>> | +---+ | | |---| |---| ... |--| >>>>>>> | | i1--| D | | | +--------+ | >>>>>>> | | | P |---| |---| PHY0.7 |--| >>>>>>> | | i2--| L | +---+ +--------+ | >>>>>>> | | | L | | >>>>>>> | \---------i3--| 1 | | >>>>>>> | +------+ | | | >>>>>>> | | MUX1 |-i4--| | | >>>>>>> | +------+ +---+ | >>>>>>> +---------------------------------------------------------+ >>>>>>> | Channel B / FW1 +---+ | >>>>>>> | i0--| | | >>>>>>> | | | | >>>>>>> | i1--| D | | >>>>>>> | +---+ | P | | >>>>>>> | PHY1.0--| | i2--| L | +---+ +--------+ | >>>>>>> | | | +------+ | L |---| |---| PHY1.0 |--| >>>>>>> | PHY1.1--| M | | MUX0 |-i3--| 0 | | | +--------+ | >>>>>>> | | U | +------+ | |---| M |---| PHY1.1 |--| >>>>>>> | PHY1.2--| X |-+---------i4--| | | A | +--------+ | >>>>>>> | | 1 | | +---+ | C |---| PHY1.2 |--| >>>>>>> | ... --| | | i0--| | | 1 | +--------+ | >>>>>>> | | | | | |---| |---| ... |--| >>>>>>> | PHY1.7--| | | i1--| D | | | +--------+ | >>>>>>> | +---+ | | P |---| |---| PHY1.7 |--| >>>>>>> | | i2--| L | +---+ +--------+ | >>>>>>> | |+------+ | L | | >>>>>>> | || MUX0 |-i3--| 1 | | >>>>>>> | |+------+ | | | >>>>>>> | \---------i4--| | | >>>>>>> | +---+ | >>>>>>> +---------------------------------------------------------+ >>>>>> >>>>>> What is "a channel" here? Are these 2 channels part of the same >> physival >>>>>> chip? Could you add the synchronizer chip/device entities to your >>>> drawing? >>>>>> >>>>> >>>>> No. >>>>> A "Synchronization Channel" on a switch would allow to separate groups >>>>> of physical ports. Each channel/group has own "Synchronizer Chip", >> which >>>> is >>>>> used to drive PHY clocks of that group. >>>>> >>>>> "Synchronizer chip" would be the 2 DPLLs on old draw, something like >>>> this: >>>>> +--------------------------------------------------------------+ >>>>> | Channel A / FW0 +-------------+ +---+ +--------+ | >>>>> | i0--|Synchronizer0|---| |---| PHY0.0 |--| >>>>> | +---+ | | | | +--------+ | >>>>> | PHY0.0--| | i1--| |---| M |---| PHY0.1 |--| >>>>> | | | | +-----+ | | A | +--------+ | >>>>> | PHY0.1--| M | i2--| |DPLL0| | | C |---| PHY0.2 |--| >>>>> | | U | | +-----+ | | 0 | +--------+ | >>>>> | PHY0.2--| X |--+---i3--| +-----+ |---| |---| ... |--| >>>>> | | 0 | | | |DPLL1| | | | +--------+ | >>>>> | ... --| | | /-i4--| +-----+ |---| |---| PHY0.7 |--| >>>>> | | | | | +-------------+ +---+ +--------+ | >>>>> | PHY0.7--| | | | | >>>>> | +---+ | | | >>>>> +----------------|-|-------------------------------------------+ >>>>> | Channel B / FW1| | +-------------+ +---+ +--------+ | >>>>> | | | i0--|Synchronizer1|---| |---| PHY1.0 |--| >>>>> | +---+ | | | | | | +--------+ | >>>>> | PHY1.0--| | | | i1--| |---| M |---| PHY1.1 |--| >>>>> | | | | | | +-----+ | | A | +--------+ | >>>>> | PHY1.1--| M | | | i2--| |DPLL0| | | C |---| PHY1.2 |--| >>>>> | | U | | | | +-----+ | | 1 | +--------+ | >>>>> | PHY1.2--| X | \-|-i3--| +-----+ |---| |---| ... |--| >>>>> | | 1 | | | |DPLL1| | | | +--------+ | >>>>> | ... --| |----+-i4--| +-----+ |---| |---| PHY1.7 |--| >>>>> | | | +-------------+ +---+ +--------+ | >>>>> | PHY1.7--| | | >>>>> | +---+ | >>>>> +--------------------------------------------------------------+ >>>>> Also, please keep in mind that is an example, there could be easily 4 >>>>> (or more) channels wired similarly. >>>>> >>>> >>>> >>>> Hi, >>>> >>>> This model tries to put too much into the synchronizer subsystem. The >>>> synchronizer device should only model inputs, DPLLs and outputs. >>>> >>>> The PHY lane to Synchronizer input muxing should be done in the >>>> PHY/netdev subsystem. That's why I wanted to start with the full model >>>> to specifically address this topic. >>>> >>>> The netdev should have an assigned list of Synchronizer inputs that it >>>> can recover its SyncE clocks into. It can be done by having a connection >>>> between the synchronizer input object(s) and the netdev, just like the >>>> netdev is connected to PHC clocks in the PHC subsystem. This is the >>>> model I initially presented about a year ago for solving this specific >>>> issue. >>>> >>>> Analogically, the netdev will be connected to a given output, however >>>> changing anything in the physical clock configuration sounds dangerous. >>>> >>>> Does that sound reasonable? >>>> >>>> Regards >>>> Maciek >>> >>> It sounds reasonable to some point. >>> You have mentioned list of Synchronizer inputs. If there is a list of >> inputs >>> it means it was created somewhere. I assume dpll subsystem? If so you >> would >>> like to export that list out of dpll subsystem, thus other entities would >> need >>> to find such list, then find particular source and somehow register with >> it. >>> All of this was proposed as part of netdev, I don't see any benefit in >> having >>> this parts separated from dpll, as only dpll would use it, right? >>> The same behavior is now provided by the MUX type pin, enclosed within >> dpll >>> subsystem. >>> >>> BR, >>> Arkadiusz >> >> The synchronizer object should expose the list of inputs that represent >> possible sources of a given chip. The list will be the same for all >> DPLLs used by the same device, so it can be a single set of sources >> linked to multiple DPLLs inside the package. A netdev can then point to >> a given input of a synchronizer that it's connected to. >> The phy lane->recovered clock (or directly a synchronizer input) muxing >> should stay in the netdev subsystem, or in the PHY driver. >> >> The reason, and benefit, of such split is when you create a board with a >> netdev X and a synchronizer Y that is not instantiated by the same >> driver. In this scenario you'd get the ice driver to instantiate >> connections and the DPLL vendor's driver for the synchronizer. In such >> case the netdev driver will simply send a netlink message to the >> input/source with a requested configuration, such as expected frequency, >> and everything from this point can be handled by a completely different >> driver creating clean and logical split. >> >> If we mix the phy lanes into the DPLL subsystem it'll get very >> challenging to add PHY lanes to the existing synchronizer exposed by a >> different driver. > > This is possible right now: > 1. obtain a dpll object: > struct dpll_device *dpll_device_get_by_clock_id(u64 clock_id, > enum dpll_type type, u8 idx); > 2. register new pin with muxed type pin: > int dpll_muxed_pin_register(struct dpll_device *dpll, > const char *parent_pin_description, > struct dpll_pin *pin, > struct dpll_pin_ops *ops, void *priv); > > To find dpll driver must know clock_id, type of dpll and its index given > when dpll was registered. > To register a pin, parent_pin_description of MUX type pin given on registering > it with dpll device. That would mean you need to repeat this process for all the DPLLs that are co-packaged in a single synchronizer. Some chips have up to 8 DPLLs, so you'd need to register number of phy lanes x number of DPLL times, say 8x8 = 64 times - that's simply too messy in the long term. >> Exporting and link between the synchronizer and the netdev is still a >> must no matter which way we go. And IMO it's best to link netdev to >> synchronizer sources, as that's the most natural way. >> > > The link is now just information for userspace Linux network interface index > in DPLLA_PIN_NETIFINDEX attribute. That's not the right way. We need to know: - which DPLL pins/sources are driven by which netdev (to be able to identify a source of frequency that is currently driving a given DPLL) - which DPLL generates a frequency for a given netdev (to know which DPLL to check for a specific netdev) So we need to have 2 connections.
Wed, Jan 11, 2023 at 04:30:44PM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Wednesday, January 11, 2023 4:05 PM >>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com> >> >>Wed, Jan 11, 2023 at 03:16:59PM CET, arkadiusz.kubalewski@intel.com wrote: >>>>From: Jiri Pirko <jiri@resnulli.us> >>>>Sent: Wednesday, January 11, 2023 9:20 AM >>>> >>>>Tue, Jan 10, 2023 at 09:05:49PM CET, kuba@kernel.org wrote: >>>>>On Mon, 9 Jan 2023 14:43:01 +0000 Kubalewski, Arkadiusz wrote: >>>>>> This is a simplified network switch board example. >>>>>> It has 2 synchronization channels, where each channel: >>>>>> - provides clk to 8 PHYs driven by separated MAC chips, >>>>>> - controls 2 DPLLs. >>>>>> >>>>>> Basically only given FW has control over its PHYs, so also a control >>>>over it's >>>>>> MUX inputs. >>>>>> All external sources are shared between the channels. >>>>>> >>>>>> This is why we believe it is not best idea to enclose multiple DPLLs >>>>with one >>>>>> object: >>>>>> - sources are shared even if DPLLs are not a single synchronizer chip, >>>>>> - control over specific MUX type input shall be controllable from >>>>different >>>>>> driver/firmware instances. >>>>>> >>>>>> As we know the proposal of having multiple DPLLs in one object was a >>try >>>>to >>>>>> simplify currently implemented shared pins. We fully support idea of >>>>having >>>>>> interfaces as simple as possible, but at the same time they shall be >>>>flexible >>>>>> enough to serve many use cases. >>>>> >>>>>I must be missing context from other discussions but what is this >>>>>proposal trying to solve? Well implemented shared pins is all we need. >>>> >>>>There is an entity containing the pins. The synchronizer chip. One >>>>synchronizer chip contains 1-n DPLLs. The source pins are connected >>>>to each DPLL (usually). What we missed in the original model was the >>>>synchronizer entity. If we have it, we don't need any notion of somehow >>>>floating pins as independent entities being attached to one or many >>>>DPLL refcounted, etc. The synchronizer device holds them in >>>>straightforward way. >>>> >>>>Example of a synchronizer chip: >>>>https://www.renesas.com/us/en/products/clocks-timing/jitter-attenuators- >>>>frequency-translation/8a34044-multichannel-dpll-dco-four-eight- >>>>channels#overview >>> >>>Not really, as explained above, multiple separated synchronizer chips can >>be >>>connected to the same external sources. >>>This is why I wrote this email, to better explain need for references >>between >>>DPLLs and shared pins. >>>Synchronizer chip object with multiple DPLLs would have sense if the pins >>would >>>only belong to that single chip, but this is not true. >> >>I don't understand how it is physically possible that 2 pins belong to 2 >>chips. Could you draw this to me? >> > >Well, sure, I was hoping this is clear, without extra connections on the draw: Okay, now I understand. It is not a shared pin but shared source for 2 pins. >+----------+ >|i0 - GPS |--------------\ >+----------+ | >+----------+ | >|i1 - SMA1 |------------\ | >+----------+ | | >+----------+ | | >|i2 - SMA2 |----------\ | | >+----------+ | | | > | | | >+---------------------|-|-|-------------------------------------------+ >| Channel A / FW0 | | | +-------------+ +---+ +--------+ | >| | | |-i0--|Synchronizer0|---| |---| PHY0.0 |--| One pin here ^^^ >| +---+ | | | | | | | +--------+ | >| PHY0.0--| | | |---i1--| |---| M |---| PHY0.1 |--| >| | | | | | | +-----+ | | A | +--------+ | >| PHY0.1--| M | |-----i2--| |DPLL0| | | C |---| PHY0.2 |--| >| | U | | | | | +-----+ | | 0 | +--------+ | >| PHY0.2--| X |--+----------i3--| +-----+ |---| |---| ... |--| >| | 0 | | | | | | |DPLL1| | | | +--------+ | >| ... --| | | /--------i4--| +-----+ |---| |---| PHY0.7 |--| >| | | | | | | | +-------------+ +---+ +--------+ | >| PHY0.7--| | | | | | | | >| +---+ | | | | | | >+----------------|-|--|-|-|-------------------------------------------+ >| Channel B / FW1| | | | | +-------------+ +---+ +--------+ | >| | | | | \-i0--|Synchronizer1|---| |---| PHY1.0 |--| And second pin here ^^^ There are 2 separate pins. Sure, they need to have the same config as they are connected to the same external entity (GPS, SMA1, SMA2). Perhaps we need to have a board description using dts to draw this picture so the drivers can use this schema in order to properly configure this? My point is, you are trying to hardcode the board geometry in the driver. Is that correct? >| +---+ | | | | | | | | +--------+ | >| PHY1.0--| | | | | \---i1--| |---| M |---| PHY1.1 |--| >| | | | | | | +-----+ | | A | +--------+ | >| PHY1.1--| M | | | \-----i2--| |DPLL0| | | C |---| PHY1.2 |--| >| | U | | | | +-----+ | | 1 | +--------+ | >| PHY1.2--| X | \-|--------i3--| +-----+ |---| |---| ... |--| >| | 1 | | | |DPLL1| | | | +--------+ | >| ... --| |----+--------i4--| +-----+ |---| |---| PHY1.7 |--| >| | | +-------------+ +---+ +--------+ | >| PHY1.7--| | | >| +---+ | >+---------------------------------------------------------------------+ > >> >>>As the pins are shared between multiple DPLLs (both inside 1 integrated >>circuit >>>and between multiple integrated circuits), all of them shall have current >>state >>>of the source or output. >>>Pins still need to be shared same as they would be inside of one >>synchronizer >>>chip. >> >>Do I understand correctly that you connect one synchronizer output to >>the input of the second synchronizer chip? > >No, I don't recall such use case. At least nothing that needs to exposed >in the DPLL subsystem itself. > >BR, >Arkadiusz > >> >>> >>>BR, >>>Arkadiusz
BR, Arkadiusz >-----Original Message----- >From: Maciek Machnikowski <maciek@machnikowski.net> >Sent: Wednesday, January 11, 2023 4:54 PM >To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Jiri Pirko ><jiri@resnulli.us> >Cc: Jakub Kicinski <kuba@kernel.org>; 'Vadim Fedorenko' ><vfedorenko@novek.ru>; 'Jonathan Lemon' <jonathan.lemon@gmail.com>; 'Paolo >Abeni' <pabeni@redhat.com>; netdev@vger.kernel.org; linux-arm- >kernel@lists.infradead.org; linux-clk@vger.kernel.org >Subject: Re: [RFC PATCH v4 0/4] Create common DPLL/clock configuration API > > > >On 1/11/2023 4:30 PM, Kubalewski, Arkadiusz wrote: >>> From: Maciek Machnikowski <maciek@machnikowski.net> >>> Sent: Wednesday, January 11, 2023 3:40 PM >>> To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Jiri Pirko >>> <jiri@resnulli.us> >>> >>> >>> On 1/11/2023 3:17 PM, Kubalewski, Arkadiusz wrote: >>>>> From: Maciek Machnikowski <maciek@machnikowski.net> >>>>> Sent: Tuesday, January 10, 2023 3:59 PM >>>>> To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Jiri Pirko >>>>> <jiri@resnulli.us> >>>>> >>>>> On 1/10/2023 11:54 AM, Kubalewski, Arkadiusz wrote: >>>>>>> From: Jiri Pirko <jiri@resnulli.us> >>>>>>> Sent: Monday, January 9, 2023 5:30 PM >>>>>>>> >>>>>>>> Hi guys, >>>>>>>> >>>>>>>> We have been trying to figure out feasibility of new approach >>> proposed >>>>> on >>>>>>> our >>>>>>>> latest meeting - to have a single object which encapsulates >multiple >>>>>>> DPLLs. >>>>>>>> >>>>>>>> Please consider following example: >>>>>>>> >>>>>>>> Shared common inputs: >>>>>>>> i0 - GPS / external >>>>>>>> i1 - SMA1 / external >>>>>>>> i2 - SMA2 / external >>>>>>>> i3 - MUX0 / clk recovered from PHY0.X driven by MAC0 >>>>>>>> i4 - MUX1 / clk recovered from PHY1.X driven by MAC1 >>>>>>>> >>>>>>>> +---------------------------------------------------------+ >>>>>>>> | Channel A / FW0 +---+ | >>>>>>>> | i0--| | | >>>>>>>> | +---+ | | | >>>>>>>> | PHY0.0--| | i1--| D | | >>>>>>>> | | | | P | | >>>>>>>> | PHY0.1--| M | i2--| L | +---+ +--------+ | >>>>>>>> | | U | | L |---| |---| PHY0.0 |--| >>>>>>>> | PHY0.2--| X |-+---------i3--| 0 | | | +--------+ | >>>>>>>> | | 0 | |+------+ | |---| M |---| PHY0.1 |--| >>>>>>>> | ... --| | || MUX1 |-i4--| | | A | +--------+ | >>>>>>>> | | | |+------+ +---+ | C |---| PHY0.2 |--| >>>>>>>> | PHY0.7--| | | i0--| | | 0 | +--------+ | >>>>>>>> | +---+ | | |---| |---| ... |--| >>>>>>>> | | i1--| D | | | +--------+ | >>>>>>>> | | | P |---| |---| PHY0.7 |--| >>>>>>>> | | i2--| L | +---+ +--------+ | >>>>>>>> | | | L | | >>>>>>>> | \---------i3--| 1 | | >>>>>>>> | +------+ | | | >>>>>>>> | | MUX1 |-i4--| | | >>>>>>>> | +------+ +---+ | >>>>>>>> +---------------------------------------------------------+ >>>>>>>> | Channel B / FW1 +---+ | >>>>>>>> | i0--| | | >>>>>>>> | | | | >>>>>>>> | i1--| D | | >>>>>>>> | +---+ | P | | >>>>>>>> | PHY1.0--| | i2--| L | +---+ +--------+ | >>>>>>>> | | | +------+ | L |---| |---| PHY1.0 |--| >>>>>>>> | PHY1.1--| M | | MUX0 |-i3--| 0 | | | +--------+ | >>>>>>>> | | U | +------+ | |---| M |---| PHY1.1 |--| >>>>>>>> | PHY1.2--| X |-+---------i4--| | | A | +--------+ | >>>>>>>> | | 1 | | +---+ | C |---| PHY1.2 |--| >>>>>>>> | ... --| | | i0--| | | 1 | +--------+ | >>>>>>>> | | | | | |---| |---| ... |--| >>>>>>>> | PHY1.7--| | | i1--| D | | | +--------+ | >>>>>>>> | +---+ | | P |---| |---| PHY1.7 |--| >>>>>>>> | | i2--| L | +---+ +--------+ | >>>>>>>> | |+------+ | L | | >>>>>>>> | || MUX0 |-i3--| 1 | | >>>>>>>> | |+------+ | | | >>>>>>>> | \---------i4--| | | >>>>>>>> | +---+ | >>>>>>>> +---------------------------------------------------------+ >>>>>>> >>>>>>> What is "a channel" here? Are these 2 channels part of the same >>> physival >>>>>>> chip? Could you add the synchronizer chip/device entities to your >>>>> drawing? >>>>>>> >>>>>> >>>>>> No. >>>>>> A "Synchronization Channel" on a switch would allow to separate >groups >>>>>> of physical ports. Each channel/group has own "Synchronizer Chip", >>> which >>>>> is >>>>>> used to drive PHY clocks of that group. >>>>>> >>>>>> "Synchronizer chip" would be the 2 DPLLs on old draw, something like >>>>> this: >>>>>> +--------------------------------------------------------------+ >>>>>> | Channel A / FW0 +-------------+ +---+ +--------+ | >>>>>> | i0--|Synchronizer0|---| |---| PHY0.0 |--| >>>>>> | +---+ | | | | +--------+ | >>>>>> | PHY0.0--| | i1--| |---| M |---| PHY0.1 |--| >>>>>> | | | | +-----+ | | A | +--------+ | >>>>>> | PHY0.1--| M | i2--| |DPLL0| | | C |---| PHY0.2 |--| >>>>>> | | U | | +-----+ | | 0 | +--------+ | >>>>>> | PHY0.2--| X |--+---i3--| +-----+ |---| |---| ... |--| >>>>>> | | 0 | | | |DPLL1| | | | +--------+ | >>>>>> | ... --| | | /-i4--| +-----+ |---| |---| PHY0.7 |--| >>>>>> | | | | | +-------------+ +---+ +--------+ | >>>>>> | PHY0.7--| | | | | >>>>>> | +---+ | | | >>>>>> +----------------|-|-------------------------------------------+ >>>>>> | Channel B / FW1| | +-------------+ +---+ +--------+ | >>>>>> | | | i0--|Synchronizer1|---| |---| PHY1.0 |--| >>>>>> | +---+ | | | | | | +--------+ | >>>>>> | PHY1.0--| | | | i1--| |---| M |---| PHY1.1 |--| >>>>>> | | | | | | +-----+ | | A | +--------+ | >>>>>> | PHY1.1--| M | | | i2--| |DPLL0| | | C |---| PHY1.2 |--| >>>>>> | | U | | | | +-----+ | | 1 | +--------+ | >>>>>> | PHY1.2--| X | \-|-i3--| +-----+ |---| |---| ... |--| >>>>>> | | 1 | | | |DPLL1| | | | +--------+ | >>>>>> | ... --| |----+-i4--| +-----+ |---| |---| PHY1.7 |--| >>>>>> | | | +-------------+ +---+ +--------+ | >>>>>> | PHY1.7--| | | >>>>>> | +---+ | >>>>>> +--------------------------------------------------------------+ >>>>>> Also, please keep in mind that is an example, there could be easily 4 >>>>>> (or more) channels wired similarly. >>>>>> >>>>> >>>>> >>>>> Hi, >>>>> >>>>> This model tries to put too much into the synchronizer subsystem. The >>>>> synchronizer device should only model inputs, DPLLs and outputs. >>>>> >>>>> The PHY lane to Synchronizer input muxing should be done in the >>>>> PHY/netdev subsystem. That's why I wanted to start with the full model >>>>> to specifically address this topic. >>>>> >>>>> The netdev should have an assigned list of Synchronizer inputs that it >>>>> can recover its SyncE clocks into. It can be done by having a >connection >>>>> between the synchronizer input object(s) and the netdev, just like the >>>>> netdev is connected to PHC clocks in the PHC subsystem. This is the >>>>> model I initially presented about a year ago for solving this specific >>>>> issue. >>>>> >>>>> Analogically, the netdev will be connected to a given output, however >>>>> changing anything in the physical clock configuration sounds >dangerous. >>>>> >>>>> Does that sound reasonable? >>>>> >>>>> Regards >>>>> Maciek >>>> >>>> It sounds reasonable to some point. >>>> You have mentioned list of Synchronizer inputs. If there is a list of >>> inputs >>>> it means it was created somewhere. I assume dpll subsystem? If so you >>> would >>>> like to export that list out of dpll subsystem, thus other entities >would >>> need >>>> to find such list, then find particular source and somehow register >with >>> it. >>>> All of this was proposed as part of netdev, I don't see any benefit in >>> having >>>> this parts separated from dpll, as only dpll would use it, right? >>>> The same behavior is now provided by the MUX type pin, enclosed within >>> dpll >>>> subsystem. >>>> >>>> BR, >>>> Arkadiusz >>> >>> The synchronizer object should expose the list of inputs that represent >>> possible sources of a given chip. The list will be the same for all >>> DPLLs used by the same device, so it can be a single set of sources >>> linked to multiple DPLLs inside the package. A netdev can then point to >>> a given input of a synchronizer that it's connected to. >>> The phy lane->recovered clock (or directly a synchronizer input) muxing >>> should stay in the netdev subsystem, or in the PHY driver. >>> >>> The reason, and benefit, of such split is when you create a board with a >>> netdev X and a synchronizer Y that is not instantiated by the same >>> driver. In this scenario you'd get the ice driver to instantiate >>> connections and the DPLL vendor's driver for the synchronizer. In such >>> case the netdev driver will simply send a netlink message to the >>> input/source with a requested configuration, such as expected frequency, >>> and everything from this point can be handled by a completely different >>> driver creating clean and logical split. >>> >>> If we mix the phy lanes into the DPLL subsystem it'll get very >>> challenging to add PHY lanes to the existing synchronizer exposed by a >>> different driver. >> >> This is possible right now: >> 1. obtain a dpll object: >> struct dpll_device *dpll_device_get_by_clock_id(u64 clock_id, >> enum dpll_type type, u8 idx); >> 2. register new pin with muxed type pin: >> int dpll_muxed_pin_register(struct dpll_device *dpll, >> const char *parent_pin_description, >> struct dpll_pin *pin, >> struct dpll_pin_ops *ops, void *priv); >> >> To find dpll driver must know clock_id, type of dpll and its index given >> when dpll was registered. >> To register a pin, parent_pin_description of MUX type pin given on >registering >> it with dpll device. > >That would mean you need to repeat this process for all the DPLLs that >are co-packaged in a single synchronizer. Some chips have up to 8 DPLLs, >so you'd need to register number of phy lanes x number of DPLL times, >say 8x8 = 64 times - that's simply too messy in the long term. > No, the pins are shared also MUX-type ones, adding to one adds to all. >>> Exporting and link between the synchronizer and the netdev is still a >>> must no matter which way we go. And IMO it's best to link netdev to >>> synchronizer sources, as that's the most natural way. >>> >> >> The link is now just information for userspace Linux network interface >index >> in DPLLA_PIN_NETIFINDEX attribute. > >That's not the right way. We need to know: >- which DPLL pins/sources are driven by which netdev (to be able to > identify a source of frequency that is currently driving a given DPLL) This is possible as explained above. >- which DPLL generates a frequency for a given netdev (to know which > DPLL to check for a specific netdev) > I agree here, we need something like it. BR, Arkadiusz. >So we need to have 2 connections.
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Wednesday, January 11, 2023 5:15 PM >To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com> > >Wed, Jan 11, 2023 at 04:30:44PM CET, arkadiusz.kubalewski@intel.com wrote: >>>From: Jiri Pirko <jiri@resnulli.us> >>>Sent: Wednesday, January 11, 2023 4:05 PM >>>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com> >>> >>>Wed, Jan 11, 2023 at 03:16:59PM CET, arkadiusz.kubalewski@intel.com >wrote: >>>>>From: Jiri Pirko <jiri@resnulli.us> >>>>>Sent: Wednesday, January 11, 2023 9:20 AM >>>>> >>>>>Tue, Jan 10, 2023 at 09:05:49PM CET, kuba@kernel.org wrote: >>>>>>On Mon, 9 Jan 2023 14:43:01 +0000 Kubalewski, Arkadiusz wrote: >>>>>>> This is a simplified network switch board example. >>>>>>> It has 2 synchronization channels, where each channel: >>>>>>> - provides clk to 8 PHYs driven by separated MAC chips, >>>>>>> - controls 2 DPLLs. >>>>>>> >>>>>>> Basically only given FW has control over its PHYs, so also a control >>>>>over it's >>>>>>> MUX inputs. >>>>>>> All external sources are shared between the channels. >>>>>>> >>>>>>> This is why we believe it is not best idea to enclose multiple DPLLs >>>>>with one >>>>>>> object: >>>>>>> - sources are shared even if DPLLs are not a single synchronizer >chip, >>>>>>> - control over specific MUX type input shall be controllable from >>>>>different >>>>>>> driver/firmware instances. >>>>>>> >>>>>>> As we know the proposal of having multiple DPLLs in one object was a >>>try >>>>>to >>>>>>> simplify currently implemented shared pins. We fully support idea of >>>>>having >>>>>>> interfaces as simple as possible, but at the same time they shall be >>>>>flexible >>>>>>> enough to serve many use cases. >>>>>> >>>>>>I must be missing context from other discussions but what is this >>>>>>proposal trying to solve? Well implemented shared pins is all we need. >>>>> >>>>>There is an entity containing the pins. The synchronizer chip. One >>>>>synchronizer chip contains 1-n DPLLs. The source pins are connected >>>>>to each DPLL (usually). What we missed in the original model was the >>>>>synchronizer entity. If we have it, we don't need any notion of somehow >>>>>floating pins as independent entities being attached to one or many >>>>>DPLL refcounted, etc. The synchronizer device holds them in >>>>>straightforward way. >>>>> >>>>>Example of a synchronizer chip: >>>>>https://www.renesas.com/us/en/products/clocks-timing/jitter- >attenuators- >>>>>frequency-translation/8a34044-multichannel-dpll-dco-four-eight- >>>>>channels#overview >>>> >>>>Not really, as explained above, multiple separated synchronizer chips >can >>>be >>>>connected to the same external sources. >>>>This is why I wrote this email, to better explain need for references >>>between >>>>DPLLs and shared pins. >>>>Synchronizer chip object with multiple DPLLs would have sense if the >pins >>>would >>>>only belong to that single chip, but this is not true. >>> >>>I don't understand how it is physically possible that 2 pins belong to 2 >>>chips. Could you draw this to me? >>> >> >>Well, sure, I was hoping this is clear, without extra connections on the >draw: > >Okay, now I understand. It is not a shared pin but shared source for 2 >pins. > Yes, exactly. > >>+----------+ >>|i0 - GPS |--------------\ >>+----------+ | >>+----------+ | >>|i1 - SMA1 |------------\ | >>+----------+ | | >>+----------+ | | >>|i2 - SMA2 |----------\ | | >>+----------+ | | | >> | | | >>+---------------------|-|-|-------------------------------------------+ >>| Channel A / FW0 | | | +-------------+ +---+ +--------+ | >>| | | |-i0--|Synchronizer0|---| |---| PHY0.0 |--| > >One pin here ^^^ > >>| +---+ | | | | | | | +--------+ | >>| PHY0.0--| | | |---i1--| |---| M |---| PHY0.1 |--| >>| | | | | | | +-----+ | | A | +--------+ | >>| PHY0.1--| M | |-----i2--| |DPLL0| | | C |---| PHY0.2 |--| >>| | U | | | | | +-----+ | | 0 | +--------+ | >>| PHY0.2--| X |--+----------i3--| +-----+ |---| |---| ... |--| >>| | 0 | | | | | | |DPLL1| | | | +--------+ | >>| ... --| | | /--------i4--| +-----+ |---| |---| PHY0.7 |--| >>| | | | | | | | +-------------+ +---+ +--------+ | >>| PHY0.7--| | | | | | | | >>| +---+ | | | | | | >>+----------------|-|--|-|-|-------------------------------------------+ >>| Channel B / FW1| | | | | +-------------+ +---+ +--------+ | >>| | | | | \-i0--|Synchronizer1|---| |---| PHY1.0 |--| > >And second pin here ^^^ > >There are 2 separate pins. Sure, they need to have the same config as >they are connected to the same external entity (GPS, SMA1, SMA2). > >Perhaps we need to have a board description using dts to draw this >picture so the drivers can use this schema in order to properly >configure this? > >My point is, you are trying to hardcode the board geometry in the >driver. Is that correct? > Well, we are trying to have userspace-friendly interface :) As we discussed yesterday dts is more of embedded world thing and we don't want to go that far, the driver knows the hardware it is using, thus it shall be enough if it has all the information needed for initialization. At least that is what I understood. BR, Arkadiusz > >>| +---+ | | | | | | | | +--------+ | >>| PHY1.0--| | | | | \---i1--| |---| M |---| PHY1.1 |--| >>| | | | | | | +-----+ | | A | +--------+ | >>| PHY1.1--| M | | | \-----i2--| |DPLL0| | | C |---| PHY1.2 |--| >>| | U | | | | +-----+ | | 1 | +--------+ | >>| PHY1.2--| X | \-|--------i3--| +-----+ |---| |---| ... |--| >>| | 1 | | | |DPLL1| | | | +--------+ | >>| ... --| |----+--------i4--| +-----+ |---| |---| PHY1.7 |--| >>| | | +-------------+ +---+ +--------+ | >>| PHY1.7--| | | >>| +---+ | >>+---------------------------------------------------------------------+ >> >>> >>>>As the pins are shared between multiple DPLLs (both inside 1 integrated >>>circuit >>>>and between multiple integrated circuits), all of them shall have >current >>>state >>>>of the source or output. >>>>Pins still need to be shared same as they would be inside of one >>>synchronizer >>>>chip. >>> >>>Do I understand correctly that you connect one synchronizer output to >>>the input of the second synchronizer chip? >> >>No, I don't recall such use case. At least nothing that needs to exposed >>in the DPLL subsystem itself. >> >>BR, >>Arkadiusz >> >>> >>>> >>>>BR, >>>>Arkadiusz
>From: Vadim Fedorenko <vfedorenko@novek.ru> >Sent: Tuesday, November 29, 2022 10:37 PM > >Implement common API for clock/DPLL configuration and status reporting. >The API utilises netlink interface as transport for commands and event >notifications. This API aim to extend current pin configuration and make it >flexible and easy to cover special configurations. > >v3 -> v4: > * redesign framework to make pins dynamically allocated (Arkadiusz) > * implement shared pins (Arkadiusz) >v2 -> v3: > * implement source select mode (Arkadiusz) > * add documentation > * implementation improvements (Jakub) >v1 -> v2: > * implement returning supported input/output types > * ptp_ocp: follow suggestions from Jonathan > * add linux-clk mailing list >v0 -> v1: > * fix code style and errors > * add linux-arm mailing list > > >Arkadiusz Kubalewski (1): > dpll: add dpll_attr/dpll_pin_attr helper classes > >Vadim Fedorenko (3): > dpll: Add DPLL framework base functions > dpll: documentation on DPLL subsystem interface > ptp_ocp: implement DPLL ops > > Documentation/networking/dpll.rst | 271 ++++++++ > Documentation/networking/index.rst | 1 + > MAINTAINERS | 8 + > drivers/Kconfig | 2 + > drivers/Makefile | 1 + > drivers/dpll/Kconfig | 7 + > drivers/dpll/Makefile | 11 + > drivers/dpll/dpll_attr.c | 278 +++++++++ > drivers/dpll/dpll_core.c | 760 +++++++++++++++++++++++ > drivers/dpll/dpll_core.h | 176 ++++++ > drivers/dpll/dpll_netlink.c | 963 +++++++++++++++++++++++++++++ > drivers/dpll/dpll_netlink.h | 24 + > drivers/dpll/dpll_pin_attr.c | 456 ++++++++++++++ > drivers/ptp/Kconfig | 1 + > drivers/ptp/ptp_ocp.c | 123 ++-- > include/linux/dpll.h | 261 ++++++++ > include/linux/dpll_attr.h | 433 +++++++++++++ > include/uapi/linux/dpll.h | 263 ++++++++ > 18 files changed, 4002 insertions(+), 37 deletions(-) create mode 100644 >Documentation/networking/dpll.rst create mode 100644 drivers/dpll/Kconfig >create mode 100644 drivers/dpll/Makefile create mode 100644 >drivers/dpll/dpll_attr.c create mode 100644 drivers/dpll/dpll_core.c >create mode 100644 drivers/dpll/dpll_core.h create mode 100644 >drivers/dpll/dpll_netlink.c create mode 100644 drivers/dpll/dpll_netlink.h >create mode 100644 drivers/dpll/dpll_pin_attr.c create mode 100644 >include/linux/dpll.h create mode 100644 include/linux/dpll_attr.h create >mode 100644 include/uapi/linux/dpll.h > >-- >2.27.0 New thread with regard of our yesterday's call. Is it possible to initialize a multiple output MUX? Yes it is. Let's consider 4 input/2 output MUX and DPLL it connects with: +---+ --| | +---+ | | --| | --| D |-- | | | P | --| M |-----| L |-- | U | | L | --| X |-----| |-- | | | | --| | --| | +---+ +---+ Basically dpll pins are initialized and assigned ids, like: 5 inputs (0-4), 3 outputs (5-7). +---+ 0--| | | | 1--| D |--5 | P | 2--| L |--6 | L | 3--| |--7 | | 4--| | +---+ Then we would create and register muxed pins with existing dpll pins. Each muxed pin is allocated and registered with each parent it can provide signal with, like below (number in bracket is parent idx): +---+ 0--| | +---+ | | 8(2) / 9(3)---| | 1--| D |--5 | | | P | 10(2) / 11(3)---| M |---2--| L |--6 | U | | L | 12(2) / 13(3)---| X |---3--| |--7 | | | | 14(2) / 15(3)---| | 4--| | +---+ +---+ Controlling the mux input/output: In this case selecting pin #8 would provide its signal into DPLLs input#2 and selecting #9 would provide its signal into DPLLs input#3. BR, Arkadiusz
Thu, Jan 12, 2023 at 01:15:30PM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Wednesday, January 11, 2023 5:15 PM >>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com> >> >>Wed, Jan 11, 2023 at 04:30:44PM CET, arkadiusz.kubalewski@intel.com wrote: >>>>From: Jiri Pirko <jiri@resnulli.us> >>>>Sent: Wednesday, January 11, 2023 4:05 PM >>>>To: Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com> >>>> >>>>Wed, Jan 11, 2023 at 03:16:59PM CET, arkadiusz.kubalewski@intel.com >>wrote: >>>>>>From: Jiri Pirko <jiri@resnulli.us> >>>>>>Sent: Wednesday, January 11, 2023 9:20 AM >>>>>> >>>>>>Tue, Jan 10, 2023 at 09:05:49PM CET, kuba@kernel.org wrote: >>>>>>>On Mon, 9 Jan 2023 14:43:01 +0000 Kubalewski, Arkadiusz wrote: >>>>>>>> This is a simplified network switch board example. >>>>>>>> It has 2 synchronization channels, where each channel: >>>>>>>> - provides clk to 8 PHYs driven by separated MAC chips, >>>>>>>> - controls 2 DPLLs. >>>>>>>> >>>>>>>> Basically only given FW has control over its PHYs, so also a control >>>>>>over it's >>>>>>>> MUX inputs. >>>>>>>> All external sources are shared between the channels. >>>>>>>> >>>>>>>> This is why we believe it is not best idea to enclose multiple DPLLs >>>>>>with one >>>>>>>> object: >>>>>>>> - sources are shared even if DPLLs are not a single synchronizer >>chip, >>>>>>>> - control over specific MUX type input shall be controllable from >>>>>>different >>>>>>>> driver/firmware instances. >>>>>>>> >>>>>>>> As we know the proposal of having multiple DPLLs in one object was a >>>>try >>>>>>to >>>>>>>> simplify currently implemented shared pins. We fully support idea of >>>>>>having >>>>>>>> interfaces as simple as possible, but at the same time they shall be >>>>>>flexible >>>>>>>> enough to serve many use cases. >>>>>>> >>>>>>>I must be missing context from other discussions but what is this >>>>>>>proposal trying to solve? Well implemented shared pins is all we need. >>>>>> >>>>>>There is an entity containing the pins. The synchronizer chip. One >>>>>>synchronizer chip contains 1-n DPLLs. The source pins are connected >>>>>>to each DPLL (usually). What we missed in the original model was the >>>>>>synchronizer entity. If we have it, we don't need any notion of somehow >>>>>>floating pins as independent entities being attached to one or many >>>>>>DPLL refcounted, etc. The synchronizer device holds them in >>>>>>straightforward way. >>>>>> >>>>>>Example of a synchronizer chip: >>>>>>https://www.renesas.com/us/en/products/clocks-timing/jitter- >>attenuators- >>>>>>frequency-translation/8a34044-multichannel-dpll-dco-four-eight- >>>>>>channels#overview >>>>> >>>>>Not really, as explained above, multiple separated synchronizer chips >>can >>>>be >>>>>connected to the same external sources. >>>>>This is why I wrote this email, to better explain need for references >>>>between >>>>>DPLLs and shared pins. >>>>>Synchronizer chip object with multiple DPLLs would have sense if the >>pins >>>>would >>>>>only belong to that single chip, but this is not true. >>>> >>>>I don't understand how it is physically possible that 2 pins belong to 2 >>>>chips. Could you draw this to me? >>>> >>> >>>Well, sure, I was hoping this is clear, without extra connections on the >>draw: >> >>Okay, now I understand. It is not a shared pin but shared source for 2 >>pins. >> > >Yes, exactly. > >> >>>+----------+ >>>|i0 - GPS |--------------\ >>>+----------+ | >>>+----------+ | >>>|i1 - SMA1 |------------\ | >>>+----------+ | | >>>+----------+ | | >>>|i2 - SMA2 |----------\ | | >>>+----------+ | | | >>> | | | >>>+---------------------|-|-|-------------------------------------------+ >>>| Channel A / FW0 | | | +-------------+ +---+ +--------+ | >>>| | | |-i0--|Synchronizer0|---| |---| PHY0.0 |--| >> >>One pin here ^^^ >> >>>| +---+ | | | | | | | +--------+ | >>>| PHY0.0--| | | |---i1--| |---| M |---| PHY0.1 |--| >>>| | | | | | | +-----+ | | A | +--------+ | >>>| PHY0.1--| M | |-----i2--| |DPLL0| | | C |---| PHY0.2 |--| >>>| | U | | | | | +-----+ | | 0 | +--------+ | >>>| PHY0.2--| X |--+----------i3--| +-----+ |---| |---| ... |--| >>>| | 0 | | | | | | |DPLL1| | | | +--------+ | >>>| ... --| | | /--------i4--| +-----+ |---| |---| PHY0.7 |--| >>>| | | | | | | | +-------------+ +---+ +--------+ | >>>| PHY0.7--| | | | | | | | >>>| +---+ | | | | | | >>>+----------------|-|--|-|-|-------------------------------------------+ >>>| Channel B / FW1| | | | | +-------------+ +---+ +--------+ | >>>| | | | | \-i0--|Synchronizer1|---| |---| PHY1.0 |--| >> >>And second pin here ^^^ >> >>There are 2 separate pins. Sure, they need to have the same config as >>they are connected to the same external entity (GPS, SMA1, SMA2). >> >>Perhaps we need to have a board description using dts to draw this >>picture so the drivers can use this schema in order to properly >>configure this? >> >>My point is, you are trying to hardcode the board geometry in the >>driver. Is that correct? >> > >Well, we are trying to have userspace-friendly interface :) >As we discussed yesterday dts is more of embedded world thing and we don't >want to go that far, the driver knows the hardware it is using, thus it >shall be enough if it has all the information needed for initialization. >At least that is what I understood. Yes, I think yesterday called cleared things up. Thanks! > >BR, >Arkadiusz > >> >>>| +---+ | | | | | | | | +--------+ | >>>| PHY1.0--| | | | | \---i1--| |---| M |---| PHY1.1 |--| >>>| | | | | | | +-----+ | | A | +--------+ | >>>| PHY1.1--| M | | | \-----i2--| |DPLL0| | | C |---| PHY1.2 |--| >>>| | U | | | | +-----+ | | 1 | +--------+ | >>>| PHY1.2--| X | \-|--------i3--| +-----+ |---| |---| ... |--| >>>| | 1 | | | |DPLL1| | | | +--------+ | >>>| ... --| |----+--------i4--| +-----+ |---| |---| PHY1.7 |--| >>>| | | +-------------+ +---+ +--------+ | >>>| PHY1.7--| | | >>>| +---+ | >>>+---------------------------------------------------------------------+ >>> >>>> >>>>>As the pins are shared between multiple DPLLs (both inside 1 integrated >>>>circuit >>>>>and between multiple integrated circuits), all of them shall have >>current >>>>state >>>>>of the source or output. >>>>>Pins still need to be shared same as they would be inside of one >>>>synchronizer >>>>>chip. >>>> >>>>Do I understand correctly that you connect one synchronizer output to >>>>the input of the second synchronizer chip? >>> >>>No, I don't recall such use case. At least nothing that needs to exposed >>>in the DPLL subsystem itself. >>> >>>BR, >>>Arkadiusz >>> >>>> >>>>> >>>>>BR, >>>>>Arkadiusz
Thu, Jan 12, 2023 at 01:23:29PM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Vadim Fedorenko <vfedorenko@novek.ru> >>Sent: Tuesday, November 29, 2022 10:37 PM >> >>Implement common API for clock/DPLL configuration and status reporting. >>The API utilises netlink interface as transport for commands and event >>notifications. This API aim to extend current pin configuration and make it >>flexible and easy to cover special configurations. >> >>v3 -> v4: >> * redesign framework to make pins dynamically allocated (Arkadiusz) >> * implement shared pins (Arkadiusz) >>v2 -> v3: >> * implement source select mode (Arkadiusz) >> * add documentation >> * implementation improvements (Jakub) >>v1 -> v2: >> * implement returning supported input/output types >> * ptp_ocp: follow suggestions from Jonathan >> * add linux-clk mailing list >>v0 -> v1: >> * fix code style and errors >> * add linux-arm mailing list >> >> >>Arkadiusz Kubalewski (1): >> dpll: add dpll_attr/dpll_pin_attr helper classes >> >>Vadim Fedorenko (3): >> dpll: Add DPLL framework base functions >> dpll: documentation on DPLL subsystem interface >> ptp_ocp: implement DPLL ops >> >> Documentation/networking/dpll.rst | 271 ++++++++ >> Documentation/networking/index.rst | 1 + >> MAINTAINERS | 8 + >> drivers/Kconfig | 2 + >> drivers/Makefile | 1 + >> drivers/dpll/Kconfig | 7 + >> drivers/dpll/Makefile | 11 + >> drivers/dpll/dpll_attr.c | 278 +++++++++ >> drivers/dpll/dpll_core.c | 760 +++++++++++++++++++++++ >> drivers/dpll/dpll_core.h | 176 ++++++ >> drivers/dpll/dpll_netlink.c | 963 +++++++++++++++++++++++++++++ >> drivers/dpll/dpll_netlink.h | 24 + >> drivers/dpll/dpll_pin_attr.c | 456 ++++++++++++++ >> drivers/ptp/Kconfig | 1 + >> drivers/ptp/ptp_ocp.c | 123 ++-- >> include/linux/dpll.h | 261 ++++++++ >> include/linux/dpll_attr.h | 433 +++++++++++++ >> include/uapi/linux/dpll.h | 263 ++++++++ >> 18 files changed, 4002 insertions(+), 37 deletions(-) create mode 100644 >>Documentation/networking/dpll.rst create mode 100644 drivers/dpll/Kconfig >>create mode 100644 drivers/dpll/Makefile create mode 100644 >>drivers/dpll/dpll_attr.c create mode 100644 drivers/dpll/dpll_core.c >>create mode 100644 drivers/dpll/dpll_core.h create mode 100644 >>drivers/dpll/dpll_netlink.c create mode 100644 drivers/dpll/dpll_netlink.h >>create mode 100644 drivers/dpll/dpll_pin_attr.c create mode 100644 >>include/linux/dpll.h create mode 100644 include/linux/dpll_attr.h create >>mode 100644 include/uapi/linux/dpll.h >> >>-- >>2.27.0 > >New thread with regard of our yesterday's call. > >Is it possible to initialize a multiple output MUX? >Yes it is. Let's consider 4 input/2 output MUX and DPLL it connects with: > +---+ > --| | > +---+ | | >--| | --| D |-- > | | | P | >--| M |-----| L |-- > | U | | L | >--| X |-----| |-- > | | | | >--| | --| | > +---+ +---+ > >Basically dpll pins are initialized and assigned ids, like: >5 inputs (0-4), 3 outputs (5-7). > +---+ >0--| | > | | >1--| D |--5 > | P | >2--| L |--6 > | L | >3--| |--7 > | | >4--| | > +---+ > >Then we would create and register muxed pins with existing dpll pins. >Each muxed pin is allocated and registered with each parent it can provide >signal with, like below (number in bracket is parent idx): > +---+ > 0--| | > +---+ | | > 8(2) / 9(3)---| | 1--| D |--5 > | | | P | >10(2) / 11(3)---| M |---2--| L |--6 > | U | | L | >12(2) / 13(3)---| X |---3--| |--7 > | | | | >14(2) / 15(3)---| | 4--| | > +---+ +---+ > >Controlling the mux input/output: >In this case selecting pin #8 would provide its signal into DPLLs input#2 and >selecting #9 would provide its signal into DPLLs input#3. Duplication of the mux pin (for example 8,9) seems a bit silly. What if the mux has 8 outputs? You would have to have 8 pin indexes for each mux input. One thing is not clear to me. The mux outputs are fixed or selectable? I mean, can the outputs be enabled/disabled wired to a specific mux input? > >BR, >Arkadiusz
On Thu, 12 Jan 2023 12:23:29 +0000 Kubalewski, Arkadiusz wrote: > Then we would create and register muxed pins with existing dpll pins. > Each muxed pin is allocated and registered with each parent it can provide > signal with, like below (number in bracket is parent idx): > +---+ > 0--| | > +---+ | | > 8(2) / 9(3)---| | 1--| D |--5 > | | | P | > 10(2) / 11(3)---| M |---2--| L |--6 > | U | | L | > 12(2) / 13(3)---| X |---3--| |--7 > | | | | > 14(2) / 15(3)---| | 4--| | > +---+ +---+ > > Controlling the mux input/output: > In this case selecting pin #8 would provide its signal into DPLLs input#2 and > selecting #9 would provide its signal into DPLLs input#3. I agree with Jiri, the duplication seems unnecessary. My thinking would be to handle this as follows: +---+ 0--| | +---+ | | 10---| | 1--| D |--5 | | | P | 11---| M |-8---2--| L |--6 | U | | L | 12---| X |-9---3--| |--7 | | | | 13---| | 4--| | +---+ +---+ Give the user the ability to both select the inputs to DPLL from 0-4 and from 10-13. If 10-13 are selected the core should give mapping things automatically a try (but we don't need to support auto-mapping for muxes with more than one output from the start). There should also be an API for manually configuring muxes. Eg. User requests DPLL inputs: 0, 1, 10, 11 Core automatically maps 10 -> 8, 11 -> 9 User requests DPLL inputs: 0, 1, 10, 11, 12 Core responds with an error User requests DPLL inputs: 0, 1, 2, 3 Core doesn't touch the mux User requests mux to direct 10 -> 8 User requests mux to direct 11 -> 9 Now the config is equivalent to case #1