From: Simon Horman <simon.horman@corigine.com>
To: Vadim Fedorenko <vadim.fedorenko@linux.dev>
Cc: Jakub Kicinski <kuba@kernel.org>, Jiri Pirko <jiri@resnulli.us>,
Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>,
Jonathan Lemon <jonathan.lemon@gmail.com>,
Paolo Abeni <pabeni@redhat.com>,
Milena Olech <milena.olech@intel.com>,
Michal Michalik <michal.michalik@intel.com>,
linux-arm-kernel@lists.infradead.org, poros@redhat.com,
mschmidt@redhat.com, netdev@vger.kernel.org,
linux-clk@vger.kernel.org, Bart Van Assche <bvanassche@acm.org>
Subject: Re: [PATCH net-next 08/11] ice: add admin commands to access cgu configuration
Date: Mon, 24 Jul 2023 19:21:54 +0200 [thread overview]
Message-ID: <ZL6zMmyIUObOY+6i@corigine.com> (raw)
In-Reply-To: <20230720091903.297066-9-vadim.fedorenko@linux.dev>
On Thu, Jul 20, 2023 at 10:19:00AM +0100, Vadim Fedorenko wrote:
...
Hi Vadim,
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c b/drivers/net/ethernet/intel/ice/ice_common.c
...
> +/**
> + * ice_aq_get_cgu_dpll_status
> + * @hw: pointer to the HW struct
> + * @dpll_num: DPLL index
> + * @ref_state: Reference clock state
> + * @dpll_state: DPLL state
./scripts/kernel-doc says that @config is missing here.
> + * @phase_offset: Phase offset in ns
> + * @eec_mode: EEC_mode
> + *
> + * Get CGU DPLL status (0x0C66)
> + */
> +int
> +ice_aq_get_cgu_dpll_status(struct ice_hw *hw, u8 dpll_num, u8 *ref_state,
> + u8 *dpll_state, u8 *config, s64 *phase_offset,
> + u8 *eec_mode)
...
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
...
> +/**
> + * ice_get_cgu_state - get the state of the DPLL
> + * @hw: pointer to the hw struct
> + * @dpll_idx: Index of internal DPLL unit
> + * @last_dpll_state: last known state of DPLL
> + * @pin: pointer to a buffer for returning currently active pin
> + * @ref_state: reference clock state
Likewise, @eec_mode is missing here.
> + * @phase_offset: pointer to a buffer for returning phase offset
> + * @dpll_state: state of the DPLL (output)
And @mode is missing here.
> + *
> + * This function will read the state of the DPLL(dpll_idx). Non-null
> + * 'pin', 'ref_state', 'eec_mode' and 'phase_offset' parameters are used to
> + * retrieve currently active pin, state, mode and phase_offset respectively.
> + *
> + * Return: state of the DPLL
> + */
> +int ice_get_cgu_state(struct ice_hw *hw, u8 dpll_idx,
> + enum dpll_lock_status last_dpll_state, u8 *pin,
> + u8 *ref_state, u8 *eec_mode, s64 *phase_offset,
> + enum dpll_lock_status *dpll_state,
> + enum dpll_mode *mode)
> +{
> + u8 hw_ref_state, hw_dpll_state, hw_eec_mode, hw_config;
> + s64 hw_phase_offset;
> + int status;
> +
> + status = ice_aq_get_cgu_dpll_status(hw, dpll_idx, &hw_ref_state,
> + &hw_dpll_state, &hw_config,
> + &hw_phase_offset, &hw_eec_mode);
> + if (status) {
> + *dpll_state = ICE_CGU_STATE_INVALID;
dpll_state is of type enum dpll_lock_status.
But the type of ICE_CGU_STATE_INVALID is enum ice_cgu_state.
Is this intended?
As flagged by gcc-12 W=1 and clang-16 W=1 builds.
> + return status;
> + }
> +
> + if (pin)
> + /* current ref pin in dpll_state_refsel_status_X register */
> + *pin = hw_config & ICE_AQC_GET_CGU_DPLL_CONFIG_CLK_REF_SEL;
> + if (phase_offset)
> + *phase_offset = hw_phase_offset;
> + if (ref_state)
> + *ref_state = hw_ref_state;
> + if (eec_mode)
> + *eec_mode = hw_eec_mode;
> + if (!dpll_state)
> + return status;
Here dpll_state is checked for NULL.
But, above, it is dereferenced in the case where ice_aq_get_cgu_dpll_status
fails. Is that safe?
Also, perhaps it makes things a bit clearer to return 0 here.
...
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h
...
> +static const struct ice_cgu_pin_desc ice_e810t_sfp_cgu_inputs[] = {
> + { "CVL-SDP22", ZL_REF0P, DPLL_PIN_TYPE_INT_OSCILLATOR,
> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> + { "CVL-SDP20", ZL_REF0N, DPLL_PIN_TYPE_INT_OSCILLATOR,
> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> + { "C827_0-RCLKA", ZL_REF1P, DPLL_PIN_TYPE_MUX, 0, },
> + { "C827_0-RCLKB", ZL_REF1N, DPLL_PIN_TYPE_MUX, 0, },
> + { "SMA1", ZL_REF3P, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> + { "SMA2/U.FL2", ZL_REF3N, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> + { "GNSS-1PPS", ZL_REF4P, DPLL_PIN_TYPE_GNSS,
> + ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
> + { "OCXO", ZL_REF4N, DPLL_PIN_TYPE_INT_OSCILLATOR, 0, },
> +};
A gcc-12 W=1 build warns that ice_e810t_sfp_cgu_inputs, and
the similar static variables below, are unused when ice_ptp_hw.h
is included in ice_main.c via ice.h.
Looking at ice_e823_zl_cgu_outputs[], it seems to only be used
in ice_ptp_hw.c, so perhaps it could be defined there.
Perhaps that is also true of the other static variables below,
but I didn't check that.
> +
> +static const struct ice_cgu_pin_desc ice_e810t_qsfp_cgu_inputs[] = {
> + { "CVL-SDP22", ZL_REF0P, DPLL_PIN_TYPE_INT_OSCILLATOR,
> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> + { "CVL-SDP20", ZL_REF0N, DPLL_PIN_TYPE_INT_OSCILLATOR,
> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> + { "C827_0-RCLKA", ZL_REF1P, DPLL_PIN_TYPE_MUX, },
> + { "C827_0-RCLKB", ZL_REF1N, DPLL_PIN_TYPE_MUX, },
> + { "C827_1-RCLKA", ZL_REF2P, DPLL_PIN_TYPE_MUX, },
> + { "C827_1-RCLKB", ZL_REF2N, DPLL_PIN_TYPE_MUX, },
> + { "SMA1", ZL_REF3P, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> + { "SMA2/U.FL2", ZL_REF3N, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> + { "GNSS-1PPS", ZL_REF4P, DPLL_PIN_TYPE_GNSS,
> + ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
> + { "OCXO", ZL_REF4N, DPLL_PIN_TYPE_INT_OSCILLATOR, },
> +};
> +
> +static const struct ice_cgu_pin_desc ice_e810t_sfp_cgu_outputs[] = {
> + { "REF-SMA1", ZL_OUT0, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> + { "REF-SMA2/U.FL2", ZL_OUT1, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> + { "PHY-CLK", ZL_OUT2, DPLL_PIN_TYPE_SYNCE_ETH_PORT, },
> + { "MAC-CLK", ZL_OUT3, DPLL_PIN_TYPE_SYNCE_ETH_PORT, },
> + { "CVL-SDP21", ZL_OUT4, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
> + { "CVL-SDP23", ZL_OUT5, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
> +};
> +
> +static const struct ice_cgu_pin_desc ice_e810t_qsfp_cgu_outputs[] = {
> + { "REF-SMA1", ZL_OUT0, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> + { "REF-SMA2/U.FL2", ZL_OUT1, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> + { "PHY-CLK", ZL_OUT2, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
> + { "PHY2-CLK", ZL_OUT3, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
> + { "MAC-CLK", ZL_OUT4, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
> + { "CVL-SDP21", ZL_OUT5, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
> + { "CVL-SDP23", ZL_OUT6, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
> +};
> +
> +static const struct ice_cgu_pin_desc ice_e823_si_cgu_inputs[] = {
> + { "NONE", SI_REF0P, 0, 0 },
> + { "NONE", SI_REF0N, 0, 0 },
> + { "SYNCE0_DP", SI_REF1P, DPLL_PIN_TYPE_MUX, 0 },
> + { "SYNCE0_DN", SI_REF1N, DPLL_PIN_TYPE_MUX, 0 },
> + { "EXT_CLK_SYNC", SI_REF2P, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> + { "NONE", SI_REF2N, 0, 0 },
> + { "EXT_PPS_OUT", SI_REF3, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> + { "INT_PPS_OUT", SI_REF4, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +};
> +
> +static const struct ice_cgu_pin_desc ice_e823_si_cgu_outputs[] = {
> + { "1588-TIME_SYNC", SI_OUT0, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> + { "PHY-CLK", SI_OUT1, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
> + { "10MHZ-SMA2", SI_OUT2, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_10_mhz), ice_cgu_pin_freq_10_mhz },
> + { "PPS-SMA1", SI_OUT3, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> +};
> +
> +static const struct ice_cgu_pin_desc ice_e823_zl_cgu_inputs[] = {
> + { "NONE", ZL_REF0P, 0, 0 },
> + { "INT_PPS_OUT", ZL_REF0N, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
> + { "SYNCE0_DP", ZL_REF1P, DPLL_PIN_TYPE_MUX, 0 },
> + { "SYNCE0_DN", ZL_REF1N, DPLL_PIN_TYPE_MUX, 0 },
> + { "NONE", ZL_REF2P, 0, 0 },
> + { "NONE", ZL_REF2N, 0, 0 },
> + { "EXT_CLK_SYNC", ZL_REF3P, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> + { "NONE", ZL_REF3N, 0, 0 },
> + { "EXT_PPS_OUT", ZL_REF4P, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
> + { "OCXO", ZL_REF4N, DPLL_PIN_TYPE_INT_OSCILLATOR, 0 },
> +};
> +
> +static const struct ice_cgu_pin_desc ice_e823_zl_cgu_outputs[] = {
> + { "PPS-SMA1", ZL_OUT0, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_1_hz), ice_cgu_pin_freq_1_hz },
> + { "10MHZ-SMA2", ZL_OUT1, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_10_mhz), ice_cgu_pin_freq_10_mhz },
> + { "PHY-CLK", ZL_OUT2, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
> + { "1588-TIME_REF", ZL_OUT3, DPLL_PIN_TYPE_SYNCE_ETH_PORT, 0 },
> + { "CPK-TIME_SYNC", ZL_OUT4, DPLL_PIN_TYPE_EXT,
> + ARRAY_SIZE(ice_cgu_pin_freq_common), ice_cgu_pin_freq_common },
> + { "NONE", ZL_OUT5, 0, 0 },
> +};
> +
> extern const struct
> ice_cgu_pll_params_e822 e822_cgu_params[NUM_ICE_TIME_REF_FREQ];
>
...
next prev parent reply other threads:[~2023-07-24 17:22 UTC|newest]
Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-20 9:18 [PATCH net-next 00/11] Create common DPLL configuration API Vadim Fedorenko
2023-07-20 9:18 ` [PATCH net-next 01/11] tools: ynl-gen: fix enum index in _decode_enum(..) Vadim Fedorenko
2023-07-20 13:40 ` Jiri Pirko
2023-07-20 13:58 ` Kubalewski, Arkadiusz
2023-07-20 14:48 ` Vadim Fedorenko
2023-07-20 9:18 ` [PATCH net-next 02/11] tools: ynl-gen: fix parse multi-attr enum attribute Vadim Fedorenko
2023-07-20 9:18 ` [PATCH net-next 03/11] dpll: documentation on DPLL subsystem interface Vadim Fedorenko
2023-07-20 13:47 ` Jiri Pirko
2023-07-20 9:18 ` [PATCH net-next 04/11] dpll: spec: Add Netlink spec in YAML Vadim Fedorenko
2023-07-24 15:52 ` Simon Horman
2023-07-20 9:18 ` [PATCH net-next 05/11] dpll: core: Add DPLL framework base functions Vadim Fedorenko
2023-07-24 15:56 ` Simon Horman
2023-07-20 9:18 ` [PATCH net-next 06/11] dpll: netlink: " Vadim Fedorenko
2023-07-24 16:34 ` Simon Horman
2023-08-01 12:23 ` Jiri Pirko
2023-07-20 9:18 ` [PATCH net-next 07/11] netdev: expose DPLL pin handle for netdevice Vadim Fedorenko
2023-08-01 13:23 ` Vadim Fedorenko
2023-08-01 15:12 ` Vadim Fedorenko
2023-07-20 9:19 ` [PATCH net-next 08/11] ice: add admin commands to access cgu configuration Vadim Fedorenko
2023-07-24 17:21 ` Simon Horman [this message]
2023-07-28 12:46 ` Kubalewski, Arkadiusz
2023-07-20 9:19 ` [PATCH 09/11] ice: implement dpll interface to control cgu Vadim Fedorenko
2023-07-20 14:08 ` Jiri Pirko
2023-07-20 17:31 ` Kubalewski, Arkadiusz
2023-07-21 7:33 ` Jiri Pirko
2023-07-21 11:17 ` Kubalewski, Arkadiusz
2023-07-21 12:02 ` Jiri Pirko
2023-07-21 13:36 ` Kubalewski, Arkadiusz
2023-07-21 15:45 ` Jiri Pirko
2023-07-21 19:48 ` Kubalewski, Arkadiusz
2023-07-22 6:37 ` Jiri Pirko
2023-07-24 15:03 ` Kubalewski, Arkadiusz
2023-07-25 8:03 ` Jiri Pirko
2023-07-25 14:01 ` Kubalewski, Arkadiusz
2023-07-26 6:38 ` Jiri Pirko
2023-07-26 21:11 ` Kubalewski, Arkadiusz
2023-07-27 10:28 ` Vadim Fedorenko
2023-07-25 22:49 ` Jakub Kicinski
2023-07-26 15:20 ` Paolo Abeni
2023-07-26 21:10 ` Kubalewski, Arkadiusz
2023-07-31 12:08 ` Jiri Pirko
2023-07-26 21:08 ` Kubalewski, Arkadiusz
2023-07-31 12:11 ` Jiri Pirko
2023-07-22 2:08 ` Jakub Kicinski
2023-07-21 11:39 ` Jiri Pirko
2023-07-28 23:03 ` Kubalewski, Arkadiusz
2023-07-31 12:19 ` Jiri Pirko
2023-08-01 14:50 ` Kubalewski, Arkadiusz
2023-08-02 6:57 ` Jiri Pirko
2023-08-02 15:48 ` Kubalewski, Arkadiusz
2023-08-03 8:02 ` Jiri Pirko
2023-08-04 8:58 ` Kubalewski, Arkadiusz
2023-07-24 17:41 ` Simon Horman
2023-07-24 17:58 ` Vadim Fedorenko
2023-07-28 23:10 ` Kubalewski, Arkadiusz
2023-07-20 9:19 ` [PATCH 10/11] ptp_ocp: implement DPLL ops Vadim Fedorenko
2023-07-21 15:51 ` Jiri Pirko
2023-07-20 9:19 ` [PATCH 11/11] mlx5: Implement SyncE support using DPLL infrastructure Vadim Fedorenko
2023-07-21 7:48 ` [PATCH net-next 00/11] Create common DPLL configuration API Jiri Pirko
2023-07-21 11:14 ` Jiri Pirko
2023-07-21 14:42 ` Vadim Fedorenko
2023-07-21 15:46 ` Jiri Pirko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZL6zMmyIUObOY+6i@corigine.com \
--to=simon.horman@corigine.com \
--cc=arkadiusz.kubalewski@intel.com \
--cc=bvanassche@acm.org \
--cc=jiri@resnulli.us \
--cc=jonathan.lemon@gmail.com \
--cc=kuba@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=michal.michalik@intel.com \
--cc=milena.olech@intel.com \
--cc=mschmidt@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=poros@redhat.com \
--cc=vadim.fedorenko@linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox