From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net [217.70.183.199]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2CEC5199FAC; Wed, 9 Oct 2024 13:34:49 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.70.183.199 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1728480892; cv=none; b=pzZ7NTkR5h8r20ylQ9kI9fHKHKFCUP1bx9unXjnoJwwG+cioU5TIjhhh+PcIBy9B4hqrEdVt7GbGWAgwuQ/cSaxx/NM9KfkkbdW16BiDYh2zEOMNeEObP6u+8mo+ob6nK7VPyHEyQBMDG+a0fyuZ45DSymrwQr0Ao6EImCWsxC8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1728480892; c=relaxed/simple; bh=/ZTDffNFDZKxF0g3qi5U9UZvxfincxZ36E4W2KgCM5A=; h=Mime-Version:Content-Type:Date:Message-Id:From:Subject:Cc:To: References:In-Reply-To; b=ORp4R5oK/2tgxrAAGuuyXkK9uujkI7P8AkLcXhrB0byjWXmDmvZ9k0N71vTO4j6enMOoIGjJ66xnZ4W1ylSLFjctCdRK77CxqO6Ijg9ypUa9sH73x8ooUYXpsxPhJgcrL0/rDMNmJCl9RZ26IbkYX+EjjFPIblbuVrUaVujiwO0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com; spf=pass smtp.mailfrom=bootlin.com; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b=UxVfI2im; arc=none smtp.client-ip=217.70.183.199 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=bootlin.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b="UxVfI2im" Received: by mail.gandi.net (Postfix) with ESMTPSA id 22F3CFF80B; Wed, 9 Oct 2024 13:34:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1728480888; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=F0MBrQbOMuGChrtybxaDhn3GkZqM1N0vWTYl1Zb6rdQ=; b=UxVfI2imCfvFG2o3d/gCVGRp4YiBJ1/zSytR9U9hdzsUs5s9Tv7kE+Y4EFzomrXLD2rR2/ AqmEvPUznit5LZzXQ2zYw6vlUHJhACH6D5+Xq4AJN3JzpiwofVjJs0wZ/PRiFQEIMBAExH +q5qik0dpm2w2EOgoQrK7qx7DQWjWS1UAyN7tCdLBikaoksD1XAsr3gdgONASQITHJ9DsU IGX97Uy+iVGNAPzSZoNoKo/qfj5KgO6ZMA/l+NpwTHVd3GvUro2+bTdzYaEXiETLbCORhN FN3Bc3UtAqfhZvb+qQAfJdb8W7AMB5/qcsbPKrwpCYINIxtx1iuw4lydId74mA== Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 09 Oct 2024 15:34:47 +0200 Message-Id: From: =?utf-8?q?Th=C3=A9o_Lebrun?= Subject: Re: [PATCH v2 5/6] i2c: nomadik: fix BRCR computation Cc: "Andi Shyti" , "Rob Herring" , "Krzysztof Kozlowski" , "Conor Dooley" , , , , , "Vladimir Kondratiev" , =?utf-8?q?Gr=C3=A9gory_Clement?= , "Thomas Petazzoni" , "Tawfik Bayouk" To: =?utf-8?q?Th=C3=A9o_Lebrun?= , "Linus Walleij" X-Mailer: aerc 0.18.2-0-ge037c095a049 References: <20241009-mbly-i2c-v2-0-ac9230a8dac5@bootlin.com> <20241009-mbly-i2c-v2-5-ac9230a8dac5@bootlin.com> In-Reply-To: X-GND-Sasl: theo.lebrun@bootlin.com On Wed Oct 9, 2024 at 3:31 PM CEST, Th=C3=A9o Lebrun wrote: > On Wed Oct 9, 2024 at 1:34 PM CEST, Linus Walleij wrote: > > On Wed, Oct 9, 2024 at 12:23=E2=80=AFPM Th=C3=A9o Lebrun wrote: > > > --- a/drivers/i2c/busses/i2c-nomadik.c > > > +++ b/drivers/i2c/busses/i2c-nomadik.c > > > @@ -454,9 +454,12 @@ static void setup_i2c_controller(struct nmk_i2c_= dev *priv) > > > * operation, and the other is for std, fast mode, fast mode > > > * plus operation. Currently we do not supprt high speed mode > > > * so set brcr1 to 0. > > > + * > > > + * BRCR is a clock divider amount. Pick highest value that > > > + * leads to rate strictly below target. > > > */ > > > > You could push in some more details from the commit message here so it'= s not > > so terse. > > Most of the details from the commit message come from behavior changes: > what was done previously versus what is the new behavior we implement. > > Having a clock divider picking the bus rate that is below the target > speed rather than above sounds rather intuitive. Eg when you ask for > 400kHz you want <=3D400kHz, not >=3D400kHz. > > I'll add that last sentence "Eg when you ask for 400kHz you want a bus > rate <=3D400kHz (and not >=3D400kHz)". It is straight forward and easy to > understand. > > > > brcr1 =3D FIELD_PREP(I2C_BRCR_BRCNT1, 0); > > > - brcr2 =3D FIELD_PREP(I2C_BRCR_BRCNT2, i2c_clk / (priv->clk_fr= eq * div)); > > > + brcr2 =3D FIELD_PREP(I2C_BRCR_BRCNT2, i2c_clk / (priv->clk_fr= eq * div) + 1); > > > > Doesn't the last part correspond to something like > > #include > > u64 scaler =3D DIV_ROUND_DOWN_ULL(i2c_clk, (priv->clk_freq * div)); > > brcr2 =3D FIELD_PREP(I2C_BRCR_BRCNT2, (u32)scaler); > > > > Certianly one of the in-kernel division helpers like DIV_ROUND_DOWN > > round_up() etc are better to use IMO, but I might not be understanding = the > > fine details of the math here. > > Indeed what we want is: > DIV_ROUND_DOWN(i2c_clk, priv->clk_freq * div) s/DIV_ROUND_DOWN/DIV_ROUND_UP/ (sorry for the confusion) > > I see no reason to use DIV_ROUND_DOWN_ULL(). It would be useful if s/DIV_ROUND_DOWN_ULL/DIV_ROUND_UP_ULL/ > i2c_clk + (priv->clk_freq * div) > had a chance to overflow. > > Worst case is: > 3_400_000 + (48_000_000 * 3) =3D 147_400_000 > > Will send v3 straight away as this is a significant change, > thanks Linus! > > -- > Th=C3=A9o Lebrun, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com Regards, -- Th=C3=A9o Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com