| Message ID | 20240606-misc-tsd-v3-5-728d721c464a@cherry.de |
|---|---|
| State | Accepted |
| Commit | 2ce40542e0ebc9b782954ae6df3a23885ff60cf1 |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | rockchip: display PMIC variant properly + misc fixes for Theobroma boards | expand |
On 2024-06-06 10:45, Quentin Schulz wrote: > From: Quentin Schulz <quentin.schulz@cherry.de> > > The ID of the PMIC is stored in the 2 16b registers but the only part > that matters right now is the 3 MSB, which make the 3 digits (in hex) > of > the part number. > > Right now, only RK808 was properly displayed, with this all currently > supported PMICs should display the proper part number. > > Additionally, when the PMIC variant is not found, print that value > instead of the masked unshifted value as all PMICs we support for now > have their LSB ignored to represent the actual part number. > > Tested on RK806 (RK3588 Jaguar), RK808 (RK3399 Puma) and RK809 (PX30 > Ringneck). > > Reviewed-by: Kever Yang <kever.yang@rock-chips.com> > Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de> Obviously, my Reviewed-by tag for the v2 of this patch still applies. > --- > drivers/power/pmic/rk8xx.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c > index 12ff26a0855..617bb511e4e 100644 > --- a/drivers/power/pmic/rk8xx.c > +++ b/drivers/power/pmic/rk8xx.c > @@ -6,6 +6,7 @@ > > #include <dm.h> > #include <dm/lists.h> > +#include <bitfield.h> > #include <errno.h> > #include <log.h> > #include <linux/bitfield.h> > @@ -277,10 +278,9 @@ static int rk8xx_probe(struct udevice *dev) > return ret; > > priv->variant = ((msb << 8) | lsb) & RK8XX_ID_MSK; > - show_variant = priv->variant; > + show_variant = bitfield_extract_by_mask(priv->variant, RK8XX_ID_MSK); > switch (priv->variant) { > case RK808_ID: > - show_variant = 0x808; /* RK808 hardware ID is 0 */ > break; > case RK805_ID: > case RK816_ID: > @@ -311,7 +311,7 @@ static int rk8xx_probe(struct udevice *dev) > init_data_num = ARRAY_SIZE(rk806_init_reg); > break; > default: > - printf("Unknown PMIC: RK%x!!\n", priv->variant); > + printf("Unknown PMIC: RK%x!!\n", show_variant); > return -EINVAL; > }
Hi all, On 6/6/24 10:45 AM, Quentin Schulz wrote: > From: Quentin Schulz <quentin.schulz@cherry.de> > > The ID of the PMIC is stored in the 2 16b registers but the only part > that matters right now is the 3 MSB, which make the 3 digits (in hex) of > the part number. > > Right now, only RK808 was properly displayed, with this all currently > supported PMICs should display the proper part number. > > Additionally, when the PMIC variant is not found, print that value > instead of the masked unshifted value as all PMICs we support for now > have their LSB ignored to represent the actual part number. > > Tested on RK806 (RK3588 Jaguar), RK808 (RK3399 Puma) and RK809 (PX30 > Ringneck). > > Reviewed-by: Kever Yang <kever.yang@rock-chips.com> > Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de> > --- > drivers/power/pmic/rk8xx.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c > index 12ff26a0855..617bb511e4e 100644 > --- a/drivers/power/pmic/rk8xx.c > +++ b/drivers/power/pmic/rk8xx.c > @@ -6,6 +6,7 @@ > > #include <dm.h> > #include <dm/lists.h> > +#include <bitfield.h> > #include <errno.h> > #include <log.h> > #include <linux/bitfield.h> > @@ -277,10 +278,9 @@ static int rk8xx_probe(struct udevice *dev) > return ret; > > priv->variant = ((msb << 8) | lsb) & RK8XX_ID_MSK; > - show_variant = priv->variant; > + show_variant = bitfield_extract_by_mask(priv->variant, RK8XX_ID_MSK); > switch (priv->variant) { > case RK808_ID: > - show_variant = 0x808; /* RK808 hardware ID is 0 */ This line removal is actually incorrect, I should have left this in as we cannot use the same logic as other PMICs for RK808 as it returns 0, so 0 masked/shifted is still zero. I saw that Kever has already sent a merge request for next with this patch, so we have two options: reject the merge and I send another patch for next branch, or Kever cancels the merge request, I send a v4 for this patch and Kever sends a new merge request? How do we want to proceed with this. I have a feeling the additional patch is going to be easier for everyone as 1) it's for next branch, 2) isn't breaking anything except some info message, which was already wrong (but for all non-RK808 PMICs :) ). Cheers, Quentin
Hello Quentin, On 2024-06-17 16:10, Quentin Schulz wrote: > On 6/6/24 10:45 AM, Quentin Schulz wrote: >> From: Quentin Schulz <quentin.schulz@cherry.de> >> >> The ID of the PMIC is stored in the 2 16b registers but the only part >> that matters right now is the 3 MSB, which make the 3 digits (in hex) >> of >> the part number. >> >> Right now, only RK808 was properly displayed, with this all currently >> supported PMICs should display the proper part number. >> >> Additionally, when the PMIC variant is not found, print that value >> instead of the masked unshifted value as all PMICs we support for now >> have their LSB ignored to represent the actual part number. >> >> Tested on RK806 (RK3588 Jaguar), RK808 (RK3399 Puma) and RK809 (PX30 >> Ringneck). >> >> Reviewed-by: Kever Yang <kever.yang@rock-chips.com> >> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de> >> --- >> drivers/power/pmic/rk8xx.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c >> index 12ff26a0855..617bb511e4e 100644 >> --- a/drivers/power/pmic/rk8xx.c >> +++ b/drivers/power/pmic/rk8xx.c >> @@ -6,6 +6,7 @@ >> #include <dm.h> >> #include <dm/lists.h> >> +#include <bitfield.h> >> #include <errno.h> >> #include <log.h> >> #include <linux/bitfield.h> >> @@ -277,10 +278,9 @@ static int rk8xx_probe(struct udevice *dev) >> return ret; >> priv->variant = ((msb << 8) | lsb) & RK8XX_ID_MSK; >> - show_variant = priv->variant; >> + show_variant = bitfield_extract_by_mask(priv->variant, >> RK8XX_ID_MSK); >> switch (priv->variant) { >> case RK808_ID: >> - show_variant = 0x808; /* RK808 hardware ID is 0 */ > > This line removal is actually incorrect, I should have left this in as > we cannot use the same logic as other PMICs for RK808 as it returns 0, > so 0 masked/shifted is still zero. Thanks for catching this! Moreover, I think we should skip reading the msb and lsb values entirely for the RK808, because its datasheet lists the default ID_MSB (0x17) and ID_LSB (0x18) registers as reserved, and provides no information about gathering the chip variant. > I saw that Kever has already sent a merge request for next with this > patch, so we have two options: reject the merge and I send another > patch for next branch, or Kever cancels the merge request, I send a v4 > for this patch and Kever sends a new merge request? How do we want to > proceed with this. I have a feeling the additional patch is going to > be easier for everyone as 1) it's for next branch, 2) isn't breaking > anything except some info message, which was already wrong (but for > all non-RK808 PMICs :) ). I think the latter is the way to go, because the pull request has been already merged.
Hi Dragan, On 6/17/24 4:58 PM, Dragan Simic wrote: > Hello Quentin, > > On 2024-06-17 16:10, Quentin Schulz wrote: >> On 6/6/24 10:45 AM, Quentin Schulz wrote: >>> From: Quentin Schulz <quentin.schulz@cherry.de> >>> >>> The ID of the PMIC is stored in the 2 16b registers but the only part >>> that matters right now is the 3 MSB, which make the 3 digits (in hex) of >>> the part number. >>> >>> Right now, only RK808 was properly displayed, with this all currently >>> supported PMICs should display the proper part number. >>> >>> Additionally, when the PMIC variant is not found, print that value >>> instead of the masked unshifted value as all PMICs we support for now >>> have their LSB ignored to represent the actual part number. >>> >>> Tested on RK806 (RK3588 Jaguar), RK808 (RK3399 Puma) and RK809 (PX30 >>> Ringneck). >>> >>> Reviewed-by: Kever Yang <kever.yang@rock-chips.com> >>> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de> >>> --- >>> drivers/power/pmic/rk8xx.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c >>> index 12ff26a0855..617bb511e4e 100644 >>> --- a/drivers/power/pmic/rk8xx.c >>> +++ b/drivers/power/pmic/rk8xx.c >>> @@ -6,6 +6,7 @@ >>> #include <dm.h> >>> #include <dm/lists.h> >>> +#include <bitfield.h> >>> #include <errno.h> >>> #include <log.h> >>> #include <linux/bitfield.h> >>> @@ -277,10 +278,9 @@ static int rk8xx_probe(struct udevice *dev) >>> return ret; >>> priv->variant = ((msb << 8) | lsb) & RK8XX_ID_MSK; >>> - show_variant = priv->variant; >>> + show_variant = bitfield_extract_by_mask(priv->variant, >>> RK8XX_ID_MSK); >>> switch (priv->variant) { >>> case RK808_ID: >>> - show_variant = 0x808; /* RK808 hardware ID is 0 */ >> >> This line removal is actually incorrect, I should have left this in as >> we cannot use the same logic as other PMICs for RK808 as it returns 0, >> so 0 masked/shifted is still zero. > > Thanks for catching this! Moreover, I think we should skip reading > the msb and lsb values entirely for the RK808, because its datasheet > lists the default ID_MSB (0x17) and ID_LSB (0x18) registers as reserved, > and provides no information about gathering the chip variant. > We've been reading those registers on RK808 in our production lines for probably what's half a decade now, I think it's probably safe to use :) Cheers, Quentin
On 2024-06-17 18:54, Quentin Schulz wrote: > On 6/17/24 4:58 PM, Dragan Simic wrote: >> On 2024-06-17 16:10, Quentin Schulz wrote: >>> On 6/6/24 10:45 AM, Quentin Schulz wrote: >>>> From: Quentin Schulz <quentin.schulz@cherry.de> >>>> >>>> The ID of the PMIC is stored in the 2 16b registers but the only >>>> part >>>> that matters right now is the 3 MSB, which make the 3 digits (in >>>> hex) of >>>> the part number. >>>> >>>> Right now, only RK808 was properly displayed, with this all >>>> currently >>>> supported PMICs should display the proper part number. >>>> >>>> Additionally, when the PMIC variant is not found, print that value >>>> instead of the masked unshifted value as all PMICs we support for >>>> now >>>> have their LSB ignored to represent the actual part number. >>>> >>>> Tested on RK806 (RK3588 Jaguar), RK808 (RK3399 Puma) and RK809 (PX30 >>>> Ringneck). >>>> >>>> Reviewed-by: Kever Yang <kever.yang@rock-chips.com> >>>> Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de> >>>> --- >>>> drivers/power/pmic/rk8xx.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c >>>> index 12ff26a0855..617bb511e4e 100644 >>>> --- a/drivers/power/pmic/rk8xx.c >>>> +++ b/drivers/power/pmic/rk8xx.c >>>> @@ -6,6 +6,7 @@ >>>> #include <dm.h> >>>> #include <dm/lists.h> >>>> +#include <bitfield.h> >>>> #include <errno.h> >>>> #include <log.h> >>>> #include <linux/bitfield.h> >>>> @@ -277,10 +278,9 @@ static int rk8xx_probe(struct udevice *dev) >>>> return ret; >>>> priv->variant = ((msb << 8) | lsb) & RK8XX_ID_MSK; >>>> - show_variant = priv->variant; >>>> + show_variant = bitfield_extract_by_mask(priv->variant, >>>> RK8XX_ID_MSK); >>>> switch (priv->variant) { >>>> case RK808_ID: >>>> - show_variant = 0x808; /* RK808 hardware ID is 0 */ >>> >>> This line removal is actually incorrect, I should have left this in >>> as >>> we cannot use the same logic as other PMICs for RK808 as it returns >>> 0, >>> so 0 masked/shifted is still zero. >> >> Thanks for catching this! Moreover, I think we should skip reading >> the msb and lsb values entirely for the RK808, because its datasheet >> lists the default ID_MSB (0x17) and ID_LSB (0x18) registers as >> reserved, >> and provides no information about gathering the chip variant. > > We've been reading those registers on RK808 in our production lines > for probably what's half a decade now, I think it's probably safe to > use :) True, it's obviously safe, but not reading those reserved RK808 registers would be more about accuracy and following the datasheets as precisely as possible. :)
diff --git a/drivers/power/pmic/rk8xx.c b/drivers/power/pmic/rk8xx.c index 12ff26a0855..617bb511e4e 100644 --- a/drivers/power/pmic/rk8xx.c +++ b/drivers/power/pmic/rk8xx.c @@ -6,6 +6,7 @@ #include <dm.h> #include <dm/lists.h> +#include <bitfield.h> #include <errno.h> #include <log.h> #include <linux/bitfield.h> @@ -277,10 +278,9 @@ static int rk8xx_probe(struct udevice *dev) return ret; priv->variant = ((msb << 8) | lsb) & RK8XX_ID_MSK; - show_variant = priv->variant; + show_variant = bitfield_extract_by_mask(priv->variant, RK8XX_ID_MSK); switch (priv->variant) { case RK808_ID: - show_variant = 0x808; /* RK808 hardware ID is 0 */ break; case RK805_ID: case RK816_ID: @@ -311,7 +311,7 @@ static int rk8xx_probe(struct udevice *dev) init_data_num = ARRAY_SIZE(rk806_init_reg); break; default: - printf("Unknown PMIC: RK%x!!\n", priv->variant); + printf("Unknown PMIC: RK%x!!\n", show_variant); return -EINVAL; }