Hi Geert,
Thanks for your review!
On 21 July 2022 13:10 Geert Uytterhoeven wrote:
> On Mon, Jul 18, 2022 at 3:45 PM Phil Edworthy wrote:
> > RZ/V2M (r9a09g011) has a few differences:
> > - The USB3_DRD_CON register has moved, its called USB_PERI_DRD_CON in
> > the RZ/V2M hardware manual.
> > It has additional bits for host and peripheral reset that need to
> > cleared to use usb host and peripheral respectively.
> > - The USB3_OTG_STA, USB3_OTG_INT_STA and USB3_OTG_INT_ENA registers
> > have been moved and renamed to USB_PERI_DRD_STA, USB_PERI_DRD_INT_STA
> > and USB_PERI_DRD_INT_E.
> > - The IDMON bit used in the above regs for role detection have moved
> > from bit 4 to bit 0.
> > - RZ/V2M has an separate interrupt for DRD, i.e. for changes to IDMON.
> > - There are reset lines for DRD and USBP
> > - There is another clock, managed by runtime PM.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@xxxxxxxxxxx>
> > Reviewed-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>
> Thanks for your patch!
>
> > --- a/drivers/usb/gadget/udc/renesas_usb3.c
> > +++ b/drivers/usb/gadget/udc/renesas_usb3.c
>
> > @@ -363,6 +368,7 @@ struct renesas_usb3 {
> > bool forced_b_device;
> > bool start_to_connect;
> > bool role_sw_by_connector;
> > + bool r9a09g011;
>
> Any better name for this feature flag?
Nothing springs to mind. We could use separate flags for has_resets,
has_drd_irq, max_nr_pipes but I am struggling to come up with names
for the offset registers and moved bits. Any suggestions?
> > };
>
> > @@ -2707,6 +2733,12 @@ static const struct renesas_usb3_priv
> renesas_usb3_priv_r8a77990 = {
> > .workaround_for_vbus = true,
> > };
> >
> > +static const struct renesas_usb3_priv renesas_usb3_priv_r9a09g011 = {
>
> renesas_usb3_priv_rzv2m?
Ok
> > + .ramsize_per_ramif = SZ_32K,
> > + .num_ramif = 1,
> > + .ramsize_per_pipe = SZ_4K,
> > +};
> > +
> > static const struct of_device_id usb3_of_match[] = {
> > {
> > .compatible = "renesas,r8a774c0-usb3-peri",
> > @@ -2717,6 +2749,9 @@ static const struct of_device_id usb3_of_match[] =
> {
> > }, {
> > .compatible = "renesas,r8a77990-usb3-peri",
> > .data = &renesas_usb3_priv_r8a77990,
> > + }, {
> > + .compatible = "renesas,r9a09g011-usb3-peri",
>
> As the bindings include a family-specific compatible value, you should
> use that ("renesas,rzv2m-usb3-peri") here.
Ok
> > + .data = &renesas_usb3_priv_r9a09g011,
> > }, {
> > .compatible = "renesas,rcar-gen3-usb3-peri",
> > .data = &renesas_usb3_priv_gen3,
>
> > @@ -2758,13 +2793,22 @@ static int renesas_usb3_probe(struct
> platform_device *pdev)
> > else
> > priv = of_device_get_match_data(&pdev->dev);
> >
> > + usb3 = devm_kzalloc(&pdev->dev, sizeof(*usb3), GFP_KERNEL);
> > + if (!usb3)
> > + return -ENOMEM;
> > +
> > + if (priv == &renesas_usb3_priv_r9a09g011)
>
> Please store the feature flag in renesas_usb3_priv instead of doing an
> explicit comparison.
Ok, will do!
> > + usb3->r9a09g011 = true;
> > +
Thanks
Phil
