Re: [PATCH v24 04/15] mmc: renesas_sdhi: Add max_divider field to support SoC-specific clock ranges

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 1721 bytes --]


> v22->v23:
>  * Changed the data type of max_divider from unsigned int to signed int.

You changed this because of a Sashiko report. While the report was
correct, I think the solution of simply using 'int' is suboptimal...

> -	mmc->f_min = max(clk_round_rate(priv->clk, 1) / 512, 1L);
> +	mmc->f_min = max(clk_round_rate(priv->clk, 1) / host->pdata->max_divider, 1L);

... because the existing code is just too subtle / fragile and hard to
understand. I think we should move clk_round_rate() out of max() and
handle it seperatly before. What do you think?

>  	 * greater than, new_clock.  As we can divide by 1 << i for
> -	 * any i in [0, 9] we want the input clock to be as close as
> +	 * any i in [0, {9,11}] we want the input clock to be as close as

Maybe instead of "{9,11}" use "ilog2(max_divider)" or something? This
matches the code and is more future-proof if the divider gets extended
again.

> diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> index c6db0418de15..2bf354331b2d 100644
> --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
> @@ -102,6 +102,7 @@ static const struct renesas_sdhi_of_data of_data_rza2 = {
>  	.max_blk_count	= UINT_MAX / TMIO_MAX_BLK_SIZE,
>  	.max_segs	= 1,
>  	.clk_mask	= SDHI_CLK_MASK_DEFAULT,
> +	.max_divider	= SDHI_MAX_DIVIDER_DEFAULT,

Same comment as for clk_mask, I would not initialize to the default
value.

> @@ -62,5 +62,6 @@ struct tmio_mmc_data {
>  	unsigned int			max_blk_count;
>  	unsigned short			max_segs;
>  	u64				clk_mask;
> +	int				max_divider;

I hope that moving clk_round_rate() out of max() will allow 'unsigned
int' here again.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]