Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Mon, 23 Oct 2023 08:43:39 +0200
From:   Markus Schneider-Pargmann <msp@baylibre.com>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>
Cc:     sboyd@kernel.org, mturquette@baylibre.com, matthias.bgg@gmail.com,
        wenst@chromium.org, amergnat@baylibre.com,
        yangyingliang@huawei.com, u.kleine-koenig@pengutronix.de,
        linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, kernel@collabora.com
Subject: Re: [PATCH v2 1/2] clk: mediatek: clk-mux: Support custom parent
 indices for muxes
Message-ID: <20231023064339.xglriq6ycedjlyd4@blmsp>
References: <20231018103546.48174-1-angelogioacchino.delregno@collabora.com>
 <20231018103546.48174-2-angelogioacchino.delregno@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20231018103546.48174-2-angelogioacchino.delregno@collabora.com>
Precedence: bulk

Hi Angelo,

On Wed, Oct 18, 2023 at 12:35:45PM +0200, AngeloGioacchino Del Regno wrote:
> Add support for customized parent indices for MediaTek muxes: this is
> necessary for the case in which we want to exclude some clocks from
> a mux's parent clocks list, where the exclusions are not from the
> very bottom of the list but either in the middle or the beginning.
> 
> Example:
> - MUX1 (all parents)
>   - parent1; idx=0
>   - parent2; idx=1
>   - parent3; idx=2
> 
> - MUX1 (wanted parents)
>   - parent1; idx=0
>   - parent3; idx=2
> 
> To achieve that add a `parent_index` array pointer to struct mtk_mux,
> then in .set_parent(), .get_parent() callbacks check if this array
> was populated and eventually get the index from that.
> 
> Also, to avoid updating all clock drivers for all SoCs, rename the
> "main" macro to __GATE_CLR_SET_UPD_FLAGS (so, `__` was added) and
> add the new member to it; furthermore, GATE_CLK_SET_UPD_FLAGS has
> been reintroduced as being fully compatible with the older version.
> 
> The new parent_index can be specified with the new `_INDEXED`
> variants of the MUX_GATE_CLR_SET_UPD_xxxx macros.
> 
> Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/clk/mediatek/clk-mux.c | 14 +++++++++++
>  drivers/clk/mediatek/clk-mux.h | 43 ++++++++++++++++++++++++++++++----
>  2 files changed, 53 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/mediatek/clk-mux.c b/drivers/clk/mediatek/clk-mux.c
> index c93bc7f926e5..60990296450b 100644
> --- a/drivers/clk/mediatek/clk-mux.c
> +++ b/drivers/clk/mediatek/clk-mux.c
> @@ -89,6 +89,17 @@ static u8 mtk_clk_mux_get_parent(struct clk_hw *hw)
>  	regmap_read(mux->regmap, mux->data->mux_ofs, &val);
>  	val = (val >> mux->data->mux_shift) & mask;
>  
> +	if (mux->data->parent_index) {
> +		int i;
> +
> +		for (i = 0; i < mux->data->num_parents; i++)
> +			if (mux->data->parent_index[i] == val)
> +				return i;
> +
> +		/* Not found: return an impossible index to generate error */
> +		return mux->data->num_parents + 1;
> +	}
> +
>  	return val;
>  }
>  
> @@ -104,6 +115,9 @@ static int mtk_clk_mux_set_parent_setclr_lock(struct clk_hw *hw, u8 index)
>  	else
>  		__acquire(mux->lock);
>  
> +	if (mux->data->parent_index)
> +		index = mux->data->parent_index[index];
> +
>  	regmap_read(mux->regmap, mux->data->mux_ofs, &orig);
>  	val = (orig & ~(mask << mux->data->mux_shift))
>  			| (index << mux->data->mux_shift);
> diff --git a/drivers/clk/mediatek/clk-mux.h b/drivers/clk/mediatek/clk-mux.h
> index 7ecb963b0ec6..943ad1d7ce4b 100644
> --- a/drivers/clk/mediatek/clk-mux.h
> +++ b/drivers/clk/mediatek/clk-mux.h
> @@ -21,6 +21,7 @@ struct mtk_mux {
>  	int id;
>  	const char *name;
>  	const char * const *parent_names;
> +	const u8 *parent_index;
>  	unsigned int flags;

I think at some point it would be nice to have a documentation of these
fields.

>  
>  	u32 mux_ofs;
> @@ -37,9 +38,10 @@ struct mtk_mux {
>  	signed char num_parents;
>  };
>  
> -#define GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs,		\
> -			_mux_set_ofs, _mux_clr_ofs, _shift, _width,	\
> -			_gate, _upd_ofs, _upd, _flags, _ops) {		\
> +#define __GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents, _paridx,		\
> +			 _num_parents, _mux_ofs, _mux_set_ofs,		\
> +			 _mux_clr_ofs, _shift, _width, _gate, _upd_ofs,	\
> +			 _upd, _flags, _ops) {				\
>  		.id = _id,						\
>  		.name = _name,						\
>  		.mux_ofs = _mux_ofs,					\
> @@ -51,11 +53,28 @@ struct mtk_mux {
>  		.gate_shift = _gate,					\
>  		.upd_shift = _upd,					\
>  		.parent_names = _parents,				\
> -		.num_parents = ARRAY_SIZE(_parents),			\
> +		.parent_index = _paridx,				\
> +		.num_parents = _num_parents,				\

I was wondering why you moved the ARRAY_SIZE() to the outer macros and
add another argument to the already huge list of arguments? I couldn't
find a use-case for this in the patches you sent.

Best,
Markus

>  		.flags = _flags,					\
>  		.ops = &_ops,						\
>  	}
>  
> +#define GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents, _mux_ofs,		\
> +			_mux_set_ofs, _mux_clr_ofs, _shift, _width,	\
> +			_gate, _upd_ofs, _upd, _flags, _ops)		\
> +		__GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents,		\
> +			NULL, ARRAY_SIZE(_parents), _mux_ofs,		\
> +			_mux_set_ofs, _mux_clr_ofs, _shift, _width,	\
> +			_gate, _upd_ofs, _upd, _flags, _ops)		\
> +
> +#define GATE_CLR_SET_UPD_FLAGS_INDEXED(_id, _name, _parents, _paridx,	\
> +			 _mux_ofs, _mux_set_ofs, _mux_clr_ofs, _shift,	\
> +			 _width, _gate, _upd_ofs, _upd, _flags, _ops)	\
> +		__GATE_CLR_SET_UPD_FLAGS(_id, _name, _parents,		\
> +			_paridx, ARRAY_SIZE(_paridx), _mux_ofs,		\
> +			_mux_set_ofs, _mux_clr_ofs, _shift, _width,	\
> +			_gate, _upd_ofs, _upd, _flags, _ops)		\
> +
>  extern const struct clk_ops mtk_mux_clr_set_upd_ops;
>  extern const struct clk_ops mtk_mux_gate_clr_set_upd_ops;
>  
> @@ -67,6 +86,14 @@ extern const struct clk_ops mtk_mux_gate_clr_set_upd_ops;
>  			_gate, _upd_ofs, _upd, _flags,			\
>  			mtk_mux_gate_clr_set_upd_ops)
>  
> +#define MUX_GATE_CLR_SET_UPD_FLAGS_INDEXED(_id, _name, _parents,	\
> +			_paridx, _mux_ofs, _mux_set_ofs, _mux_clr_ofs,	\
> +			_shift, _width, _gate, _upd_ofs, _upd, _flags)	\
> +		GATE_CLR_SET_UPD_FLAGS_INDEXED(_id, _name, _parents,	\
> +			_paridx, _mux_ofs, _mux_set_ofs, _mux_clr_ofs,	\
> +			_shift, _width, _gate, _upd_ofs, _upd, _flags,	\
> +			mtk_mux_gate_clr_set_upd_ops)
> +
>  #define MUX_GATE_CLR_SET_UPD(_id, _name, _parents, _mux_ofs,		\
>  			_mux_set_ofs, _mux_clr_ofs, _shift, _width,	\
>  			_gate, _upd_ofs, _upd)				\
> @@ -75,6 +102,14 @@ extern const struct clk_ops mtk_mux_gate_clr_set_upd_ops;
>  			_width, _gate, _upd_ofs, _upd,			\
>  			CLK_SET_RATE_PARENT)
>  
> +#define MUX_GATE_CLR_SET_UPD_INDEXED(_id, _name, _parents, _paridx,	\
> +			_mux_ofs, _mux_set_ofs, _mux_clr_ofs, _shift,	\
> +			_width, _gate, _upd_ofs, _upd)			\
> +		MUX_GATE_CLR_SET_UPD_FLAGS_INDEXED(_id, _name,		\
> +			_parents, _paridx, _mux_ofs, _mux_set_ofs,	\
> +			_mux_clr_ofs, _shift, _width, _gate, _upd_ofs,	\
> +			_upd, CLK_SET_RATE_PARENT)
> +
>  #define MUX_CLR_SET_UPD(_id, _name, _parents, _mux_ofs,			\
>  			_mux_set_ofs, _mux_clr_ofs, _shift, _width,	\
>  			_upd_ofs, _upd)					\
> -- 
> 2.42.0
>