From: Shawn Lin <shawn.lin@rock-chips.com>

Some Rockchip devices put the phase settings into the dw_mmc controller.

When the feature is present, the ciu-drive and ciu-sample clocks are
not used and the phase configuration is done directly through the mmc
controller.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
Acked-by: Shawn Lin <shawn.lin@rock-chips.com>
---
 drivers/mmc/host/dw_mmc-rockchip.c | 171 +++++++++++++++++++++++++++--
 1 file changed, 160 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc-rockchip.c b/drivers/mmc/host/dw_mmc-rockchip.c
index b07190ba4b7a..2748f9bf2691 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -15,7 +15,17 @@
 #include "dw_mmc.h"
 #include "dw_mmc-pltfm.h"
 
-#define RK3288_CLKGEN_DIV	2
+#define RK3288_CLKGEN_DIV		2
+#define SDMMC_TIMING_CON0		0x130
+#define SDMMC_TIMING_CON1		0x134
+#define ROCKCHIP_MMC_DELAY_SEL		BIT(10)
+#define ROCKCHIP_MMC_DEGREE_MASK	0x3
+#define ROCKCHIP_MMC_DEGREE_OFFSET	1
+#define ROCKCHIP_MMC_DELAYNUM_OFFSET	2
+#define ROCKCHIP_MMC_DELAYNUM_MASK	(0xff << ROCKCHIP_MMC_DELAYNUM_OFFSET)
+#define ROCKCHIP_MMC_DELAY_ELEMENT_PSEC	60
+#define HIWORD_UPDATE(val, mask, shift) \
+		((val) << (shift) | (mask) << ((shift) + 16))
 
 static const unsigned int freqs[] = { 100000, 200000, 300000, 400000 };
 
@@ -24,8 +34,143 @@ struct dw_mci_rockchip_priv_data {
 	struct clk		*sample_clk;
 	int			default_sample_phase;
 	int			num_phases;
+	int			internal_phase;
 };
 
+/*
+ * Each fine delay is between 44ps-77ps. Assume each fine delay is 60ps to
+ * simplify calculations. So 45degs could be anywhere between 33deg and 57.8deg.
+ */
+static int rockchip_mmc_get_internal_phase(struct dw_mci *host, bool sample)
+{
+	unsigned long rate = clk_get_rate(host->ciu_clk);
+	u32 raw_value;
+	u16 degrees;
+	u32 delay_num = 0;
+
+	/* Constant signal, no measurable phase shift */
+	if (!rate)
+		return 0;
+
+	if (sample)
+		raw_value = mci_readl(host, TIMING_CON1);
+	else
+		raw_value = mci_readl(host, TIMING_CON0);
+
+	raw_value >>= ROCKCHIP_MMC_DEGREE_OFFSET;
+	degrees = (raw_value & ROCKCHIP_MMC_DEGREE_MASK) * 90;
+
+	if (raw_value & ROCKCHIP_MMC_DELAY_SEL) {
+		/* degrees/delaynum * 1000000 */
+		unsigned long factor = (ROCKCHIP_MMC_DELAY_ELEMENT_PSEC / 10) *
+					36 * (rate / 10000);
+
+		delay_num = (raw_value & ROCKCHIP_MMC_DELAYNUM_MASK);
+		delay_num >>= ROCKCHIP_MMC_DELAYNUM_OFFSET;
+		degrees += DIV_ROUND_CLOSEST(delay_num * factor, 1000000);
+	}
+
+	return degrees % 360;
+}
+
+static int rockchip_mmc_get_phase(struct dw_mci *host, bool sample)
+{
+	struct dw_mci_rockchip_priv_data *priv = host->priv;
+	struct clk *clock = sample ? priv->sample_clk : priv->drv_clk;
+
+	if (priv->internal_phase)
+		return rockchip_mmc_get_internal_phase(host, sample);
+	else
+		return clk_get_phase(clock);
+}
+
+static int rockchip_mmc_set_internal_phase(struct dw_mci *host, bool sample, int degrees)
+{
+	unsigned long rate = clk_get_rate(host->ciu_clk);
+	u8 nineties, remainder;
+	u8 delay_num;
+	u32 raw_value;
+	u32 delay;
+
+	/*
+	 * The below calculation is based on the output clock from
+	 * MMC host to the card, which expects the phase clock inherits
+	 * the clock rate from its parent, namely the output clock
+	 * provider of MMC host. However, things may go wrong if
+	 * (1) It is orphan.
+	 * (2) It is assigned to the wrong parent.
+	 *
+	 * This check help debug the case (1), which seems to be the
+	 * most likely problem we often face and which makes it difficult
+	 * for people to debug unstable mmc tuning results.
+	 */
+	if (!rate) {
+		dev_err(host->dev, "%s: invalid clk rate\n", __func__);
+		return -EINVAL;
+	}
+
+	nineties = degrees / 90;
+	remainder = (degrees % 90);
+
+	/*
+	 * Due to the inexact nature of the "fine" delay, we might
+	 * actually go non-monotonic.  We don't go _too_ monotonic
+	 * though, so we should be OK.  Here are options of how we may
+	 * work:
+	 *
+	 * Ideally we end up with:
+	 *   1.0, 2.0, ..., 69.0, 70.0, ...,  89.0, 90.0
+	 *
+	 * On one extreme (if delay is actually 44ps):
+	 *   .73, 1.5, ..., 50.6, 51.3, ...,  65.3, 90.0
+	 * The other (if delay is actually 77ps):
+	 *   1.3, 2.6, ..., 88.6. 89.8, ..., 114.0, 90
+	 *
+	 * It's possible we might make a delay that is up to 25
+	 * degrees off from what we think we're making.  That's OK
+	 * though because we should be REALLY far from any bad range.
+	 */
+
+	/*
+	 * Convert to delay; do a little extra work to make sure we
+	 * don't overflow 32-bit / 64-bit numbers.
+	 */
+	delay = 10000000; /* PSECS_PER_SEC / 10000 / 10 */
+	delay *= remainder;
+	delay = DIV_ROUND_CLOSEST(delay,
+			(rate / 1000) * 36 *
+				(ROCKCHIP_MMC_DELAY_ELEMENT_PSEC / 10));
+
+	delay_num = (u8) min_t(u32, delay, 255);
+
+	raw_value = delay_num ? ROCKCHIP_MMC_DELAY_SEL : 0;
+	raw_value |= delay_num << ROCKCHIP_MMC_DELAYNUM_OFFSET;
+	raw_value |= nineties;
+
+	if (sample)
+		mci_writel(host, TIMING_CON1, HIWORD_UPDATE(raw_value, 0x07ff, 1));
+	else
+		mci_writel(host, TIMING_CON0, HIWORD_UPDATE(raw_value, 0x07ff, 1));
+
+	dev_dbg(host->dev, "set %s_phase(%d) delay_nums=%u actual_degrees=%d\n",
+		sample ? "sample" : "drv", degrees, delay_num,
+		rockchip_mmc_get_phase(host, sample)
+	);
+
+	return 0;
+}
+
+static int rockchip_mmc_set_phase(struct dw_mci *host, bool sample, int degrees)
+{
+	struct dw_mci_rockchip_priv_data *priv = host->priv;
+	struct clk *clock = sample ? priv->sample_clk : priv->drv_clk;
+
+	if (priv->internal_phase)
+		return rockchip_mmc_set_internal_phase(host, sample, degrees);
+	else
+		return clk_set_phase(clock, degrees);
+}
+
 static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
 {
 	struct dw_mci_rockchip_priv_data *priv = host->priv;
@@ -64,7 +209,7 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
 
 	/* Make sure we use phases which we can enumerate with */
 	if (!IS_ERR(priv->sample_clk) && ios->timing <= MMC_TIMING_SD_HS)
-		clk_set_phase(priv->sample_clk, priv->default_sample_phase);
+		rockchip_mmc_set_phase(host, true, priv->default_sample_phase);
 
 	/*
 	 * Set the drive phase offset based on speed mode to achieve hold times.
@@ -127,7 +272,7 @@ static void dw_mci_rk3288_set_ios(struct dw_mci *host, struct mmc_ios *ios)
 			break;
 		}
 
-		clk_set_phase(priv->drv_clk, phase);
+		rockchip_mmc_set_phase(host, false, phase);
 	}
 }
 
@@ -151,6 +296,7 @@ static int dw_mci_rk3288_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
 	int longest_range_len = -1;
 	int longest_range = -1;
 	int middle_phase;
+	int phase;
 
 	if (IS_ERR(priv->sample_clk)) {
 		dev_err(host->dev, "Tuning clock (sample_clk) not defined.\n");
@@ -164,8 +310,10 @@ static int dw_mci_rk3288_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
 
 	/* Try each phase and extract good ranges */
 	for (i = 0; i < priv->num_phases; ) {
-		clk_set_phase(priv->sample_clk,
-			      TUNING_ITERATION_TO_PHASE(i, priv->num_phases));
+		rockchip_mmc_set_phase(host, true,
+				       TUNING_ITERATION_TO_PHASE(
+						i,
+						priv->num_phases));
 
 		v = !mmc_send_tuning(mmc, opcode, NULL);
 
@@ -211,7 +359,8 @@ static int dw_mci_rk3288_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
 	}
 
 	if (ranges[0].start == 0 && ranges[0].end == priv->num_phases - 1) {
-		clk_set_phase(priv->sample_clk, priv->default_sample_phase);
+		rockchip_mmc_set_phase(host, true, priv->default_sample_phase);
+
 		dev_info(host->dev, "All phases work, using default phase %d.",
 			 priv->default_sample_phase);
 		goto free;
@@ -248,12 +397,10 @@ static int dw_mci_rk3288_execute_tuning(struct dw_mci_slot *slot, u32 opcode)
 
 	middle_phase = ranges[longest_range].start + longest_range_len / 2;
 	middle_phase %= priv->num_phases;
-	dev_info(host->dev, "Successfully tuned phase to %d\n",
-		 TUNING_ITERATION_TO_PHASE(middle_phase, priv->num_phases));
+	phase = TUNING_ITERATION_TO_PHASE(middle_phase, priv->num_phases);
+	dev_info(host->dev, "Successfully tuned phase to %d\n", phase);
 
-	clk_set_phase(priv->sample_clk,
-		      TUNING_ITERATION_TO_PHASE(middle_phase,
-						priv->num_phases));
+	rockchip_mmc_set_phase(host, true, phase);
 
 free:
 	kfree(ranges);
@@ -287,6 +434,8 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci *host)
 
 	host->priv = priv;
 
+	priv->internal_phase = false;
+
 	return 0;
 }
 
-- 
2.46.0

Hello Detlev,

Please see a comment below.

On 2024-08-22 23:15, Detlev Casanova wrote:
> From: Shawn Lin <shawn.lin@rock-chips.com>
> 
> Some Rockchip devices put the phase settings into the dw_mmc 
> controller.
> 
> When the feature is present, the ciu-drive and ciu-sample clocks are
> not used and the phase configuration is done directly through the mmc
> controller.
> 
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> Acked-by: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>  drivers/mmc/host/dw_mmc-rockchip.c | 171 +++++++++++++++++++++++++++--
>  1 file changed, 160 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc-rockchip.c
> b/drivers/mmc/host/dw_mmc-rockchip.c
> index b07190ba4b7a..2748f9bf2691 100644
> --- a/drivers/mmc/host/dw_mmc-rockchip.c
> +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> @@ -15,7 +15,17 @@
>  #include "dw_mmc.h"
>  #include "dw_mmc-pltfm.h"
> 
> -#define RK3288_CLKGEN_DIV	2
> +#define RK3288_CLKGEN_DIV		2
> +#define SDMMC_TIMING_CON0		0x130
> +#define SDMMC_TIMING_CON1		0x134
> +#define ROCKCHIP_MMC_DELAY_SEL		BIT(10)
> +#define ROCKCHIP_MMC_DEGREE_MASK	0x3
> +#define ROCKCHIP_MMC_DEGREE_OFFSET	1
> +#define ROCKCHIP_MMC_DELAYNUM_OFFSET	2
> +#define ROCKCHIP_MMC_DELAYNUM_MASK	(0xff << 
> ROCKCHIP_MMC_DELAYNUM_OFFSET)
> +#define ROCKCHIP_MMC_DELAY_ELEMENT_PSEC	60
> +#define HIWORD_UPDATE(val, mask, shift) \
> +		((val) << (shift) | (mask) << ((shift) + 16))
> 
>  static const unsigned int freqs[] = { 100000, 200000, 300000, 400000 
> };
> 
> @@ -24,8 +34,143 @@ struct dw_mci_rockchip_priv_data {
>  	struct clk		*sample_clk;
>  	int			default_sample_phase;
>  	int			num_phases;
> +	int			internal_phase;
>  };

It might be good to declare internal_phase as "unsigned int 
internal_phase:1",
i.e. as a bit field, which isn't going to save some memory in this 
particular
case, but it would show additional attention to detail.

Hi Dragan,

On Friday, 23 August 2024 01:41:44 EDT Dragan Simic wrote:
> Hello Detlev,
> 
> Please see a comment below.
> 
> On 2024-08-22 23:15, Detlev Casanova wrote:
> > From: Shawn Lin <shawn.lin@rock-chips.com>
> > 
> > Some Rockchip devices put the phase settings into the dw_mmc
> > controller.
> > 
> > When the feature is present, the ciu-drive and ciu-sample clocks are
> > not used and the phase configuration is done directly through the mmc
> > controller.
> > 
> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > Acked-by: Shawn Lin <shawn.lin@rock-chips.com>
> > ---
> > 
> >  drivers/mmc/host/dw_mmc-rockchip.c | 171 +++++++++++++++++++++++++++--
> >  1 file changed, 160 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c
> > b/drivers/mmc/host/dw_mmc-rockchip.c
> > index b07190ba4b7a..2748f9bf2691 100644
> > --- a/drivers/mmc/host/dw_mmc-rockchip.c
> > +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> > @@ -15,7 +15,17 @@
> > 
> >  #include "dw_mmc.h"
> >  #include "dw_mmc-pltfm.h"
> > 
> > -#define RK3288_CLKGEN_DIV	2
> > +#define RK3288_CLKGEN_DIV		2
> > +#define SDMMC_TIMING_CON0		0x130
> > +#define SDMMC_TIMING_CON1		0x134
> > +#define ROCKCHIP_MMC_DELAY_SEL		BIT(10)
> > +#define ROCKCHIP_MMC_DEGREE_MASK	0x3
> > +#define ROCKCHIP_MMC_DEGREE_OFFSET	1
> > +#define ROCKCHIP_MMC_DELAYNUM_OFFSET	2
> > +#define ROCKCHIP_MMC_DELAYNUM_MASK	(0xff <<
> > ROCKCHIP_MMC_DELAYNUM_OFFSET)
> > +#define ROCKCHIP_MMC_DELAY_ELEMENT_PSEC	60
> > +#define HIWORD_UPDATE(val, mask, shift) \
> > +		((val) << (shift) | (mask) << ((shift) + 16))
> > 
> >  static const unsigned int freqs[] = { 100000, 200000, 300000, 400000
> > 
> > };
> > 
> > @@ -24,8 +34,143 @@ struct dw_mci_rockchip_priv_data {
> > 
> >  	struct clk		*sample_clk;
> >  	int			default_sample_phase;
> >  	int			num_phases;
> > 
> > +	int			internal_phase;
> > 
> >  };
> 
> It might be good to declare internal_phase as "unsigned int
> internal_phase:1",
> i.e. as a bit field, which isn't going to save some memory in this
> particular
> case, but it would show additional attention to detail.

In that case, I would go with a bool instead of int, that makes things even 
clearer.

Hello Detlev,

On 2024-08-23 15:34, Detlev Casanova wrote:
> On Friday, 23 August 2024 01:41:44 EDT Dragan Simic wrote:
>> Hello Detlev,
>> 
>> Please see a comment below.
>> 
>> On 2024-08-22 23:15, Detlev Casanova wrote:
>> > From: Shawn Lin <shawn.lin@rock-chips.com>
>> >
>> > Some Rockchip devices put the phase settings into the dw_mmc
>> > controller.
>> >
>> > When the feature is present, the ciu-drive and ciu-sample clocks are
>> > not used and the phase configuration is done directly through the mmc
>> > controller.
>> >
>> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
>> > Acked-by: Shawn Lin <shawn.lin@rock-chips.com>
>> > ---
>> >
>> >  drivers/mmc/host/dw_mmc-rockchip.c | 171 +++++++++++++++++++++++++++--
>> >  1 file changed, 160 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c
>> > b/drivers/mmc/host/dw_mmc-rockchip.c
>> > index b07190ba4b7a..2748f9bf2691 100644
>> > --- a/drivers/mmc/host/dw_mmc-rockchip.c
>> > +++ b/drivers/mmc/host/dw_mmc-rockchip.c
>> > @@ -15,7 +15,17 @@
>> >
>> >  #include "dw_mmc.h"
>> >  #include "dw_mmc-pltfm.h"
>> >
>> > -#define RK3288_CLKGEN_DIV	2
>> > +#define RK3288_CLKGEN_DIV		2
>> > +#define SDMMC_TIMING_CON0		0x130
>> > +#define SDMMC_TIMING_CON1		0x134
>> > +#define ROCKCHIP_MMC_DELAY_SEL		BIT(10)
>> > +#define ROCKCHIP_MMC_DEGREE_MASK	0x3
>> > +#define ROCKCHIP_MMC_DEGREE_OFFSET	1
>> > +#define ROCKCHIP_MMC_DELAYNUM_OFFSET	2
>> > +#define ROCKCHIP_MMC_DELAYNUM_MASK	(0xff <<
>> > ROCKCHIP_MMC_DELAYNUM_OFFSET)
>> > +#define ROCKCHIP_MMC_DELAY_ELEMENT_PSEC	60
>> > +#define HIWORD_UPDATE(val, mask, shift) \
>> > +		((val) << (shift) | (mask) << ((shift) + 16))
>> >
>> >  static const unsigned int freqs[] = { 100000, 200000, 300000, 400000
>> >
>> > };
>> >
>> > @@ -24,8 +34,143 @@ struct dw_mci_rockchip_priv_data {
>> >
>> >  	struct clk		*sample_clk;
>> >  	int			default_sample_phase;
>> >  	int			num_phases;
>> >
>> > +	int			internal_phase;
>> >
>> >  };
>> 
>> It might be good to declare internal_phase as "unsigned int
>> internal_phase:1",
>> i.e. as a bit field, which isn't going to save some memory in this
>> particular
>> case, but it would show additional attention to detail.
> 
> In that case, I would go with a bool instead of int, that makes things
> even clearer.

My suggestion to use "unsigned int internal_phase:1" actually takes
inspiration from the ASoC code, in which such bit fields are used
quite a lot, even when using them actually doesn't save space.

In this particular case, using plain bool would make sense, but I
still think that using an "unsigned int internal_phase:1" bit field
would fit better, because it would show the intention to possibly
save a bit of RAM at some point.  OTOH, I don't think that using
bool with such bit fields would actually work cleanly, because bool
actually resolves to int that's a signed type.

On Monday, 26 August 2024 10:39:58 EDT Dragan Simic wrote:
> Hello Detlev,
> 
> On 2024-08-23 15:34, Detlev Casanova wrote:
> > On Friday, 23 August 2024 01:41:44 EDT Dragan Simic wrote:
> >> Hello Detlev,
> >> 
> >> Please see a comment below.
> >> 
> >> On 2024-08-22 23:15, Detlev Casanova wrote:
> >> > From: Shawn Lin <shawn.lin@rock-chips.com>
> >> > 
> >> > Some Rockchip devices put the phase settings into the dw_mmc
> >> > controller.
> >> > 
> >> > When the feature is present, the ciu-drive and ciu-sample clocks are
> >> > not used and the phase configuration is done directly through the mmc
> >> > controller.
> >> > 
> >> > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
> >> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> >> > Acked-by: Shawn Lin <shawn.lin@rock-chips.com>
> >> > ---
> >> > 
> >> >  drivers/mmc/host/dw_mmc-rockchip.c | 171 +++++++++++++++++++++++++++--
> >> >  1 file changed, 160 insertions(+), 11 deletions(-)
> >> > 
> >> > diff --git a/drivers/mmc/host/dw_mmc-rockchip.c
> >> > b/drivers/mmc/host/dw_mmc-rockchip.c
> >> > index b07190ba4b7a..2748f9bf2691 100644
> >> > --- a/drivers/mmc/host/dw_mmc-rockchip.c
> >> > +++ b/drivers/mmc/host/dw_mmc-rockchip.c
> >> > @@ -15,7 +15,17 @@
> >> > 
> >> >  #include "dw_mmc.h"
> >> >  #include "dw_mmc-pltfm.h"
> >> > 
> >> > -#define RK3288_CLKGEN_DIV	2
> >> > +#define RK3288_CLKGEN_DIV		2
> >> > +#define SDMMC_TIMING_CON0		0x130
> >> > +#define SDMMC_TIMING_CON1		0x134
> >> > +#define ROCKCHIP_MMC_DELAY_SEL		BIT(10)
> >> > +#define ROCKCHIP_MMC_DEGREE_MASK	0x3
> >> > +#define ROCKCHIP_MMC_DEGREE_OFFSET	1
> >> > +#define ROCKCHIP_MMC_DELAYNUM_OFFSET	2
> >> > +#define ROCKCHIP_MMC_DELAYNUM_MASK	(0xff <<
> >> > ROCKCHIP_MMC_DELAYNUM_OFFSET)
> >> > +#define ROCKCHIP_MMC_DELAY_ELEMENT_PSEC	60
> >> > +#define HIWORD_UPDATE(val, mask, shift) \
> >> > +		((val) << (shift) | (mask) << ((shift) + 16))
> >> > 
> >> >  static const unsigned int freqs[] = { 100000, 200000, 300000, 400000
> >> > 
> >> > };
> >> > 
> >> > @@ -24,8 +34,143 @@ struct dw_mci_rockchip_priv_data {
> >> > 
> >> >  	struct clk		*sample_clk;
> >> >  	int			default_sample_phase;
> >> >  	int			num_phases;
> >> > 
> >> > +	int			internal_phase;
> >> > 
> >> >  };
> >> 
> >> It might be good to declare internal_phase as "unsigned int
> >> internal_phase:1",
> >> i.e. as a bit field, which isn't going to save some memory in this
> >> particular
> >> case, but it would show additional attention to detail.
> > 
> > In that case, I would go with a bool instead of int, that makes things
> > even clearer.
> 
> My suggestion to use "unsigned int internal_phase:1" actually takes
> inspiration from the ASoC code, in which such bit fields are used
> quite a lot, even when using them actually doesn't save space.
> 
> In this particular case, using plain bool would make sense, but I
> still think that using an "unsigned int internal_phase:1" bit field
> would fit better, because it would show the intention to possibly
> save a bit of RAM at some point.  OTOH, I don't think that using
> bool with such bit fields would actually work cleanly, because bool
> actually resolves to int that's a signed type.

I wouldn't use bool with a bit field of course. I've always considered using 
bit fileds only for structs that must have a certain format, like a header 
format definition.

For me, it is better to use "bool internal_phase" so that it is obvious that 
the feature can be on or off when reading the code.

When using bit fields with a struct that is not marked as "__packed", I 
immediately think that there could be a bug there and wonder why the bit field 
is used, not really thinking "the dev wanted to show they cared about memory 
usage".
But I guess that is all about preferences. In the end, it won't change how it 
works.

Detlev.