| Message ID | 20240726194948.109326-1-macroalpha82@gmail.com |
|---|---|
| Headers | show |
| Series | Add GameForce Ace | expand |
Hi, On Fri, Jul 26, 2024 at 02:49:45PM GMT, Chris Morgan wrote: > From: Chris Morgan <macromorgan@hotmail.com> > > The Cellwise cw2015 datasheet reports that it can handle two cells > in a series configuration. Allow a device tree parameter to note > this condition so that the driver reports the correct voltage values > to userspace. I found this: http://www.cellwise-semi.com/Public/assests/menu/20230302/6400076806706.pdf Which says: CW2015 can be used in 2 or more batteries connected in series, or several cells connected in parallel. So dual-cell seems like a bad property name. Instead the number of serial cells should be provided. This property is then not really specific to the Cellwise fuel gauge and instead a property of the battery pack (i.e. simple-battery.yaml). > Signed-off-by: Chris Morgan <macromorgan@hotmail.com> > --- > drivers/power/supply/cw2015_battery.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/power/supply/cw2015_battery.c b/drivers/power/supply/cw2015_battery.c > index f63c3c410451..b23a6d4fa4fa 100644 > --- a/drivers/power/supply/cw2015_battery.c > +++ b/drivers/power/supply/cw2015_battery.c > @@ -77,6 +77,8 @@ struct cw_battery { > u32 poll_interval_ms; > u8 alert_level; > > + bool dual_cell; > + > unsigned int read_errors; > unsigned int charge_stuck_cnt; > }; > @@ -325,6 +327,9 @@ static int cw_get_voltage(struct cw_battery *cw_bat) > */ > voltage_mv = avg * 312 / 1024; > > + if (cw_bat->dual_cell) > + voltage_mv *= 2; Unfortunately there are no details in the document, but this looks very fishy. Does it only measure the first cell and hope that the other cells have the same voltage? This (unmerged) series also applies to your problem to some degree: https://lore.kernel.org/all/20240416121818.543896-3-mike.looijmans@topic.nl/ -- Sebastian > dev_dbg(cw_bat->dev, "Read voltage: %d mV, raw=0x%04x\n", > voltage_mv, reg_val); > return voltage_mv; > @@ -587,6 +592,8 @@ static int cw2015_parse_properties(struct cw_battery *cw_bat) > return ret; > } > > + cw_bat->dual_cell = device_property_read_bool(dev, "cellwise,dual-cell"); > + > ret = device_property_read_u32(dev, "cellwise,monitor-interval-ms", > &cw_bat->poll_interval_ms); > if (ret) { > -- > 2.34.1 > >
Am Freitag, 26. Juli 2024, 21:49:46 CEST schrieb Chris Morgan: > From: Chris Morgan <macromorgan@hotmail.com> > > When using an Ampak derived bcm43456 on an RK3588s based GameForce Ace > the WiFi failed to work properly until I set the SDIO pins from > pull-none to pull-up. This matches the vendor kernel located at [1]. > I tested this then on an RK3588s based Indiedroid Nova and did not > observe any adverse effects. > > [1] https://github.com/rockchip-linux/kernel/commit/b96485b7af46a99c14f3c4818eb18c7836eb809c As you're essentially duplicating the change by Alex Zhao it might be nice to keep their authorship? So, "From" + first Signed-off from Alex, then you add a [adapted to pinctrl filename change] Signed-off-by: Chris Morgan <macromorgan@hotmail.com> below that. > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com> > --- > arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi > index 30db12c4fc82..d1368418502a 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi > @@ -2449,15 +2449,15 @@ sdiom1_pins: sdiom1-pins { > /* sdio_clk_m1 */ > <3 RK_PA5 2 &pcfg_pull_none>, > /* sdio_cmd_m1 */ > - <3 RK_PA4 2 &pcfg_pull_none>, > + <3 RK_PA4 2 &pcfg_pull_up>, > /* sdio_d0_m1 */ > - <3 RK_PA0 2 &pcfg_pull_none>, > + <3 RK_PA0 2 &pcfg_pull_up>, > /* sdio_d1_m1 */ > - <3 RK_PA1 2 &pcfg_pull_none>, > + <3 RK_PA1 2 &pcfg_pull_up>, > /* sdio_d2_m1 */ > - <3 RK_PA2 2 &pcfg_pull_none>, > + <3 RK_PA2 2 &pcfg_pull_up>, > /* sdio_d3_m1 */ > - <3 RK_PA3 2 &pcfg_pull_none>; > + <3 RK_PA3 2 &pcfg_pull_up>; > }; > }; > >
On Fri, 26 Jul 2024 14:49:43 -0500, Chris Morgan wrote: > From: Chris Morgan <macromorgan@hotmail.com> > > Add support for the GameForce Ace. The GameForce Ace is an RK3588s > based gaming device with a 1080p display, touchscreen, hall effect > joysticks and triggers, 128GB of eMMC, 8GB or 12GB of RAM, WiFi 5, > and support for a 2242 NVME. > > Chris Morgan (5): > dt-bindings: power: supply: add dual-cell for cw2015 > power: supply: cw2015: Add support for dual-cell configurations > arm64: dts: rockchip: Pull up sdio pins on RK3588 > dt-bindings: arm: rockchip: Add GameForce Ace > arm64: dts: rockchip: Add GameForce Ace > > .../devicetree/bindings/arm/rockchip.yaml | 5 + > .../bindings/power/supply/cw2015_battery.yaml | 6 + > arch/arm64/boot/dts/rockchip/Makefile | 1 + > .../dts/rockchip/rk3588-base-pinctrl.dtsi | 10 +- > .../dts/rockchip/rk3588s-gameforce-ace.dts | 1315 +++++++++++++++++ > drivers/power/supply/cw2015_battery.c | 7 + > 6 files changed, 1339 insertions(+), 5 deletions(-) > create mode 100644 arch/arm64/boot/dts/rockchip/rk3588s-gameforce-ace.dts > > -- > 2.34.1 > > > My bot found new DTB warnings on the .dts files added or changed in this series. Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings are fixed by another series. Ultimately, it is up to the platform maintainer whether these warnings are acceptable or not. No need to reply unless the platform maintainer has comments. If you already ran DT checks and didn't see these error(s), then make sure dt-schema is up to date: pip3 install dtschema --upgrade New warnings running 'make CHECK_DTBS=y rockchip/rk3588s-gameforce-ace.dtb' for 20240726194948.109326-1-macroalpha82@gmail.com: arch/arm64/boot/dts/rockchip/rk3588s-gameforce-ace.dtb: typec-portc@22: 'vbus-supply' is a required property from schema $id: http://devicetree.org/schemas/usb/fcs,fusb302.yaml#
On Fri, Jul 26, 2024 at 11:42:28PM +0200, Heiko Stübner wrote: > Am Freitag, 26. Juli 2024, 21:49:46 CEST schrieb Chris Morgan: > > From: Chris Morgan <macromorgan@hotmail.com> > > > > When using an Ampak derived bcm43456 on an RK3588s based GameForce Ace > > the WiFi failed to work properly until I set the SDIO pins from > > pull-none to pull-up. This matches the vendor kernel located at [1]. > > I tested this then on an RK3588s based Indiedroid Nova and did not > > observe any adverse effects. > > > > [1] https://github.com/rockchip-linux/kernel/commit/b96485b7af46a99c14f3c4818eb18c7836eb809c > > As you're essentially duplicating the change by Alex Zhao it might be > nice to keep their authorship? > > So, "From" + first Signed-off from Alex, then you add a > [adapted to pinctrl filename change] > Signed-off-by: Chris Morgan <macromorgan@hotmail.com> > > below that. Thank you, that was an oversight but I'll get it done in v2. > > > > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com> > > --- > > arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi > > index 30db12c4fc82..d1368418502a 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-base-pinctrl.dtsi > > @@ -2449,15 +2449,15 @@ sdiom1_pins: sdiom1-pins { > > /* sdio_clk_m1 */ > > <3 RK_PA5 2 &pcfg_pull_none>, > > /* sdio_cmd_m1 */ > > - <3 RK_PA4 2 &pcfg_pull_none>, > > + <3 RK_PA4 2 &pcfg_pull_up>, > > /* sdio_d0_m1 */ > > - <3 RK_PA0 2 &pcfg_pull_none>, > > + <3 RK_PA0 2 &pcfg_pull_up>, > > /* sdio_d1_m1 */ > > - <3 RK_PA1 2 &pcfg_pull_none>, > > + <3 RK_PA1 2 &pcfg_pull_up>, > > /* sdio_d2_m1 */ > > - <3 RK_PA2 2 &pcfg_pull_none>, > > + <3 RK_PA2 2 &pcfg_pull_up>, > > /* sdio_d3_m1 */ > > - <3 RK_PA3 2 &pcfg_pull_none>; > > + <3 RK_PA3 2 &pcfg_pull_up>; > > }; > > }; > > > > > > > >
On Mon, Jul 29, 2024 at 12:52:50PM -0600, Rob Herring (Arm) wrote: > > On Fri, 26 Jul 2024 14:49:43 -0500, Chris Morgan wrote: > > From: Chris Morgan <macromorgan@hotmail.com> > > > > Add support for the GameForce Ace. The GameForce Ace is an RK3588s > > based gaming device with a 1080p display, touchscreen, hall effect > > joysticks and triggers, 128GB of eMMC, 8GB or 12GB of RAM, WiFi 5, > > and support for a 2242 NVME. > > > > Chris Morgan (5): > > dt-bindings: power: supply: add dual-cell for cw2015 > > power: supply: cw2015: Add support for dual-cell configurations > > arm64: dts: rockchip: Pull up sdio pins on RK3588 > > dt-bindings: arm: rockchip: Add GameForce Ace > > arm64: dts: rockchip: Add GameForce Ace > > > > .../devicetree/bindings/arm/rockchip.yaml | 5 + > > .../bindings/power/supply/cw2015_battery.yaml | 6 + > > arch/arm64/boot/dts/rockchip/Makefile | 1 + > > .../dts/rockchip/rk3588-base-pinctrl.dtsi | 10 +- > > .../dts/rockchip/rk3588s-gameforce-ace.dts | 1315 +++++++++++++++++ > > drivers/power/supply/cw2015_battery.c | 7 + > > 6 files changed, 1339 insertions(+), 5 deletions(-) > > create mode 100644 arch/arm64/boot/dts/rockchip/rk3588s-gameforce-ace.dts > > > > -- > > 2.34.1 > > > > > > > > > My bot found new DTB warnings on the .dts files added or changed in this > series. > > Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings > are fixed by another series. Ultimately, it is up to the platform > maintainer whether these warnings are acceptable or not. No need to reply > unless the platform maintainer has comments. > > If you already ran DT checks and didn't see these error(s), then > make sure dt-schema is up to date: > > pip3 install dtschema --upgrade > > > New warnings running 'make CHECK_DTBS=y rockchip/rk3588s-gameforce-ace.dtb' for 20240726194948.109326-1-macroalpha82@gmail.com: > > arch/arm64/boot/dts/rockchip/rk3588s-gameforce-ace.dtb: typec-portc@22: 'vbus-supply' is a required property > from schema $id: http://devicetree.org/schemas/usb/fcs,fusb302.yaml# > For the moment this is expected, because the regulator is driven by a TI BQ25703 chip that I'm still working on getting supported. Is it preferable to either keep it empty (and cause this error) or fill it with a dummy regulator for now? Thank you. > > > >
On Fri, Jul 26, 2024 at 11:06:21PM +0200, Sebastian Reichel wrote: > Hi, > > On Fri, Jul 26, 2024 at 02:49:45PM GMT, Chris Morgan wrote: > > From: Chris Morgan <macromorgan@hotmail.com> > > > > The Cellwise cw2015 datasheet reports that it can handle two cells > > in a series configuration. Allow a device tree parameter to note > > this condition so that the driver reports the correct voltage values > > to userspace. > > I found this: > > http://www.cellwise-semi.com/Public/assests/menu/20230302/6400076806706.pdf > > Which says: > > CW2015 can be used in 2 or more batteries connected in series, or > several cells connected in parallel. > > So dual-cell seems like a bad property name. Instead the number of > serial cells should be provided. This property is then not really > specific to the Cellwise fuel gauge and instead a property of the > battery pack (i.e. simple-battery.yaml). It's conflicting information (which further confuses me). I see in that datasheet it says 2 or more, whereas the datasheet found here says only 2 cells: https://www.lestat.st/_media/informatique/projets/python-cw2015/cw2015-power-management-datasheet.pdf But I agree in principle that we should be setting this as a property of a simple-battery rather than a manufacturer specific parameter. > > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com> > > --- > > drivers/power/supply/cw2015_battery.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/power/supply/cw2015_battery.c b/drivers/power/supply/cw2015_battery.c > > index f63c3c410451..b23a6d4fa4fa 100644 > > --- a/drivers/power/supply/cw2015_battery.c > > +++ b/drivers/power/supply/cw2015_battery.c > > @@ -77,6 +77,8 @@ struct cw_battery { > > u32 poll_interval_ms; > > u8 alert_level; > > > > + bool dual_cell; > > + > > unsigned int read_errors; > > unsigned int charge_stuck_cnt; > > }; > > @@ -325,6 +327,9 @@ static int cw_get_voltage(struct cw_battery *cw_bat) > > */ > > voltage_mv = avg * 312 / 1024; > > > > + if (cw_bat->dual_cell) > > + voltage_mv *= 2; > > Unfortunately there are no details in the document, but this looks > very fishy. Does it only measure the first cell and hope that the > other cells have the same voltage? > > This (unmerged) series also applies to your problem to some degree: > > https://lore.kernel.org/all/20240416121818.543896-3-mike.looijmans@topic.nl/ I think based on the application diagram it is in fact measuring the cell voltage. That said, this ultimately boils down to a cosmetic thing as not having this property simply means userspace sees the battery voltage as 3.8v instead of 7.6v as is written on the side. I think for simplification sake I will remove this from the series, add a note to the device tree, and wait for the other battery series to get pulled. When it gets pulled I'll request a device tree property so we can add POWER_SUPPLY_PROP_NUMBER_OF_SERIAL_CELLS to the simple-battery and then parse this value. Or if that series ends up getting abandoned I can just add a quick and dirty new property like this. Thank you, Chris > > -- Sebastian > > > dev_dbg(cw_bat->dev, "Read voltage: %d mV, raw=0x%04x\n", > > voltage_mv, reg_val); > > return voltage_mv; > > @@ -587,6 +592,8 @@ static int cw2015_parse_properties(struct cw_battery *cw_bat) > > return ret; > > } > > > > + cw_bat->dual_cell = device_property_read_bool(dev, "cellwise,dual-cell"); > > + > > ret = device_property_read_u32(dev, "cellwise,monitor-interval-ms", > > &cw_bat->poll_interval_ms); > > if (ret) { > > -- > > 2.34.1 > > > >
Hi, On Wed, Jul 31, 2024 at 11:02:11AM GMT, Chris Morgan wrote: > On Fri, Jul 26, 2024 at 11:06:21PM +0200, Sebastian Reichel wrote: > > Hi, > > > > On Fri, Jul 26, 2024 at 02:49:45PM GMT, Chris Morgan wrote: > > > From: Chris Morgan <macromorgan@hotmail.com> > > > > > > The Cellwise cw2015 datasheet reports that it can handle two cells > > > in a series configuration. Allow a device tree parameter to note > > > this condition so that the driver reports the correct voltage values > > > to userspace. > > > > I found this: > > > > http://www.cellwise-semi.com/Public/assests/menu/20230302/6400076806706.pdf > > > > Which says: > > > > CW2015 can be used in 2 or more batteries connected in series, or > > several cells connected in parallel. > > > > So dual-cell seems like a bad property name. Instead the number of > > serial cells should be provided. This property is then not really > > specific to the Cellwise fuel gauge and instead a property of the > > battery pack (i.e. simple-battery.yaml). > > It's conflicting information (which further confuses me). I see in that > datasheet it says 2 or more, whereas the datasheet found here says only > 2 cells: > > https://www.lestat.st/_media/informatique/projets/python-cw2015/cw2015-power-management-datasheet.pdf > > But I agree in principle that we should be setting this as a property > of a simple-battery rather than a manufacturer specific parameter. > > > > > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com> > > > --- > > > drivers/power/supply/cw2015_battery.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/drivers/power/supply/cw2015_battery.c b/drivers/power/supply/cw2015_battery.c > > > index f63c3c410451..b23a6d4fa4fa 100644 > > > --- a/drivers/power/supply/cw2015_battery.c > > > +++ b/drivers/power/supply/cw2015_battery.c > > > @@ -77,6 +77,8 @@ struct cw_battery { > > > u32 poll_interval_ms; > > > u8 alert_level; > > > > > > + bool dual_cell; > > > + > > > unsigned int read_errors; > > > unsigned int charge_stuck_cnt; > > > }; > > > @@ -325,6 +327,9 @@ static int cw_get_voltage(struct cw_battery *cw_bat) > > > */ > > > voltage_mv = avg * 312 / 1024; > > > > > > + if (cw_bat->dual_cell) > > > + voltage_mv *= 2; > > > > Unfortunately there are no details in the document, but this looks > > very fishy. Does it only measure the first cell and hope that the > > other cells have the same voltage? > > > > This (unmerged) series also applies to your problem to some degree: > > > > https://lore.kernel.org/all/20240416121818.543896-3-mike.looijmans@topic.nl/ > > I think based on the application diagram it is in fact measuring the > cell voltage. > > That said, this ultimately boils down to a cosmetic thing > as not having this property simply means userspace sees the battery > voltage as 3.8v instead of 7.6v as is written on the side. With the cells being connected in serial, the voltage of both cells can be different. There is not "the cell voltage". Instead there is a voltage for cell 1 and a voltage for cell 2. In a perfect battery they are the same, but in reality they are not. In the extreme case one of the cells may be broken while the other is still fine. It sounds as if this is just measuring the voltage from the first cell and assumes the second cell has the same voltage. Ideally the voltage on these platforms is not exposed via the normal VOLTAGE property and instead uses a new property for telling userspace the voltage for a single cell. The kernel simply does not know the voltage of the whole battery pack. FWIW this is the worst battery measurement system I've seen so far if my understanding of the hardware design is correct. -- Sebastian > I think for simplification sake I will remove this from the series, add > a note to the device tree, and wait for the other battery series to get > pulled. When it gets pulled I'll request a device tree property so we > can add POWER_SUPPLY_PROP_NUMBER_OF_SERIAL_CELLS to the simple-battery > and then parse this value. Or if that series ends up getting abandoned > I can just add a quick and dirty new property like this. > > Thank you, > Chris > > > > > -- Sebastian > > > > > dev_dbg(cw_bat->dev, "Read voltage: %d mV, raw=0x%04x\n", > > > voltage_mv, reg_val); > > > return voltage_mv; > > > @@ -587,6 +592,8 @@ static int cw2015_parse_properties(struct cw_battery *cw_bat) > > > return ret; > > > } > > > > > > + cw_bat->dual_cell = device_property_read_bool(dev, "cellwise,dual-cell"); > > > + > > > ret = device_property_read_u32(dev, "cellwise,monitor-interval-ms", > > > &cw_bat->poll_interval_ms); > > > if (ret) { > > > -- > > > 2.34.1 > > > > > > > >
On Wed, Jul 31, 2024 at 07:02:28PM +0200, Sebastian Reichel wrote: > Hi, > > On Wed, Jul 31, 2024 at 11:02:11AM GMT, Chris Morgan wrote: > > On Fri, Jul 26, 2024 at 11:06:21PM +0200, Sebastian Reichel wrote: > > > Hi, > > > > > > On Fri, Jul 26, 2024 at 02:49:45PM GMT, Chris Morgan wrote: > > > > From: Chris Morgan <macromorgan@hotmail.com> > > > > > > > > The Cellwise cw2015 datasheet reports that it can handle two cells > > > > in a series configuration. Allow a device tree parameter to note > > > > this condition so that the driver reports the correct voltage values > > > > to userspace. > > > > > > I found this: > > > > > > http://www.cellwise-semi.com/Public/assests/menu/20230302/6400076806706.pdf > > > > > > Which says: > > > > > > CW2015 can be used in 2 or more batteries connected in series, or > > > several cells connected in parallel. > > > > > > So dual-cell seems like a bad property name. Instead the number of > > > serial cells should be provided. This property is then not really > > > specific to the Cellwise fuel gauge and instead a property of the > > > battery pack (i.e. simple-battery.yaml). > > > > It's conflicting information (which further confuses me). I see in that > > datasheet it says 2 or more, whereas the datasheet found here says only > > 2 cells: > > > > https://www.lestat.st/_media/informatique/projets/python-cw2015/cw2015-power-management-datasheet.pdf > > > > But I agree in principle that we should be setting this as a property > > of a simple-battery rather than a manufacturer specific parameter. > > > > > > > > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com> > > > > --- > > > > drivers/power/supply/cw2015_battery.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/drivers/power/supply/cw2015_battery.c b/drivers/power/supply/cw2015_battery.c > > > > index f63c3c410451..b23a6d4fa4fa 100644 > > > > --- a/drivers/power/supply/cw2015_battery.c > > > > +++ b/drivers/power/supply/cw2015_battery.c > > > > @@ -77,6 +77,8 @@ struct cw_battery { > > > > u32 poll_interval_ms; > > > > u8 alert_level; > > > > > > > > + bool dual_cell; > > > > + > > > > unsigned int read_errors; > > > > unsigned int charge_stuck_cnt; > > > > }; > > > > @@ -325,6 +327,9 @@ static int cw_get_voltage(struct cw_battery *cw_bat) > > > > */ > > > > voltage_mv = avg * 312 / 1024; > > > > > > > > + if (cw_bat->dual_cell) > > > > + voltage_mv *= 2; > > > > > > Unfortunately there are no details in the document, but this looks > > > very fishy. Does it only measure the first cell and hope that the > > > other cells have the same voltage? > > > > > > This (unmerged) series also applies to your problem to some degree: > > > > > > https://lore.kernel.org/all/20240416121818.543896-3-mike.looijmans@topic.nl/ > > > > I think based on the application diagram it is in fact measuring the > > cell voltage. > > > > That said, this ultimately boils down to a cosmetic thing > > as not having this property simply means userspace sees the battery > > voltage as 3.8v instead of 7.6v as is written on the side. > > With the cells being connected in serial, the voltage of both cells > can be different. There is not "the cell voltage". Instead there is > a voltage for cell 1 and a voltage for cell 2. In a perfect battery > they are the same, but in reality they are not. In the extreme case > one of the cells may be broken while the other is still fine. It > sounds as if this is just measuring the voltage from the first > cell and assumes the second cell has the same voltage. > > Ideally the voltage on these platforms is not exposed via the normal > VOLTAGE property and instead uses a new property for telling > userspace the voltage for a single cell. The kernel simply does not > know the voltage of the whole battery pack. > > FWIW this is the worst battery measurement system I've seen so far > if my understanding of the hardware design is correct. > > -- Sebastian > Don't get me started, this whole stack is a nightmare. The battery won't charge at all without a bq25703 driver (so this current DTS basically has the device running while plugged in); and for some reason the fcs,fc302 chip wants to go interrupt crazy if you're not using a PD charger. Part of the reason of getting this upstreamed is so I can have a common base for others to help me get this last bit of nonsense sorted out. Displaying the correct voltage on the battery however isn't critical, as the device will still report the correct SoC and will even charge and function just fine when a driver is in place for the bq25703. It's just the voltage reported in sysfs will be wrong. Thank you, Chris > > I think for simplification sake I will remove this from the series, add > > a note to the device tree, and wait for the other battery series to get > > pulled. When it gets pulled I'll request a device tree property so we > > can add POWER_SUPPLY_PROP_NUMBER_OF_SERIAL_CELLS to the simple-battery > > and then parse this value. Or if that series ends up getting abandoned > > I can just add a quick and dirty new property like this. > > > > > > Thank you, > > Chris > > > > > > > > -- Sebastian > > > > > > > dev_dbg(cw_bat->dev, "Read voltage: %d mV, raw=0x%04x\n", > > > > voltage_mv, reg_val); > > > > return voltage_mv; > > > > @@ -587,6 +592,8 @@ static int cw2015_parse_properties(struct cw_battery *cw_bat) > > > > return ret; > > > > } > > > > > > > > + cw_bat->dual_cell = device_property_read_bool(dev, "cellwise,dual-cell"); > > > > + > > > > ret = device_property_read_u32(dev, "cellwise,monitor-interval-ms", > > > > &cw_bat->poll_interval_ms); > > > > if (ret) { > > > > -- > > > > 2.34.1 > > > > > > > > > > > >
Hello all, On 2024-07-31 19:02, Sebastian Reichel wrote: > On Wed, Jul 31, 2024 at 11:02:11AM GMT, Chris Morgan wrote: >> On Fri, Jul 26, 2024 at 11:06:21PM +0200, Sebastian Reichel wrote: >> > On Fri, Jul 26, 2024 at 02:49:45PM GMT, Chris Morgan wrote: >> > > From: Chris Morgan <macromorgan@hotmail.com> >> > > >> > > The Cellwise cw2015 datasheet reports that it can handle two cells >> > > in a series configuration. Allow a device tree parameter to note >> > > this condition so that the driver reports the correct voltage values >> > > to userspace. >> > >> > I found this: >> > >> > http://www.cellwise-semi.com/Public/assests/menu/20230302/6400076806706.pdf >> > >> > Which says: >> > >> > CW2015 can be used in 2 or more batteries connected in series, or >> > several cells connected in parallel. >> > >> > So dual-cell seems like a bad property name. Instead the number of >> > serial cells should be provided. This property is then not really >> > specific to the Cellwise fuel gauge and instead a property of the >> > battery pack (i.e. simple-battery.yaml). >> >> It's conflicting information (which further confuses me). I see in >> that >> datasheet it says 2 or more, whereas the datasheet found here says >> only >> 2 cells: >> >> https://www.lestat.st/_media/informatique/projets/python-cw2015/cw2015-power-management-datasheet.pdf >> >> But I agree in principle that we should be setting this as a property >> of a simple-battery rather than a manufacturer specific parameter. >> >> > >> > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com> >> > > --- >> > > drivers/power/supply/cw2015_battery.c | 7 +++++++ >> > > 1 file changed, 7 insertions(+) >> > > >> > > diff --git a/drivers/power/supply/cw2015_battery.c b/drivers/power/supply/cw2015_battery.c >> > > index f63c3c410451..b23a6d4fa4fa 100644 >> > > --- a/drivers/power/supply/cw2015_battery.c >> > > +++ b/drivers/power/supply/cw2015_battery.c >> > > @@ -77,6 +77,8 @@ struct cw_battery { >> > > u32 poll_interval_ms; >> > > u8 alert_level; >> > > >> > > + bool dual_cell; >> > > + >> > > unsigned int read_errors; >> > > unsigned int charge_stuck_cnt; >> > > }; >> > > @@ -325,6 +327,9 @@ static int cw_get_voltage(struct cw_battery *cw_bat) >> > > */ >> > > voltage_mv = avg * 312 / 1024; >> > > >> > > + if (cw_bat->dual_cell) >> > > + voltage_mv *= 2; >> > >> > Unfortunately there are no details in the document, but this looks >> > very fishy. Does it only measure the first cell and hope that the >> > other cells have the same voltage? >> > >> > This (unmerged) series also applies to your problem to some degree: >> > >> > https://lore.kernel.org/all/20240416121818.543896-3-mike.looijmans@topic.nl/ >> >> I think based on the application diagram it is in fact measuring the >> cell voltage. >> >> That said, this ultimately boils down to a cosmetic thing >> as not having this property simply means userspace sees the battery >> voltage as 3.8v instead of 7.6v as is written on the side. > > With the cells being connected in serial, the voltage of both cells > can be different. There is not "the cell voltage". Instead there is > a voltage for cell 1 and a voltage for cell 2. In a perfect battery > they are the same, but in reality they are not. In the extreme case > one of the cells may be broken while the other is still fine. It > sounds as if this is just measuring the voltage from the first > cell and assumes the second cell has the same voltage. > > Ideally the voltage on these platforms is not exposed via the normal > VOLTAGE property and instead uses a new property for telling > userspace the voltage for a single cell. The kernel simply does not > know the voltage of the whole battery pack. > > FWIW this is the worst battery measurement system I've seen so far > if my understanding of the hardware design is correct. Please note that two facts should be considered here: - The GenBook schematic [1] clearly shows that the individual battery cells aren't exposed at its internal battery connector and, as a result, aren't available for individual cell voltage monitoring - The GenBook uses a CW2013 as it fuel gauge, [1] instead of CW2015 as mentioned here a few times, but I haven't went through the CW2013 datasheet(s) yet to see what are the actual differences between it and the CW2015 [1] https://wiki.cool-pi.com/notebook/coolpi-genbook-v20.pdf >> I think for simplification sake I will remove this from the series, >> add >> a note to the device tree, and wait for the other battery series to >> get >> pulled. When it gets pulled I'll request a device tree property so we >> can add POWER_SUPPLY_PROP_NUMBER_OF_SERIAL_CELLS to the simple-battery >> and then parse this value. Or if that series ends up getting abandoned >> I can just add a quick and dirty new property like this. >> > >> > > dev_dbg(cw_bat->dev, "Read voltage: %d mV, raw=0x%04x\n", >> > > voltage_mv, reg_val); >> > > return voltage_mv; >> > > @@ -587,6 +592,8 @@ static int cw2015_parse_properties(struct cw_battery *cw_bat) >> > > return ret; >> > > } >> > > >> > > + cw_bat->dual_cell = device_property_read_bool(dev, "cellwise,dual-cell"); >> > > + >> > > ret = device_property_read_u32(dev, "cellwise,monitor-interval-ms", >> > > &cw_bat->poll_interval_ms); >> > > if (ret) { >> > > -- >> > > 2.34.1
Hi, On Fri, Aug 02, 2024 at 11:39:24PM GMT, Dragan Simic wrote: > On 2024-07-31 19:02, Sebastian Reichel wrote: > > On Wed, Jul 31, 2024 at 11:02:11AM GMT, Chris Morgan wrote: > > > On Fri, Jul 26, 2024 at 11:06:21PM +0200, Sebastian Reichel wrote: > > > > On Fri, Jul 26, 2024 at 02:49:45PM GMT, Chris Morgan wrote: > > > > > From: Chris Morgan <macromorgan@hotmail.com> > > > > > > > > > > The Cellwise cw2015 datasheet reports that it can handle two cells > > > > > in a series configuration. Allow a device tree parameter to note > > > > > this condition so that the driver reports the correct voltage values > > > > > to userspace. > > > > > > > > I found this: > > > > > > > > http://www.cellwise-semi.com/Public/assests/menu/20230302/6400076806706.pdf > > > > > > > > Which says: > > > > > > > > CW2015 can be used in 2 or more batteries connected in series, or > > > > several cells connected in parallel. > > > > > > > > So dual-cell seems like a bad property name. Instead the number of > > > > serial cells should be provided. This property is then not really > > > > specific to the Cellwise fuel gauge and instead a property of the > > > > battery pack (i.e. simple-battery.yaml). > > > > > > It's conflicting information (which further confuses me). I see in > > > that > > > datasheet it says 2 or more, whereas the datasheet found here says > > > only > > > 2 cells: > > > > > > https://www.lestat.st/_media/informatique/projets/python-cw2015/cw2015-power-management-datasheet.pdf > > > > > > But I agree in principle that we should be setting this as a property > > > of a simple-battery rather than a manufacturer specific parameter. > > > > > > > > > > > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com> > > > > > --- > > > > > drivers/power/supply/cw2015_battery.c | 7 +++++++ > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > diff --git a/drivers/power/supply/cw2015_battery.c b/drivers/power/supply/cw2015_battery.c > > > > > index f63c3c410451..b23a6d4fa4fa 100644 > > > > > --- a/drivers/power/supply/cw2015_battery.c > > > > > +++ b/drivers/power/supply/cw2015_battery.c > > > > > @@ -77,6 +77,8 @@ struct cw_battery { > > > > > u32 poll_interval_ms; > > > > > u8 alert_level; > > > > > > > > > > + bool dual_cell; > > > > > + > > > > > unsigned int read_errors; > > > > > unsigned int charge_stuck_cnt; > > > > > }; > > > > > @@ -325,6 +327,9 @@ static int cw_get_voltage(struct cw_battery *cw_bat) > > > > > */ > > > > > voltage_mv = avg * 312 / 1024; > > > > > > > > > > + if (cw_bat->dual_cell) > > > > > + voltage_mv *= 2; > > > > > > > > Unfortunately there are no details in the document, but this looks > > > > very fishy. Does it only measure the first cell and hope that the > > > > other cells have the same voltage? > > > > > > > > This (unmerged) series also applies to your problem to some degree: > > > > > > > > https://lore.kernel.org/all/20240416121818.543896-3-mike.looijmans@topic.nl/ > > > > > > I think based on the application diagram it is in fact measuring the > > > cell voltage. > > > > > > That said, this ultimately boils down to a cosmetic thing > > > as not having this property simply means userspace sees the battery > > > voltage as 3.8v instead of 7.6v as is written on the side. > > > > With the cells being connected in serial, the voltage of both cells > > can be different. There is not "the cell voltage". Instead there is > > a voltage for cell 1 and a voltage for cell 2. In a perfect battery > > they are the same, but in reality they are not. In the extreme case > > one of the cells may be broken while the other is still fine. It > > sounds as if this is just measuring the voltage from the first > > cell and assumes the second cell has the same voltage. > > > > Ideally the voltage on these platforms is not exposed via the normal > > VOLTAGE property and instead uses a new property for telling > > userspace the voltage for a single cell. The kernel simply does not > > know the voltage of the whole battery pack. > > > > FWIW this is the worst battery measurement system I've seen so far > > if my understanding of the hardware design is correct. > > Please note that two facts should be considered here: > > - The GenBook schematic [1] clearly shows that the individual battery > cells aren't exposed at its internal battery connector and, as a > result, aren't available for individual cell voltage monitoring > > - The GenBook uses a CW2013 as it fuel gauge, [1] instead of CW2015 > as mentioned here a few times, but I haven't went through the CW2013 > datasheet(s) yet to see what are the actual differences between it > and the CW2015 > > [1] https://wiki.cool-pi.com/notebook/coolpi-genbook-v20.pdf Ah, thanks for pointing to the schematics. So the fuel gauge can only measure the voltage of the whole battery, but VCELL register provides the voltage for 1 cell? What is the origin of the dual-cell property? Was this something you came up with yourself when noticing the wrong voltage? Based on the above information my guess would be that CW2013 uses a different voltage resolution than CW2015 for the VCELL register. The datasheet for CW2015 says 14bit ADC with 305uV resolution. Maybe the CW2013 simply uses a different ADC. Do you have the datasheet for the chip? -- Sebastian
Hello Sebastian, (I'm adding a couple of Cool Pi contacts to the Cc list.) On 2024-08-04 18:29, Sebastian Reichel wrote: > On Fri, Aug 02, 2024 at 11:39:24PM GMT, Dragan Simic wrote: >> On 2024-07-31 19:02, Sebastian Reichel wrote: >> > On Wed, Jul 31, 2024 at 11:02:11AM GMT, Chris Morgan wrote: >> > > On Fri, Jul 26, 2024 at 11:06:21PM +0200, Sebastian Reichel wrote: >> > > > On Fri, Jul 26, 2024 at 02:49:45PM GMT, Chris Morgan wrote: >> > > > > From: Chris Morgan <macromorgan@hotmail.com> >> > > > > >> > > > > The Cellwise cw2015 datasheet reports that it can handle two cells >> > > > > in a series configuration. Allow a device tree parameter to note >> > > > > this condition so that the driver reports the correct voltage values >> > > > > to userspace. >> > > > >> > > > I found this: >> > > > >> > > > http://www.cellwise-semi.com/Public/assests/menu/20230302/6400076806706.pdf >> > > > >> > > > Which says: >> > > > >> > > > CW2015 can be used in 2 or more batteries connected in series, or >> > > > several cells connected in parallel. >> > > > >> > > > So dual-cell seems like a bad property name. Instead the number of >> > > > serial cells should be provided. This property is then not really >> > > > specific to the Cellwise fuel gauge and instead a property of the >> > > > battery pack (i.e. simple-battery.yaml). >> > > >> > > It's conflicting information (which further confuses me). I see in >> > > that >> > > datasheet it says 2 or more, whereas the datasheet found here says >> > > only >> > > 2 cells: >> > > >> > > https://www.lestat.st/_media/informatique/projets/python-cw2015/cw2015-power-management-datasheet.pdf >> > > >> > > But I agree in principle that we should be setting this as a property >> > > of a simple-battery rather than a manufacturer specific parameter. >> > > >> > > > >> > > > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com> >> > > > > --- >> > > > > drivers/power/supply/cw2015_battery.c | 7 +++++++ >> > > > > 1 file changed, 7 insertions(+) >> > > > > >> > > > > diff --git a/drivers/power/supply/cw2015_battery.c b/drivers/power/supply/cw2015_battery.c >> > > > > index f63c3c410451..b23a6d4fa4fa 100644 >> > > > > --- a/drivers/power/supply/cw2015_battery.c >> > > > > +++ b/drivers/power/supply/cw2015_battery.c >> > > > > @@ -77,6 +77,8 @@ struct cw_battery { >> > > > > u32 poll_interval_ms; >> > > > > u8 alert_level; >> > > > > >> > > > > + bool dual_cell; >> > > > > + >> > > > > unsigned int read_errors; >> > > > > unsigned int charge_stuck_cnt; >> > > > > }; >> > > > > @@ -325,6 +327,9 @@ static int cw_get_voltage(struct cw_battery *cw_bat) >> > > > > */ >> > > > > voltage_mv = avg * 312 / 1024; >> > > > > >> > > > > + if (cw_bat->dual_cell) >> > > > > + voltage_mv *= 2; >> > > > >> > > > Unfortunately there are no details in the document, but this looks >> > > > very fishy. Does it only measure the first cell and hope that the >> > > > other cells have the same voltage? >> > > > >> > > > This (unmerged) series also applies to your problem to some degree: >> > > > >> > > > https://lore.kernel.org/all/20240416121818.543896-3-mike.looijmans@topic.nl/ >> > > >> > > I think based on the application diagram it is in fact measuring the >> > > cell voltage. >> > > >> > > That said, this ultimately boils down to a cosmetic thing >> > > as not having this property simply means userspace sees the battery >> > > voltage as 3.8v instead of 7.6v as is written on the side. >> > >> > With the cells being connected in serial, the voltage of both cells >> > can be different. There is not "the cell voltage". Instead there is >> > a voltage for cell 1 and a voltage for cell 2. In a perfect battery >> > they are the same, but in reality they are not. In the extreme case >> > one of the cells may be broken while the other is still fine. It >> > sounds as if this is just measuring the voltage from the first >> > cell and assumes the second cell has the same voltage. >> > >> > Ideally the voltage on these platforms is not exposed via the normal >> > VOLTAGE property and instead uses a new property for telling >> > userspace the voltage for a single cell. The kernel simply does not >> > know the voltage of the whole battery pack. >> > >> > FWIW this is the worst battery measurement system I've seen so far >> > if my understanding of the hardware design is correct. >> >> Please note that two facts should be considered here: >> >> - The GenBook schematic [1] clearly shows that the individual battery >> cells aren't exposed at its internal battery connector and, as a >> result, aren't available for individual cell voltage monitoring >> >> - The GenBook uses a CW2013 as it fuel gauge, [1] instead of CW2015 >> as mentioned here a few times, but I haven't went through the >> CW2013 >> datasheet(s) yet to see what are the actual differences between it >> and the CW2015 >> >> [1] https://wiki.cool-pi.com/notebook/coolpi-genbook-v20.pdf > > Ah, thanks for pointing to the schematics. So the fuel gauge can > only measure the voltage of the whole battery, but VCELL register > provides the voltage for 1 cell? What is the origin of the dual-cell > property? Was this something you came up with yourself when noticing > the wrong voltage? I'm not sure where does the idea for the dual-cell property come from, but please note that it wasn't me who invented it, so to speak. :) I don't even have the required hardware to test and develop these patches on, i.e. I don't have a CM5-based GenBook. > Based on the above information my guess would be that CW2013 uses a > different voltage resolution than CW2015 for the VCELL register. The > datasheet for CW2015 says 14bit ADC with 305uV resolution. Maybe > the CW2013 simply uses a different ADC. Do you have the datasheet > for the chip? I've managed to find a few (seemingly a bit different) CW2013 datasheets, but as usual with CellWise, some of them may contain a bit confusing or incomplete information. I'd suggest that you wait for a couple of days until I sift through the obtained datasheets, to save you from doing the same. :) Of course, I'll then send over the datasheet that seems correct and the most complete to me. I'll see to add support for CW2013 to the cw2015 driver, for which I already have a couple of pending cleanup patches.
From: Chris Morgan <macromorgan@hotmail.com> Add support for the GameForce Ace. The GameForce Ace is an RK3588s based gaming device with a 1080p display, touchscreen, hall effect joysticks and triggers, 128GB of eMMC, 8GB or 12GB of RAM, WiFi 5, and support for a 2242 NVME. Chris Morgan (5): dt-bindings: power: supply: add dual-cell for cw2015 power: supply: cw2015: Add support for dual-cell configurations arm64: dts: rockchip: Pull up sdio pins on RK3588 dt-bindings: arm: rockchip: Add GameForce Ace arm64: dts: rockchip: Add GameForce Ace .../devicetree/bindings/arm/rockchip.yaml | 5 + .../bindings/power/supply/cw2015_battery.yaml | 6 + arch/arm64/boot/dts/rockchip/Makefile | 1 + .../dts/rockchip/rk3588-base-pinctrl.dtsi | 10 +- .../dts/rockchip/rk3588s-gameforce-ace.dts | 1315 +++++++++++++++++ drivers/power/supply/cw2015_battery.c | 7 + 6 files changed, 1339 insertions(+), 5 deletions(-) create mode 100644 arch/arm64/boot/dts/rockchip/rk3588s-gameforce-ace.dts